'Ello Caleb, On Tuesday, 21 January 2025 at 13:33, Caleb Connolly <[email protected]> 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 [email protected] > > --- > > 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)
