Add tests for missing tools to complete the test coverage for this etype.

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

tools/binman/etype/efi_capsule.py | 2 ++ tools/binman/ftest.py | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py index 5941545d0b2..aac8789f9cb 100644 --- a/tools/binman/etype/efi_capsule.py +++ b/tools/binman/etype/efi_capsule.py @@ -151,6 +151,8 @@ class Entry_efi_capsule(Entry_section): return tools.read_file(capsule_fname) else: # Bintool is missing; just use the input data as the output + if not self.GetAllowMissing(): + self.Raise("Missing tool: 'mkeficapsule'") self.record_missing_bintool(self.mkeficapsule) return data

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 93f3d22cf57..50ba51f8b63 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -403,8 +403,10 @@ class TestFunctional(unittest.TestCase): test_section_timeout: True to force the first time to timeout, as used in testThreadTimeout() update_fdt_in_elf: Value to pass with --update-fdt-in-elf=xxx - force_missing_tools (str): comma-separated list of bintools to + force_missing_bintools (str): comma-separated list of bintools to regard as missing + ignore_missing (bool): True to return success even if there are + missing blobs or bintools output_dir: Specific output directory to use for image using -O

Returns: @@ -7690,6 +7692,24 @@ fdt fdtmap Extract the devicetree blob from the fdtmap # Make sure the other node is gone self.assertIsNone(dtb.GetNode('/node/other-node'))

+ def testMkeficapsuleMissing(self): + """Test that binman complains if mkeficapsule is missing""" + with self.assertRaises(ValueError) as e: + self._DoTestFile('311_capsule.dts', + force_missing_bintools='mkeficapsule') + self.assertIn("Node '/binman/efi-capsule': Missing tool: 'mkeficapsule'", + str(e.exception)) + + def testMkeficapsuleMissingOk(self): + """Test that binman deals with mkeficapsule being missing""" + with test_util.capture_sys_output() as (stdout, stderr): + ret = self._DoTestFile('311_capsule.dts', + force_missing_bintools='mkeficapsule', + allow_missing=True) + self.assertEqual(103, ret) + err = stderr.getvalue() + self.assertRegex(err, "Image 'image'.*missing bintools.*: mkeficapsule") +

if __name__ == "__main__": unittest.main()

This returns stdout, not a CommandResult so update the comment.

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

tools/binman/btool/fdtgrep.py | 3 +-- tools/binman/etype/fit.py | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/binman/btool/fdtgrep.py b/tools/binman/btool/fdtgrep.py index da1f8c7bf4e..446b2f4144b 100644 --- a/tools/binman/btool/fdtgrep.py +++ b/tools/binman/btool/fdtgrep.py @@ -74,8 +74,7 @@ class Bintoolfdtgrep(bintool.Bintool): (with only neceesary nodes and properties)

Returns: - CommandResult: Resulting output from the bintool, or None if the - tool is not present + str or bytes: Resulting stdout from the bintool """ if phase == 'tpl': tag = 'bootph-pre-sram' diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index ee44e5a1cd6..b957df54548 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -550,6 +550,9 @@ class Entry_fit(Entry_section): phase (str): Phase to generate for ('tpl', 'vpl', 'spl') outfile (str): Output filename to write the grepped FDT contents to (with only neceesary nodes and properties) + + Returns: + str or bytes: Resulting stdout from fdtgrep """ return self.fdtgrep.create_for_phase(infile, phase, outfile, self._remove_props)

Improve the comments for this function.

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

tools/binman/entry.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 6d2f3789940..7d4d4692776 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -576,8 +576,16 @@ class Entry(object): def GetEntryArgsOrProps(self, props, required=False): """Return the values of a set of properties

+ Looks up the named entryargs and returns the value for each. If any + required ones are missing, the error is reported to the user. + Args: - props: List of EntryArg objects + props (list of EntryArg): List of entry arguments to look up + required (bool): True if these entry arguments are required + + Returns: + list of values: one for each item in props, the type is determined + by the EntryArg's 'datatype' property (str or int)

Raises: ValueError if a property is not found

Update this entry type to resolve some pylint warnings and make sure that functions and members are fully commented.

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

tools/binman/etype/fit.py | 73 ++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index b957df54548..f25226d3a73 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -6,9 +6,10 @@ """Entry-type module for producing a FIT"""

import glob -import libfdt import os

+import libfdt + from binman.entry import Entry, EntryArg from binman.etype.section import Entry_section from binman import elf @@ -23,6 +24,7 @@ OPERATIONS = { 'split-elf': OP_SPLIT_ELF, }

+# pylint: disable=invalid-name class Entry_fit(Entry_section):

"""Flat Image Tree (FIT) @@ -381,31 +383,46 @@ class Entry_fit(Entry_section): def __init__(self, section, etype, node): """ Members: - _fit: FIT file being built - _entries: dict from Entry_section: + _fit (str): FIT file being built + _fit_props (list of str): 'fit,...' properties found in the + top-level node + _fdts (list of str): Filenames of .dtb files to process + _fdt_dir (str): Directory to scan to find .dtb files, or None + _fit_list_prop (str): Name of the EntryArg containing a list of .dtb + files + _fit_default_dt (str): Name of the EntryArg containing the default + .dtb file + _entries (dict of entries): from Entry_section: key: relative path to entry Node (from the base of the FIT) value: Entry_section object comprising the contents of this node - _priv_entries: Internal copy of _entries which includes 'generator' - entries which are used to create the FIT, but should not be - processed as real entries. This is set up once we have the - entries - _loadables: List of generated split-elf nodes, each a node name + _priv_entries (dict of entries): Internal copy of _entries which + includes 'generator' entries which are used to create the FIT, + but should not be processed as real entries. This is set up once + we have the entries + _loadables (list of str): List of generated split-elf nodes, each + a node name + _remove_props (list of str): Value of of-spl-remove-props EntryArg, + the list of properties to remove with fdtgrep + mkimage (Bintool): mkimage tool + fdtgrep (Bintool): fdtgrep tool """ super().__init__(section, etype, node) self._fit = None self._fit_props = {} self._fdts = None self._fdt_dir = None - self.mkimage = None - self.fdtgrep = None + self._fit_list_prop = None + self._fit_default_dt = None self._priv_entries = {} self._loadables = [] self._remove_props = [] - props, = self.GetEntryArgsOrProps( - [EntryArg('of-spl-remove-props', str)], required=False) + props = self.GetEntryArgsOrProps( + [EntryArg('of-spl-remove-props', str)], required=False)[0] if props: self._remove_props = props.split() + self.mkimage = None + self.fdtgrep = None

