Two test devicetree files currently have 192 as their unique number. Fix this by separating them out.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/ftest.py | 2 +- .../test/{192_symbols_nodtb.dts => 196_symbols_nodtb.dts} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename tools/binman/test/{192_symbols_nodtb.dts => 196_symbols_nodtb.dts} (100%)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 81c213a908a..34449553ca0 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1377,7 +1377,7 @@ class TestFunctional(unittest.TestCase):

def testSymbolsNoDtb(self): """Test binman can assign symbols embedded in U-Boot SPL""" - self.checkSymbols('192_symbols_nodtb.dts', + self.checkSymbols('196_symbols_nodtb.dts', U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA, 0x38)

diff --git a/tools/binman/test/192_symbols_nodtb.dts b/tools/binman/test/196_symbols_nodtb.dts similarity index 100% rename from tools/binman/test/192_symbols_nodtb.dts rename to tools/binman/test/196_symbols_nodtb.dts

At present there is a command-line flag to disable substitution of expanded entries. Add an option to the entry node as well, so it can be controlled at the node level.

Add a test to cover this. Fix up the comment to the checkSymbols() function it uses, while we are here.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/binman.rst | 7 +++++++ tools/binman/entries.rst | 6 +++--- tools/binman/entry.py | 3 ++- tools/binman/etype/u_boot.py | 2 +- tools/binman/etype/u_boot_spl.py | 2 +- tools/binman/etype/u_boot_tpl.py | 2 +- tools/binman/ftest.py | 20 ++++++++++++++++++-- tools/binman/test/197_symbols_expand.dts | 23 +++++++++++++++++++++++ 8 files changed, 56 insertions(+), 9 deletions(-) create mode 100644 tools/binman/test/197_symbols_expand.dts

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 15314d19586..28cb2e7a9c9 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -472,6 +472,13 @@ missing-msg: information about what needs to be fixed. See missing-blob-help for the message for each tag.

+no-expanded: + By default binman substitutes entries with expanded versions if available, + so that a `u-boot` entry type turns into `u-boot-expanded`, for example. The + `--no-expanded` command-line option disables this globally. The + `no-expanded` property disables this just for a single entry. Put the + `no-expanded` boolean property in the node to select this behaviour. + The attributes supported for images and sections are described below. Several are similar to those for entries.

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index f6faa15562f..1a71413f940 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -863,7 +863,7 @@ U-Boot can access binman symbols at runtime. See: in the binman README for more information.

Note that this entry is automatically replaced with u-boot-expanded unless ---no-expanded is used. +--no-expanded is used or the node has a 'no-expanded' property.

@@ -984,7 +984,7 @@ The ELF file 'spl/u-boot-spl' must also be available for this to work, since binman uses that to look up symbols to write into the SPL binary.

Note that this entry is automatically replaced with u-boot-spl-expanded -unless --no-expanded is used. +unless --no-expanded is used or the node has a 'no-expanded' property.

@@ -1113,7 +1113,7 @@ The ELF file 'tpl/u-boot-tpl' must also be available for this to work, since binman uses that to look up symbols to write into the TPL binary.

Note that this entry is automatically replaced with u-boot-tpl-expanded -unless --no-expanded is used. +unless --no-expanded is used or the node has a 'no-expanded' property.

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 1cfa024a9f9..ac25986ee6e 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -164,7 +164,8 @@ class Entry(object): if obj and expanded: # Check whether to use the expanded entry new_etype = etype + '-expanded' - if obj.UseExpanded(node, etype, new_etype): + can_expand = not fdt_util.GetBool(node, 'no-expanded') + if can_expand and obj.UseExpanded(node, etype, new_etype): etype = new_etype else: obj = None diff --git a/tools/binman/etype/u_boot.py b/tools/binman/etype/u_boot.py index d2eaba6d4aa..e8d180a46dd 100644 --- a/tools/binman/etype/u_boot.py +++ b/tools/binman/etype/u_boot.py @@ -25,7 +25,7 @@ class Entry_u_boot(Entry_blob): in the binman README for more information.

Note that this entry is automatically replaced with u-boot-expanded unless - --no-expanded is used. + --no-expanded is used or the node has a 'no-expanded' property. """ def __init__(self, section, etype, node): super().__init__(section, etype, node) diff --git a/tools/binman/etype/u_boot_spl.py b/tools/binman/etype/u_boot_spl.py index 1c39f982519..6f79bf59f9f 100644 --- a/tools/binman/etype/u_boot_spl.py +++ b/tools/binman/etype/u_boot_spl.py @@ -32,7 +32,7 @@ class Entry_u_boot_spl(Entry_blob): binman uses that to look up symbols to write into the SPL binary.

Note that this entry is automatically replaced with u-boot-spl-expanded - unless --no-expanded is used. + unless --no-expanded is used or the node has a 'no-expanded' property. """ def __init__(self, section, etype, node): super().__init__(section, etype, node) diff --git a/tools/binman/etype/u_boot_tpl.py b/tools/binman/etype/u_boot_tpl.py index 95d9bd6b20e..0c575df8cdc 100644 --- a/tools/binman/etype/u_boot_tpl.py +++ b/tools/binman/etype/u_boot_tpl.py @@ -32,7 +32,7 @@ class Entry_u_boot_tpl(Entry_blob): binman uses that to look up symbols to write into the TPL binary.

Note that this entry is automatically replaced with u-boot-tpl-expanded - unless --no-expanded is used. + unless --no-expanded is used or the node has a 'no-expanded' property. """ def __init__(self, section, etype, node): super().__init__(section, etype, node) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 34449553ca0..cd93dc153a7 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1344,13 +1344,19 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('052_u_boot_spl_nodtb.dts') self.assertEqual(U_BOOT_SPL_NODTB_DATA, data[:len(U_BOOT_SPL_NODTB_DATA)])

- def checkSymbols(self, dts, base_data, u_boot_offset): + def checkSymbols(self, dts, base_data, u_boot_offset, entry_args=None, + use_expanded=False): """Check the image contains the expected symbol values

Args: dts: Device tree file to use for test base_data: Data before and after 'u-boot' section u_boot_offset: Offset of 'u-boot' section in image + entry_args: Dict of entry args to supply to binman + key: arg name + value: value of that arg + use_expanded: True to use expanded entries where available, e.g. + 'u-boot-expanded' instead of 'u-boot' """ elf_fname = self.ElfTestFile('u_boot_binman_syms') syms = elf.GetSymbols(elf_fname, ['binman', 'image']) @@ -1359,7 +1365,8 @@ class TestFunctional(unittest.TestCase): addr)

self._SetupSplElf('u_boot_binman_syms') - data = self._DoReadFile(dts) + data = self._DoReadFileDtb(dts, entry_args=entry_args, + use_expanded=use_expanded)[0] # The image should contain the symbols from u_boot_binman_syms.c # Note that image_pos is adjusted by the base address of the image, # which is 0x10 in our test image @@ -4460,5 +4467,14 @@ class TestFunctional(unittest.TestCase): start += fdt_size + len(U_BOOT_TPL_DATA) self.assertEqual(len(data), start)

+ def testSymbolsExpanded(self): + """Test binman can assign symbols in expanded entries""" + entry_args = { + 'spl-dtb': '1', + } + self.checkSymbols('197_symbols_expand.dts', U_BOOT_SPL_NODTB_DATA + + U_BOOT_SPL_DTB_DATA, 0x38, + entry_args=entry_args, use_expanded=True) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/197_symbols_expand.dts b/tools/binman/test/197_symbols_expand.dts new file mode 100644 index 00000000000..8aee76dc755 --- /dev/null +++ b/tools/binman/test/197_symbols_expand.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + }; + + u-boot { + offset = <0x38>; + no-expanded; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +};

The vblock entry type includes code to collect the data from a number of other entries (not necessarily subentries) and concatenating it. This is a useful feature for other entry types.

Make it a base class, so that vblock can use it, along with other entry types.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/entries.rst | 13 ++++++ tools/binman/entry.py | 5 +++ tools/binman/etype/collection.py | 61 ++++++++++++++++++++++++++++ tools/binman/etype/vblock.py | 26 ++++++------ tools/binman/ftest.py | 10 ++++- tools/binman/test/198_collection.dts | 27 ++++++++++++ 6 files changed, 128 insertions(+), 14 deletions(-) create mode 100644 tools/binman/etype/collection.py create mode 100644 tools/binman/test/198_collection.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index 1a71413f940..d5f8d95dc19 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -248,6 +248,19 @@ both of size 1MB.

+Entry: collection: An entry which contains a collection of other entries +------------------------------------------------------------------------ + +Properties / Entry arguments: + - content: List of phandles to entries to include + +This allows reusing the contents of other entries. The contents of the +listed entries are combined to form this entry. This serves as a useful +base class for entry types which need to process data from elsewhere in +the image, not necessarily child entries. + + + Entry: cros-ec-rw: A blob entry which contains a Chromium OS read-write EC image --------------------------------------------------------------------------------

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index ac25986ee6e..a157038d4e3 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -438,6 +438,11 @@ class Entry(object): """Convenience function to raise an error referencing a node""" raise ValueError("Node '%s': %s" % (self._node.path, msg))

+ def Info(self, msg): + """Convenience function to log info referencing a node""" + tag = "Info '%s'" % self._node.path + tout.Detail('%30s: %s' % (tag, msg)) + def Detail(self, msg): """Convenience function to log detail referencing a node""" tag = "Node '%s'" % self._node.path diff --git a/tools/binman/etype/collection.py b/tools/binman/etype/collection.py new file mode 100644 index 00000000000..c0c552ac4f3 --- /dev/null +++ b/tools/binman/etype/collection.py @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright 2021 Google LLC +# Written by Simon Glass sjg@chromium.org +# + +# Support for a collection of entries from other parts of an image + +from collections import OrderedDict +import os + +from binman.entry import Entry +from dtoc import fdt_util + +class Entry_collection(Entry): + """An entry which contains a collection of other entries + + Properties / Entry arguments: + - content: List of phandles to entries to include + + This allows reusing the contents of other entries. The contents of the + listed entries are combined to form this entry. This serves as a useful + base class for entry types which need to process data from elsewhere in + the image, not necessarily child entries. + """ + def __init__(self, section, etype, node): + super().__init__(section, etype, node) + self.content = fdt_util.GetPhandleList(self._node, 'content') + if not self.content: + self.Raise("Collection must have a 'content' property") + + def GetContents(self): + """Get the contents of this entry + + Returns: + bytes content of the entry + """ + # Join up all the data + self.Info('Getting content') + data = b'' + for entry_phandle in self.content: + entry_data = self.section.GetContentsByPhandle(entry_phandle, self) + if entry_data is None: + # Data not available yet + return None + data += entry_data + + self.Info('Returning contents size %x' % len(data)) + + return data + + def ObtainContents(self): + data = self.GetContents() + if data is None: + return False + self.SetContents(data) + return True + + def ProcessContents(self): + # The blob may have changed due to WriteSymbols() + data = self.GetContents() + return self.ProcessContentsUpdate(data) diff --git a/tools/binman/etype/vblock.py b/tools/binman/etype/vblock.py index eba5351dd52..d473083cab8 100644 --- a/tools/binman/etype/vblock.py +++ b/tools/binman/etype/vblock.py @@ -9,12 +9,13 @@ from collections import OrderedDict import os

-from binman.entry import Entry, EntryArg +from binman.entry import EntryArg +from binman.etype.collection import Entry_collection

from dtoc import fdt_util from patman import tools

-class Entry_vblock(Entry): +class Entry_vblock(Entry_collection): """An entry which contains a Chromium OS verified boot block

Properties / Entry arguments: @@ -37,9 +38,6 @@ class Entry_vblock(Entry): """ def __init__(self, section, etype, node): super().__init__(section, etype, node) - self.content = fdt_util.GetPhandleList(self._node, 'content') - if not self.content: - self.Raise("Vblock must have a 'content' property") (self.keydir, self.keyblock, self.signprivate, self.version, self.kernelkey, self.preamble_flags) = self.GetEntryArgsOrProps([ EntryArg('keydir', str), @@ -50,14 +48,16 @@ class Entry_vblock(Entry): EntryArg('preamble-flags', int)])

def GetVblock(self): + """Get the contents of this entry + + Returns: + bytes content of the entry, which is the signed vblock for the + provided data + """ # Join up the data files to be signed - input_data = b'' - for entry_phandle in self.content: - data = self.section.GetContentsByPhandle(entry_phandle, self) - if data is None: - # Data not available yet - return False - input_data += data + input_data = self.GetContents() + if input_data is None: + return None

uniq = self.GetUniqueName() output_fname = tools.GetOutputFilename('vblock.%s' % uniq) @@ -80,7 +80,7 @@ class Entry_vblock(Entry):

def ObtainContents(self): data = self.GetVblock() - if data is False: + if data is None: return False self.SetContents(data) return True diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index cd93dc153a7..fdd4f4d2fad 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1718,7 +1718,7 @@ class TestFunctional(unittest.TestCase): """Test we detect a vblock which has no content to sign""" with self.assertRaises(ValueError) as e: self._DoReadFile('075_vblock_no_content.dts') - self.assertIn("Node '/binman/vblock': Vblock must have a 'content' " + self.assertIn("Node '/binman/vblock': Collection must have a 'content' " 'property', str(e.exception))

def testVblockBadPhandle(self): @@ -4476,5 +4476,13 @@ class TestFunctional(unittest.TestCase): U_BOOT_SPL_DTB_DATA, 0x38, entry_args=entry_args, use_expanded=True)

+ def testCollection(self): + """Test a collection""" + data = self._DoReadFile('198_collection.dts') + self.assertEqual(U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA + + tools.GetBytes(0xff, 2) + U_BOOT_NODTB_DATA + + tools.GetBytes(0xfe, 3) + U_BOOT_DTB_DATA, + data) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/198_collection.dts b/tools/binman/test/198_collection.dts new file mode 100644 index 00000000000..484a1b0050d --- /dev/null +++ b/tools/binman/test/198_collection.dts @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + collection { + content = <&u_boot_nodtb &dtb>; + }; + fill { + size = <2>; + fill-byte = [ff]; + }; + u_boot_nodtb: u-boot-nodtb { + }; + fill2 { + type = "fill"; + size = <3>; + fill-byte = [fe]; + }; + dtb: u-boot-dtb { + }; + }; +};

Generally the content of sections is not built until the final assembly of the image. This is partly to avoid wasting time, since the entries within sections may change multiple times as binman works through its various stages. This works quite well since sections exist in a strict hierarchy, so they can be processed in a depth-first manner.

However the 'collection' entry type does not have this luxury. If it contains a section within its 'content' list, then it must produce the section contents, if available. That section is typically a sibling node, i.e. not part oc the collection's hierarchy.

Add a new 'required' argument to section.GetData() to support this. When required is True, any referenced sections are immediately built. If this is not possible (because one of the subentries does not have its data yet) then an error is produced.

The test for this uses a 'collection' entry type, referencing a section as its first member. This forces a call to _BuildSectionData() with required set to False, at first, then True later, when the image is assembled.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/entry.py | 6 +++- tools/binman/etype/collection.py | 18 +++++++---- tools/binman/etype/section.py | 33 +++++++++++++++----- tools/binman/etype/vblock.py | 12 ++++--- tools/binman/ftest.py | 13 ++++++++ tools/binman/test/199_collection_section.dts | 32 +++++++++++++++++++ 6 files changed, 95 insertions(+), 19 deletions(-) create mode 100644 tools/binman/test/199_collection_section.dts

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index a157038d4e3..b7b9791b10d 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -482,9 +482,13 @@ class Entry(object): """ return self._node.path

- def GetData(self): + def GetData(self, required=True): """Get the contents of an entry

+ Args: + required: True if the data must be present, False if it is OK to + return None + Returns: bytes content of the entry, excluding any padding. If the entry is compressed, the compressed data is returned diff --git a/tools/binman/etype/collection.py b/tools/binman/etype/collection.py index c0c552ac4f3..1625575fe98 100644 --- a/tools/binman/etype/collection.py +++ b/tools/binman/etype/collection.py @@ -28,18 +28,24 @@ class Entry_collection(Entry): if not self.content: self.Raise("Collection must have a 'content' property")

- def GetContents(self): + def GetContents(self, required): """Get the contents of this entry

+ Args: + required: True if the data must be present, False if it is OK to + return None + Returns: bytes content of the entry """ # Join up all the data - self.Info('Getting content') + self.Info('Getting contents, required=%s' % required) data = b'' for entry_phandle in self.content: - entry_data = self.section.GetContentsByPhandle(entry_phandle, self) - if entry_data is None: + entry_data = self.section.GetContentsByPhandle(entry_phandle, self, + required) + if not required and entry_data is None: + self.Info('Contents not available yet') # Data not available yet return None data += entry_data @@ -49,7 +55,7 @@ class Entry_collection(Entry): return data

def ObtainContents(self): - data = self.GetContents() + data = self.GetContents(False) if data is None: return False self.SetContents(data) @@ -57,5 +63,5 @@ class Entry_collection(Entry):

def ProcessContents(self): # The blob may have changed due to WriteSymbols() - data = self.GetContents() + data = self.GetContents(True) return self.ProcessContentsUpdate(data) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index cce1500b4e5..d4a097b94bf 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -180,7 +180,7 @@ class Entry_section(Entry):

return data

- def _BuildSectionData(self): + def _BuildSectionData(self, required): """Build the contents of a section

This places all entries at the right place, dealing with padding before @@ -188,13 +188,20 @@ class Entry_section(Entry): pad-before and pad-after properties in the section items) since that is handled by the parent section.

