Re: [PATCH V9 4/7] interconnect: qcom: icc-rpmh: Add dynamic icc node id support

[Raviteja Laggyshetty]

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