def ReadNode(self): super().ReadNode() @@ -414,8 +431,8 @@ class Entry_fit(Entry_section): self._fit_props[pname] = prop self._fit_list_prop = self._fit_props.get('fit,fdt-list') if self._fit_list_prop: - fdts, = self.GetEntryArgsOrProps( - [EntryArg(self._fit_list_prop.value, str)]) + fdts = self.GetEntryArgsOrProps( + [EntryArg(self._fit_list_prop.value, str)])[0] if fdts is not None: self._fdts = fdts.split() else: @@ -431,7 +448,7 @@ class Entry_fit(Entry_section): self._fit_default_dt = self.GetEntryArgsOrProps([EntryArg('default-dt', str)])[0]

- def _get_operation(self, base_node, node): + def _get_operation(self, node): """Get the operation referenced by a subnode

Args: @@ -560,9 +577,6 @@ class Entry_fit(Entry_section): def _build_input(self): """Finish the FIT by adding the 'data' properties to it

- Arguments: - fdt: FIT to update - Returns: bytes: New fdt contents """ @@ -637,7 +651,7 @@ class Entry_fit(Entry_section): result.append(name) return firmware, result

- def _gen_fdt_nodes(base_node, node, depth, in_images): + def _gen_fdt_nodes(node, depth, in_images): """Generate FDT nodes

This creates one node for each member of self._fdts using the @@ -710,11 +724,10 @@ class Entry_fit(Entry_section): else: self.Raise("Generator node requires 'fit,fdt-list' property")

- def _gen_split_elf(base_node, node, depth, segments, entry_addr): + def _gen_split_elf(node, depth, segments, entry_addr): """Add nodes for the ELF file, one per group of contiguous segments

Args: - base_node (Node): Template node from the binman definition node (Node): Node to replace (in the FIT being built) depth: Current node depth (0 is the base 'fit' node) segments (list): list of segments, each: @@ -745,7 +758,7 @@ class Entry_fit(Entry_section): with fsw.add_node(subnode.name): _add_node(node, depth + 1, subnode)

- def _gen_node(base_node, node, depth, in_images, entry): + def _gen_node(node, depth, in_images, entry): """Generate nodes from a template

This creates one or more nodes depending on the fit,operation being @@ -761,8 +774,6 @@ class Entry_fit(Entry_section): If the file is missing, nothing is generated.

Args: - base_node (Node): Base Node of the FIT (with 'description' - property) node (Node): Generator node to process depth (int): Current node depth (0 is the base 'fit' node) in_images (bool): True if this is inside the 'images' node, so @@ -770,13 +781,12 @@ class Entry_fit(Entry_section): entry (entry_Section): Entry for the section containing the contents of this node """ - oper = self._get_operation(base_node, node) + oper = self._get_operation(node) if oper == OP_GEN_FDT_NODES: - _gen_fdt_nodes(base_node, node, depth, in_images) + _gen_fdt_nodes(node, depth, in_images) elif oper == OP_SPLIT_ELF: # Entry_section.ObtainContents() either returns True or # raises an exception. - data = None missing_opt_list = [] entry.ObtainContents() entry.Pack(0) @@ -798,7 +808,7 @@ class Entry_fit(Entry_section): self._raise_subnode( node, f'Failed to read ELF file: {str(exc)}')

- _gen_split_elf(base_node, node, depth, segments, entry_addr) + _gen_split_elf(node, depth, segments, entry_addr)

def _add_node(base_node, depth, node): """Add nodes to the output FIT @@ -829,7 +839,6 @@ class Entry_fit(Entry_section): fsw.property('data', bytes(data))

for subnode in node.subnodes: - subnode_path = f'{rel_path}/{subnode.name}' if has_images and not self.IsSpecialSubnode(subnode): # This subnode is a content node not meant to appear in # the FIT (e.g. "/images/kernel/u-boot"), so don't call @@ -837,7 +846,7 @@ class Entry_fit(Entry_section): pass elif self.GetImage().generate and subnode.name.startswith('@'): entry = self._priv_entries.get(subnode.name) - _gen_node(base_node, subnode, depth, in_images, entry) + _gen_node(subnode, depth, in_images, entry) # This is a generator (template) entry, so remove it from # the list of entries used by PackEntries(), etc. Otherwise # it will appear in the binman output @@ -917,6 +926,8 @@ class Entry_fit(Entry_section):

# This should never happen else: # pragma: no cover + offset = None + size = None self.Raise(f'{path}: missing data properties')

section.SetOffsetSize(offset, size) @@ -950,7 +961,7 @@ class Entry_fit(Entry_section): if input_fname: fname = input_fname else: - fname = tools.get_output_filename('%s.fit' % uniq) + fname = tools.get_output_filename(f'{uniq}.fit') tools.write_file(fname, self.GetData()) args.append(fname)

Two places set this attribute directly. Update them to use the function provided.

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

