[RFC] Load U-Boot without LK on DragonBoard 410c (+ DB820c?)

[Ramon Fried]

On Sun, Jul 4, 2021 at 2:14 PM Stephan Gerhold <stephan at gerhold.net> wrote:
>
> On Sun, Jul 04, 2021 at 02:08:39PM +0300, Ramon Fried wrote:
> > On Sun, Jul 4, 2021 at 12:42 AM Stephan Gerhold <stephan at gerhold.net> wrote:
> > >
> > > On Thu, Jul 01, 2021 at 11:07:55AM +0200, Stephan Gerhold wrote:
> > > > Hi!
> > > >
> > > > at the moment the U-Boot ports for both DragonBoard 410c and 820c are
> > > > designed to be loaded as an Android boot image after Qualcomm's LK
> > > > bootloader. This is simple to set up but LK is redundant in this case,
> > > > since everything done by LK can be also done directly by U-Boot.
> > > >
> > > > Dropping LK entirely would have at least the following advantages:
> > > >   - Easier installation/board code (no need for Android boot images)
> > > >   - (Slightly) faster boot
> > > >   - Boot directly in 64-bit without a round trip to 32-bit for LK
> > > >
> > > > [...]
> > > >
> > > > 3. Remaining open questions
> > > > ===========================
> > > >
> > > > [...]
> > > >
> > > >   3. CONFIG_OF_EMBED: There is a big warning about this in the build log:
> > > >      "This option should only be used for debugging purposes. Please use
> > > >       CONFIG_OF_SEPARATE for boards in mainline."
> > > >
> > > >      The important part here is that we need an ELF image with both
> > > >      U-Boot and the DTB. CONFIG_OF_EMBED is convenient for that because
> > > >      we can just use the ELF image built by the linker and it already
> > > >      contains the DTB.
> > > >
> > > >      If CONFIG_OF_EMBED is really so bad it might be possible to build
> > > >      a new boot image based on "u-boot-dtb.bin" (which is U-Boot with
> > > >      DTB appended). I'm not sure if this is really much better though.
> > > >
> > >
> > > After looking some more I found "CONFIG_REMAKE_ELF" which seems to do
> > > exactly this (build a new ELF image based on "u-boot-dtb.bin" with
> > > appended DTB). So this avoids setting CONFIG_OF_EMBED and therefore the
> > > build warning. Sounds like the solution I was looking for. :)
> > >
> > > Unfortunately it looks like appending the DTB to U-Boot currently
> > > produces very strange behavior on DragonBoard 410c. It's either:
> > >
> > >   - Working fine, or
> > >   - Rebooting continously without serial output from U-Boot, or
> > >   - The following serial output:
> > >
> > >       Qualcomm-DragonBoard 410C
> > >       DRAM:  986 MiB
> > >       No serial driver found
> > >       resetting ...
> > >
> > > It behaves consistently given a U-Boot binary but varies when
> > > adding/removing random features from the U-Boot binary.
> > >
> > > After a couple of hours of debugging, I realized that the appended DTB
> > > becomes corrupted. Specifically, there is a "GPIO_5" written into it, e.g.
> > >
> > > 8f6905b8: edfe0dd0 9f100000 4f495047 c000355f    ........GPIO_5..
> > > 8f6905c8: 28000000 11000000 10000000 00000000    ...(............
> > > 8f6905d8: df010000 880e0000 00000000 00000000    ................
> > > 8f6905e8: 00000000 00000000 01000000 00000000    ................
> > > 8f6905f8: 03000000 04000000 00000000 02000000    ................
> > > 8f690608: 03000000 04000000 0f000000 02000000    ................
> > > 8f690618: 03000000 2d000000 1b000000 6c617551    .......-....Qual
> > > 8f690628: 6d6d6f63 63655420 6c6f6e68 6569676f    comm Technologie
> > >
> > > Depending on enabled features in U-Boot the "GPIO_5" corrupts different
> > > parts of the DTB, that's why it works somewhat sometimes.
> > >
> > > After staring at some drivers and the U-Boot linker script for a while
> > > I realized that the BSS section overlaps with the appended DTB before
> > > relocation. And indeed, mach-snapdragon/pinctrl-apq8016.c writes "GPIO_5"
> > > into "static char pin_name[MAX_PIN_NAME_LEN];" (= BSS) before relocation.
> > >
> > > Actually, arch/arm/lib/crt0_64.S says that BSS should not be used at all
> > > before relocation, because it's uninitialized and might corrupt other
> > > memory. I found several other commits where similar problems happened
> > > and it was usually fixed by moving the variables into the data section.
> > >
> > > So, I fixed the problem with the diff below and will include it together
> > > with the changes to drop all the LK-related code. Now everything seems
> > > fine. (I just wish this would have somehow been more obvious instead of
> > > the strange behavior...)
> > >
> > > Stephan
> > >
> > > diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > index 1042b564c3..7940c74287 100644
> > > --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c
> > > @@ -10,7 +10,7 @@
> > >  #include <common.h>
> > >
> > >  #define MAX_PIN_NAME_LEN 32
> > > -static char pin_name[MAX_PIN_NAME_LEN];
> > > +static char pin_name[MAX_PIN_NAME_LEN] __section(".data");
> > >  static const char * const msm_pinctrl_pins[] = {
> > >         "SDC1_CLK",
> > >         "SDC1_CMD",
> > >
> > Hi.
> > If I recall correctly, the signing tool only used the ELF sections, so
> > the appended DTB was deleted.
> > That's why I kept the "embedded DTB".
>
> Yes, it doesn't make sense to append the DTB to the ELF file.
>
> That's why "CONFIG_REMAKE_ELF" is useful, it builds a new ELF file
> where U-Boot + appended DTB is put into a single, new ELF section.
>
> > In your signing tool, you probably sign the complete file without
> > parsing the ELF.
>
> The "signature" actually consists out of additional ELF headers.
> There is no way to "sign" without parsing the ELF file. So my tool
> parses the ELF file just like signlk.
>
> Thanks!
> Stephan
Cool, thanks!