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

[Jonathan Liu]

Hi Stephen,

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