+ Args: + required: True if the data must be present, False if it is OK to + return None + Returns: Contents of the section (bytes) """ section_data = b''

for entry in self._entries.values(): - data = self.GetPaddedDataForEntry(entry, entry.GetData()) + entry_data = entry.GetData(required) + if not required and entry_data is None: + return None + data = self.GetPaddedDataForEntry(entry, entry_data) # Handle empty space before the entry pad = (entry.offset or 0) - self._skip_at_start - len(section_data) if pad > 0: @@ -226,18 +233,24 @@ class Entry_section(Entry): data = self.GetData() return section.GetPaddedDataForEntry(self, data)

- def GetData(self): + def GetData(self, required=True): """Get the contents of an entry

This builds the contents of the section, stores this as the contents of the section and returns it

+ Args: + required: True if the data must be present, False if it is OK to + return None + Returns: bytes content of the section, made up for all all of its subentries. This excludes any padding. If the section is compressed, the compressed data is returned """ - data = self._BuildSectionData() + data = self._BuildSectionData(required) + if data is None: + return None self.SetContents(data) return data

@@ -263,7 +276,7 @@ class Entry_section(Entry): self._SortEntries() self._ExpandEntries()

- data = self._BuildSectionData() + data = self._BuildSectionData(True) self.SetContents(data)

self.CheckSize() @@ -360,16 +373,20 @@ class Entry_section(Entry): def GetEntries(self): return self._entries

- def GetContentsByPhandle(self, phandle, source_entry): + def GetContentsByPhandle(self, phandle, source_entry, required): """Get the data contents of an entry specified by a phandle

