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