[PATCH v2 1/7] Introduce the Interconnect Subsystem
Simon Glass
sjg at chromium.org
Fri Oct 10 13:31:11 CEST 2025
Hi Neil,
On Thu, 9 Oct 2025 at 17:48, Neil Armstrong <neil.armstrong at linaro.org> 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 <neil.armstrong at linaro.org> wrote:
> >>>
> >>> On 10/9/25 13:16, Simon Glass wrote:
> >>>> Hi Neil,
> >>>>
> >>>> On Thu, 9 Oct 2025 at 02:18, Neil Armstrong <neil.armstrong at linaro.org> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On 10/9/25 02:25, Simon Glass wrote:
> >>>>>> Hi Neil,
> >>>>>>
> >>>>>> On Wed, 8 Oct 2025 at 09:31, Neil Armstrong <neil.armstrong at linaro.org> 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 <neil.armstrong at linaro.org>
> >>>>>>> ---
> >>>>>>> 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
More information about the U-Boot
mailing list