Re: [PATCH V5 3/4] interconnect: qcom: Add EPSS L3 support on SA8775P

[Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>]

On 11/22/2024 3:44 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>>> Add Epoch Subsystem (EPSS) L3 interconnect provider on
>>>> SA8775P SoCs with multiple device support.
>>>>
>>>
>>>
>>> ...
>>>
>>>> -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_master, 16, osm_l3_slave);
>>>> +DEFINE_QNODE(osm_l3_slave, 16);
>>>>  
>>>> -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_master, 32, epss_l3_slave);
>>>> +DEFINE_QNODE(epss_l3_slave, 32);
>>>>  
>>>> -static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>> +static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>
>>>
>>> I think dropping const makes the code worse, not better. Commit msg does
>>> not explain all these changes and I could not figure out the intention
>>> (except modifying but that's just obvious).
>>
>> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
>> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
>> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA 
>> and the node name is made dynamic (nodename@address).
>> Updating node id and node name is not possible with const.
> 
> Has this been described in the commit message? No. Have you had similar
> questions since v1? Yes. What does that tell us?

I will update the commit message in the next patch revision.
> 
> Also, while we are at it. Please fix your email client settings to wrap
> message body text on some sensible (72-75) width.

Thanks for suggesting!
> 
>>>
>>>
>>>
>>>>  	[MASTER_EPSS_L3_APPS] = &epss_l3_master,
>>>>  	[SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>>>>  };
>>>> @@ -123,6 +125,19 @@ static const struct qcom_osm_l3_desc epss_l3_l3_vote = {
>>>>  	.reg_perf_state = EPSS_REG_L3_VOTE,
>>>>  };
>>>>  
>>>> +static u16 get_node_id_by_name(const char *node_name,
>>>> +			       const struct qcom_osm_l3_desc *desc)
>>>> +{
>>>> +	struct qcom_osm_l3_node *const *nodes = desc->nodes;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < desc->num_nodes; i++) {
>>>> +		if (!strcmp(nodes[i]->name, node_name))
>>>> +			return nodes[i]->id;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int qcom_osm_l3_set(struct icc_node *src, struct icc_node *dst)
>>>>  {
>>>>  	struct qcom_osm_l3_icc_provider *qp;
>>>> @@ -164,10 +179,11 @@ 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;
>>>> +	u64 addr;
>>>>  	int ret;
>>>>  
>>>>  	clk = clk_get(&pdev->dev, "xo");
>>>> @@ -188,6 +204,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  	if (!qp)
>>>>  		return -ENOMEM;
>>>>  
>>>> +	ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>  	qp->base = devm_platform_ioremap_resource(pdev, 0);
>>>>  	if (IS_ERR(qp->base))
>>>>  		return PTR_ERR(qp->base);
>>>> @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  
>>>>  	icc_provider_init(provider);
>>>>  
>>>> +	/* Allocate unique id for qnodes */
>>>> +	for (i = 0; i < num_nodes; i++)
>>>> +		qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
>>>> +
>>>>  	for (i = 0; i < num_nodes; i++) {
>>>> -		size_t j;
>>>> +		char *node_name;
>>>> +		size_t j, len;
>>>>  
>>>>  		node = icc_node_create(qnodes[i]->id);
>>>>  		if (IS_ERR(node)) {
>>>> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>  			goto err;
>>>>  		}
>>>>  
>>>> -		node->name = qnodes[i]->name;
>>>> +		/* len = strlen(node->name) + @ + 8 (base-address) + NULL */
>>>> +		len = strlen(qnodes[i]->name) + OSM_NODE_NAME_SUFFIX_SIZE;
>>>> +		node_name = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
>>>> +		if (!node_name) {
>>>> +			ret = -ENOMEM;
>>>> +			goto err;
>>>> +		}
>>>> +
>>>> +		snprintf(node_name, len, "%s@%08llx", qnodes[i]->name, addr);
>>>> +		node->name = node_name;
>>>
>>>
>>> Why the node name becomes dynamic?
>>>
>>> Best regards,
>>> Krzysztof
>>
>