Hey Caleb,

On Wednesday, 22 January 2025 at 16:49, Caleb Connolly caleb.connolly@linaro.org wrote:

Hi Sam,

Thanks for re-spinning this, one small issue below but otherwise LGTM.

I'm picking up your first patch, so you only need to re-send this one.

On 22/01/2025 11:27, Sam Day wrote:

...

qcom_parse_memory is updated to return a -ENODATA error if the passed FDT does not contain a /memory node, or that node is incomplete (size=0)

board_fdt_blob_setup first tries to call qcom_parse_memory with the internal FDT (if present+valid). If that fails, it tries again with the external FDT (again, if present+valid).

When booting with an internal FDT from upstream, it's likely that this change results in a slight performance hit, since virtually all upstream qcom DTs lack a fully specified memory node. The impact should be negligible, though.

qcom_parse_memory was given a detailed docstring adapted from Caleb's original commit message that introduced the function.

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

arch/arm/mach-snapdragon/board.c | 93 +++++++++++++++++++++++++++------------- 1 file changed, 63 insertions(+), 30 deletions(-)

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

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

• • 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.
• • @fdt: FDT blob to parse /memory node from
• • Return: 0 on success or -ENODATA if /memory node is missing or incomplete
• */

+static int qcom_parse_memory(const void *fdt) { int offset; const fdt64_t *memory; @@ -97,16 +119,12 @@ static void qcom_parse_memory(const void *fdt) int i, j, banks;

offset = fdt_path_offset(fdt, "/memory");

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

• if (offset < 0)
• return -ENODATA;

memory = fdt_getprop(fdt, offset, "reg", &memsize);

• if (!memory) {
• log_err("No memory configuration was provided by the previous bootloader!\n");
• return;
• }

• if (!memory)
• return -ENODATA;

banks = min(memsize / (2 * sizeof(u64)), (ulong)CONFIG_NR_DRAM_BANKS);

@@ -119,7 +137,6 @@ static void qcom_parse_memory(const void fdt) for (i = 0, j = 0; i < banks * 2; i += 2, j++) { prevbl_ddr_banks[j].start = get_unaligned_be64(&memory[i]); prevbl_ddr_banks[j].size = get_unaligned_be64(&memory[i + 1]);

• / SM8650 boards sometimes have empty regions! */

if (!prevbl_ddr_banks[j].size) { j--; continue; @@ -127,13 +144,16 @@ static void qcom_parse_memory(const void *fdt) ram_end = max(ram_end, prevbl_ddr_banks[j].start + prevbl_ddr_banks[j].size); }

• if (!j)
• return -ENODATA;

I think this should be if (!banks || !prevbl_ddr_banks[0].size)

That way it will properly detect the typical case where the source dts has a single entry with a 0 size.

Hmm, I did test this on my samsung-a5, booting with an internal FDT with a zero size memory node and it worked okay. If there's only one node, then j gets incremented in the single run of the previous loop, and then decremented because the size == 0, thus exiting the loop with j==0, which is being checked here.

Hmm, I did test this on my samsung-a5, booting an internal FDT with a zero size memory node and it worked okay. If there's only one node, then j gets incremented in the single run of the previous loop, and then decremented because size == 0, thus exiting the loop with j==0, which is being checked here.

But, your suggestion is more readable and semantically correct, as well as being more robust against the inevitable future refactorings of this method ;) I'll submit v3 with just this patch in a mo'.

Cheers, -Sam

With that change:

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

Kind regards,

...

/* Sort our RAM banks -_- */ qsort(prevbl_ddr_banks, banks, sizeof(prevbl_ddr_banks[0]), ddr_bank_cmp);

gd->ram_base = prevbl_ddr_banks[0].start; gd->ram_size = ram_end - gd->ram_base;

• debug("ram_base = %#011lx, ram_size = %#011llx, ram_end = %#011llx\n",
• gd->ram_base, gd->ram_size, ram_end);

• return 0;

}

static void show_psci_version(void) @@ -153,13 +173,14 @@ 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;

• int ret = -ENODATA;

• 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 +188,36 @@ 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);
• / Prefer memory information from internal DT if it's present /
• if (internal_valid)
• ret = qcom_parse_memory(internal_fdt);
• if (ret < 0 && external_valid) {
• / No internal FDT or it lacks a proper /memory node.
• • The previous bootloader handed us something, let's try that.
• */
• if (internal_valid)
• debug("No memory info in internal FDT, falling back to external\n");
• ret = qcom_parse_memory(external_fdt);
• }
• if (ret < 0)
• panic("No valid memory ranges found!\n");
• debug("ram_base = %#011lx, ram_size = %#011llx\n",
• gd->ram_base, gd->ram_size);

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)

-- // Caleb (they/them)