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? Regards, Simon
