Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
Hi Cédric, On 6 November 2016 at 21:06, Cédric Schieli wrote: > > 2016-11-06 1:36 GMT+01:00 Jonathan Liu : >> >> I did a similar patch without noticing you already submitted this series. >> The save_boot_params function can be written in C instead of assembly >> and placed in rpi.c. >> See >> https://lists.yoctoproject.org/pipermail/yocto/2016-November/032934.html >> for how to write save_boot_params in C as this can simplify your >> patch. It also has a U-Boot script for using the FDT provided by the >> firmware, though you would need to change ${fdt_addr_r} to >> ${fw_fdt_addr} for it to work with your patch series. > > > I do not have a strong opinion on wether save_boot_params should be C or > assembly code. I'll opt for what the maintainers prefer here. > > Regarding the script, I'll include a example in my next version. I don't > like the idea to hijack ${fdt_addr_r} as it is documented as a pointer to > where one can manually load a custom FDT blob. If we make it points to the > firmware provided one and the user manually loads a bigger blob, unexpected > results may happen. Yes, I prefer your method of storing it in a separate environment variable. Regards, Jonathan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
2016-11-06 3:48 GMT+01:00 Stephen Warren : > On 11/02/2016 12:06 PM, Cédric Schieli wrote: > >> At U-Boot entry point, the r2 register holds the address of the >> firmware provided boot param. Let's save it for further processing. >> > > diff --git a/board/raspberrypi/rpi/lowlevel_init.S >> b/board/raspberrypi/rpi/lowlevel_init.S >> > > +.global fw_boot_param >> +fw_boot_param: >> + .word 0x >> > > fw_dtb_pointer might be a better name; there are multiple different > registers set up by the FW in some cases; best to be explicit about what > kind of parameter is being saved. > I did not want to use a DT oriented naming because of the possibility to pass a ATAG instead. But I can change it if you prefer. > See the note later about the size/alignment requirements for this value. > > +/* >> + * Routine: save_boot_params (called after reset from start.S) >> + * Description: save ATAG/FDT address provided by the firmware at boot >> time >> + */ >> + >> +.global save_boot_params >> +save_boot_params: >> + >> + /* The firmware provided ATAG/FDT address can be found in r2 */ >> + str r2, fw_boot_param >> > > For the 64-bit RPi builds, you need to save x0 not r2. The assembly above > doesn't compile since r2 isn't a valid register (it's named x2 on 64-bit), > plus the DTB pointer is actually in x0 not x2. > Noted. I'll address all the 64-bit issues in the next round. > > + /* Returns */ >> + b save_boot_params_ret >> > > With these patches applied, the build of rpi_defconfig fails since the > ARM1176 CPU startup file doesn't define that symbol. > > diff --git a/include/configs/rpi.h b/include/configs/rpi.h >> > > +#ifndef __ASSEMBLY__ >> +/* Firmware provided boot param */ >> +extern const void *fw_boot_param; >> +#endif >> > This issue will go away with the next round as the extern declaration will be moved to the rpi.c file, or simply removed if the save_boot_params code is converted to C. Regards, Cédric ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
2016-11-06 1:36 GMT+01:00 Jonathan Liu : > I did a similar patch without noticing you already submitted this series. > The save_boot_params function can be written in C instead of assembly > and placed in rpi.c. > See https://lists.yoctoproject.org/pipermail/yocto/2016- > November/032934.html > for how to write save_boot_params in C as this can simplify your > patch. It also has a U-Boot script for using the FDT provided by the > firmware, though you would need to change ${fdt_addr_r} to > ${fw_fdt_addr} for it to work with your patch series. > I do not have a strong opinion on wether save_boot_params should be C or assembly code. I'll opt for what the maintainers prefer here. Regarding the script, I'll include a example in my next version. I don't like the idea to hijack ${fdt_addr_r} as it is documented as a pointer to where one can manually load a custom FDT blob. If we make it points to the firmware provided one and the user manually loads a bigger blob, unexpected results may happen. Regards, Cédric ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
On 11/02/2016 12:06 PM, Cédric Schieli wrote: At U-Boot entry point, the r2 register holds the address of the firmware provided boot param. Let's save it for further processing. diff --git a/board/raspberrypi/rpi/lowlevel_init.S b/board/raspberrypi/rpi/lowlevel_init.S +.global fw_boot_param +fw_boot_param: + .word 0x fw_dtb_pointer might be a better name; there are multiple different registers set up by the FW in some cases; best to be explicit about what kind of parameter is being saved. See the note later about the size/alignment requirements for this value. +/* + * Routine: save_boot_params (called after reset from start.S) + * Description: save ATAG/FDT address provided by the firmware at boot time + */ + +.global save_boot_params +save_boot_params: + + /* The firmware provided ATAG/FDT address can be found in r2 */ + str r2, fw_boot_param For the 64-bit RPi builds, you need to save x0 not r2. The assembly above doesn't compile since r2 isn't a valid register (it's named x2 on 64-bit), plus the DTB pointer is actually in x0 not x2. + /* Returns */ + b save_boot_params_ret With these patches applied, the build of rpi_defconfig fails since the ARM1176 CPU startup file doesn't define that symbol. diff --git a/include/configs/rpi.h b/include/configs/rpi.h +#ifndef __ASSEMBLY__ +/* Firmware provided boot param */ +extern const void *fw_boot_param; +#endif For the rpi_3 build, a void* is a 64-bit value, yet in lowlevel_init.S, it's defined a a .word (32-bit) rather than a .dword (64-bit). I'd suggest adjusting the assembly file so that fw_boot_param is either a .word or a .dword depending on whether the code is being built for 32-bit or 64-bit mode. That will allow the C definition to be identical across all RPi builds. Note: In the .dword case the symbol must be aligned to 8-bytes, and it won't hurt in the 32-bit case; see the following patch: https://patchwork.ozlabs.org/patch/684429/ ARM: tegra: ensure nvtboot_boot_x0 alignment ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
Hi Cédric, On 3 November 2016 at 05:06, Cédric Schieli wrote: > At U-Boot entry point, the r2 register holds the address of the > firmware provided boot param. Let's save it for further processing. > > Signed-off-by: Cédric Schieli > --- > > board/raspberrypi/rpi/Makefile| 1 + > board/raspberrypi/rpi/lowlevel_init.S | 26 ++ > include/configs/rpi.h | 4 > 3 files changed, 31 insertions(+) > create mode 100644 board/raspberrypi/rpi/lowlevel_init.S > > diff --git a/board/raspberrypi/rpi/Makefile b/board/raspberrypi/rpi/Makefile > index 4ce2c98..dcb25ac 100644 > --- a/board/raspberrypi/rpi/Makefile > +++ b/board/raspberrypi/rpi/Makefile > @@ -5,3 +5,4 @@ > # > > obj-y := rpi.o > +obj-y += lowlevel_init.o > diff --git a/board/raspberrypi/rpi/lowlevel_init.S > b/board/raspberrypi/rpi/lowlevel_init.S > new file mode 100644 > index 000..446a70b > --- /dev/null > +++ b/board/raspberrypi/rpi/lowlevel_init.S > @@ -0,0 +1,26 @@ > +/* > + * (C) Copyright 2016 > + * Cédric Schieli > + * > + * SPDX-License-Identifier:GPL-2.0+ > + */ > + > +#include > + > +.global fw_boot_param > +fw_boot_param: > + .word 0x > + > +/* > + * Routine: save_boot_params (called after reset from start.S) > + * Description: save ATAG/FDT address provided by the firmware at boot time > + */ > + > +.global save_boot_params > +save_boot_params: > + > + /* The firmware provided ATAG/FDT address can be found in r2 */ > + str r2, fw_boot_param > + > + /* Returns */ > + b save_boot_params_ret > diff --git a/include/configs/rpi.h b/include/configs/rpi.h > index 8d4ad5d..2d1e619 100644 > --- a/include/configs/rpi.h > +++ b/include/configs/rpi.h > @@ -208,5 +208,9 @@ > ENV_MEM_LAYOUT_SETTINGS \ > BOOTENV > > +#ifndef __ASSEMBLY__ > +/* Firmware provided boot param */ > +extern const void *fw_boot_param; > +#endif > > #endif > -- > 2.7.3 I did a similar patch without noticing you already submitted this series. The save_boot_params function can be written in C instead of assembly and placed in rpi.c. See https://lists.yoctoproject.org/pipermail/yocto/2016-November/032934.html for how to write save_boot_params in C as this can simplify your patch. It also has a U-Boot script for using the FDT provided by the firmware, though you would need to change ${fdt_addr_r} to ${fw_fdt_addr} for it to work with your patch series. Regards, Jonathan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
On Fri, Nov 04, 2016 at 08:18:29PM +0100, Cédric Schieli wrote: > 2016-11-04 19:04 GMT+01:00 Tom Rini : > > > > At U-Boot entry point, the r2 register holds the address of the > > > firmware provided boot param. Let's save it for further processing. > > > > > > Signed-off-by: Cédric Schieli > > [snip] > > > diff --git a/include/configs/rpi.h b/include/configs/rpi.h > > > index 8d4ad5d..2d1e619 100644 > > > --- a/include/configs/rpi.h > > > +++ b/include/configs/rpi.h > > > @@ -208,5 +208,9 @@ > > > ENV_MEM_LAYOUT_SETTINGS \ > > > BOOTENV > > > > > > +#ifndef __ASSEMBLY__ > > > +/* Firmware provided boot param */ > > > +extern const void *fw_boot_param; > > > +#endif > > > > This is not a good place to hold this. Looking at 2/2, just put this > > into the board file and comment above it that it's declared in > > lowlevel_init.S. > > That's where I've put it in the first place, but patmat complained about > externs in .c file... > I'll adress that in the next version. OK. Yes, in this case it's better to ignore that warning than to throw it into config.h. If there was already an rpi.h, it should go in there. I suppose that it's probably best overall to create an rpi.h and put it in there. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
2016-11-04 19:04 GMT+01:00 Tom Rini : > > At U-Boot entry point, the r2 register holds the address of the > > firmware provided boot param. Let's save it for further processing. > > > > Signed-off-by: Cédric Schieli > [snip] > > diff --git a/include/configs/rpi.h b/include/configs/rpi.h > > index 8d4ad5d..2d1e619 100644 > > --- a/include/configs/rpi.h > > +++ b/include/configs/rpi.h > > @@ -208,5 +208,9 @@ > > ENV_MEM_LAYOUT_SETTINGS \ > > BOOTENV > > > > +#ifndef __ASSEMBLY__ > > +/* Firmware provided boot param */ > > +extern const void *fw_boot_param; > > +#endif > > This is not a good place to hold this. Looking at 2/2, just put this > into the board file and comment above it that it's declared in > lowlevel_init.S. That's where I've put it in the first place, but patmat complained about externs in .c file... I'll adress that in the next version. Thanks. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
On Wed, Nov 02, 2016 at 07:06:12PM +0100, Cédric Schieli wrote: > At U-Boot entry point, the r2 register holds the address of the > firmware provided boot param. Let's save it for further processing. > > Signed-off-by: Cédric Schieli [snip] > diff --git a/include/configs/rpi.h b/include/configs/rpi.h > index 8d4ad5d..2d1e619 100644 > --- a/include/configs/rpi.h > +++ b/include/configs/rpi.h > @@ -208,5 +208,9 @@ > ENV_MEM_LAYOUT_SETTINGS \ > BOOTENV > > +#ifndef __ASSEMBLY__ > +/* Firmware provided boot param */ > +extern const void *fw_boot_param; > +#endif This is not a good place to hold this. Looking at 2/2, just put this into the board file and comment above it that it's declared in lowlevel_init.S. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use
At U-Boot entry point, the r2 register holds the address of the firmware provided boot param. Let's save it for further processing. Signed-off-by: Cédric Schieli --- board/raspberrypi/rpi/Makefile| 1 + board/raspberrypi/rpi/lowlevel_init.S | 26 ++ include/configs/rpi.h | 4 3 files changed, 31 insertions(+) create mode 100644 board/raspberrypi/rpi/lowlevel_init.S diff --git a/board/raspberrypi/rpi/Makefile b/board/raspberrypi/rpi/Makefile index 4ce2c98..dcb25ac 100644 --- a/board/raspberrypi/rpi/Makefile +++ b/board/raspberrypi/rpi/Makefile @@ -5,3 +5,4 @@ # obj-y := rpi.o +obj-y += lowlevel_init.o diff --git a/board/raspberrypi/rpi/lowlevel_init.S b/board/raspberrypi/rpi/lowlevel_init.S new file mode 100644 index 000..446a70b --- /dev/null +++ b/board/raspberrypi/rpi/lowlevel_init.S @@ -0,0 +1,26 @@ +/* + * (C) Copyright 2016 + * Cédric Schieli + * + * SPDX-License-Identifier:GPL-2.0+ + */ + +#include + +.global fw_boot_param +fw_boot_param: + .word 0x + +/* + * Routine: save_boot_params (called after reset from start.S) + * Description: save ATAG/FDT address provided by the firmware at boot time + */ + +.global save_boot_params +save_boot_params: + + /* The firmware provided ATAG/FDT address can be found in r2 */ + str r2, fw_boot_param + + /* Returns */ + b save_boot_params_ret diff --git a/include/configs/rpi.h b/include/configs/rpi.h index 8d4ad5d..2d1e619 100644 --- a/include/configs/rpi.h +++ b/include/configs/rpi.h @@ -208,5 +208,9 @@ ENV_MEM_LAYOUT_SETTINGS \ BOOTENV +#ifndef __ASSEMBLY__ +/* Firmware provided boot param */ +extern const void *fw_boot_param; +#endif #endif -- 2.7.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot