Hi Simon, On 23/01/2025 15:37, Simon Glass wrote: > Hi Caleb, > > On Fri, 17 Jan 2025 at 03:29, Caleb Connolly <[email protected]> > wrote: >> >> In fc37a73e6679 (fdt: Swap the signature for board_fdt_blob_setup()) >> there was a subtle change to the Snapdragon implementation, removing the >> assignment to gd->fdt_blob partway through the function. >> >> This breaks qcom_parse_memory() which was also called during >> board_fdt_blob_setup(). >> >> Since this was a strange (and seemingly unnecessary choice), take the >> chance to move this to the more typical dram_init() phase so that we >> don't depend on gd->fdt_blob being correct until after >> board_fdt_blob_setup() has finished. >> >> Fixes: fc37a73e6679 (fdt: Swap the signature for board_fdt_blob_setup()) >> Signed-off-by: Caleb Connolly <[email protected]> >> --- >> arch/arm/mach-snapdragon/board.c | 67 +++++++++++++++----------------- >> 1 file changed, 31 insertions(+), 36 deletions(-) >> >> diff --git a/arch/arm/mach-snapdragon/board.c >> b/arch/arm/mach-snapdragon/board.c >> index f1319df43147..fbb3d6e588e3 100644 >> --- a/arch/arm/mach-snapdragon/board.c >> +++ b/arch/arm/mach-snapdragon/board.c >> @@ -45,17 +45,8 @@ static struct { >> phys_addr_t start; >> phys_size_t size; >> } prevbl_ddr_banks[CONFIG_NR_DRAM_BANKS] __section(".data") = { 0 }; >> >> -int dram_init(void) >> -{ >> - /* >> - * gd->ram_base / ram_size have been setup already >> - * in qcom_parse_memory(). >> - */ >> - return 0; >> -} >> - >> static int ddr_bank_cmp(const void *v1, const void *v2) >> { >> const struct { >> phys_addr_t start; >> @@ -69,42 +60,22 @@ static int ddr_bank_cmp(const void *v1, const void *v2) >> >> return (res1->start >> 24) - (res2->start >> 24); >> } >> >> -/* This has to be done post-relocation since gd->bd isn't preserved */ >> -static void qcom_configure_bi_dram(void) >> -{ >> - int i; >> - >> - for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >> - gd->bd->bi_dram[i].start = prevbl_ddr_banks[i].start; >> - gd->bd->bi_dram[i].size = prevbl_ddr_banks[i].size; >> - } >> -} >> - >> -int dram_init_banksize(void) >> -{ >> - qcom_configure_bi_dram(); >> - >> - return 0; >> -} >> - >> static void qcom_parse_memory(void) >> { >> ofnode node; >> - const fdt64_t *memory; >> + const fdt64_t *memory = NULL; >> int memsize; >> phys_addr_t ram_end = 0; >> int i, j, banks; >> >> node = ofnode_path("/memory"); >> - if (!ofnode_valid(node)) { >> - log_err("No memory node found in device tree!\n"); >> - return; >> - } >> - memory = ofnode_read_prop(node, "reg", &memsize); >> - if (!memory) { >> - log_err("No memory configuration was provided by the >> previous bootloader!\n"); >> + if (ofnode_valid(node)) >> + memory = ofnode_read_prop(node, "reg", &memsize); >> + >> + if (!memory || !ofnode_valid(node)) { >> + panic("No memory configuration was provided by the previous >> bootloader!\n"); >> return; >> } >> >> banks = min(memsize / (2 * sizeof(u64)), >> (ulong)CONFIG_NR_DRAM_BANKS); >> @@ -134,8 +105,32 @@ static void qcom_parse_memory(void) >> debug("ram_base = %#011lx, ram_size = %#011llx, ram_end = >> %#011llx\n", >> gd->ram_base, gd->ram_size, ram_end); >> } >> >> +int dram_init(void) >> +{ >> + qcom_parse_memory(); >> + return 0; >> +} >> + >> +/* This has to be done post-relocation since gd->bd isn't preserved */ >> +static void qcom_configure_bi_dram(void) >> +{ >> + int i; >> + >> + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >> + gd->bd->bi_dram[i].start = prevbl_ddr_banks[i].start; >> + gd->bd->bi_dram[i].size = prevbl_ddr_banks[i].size; >> + } >> +} >> + >> +int dram_init_banksize(void) >> +{ >> + qcom_configure_bi_dram(); >> + >> + return 0; >> +} >> + >> static void show_psci_version(void) >> { >> struct arm_smccc_res res; >> >> @@ -173,9 +168,9 @@ int board_fdt_blob_setup(void **fdtp) >> ret = -EEXIST; >> } else { >> debug("Using external FDT\n"); >> /* So we can use it before returning */ >> - *fdtp = fdt; >> + gd->fdt_blob = *fdtp = fdt; > > Can you avoid assigning to gd->fdt_blob here? The intent is that this > is done by the caller. > >> } >> >> /* >> * Parse the /memory node while we're here, >> -- >> 2.48.0 >> > > I notice this is rejected in patchwork, but I don't see any discussion > as to why?
I discussed with Sam on IRC and took his patch instead since it fixes the same issue and enables some additional usecases. https://lore.kernel.org/u-boot/[email protected] Kind regards, > > Regards, > Simon -- // Caleb (they/them)
