Hi Simon, On 2023-06-28 13:41, Simon Glass wrote:

From: Marek Vasut marex@denx.de

This is needed to handle mkimage with inner section located itself in a section.

Signed-off-by: Marek Vasut marex@denx.de Use BuildSectionData() instead of ObtainContents(), add tests and a few other minor fixes: Signed-off-by: Simon Glass sjg@chromium.org

---

tools/binman/entry.py | 6 +- tools/binman/etype/mkimage.py | 76 ++++++++++++++--------- tools/binman/ftest.py | 45 +++++++++++++- tools/binman/test/283_mkimage_special.dts | 24 +++++++ 4 files changed, 117 insertions(+), 34 deletions(-) create mode 100644 tools/binman/test/283_mkimage_special.dts

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 328b5bc568a9..8f06fea51ad4 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -1311,10 +1311,8 @@ features to produce new behaviours. """ data = b'' for entry in entries:

•        # First get the input data and put it in a file. If not available,
  

•        # try later.
  

•        if not entry.ObtainContents(fake_size=fake_size):
  

•            return None, None, None
  

•        # First get the input data and put it in a file
  

•        entry.ObtainContents(fake_size=fake_size)
         data += entry.GetData()
     uniq = self.GetUniqueName()
     fname = tools.get_output_filename(f'{prefix}.{uniq}')
  

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py index cb3e10672ad7..8311fed59762 100644 --- a/tools/binman/etype/mkimage.py +++ b/tools/binman/etype/mkimage.py @@ -8,10 +8,11 @@ from collections import OrderedDict

from binman.entry import Entry +from binman.etype.section import Entry_section from dtoc import fdt_util from u_boot_pylib import tools

-class Entry_mkimage(Entry): +class Entry_mkimage(Entry_section): """Binary produced by mkimage

 Properties / Entry arguments:

@@ -121,10 +122,8 @@ class Entry_mkimage(Entry): """ def __init__(self, section, etype, node): super().__init__(section, etype, node)

•    self._mkimage_entries = OrderedDict()
     self._imagename = None
  

•    self._filename = fdt_util.GetString(self._node, 'filename')
  

•    self.align_default = None
  

•    self._multiple_data_files = False
  
  

  def ReadNode(self): super().ReadNode()

@@ -135,41 +134,60 @@ class Entry_mkimage(Entry): 'data-to-imagename') if self._data_to_imagename and self._node.FindNode('imagename'): self.Raise('Cannot use both imagename node and data-to-imagename')

•    self.ReadEntries()
  
  

  def ReadEntries(self): """Read the subnodes to find out what should go in this image""" for node in self._node.subnodes:

•        entry = Entry.Create(self, node)
  

•        if self.IsSpecialSubnode(node):
  

•            continue
  

•        entry = Entry.Create(self, node,
  

•                             expanded=self.GetImage().use_expanded,
  

•                             missing_etype=self.GetImage().missing_etype)
         entry.ReadNode()
  

•        entry.SetPrefix(self._name_prefix)
         if entry.name == 'imagename':
             self._imagename = entry
         else:
  

•            self._mkimage_entries[entry.name] = entry
  

•            self._entries[entry.name] = entry
  
  

• def ObtainContents(self):

• def BuildSectionData(self, required):

•    """Build mkimage entry contents
  

•    Runs mkimage to build the entry contents
  

•    Args:
  

•        required (bool): True if the data must be present, False if it is OK
  

•            to return None
  

•    Returns:
  

•        bytes: Contents of the section
  

