| Message ID | 1478529895-9766-1-git-send-email-cschieli@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 11/07/2016 07:44 AM, Cédric Schieli wrote: > Raspberry firmware used to pass a FDT blob at a fixed address (0x100), > but this is not true anymore. The address now depends on both the > memory size and the blob size [1]. > > If one wants to passthrough this FDT blob to the kernel, the most > reliable way is to save its address from the r2/x0 register in the > U-Boot entry point and expose it in a environment variable for > further processing. > > This patch just does this: > - save the provided address in the global variable fw_dtb_pointer > - expose it in ${fdt_addr} if it points to a a valid FDT blob > > There are many different ways to use it. One can, for example, use > the following script which will extract from the tree the command > line built by the firmware, then hand over the blob to a previously > loaded kernel: > > if fdt addr ${fdt_addr} > then > fdt get value bootargs /chosen bootargs > bootz ${kernel_addr_r} - ${fdt_addr} > fi FWIW, I'd just hard-code the commands into any script that I wrote, and avoid the if test completely. If the FW doesn't pass a DTB, the system isn't going to boot correctly anyway, and I'd only use this custom script in a situation I know that it'd work. > Alternatively, users relying on sysboot/pxe can simply omit any FDT > statement in their extlinux.conf file, U-Boot will automagically pick > ${fdt_addr} and pass it to the kernel. > > Please note that for this to work the U-Boot binary must be tagged > with a recent version of the mkknlimg script found in the Rasperry > Fundation's kernel tree: > > <kernel>/scripts/mkknlimg --dtok <uboot>/u-boot.bin /boot/u-boot.bin > > [1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018 I believe the very latest firmware has been fixed to default to DTB rather than ATAGs, since essentially nothing uses ATAGs any more. At least, Phil mentioned that sometime; I haven't tracked the status of that change or tested it. > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > +#ifdef CONFIG_ARM64 > +void save_boot_params(unsigned long dtb) > +#else > +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long dtb) > +#endif > +{ > + fw_dtb_pointer = dtb; > + save_boot_params_ret(); > +} I think you need to write that function in assembly. This "function" is called very early during U-Boot startup, before any stack pointer is set up. You can't guarantee that a function written in C won't attempt to use the stack, although admittedly with the current code, Makefile, and the Ubuntu compilers, it just happens not to. Aside from that issue, this version looks fine.
2016-11-08 4:14 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>: > Please note that for this to work the U-Boot binary must be tagged >> with a recent version of the mkknlimg script found in the Rasperry >> Fundation's kernel tree: >> >> <kernel>/scripts/mkknlimg --dtok <uboot>/u-boot.bin /boot/u-boot.bin >> >> [1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018 >> > > > I believe the very latest firmware has been fixed to default to DTB rather > than ATAGs, since essentially nothing uses ATAGs any more. At least, Phil > mentioned that sometime; I haven't tracked the status of that change or > tested it. > You're right, it doesn't seem to be needed anymore. I've just booted a plain u-boot.bin binary and the DTB was provided. > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >> > > +#ifdef CONFIG_ARM64 >> +void save_boot_params(unsigned long dtb) >> +#else >> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long >> dtb) >> +#endif >> +{ >> + fw_dtb_pointer = dtb; >> + save_boot_params_ret(); >> +} >> > > I think you need to write that function in assembly. This "function" is > called very early during U-Boot startup, before any stack pointer is set > up. You can't guarantee that a function written in C won't attempt to use > the stack, although admittedly with the current code, Makefile, and the > Ubuntu compilers, it just happens not to. > Is inline assembly enough to guarantee that, or is it mandatory to revert to an assembly file ?
Hi Stephen, On 8 November 2016 at 14:14, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 11/07/2016 07:44 AM, Cédric Schieli wrote: >> >> Raspberry firmware used to pass a FDT blob at a fixed address (0x100), >> but this is not true anymore. The address now depends on both the >> memory size and the blob size [1]. >> >> If one wants to passthrough this FDT blob to the kernel, the most >> reliable way is to save its address from the r2/x0 register in the >> U-Boot entry point and expose it in a environment variable for >> further processing. >> >> This patch just does this: >> - save the provided address in the global variable fw_dtb_pointer >> - expose it in ${fdt_addr} if it points to a a valid FDT blob >> >> There are many different ways to use it. One can, for example, use >> the following script which will extract from the tree the command >> line built by the firmware, then hand over the blob to a previously >> loaded kernel: >> >> if fdt addr ${fdt_addr} >> then >> fdt get value bootargs /chosen bootargs >> bootz ${kernel_addr_r} - ${fdt_addr} >> fi > > > FWIW, I'd just hard-code the commands into any script that I wrote, and > avoid the if test completely. If the FW doesn't pass a DTB, the system isn't > going to boot correctly anyway, and I'd only use this custom script in a > situation I know that it'd work. > >> Alternatively, users relying on sysboot/pxe can simply omit any FDT >> statement in their extlinux.conf file, U-Boot will automagically pick >> ${fdt_addr} and pass it to the kernel. >> >> Please note that for this to work the U-Boot binary must be tagged >> with a recent version of the mkknlimg script found in the Rasperry >> Fundation's kernel tree: >> >> <kernel>/scripts/mkknlimg --dtok <uboot>/u-boot.bin /boot/u-boot.bin >> >> [1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018 > > > I believe the very latest firmware has been fixed to default to DTB rather > than ATAGs, since essentially nothing uses ATAGs any more. At least, Phil > mentioned that sometime; I haven't tracked the status of that change or > tested it. > >> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > > >> +#ifdef CONFIG_ARM64 >> +void save_boot_params(unsigned long dtb) >> +#else >> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long >> dtb) >> +#endif >> +{ >> + fw_dtb_pointer = dtb; >> + save_boot_params_ret(); >> +} > > > I think you need to write that function in assembly. This "function" is > called very early during U-Boot startup, before any stack pointer is set up. > You can't guarantee that a function written in C won't attempt to use the > stack, although admittedly with the current code, Makefile, and the Ubuntu > compilers, it just happens not to. Apparently tegra also uses a C function for save_boot_params in arch/arm/mach-tegra/board.c... The comment in arch/arm/include/asm/system.h suggests that save_boot_params() can be written in C: /** * save_boot_params_ret() - Return from save_boot_params() * * If you provide save_boot_params(), then you should jump back to this * function when done. Try to preserve all registers. * * If your implementation of save_boot_params() is in C then it is acceptable * to simply call save_boot_params_ret() at the end of your function. Since * there is no link register set up, you cannot just exit the function. U-Boot * will return to the (initialised) value of lr, and likely crash/hang. * * If your implementation of save_boot_params() is in assembler then you * should use 'b' or 'bx' to return to save_boot_params_ret. */ void save_boot_params_ret(void); As mentioned in the commit that added it: commit 5519912164698b634893913b4408fee736d01d06 Author: Simon Glass <sjg@chromium.org> Date: Mon May 4 11:31:03 2015 -0600 arm: Add a prototype for save_boot_params_ret() It is convenient for some boards to implement save_boot_params() in C rather than assembler. Provide a way to return in this case. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com> > > Aside from that issue, this version looks fine. Regards, Jonathan
On 11/08/2016 03:32 AM, Cédric Schieli wrote: > > 2016-11-08 4:14 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org > <mailto:swarren@wwwdotorg.org>>: > > Please note that for this to work the U-Boot binary must be tagged > with a recent version of the mkknlimg script found in the Rasperry > Fundation's kernel tree: > > <kernel>/scripts/mkknlimg --dtok <uboot>/u-boot.bin /boot/u-boot.bin > > [1] > https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018 > <https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018> > diff --git a/board/raspberrypi/rpi/rpi.c > b/board/raspberrypi/rpi/rpi.c > > > +#ifdef CONFIG_ARM64 > +void save_boot_params(unsigned long dtb) > +#else > +void save_boot_params(unsigned long r0, unsigned long r1, > unsigned long dtb) > +#endif > +{ > + fw_dtb_pointer = dtb; > + save_boot_params_ret(); > +} > > > I think you need to write that function in assembly. This "function" > is called very early during U-Boot startup, before any stack pointer > is set up. You can't guarantee that a function written in C won't > attempt to use the stack, although admittedly with the current code, > Makefile, and the Ubuntu compilers, it just happens not to. > > > Is inline assembly enough to guarantee that, or is it mandatory to > revert to an assembly file ? The entire function needs to be in assembly, since the compiler is free to touch the stack during function prologue/epilogue too.
On 11/08/2016 04:50 AM, Jonathan Liu wrote: > On 8 November 2016 at 14:14, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 11/07/2016 07:44 AM, Cédric Schieli wrote: >>> >>> Raspberry firmware used to pass a FDT blob at a fixed address (0x100), >>> but this is not true anymore. The address now depends on both the >>> memory size and the blob size [1]. >>> >>> If one wants to passthrough this FDT blob to the kernel, the most >>> reliable way is to save its address from the r2/x0 register in the >>> U-Boot entry point and expose it in a environment variable for >>> further processing. >>> >>> This patch just does this: >>> - save the provided address in the global variable fw_dtb_pointer >>> - expose it in ${fdt_addr} if it points to a a valid FDT blob >>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c >> >>> +#ifdef CONFIG_ARM64 >>> +void save_boot_params(unsigned long dtb) >>> +#else >>> +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long >>> dtb) >>> +#endif >>> +{ >>> + fw_dtb_pointer = dtb; >>> + save_boot_params_ret(); >>> +} >> >> I think you need to write that function in assembly. This "function" is >> called very early during U-Boot startup, before any stack pointer is set up. >> You can't guarantee that a function written in C won't attempt to use the >> stack, although admittedly with the current code, Makefile, and the Ubuntu >> compilers, it just happens not to. > > Apparently tegra also uses a C function for save_boot_params in > arch/arm/mach-tegra/board.c... Yes, that should be fixed. Simon, can you please look into that? I'm not sure whether the function is even needed; perhaps best to simply delete it. > The comment in arch/arm/include/asm/system.h suggests that > save_boot_params() can be written in C: > /** > * save_boot_params_ret() - Return from save_boot_params() > * > * If you provide save_boot_params(), then you should jump back to this > * function when done. Try to preserve all registers. > * > * If your implementation of save_boot_params() is in C then it is acceptable That should be fixed too; writing the function in C isn't guaranteed to work. > * to simply call save_boot_params_ret() at the end of your function. Since > * there is no link register set up, you cannot just exit the function. U-Boot > * will return to the (initialised) value of lr, and likely crash/hang. > * > * If your implementation of save_boot_params() is in assembler then you > * should use 'b' or 'bx' to return to save_boot_params_ret. > */ > void save_boot_params_ret(void); > > As mentioned in the commit that added it: > commit 5519912164698b634893913b4408fee736d01d06 > Author: Simon Glass <sjg@chromium.org> > Date: Mon May 4 11:31:03 2015 -0600 > > arm: Add a prototype for save_boot_params_ret() > > It is convenient for some boards to implement save_boot_params() in C rather > than assembler. Provide a way to return in this case. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Joe Hershberger <joe.hershberger@ni.com> > >> >> Aside from that issue, this version looks fine. > > Regards, > Jonathan >
diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c index 6245b36..b1614b9 100644 --- a/board/raspberrypi/rpi/rpi.c +++ b/board/raspberrypi/rpi/rpi.c @@ -25,6 +25,21 @@ DECLARE_GLOBAL_DATA_PTR; +/* From init.S */ +extern void save_boot_params_ret(void); + +static unsigned long fw_dtb_pointer __attribute__ ((section(".data"))); + +#ifdef CONFIG_ARM64 +void save_boot_params(unsigned long dtb) +#else +void save_boot_params(unsigned long r0, unsigned long r1, unsigned long dtb) +#endif +{ + fw_dtb_pointer = dtb; + save_boot_params_ret(); +} + static const struct bcm2835_gpio_platdata gpio_platdata = { .base = BCM2835_GPIO_BASE, }; @@ -285,6 +300,31 @@ static void set_fdtfile(void) setenv("fdtfile", fdtfile); } +/* + * If the firmware provided a valid FDT at boot time, let's expose it in + * ${fdt_addr} so it may be passed unmodified to the kernel. + */ +static void set_fdt_addr(void) +{ + if (getenv("fdt_addr")) + return; + + if (fdt_magic(fw_dtb_pointer) != FDT_MAGIC) + return; + + setenv_hex("fdt_addr", fw_dtb_pointer); +} + +/* + * Prevent relocation from stomping on a firmware provided FDT blob. + */ +unsigned long board_get_usable_ram_top(unsigned long total_size) +{ + if ((gd->ram_top - fw_dtb_pointer) > SZ_64M) + return gd->ram_top; + return fw_dtb_pointer &~0xffff; +} + static void set_usbethaddr(void) { ALLOC_CACHE_ALIGN_BUFFER(struct msg_get_mac_address, msg, 1); @@ -356,6 +396,7 @@ static void set_serial_number(void) int misc_init_r(void) { + set_fdt_addr(); set_fdtfile(); set_usbethaddr(); #ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
Raspberry firmware used to pass a FDT blob at a fixed address (0x100), but this is not true anymore. The address now depends on both the memory size and the blob size [1]. If one wants to passthrough this FDT blob to the kernel, the most reliable way is to save its address from the r2/x0 register in the U-Boot entry point and expose it in a environment variable for further processing. This patch just does this: - save the provided address in the global variable fw_dtb_pointer - expose it in ${fdt_addr} if it points to a a valid FDT blob There are many different ways to use it. One can, for example, use the following script which will extract from the tree the command line built by the firmware, then hand over the blob to a previously loaded kernel: if fdt addr ${fdt_addr} then fdt get value bootargs /chosen bootargs bootz ${kernel_addr_r} - ${fdt_addr} fi Alternatively, users relying on sysboot/pxe can simply omit any FDT statement in their extlinux.conf file, U-Boot will automagically pick ${fdt_addr} and pass it to the kernel. Please note that for this to work the U-Boot binary must be tagged with a recent version of the mkknlimg script found in the Rasperry Fundation's kernel tree: <kernel>/scripts/mkknlimg --dtok <uboot>/u-boot.bin /boot/u-boot.bin [1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018 Signed-off-by: Cédric Schieli <cschieli@gmail.com> --- Changes in v2: - merge the series in a single patch - convert the save_boot_params() function to C code - add a board_get_usable_ram_top() function to protect the blob during relocation - remove the (obsolete) extern declaration from include/configs/rpi.h - rename the global variable to fw_dtb_pointer - rename the environment variable to ${fdt_addr} - fix 64-bits compatibility issues - document possible uses of ${fdt_addr} board/raspberrypi/rpi/rpi.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)