[PATCH v2 1/7] Introduce the Interconnect Subsystem

[Simon Glass]

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.

Regards,
Simon

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