[U-Boot] [PATCH v2] rpi: passthrough of the firmware provided FDT blob

[Stephen Warren]

On 11/08/2016 04:50 AM, Jonathan Liu wrote:
> On 8 November 2016 at 14:14, Stephen Warren <swarren at 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 at 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 at chromium.org>
>     Reviewed-by: Joe Hershberger <joe.hershberger at ni.com>
>
>>
>> Aside from that issue, this version looks fine.
>
> Regards,
> Jonathan
>