| Message ID | 20220725172953.GD2029@begut |
|---|---|
| State | Not Applicable |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI | expand |
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. > --- > 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"; > + }; > + }; > + }; > + }; > simple-bin { > filename = "u-boot-rockchip.bin"; > pad-byte = <0xff>; > @@ -62,6 +131,7 @@ > #ifdef CONFIG_ARM64 > blob { > filename = "u-boot.itb"; > + > #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 >
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 >>
El Tue, Jul 26, 2022 at 11:08:21AM +0200, Quentin Schulz deia: > > You could also have a fit,align = <8>; property instead of hardcoding it. I'm not sure the fit entry in binman heeds this property, may have overlooked something. I'd have to try 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). The commit message said that the device tree spec requires 4 or 8 byte alignment, so maybe all fits want it because all fits are device trees ? Not sure. > > > + 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. > I'm sending a patch below that adds a couple of configuration properties to binman so that split-elf can fill the properties. How many segments are in bl31.elf or optee is not something that we have in CONFIGs, I think, so it may be difficult to catch all cases with ifdefs. It doesn't have to be this patch, or these properties, but maybe better something like this than ifdefs ? Also missing tests... > > 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). > I'm sending another patch below "fixing" the phandle issue, but it's a dirty hack without too much thought given. It could still fail if threads try to read data from the image of another thread before it's ready or something. Only lightly tested with binman -T 0, not parallel. It seems to run mkimage -B 8 -E -t -F ./itb.fit.fit SIX times each time I run binman. Not sure why. But it boots. I wonder if we shouldn't just run binman several times from make instead of once at the end, have make run it once for each file we want to create, so that we can reuse u-boot.itb for both the MMC and the SPI image. We could have one .dts for each image, so when make want u-boot.itb it runs binman -a of-list="rk3399-rock-pi-4b.dts rockchip-itb-u-boot.dts" And in this rockchip-itb-u-boot.dts there's only /binman { itb: itb { filename = "u-boot.itb"; fit { filename = "u-boot.itb"; ... }; }; } Then when it wants u-boot-rockchip.bin it runs binman -a of-list="rk3399-rock-pi-4b.dts rockchip-rksd-u-boot.dts" And in this rockchip-rksd-u-boot.dts there's only /binman { simple-bin { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; ... }; blob { filename = "u-boot.itb"; ... }; ... }; }; Then when make wants u-boot-rockchip-spi.bin it runs binman -a of-list="rk3399-rock-pi-4b.dts rockchip-rkspi-u-boot.dts" And in this rockchip-rkspi-u-boot.dts there's only /binman { simple-bin-spi { filename = "u-boot-rockchip-spi.bin"; pad-byte = <0xff>; mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rkspi"; ... }; blob { filename = "u-boot.itb"; ... }; ... }; }; I don't know if it's possible or if reading so many .dts so many times would be slower. But dependencies between binaries seems more a job for make than binman. Anyway, what I've tried so far: rockchip-u-boot.dtsi: // SPDX-License-Identifier: GPL-2.0+ /* * Copyright (C) 2019 Jagan Teki <jagan@amarulasolutions.com> */ #include <config.h> / { binman: binman { multiple-images; }; }; #ifdef CONFIG_SPL &binman { #ifndef CONFIG_USE_SPL_FIT_GENERATOR itb: 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"; fit,firmware = "atf_1"; fit,prepend-to-loadables = "uboot"; fit,loadables; }; }; }; }; #endif simple-bin { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; #ifndef CONFIG_TPL u-boot-spl { }; }; #else u-boot-tpl { }; }; u-boot-spl { }; #endif #ifdef CONFIG_ARM64 #ifdef CONFIG_USE_SPL_FIT_GENERATOR blob { filename = "u-boot.itb"; #else collection { content = <&itb>; #endif #else u-boot-img { #endif offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512)>; }; }; #ifdef CONFIG_ROCKCHIP_SPI_IMAGE simple-bin-spi { filename = "u-boot-rockchip-spi.bin"; pad-byte = <0xff>; mkimage { args = "-n", CONFIG_SYS_SOC, "-T", "rkspi"; #ifdef CONFIG_TPL multiple-data-files; u-boot-tpl { }; #endif u-boot-spl { }; }; #ifdef CONFIG_ARM64 #ifdef CONFIG_USE_SPL_FIT_GENERATOR blob { filename = "u-boot.itb"; #else collection { content = <&itb>; #endif #else u-boot-img { #endif /* Sync with u-boot,spl-payload-offset if present */ offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>; }; }; #endif }; #endif From 0ccdd5a27d86a697ae15aaeae2c54bf574928074 Mon Sep 17 00:00:00 2001 From: Xavier Drudis Ferran <xdrudis@tinet.cat> Date: Tue, 26 Jul 2022 15:31:18 +0200 Subject: [PATCH 1/2] Let entry collection pull contents from a different image. This is a quick and dirty change to binman to let a section of an image reference the content of another image or a section of it. Or it could also reference an entry in a different section of the same image. The only forbidden use is to reference itself, a descendent section or an ascendent section, brothers, cousins, and further relatives are fair game. It's very little tested, and if it ever is wanted should be better written possibly. In my tests it built an MMC image that boots my Rock Pi 4B, but it seems to build u-boot.itb 3 times with the same content. I wonder if it wouldn't be better to use binman for a sigle image at each execution, and let make schedule the calls like it was just a compiler or linker, so that if the MMC and SPI each include u-boot.itb we don't have to build u-boot.itb 3 times. The drawback is that we would be parsing the device tree 3 times. --- arch/arm/dts/rockchip-u-boot.dtsi | 15 +++++++- tools/binman/control.py | 10 +++-- tools/binman/etype/collection.py | 64 ++++++++++++++++++++++++++++++- tools/binman/etype/section.py | 2 +- 4 files changed, 82 insertions(+), 9 deletions(-) diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi index 5a613650f5..cd775ccae8 100644 --- a/arch/arm/dts/rockchip-u-boot.dtsi +++ b/arch/arm/dts/rockchip-u-boot.dtsi @@ -13,7 +13,8 @@ #ifdef CONFIG_SPL &binman { - itb { +#ifndef CONFIG_USE_SPL_FIT_GENERATOR + itb: itb { filename = "u-boot.itb"; fit { filename = "u-boot.itb"; @@ -82,6 +83,7 @@ }; }; }; +#endif simple-bin { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; @@ -102,8 +104,13 @@ #endif #ifdef CONFIG_ARM64 +#ifdef CONFIG_USE_SPL_FIT_GENERATOR blob { filename = "u-boot.itb"; +#else + collection { + content = <&itb>; +#endif #else u-boot-img { #endif @@ -129,9 +136,13 @@ }; #ifdef CONFIG_ARM64 +#ifdef CONFIG_USE_SPL_FIT_GENERATOR blob { filename = "u-boot.itb"; - +#else + collection { + content = <&itb>; +#endif #else u-boot-img { #endif diff --git a/tools/binman/control.py b/tools/binman/control.py index ce57dc7efc..bb33199b42 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -19,6 +19,7 @@ from binman import cbfs_util from binman import elf from patman import command from patman import tout +from binman.etype import section # These are imported if needed since they import libfdt state = None @@ -47,14 +48,15 @@ def _ReadImageDesc(binman_node, use_expanded): """ # For Image() # pylint: disable=E1102 - images = OrderedDict() + binman_section=section.Entry_section.Create(None,binman_node,etype='section') if 'multiple-images' in binman_node.props: for node in binman_node.subnodes: - images[node.name] = Image(node.name, node, + binman_section._entries[node.name] = Image(node.name, node, use_expanded=use_expanded) + binman_section._entries[node.name].binman_section = binman_section else: - images['image'] = Image('image', binman_node, use_expanded=use_expanded) - return images + binman_section._entries['image'] = Image('image', binman_node, use_expanded=use_expanded) + return binman_section._entries def _FindBinmanNode(dtb): """Find the 'binman' node in the device tree diff --git a/tools/binman/etype/collection.py b/tools/binman/etype/collection.py index 442b40b48b..5149576adb 100644 --- a/tools/binman/etype/collection.py +++ b/tools/binman/etype/collection.py @@ -11,6 +11,26 @@ import os from binman.entry import Entry from dtoc import fdt_util +def PathStartsWith(path, start): + """Does path start with start regarding whole path components? + + PathStartsWith("/foo/bar","/foo/bar") => True + PathStartsWith("/foo/bar/baz","/foo/bar") => True + PathStartsWith("/foo/bar/baz","/foo/bar/") => True + PathStartsWith("/foo/bar/baz/","/foo/bar") => True + PathStartsWith("/foo/bar/baz","/foo/bar/") => True + PathStartsWith("/foo/barbaz","/foo/bar") => False + PathStartsWith("/foo/bar","") => True + PathStartsWith("/foo/bar","/foo/bar") => True + PathStartsWith("/foo/bar","/foo/bar/") => True + PathStartsWith("/foo/bar","/foo/bar//") => False + PathStartsWith("/foo/bar/","/foo/bar") => True + """ + return ((path.startswith(start) and (path == start or + path[len(start)] == '/' or + start.endswith('/')) + ) or (path == start+"/")) + class Entry_collection(Entry): """An entry which contains a collection of other entries @@ -28,6 +48,47 @@ class Entry_collection(Entry): if not self.content: self.Raise("Collection must have a 'content' property") + def GetContentsByPhandle(self, phandle, required): + """Get the contents of an entry that may not be a direct sibling + Args: + required: True if the data must be present, False if it is OK to + return None + + Returns: + bytes content of the entry + """ + node = self._node.GetFdt().LookupPhandle(phandle) + if not node: + self.Raise("Cannot find node for phandle %d" % phandle) + if PathStartsWith(node.path, self._node.path): + self.Raise("Cannot reference self or descendant nodes with phandle %d for %s" + % (phandle, node.path)) + if PathStartsWith(self._node.path, node.path): + self.Raise("Cannot reference ascendant nodes with phandle %d for %s" + % (phandle, node.path)) + sec = self.section; + while sec: + if PathStartsWith(node.path, sec._node.path): + if node.path == sec._node.path: + return sec.GetData(required) + else: + for entry in sec._entries.values(): + if entry._node.path == node.path: + return entry.GetData(required) + else: + if (PathStartsWith(node.path, entry._node.path) + and entry is Entry_section): + sec = entry #try child + break + else: # exit while if no sibling matches + sec = None + else: # try parent + if sec.section is None and sec.binman_section: + sec=sec.binman_section + else: + sec=sec.section + self.Raise("Cannot find entry for phandle %d" % phandle) + def GetContents(self, required): """Get the contents of this entry @@ -42,8 +103,7 @@ class Entry_collection(Entry): self.Info('Getting contents, required=%s' % required) data = bytearray() for entry_phandle in self.content: - entry_data = self.section.GetContentsByPhandle(entry_phandle, self, - required) + entry_data = self.GetContentsByPhandle(entry_phandle, required) if not required and entry_data is None: self.Info('Contents not available yet') # Data not available yet diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index bd67238b91..e943f27a6d 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -744,7 +744,7 @@ class Entry_section(Entry): Returns: Image object containing this section """ - if not self.section: + if not self.section or self.section._node.path == '/binman': return self return self.section.GetImage()
On 7/26/22 21:08, Xavier Drudis Ferran wrote: > El Tue, Jul 26, 2022 at 11:08:21AM +0200, Quentin Schulz deia: >> >> You could also have a fit,align = <8>; property instead of hardcoding it. > > I'm not sure the fit entry in binman heeds this property, may have > overlooked something. I'd have to try 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). > > The commit message said that the device tree spec requires 4 or 8 byte > alignment, so maybe all fits want it because all fits are device trees > ? Not sure. > >>>> + 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. >> > > I'm sending a patch below that adds a couple of configuration properties to > binman so that split-elf can fill the properties. How many segments are > in bl31.elf or optee is not something that we have in CONFIGs, I think, > so it may be difficult to catch all cases with ifdefs. Careful with OP-TEE: tee.elf must NOT be used, please see commit 348310233dac ("mach-rockchip: make_fit_atf.py: support OP-TEE tee.bin v1 format").
El Tue, Jul 26, 2022 at 09:15:10PM +0200, Jerome Forissier deia: > > > I'm sending a patch below that adds a couple of configuration properties to > > binman so that split-elf can fill the properties. How many segments are > > in bl31.elf or optee is not something that we have in CONFIGs, I think, > > so it may be difficult to catch all cases with ifdefs. > > Careful with OP-TEE: tee.elf must NOT be used, please see commit > 348310233dac ("mach-rockchip: make_fit_atf.py: support OP-TEE tee.bin v1 > format"). > Uh. Maybe it isn't worth to migrate from make_fit_atf.py to split-elf ?
El Tue, Jul 26, 2022 at 09:08:25PM +0200, Xavier Drudis Ferran deia:
> want u-boot.itb it runs binman -a of-list="rk3399-rock-pi-4b.dts rockchip-itb-u-boot.dts"
Sorry. I mistook the parameter. That would only generate two config entries and 2 fdts in the .itb image.
We could use some includes, but then we'd need too many .dts files.
Never mind.
Maybe we could just stay with make_fit_atf.py
kHi Xavier, On Tue, 26 Jul 2022 at 13:08, Xavier Drudis Ferran <xdrudis@tinet.cat> wrote: > > El Tue, Jul 26, 2022 at 11:08:21AM +0200, Quentin Schulz deia: > > > > You could also have a fit,align = <8>; property instead of hardcoding it. > > I'm not sure the fit entry in binman heeds this property, may have > overlooked something. I'd have to try it. [..] > > > > > 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). > > > > I'm sending another patch below "fixing" the phandle issue, but it's a > dirty hack without too much thought given. It could still fail if threads > try to read data from the image of another thread before it's ready or something. > Only lightly tested with binman -T 0, not parallel. > It seems to run mkimage -B 8 -E -t -F ./itb.fit.fit SIX times each time > I run binman. Not sure why. > > But it boots. > > I wonder if we shouldn't just run binman several times from make instead of once at the end, > have make run it once for each file we want to create, so that we can reuse u-boot.itb for both > the MMC and the SPI image. We could have one .dts for each image, so when make > want u-boot.itb it runs binman -a of-list="rk3399-rock-pi-4b.dts rockchip-itb-u-boot.dts" We must not move to multiple invocations of binman. It is supposed to handle everything at once. But for the case you have here, we could perhaps add a feature to specify an ordering for images and to explicitly allow one image to include another? [..] Regards, Simon
Hi Quentin, On Tue, 26 Jul 2022 at 03:08, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > 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. > > [..] > > >> + }; > >> + }; > >> + }; > >> + }; > >> 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. You are correct, but this is something that has bothered me. At the moment we handle this by embedding the definition for one file into the one that uses it. This is on the theory that there is no need to actually write the embedded file, since it is a temporary file. In fact binman does generate temporary files though. Is that good enough? If not I'd like to understand the need better, before we make any changes. > > 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 [..] Regards, Simon
Hi Simon, On 7/26/22 21:58, Simon Glass wrote: > Hi Quentin, > > On Tue, 26 Jul 2022 at 03:08, Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> >> 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. >>> > > [..] > >> >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> 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. > > You are correct, but this is something that has bothered me. > > At the moment we handle this by embedding the definition for one file > into the one that uses it. This is on the theory that there is no need > to actually write the embedded file, since it is a temporary file. In > fact binman does generate temporary files though. > > Is that good enough? If not I'd like to understand the need better, > before we make any changes. > For Rockchip, with the patch series from https://lore.kernel.org/u-boot/20220722113505.3875669-1-foss+uboot@0leil.net/, we now have two binaries: u-boot-rockchip.bin and u-boot-rockchip-spi.bin Both share the exact same u-boot.itb file (though at a different offset in the storage medium) embedded. This u-boot.itb is currently externally created via the Makefile + make_fit_atf.py prior to binman being called. This allows us to have a blob { filename = "u-boot.itb"; } in the binman DT node and that's enough for it to work fine. There's a desire to get rid of make_fit_atf.py in favor of binman. This means that we need to create this file in binman now. There's been some resistance to making binman not create idbloader.img image in the patch series mentioned above (it is *technically* created by binman but only in temporary files and then embedded in u-boot-rockchip*.bin). I assume the same resistance will be met for u-boot.itb. With how binman works currently and since we have u-boot.itb content embedded in at least two different images created by binman (might be even more once there's USB/NAND support?), we'd need to have the fit device tree node appear thrice (or more). One for u-boot.itb creation (because of people not wanting it disappear from binaries generated by U-Boot), one for embedding into u-boot-rockchip.bin and one for embedding into u-boot-rockchip-spi.bin. This increases the risk of not updating all fit device tree nodes at once. This is suboptimal in terms of build time because the image will effectively be created thrice (or more). This is also not the best for easy check of reproducibility since the content of the fit device tree node is technically not the same as u-boot.itb file (because it is the result of a different image built by binman). So I think the minimal implementation would be to allow to define an image/section (u-boot.itb) and to "#include" it inline in another section (where blob { filename = "u-boot.itb"; } currently is for u-boot-rockchip.bin and u-boot-rockchip-spi.bin). From reading the docs, I expected collection entry to be exactly this? But I couldn't make it work. Ideally, allowing binman to define a dependency from one image on another would mean we could still use the blob image/section, but safely, because the dependency is explicit so binman will know which image to build first. Phandles is what we're after on the Device Tree side I would assume. We could have something like: blob { image = <&u-boot-itb>; } for example. No idea how binman would create this dependency tree though :) Straight from my brain, lemme know if something needs to be clarified or rephrased. Cheers, Quentin
Hi Quentin, On Wed, 27 Jul 2022 at 04:34, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 7/26/22 21:58, Simon Glass wrote: > > Hi Quentin, > > > > On Tue, 26 Jul 2022 at 03:08, Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> > >> 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. > >>> > > > > [..] > > > >> > >>>> + }; > >>>> + }; > >>>> + }; > >>>> + }; > >>>> 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. > > > > You are correct, but this is something that has bothered me. > > > > At the moment we handle this by embedding the definition for one file > > into the one that uses it. This is on the theory that there is no need > > to actually write the embedded file, since it is a temporary file. In > > fact binman does generate temporary files though. > > > > Is that good enough? If not I'd like to understand the need better, > > before we make any changes. > > > > For Rockchip, with the patch series from > https://lore.kernel.org/u-boot/20220722113505.3875669-1-foss+uboot@0leil.net/, > we now have two binaries: > u-boot-rockchip.bin and u-boot-rockchip-spi.bin > > Both share the exact same u-boot.itb file (though at a different offset > in the storage medium) embedded. > > This u-boot.itb is currently externally created via the Makefile + > make_fit_atf.py prior to binman being called. This allows us to have a > blob { filename = "u-boot.itb"; } in the binman DT node and that's > enough for it to work fine. > > There's a desire to get rid of make_fit_atf.py in favor of binman. This > means that we need to create this file in binman now. > > There's been some resistance to making binman not create idbloader.img > image in the patch series mentioned above (it is *technically* created > by binman but only in temporary files and then embedded in > u-boot-rockchip*.bin). I assume the same resistance will be met for > u-boot.itb. > > With how binman works currently and since we have u-boot.itb content > embedded in at least two different images created by binman (might be > even more once there's USB/NAND support?), we'd need to have the fit > device tree node appear thrice (or more). One for u-boot.itb creation > (because of people not wanting it disappear from binaries generated by > U-Boot), one for embedding into u-boot-rockchip.bin and one for > embedding into u-boot-rockchip-spi.bin. > This increases the risk of not updating all fit device tree nodes at once. > This is suboptimal in terms of build time because the image will > effectively be created thrice (or more). > This is also not the best for easy check of reproducibility since the > content of the fit device tree node is technically not the same as > u-boot.itb file (because it is the result of a different image built by > binman). > > So I think the minimal implementation would be to allow to define an > image/section (u-boot.itb) and to "#include" it inline in another > section (where blob { filename = "u-boot.itb"; } currently is for > u-boot-rockchip.bin and u-boot-rockchip-spi.bin). From reading the docs, > I expected collection entry to be exactly this? But I couldn't make it work. > > Ideally, allowing binman to define a dependency from one image on > another would mean we could still use the blob image/section, but > safely, because the dependency is explicit so binman will know which > image to build first. Phandles is what we're after on the Device Tree > side I would assume. We could have something like: blob { image = > <&u-boot-itb>; } for example. No idea how binman would create this > dependency tree though :) > > Straight from my brain, lemme know if something needs to be clarified or > rephrased. Collections should allow this. Can you try running with threading disabled (-T0)? Another thought is that we could provide a way for binman to write a section to a file in an official way, i.e. give it a filename property. Regards, Simon
Hi Simon, On 7/31/22 03:27, Simon Glass wrote: > Hi Quentin, > > On Wed, 27 Jul 2022 at 04:34, Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> >> Hi Simon, >> >> On 7/26/22 21:58, Simon Glass wrote: >>> Hi Quentin, >>> >>> On Tue, 26 Jul 2022 at 03:08, Quentin Schulz >>> <quentin.schulz@theobroma-systems.com> wrote: >>>> >>>> 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. >>>>> >>> >>> [..] >>> >>>> >>>>>> + }; >>>>>> + }; >>>>>> + }; >>>>>> + }; >>>>>> 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. >>> >>> You are correct, but this is something that has bothered me. >>> >>> At the moment we handle this by embedding the definition for one file >>> into the one that uses it. This is on the theory that there is no need >>> to actually write the embedded file, since it is a temporary file. In >>> fact binman does generate temporary files though. >>> >>> Is that good enough? If not I'd like to understand the need better, >>> before we make any changes. >>> >> >> For Rockchip, with the patch series from >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220722113505.3875669-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=OevHoqt3vOXsWmXfEPFnvZ2KSNns-ZKBiiZBUovrynOXVCUZtFfK9jsk3C1r4Y_J&s=nXnNRN3hfa_mkkt_suH6wLGbwj06I7HmQKcagZ6JPE0&e= , >> we now have two binaries: >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin >> >> Both share the exact same u-boot.itb file (though at a different offset >> in the storage medium) embedded. >> >> This u-boot.itb is currently externally created via the Makefile + >> make_fit_atf.py prior to binman being called. This allows us to have a >> blob { filename = "u-boot.itb"; } in the binman DT node and that's >> enough for it to work fine. >> >> There's a desire to get rid of make_fit_atf.py in favor of binman. This >> means that we need to create this file in binman now. >> >> There's been some resistance to making binman not create idbloader.img >> image in the patch series mentioned above (it is *technically* created >> by binman but only in temporary files and then embedded in >> u-boot-rockchip*.bin). I assume the same resistance will be met for >> u-boot.itb. >> >> With how binman works currently and since we have u-boot.itb content >> embedded in at least two different images created by binman (might be >> even more once there's USB/NAND support?), we'd need to have the fit >> device tree node appear thrice (or more). One for u-boot.itb creation >> (because of people not wanting it disappear from binaries generated by >> U-Boot), one for embedding into u-boot-rockchip.bin and one for >> embedding into u-boot-rockchip-spi.bin. >> This increases the risk of not updating all fit device tree nodes at once. >> This is suboptimal in terms of build time because the image will >> effectively be created thrice (or more). >> This is also not the best for easy check of reproducibility since the >> content of the fit device tree node is technically not the same as >> u-boot.itb file (because it is the result of a different image built by >> binman). >> >> So I think the minimal implementation would be to allow to define an >> image/section (u-boot.itb) and to "#include" it inline in another >> section (where blob { filename = "u-boot.itb"; } currently is for >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin). From reading the docs, >> I expected collection entry to be exactly this? But I couldn't make it work. >> >> Ideally, allowing binman to define a dependency from one image on >> another would mean we could still use the blob image/section, but >> safely, because the dependency is explicit so binman will know which >> image to build first. Phandles is what we're after on the Device Tree >> side I would assume. We could have something like: blob { image = >> <&u-boot-itb>; } for example. No idea how binman would create this >> dependency tree though :) >> >> Straight from my brain, lemme know if something needs to be clarified or >> rephrased. > > Collections should allow this. Can you try running with threading > disabled (-T0)? > Do they? What (I think) I want is basically the following: &binman { multiple-images; u_boot_itb: u-boot-itb { fit {}; }; simple-bin { [...] collection { content = <&u_boot_itb>; }; }; simple-bin-spi { [...] collection { content = <&u_boot_itb>; }; }; }; or I guess something like: &binman { multiple-images; u-boot-itb { filename = "u-boot.itb"; collection { content = <&u_boot_itb>; }; }; simple-bin { [...] u_boot_itb: fit {}; }; simple-bin-spi { [...] collection { content = <&u_boot_itb>; }; }; }; However, I tried with: // SPDX-License-Identifier: GPL-2.0+ /dts-v1/; / { #address-cells = <1>; #size-cells = <1>; binman { multiple-images; text2: foo { text1: text { text = "bl31.bin"; }; }; bar { collection { content = <&text2>; }; }; }; }; and: // SPDX-License-Identifier: GPL-2.0+ /dts-v1/; / { #address-cells = <1>; #size-cells = <1>; binman { multiple-images; text2: foo { text1: text { text = "bl31.bin"; }; }; bar { collection { content = <&text1>; }; }; }; }; but tools/binman/binman build -d test.dts returns for both: binman: Node '/binman/bar/collection': Cannot find entry for node 'text' (or 'foo') I guess the issue is that this collection would be crossing the image boundary and this is not supported? I'm not sure to really understand the point of the collection section? I would assume wanting to get multiple times the same entries in the same image should be pretty rare? I guess I could simply have a huge constant defining the u-boot-itb fit node and add it to the binman DT node in the correct places to avoid having to maintain multiple copies of the DT node, but it's still inefficient IMO since the image would be created as many times as it's used. Ideally it'd be a dependency on an image being created and one uses blob-ext or something similar to use this newly generated image. I don't know, throwing ideas right now. Am I completely lost or does what I want to do make some kind of sense? Cheers, Quentin
Hi Quentin, On Mon, 1 Aug 2022 at 11:05, Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > > Hi Simon, > > On 7/31/22 03:27, Simon Glass wrote: > > Hi Quentin, > > > > On Wed, 27 Jul 2022 at 04:34, Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> > >> Hi Simon, > >> > >> On 7/26/22 21:58, Simon Glass wrote: > >>> Hi Quentin, > >>> > >>> On Tue, 26 Jul 2022 at 03:08, Quentin Schulz > >>> <quentin.schulz@theobroma-systems.com> wrote: > >>>> > >>>> 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. > >>>>> > >>> > >>> [..] > >>> > >>>> > >>>>>> + }; > >>>>>> + }; > >>>>>> + }; > >>>>>> + }; > >>>>>> 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. > >>> > >>> You are correct, but this is something that has bothered me. > >>> > >>> At the moment we handle this by embedding the definition for one file > >>> into the one that uses it. This is on the theory that there is no need > >>> to actually write the embedded file, since it is a temporary file. In > >>> fact binman does generate temporary files though. > >>> > >>> Is that good enough? If not I'd like to understand the need better, > >>> before we make any changes. > >>> > >> > >> For Rockchip, with the patch series from > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220722113505.3875669-2D1-2Dfoss-2Buboot-400leil.net_&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=OevHoqt3vOXsWmXfEPFnvZ2KSNns-ZKBiiZBUovrynOXVCUZtFfK9jsk3C1r4Y_J&s=nXnNRN3hfa_mkkt_suH6wLGbwj06I7HmQKcagZ6JPE0&e= , > >> we now have two binaries: > >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin > >> > >> Both share the exact same u-boot.itb file (though at a different offset > >> in the storage medium) embedded. > >> > >> This u-boot.itb is currently externally created via the Makefile + > >> make_fit_atf.py prior to binman being called. This allows us to have a > >> blob { filename = "u-boot.itb"; } in the binman DT node and that's > >> enough for it to work fine. > >> > >> There's a desire to get rid of make_fit_atf.py in favor of binman. This > >> means that we need to create this file in binman now. > >> > >> There's been some resistance to making binman not create idbloader.img > >> image in the patch series mentioned above (it is *technically* created > >> by binman but only in temporary files and then embedded in > >> u-boot-rockchip*.bin). I assume the same resistance will be met for > >> u-boot.itb. > >> > >> With how binman works currently and since we have u-boot.itb content > >> embedded in at least two different images created by binman (might be > >> even more once there's USB/NAND support?), we'd need to have the fit > >> device tree node appear thrice (or more). One for u-boot.itb creation > >> (because of people not wanting it disappear from binaries generated by > >> U-Boot), one for embedding into u-boot-rockchip.bin and one for > >> embedding into u-boot-rockchip-spi.bin. > >> This increases the risk of not updating all fit device tree nodes at once. > >> This is suboptimal in terms of build time because the image will > >> effectively be created thrice (or more). > >> This is also not the best for easy check of reproducibility since the > >> content of the fit device tree node is technically not the same as > >> u-boot.itb file (because it is the result of a different image built by > >> binman). > >> > >> So I think the minimal implementation would be to allow to define an > >> image/section (u-boot.itb) and to "#include" it inline in another > >> section (where blob { filename = "u-boot.itb"; } currently is for > >> u-boot-rockchip.bin and u-boot-rockchip-spi.bin). From reading the docs, > >> I expected collection entry to be exactly this? But I couldn't make it work. > >> > >> Ideally, allowing binman to define a dependency from one image on > >> another would mean we could still use the blob image/section, but > >> safely, because the dependency is explicit so binman will know which > >> image to build first. Phandles is what we're after on the Device Tree > >> side I would assume. We could have something like: blob { image = > >> <&u-boot-itb>; } for example. No idea how binman would create this > >> dependency tree though :) > >> > >> Straight from my brain, lemme know if something needs to be clarified or > >> rephrased. > > > > Collections should allow this. Can you try running with threading > > disabled (-T0)? > > > > Do they? > > What (I think) I want is basically the following: > > &binman { > multiple-images; > u_boot_itb: u-boot-itb { > fit {}; > }; > simple-bin { > [...] > collection { > content = <&u_boot_itb>; > }; > }; > simple-bin-spi { > [...] > collection { > content = <&u_boot_itb>; > }; > }; > }; > > or I guess something like: > &binman { > multiple-images; > u-boot-itb { > filename = "u-boot.itb"; > collection { > content = <&u_boot_itb>; > }; > }; > simple-bin { > [...] > u_boot_itb: fit {}; > }; > simple-bin-spi { > [...] > collection { > content = <&u_boot_itb>; > }; > }; > }; > > However, I tried with: > // SPDX-License-Identifier: GPL-2.0+ > /dts-v1/; > > / { > #address-cells = <1>; > #size-cells = <1>; > > binman { > multiple-images; > text2: foo { > text1: text { > text = "bl31.bin"; > }; > }; > bar { > collection { > content = <&text2>; > }; > }; > }; > }; > > and: > // SPDX-License-Identifier: GPL-2.0+ > /dts-v1/; > > / { > #address-cells = <1>; > #size-cells = <1>; > > binman { > multiple-images; > text2: foo { > text1: text { > text = "bl31.bin"; > }; > }; > bar { > collection { > content = <&text1>; > }; > }; > }; > }; > > but tools/binman/binman build -d test.dts returns for both: > binman: Node '/binman/bar/collection': Cannot find entry for node 'text' > (or 'foo') > I guess the issue is that this collection would be crossing the image > boundary and this is not supported? Yes, they need to be in the same section / image. > > I'm not sure to really understand the point of the collection section? I > would assume wanting to get multiple times the same entries in the same > image should be pretty rare? Yes it should, but it does happen. > > I guess I could simply have a huge constant defining the u-boot-itb fit > node and add it to the binman DT node in the correct places to avoid > having to maintain multiple copies of the DT node, but it's still > inefficient IMO since the image would be created as many times as it's > used. Ideally it'd be a dependency on an image being created and one > uses blob-ext or something similar to use this newly generated image. I > don't know, throwing ideas right now. > > Am I completely lost or does what I want to do make some kind of sense? Well I still feel that we should handle this properly in binman. I don't even think we need to allow images to be incorporated in other images. We can probably just allow a section to have a filename, a bit like this patch [1]. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/patch/20220801160610.2330151-3-foss+uboot@0leil.net/
El Mon, Aug 01, 2022 at 01:13:27PM -0600, Simon Glass deia: > > > > Am I completely lost or does what I want to do make some kind of sense? > > Well I still feel that we should handle this properly in binman. > I respect your feelings but would you care explaining the goal of binman? I first thought it was about handling binaries (things like copy binary content, aligning it, padding, extracting and selecting parts from one binary to form the final image...). A little like a linker but for other kinds of binary files, with maybe headers but no symbol resolution and all. Maybe more like GNU ar but for images instead of executables. But then you seem to say it should handle dependencies between build files and be called only once ? So should it eventually become a sort of make itself ? > I don't even think we need to allow images to be incorporated in other > images. We can probably just allow a section to have a filename, a bit > like this patch [1]. > There's a image r-sd and an image r-spi. Both need to have inside the same binary itb, packed differently. If r-sd has a section with itb and writes that section to a file, r-spi still needs to incorporate that section as a binary, and to do that it needs to know the other image has already written its section to a file or built into somewhere in memory. So there're dependencies, and synchronisation if you want to built images in parallel. make can handle that if you call binman multiple times. But if you don't want that, then you need to either include dependency management into binman (turning it into a little make working from dts subtrees instead of makefiles) or forbid that binaries used by binman could be images produced by binman, and then you need other binary manipulation tools (like make_fit_atf.py, which is basically what you have now, but that's apparently not desired either). If it was just me who feels lost, then it'd be just that I don't get it, but Quentin is saying something similar. In any case, don't forget what Jerome said about tee.bin needing different parsing than split-elf does in binman. The sections of tee.bin can also end up in u-boot.itb. Adding that to binman would maybe make more sense that what I was trying to add, in the sense that binman be a swiss army knive of parsing and building binaries. But then you'd still have the problem that if images built by binman cannot be incorporated in images built by binman, then something else has to build u-boot.itb. And that something else is currently make_fit_atf.py and works fine, so why the trouble ? We're kind of running in circles, and for me it would be helpful to understand the goal and scope of binman to understand what would be desirable, because the status quo seems preferable to some kind of feature creep in binman that I can't seem to reconcile with the philosophy of one tool doing one task and well. Maybe most people here already understand all this, but sometimes explaining the basics to a foreign bystander who knows little of the subject (hello, that's me) can help people sort out thoughts? Thanks, and sorry for the wall of text.
Hi Xavier, On Tue, 2 Aug 2022 at 02:19, Xavier Drudis Ferran <xdrudis@tinet.cat> wrote: > > El Mon, Aug 01, 2022 at 01:13:27PM -0600, Simon Glass deia: > > > > > > Am I completely lost or does what I want to do make some kind of sense? > > > > Well I still feel that we should handle this properly in binman. > > > > I respect your feelings but would you care explaining the goal of binman? > I first thought it was about handling binaries (things like copy binary content, > aligning it, padding, extracting and selecting parts from one binary to form > the final image...). A little like a linker but for other kinds of binary files, > with maybe headers but no symbol resolution and all. Maybe more like GNU ar but > for images instead of executables. > > But then you seem to say it should handle dependencies between build files and > be called only once ? So should it eventually become a sort of make itself ? > > > I don't even think we need to allow images to be incorporated in other > > images. We can probably just allow a section to have a filename, a bit > > like this patch [1]. > > > > There's a image r-sd and an image r-spi. Both need to have inside the > same binary itb, packed differently. If r-sd has a section with itb > and writes that section to a file, r-spi still needs to incorporate > that section as a binary, and to do that it needs to know the other > image has already written its section to a file or built into > somewhere in memory. So there're dependencies, and synchronisation if > you want to built images in parallel. make can handle that if you call > binman multiple times. But if you don't want that, then you need to > either include dependency management into binman (turning it into a > little make working from dts subtrees instead of makefiles) or forbid > that binaries used by binman could be images produced by binman, and > then you need other binary manipulation tools (like make_fit_atf.py, > which is basically what you have now, but that's apparently not > desired either). > > If it was just me who feels lost, then it'd be just that I don't get it, > but Quentin is saying something similar. It seems we need a lot more guidance here. I can help write more into the packaging docs, perhaps: https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivation Can you please suggest a few questions to answer? > > In any case, don't forget what Jerome said about tee.bin needing different > parsing than split-elf does in binman. The sections of tee.bin can also > end up in u-boot.itb. Adding that to binman would maybe make more sense > that what I was trying to add, in the sense that binman be a swiss army knive > of parsing and building binaries. But then you'd still have the problem > that if images built by binman cannot be incorporated in images built by binman, > then something else has to build u-boot.itb. And that something else is > currently make_fit_atf.py and works fine, so why the trouble ? Because we have lots of scripts with no tests and it will proliferate even more. Binman is data-driven, has tests and allows us to build common functionality used by all SoCs. > > We're kind of running in circles, and for me it would be helpful to > understand the goal and scope of binman to understand what would be > desirable, because the status quo seems preferable to some kind of > feature creep in binman that I can't seem to reconcile with the > philosophy of one tool doing one task and well. > > Maybe most people here already understand all this, but sometimes > explaining the basics to a foreign bystander who knows little of > the subject (hello, that's me) can help people sort out thoughts? > > Thanks, and sorry for the wall of text. Have you looked at osfc binman talk from a few years ago? Binman is not to replace make. The dependency we are talking about is between different images generated by Binman. Part of the problem is that you know what you want, but it is a bit foggy to me. To move forward, can I suggest you send a patch with a binman description file containing what you want. Obviously it won't work, due to the dependencies, but I can then figure out how to enable that in binman. Can you try that? Regards, Simon
El Tue, Aug 02, 2022 at 06:41:40AM -0600, Simon Glass deia: > > It seems we need a lot more guidance here. I can help write more into > the packaging docs, perhaps: > > https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivation > > Can you please suggest a few questions to answer? > I can try. But let me start by saying thank you for the guidance that was already there. I had read it and I still don't understand it together with some of what you said on the list. So how about...: - Exact criteria for what is a binary you take from external sources and and what is a packaged image that's binman responsability. So, TF-A is compiled externally, and the U-Boot build system just gets a BL31 env variable pointing at an bl31.elf or so. But u-boot.itb, despite being a FIT that includes parts of TF-A, maybe parts of tee.bin, some dtbs and binaries built by U-Boot itself and who knows if public keys or extra dtb overlays provided directly by the user, or something I can't think of now, it's not a binary from external sources but a U-Boot image that needs to be packaged by binman? For me there's some grey zone. Since make_fit_atf.py is in U-Boot then u-boot.itb is a binary, but since part of the contents are external, and mixed with U-Boot's own code then it's a packaged image. But then it's just part of the final packaged image that one can store in SPI, SD or wherever. - Scope ? how customizable needs the description of the image to be ? If a board meeds different images for booting from sd, spi and nand, should the *-u-boot.dts* provide the 3 images or one or ...? Must the image description recognize all possible kconfig options ? So should it look for splash source = spi and put a copy of some bmp or bmp.gz at some offset if the user opted for splash image in spi? Or that's a stretch and it's basically it should work for the default config for each board and users who change things should change the dts or package manually following the various READMEs ? (taking into account that any manual packaging risks the image description in dts no longer matching the image in case some soft is reading that) - Use ? Visibility ? From the motivation text one would think that users run make boardX_defconfig make BL31=bl31.elf binman But in fact currently it is make who calls binman. Is it just a temporary situation and the intent is for make just to build binaries and then binman will be called afterwards to package images ? Because binman not only packages what make built, it changes some properties of the dtb that was built by make, if I've understood it. In that sense it seems to be part of the build system not just a packager that comes afterwards. And the different parameters of binman are supposed to be set by make, and then more technical or is binman supposed to be used by the user to build some custom image too? - What about reuse ? the binman recipes go into u-boot.dts* files . But into which ? The SOC .dtsi ? the mach .dtsi ? the board .dtsi ? For example many boards include rk3399-u-boot.dtsi. Rk3399 can boot from spi, sd, or emmc (the same image can be used for emmc and sd). That's the bootrom. In theory SPL (a fatter SPL) could load U-Boot from nvme, I think, or USB, or serial, or network but I don't think that's implemented? So if you put the binman node in rk3399-u-boot.dtsi you could have it generate two images, one for sd/emmc and one for spi (plus intermediate images that people asked for to customize things). If you put it in rockchip-u-boot.dtsi then it can be more reused, but maybe then you need a format for nand too? And what if a board doesn't have emmc nor SD ? or doesn't have spi NOR? Should it modify the inherited binman node to disable one of the images, or just generate some unused image and call it a day ? Or you should put your binman subnodes in independent .dtsi files and carefully include the needed ones in each board -u-boot.dts ? Or you put it all in the most generic u-boot.dtsi you can and #ifdef away the parts that apply to each board/configuration ? So far we seem to prefer this last approach, but I don't know whether it was a plan or what. > > Because we have lots of scripts with no tests and it will proliferate > even more. Binman is data-driven, has tests and allows us to build > common functionality used by all SoCs. > Makes sense, tests are necessary, but some functionality may be common and some may be very specific for some arch or SOC or vendor. If all specific kirks also go into binman it can get a little complex. It can still be worth it, it's just that when some external entity decides things work some way (because they design a SOC or write a bootrom, or whatever) it'll be easier for them to give a shell script that implements what's needed than to understand and adapt binman. So that's a job for others to do possibly. So until someone does (like Quentin just did for rkspi) then we'll still have binman and whatever the external vendor came up with. One option would be to use binman just for generic things (padding, alignment, extraction, concatenation, calling external tools). And resign ourselves to keep having some external tools that make calls before calling binman, or that binman itself calls. I don't know. > > Have you looked at osfc binman talk from a few years ago? > No, or maybe so long ago that I forgot. I read what I could from doc/. I generally prefer text than video. But I may take a look, now that you mention it. > Binman is not to replace make. The dependency we are talking about is > between different images generated by Binman. Part of the problem is > that you know what you want, but it is a bit foggy to me. > We are currently building images that include images, so some files are both binaries and images, and that is not supported. If you support that in a general way (because binman seems to be intended to generalize a bit packaging) then it can become a little complex because new dependencies between images or sections arise. If you want to keep binman simpler, you could offload that to make. But you don't want it, I still don't know why. > To move forward, can I suggest you send a patch with a binman > description file containing what you want. Obviously it won't work, > due to the dependencies, but I can then figure out how to enable that > in binman. Can you try that? > I can help explaining my doubts because they're big and candid, and I could try to explain what is needed for Rock Pi 4, but I don't think I could explain it better than what Quentin and Jerome already did. I sent a dtsi with a binman description file https://lists.denx.de/pipermail/u-boot/2022-July/490068.html (you replied to that message, but maybe didn't read until the end ?) Your suggestion was to add ordering properties to binman. I guess we could, but I'm not sure that's very scalable or even very data driven. Its simpler than managing dependencies, I guess, so that's good. But I'm no longer sure that's what I want. Well I'm sure that's not what I want since Jerome pointed out that that would mistreat tee.bin. Maybe what I want is to keep using make_fit_atf.py to build u-boot.itb and then consider u-boot.itb a binary for binman, that's what I think Quentin already sent, before I messed it up by mentioning make_fit_atf.py at all. Maybe just remove the message make outputs saying CONFIG_USE_SPL_FIT_GENERATOR looks ugly ? Because that's what lead me to the rabbit hole...
Hi Xavier, On Tue, 2 Aug 2022 at 11:19, Xavier Drudis Ferran <xdrudis@tinet.cat> wrote: > > El Tue, Aug 02, 2022 at 06:41:40AM -0600, Simon Glass deia: > > > > It seems we need a lot more guidance here. I can help write more into > > the packaging docs, perhaps: > > > > https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivation > > > > Can you please suggest a few questions to answer? > > > > I can try. But let me start by saying thank you for the guidance that was already there. > I had read it and I still don't understand it together with some of what you said on the list. > So how about...: > > - Exact criteria for what is a binary you take from external sources > and and what is a packaged image that's binman responsability. So, > TF-A is compiled externally, and the U-Boot build system just gets > a BL31 env variable pointing at an bl31.elf or so. But u-boot.itb, > despite being a FIT that includes parts of TF-A, maybe parts of > tee.bin, some dtbs and binaries built by U-Boot itself and who > knows if public keys or extra dtb overlays provided directly by the > user, or something I can't think of now, it's not a binary from > external sources but a U-Boot image that needs to be packaged by > binman? For me there's some grey zone. Since make_fit_atf.py is in > U-Boot then u-boot.itb is a binary, but since part of the contents > are external, and mixed with U-Boot's own code then it's a packaged > image. But then it's just part of the final packaged image that > one can store in SPI, SD or wherever. > > - Scope ? how customizable needs the description of the image to be ? > > If a board meeds different images for booting from sd, spi and nand, > should the *-u-boot.dts* provide the 3 images or one or ...? > > Must the image description recognize all possible kconfig options ? > So should it look for splash source = spi and put a copy of some > bmp or bmp.gz at some offset if the user opted for splash image in spi? > > Or that's a stretch and it's basically it should work for the > default config for each board and users who change things should > change the dts or package manually following the various READMEs ? > (taking into account that any manual packaging risks the image > description in dts no longer matching the image in case some soft is > reading that) > > - Use ? Visibility ? > > From the motivation text one would think that users run > make boardX_defconfig > make > BL31=bl31.elf binman > > But in fact currently it is make who calls binman. Is it just a temporary > situation and the intent is for make just to build binaries and then > binman will be called afterwards to package images ? Because binman not > only packages what make built, it changes some properties of the dtb that > was built by make, if I've understood it. In that sense it seems to be > part of the build system not just a packager that comes afterwards. > > And the different parameters of binman are supposed to be set by make, > and then more technical or is binman supposed to be used by the user > to build some custom image too? > > - What about reuse ? > > the binman recipes go into u-boot.dts* files . But into which ? The > SOC .dtsi ? the mach .dtsi ? the board .dtsi ? For example many > boards include rk3399-u-boot.dtsi. Rk3399 can boot from spi, sd, or > emmc (the same image can be used for emmc and sd). That's the > bootrom. In theory SPL (a fatter SPL) could load U-Boot from nvme, I > think, or USB, or serial, or network but I don't think that's > implemented? > > So if you put the binman node in rk3399-u-boot.dtsi you could have > it generate two images, one for sd/emmc and one for spi (plus > intermediate images that people asked for to customize things). If > you put it in rockchip-u-boot.dtsi then it can be more reused, but > maybe then you need a format for nand too? And what if a board > doesn't have emmc nor SD ? or doesn't have spi NOR? Should it modify > the inherited binman node to disable one of the images, or just generate > some unused image and call it a day ? Or you should put your binman > subnodes in independent .dtsi files and carefully include the needed > ones in each board -u-boot.dts ? Or you put it all in the most > generic u-boot.dtsi you can and #ifdef away the parts that apply to > each board/configuration ? So far we seem to prefer this last > approach, but I don't know whether it was a plan or what. > > > > > > Because we have lots of scripts with no tests and it will proliferate > > even more. Binman is data-driven, has tests and allows us to build > > common functionality used by all SoCs. > > > > Makes sense, tests are necessary, but some functionality may be common > and some may be very specific for some arch or SOC or vendor. If all > specific kirks also go into binman it can get a little complex. It can > still be worth it, it's just that when some external entity decides > things work some way (because they design a SOC or write a bootrom, or > whatever) it'll be easier for them to give a shell script that > implements what's needed than to understand and adapt binman. So > that's a job for others to do possibly. So until someone does (like > Quentin just did for rkspi) then we'll still have binman and whatever > the external vendor came up with. > > One option would be to use binman just for generic things (padding, > alignment, extraction, concatenation, calling external tools). And > resign ourselves to keep having some external tools that make calls > before calling binman, or that binman itself calls. I don't know. > > > > > Have you looked at osfc binman talk from a few years ago? > > > > No, or maybe so long ago that I forgot. I read what I could from > doc/. I generally prefer text than video. But I may take a look, now > that you mention it. > > > Binman is not to replace make. The dependency we are talking about is > > between different images generated by Binman. Part of the problem is > > that you know what you want, but it is a bit foggy to me. > > > > We are currently building images that include images, so some files > are both binaries and images, and that is not supported. If you > support that in a general way (because binman seems to be intended to > generalize a bit packaging) then it can become a little complex > because new dependencies between images or sections arise. If you want > to keep binman simpler, you could offload that to make. But you don't > want it, I still don't know why. > > > To move forward, can I suggest you send a patch with a binman > > description file containing what you want. Obviously it won't work, > > due to the dependencies, but I can then figure out how to enable that > > in binman. Can you try that? > > > > I can help explaining my doubts because they're big and candid, and I > could try to explain what is needed for Rock Pi 4, but I don't think I > could explain it better than what Quentin and Jerome already did. > > I sent a dtsi with a binman description file > > https://lists.denx.de/pipermail/u-boot/2022-July/490068.html > > (you replied to that message, but maybe didn't read until the end ?) > > Your suggestion was to add ordering properties to binman. I guess we > could, but I'm not sure that's very scalable or even very data driven. > Its simpler than managing dependencies, I guess, so that's good. > > But I'm no longer sure that's what I want. Well I'm sure that's not > what I want since Jerome pointed out that that would mistreat > tee.bin. Maybe what I want is to keep using make_fit_atf.py to build > u-boot.itb and then consider u-boot.itb a binary for binman, that's > what I think Quentin already sent, before I messed it up by mentioning > make_fit_atf.py at all. Maybe just remove the message make outputs > saying CONFIG_USE_SPL_FIT_GENERATOR looks ugly ? Because that's what > lead me to the rabbit hole... > I think there is a bit much agonising over things here. I'll see if I can create a patch that addresses these points and send it out. Thank you for writing it all up. Regards, Simon
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"; + }; + }; + }; + }; simple-bin { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; @@ -62,6 +131,7 @@ #ifdef CONFIG_ARM64 blob { filename = "u-boot.itb"; + #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