This uses a phandle to look up a node and and find the entry - associated with it. Then it returnst he contents of that entry. + associated with it. Then it returns the contents of that entry. + + The node must be a direct subnode of this section.

Args: phandle: Phandle to look up (integer) source_entry: Entry containing that phandle (used for error reporting) + required: True if the data must be present, False if it is OK to + return None

Returns: data from associated entry (as a string), or None if not found @@ -379,7 +396,7 @@ class Entry_section(Entry): source_entry.Raise("Cannot find node for phandle %d" % phandle) for entry in self._entries.values(): if entry._node == node: - return entry.GetData() + return entry.GetData(required) source_entry.Raise("Cannot find entry for node '%s'" % node.name)

def LookupSymbol(self, sym_name, optional, msg, base_addr, entries=None): diff --git a/tools/binman/etype/vblock.py b/tools/binman/etype/vblock.py index d473083cab8..c0a6a28c943 100644 --- a/tools/binman/etype/vblock.py +++ b/tools/binman/etype/vblock.py @@ -47,15 +47,19 @@ class Entry_vblock(Entry_collection): EntryArg('kernelkey', str), EntryArg('preamble-flags', int)])

- def GetVblock(self): + def GetVblock(self, required): """Get the contents of this entry

+ Args: + required: True if the data must be present, False if it is OK to + return None + Returns: bytes content of the entry, which is the signed vblock for the provided data """ # Join up the data files to be signed - input_data = self.GetContents() + input_data = self.GetContents(required) if input_data is None: return None

