If an entry argument is needed by an entry but the entry argument is not present, then a strange error can occur when trying to read the file.

Fix this by allowing arguments to be required. Select this option for the cros-ec-rw entry. If a filename is provided in the node, allow that to be used.

Also tidy up a few related tests to make the error string easier to find, and fully ignore unused return values.

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

tools/binman/README.entries | 7 ++++++- tools/binman/etype/blob_named_by_arg.py | 10 ++++++---- tools/binman/etype/cros_ec_rw.py | 3 +-- tools/binman/ftest.py | 15 +++++++++++---- 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/tools/binman/README.entries b/tools/binman/README.entries index bf8edce02b4..97bfae16116 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -60,7 +60,7 @@ Entry: blob-named-by-arg: A blob entry which gets its filename property from its

Properties / Entry arguments: - <xxx>-path: Filename containing the contents of this entry (optional, - defaults to 0) + defaults to None)

where <xxx> is the blob_fname argument to the constructor.

@@ -691,6 +691,11 @@ Properties / Entry arguments: (see binman README for more information) name-prefix: Adds a prefix to the name of every entry in the section when writing out the map

+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. + Since a section is also an entry, it inherits all the properies of entries too.

diff --git a/tools/binman/etype/blob_named_by_arg.py b/tools/binman/etype/blob_named_by_arg.py index e95dabe4d07..7c486b2dc91 100644 --- a/tools/binman/etype/blob_named_by_arg.py +++ b/tools/binman/etype/blob_named_by_arg.py @@ -17,7 +17,7 @@ class Entry_blob_named_by_arg(Entry_blob):

Properties / Entry arguments: - <xxx>-path: Filename containing the contents of this entry (optional, - defaults to 0) + defaults to None)

where <xxx> is the blob_fname argument to the constructor.

@@ -28,7 +28,9 @@ class Entry_blob_named_by_arg(Entry_blob):

See cros_ec_rw for an example of this. """ - def __init__(self, section, etype, node, blob_fname): + def __init__(self, section, etype, node, blob_fname, required=False): super().__init__(section, etype, node) - self._filename, = self.GetEntryArgsOrProps( - [EntryArg('%s-path' % blob_fname, str)]) + filename, = self.GetEntryArgsOrProps( + [EntryArg('%s-path' % blob_fname, str)], required=required) + if filename: + self._filename = filename diff --git a/tools/binman/etype/cros_ec_rw.py b/tools/binman/etype/cros_ec_rw.py index 741372e1af4..bf676b2d1a7 100644 --- a/tools/binman/etype/cros_ec_rw.py +++ b/tools/binman/etype/cros_ec_rw.py @@ -7,7 +7,6 @@

from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg

- class Entry_cros_ec_rw(Entry_blob_named_by_arg): """A blob entry which contains a Chromium OS read-write EC image

@@ -18,5 +17,5 @@ class Entry_cros_ec_rw(Entry_blob_named_by_arg): updating the EC on startup via software sync. """ def __init__(self, section, etype, node): - super().__init__(section, etype, node, 'cros-ec-rw') + super().__init__(section, etype, node, 'cros-ec-rw', required=True) self.external = True diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index bf7f59fb841..258296f2459 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1383,8 +1383,9 @@ class TestFunctional(unittest.TestCase): } with self.assertRaises(ValueError) as e: self._DoReadFileDtb('064_entry_args_required.dts') - self.assertIn("Node '/binman/_testing': Missing required " - 'properties/entry args: test-str-arg, test-int-fdt, test-int-arg', + self.assertIn("Node '/binman/_testing': " + 'Missing required properties/entry args: test-str-arg, ' + 'test-int-fdt, test-int-arg', str(e.exception))

def testEntryArgsInvalidFormat(self): @@ -1488,8 +1489,7 @@ class TestFunctional(unittest.TestCase): entry_args = { 'cros-ec-rw-path': 'ecrw.bin', } - data, _, _, _ = self._DoReadFileDtb('068_blob_named_by_arg.dts', - entry_args=entry_args) + self._DoReadFileDtb('068_blob_named_by_arg.dts', entry_args=entry_args)

def testFill(self): """Test for an fill entry type""" @@ -3478,5 +3478,12 @@ class TestFunctional(unittest.TestCase): fnode = dtb.GetNode('/images/kernel') self.assertNotIn('data', fnode.props)

+ def testBlobNamedByArgMissing(self): + """Test handling of a missing entry arg""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('068_blob_named_by_arg.dts') + self.assertIn("Missing required properties/entry args: cros-ec-rw-path", + str(e.exception)) + if __name__ == "__main__": unittest.main()