•    """
     # Use a non-zero size for any fake files to keep mkimage happy
     # Note that testMkimageImagename() relies on this 'mkimage' parameter
     fake_size = 1024
     if self._multiple_data_files:
         fnames = []
         uniq = self.GetUniqueName()
  

•        for entry in self._mkimage_entries.values():
  

•            if not entry.ObtainContents(fake_size=fake_size):
  

•                return False
  

•            if entry._pathname:
  

•                fnames.append(entry._pathname)
  

•        for entry in self._entries.values():
  

•            entry.ObtainContents(fake_size=fake_size)
  

•            # If this is a section, put the contents in a temporary file.
  

•            # Otherwise, assume it is a blob and use the pathname
  

•            if isinstance(entry, Entry_section):
  

•                ename = f'mkimage-in-{uniq}-{entry.name}'
  

•                fname = tools.get_output_filename(ename)
  

•                tools.write_file(fname, entry.data)
  

•            elif entry._pathname:
  

•                fname = entry._pathname
  

•            fnames.append(fname)
         input_fname = ":".join(fnames)
  

•        data = b''
     else:
         data, input_fname, uniq = self.collect_contents_to_file(
  

•            self._mkimage_entries.values(), 'mkimage', fake_size)
  

•        if data is None:
  

•            return False
  

•            self._entries.values(), 'mkimage', fake_size)
     if self._imagename:
         image_data, imagename_fname, _ = self.collect_contents_to_file(
             [self._imagename], 'mkimage-n', 1024)
  

•        if image_data is None:
  

•            return False
     outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
     output_fname = tools.get_output_filename(outfile)
  
  

@@ -177,8 +195,7 @@ class Entry_mkimage(Entry): self.CheckMissing(missing_list) self.missing = bool(missing_list) if self.missing:

•        self.SetContents(b'')
  

•        return self.allow_missing
  

•        return b''
  
     args = ['-d', input_fname]
     if self._data_to_imagename:
  

@@ -187,17 +204,15 @@ class Entry_mkimage(Entry): args += ['-n', imagename_fname] args += self._args + [output_fname] if self.mkimage.run_cmd(*args) is not None:

•        self.SetContents(tools.read_file(output_fname))
  

•        return tools.read_file(output_fname)
     else:
         # Bintool is missing; just use the input data as the output
         self.record_missing_bintool(self.mkimage)
  

•        self.SetContents(data)
  

•    return True
  

•        return data
  
  

  def GetEntries(self): # Make a copy so we don't change the original

•    entries = OrderedDict(self._mkimage_entries)
  

•    entries = OrderedDict(self._entries)
     if self._imagename:
         entries['imagename'] = self._imagename
     return entries
  

@@ -209,7 +224,7 @@ class Entry_mkimage(Entry): allow_missing: True if allowed, False if not allowed """ self.allow_missing = allow_missing

•    for entry in self._mkimage_entries.values():
  

•    for entry in self._entries.values():
  

With the changes to use GetEntries() in Entry_section for this and following Check and Set functions, I suspect these can be removed and use the base implementation from Entry_section.

GetEntries() returns self._entries + self._imagename

Regards, Jonas

         entry.SetAllowMissing(allow_missing)
     if self._imagename:
         self._imagename.SetAllowMissing(allow_missing)

@@ -220,7 +235,7 @@ class Entry_mkimage(Entry): Args: allow_fake: True if allowed, False if not allowed """

•    for entry in self._mkimage_entries.values():
  

•    for entry in self._entries.values():
         entry.SetAllowFakeBlob(allow_fake)
     if self._imagename:
         self._imagename.SetAllowFakeBlob(allow_fake)
  

@@ -234,7 +249,7 @@ class Entry_mkimage(Entry): Args: missing_list: List of Entry objects to be added to """

•    for entry in self._mkimage_entries.values():
  

•    for entry in self._entries.values():
         entry.CheckMissing(missing_list)
     if self._imagename:
         self._imagename.CheckMissing(missing_list)
  