@@ -79,7 +83,7 @@ class Entry_vblock(Entry_collection): return tools.ReadFile(output_fname)

def ObtainContents(self): - data = self.GetVblock() + data = self.GetVblock(False) if data is None: return False self.SetContents(data) @@ -87,5 +91,5 @@ class Entry_vblock(Entry_collection):

def ProcessContents(self): # The blob may have changed due to WriteSymbols() - data = self.GetVblock() + data = self.GetVblock(True) return self.ProcessContentsUpdate(data) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index fdd4f4d2fad..043b1b037c4 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -4484,5 +4484,18 @@ class TestFunctional(unittest.TestCase): tools.GetBytes(0xfe, 3) + U_BOOT_DTB_DATA, data)

+ def testCollectionSection(self): + """Test a collection where a section must be built first""" + # Sections never have their contents when GetData() is called, but when + # _BuildSectionData() is called with required=True, a section will force + # building the contents, producing an error is anything is still + # missing. + data = self._DoReadFile('199_collection_section.dts') + section = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA + self.assertEqual(section + U_BOOT_DATA + tools.GetBytes(0xff, 2) + + section + tools.GetBytes(0xfe, 3) + U_BOOT_DATA, + data) + + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/199_collection_section.dts b/tools/binman/test/199_collection_section.dts new file mode 100644 index 00000000000..03a73194c3f --- /dev/null +++ b/tools/binman/test/199_collection_section.dts @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + collection { + content = <&section &u_boot>; + }; + fill { + size = <2>; + fill-byte = [ff]; + }; + section: section { + u-boot-nodtb { + }; + u-boot-dtb { + }; + }; + fill2 { + type = "fill"; + size = <3>; + fill-byte = [fe]; + }; + u_boot: u-boot { + no-expanded; + }; + }; +};

