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@xxxxxxxxxxx>
>> ---
>> 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
>>
>>
