[PATCH v2 1/7] Introduce the Interconnect Subsystem

[Simon Glass]

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.

>
>    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?

>
> Note I won't re-invent a new API here, the api in interconnect.h is fine and
> works well, I made sure to use all U-Boot driver model offers to avoir allocating
> random objects myself, and keep the interconnect core functions strictly
> identical to the kernel, making it:
> - well integrated in DM (on-demand probe, nodes as DM devices)
> - resulting with the same results as in Linux
> - using the same API
> - easy to sync back from Linux
> - easy to port a driver from Linux

Yes that sounds good.

>
> >
> > Also, please can you add:
> >
> > - comments to the header file, at least
> > - something in doc/
> > - a command to list things / provide information
>
> Yep, I'll add those.

OK thanks.

>
> Thanks,
> Neil
>
> >
> > Great to see the automated tests!
> >
> > Regards,
> > Simon
> >
> > [1] https://docs.u-boot.org/en/latest/develop/driver-model/design.html#removal-stage
>

Regards,
Simon