@@ -247,7 +262,7 @@ class Entry_mkimage(Entry): Args: faked_blobs_list: List of Entry objects to be added to """

•    for entry in self._mkimage_entries.values():
  

•    for entry in self._entries.values():
         entry.CheckFakedBlobs(faked_blobs_list)
     if self._imagename:
         self._imagename.CheckFakedBlobs(faked_blobs_list)
  

@@ -255,3 +270,6 @@ class Entry_mkimage(Entry): def AddBintools(self, btools): super().AddBintools(btools) self.mkimage = self.AddBintool(btools, 'mkimage')

• def CheckEntries(self):

•    pass
  

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index dabb3f689fdb..6d0ffda2f432 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3737,6 +3737,7 @@ class TestFunctional(unittest.TestCase):

 def testMkimage(self):
     """Test using mkimage to build an image"""

•    self._SetupSplElf()
     data = self._DoReadFile('156_mkimage.dts')
  
     # Just check that the data appears in the file somewhere
  

@@ -3744,6 +3745,7 @@ class TestFunctional(unittest.TestCase):

 def testMkimageMissing(self):
     """Test that binman still produces an image if mkimage is missing"""

•    self._SetupSplElf()
     with test_util.capture_sys_output() as (_, stderr):
         self._DoTestFile('156_mkimage.dts',
                          force_missing_bintools='mkimage')
  

@@ -5952,6 +5954,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap

 def testMkimageCollection(self):
     """Test using a collection referring to an entry in a mkimage entry"""

•    self._SetupSplElf()
     data = self._DoReadFile('247_mkimage_coll.dts')
     expect = U_BOOT_SPL_DATA + U_BOOT_DATA
     self.assertEqual(expect, data[:len(expect)])
  

@@ -6051,6 +6054,39 @@ fdt fdtmap Extract the devicetree blob from the fdtmap expect += U_BOOT_SPL_DATA self.assertEqual(expect, data[-len(expect):])

• def testMkimageMultipleExpanded(self):

•    """Test passing multiple files to mkimage in a mkimage entry"""
  

•    self._SetupSplElf()
  

•    self._SetupTplElf()
  

•    entry_args = {
  

•        'spl-bss-pad': 'y',
  

•        'spl-dtb': 'y',
  

•    }
  

•    data = self._DoReadFileDtb('252_mkimage_mult_data.dts',
  

•                               use_expanded=True, entry_args=entry_args)[0]
  

•    pad_len = 10
  

•    tpl_expect = U_BOOT_TPL_DATA
  

•    spl_expect = U_BOOT_SPL_NODTB_DATA + tools.get_bytes(0, pad_len)
  

•    spl_expect += U_BOOT_SPL_DTB_DATA
  

•    content = data[0x40:]
  

•    lens = struct.unpack('>III', content[:12])
  

•    # Size of files are packed in their 4B big-endian format
  

•    # Size info is always followed by a 4B zero value.
  

•    self.assertEqual(len(tpl_expect), lens[0])
  

•    self.assertEqual(len(spl_expect), lens[1])
  

•    self.assertEqual(0, lens[2])
  

•    rest = content[12:]
  

•    self.assertEqual(tpl_expect, rest[:len(tpl_expect)])
  

•    rest = rest[len(tpl_expect):]
  

•    align_pad = len(tpl_expect) % 4
  

•    self.assertEqual(tools.get_bytes(0, align_pad), rest[:align_pad])
  

•    rest = rest[align_pad:]
  

•    self.assertEqual(spl_expect, rest)
  

• def testMkimageMultipleNoContent(self): """Test passing multiple data files to mkimage with one data file having no content""" with self.assertRaises(ValueError) as exc:

@@ -6696,6 +6732,13 @@ fdt fdtmap Extract the devicetree blob from the fdtmap entry_args=entry_args, use_expanded=True, no_write_symbols=True)

• def testMkimageSpecial(self):

•    """Test mkimage ignores special hash-1 node"""
  

•    data = self._DoReadFile('283_mkimage_special.dts')
  

•    # Just check that the data appears in the file somewhere
  

•    self.assertIn(U_BOOT_DATA, data)
  

-if __name__ == "__main__": +if __name__ == "_s_main__": unittest.main() diff --git a/tools/binman/test/283_mkimage_special.dts b/tools/binman/test/283_mkimage_special.dts new file mode 100644 index 000000000000..c234093e6ece --- /dev/null +++ b/tools/binman/test/283_mkimage_special.dts @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+/ {

• #address-cells = <1>;
• #size-cells = <1>;
• binman {

• mkimage {
  

• 	args = "-T script";
  

• 	u-boot {
  

• 	};
  

• 	hash {
  

• 	};
  

• 	imagename {
  

• 		type = "u-boot";
  

• 	};
  

• };
  

• };

+};

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>; }

2. We also have the need to customize a value in a generator sequence

that is part of the template. See IOT2050_FLASH_EXTRA_LOADABLES in @config-SEQ in [1].

Again, can you not just override it in the user, as above?

Regards, Simon

Jan

[1] https://github.com/siemens/u-boot/commit/393ce2b78cee9214edda7f7e04f6ca2ce14...

-- Siemens AG, Technology Competence Center Embedded Linux

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.

Regards, Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230707124024.1668724-1-sj...

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

Regards, Simon

Jan

...

Regards, Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230707124024.1668724-1-sj...

-- Siemens AG, Technology Competence Center Embedded Linux

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.

Regards, Simon

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