Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

Quentin Schulz
2022-07-26T09:08:34Z

Hi Xavier,

On 7/25/22 19:33, Xavier Drudis Ferran wrote:
> El Mon, Jul 25, 2022 at 07:29:53PM +0200, Xavier Drudis Ferran deia:
>>
>> I copy here the rockchip-u-boot.dtsi file and then 2 patches on top of yours.
>>
>
> Sorry I copied a dirty version that din't work. The patches were correct, the dtsi wasn't.
>
>> #else
>> collection {
>> content = <&/binman/itb>;
>
> It doesn't like this phandles nor just &itb.
>
> These patches were the real thing. Sorry for the noise.
>
>
>>
>> >From 0f9cf7452a62268ec5978c80f46bf9323a269630 Mon Sep 17 00:00:00 2001
>> From: Xavier Drudis Ferran <xdrudis@tinet.cat>
>> Date: Mon, 25 Jul 2022 17:35:27 +0200
>> Subject: [PATCH] Align the fit images that binman produces to 8 bytes.
>>
>> In commit 570c4636808 ("Makefile: Align fit-dtb.blob and u-boot.itb by
>> 64bits") Michal Simek claims that according to the device tree spec,
>> some things should be aligned to 4 bytes and some to 8, so passes -B 8
>> to mkimage. Do the same when using binman so that we can try to
>> replace make_fit_atf.py .
>>
>> Should this be optional? I haven't found any uses of split-elf that I
>> might be breaking, and Marek said it's from dt spec, so it should be
>> always required. So I'm hard coding it until the need for being
>> optional arises.

Makes sense to me looking at the current Makefile implementation for
u-boot.itb mkimage flags.

You could also have a fit,align = <8>; property instead of hardcoding
it. The issue is that this flag seems to be added only for u-boot.itb
and fit-dtb.blob. I assume there are usecases outside of those two
binaries where the user does not want the fit header to be aligned (or
don't need it).

>> ---
>> tools/binman/btool/mkimage.py | 4 +++-
>> tools/binman/etype/fit.py | 1 +
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
>> index c85bfe053c..d614d72d62 100644
>> --- a/tools/binman/btool/mkimage.py
>> +++ b/tools/binman/btool/mkimage.py
>> @@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
>>
>> # pylint: disable=R0913
>> def run(self, reset_timestamp=False, output_fname=None, external=False,
>> - pad=None, version=False):
>> + pad=None, version=False, align=None):
>> """Run mkimage
>>
>> Args:
>> @@ -36,6 +36,8 @@ class Bintoolmkimage(bintool.Bintool):
>> version: True to get the mkimage version
>> """
>> args = []
>> + if align:
>> + args += ['-B', f'{align:x}']
>> if external:
>> args.append('-E')
>> if pad:
>> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
>> index 12306623af..7b99b83fa3 100644
>> --- a/tools/binman/etype/fit.py
>> +++ b/tools/binman/etype/fit.py
>> @@ -420,6 +420,7 @@ class Entry_fit(Entry_section):
>> ext_offset = self._fit_props.get('fit,external-offset')
>> if ext_offset is not None:
>> args = {
>> + 'align': 8,
>> 'external': True,
>> 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
>> }
>> --
>> 2.20.1
>>
>> >From 9fc65a2eb55f742dd805ed96e3d2028b20078a03 Mon Sep 17 00:00:00 2001
>> From: Xavier Drudis Ferran <xdrudis@tinet.cat>
>> Date: Mon, 25 Jul 2022 18:01:24 +0200
>> Subject: [PATCH] Replace make_fit_atf.py with binman.
>>
>> This boots my Rock Pi 4B, but likely needs more generalisation to
>> cover more boards and configurations.
>> ---
>> Makefile | 3 --
>> arch/arm/dts/rockchip-u-boot.dtsi | 70 ++++++++++++++++++++++++++++++
>> configs/rock-pi-4-rk3399_defconfig | 1 +
>> 3 files changed, 71 insertions(+), 3 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 279aeacee3..ad739ef357 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -997,9 +997,6 @@ endif
>>
>> ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL),yy)
>> # Binman image dependencies
>> -ifeq ($(CONFIG_ARM64),y)
>> -INPUTS-y += u-boot.itb
>> -endif
>> else
>> INPUTS-y += u-boot.img
>> endif
>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>> index 4c26caa92a..5a613650f5 100644
>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>> @@ -13,6 +13,75 @@
>>
>> #ifdef CONFIG_SPL
>> &binman {
>> + itb {
>> + filename = "u-boot.itb";
>> + fit {
>> + filename = "u-boot.itb";
>> + description = "U-Boot FIT";
>> + fit,fdt-list = "of-list";
>> + fit,external-offset=<0>;
>> +
>> + images {
>> + uboot {
>> + description = "U-Boot (64-bit)";
>> + type = "standalone";
>> + os = "U-Boot";
>> + arch = "arm64";
>> + compression = "none";
>> + load = <CONFIG_SYS_TEXT_BASE>;
>> + u-boot-nodtb {
>> + };
>> + };
>> +#ifdef CONFIG_SPL_ATF
>> + @atf_SEQ {
>> + fit,operation = "split-elf";
>> + description = "ARM Trusted Firmware";
>> + type = "firmware";
>> + arch = "arm64";
>> + os = "arm-trusted-firmware";
>> + compression = "none";
>> + fit,load;
>> + fit,entry;
>> + fit,data;
>> +
>> + atf-bl31 {
>> + };
>> + };
>> +#endif
>> +#ifdef CONFIG_TEE
>> + @tee_SEQ {
>> + fit,operation = "split-elf";
>> + description = "TEE";
>> + type = "tee";
>> + arch = "arm64";
>> + os = "tee";
>> + compression = "none";
>> + fit,load;
>> + fit,entry;
>> + fit,data;
>> +
>> + tee-os {
>> + };
>> + };
>> +#endif
>> + @fdt_SEQ {
>> + description = "NAME.dtb";
>> + type = "flat_dt";
>> + compression = "none";
>> + };
>> + };
>> + configurations {
>> + default = "@config_DEFAULT-SEQ";
>> +
>> + @config_SEQ {
>> + description = "NAME.dtb";
>> + fdt = "fdt_SEQ";
>> + firmware = "atf_1";
>> + loadables = "uboot","atf_2","atf_3";

This section will need some more love with some ifdef for ATF_SPL and TEE.

>> + };
>> + };
>> + };
>> + };
>> simple-bin {
>> filename = "u-boot-rockchip.bin";
>> pad-byte = <0xff>;
>> @@ -62,6 +131,7 @@
>> #ifdef CONFIG_ARM64
>> blob {
>> filename = "u-boot.itb";
>> +

This is unfortunately not possible since binman parallelizes the build
of images in the binman DT node. This means there is no guarantee the
u-boot.itb file is generated before this section is worked on by binman.
Or maybe I misunderstood the docs.

But good progress, I guess this phandle thing "just" needs to be fixed
to have something nice (both for this patch series and mine).

Cheers,
Quentin

>> #else
>> u-boot-img {
>> #endif
>> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
>> index f12cf24cb4..1c07114528 100644
>> --- a/configs/rock-pi-4-rk3399_defconfig
>> +++ b/configs/rock-pi-4-rk3399_defconfig
>> @@ -22,6 +22,7 @@ CONFIG_DEBUG_UART=y
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x3000000
>> CONFIG_TPL_SYS_MALLOC_F_LEN=0x4000
>> +# CONFIG_USE_SPL_FIT_GENERATOR is not set
>> CONFIG_SYS_LOADADDR_ALIGN_DOWN_BITS=16
>> CONFIG_BOOTSTAGE=y
>> CONFIG_SPL_BOOTSTAGE=y
>> --
>> 2.20.1
>>