Hi Jan, On Fri, 7 Jul 2023 at 11:05, Jan Kiszka <jan.kis...@siemens.com> wrote: > > On 05.07.23 00:14, Simon Glass wrote: > > Hi Jan, > > > > On Mon, 3 Jul 2023 at 20:34, Jan Kiszka <jan.kis...@siemens.com> wrote: > >> > >> Hi Simon, > >> > >> On 28.06.23 13:41, Simon Glass wrote: > >>> Collections can used to collect the contents of other entries into a > >>> single entry, but they result in a single entry, with the original entries > >>> 'left behind' in their old place. > >>> > >>> It is useful to be able to specific a set of entries ones and have it used > >>> in multiple images, or parts of an image. > >>> > >>> Implement this mechanism. > >>> > >>> Signed-off-by: Simon Glass <s...@chromium.org> > >>> --- > >>> > >>> tools/binman/binman.rst | 80 ++++++++++++++++++++++++ > >>> tools/binman/control.py | 9 +++ > >>> tools/binman/etype/section.py | 3 +- > >>> tools/binman/ftest.py | 8 +++ > >>> tools/binman/test/286_entry_template.dts | 42 +++++++++++++ > >>> 5 files changed, 141 insertions(+), 1 deletion(-) > >>> create mode 100644 tools/binman/test/286_entry_template.dts > >>> > >>> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst > >>> index a4b31fe5397b..9be979ae1c5b 100644 > >>> --- a/tools/binman/binman.rst > >>> +++ b/tools/binman/binman.rst > >>> @@ -727,6 +727,13 @@ optional: > >>> Note that missing, optional blobs do not produce a non-zero exit > >>> code from > >>> binman, although it does show a warning about the missing external > >>> blob. > >>> > >>> +insert-template: > >>> + This is not strictly speaking an entry property, since it is > >>> processed early > >>> + in Binman before the entries are read. It is a list of phandles of > >>> nodes to > >>> + include in the current (target) node. For each node, its subnodes > >>> and their > >>> + properties are brought into the target node. See Templates_ below for > >>> + more information. > >>> + > >>> The attributes supported for images and sections are described below. > >>> Several > >>> are similar to those for entries. > >>> > >>> @@ -1172,6 +1179,79 @@ If you are having trouble figuring out what is > >>> going on, you can use > >>> arch/arm/dts/u-boot.dtsi ... found: > >>> "arch/arm/dts/juno-r2-u-boot.dtsi" > >>> > >>> > >>> +Templates > >>> +========= > >>> + > >>> +Sometimes multiple images need to be created which have all have a common > >>> +part. For example, a board may generate SPI and eMMC images which both > >>> include > >>> +a FIT. Since the FIT includes many entries, it is tedious to repeat them > >>> twice > >>> +in the image description. > >>> + > >>> +Templates provide a simple way to handle this:: > >>> + > >>> + binman { > >>> + multiple-images; > >>> + common_part: template-1 { > >>> + fit { > >>> + ... lots of entries in here > >>> + }; > >>> + > >>> + text { > >>> + text = "base image"; > >>> + }; > >>> + }; > >>> + > >>> + spi-image { > >>> + filename = "image-spi.bin"; > >>> + insert-template = <&fit>; > >>> + > >>> + /* things specific to SPI follow */ > >>> + header { > >>> + ]; > >>> + > >>> + text { > >>> + text = "SPI image"; > >>> + }; > >>> + }; > >>> + > >>> + mmc-image { > >>> + filename = "image-mmc.bin"; > >>> + insert-template = <&fit>; > >>> + > >>> + /* things specific to MMC follow */ > >>> + header { > >>> + ]; > >>> + > >>> + text { > >>> + text = "MMC image"; > >>> + }; > >>> + }; > >>> + }; > >>> + > >>> +The template node name must start with 'template', so it is not > >>> considered to be > >>> +an image itself. > >>> + > >>> +The mechanism is very simple. For each phandle in the 'insert-templates' > >>> +property, the source node is looked up. Then the subnodes of that source > >>> node > >>> +are copied into the target node, i.e. the one containing the > >>> `insert-template` > >>> +property. > >>> + > >>> +If the target node has a node with the same name as a template, its > >>> properties > >>> +override corresponding properties in the template. This allows the > >>> template to > >>> +be uses as a base, with the node providing updates to the properties as > >>> needed. > >>> +The overriding happens recursively. > >>> + > >>> +At present there is an unpleasant limitation on this feature: it works by > >>> +appending the template nodes after any existing subnodes to the target > >>> node. > >>> +This means that if the target node includes any subnodes, these appear > >>> in order > >>> +before the template node. In the above example, 'header' becomes the > >>> first > >>> +subnode of each image, followed by `fit` and `text`. If this is not what > >>> is > >>> +desired, there is no way to adjust it. > >>> + > >>> +Note: The above limitation will likely be removed in future, so that the > >>> +template subnodes appear before the target subnodes. > >>> + > >>> + > >>> Updating an ELF file > >>> ==================== > >>> > >>> diff --git a/tools/binman/control.py b/tools/binman/control.py > >>> index 68597c4e7792..e7faca78e9aa 100644 > >>> --- a/tools/binman/control.py > >>> +++ b/tools/binman/control.py > >>> @@ -22,6 +22,7 @@ from binman import bintool > >>> from binman import cbfs_util > >>> from binman import elf > >>> from binman import entry > >>> +from dtoc import fdt_util > >>> from u_boot_pylib import command > >>> from u_boot_pylib import tools > >>> from u_boot_pylib import tout > >>> @@ -478,6 +479,12 @@ def SignEntries(image_fname, input_fname, > >>> privatekey_fname, algo, entry_paths, > >>> > >>> AfterReplace(image, allow_resize=True, write_map=write_map) > >>> > >>> +def _ProcessTemplates(parent): > >>> + for node in parent.subnodes: > >>> + tmpl = fdt_util.GetPhandleList(node, 'insert-template') > >>> + if tmpl: > >>> + node.copy_subnodes_from_phandles(tmpl) > >>> + > >>> def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, > >>> use_expanded): > >>> """Prepare the images to be processed and select the device tree > >>> > >>> @@ -520,6 +527,8 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, > >>> update_fdt, use_expanded): > >>> raise ValueError("Device tree '%s' does not have a 'binman' " > >>> "node" % dtb_fname) > >>> > >>> + _ProcessTemplates(node) > >>> + > >>> images = _ReadImageDesc(node, use_expanded) > >>> > >>> if select_images: > >>> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py > >>> index d56cc11d1023..adac2ff7fa87 100644 > >>> --- a/tools/binman/etype/section.py > >>> +++ b/tools/binman/etype/section.py > >>> @@ -179,7 +179,8 @@ class Entry_section(Entry): > >>> Returns: > >>> bool: True if the node is a special one, else False > >>> """ > >>> - return node.name.startswith('hash') or > >>> node.name.startswith('signature') > >>> + start_list = ('hash', 'signature', 'template') > >>> + return any(node.name.startswith(name) for name in start_list) > >>> > >>> def ReadNode(self): > >>> """Read properties from the section node""" > >>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > >>> index 4db54c69682c..9d9e47ce26b0 100644 > >>> --- a/tools/binman/ftest.py > >>> +++ b/tools/binman/ftest.py > >>> @@ -6763,6 +6763,14 @@ fdt fdtmap Extract the > >>> devicetree blob from the fdtmap > >>> data = self._DoReadFileDtb('285_spl_expand.dts', > >>> use_expanded=True, > >>> entry_args=entry_args)[0] > >>> > >>> + def testEntryTemplate(self): > >>> + """Test using a template""" > >>> + TestFunctional._MakeInputFile('vga2.bin', b'#' + VGA_DATA) > >>> + data = self._DoReadFile('286_entry_template.dts') > >>> + first = U_BOOT_DTB_DATA + U_BOOT_DATA + VGA_DATA > >>> + second = U_BOOT_DTB_DATA + b'#' + VGA_DATA + U_BOOT_DATA > >>> + self.assertEqual(U_BOOT_IMG_DATA + first + second, data) > >>> + > >>> > >>> if __name__ == "__main__": > >>> unittest.main() > >>> diff --git a/tools/binman/test/286_entry_template.dts > >>> b/tools/binman/test/286_entry_template.dts > >>> new file mode 100644 > >>> index 000000000000..6980dbfafcc6 > >>> --- /dev/null > >>> +++ b/tools/binman/test/286_entry_template.dts > >>> @@ -0,0 +1,42 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> + > >>> +/dts-v1/; > >>> + > >>> +/ { > >>> + #address-cells = <1>; > >>> + #size-cells = <1>; > >>> + > >>> + binman { > >>> + u-boot-img { > >>> + }; > >>> + > >>> + common_part: template { > >>> + u-boot { > >>> + }; > >>> + > >>> + intel-vga { > >>> + filename = "vga.bin"; > >>> + }; > >>> + }; > >>> + > >>> + first { > >>> + type = "section"; > >>> + insert-template = <&common_part>; > >>> + > >>> + u-boot-dtb { > >>> + }; > >>> + }; > >>> + > >>> + second { > >>> + type = "section"; > >>> + insert-template = <&common_part>; > >>> + > >>> + u-boot-dtb { > >>> + }; > >>> + > >>> + intel-vga { > >>> + filename = "vga2.bin"; > >>> + }; > >>> + }; > >>> + }; > >>> +}; > >> > >> I tried to apply this on the IOT2050 use case, but it failed in two cases: > >> > >> > >> 1. We need a template where nodes can be manipulated afterwards, thus we > >> need anchors into those nodes. Naturally, those anchors need to get > >> unique names so that something like this is possible: > >> > >> common: template { > >> anchor: some-node { > >> value-1 = <1>; > >> }; > >> }; > >> > >> user-a { > >> insert-template = <&common>; > >> }; > >> > >> user-b { > >> insert-template = <&common>; > >> }; > >> > >> &anchor-a { > >> value-2 = <2>; > >> }; > >> > >> &anchor-b { > >> value-2 = <3>; > >> }; > > > > Rather than using a phandle, can you not use the node name in the > > template? For example: > > > > user-a { > > insert-template = <&common>; > > some-node { > > value2 = <2>; > > } > > > > user-b { > > insert-template = <&common>; > > some-node { > > value2 = <3>; > > } > > > > I don't think this is working already. This is what I tried: > > /dts-v1/; > / { > binman: binman { > multiple-images; > > my_template: template { > blob-ext@0 { > filename = "my-blob.bin"; > offset = <0>; > }; > blob-ext@100 { > filename = "/dev/null"; > offset = <0x100>; > }; > }; > > my-image { > filename = "my-image.bin"; > insert-template = <&my_template>; > blob-ext@100 { > filename = "my-blob2.bin"; > }; > }; > }; > }; > > First, I had to apply that null filename workaround, and the overridden > filename is taken for the final image (if I leave out blob-ext@0), but > this is at least ugly. > > However, the file above as-is does not build: > > binman: Node '/binman/my-image/blob-ext@0': Offset 0x0 (0) overlaps with > previous entry '/binman/my-image/blob-ext@100' ending at 0x107 (263) > > > Probably for the same reason, leaving fit,fdt-list-val out in a template > also generates an error during build. Putting an empty string there and > overriding it later does not work.
I think it is just that it doesn't support templating of multiple-images nodes property. I sent a patch for this [1] There may be other cases too, but if you have a failure, please see if you can adjust that test (287) to fail for you. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/patch/20230707124024.1668724-1-...@chromium.org/