Hi Neil, On Thu, 9 Oct 2025 at 17:48, Neil Armstrong <[email protected]> wrote: > > On 10/9/25 18:43, Neil Armstrong wrote: > > On 10/9/25 14:28, Simon Glass wrote: > >> Hi Neil, > >> > >> On Thu, 9 Oct 2025 at 05:54, Neil Armstrong <[email protected]> > >> wrote: > >>> > >>> On 10/9/25 13:16, Simon Glass wrote: > >>>> Hi Neil, > >>>> > >>>> On Thu, 9 Oct 2025 at 02:18, Neil Armstrong <[email protected]> > >>>> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> On 10/9/25 02:25, Simon Glass wrote: > >>>>>> Hi Neil, > >>>>>> > >>>>>> On Wed, 8 Oct 2025 at 09:31, Neil Armstrong > >>>>>> <[email protected]> wrote: > >>>>>>> > >>>>>>> Let's introduce the Interconnect subsystem based on the Linux > >>>>>>> framework > >>>>>>> which is used to vote for bandwidth across multiple SoC busses. > >>>>>>> > >>>>>>> Each bus endpoints are materialised as "nodes" which are linked > >>>>>>> together, > >>>>>>> and the DT will specify a pair of nodes to enable and set a bandwidth > >>>>>>> on the route between those endpoints. > >>>>>>> > >>>>>>> The hardware resources that provide those nodes and provides the way > >>>>>>> to vote for the bandwidth are called "providers". > >>>>>>> > >>>>>>> The Interconnect uclass code is heavily based on the Linux one, with > >>>>>>> some small differences: > >>>>>>> - nodes are allocated as udevices instead of Linux idr_alloc() > >>>>>>> - tag management is minimal, only normal xlate is supported > >>>>>>> - dynamic IDs is not implemented > >>>>>>> - getting nodes states at probe is not implemented > >>>>>>> - providers are probed on demand while the nodes links are traversed > >>>>>>> - nodes are populated on bind > >>>>>>> > >>>>>>> Fully tested with associated DM test suite. > >>>>>>> > >>>>>>> Signed-off-by: Neil Armstrong <[email protected]> > >>>>>>> --- > >>>>>>> drivers/Kconfig | 2 + > >>>>>>> drivers/Makefile | 1 + > >>>>>>> drivers/interconnect/Kconfig | 10 + > >>>>>>> drivers/interconnect/Makefile | 6 + > >>>>>>> drivers/interconnect/interconnect-uclass.c | 548 > >>>>>>> +++++++++++++++++++++++++++++ > >>>>>>> include/dm/uclass-id.h | 2 + > >>>>>>> include/interconnect-uclass.h | 74 ++++ > >>>>>>> include/interconnect.h | 81 +++++ > >>>>>>> 8 files changed, 724 insertions(+) > >>>>>> > >>>>>> 'interconnect' is such a long and vague word :-) > >>>>> > >>>>> I didn't chose the name => > >>>>> https://docs.kernel.org/driver-api/interconnect.html > >>>>> > >>>>> I don't see why we should take a different name in U-Boot. > >>>>> > >>>>> But I can definitely use "Generic System Interconnect" in the > >>>>> documentation > >>>>> and commit messages if it's clearer. > >>>> > >>>> Yes, we should use the same name. Docs can help. > >>>> > >>>>> > >>>>>> > >>>>>> There seems to be a piece missing here. The uclass should have > >>>>>> operations, so that it makes sense for different drivers to implement > >>>>>> the methods. I see some there, but also a lot of direct calls from the > >>>>>> test to sandbox. I see quite a few functions which lack a struct > >>>>>> udevice * - I suspect I am missing something? > >>>>>> > >>>>>> We normally have a ..._get_ops() macro in the uclass header. Also we > >>>>>> normally have stub functions which get the ops and call the function. > >>>>>> > >>>>>> I see code to remove children, but driver model does this > >>>>>> automatically[1]. Same with bind(). If there is a bug in this, can you > >>>>>> give a few details, or ideally a test case? > >>>>> > >>>>> All of this is present and well thought, I went into multiple designs: > >>>>> - interconnect uclass provides ops -> interconnect_ops, icc_node doesn't > >>>>> provide any ops since they are just internal objects > >>>>> - there's only a single direct call to the sandbox driver to get the > >>>>> setting resulting of the aggregate() & set() op calls, this is > >>>>> purely for > >>>>> validation of the calculations, the sandbox-interconnect-test only > >>>>> uses > >>>>> the public inteconnect.h API > >>>> > >>>> OK, I missed that. > >>>> > >>>>> - For the lack of udevice, it's because I did use the same interconnect > >>>>> API which > >>>>> return a "icc_path" private structure which contains a set of > >>>>> interconnect > >>>>> nodes which forms path across the different providers. > >>>> > >>>> We do a similar think with clk, although the function signature is > >>>> different in U-Boot from Linux. > >>>> > >>>> I assume the caller would be a driver? If so, passing in a struct > >>>> icc_path * might be more efficient than using malloc() each time. > >>> > >>> I'm not against that but the size of icc_path depends on it's size: > >>> > >>> +struct icc_path { > >>> + size_t num_nodes; > >>> + struct icc_req reqs[]; > >>> +}; > >>> > >>> So we would still need to alloc the reqs anyway and expose the internals > >>> with no benefits. > >> > >> Indeed. Sometimes in U-Boot we use fixed sizes to avoid realloc(), but > >> I'm not sure if it makes sense here. > >> > >>> > >>>> > >>>>> > >>>>> Setting a bandwidth on a node is meaningless, same for > >>>>> interconnect providers, > >>>>> this is why we set a bandwidth on a set of nodes, which get > >>>>> aggregated with the > >>>>> other path votes from other devices. > >>>>> > >>>>> The key here is the "nodes" which are subdevices of interconnect > >>>>> providers > >>>>> created at bind time, and gets referenced in icc_path objects. > >>>>> > >>>>> I made sure we can't remove either interconnect devices or > >>>>> icc_node devices > >>>>> if they are still referenced in an active icc_path. > >>>> > >>>> OK > >>>> > >>>>> This is the reason of the device_remove() & device_unbind() calls > >>>>> in the test > >>>>> to make sure we're not left with dangling pointers to non-existent > >>>>> icc_node > >>>>> and providers devices. > >>>> > >>>> The specific code is this: > >>>> > >>>> + device_foreach_child(pos, dev) { > >>>> + debug("%s(pos=%s)\n", __func__, pos->name); > >>>> + ret = device_remove(pos, DM_REMOVE_NORMAL); > >>>> + if (ret) > >>>> + return ret; > >>>> + } > >>>> > >>>> This is in the pre_remove() method. But if you just remove that code, > >>>> doesn't driver model remove the children anyway? > >>> > >>> No it only removes the children from the same driver: > >>> =============><===================== > >>> int device_chld_remove(struct udevice *dev, struct driver *drv, > >>> uint flags) > >>> { > >>> > >>> <snip> > >>> > >>> device_foreach_child_safe(pos, n, dev) { > >>> int ret; > >>> > >>> if (drv && (pos->driver != drv)) > >>> continue; > >>> =============><===================== > >>> Same for unbind. > >>> > >>> Added in https://lists.denx.de/pipermail/u-boot/2018-June/332597.html > >>> with the bind/unbind command. > >> > >> Ick, that is a bug. > >> > >>> > >>> So perhaps I'm doing something unintended here, I was > >>> also expecting removing/unbinding a device would remove all the > >>> childs whatever the uclass. > >> > >> Yes, it should be fixed. > > > > I guess it should be fixed and a test case added. > > > > Should I handle this ? > > Wait the code is right, but still it doesn't work properly. > > I'll investigate further to understand.
OK...I'm not sure what that patch was about, but if the command needs to work that way you could create a new function, e.g. device_remove_by_drv() which only removes things for particular drivers? Regards, Simon
