On Sun, Jul 4, 2021 at 12:42 AM Stephan Gerhold <step...@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".
In your signing tool, you probably sign the complete file without
parsing the ELF.
Thanks,
Ramon.
