[PATCH v2 2/2] mach-snapdragon: support parsing memory info from external FDT
Caleb Connolly
caleb.connolly at linaro.org
Wed Jan 22 16:49:25 CET 2025
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.
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)
More information about the U-Boot
mailing list