[PATCH 2/2] fdt: Swap the signature for board_fdt_blob_setup()

[Caleb Connolly]

Hi Simon,

On 21/10/2024 13:42, Simon Glass wrote:
> This returns a devicetree and updates a parameter with an error code.
> Swap it, since this fits better with the way U-Boot normally works. It
> also (more easily) allows leaving the existing pointer unchanged.
> 
> No yaks were harmed in this change, but there is a very small code-size
> reduction.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---

...

> diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
> index 2ab2ceb5138..7a7a36831c3 100644
> --- a/arch/arm/mach-snapdragon/board.c
> +++ b/arch/arm/mach-snapdragon/board.c
> @@ -147,12 +147,11 @@ static void show_psci_version(void)
>    * or for supporting quirky devices where it's easier to leave the downstream DT in place
>    * to improve ABL compatibility. Otherwise, we use the DT provided by ABL.
>    */
> -void *board_fdt_blob_setup(int *err)
> +int board_fdt_blob_setup(void **fdtp)
>   {
>   	struct fdt_header *fdt;
>   	bool internal_valid, external_valid;
>   
> -	*err = 0;
>   	fdt = (struct fdt_header *)get_prev_bl_fdt_addr();
>   	external_valid = fdt && !fdt_check_header(fdt);
>   	internal_valid = !fdt_check_header(gd->fdt_blob);
> @@ -170,7 +169,7 @@ void *board_fdt_blob_setup(int *err)
>   	} else {
>   		debug("Using external FDT\n");
>   		/* So we can use it before returning */
> -		gd->fdt_blob = fdt;
> +		*fdtp = fdt;

I believe this is a breaking change. The qcom_parse_memory() call below 
expects gd->fdt_blob to point to the in-use FDT. This is a bit of a 
hack, but doing memory parsing this early simplifies things for us.

Additionally, this change doesn't make the function return -EEXIST when 
it should.

...

> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 555c9520379..9e36acc7e9b 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -1191,11 +1191,13 @@ int fdtdec_resetup(int *rescan);
>    *
>    * The existing devicetree is available at gd->fdt_blob
>    *
> - * @err: 0 on success, -EEXIST if the devicetree is already correct, or other
> + * @fdtp: Existing devicetree blob pointer; update this and return 0 if a

It doesn't looks like this is initialised before calling 
board_fdt_blob_setup()?

Kind regards,
> + * different devicetree should be used
> + * Return: 0 on success, -EEXIST if the devicetree is already correct, 0 to use
> + * *@fdtp as the new devicetree, or other
>    * internal error code if we fail to setup a DTB
> - * @returns new devicetree blob pointer
>    */
> -void *board_fdt_blob_setup(int *err);
> +int board_fdt_blob_setup(void **fdtp);
>   
>   /*
>    * Decode the size of memory
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 60e28173c03..e876b17f9ad 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1706,11 +1706,16 @@ int fdtdec_setup(void)
>   
>   	/* Allow the board to override the fdt address. */
>   	if (IS_ENABLED(CONFIG_OF_BOARD)) {
> -		gd->fdt_blob = board_fdt_blob_setup(&ret);
> -		if (!ret)
> +		void *blob;
> +
> +		ret = board_fdt_blob_setup(&blob);
> +		if (ret) {
> +			if (ret != -EEXIST)
> +				return ret;
> +		} else {
>   			gd->fdt_src = FDTSRC_BOARD;
> -		else if (ret != -EEXIST)
> -			return ret;
> +			gd->fdt_blob = blob;
> +		}
>   	}
>   
>   	/* Allow the early environment to override the fdt address */

-- 
// Caleb (they/them)