[PATCH 00/17] binman: Various tidy-ups and refactors

[PATCH 00/17] binman: Various tidy-ups and refactors

[Simon Glass]

This series improves the implementation of the entry_Section class so that
it is easier to subclass it. It also updates the documentation in this
area and adds support for a few more types in the Fdt class.

Using this, the CBFS implementation is tidied up a bit, but is not yet
updated to use entry_Section as a base class. That needs a bit more work.

Still, this provides a better basis for new file formats.


Simon Glass (17):
  dtoc: Bring in the libfdt module automatically
  dtoc: Add support for reading 64-bit ints
  dtoc: Add support for reading fixed-length bytes properties
  binman: Tidy up style in cmdline
  binman: Add a way to obtain the version
  binman: Correct init of entry in Entry class
  binman: Correct comments for ReadChildData()
  binman: Drop the underscore in _ReadEntries()
  binman: Drop the filename property in entry_Section
  binman: Allow overriding BuildSectionData()
  binman: Allow control of which entries to read
  binman: Update the section documentation
  binman: Move cbfs.ObtainContents() down a bit
  binman: Use normal entries in cbfs
  binman: cbfs: Refactor the init process
  binman: cfbs: Refactor ObtainContents() for consistency
  binman: Rename testCbfsNoCOntents()

 scripts/pylint.base                  |   6 +-
 tools/binman/cmdline.py              |  74 ++++++++---
 tools/binman/entries.rst             | 149 +++++++++++++++++----
 tools/binman/entry.py                |   7 +-
 tools/binman/etype/blob_phase.py     |   2 +-
 tools/binman/etype/cbfs.py           |  90 +++++++------
 tools/binman/etype/files.py          |   2 +-
 tools/binman/etype/section.py        | 190 ++++++++++++++++++++-------
 tools/binman/ftest.py                |  24 +++-
 tools/binman/state.py                |  18 +++
 tools/dtoc/fdt_util.py               |  55 ++++++++
 tools/dtoc/test/dtoc_test_simple.dts |   1 +
 tools/dtoc/test_dtoc.py              |   2 +
 tools/dtoc/test_fdt.py               |  44 ++++++-
 14 files changed, 520 insertions(+), 144 deletions(-)

-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 01/17] dtoc: Bring in the libfdt module automatically

[Simon Glass]

Use the same technique as with binman to load this module from the U-Boot
tree if available. This allows running tests without having to specify
the PYTHONPATH variable.

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

 tools/dtoc/test_fdt.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index d104f3c7745..d86fc86187e 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -16,6 +16,12 @@ import unittest
 our_path = os.path.dirname(os.path.realpath(__file__))
 sys.path.insert(1, os.path.join(our_path, '..'))
 
+# Bring in the libfdt module
+sys.path.insert(2, 'scripts/dtc/pylibfdt')
+sys.path.insert(2, os.path.join(our_path, '../../scripts/dtc/pylibfdt'))
+sys.path.insert(2, os.path.join(our_path,
+                '../../build-sandbox_spl/scripts/dtc/pylibfdt'))
+
 from dtoc import fdt
 from dtoc import fdt_util
 from dtoc.fdt_util import fdt32_to_cpu
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 02/17] dtoc: Add support for reading 64-bit ints

[Simon Glass]

Add functions to read a 64-bit integer property from the devicetree.

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

 tools/dtoc/fdt_util.py               | 35 ++++++++++++++++++++++++++++
 tools/dtoc/test/dtoc_test_simple.dts |  1 +
 tools/dtoc/test_dtoc.py              |  2 ++
 tools/dtoc/test_fdt.py               | 21 ++++++++++++++---
 4 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
index 37e96b98642..51d0eb52423 100644
--- a/tools/dtoc/fdt_util.py
+++ b/tools/dtoc/fdt_util.py
@@ -27,6 +27,18 @@ def fdt32_to_cpu(val):
     """
     return struct.unpack('>I', val)[0]
 
+def fdt64_to_cpu(val):
+    """Convert a device tree cell to an integer
+
+    Args:
+        val (list): Value to convert (list of 2 4-character strings representing
+            the cell value)
+
+    Return:
+        int: A native-endian integer value
+    """
+    return fdt32_to_cpu(val[0]) << 32 | fdt32_to_cpu(val[1])
+
 def fdt_cells_to_cpu(val, cells):
     """Convert one or two cells to a long integer
 
@@ -108,6 +120,29 @@ def GetInt(node, propname, default=None):
     value = fdt32_to_cpu(prop.value)
     return value
 