tools/binman/etype/atf_fip.py | 2 +- tools/binman/etype/cbfs.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/binman/etype/atf_fip.py b/tools/binman/etype/atf_fip.py index 73a3f85b9f4..4d84e96c376 100644 --- a/tools/binman/etype/atf_fip.py +++ b/tools/binman/etype/atf_fip.py @@ -248,7 +248,7 @@ class Entry_atf_fip(Entry_section): fent = entry._fip_entry entry.size = fent.size entry.offset = fent.offset - entry.image_pos = self.image_pos + entry.offset + entry.SetImagePos(image_pos + self.offset)

def ReadChildData(self, child, decomp=True, alt_format=None): if not self.reader: diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 575aa624f6c..124fa1e4ffc 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -245,7 +245,7 @@ class Entry_cbfs(Entry): cfile = entry._cbfs_file entry.size = cfile.data_len entry.offset = cfile.calced_cbfs_offset - entry.image_pos = self.image_pos + entry.offset + entry.SetImagePos(image_pos + self.offset) if entry._cbfs_compress: entry.uncomp_size = cfile.memlen

Since the files are known to be in the provided directory, use that instead of requiring it to be added to the list of input directories.

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

tools/binman/etype/fit.py | 10 ++++++++-- tools/binman/ftest.py | 7 ++++++- 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index f25226d3a73..51c82c55e4a 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -96,7 +96,10 @@ class Entry_fit(Entry_section): can be provided as a directory. Each .dtb file in the directory is processed, , e.g.::

- fit,fdt-list-dir = "arch/arm/dts + fit,fdt-list-dir = "arch/arm/dts"; + + In this case the input directories are ignored and all devicetree + files must be in that directory.

Substitutions ~~~~~~~~~~~~~ @@ -671,7 +674,10 @@ class Entry_fit(Entry_section): # Generate nodes for each FDT for seq, fdt_fname in enumerate(self._fdts): node_name = node.name[1:].replace('SEQ', str(seq + 1)) - fname = tools.get_input_filename(fdt_fname + '.dtb') + if self._fdt_dir: + fname = os.path.join(self._fdt_dir, fdt_fname + '.dtb') + else: + fname = tools.get_input_filename(fdt_fname + '.dtb') fdt_phase = None with fsw.add_node(node_name): for pname, prop in node.props.items(): diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 50ba51f8b63..12cc3b78a6a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7626,7 +7626,12 @@ fdt fdtmap Extract the devicetree blob from the fdtmap

def testFitFdtListDir(self): """Test an image with an FIT with FDT images using fit,fdt-list-dir""" - self.CheckFitFdt('333_fit_fdt_dir.dts', False) + old_dir = os.getcwd() + try: + os.chdir(self._indir) + self.CheckFitFdt('333_fit_fdt_dir.dts', False) + finally: + os.chdir(old_dir)

def testFitFdtCompat(self): """Test an image with an FIT with compatible in the config nodes"""

Use the more generic variable name 'entry' to avoid confusion.

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

tools/binman/etype/fit.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 51c82c55e4a..3adc4a959e8 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -904,7 +904,7 @@ class Entry_fit(Entry_section): fdt = Fdt.FromData(self.GetData()) fdt.Scan()

- for image_name, section in self._entries.items(): + for image_name, entry in self._entries.items(): path = f"/images/{image_name}" node = fdt.GetNode(path)

@@ -936,8 +936,8 @@ class Entry_fit(Entry_section): size = None self.Raise(f'{path}: missing data properties')

- section.SetOffsetSize(offset, size) - section.SetImagePos(self.image_pos) + entry.SetOffsetSize(offset, size) + entry.SetImagePos(image_pos + self.offset)

def AddBintools(self, btools): super().AddBintools(btools)

The section etype has its own implementation of SetImagePos(), most of which is not useful since the code is included here. So call Entry.SetImagePos() which has the only piece of this which we actually want.

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

