Re: [PATCH V8 3/7] interconnect: qcom: Add multidev EPSS L3 support

[Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>]

On 2/17/2025 6:38 AM, Dmitry Baryshkov wrote:
> On Sun, Feb 16, 2025 at 09:58:41PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 2/10/2025 4:27 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 05, 2025 at 06:27:39PM +0000, Raviteja Laggyshetty wrote:
>>>> EPSS on SA8775P has two instances, necessitating the creation of two
>>>> device nodes with different compatibles due to the unique ICC node ID
>>>> and name limitations in the interconnect framework. Add multidevice
>>>> support for the OSM-L3 provider to dynamically obtain unique node IDs
>>>> and register with the framework.
>>>>
>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>>> ---
>>>>  drivers/interconnect/qcom/osm-l3.c | 46 +++++++++++++++++-------------
>>>>  1 file changed, 26 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
>>>> index 6a656ed44d49..da2d82700b5a 100644
>>>> --- a/drivers/interconnect/qcom/osm-l3.c
>>>> +++ b/drivers/interconnect/qcom/osm-l3.c
>>>> @@ -1,6 +1,7 @@
>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>  /*
>>>>   * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>   */
>>>>  
>>>>  #include <linux/args.h>
>>>> @@ -33,6 +34,7 @@
>>>>  #define EPSS_REG_PERF_STATE		0x320
>>>>  
>>>>  #define OSM_L3_MAX_LINKS		1
>>>> +#define ALLOC_DYN_ID			-1
>>>
>>> This should be defined by ICC framework.
>>
>> ok, I will move this to framework.
>>>
>>>>  
>>>>  #define to_osm_l3_provider(_provider) \
>>>>  	container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
>>>> @@ -55,46 +57,40 @@ struct qcom_osm_l3_icc_provider {
>>>>   */
>>>>  struct qcom_osm_l3_node {
>>>>  	const char *name;
>>>> -	u16 links[OSM_L3_MAX_LINKS];
>>>> -	u16 id;
>>>> +	struct qcom_osm_l3_node *links[OSM_L3_MAX_LINKS];
>>>> +	int id;
>>>>  	u16 num_links;
>>>>  	u16 buswidth;
>>>>  };
>>>>  
>>>>  struct qcom_osm_l3_desc {
>>>> -	const struct qcom_osm_l3_node * const *nodes;
>>>> +	struct qcom_osm_l3_node * const *nodes;
>>>>  	size_t num_nodes;
>>>>  	unsigned int lut_row_size;
>>>>  	unsigned int reg_freq_lut;
>>>>  	unsigned int reg_perf_state;
>>>>  };
>>>>  
>>>> -enum {
>>>> -	OSM_L3_MASTER_NODE = 10000,
>>>> -	OSM_L3_SLAVE_NODE,
>>>> -};
>>>> -
>>>> -#define DEFINE_QNODE(_name, _id, _buswidth, ...)			\
>>>> -	static const struct qcom_osm_l3_node _name = {			\
>>>> +#define DEFINE_QNODE(_name, _buswidth, ...)			\
>>>> +	static struct qcom_osm_l3_node _name = {			\
> 
> No. Global data _must_ remain const.

Ok, will make the global struct const.

> 
>>>>  		.name = #_name,						\
>>>> -		.id = _id,						\
>>>>  		.buswidth = _buswidth,					\
>>>>  		.num_links = COUNT_ARGS(__VA_ARGS__),			\
>>>>  		.links = { __VA_ARGS__ },				\
>>>>  	}
>>>>  
>>>> -DEFINE_QNODE(osm_l3_master, OSM_L3_MASTER_NODE, 16, OSM_L3_SLAVE_NODE);
>>>> -DEFINE_QNODE(osm_l3_slave, OSM_L3_SLAVE_NODE, 16);
>>>> +DEFINE_QNODE(osm_l3_slave, 16);
>>>> +DEFINE_QNODE(osm_l3_master, 16, &osm_l3_slave);
>>>>  
>>>> -static const struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>>>> +static struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>>>>  	[MASTER_OSM_L3_APPS] = &osm_l3_master,
>>>>  	[SLAVE_OSM_L3] = &osm_l3_slave,
>>>>  };
>>>>  
>>>> -DEFINE_QNODE(epss_l3_master, OSM_L3_MASTER_NODE, 32, OSM_L3_SLAVE_NODE);
>>>> -DEFINE_QNODE(epss_l3_slave, OSM_L3_SLAVE_NODE, 32);
>>>> +DEFINE_QNODE(epss_l3_slave, 32);
>>>> +DEFINE_QNODE(epss_l3_master, 32, &epss_l3_slave);
>>>>  
>>>> -static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>> +static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>>  	[MASTER_EPSS_L3_APPS] = &epss_l3_master,
>>>>  	[SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>>>>  };
>>>> @@ -164,7 +160,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  	const struct qcom_osm_l3_desc *desc;
>>>>  	struct icc_onecell_data *data;
>>>>  	struct icc_provider *provider;
>>>> -	const struct qcom_osm_l3_node * const *qnodes;
>>>> +	struct qcom_osm_l3_node * const *qnodes;
>>>>  	struct icc_node *node;
>>>>  	size_t num_nodes;
>>>>  	struct clk *clk;
>>>> @@ -242,6 +238,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  
>>>>  	icc_provider_init(provider);
>>>>  
>>>> +	/*Initialize IDs to ALLOC_DYN_ID to indicate dynamic id allocation*/
>>>> +	for (i = 0; i < num_nodes; i++)
>>>> +		qnodes[i]->id = ALLOC_DYN_ID;
>>>
>>> This can be initialized statically.
>>
>> There are two instances of EPSS L3 and the target specific compatible
>> data is global which requires resetting the IDs for the second instance
>> probe. If we don't the reset the IDs back to ALLOC_DYN_ID, then ICC
>> framework assumes that ID has been already allocated and doesn't create
>> the new ICC nodes for the second instance.
> 
> Well, don't use global data for shared purposes. Consider both your
> instances probing at the same time. So, please drop the
> qcom_osm_l3_node.id, pass ALLOC_DYN_ID directly to the
> icc_node_create(), store returned nodes in a local array and pass node
> pointers to icc_link_create().
> 

Will pass ALLOC_DYN_ID as argument to create node instead of
qcom_osm_l3_node.id and avoid its usage.
Instead of creating the local array to store the pointers, will make use
of icc_onecell_data which stores all the nodes present in the provider.
>>
>>>
>>>> +
>>>>  	for (i = 0; i < num_nodes; i++) {
>>>>  		size_t j;
>>>>  
>>>> @@ -250,14 +250,19 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  			ret = PTR_ERR(node);
>>>>  			goto err;
>>>>  		}
>>>> +		qnodes[i]->id = node->id;
>>>
>>> Should not be necessary.
>> This is required, each qnode corresponds to a ICC node in framework and
>> some nodes get created in icc_node_create() API and some in
>> icc_link_create() API, to have a track of node creation qnode->id is
>> used, hence initializing qnode->id with id allocated during icc node
>> creation and avoid creation of duplicate nodes.
> 
> Basically, no. You cannot do that. Create nodes first, create links
> afterwards.

Sure, Will create nodes first and then create the links.
> 
>>>
>>>>  
>>>>  		node->name = qnodes[i]->name;
>>>>  		/* Cast away const and add it back in qcom_osm_l3_set() */
>>>>  		node->data = (void *)qnodes[i];
>>>>  		icc_node_add(node, provider);
>>>>  
>>>> -		for (j = 0; j < qnodes[i]->num_links; j++)
>>>> -			icc_link_create(node, qnodes[i]->links[j]);
>>>> +		for (j = 0; j < qnodes[i]->num_links; j++) {
>>>> +			struct qcom_osm_l3_node *link_node = qnodes[i]->links[j];
>>>> +
>>>> +			icc_link_create(node, link_node->id);
>>>
>>> Please add icc_link_nodes() (or something like that), taking two struct
>>> icc_node instances. Then you can use it here, instead of reading back
>>> the ID. Ideally the 'ID' should become an internal detail which is of no
>>> concern for the ICC drivers.
>>>
>>
>> Instead of reading back the link node id from the framework, I will call
>> icc_node_create before calling the icc_link_create() API and assign the
>> allocated id to respective qnode in the following way:
>>
>> struct qcom_osm_l3_node *qn_link_node = qnodes[i]->links[j];
>> struct icc_node *link_node = icc_node_create(qnodes[i]->links[j]->id);
>> qn_link_node->id = link_node->id;
>> icc_link_create(node, link_node->id);
>>
>> This looks cleaner than reading back the id.
> 
> As you might have guessed from the the earlier comments, no. Don't write
> _anything_ to a global data.
> 

Will not modify or update the global data.
>>
>>
>>>> +			link_node->id = (node->links[node->num_links - 1])->id;
>>>> +		}
>>>>  
>>>>  		data->nodes[i] = node;
>>>>  	}
>>>> @@ -278,6 +283,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  static const struct of_device_id osm_l3_of_match[] = {
>>>>  	{ .compatible = "qcom,epss-l3", .data = &epss_l3_l3_vote },
>>>>  	{ .compatible = "qcom,osm-l3", .data = &osm_l3 },
>>>> +	{ .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
>>>>  	{ .compatible = "qcom,sc7180-osm-l3", .data = &osm_l3 },
>>>>  	{ .compatible = "qcom,sc7280-epss-l3", .data = &epss_l3_perf_state },
>>>>  	{ .compatible = "qcom,sdm845-osm-l3", .data = &osm_l3 },
>>>> -- 
>>>> 2.39.2
>>>>
>>>
>>
>