| Message ID | 20231024-b4-prevbl-fdt-v1-1-541f71dba802@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: init: add an option to use FDT from previous bootloader | expand |
Hi Caleb, On Tue, 24 Oct 2023 at 04:32, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Add a new config option to allow u-boot to reuse the FDT provided by the U-Boot (please fix throughout) > previous stage bootloader when available. > > On some boards the previous stage bootloader can populate > platform-specific parts of the devicetree such as the memory node, this > allows us to avoid hardcoding it in u-boot and instead determine it > dynamically at runtime. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > This patch will improve generic support for Qualcomm boards by enabling > us to configure the memory map at runtime rather than having hardcoded > maps on a per-device basis. I've gone for this approach initially to try > and avoid introducing board specific code where possible, but I'm happy > to rework this into mach-snapdragon if that's preferred. Do you think it could use bloblist instead? I did send a patch to use the FDT in the bloblist: https://patchwork.ozlabs.org/project/uboot/patch/20230926141514.2101787-40-sjg@chromium.org/ Regards, Simon > --- > base-commit: e65b5d35c9116485366bb08138043d51220551da > > // Caleb (they/them) > --- > arch/arm/lib/save_prev_bl_data.c | 7 +++++++ > boot/Kconfig | 10 ++++++++++ > include/init.h | 9 +++++++++ > lib/fdtdec.c | 7 ++++++- > 4 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c > index f7b23faf0d66..b0608502535e 100644 > --- a/arch/arm/lib/save_prev_bl_data.c > +++ b/arch/arm/lib/save_prev_bl_data.c > @@ -45,6 +45,13 @@ bool is_addr_accessible(phys_addr_t addr) > return false; > } > > +phys_addr_t get_prev_bl_fdt_addr(void) > +{ > + if (!is_addr_accessible((phys_addr_t)reg0)) > + return 0; > + return reg0; > +} > + > int save_prev_bl_data(void) > { > struct fdt_header *fdt_blob; > diff --git a/boot/Kconfig b/boot/Kconfig > index a01e6cb8aafe..c127ba254589 100644 > --- a/boot/Kconfig > +++ b/boot/Kconfig > @@ -1599,6 +1599,16 @@ config SAVE_PREV_BL_INITRAMFS_START_ADDR > If no initramfs was provided by previous bootloader, no env variables > will be created. > > +config USE_PREV_BL_FDT > + depends on SAVE_PREV_BL_FDT_ADDR && !OF_BOARD > + bool "Use the FDT provided by the previous stage bootloader" > + help > + When u-boot is chain-loaded from a previous bootloader, enable this option > + to use the FDT provided by the previous bootloader instead of any built-in > + to u-boot. > + > + If no FDT was available, u-boot will fall back to its internal FDT. > + > menu "Configuration editor" > > config CEDIT > diff --git a/include/init.h b/include/init.h > index 4e7fe26c2004..58604cd98758 100644 > --- a/include/init.h > +++ b/include/init.h > @@ -168,6 +168,15 @@ defined(CONFIG_SAVE_PREV_BL_FDT_ADDR) > * Return: 0 if ok; -ENODATA on error > */ > int save_prev_bl_data(void); > + > +/** > + * get_prev_bl_fdt_addr - When u-boot is chainloaded, get the address > + * of the FDT passed by the previous bootloader. > + * > + * Return: the address of the FDT passed by the previous bootloader > + * or 0 if not found. > + */ > +phys_addr_t get_prev_bl_fdt_addr(void); > #endif > > /** > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index 7a6916764835..85425f2dc1ee 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -1222,7 +1222,7 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) > */ > static void *fdt_find_separate(void) > { > - void *fdt_blob = NULL; > + void *fdt_blob = NULL, *prevbl_fdt_blob; > > if (IS_ENABLED(CONFIG_SANDBOX)) > return NULL; > @@ -1234,6 +1234,11 @@ static void *fdt_find_separate(void) > else > fdt_blob = (ulong *)__bss_end; > #else > +#if defined(CONFIG_USE_PREV_BL_FDT) > + prevbl_fdt_blob = (void *)get_prev_bl_fdt_addr(); > + if (prevbl_fdt_blob) > + return prevbl_fdt_blob; > +#endif > /* FDT is at end of image */ > fdt_blob = (ulong *)_end; > > Regards, SImon
On 24/10/2023 19:03, Simon Glass wrote: > Hi Caleb, > > On Tue, 24 Oct 2023 at 04:32, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Add a new config option to allow u-boot to reuse the FDT provided by the > > U-Boot (please fix throughout) Will do! > >> previous stage bootloader when available. >> >> On some boards the previous stage bootloader can populate >> platform-specific parts of the devicetree such as the memory node, this >> allows us to avoid hardcoding it in u-boot and instead determine it >> dynamically at runtime. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> This patch will improve generic support for Qualcomm boards by enabling >> us to configure the memory map at runtime rather than having hardcoded >> maps on a per-device basis. I've gone for this approach initially to try >> and avoid introducing board specific code where possible, but I'm happy >> to rework this into mach-snapdragon if that's preferred. > > Do you think it could use bloblist instead? I did send a patch to use > the FDT in the bloblist: If it would require changes to the previous stage bootloader then unfortunately not. We're mostly stuck with the behaviour that we've got for now... This mechanism of retrieving the DTB is also used on the apple M1 I think, and any other board where we're booting U-Boot as a drop-in for the kernel on arm64. > > https://patchwork.ozlabs.org/project/uboot/patch/20230926141514.2101787-40-sjg@chromium.org/ > > Regards, > Simon > >> --- >> base-commit: e65b5d35c9116485366bb08138043d51220551da >> >> // Caleb (they/them) >> --- >> arch/arm/lib/save_prev_bl_data.c | 7 +++++++ >> boot/Kconfig | 10 ++++++++++ >> include/init.h | 9 +++++++++ >> lib/fdtdec.c | 7 ++++++- >> 4 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c >> index f7b23faf0d66..b0608502535e 100644 >> --- a/arch/arm/lib/save_prev_bl_data.c >> +++ b/arch/arm/lib/save_prev_bl_data.c >> @@ -45,6 +45,13 @@ bool is_addr_accessible(phys_addr_t addr) >> return false; >> } >> >> +phys_addr_t get_prev_bl_fdt_addr(void) >> +{ >> + if (!is_addr_accessible((phys_addr_t)reg0)) >> + return 0; >> + return reg0; >> +} >> + >> int save_prev_bl_data(void) >> { >> struct fdt_header *fdt_blob; >> diff --git a/boot/Kconfig b/boot/Kconfig >> index a01e6cb8aafe..c127ba254589 100644 >> --- a/boot/Kconfig >> +++ b/boot/Kconfig >> @@ -1599,6 +1599,16 @@ config SAVE_PREV_BL_INITRAMFS_START_ADDR >> If no initramfs was provided by previous bootloader, no env variables >> will be created. >> >> +config USE_PREV_BL_FDT >> + depends on SAVE_PREV_BL_FDT_ADDR && !OF_BOARD >> + bool "Use the FDT provided by the previous stage bootloader" >> + help >> + When u-boot is chain-loaded from a previous bootloader, enable this option >> + to use the FDT provided by the previous bootloader instead of any built-in >> + to u-boot. >> + >> + If no FDT was available, u-boot will fall back to its internal FDT. >> + >> menu "Configuration editor" >> >> config CEDIT >> diff --git a/include/init.h b/include/init.h >> index 4e7fe26c2004..58604cd98758 100644 >> --- a/include/init.h >> +++ b/include/init.h >> @@ -168,6 +168,15 @@ defined(CONFIG_SAVE_PREV_BL_FDT_ADDR) >> * Return: 0 if ok; -ENODATA on error >> */ >> int save_prev_bl_data(void); >> + >> +/** >> + * get_prev_bl_fdt_addr - When u-boot is chainloaded, get the address >> + * of the FDT passed by the previous bootloader. >> + * >> + * Return: the address of the FDT passed by the previous bootloader >> + * or 0 if not found. >> + */ >> +phys_addr_t get_prev_bl_fdt_addr(void); >> #endif >> >> /** >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c >> index 7a6916764835..85425f2dc1ee 100644 >> --- a/lib/fdtdec.c >> +++ b/lib/fdtdec.c >> @@ -1222,7 +1222,7 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) >> */ >> static void *fdt_find_separate(void) >> { >> - void *fdt_blob = NULL; >> + void *fdt_blob = NULL, *prevbl_fdt_blob; >> >> if (IS_ENABLED(CONFIG_SANDBOX)) >> return NULL; >> @@ -1234,6 +1234,11 @@ static void *fdt_find_separate(void) >> else >> fdt_blob = (ulong *)__bss_end; >> #else >> +#if defined(CONFIG_USE_PREV_BL_FDT) >> + prevbl_fdt_blob = (void *)get_prev_bl_fdt_addr(); >> + if (prevbl_fdt_blob) >> + return prevbl_fdt_blob; >> +#endif >> /* FDT is at end of image */ >> fdt_blob = (ulong *)_end; >> >> > > Regards, > SImon
On Tue, Oct 24, 2023 at 12:32:35PM +0100, Caleb Connolly wrote: > Add a new config option to allow u-boot to reuse the FDT provided by the > previous stage bootloader when available. > > On some boards the previous stage bootloader can populate > platform-specific parts of the devicetree such as the memory node, this > allows us to avoid hardcoding it in u-boot and instead determine it > dynamically at runtime. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > This patch will improve generic support for Qualcomm boards by enabling > us to configure the memory map at runtime rather than having hardcoded > maps on a per-device basis. I've gone for this approach initially to try > and avoid introducing board specific code where possible, but I'm happy > to rework this into mach-snapdragon if that's preferred. > --- > base-commit: e65b5d35c9116485366bb08138043d51220551da > > // Caleb (they/them) > --- > arch/arm/lib/save_prev_bl_data.c | 7 +++++++ > boot/Kconfig | 10 ++++++++++ > include/init.h | 9 +++++++++ > lib/fdtdec.c | 7 ++++++- So what is different with this instead of using save_boot_params ? I'm not saying this isn't needed, but I don't immediately see why save_boot_params + (OF_HAS_PRIOR_STAGE=y&&OF_BOARD=y) isn't the solution to the problem.
Hi Caleb, On Tue, 24 Oct 2023 at 11:10, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 24/10/2023 19:03, Simon Glass wrote: > > Hi Caleb, > > > > On Tue, 24 Oct 2023 at 04:32, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> Add a new config option to allow u-boot to reuse the FDT provided by the > > > > U-Boot (please fix throughout) > > Will do! > > > >> previous stage bootloader when available. > >> > >> On some boards the previous stage bootloader can populate > >> platform-specific parts of the devicetree such as the memory node, this > >> allows us to avoid hardcoding it in u-boot and instead determine it > >> dynamically at runtime. > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> This patch will improve generic support for Qualcomm boards by enabling > >> us to configure the memory map at runtime rather than having hardcoded > >> maps on a per-device basis. I've gone for this approach initially to try > >> and avoid introducing board specific code where possible, but I'm happy > >> to rework this into mach-snapdragon if that's preferred. > > > > Do you think it could use bloblist instead? I did send a patch to use > > the FDT in the bloblist: > > If it would require changes to the previous stage bootloader then > unfortunately not. We're mostly stuck with the behaviour that we've got > for now... Yes it would. What is the previous-stage bootloader? > > This mechanism of retrieving the DTB is also used on the apple M1 I > think, and any other board where we're booting U-Boot as a drop-in for > the kernel on arm64. I believe M1 is an open source project so perhaps they could adopt firmware handoff when it is finalised. Anyway, can this use OF_BOARD instead, perhaps? It already permits board-specific actions and is used by some rpi boards. > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230926141514.2101787-40-sjg@chromium.org/ > > > > Regards, > > Simon > > Regards, Simon
On 24/10/2023 20:25, Tom Rini wrote: > On Tue, Oct 24, 2023 at 12:32:35PM +0100, Caleb Connolly wrote: > >> Add a new config option to allow u-boot to reuse the FDT provided by the >> previous stage bootloader when available. >> >> On some boards the previous stage bootloader can populate >> platform-specific parts of the devicetree such as the memory node, this >> allows us to avoid hardcoding it in u-boot and instead determine it >> dynamically at runtime. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> This patch will improve generic support for Qualcomm boards by enabling >> us to configure the memory map at runtime rather than having hardcoded >> maps on a per-device basis. I've gone for this approach initially to try >> and avoid introducing board specific code where possible, but I'm happy >> to rework this into mach-snapdragon if that's preferred. >> --- >> base-commit: e65b5d35c9116485366bb08138043d51220551da >> >> // Caleb (they/them) >> --- >> arch/arm/lib/save_prev_bl_data.c | 7 +++++++ >> boot/Kconfig | 10 ++++++++++ >> include/init.h | 9 +++++++++ >> lib/fdtdec.c | 7 ++++++- > > So what is different with this instead of using save_boot_params ? I'm > not saying this isn't needed, but I don't immediately see why > save_boot_params + (OF_HAS_PRIOR_STAGE=y&&OF_BOARD=y) isn't the solution > to the problem. OF_BOARD would work here by implementing board_fdt_blob_setup() for mach-snapdragon, my hope was to avoid doing this in a board-specific way, but if that's preferred then I can totally do that instead. I left a comment below the patch about this, although it probably wasn't super clear. Thanks, >
On Tue, Oct 24, 2023 at 08:52:22PM +0100, Caleb Connolly wrote: > > > On 24/10/2023 20:25, Tom Rini wrote: > > On Tue, Oct 24, 2023 at 12:32:35PM +0100, Caleb Connolly wrote: > > > >> Add a new config option to allow u-boot to reuse the FDT provided by the > >> previous stage bootloader when available. > >> > >> On some boards the previous stage bootloader can populate > >> platform-specific parts of the devicetree such as the memory node, this > >> allows us to avoid hardcoding it in u-boot and instead determine it > >> dynamically at runtime. > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> This patch will improve generic support for Qualcomm boards by enabling > >> us to configure the memory map at runtime rather than having hardcoded > >> maps on a per-device basis. I've gone for this approach initially to try > >> and avoid introducing board specific code where possible, but I'm happy > >> to rework this into mach-snapdragon if that's preferred. > >> --- > >> base-commit: e65b5d35c9116485366bb08138043d51220551da > >> > >> // Caleb (they/them) > >> --- > >> arch/arm/lib/save_prev_bl_data.c | 7 +++++++ > >> boot/Kconfig | 10 ++++++++++ > >> include/init.h | 9 +++++++++ > >> lib/fdtdec.c | 7 ++++++- > > > > So what is different with this instead of using save_boot_params ? I'm > > not saying this isn't needed, but I don't immediately see why > > save_boot_params + (OF_HAS_PRIOR_STAGE=y&&OF_BOARD=y) isn't the solution > > to the problem. > > OF_BOARD would work here by implementing board_fdt_blob_setup() for > mach-snapdragon, my hope was to avoid doing this in a board-specific > way, but if that's preferred then I can totally do that instead. > > I left a comment below the patch about this, although it probably wasn't > super clear. Ah, yes. I guess I don't see what I suggested as "board specific" in that outside of efforts like bloblist that Simon mentioned, every semiconductor is slightly different. So yes please, OF_BOARD and board_fdt_blob_setup and so forth instead, thanks.
> From: Simon Glass <sjg@chromium.org> > Date: Tue, 24 Oct 2023 12:34:39 -0700 > > > This mechanism of retrieving the DTB is also used on the apple M1 I > > think, and any other board where we're booting U-Boot as a drop-in for > > the kernel on arm64. > > I believe M1 is an open source project so perhaps they could adopt > firmware handoff when it is finalised. Hi Simon, I'll repeat what I've said before; handoff for M1 is deliberately 100% compatible with handoff to the Linux kernel such that it is possible to bypass U-Boot and directly boot a Linux kernel. This is a feature the Asahi Linux developers depend on. If you want us to make changes here, you need to convince the Linux kernel developers to adjust Documentation/arch/arm64/bootig.rst to allow for the firmware handoff convention.
Hi Mark, On Wed, 25 Oct 2023 at 08:05, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Tue, 24 Oct 2023 12:34:39 -0700 > > > > > This mechanism of retrieving the DTB is also used on the apple M1 I > > > think, and any other board where we're booting U-Boot as a drop-in for > > > the kernel on arm64. > > > > I believe M1 is an open source project so perhaps they could adopt > > firmware handoff when it is finalised. > > Hi Simon, > > I'll repeat what I've said before; handoff for M1 is deliberately 100% > compatible with handoff to the Linux kernel such that it is possible > to bypass U-Boot and directly boot a Linux kernel. This is a feature > the Asahi Linux developers depend on. > > If you want us to make changes here, you need to convince the Linux > kernel developers to adjust Documentation/arch/arm64/bootig.rst to > allow for the firmware handoff convention. I don't have an infinite memory. In fact it does not extend all that far back, so I appreciate you mentioning this again. What is U-Boot for, if Asahi boots Linux directly? In any case, firmware handoff needs to be treated separately from Linux handoff. If you are trying to push the Linux approach into firmware, it will cause all sorts of things to become impossible, including universal payload. We can handle this with magic values in the registers, if needed. Of course, I have to mention that if we used a sensible format with metadata for Linux (like FIT) programs would *know* what they are booting, and be able to set things up correctly accordingly. It is high time we started thinking about this a bit more holistically, instead of going for the lower common denominator. Regards, Simon
diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c index f7b23faf0d66..b0608502535e 100644 --- a/arch/arm/lib/save_prev_bl_data.c +++ b/arch/arm/lib/save_prev_bl_data.c @@ -45,6 +45,13 @@ bool is_addr_accessible(phys_addr_t addr) return false; } +phys_addr_t get_prev_bl_fdt_addr(void) +{ + if (!is_addr_accessible((phys_addr_t)reg0)) + return 0; + return reg0; +} + int save_prev_bl_data(void) { struct fdt_header *fdt_blob; diff --git a/boot/Kconfig b/boot/Kconfig index a01e6cb8aafe..c127ba254589 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -1599,6 +1599,16 @@ config SAVE_PREV_BL_INITRAMFS_START_ADDR If no initramfs was provided by previous bootloader, no env variables will be created. +config USE_PREV_BL_FDT + depends on SAVE_PREV_BL_FDT_ADDR && !OF_BOARD + bool "Use the FDT provided by the previous stage bootloader" + help + When u-boot is chain-loaded from a previous bootloader, enable this option + to use the FDT provided by the previous bootloader instead of any built-in + to u-boot. + + If no FDT was available, u-boot will fall back to its internal FDT. + menu "Configuration editor" config CEDIT diff --git a/include/init.h b/include/init.h index 4e7fe26c2004..58604cd98758 100644 --- a/include/init.h +++ b/include/init.h @@ -168,6 +168,15 @@ defined(CONFIG_SAVE_PREV_BL_FDT_ADDR) * Return: 0 if ok; -ENODATA on error */ int save_prev_bl_data(void); + +/** + * get_prev_bl_fdt_addr - When u-boot is chainloaded, get the address + * of the FDT passed by the previous bootloader. + * + * Return: the address of the FDT passed by the previous bootloader + * or 0 if not found. + */ +phys_addr_t get_prev_bl_fdt_addr(void); #endif /** diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 7a6916764835..85425f2dc1ee 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1222,7 +1222,7 @@ static int uncompress_blob(const void *src, ulong sz_src, void **dstp) */ static void *fdt_find_separate(void) { - void *fdt_blob = NULL; + void *fdt_blob = NULL, *prevbl_fdt_blob; if (IS_ENABLED(CONFIG_SANDBOX)) return NULL; @@ -1234,6 +1234,11 @@ static void *fdt_find_separate(void) else fdt_blob = (ulong *)__bss_end; #else +#if defined(CONFIG_USE_PREV_BL_FDT) + prevbl_fdt_blob = (void *)get_prev_bl_fdt_addr(); + if (prevbl_fdt_blob) + return prevbl_fdt_blob; +#endif /* FDT is at end of image */ fdt_blob = (ulong *)_end;
Add a new config option to allow u-boot to reuse the FDT provided by the previous stage bootloader when available. On some boards the previous stage bootloader can populate platform-specific parts of the devicetree such as the memory node, this allows us to avoid hardcoding it in u-boot and instead determine it dynamically at runtime. Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- This patch will improve generic support for Qualcomm boards by enabling us to configure the memory map at runtime rather than having hardcoded maps on a per-device basis. I've gone for this approach initially to try and avoid introducing board specific code where possible, but I'm happy to rework this into mach-snapdragon if that's preferred. --- base-commit: e65b5d35c9116485366bb08138043d51220551da // Caleb (they/them) --- arch/arm/lib/save_prev_bl_data.c | 7 +++++++ boot/Kconfig | 10 ++++++++++ include/init.h | 9 +++++++++ lib/fdtdec.c | 7 ++++++- 4 files changed, 32 insertions(+), 1 deletion(-)