At present the Python sequential-write interface can produce an error when it calls fdt_finish(), since this needs to add a terminating tag to the end of the struct section.

Fix this by automatically expanding the buffer if needed.

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

scripts/dtc/pylibfdt/libfdt.i_shipped | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/pylibfdt/libfdt.i_shipped b/scripts/dtc/pylibfdt/libfdt.i_shipped index fae0b27d7d0..1d69ad38e2e 100644 --- a/scripts/dtc/pylibfdt/libfdt.i_shipped +++ b/scripts/dtc/pylibfdt/libfdt.i_shipped @@ -786,7 +786,8 @@ class FdtSw(FdtRo): Fdt object allowing access to the newly created device tree """ fdtsw = bytearray(self._fdt) - check_err(fdt_finish(fdtsw)) + while self.check_space(fdt_finish(fdtsw)): + fdtsw = bytearray(self._fdt) return Fdt(fdtsw)

def check_space(self, val):

In some cases it is useful to generate a FIT which has a number of DTB images, selectable by configuration. Add support for this in binman, using a simple iterator and string substitution.

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

tools/binman/README.entries | 42 +++++++++++++++- tools/binman/etype/fit.py | 76 ++++++++++++++++++++++++++--- tools/binman/ftest.py | 80 +++++++++++++++++++++++++++++-- tools/binman/test/166_fit_fdt.dts | 55 +++++++++++++++++++++ 4 files changed, 242 insertions(+), 11 deletions(-) create mode 100644 tools/binman/test/166_fit_fdt.dts

diff --git a/tools/binman/README.entries b/tools/binman/README.entries index 85b8ec9fa73..2bf7655b7f0 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -335,6 +335,7 @@ For example, this creates an image containing a FIT with U-Boot SPL: binman { fit { description = "Test FIT"; + fit,fdt-list = "of-list";

images { kernel@1 { @@ -353,7 +354,46 @@ For example, this creates an image containing a FIT with U-Boot SPL: }; };

-Properties: +U-Boot supports creating fdt and config nodes automatically. To do this, +pass an of-list property (e.g. -a of-list=file1 file2). This tells binman +that you want to generates nodes for two files: file1.dtb and file2.dtb +The fit,fdt-list property (see above) indicates that of-list should be used. + +Then add a 'generator node', a node with a name starting with '@': + + @fdt-SEQ { + description = "fdt-NAME"; + type = "flat_dt"; + compression = "none"; + }; + +This tells binman to create nodes fdt-1 and fdt-2 for each of your two +files. All the properties you specify will be included in the node. This +node acts like a template to generate the nodes. The generator node itself +does not appear in the output - it is replaced with what binman generates. + +You can create config nodes in a similar way: + + @config-SEQ { + description = "NAME"; + firmware = "uboot"; + loadables = "atf"; + fdt = "fdt-SEQ"; + }; + +This tells binman to create nodes config-1 and config-2, i.e. a config for +each of your two files. + +Available substitutions for '@' nodes are: + + SEQ Sequence number of the generated fdt (1, 2, ...) + NAME Name of the dtb as provided (i.e. without adding '.dtb') + +Note that if no device tree files are provided (with '-a of-list' as above) +then no nodes will be generated. + + +Properties (in the 'fit' node itself): fit,external-offset: Indicates that the contents of the FIT are external and provides the external offset. This is passsed to mkimage via the -E and -p flags. diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 75712f44092..e45a99dfd73 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -8,7 +8,7 @@ from collections import defaultdict, OrderedDict import libfdt

-from binman.entry import Entry +from binman.entry import Entry, EntryArg from dtoc import fdt_util from dtoc.fdt import Fdt from patman import tools @@ -27,6 +27,7 @@ class Entry_fit(Entry): binman { fit { description = "Test FIT"; + fit,fdt-list = "of-list";

images { kernel@1 { @@ -45,7 +46,46 @@ class Entry_fit(Entry): }; };

- Properties: + U-Boot supports creating fdt and config nodes automatically. To do this, + pass an of-list property (e.g. -a of-list=file1 file2). This tells binman + that you want to generates nodes for two files: file1.dtb and file2.dtb + The fit,fdt-list property (see above) indicates that of-list should be used. + + Then add a 'generator node', a node with a name starting with '@': + + @fdt-SEQ { + description = "fdt-NAME"; + type = "flat_dt"; + compression = "none"; + }; + + This tells binman to create nodes fdt-1 and fdt-2 for each of your two + files. All the properties you specify will be included in the node. This + node acts like a template to generate the nodes. The generator node itself + does not appear in the output - it is replaced with what binman generates. + + You can create config nodes in a similar way: + + @config-SEQ { + description = "NAME"; + firmware = "uboot"; + loadables = "atf"; + fdt = "fdt-SEQ"; + }; + + This tells binman to create nodes config-1 and config-2, i.e. a config for + each of your two files. + + Available substitutions for '@' nodes are: + + SEQ Sequence number of the generated fdt (1, 2, ...) + NAME Name of the dtb as provided (i.e. without adding '.dtb') + + Note that if no device tree files are provided (with '-a of-list' as above) + then no nodes will be generated. + + + Properties (in the 'fit' node itself): fit,external-offset: Indicates that the contents of the FIT are external and provides the external offset. This is passsed to mkimage via the -E and -p flags. @@ -64,6 +104,15 @@ class Entry_fit(Entry): self._fit = None self._fit_content = defaultdict(list) self._fit_props = {} + for pname, prop in self._node.props.items(): + if pname.startswith('fit,'): + self._fit_props[pname] = prop + + self._fdts = [] + list_prop = self._fit_props.get('fit,fdt-list') + if list_prop: + fdts, = self.GetEntryArgsOrProps([EntryArg(list_prop.value, str)]) + self._fdts = fdts and fdts.split() or []

def ReadNode(self): self._ReadSubnodes() @@ -84,13 +133,12 @@ class Entry_fit(Entry): image """ for pname, prop in node.props.items(): - if pname.startswith('fit,'): - self._fit_props[pname] = prop - else: + if not pname.startswith('fit,'): fsw.property(pname, prop.bytes)