tools/binman/etype/fit.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 3adc4a959e8..96f4fdf3333 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -894,7 +894,10 @@ class Entry_fit(Entry_section): """ if self.build_done: return - super().SetImagePos(image_pos) + + # Skip the section processing, since we do that below. Just call the + # entry method + Entry.SetImagePos(self, image_pos)

# If mkimage is missing we'll have empty data, # which will cause a FDT_ERR_BADMAGIC error

With OF_UPSTREAM the dts files are in an SoC-specific subdirectory, meaning that the resulting dtb files all end up in a similar subdirectory.

We don't want the subdirectory name to appear as a node name in the FIT, so handle this as a special case.

Also the default devicetree may have a directory-name prefix, so handle that when searching through the available devicetree files.

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

tools/binman/etype/fit.py | 19 ++++++++++++------- tools/binman/ftest.py | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 96f4fdf3333..0abe1c78c43 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -600,13 +600,17 @@ class Entry_fit(Entry_section): if val.startswith('@'): if not self._fdts: return - if not self._fit_default_dt: + default_dt = self._fit_default_dt + if not default_dt: self.Raise("Generated 'default' node requires default-dt entry argument") - if self._fit_default_dt not in self._fdts: - self.Raise( - f"default-dt entry argument '{self._fit_default_dt}' " - f"not found in fdt list: {', '.join(self._fdts)}") - seq = self._fdts.index(self._fit_default_dt) + if default_dt not in self._fdts: + if self._fdt_dir: + default_dt = os.path.basename(default_dt) + if default_dt not in self._fdts: + self.Raise( + f"default-dt entry argument '{self._fit_default_dt}' " + f"not found in fdt list: {', '.join(self._fdts)}") + seq = self._fdts.index(default_dt) val = val[1:].replace('DEFAULT-SEQ', str(seq + 1)) fsw.property_string(pname, val) return @@ -711,8 +715,9 @@ class Entry_fit(Entry_section): # Add data for 'images' nodes (but not 'config') if depth == 1 and in_images: if fdt_phase: + leaf = os.path.basename(fdt_fname) phase_fname = tools.get_output_filename( - f'{fdt_fname}-{fdt_phase}.dtb') + f'{leaf}-{fdt_phase}.dtb') self._run_fdtgrep(fname, fdt_phase, phase_fname) data = tools.read_file(phase_fname) else: diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 12cc3b78a6a..56deece7230 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -4183,7 +4183,8 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('172_scp.dts') self.assertEqual(SCP_DATA, data[:len(SCP_DATA)])

- def CheckFitFdt(self, dts='170_fit_fdt.dts', use_fdt_list=True): + def CheckFitFdt(self, dts='170_fit_fdt.dts', use_fdt_list=True, + default_dt=None): """Check an image with an FIT with multiple FDT images""" def _CheckFdt(seq, expected_data): """Check the FDT nodes @@ -4227,6 +4228,8 @@ class TestFunctional(unittest.TestCase): } if use_fdt_list: entry_args['of-list'] = 'test-fdt1 test-fdt2' + if default_dt: + entry_args['default-dt'] = default_dt data = self._DoReadFileDtb( dts, entry_args=entry_args, @@ -7633,6 +7636,16 @@ fdt fdtmap Extract the devicetree blob from the fdtmap finally: os.chdir(old_dir)

+ def testFitFdtListDirDefault(self): + """Test an FIT fit,fdt-list-dir where the default DT in is a subdir""" + old_dir = os.getcwd() + try: + os.chdir(self._indir) + self.CheckFitFdt('333_fit_fdt_dir.dts', False, + default_dt='rockchip/test-fdt2') + finally: + os.chdir(old_dir) + def testFitFdtCompat(self): """Test an image with an FIT with compatible in the config nodes""" entry_args = {

These functions get the value of a symbol. The reference to ELF files is confusing since they are reading the position/size of entries, not ELF symbols. Rename the functions and adjust the comments also.

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

tools/binman/elf.py | 4 ++-- tools/binman/elf_test.py | 4 ++-- tools/binman/etype/section.py | 13 ++++++------- tools/binman/image.py | 11 +++++------ tools/binman/image_test.py | 8 ++++---- 5 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/tools/binman/elf.py b/tools/binman/elf.py index a4694056391..1c3b1e225f4 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -301,8 +301,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, value = BINMAN_SYM_MAGIC_VALUE else: # Look up the symbol in our entry tables. - value = section.GetImage().LookupImageSymbol(name, sym.weak, - msg, base_addr) + value = section.GetImage().GetImageSymbolValue(name, sym.weak, + msg, base_addr) if value is None: value = -1 pack_string = pack_string.lower() diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py index b64134123c1..2f22639dffc 100644 --- a/tools/binman/elf_test.py +++ b/tools/binman/elf_test.py @@ -37,7 +37,7 @@ class FakeSection: """A fake Section object, used for testing

This has the minimum feature set needed to support testing elf functions. - A LookupSymbol() function is provided which returns a fake value for amu + A GetSymbolValue() function is provided which returns a fake value for any symbol requested. """ def __init__(self, sym_value=1): @@ -46,7 +46,7 @@ class FakeSection: def GetPath(self): return 'section_path'

- def LookupImageSymbol(self, name, weak, msg, base_addr): + def GetImageSymbolValue(self, name, weak, msg, base_addr): """Fake implementation which returns the same value for all symbols""" return self.sym_value

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 30c1041c7e8..6c181531b8b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -563,13 +563,13 @@ class Entry_section(Entry): return entry.GetData(required)

def LookupEntry(self, entries, sym_name, msg): - """Look up the entry for an ENF symbol + """Look up the entry for a binman symbol

Args: entries (dict): entries to search: key: entry name value: Entry object - sym_name: Symbol name in the ELF file to look up in the format + sym_name: Symbol name to look up in the format _binman_<entry>_prop_<property> where <entry> is the name of the entry and <property> is the property to find (e.g. _binman_u_boot_prop_offset). As a special case, you can append @@ -606,11 +606,10 @@ class Entry_section(Entry): entry = entries[name] return entry, entry_name, prop_name

- def LookupSymbol(self, sym_name, optional, msg, base_addr, entries=None): - """Look up a symbol in an ELF file + def GetSymbolValue(self, sym_name, optional, msg, base_addr, entries=None): + """Get the value of a Binman symbol

- Looks up a symbol in an ELF file. Only entry types which come from an - ELF image can be used by this function. + Look up a Binman symbol and obtain its value.

At present the only entry properties supported are: offset @@ -618,7 +617,7 @@ class Entry_section(Entry): size

Args: - sym_name: Symbol name in the ELF file to look up in the format + sym_name: Symbol name to look up in the format _binman_<entry>_prop_<property> where <entry> is the name of the entry and <property> is the property to find (e.g. _binman_u_boot_prop_offset). As a special case, you can append diff --git a/tools/binman/image.py b/tools/binman/image.py index 702c9055585..c667f583db6 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -381,11 +381,10 @@ class Image(section.Entry_section): selected_entries.append(entry) return selected_entries, lines, widths

- def LookupImageSymbol(self, sym_name, optional, msg, base_addr): - """Look up a symbol in an ELF file + def GetImageSymbolValue(self, sym_name, optional, msg, base_addr): + """Get the value of a Binman symbol

- Looks up a symbol in an ELF file. Only entry types which come from an - ELF image can be used by this function. + Look up a Binman symbol and obtain its value.

This searches through this image including all of its subsections.

@@ -423,8 +422,8 @@ class Image(section.Entry_section): entries = OrderedDict() entries_by_name = {} self._CollectEntries(entries, entries_by_name, self) - return self.LookupSymbol(sym_name, optional, msg, base_addr, - entries_by_name) + return self.GetSymbolValue(sym_name, optional, msg, base_addr, + entries_by_name)

def CollectBintools(self): """Collect all the bintools used by this image diff --git a/tools/binman/image_test.py b/tools/binman/image_test.py index bd51c1e55d1..7d65e2d589a 100644 --- a/tools/binman/image_test.py +++ b/tools/binman/image_test.py @@ -13,7 +13,7 @@ class TestImage(unittest.TestCase): def testInvalidFormat(self): image = Image('name', 'node', test=True) with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_something_prop_', False, 'msg', 0) + image.GetSymbolValue('_binman_something_prop_', False, 'msg', 0) self.assertIn( "msg: Symbol '_binman_something_prop_' has invalid format", str(e.exception)) @@ -22,7 +22,7 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {} with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_type_prop_pname', False, 'msg', 0) + image.GetSymbolValue('_binman_type_prop_pname', False, 'msg', 0) self.assertIn("msg: Entry 'type' not found in list ()", str(e.exception))

@@ -30,7 +30,7 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {} with capture_sys_output() as (stdout, stderr): - val = image.LookupSymbol('_binman_type_prop_pname', True, 'msg', 0) + val = image.GetSymbolValue('_binman_type_prop_pname', True, 'msg', 0) self.assertEqual(val, None) self.assertEqual("Warning: msg: Entry 'type' not found in list ()\n", stderr.getvalue()) @@ -40,5 +40,5 @@ class TestImage(unittest.TestCase): image = Image('name', 'node', test=True) image._entries = {'u-boot': 1} with self.assertRaises(ValueError) as e: - image.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg', 0) + image.GetSymbolValue('_binman_u_boot_prop_bad', False, 'msg', 0) self.assertIn("msg: No such property 'bad", str(e.exception))

Add a clarification to the documentation and add a missing comment. Also update the test so that when it fails it is easier to see what is going on, rather than having to decode hex strings.

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

tools/binman/binman.rst | 4 +++- tools/binman/elf.py | 3 ++- tools/binman/ftest.py | 36 +++++++++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 0cafc36bdcb..04564f4f66f 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -494,7 +494,9 @@ point into the image. For example, say SPL is at the start of the image and linked to start at address 80108000. If U-Boot's image-pos is 0x8000 then binman will write an image-pos for U-Boot of 80110000 into the SPL binary, since it assumes the image is loaded -to 80108000, with SPL at 80108000 and U-Boot at 80110000. +to 80108000, with SPL at 80108000 and U-Boot at 80110000. In other words, the +positions are calculated relative to the start address of the image to which +they are being written.

For x86 devices (with the end-at-4gb property) this base address is not added since it is assumed that images are XIP and the offsets already include the diff --git a/tools/binman/elf.py b/tools/binman/elf.py index 1c3b1e225f4..73394830ebe 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -247,7 +247,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, entry entry: Entry to process section: Section which can be used to lookup symbol values - base_sym: Base symbol marking the start of the image + base_sym: Base symbol marking the start of the image (__image_copy_start + by default)

Returns: int: Number of symbols written diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 56deece7230..9f329704115 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1526,18 +1526,40 @@ class TestFunctional(unittest.TestCase): # 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 - sym_values = struct.pack('<LLQLL', elf.BINMAN_SYM_MAGIC_VALUE, - 0x00, u_boot_offset + len(U_BOOT_DATA), - 0x10 + u_boot_offset, 0x04) + vals = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, + u_boot_offset + len(U_BOOT_DATA), + 0x10 + u_boot_offset, 0x04) + sym_values = struct.pack('<LLQLL', *vals) if no_write_symbols: - expected = (base_data + - tools.get_bytes(0xff, 0x38 - len(base_data)) + - U_BOOT_DATA + base_data) + self.assertEqual( + base_data + + tools.get_bytes(0xff, 0x38 - len(base_data)) + + U_BOOT_DATA + base_data, data) else: + got_vals = struct.unpack('<LLQLL', data[:24]) + + # For debugging: + #print('expect:', list(f'{v:x}' for v in vals)) + #print(' got:', list(f'{v:x}' for v in got_vals)) + + self.assertEqual(vals, got_vals) + self.assertEqual(sym_values, data[:24]) + + blen = len(base_data) + self.assertEqual(base_data[24:], data[24:blen]) + self.assertEqual(0xff, data[blen]) + + ofs = blen + 1 + len(U_BOOT_DATA) + self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs]) + + self.assertEqual(sym_values, data[ofs:ofs + 24]) + self.assertEqual(base_data[24:], data[ofs + 24:]) + + # Just repeating the above asserts all at once, for clarity expected = (sym_values + base_data[24:] + tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values + base_data[24:]) - self.assertEqual(expected, data) + self.assertEqual(expected, data)

def testSymbols(self): """Test binman can assign symbols embedded in U-Boot"""

The base address of the ELF containing symbols is normally added to any symbols written, so that the value points to the correct address in memory when everything is loaded. When the binary resides on disk, a different offset may be needed, typically 0. Provide a way to specify this.

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

tools/binman/binman.rst | 15 ++++++++ tools/binman/elf.py | 7 ++-- tools/binman/entry.py | 8 ++++- tools/binman/etype/blob_phase.py | 5 +++ tools/binman/ftest.py | 35 ++++++++++++++++--- tools/binman/test/336_symbols_base.dts | 23 ++++++++++++ tools/binman/test/337_symbols_base_expand.dts | 24 +++++++++++++ 7 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 tools/binman/test/336_symbols_base.dts create mode 100644 tools/binman/test/337_symbols_base_expand.dts

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 04564f4f66f..f9a3a42183b 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -502,6 +502,10 @@ For x86 devices (with the end-at-4gb property) this base address is not added since it is assumed that images are XIP and the offsets already include the address.

+For non-x86 cases where the symbol is used as a flash offset, the symbols-base +property can be set to that offset (e.g. 0), so that the unadjusted image-pos +is written into the image. + While U-Boot's symbol updating is handled automatically by the u-boot-spl entry type (and others), it is possible to use this feature with any blob. To do this, add a `write-symbols` (boolean) property to the node, set the ELF @@ -743,6 +747,17 @@ insert-template: properties are brought into the target node. See Templates_ below for more information.

+symbols-base: + When writing symbols into a binary, the value of that symbol is assumed to + be relative to the base address of the binary. This allow the binary to be + loaded in memory at its base address, so that symbols point into the binary + correctly. In some cases the binary is in fact not yet in memory, but must + be read from storage. In this case there is no base address for the symbols. + This property can be set to 0 to indicate this. Other values for + symbols-base are allowed, but care must be taken that the code which uses + the symbol is aware of the base being used. If omitted, the binary's base + address is used. + The attributes supported for images and sections are described below. Several are similar to those for entries.

diff --git a/tools/binman/elf.py b/tools/binman/elf.py index 73394830ebe..c75f4478813 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -234,7 +234,7 @@ def GetSymbolOffset(elf_fname, sym_name, base_sym=None): return val - base

def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, - base_sym=None): + base_sym=None, base_addr=None): """Replace all symbols in an entry with their correct values

The entry contents is updated so that values for referenced symbols will be @@ -249,6 +249,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, section: Section which can be used to lookup symbol values base_sym: Base symbol marking the start of the image (__image_copy_start by default) + base_addr (int): Base address to use for the entry being written. If + None then the value of base_sym is used

Returns: int: Number of symbols written @@ -278,7 +280,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section, is_elf=False, if not base and not is_elf: tout.debug(f'LookupAndWriteSymbols: no base: elf_fname={elf_fname}, base_sym={base_sym}, is_elf={is_elf}') return 0 - base_addr = 0 if is_elf else base.address + if base_addr is None: + base_addr = 0 if is_elf else base.address count = 0 for name, sym in syms.items(): if name.startswith('_binman'): diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 7d4d4692776..8ad9fe89e0c 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -108,6 +108,9 @@ class Entry(object): not need to be done again. This is only used with 'binman replace', to stop sections from being rebuilt if their entries have not been replaced + symbols_base (int): Use this value as the assumed load address of the + target entry, when calculating the symbol value. If None, this is + 0 for blobs and the image-start address for ELF files """ fake_dir = None