+def GetInt64(node, propname, default=None):
+    """Get a 64-bit integer from a property
+
+    Args:
+        node (Node): Node object to read from
+        propname (str): property name to read
+        default (int): Default value to use if the node/property do not exist
+
+    Returns:
+        int: value read, or default if none
+
+    Raises:
+        ValueError: Property is not of the correct size
+    """
+    prop = node.props.get(propname)
+    if not prop:
+        return default
+    if not isinstance(prop.value, list) or len(prop.value) != 2:
+        raise ValueError("Node '%s' property '%s' should be a list with 2 items for 64-bit values" %
+                         (node.name, propname))
+    value = fdt64_to_cpu(prop.value)
+    return value
+
 def GetString(node, propname, default=None):
     """Get a string from a property
 
diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts
index 5a6fa88d5cc..4c2c70af222 100644
--- a/tools/dtoc/test/dtoc_test_simple.dts
+++ b/tools/dtoc/test/dtoc_test_simple.dts
@@ -16,6 +16,7 @@
 		boolval;
 		maybe-empty-int = <>;
 		intval = <1>;
+		int64val = /bits/ 64 <0x123456789abcdef0>;
 		intarray = <2 3 4>;
 		byteval = [05];
 		bytearray = [06];
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 752061f27a4..ee17b8daf9a 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -296,6 +296,7 @@ struct dtd_sandbox_spl_test {
 \tbool\t\tboolval;
 \tunsigned char\tbytearray[3];
 \tunsigned char\tbyteval;
+\tfdt32_t\t\tint64val[2];
 \tfdt32_t\t\tintarray[3];
 \tfdt32_t\t\tintval;
 \tunsigned char\tlongbytearray[9];
@@ -355,6 +356,7 @@ static struct dtd_sandbox_spl_test dtv_spl_test = {
 \t.boolval\t\t= true,
 \t.bytearray\t\t= {0x6, 0x0, 0x0},
 \t.byteval\t\t= 0x5,
+\t.int64val\t\t= {0x12345678, 0x9abcdef0},
 \t.intarray\t\t= {0x2, 0x3, 0x4},
 \t.intval\t\t\t= 0x1,
 \t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10,
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index d86fc86187e..21a9a7ca063 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -24,7 +24,7 @@ sys.path.insert(2, os.path.join(our_path,
 
 from dtoc import fdt
 from dtoc import fdt_util
-from dtoc.fdt_util import fdt32_to_cpu
+from dtoc.fdt_util import fdt32_to_cpu, fdt64_to_cpu
 from fdt import Type, BytesToValue
 import libfdt
 from patman import command
@@ -128,7 +128,7 @@ class TestFdt(unittest.TestCase):
         node = self.dtb.GetNode('/spl-test')
         props = self.dtb.GetProps(node)
         self.assertEqual(['boolval', 'bytearray', 'byteval', 'compatible',
-                          'intarray', 'intval', 'longbytearray',
+                          'int64val', 'intarray', 'intval', 'longbytearray',
                           'maybe-empty-int', 'notstring', 'stringarray',
                           'stringval', 'u-boot,dm-pre-reloc'],
                          sorted(props.keys()))
@@ -335,6 +335,10 @@ class TestProp(unittest.TestCase):
         self.assertEqual(Type.INT, prop.type)
         self.assertEqual(1, fdt32_to_cpu(prop.value))
 
+        prop = self._ConvertProp('int64val')
+        self.assertEqual(Type.INT, prop.type)
+        self.assertEqual(0x123456789abcdef0, fdt64_to_cpu(prop.value))
+
         prop = self._ConvertProp('intarray')
         self.assertEqual(Type.INT, prop.type)
         val = [fdt32_to_cpu(val) for val in prop.value]
@@ -586,10 +590,21 @@ class TestFdtUtil(unittest.TestCase):
         self.assertEqual(3, fdt_util.GetInt(self.node, 'missing', 3))
 
         with self.assertRaises(ValueError) as e:
-            self.assertEqual(3, fdt_util.GetInt(self.node, 'intarray'))
+            fdt_util.GetInt(self.node, 'intarray')
         self.assertIn("property 'intarray' has list value: expecting a single "
                       'integer', str(e.exception))
 
+    def testGetInt64(self):
+        self.assertEqual(0x123456789abcdef0,
+                         fdt_util.GetInt64(self.node, 'int64val'))
+        self.assertEqual(3, fdt_util.GetInt64(self.node, 'missing', 3))
+
+        with self.assertRaises(ValueError) as e:
+            fdt_util.GetInt64(self.node, 'intarray')
+        self.assertIn(
+            "property 'intarray' should be a list with 2 items for 64-bit values",
+            str(e.exception))
+
     def testGetString(self):
         self.assertEqual('message', fdt_util.GetString(self.node, 'stringval'))
         self.assertEqual('test', fdt_util.GetString(self.node, 'missing',
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 03/17] dtoc: Add support for reading fixed-length bytes properties

[Simon Glass]

Add functions to read a sequence of bytes from the devicetree.

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

 scripts/pylint.base    |  4 ++--
 tools/dtoc/fdt_util.py | 20 ++++++++++++++++++++
 tools/dtoc/test_fdt.py | 17 +++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/scripts/pylint.base b/scripts/pylint.base
index d848ebe9058..f4f226799c1 100644
--- a/scripts/pylint.base
+++ b/scripts/pylint.base
@@ -44,12 +44,12 @@ cros_ec_rw -6.00
 defs 6.67
 dtoc.dtb_platdata 7.82
 dtoc.fdt 3.47
-dtoc.fdt_util 4.53
+dtoc.fdt_util 4.95
 dtoc.main 7.33
 dtoc.setup 5.00
 dtoc.src_scan 8.75
 dtoc.test_dtoc 8.54
-dtoc.test_fdt 6.92
+dtoc.test_fdt 6.93
 dtoc.test_src_scan 9.43
 efivar 6.71
 endian-swap 8.93
diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
index 51d0eb52423..51bdbdcd3b2 100644
--- a/tools/dtoc/fdt_util.py
+++ b/tools/dtoc/fdt_util.py
@@ -202,6 +202,26 @@ def GetByte(node, propname, default=None):
                          (node.name, propname, len(value), 1))
     return ord(value[0])
 
+def GetBytes(node, propname, size, default=None):
+    """Get a set of bytes from a property
+
+    Args:
+        node (Node): Node object to read from
+        propname (str): property name to read
+        size (int): Number of bytes to expect
+        default (bytes): Default value or None
+
+    Returns:
+        bytes: Bytes value read, or default if none
+    """
+    prop = node.props.get(propname)
+    if not prop:
+        return default
+    if len(prop.bytes) != size:
+        raise ValueError("Node '%s' property '%s' has length %d, expecting %d" %
+                         (node.name, propname, len(prop.bytes), size))
+    return prop.bytes
+
 def GetPhandleList(node, propname):
     """Get a list of phandles from a property
 
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index 21a9a7ca063..7a4c7efaa4a 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -635,6 +635,23 @@ class TestFdtUtil(unittest.TestCase):
         self.assertIn("property 'intval' has length 4, expecting 1",
                       str(e.exception))
 
+    def testGetBytes(self):
+        self.assertEqual(bytes([5]), fdt_util.GetBytes(self.node, 'byteval', 1))
+        self.assertEqual(None, fdt_util.GetBytes(self.node, 'missing', 3))
+        self.assertEqual(
+            bytes([3]), fdt_util.GetBytes(self.node, 'missing', 3,  bytes([3])))
+
+        with self.assertRaises(ValueError) as e:
+            fdt_util.GetBytes(self.node, 'longbytearray', 7)
+        self.assertIn(
+            "Node 'spl-test' property 'longbytearray' has length 9, expecting 7",
+             str(e.exception))
+
+        self.assertEqual(
+            bytes([0, 0, 0, 1]), fdt_util.GetBytes(self.node, 'intval', 4))
+        self.assertEqual(
+            bytes([3]), fdt_util.GetBytes(self.node, 'missing', 3,  bytes([3])))
+
     def testGetPhandleList(self):
         dtb = fdt.FdtScan(find_dtb_file('dtoc_test_phandle.dts'))
         node = dtb.GetNode('/phandle-source2')
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 04/17] binman: Tidy up style in cmdline

[Simon Glass]

Update this file to improve the pylint score a little. The remaining item
is:

   Function name "ParseArgs" doesn't conform to snake_case naming style

which needs some binman-wide renaming.

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

 scripts/pylint.base     |  2 +-
 tools/binman/cmdline.py | 45 ++++++++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/scripts/pylint.base b/scripts/pylint.base
index f4f226799c1..4e82dd4a616 100644
--- a/scripts/pylint.base
+++ b/scripts/pylint.base
@@ -2,7 +2,7 @@ _testing 0.83
 atf_bl31 -6.00
 binman.cbfs_util 7.70
 binman.cbfs_util_test 9.19
-binman.cmdline 8.87
+binman.cmdline 9.06
 binman.control 4.39
 binman.elf 6.42
 binman.elf_test 5.41
diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
index e73ff780956..14ec95c25be 100644
--- a/tools/binman/cmdline.py
+++ b/tools/binman/cmdline.py
@@ -2,18 +2,38 @@
 # Copyright (c) 2016 Google, Inc
 # Written by Simon Glass <sjg@chromium.org>
 #
-# Command-line parser for binman
-#
+
+"""Command-line parser for binman"""
 
 from argparse import ArgumentParser
 
