[PATCH 12/12] binman: Support simple templates
Jan Kiszka
jan.kiszka at siemens.com
Fri Jul 7 18:02:56 CEST 2023
On 07.07.23 17:35, Simon Glass wrote:
> Hi Jan,
>
> On Fri, 7 Jul 2023 at 14:56, Jan Kiszka <jan.kiszka at 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 at 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 at 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 at 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 at 0 {
>>>> filename = "my-blob.bin";
>>>> offset = <0>;
>>>> };
>>>> blob-ext at 100 {
>>>> filename = "/dev/null";
>>>> offset = <0x100>;
>>>> };
>>>> };
>>>>
>>>> my-image {
>>>> filename = "my-image.bin";
>>>> insert-template = <&my_template>;
>>>> blob-ext at 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 at 0), but
>>>> this is at least ugly.
>>>>
>>>> However, the file above as-is does not build:
>>>>
>>>> binman: Node '/binman/my-image/blob-ext at 0': Offset 0x0 (0) overlaps with
>>>> previous entry '/binman/my-image/blob-ext at 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 at 0': Offset 0x0 (0) overlaps with
>> previous entry '/binman/image/blob-ext at 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.
Jan
--
Siemens AG, Technology
Competence Center Embedded Linux
More information about the U-Boot
mailing list