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

[Stephan Gerhold <stephan@gerhold.net>]

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