On 20/01/2025 20:43, Sam Day wrote:

commit fc37a73e6679 ("fdt: Swap the signature for board_fdt_blob_setup()") introduced a subtle change to the Snapdragon implementation, removing the assignment to gd->fdt_blob partway through the function.

This breaks qcom_parse_memory() which was also called during board_fdt_blob_setup().

The underlying issue here is that qcom_parse_memory is using the of_ api to traverse a devicetree, which relies on the fdt_blob in global data.

Rather than relying on this subtle behaviour, explicitly pass the FDT that should be consulted for a /memory node.

Using the OF API is typically preferable because it's easier to read, but using the lower level fdt_ methods instead here doesn't add too much complexity, I think.

Finally, a minor tweak was made to board_fdt_blob_setup to use the passed fdt blob pointer instead of gd->fdt_blob, which removes the last of the references to global data in this area.

Fixes: fc37a73e6679 (fdt: Swap the signature for board_fdt_blob_setup()) Signed-off-by: Sam Day me@samcday.com

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

Thanks so much for sending this! I'm pretty sure the whole reason we called qcom_parse_memory() from board_fdt_blob_setup() originally was to allow for the usecase you add in patch 2, so it's nice to see that fulfilled :>

---

arch/arm/mach-snapdragon/board.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c index f1319df4314783622bb5f4f2410d1d4fd28a1c83..2ef936aab757c7045729a2dd91944f4f9bff917e 100644 --- a/arch/arm/mach-snapdragon/board.c +++ b/arch/arm/mach-snapdragon/board.c @@ -88,20 +88,21 @@ int dram_init_banksize(void) return 0; }

