[PATCH 0/8] dtoc: Improvements to warnings
The warning about a missing driver can be a bit confusing and is not specifically mentioned in the documentation. Also some parse errors result in the driver simply being ignored, which is hard to diagnose.
This series updates dtoc to try harder to find common errors and report a useful warning. It also adds a section about possible problems to the of-platdata documentation.
Unfortunately the use of match-object subscripts is not supported in Python 3.5 and earlier, so a patch is included to avoid this.
Finally, a short usage string it added to the help output, with dtoc being converted to use ArgumentParser in the process.
Simon Glass (8): dtoc: Avoid using subscripts on match objects dtoc: Convert to use ArgumentParser dtoc: Allow multiple warnings for a driver dtoc: Correct the re_compat regular expression dtoc: Add a stdout check in test_normalized_name() dtoc: Detect unexpected suffix on .of_match dtoc: Detect drivers which do not parse correctly dtoc: Update documentation to cover warnings in more detail
doc/develop/driver-model/of-plat.rst | 53 +++++++++++ tools/dtoc/main.py | 51 +++++----- tools/dtoc/src_scan.py | 45 +++++++-- tools/dtoc/test_src_scan.py | 133 ++++++++++++++++++++++++++- 4 files changed, 247 insertions(+), 35 deletions(-)
These are not supported before Python 3.6 so avoid them.
Signed-off-by: Simon Glass sjg@chromium.org ---
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():
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
These are not supported before Python 3.6 so avoid them.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/src_scan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
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():
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
These are not supported before Python 3.6 so avoid them.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/src_scan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
Applied to u-boot-dm, thanks!
Use this parser instead of OptionParser, which is deprecated.
Signed-off-by: Simon Glass sjg@chromium.org ---
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)
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
Use this parser instead of OptionParser, which is deprecated.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/main.py | 51 ++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
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)
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
Use this parser instead of OptionParser, which is deprecated.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/main.py | 51 ++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
Applied to u-boot-dm, thanks!
At present we show when a driver is missing but this is not always that useful. There are various reasons way 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 sjg@chromium.org ---
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
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
At present we show when a driver is missing but this is not always that useful. There are various reasons way a driver may appear to be missing,
Did you mean "why" instead of "way"?
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 sjg@chromium.org
tools/dtoc/src_scan.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thank you, it is indeed something worth to be added!
Walter
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 namevalue: 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_driversfor 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 warningprint('%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
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
At present we show when a driver is missing but this is not always that useful. There are various reasons way a driver may appear to be missing,
Did you mean "why" instead of "way"?
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 sjg@chromium.org
tools/dtoc/src_scan.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thank you, it is indeed something worth to be added!
Walter
Applied to u-boot-dm, thanks!
On Sun, 11 Jul 2021 at 17:00, Simon Glass sjg@chromium.org wrote:
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
At present we show when a driver is missing but this is not always that useful. There are various reasons way a driver may appear to be missing,
Did you mean "why" instead of "way"?
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 sjg@chromium.org
tools/dtoc/src_scan.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thank you, it is indeed something worth to be added!
Walter
Applied to u-boot-dm, thanks!
(Note: I fixed the nits in this patch and other other one when applying...thanks for the review!)
This expects a . before the field name (.e.g '.compatible = ...) but presently accepts anything at all. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
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'
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
This expects a . before the field name (.e.g '.compatible = ...) but presently accepts anything at all. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/src_scan.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
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'
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
This expects a . before the field name (.e.g '.compatible = ...) but presently accepts anything at all. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
tools/dtoc/src_scan.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
Applied to u-boot-dm, thanks!
This test captures output but does not always check it. Add the missing code and drop the old comment.
Signed-off-by: Simon Glass sjg@chromium.org ---
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',
This test captures output but does not always check it. Add the missing code and drop the old comment.
Signed-off-by: Simon Glass sjg@chromium.org ---
tools/dtoc/test_src_scan.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Applied to u-boot-dm, thanks!
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 sjg@chromium.org ---
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())
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 sjg@chromium.org ---
tools/dtoc/src_scan.py | 12 +++-- tools/dtoc/test_src_scan.py | 92 +++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-)
Applied to u-boot-dm, thanks!
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 sjg@chromium.org ---
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)
On 7/4/21 3:19 PM, Simon Glass wrote:
At present if a driver is missing a uclass or compatible stirng, this
Most probably it should be "string"
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 sjg@chromium.org
tools/dtoc/src_scan.py | 7 ++++++- tools/dtoc/test_src_scan.py | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
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 = Nonediff --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)
On 7/4/21 3:19 PM, Simon Glass wrote:
At present if a driver is missing a uclass or compatible stirng, this
Most probably it should be "string"
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 sjg@chromium.org
tools/dtoc/src_scan.py | 7 ++++++- tools/dtoc/test_src_scan.py | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks!
Walter
Applied to u-boot-dm, thanks!
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 sjg@chromium.org ---
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..e2763965839 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 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 -------
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
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 sjg@chromium.org
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..e2763965839 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 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.
This paragraph sound strange, maybe
"Checks are are also made to confirm that the referenced driver..."
but your English is better than mine.
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Regards,
Walter
Caveats
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
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 sjg@chromium.org
doc/develop/driver-model/of-plat.rst | 53 ++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Applied to u-boot-dm, thanks!
Hi Walter,
On Mon, 5 Jul 2021 at 12:04, Walter Lozano wlozano@collabora.com wrote:
Hi Simon,
On 7/4/21 3:19 PM, Simon Glass wrote:
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 sjg@chromium.org
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..e2763965839 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 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.
This paragraph sound strange, maybe
"Checks are are also made to confirm that the referenced driver..."
but your English is better than mine.
Reviewed-by: Walter Lozano walter.lozano@collabora.com
Thanks, I fixed this when applying.
- Simon
participants (2)
-
Simon Glass -
Walter Lozano