Hi, thanks for taking a look at it.
On 8/24/26 3:38 PM, Quentin Schulz wrote: > Hi Wadim, > > On 8/24/26 2:06 PM, Wadim Egorov wrote: >> Set /chosen/bootsource in the device tree passed to the OS to the full >> path of the node of the device the BootROM booted from. > > Please consider implementing this for *all* DTBs, not only the one we pass to > the OS. e.g. this is also quite useful in U-Boot proper itself for checking > which boot medium was used by the BootROM and which one was used by the SPL > to load U-Boot (via /chosen/u-boot,spl-boot-device; and e.g. verify fallback > mechanisms). I was not sure if this was wanted. I don't remember the spl-boot-device use cases. AFAIR it was not always matching what the ROM was booted from. > >> Platforms provide the path by implementing bootsource_get_ofpath(). The >> property is only set if the device tree passed to the OS has a node at >> that path. >> >> Signed-off-by: Wadim Egorov <[email protected]> >> --- >> boot/Kconfig | 8 ++++++++ >> boot/fdt_support.c | 30 +++++++++++++++++++++++++++++ >> doc/device-tree-bindings/chosen.txt | 9 +++++++++ >> include/fdt_support.h | 12 ++++++++++++ >> 4 files changed, 59 insertions(+) >> >> diff --git a/boot/Kconfig b/boot/Kconfig >> index c67dc0ba493..0dbd715999d 100644 >> --- a/boot/Kconfig >> +++ b/boot/Kconfig >> @@ -1922,6 +1922,14 @@ config OF_SYSTEM_SETUP >> system-specific information in the device tree for use by the OS. >> The device tree is then passed to the OS. >> +config FDT_FIXUP_BOOTSOURCE >> + bool "Add the BootROM boot source to the device tree before boot" >> + help >> + Record the device the SoC's BootROM loaded the very first boot >> + stage from as the /chosen/bootsource property in the device >> + tree passed to the OS. >> + The platform must implement bootsource_get_ofpath(). >> + > > Do we *really* need a Kconfig symbol for this? Why not always add it? I don't know, I don't have a big opinion on that. I was thinking its handy to have an option. > > If you're attempting to implement a generic solution, it'd be good to switch > the only implementer today (Rockchip, via spl_perform_arch_fixups in > arch/arm/mach-rockchip/spl-boot-order.c) to that generic solution. Would be nice if you could advice me, I have not touched rockchips for a while. But I would prefer if the Rockchip folks could do the switch once the first patch landed? Can I simply use boot_devices[] in every stage? Hm, maybe a const char *bootsource_get_ofpath(void) { return ofnode_read_chosen_string("bootsource"); } is enough? > >> config OF_STDOUT_VIA_ALIAS >> bool "Update the device-tree stdout alias from U-Boot" >> help >> diff --git a/boot/fdt_support.c b/boot/fdt_support.c >> index 2941df55996..95644b9b67c 100644 >> --- a/boot/fdt_support.c >> +++ b/boot/fdt_support.c >> @@ -334,6 +334,33 @@ __weak const char *board_fdt_chosen_bootargs(const >> struct fdt_property *fdt_ba) >> return env_get("bootargs"); >> } >> +__weak const char *bootsource_get_ofpath(void) >> +{ >> + return NULL; >> +} >> + >> +static void fdt_setup_bootsource(void *fdt, int chosen) >> +{ >> + const char *path; >> + int err; >> + >> + path = bootsource_get_ofpath(); >> + if (!path) >> + return; >> + >> + /* Only record a path this tree has a node at, so the OS can resolve it >> */ >> + if (fdt_path_offset(fdt, path) < 0) { >> + debug("%s: no node %s, not setting bootsource\n", >> + __func__, path); >> + return; >> + } >> + >> + err = fdt_setprop_string(fdt, chosen, "bootsource", path); >> + if (err < 0) >> + printf("WARNING: could not set bootsource %s.\n", >> + fdt_strerror(err)); > >> +} >> + >> int fdt_chosen(void *fdt) >> { >> struct abuf buf = {}; >> @@ -353,6 +380,9 @@ int fdt_chosen(void *fdt) >> if (nodeoffset < 0) >> return nodeoffset; >> + if (IS_ENABLED(CONFIG_FDT_FIXUP_BOOTSOURCE)) >> + fdt_setup_bootsource(fdt, nodeoffset); >> + >> /* if DM_RNG enabled automatically inject kaslr-seed node unless: >> * CONFIG_MEASURED_BOOT enabled: as dt modifications break measured >> boot >> * CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT enabled: as that implementation >> does not use dm yet >> diff --git a/doc/device-tree-bindings/chosen.txt >> b/doc/device-tree-bindings/chosen.txt >> index c8312540f57..5d737feb48b 100644 >> --- a/doc/device-tree-bindings/chosen.txt >> +++ b/doc/device-tree-bindings/chosen.txt >> @@ -114,6 +114,15 @@ of where said later stage was booted from. >> You should not define this property yourself in the device-tree, as it >> may be overwritten without warning. >> +bootsource property >> +------------------- >> + >> +This property is defined in the dt-schema chosen.yaml binding and >> +holds the full path of the node of the device the BootROM booted from. >> +With CONFIG_FDT_FIXUP_BOOTSOURCE enabled, U-Boot sets it in the device >> +tree passed to the OS if the platform implements bootsource_get_ofpath() >> +and that tree has a node at the returned path. >> + > > Do we really need to describe something that's already in the standard? Yeah, I can drop it. > >> firmware-loader property >> ------------------------ >> Multiple file system firmware loader nodes could be defined in device >> trees for >> diff --git a/include/fdt_support.h b/include/fdt_support.h >> index 47b8b63d13d..db2611da750 100644 >> --- a/include/fdt_support.h >> +++ b/include/fdt_support.h >> @@ -55,6 +55,18 @@ int fdt_root(void *fdt); >> */ >> int fdt_chosen(void *fdt); >> +/** >> + * bootsource_get_ofpath() - get the node path of the BootROM boot device > > The boot medium the BootROM used to load the very first stage after itself. Hm, okay. Seem to be just a longer version. > >> + * >> + * Platforms implement this so fdt_chosen() can set /chosen/bootsource in >> + * the device tree passed to the OS. The property is only set if that >> + * tree has a node at the returned path, so use the node name from the > > Not the node name, the node *path*. Yep, it is the path. > >> + * SoC device tree. > > If the boot device is a NOR flash for example, then we could have > /path/to/spi/flash@0 and flash@0 is very unlikely to be described in the SoC > include device tree. I'm not sure what you were trying to convey here, can > you clarify what you're trying to prevent the user from doing? I don't think it is very useful to pass a path which does not exist, no? Regards, Wadim > > Cheers, > Quentin
