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

Stephan Gerhold
2021-07-03T21:42:53Z

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