Hi Peter! Am 15.06.25 um 14:14 schrieb Peter Robinson:
do you have any comments? My main concern is if creating a temporary overlay is the right approach here, or if there's better way to create the /system node if needed.Apologies, I looked at this originally and my thought then was it needed more thought and then it slipped through the cracks. Off the top of my head I have a few concerns. I will have another look at this but to provide some initial feedback.
Thanks for your review!
Firstly there's no bindings upstream [1] that I can see for a root system node so it's completely RPi specific and doesn't adhere to anything. I don't really want to be adding this to the kernel DT in this mode if it's not really verifiable. I also wonder what the value is if we end up copying large amounts of the FW DT into the other DT, why not just use the firmware DT instead?
Because that way the kernel and the DT it uses (no matter if upstream or RPi downstream) can be updated together (e.g. in a FIT image), without affecting how U-Boot runs (which uses the FW DT). For the system I'm working on we have A/B boot implemented via U-Boot, so a broken update to the DT loaded by the firmware could break the whole boot (requiring manual intervention to fix), while a broken update to the kernel DT would just revert to the previous boot slot.
In the terms of the RPi kernel, again I think for that you'd be better of just passing through the FW DT as they should both match in that case, for the upstream kernel this won't make any difference. We already have the rev details in the model, we should look and see if there's a standard way way to pass HW rev/serial number through in device tree.
The /serial-number node already works just fine with U-Boot. :-)I certainly agree it'd be better to have a standard (or at least well-documented) way to pass the HW revision to userspace, but that's not something I can make happen (at least not in the RPi firmware).
In terms of userspace I think getting it that way in the example is broken, as it was in /proc/cpuinfo which was why it never made it to aarch64. but I'm not sure what details are being used there and why they matter for RGB LEDs.
To elaborate (mind I'm not saying all of this is necessarily good design, just describing the reality I'm trying to deal with):
The connection to the LEDs is that they're LED strips driven using a PWM-based protocol, and the rpi_ws281x code drives the PWM output of the RPi board via the RPi Videocore to get the required timing precision. It uses the revision to select Videocore version and memory addresses [1].
Rpi_ws281x gets the revision either directly from the device tree /system/linux,revision node or from /proc/cpuinfo, but the difference doesn't matter: The RPi downstream kernel is patched to add the revision to /proc/cpuinfo from the same DT node [2], so if the revision node is missing /proc/cpuinfo won't have the revision either. I'm not the first person to bump into the issue [3]. Upon closer inspection using the model might be sufficient for rpi_ws281x specifically. I left a comment in [3] while considering your review and alternatives to this patch, we'll see if they're open to it.
However, the Raspberry Pi documentation explicitly recommends using revision codes [4] to identify boards because the "new style" codes aren't just numbers but encode certain characteristics of the board (e.g. model, revision, memory size). So people using other tools that follow that recommendation in combination with U-Boot loading a device tree will encounter the same problem even if rpi_ws281x adopts the model-based detection.
Best regards, Fiona[1] https://github.com/jgarff/rpi_ws281x/blob/7fc0bf8b31d715bbecf28e852ede5aaa388180da/rpihw.c#L42 [2] https://github.com/raspberrypi/linux/blob/5aec9916361bc36455a4df34dfd376472adeaaf6/arch/arm64/kernel/cpuinfo.c#L266
[3] https://github.com/jgarff/rpi_ws281x/pull/495[4] https://www.raspberrypi.com/documentation/computers/raspberry-pi.html#raspberry-pi-revision-codes
Overall I am not currently convinced this is something we currently want. Peter [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindingsBest regards, Fiona Am 15.02.25 um 14:01 schrieb Peter Robinson:Hi Fiona, I'll review and test this the coming week and provide feedback. On Tue, 11 Feb 2025 at 13:58, Fiona Klute <fiona.kl...@gmx.de> wrote:Both the Raspberry Pi downstream kernel [1] and some userspace tools [2] use the revision property to identify the board, and the latter fails if it is absent. The firmware creates the /system node, so we need to do the same. [1] https://github.com/raspberrypi/linux/blob/0f292fbb6346b05766152902076895558ac23f9a/arch/arm/mach-bcm/board_bcm2835.c#L23-L33 [2] https://github.com/jgarff/rpi_ws281x/blob/7fc0bf8b31d715bbecf28e852ede5aaa388180da/rpihw.c#L579 Signed-off-by: Fiona Klute <fiona.kl...@gmx.de> Cc: Matthias Brugger <mbrug...@suse.com> Cc: Peter Robinson <pbrobin...@gmail.com> Cc: Tom Rini <tr...@konsulko.com> --- I'm sending this as RFC because I'm not sure if creating a temporary overlay is the best approach here, advice on a better way to create the /system node if needed is very much welcome. I know I'll need to add more error checks, I'll do that after I know I have the right approach. board/raspberrypi/rpi/rpi.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index aa39afa338a..647716127c7 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -550,6 +550,41 @@ int copy_property(void *dst, void *src, char *path, char *property) return fdt_setprop(dst, dst_offset, property, prop, len); } +int copy_system_node(void *fdt, void *fw_fdt) +{ + const fdt32_t *prop; + char fdto[2048]; + int src_offset; + int len; + + src_offset = fdt_path_offset(fw_fdt, "/system"); + if (src_offset < 0) + return -1; + + fdt_create(fdto, sizeof(fdto)); + fdt_finish_reservemap(fdto); + fdt_begin_node(fdto, ""); + fdt_begin_node(fdto, "fragment"); + fdt_property_string(fdto, "target-path", "/"); + fdt_begin_node(fdto, "__overlay__"); + fdt_begin_node(fdto, "system"); + + prop = fdt_getprop(fw_fdt, src_offset, "linux,serial", &len); + if (prop) + fdt_property(fdto, "linux,serial", prop, len); + prop = fdt_getprop(fw_fdt, src_offset, "linux,revision", &len); + if (prop) + fdt_property(fdto, "linux,revision", prop, len); + + fdt_end_node(fdto); + fdt_end_node(fdto); + fdt_end_node(fdto); + fdt_end_node(fdto); + fdt_finish(fdto); + + fdt_overlay_apply_verbose(fdt, fdto); +} + /* Copy tweaks from the firmware dtb to the loaded dtb */ void update_fdt_from_fw(void *fdt, void *fw_fdt) { @@ -560,6 +595,8 @@ void update_fdt_from_fw(void *fdt, void *fw_fdt) /* The firmware provides a more precise model; so copy that */ copy_property(fdt, fw_fdt, "/", "model"); + copy_system_node(fdt, fw_fdt); + /* memory reserve as suggested by the firmware */ copy_property(fdt, fw_fdt, "/", "memreserve"); -- 2.47.2
