[PATCH 0/8] dtoc: Improvements to warnings

[PATCH 0/8] dtoc: Improvements to warnings

[Simon Glass]

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(-)

-- 
2.32.0.93.g670b81a890-goog

[PATCH 1/8] dtoc: Avoid using subscripts on match objects

[Simon Glass]

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():
-- 
2.32.0.93.g670b81a890-goog

[PATCH 2/8] dtoc: Convert to use ArgumentParser

[Simon Glass]

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)
-- 
2.32.0.93.g670b81a890-goog

[PATCH 3/8] dtoc: Allow multiple warnings for a driver

[Simon Glass]

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
-- 
2.32.0.93.g670b81a890-goog

[PATCH 4/8] dtoc: Correct the re_compat regular expression

[Simon Glass]

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'
-- 
2.32.0.93.g670b81a890-goog

[PATCH 5/8] dtoc: Add a stdout check in test_normalized_name()

[Simon Glass]

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',
-- 
2.32.0.93.g670b81a890-goog

[PATCH 6/8] dtoc: Detect unexpected suffix on .of_match

[Simon Glass]

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())
-- 
2.32.0.93.g670b81a890-goog

[PATCH 7/8] dtoc: Detect drivers which do not parse correctly

[Simon Glass]

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)
-- 
2.32.0.93.g670b81a890-goog

[PATCH 8/8] dtoc: Update documentation to cover warnings in more detail

[Simon Glass]

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
 -------
 
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH 1/8] dtoc: Avoid using subscripts on match objects

[Walter Lozano]

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():

Re: [PATCH 2/8] dtoc: Convert to use ArgumentParser

[Walter Lozano]

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)

Re: [PATCH 3/8] dtoc: Allow multiple warnings for a driver

[Walter Lozano]

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 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

Re: [PATCH 4/8] dtoc: Correct the re_compat regular expression

[Walter Lozano]

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'

Re: [PATCH 7/8] dtoc: Detect drivers which do not parse correctly

[Walter Lozano]

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 = 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)

Re: [PATCH 8/8] dtoc: Update documentation to cover warnings in more detail

[Walter Lozano]

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
>   -------
>   

Re: [PATCH 8/8] dtoc: Update documentation to cover warnings in more detail

[Simon Glass]

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!

Re: [PATCH 7/8] dtoc: Detect drivers which do not parse correctly

[Simon Glass]

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!

Re: [PATCH 6/8] dtoc: Detect unexpected suffix on .of_match

[Simon Glass]

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!

Re: [PATCH 5/8] dtoc: Add a stdout check in test_normalized_name()

[Simon Glass]

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!

Re: [PATCH 4/8] dtoc: Correct the re_compat regular expression

[Simon Glass]

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!

Re: [PATCH 3/8] dtoc: Allow multiple warnings for a driver

[Simon Glass]

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!

Re: [PATCH 2/8] dtoc: Convert to use ArgumentParser

[Simon Glass]

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!

Re: [PATCH 1/8] dtoc: Avoid using subscripts on match objects

[Simon Glass]

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!

Re: [PATCH 3/8] dtoc: Allow multiple warnings for a driver

[Simon Glass]

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!)

Re: [PATCH 8/8] dtoc: Update documentation to cover warnings in more detail

[Simon Glass]

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