'Ello Caleb,
On Tuesday, 21 January 2025 at 13:33, Caleb Connolly caleb.connolly@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@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:
- It sets gd->ram_size and gd->ram_base to represent a single memory block
- 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)