@@ -159,6 +162,7 @@ class Entry(object): self.preserve = False self.build_done = False self.no_write_symbols = False + self.symbols_base = None

@staticmethod def FindEntryClass(etype, expanded): @@ -324,6 +328,7 @@ class Entry(object):

self.preserve = fdt_util.GetBool(self._node, 'preserve') self.no_write_symbols = fdt_util.GetBool(self._node, 'no-write-symbols') + self.symbols_base = fdt_util.GetInt(self._node, 'symbols-base')

def GetDefaultFilename(self): return None @@ -713,7 +718,8 @@ class Entry(object): # Check if we are writing symbols into an ELF file is_elf = self.GetDefaultFilename() == self.elf_fname elf.LookupAndWriteSymbols(self.elf_fname, self, section.GetImage(), - is_elf, self.elf_base_sym) + is_elf, self.elf_base_sym, + self.symbols_base)

def CheckEntries(self): """Check that the entry offsets are correct diff --git a/tools/binman/etype/blob_phase.py b/tools/binman/etype/blob_phase.py index 951d9934050..09bb89b3b78 100644 --- a/tools/binman/etype/blob_phase.py +++ b/tools/binman/etype/blob_phase.py @@ -57,3 +57,8 @@ class Entry_blob_phase(Entry_section): if self.no_write_symbols: for entry in self._entries.values(): entry.no_write_symbols = True + + # Propagate the symbols-base property + if self.symbols_base is not None: + for entry in self._entries.values(): + entry.symbols_base = self.symbols_base diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 9f329704115..a1a75266947 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1500,7 +1500,8 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_SPL_NODTB_DATA, data[:len(U_BOOT_SPL_NODTB_DATA)])

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

Args: @@ -1512,6 +1513,8 @@ class TestFunctional(unittest.TestCase): value: value of that arg use_expanded: True to use expanded entries where available, e.g. 'u-boot-expanded' instead of 'u-boot' + symbols_base (int): Value to expect for symbols-base in u-boot-spl, + None if none """ elf_fname = self.ElfTestFile('u_boot_binman_syms') syms = elf.GetSymbols(elf_fname, ['binman', 'image']) @@ -1526,10 +1529,19 @@ class TestFunctional(unittest.TestCase): # 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 - vals = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, + vals2 = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, u_boot_offset + len(U_BOOT_DATA), 0x10 + u_boot_offset, 0x04) + + # u-boot-spl has a symbols-base property, so take that into account if + # required. The caller must supply the value + vals = list(vals2) + if symbols_base is not None: + vals[3] = symbols_base + u_boot_offset + vals = tuple(vals) + sym_values = struct.pack('<LLQLL', *vals) + sym_values2 = struct.pack('<LLQLL', *vals2) if no_write_symbols: self.assertEqual( base_data + @@ -1552,12 +1564,12 @@ class TestFunctional(unittest.TestCase): ofs = blen + 1 + len(U_BOOT_DATA) self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs])