Sometimes it is useful to specify the default alignment for all entries in a section, such as when word-alignment is necessary, for example. It is tedious and error-prone to specify this individually for each section.

Add a property to control this for a section.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/binman.rst | 7 ++++++ tools/binman/entries.rst | 2 ++ tools/binman/entry.py | 2 ++ tools/binman/etype/cbfs.py | 1 + tools/binman/etype/mkimage.py | 1 + tools/binman/etype/section.py | 3 +++ tools/binman/ftest.py | 12 ++++++++++ tools/binman/test/200_align_default.dts | 30 +++++++++++++++++++++++++ 8 files changed, 58 insertions(+) create mode 100644 tools/binman/test/200_align_default.dts

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 28cb2e7a9c9..1aa2459d50c 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -562,6 +562,13 @@ skip-at-start: 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE + Image size != 4gb.

+align-default: + Specifies the default alignment for entries in this section, if they do + not specify an alignment. Note that this only applies to top-level entries + in the section (direct subentries), not any subentries of those entries. + This means that each section must specify its own default alignment, if + required. + Examples of the above options can be found in the tests. See the tools/binman/test directory.

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index d5f8d95dc19..a91211e93ed 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -796,6 +796,8 @@ Properties / Entry arguments: (see binman README for more information): file, since the first 16 bytes are skipped when writing. name-prefix: Adds a prefix to the name of every entry in the section when writing out the map + align_default: Default alignment for this section, if no alignment is + given in the entry

Properties: allow_missing: True if this section permits external blobs to be diff --git a/tools/binman/entry.py b/tools/binman/entry.py index b7b9791b10d..70222718ea9 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -201,6 +201,8 @@ class Entry(object): if tools.NotPowerOfTwo(self.align): raise ValueError("Node '%s': Alignment %s must be a power of two" % (self._node.path, self.align)) + if self.section and self.align is None: + self.align = self.section.align_default self.pad_before = fdt_util.GetInt(self._node, 'pad-before', 0) self.pad_after = fdt_util.GetInt(self._node, 'pad-after', 0) self.align_size = fdt_util.GetInt(self._node, 'align-size') diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 1daddeb8229..44db7b9bb20 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -169,6 +169,7 @@ class Entry_cbfs(Entry):

super().__init__(section, etype, node) self._cbfs_arg = fdt_util.GetString(node, 'cbfs-arch', 'x86') + self.align_default = None self._cbfs_entries = OrderedDict() self._ReadSubnodes() self.reader = None diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py index e9f82729ab4..e49977522e3 100644 --- a/tools/binman/etype/mkimage.py +++ b/tools/binman/etype/mkimage.py @@ -36,6 +36,7 @@ class Entry_mkimage(Entry): super().__init__(section, etype, node) self._args = fdt_util.GetString(self._node, 'args').split(' ') self._mkimage_entries = OrderedDict() + self.align_default = None self._ReadSubnodes()

def ObtainContents(self): diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index d4a097b94bf..c3bac026c14 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -36,6 +36,8 @@ class Entry_section(Entry): file, since the first 16 bytes are skipped when writing. name-prefix: Adds a prefix to the name of every entry in the section when writing out the map + align_default: Default alignment for this section, if no alignment is + given in the entry

Properties: allow_missing: True if this section permits external blobs to be @@ -76,6 +78,7 @@ class Entry_section(Entry): if self._skip_at_start is None: self._skip_at_start = 0 self._name_prefix = fdt_util.GetString(self._node, 'name-prefix') + self.align_default = fdt_util.GetInt(self._node, 'align-default', 0) filename = fdt_util.GetString(self._node, 'filename') if filename: self._filename = filename diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 043b1b037c4..89fe6612e1b 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -4496,6 +4496,18 @@ class TestFunctional(unittest.TestCase): section + tools.GetBytes(0xfe, 3) + U_BOOT_DATA, data)

