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@xxxxxxxxxxx> > >>> --- > >> > >> [...] > >> > >>> + 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. > > 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. > > -- With best wishes Dmitry
