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

Hi Quentin,

On Wed Oct 15, 2025 at 5:11 PM UTC, Quentin Schulz wrote:
> <...>
>> 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.
>

Sounds good to me, will do.

>> +#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?
>

Yeah, it is, so this is redundant.

>> +/* 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).
>

Will do.

> 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 :( )
>

This patch series actually already adds support for all SoCs of the Meson GX
family.  Newer SoCs are too different from GX SoCs and need their own
-u-boot.dtsi (and init and all)...

>> +/*
>> + * 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.
>

I think this is what I'll go with. Thanks!

>> +
>> +			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?
>

spl/u-boot-spl.bin is generated on build-time, it's the U-Boot SPL
binary. And that does work according to my testing...

> Typically it is generated with:
>
> u-boot-spl {
> };
>

...but I think this is much cleaner, so I'll go with that.

> ?
>
> 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.
>

I did know of SPL_MAX_SIZE... Might have just overlooked it though.
I think this should be the proper way to do this, yeah.

>> +		};
>> +
>> +		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.
>

You can already point to a BL31 outside U-Boot's tree, and this is what
I've been always doing when testing.

But I think having it check for a bl31.bin inside the tree when $BL31 is
not set is a sane default IMO .. but correct me if I'm wrong and it's
considered poor practise.

>> +					};
>> +					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).
>

ACK.

>> +				};
>> +
>> +				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.
>

One can already `export SCP=../path/to/scp/firmware` and have binman use
that.

>> +					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???
>

I'll split this to a new commit.

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

Thanks for the review!

Best regards,
Ferass