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

[Raviteja Laggyshetty]

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@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);
> 
> 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
>>
>