- self.assertEqual(sym_values, data[ofs:ofs + 24]) + self.assertEqual(sym_values2, data[ofs:ofs + 24]) self.assertEqual(base_data[24:], data[ofs + 24:])

# Just repeating the above asserts all at once, for clarity expected = (sym_values + base_data[24:] + - tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values + + tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values2 + base_data[24:]) self.assertEqual(expected, data)

@@ -7750,6 +7762,21 @@ fdt fdtmap Extract the devicetree blob from the fdtmap err = stderr.getvalue() self.assertRegex(err, "Image 'image'.*missing bintools.*: mkeficapsule")

+ def testSymbolsBase(self): + """Test handling of symbols-base""" + self.checkSymbols('336_symbols_base.dts', U_BOOT_SPL_DATA, 0x1c, + symbols_base=0) + + def testSymbolsBaseExpanded(self): + """Test handling of symbols-base with expanded entries""" + entry_args = { + 'spl-dtb': '1', + } + self.checkSymbols('337_symbols_base_expand.dts', U_BOOT_SPL_NODTB_DATA + + U_BOOT_SPL_DTB_DATA, 0x38, + entry_args=entry_args, use_expanded=True, + symbols_base=0) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/336_symbols_base.dts b/tools/binman/test/336_symbols_base.dts new file mode 100644 index 00000000000..e4dccd38c22 --- /dev/null +++ b/tools/binman/test/336_symbols_base.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 { + symbols-base = <0>; + }; + + u-boot { + offset = <0x1c>; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +}; diff --git a/tools/binman/test/337_symbols_base_expand.dts b/tools/binman/test/337_symbols_base_expand.dts new file mode 100644 index 00000000000..5a777ae63b8 --- /dev/null +++ b/tools/binman/test/337_symbols_base_expand.dts @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + symbols-base = <0>; + }; + + u-boot { + offset = <0x38>; + no-expanded; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +};

