[U-Boot,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 <cschieli@gmail.com>
---

 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

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 <cschieli@gmail.com>
[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.

2016-11-04 19:04 GMT+01:00 Tom Rini <trini@konsulko.com>:

> > 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 <cschieli@gmail.com>
> [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.

On Fri, Nov 04, 2016 at 08:18:29PM +0100, Cédric Schieli wrote:
> 2016-11-04 19:04 GMT+01:00 Tom Rini <trini@konsulko.com>:
> 
> > > 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 <cschieli@gmail.com>
> > [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.

Hi Cédric,

On 3 November 2016 at 05:06, Cédric Schieli <cschieli@gmail.com> 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 <cschieli@gmail.com>
> ---
>
>  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 0000000..446a70b
> --- /dev/null
> +++ b/board/raspberrypi/rpi/lowlevel_init.S
> @@ -0,0 +1,26 @@
> +/*
> + * (C) Copyright 2016
> + * Cédric Schieli <cschieli@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <config.h>
> +
> +.global fw_boot_param
> +fw_boot_param:
> +       .word 0x00000000
> +
> +/*
> + * 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

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 0x00000000

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

2016-11-06 1:36 GMT+01:00 Jonathan Liu <net147@gmail.com>:

> 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

2016-11-06 3:48 GMT+01:00 Stephen Warren <swarren@wwwdotorg.org>:

> 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 0x00000000
>>
>
> 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

Hi Cédric,

On 6 November 2016 at 21:06, Cédric Schieli <cschieli@gmail.com> wrote:
>
> 2016-11-06 1:36 GMT+01:00 Jonathan Liu <net147@gmail.com>:
>>
>> 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