+ def testAlignDefault(self): + """Test that default alignment works on sections""" + data = self._DoReadFile('200_align_default.dts') + expected = (U_BOOT_DATA + tools.GetBytes(0, 8 - len(U_BOOT_DATA)) + + U_BOOT_DATA) + # Special alignment for section + expected += tools.GetBytes(0, 32 - len(expected)) + # No alignment within the nested section + expected += U_BOOT_DATA + U_BOOT_NODTB_DATA; + # Now the final piece, which should be default-aligned + expected += tools.GetBytes(0, 88 - len(expected)) + U_BOOT_NODTB_DATA + self.assertEqual(expected, data)

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/200_align_default.dts b/tools/binman/test/200_align_default.dts new file mode 100644 index 00000000000..1b155770d4c --- /dev/null +++ b/tools/binman/test/200_align_default.dts @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + align-default = <8>; + u-boot { + }; + + u-boot-align { + type = "u-boot"; + }; + + section { + align = <32>; + u-boot { + }; + + u-boot-nodtb { + }; + }; + + u-boot-nodtb { + }; + }; +};

Add the node name too so it is easy to see which node failed.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 4 ++-- tools/dtoc/test_fdt.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 25ce5136ebf..f0d1384ccc3 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -339,8 +339,8 @@ class Node: p = fdt_obj.get_property_by_offset(poffset) prop = self.props.get(p.name) if not prop: - raise ValueError("Internal error, property '%s' missing, " - 'offset %d' % (p.name, poffset)) + raise ValueError("Internal error, node '%s' property '%s' missing, " + 'offset %d' % (self.path, p.name, poffset)) prop.RefreshOffset(poffset) poffset = fdt_obj.next_property_offset(poffset, QUIET_NOTFOUND)

diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 1c3a8a2ab1e..72095b05434 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -209,7 +209,7 @@ class TestNode(unittest.TestCase): del self.node.props['notstring'] with self.assertRaises(ValueError) as e: self.dtb.Refresh() - self.assertIn("Internal error, property 'notstring' missing, offset ", + self.assertIn("Internal error, node '/spl-test' property 'notstring' missing, offset ", str(e.exception))

def testLookupPhandle(self):

If a property does not yet have an offset, then that means it exists in the cache'd fdt but has not yet been synced back to the flat tree. Use the dirty flag for this so we don't need to check the offset too. Improve the comments for Prop and Node to make it clear what an offset of None means.

Also clear the dirty flag after the property is synced.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index f0d1384ccc3..36993c29ca4 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -103,6 +103,8 @@ class Prop: """A device tree property

Properties: + node: Node containing this property + offset: Offset of the property (None if still to be synced) name: Property name (as per the device tree) value: Property value as a string of bytes, or a list of strings of bytes @@ -114,7 +116,7 @@ class Prop: self.name = name self.value = None self.bytes = bytes(data) - self.dirty = False + self.dirty = offset is None if not data: self.type = Type.BOOL self.value = True @@ -228,7 +230,7 @@ class Prop: Raises: FdtException if auto_resize is False and there is not enough space """ - if self._offset is None or self.dirty: + if self.dirty: node = self._node fdt_obj = node._fdt._fdt_obj if auto_resize: @@ -239,13 +241,15 @@ class Prop: fdt_obj.setprop(node.Offset(), self.name, self.bytes) else: fdt_obj.setprop(node.Offset(), self.name, self.bytes) + self.dirty = False

class Node: """A device tree node

Properties: - offset: Integer offset in the device tree + parent: Parent Node + offset: Integer offset in the device tree (None if to be synced) name: Device tree node tname path: Full path to node, along with the node name itself _fdt: Device tree object

Once the tree has been synced, thus potentially moving things around in the fdt, we set _cached_offsets to False so that a refresh will happen next time a property is accessed.

This 'lazy' refresh doesn't really save much time, since refresh is a very fast operation, just a single walk of the tree. Also, having the refresh happen in the bowels of property access it makes it harder to figure out what is going on.

Simplify the code by always doing a refresh before and after a sync. Set _cached_offsets to True immediately after this, in the Refresh() function, since this makes more sense than doing it in the caller.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 36993c29ca4..a5e1d0b52f6 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -646,8 +646,9 @@ class Fdt: Raises: FdtException if auto_resize is False and there is not enough space """ + self.CheckCache() self._root.Sync(auto_resize) - self.Invalidate() + self.Refresh()

def Pack(self): """Pack the device tree down to its minimum size @@ -656,7 +657,7 @@ class Fdt: build up in the device tree binary. """ CheckErr(self._fdt_obj.pack(), 'pack') - self.Invalidate() + self.Refresh()

def GetContents(self): """Get the contents of the FDT @@ -708,11 +709,11 @@ class Fdt: if self._cached_offsets: return self.Refresh() - self._cached_offsets = True

def Refresh(self): """Refresh the offset cache""" self._root.Refresh(0) + self._cached_offsets = True

def GetStructOffset(self, offset): """Get the file offset of a given struct offset

Add a new test that adds a subnode alongside an existing one, as well as adding properties to a subnode. This will expand to adding multiple subnodes in future patches. Put a node after the one we are adding to so we can check that things sync correctly.

The testAddNode() test should be in the TestNode class since it is a node test, so move it.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/test/dtoc_test_simple.dts | 4 +++ tools/dtoc/test_fdt.py | 42 ++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts index d8ab8613ee3..b5c1274bb7c 100644 --- a/tools/dtoc/test/dtoc_test_simple.dts +++ b/tools/dtoc/test/dtoc_test_simple.dts @@ -56,4 +56,8 @@ low-power; }; }; + + orig-node { + orig = <1 23 4>; + }; }; diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 72095b05434..1e66e1bc353 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -154,6 +154,7 @@ class TestNode(unittest.TestCase): def setUp(self): self.dtb = fdt.FdtScan(find_dtb_file('dtoc_test_simple.dts')) self.node = self.dtb.GetNode('/spl-test') + self.fdt = self.dtb.GetFdtObj()

