[PATCH 2/2] mach-snapdragon: support parsing memory info from external FDT

[Caleb Connolly]

On 20/01/2025 20:43, Sam Day wrote:
> Prior to this commit, board_fdt_blob_setup would decide which FDT to
> use, either the external one from the previous bootloader, or the
> internal one that was compiled into the binary.
> 
> This commit expands that behaviour a bit further, and introduces a
> (default-enabled) option to always parse the /memory node from the
> external FDT (if it's present and valid), even if we're going to
> continue the boot with an internal one.
> 
> The rationale here is that many (all?) of the upstream DTs for the
> msm8916 + sdm845 (and likely others) socs do not fully specify the /memory
> node(s), since it's typically expected that the bootloader has selected
> one of these from an Android boot.img and then filled in the memory
> info.
> 
> Caleb's original explanation in the commit that introduced
> qcom_parse_memory was dropped into a comment above that function, since
> this part of the early initialization is getting quite nuanced. We don't
> want uninitiated folks to see that code and run away screaming or give
> away all their possessions and move to a monastery or something.

Maaaybe this last sentence isn't super necessary :P
> 
> Signed-off-by: Sam Day <me at samcday.com>
> ---
>  arch/arm/mach-snapdragon/Kconfig | 13 +++++++++
>  arch/arm/mach-snapdragon/board.c | 57 +++++++++++++++++++++++++++-------------
>  2 files changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/mach-snapdragon/Kconfig b/arch/arm/mach-snapdragon/Kconfig
> index 976c0e35fcef29bafecbbd6ea04459de852df275..0feb65a916459160a4da9947a6d7224930d507b4 100644
> --- a/arch/arm/mach-snapdragon/Kconfig
> +++ b/arch/arm/mach-snapdragon/Kconfig
> @@ -45,4 +45,17 @@ config SYS_CONFIG_NAME
>  	  Based on this option include/configs/<CONFIG_SYS_CONFIG_NAME>.h header
>  	  will be used for board configuration.
>  
> +config SYS_MEMORY_FROM_EXTERNAL_FDT
> +	bool "Always parse /memory from external device tree, if it's provided"
> +	default y
> +	help
> +	  When booting with an internal DT, we'll always use that since it's
> +	  expected to be more accurate + up-to-date than whatever the previous
> +	  bootloader hands us.
> +
> +	  The notable exception here is the /memory node. With this config option
> +	  enabled, we'll initialize memory based on the information provided in the
> +	  external DT from the previous bootloader, even if we use the internal DT
> +	  for everything else.

I definitely understand why you made this a config option, but I think
we can make this make sense at runtime (admittedly with a little bit of
complexity).

What if we adjust qcom_parse_memory() to return an int, 0 on success and
-ENODATA IFF (If and only if) the memory node has only a single entry
with a size of 0.

Then we can call it first with the internal fdt, if it returns -ENODATA
and we have a valid external fdt then we can try that next.

This way, your usecase with the msm8916 device will still work as
expected (since it will gracefully fall back to the external fdt), but
if for some reason we have a device where there needs to be an external
fdt but we want to override the memory map we can still do so.

I think we can live with the small overhead here and optimise it in the
future if that becomes desirable, and avoid introducing a
device-specific config option.
> +
>  endif
> diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
> index 2ef936aab757c7045729a2dd91944f4f9bff917e..f637bce18546fb3a9fe389e07f457346ad5f95c8 100644
> --- a/arch/arm/mach-snapdragon/board.c
> +++ b/arch/arm/mach-snapdragon/board.c
> @@ -88,6 +88,29 @@ int dram_init_banksize(void)
>  	return 0;
>  }
>  
> +/**
> + * The generic memory parsing code in U-Boot lacks a few things that we
> + * need on Qualcomm:
> + *
> + * 1. It sets gd->ram_size and gd->ram_base to represent a single memory block
> + * 2. setup_dest_addr() later relocates U-Boot to ram_base + ram_size, the end
> + *    of that first memory block.
> + *
> + * This results in all memory beyond U-Boot being unusable in Linux when booting
> + * with EFI.
> + *
> + * Since the ranges in the memory node may be out of order, the only way for us
> + * to correctly determine the relocation address for U-Boot is to parse all
> + * memory regions and find the highest valid address.
> + *
> + * We can't use fdtdec_setup_memory_banksize() since it stores the result in
> + * gd->bd, which is not yet allocated.

Thanks for adding this!

Kind regards,
> + *
> + * This function requires a pointer to the specific FDT that we should parse
> + * for its /memory block. This is because when SYS_MEMORY_FROM_EXTERNAL_FDT is
> + * enabled, we might be looking at the FDT provided by the previous bootloader,
> + * rather than our internal (gd->fdt_blob) one.
> + */
>  static void qcom_parse_memory(const void *fdt)
>  {
>  	int offset;
> @@ -153,13 +176,13 @@ static void show_psci_version(void)
>   */
>  int board_fdt_blob_setup(void **fdtp)
>  {
> -	struct fdt_header *fdt;
> +	struct fdt_header *external_fdt, *internal_fdt;
>  	bool internal_valid, external_valid;
> -	int ret = 0;
>  
> -	fdt = (struct fdt_header *)get_prev_bl_fdt_addr();
> -	external_valid = fdt && !fdt_check_header(fdt);
> -	internal_valid = !fdt_check_header(*fdtp);
> +	internal_fdt = (struct fdt_header *)*fdtp;
> +	external_fdt = (struct fdt_header *)get_prev_bl_fdt_addr();
> +	external_valid = external_fdt && !fdt_check_header(external_fdt);
> +	internal_valid = !fdt_check_header(internal_fdt);
>  
>  	/*
>  	 * There is no point returning an error here, U-Boot can't do anything useful in this situation.
> @@ -167,24 +190,22 @@ int board_fdt_blob_setup(void **fdtp)
>  	 */
>  	if (!internal_valid && !external_valid)
>  		panic("Internal FDT is invalid and no external FDT was provided! (fdt=%#llx)\n",
> -		      (phys_addr_t)fdt);
> +		      (phys_addr_t)external_fdt);
> +
> +	if (IS_ENABLED(CONFIG_SYS_MEMORY_FROM_EXTERNAL_FDT) && external_valid)
> +		/* Explicitly use the external FDT to configure memory */
> +		qcom_parse_memory(external_fdt);
> +	else
> +		qcom_parse_memory(internal_fdt);
>  
>  	if (internal_valid) {
>  		debug("Using built in FDT\n");
> -		ret = -EEXIST;
> -	} else {
> -		debug("Using external FDT\n");
> -		/* So we can use it before returning */
> -		*fdtp = fdt;
> +		return -EEXIST;
>  	}
>  
> -	/*
> -	 * Parse the /memory node while we're here,
> -	 * this makes it easy to do other things early.
> -	 */
> -	qcom_parse_memory(*fdtp);
> -
> -	return ret;
> +	debug("Using external FDT\n");
> +	*fdtp = external_fdt;
> +	return 0;
>  }
>  
>  void reset_cpu(void)
> 

-- 
// Caleb (they/them)