Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>]

On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
> On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
>> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
>>> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
>>>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
>>>>> EPSS on SA8775P has two instances which requires creation of two device
>>>>> nodes with different compatible and device data because of unique
>>>>> icc node id and name limitation in interconnect framework.
>>>>> Add multidevice support to osm-l3 code to get unique node id from IDA
>>>>> and node name is made unique by appending node address.
>>>>>
>>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> +	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);
>>>>
>>>> As I've said in my previous emails, this is a framework-level problem.
>>>>
>>>> Up until now we've simply silently ignored the possibility of an
>>>> interconnect provider having more than one instance, as conveniently
>>>> most previous SoCs had a bunch of distinct bus masters.
>>>>
>>>> Currently, debugfs-client.c relies on the node names being unique.
>>>> Keeping them as such is also useful for having a sane sysfs/debugfs
>>>> interface. But it's not always feasible, and a hierarchical approach
>>>> (like in pmdomain) may be a better fit.
>>>>
>>>> Then, node->id is used for creating links, and we unfortunately cannot
>>>> assume that both src and dst are within the same provider.
>>>> I'm not a fan of these IDs being hardcoded, but there are some drivers
>>>> that rely on that, which itself is also a bit unfortunate..
>>>>
>>>>
>>>> If Mike (who introduced debugfs-client and is probably the main user)
>>>> doesn't object to a small ABI break (which is "fine" with a debugfs
>>>> driver that requires editing the source code to be compiled), we could
>>>> add a property within icc_provider like `bool dynamic_ids` and have an
>>>> ICC-global IDA that would take care of any conflicts.
>>>
>>> Frankly speaking, I think this just delays the inevitable. We have been
>>> there with GPIOs and with some other suppliers. In my opinion the ICC
>>> subsystem needs to be refactored in order to support linking based on
>>> the supplier (fwnode?) + offset_id, but that's a huuuge rework.
>>
>> I thought about this too, but ended up not including it in the email..
>>
>> I think this will be more difficult with ICC, as tons of circular
>> dependencies are inevitable by design and we'd essentially have to
>> either provide placeholder nodes (like it's the case today) or probe
>> only parts of a device, recursively, to make sure all links can be
>> created
> 
> Or just allow probing, but then fail path creation. It will be a
> redesign, but I think it is inevitable in the end.
> 

There are no two instances of l3 or NoC on any SoC except qcs9100 and
qcs8300. I dont expect any new SoC as well.
As second instance is needed only on qcs9100 and qcs8300, I am keeping
the patch (patchset v6) as is and limit the dynamic id addition to l3
provider only.

>>
>> Konrad
>>
>>>> Provider drivers whose consumers don't already rely on programmatical
>>>> use of hardcoded IDs *and* don't have cross-provider links could then
>>>> enable that flag and have the node IDs and names set like you did in
>>>> this patch. This also sounds very useful for icc-clk.
>>>
>