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

[Sam Day]

'Ello Caleb,

On Tuesday, 21 January 2025 at 13:33, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> 
> 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

No, I suppose not. We wouldn't want folks reading that last sentence to
experience an existential crisis, smear themselves in mud and run through
a supermarket completely naked. So I've removed it from the v2 patch :)

> 
> > Signed-off-by: Sam Day me at 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.

Thanks for the suggestion, I like that approach and agree that the overhead
introduced should be minimal, whilst giving us maximum compatibility across
the myriad ways a boot might take place.

I've already implemented and tested the requested changes in v2, which should
hit the list shortly.

Cheers,
Sam

> 
> > +
> > 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)
> 
> 
> --
> // Caleb (they/them)