[PATCH] mach-snapdragon: fix board_fdt_blob_setup()
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; }
/* * Parse the /memory node while we're here,
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-98dfc...
Kind regards,
Regards, Simon
participants (2)
-
Caleb Connolly -
Simon Glass