[PATCH] mach-snapdragon: fix board_fdt_blob_setup()

[Simon Glass]

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?

Regards,
Simon