rel_path = node.path[len(base_node.path):] - has_images = depth == 2 and rel_path.startswith('/images/') + in_images = rel_path.startswith('/images') + has_images = depth == 2 and in_images for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or subnode.name.startswith('signature')): @@ -100,6 +148,22 @@ class Entry_fit(Entry): entry = Entry.Create(self.section, subnode) entry.ReadNode() self._fit_content[rel_path].append(entry) + elif subnode.name.startswith('@'): + for seq, fdt_fname in enumerate(self._fdts): + node_name = subnode.name[1:].replace('SEQ', + str(seq + 1)) + fname = tools.GetInputFilename(fdt_fname + '.dtb') + with fsw.add_node(node_name): + for pname, prop in subnode.props.items(): + val = prop.bytes.replace( + b'NAME', tools.ToBytes(fdt_fname)) + val = val.replace(b'SEQ', + tools.ToBytes(str(seq + 1))) + fsw.property(pname, val) + + # Add data for 'fdt' nodes (but not 'config') + if depth == 1 and in_images: + fsw.property('data', tools.ReadFile(fname)) else: with fsw.add_node(subnode.name): _AddNode(base_node, depth + 1, subnode) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 30c9c764327..67b86276724 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -76,6 +76,11 @@ FSP_M_DATA = b'fsp_m' FSP_S_DATA = b'fsp_s' FSP_T_DATA = b'fsp_t' ATF_BL31_DATA = b'bl31' +TEST_FDT1_DATA = b'fdt1' +TEST_FDT2_DATA = b'test-fdt2' + +# Subdirectory of the input dir to use to put test FDTs +TEST_FDT_SUBDIR = 'fdts'

# The expected size for the device tree in some tests EXTRACT_DTB_SIZE = 0x3c9 @@ -171,6 +176,12 @@ class TestFunctional(unittest.TestCase): TestFunctional._MakeInputFile('compress', COMPRESS_DATA) TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA)