+def make_extract_parser(subparsers):
+    """make_extract_parser: Make a subparser for the 'extract' command
+
+    Args:
+        subparsers (ArgumentParser): parser to add this one to
+    """
+    extract_parser = subparsers.add_parser('extract',
+                                           help='Extract files from an image')
+    extract_parser.add_argument('-i', '--image', type=str, required=True,
+                                help='Image filename to extract')
+    extract_parser.add_argument('-f', '--filename', type=str,
+                                help='Output filename to write to')
+    extract_parser.add_argument('-O', '--outdir', type=str, default='',
+        help='Path to directory to use for output files')
+    extract_parser.add_argument('paths', type=str, nargs='*',
+                                help='Paths within file to extract (wildcard)')
+    extract_parser.add_argument('-U', '--uncompressed', action='store_true',
+        help='Output raw uncompressed data for compressed entries')
+
 def ParseArgs(argv):
     """Parse the binman command-line arguments
 
     Args:
-        argv: List of string arguments
+        argv (list of str): List of string arguments
+
     Returns:
-        Tuple (options, args) with the command-line options and arugments.
+        tuple: (options, args) with the command-line options and arugments.
             options provides access to the options (e.g. option.debug)
             args is a list of string arguments
     """
@@ -74,8 +94,8 @@ controlled by a description in the board device tree.'''
     build_parser.add_argument('--update-fdt-in-elf', type=str,
         help='Update an ELF file with the output dtb: infile,outfile,begin_sym,end_sym')
 
-    entry_parser = subparsers.add_parser('entry-docs',
-        help='Write out entry documentation (see entries.rst)')
+    subparsers.add_parser(
+        'entry-docs', help='Write out entry documentation (see entries.rst)')
 
     list_parser = subparsers.add_parser('ls', help='List files in an image')
     list_parser.add_argument('-i', '--image', type=str, required=True,
@@ -83,18 +103,7 @@ controlled by a description in the board device tree.'''
     list_parser.add_argument('paths', type=str, nargs='*',
                              help='Paths within file to list (wildcard)')
 
-    extract_parser = subparsers.add_parser('extract',
-                                           help='Extract files from an image')
-    extract_parser.add_argument('-i', '--image', type=str, required=True,
-                                help='Image filename to extract')
-    extract_parser.add_argument('-f', '--filename', type=str,
-                                help='Output filename to write to')
-    extract_parser.add_argument('-O', '--outdir', type=str, default='',
-        help='Path to directory to use for output files')
-    extract_parser.add_argument('paths', type=str, nargs='*',
-                                help='Paths within file to extract (wildcard)')
-    extract_parser.add_argument('-U', '--uncompressed', action='store_true',
-        help='Output raw uncompressed data for compressed entries')
+    make_extract_parser(subparsers)
 
     replace_parser = subparsers.add_parser('replace',
                                            help='Replace entries in an image')
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 05/17] binman: Add a way to obtain the version

[Simon Glass]

Add a -V option which shows the version number of binman. For now this
just uses a local 'version' file. Once the tool is packaged in some way
we can figure out an approach that suits.

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

 tools/binman/cmdline.py | 29 +++++++++++++++++++++++++++++
 tools/binman/ftest.py   | 20 ++++++++++++++++++++
 tools/binman/state.py   | 18 ++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
index 14ec95c25be..2229316f10e 100644
--- a/tools/binman/cmdline.py
+++ b/tools/binman/cmdline.py
@@ -5,7 +5,9 @@
 
 """Command-line parser for binman"""
 
+import argparse
 from argparse import ArgumentParser
+import state
 
 def make_extract_parser(subparsers):
     """make_extract_parser: Make a subparser for the 'extract' command
@@ -26,6 +28,32 @@ def make_extract_parser(subparsers):
     extract_parser.add_argument('-U', '--uncompressed', action='store_true',
         help='Output raw uncompressed data for compressed entries')
 
+
+#pylint: disable=R0903
+class BinmanVersion(argparse.Action):
+    """Handles the -V option to binman
+
+    This reads the version information from a file called 'version' in the same
+    directory as this file.
+
+    If not present it assumes this is running from the U-Boot tree and collects
+    the version from the Makefile.
+
+    The format of the version information is three VAR = VALUE lines, for
+    example:
+
+        VERSION = 2022
+        PATCHLEVEL = 01
+        EXTRAVERSION = -rc2
+    """
+    def __init__(self, nargs=0, **kwargs):
+        super().__init__(nargs=nargs, **kwargs)
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        parser._print_message(f'Binman {state.GetVersion()}\n')
+        parser.exit()
+
+
 def ParseArgs(argv):
     """Parse the binman command-line arguments
 
