Re: [PATCH v4 6/8] arm: dts: meson-gx-u-boot: add binman configuration for U-Boot SPL

Hi Ferass,

On 10/15/25 6:25 PM, Ferass El Hafidi wrote:
> Add binman configuration to meson-gx-u-boot.dtsi to automate building
> bootable images using amlimage.
> 
> Signed-off-by: Ferass El Hafidi <[email protected]>
> Reviewed-by: Neil Armstrong <[email protected]>

Quick drive-by comments.

> ---
>   arch/arm/dts/meson-gx-u-boot.dtsi | 142 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 142 insertions(+)
> 
> diff --git a/arch/arm/dts/meson-gx-u-boot.dtsi b/arch/arm/dts/meson-gx-u-boot.dtsi
> index 9e0620f395e..d05f869c06e 100644
> --- a/arch/arm/dts/meson-gx-u-boot.dtsi
> +++ b/arch/arm/dts/meson-gx-u-boot.dtsi
> @@ -2,6 +2,7 @@
>   /*
>    * Copyright (c) 2019 BayLibre, SAS.
>    * Author: Maxime Jourdan <[email protected]>
> + * Copyright (c) 2023 Ferass El Hafidi <[email protected]>
>    */
>   
>   / {
> @@ -15,6 +16,16 @@
>   	soc {
>   		bootph-all;
>   	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +

I don't believe this is required for binman so please split into its own 
commit with justification.

> +#if defined(CONFIG_BINMAN)
> +	binman: binman {
> +		multiple-images;
> +	};
> +#endif
>   };
>   
>   &vpu {
> @@ -30,3 +41,134 @@
>   	      <0x0 0xc883c000 0x0 0x1000>;
>   	reg-names = "hdmitx", "hhi";
>   };
> +
> +#if defined(CONFIG_MESON_GX) && defined(CONFIG_BINMAN)

Isn't MESON_GX implied since we are in meson-gx-u-boot.dtsi?

> +/* binman configuration on GXBB and GXL */
> +
> +#if defined(CONFIG_MESON_GXBB)
> +#define BL31_ADDR 0x10100000
> +#else /* GXL */
> +#define BL31_ADDR 0x05100000
> +#endif
> +

First question is... can't this be derived from the binary already?

On Rockchip we do

fit,load;
fit,entry;
fit,data;

for OP-TEE OS and TF-A though we do split the binaries into multiple 
nodes (see @atf-SEQ and @tee-SEQ). I'm not sure if those properties are 
applicable to only tee and atf nodes without a node for each segment, if 
it isn't...

I would highly recommend against doing this.

I would instead have a

arch/arm/dts/meson-gxbb-u-boot.dtsi with

&fit {
     atf {
         entry = <0x10100000>;
         load = <0x10100000>;
     };
};

and have the default entry and load be 0x05100000 if that really is a 
sensible default (otherwise put none in meson-gx-u-boot.dtsi and declare 
it in each meson-gxXXX-u-boot.dtsi). Then your boards would include 
meson-gxbb-u-boot.dtsi instead of meson-gx-u-boot.dtsi (and the former 
DTSI would include the latter DTSI).

The benefit is that it makes it much clearer what would be required to 
support a new family/SoC of the Meson GX family. Or what exactly could 
differ between families. But also keeps the main -u-boot.dtsi readable 
(which is not that easy to do, see our horrendous rockchip-u-boot.dtsi :( )

> +/*
> + * On GXBB the base address of the SCP firmware doesn't matter as SPL will
> + * send the firmware to the SCP anyway, and can get the base address from the
> + * FIT. On GXL it matters, as BL31 is supposed to send the firmware, so set the
> + * base address to what GXL BL2 would load the binary to.
> + */
> +#define  SCP_ADDR 0x13c0000
> +
> +&binman {
> +	u-boot-amlogic {
> +		filename = "u-boot-meson-with-spl.bin";
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			filename = "spl/u-boot-spl-signed.bin";
> +#if defined(CONFIG_MESON_GXBB)
> +			args = "-n", "gxbb", "-T", "amlimage";
> +#elif defined(CONFIG_MESON_GXL)
> +			args = "-n", "gxl", "-T", "amlimage";
> +#endif

Same here as suggested for the load address of TF-A.

On Rockchip we use CONFIG_SYS_SOC but I don't think you can on Amlogic.

You could set SYS_SOC depending on MESON_GXBB/MESON_GXL/... in 
arch/arm/mach-meson/Kconfig and be done with it, but there are some 
side-effects (read SYS_SOC doc).

Another way is to add

&binman {
     mkimage {
         args = "-n", "gxbb", "-T", "amlimage";
     };
};

to arch/arm/dts/meson-gxbb-u-boot.dtsi.

> +
> +			blob {
> +				size = <0xb000>; /* The BootROM loads 48K max (header is 4K) */
> +				filename = "spl/u-boot-spl.bin";
> +			};

I'm not sure this actually works? It would rely on a binary located at 
spl/u-boot-spl.bin, which binman doesn't seem to be generating? Either 
we;re missing something to generate it with binman, or something 
external to binman generates it?

Typically it is generated with:

u-boot-spl {
};

?

If you're trying to enforce a max size, then we have Kconfig symbols for 
those, probably SPL_MAX_SIZE and then I believe binman enforces those 
one way or another. If it doesn't, then many boards and architecture 
have a problem and we should fix that so you can rely on it.

> +		};
> +
> +		fit: fit {
> +			description = "ATF and U-Boot images";
> +			#address-cells = <1>;
> +			fit,fdt-list = "of-list";
> +			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> +			fit,align = <512>;
> +			offset = <CONFIG_SPL_PAD_TO>;
> +
> +			images {
> +				u-boot {
> +					description = "U-Boot";
> +					type = "standalone";
> +					os = "u-boot";
> +					arch = "arm64";
> +					compression = "none";
> +					load = <CONFIG_TEXT_BASE>;
> +					entry = <CONFIG_TEXT_BASE>;
> +
> +					u-boot-nodtb {
> +					};

Why no hash for U-Boot?

> +				};
> +
> +				atf {
> +					description = "ARM Trusted Firmware";
> +					type = "firmware";
> +					os = "arm-trusted-firmware";
> +					arch = "arm64";
> +					compression = "none";
> +					load = <BL31_ADDR>;
> +					entry = <BL31_ADDR>;
> +
> +					atf-bl31 {
> +						filename = "bl31.bin";

I believe this is already the case if you set BL31 environment variable 
to bl31.bin. I would simply remove it and let the user decide what's 
best for them. For binman this comes from the -a atf-bl31-path=${BL31} 
argument.

The benefit is that you can now point to a BL31 outside of U-Boot's 
tree, which is super convenient for debugging TF-A, or with build 
systems like Yocto or Buildroot.

> +					};
> +					hash {
> +						algo = "sha256";
> +					};

This means your boards all need to have CONFIG_SPL_SHA256 if 
CONFIG_SPL_FIT_SIGNATURE is provided otherwise the hash cannot be 
verified. Please check if that is the case (enforced at the Kconfig 
symbol-level would be best).

> +				};
> +
> +				scp {
> +					description = "SCP firmware";
> +					type = "scp";
> +					arch = "arm"; /* The Cortex-M core is used as SCP */
> +					compression = "none";
> +					load = <SCP_ADDR>;
> +
> +					scp {
> +						filename = "scp.bin";
> +					};

I would recommend to attempt mimicking what's been done for atf-bl31 
(then you need to add a new argument to the lists of arguments for 
binman in the root Makefile). See above for benefits.

> +					hash {
> +						/*
> +						 * The hash is used by the SCP and passed to it
> +						 * by U-Boot SPL.
> +						 */
> +						algo = "sha256";
> +					};
> +				};
> +
> +				@fdt-SEQ {
> +					description = "NAME";
> +					type = "flat_dt";
> +					compression = "none";

why no hash for FDTs?

> +				};
> +
> +			};
> +			configurations {
> +				default = "@config-DEFAULT-SEQ";
> +				@config-SEQ {
> +					description = "NAME.dtb";
> +					fdt = "fdt-SEQ";
> +					firmware = "atf";
> +					loadables = "scp", "u-boot";
> +				};
> +			};
> +		};
> +	};
> +};
> +

Is the below really related to adding binman configuration???

> +&vpu {
> +	/delete-property/ bootph-all;
> +};
> +
> +&apb {
> +	bootph-all;
> +};
> +
> +&sd_emmc_b {
> +	bootph-all;
> +};
> +
> +&sd_emmc_c {
> +	bootph-all;
> +};
> +#endif
> 

Cheers,
Quentin