[PATCH v2 2/2] mach-snapdragon: support parsing memory info from external FDT

[Sam Day]

Hey Caleb,

On Wednesday, 22 January 2025 at 16:49, Caleb Connolly <caleb.connolly at 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 at 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 at 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)