[PATCH v4 6/8] arm: dts: meson-gx-u-boot: add binman configuration for U-Boot SPL
Ferass El Hafidi
funderscore at postmarketos.org
Wed Oct 15 19:58:45 CEST 2025
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 <mjourdan at baylibre.com>
>> + * Copyright (c) 2023 Ferass El Hafidi <funderscore at postmarketos.org>
>> */
>>
>> / {
>> @@ -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
More information about the U-Boot
mailing list