From c7967653daffa56caaa410937107271b0dc682a9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:43 -0600 Subject: [PATCH 01/37] dtoc: Avoid using subscripts on match objects These are not supported before Python 3.6 so avoid them. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- tools/dtoc/src_scan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/dtoc/src_scan.py b/tools/dtoc/src_scan.py index 2db96884c85..1dbb56712a3 100644 --- a/tools/dtoc/src_scan.py +++ b/tools/dtoc/src_scan.py @@ -555,7 +555,7 @@ class Scanner: if ids_m: ids_name = ids_m.group(1) elif m_alias: - self._driver_aliases[m_alias[2]] = m_alias[1] + self._driver_aliases[m_alias.group(2)] = m_alias.group(1) # Make the updates based on what we found for driver in drivers.values(): From 973fa52416d7853d1c146c8a7bc374a9a890c49c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:44 -0600 Subject: [PATCH 02/37] dtoc: Convert to use ArgumentParser Use this parser instead of OptionParser, which is deprecated. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- tools/dtoc/main.py | 51 ++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/tools/dtoc/main.py b/tools/dtoc/main.py index 93706de89bf..6f9b526bd74 100755 --- a/tools/dtoc/main.py +++ b/tools/dtoc/main.py @@ -21,7 +21,7 @@ options. For more information about the use of this options and tool please see doc/driver-model/of-plat.rst """ -from optparse import OptionParser +from argparse import ArgumentParser import os import sys import unittest @@ -51,7 +51,7 @@ def run_tests(processes, args): result = unittest.TestResult() sys.argv = [sys.argv[0]] - test_name = args and args[0] or None + test_name = args.files and args.files[0] or None test_dtoc.setup() @@ -66,47 +66,50 @@ def RunTestCoverage(): """Run the tests and check that we get 100% coverage""" sys.argv = [sys.argv[0]] test_util.RunTestCoverage('tools/dtoc/dtoc', '/main.py', - ['tools/patman/*.py', '*/fdt*', '*test*'], options.build_dir) + ['tools/patman/*.py', '*/fdt*', '*test*'], args.build_dir) if __name__ != '__main__': sys.exit(1) -parser = OptionParser() -parser.add_option('-B', '--build-dir', type='string', default='b', +epilog = '''Generate C code from devicetree files. See of-plat.rst for details''' + +parser = ArgumentParser(epilog=epilog) +parser.add_argument('-B', '--build-dir', type=str, default='b', help='Directory containing the build output') -parser.add_option('-c', '--c-output-dir', action='store', +parser.add_argument('-c', '--c-output-dir', action='store', help='Select output directory for C files') -parser.add_option('-C', '--h-output-dir', action='store', +parser.add_argument('-C', '--h-output-dir', action='store', help='Select output directory for H files (defaults to --c-output-di)') -parser.add_option('-d', '--dtb-file', action='store', +parser.add_argument('-d', '--dtb-file', action='store', help='Specify the .dtb input file') -parser.add_option('-i', '--instantiate', action='store_true', default=False, +parser.add_argument('-i', '--instantiate', action='store_true', default=False, help='Instantiate devices to avoid needing device_bind()') -parser.add_option('--include-disabled', action='store_true', +parser.add_argument('--include-disabled', action='store_true', help='Include disabled nodes') -parser.add_option('-o', '--output', action='store', +parser.add_argument('-o', '--output', action='store', help='Select output filename') -parser.add_option('-p', '--phase', type=str, +parser.add_argument('-p', '--phase', type=str, help='set phase of U-Boot this invocation is for (spl/tpl)') -parser.add_option('-P', '--processes', type=int, +parser.add_argument('-P', '--processes', type=int, help='set number of processes to use for running tests') -parser.add_option('-t', '--test', action='store_true', dest='test', +parser.add_argument('-t', '--test', action='store_true', dest='test', default=False, help='run tests') -parser.add_option('-T', '--test-coverage', action='store_true', - default=False, help='run tests and check for 100% coverage') -(options, args) = parser.parse_args() +parser.add_argument('-T', '--test-coverage', action='store_true', + default=False, help='run tests and check for 100%% coverage') +parser.add_argument('files', nargs='*') +args = parser.parse_args() # Run our meagre tests -if options.test: - ret_code = run_tests(options.processes, args) +if args.test: + ret_code = run_tests(args.processes, args) sys.exit(ret_code) -elif options.test_coverage: +elif args.test_coverage: RunTestCoverage() else: - dtb_platdata.run_steps(args, options.dtb_file, options.include_disabled, - options.output, - [options.c_output_dir, options.h_output_dir], - options.phase, instantiate=options.instantiate) + dtb_platdata.run_steps(args.files, args.dtb_file, args.include_disabled, + args.output, + [args.c_output_dir, args.h_output_dir], + args.phase, instantiate=args.instantiate) From 1b5fe11d957491a2f53a409b32b1a281c708e2fd Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:45 -0600 Subject: [PATCH 03/37] dtoc: Allow multiple warnings for a driver At present we show when a driver is missing but this is not always that useful. There are various reasons why a driver may appear to be missing, such as a parse error in the source code or a missing field in the driver declaration. Update the implementation to record all warnings for each driver, showing only those which relate to drivers that are actually used. This avoids spamming the user with warnings related to a driver for a different board. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- tools/dtoc/src_scan.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tools/dtoc/src_scan.py b/tools/dtoc/src_scan.py index 1dbb56712a3..6c37a71e978 100644 --- a/tools/dtoc/src_scan.py +++ b/tools/dtoc/src_scan.py @@ -13,6 +13,7 @@ U_BOOT_DRIVER(), UCLASS_DRIVER and all struct declarations in header files. See doc/driver-model/of-plat.rst for more informaiton """ +import collections import os import re import sys @@ -190,6 +191,9 @@ class Scanner: value: Driver name declared with U_BOOT_DRIVER(driver_name) _drivers_additional (list or str): List of additional drivers to use during scanning + _warnings: Dict of warnings found: + key: Driver name + value: Set of warnings _of_match: Dict holding information about compatible strings key: Name of struct udevice_id variable value: Dict of compatible info in that variable: @@ -217,6 +221,7 @@ class Scanner: self._driver_aliases = {} self._drivers_additional = drivers_additional or [] self._missing_drivers = set() + self._warnings = collections.defaultdict(set) self._of_match = {} self._compat_to_driver = {} self._uclass = {} @@ -267,7 +272,10 @@ class Scanner: aliases_c.remove(compat_c) return compat_c, aliases_c - self._missing_drivers.add(compat_list_c[0]) + name = compat_list_c[0] + self._missing_drivers.add(name) + self._warnings[name].add( + 'WARNING: the driver %s was not found in the driver list' % name) return compat_list_c[0], compat_list_c[1:] @@ -577,9 +585,17 @@ class Scanner: def show_warnings(self): """Show any warnings that have been collected""" - for name in sorted(list(self._missing_drivers)): - print('WARNING: the driver %s was not found in the driver list' - % name) + used_drivers = [drv.name for drv in self._drivers.values() if drv.used] + missing = self._missing_drivers + for name in sorted(self._warnings.keys()): + if name in missing or name in used_drivers: + warns = sorted(list(self._warnings[name])) + # For now there is only ever one warning + print('%s: %s' % (name, warns[0])) + indent = ' ' * len(name) + if name in missing: + missing.remove(name) + print() def scan_driver(self, fname): """Scan a driver file to build a list of driver names and aliases From 893142aa3bce28155905594bb054b0f434cafdfd Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:46 -0600 Subject: [PATCH 04/37] dtoc: Correct the re_compat regular expression This expects a . before the field name (.e.g '.compatible = ...) but presently accepts anything at all. Fix it. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- tools/dtoc/src_scan.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/dtoc/src_scan.py b/tools/dtoc/src_scan.py index 6c37a71e978..6e8e1ba51a0 100644 --- a/tools/dtoc/src_scan.py +++ b/tools/dtoc/src_scan.py @@ -452,8 +452,8 @@ class Scanner: # Collect the compatible string, e.g. 'rockchip,rk3288-grf' compat = None - re_compat = re.compile(r'{\s*.compatible\s*=\s*"(.*)"\s*' - r'(,\s*.data\s*=\s*(\S*))?\s*},') + re_compat = re.compile(r'{\s*\.compatible\s*=\s*"(.*)"\s*' + r'(,\s*\.data\s*=\s*(\S*))?\s*},') # This is a dict of compatible strings that were found: # key: Compatible string, e.g. 'rockchip,rk3288-grf' From 4f1727a7e31c192d294280b0379b522f57b54d31 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:47 -0600 Subject: [PATCH 05/37] dtoc: Add a stdout check in test_normalized_name() This test captures output but does not always check it. Add the missing code and drop the old comment. Signed-off-by: Simon Glass --- tools/dtoc/test_src_scan.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/dtoc/test_src_scan.py b/tools/dtoc/test_src_scan.py index d6da03849f9..4e38b25a2f8 100644 --- a/tools/dtoc/test_src_scan.py +++ b/tools/dtoc/test_src_scan.py @@ -171,8 +171,7 @@ class TestSrcScan(unittest.TestCase): self.assertEqual([], aliases) self.assertEqual(1, len(scan._missing_drivers)) self.assertEqual({'rockchip_rk3288_grf'}, scan._missing_drivers) - #'WARNING: the driver rockchip_rk3288_grf was not found in the driver list', - #stdout.getvalue().strip()) + self.assertEqual('', stdout.getvalue().strip()) i2c = 'I2C_UCLASS' compat = {'rockchip,rk3288-grf': 'ROCKCHIP_SYSCON_GRF', From 86ff01e890a72fb2b8cefab542d4b9123fe83036 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:48 -0600 Subject: [PATCH 06/37] dtoc: Detect unexpected suffix on .of_match Some rockchip drivers use a suffix on the of_match line which is not strictly valid. At present this causes the parsing to fail. Fix this and offer a warning. Signed-off-by: Simon Glass --- tools/dtoc/src_scan.py | 12 +++-- tools/dtoc/test_src_scan.py | 92 +++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/tools/dtoc/src_scan.py b/tools/dtoc/src_scan.py index 6e8e1ba51a0..847677757d9 100644 --- a/tools/dtoc/src_scan.py +++ b/tools/dtoc/src_scan.py @@ -468,7 +468,7 @@ class Scanner: # Matches the references to the udevice_id list re_of_match = re.compile( - r'\.of_match\s*=\s*(of_match_ptr\()?([a-z0-9_]+)(\))?,') + r'\.of_match\s*=\s*(of_match_ptr\()?([a-z0-9_]+)([^,]*),') re_phase = re.compile('^\s*DM_PHASE\((.*)\).*$') re_hdr = re.compile('^\s*DM_HEADER\((.*)\).*$') @@ -514,6 +514,11 @@ class Scanner: driver.uclass_id = m_id.group(1) elif m_of_match: compat = m_of_match.group(2) + suffix = m_of_match.group(3) + if suffix and suffix != ')': + self._warnings[driver.name].add( + "%s: Warning: unexpected suffix '%s' on .of_match line for compat '%s'" % + (fname, suffix, compat)) elif m_phase: driver.phase = m_phase.group(1) elif m_hdr: @@ -586,13 +591,14 @@ class Scanner: def show_warnings(self): """Show any warnings that have been collected""" used_drivers = [drv.name for drv in self._drivers.values() if drv.used] - missing = self._missing_drivers + missing = self._missing_drivers.copy() for name in sorted(self._warnings.keys()): if name in missing or name in used_drivers: warns = sorted(list(self._warnings[name])) - # For now there is only ever one warning print('%s: %s' % (name, warns[0])) indent = ' ' * len(name) + for warn in warns[1:]: + print('%-s: %s' % (indent, warn)) if name in missing: missing.remove(name) print() diff --git a/tools/dtoc/test_src_scan.py b/tools/dtoc/test_src_scan.py index 4e38b25a2f8..62500e80fe7 100644 --- a/tools/dtoc/test_src_scan.py +++ b/tools/dtoc/test_src_scan.py @@ -20,6 +20,9 @@ from patman import tools OUR_PATH = os.path.dirname(os.path.realpath(__file__)) +EXPECT_WARN = {'rockchip_rk3288_grf': + {'WARNING: the driver rockchip_rk3288_grf was not found in the driver list'}} + class FakeNode: """Fake Node object for testing""" def __init__(self): @@ -152,6 +155,7 @@ class TestSrcScan(unittest.TestCase): 'nvidia,tegra20-i2c-dvc': 'TYPE_DVC'}, drv.compat) self.assertEqual('i2c_bus', drv.priv) self.assertEqual(1, len(scan._drivers)) + self.assertEqual({}, scan._warnings) def test_normalized_name(self): """Test operation of get_normalized_compat_name()""" @@ -172,6 +176,7 @@ class TestSrcScan(unittest.TestCase): self.assertEqual(1, len(scan._missing_drivers)) self.assertEqual({'rockchip_rk3288_grf'}, scan._missing_drivers) self.assertEqual('', stdout.getvalue().strip()) + self.assertEqual(EXPECT_WARN, scan._warnings) i2c = 'I2C_UCLASS' compat = {'rockchip,rk3288-grf': 'ROCKCHIP_SYSCON_GRF', @@ -188,6 +193,7 @@ class TestSrcScan(unittest.TestCase): self.assertEqual('', stdout.getvalue().strip()) self.assertEqual('rockchip_rk3288_grf', name) self.assertEqual([], aliases) + self.assertEqual(EXPECT_WARN, scan._warnings) prop.value = 'rockchip,rk3288-srf' with test_util.capture_sys_output() as (stdout, _): @@ -195,6 +201,7 @@ class TestSrcScan(unittest.TestCase): self.assertEqual('', stdout.getvalue().strip()) self.assertEqual('rockchip_rk3288_grf', name) self.assertEqual(['rockchip_rk3288_srf'], aliases) + self.assertEqual(EXPECT_WARN, scan._warnings) def test_scan_errors(self): """Test detection of scanning errors""" @@ -489,3 +496,88 @@ U_BOOT_DRIVER(%s) = { self.assertEqual(3, seq) self.assertEqual({'mypath': 3}, uc.alias_path_to_num) self.assertEqual({2: node, 3: node}, uc.alias_num_to_node) + + def test_scan_warnings(self): + """Test detection of scanning warnings""" + buff = ''' +static const struct udevice_id tegra_i2c_ids[] = { + { .compatible = "nvidia,tegra114-i2c", .data = TYPE_114 }, + { } +}; + +U_BOOT_DRIVER(i2c_tegra) = { + .name = "i2c_tegra", + .id = UCLASS_I2C, + .of_match = tegra_i2c_ids + 1, +}; +''' + # The '+ 1' above should generate a warning + + prop = FakeProp() + prop.name = 'compatible' + prop.value = 'rockchip,rk3288-grf' + node = FakeNode() + node.props = {'compatible': prop} + + # get_normalized_compat_name() uses this to check for root node + node.parent = FakeNode() + + scan = src_scan.Scanner(None, None) + scan._parse_driver('file.c', buff) + self.assertEqual( + {'i2c_tegra': + {"file.c: Warning: unexpected suffix ' + 1' on .of_match line for compat 'tegra_i2c_ids'"}}, + scan._warnings) + + tprop = FakeProp() + tprop.name = 'compatible' + tprop.value = 'nvidia,tegra114-i2c' + tnode = FakeNode() + tnode.props = {'compatible': tprop} + + # get_normalized_compat_name() uses this to check for root node + tnode.parent = FakeNode() + + with test_util.capture_sys_output() as (stdout, _): + scan.get_normalized_compat_name(node) + scan.get_normalized_compat_name(tnode) + self.assertEqual('', stdout.getvalue().strip()) + + self.assertEqual(2, len(scan._missing_drivers)) + self.assertEqual({'rockchip_rk3288_grf', 'nvidia_tegra114_i2c'}, + scan._missing_drivers) + with test_util.capture_sys_output() as (stdout, _): + scan.show_warnings() + self.assertIn('rockchip_rk3288_grf', stdout.getvalue()) + + # This should show just the rockchip warning, since the tegra driver + # is not in self._missing_drivers + scan._missing_drivers.remove('nvidia_tegra114_i2c') + with test_util.capture_sys_output() as (stdout, _): + scan.show_warnings() + self.assertIn('rockchip_rk3288_grf', stdout.getvalue()) + self.assertNotIn('tegra_i2c_ids', stdout.getvalue()) + + # Do a similar thing with used drivers. By marking the tegra driver as + # used, the warning related to that driver will be shown + drv = scan._drivers['i2c_tegra'] + drv.used = True + with test_util.capture_sys_output() as (stdout, _): + scan.show_warnings() + self.assertIn('rockchip_rk3288_grf', stdout.getvalue()) + self.assertIn('tegra_i2c_ids', stdout.getvalue()) + + # Add a warning to make sure multiple warnings are shown + scan._warnings['i2c_tegra'].update( + scan._warnings['nvidia_tegra114_i2c']) + del scan._warnings['nvidia_tegra114_i2c'] + with test_util.capture_sys_output() as (stdout, _): + scan.show_warnings() + self.assertEqual('''i2c_tegra: WARNING: the driver nvidia_tegra114_i2c was not found in the driver list + : file.c: Warning: unexpected suffix ' + 1' on .of_match line for compat 'tegra_i2c_ids' + +rockchip_rk3288_grf: WARNING: the driver rockchip_rk3288_grf was not found in the driver list + +''', + stdout.getvalue()) + self.assertIn('tegra_i2c_ids', stdout.getvalue()) From 43ba4926702ca0c6d896b8d318b4c596979d2f32 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:49 -0600 Subject: [PATCH 07/37] dtoc: Detect drivers which do not parse correctly At present if a driver is missing a uclass or compatible stirng, this is silently ignored. This makes sense in most cases, particularly for the compatible string, since it is not required except when the driver is used with of-platdata. But it is also not very helpful. When there is some sort of problem with a driver, the missing compatible string (for example) may be the cause. Add a warning in this case, showing it only for drivers which are used by the build. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- tools/dtoc/src_scan.py | 7 ++++++- tools/dtoc/test_src_scan.py | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/tools/dtoc/src_scan.py b/tools/dtoc/src_scan.py index 847677757d9..3bef59d616e 100644 --- a/tools/dtoc/src_scan.py +++ b/tools/dtoc/src_scan.py @@ -546,7 +546,12 @@ class Scanner: # The driver does not have a uclass or compat string. # The first is required but the second is not, so just # ignore this. - pass + if not driver.uclass_id: + warn = 'Missing .uclass' + else: + warn = 'Missing .compatible' + self._warnings[driver.name].add('%s in %s' % + (warn, fname)) driver = None ids_name = None compat = None diff --git a/tools/dtoc/test_src_scan.py b/tools/dtoc/test_src_scan.py index 62500e80fe7..f03cf8ed7c7 100644 --- a/tools/dtoc/test_src_scan.py +++ b/tools/dtoc/test_src_scan.py @@ -581,3 +581,41 @@ rockchip_rk3288_grf: WARNING: the driver rockchip_rk3288_grf was not found in th ''', stdout.getvalue()) self.assertIn('tegra_i2c_ids', stdout.getvalue()) + + def scan_uclass_warning(self): + """Test a missing .uclass in the driver""" + buff = ''' +static const struct udevice_id tegra_i2c_ids[] = { + { .compatible = "nvidia,tegra114-i2c", .data = TYPE_114 }, + { } +}; + +U_BOOT_DRIVER(i2c_tegra) = { + .name = "i2c_tegra", + .of_match = tegra_i2c_ids, +}; +''' + scan = src_scan.Scanner(None, None) + scan._parse_driver('file.c', buff) + self.assertEqual( + {'i2c_tegra': {'Missing .uclass in file.c'}}, + scan._warnings) + + def scan_compat_warning(self): + """Test a missing .compatible in the driver""" + buff = ''' +static const struct udevice_id tegra_i2c_ids[] = { + { .compatible = "nvidia,tegra114-i2c", .data = TYPE_114 }, + { } +}; + +U_BOOT_DRIVER(i2c_tegra) = { + .name = "i2c_tegra", + .id = UCLASS_I2C, +}; +''' + scan = src_scan.Scanner(None, None) + scan._parse_driver('file.c', buff) + self.assertEqual( + {'i2c_tegra': {'Missing .compatible in file.c'}}, + scan._warnings) From e3dab846bd0acaca775792eace9e1a12caaec749 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 4 Jul 2021 12:19:50 -0600 Subject: [PATCH 08/37] dtoc: Update documentation to cover warnings in more detail When things go wrong it can be confusing to figure out what to change. Add a few more details to the documentation. Fix a 'make htmldocs' warning while we are here. Signed-off-by: Simon Glass Reviewed-by: Walter Lozano --- doc/develop/driver-model/of-plat.rst | 53 ++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/doc/develop/driver-model/of-plat.rst b/doc/develop/driver-model/of-plat.rst index 74f1932473b..8a8eaed4c11 100644 --- a/doc/develop/driver-model/of-plat.rst +++ b/doc/develop/driver-model/of-plat.rst @@ -597,6 +597,59 @@ as a macro included in the driver definition:: +Problems +-------- + +In some cases you will you see something like this:: + + WARNING: the driver rockchip_rk3188_grf was not found in the driver list + +The driver list is a list of drivers, each with a name. The name is in the +U_BOOT_DRIVER() declaration, repeated twice, one in brackets and once as the +.name member. For example, in the following declaration the driver name is +`rockchip_rk3188_grf`:: + + U_BOOT_DRIVER(rockchip_rk3188_grf) = { + .name = "rockchip_rk3188_grf", + .id = UCLASS_SYSCON, + .of_match = rk3188_syscon_ids + 1, + .bind = rk3188_syscon_bind_of_plat, + }; + +The first name U_BOOT_DRIVER(xx) is used to create a linker symbol so that the +driver can be accessed at build-time without any overhead. The second one +(.name = "xx") is used at runtime when something wants to print out the driver +name. + +The dtoc tool expects to be able to find a driver for each compatible string in +the devicetree. For example, if the devicetree has:: + + grf: grf@20008000 { + compatible = "rockchip,rk3188-grf", "syscon"; + reg = <0x20008000 0x200>; + u-boot,dm-spl; + }; + +then dtoc looks at the first compatible string ("rockchip,rk3188-grf"), +converts that to a C identifier (rockchip_rk3188_grf) and then looks for that. + +Various things can cause dtoc to fail to find the driver and it tries to +warn about these. For example: + + rockchip_rk3188_uart: Missing .compatible in drivers/serial/serial_rockchip.c + : WARNING: the driver rockchip_rk3188_uart was not found in the driver list + +Without a compatible string a driver cannot be used by dtoc, even if the +compatible string is not actually needed at runtime. + +If the problem is simply that there are multiple compatible strings, the +DM_DRIVER_ALIAS() macro can be used to tell dtoc about this and avoid a problem. + +Checks are also made to confirm that the referenced driver has a .compatible +member and a .id member. The first provides the array of compatible strings and +the second provides the uclass ID. + + Caveats ------- From 4bbaa88fad00205b6f2ae9ea0e57cfd878354a3a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:39 -0600 Subject: [PATCH 09/37] dm: core: Add logging for DM_SEQ_ALIAS It is sometimes helpful to see which sequence is assigned to a device. Add debugging info for that. Signed-off-by: Simon Glass --- drivers/core/device.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/core/device.c b/drivers/core/device.c index 9f1400768de..29668f6fb30 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -87,8 +87,10 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv, if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) { if (uc->uc_drv->name && ofnode_valid(node)) { - if (!dev_read_alias_seq(dev, &dev->seq_)) + if (!dev_read_alias_seq(dev, &dev->seq_)) { auto_seq = false; + log_debug(" - seq=%d\n", dev->seq_); + } } } } From 504fb6699778c77be80498400ef6206204f69a6b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:40 -0600 Subject: [PATCH 10/37] dm: Support lzma in the flashmap Allow lzma compression as well as lz4. Signed-off-by: Simon Glass --- drivers/core/of_extra.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/core/of_extra.c b/drivers/core/of_extra.c index 7702beff97b..632a1c2210e 100644 --- a/drivers/core/of_extra.c +++ b/drivers/core/of_extra.c @@ -31,6 +31,8 @@ int ofnode_read_fmap_entry(ofnode node, struct fmap_entry *entry) if (prop) { if (!strcmp(prop, "lz4")) entry->compress_algo = FMAP_COMPRESS_LZ4; + else if (!strcmp(prop, "lzma")) + entry->compress_algo = FMAP_COMPRESS_LZMA; else return log_msg_ret("compression algo", -EINVAL); } else { From f093f8649c794866f5793f132f93c95335aea817 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:41 -0600 Subject: [PATCH 11/37] test: Allow CONFIG_SPL_LOAD_FIT to be disabled At present if this is not enabled on a sandbox build, the build fails. Add a condition to avoid this. Signed-off-by: Simon Glass --- test/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/Makefile b/test/Makefile index a26e915e050..117839e5847 100644 --- a/test/Makefile +++ b/test/Makefile @@ -3,7 +3,9 @@ # (C) Copyright 2012 The Chromium Authors obj-y += test-main.o +ifdef CONFIG_SPL_LOAD_FIT obj-$(CONFIG_SANDBOX) += image/ +endif ifneq ($(CONFIG_$(SPL_)BLOBLIST),) obj-$(CONFIG_$(SPL_)CMDLINE) += bloblist.o From bf8188e4aa442a9fcd6b5332d177ec76a8faae65 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:42 -0600 Subject: [PATCH 12/37] test: Add DM_DMA to be disabled At present if DM_DMA is disabled on a sandbox build, the build fails. Make the test conditional. Signed-off-by: Simon Glass --- test/dm/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/dm/core.c b/test/dm/core.c index 2210345dd14..0492698997c 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -1171,6 +1171,7 @@ static int dm_test_all_have_seq(struct unit_test_state *uts) } DM_TEST(dm_test_all_have_seq, UT_TESTF_SCAN_PDATA); +#if CONFIG_IS_ENABLED(DM_DMA) static int dm_test_dma_offset(struct unit_test_state *uts) { struct udevice *dev; @@ -1200,3 +1201,4 @@ static int dm_test_dma_offset(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_dma_offset, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); +#endif From 719d286475d2df1f1d8f4413659893934f38de7d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:43 -0600 Subject: [PATCH 13/37] test: Avoid a build error with SPL At present this fails to build chromeos_sandbox due to a rebase error in dm_test_pre_run(). Fix it. Signed-off-by: Simon Glass --- test/test-main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-main.c b/test/test-main.c index 7afe8741cf9..3cdf6849c57 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -45,7 +45,7 @@ static int dm_test_pre_run(struct unit_test_state *uts) uts->force_fail_alloc = false; uts->skip_post_probe = false; gd->dm_root = NULL; - if (IS_ENABLED(CONFIG_UT_DM) && !CONFIG_IS_ENABLED(OF_PLATDATA)) + if (CONFIG_IS_ENABLED(UT_DM) && !CONFIG_IS_ENABLED(OF_PLATDATA)) memset(dm_testdrv_op_count, '\0', sizeof(dm_testdrv_op_count)); arch_reset_for_test(); From f178bebf551a43445ba8f1e1a4c1638eff8eb612 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:45 -0600 Subject: [PATCH 14/37] sandbox: Support executables for more phases The SPL header has a function for obtaining the phase in capital letters, e.g. 'SPL'. Add one for lower-case also, as used by sandbox. Use this to generalise the sandbox logic for determining the filename of the next sandbox executable. This can provide support for VPL. Signed-off-by: Simon Glass --- arch/sandbox/cpu/os.c | 63 +++++++++++++++------------------- arch/sandbox/cpu/spl.c | 16 ++++++++- arch/sandbox/include/asm/spl.h | 13 +++++++ include/os.h | 5 ++- include/spl.h | 21 ++++++++++++ test/image/spl_load.c | 6 +++- 6 files changed, 86 insertions(+), 38 deletions(-) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 0d21827e1b7..a8aa9def27f 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -783,12 +783,14 @@ int os_jump_to_image(const void *dest, int size) return os_jump_to_file(fname, true); } -int os_find_u_boot(char *fname, int maxlen, bool use_img) +int os_find_u_boot(char *fname, int maxlen, bool use_img, + const char *cur_prefix, const char *next_prefix) { struct sandbox_state *state = state_get_current(); const char *progname = state->argv[0]; int len = strlen(progname); - const char *suffix; + char subdir[10]; + char *suffix; char *p; int fd; @@ -798,45 +800,36 @@ int os_find_u_boot(char *fname, int maxlen, bool use_img) strcpy(fname, progname); suffix = fname + len - 4; - /* If we are TPL, boot to SPL */ - if (!strcmp(suffix, "-tpl")) { - fname[len - 3] = 's'; - fd = os_open(fname, O_RDONLY); - if (fd >= 0) { - close(fd); - return 0; - } + /* Change the existing suffix to the new one */ + if (*suffix != '-') + return -EINVAL; - /* Look for 'u-boot-spl' in the spl/ directory */ - p = strstr(fname, "/spl/"); - if (p) { - p[1] = 's'; - fd = os_open(fname, O_RDONLY); - if (fd >= 0) { - close(fd); - return 0; - } - } - return -ENOENT; + if (*next_prefix) + strcpy(suffix + 1, next_prefix); /* e.g. "-tpl" to "-spl" */ + else + *suffix = '\0'; /* e.g. "-spl" to "" */ + fd = os_open(fname, O_RDONLY); + if (fd >= 0) { + close(fd); + return 0; } - /* Look for 'u-boot' in the same directory as 'u-boot-spl' */ - if (!strcmp(suffix, "-spl")) { - fname[len - 4] = '\0'; - fd = os_open(fname, O_RDONLY); - if (fd >= 0) { - close(fd); - return 0; - } - } - - /* Look for 'u-boot' in the parent directory of spl/ */ - p = strstr(fname, "spl/"); + /* + * We didn't find it, so try looking for 'u-boot-xxx' in the xxx/ + * directory. Replace the old dirname with the new one. + */ + snprintf(subdir, sizeof(subdir), "/%s/", cur_prefix); + p = strstr(fname, subdir); if (p) { - /* Remove the "spl" characters */ - memmove(p, p + 4, strlen(p + 4) + 1); + if (*next_prefix) + /* e.g. ".../tpl/u-boot-spl" to "../spl/u-boot-spl" */ + memcpy(p + 1, next_prefix, strlen(next_prefix)); + else + /* e.g. ".../spl/u-boot" to ".../u-boot" */ + strcpy(p, p + 1 + strlen(cur_prefix)); if (use_img) strcat(p, ".img"); + fd = os_open(fname, O_RDONLY); if (fd >= 0) { close(fd); diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c index f82b0d3de16..650bdb0a701 100644 --- a/arch/sandbox/cpu/spl.c +++ b/arch/sandbox/cpu/spl.c @@ -17,6 +17,20 @@ DECLARE_GLOBAL_DATA_PTR; +int sandbox_find_next_phase(char *fname, int maxlen, bool use_img) +{ + const char *cur_prefix, *next_prefix; + int ret; + + cur_prefix = spl_phase_prefix(spl_phase()); + next_prefix = spl_phase_prefix(spl_next_phase()); + ret = os_find_u_boot(fname, maxlen, use_img, cur_prefix, next_prefix); + if (ret) + return log_msg_ret("find", ret); + + return 0; +} + /* SPL / TPL init function */ void board_init_f(ulong flag) { @@ -37,7 +51,7 @@ static int spl_board_load_image(struct spl_image_info *spl_image, char fname[256]; int ret; - ret = os_find_u_boot(fname, sizeof(fname), false); + ret = sandbox_find_next_phase(fname, sizeof(fname), false); if (ret) { printf("(%s not found, error %d)\n", fname, ret); return ret; diff --git a/arch/sandbox/include/asm/spl.h b/arch/sandbox/include/asm/spl.h index 51e9d95d557..d25dc7c82a0 100644 --- a/arch/sandbox/include/asm/spl.h +++ b/arch/sandbox/include/asm/spl.h @@ -12,4 +12,17 @@ enum { BOOT_DEVICE_BOARD, }; +/** + * sandbox_find_next_phase() - Find the next phase of U-Boot + * + * This function is intended to be called from within sandbox SPL. It uses + * a few rules to find the filename of the next U-Boot phase. See also + * os_find_u_boot(). + * + * @fname: place to put full path to U-Boot + * @maxlen: maximum size of @fname + * @use_img: select the 'u-boot.img' file instead of the 'u-boot' ELF file + */ +int sandbox_find_next_phase(char *fname, int maxlen, bool use_img); + #endif diff --git a/include/os.h b/include/os.h index bd1096eb8b3..7b20d606dd0 100644 --- a/include/os.h +++ b/include/os.h @@ -327,9 +327,12 @@ int os_jump_to_image(const void *dest, int size); * @fname: place to put full path to U-Boot * @maxlen: maximum size of @fname * @use_img: select the 'u-boot.img' file instead of the 'u-boot' ELF file + * @cur_prefix: prefix of current executable, e.g. "spl" or "tpl" + * @next_prefix: prefix of executable to find, e.g. "spl" or "" * Return: 0 if OK, -NOSPC if the filename is too large, -ENOENT if not found */ -int os_find_u_boot(char *fname, int maxlen, bool use_img); +int os_find_u_boot(char *fname, int maxlen, bool use_img, + const char *cur_prefix, const char *next_prefix); /** * os_spl_to_uboot() - Run U-Boot proper diff --git a/include/spl.h b/include/spl.h index c643943482d..d88fb79a676 100644 --- a/include/spl.h +++ b/include/spl.h @@ -176,6 +176,27 @@ static inline const char *spl_phase_name(enum u_boot_phase phase) } } +/** + * spl_phase_prefix() - Get the prefix of the current phase + * + * @phase: Phase to look up + * @return phase prefix ("spl", "tpl", etc.) + */ +static inline const char *spl_phase_prefix(enum u_boot_phase phase) +{ + switch (phase) { + case PHASE_TPL: + return "tpl"; + case PHASE_SPL: + return "spl"; + case PHASE_BOARD_F: + case PHASE_BOARD_R: + return ""; + default: + return "phase?"; + } +} + /* A string name for SPL or TPL */ #ifdef CONFIG_SPL_BUILD # ifdef CONFIG_TPL_BUILD diff --git a/test/image/spl_load.c b/test/image/spl_load.c index 851603ddd75..e7cabf5680c 100644 --- a/test/image/spl_load.c +++ b/test/image/spl_load.c @@ -56,6 +56,7 @@ struct image_header *spl_get_load_buffer(ssize_t offset, size_t size) static int spl_test_load(struct unit_test_state *uts) { + const char *cur_prefix, *next_prefix; struct spl_image_info image; struct image_header *header; struct text_ctx text_ctx; @@ -68,7 +69,10 @@ static int spl_test_load(struct unit_test_state *uts) load.bl_len = 512; load.read = read_fit_image; - ret = os_find_u_boot(fname, sizeof(fname), true); + cur_prefix = spl_phase_prefix(spl_phase()); + next_prefix = spl_phase_prefix(spl_next_phase()); + ret = os_find_u_boot(fname, sizeof(fname), true, cur_prefix, + next_prefix); if (ret) { printf("(%s not found, error %d)\n", fname, ret); return ret; From fcb7e31082c8abf77f5efac5920093f3bcb5d8ca Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:46 -0600 Subject: [PATCH 15/37] sandbox: Add work-around for SDL2 display At present the display does not show on some machines, e.g. Ubunutu 20.04 but the reason is unknown. Add a work-around until this can be determined. Also include more error checking just in case. Signed-off-by: Simon Glass --- arch/sandbox/cpu/sdl.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index 8102649be3a..e2649494818 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -164,8 +164,29 @@ int sandbox_sdl_init_display(int width, int height, int log2_bpp, int sandbox_sdl_sync(void *lcd_base) { + struct SDL_Rect rect; + int ret; + + if (!sdl.texture) + return 0; + SDL_RenderClear(sdl.renderer); SDL_UpdateTexture(sdl.texture, NULL, lcd_base, sdl.pitch); - SDL_RenderCopy(sdl.renderer, sdl.texture, NULL, NULL); + ret = SDL_RenderCopy(sdl.renderer, sdl.texture, NULL, NULL); + if (ret) { + printf("SDL copy %d: %s\n", ret, SDL_GetError()); + return -EIO; + } + + /* + * On some machines this does not appear. Draw an empty rectangle which + * seems to fix that. + */ + rect.x = 0; + rect.y = 0; + rect.w = 0; + rect.h = 0; + SDL_RenderDrawRect(sdl.renderer, &rect); + SDL_RenderPresent(sdl.renderer); sandbox_sdl_poll_events(); From 350b1d3b6830bf7383b7df051521526eb2c0d8c4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:47 -0600 Subject: [PATCH 16/37] sandbox: Use hinting with the display SDL provides a hinting feature which provides a higher-quality image with the double-display option (-K). Enable it. Signed-off-by: Simon Glass --- arch/sandbox/cpu/sdl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index e2649494818..bef5abd039d 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -123,6 +123,9 @@ int sandbox_sdl_init_display(int width, int height, int log2_bpp, sdl.vis_height = sdl.height; } + if (!SDL_SetHint(SDL_HINT_RENDER_SCALE_QUALITY, "1")) + printf("Unable to init hinting: %s", SDL_GetError()); + sdl.depth = 1 << log2_bpp; sdl.pitch = sdl.width * sdl.depth / 8; SDL_Window *screen = SDL_CreateWindow("U-Boot", SDL_WINDOWPOS_UNDEFINED, From ecc1ed912e4400b19783d4bd384914597c4a3679 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:48 -0600 Subject: [PATCH 17/37] sandbox: Adjust the bloblist default address Move this down to provide more space for the bloblist. Signed-off-by: Simon Glass --- common/Kconfig | 2 +- doc/arch/sandbox.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/Kconfig b/common/Kconfig index 26496f9a2e7..1f0c3663af0 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -724,7 +724,7 @@ config BLOBLIST_SIZE config BLOBLIST_ADDR hex "Address of bloblist" depends on BLOBLIST - default 0xe000 if SANDBOX + default 0xc000 if SANDBOX help Sets the address of the bloblist, set up by the first part of U-Boot which runs. Subsequent U-Boot stages typically use the same address. diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst index e052b6bdb09..9e23e1618c7 100644 --- a/doc/arch/sandbox.rst +++ b/doc/arch/sandbox.rst @@ -510,7 +510,7 @@ that are mapped into that memory: Addr Config Usage ======= ======================== =============================== 0 CONFIG_SYS_FDT_LOAD_ADDR Device tree - e000 CONFIG_BLOBLIST_ADDR Blob list + c000 CONFIG_BLOBLIST_ADDR Blob list 10000 CONFIG_MALLOC_F_ADDR Early memory allocation f0000 CONFIG_PRE_CON_BUF_ADDR Pre-console buffer 100000 CONFIG_TRACE_EARLY_ADDR Early trace buffer (if enabled). Also used From 201efb2bb0f7c4ae5f08f51b6f6b7cdfdba5b4f4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:49 -0600 Subject: [PATCH 18/37] cros_ec: Allow reading the battery-charge state Add a function to read this information from the EC. It is useful for determining whether the battery has enough charge to boot. Signed-off-by: Simon Glass --- drivers/misc/cros_ec.c | 17 +++++++++++++++++ include/cros_ec.h | 8 ++++++++ 2 files changed, 25 insertions(+) diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c index 7904d5cc72d..0818ef8ad6e 100644 --- a/drivers/misc/cros_ec.c +++ b/drivers/misc/cros_ec.c @@ -1661,6 +1661,23 @@ int cros_ec_get_switches(struct udevice *dev) return ret; } +int cros_ec_read_batt_charge(struct udevice *dev, uint *chargep) +{ + struct ec_params_charge_state req; + struct ec_response_charge_state resp; + int ret; + + req.cmd = CHARGE_STATE_CMD_GET_STATE; + ret = ec_command(dev, EC_CMD_CHARGE_STATE, 0, &req, sizeof(req), + &resp, sizeof(resp)); + if (ret) + return log_msg_ret("read", ret); + + *chargep = resp.get_state.batt_state_of_charge; + + return 0; +} + UCLASS_DRIVER(cros_ec) = { .id = UCLASS_CROS_EC, .name = "cros-ec", diff --git a/include/cros_ec.h b/include/cros_ec.h index 9396b4d2466..9dab6cdf9ba 100644 --- a/include/cros_ec.h +++ b/include/cros_ec.h @@ -652,4 +652,12 @@ int cros_ec_vstore_read(struct udevice *dev, int slot, uint8_t *data); int cros_ec_vstore_write(struct udevice *dev, int slot, const uint8_t *data, size_t size); +/** + * cros_ec_read_batt_charge() - Read the battery-charge state + * + * @dev: CROS-EC device + * @chargep: Return battery-charge state as a percentage + */ +int cros_ec_read_batt_charge(struct udevice *dev, uint *chargep); + #endif From 1e465eb6698e294e8fa9ed41165483b88062396a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:50 -0600 Subject: [PATCH 19/37] cros_ec: Drop cros_ec_entering_mode() This function is not needed anymore. Drop it. Signed-off-by: Simon Glass --- drivers/misc/cros_ec.c | 11 ----------- drivers/misc/cros_ec_sandbox.c | 3 --- include/cros_ec.h | 9 --------- 3 files changed, 23 deletions(-) diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c index 0818ef8ad6e..2a15094d20a 100644 --- a/drivers/misc/cros_ec.c +++ b/drivers/misc/cros_ec.c @@ -754,17 +754,6 @@ int cros_ec_flash_protect(struct udevice *dev, uint32_t set_mask, return 0; } -int cros_ec_entering_mode(struct udevice *dev, int mode) -{ - int rc; - - rc = ec_command(dev, EC_CMD_ENTERING_MODE, 0, &mode, sizeof(mode), - NULL, 0); - if (rc) - return -1; - return 0; -} - static int cros_ec_check_version(struct udevice *dev) { struct cros_ec_dev *cdev = dev_get_uclass_priv(dev); diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index beea47caa33..ab91f3fa67a 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -496,9 +496,6 @@ static int process_cmd(struct ec_state *ec, case EC_CMD_MKBP_STATE: len = cros_ec_keyscan(ec, resp_data); break; - case EC_CMD_ENTERING_MODE: - len = 0; - break; case EC_CMD_GET_NEXT_EVENT: { struct ec_response_get_next_event *resp = resp_data; diff --git a/include/cros_ec.h b/include/cros_ec.h index 9dab6cdf9ba..ef89deff762 100644 --- a/include/cros_ec.h +++ b/include/cros_ec.h @@ -198,15 +198,6 @@ int cros_ec_flash_protect(struct udevice *dev, uint32_t set_mask, uint32_t set_flags, struct ec_response_flash_protect *resp); -/** - * Notify EC of current boot mode - * - * @param dev CROS-EC device - * @param vboot_mode Verified boot mode - * @return 0 if ok, <0 on error - */ -int cros_ec_entering_mode(struct udevice *dev, int mode); - /** * Run internal tests on the cros_ec interface. * From 656d7447705550af0b464e7f6bc10fc1547e69ee Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:51 -0600 Subject: [PATCH 20/37] cros_ec: Support the full-size vboot context The v2 format is 64-bytes in size. Support this and drop v1 since it is not used anymore. Signed-off-by: Simon Glass --- drivers/misc/cros_ec_sandbox.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index ab91f3fa67a..12cc11efbd0 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -343,15 +343,13 @@ static int process_cmd(struct ec_state *ec, switch (req->op) { case EC_VBNV_CONTEXT_OP_READ: - /* TODO(sjg@chromium.org): Support full-size context */ memcpy(resp->block, ec->vbnv_context, - EC_VBNV_BLOCK_SIZE); - len = 16; + EC_VBNV_BLOCK_SIZE_V2); + len = EC_VBNV_BLOCK_SIZE_V2; break; case EC_VBNV_CONTEXT_OP_WRITE: - /* TODO(sjg@chromium.org): Support full-size context */ memcpy(ec->vbnv_context, req->block, - EC_VBNV_BLOCK_SIZE); + EC_VBNV_BLOCK_SIZE_V2); len = 0; break; default: From 56dae9ef3c56a7de6ed4af5efb82e661329d4738 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:52 -0600 Subject: [PATCH 21/37] cros_ec: Use standard calls for recovery-request checking Rather than calling directly into the sandbox SDL code, we can use the normal U-Boot console handling for this feature. Update the code, to make it more generic. Signed-off-by: Simon Glass --- drivers/misc/cros_ec_sandbox.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index 12cc11efbd0..2173517cff3 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -624,15 +624,19 @@ void cros_ec_check_keyboard(struct udevice *dev) struct ec_state *ec = dev_get_priv(dev); ulong start; - printf("Press keys for EC to detect on reset (ESC=recovery)..."); + printf("\nPress keys for EC to detect on reset (ESC=recovery)..."); start = get_timer(0); - while (get_timer(start) < 1000) - ; - putc('\n'); - if (!sandbox_sdl_key_pressed(KEY_ESC)) { - ec->recovery_req = true; - printf(" - EC requests recovery\n"); + while (get_timer(start) < 2000) { + if (tstc()) { + int ch = getchar(); + + if (ch == 0x1b) { + ec->recovery_req = true; + printf("EC requests recovery"); + } + } } + putc('\n'); } /* Return the byte of EC switch states */ From 1fe59375498fb84cd9ab72cf1f7f89437cd24cf4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:53 -0600 Subject: [PATCH 22/37] bloblist: Support resizing a blob Sometimes a blob needs to expand, e.g. because it needs to hold more log data. Add support for this. Note that the bloblist must have sufficient spare space for this to work. Signed-off-by: Simon Glass --- common/bloblist.c | 71 ++++++++++++++++- include/bloblist.h | 13 +++ test/bloblist.c | 192 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 274 insertions(+), 2 deletions(-) diff --git a/common/bloblist.c b/common/bloblist.c index eab63e9ca51..bb49ecab92e 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -57,13 +57,22 @@ static struct bloblist_rec *bloblist_first_blob(struct bloblist_hdr *hdr) return (struct bloblist_rec *)((void *)hdr + hdr->hdr_size); } -static struct bloblist_rec *bloblist_next_blob(struct bloblist_hdr *hdr, - struct bloblist_rec *rec) +static ulong bloblist_blob_end_ofs(struct bloblist_hdr *hdr, + struct bloblist_rec *rec) { ulong offset; offset = (void *)rec - (void *)hdr; offset += rec->hdr_size + ALIGN(rec->size, BLOBLIST_ALIGN); + + return offset; +} + +static struct bloblist_rec *bloblist_next_blob(struct bloblist_hdr *hdr, + struct bloblist_rec *rec) +{ + ulong offset = bloblist_blob_end_ofs(hdr, rec); + if (offset >= hdr->alloced) return NULL; return (struct bloblist_rec *)((void *)hdr + offset); @@ -215,6 +224,64 @@ int bloblist_ensure_size_ret(uint tag, int *sizep, void **blobp) return 0; } +static int bloblist_resize_rec(struct bloblist_hdr *hdr, + struct bloblist_rec *rec, + int new_size) +{ + int expand_by; /* Number of bytes to expand by (-ve to contract) */ + int new_alloced; /* New value for @hdr->alloced */ + ulong next_ofs; /* Offset of the record after @rec */ + + expand_by = ALIGN(new_size - rec->size, BLOBLIST_ALIGN); + new_alloced = ALIGN(hdr->alloced + expand_by, BLOBLIST_ALIGN); + if (new_size < 0) { + log(LOGC_BLOBLIST, LOGL_DEBUG, + "Attempt to shrink blob size below 0 (%x)\n", new_size); + return log_msg_ret("size", -EINVAL); + } + if (new_alloced > hdr->size) { + log(LOGC_BLOBLIST, LOGL_ERR, + "Failed to allocate %x bytes size=%x, need size=%x\n", + new_size, hdr->size, new_alloced); + return log_msg_ret("alloc", -ENOSPC); + } + + /* Move the following blobs up or down, if this is not the last */ + next_ofs = bloblist_blob_end_ofs(hdr, rec); + if (next_ofs != hdr->alloced) { + memmove((void *)hdr + next_ofs + expand_by, + (void *)hdr + next_ofs, new_alloced - next_ofs); + } + hdr->alloced = new_alloced; + + /* Zero the new part of the blob */ + if (expand_by > 0) { + memset((void *)rec + rec->hdr_size + rec->size, '\0', + new_size - rec->size); + } + + /* Update the size of this blob */ + rec->size = new_size; + + return 0; +} + +int bloblist_resize(uint tag, int new_size) +{ + struct bloblist_hdr *hdr = gd->bloblist; + struct bloblist_rec *rec; + int ret; + + rec = bloblist_findrec(tag); + if (!rec) + return log_msg_ret("find", -ENOENT); + ret = bloblist_resize_rec(hdr, rec, new_size); + if (ret) + return log_msg_ret("resize", ret); + + return 0; +} + static u32 bloblist_calc_chksum(struct bloblist_hdr *hdr) { struct bloblist_rec *rec; diff --git a/include/bloblist.h b/include/bloblist.h index 964b974fdaf..b659d2bc93d 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -179,6 +179,19 @@ void *bloblist_ensure(uint tag, int size); */ int bloblist_ensure_size_ret(uint tag, int *sizep, void **blobp); +/** + * bloblist_resize() - resize a blob + * + * Any blobs above this one are relocated up or down. The resized blob remains + * in the same place. + * + * @tag: Tag to add (enum bloblist_tag_t) + * @new_size: New size of the blob (>0 to expand, <0 to contract) + * @return 0 if OK, -ENOSPC if the bloblist does not have enough space, -ENOENT + * if the tag is not found + */ +int bloblist_resize(uint tag, int new_size); + /** * bloblist_new() - Create a new, empty bloblist of a given size * diff --git a/test/bloblist.c b/test/bloblist.c index d876b639184..345eb181fff 100644 --- a/test/bloblist.c +++ b/test/bloblist.c @@ -33,6 +33,9 @@ enum { ERASE_BYTE = '\xff', }; +static const char test1_str[] = "the eyes are open"; +static const char test2_str[] = "the mouth moves"; + static struct bloblist_hdr *clear_bloblist(void) { struct bloblist_hdr *hdr; @@ -384,6 +387,195 @@ static int bloblist_test_reloc(struct unit_test_state *uts) } BLOBLIST_TEST(bloblist_test_reloc, 0); +/* Test expansion of a blob */ +static int bloblist_test_grow(struct unit_test_state *uts) +{ + const uint small_size = 0x20; + void *blob1, *blob2, *blob1_new; + struct bloblist_hdr *hdr; + void *ptr; + + ptr = map_sysmem(TEST_ADDR, TEST_BLOBLIST_SIZE); + hdr = ptr; + memset(hdr, ERASE_BYTE, TEST_BLOBLIST_SIZE); + + /* Create two blobs */ + ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0)); + blob1 = bloblist_add(TEST_TAG, small_size, 0); + ut_assertnonnull(blob1); + ut_assertok(check_zero(blob1, small_size)); + strcpy(blob1, test1_str); + + blob2 = bloblist_add(TEST_TAG2, small_size, 0); + ut_assertnonnull(blob2); + strcpy(blob2, test2_str); + + ut_asserteq(sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2, + hdr->alloced); + + /* Resize the first one */ + ut_assertok(bloblist_resize(TEST_TAG, small_size + 4)); + + /* The first one should not have moved, just got larger */ + blob1_new = bloblist_find(TEST_TAG, small_size + 4); + ut_asserteq_ptr(blob1, blob1_new); + + /* The new space should be zeroed */ + ut_assertok(check_zero(blob1 + small_size, 4)); + + /* The second one should have moved */ + blob2 = bloblist_find(TEST_TAG2, small_size); + ut_assertnonnull(blob2); + ut_asserteq_str(test2_str, blob2); + + /* The header should have more bytes in use */ + hdr = ptr; + ut_asserteq(sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2 + + BLOBLIST_ALIGN, + hdr->alloced); + + return 0; +} +BLOBLIST_TEST(bloblist_test_grow, 0); + +/* Test shrinking of a blob */ +static int bloblist_test_shrink(struct unit_test_state *uts) +{ + const uint small_size = 0x20; + void *blob1, *blob2, *blob1_new; + struct bloblist_hdr *hdr; + int new_size; + void *ptr; + + ptr = map_sysmem(TEST_ADDR, TEST_BLOBLIST_SIZE); + + /* Create two blobs */ + ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0)); + blob1 = bloblist_add(TEST_TAG, small_size, 0); + ut_assertnonnull(blob1); + strcpy(blob1, test1_str); + + blob2 = bloblist_add(TEST_TAG2, small_size, 0); + ut_assertnonnull(blob2); + strcpy(blob2, test2_str); + + hdr = ptr; + ut_asserteq(sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2, + hdr->alloced); + + /* Resize the first one */ + new_size = small_size - BLOBLIST_ALIGN - 4; + ut_assertok(bloblist_resize(TEST_TAG, new_size)); + + /* The first one should not have moved, just got smaller */ + blob1_new = bloblist_find(TEST_TAG, new_size); + ut_asserteq_ptr(blob1, blob1_new); + + /* The second one should have moved */ + blob2 = bloblist_find(TEST_TAG2, small_size); + ut_assertnonnull(blob2); + ut_asserteq_str(test2_str, blob2); + + /* The header should have fewer bytes in use */ + hdr = ptr; + ut_asserteq(sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2 - + BLOBLIST_ALIGN, + hdr->alloced); + + return 0; +} +BLOBLIST_TEST(bloblist_test_shrink, 0); + +/* Test failing to adjust a blob size */ +static int bloblist_test_resize_fail(struct unit_test_state *uts) +{ + const uint small_size = 0x20; + struct bloblist_hdr *hdr; + void *blob1, *blob2; + int new_size; + void *ptr; + + ptr = map_sysmem(TEST_ADDR, TEST_BLOBLIST_SIZE); + + /* Create two blobs */ + ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0)); + blob1 = bloblist_add(TEST_TAG, small_size, 0); + ut_assertnonnull(blob1); + + blob2 = bloblist_add(TEST_TAG2, small_size, 0); + ut_assertnonnull(blob2); + + hdr = ptr; + ut_asserteq(sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2, + hdr->alloced); + + /* Resize the first one, to check the boundary conditions */ + ut_asserteq(-EINVAL, bloblist_resize(TEST_TAG, -1)); + + new_size = small_size + (hdr->size - hdr->alloced); + ut_asserteq(-ENOSPC, bloblist_resize(TEST_TAG, new_size + 1)); + ut_assertok(bloblist_resize(TEST_TAG, new_size)); + + return 0; +} +BLOBLIST_TEST(bloblist_test_resize_fail, 0); + +/* Test expanding the last blob in a bloblist */ +static int bloblist_test_resize_last(struct unit_test_state *uts) +{ + const uint small_size = 0x20; + struct bloblist_hdr *hdr; + void *blob1, *blob2, *blob2_new; + int alloced_val; + void *ptr; + + ptr = map_sysmem(TEST_ADDR, TEST_BLOBLIST_SIZE); + memset(ptr, ERASE_BYTE, TEST_BLOBLIST_SIZE); + hdr = ptr; + + /* Create two blobs */ + ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0)); + blob1 = bloblist_add(TEST_TAG, small_size, 0); + ut_assertnonnull(blob1); + + blob2 = bloblist_add(TEST_TAG2, small_size, 0); + ut_assertnonnull(blob2); + + /* Check the byte after the last blob */ + alloced_val = sizeof(struct bloblist_hdr) + + sizeof(struct bloblist_rec) * 2 + small_size * 2; + ut_asserteq(alloced_val, hdr->alloced); + ut_asserteq_ptr((void *)hdr + alloced_val, blob2 + small_size); + ut_asserteq((u8)ERASE_BYTE, *((u8 *)hdr + hdr->alloced)); + + /* Resize the second one, checking nothing changes */ + ut_asserteq(0, bloblist_resize(TEST_TAG2, small_size + 4)); + + blob2_new = bloblist_find(TEST_TAG2, small_size + 4); + ut_asserteq_ptr(blob2, blob2_new); + + /* + * the new blob should encompass the byte we checked now, so it should + * be zeroed. This zeroing should affect only the four new bytes added + * to the blob. + */ + ut_asserteq(0, *((u8 *)hdr + alloced_val)); + ut_asserteq((u8)ERASE_BYTE, *((u8 *)hdr + alloced_val + 4)); + + /* Check that the new top of the allocated blobs has not been touched */ + alloced_val += BLOBLIST_ALIGN; + ut_asserteq(alloced_val, hdr->alloced); + ut_asserteq((u8)ERASE_BYTE, *((u8 *)hdr + hdr->alloced)); + + return 0; +} +BLOBLIST_TEST(bloblist_test_resize_last, 0); + int do_ut_bloblist(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { From faff554292824037c4f098020ddbc8d979900415 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:54 -0600 Subject: [PATCH 23/37] bloblist: Tidy up a few API comments Some comments for struct bloblist_hdr are a bit ambiguous. Update them to clarify the meaning more precisely. Also document bloblist_get_stats() properly. Signed-off-by: Simon Glass --- include/bloblist.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/include/bloblist.h b/include/bloblist.h index b659d2bc93d..9f007c7a94d 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -64,10 +64,10 @@ enum bloblist_tag_t { * first bloblist_rec starts at this offset from the start of the header * @flags: Space for BLOBLISTF_... flags (none yet) * @magic: BLOBLIST_MAGIC - * @size: Total size of all records (non-zero if valid) including this header. + * @size: Total size of the bloblist (non-zero if valid) including this header. * The bloblist extends for this many bytes from the start of this header. - * @alloced: Total size allocated for this bloblist. When adding new records, - * the bloblist can grow up to this size. This starts out as + * When adding new records, the bloblist can grow up to this size. + * @alloced: Total size allocated so far for this bloblist. This starts out as * sizeof(bloblist_hdr) since we need at least that much space to store a * valid bloblist * @spare: Spare space (for future use) @@ -230,6 +230,10 @@ int bloblist_finish(void); * bloblist_get_stats() - Get information about the bloblist * * This returns useful information about the bloblist + * + * @basep: Returns base address of bloblist + * @sizep: Returns the number of bytes used in the bloblist + * @allocedp: Returns the total space allocated to the bloblist */ void bloblist_get_stats(ulong *basep, ulong *sizep, ulong *allocedp); From 1f618d528e234d2b7b0047750284dd531f5d718f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:55 -0600 Subject: [PATCH 24/37] bloblist: Correct condition in bloblist_addrec() It is possible to add a blob that ends at the end of the bloblist, but at present this is not supported. Fix it and add a regression test for this case. Signed-off-by: Simon Glass --- common/bloblist.c | 2 +- test/bloblist.c | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/common/bloblist.c b/common/bloblist.c index bb49ecab92e..1290fff8504 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -118,7 +118,7 @@ static int bloblist_addrec(uint tag, int size, int align, /* Calculate the new allocated total */ new_alloced = data_start + ALIGN(size, align); - if (new_alloced >= hdr->size) { + if (new_alloced > hdr->size) { log(LOGC_BLOBLIST, LOGL_ERR, "Failed to allocate %x bytes size=%x, need size=%x\n", size, hdr->size, new_alloced); diff --git a/test/bloblist.c b/test/bloblist.c index 345eb181fff..4104e6a92f6 100644 --- a/test/bloblist.c +++ b/test/bloblist.c @@ -576,6 +576,29 @@ static int bloblist_test_resize_last(struct unit_test_state *uts) } BLOBLIST_TEST(bloblist_test_resize_last, 0); +/* Check a completely full bloblist */ +static int bloblist_test_blob_maxsize(struct unit_test_state *uts) +{ + void *ptr; + int size; + + /* At the start there should be no records */ + clear_bloblist(); + ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0)); + + /* Add a blob that takes up all space */ + size = TEST_BLOBLIST_SIZE - sizeof(struct bloblist_hdr) - + sizeof(struct bloblist_rec); + ptr = bloblist_add(TEST_TAG, size, 0); + ut_assertnonnull(ptr); + + ptr = bloblist_add(TEST_TAG, size + 1, 0); + ut_assertnull(ptr); + + return 0; +} +BLOBLIST_TEST(bloblist_test_blob_maxsize, 0); + int do_ut_bloblist(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { From 1ac9c4cef521c5c5fd6591e83acf9728b167aaee Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:56 -0600 Subject: [PATCH 25/37] image: Allow @ in node names when not using signatures If signature verification is not in use we don't need to worry about the risk of using @ in node names. Update fit_image_verify() to allow it if the function is not enabled. Signed-off-by: Simon Glass --- common/image-fit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/image-fit.c b/common/image-fit.c index 8e23d51cf25..28bd8e78c75 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1377,7 +1377,7 @@ int fit_image_verify(const void *fit, int image_noffset) size_t size; char *err_msg = ""; - if (strchr(name, '@')) { + if (IS_ENABLED(CONFIG_FIT_SIGNATURE) && strchr(name, '@')) { /* * We don't support this since libfdt considers names with the * name root but different @ suffix to be equal From 7d84fbb57312ac0224dc67860f4d0306a3bb3f81 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:57 -0600 Subject: [PATCH 26/37] spl: Provide more information on boot failure If SPL fails to boot, try to provide an error code to indicate what is wrong. For example, if a uclass is missing, this can return -EPFNOSUPPORT (-96) which provides useful information. Add a helper for accessing the image-loader name so we can drop the use of #ifdefs in this code. Put this feature behind a CONFIG_SHOW_ERRORS option to avoid increasing the code size. Signed-off-by: Simon Glass --- common/spl/Kconfig | 10 ++++++++++ common/spl/spl.c | 46 +++++++++++++++++++++++++++++++--------------- include/spl.h | 10 ++++++++++ 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 2df3e5d8690..c0183521d20 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -91,6 +91,16 @@ config SPL_SYS_REPORT_STACK_F_USAGE occurrence of non 0xaa bytes. This default implementation works for stacks growing down only. +config SPL_SHOW_ERRORS + bool "Show more information when something goes wrong" + help + This enabled more verbose error messages and checking when something + goes wrong in SPL. For example, it shows the error code when U-Boot + cannot be located. This can help to diagnose the problem and figure + out a fix, particularly during development. + + This adds a small amount to SPL code size, perhaps 100 bytes. + menu "PowerPC and LayerScape SPL Boot options" config SPL_NAND_BOOT diff --git a/common/spl/spl.c b/common/spl/spl.c index eba77cace6d..3b96f2fc310 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -593,32 +593,42 @@ static int spl_load_image(struct spl_image_info *spl_image, * @spl_image: Place to put the image details if successful * @spl_boot_list: List of boot devices to try * @count: Number of elements in spl_boot_list - * @return 0 if OK, -ve on error + * @return 0 if OK, -ENODEV if there were no boot devices + * if CONFIG_SHOW_ERRORS is enabled, returns -ENXIO if there were + * devices but none worked */ static int boot_from_devices(struct spl_image_info *spl_image, u32 spl_boot_list[], int count) { + int ret = -ENODEV; int i; for (i = 0; i < count && spl_boot_list[i] != BOOT_DEVICE_NONE; i++) { struct spl_image_loader *loader; + int bootdev = spl_boot_list[i]; - loader = spl_ll_find_loader(spl_boot_list[i]); -#if defined(CONFIG_SPL_SERIAL_SUPPORT) \ - && defined(CONFIG_SPL_LIBCOMMON_SUPPORT) \ - && !defined(CONFIG_SILENT_CONSOLE) - if (loader) - printf("Trying to boot from %s\n", loader->name); - else - puts(SPL_TPL_PROMPT "Unsupported Boot Device!\n"); -#endif + if (CONFIG_IS_ENABLED(SHOW_ERRORS)) + ret = -ENXIO; + loader = spl_ll_find_loader(bootdev); + if (CONFIG_IS_ENABLED(SERIAL_SUPPORT) && + CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) && + !IS_ENABLED(CONFIG_SILENT_CONSOLE)) { + if (loader) + printf("Trying to boot from %s\n", + spl_loader_name(loader)); + else if (CONFIG_IS_ENABLED(SHOW_ERRORS)) + printf(SPL_TPL_PROMPT + "Unsupported Boot Device %d\n", bootdev); + else + puts(SPL_TPL_PROMPT "Unsupported Boot Device!\n"); + } if (loader && !spl_load_image(spl_image, loader)) { - spl_image->boot_device = spl_boot_list[i]; + spl_image->boot_device = bootdev; return 0; } } - return -ENODEV; + return ret; } #if defined(CONFIG_SPL_FRAMEWORK_BOARD_INIT_F) @@ -710,9 +720,15 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_image.boot_device = BOOT_DEVICE_NONE; board_boot_order(spl_boot_list); - if (boot_from_devices(&spl_image, spl_boot_list, - ARRAY_SIZE(spl_boot_list))) { - puts(SPL_TPL_PROMPT "failed to boot from all boot devices\n"); + ret = boot_from_devices(&spl_image, spl_boot_list, + ARRAY_SIZE(spl_boot_list)); + if (ret) { + if (CONFIG_IS_ENABLED(SHOW_ERRORS) && + CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)) + printf(SPL_TPL_PROMPT "failed to boot from all boot devices (err=%d)\n", + ret); + else + puts(SPL_TPL_PROMPT "failed to boot from all boot devices\n"); hang(); } diff --git a/include/spl.h b/include/spl.h index d88fb79a676..74a19394520 100644 --- a/include/spl.h +++ b/include/spl.h @@ -505,6 +505,16 @@ struct spl_image_loader { struct spl_boot_device *bootdev); }; +/* Helper function for accessing the name */ +static inline const char *spl_loader_name(const struct spl_image_loader *loader) +{ +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + return loader->name; +#else + return NULL; +#endif +} + /* Declare an SPL image loader */ #define SPL_LOAD_IMAGE(__name) \ ll_entry_declare(struct spl_image_loader, __name, spl_image_loader) From 6b165ab2b7b1dc89357afbe86c1977addf5aed3b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:58 -0600 Subject: [PATCH 27/37] sandbox: mmc: Support fixed MMC devices Add support for reading devicetree flags for MMC devices. With this we can distinguish between fixed and removable drives. Note that this information is only available when the device is probed, not when it is bound, since it is read in the of_to_plat() method. This could be changed if needed later. Signed-off-by: Simon Glass Reviewed-by: Jaehoon Chung --- arch/sandbox/dts/test.dts | 1 + drivers/mmc/sandbox_mmc.c | 24 +++++++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index d85bb46ceba..0cee15a0ea2 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -819,6 +819,7 @@ mmc2 { compatible = "sandbox,mmc"; + non-removable; }; mmc1 { diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c index 18ba020aacc..895fbffecfc 100644 --- a/drivers/mmc/sandbox_mmc.c +++ b/drivers/mmc/sandbox_mmc.c @@ -136,14 +136,31 @@ static const struct dm_mmc_ops sandbox_mmc_ops = { .get_cd = sandbox_mmc_get_cd, }; -int sandbox_mmc_probe(struct udevice *dev) +static int sandbox_mmc_of_to_plat(struct udevice *dev) +{ + struct sandbox_mmc_plat *plat = dev_get_plat(dev); + struct mmc_config *cfg = &plat->cfg; + struct blk_desc *blk; + int ret; + + ret = mmc_of_parse(dev, cfg); + if (ret) + return ret; + blk = mmc_get_blk_desc(&plat->mmc); + if (blk) + blk->removable = !(cfg->host_caps & MMC_CAP_NONREMOVABLE); + + return 0; +} + +static int sandbox_mmc_probe(struct udevice *dev) { struct sandbox_mmc_plat *plat = dev_get_plat(dev); return mmc_init(&plat->mmc); } -int sandbox_mmc_bind(struct udevice *dev) +static int sandbox_mmc_bind(struct udevice *dev) { struct sandbox_mmc_plat *plat = dev_get_plat(dev); struct mmc_config *cfg = &plat->cfg; @@ -158,7 +175,7 @@ int sandbox_mmc_bind(struct udevice *dev) return mmc_bind(dev, &plat->mmc, cfg); } -int sandbox_mmc_unbind(struct udevice *dev) +static int sandbox_mmc_unbind(struct udevice *dev) { mmc_unbind(dev); @@ -177,6 +194,7 @@ U_BOOT_DRIVER(mmc_sandbox) = { .ops = &sandbox_mmc_ops, .bind = sandbox_mmc_bind, .unbind = sandbox_mmc_unbind, + .of_to_plat = sandbox_mmc_of_to_plat, .probe = sandbox_mmc_probe, .priv_auto = sizeof(struct sandbox_mmc_priv), .plat_auto = sizeof(struct sandbox_mmc_plat), From 96f37b092fdbb604e43c313265995d72ff1a82fe Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:32:59 -0600 Subject: [PATCH 28/37] blk: Support iteration It is useful to be able to iterate over block devices. Typically there are fixed and removable devices. For security reasons it is sometimes useful to ignore removable devices since they are under user control. Add iterators which support selecting the block-device type. Signed-off-by: Simon Glass --- drivers/block/blk-uclass.c | 49 +++++++++++++++++++++++++++++++++ include/blk.h | 56 ++++++++++++++++++++++++++++++++++++++ test/dm/blk.c | 55 +++++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+) diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index dfc0d469702..83682dcc181 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -540,6 +540,55 @@ int blk_next_free_devnum(enum if_type if_type) return ret + 1; } +static int blk_flags_check(struct udevice *dev, enum blk_flag_t req_flags) +{ + const struct blk_desc *desc = dev_get_uclass_plat(dev); + enum blk_flag_t flags; + + flags = desc->removable ? BLKF_REMOVABLE : BLKF_FIXED; + + return flags & req_flags ? 0 : 1; +} + +int blk_first_device_err(enum blk_flag_t flags, struct udevice **devp) +{ + int ret; + + for (ret = uclass_first_device_err(UCLASS_BLK, devp); + !ret; + ret = uclass_next_device_err(devp)) { + if (!blk_flags_check(*devp, flags)) + return 0; + } + + return -ENODEV; +} + +int blk_next_device_err(enum blk_flag_t flags, struct udevice **devp) +{ + int ret; + + for (ret = uclass_next_device_err(devp); + !ret; + ret = uclass_next_device_err(devp)) { + if (!blk_flags_check(*devp, flags)) + return 0; + } + + return -ENODEV; +} + +int blk_count_devices(enum blk_flag_t flag) +{ + struct udevice *dev; + int count = 0; + + blk_foreach_probe(flag, dev) + count++; + + return count; +} + static int blk_claim_devnum(enum if_type if_type, int devnum) { struct udevice *dev; diff --git a/include/blk.h b/include/blk.h index c4401b00253..19bab081c2c 100644 --- a/include/blk.h +++ b/include/blk.h @@ -19,6 +19,8 @@ typedef ulong lbaint_t; #define LBAF "%" LBAFlength "x" #define LBAFU "%" LBAFlength "u" +struct udevice; + /* Interface types: */ enum if_type { IF_TYPE_UNKNOWN = 0, @@ -683,4 +685,58 @@ const char *blk_get_if_type_name(enum if_type if_type); int blk_common_cmd(int argc, char *const argv[], enum if_type if_type, int *cur_devnump); +enum blk_flag_t { + BLKF_FIXED = 1 << 0, + BLKF_REMOVABLE = 1 << 1, + BLKF_BOTH = BLKF_FIXED | BLKF_REMOVABLE, +}; + +/** + * blk_first_device_err() - Get the first block device + * + * The device returned is probed if necessary, and ready for use + * + * @flags: Indicates type of device to return + * @devp: Returns pointer to the first device in that uclass, or NULL if none + * @return 0 if found, -ENODEV if not found, other -ve on error + */ +int blk_first_device_err(enum blk_flag_t flags, struct udevice **devp); + +/** + * blk_next_device_err() - Get the next block device + * + * The device returned is probed if necessary, and ready for use + * + * @flags: Indicates type of device to return + * @devp: On entry, pointer to device to lookup. On exit, returns pointer + * to the next device in the uclass if no error occurred, or -ENODEV if + * there is no next device. + * @return 0 if found, -ENODEV if not found, other -ve on error + */ +int blk_next_device_err(enum blk_flag_t flags, struct udevice **devp); + +/** + * blk_foreach_probe() - Helper function to iteration through block devices + * + * This creates a for() loop which works through the available devices in + * a uclass in order from start to end. Devices are probed if necessary, + * and ready for use. + * + * @flags: Indicates type of device to return + * @dev: struct udevice * to hold the current device. Set to NULL when there + * are no more devices. + */ +#define blk_foreach_probe(flags, pos) \ + for (int _ret = blk_first_device_err(flags, &(pos)); \ + !_ret && pos; \ + _ret = blk_next_device_err(flags, &(pos))) + +/** + * blk_count_devices() - count the number of devices of a particular type + * + * @flags: Indicates type of device to find + * @return number of devices matching those flags + */ +int blk_count_devices(enum blk_flag_t flag); + #endif diff --git a/test/dm/blk.c b/test/dm/blk.c index b7f4304e9e9..deccf05289b 100644 --- a/test/dm/blk.c +++ b/test/dm/blk.c @@ -162,3 +162,58 @@ static int dm_test_blk_get_from_parent(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_blk_get_from_parent, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +/* Test iteration through block devices */ +static int dm_test_blk_iter(struct unit_test_state *uts) +{ + struct udevice *dev; + int i; + + /* + * See sandbox test.dts - it has: + * + * mmc0 - removable + * mmc1 - removable + * mmc2 - fixed + */ + ut_assertok(blk_first_device_err(BLKF_FIXED, &dev)); + ut_asserteq_str("mmc2.blk", dev->name); + ut_asserteq(-ENODEV, blk_next_device_err(BLKF_FIXED, &dev)); + + ut_assertok(blk_first_device_err(BLKF_REMOVABLE, &dev)); + ut_asserteq_str("mmc1.blk", dev->name); + ut_assertok(blk_next_device_err(BLKF_REMOVABLE, &dev)); + ut_asserteq_str("mmc0.blk", dev->name); + ut_asserteq(-ENODEV, blk_next_device_err(BLKF_REMOVABLE, &dev)); + + ut_assertok(blk_first_device_err(BLKF_BOTH, &dev)); + ut_asserteq_str("mmc2.blk", dev->name); + ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_asserteq_str("mmc1.blk", dev->name); + ut_assertok(blk_next_device_err(BLKF_BOTH, &dev)); + ut_asserteq_str("mmc0.blk", dev->name); + ut_asserteq(-ENODEV, blk_next_device_err(BLKF_FIXED, &dev)); + + ut_asserteq(1, blk_count_devices(BLKF_FIXED)); + ut_asserteq(2, blk_count_devices(BLKF_REMOVABLE)); + ut_asserteq(3, blk_count_devices(BLKF_BOTH)); + + i = 0; + blk_foreach_probe(BLKF_FIXED, dev) + ut_asserteq_str((i++, "mmc2.blk"), dev->name); + ut_asserteq(1, i); + + i = 0; + blk_foreach_probe(BLKF_REMOVABLE, dev) + ut_asserteq_str(i++ ? "mmc0.blk" : "mmc1.blk", dev->name); + ut_asserteq(2, i); + + i = 0; + blk_foreach_probe(BLKF_BOTH, dev) + ut_asserteq_str((++i == 1 ? "mmc2.blk" : i == 2 ? + "mmc1.blk" : "mmc0.blk"), dev->name); + ut_asserteq(3, i); + + return 0; +} +DM_TEST(dm_test_blk_iter, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); From 72fa1ad8d9fe58153ca72e3886b8d846299e8fff Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 5 Jul 2021 16:33:00 -0600 Subject: [PATCH 29/37] log: Allow padding of the function name At present when function names are logged, the output is a little hard to read since every function is a different length. Add a way to pad the names so that the log messages line up vertically. This doesn't work if the function name is very long, but it makes a big difference in most cases. Use 20 characters as a default since this covers the vast majority of functions. Signed-off-by: Simon Glass --- common/Kconfig | 8 ++++++++ common/log_console.c | 2 +- test/log/log_test.c | 16 +++++++++------- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/common/Kconfig b/common/Kconfig index 1f0c3663af0..2ab20a6c85b 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -322,6 +322,14 @@ config LOGF_FUNC Show the function name in log messages by default. This value can be overridden using the 'log format' command. +config LOGF_FUNC_PAD + int "Number of characters to use for function" + default 20 + help + Sets the field width to use when showing the function. Set this to + a larger value if you have lots of long function names, and want + things to line up. + config LOG_SYSLOG bool "Log output to syslog server" depends on NET diff --git a/common/log_console.c b/common/log_console.c index 3f6177499ef..f1dcc04b97c 100644 --- a/common/log_console.c +++ b/common/log_console.c @@ -38,7 +38,7 @@ static int log_console_emit(struct log_device *ldev, struct log_rec *rec) if (fmt & BIT(LOGF_LINE)) printf("%d-", rec->line); if (fmt & BIT(LOGF_FUNC)) - printf("%s()", rec->func); + printf("%*s()", CONFIG_LOGF_FUNC_PAD, rec->func); } if (fmt & BIT(LOGF_MSG)) printf("%s%s", add_space ? " " : "", rec->msg); diff --git a/test/log/log_test.c b/test/log/log_test.c index f1e67509c17..db7170f3042 100644 --- a/test/log/log_test.c +++ b/test/log/log_test.c @@ -62,9 +62,9 @@ static int do_check_log_entries(struct unit_test_state *uts, int flags, int min, for (i = min; i <= max; i++) { if (flags & EXPECT_LOG) - ut_assert_nextline("do_log_run() log %d", i); + ut_assert_nextline(" do_log_run() log %d", i); if (flags & EXPECT_DIRECT) - ut_assert_nextline("func() _log %d", i); + ut_assert_nextline(" func() _log %d", i); if (flags & EXPECT_DEBUG) { ut_assert_nextline("log %d", i); ut_assert_nextline("_log %d", i); @@ -72,11 +72,12 @@ static int do_check_log_entries(struct unit_test_state *uts, int flags, int min, } if (flags & EXPECT_EXTRA) for (; i <= LOGL_MAX ; i++) - ut_assert_nextline("func() _log %d", i); + ut_assert_nextline(" func() _log %d", i); for (i = LOGL_FIRST; i < LOGL_COUNT; i++) { if (flags & EXPECT_FORCE) - ut_assert_nextline("func() _log force %d", i); + ut_assert_nextline(" func() _log force %d", + i); if (flags & EXPECT_DEBUG) ut_assert_nextline("_log force %d", i); } @@ -277,7 +278,8 @@ int do_log_test_helpers(struct unit_test_state *uts) log_io("level %d\n", LOGL_DEBUG_IO); for (i = LOGL_EMERG; i <= _LOG_MAX_LEVEL; i++) - ut_assert_nextline("%s() level %d", __func__, i); + ut_assert_nextline("%*s() level %d", CONFIG_LOGF_FUNC_PAD, + __func__, i); ut_assert_console_end(); return 0; } @@ -297,14 +299,14 @@ int do_log_test_disable(struct unit_test_state *uts) { ut_assertok(console_record_reset_enable()); log_err("default\n"); - ut_assert_nextline("%s() default", __func__); + ut_assert_nextline("%*s() default", CONFIG_LOGF_FUNC_PAD, __func__); ut_assertok(log_device_set_enable(LOG_GET_DRIVER(console), false)); log_err("disabled\n"); ut_assertok(log_device_set_enable(LOG_GET_DRIVER(console), true)); log_err("enabled\n"); - ut_assert_nextline("%s() enabled", __func__); + ut_assert_nextline("%*s() enabled", CONFIG_LOGF_FUNC_PAD, __func__); ut_assert_console_end(); return 0; } From 650ead1a4aac3010a029526d639682096c1d0469 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:36 -0600 Subject: [PATCH 30/37] binman: Put compressed data into separate files At present compression uses the same temporary file for all invocations. With multithreading this causes the data to become corrupted. Use a different filename each time. Signed-off-by: Simon Glass --- tools/patman/tools.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/patman/tools.py b/tools/patman/tools.py index ec95a543bd9..877e37cd8da 100644 --- a/tools/patman/tools.py +++ b/tools/patman/tools.py @@ -466,6 +466,9 @@ def Compress(indata, algo, with_header=True): This requires 'lz4' and 'lzma_alone' tools. It also requires an output directory to be previously set up, by calling PrepareOutputDir(). + Care is taken to use unique temporary files so that this function can be + called from multiple threads. + Args: indata: Input data to compress algo: Algorithm to use ('none', 'gzip', 'lz4' or 'lzma') @@ -475,14 +478,16 @@ def Compress(indata, algo, with_header=True): """ if algo == 'none': return indata - fname = GetOutputFilename('%s.comp.tmp' % algo) + fname = tempfile.NamedTemporaryFile(prefix='%s.comp.tmp' % algo, + dir=outdir).name WriteFile(fname, indata) if algo == 'lz4': data = Run('lz4', '--no-frame-crc', '-B4', '-5', '-c', fname, binary=True) # cbfstool uses a very old version of lzma elif algo == 'lzma': - outfname = GetOutputFilename('%s.comp.otmp' % algo) + outfname = tempfile.NamedTemporaryFile(prefix='%s.comp.otmp' % algo, + dir=outdir).name Run('lzma_alone', 'e', fname, outfname, '-lc1', '-lp0', '-pb0', '-d8') data = ReadFile(outfname) elif algo == 'gzip': From c69d19c8f829d3320db5224f9f28d13cfb16049e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:37 -0600 Subject: [PATCH 31/37] binman: Support multithreading for building images Some images may take a while to build, e.g. if they are large and use slow compression. Support compiling sections in parallel to speed things up. Signed-off-by: Simon Glass (fixed to use a separate test file to fix flakiness) --- tools/binman/binman.rst | 18 ++++++++++++ tools/binman/cmdline.py | 4 +++ tools/binman/control.py | 4 +++ tools/binman/etype/section.py | 36 +++++++++++++++++++++-- tools/binman/ftest.py | 33 +++++++++++++++++++-- tools/binman/image.py | 3 ++ tools/binman/state.py | 23 +++++++++++++++ tools/binman/test/202_section_timeout.dts | 21 +++++++++++++ 8 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 tools/binman/test/202_section_timeout.dts diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index bc635aa00a5..09e7b571982 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1142,6 +1142,22 @@ adds a -v option to the call to binman:: make BINMAN_VERBOSE=5 +Building sections in parallel +----------------------------- + +By default binman uses multiprocessing to speed up compilation of large images. +This works at a section level, with one thread for each entry in the section. +This can speed things up if the entries are large and use compression. + +This feature can be disabled with the '-T' flag, which defaults to a suitable +value for your machine. This depends on the Python version, e.g on v3.8 it uses +12 threads on an 8-core machine. See ConcurrentFutures_ for more details. + +The special value -T0 selects single-threaded mode, useful for debugging during +development, since dealing with exceptions and problems in threads is more +difficult. This avoids any use of ThreadPoolExecutor. + + History / Credits ----------------- @@ -1190,3 +1206,5 @@ Some ideas: -- Simon Glass 7/7/2016 + +.. _ConcurrentFutures: https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.ThreadPoolExecutor diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index 95f9ba27fbd..d6156df408b 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -32,6 +32,10 @@ controlled by a description in the board device tree.''' default=False, help='Display the README file') parser.add_argument('--toolpath', type=str, action='append', help='Add a path to the directories containing tools') + parser.add_argument('-T', '--threads', type=int, + default=None, help='Number of threads to use (0=single-thread)') + parser.add_argument('--test-section-timeout', action='store_true', + help='Use a zero timeout for section multi-threading (for testing)') parser.add_argument('-v', '--verbosity', default=1, type=int, help='Control verbosity: 0=silent, 1=warnings, 2=notices, ' '3=info, 4=detail, 5=debug') diff --git a/tools/binman/control.py b/tools/binman/control.py index f57e34daaaa..b2113b6e64f 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -628,9 +628,13 @@ def Binman(args): tools.PrepareOutputDir(args.outdir, args.preserve) tools.SetToolPaths(args.toolpath) state.SetEntryArgs(args.entry_arg) + state.SetThreads(args.threads) images = PrepareImagesAndDtbs(dtb_fname, args.image, args.update_fdt, use_expanded) + if args.test_section_timeout: + # Set the first image to timeout, used in testThreadTimeout() + images[list(images.keys())[0]].test_section_timeout = True missing = False for image in images.values(): missing |= ProcessImage(image, args.update_fdt, args.map, diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c3bac026c14..92d3f3add4c 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -9,10 +9,12 @@ images to be created. """ from collections import OrderedDict +import concurrent.futures import re import sys from binman.entry import Entry +from binman import state from dtoc import fdt_util from patman import tools from patman import tout @@ -525,15 +527,43 @@ class Entry_section(Entry): def GetEntryContents(self): """Call ObtainContents() for each entry in the section """ + def _CheckDone(entry): + if not entry.ObtainContents(): + next_todo.append(entry) + return entry + todo = self._entries.values() for passnum in range(3): + threads = state.GetThreads() next_todo = [] - for entry in todo: - if not entry.ObtainContents(): - next_todo.append(entry) + + if threads == 0: + for entry in todo: + _CheckDone(entry) + else: + with concurrent.futures.ThreadPoolExecutor( + max_workers=threads) as executor: + future_to_data = { + entry: executor.submit(_CheckDone, entry) + for entry in todo} + timeout = 60 + if self.GetImage().test_section_timeout: + timeout = 0 + done, not_done = concurrent.futures.wait( + future_to_data.values(), timeout=timeout) + # Make sure we check the result, so any exceptions are + # generated. Check the results in entry order, since tests + # may expect earlier entries to fail first. + for entry in todo: + job = future_to_data[entry] + job.result() + if not_done: + self.Raise('Timed out obtaining contents') + todo = next_todo if not todo: break + if todo: self.Raise('Internal error: Could not complete processing of contents: remaining %s' % todo) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5383eec489a..531ea657718 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -308,7 +308,8 @@ class TestFunctional(unittest.TestCase): def _DoTestFile(self, fname, debug=False, map=False, update_dtb=False, entry_args=None, images=None, use_real_dtb=False, use_expanded=False, verbosity=None, allow_missing=False, - extra_indirs=None): + extra_indirs=None, threads=None, + test_section_timeout=False): """Run binman with a given test file Args: @@ -331,6 +332,8 @@ class TestFunctional(unittest.TestCase): 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 + threads: Number of threads to use (None for default, 0 for + single-threaded) """ args = [] if debug: @@ -342,6 +345,10 @@ class TestFunctional(unittest.TestCase): if self.toolpath: for path in self.toolpath: args += ['--toolpath', path] + if threads is not None: + args.append('-T%d' % threads) + if test_section_timeout: + args.append('--test-section-timeout') args += ['build', '-p', '-I', self._indir, '-d', self.TestFile(fname)] if map: args.append('-m') @@ -412,7 +419,7 @@ class TestFunctional(unittest.TestCase): 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): + 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 @@ -439,6 +446,8 @@ class TestFunctional(unittest.TestCase): 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 + threads: Number of threads to use (None for default, 0 for + single-threaded) Returns: Tuple: @@ -463,7 +472,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, extra_indirs=extra_indirs, + threads=threads) self.assertEqual(0, retcode) out_dtb_fname = tools.GetOutputFilename('u-boot.dtb.out') @@ -4542,5 +4552,22 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('201_opensbi.dts') self.assertEqual(OPENSBI_DATA, data[:len(OPENSBI_DATA)]) + def testSectionsSingleThread(self): + """Test sections without multithreading""" + data = self._DoReadFileDtb('055_sections.dts', threads=0)[0] + expected = (U_BOOT_DATA + tools.GetBytes(ord('!'), 12) + + U_BOOT_DATA + tools.GetBytes(ord('a'), 12) + + U_BOOT_DATA + tools.GetBytes(ord('&'), 4)) + self.assertEqual(expected, data) + + def testThreadTimeout(self): + """Test handling a thread that takes too long""" + with self.assertRaises(ValueError) as e: + self._DoTestFile('202_section_timeout.dts', + test_section_timeout=True) + self.assertIn("Node '/binman/section@0': Timed out obtaining contents", + str(e.exception)) + + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/image.py b/tools/binman/image.py index 10778f47fe9..cdc58b39a40 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -36,6 +36,8 @@ class Image(section.Entry_section): fdtmap_data: Contents of the fdtmap when loading from a file allow_repack: True to add properties to allow the image to be safely repacked later + test_section_timeout: Use a zero timeout for section multi-threading + (for testing) Args: copy_to_orig: Copy offset/size to orig_offset/orig_size after reading @@ -74,6 +76,7 @@ class Image(section.Entry_section): self.allow_repack = False self._ignore_missing = ignore_missing self.use_expanded = use_expanded + self.test_section_timeout = False if not test: self.ReadNode() diff --git a/tools/binman/state.py b/tools/binman/state.py index dfb17604554..2f567587382 100644 --- a/tools/binman/state.py +++ b/tools/binman/state.py @@ -7,6 +7,7 @@ import hashlib import re +import threading from dtoc import fdt import os @@ -55,6 +56,9 @@ allow_entry_expansion = True # to the new ones, the compressed size increases, etc. allow_entry_contraction = False +# Number of threads to use for binman (None means machine-dependent) +num_threads = None + def GetFdtForEtype(etype): """Get the Fdt object for a particular device-tree entry @@ -420,3 +424,22 @@ def AllowEntryContraction(): raised """ return allow_entry_contraction + +def SetThreads(threads): + """Set the number of threads to use when building sections + + Args: + threads: Number of threads to use (None for default, 0 for + single-threaded) + """ + global num_threads + + num_threads = threads + +def GetThreads(): + """Get the number of threads to use when building sections + + Returns: + Number of threads to use (None for default, 0 for single-threaded) + """ + return num_threads diff --git a/tools/binman/test/202_section_timeout.dts b/tools/binman/test/202_section_timeout.dts new file mode 100644 index 00000000000..1481450367a --- /dev/null +++ b/tools/binman/test/202_section_timeout.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0x26>; + size = <0x28>; + section@0 { + read-only; + size = <0x10>; + pad-byte = <0x21>; + + u-boot { + }; + }; + }; +}; From edd4b6ea41075b34619e774dba3d232b12c1ea53 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:38 -0600 Subject: [PATCH 32/37] binman: Split node-reading out from constructor in files The constructor should not read the node information. Move it to the ReadNode() method instead. This allows this etype to be subclassed. Signed-off-by: Simon Glass --- tools/binman/etype/files.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py index 5db36abef0b..9b04a496a85 100644 --- a/tools/binman/etype/files.py +++ b/tools/binman/etype/files.py @@ -34,6 +34,9 @@ class Entry_files(Entry_section): from binman import state super().__init__(section, etype, node) + + def ReadNode(self): + super().ReadNode() self._pattern = fdt_util.GetString(self._node, 'pattern') if not self._pattern: self.Raise("Missing 'pattern' property") From 43332d881baa2d66a18e80ec636e0e0da5623c46 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:39 -0600 Subject: [PATCH 33/37] binman: Use bytearray instead of string This is faster if data is being concatenated. Update the section and collection etypes. Signed-off-by: Simon Glass --- tools/binman/etype/collection.py | 2 +- tools/binman/etype/section.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/binman/etype/collection.py b/tools/binman/etype/collection.py index 1625575fe98..442b40b48b3 100644 --- a/tools/binman/etype/collection.py +++ b/tools/binman/etype/collection.py @@ -40,7 +40,7 @@ class Entry_collection(Entry): """ # Join up all the data self.Info('Getting contents, required=%s' % required) - data = b'' + data = bytearray() for entry_phandle in self.content: entry_data = self.section.GetContentsByPhandle(entry_phandle, self, required) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 92d3f3add4c..e2949fc9163 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -166,7 +166,7 @@ class Entry_section(Entry): pad_byte = (entry._pad_byte if isinstance(entry, Entry_section) else self._pad_byte) - data = b'' + data = bytearray() # Handle padding before the entry if entry.pad_before: data += tools.GetBytes(self._pad_byte, entry.pad_before) @@ -200,7 +200,7 @@ class Entry_section(Entry): Returns: Contents of the section (bytes) """ - section_data = b'' + section_data = bytearray() for entry in self._entries.values(): entry_data = entry.GetData(required) From c31d0cb68c1c29f210ab44803f5e5fdcdcfa250b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:40 -0600 Subject: [PATCH 34/37] patman: Use bytearray instead of string If the process outputs a lot of data on stdout this can be quite slow, since the bytestring is regenerated each time. Use a bytearray instead. Signed-off-by: Simon Glass --- tools/patman/cros_subprocess.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/patman/cros_subprocess.py b/tools/patman/cros_subprocess.py index efd0a5aaf72..fdd51386856 100644 --- a/tools/patman/cros_subprocess.py +++ b/tools/patman/cros_subprocess.py @@ -169,11 +169,11 @@ class Popen(subprocess.Popen): self.stdin.close() if self.stdout: read_set.append(self.stdout) - stdout = b'' + stdout = bytearray() if self.stderr and self.stderr != self.stdout: read_set.append(self.stderr) - stderr = b'' - combined = b'' + stderr = bytearray() + combined = bytearray() input_offset = 0 while read_set or write_set: From 03ebc20de3b30fca5230a4c73cf4494b0d8d8d08 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 6 Jul 2021 10:36:41 -0600 Subject: [PATCH 35/37] binman: Add basic support for debugging performance One of binman's attributes is that it is extremely fast, at least for a Python program. Add some simple timing around operations that might take a while, such as reading an image and compressing it. This should help to maintain the performance as new features are added. This is for debugging purposes only. Signed-off-by: Simon Glass --- tools/binman/control.py | 3 ++ tools/binman/etype/blob.py | 5 +++ tools/binman/ftest.py | 8 +++++ tools/binman/state.py | 72 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) diff --git a/tools/binman/control.py b/tools/binman/control.py index b2113b6e64f..dcba02ff7f8 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -646,6 +646,9 @@ def Binman(args): if missing: tout.Warning("\nSome images are invalid") + + # Use this to debug the time take to pack the image + #state.TimingShow() finally: tools.FinaliseOutputDir() finally: diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index 018f8c9a319..fae86ca3ec0 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -6,6 +6,7 @@ # from binman.entry import Entry +from binman import state from dtoc import fdt_util from patman import tools from patman import tout @@ -59,8 +60,12 @@ class Entry_blob(Entry): the data in chunks and avoid reading it all at once. For now this seems like an unnecessary complication. """ + state.TimingStart('read') indata = tools.ReadFile(self._pathname) + state.TimingAccum('read') + state.TimingStart('compress') data = self.CompressData(indata) + state.TimingAccum('compress') self.SetContents(data) return True diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 531ea657718..cea3ebf2b9f 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -4568,6 +4568,14 @@ class TestFunctional(unittest.TestCase): self.assertIn("Node '/binman/section@0': Timed out obtaining contents", str(e.exception)) + def testTiming(self): + """Test output of timing information""" + data = self._DoReadFile('055_sections.dts') + with test_util.capture_sys_output() as (stdout, stderr): + state.TimingShow() + self.assertIn('read:', stdout.getvalue()) + self.assertIn('compress:', stdout.getvalue()) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/state.py b/tools/binman/state.py index 2f567587382..9e5b8a39310 100644 --- a/tools/binman/state.py +++ b/tools/binman/state.py @@ -5,8 +5,10 @@ # Holds and modifies the state information held by binman # +from collections import defaultdict import hashlib import re +import time import threading from dtoc import fdt @@ -59,6 +61,27 @@ allow_entry_contraction = False # Number of threads to use for binman (None means machine-dependent) num_threads = None + +class Timing: + """Holds information about an operation that is being timed + + Properties: + name: Operation name (only one of each name is stored) + start: Start time of operation in seconds (None if not start) + accum:: Amount of time spent on this operation so far, in seconds + """ + def __init__(self, name): + self.name = name + self.start = None # cause an error if TimingStart() is not called + self.accum = 0.0 + + +# Holds timing info for each name: +# key: name of Timing info (Timing.name) +# value: Timing object +timing_info = {} + + def GetFdtForEtype(etype): """Get the Fdt object for a particular device-tree entry @@ -443,3 +466,52 @@ def GetThreads(): Number of threads to use (None for default, 0 for single-threaded) """ return num_threads + +def GetTiming(name): + """Get the timing info for a particular operation + + The object is created if it does not already exist. + + Args: + name: Operation name to get + + Returns: + Timing object for the current thread + """ + threaded_name = '%s:%d' % (name, threading.get_ident()) + timing = timing_info.get(threaded_name) + if not timing: + timing = Timing(threaded_name) + timing_info[threaded_name] = timing + return timing + +def TimingStart(name): + """Start the timer for an operation + + Args: + name: Operation name to start + """ + timing = GetTiming(name) + timing.start = time.monotonic() + +def TimingAccum(name): + """Stop and accumlate the time for an operation + + This measures the time since the last TimingStart() and adds that to the + accumulated time. + + Args: + name: Operation name to start + """ + timing = GetTiming(name) + timing.accum += time.monotonic() - timing.start + +def TimingShow(): + """Show all timing information""" + duration = defaultdict(float) + for threaded_name, timing in timing_info.items(): + name = threaded_name.split(':')[0] + duration[name] += timing.accum + + for name, seconds in duration.items(): + print('%10s: %10.1fms' % (name, seconds * 1000)) From fd25ca3275946476d5c3fa32e3e7e3087fa5c572 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 5 Jul 2021 19:43:00 +0200 Subject: [PATCH 36/37] sandbox: don't set SA_NODEFER in signal handler The sandbox can handle signals. Due to a damaged global data pointer additional exceptions in the signal handler may occur leading to an endless loop. In this case leave the handling of the secondary exception to the operating system. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/sandbox/cpu/os.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index a8aa9def27f..1103530941b 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -226,7 +226,7 @@ int os_setup_signal_handlers(void) act.sa_sigaction = os_signal_handler; sigemptyset(&act.sa_mask); - act.sa_flags = SA_SIGINFO | SA_NODEFER; + act.sa_flags = SA_SIGINFO; if (sigaction(SIGILL, &act, NULL) || sigaction(SIGBUS, &act, NULL) || sigaction(SIGSEGV, &act, NULL)) From 1b098b3e655451572054ce933a87231ee16f7133 Mon Sep 17 00:00:00 2001 From: Chen Guanqiao Date: Mon, 12 Jul 2021 15:40:20 +0800 Subject: [PATCH 37/37] dm: core: fix no null pointer detection in ofnode_get_addr_size_index() Fixed a defect of a null pointer being discovered by Coverity Scan: CID 331544: Null pointer dereferences (REVERSE_INULL) Null-checking "size" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. Signed-off-by: Chen Guanqiao --- drivers/core/ofnode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index eeeccfb4467..dda6c76e834 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -329,7 +329,8 @@ static fdt_addr_t __ofnode_get_addr_size_index(ofnode node, int index, { int na, ns; - *size = FDT_SIZE_T_NONE; + if (size) + *size = FDT_SIZE_T_NONE; if (ofnode_is_np(node)) { const __be32 *prop_val; @@ -340,6 +341,7 @@ static fdt_addr_t __ofnode_get_addr_size_index(ofnode node, int index, &flags); if (!prop_val) return FDT_ADDR_T_NONE; + if (size) *size = size64; @@ -359,8 +361,6 @@ static fdt_addr_t __ofnode_get_addr_size_index(ofnode node, int index, index, na, ns, size, translate); } - - return FDT_ADDR_T_NONE; } fdt_addr_t ofnode_get_addr_size_index(ofnode node, int index, fdt_size_t *size)