@@ -59,6 +87,7 @@ controlled by a description in the board device tree.'''
     parser.add_argument('-v', '--verbosity', default=1,
         type=int, help='Control verbosity: 0=silent, 1=warnings, 2=notices, '
         '3=info, 4=detail, 5=debug')
+    parser.add_argument('-V', '--version', nargs=0, action=BinmanVersion)
 
     subparsers = parser.add_subparsers(dest='cmd')
     subparsers.required = True
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 6be003786e8..6a36e8f3158 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4661,6 +4661,26 @@ class TestFunctional(unittest.TestCase):
             str(e.exception),
             "Not enough space in '.*u_boot_binman_embed_sm' for data length.*")
 
+    def testVersion(self):
+        """Test we can get the binman version"""
+        version = '(unreleased)'
+        self.assertEqual(version, state.GetVersion(self._indir))
+
+        with self.assertRaises(SystemExit):
+            with test_util.capture_sys_output() as (_, stderr):
+                self._DoBinman('-V')
+        self.assertEqual('Binman %s\n' % version, stderr.getvalue())
+
+        # Try running the tool too, just to be safe
+        result = self._RunBinman('-V')
+        self.assertEqual('Binman %s\n' % version, result.stderr)
+
+        # Set up a version file to make sure that works
+        version = 'v2025.01-rc2'
+        tools.WriteFile(os.path.join(self._indir, 'version'), version,
+                        binary=False)
+        self.assertEqual(version, state.GetVersion(self._indir))
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/state.py b/tools/binman/state.py
index 9e5b8a39310..af0a65e8418 100644
--- a/tools/binman/state.py
+++ b/tools/binman/state.py
@@ -16,6 +16,8 @@ import os
 from patman import tools
 from patman import tout
 
+OUR_PATH = os.path.dirname(os.path.realpath(__file__))
+
 # Map an dtb etype to its expected filename
 DTB_TYPE_FNAME = {
     'u-boot-spl-dtb': 'spl/u-boot-spl.dtb',
@@ -515,3 +517,19 @@ def TimingShow():
 
     for name, seconds in duration.items():
         print('%10s: %10.1fms' % (name, seconds * 1000))
+
+def GetVersion(path=OUR_PATH):
+    """Get the version string for binman
+
+    Args:
+        path: Path to 'version' file
+
+    Returns:
+        str: String version, e.g. 'v2021.10'
+    """
+    version_fname = os.path.join(path, 'version')
+    if os.path.exists(version_fname):
+        version = tools.ReadFile(version_fname, binary=False)
+    else:
+        version = '(unreleased)'
+    return version
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 06/17] binman: Correct init of entry in Entry class

[Simon Glass]

This should not have an underscore. Drop it so that derived classes can
rely on it being set correctly.

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

 tools/binman/entry.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 70222718ea9..5e66aa4fa54 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -95,7 +95,7 @@ class Entry(object):
         self.pad_after = 0
         self.offset_unset = False
         self.image_pos = None
-        self._expand_size = False
+        self.expand_size = False
         self.compress = 'none'
         self.missing = False
         self.external = False
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 07/17] binman: Correct comments for ReadChildData()

[Simon Glass]

The comment here is incomplete. Fix it.

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

 tools/binman/entry.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 5e66aa4fa54..2205bc8d923 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -860,7 +860,8 @@ features to produce new behaviours.
         """Handle writing the data in a child entry
 
         This should be called on the child's parent section after the child's
-        data has been updated. It
+        data has been updated. It should update any data structures needed to
+        validate that the update is successful.
 
         This base-class implementation does nothing, since the base Entry object
         does not have any children.
@@ -870,7 +871,7 @@ features to produce new behaviours.
 
         Returns:
             True if the section could be updated successfully, False if the
-                data is such that the section could not updat
+                data is such that the section could not update
         """
         return True
 
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 08/17] binman: Drop the underscore in _ReadEntries()

[Simon Glass]

This function can be overridden so should not have an underscore. Drop it.

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

 tools/binman/etype/blob_phase.py | 2 +-
 tools/binman/etype/cbfs.py       | 4 ++--
 tools/binman/etype/files.py      | 2 +-
 tools/binman/etype/section.py    | 8 ++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/binman/etype/blob_phase.py b/tools/binman/etype/blob_phase.py
index 54ca54c50c1..ed25e467a13 100644
--- a/tools/binman/etype/blob_phase.py
+++ b/tools/binman/etype/blob_phase.py
@@ -48,4 +48,4 @@ class Entry_blob_phase(Entry_section):
             subnode = state.AddSubnode(self._node, name)
 
         # Read entries again, now that we have some
-        self._ReadEntries()
+        self.ReadEntries()
diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 44db7b9bb20..0a858b8b849 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -171,7 +171,7 @@ class Entry_cbfs(Entry):
         self._cbfs_arg = fdt_util.GetString(node, 'cbfs-arch', 'x86')
         self.align_default = None
         self._cbfs_entries = OrderedDict()
-        self._ReadSubnodes()
+        self.ReadEntries()
         self.reader = None
 
     def ObtainContents(self, skip=None):
@@ -204,7 +204,7 @@ class Entry_cbfs(Entry):
         self.SetContents(data)
         return True
 
-    def _ReadSubnodes(self):
+    def ReadEntries(self):
         """Read the subnodes to find out what should go in this CBFS"""
         for node in self._node.subnodes:
             entry = Entry.Create(self, node)
diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py
index 9b04a496a85..927d0f071df 100644
--- a/tools/binman/etype/files.py
+++ b/tools/binman/etype/files.py
@@ -64,4 +64,4 @@ class Entry_files(Entry_section):
                 state.AddInt(subnode, 'align', self._files_align)
 
         # Read entries again, now that we have some
-        self._ReadEntries()
+        self.ReadEntries()
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index e2949fc9163..281a228cd0e 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -85,9 +85,9 @@ class Entry_section(Entry):
         if filename:
             self._filename = filename
 
-        self._ReadEntries()
+        self.ReadEntries()
 
-    def _ReadEntries(self):
+    def ReadEntries(self):
         for node in self._node.subnodes:
             if node.name.startswith('hash') or node.name.startswith('signature'):
                 continue
@@ -741,5 +741,5 @@ class Entry_section(Entry):
             missing: List of missing properties / entry args, each a string
         """
         if not self._ignore_missing:
-            entry.Raise('Missing required properties/entry args: %s' %
-                       (', '.join(missing)))
+            missing = ', '.join(missing)
+            entry.Raise(f'Missing required properties/entry args: {missing}')
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 09/17] binman: Drop the filename property in entry_Section

[Simon Glass]

This is not used and does nothing. Drop it.

Add a tweak to avoid reducing the pylint score.

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

 tools/binman/etype/section.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 281a228cd0e..952c01de186 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -81,9 +81,6 @@ class Entry_section(Entry):
                 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
 
         self.ReadEntries()
 
@@ -661,7 +658,7 @@ class Entry_section(Entry):
         return data
 
     def ReadChildData(self, child, decomp=True):
-        tout.Debug("ReadChildData for child '%s'" % child.GetPath())
+        tout.Debug(f"ReadChildData for child '{child.GetPath()}'")
         parent_data = self.ReadData(True)
         offset = child.offset - self._skip_at_start
         tout.Debug("Extract for child '%s': offset %#x, skip_at_start %#x, result %#x" %
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 10/17] binman: Allow overriding BuildSectionData()

[Simon Glass]

This method is currently marked private. However it is useful to be able
to subclass it, since much of the entry_Section code can be reused. Rename
it.

Also document one confusing part of this code, so people can understand
how to add a test for this case.

Fix up a few pylint warnings to avoid regressing the score.

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

 tools/binman/etype/section.py | 16 ++++++++++++----
 tools/binman/ftest.py         |  2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 952c01de186..334240384ea 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -182,7 +182,7 @@ class Entry_section(Entry):
 
         return data
 
-    def _BuildSectionData(self, required):
+    def BuildSectionData(self, required):
         """Build the contents of a section
 
         This places all entries at the right place, dealing with padding before
@@ -190,6 +190,9 @@ class Entry_section(Entry):
         pad-before and pad-after properties in the section items) since that is
         handled by the parent section.
 
+        This should be overridden by subclasses which want to build their own
+        data structure for the section.
+
         Args:
             required: True if the data must be present, False if it is OK to
                 return None
@@ -201,6 +204,9 @@ class Entry_section(Entry):
 
         for entry in self._entries.values():
             entry_data = entry.GetData(required)
+
+            # This can happen when this section is referenced from a collection
+            # earlier in the image description. See testCollectionSection().
             if not required and entry_data is None:
                 return None
             data = self.GetPaddedDataForEntry(entry, entry_data)
@@ -250,7 +256,7 @@ class Entry_section(Entry):
             This excludes any padding. If the section is compressed, the
             compressed data is returned
         """
-        data = self._BuildSectionData(required)
+        data = self.BuildSectionData(required)
         if data is None:
             return None
         self.SetContents(data)
@@ -278,7 +284,7 @@ class Entry_section(Entry):
             self._SortEntries()
         self._ExpandEntries()
 
-        data = self._BuildSectionData(True)
+        data = self.BuildSectionData(True)
         self.SetContents(data)
 
         self.CheckSize()
@@ -735,7 +741,9 @@ class Entry_section(Entry):
         nothing.
 
         Args:
-            missing: List of missing properties / entry args, each a string
+            entry (Entry): Entry to raise the error on
+            missing (list of str): List of missing properties / entry args, each
+            a string
         """
         if not self._ignore_missing:
             missing = ', '.join(missing)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 6a36e8f3158..3982560c47c 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4533,7 +4533,7 @@ class TestFunctional(unittest.TestCase):
     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
+        # 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')
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 11/17] binman: Allow control of which entries to read

[Simon Glass]

The ObtainContents() and GetEntryContents() methods in this file read
every single entry in the section. This is the common case.

However when one of the entries has had its data updated (e.g. with
'binman replace') we don't want to read it again from the file. Allow
the entry to be skipped, for this purpose. This is currently done in the
CBFS implementation, so adding it here will allow that to use more of
the entry_Section code.

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

 tools/binman/etype/section.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 334240384ea..76e5eb19648 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -143,8 +143,8 @@ class Entry_section(Entry):
         for entry in self._entries.values():
             entry.AddMissingProperties(have_image_pos)
 
-    def ObtainContents(self):
-        return self.GetEntryContents()
+    def ObtainContents(self, skip_entry=None):
+        return self.GetEntryContents(skip_entry=skip_entry)
 
     def GetPaddedDataForEntry(self, entry, entry_data):
         """Get the data for an entry including any padding
@@ -527,12 +527,13 @@ class Entry_section(Entry):
                 return entry
         return None
 
-    def GetEntryContents(self):
+    def GetEntryContents(self, skip_entry=None):
         """Call ObtainContents() for each entry in the section
         """
         def _CheckDone(entry):
-            if not entry.ObtainContents():
-                next_todo.append(entry)
+            if entry != skip_entry:
+                if not entry.ObtainContents():
+                    next_todo.append(entry)
             return entry
 
         todo = self._entries.values()
@@ -620,7 +621,7 @@ class Entry_section(Entry):
 
     def ListEntries(self, entries, indent):
         """List the files in the section"""
-        Entry.AddEntryInfo(entries, indent, self.name, 'section', self.size,
+        Entry.AddEntryInfo(entries, indent, self.name, self.etype, self.size,
                            self.image_pos, None, self.offset, self)
         for entry in self._entries.values():
             entry.ListEntries(entries, indent + 1)
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 12/17] binman: Update the section documentation

[Simon Glass]

Expand this to explain subclassing better and also to tidy up formatting
for rST.

Fix a few pylint warnings to avoid dropping the score.

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

 tools/binman/entries.rst      | 149 +++++++++++++++++++++++++++-------
 tools/binman/etype/section.py | 148 +++++++++++++++++++++++++++------
 2 files changed, 242 insertions(+), 55 deletions(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index dcac700c461..748277c1cde 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -799,39 +799,130 @@ This entry holds firmware for an external platform-specific coprocessor.
 Entry: section: Entry that contains other entries
 -------------------------------------------------
 
-Properties / Entry arguments: (see binman README for more information):
-    pad-byte: Pad byte to use when padding
-    sort-by-offset: True if entries should be sorted by offset, False if
-    they must be in-order in the device tree description
-
-    end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32)
-
-    skip-at-start: Number of bytes before the first entry starts. These
-        effectively adjust the starting offset of entries. For example,
-        if this is 16, then the first entry would start at 16. An entry
-        with offset = 20 would in fact be written at offset 4 in the image
-        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
-        missing their contents. The second will produce an image but of
-        course it will not work.
-
-Properties:
-    _allow_missing: True if this section permits external blobs to be
-        missing their contents. The second will produce an image but of
-        course it will not work.
+A section is an entry which can contain other entries, thus allowing
+hierarchical images to be created. See 'Sections and hierarchical images'
+in the binman README for more information.
+
+The base implementation simply joins the various entries together, using
+various rules about alignment, etc.
+
+Subclassing
+~~~~~~~~~~~
+
+This class can be subclassed to support other file formats which hold
+multiple entries, such as CBFS. To do this, override the following
+functions. The documentation here describes what your function should do.
+For example code, see etypes which subclass `Entry_section`, or `cbfs.py`
+for a more involved example::
+
+   $ grep -l \(Entry_section tools/binman/etype/*.py
+
+ReadNode()
+    Call `super().ReadNode()`, then read any special properties for the
+    section. Then call `self.ReadEntries()` to read the entries.
+
+    Binman calls this at the start when reading the image description.
+
+ReadEntries()
+    Read in the subnodes of the section. This may involve creating entries
+    of a particular etype automatically, as well as reading any special
+    properties in the entries. For each entry, entry.ReadNode() should be
+    called, to read the basic entry properties. The properties should be
+    added to `self._entries[]`, in the correct order, with a suitable name.
+
+    Binman calls this at the start when reading the image description.
+
+BuildSectionData(required)
+    Create the custom file format that you want and return it as bytes.
+    This likely sets up a file header, then loops through the entries,
+    adding them to the file. For each entry, call `entry.GetData()` to
+    obtain the data. If that returns None, and `required` is False, then
+    this method must give up and return None. But if `required` is True then
+    it should assume that all data is valid.
+
+    Binman calls this when packing the image, to find out the size of
+    everything. It is called again at the end when building the final image.
+
+SetImagePos(image_pos):
+    Call `super().SetImagePos(image_pos)`, then set the `image_pos` values
+    for each of the entries. This should use the custom file format to find
+    the `start offset` (and `image_pos`) of each entry. If the file format
+    uses compression in such a way that there is no offset available (other
+    than reading the whole file and decompressing it), then the offsets for
+    affected entries can remain unset (`None`). The size should also be set
+    if possible.
+
+    Binman calls this after the image has been packed, to update the
+    location that all the entries ended up at.
+
+ReadChildData(child, decomp):
+    The default version of this may be good enough, if you are able to
+    implement SetImagePos() correctly. But that is a bit of a bypass, so
+    you can override this method to read from your custom file format. It
+    should read the entire entry containing the custom file using
+    `super().ReadData(True)`, then parse the file to get the data for the
+    given child, then return that data.
+
+    If your file format supports compression, the `decomp` argument tells
+    you whether to return the compressed data (`decomp` is False) or to
+    uncompress it first, then return the uncompressed data (`decomp` is
+    True). This is used by the `binman extract -U` option.
+
+    Binman calls this when reading in an image, in order to populate all the
+    entries with the data from that image (`binman ls`).
+
+WriteChildData(child):
+    Binman calls this after `child.data` is updated, to inform the custom
+    file format about this, in case it needs to do updates.
+
+    The default version of this does nothing and probably needs to be
+    overridden for the 'binman replace' command to work. Your version should
+    use `child.data` to update the data for that child in the custom file
+    format.
+
+    Binman calls this when updating an image that has been read in and in
+    particular to update the data for a particular entry (`binman replace`)
+
+Properties / Entry arguments
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+See :ref:`develop/package/binman:Image description format` for more
+information.
+
+align-default
+    Default alignment for this section, if no alignment is given in the
+    entry
+
+pad-byte
+    Pad byte to use when padding
+
+sort-by-offset
+    True if entries should be sorted by offset, False if they must be
+    in-order in the device tree description
+
+end-at-4gb
+    Used to build an x86 ROM which ends at 4GB (2^32)
+
+name-prefix
+    Adds a prefix to the name of every entry in the section when writing out
+    the map
+
+skip-at-start
+    Number of bytes before the first entry starts. These effectively adjust
+    the starting offset of entries. For example, if this is 16, then the
+    first entry would start at 16. An entry with offset = 20 would in fact
+    be written at offset 4 in the image file, since the first 16 bytes are
+    skipped when writing.
 
 Since a section is also an entry, it inherits all the properies of entries
 too.
 
-A section is an entry which can contain other entries, thus allowing
-hierarchical images to be created. See 'Sections and hierarchical images'
-in the binman README for more information.
+Note that the `allow_missing` member controls whether this section permits
+external blobs to be missing their contents. The option will produce an
+image but of course it will not work. It is useful to make sure that
+Continuous Integration systems can build without the binaries being
+available. This is set by the `SetAllowMissing()` method, if
+`--allow-missing` is passed to binman.
 
 
 
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 76e5eb19648..2e01dccc6db 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -24,34 +24,130 @@ from patman.tools import ToHexSize
 class Entry_section(Entry):
     """Entry that contains other entries
 
-    Properties / Entry arguments: (see binman README for more information):
-        pad-byte: Pad byte to use when padding
-        sort-by-offset: True if entries should be sorted by offset, False if
-        they must be in-order in the device tree description
-
-        end-at-4gb: Used to build an x86 ROM which ends at 4GB (2^32)
-
-        skip-at-start: Number of bytes before the first entry starts. These
-            effectively adjust the starting offset of entries. For example,
-            if this is 16, then the first entry would start at 16. An entry
-            with offset = 20 would in fact be written at offset 4 in the image
-            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
-            missing their contents. The second will produce an image but of
-            course it will not work.
+    A section is an entry which can contain other entries, thus allowing
+    hierarchical images to be created. See 'Sections and hierarchical images'
+    in the binman README for more information.
+
+    The base implementation simply joins the various entries together, using
+    various rules about alignment, etc.
+
+    Subclassing
+    ~~~~~~~~~~~
+
+    This class can be subclassed to support other file formats which hold
+    multiple entries, such as CBFS. To do this, override the following
+    functions. The documentation here describes what your function should do.
+    For example code, see etypes which subclass `Entry_section`, or `cbfs.py`
+    for a more involved example::
+
+       $ grep -l \(Entry_section tools/binman/etype/*.py
+
+    ReadNode()
+        Call `super().ReadNode()`, then read any special properties for the
+        section. Then call `self.ReadEntries()` to read the entries.
+
+        Binman calls this at the start when reading the image description.
+
+    ReadEntries()
+        Read in the subnodes of the section. This may involve creating entries
+        of a particular etype automatically, as well as reading any special
+        properties in the entries. For each entry, entry.ReadNode() should be
+        called, to read the basic entry properties. The properties should be
+        added to `self._entries[]`, in the correct order, with a suitable name.
+
+        Binman calls this at the start when reading the image description.
+
+    BuildSectionData(required)
+        Create the custom file format that you want and return it as bytes.
+        This likely sets up a file header, then loops through the entries,
+        adding them to the file. For each entry, call `entry.GetData()` to
+        obtain the data. If that returns None, and `required` is False, then
+        this method must give up and return None. But if `required` is True then
+        it should assume that all data is valid.
+
+        Binman calls this when packing the image, to find out the size of
+        everything. It is called again at the end when building the final image.
+
+    SetImagePos(image_pos):
+        Call `super().SetImagePos(image_pos)`, then set the `image_pos` values
+        for each of the entries. This should use the custom file format to find
+        the `start offset` (and `image_pos`) of each entry. If the file format
+        uses compression in such a way that there is no offset available (other
+        than reading the whole file and decompressing it), then the offsets for
+        affected entries can remain unset (`None`). The size should also be set
+        if possible.
+
+        Binman calls this after the image has been packed, to update the
+        location that all the entries ended up at.
+
+    ReadChildData(child, decomp):
+        The default version of this may be good enough, if you are able to
+        implement SetImagePos() correctly. But that is a bit of a bypass, so
+        you can override this method to read from your custom file format. It
+        should read the entire entry containing the custom file using
+        `super().ReadData(True)`, then parse the file to get the data for the
+        given child, then return that data.
+
+        If your file format supports compression, the `decomp` argument tells
+        you whether to return the compressed data (`decomp` is False) or to
+        uncompress it first, then return the uncompressed data (`decomp` is
+        True). This is used by the `binman extract -U` option.
+
+        Binman calls this when reading in an image, in order to populate all the
+        entries with the data from that image (`binman ls`).
+
+    WriteChildData(child):
+        Binman calls this after `child.data` is updated, to inform the custom
+        file format about this, in case it needs to do updates.
+
+        The default version of this does nothing and probably needs to be
+        overridden for the 'binman replace' command to work. Your version should
+        use `child.data` to update the data for that child in the custom file
+        format.
+
+        Binman calls this when updating an image that has been read in and in
+        particular to update the data for a particular entry (`binman replace`)
+
+    Properties / Entry arguments
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+    See :ref:`develop/package/binman:Image description format` for more
+    information.
+
+    align-default
+        Default alignment for this section, if no alignment is given in the
+        entry
+
+    pad-byte
+        Pad byte to use when padding
+
+    sort-by-offset
+        True if entries should be sorted by offset, False if they must be
+        in-order in the device tree description
+
+    end-at-4gb
+        Used to build an x86 ROM which ends at 4GB (2^32)
+
+    name-prefix
+        Adds a prefix to the name of every entry in the section when writing out
+        the map
+
+    skip-at-start
+        Number of bytes before the first entry starts. These effectively adjust
+        the starting offset of entries. For example, if this is 16, then the
+        first entry would start at 16. An entry with offset = 20 would in fact
+        be written at offset 4 in the image file, since the first 16 bytes are
+        skipped when writing.
 
     Since a section is also an entry, it inherits all the properies of entries
     too.
 
-    A section is an entry which can contain other entries, thus allowing
-    hierarchical images to be created. See 'Sections and hierarchical images'
-    in the binman README for more information.
+    Note that the `allow_missing` member controls whether this section permits
+    external blobs to be missing their contents. The option will produce an
+    image but of course it will not work. It is useful to make sure that
+    Continuous Integration systems can build without the binaries being
+    available. This is set by the `SetAllowMissing()` method, if
+    `--allow-missing` is passed to binman.
     """
     def __init__(self, section, etype, node, test=False):
         if not test:
@@ -98,9 +194,9 @@ class Entry_section(Entry):
         """Raises an error for this section
 
         Args:
-            msg: Error message to use in the raise string
+            msg (str): Error message to use in the raise string
         Raises:
-            ValueError()
+            ValueError: always
         """
         raise ValueError("Section '%s': %s" % (self._node.path, msg))
 
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 13/17] binman: Move cbfs.ObtainContents() down a bit

[Simon Glass]

It is easier to understand this file if reading the entries comes before
obtaining the contents, since that is the order in which Binman proceeds.
Move the function down a bit.

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

 tools/binman/etype/cbfs.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 0a858b8b849..9e04897d71e 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -174,6 +174,21 @@ class Entry_cbfs(Entry):
         self.ReadEntries()
         self.reader = None
 
+    def ReadEntries(self):
+        """Read the subnodes to find out what should go in this CBFS"""
+        for node in self._node.subnodes:
+            entry = Entry.Create(self, node)
+            entry.ReadNode()
+            entry._cbfs_name = fdt_util.GetString(node, 'cbfs-name', entry.name)
+            entry._type = fdt_util.GetString(node, 'cbfs-type')
+            compress = fdt_util.GetString(node, 'cbfs-compress', 'none')
+            entry._cbfs_offset = fdt_util.GetInt(node, 'cbfs-offset')
+            entry._cbfs_compress = cbfs_util.find_compress(compress)
+            if entry._cbfs_compress is None:
+                self.Raise("Invalid compression in '%s': '%s'" %
+                           (node.name, compress))
+            self._cbfs_entries[entry._cbfs_name] = entry
+
     def ObtainContents(self, skip=None):
         arch = cbfs_util.find_arch(self._cbfs_arg)
         if arch is None:
@@ -204,21 +219,6 @@ class Entry_cbfs(Entry):
         self.SetContents(data)
         return True
 
-    def ReadEntries(self):
-        """Read the subnodes to find out what should go in this CBFS"""
-        for node in self._node.subnodes:
-            entry = Entry.Create(self, node)
-            entry.ReadNode()
-            entry._cbfs_name = fdt_util.GetString(node, 'cbfs-name', entry.name)
-            entry._type = fdt_util.GetString(node, 'cbfs-type')
-            compress = fdt_util.GetString(node, 'cbfs-compress', 'none')
-            entry._cbfs_offset = fdt_util.GetInt(node, 'cbfs-offset')
-            entry._cbfs_compress = cbfs_util.find_compress(compress)
-            if entry._cbfs_compress is None:
-                self.Raise("Invalid compression in '%s': '%s'" %
-                           (node.name, compress))
-            self._cbfs_entries[entry._cbfs_name] = entry
-
     def SetImagePos(self, image_pos):
         """Override this function to set all the entry properties from CBFS
 
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 14/17] binman: Use normal entries in cbfs

[Simon Glass]

This currently uses _cbfs_entries[] to store entries. Since the entries
are in fact valid etypes, we may as well use the same name as
entry_Section uses, which is _entries. This allows reusing more of the
code there (in a future patch).

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

 tools/binman/etype/cbfs.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 9e04897d71e..873b199a91d 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -170,7 +170,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._entries = OrderedDict()
         self.ReadEntries()
         self.reader = None
 
@@ -187,7 +187,7 @@ class Entry_cbfs(Entry):
             if entry._cbfs_compress is None:
                 self.Raise("Invalid compression in '%s': '%s'" %
                            (node.name, compress))
-            self._cbfs_entries[entry._cbfs_name] = entry
+            self._entries[entry._cbfs_name] = entry
 
     def ObtainContents(self, skip=None):
         arch = cbfs_util.find_arch(self._cbfs_arg)
@@ -196,7 +196,7 @@ class Entry_cbfs(Entry):
         if self.size is None:
             self.Raise("'cbfs' entry must have a size property")
         cbfs = CbfsWriter(self.size, arch)
-        for entry in self._cbfs_entries.values():
+        for entry in self._entries.values():
             # First get the input data and put it in a file. If not available,
             # try later.
             if entry != skip and not entry.ObtainContents():
@@ -230,7 +230,7 @@ class Entry_cbfs(Entry):
         super().SetImagePos(image_pos)
 
         # Now update the entries with info from the CBFS entries
-        for entry in self._cbfs_entries.values():
+        for entry in self._entries.values():
             cfile = entry._cbfs_file
             entry.size = cfile.data_len
             entry.offset = cfile.calced_cbfs_offset
@@ -240,7 +240,7 @@ class Entry_cbfs(Entry):
 
     def AddMissingProperties(self, have_image_pos):
         super().AddMissingProperties(have_image_pos)
-        for entry in self._cbfs_entries.values():
+        for entry in self._entries.values():
             entry.AddMissingProperties(have_image_pos)
             if entry._cbfs_compress:
                 state.AddZeroProp(entry._node, 'uncomp-size')
@@ -252,7 +252,7 @@ class Entry_cbfs(Entry):
     def SetCalculatedProperties(self):
         """Set the value of device-tree properties calculated by binman"""
         super().SetCalculatedProperties()
-        for entry in self._cbfs_entries.values():
+        for entry in self._entries.values():
             state.SetInt(entry._node, 'offset', entry.offset)
             state.SetInt(entry._node, 'size', entry.size)
             state.SetInt(entry._node, 'image-pos', entry.image_pos)
@@ -262,11 +262,11 @@ class Entry_cbfs(Entry):
     def ListEntries(self, entries, indent):
         """Override this method to list all files in the section"""
         super().ListEntries(entries, indent)
-        for entry in self._cbfs_entries.values():
+        for entry in self._entries.values():
             entry.ListEntries(entries, indent + 1)
 
     def GetEntries(self):
-        return self._cbfs_entries
+        return self._entries
 
     def ReadData(self, decomp=True):
         data = super().ReadData(True)
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 15/17] binman: cbfs: Refactor the init process

[Simon Glass]

Update the constructor to work in the recommended way, where the node
properties are read in a separate function. This makes it more similar to
entry_Section.

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

 tools/binman/etype/cbfs.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 873b199a91d..a5120127059 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -168,12 +168,16 @@ class Entry_cbfs(Entry):
         from binman import state
 
         super().__init__(section, etype, node)
-        self._cbfs_arg = fdt_util.GetString(node, 'cbfs-arch', 'x86')
         self.align_default = None
         self._entries = OrderedDict()
-        self.ReadEntries()
         self.reader = None
 
+    def ReadNode(self):
+        """Read properties from the atf-fip node"""
+        super().ReadNode()
+        self._cbfs_arg = fdt_util.GetString(self._node, 'cbfs-arch', 'x86')
+        self.ReadEntries()
+
     def ReadEntries(self):
         """Read the subnodes to find out what should go in this CBFS"""
         for node in self._node.subnodes:
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 16/17] binman: cfbs: Refactor ObtainContents() for consistency

[Simon Glass]

Update this to use the same arguments as entry_Section uses.

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

 tools/binman/etype/cbfs.py | 40 ++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index a5120127059..2459388f842 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -193,7 +193,24 @@ class Entry_cbfs(Entry):
                            (node.name, compress))
             self._entries[entry._cbfs_name] = entry
 
-    def ObtainContents(self, skip=None):
+    def ObtainCfile(self, cbfs, entry):
+        # First get the input data and put it in a file. If not available,
+        # try later.
+        data = entry.GetData()
+        cfile = None
+        if entry._type == 'raw':
+            cfile = cbfs.add_file_raw(entry._cbfs_name, data,
+                                      entry._cbfs_offset,
+                                      entry._cbfs_compress)
+        elif entry._type == 'stage':
+            cfile = cbfs.add_file_stage(entry._cbfs_name, data,
+                                        entry._cbfs_offset)
+        else:
+            entry.Raise("Unknown cbfs-type '%s' (use 'raw', 'stage')" %
+                        entry._type)
+        return cfile
+
+    def ObtainContents(self, skip_entry=None):
         arch = cbfs_util.find_arch(self._cbfs_arg)
         if arch is None:
             self.Raise("Invalid architecture '%s'" % self._cbfs_arg)
@@ -201,22 +218,9 @@ class Entry_cbfs(Entry):
             self.Raise("'cbfs' entry must have a size property")
         cbfs = CbfsWriter(self.size, arch)
         for entry in self._entries.values():
-            # First get the input data and put it in a file. If not available,
-            # try later.
-            if entry != skip and not entry.ObtainContents():
+            if entry != skip_entry and not entry.ObtainContents():
                 return False
-            data = entry.GetData()
-            cfile = None
-            if entry._type == 'raw':
-                cfile = cbfs.add_file_raw(entry._cbfs_name, data,
-                                          entry._cbfs_offset,
-                                          entry._cbfs_compress)
-            elif entry._type == 'stage':
-                cfile = cbfs.add_file_stage(entry._cbfs_name, data,
-                                            entry._cbfs_offset)
-            else:
-                entry.Raise("Unknown cbfs-type '%s' (use 'raw', 'stage')" %
-                            entry._type)
+            cfile = self.ObtainCfile(cbfs, entry)
             if cfile:
                 entry._cbfs_file = cfile
         data = cbfs.get_data()
@@ -285,5 +289,7 @@ class Entry_cbfs(Entry):
         return cfile.data if decomp else cfile.orig_data
 
     def WriteChildData(self, child):
-        self.ObtainContents(skip=child)
+        # Recreate the data structure, leaving the data for this child alone,
+        # so that child.data is used to pack into the FIP.
+        self.ObtainContents(skip_entry=child)
         return True
-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH 17/17] binman: Rename testCbfsNoCOntents()

[Simon Glass]

Use a lower-case O as was intended.

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

 tools/binman/ftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 3982560c47c..0f4330b6807 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -2251,7 +2251,7 @@ class TestFunctional(unittest.TestCase):
             self._DoReadFile('107_cbfs_no_size.dts')
         self.assertIn('entry must have a size property', str(e.exception))
 
-    def testCbfsNoCOntents(self):
+    def testCbfsNoContents(self):
         """Test handling of a CBFS entry which does not provide contentsy"""
         with self.assertRaises(ValueError) as e:
             self._DoReadFile('108_cbfs_no_contents.dts')
-- 
2.34.0.rc2.393.gf8c9666880-goog

Re: [PATCH 17/17] binman: Rename testCbfsNoCOntents()

[Simon Glass]

Use a lower-case O as was intended.

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

 tools/binman/ftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 15/17] binman: cbfs: Refactor the init process

[Simon Glass]

Update the constructor to work in the recommended way, where the node
properties are read in a separate function. This makes it more similar to
entry_Section.

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

 tools/binman/etype/cbfs.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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

Re: [PATCH 16/17] binman: cfbs: Refactor ObtainContents() for consistency

[Simon Glass]

Update this to use the same arguments as entry_Section uses.

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

 tools/binman/etype/cbfs.py | 40 ++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

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

Re: [PATCH 14/17] binman: Use normal entries in cbfs

[Simon Glass]

This currently uses _cbfs_entries[] to store entries. Since the entries
are in fact valid etypes, we may as well use the same name as
entry_Section uses, which is _entries. This allows reusing more of the
code there (in a future patch).

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

 tools/binman/etype/cbfs.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

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

Re: [PATCH 13/17] binman: Move cbfs.ObtainContents() down a bit

[Simon Glass]

It is easier to understand this file if reading the entries comes before
obtaining the contents, since that is the order in which Binman proceeds.
Move the function down a bit.

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

 tools/binman/etype/cbfs.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

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

Re: [PATCH 12/17] binman: Update the section documentation

[Simon Glass]

Expand this to explain subclassing better and also to tidy up formatting
for rST.

Fix a few pylint warnings to avoid dropping the score.

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

 tools/binman/entries.rst      | 149 +++++++++++++++++++++++++++-------
 tools/binman/etype/section.py | 148 +++++++++++++++++++++++++++------
 2 files changed, 242 insertions(+), 55 deletions(-)

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

Re: [PATCH 11/17] binman: Allow control of which entries to read

[Simon Glass]

The ObtainContents() and GetEntryContents() methods in this file read
every single entry in the section. This is the common case.

However when one of the entries has had its data updated (e.g. with
'binman replace') we don't want to read it again from the file. Allow
the entry to be skipped, for this purpose. This is currently done in the
CBFS implementation, so adding it here will allow that to use more of
the entry_Section code.

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

 tools/binman/etype/section.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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

Re: [PATCH 10/17] binman: Allow overriding BuildSectionData()

[Simon Glass]

This method is currently marked private. However it is useful to be able
to subclass it, since much of the entry_Section code can be reused. Rename
it.

Also document one confusing part of this code, so people can understand
how to add a test for this case.

Fix up a few pylint warnings to avoid regressing the score.

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

 tools/binman/etype/section.py | 16 ++++++++++++----
 tools/binman/ftest.py         |  2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

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

Re: [PATCH 09/17] binman: Drop the filename property in entry_Section

[Simon Glass]

This is not used and does nothing. Drop it.

Add a tweak to avoid reducing the pylint score.

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

 tools/binman/etype/section.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

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

Re: [PATCH 07/17] binman: Correct comments for ReadChildData()

[Simon Glass]

The comment here is incomplete. Fix it.

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

 tools/binman/entry.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

Re: [PATCH 08/17] binman: Drop the underscore in _ReadEntries()

[Simon Glass]

This function can be overridden so should not have an underscore. Drop it.

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

 tools/binman/etype/blob_phase.py | 2 +-
 tools/binman/etype/cbfs.py       | 4 ++--
 tools/binman/etype/files.py      | 2 +-
 tools/binman/etype/section.py    | 8 ++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

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

Re: [PATCH 06/17] binman: Correct init of entry in Entry class

[Simon Glass]

This should not have an underscore. Drop it so that derived classes can
rely on it being set correctly.

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

 tools/binman/entry.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 05/17] binman: Add a way to obtain the version

[Simon Glass]

Add a -V option which shows the version number of binman. For now this
just uses a local 'version' file. Once the tool is packaged in some way
we can figure out an approach that suits.

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

 tools/binman/cmdline.py | 29 +++++++++++++++++++++++++++++
 tools/binman/ftest.py   | 20 ++++++++++++++++++++
 tools/binman/state.py   | 18 ++++++++++++++++++
 3 files changed, 67 insertions(+)

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

Re: [PATCH 03/17] dtoc: Add support for reading fixed-length bytes properties

[Simon Glass]

Add functions to read a sequence of bytes from the devicetree.

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

 scripts/pylint.base    |  4 ++--
 tools/dtoc/fdt_util.py | 20 ++++++++++++++++++++
 tools/dtoc/test_fdt.py | 17 +++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

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

Re: [PATCH 04/17] binman: Tidy up style in cmdline

[Simon Glass]

Update this file to improve the pylint score a little. The remaining item
is:

   Function name "ParseArgs" doesn't conform to snake_case naming style

which needs some binman-wide renaming.

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

 scripts/pylint.base     |  2 +-
 tools/binman/cmdline.py | 45 ++++++++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 19 deletions(-)

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

Re: [PATCH 02/17] dtoc: Add support for reading 64-bit ints

[Simon Glass]

Add functions to read a 64-bit integer property from the devicetree.

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

 tools/dtoc/fdt_util.py               | 35 ++++++++++++++++++++++++++++
 tools/dtoc/test/dtoc_test_simple.dts |  1 +
 tools/dtoc/test_dtoc.py              |  2 ++
 tools/dtoc/test_fdt.py               | 21 ++++++++++++++---
 4 files changed, 56 insertions(+), 3 deletions(-)

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

Re: [PATCH 01/17] dtoc: Bring in the libfdt module automatically

[Simon Glass]

Use the same technique as with binman to load this module from the U-Boot
tree if available. This allows running tests without having to specify
the PYTHONPATH variable.

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

 tools/dtoc/test_fdt.py | 6 ++++++
 1 file changed, 6 insertions(+)

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