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

[Stephan Gerhold <stephan@gerhold.net>]

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