Hey Caleb, On Wednesday, 22 January 2025 at 16:49, Caleb Connolly <[email protected]> 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 [email protected] > > --- > > 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 [email protected] > > > 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)
