[PATCH 00/11] binman: Enhancements to expanded entries

[PATCH 00/11] binman: Enhancements to expanded entries

[Simon Glass]

At present it is not always possible to have an expanded entry as a
sibling of another node. This limitation applies in most situations where
the other node has properties which binman must update, or when other
nodes appear after the expanded entry. An example is a 'hash' node in
the same section as an expanding node, since binman must update the hash
node with the hash of the expanding node, but it cannot do this while
also inserting the expanding node.

These limitations are mostly to do with how the fdt library worked (the one
provided by dtoc). This library still uses pylibfdt directly. At some
point it would be nice to have a hierarchical (live-tree) implementation
in pylibfdt (and therefore libfdt), but this does not exist at present. It
would simplify the code in the fdt library.

This series takes the support for expanded entries a little further, allow
them to be used in nearly any situation.

Where expanded entries are not wanted in a paricular case, a new
'no-expanded' property can be used.

Another problem found in the real world is that some systems require a
particular alignment for entries. For example, Intel chips struggle to
read data from SPI flash unless it is word-aligned. This series provides
a way to specify the default alignment for all the entries in a section,
to avoid the tedium of putting this in every single entry.

Finally, the vblock implementation includes code to collect the contents of
other entries, as specified by a phandle. It happens that this doesn't work
if one of the entries is a section, e.g. an expanding entry, since (for
efficiency reasons) binman currently only calculates section contents when
performing final image assembly. This series removes this limitation. It
also creates a new 'collection' entry type, to serve as a base class for
vblock, since it is likely that this functionality will be useful for other
entry types.


Simon Glass (11):
  binman: Use a unique number for the symbols test file
  binman: Allow disabling expanding an entry
  binman: Add support for a collection of entries
  binman: Support obtaining section contents immediately
  binman: Support default alignment for sections
  dtoc: Improve internal error for Refresh()
  dtoc: Tidy up property-offset handling
  dtoc: Tweak ordering of fdt-offsets refreshing
  dtoc: Add a subnode test for multiple nodes
  dtoc: Support adding subnodes alongside existing ones
  dtoc: Add new check that offsets are correct

 tools/binman/binman.rst                       | 14 +++
 tools/binman/entries.rst                      | 21 ++++-
 tools/binman/entry.py                         | 16 +++-
 tools/binman/etype/cbfs.py                    |  1 +
 tools/binman/etype/collection.py              | 67 ++++++++++++++
 tools/binman/etype/mkimage.py                 |  1 +
 tools/binman/etype/section.py                 | 36 ++++++--
 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/etype/vblock.py                  | 36 ++++----
 tools/binman/ftest.py                         | 57 +++++++++++-
 ...ymbols_nodtb.dts => 196_symbols_nodtb.dts} |  0
 tools/binman/test/197_symbols_expand.dts      | 23 +++++
 tools/binman/test/198_collection.dts          | 27 ++++++
 tools/binman/test/199_collection_section.dts  | 32 +++++++
 tools/binman/test/200_align_default.dts       | 30 ++++++
 tools/dtoc/fdt.py                             | 92 +++++++++++++++----
 tools/dtoc/test/dtoc_test_simple.dts          |  4 +
 tools/dtoc/test_fdt.py                        | 76 ++++++++++++---
 20 files changed, 470 insertions(+), 69 deletions(-)
 create mode 100644 tools/binman/etype/collection.py
 rename tools/binman/test/{192_symbols_nodtb.dts => 196_symbols_nodtb.dts} (100%)
 create mode 100644 tools/binman/test/197_symbols_expand.dts
 create mode 100644 tools/binman/test/198_collection.dts
 create mode 100644 tools/binman/test/199_collection_section.dts
 create mode 100644 tools/binman/test/200_align_default.dts

-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 01/11] binman: Use a unique number for the symbols test file

[Simon Glass]

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
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 02/11] binman: Allow disabling expanding an entry

[Simon Glass]

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";
+		};
+	};
+};
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 03/11] binman: Add support for a collection of entries

[Simon Glass]

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 {
+		};
+	};
+};
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 04/11] binman: Support obtaining section contents immediately

[Simon Glass]

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;
+		};
+	};
+};
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 05/11] binman: Support default alignment for sections

[Simon Glass]

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 {
+		};
+	};
+};
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 06/11] dtoc: Improve internal error for Refresh()

[Simon Glass]

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):
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 07/11] dtoc: Tidy up property-offset handling

[Simon Glass]

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
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 08/11] dtoc: Tweak ordering of fdt-offsets refreshing

[Simon Glass]

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
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 09/11] dtoc: Add a subnode test for multiple nodes

[Simon Glass]

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 at 0')
+
+        # Add a property to the node after i2c at 0 to check that this is not
+        # disturbed by adding a subnode to i2c at 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 at 0/pmic at 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')
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 10/11] dtoc: Support adding subnodes alongside existing ones

[Simon Glass]

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 at 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 at 0 to check that this is not
         # disturbed by adding a subnode to i2c@0
         orig_node = self.dtb.GetNode('/orig-node')
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 11/11] dtoc: Add new check that offsets are correct

[Simon Glass]

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 at 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 at 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 at 0'",
+                      str(e.exception))
+
 
 class TestProp(unittest.TestCase):
     """Test operation of the Prop class"""
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 11/11] dtoc: Add new check that offsets are correct

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 10/11] dtoc: Support adding subnodes alongside existing ones

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 09/11] dtoc: Add a subnode test for multiple nodes

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 08/11] dtoc: Tweak ordering of fdt-offsets refreshing

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 07/11] dtoc: Tidy up property-offset handling

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 06/11] dtoc: Improve internal error for Refresh()

[Simon Glass]

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(-)

Applied to u-boot-dm/next, thanks!

[PATCH 05/11] binman: Support default alignment for sections

[Simon Glass]

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

Applied to u-boot-dm/next, thanks!

[PATCH 04/11] binman: Support obtaining section contents immediately

[Simon Glass]

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

Applied to u-boot-dm/next, thanks!

[PATCH 03/11] binman: Add support for a collection of entries

[Simon Glass]

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

Applied to u-boot-dm/next, thanks!

[PATCH 02/11] binman: Allow disabling expanding an entry

[Simon Glass]

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

Applied to u-boot-dm/next, thanks!

[PATCH 01/11] binman: Use a unique number for the symbols test file

[Simon Glass]

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%)

Applied to u-boot-dm/next, thanks!