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