+ # Add a few .dtb files for testing + TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR, + TEST_FDT1_DATA) + TestFunctional._MakeInputFile('%s/test-fdt2.dtb' % TEST_FDT_SUBDIR, + TEST_FDT2_DATA) + # Travis-CI may have an old lz4 cls.have_lz4 = True try: @@ -288,7 +299,7 @@ class TestFunctional(unittest.TestCase):

def _DoTestFile(self, fname, debug=False, map=False, update_dtb=False, entry_args=None, images=None, use_real_dtb=False, - verbosity=None, allow_missing=False): + verbosity=None, allow_missing=False, extra_indirs=None): """Run binman with a given test file

Args: @@ -308,6 +319,7 @@ class TestFunctional(unittest.TestCase): verbosity: Verbosity level to use (0-3, None=don't set it) allow_missing: Set the '--allow-missing' flag so that missing external binaries just produce a warning instead of an error + extra_indirs: Extra input directories to add using -I """ args = [] if debug: @@ -334,6 +346,9 @@ class TestFunctional(unittest.TestCase): if images: for image in images: args += ['-i', image] + if extra_indirs: + for indir in extra_indirs: + args += ['-I', indir] return self._DoBinman(*args)

def _SetupDtb(self, fname, outfile='u-boot.dtb'): @@ -383,7 +398,8 @@ class TestFunctional(unittest.TestCase): return dtb.GetContents()

