[PATCH v2 1/7] Introduce the Interconnect Subsystem

[Neil Armstrong]

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.

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

So perhaps I'm doing something unintended here, I was
also expecting removing/unbinding a device would remove all the
childs whatever the uclass.

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