def testOffset(self): """Tests that we can obtain the offset of a node""" @@ -197,7 +198,7 @@ class TestNode(unittest.TestCase): def testRefreshExtraNode(self): """Test refreshing offsets when an expected node is missing""" # Delete it from the device tre, not our tables - self.dtb.GetFdtObj().del_node(self.node.Offset()) + self.fdt.del_node(self.node.Offset()) with self.assertRaises(ValueError) as e: self.dtb.Refresh() self.assertIn('Internal error, node name mismatch ' @@ -220,6 +221,34 @@ class TestNode(unittest.TestCase): target = dtb.GetNode('/phandle-target') self.assertEqual(target, dtb.LookupPhandle(fdt32_to_cpu(prop.value)))

+ def testAddNodeSpace(self): + """Test adding a single node when out of space""" + self.fdt.pack() + self.node.AddSubnode('subnode') + with self.assertRaises(libfdt.FdtException) as e: + self.dtb.Sync(auto_resize=False) + self.assertIn('FDT_ERR_NOSPACE', str(e.exception)) + + self.dtb.Sync(auto_resize=True) + offset = self.fdt.path_offset('/spl-test/subnode') + self.assertTrue(offset > 0) + + def testAddNodes(self): + """Test adding various subnode and properies""" + node = self.dtb.GetNode('/i2c@0') + + # Add a property to the node after i2c@0 to check that this is not + # disturbed by adding a subnode to i2c@0 + orig_node = self.dtb.GetNode('/orig-node') + orig_node.AddInt('integer-4', 456) + + # Add a property to the pmic node to check that pmic properties are not + # disturbed + pmic = self.dtb.GetNode('/i2c@0/pmic@9') + pmic.AddInt('integer-5', 567) + + self.dtb.Sync(auto_resize=True) +

class TestProp(unittest.TestCase): """Test operation of the Prop class""" @@ -385,17 +414,6 @@ class TestProp(unittest.TestCase): self.assertIn('FDT_ERR_NOSPACE', str(e.exception)) self.dtb.Sync(auto_resize=True)

- def testAddNode(self): - self.fdt.pack() - self.node.AddSubnode('subnode') - with self.assertRaises(libfdt.FdtException) as e: - self.dtb.Sync(auto_resize=False) - self.assertIn('FDT_ERR_NOSPACE', str(e.exception)) - - self.dtb.Sync(auto_resize=True) - offset = self.fdt.path_offset('/spl-test/subnode') - self.assertTrue(offset > 0) - def testAddMore(self): """Test various other methods for adding and setting properties""" self.node.AddZeroProp('one')

So far we have only needed to add subnodes to empty notds, so have not had to deal with ordering. However this feature is needed for binman's expanded nodes, since there may be another node in the same section.

While libfdt adds new properties after existing properties, it adds new subnodes before existing subnodes. This means that we must reorder the nodes in the cached version, so that the ordering remains consistent.

Update the sync implementation to sync existing subnodes first, then add new ones, then tidy up the ordering in the cached version. Update the test to cover this behaviour.

Also improve the comment about property syncing while we are here.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 44 +++++++++++++++++++++++++++++++++--------- tools/dtoc/test_fdt.py | 16 +++++++++++++++ 2 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index a5e1d0b52f6..63d1f68d816 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -503,9 +503,13 @@ class Node: auto_resize: Resize the device tree automatically if it does not have enough space for the update

