[RFC] Raspberry Pi: copy /system/linux, {serial, revision} from firmware

[Peter Robinson]

Hi Fiona,

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

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?

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.

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.

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/bindings

> Best 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.klute at 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.klute at gmx.de>
> >> Cc: Matthias Brugger <mbrugger at suse.com>
> >> Cc: Peter Robinson <pbrobinson at gmail.com>
> >> Cc: Tom Rini <trini at 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
> >>
> >>