[PATCH v2 1/7] Introduce the Interconnect Subsystem
Neil Armstrong
neil.armstrong at linaro.org
Fri Oct 10 14:53:57 CEST 2025
On 10/10/25 13:31, Simon Glass wrote:
> 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?
Yeah my bad, it works as intended, I did a bad iteration where the parent
was wrong and it didn't work, I can remove everything and it works fine.
Neil
>
> Regards,
> Simon
More information about the U-Boot
mailing list