On 07.07.23 19:35, Simon Glass wrote:
> Hi Jan,
>
> On Fri, 7 Jul 2023 at 17:03, Jan Kiszka <jan.kis...@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.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
>>>
>>
>> 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.
Jan
--
Siemens AG, Technology
Competence Center Embedded Linux
