| Message ID | 20250227155213.404-5-quic_rlaggysh@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add EPSS L3 provider support on SA8775P SoC | expand |
On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote: > To facilitate dynamic node ID support, the driver now uses > node pointers for links instead of static node IDs. > Additionally, the default node ID is set to -1 to prompt > the ICC framework for dynamic node ID allocation. > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > --- > drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- > drivers/interconnect/qcom/icc-rpmh.h | 3 ++- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c > index f2d63745be54..2e654917f535 100644 > --- a/drivers/interconnect/qcom/icc-rpmh.c > +++ b/drivers/interconnect/qcom/icc-rpmh.c > @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) > ret = PTR_ERR(node); > goto err_remove_nodes; > } > + qn->id = node->id; > > node->name = qn->name; > node->data = qn; > icc_node_add(node, provider); > > - for (j = 0; j < qn->num_links; j++) > - icc_link_create(node, qn->links[j]); > + for (j = 0; j < qn->num_links; j++) { > + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; > + struct icc_node *link_node; > + > + if (qn_link_node) { > + link_node = icc_node_create(qn_link_node->id); > + qn_link_node->id = link_node->id; > + icc_link_create(node, qn_link_node->id); I really don't like the idea of reading the ->id back. I think in the last cycle I have already asked to add an API to link two nodes instead of linking a node and an ID. Is there an issue with such an API? > + } else { > + /* backward compatibility for target using static IDs */ > + icc_link_create(node, qn->links[j]); > + } > + } > > data->nodes[i] = node; > } > diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h > index 82344c734091..cf4aa69c707c 100644 > --- a/drivers/interconnect/qcom/icc-rpmh.h > +++ b/drivers/interconnect/qcom/icc-rpmh.h > @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { > struct qcom_icc_node { > const char *name; > u16 links[MAX_LINKS]; > - u16 id; > + struct qcom_icc_node *link_nodes[MAX_LINKS]; > + int id; > u16 num_links; > u16 channels; > u16 buswidth; > -- > 2.43.0 >
On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote: > To facilitate dynamic node ID support, the driver now uses > node pointers for links instead of static node IDs. > Additionally, the default node ID is set to -1 to prompt > the ICC framework for dynamic node ID allocation. > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > --- > drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- > drivers/interconnect/qcom/icc-rpmh.h | 3 ++- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c > index f2d63745be54..2e654917f535 100644 > --- a/drivers/interconnect/qcom/icc-rpmh.c > +++ b/drivers/interconnect/qcom/icc-rpmh.c > @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) > ret = PTR_ERR(node); > goto err_remove_nodes; > } > + qn->id = node->id; > > node->name = qn->name; > node->data = qn; > icc_node_add(node, provider); > > - for (j = 0; j < qn->num_links; j++) > - icc_link_create(node, qn->links[j]); > + for (j = 0; j < qn->num_links; j++) { > + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; > + struct icc_node *link_node; > + > + if (qn_link_node) { > + link_node = icc_node_create(qn_link_node->id); > + qn_link_node->id = link_node->id; > + icc_link_create(node, qn_link_node->id); > + } else { > + /* backward compatibility for target using static IDs */ > + icc_link_create(node, qn->links[j]); > + } > + } > > data->nodes[i] = node; > } > diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h > index 82344c734091..cf4aa69c707c 100644 > --- a/drivers/interconnect/qcom/icc-rpmh.h > +++ b/drivers/interconnect/qcom/icc-rpmh.h > @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { > struct qcom_icc_node { > const char *name; > u16 links[MAX_LINKS]; > - u16 id; > + struct qcom_icc_node *link_nodes[MAX_LINKS]; This is very inefficient. MAX_LINKS = 128, which means we're adding an additional 1KB *per-node*. The vast majority of nodes don't come anywhere close to this number of links, so this is almost entirely unused and wasted space. As an example: sa8775p has 193 nodes, so we're adding 193K to the driver from this alone. The current driver size is 84K, and the size after this change is 283K. Instead of embedding this array with a hardcoded size, we could point to an array that's sized for the number of links required by the node: - struct qcom_icc_node *link_nodes[MAX_LINKS]; + struct qcom_icc_node **link_nodes; Then when initializing the arrays, we could: - .link_nodes = { &qns_a1noc_snoc }, + .link_nodes = (struct qcom_icc_node *[]) { &qns_a1noc_snoc }, And for handling compatiblity with older drivers, we'd check for link_nodes != NULL instead of checking the array indices. Doing it this way would reduce the new sa8775p size from 283K to 88K. A similar argument could be made for qcom_icc_node::links, since that's also hardcoded to MAX_LINKS. But it's not quite as bad since it's an array of u16 rather than an array of pointers. Still, if we implemented similar changes for qcom_icc_node::links, then we'd save almost 256B per-node, which for sa8775p would reduce the size by roughly another 50K. If we're ultimately planning on switching all the old drivers over to link_nodes, then we could just wait and get rid of links entirely. Regardless, optimizing links doesn't have to happen in this series, but I don't want to further bloat the size from the addition of link_nodes. > + int id; > u16 num_links; > u16 channels; > u16 buswidth; > -- > 2.43.0 > >
On 3/7/2025 9:23 AM, Mike Tipton wrote: > On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote: >> To facilitate dynamic node ID support, the driver now uses >> node pointers for links instead of static node IDs. >> Additionally, the default node ID is set to -1 to prompt >> the ICC framework for dynamic node ID allocation. >> >> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> >> --- >> drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- >> drivers/interconnect/qcom/icc-rpmh.h | 3 ++- >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c >> index f2d63745be54..2e654917f535 100644 >> --- a/drivers/interconnect/qcom/icc-rpmh.c >> +++ b/drivers/interconnect/qcom/icc-rpmh.c >> @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) >> ret = PTR_ERR(node); >> goto err_remove_nodes; >> } >> + qn->id = node->id; >> >> node->name = qn->name; >> node->data = qn; >> icc_node_add(node, provider); >> >> - for (j = 0; j < qn->num_links; j++) >> - icc_link_create(node, qn->links[j]); >> + for (j = 0; j < qn->num_links; j++) { >> + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; >> + struct icc_node *link_node; >> + >> + if (qn_link_node) { >> + link_node = icc_node_create(qn_link_node->id); >> + qn_link_node->id = link_node->id; >> + icc_link_create(node, qn_link_node->id); >> + } else { >> + /* backward compatibility for target using static IDs */ >> + icc_link_create(node, qn->links[j]); >> + } >> + } >> >> data->nodes[i] = node; >> } >> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h >> index 82344c734091..cf4aa69c707c 100644 >> --- a/drivers/interconnect/qcom/icc-rpmh.h >> +++ b/drivers/interconnect/qcom/icc-rpmh.h >> @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { >> struct qcom_icc_node { >> const char *name; >> u16 links[MAX_LINKS]; >> - u16 id; >> + struct qcom_icc_node *link_nodes[MAX_LINKS]; > > This is very inefficient. MAX_LINKS = 128, which means we're adding an > additional 1KB *per-node*. The vast majority of nodes don't come > anywhere close to this number of links, so this is almost entirely > unused and wasted space. > > As an example: sa8775p has 193 nodes, so we're adding 193K to the driver > from this alone. The current driver size is 84K, and the size after this > change is 283K. > > Instead of embedding this array with a hardcoded size, we could point to > an array that's sized for the number of links required by the node: > > - struct qcom_icc_node *link_nodes[MAX_LINKS]; > + struct qcom_icc_node **link_nodes; > > Then when initializing the arrays, we could: > > - .link_nodes = { &qns_a1noc_snoc }, > + .link_nodes = (struct qcom_icc_node *[]) { &qns_a1noc_snoc }, > > And for handling compatiblity with older drivers, we'd check for > link_nodes != NULL instead of checking the array indices. > > Doing it this way would reduce the new sa8775p size from 283K to 88K. > > A similar argument could be made for qcom_icc_node::links, since that's > also hardcoded to MAX_LINKS. But it's not quite as bad since it's an > array of u16 rather than an array of pointers. Still, if we implemented > similar changes for qcom_icc_node::links, then we'd save almost 256B > per-node, which for sa8775p would reduce the size by roughly another > 50K. If we're ultimately planning on switching all the old drivers over > to link_nodes, then we could just wait and get rid of links entirely. > Regardless, optimizing links doesn't have to happen in this series, but > I don't want to further bloat the size from the addition of link_nodes. > Ok Mike, I would make use of struct qcom_icc_node **link_nodes instead of *link_nodes[MAX_LINKS] in the next patch series, we can clean up the links[MAX_LINKS] as part of another patch series. This suggestion does help in reducing size of the driver. >> + int id; >> u16 num_links; >> u16 channels; >> u16 buswidth; >> -- >> 2.43.0 >> >>
On 2/27/2025 9:46 PM, Dmitry Baryshkov wrote: > On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote: >> To facilitate dynamic node ID support, the driver now uses >> node pointers for links instead of static node IDs. >> Additionally, the default node ID is set to -1 to prompt >> the ICC framework for dynamic node ID allocation. >> >> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> >> --- >> drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- >> drivers/interconnect/qcom/icc-rpmh.h | 3 ++- >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c >> index f2d63745be54..2e654917f535 100644 >> --- a/drivers/interconnect/qcom/icc-rpmh.c >> +++ b/drivers/interconnect/qcom/icc-rpmh.c >> @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) >> ret = PTR_ERR(node); >> goto err_remove_nodes; >> } >> + qn->id = node->id; >> >> node->name = qn->name; >> node->data = qn; >> icc_node_add(node, provider); >> >> - for (j = 0; j < qn->num_links; j++) >> - icc_link_create(node, qn->links[j]); >> + for (j = 0; j < qn->num_links; j++) { >> + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; >> + struct icc_node *link_node; >> + >> + if (qn_link_node) { >> + link_node = icc_node_create(qn_link_node->id); >> + qn_link_node->id = link_node->id; >> + icc_link_create(node, qn_link_node->id); > > I really don't like the idea of reading the ->id back. I think in the > last cycle I have already asked to add an API to link two nodes instead > of linking a node and an ID. Is there an issue with such an API? Yes, the link pointer may or may not be initialized during the link creation as the link can belong to other provider which is yet to probe. So, it is not possible to pass two node pointers as arguments for linking. RPMh driver has multiple providers and during the creation of links, nodes associated with other providers are created in the icc_link_create API. When the actual provider to which the link belongs is probed, its initialization/node creation is skipped by checking the ID. To ensure proper tracking of node initialization and prevent re-initialization, it is essential to read back and store the node’s ID in qnode. > >> + } else { >> + /* backward compatibility for target using static IDs */ >> + icc_link_create(node, qn->links[j]); >> + } >> + } >> >> data->nodes[i] = node; >> } >> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h >> index 82344c734091..cf4aa69c707c 100644 >> --- a/drivers/interconnect/qcom/icc-rpmh.h >> +++ b/drivers/interconnect/qcom/icc-rpmh.h >> @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { >> struct qcom_icc_node { >> const char *name; >> u16 links[MAX_LINKS]; >> - u16 id; >> + struct qcom_icc_node *link_nodes[MAX_LINKS]; >> + int id; >> u16 num_links; >> u16 channels; >> u16 buswidth; >> -- >> 2.43.0 >> >
On Mon, Mar 10, 2025 at 07:54:15AM +0530, Raviteja Laggyshetty wrote: > > > On 2/27/2025 9:46 PM, Dmitry Baryshkov wrote: > > On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote: > >> To facilitate dynamic node ID support, the driver now uses > >> node pointers for links instead of static node IDs. > >> Additionally, the default node ID is set to -1 to prompt > >> the ICC framework for dynamic node ID allocation. > >> > >> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > >> --- > >> drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- > >> drivers/interconnect/qcom/icc-rpmh.h | 3 ++- > >> 2 files changed, 16 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c > >> index f2d63745be54..2e654917f535 100644 > >> --- a/drivers/interconnect/qcom/icc-rpmh.c > >> +++ b/drivers/interconnect/qcom/icc-rpmh.c > >> @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) > >> ret = PTR_ERR(node); > >> goto err_remove_nodes; > >> } > >> + qn->id = node->id; > >> > >> node->name = qn->name; > >> node->data = qn; > >> icc_node_add(node, provider); > >> > >> - for (j = 0; j < qn->num_links; j++) > >> - icc_link_create(node, qn->links[j]); > >> + for (j = 0; j < qn->num_links; j++) { > >> + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; > >> + struct icc_node *link_node; > >> + > >> + if (qn_link_node) { > >> + link_node = icc_node_create(qn_link_node->id); > >> + qn_link_node->id = link_node->id; > >> + icc_link_create(node, qn_link_node->id); > > > > I really don't like the idea of reading the ->id back. I think in the > > last cycle I have already asked to add an API to link two nodes instead > > of linking a node and an ID. Is there an issue with such an API? > > Yes, the link pointer may or may not be initialized during the link > creation as the link can belong to other provider which is yet to probe. > So, it is not possible to pass two node pointers as arguments for linking. Obviously, this needs to be handled. e.g. by specifying external provider + ID from dt-bindings. Yes, it requires a thought on how to solve it properly. No, in my opinion, reading the ID back is not a viable option. Ideally after converting to dynamic IDs we should be able to declare the ID to be an internal detail, which is of no concern to ICC providers (or even drop it completely). Historically we have had several subsystems which were using single-list IDs. Most of them have migrated from using those IDs. > RPMh driver has multiple providers and during the creation of links, > nodes associated with other providers are created in the icc_link_create > API. When the actual provider to which the link belongs is probed, its > initialization/node creation is skipped by checking the ID. To ensure > proper tracking of node initialization and prevent re-initialization, it > is essential to read back and store the node’s ID in qnode. > > > > > >> + } else { > >> + /* backward compatibility for target using static IDs */ > >> + icc_link_create(node, qn->links[j]); > >> + } > >> + } > >> > >> data->nodes[i] = node; > >> } > >> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h > >> index 82344c734091..cf4aa69c707c 100644 > >> --- a/drivers/interconnect/qcom/icc-rpmh.h > >> +++ b/drivers/interconnect/qcom/icc-rpmh.h > >> @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { > >> struct qcom_icc_node { > >> const char *name; > >> u16 links[MAX_LINKS]; > >> - u16 id; > >> + struct qcom_icc_node *link_nodes[MAX_LINKS]; > >> + int id; > >> u16 num_links; > >> u16 channels; > >> u16 buswidth; > >> -- > >> 2.43.0 > >> > > >
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c index f2d63745be54..2e654917f535 100644 --- a/drivers/interconnect/qcom/icc-rpmh.c +++ b/drivers/interconnect/qcom/icc-rpmh.c @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) ret = PTR_ERR(node); goto err_remove_nodes; } + qn->id = node->id; node->name = qn->name; node->data = qn; icc_node_add(node, provider); - for (j = 0; j < qn->num_links; j++) - icc_link_create(node, qn->links[j]); + for (j = 0; j < qn->num_links; j++) { + struct qcom_icc_node *qn_link_node = qn->link_nodes[j]; + struct icc_node *link_node; + + if (qn_link_node) { + link_node = icc_node_create(qn_link_node->id); + qn_link_node->id = link_node->id; + icc_link_create(node, qn_link_node->id); + } else { + /* backward compatibility for target using static IDs */ + icc_link_create(node, qn->links[j]); + } + } data->nodes[i] = node; } diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h index 82344c734091..cf4aa69c707c 100644 --- a/drivers/interconnect/qcom/icc-rpmh.h +++ b/drivers/interconnect/qcom/icc-rpmh.h @@ -95,7 +95,8 @@ struct qcom_icc_qosbox { struct qcom_icc_node { const char *name; u16 links[MAX_LINKS]; - u16 id; + struct qcom_icc_node *link_nodes[MAX_LINKS]; + int id; u16 num_links; u16 channels; u16 buswidth;
To facilitate dynamic node ID support, the driver now uses node pointers for links instead of static node IDs. Additionally, the default node ID is set to -1 to prompt the ICC framework for dynamic node ID allocation. Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> --- drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++-- drivers/interconnect/qcom/icc-rpmh.h | 3 ++- 2 files changed, 16 insertions(+), 3 deletions(-)