[U-Boot] [PATCH 0/2] rpi: enable passthrough of FDT blob provided by the firmware

[U-Boot] [PATCH 0/2] rpi: enable passthrough of FDT blob provided by the firmware

[Cédric Schieli]

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 register passed to
U-Boot in the entry point and expose it in a environment variable for
further processing.

[1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018


C?dric Schieli (2):
  rpi: save firmware provided boot param for later use
  rpi: expose the firmware provided FDT blob in ${fw_fdt_addr}

 board/raspberrypi/rpi/Makefile        |  1 +
 board/raspberrypi/rpi/lowlevel_init.S | 26 ++++++++++++++++++++++++++
 board/raspberrypi/rpi/rpi.c           | 19 +++++++++++++++++++
 include/configs/rpi.h                 |  4 ++++
 4 files changed, 50 insertions(+)
 create mode 100644 board/raspberrypi/rpi/lowlevel_init.S

-- 
2.7.3

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Cédric Schieli]

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

[U-Boot] [PATCH 2/2] rpi: expose the firmware provided FDT blob in ${fw_fdt_addr}

[Cédric Schieli]

If the fw_boot_param saved at an early stage points to a valid FDT
blob, let's expose it in ${fw_fdt_addr}.

Signed-off-by: C?dric Schieli <cschieli@gmail.com>
---

 board/raspberrypi/rpi/rpi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index 6245b36..7261bc5 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -285,6 +285,24 @@ static void set_fdtfile(void)
 	setenv("fdtfile", fdtfile);
 }
 
+/*
+ * If the firmware provided us with a valid FDT at boot time, let's expose it
+ * in ${fw_fdt_addr} so it may be passed unmodified to the kernel.
+ */
+static void set_fw_fdt_addr(void)
+{
+	char s[3 + 2 * sizeof(fw_boot_param)];
+
+	if (getenv("fw_fdt_addr"))
+		return;
+
+	if (fdt_magic(fw_boot_param) != FDT_MAGIC)
+		return;
+
+	snprintf(s, sizeof(s), "0x%X", (unsigned int)fw_boot_param);
+	setenv("fw_fdt_addr", s);
+}
+
 static void set_usbethaddr(void)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(struct msg_get_mac_address, msg, 1);
@@ -356,6 +374,7 @@ static void set_serial_number(void)
 
 int misc_init_r(void)
 {
+	set_fw_fdt_addr();
 	set_fdtfile();
 	set_usbethaddr();
 #ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
-- 
2.7.3

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161104/df3d43a2/attachment.sig>

[U-Boot] [PATCH 2/2] rpi: expose the firmware provided FDT blob in ${fw_fdt_addr}

[Tom Rini]

On Wed, Nov 02, 2016 at 07:06:13PM +0100, C?dric Schieli wrote:

> If the fw_boot_param saved at an early stage points to a valid FDT
> blob, let's expose it in ${fw_fdt_addr}.
> 
> Signed-off-by: C?dric Schieli <cschieli@gmail.com>

OK, but how is fw_fdt_addr actually used?  I think we need to include
that in this patch too, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161104/d3a1d8a4/attachment.sig>

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Cédric Schieli]

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.

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161104/1b6de144/attachment.sig>

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Jonathan Liu]

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

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[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 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

[U-Boot] [PATCH 2/2] rpi: expose the firmware provided FDT blob in ${fw_fdt_addr}

[Stephen Warren]

On 11/02/2016 12:06 PM, C?dric Schieli wrote:
> If the fw_boot_param saved at an early stage points to a valid FDT
> blob, let's expose it in ${fw_fdt_addr}.

I'd suggest naming the variable dtb_addr not fw_fdt_addr. Both names are 
just as easy to use from custom U-Boot scripts, however certain parts of 
U-Boot (such as the extlinux.conf interpreter) automatically use the 
value stored in $fdt_addr if not DTB statement is found in 
extlinux.conf. That will make it much easier for people to actually pass 
this FW-supplied DTB to the kernel automatically.

> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c

> +/*
> + * If the firmware provided us with a valid FDT at boot time, let's expose it
> + * in ${fw_fdt_addr} so it may be passed unmodified to the kernel.
> + */
> +static void set_fw_fdt_addr(void)
> +{
> +	char s[3 + 2 * sizeof(fw_boot_param)];
> +
> +	if (getenv("fw_fdt_addr"))
> +		return;
> +
> +	if (fdt_magic(fw_boot_param) != FDT_MAGIC)
> +		return;
> +
> +	snprintf(s, sizeof(s), "0x%X", (unsigned int)fw_boot_param);
> +	setenv("fw_fdt_addr", s);

If you use setenv_hex(), that will simplify the code here.

Also, for this to work, you probably want to implement 
board_get_usable_ram_top(). Doing so will guarantee that when U-Boot 
relocates to the top of RAM, it won't over-write the FW-supplied DTB 
content. For an example (only partially tested, and that will only 
build/run on a subset of supported RPi models), see:

> https://github.com/swarren/u-boot/commit/8097d58931b42e88c74c1e88819f67b2e3cfb6bf#diff-e3f2f2e4eec27fe7837b4853cb5be10bR292

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Cédric Schieli]

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

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Cédric Schieli]

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

[U-Boot] [PATCH 1/2] rpi: save firmware provided boot param for later use

[Jonathan Liu]

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