Move the check for this further out, so that base_addr is computed in Entry.WriteSymbols() rather than at lower levels.

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

tools/binman/entry.py | 11 +++++++++-- tools/binman/etype/section.py | 15 +++++---------- tools/binman/image.py | 10 ++++------ 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 8ad9fe89e0c..68f8d62bba9 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -711,15 +711,22 @@ class Entry(object): def WriteSymbols(self, section): """Write symbol values into binary files for access at run time

+ As a special case, if symbols_base is not specified and this is an + end-at-4gb image, a symbols_base of 0 is used + Args: section: Section containing the entry """ if self.auto_write_symbols and not self.no_write_symbols: # Check if we are writing symbols into an ELF file is_elf = self.GetDefaultFilename() == self.elf_fname + + symbols_base = self.symbols_base + if symbols_base is None and self.GetImage()._end_4gb: + symbols_base = 0 + elf.LookupAndWriteSymbols(self.elf_fname, self, section.GetImage(), - is_elf, self.elf_base_sym, - self.symbols_base) + is_elf, self.elf_base_sym, symbols_base)

def CheckEntries(self): """Check that the entry offsets are correct diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 6c181531b8b..7d2eb42627d 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -627,12 +627,10 @@ class Entry_section(Entry): optional: True if the symbol is optional. If False this function will raise if the symbol is not found msg: Message to display if an error occurs - base_addr: Base address of image. This is added to the returned - image_pos in most cases so that the returned position indicates - where the targetted entry/binary has actually been loaded. But - if end-at-4gb is used, this is not done, since the binary is - already assumed to be linked to the ROM position and using - execute-in-place (XIP). + base_addr (int): Base address of image. This is added to the + returned value of image-pos so that the returned position + indicates where the targeted entry/binary has actually been + loaded

Returns: Value that should be assigned to that symbol, or None if it was @@ -655,10 +653,7 @@ class Entry_section(Entry): if prop_name == 'offset': return entry.offset elif prop_name == 'image_pos': - value = entry.image_pos - if not self.GetImage()._end_4gb: - value += base_addr - return value + return base_addr + entry.image_pos if prop_name == 'size': return entry.size else: diff --git a/tools/binman/image.py b/tools/binman/image.py index c667f583db6..24ce0af7c72 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -404,12 +404,10 @@ class Image(section.Entry_section): optional: True if the symbol is optional. If False this function will raise if the symbol is not found msg: Message to display if an error occurs - base_addr: Base address of image. This is added to the returned - image_pos in most cases so that the returned position indicates - where the targeted entry/binary has actually been loaded. But - if end-at-4gb is used, this is not done, since the binary is - already assumed to be linked to the ROM position and using - execute-in-place (XIP). + base_addr (int): Base address of image. This is added to the + returned value of image-pos so that the returned position + indicates where the targeted entry/binary has actually been + loaded

Returns: Value that should be assigned to that symbol, or None if it was

Some images do not have an image_pos value, for example an image which is part of a compressed section and therefore cannot be accessed directly.

Handle this case, returning None as the value.

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

tools/binman/etype/section.py | 3 ++ tools/binman/ftest.py | 49 +++++++++++++++++++------- tools/binman/test/338_symbols_comp.dts | 26 ++++++++++++++ 3 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 tools/binman/test/338_symbols_comp.dts

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 7d2eb42627d..f4f48c00e87 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -653,6 +653,9 @@ class Entry_section(Entry): if prop_name == 'offset': return entry.offset elif prop_name == 'image_pos': + if not entry.image_pos: + tout.info(f'Symbol-writing: no value for {entry._node.path}') + return None return base_addr + entry.image_pos if prop_name == 'size': return entry.size diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index a1a75266947..6317e72e649 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -505,8 +505,9 @@ class TestFunctional(unittest.TestCase): return dtb.GetContents()

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

This runs binman with a given test file and then reads the resulting @@ -523,6 +524,7 @@ class TestFunctional(unittest.TestCase): But in some test we need the real contents. use_expanded: True to use expanded entries where available, e.g. 'u-boot-expanded' instead of 'u-boot' + verbosity: Verbosity level to use (0-3, None=don't set it) map: True to output map files for the images update_dtb: Update the offset and size of each entry in the device tree before packing it into the image @@ -559,7 +561,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, - use_expanded=use_expanded, extra_indirs=extra_indirs, + use_expanded=use_expanded, verbosity=verbosity, + extra_indirs=extra_indirs, threads=threads) self.assertEqual(0, retcode) out_dtb_fname = tools.get_output_filename('u-boot.dtb.out') @@ -1507,7 +1510,8 @@ class TestFunctional(unittest.TestCase): 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 + u_boot_offset (int): Offset of 'u-boot' section in image, or None if + the offset not available due to it being in a compressed section entry_args: Dict of entry args to supply to binman key: arg name value: value of that arg @@ -1525,13 +1529,20 @@ class TestFunctional(unittest.TestCase):

self._SetupSplElf('u_boot_binman_syms') data = self._DoReadFileDtb(dts, entry_args=entry_args, - use_expanded=use_expanded)[0] + use_expanded=use_expanded, + verbosity=None if u_boot_offset else 3)[0] + + # The lz4-compressed version of the U-Boot data is 19 bytes long + comp_uboot_len = 19 + # 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 + # If u_boot_offset is None, Binman should write -1U into the image vals2 = (elf.BINMAN_SYM_MAGIC_VALUE, 0x00, - u_boot_offset + len(U_BOOT_DATA), - 0x10 + u_boot_offset, 0x04) + u_boot_offset + len(U_BOOT_DATA) if u_boot_offset else + len(U_BOOT_SPL_DATA) + 1 + comp_uboot_len, + 0x10 + u_boot_offset if u_boot_offset else 0xffffffff, 0x04)

# u-boot-spl has a symbols-base property, so take that into account if # required. The caller must supply the value @@ -1561,17 +1572,21 @@ class TestFunctional(unittest.TestCase): self.assertEqual(base_data[24:], data[24:blen]) self.assertEqual(0xff, data[blen])

- ofs = blen + 1 + len(U_BOOT_DATA) - self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs]) + if u_boot_offset: + ofs = blen + 1 + len(U_BOOT_DATA) + self.assertEqual(U_BOOT_DATA, data[blen + 1:ofs]) + else: + ofs = blen + 1 + comp_uboot_len

self.assertEqual(sym_values2, data[ofs:ofs + 24]) self.assertEqual(base_data[24:], data[ofs + 24:])

# Just repeating the above asserts all at once, for clarity - expected = (sym_values + base_data[24:] + - tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values2 + - base_data[24:]) - self.assertEqual(expected, data) + if u_boot_offset: + expected = (sym_values + base_data[24:] + + tools.get_bytes(0xff, 1) + U_BOOT_DATA + + sym_values2 + base_data[24:]) + self.assertEqual(expected, data)

def testSymbols(self): """Test binman can assign symbols embedded in U-Boot""" @@ -7777,6 +7792,14 @@ fdt fdtmap Extract the devicetree blob from the fdtmap entry_args=entry_args, use_expanded=True, symbols_base=0)

+ def testSymbolsCompressed(self): + """Test binman complains about symbols from a compressed section""" + with test_util.capture_sys_output() as (stdout, stderr): + self.checkSymbols('338_symbols_comp.dts', U_BOOT_SPL_DATA, None) + out = stdout.getvalue() + self.assertIn('Symbol-writing: no value for /binman/section/u-boot', + out) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/338_symbols_comp.dts b/tools/binman/test/338_symbols_comp.dts new file mode 100644 index 00000000000..15008507cfd --- /dev/null +++ b/tools/binman/test/338_symbols_comp.dts @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0xff>; + u-boot-spl { + }; + + section { + offset = <0x1c>; + compress = "lz4"; + + u-boot { + }; + }; + + u-boot-spl2 { + type = "u-boot-spl"; + }; + }; +};

This patch is for Marek, to provide a starting point.

To try it, use 'binman test -T' and see the missing coverage.

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

tools/binman/etype/nxp_imx8mimage.py | 3 ++- tools/binman/ftest.py | 4 ++++ tools/binman/test/339_nxp_imx8.dts | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/339_nxp_imx8.dts

diff --git a/tools/binman/etype/nxp_imx8mimage.py b/tools/binman/etype/nxp_imx8mimage.py index 3585120b79b..8ad177b3b65 100644 --- a/tools/binman/etype/nxp_imx8mimage.py +++ b/tools/binman/etype/nxp_imx8mimage.py @@ -27,7 +27,8 @@ class Entry_nxp_imx8mimage(Entry_mkimage):

def __init__(self, section, etype, node): super().__init__(section, etype, node) - self.required_props = ['nxp,boot-from', 'nxp,rom-version', 'nxp,loader-address'] + self.required_props = ['nxp,boot-from', 'nxp,rom-version', + 'nxp,loader-address']

def ReadNode(self): super().ReadNode() diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 6317e72e649..225b5639b58 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7800,6 +7800,10 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertIn('Symbol-writing: no value for /binman/section/u-boot', out)

+ def testNxpImx8Image(self): + """Test that binman can produce an iMX8 image""" + self._DoTestFile('339_nxp_imx8.dts') +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/339_nxp_imx8.dts b/tools/binman/test/339_nxp_imx8.dts new file mode 100644 index 00000000000..cb512ae9aa2 --- /dev/null +++ b/tools/binman/test/339_nxp_imx8.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + nxp-imx8mimage { + args; /* TODO: Needed by mkimage etype superclass */ + nxp,boot-from = "sd"; + nxp,rom-version = <1>; + nxp,loader-address = <0x10>; + }; + }; +};