Hi Jan,
On Fri, 7 Jul 2023 at 11:46, Jan Kiszka jan.kiszka@siemens.com wrote:
On 07.07.23 19:35, Simon Glass wrote:
Hi Jan,
On Fri, 7 Jul 2023 at 17:03, Jan Kiszka jan.kiszka@siemens.com wrote:
On 07.07.23 17:35, Simon Glass wrote:
Hi Jan,
On Fri, 7 Jul 2023 at 14:56, Jan Kiszka jan.kiszka@siemens.com wrote:
On 07.07.23 14:42, Simon Glass wrote:
Hi Jan,
On Fri, 7 Jul 2023 at 11:05, Jan Kiszka jan.kiszka@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.kiszka@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 sjg@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.
Hmm, already the test pattern from [1] still gives me
binman: Node '/binman/image/blob-ext@0': Offset 0x0 (0) overlaps with previous entry '/binman/image/blob-ext@8' ending at 0xf (15)
I've applied this series and then [1] on top.
Can you please list the commits so I can check it? I have:
88a31cccc20 (HEAD -> mkim2, dm/mkim-working) binman: Support templating with multiple images 71f1ef46d0c (dm-public/mkim-working) binman: Support simple templates 8841829b4c8 dtoc: Allow inserting a list of nodes into another fd2fb91c616 dtoc: Support copying the contents of a node into another 288f9e7c3b6 binman: Correct handling of zero bss size 4725a43d0ef binman: Drop __bss_size variable in bss_data.c c4bfe4917c9 binman: Provide a way to specific the fdt-list directly 6e571e4d260 binman: Convert mkimage to Entry_section 5fd2892283f stm32mp15: Avoid writing symbols in SPL 24c60031f48 binman: Allow disabling symbol writing 5c6a660333c binman: Read _multiple_data_files in the correct place 24a273113bf binman: Use GetEntries() to obtain section contents 1e0659321d8 binman: Init align_default in entry_Section cc3699d1d04 binman: Correct coverage gap in control 67d8b46e6ef (us/WIP/28Jun2023-next) Merge tag 'u-boot-amlogic-next-20230628' of https://source.denx.de/u-boot/custodians/u-boot-amlogic into next
I was using the dtoc patches from this series, not the updated but apparently not yet sent ones from your branch. Those seem to make a difference. Continuing to test.
I thought I did send out v2. There are some test fails with large numbers of CPUs which I need to fix in v3, but anyway, please use the branch I pushed for testing, and sorry for any discrepancy.
Doing this now. And here is the next issue - maybe the last:
/dts-v1/;
/ { binman: binman { multiple-images;
my_template: template { fit@0 { images { kernel-1 { }; kernel-2 { }; }; }; }; image { filename = "my-image.bin"; insert-template = <&my_template>; fit@0 { images { kernel-3 { }; }; }; }; };};
Gives
binman: pylibfdt error -4: FDT_ERR_BADOFFSET
when trying to build it. That subnode kernel-3 can't be added.
OK I can repeat that. I think this is a problem with multi-level (in hierarchy) inserting of nodes. I'll need to fiddle with it so will try at weekend.
Regards, Simon