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 <[email protected]> wrote:
On 10/9/25 13:16, Simon Glass wrote:
Hi Neil,
On Thu, 9 Oct 2025 at 02:18, Neil Armstrong <[email protected]> wrote:
Hi,
On 10/9/25 02:25, Simon Glass wrote:
Hi Neil,
On Wed, 8 Oct 2025 at 09:31, Neil Armstrong <[email protected]> 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 <[email protected]>
---
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.
Neil
Neil
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