+ Returns: + True if the node had to be added, False if it already existed + Raises: FdtException if auto_resize is False and there is not enough space """ + added = False if self._offset is None: # The subnode doesn't exist yet, so add it fdt_obj = self._fdt._fdt_obj @@ -519,23 +523,45 @@ class Node: else: offset = fdt_obj.add_subnode(self.parent._offset, self.name) self._offset = offset + added = True

- # Sync subnodes in reverse so that we don't disturb node offsets for - # nodes that are earlier in the DT. This avoids an O(n^2) rescan of - # node offsets. + # Sync the existing subnodes first, so that we can rely on the offsets + # being correct. As soon as we add new subnodes, it pushes all the + # existing subnodes up. for node in reversed(self.subnodes): - node.Sync(auto_resize) + if node._offset is not None: + node.Sync(auto_resize)

- # Sync properties now, whose offsets should not have been disturbed. - # We do this after subnodes, since this disturbs the offsets of these - # properties. Note that new properties will have an offset of None here, - # which Python 3 cannot sort against int. So use a large value instead - # to ensure that the new properties are added first. + # Sync subnodes in reverse so that we get the expected order. Each + # new node goes at the start of the subnode list. This avoids an O(n^2) + # rescan of node offsets. + num_added = 0 + for node in reversed(self.subnodes): + if node.Sync(auto_resize): + num_added += 1 + if num_added: + # Reorder our list of nodes to put the new ones first, since that's + # what libfdt does + old_count = len(self.subnodes) - num_added + subnodes = self.subnodes[old_count:] + self.subnodes[:old_count] + self.subnodes = subnodes + + # Sync properties now, whose offsets should not have been disturbed, + # since properties come before subnodes. This is done after all the + # subnode processing above, since updating properties can disturb the + # offsets of those subnodes. + # Properties are synced in reverse order, with new properties added + # before existing properties are synced. This ensures that the offsets + # of earlier properties are not disturbed. + # Note that new properties will have an offset of None here, which + # Python cannot sort against int. So use a large value instead so that + # new properties are added first. prop_list = sorted(self.props.values(), key=lambda prop: prop._offset or 1 << 31, reverse=True) for prop in prop_list: prop.Sync(auto_resize) + return added

class Fdt: diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 1e66e1bc353..49a2853f07f 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -237,6 +237,22 @@ class TestNode(unittest.TestCase): """Test adding various subnode and properies""" node = self.dtb.GetNode('/i2c@0')

+ # Add one more node next to the pmic one + sn1 = node.AddSubnode('node-one') + sn1.AddInt('integer-a', 12) + sn1.AddInt('integer-b', 23) + + # Sync so that everything is clean + self.dtb.Sync(auto_resize=True) + + # Add two subnodes next to pmic and node-one + sn2 = node.AddSubnode('node-two') + sn2.AddInt('integer-2a', 34) + sn2.AddInt('integer-2b', 45) + + sn3 = node.AddSubnode('node-three') + sn3.AddInt('integer-3', 123) + # Add a property to the node after i2c@0 to check that this is not # disturbed by adding a subnode to i2c@0 orig_node = self.dtb.GetNode('/orig-node')

Add a few more internal checks to make sure offsets are correct, before updating the dtb.

To make this easier, update the functions which add a property to return that property,.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 27 ++++++++++++++++++++++++--- tools/dtoc/test_fdt.py | 16 ++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 63d1f68d816..3996971e39c 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -233,6 +233,11 @@ class Prop: if self.dirty: node = self._node fdt_obj = node._fdt._fdt_obj + node_name = fdt_obj.get_name(node._offset) + if node_name and node_name != node.name: + raise ValueError("Internal error, node '%s' name mismatch '%s'" % + (node.path, node_name)) + if auto_resize: while fdt_obj.setprop(node.Offset(), self.name, self.bytes, (libfdt.NOSPACE,)) == -libfdt.NOSPACE: @@ -328,6 +333,11 @@ class Node: fdt_obj = self._fdt._fdt_obj if self._offset != my_offset: self._offset = my_offset + name = fdt_obj.get_name(self._offset) + if name and self.name != name: + raise ValueError("Internal error, node '%s' name mismatch '%s'" % + (self.path, name)) + offset = fdt_obj.first_subnode(self._offset, QUIET_NOTFOUND) for subnode in self.subnodes: if subnode.name != fdt_obj.get_name(offset): @@ -451,8 +461,13 @@ class Node: Args: prop_name: Name of property to add val: Bytes value of property + + Returns: + Prop added """ - self.props[prop_name] = Prop(self, None, prop_name, val) + prop = Prop(self, None, prop_name, val) + self.props[prop_name] = prop + return prop

def AddString(self, prop_name, val): """Add a new string property to a node @@ -463,9 +478,12 @@ class Node: Args: prop_name: Name of property to add val: String value of property + + Returns: + Prop added """ val = bytes(val, 'utf-8') - self.AddData(prop_name, val + b'\0') + return self.AddData(prop_name, val + b'\0')

def AddInt(self, prop_name, val): """Add a new integer property to a node @@ -476,8 +494,11 @@ class Node: Args: prop_name: Name of property to add val: Integer value of property + + Returns: + Prop added """ - self.AddData(prop_name, struct.pack('>I', val)) + return self.AddData(prop_name, struct.pack('>I', val))

def AddSubnode(self, name): """Add a new subnode to the node diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 49a2853f07f..856392b1bd9 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -265,6 +265,22 @@ class TestNode(unittest.TestCase):

self.dtb.Sync(auto_resize=True)

+ def testRefreshNameMismatch(self): + """Test name mismatch when syncing nodes and properties""" + prop = self.node.AddInt('integer-a', 12) + + wrong_offset = self.dtb.GetNode('/i2c@0')._offset + self.node._offset = wrong_offset + with self.assertRaises(ValueError) as e: + self.dtb.Sync() + self.assertIn("Internal error, node '/spl-test' name mismatch 'i2c@0'", + str(e.exception)) + + with self.assertRaises(ValueError) as e: + self.node.Refresh(wrong_offset) + self.assertIn("Internal error, node '/spl-test' name mismatch 'i2c@0'", + str(e.exception)) +

class TestProp(unittest.TestCase): """Test operation of the Prop class"""