def _DoReadFileDtb(self, fname, use_real_dtb=False, map=False, - update_dtb=False, entry_args=None, reset_dtbs=True): + update_dtb=False, entry_args=None, reset_dtbs=True, + extra_indirs=None): """Run binman and return the resulting image

This runs binman with a given test file and then reads the resulting @@ -407,6 +423,7 @@ class TestFunctional(unittest.TestCase): reset_dtbs: With use_real_dtb the test dtb is overwritten by this function. If reset_dtbs is True, then the original test dtb is written back before this function finishes + extra_indirs: Extra input directories to add using -I

Returns: Tuple: @@ -430,7 +447,8 @@ class TestFunctional(unittest.TestCase):

try: retcode = self._DoTestFile(fname, map=map, update_dtb=update_dtb, - entry_args=entry_args, use_real_dtb=use_real_dtb) + entry_args=entry_args, use_real_dtb=use_real_dtb, + extra_indirs=extra_indirs) self.assertEqual(0, retcode) out_dtb_fname = tools.GetOutputFilename('u-boot.dtb.out')

@@ -3512,7 +3530,61 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('165_atf_bl31.dts') self.assertEqual(ATF_BL31_DATA, data[:len(ATF_BL31_DATA)])

-ATF_BL31_DATA + def testFitFdt(self): + """Test an image with an FIT with multiple FDT images""" + def _CheckFdt(seq, expected_data): + """Check the FDT nodes + + Args: + seq: Sequence number to check (0 or 1) + expected_data: Expected contents of 'data' property + """ + name = 'fdt-%d' % seq + fnode = dtb.GetNode('/images/%s' % name) + self.assertIsNotNone(fnode) + self.assertEqual({'description','type', 'compression', 'data'}, + set(fnode.props.keys())) + self.assertEqual(expected_data, fnode.props['data'].bytes) + self.assertEqual('fdt-test-fdt%d.dtb' % seq, + fnode.props['description'].value) + def _CheckConfig(seq, expected_data): + """Check the configuration nodes + + Args: + seq: Sequence number to check (0 or 1) + expected_data: Expected contents of 'data' property + """ + name = 'config-%d' % seq + fnode = dtb.GetNode('/configurations/%s' % name) + self.assertIsNotNone(fnode) + self.assertEqual({'description','firmware', 'loadables', 'fdt'}, + set(fnode.props.keys())) + self.assertEqual('conf-test-fdt%d.dtb' % seq, + fnode.props['description'].value) + self.assertEqual('fdt-%d' % seq, fnode.props['fdt'].value) + + entry_args = { + 'of-list': 'test-fdt1 test-fdt2', + } + data = self._DoReadFileDtb( + '166_fit_fdt.dts', + entry_args=entry_args, + extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0] + self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):]) + fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)] + + dtb = fdt.Fdt.FromData(fit_data) + dtb.Scan() + fnode = dtb.GetNode('/images/kernel') + self.assertIn('data', fnode.props) + + # Check all the properties in fdt-1 and fdt-2 + _CheckFdt(1, TEST_FDT1_DATA) + _CheckFdt(2, TEST_FDT2_DATA) + + # Check configurations + _CheckConfig(1, TEST_FDT1_DATA) + _CheckConfig(2, TEST_FDT2_DATA)

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/166_fit_fdt.dts b/tools/binman/test/166_fit_fdt.dts new file mode 100644 index 00000000000..89142e91017 --- /dev/null +++ b/tools/binman/test/166_fit_fdt.dts @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + fit { + description = "test-desc"; + #address-cells = <1>; + fit,fdt-list = "of-list"; + + images { + kernel { + description = "Vanilla Linux kernel"; + type = "kernel"; + arch = "ppc"; + os = "linux"; + compression = "gzip"; + load = <00000000>; + entry = <00000000>; + hash-1 { + algo = "crc32"; + }; + hash-2 { + algo = "sha1"; + }; + u-boot { + }; + }; + @fdt-SEQ { + description = "fdt-NAME.dtb"; + type = "flat_dt"; + compression = "none"; + }; + }; + + configurations { + default = "config-1"; + @config-SEQ { + description = "conf-NAME.dtb"; + firmware = "uboot"; + loadables = "atf"; + fdt = "fdt-SEQ"; + }; + }; + }; + u-boot-nodtb { + }; + }; +};

This file is no-longer used. Drop it.

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

board/sunxi/mksunxi_fit_atf.sh | 87 ---------------------------------- 1 file changed, 87 deletions(-) delete mode 100755 board/sunxi/mksunxi_fit_atf.sh

diff --git a/board/sunxi/mksunxi_fit_atf.sh b/board/sunxi/mksunxi_fit_atf.sh deleted file mode 100755 index 88ad7197470..00000000000 --- a/board/sunxi/mksunxi_fit_atf.sh +++ /dev/null @@ -1,87 +0,0 @@ -#!/bin/sh -# -# script to generate FIT image source for 64-bit sunxi boards with -# ARM Trusted Firmware and multiple device trees (given on the command line) -# -# usage: $0 <dt_name> [<dt_name> [<dt_name] ...] - -[ -z "$BL31" ] && BL31="bl31.bin" - -if [ ! -f $BL31 ]; then - echo "WARNING: BL31 file $BL31 NOT found, resulting binary is non-functional" >&2 - echo "Please read the section on ARM Trusted Firmware (ATF) in board/sunxi/README.sunxi64" >&2 - BL31=/dev/null -fi - -if grep -q "^CONFIG_MACH_SUN50I_H6=y" .config; then - BL31_ADDR=0x104000 -else - BL31_ADDR=0x44000 -fi - -cat << __HEADER_EOF -/dts-v1/; - -/ { - description = "Configuration to load ATF before U-Boot"; - #address-cells = <1>; - - images { - uboot { - description = "U-Boot (64-bit)"; - data = /incbin/("u-boot-nodtb.bin"); - type = "standalone"; - arch = "arm64"; - compression = "none"; - load = <0x4a000000>; - }; - atf { - description = "ARM Trusted Firmware"; - data = /incbin/("$BL31"); - type = "firmware"; - arch = "arm64"; - compression = "none"; - load = <$BL31_ADDR>; - entry = <$BL31_ADDR>; - }; -__HEADER_EOF - -cnt=1 -for dtname in $* -do - cat << __FDT_IMAGE_EOF - fdt_$cnt { - description = "$(basename $dtname .dtb)"; - data = /incbin/("$dtname"); - type = "flat_dt"; - compression = "none"; - }; -__FDT_IMAGE_EOF - cnt=$((cnt+1)) -done - -cat << __CONF_HEADER_EOF - }; - configurations { - default = "config_1"; - -__CONF_HEADER_EOF - -cnt=1 -for dtname in $* -do - cat << __CONF_SECTION_EOF - config_$cnt { - description = "$(basename $dtname .dtb)"; - firmware = "uboot"; - loadables = "atf"; - fdt = "fdt_$cnt"; - }; -__CONF_SECTION_EOF - cnt=$((cnt+1)) -done - -cat << __ITS_EOF - }; -}; -__ITS_EOF