-static void qcom_parse_memory(void) +static void qcom_parse_memory(const void *fdt) {

• ofnode node;

• int offset; const fdt64_t *memory; int memsize; phys_addr_t ram_end = 0; int i, j, banks;

• node = ofnode_path("/memory");
• if (!ofnode_valid(node)) {

• offset = fdt_path_offset(fdt, "/memory");
• if (offset < 0) { log_err("No memory node found in device tree!\n"); return; }

• memory = ofnode_read_prop(node, "reg", &memsize);

• memory = fdt_getprop(fdt, offset, "reg", &memsize); if (!memory) { log_err("No memory configuration was provided by the previous bootloader!\n"); return;

@@ -158,7 +159,7 @@ int board_fdt_blob_setup(void **fdtp)

fdt = (struct fdt_header *)get_prev_bl_fdt_addr(); external_valid = fdt && !fdt_check_header(fdt);

• internal_valid = !fdt_check_header(gd->fdt_blob);

• internal_valid = !fdt_check_header(*fdtp);

  /*

  • There is no point returning an error here, U-Boot can't do anything useful in this situation.

@@ -181,7 +182,7 @@ int board_fdt_blob_setup(void **fdtp) * Parse the /memory node while we're here, * this makes it easy to do other things early. */

• qcom_parse_memory();

• qcom_parse_memory(*fdtp);

  return ret;

}

On 20/01/2025 20:43, Sam Day wrote:

Prior to this commit, board_fdt_blob_setup would decide which FDT to use, either the external one from the previous bootloader, or the internal one that was compiled into the binary.

This commit expands that behaviour a bit further, and introduces a (default-enabled) option to always parse the /memory node from the external FDT (if it's present and valid), even if we're going to continue the boot with an internal one.

The rationale here is that many (all?) of the upstream DTs for the msm8916 + sdm845 (and likely others) socs do not fully specify the /memory node(s), since it's typically expected that the bootloader has selected one of these from an Android boot.img and then filled in the memory info.

Caleb's original explanation in the commit that introduced qcom_parse_memory was dropped into a comment above that function, since this part of the early initialization is getting quite nuanced. We don't want uninitiated folks to see that code and run away screaming or give away all their possessions and move to a monastery or something.

Maaaybe this last sentence isn't super necessary :P

Signed-off-by: Sam Day me@samcday.com

arch/arm/mach-snapdragon/Kconfig | 13 +++++++++ arch/arm/mach-snapdragon/board.c | 57 +++++++++++++++++++++++++++------------- 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-snapdragon/Kconfig b/arch/arm/mach-snapdragon/Kconfig index 976c0e35fcef29bafecbbd6ea04459de852df275..0feb65a916459160a4da9947a6d7224930d507b4 100644 --- a/arch/arm/mach-snapdragon/Kconfig +++ b/arch/arm/mach-snapdragon/Kconfig @@ -45,4 +45,17 @@ config SYS_CONFIG_NAME Based on this option include/configs/<CONFIG_SYS_CONFIG_NAME>.h header will be used for board configuration.

+config SYS_MEMORY_FROM_EXTERNAL_FDT

• bool "Always parse /memory from external device tree, if it's provided"
• default y
• help

•  When booting with an internal DT, we'll always use that since it's
  

•  expected to be more accurate + up-to-date than whatever the previous
  

•  bootloader hands us.
  

•  The notable exception here is the /memory node. With this config option
  

•  enabled, we'll initialize memory based on the information provided in the
  

•  external DT from the previous bootloader, even if we use the internal DT
  

•  for everything else.
  

I definitely understand why you made this a config option, but I think we can make this make sense at runtime (admittedly with a little bit of complexity).

What if we adjust qcom_parse_memory() to return an int, 0 on success and -ENODATA IFF (If and only if) the memory node has only a single entry with a size of 0.

Then we can call it first with the internal fdt, if it returns -ENODATA and we have a valid external fdt then we can try that next.

This way, your usecase with the msm8916 device will still work as expected (since it will gracefully fall back to the external fdt), but if for some reason we have a device where there needs to be an external fdt but we want to override the memory map we can still do so.

I think we can live with the small overhead here and optimise it in the future if that becomes desirable, and avoid introducing a device-specific config option.

endif diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c index 2ef936aab757c7045729a2dd91944f4f9bff917e..f637bce18546fb3a9fe389e07f457346ad5f95c8 100644 --- a/arch/arm/mach-snapdragon/board.c +++ b/arch/arm/mach-snapdragon/board.c @@ -88,6 +88,29 @@ int dram_init_banksize(void) return 0; }

+/**

• • The generic memory parsing code in U-Boot lacks a few things that we
• • need on Qualcomm:
• • 1. It sets gd->ram_size and gd->ram_base to represent a single memory block
• • 2. setup_dest_addr() later relocates U-Boot to ram_base + ram_size, the end
• • of that first memory block.
• • This results in all memory beyond U-Boot being unusable in Linux when booting
• • with EFI.
• • Since the ranges in the memory node may be out of order, the only way for us
• • to correctly determine the relocation address for U-Boot is to parse all
• • memory regions and find the highest valid address.
• • We can't use fdtdec_setup_memory_banksize() since it stores the result in
• • gd->bd, which is not yet allocated.

Thanks for adding this!

Kind regards,

• • This function requires a pointer to the specific FDT that we should parse
• • for its /memory block. This is because when SYS_MEMORY_FROM_EXTERNAL_FDT is
• • enabled, we might be looking at the FDT provided by the previous bootloader,
• • rather than our internal (gd->fdt_blob) one.
• */

static void qcom_parse_memory(const void *fdt) { int offset; @@ -153,13 +176,13 @@ static void show_psci_version(void) */ int board_fdt_blob_setup(void **fdtp) {

• struct fdt_header *fdt;

• struct fdt_header *external_fdt, *internal_fdt; bool internal_valid, external_valid;

• int ret = 0;

• fdt = (struct fdt_header *)get_prev_bl_fdt_addr();

• external_valid = fdt && !fdt_check_header(fdt);

• internal_valid = !fdt_check_header(*fdtp);

• internal_fdt = (struct fdt_header *)*fdtp;

• external_fdt = (struct fdt_header *)get_prev_bl_fdt_addr();

• external_valid = external_fdt && !fdt_check_header(external_fdt);

• internal_valid = !fdt_check_header(internal_fdt);

  /*

  • There is no point returning an error here, U-Boot can't do anything useful in this situation.

@@ -167,24 +190,22 @@ int board_fdt_blob_setup(void **fdtp) */ if (!internal_valid && !external_valid) panic("Internal FDT is invalid and no external FDT was provided! (fdt=%#llx)\n",

•       (phys_addr_t)fdt);
  

•       (phys_addr_t)external_fdt);
  

• if (IS_ENABLED(CONFIG_SYS_MEMORY_FROM_EXTERNAL_FDT) && external_valid)

• /* Explicitly use the external FDT to configure memory */
  

• qcom_parse_memory(external_fdt);
  

• else

• qcom_parse_memory(internal_fdt);
  
  

  if (internal_valid) { debug("Using built in FDT\n");

• ret = -EEXIST;
  

• } else {

• debug("Using external FDT\n");
  

• /* So we can use it before returning */
  

• *fdtp = fdt;
  

• return -EEXIST;
  

  }

• /*

• * Parse the /memory node while we're here,
  

• * this makes it easy to do other things early.
  

• */
  

• qcom_parse_memory(*fdtp);
• return ret;

• debug("Using external FDT\n");
• *fdtp = external_fdt;
• return 0;

}

void reset_cpu(void)