| Message ID | 20250117102734.3725009-2-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mach-snapdragon: fix board_fdt_blob_setup() | expand |
Hi Caleb, On Fri, 17 Jan 2025 at 03:29, Caleb Connolly <caleb.connolly@linaro.org> 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 <caleb.connolly@linaro.org> > --- > 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
Hi Simon, On 23/01/2025 15:37, Simon Glass wrote: > Hi Caleb, > > On Fri, 17 Jan 2025 at 03:29, Caleb Connolly <caleb.connolly@linaro.org> 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 <caleb.connolly@linaro.org> >> --- >> 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/20250122-qcom-parse-memory-updates-v2-0-98dfcac821d7@samcday.com Kind regards, > > Regards, > Simon
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; } /* * Parse the /memory node while we're here,
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 <caleb.connolly@linaro.org> --- arch/arm/mach-snapdragon/board.c | 67 +++++++++++++++----------------- 1 file changed, 31 insertions(+), 36 deletions(-)