[PATCH] mach-snapdragon: fix board_fdt_blob_setup()

[Caleb Connolly]

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 at 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 at 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

-- 
// Caleb (they/them)