[U-Boot] [PATCH 1/2] Pass through start.elf bootargs on Raspberry Pi

[Stephen Warren <swarren@wwwdotorg.org>]

On 11/29/2015 11:40 AM, Marco Schuster wrote:
> On Raspberry Pi, the primary bootloader start.elf uses the options in
> config.txt, as well as options hidden in the firmware itself, to tell
> the Linux kernel e.g. framebuffer sizes, memory regions, MAC addresses
> and more.

Was this patch sent using "git send-email"? I assume not, since it is
corrupted due to wrapping.

I suspect ./scripts/checkpatch.pl also wasn't run, since it would
complain about over-long lines, and multiple other formatting errors.

> Normally, u-boot would not be able to pass through these options to
> the Linux kernel. This patch adds pass-through support via an
> additional env variable "bootargs_orig" which is loaded with the
> original cmdline obtained from ATAG or the FDT.
> 
> In addition, this allows the user to configure DT overlays the "Pi
> way" by enabling them in config.txt, as this patch passes through the
> full FDT if it has been passed one by u-boot.
> 
> Note that, as stated in the comment block, for FDT passthrough to
> work, the u-boot.img must be run through a perl script by the
> Raspberry Pi Foundation, which appends a trailer to the image telling
> start.elf to supply a FDT instead of an ATAG struct.

This feature sounds fine. However, it should be optional. It likely only
makes sense when booting a downstream Pi Foundation kernel, and not when
booting an upstream/mainline kernel with matching DT using the
standardized DT bindings. As such, I think this feature should be
optional (e.g. perhaps en-/dis-abled by an environment variable at
run-time) so that the user gets to choose whether to use the feature.

> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
> index 6451d1d..d8d0fbb 100644
> --- a/board/raspberrypi/rpi/rpi.c
> +++ b/board/raspberrypi/rpi/rpi.c
> @@ -259,6 +259,72 @@ int misc_init_r(void)
>  #ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
>         set_board_info();
>  #endif
> +
> +       /*
> +        * The RPi start.elf passes important system configuration
> +        * like memory sizes and LED GPIO assignments to the kernel
> +        * via kernel command line.
> +        *
> +        * Therefore we have to parse the passed ATAG or FDT to get the

s/have to/may/

> +        * commandline and pass it through to script-land via the env
> +        * variable "bootargs_orig" so it can be passed on.
> +        *
> +        * By default, start.elf assumes a non-DT capable kernel and
> +        * passes the commandline via ATAG; this requires u-boot to
> +        * load the FDT from /boot and pass it on. This works if no
> +        * overlays have been passed through, but once overlays are in
> +        * the mix, stuff gets complicated to do in u-boot.

Ideally, DT overlays would be either implemented in U-Boot, or deferred
until the kernel boots and user-space can load overlays. I suspect the
latter is more inline with the usage model expected by
upstream/mainline, although I haven't checked to see.

> +        *
> +        * To force start.elf to pass a processed, ready-to-go DT,
> +        * you have to use the mkknlimg tool on the u-boot image:
> +        * ./mkknlimg --dtok u-boot.bin /boot/u-boot.bin
> +        *
> +        * mkknlimg can be obtained from
> https://github.com/raspberrypi/tools/blob/master/mkimage/knlinfo
> +        *
> +        * User scripts can check for successful bootargs retrieval
> +        * in the env variable pi_bootmode, which is either fdt, atag
> +        * or unknown in case of an error.
> +        *
> +        * The location 0x100 is hard-coded in start.elf, and it is
> +        * the same as in the default bootscripts, so you only have
> +        * to omit the fatload command loading the raw FDT to get
> +        * going.

"omit the fatload command" from where?

Personally I've been using extlinux.conf on my Pi for a while, so there
is no "fatload" anywhere that I'm aware of.

> +        */
> +
> +       void* ptr = (char*) 0x100;
> +       struct tag_header* atag_ptr = ptr;
> +       setenv("pi_bootmode","unknown");
> +
> +       if(atag_ptr->tag != ATAG_CORE) {
> +               if(atag_ptr->size == be32_to_cpu(FDT_MAGIC)) {
> +                       set_working_fdt_addr((ulong) ptr);

Doesn't that set the address of the DT that U-Boot uses to configure
itself, not the DT that U-Boot will pass to the kernel? The two are
certainly separate concepts. This code should set $fdt_addr/$fdt_addr_r
if the DT is detected, so that scripts can affect whether this DT is used.

> +                       int nodeoffset = fdt_path_offset(working_fdt,
> "/chosen");
> +                       int len;
> +                       const void* nodep = fdt_getprop(working_fdt,
> nodeoffset, "bootargs", &len);
> +                       if(len==0) {
> +                               printf("WARNING: Could not determine
> bootargs from FDT!\n");
> +                       } else {
> +                               printf("Set bootargs_orig from FDT\n");

debug() seems more appropriate than printf(), except for errors.

> +                               setenv("bootargs_orig", (char*) nodep);
> +                               setenv("pi_bootmode","fdt");
> +                       }
> +               } else {
> +                       printf("Warning: start.elf did not pass ATAG
> or FDT parameters\n");
> +               }
> +       } else {
> +               while(atag_ptr->tag != ATAG_NONE) {
> +                       printf("at %p, have %x (len
> %x)\n",atag_ptr,atag_ptr->tag, atag_ptr->size);
> +                       if(atag_ptr->tag == ATAG_CMDLINE) {
> +                               char* old_cmdline = ptr + 8;
> +                               printf("Set bootargs_orig from ATAG\n");
> +                               setenv("bootargs_orig", old_cmdline);
> +                               setenv("pi_bootmode", "atag");
> +                       }
> +                       ptr = ptr + (atag_ptr->size * 4);
> +                       atag_ptr=ptr;
> +               }
> +       }
> +
>         return 0;
>  }