* [PATCH 00/10] improve OF_PLATDATA support
@ 2020-05-29 18:15 Walter Lozano
2020-05-29 18:15 ` [PATCH 01/10] dtoc: add support to scan drivers Walter Lozano
` (10 more replies)
0 siblings, 11 replies; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
When using OF_PLATDATA dtbs are converted to C structs in order to save
space as we can remove both dtbs and libraries from TPL/SPL binaries.
This patchset tries to improve its support by overcoming some limitations
in the current implementation
First, the support for scan and check for valid driver/aliases is added
in order to generate U_BOOT_DEVICE entries with valid driver names.
Secondly, the way information about linked noded (phandle) is generated
in C structs is improved in order to make it easier to get a device
associated to its data.
Lastly the the suport for the property cd-gpios is added, which is used to
configure the card detection gpio on MMC is added.
This implementation is based in discussion in [1], [2] and [3]
[1] https://patchwork.ozlabs.org/patch/1249198/
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
[3] https://patchwork.ozlabs.org/project/uboot/list/?series=176759&state=*
Walter Lozano (10):
dtoc: add support to scan drivers
dtoc: add option to disable warnings
dm: doc: update of-plat with the suppor for driver aliases
core: drop const for struct driver_info
core: extend struct driver_info to point to device
dtoc: extend dtoc to use struct driver_info when linking nodes
dm: doc: update of-plat with new phandle support
dtoc: update tests to match new platdata
dtoc: update dtb_platdata to support cd-gpios
dtoc add test for cd-gpios
doc/driver-model/of-plat.rst | 38 +++-
drivers/clk/clk-uclass.c | 11 +-
drivers/core/device.c | 28 ++-
drivers/core/root.c | 6 +-
drivers/misc/irq-uclass.c | 10 +-
drivers/mmc/ftsdc010_mci.c | 2 +-
drivers/mmc/rockchip_dw_mmc.c | 2 +-
drivers/mmc/rockchip_sdhci.c | 2 +-
drivers/ram/rockchip/sdram_rk3399.c | 2 +-
drivers/spi/rk_spi.c | 2 +-
include/clk.h | 4 +-
include/dm/device-internal.h | 2 +-
include/dm/device.h | 21 +++
include/dm/platdata.h | 14 ++
tools/dtoc/dtb_platdata.py | 129 +++++++++++--
tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 +++++
tools/dtoc/test_dtoc.py | 218 +++++++++++++++-------
17 files changed, 415 insertions(+), 118 deletions(-)
create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
--
2.20.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 02/10] dtoc: add option to disable warnings Walter Lozano
` (9 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Currently dtoc scans dtbs to convert them to struct platdata and
to generate U_BOOT_DEVICE entries. These entries need to be filled
with the driver name, but at this moment the information used is the
compatible name present in the dtb. This causes that only nodes with
a compatible name that matches a driver name generate a working
entry.
In order to improve this behaviour, this patch adds to dtoc the
capability of scan drivers source code to generate a list of valid driver
names. This allows to rise a warning in the case that an U_BOOT_DEVICE
entry will try to use a name not valid.
Additionally, in order to add more flexibility to the solution, adds the
U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
easy way to declare driver name aliases. Thanks to this, dtoc can look
for the driver name based on its alias when it populates the U_BOOT_DEVICE
entry.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
include/dm/device.h | 7 ++++
tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
2 files changed, 86 insertions(+), 4 deletions(-)
diff --git a/include/dm/device.h b/include/dm/device.h
index 975eec5d0e..2cfe10766f 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -282,6 +282,13 @@ struct driver {
#define DM_GET_DRIVER(__name) \
ll_entry_get(struct driver, __name, driver)
+/**
+ * Declare a macro to state a alias for a driver name. This macro will
+ * produce no code but its information will be parsed by tools like
+ * dtoc
+ */
+#define U_BOOT_DRIVER_ALIAS(__name, __alias)
+
/**
* dev_get_platdata() - Get the platform data for a device
*
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index ecfe0624d1..23cfda2f88 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -13,6 +13,8 @@ static data.
import collections
import copy
+import os
+import re
import sys
from dtoc import fdt
@@ -140,6 +142,9 @@ class DtbPlatdata(object):
_include_disabled: true to include nodes marked status = "disabled"
_outfile: The current output file (sys.stdout or a real file)
_lines: Stashed list of output lines for outputting in the future
+ _aliases: Dict that hold aliases for compatible strings
+ _drivers: List of valid driver names found in drivers/
+ _driver_aliases: Dict that holds aliases for driver names
"""
def __init__(self, dtb_fname, include_disabled):
self._fdt = None
@@ -149,6 +154,35 @@ class DtbPlatdata(object):
self._outfile = None
self._lines = []
self._aliases = {}
+ self._drivers = []
+ self._driver_aliases = {}
+
+ def get_normalized_compat_name(self, node):
+ """Get a node's normalized compat name
+
+ Returns a valid driver name by retrieving node's first compatible
+ string as a C identifier and perfomrming a check against _drivers
+ and a lookup in driver_aliases rising a warning in case of failure.
+
+ Args:
+ node: Node object to check
+ Return:
+ Tuple:
+ Driver name associated with the first compatible string
+ List of C identifiers for all the other compatible strings
+ (possibly empty)
+ """
+ compat_c, aliases_c = get_compat_name(node)
+ if compat_c not in self._drivers:
+ compat_c_old = compat_c
+ compat_c = self._driver_aliases.get(compat_c)
+ if not compat_c:
+ print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
+ compat_c = compat_c_old
+ else: # pragma: no cover
+ aliases_c = [compat_c_old] + aliases_c
+
+ return compat_c, aliases_c
def setup_output(self, fname):
"""Set up the output destination
@@ -243,6 +277,46 @@ class DtbPlatdata(object):
return PhandleInfo(max_args, args)
return None
+ def scan_driver(self, fn):
+ """Scan a driver file to build a list of driver names and aliases
+
+ This procedure will populate self._drivers and self._driver_aliases
+
+ Args
+ fn: Driver filename to scan
+ """
+ with open(fn) as fd:
+
+ buff = fd.read()
+
+ # The following re will search for driver names declared as
+ # U_BOOT_DRIVER(driver_name)
+ drivers = re.findall('U_BOOT_DRIVER\((.*)\)', buff)
+
+ for driver in drivers:
+ self._drivers.append(driver)
+
+ # The following re will search for driver aliases declared as
+ # U_BOOT_DRIVER_ALIAS(alias, driver_name)
+ driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', buff)
+
+ for alias in driver_aliases: # pragma: no cover
+ if len(alias) != 2:
+ continue
+ self._driver_aliases[alias[1]] = alias[0]
+
+ def scan_drivers(self):
+ """Scan the driver folders to build a list of driver names and aliases
+
+ This procedure will populate self._drivers and self._driver_aliases
+
+ """
+ for (dirpath, dirnames, filenames) in os.walk('./'):
+ for fn in filenames:
+ if not fn.endswith('.c'):
+ continue
+ self.scan_driver(dirpath + '/' + fn)
+
def scan_dtb(self):
"""Scan the device tree to obtain a tree of nodes and properties
@@ -353,7 +427,7 @@ class DtbPlatdata(object):
"""
structs = {}
for node in self._valid_nodes:
- node_name, _ = get_compat_name(node)
+ node_name, _ = self.get_normalized_compat_name(node)
fields = {}
# Get a list of all the valid properties in this node.
@@ -377,14 +451,14 @@ class DtbPlatdata(object):
upto = 0
for node in self._valid_nodes:
- node_name, _ = get_compat_name(node)
+ node_name, _ = self.get_normalized_compat_name(node)
struct = structs[node_name]
for name, prop in node.props.items():
if name not in PROP_IGNORE_LIST and name[0] != '#':
prop.Widen(struct[name])
upto += 1
- struct_name, aliases = get_compat_name(node)
+ struct_name, aliases = self.get_normalized_compat_name(node)
for alias in aliases:
self._aliases[alias] = struct_name
@@ -461,7 +535,7 @@ class DtbPlatdata(object):
Args:
node: node to output
"""
- struct_name, _ = get_compat_name(node)
+ struct_name, _ = self.get_normalized_compat_name(node)
var_name = conv_name_to_c(node.name)
self.buf('static const struct %s%s %s%s = {\n' %
(STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
@@ -562,6 +636,7 @@ def run_steps(args, dtb_file, include_disabled, output):
raise ValueError('Please specify a command: struct, platdata')
plat = DtbPlatdata(dtb_file, include_disabled)
+ plat.scan_drivers()
plat.scan_dtb()
plat.scan_tree()
plat.scan_reg_sizes()
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 02/10] dtoc: add option to disable warnings
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
2020-05-29 18:15 ` [PATCH 01/10] dtoc: add support to scan drivers Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases Walter Lozano
` (8 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
As dtoc now performs checks for valid driver names, when running dtoc
tests several warnings arise as these tests don't use valid driver
names.
This patch adds an option to disable those warning, which is only
intended for running tests.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/dtb_platdata.py | 11 +++++---
tools/dtoc/test_dtoc.py | 54 +++++++++++++++++++-------------------
2 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 23cfda2f88..0a54188348 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -141,17 +141,19 @@ class DtbPlatdata(object):
_valid_nodes: A list of Node object with compatible strings
_include_disabled: true to include nodes marked status = "disabled"
_outfile: The current output file (sys.stdout or a real file)
+ _warning_disabled: true to disable warnings about driver names not found
_lines: Stashed list of output lines for outputting in the future
_aliases: Dict that hold aliases for compatible strings
_drivers: List of valid driver names found in drivers/
_driver_aliases: Dict that holds aliases for driver names
"""
- def __init__(self, dtb_fname, include_disabled):
+ def __init__(self, dtb_fname, include_disabled, warning_disable):
self._fdt = None
self._dtb_fname = dtb_fname
self._valid_nodes = None
self._include_disabled = include_disabled
self._outfile = None
+ self._warning_disable = warning_disable
self._lines = []
self._aliases = {}
self._drivers = []
@@ -177,7 +179,8 @@ class DtbPlatdata(object):
compat_c_old = compat_c
compat_c = self._driver_aliases.get(compat_c)
if not compat_c:
- print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
+ if not self._warning_disable: # pragma: no cover
+ print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
compat_c = compat_c_old
else: # pragma: no cover
aliases_c = [compat_c_old] + aliases_c
@@ -623,7 +626,7 @@ class DtbPlatdata(object):
nodes_to_output.remove(node)
-def run_steps(args, dtb_file, include_disabled, output):
+def run_steps(args, dtb_file, include_disabled, output, warning_disable = False):
"""Run all the steps of the dtoc tool
Args:
@@ -635,7 +638,7 @@ def run_steps(args, dtb_file, include_disabled, output):
if not args:
raise ValueError('Please specify a command: struct, platdata')
- plat = DtbPlatdata(dtb_file, include_disabled)
+ plat = DtbPlatdata(dtb_file, include_disabled, warning_disable)
plat.scan_drivers()
plat.scan_dtb()
plat.scan_tree()
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 8498e8303c..a9b605cac8 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -154,12 +154,12 @@ class TestDtoc(unittest.TestCase):
"""Test output from a device tree file with no nodes"""
dtb_file = get_dtb_file('dtoc_test_empty.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
lines = infile.read().splitlines()
self.assertEqual(HEADER.splitlines(), lines)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
lines = infile.read().splitlines()
self.assertEqual(C_HEADER.splitlines() + [''], lines)
@@ -168,7 +168,7 @@ class TestDtoc(unittest.TestCase):
"""Test output from some simple nodes with various types of data"""
dtb_file = get_dtb_file('dtoc_test_simple.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -193,7 +193,7 @@ struct dtd_sandbox_spl_test_2 {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -272,7 +272,7 @@ U_BOOT_DEVICE(pmic_at_9) = {
"""Test output from a node containing a phandle reference"""
dtb_file = get_dtb_file('dtoc_test_phandle.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -284,7 +284,7 @@ struct dtd_target {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -344,7 +344,7 @@ U_BOOT_DEVICE(phandle_source2) = {
"""Test output from a node containing a phandle reference"""
dtb_file = get_dtb_file('dtoc_test_phandle_single.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -360,7 +360,7 @@ struct dtd_target {
"""Test that phandle targets are generated before their references"""
dtb_file = get_dtb_file('dtoc_test_phandle_reorder.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -390,7 +390,7 @@ U_BOOT_DEVICE(phandle_source2) = {
capture_stderr=True)
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
self.assertIn("Cannot parse 'clocks' in node 'phandle-source'",
str(e.exception))
@@ -400,7 +400,7 @@ U_BOOT_DEVICE(phandle_source2) = {
capture_stderr=True)
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
self.assertIn("Node 'phandle-target' has no '#clock-cells' property",
str(e.exception))
@@ -408,7 +408,7 @@ U_BOOT_DEVICE(phandle_source2) = {
"""Test output from a node with multiple compatible strings"""
dtb_file = get_dtb_file('dtoc_test_aliases.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -419,7 +419,7 @@ struct dtd_compat1 {
#define dtd_compat3 dtd_compat1
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -438,7 +438,7 @@ U_BOOT_DEVICE(spl_test) = {
"""Test output from a node with a 'reg' property with na=2, ns=2"""
dtb_file = get_dtb_file('dtoc_test_addr64.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -453,7 +453,7 @@ struct dtd_test3 {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -490,7 +490,7 @@ U_BOOT_DEVICE(test3) = {
"""Test output from a node with a 'reg' property with na=1, ns=1"""
dtb_file = get_dtb_file('dtoc_test_addr32.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -502,7 +502,7 @@ struct dtd_test2 {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -530,7 +530,7 @@ U_BOOT_DEVICE(test2) = {
"""Test output from a node with a 'reg' property with na=2, ns=1"""
dtb_file = get_dtb_file('dtoc_test_addr64_32.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -545,7 +545,7 @@ struct dtd_test3 {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -582,7 +582,7 @@ U_BOOT_DEVICE(test3) = {
"""Test output from a node with a 'reg' property with na=1, ns=2"""
dtb_file = get_dtb_file('dtoc_test_addr32_64.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -597,7 +597,7 @@ struct dtd_test3 {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -636,7 +636,7 @@ U_BOOT_DEVICE(test3) = {
dtb_file = get_dtb_file('dtoc_test_bad_reg.dts', capture_stderr=True)
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
self.assertIn("Node 'spl-test' reg property is not an int",
str(e.exception))
@@ -646,7 +646,7 @@ U_BOOT_DEVICE(test3) = {
dtb_file = get_dtb_file('dtoc_test_bad_reg2.dts', capture_stderr=True)
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
self.assertIn("Node 'spl-test' reg property has 3 cells which is not a multiple of na + ns = 1 + 1)",
str(e.exception))
@@ -654,7 +654,7 @@ U_BOOT_DEVICE(test3) = {
"""Test that a subequent node can add a new property to a struct"""
dtb_file = get_dtb_file('dtoc_test_add_prop.dts')
output = tools.GetOutputFilename('output')
- dtb_platdata.run_steps(['struct'], dtb_file, False, output)
+ dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(HEADER + '''
@@ -664,7 +664,7 @@ struct dtd_sandbox_spl_test {
};
''', data)
- dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
@@ -692,12 +692,12 @@ U_BOOT_DEVICE(spl_test2) = {
"""Test output to stdout"""
dtb_file = get_dtb_file('dtoc_test_simple.dts')
with test_util.capture_sys_output() as (stdout, stderr):
- dtb_platdata.run_steps(['struct'], dtb_file, False, '-')
+ dtb_platdata.run_steps(['struct'], dtb_file, False, '-', True)
def testNoCommand(self):
"""Test running dtoc without a command"""
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps([], '', False, '')
+ dtb_platdata.run_steps([], '', False, '', True)
self.assertIn("Please specify a command: struct, platdata",
str(e.exception))
@@ -706,6 +706,6 @@ U_BOOT_DEVICE(spl_test2) = {
dtb_file = get_dtb_file('dtoc_test_simple.dts')
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
- dtb_platdata.run_steps(['invalid-cmd'], dtb_file, False, output)
+ dtb_platdata.run_steps(['invalid-cmd'], dtb_file, False, output, True)
self.assertIn("Unknown command 'invalid-cmd': (use: struct, platdata)",
str(e.exception))
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
2020-05-29 18:15 ` [PATCH 01/10] dtoc: add support to scan drivers Walter Lozano
2020-05-29 18:15 ` [PATCH 02/10] dtoc: add option to disable warnings Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 04/10] core: drop const for struct driver_info Walter Lozano
` (7 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Update the documentation with the support for driver aliases using
U_BOOT_DRIVER_ALIAS.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
doc/driver-model/of-plat.rst | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/doc/driver-model/of-plat.rst b/doc/driver-model/of-plat.rst
index 034a68bb4e..aa0c8dde91 100644
--- a/doc/driver-model/of-plat.rst
+++ b/doc/driver-model/of-plat.rst
@@ -183,6 +183,17 @@ via U_BOOT_DRIVER(). This effectively means that a U_BOOT_DRIVER() with a
it to a valid name for C) is needed, so a dedicated driver is required for
each 'compatible' string.
+In order to make this a bit more flexible U_BOOT_DRIVER_ALIAS macro can be
+used to declare an alias for a driver name, typically a 'compatible' string.
+This macro produces no code, but it is by dtoc tool.
+
+During the build process dtoc parses both U_BOOT_DRIVER and U_BOOT_DRIVER_ALIAS
+to build a list of valid driver names and driver aliases. If the 'compatible'
+string used for a device does not not match a valid driver name, it will be
+checked against the list of driver aliases in order to get the right driver
+name to use. If in this step there is no match found a warning is issued to
+avoid run-time failures.
+
Where a node has multiple compatible strings, a #define is used to make them
equivalent, e.g.:
@@ -269,7 +280,7 @@ For example:
};
U_BOOT_DRIVER(mmc_drv) = {
- .name = "vendor_mmc", /* matches compatible string */
+ .name = "mmc_drv",
.id = UCLASS_MMC,
.of_match = mmc_ids,
.ofdata_to_platdata = mmc_ofdata_to_platdata,
@@ -278,6 +289,7 @@ For example:
.platdata_auto_alloc_size = sizeof(struct mmc_platdata),
};
+ U_BOOT_DRIVER_ALIAS(vendor_mmc, mmc_drv) /* matches compatible string */
Note that struct mmc_platdata is defined in the C file, not in a header. This
is to avoid needing to include dt-structs.h in a header file. The idea is to
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 04/10] core: drop const for struct driver_info
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (2 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 05/10] core: extend struct driver_info to point to device Walter Lozano
` (6 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
In order to prepare for a new support of phandle when OF_PLATDATA is used
drop the const for struct driver_info as this struct will need to be
updated on runtime.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
drivers/core/device.c | 2 +-
drivers/core/root.c | 2 +-
include/dm/device-internal.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 0157bb1fe0..a0ad080aaf 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -246,7 +246,7 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv,
}
int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
- const struct driver_info *info, struct udevice **devp)
+ struct driver_info *info, struct udevice **devp)
{
struct driver *drv;
uint platdata_size = 0;
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 14df16c280..c9ee56478a 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -25,7 +25,7 @@
DECLARE_GLOBAL_DATA_PTR;
-static const struct driver_info root_info = {
+static struct driver_info root_info = {
.name = "root_driver",
};
diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
index 294d6c1810..5145fb4e14 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent,
* @return 0 if OK, -ve on error
*/
int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
- const struct driver_info *info, struct udevice **devp);
+ struct driver_info *info, struct udevice **devp);
/**
* device_ofdata_to_platdata() - Read platform data for a device
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (3 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 04/10] core: drop const for struct driver_info Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-05-29 18:56 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
` (5 subsequent siblings)
10 siblings, 2 replies; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Currently when creating an U_BOOT_DEVICE entry a struct driver_info
is declared, which contains the data needed to instantiate the device.
However, the actual device is created at runtime and there is no proper
way to get the device based on its struct driver_info.
This patch extends struct driver_info adding a pointer to udevice which
is populated during the bind process, allowing to generate a set of
functions to get the device based on its struct driver_info.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
drivers/core/device.c | 26 +++++++++++++++++++++++---
drivers/core/root.c | 4 ++++
include/dm/device.h | 14 ++++++++++++++
include/dm/platdata.h | 14 ++++++++++++++
4 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c
index a0ad080aaf..5adbc30849 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
{
struct driver *drv;
uint platdata_size = 0;
+ int ret = 0;
drv = lists_driver_lookup_name(info->name);
if (!drv)
@@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
#if CONFIG_IS_ENABLED(OF_PLATDATA)
platdata_size = info->platdata_size;
#endif
- return device_bind_common(parent, drv, info->name,
- (void *)info->platdata, 0, ofnode_null(), platdata_size,
- devp);
+ ret = device_bind_common(parent, drv, info->name,
+ (void *)info->platdata, 0, ofnode_null(),
+ platdata_size, devp);
+ if (ret)
+ return ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+ info->dev = *devp;
+#endif
+
+ return ret;
}
static void *alloc_priv(int size, uint flags)
@@ -727,6 +735,18 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
}
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+int device_get_by_driver_info(const struct driver_info *info,
+ struct udevice **devp)
+{
+ struct udevice *dev;
+
+ dev = info->dev;
+
+ return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
+}
+#endif
+
int device_find_first_child(const struct udevice *parent, struct udevice **devp)
{
if (list_empty(&parent->child_head)) {
diff --git a/drivers/core/root.c b/drivers/core/root.c
index c9ee56478a..8f47a6b356 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
{
int ret;
+#if CONFIG_IS_ENABLED(OF_PLATDATA)
+ populate_phandle_data();
+#endif
+
ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
if (ret) {
debug("dm_init() failed: %d\n", ret);
diff --git a/include/dm/device.h b/include/dm/device.h
index 2cfe10766f..a3b3e5bc46 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -538,6 +538,20 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
*/
int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
+/**
+ * device_get_by_driver_info() - Get a device based on driver_info
+ *
+ * Locates a device by its struct driver_info.
+ *
+ * The device is probed to activate it ready for use.
+ *
+ * @info: Struct driver_info
+ * @devp: Returns pointer to device if found, otherwise this is set to NULL
+ * @return 0 if OK, -ve on error
+ */
+int device_get_by_driver_info(const struct driver_info *info,
+ struct udevice **devp);
+
/**
* device_find_first_child() - Find the first child of a device
*
diff --git a/include/dm/platdata.h b/include/dm/platdata.h
index c972fa6936..238379b0e4 100644
--- a/include/dm/platdata.h
+++ b/include/dm/platdata.h
@@ -22,12 +22,14 @@
* @name: Driver name
* @platdata: Driver-specific platform data
* @platdata_size: Size of platform data structure
+ * @dev: Device created from this structure data
*/
struct driver_info {
const char *name;
const void *platdata;
#if CONFIG_IS_ENABLED(OF_PLATDATA)
uint platdata_size;
+ struct udevice *dev;
#endif
};
@@ -43,4 +45,16 @@ struct driver_info {
#define U_BOOT_DEVICES(__name) \
ll_entry_declare_list(struct driver_info, __name, driver_info)
+/* Get a pointer to a given driver */
+#define DM_GET_DEVICE(__name) \
+ ll_entry_get(struct driver_info, __name, driver_info)
+
+/**
+ * populate_phandle_data() - Populates phandle data in platda
+ *
+ * This populates phandle data with an U_BOOT_DEVICE entry get by
+ * DM_GET_DEVICE. The implementation of this function will be done
+ * by dtoc when parsing dtb.
+ */
+void populate_phandle_data(void);
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (4 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 05/10] core: extend struct driver_info to point to device Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 07/10] dm: doc: update of-plat with new phandle support Walter Lozano
` (4 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
In the current implementation, when dtoc parses a dtb to generate a struct
platdata it converts the information related to linked nodes as pointers
to struct platdata of destination nodes. By doing this, it makes
difficult to get pointer to udevices created based on these
information.
This patch extends dtoc to use struct driver_info when populating
information about linked nodes, which makes it easier to later get
the devices created. In this context, reimplement functions like
clk_get_by_index_platdata() which made use of the previous approach.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
drivers/clk/clk-uclass.c | 11 +++++------
drivers/misc/irq-uclass.c | 10 ++++------
drivers/mmc/ftsdc010_mci.c | 2 +-
drivers/mmc/rockchip_dw_mmc.c | 2 +-
drivers/mmc/rockchip_sdhci.c | 2 +-
drivers/ram/rockchip/sdram_rk3399.c | 2 +-
drivers/spi/rk_spi.c | 2 +-
include/clk.h | 4 ++--
tools/dtoc/dtb_platdata.py | 24 +++++++++++++++++++++---
9 files changed, 37 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 71878474eb..412f26cd29 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -25,17 +25,16 @@ static inline const struct clk_ops *clk_dev_ops(struct udevice *dev)
#if CONFIG_IS_ENABLED(OF_CONTROL)
# if CONFIG_IS_ENABLED(OF_PLATDATA)
-int clk_get_by_index_platdata(struct udevice *dev, int index,
- struct phandle_1_arg *cells, struct clk *clk)
+int clk_get_by_driver_info(struct udevice *dev, struct phandle_1_arg *cells,
+ struct clk *clk)
{
int ret;
- if (index != 0)
- return -ENOSYS;
- ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev);
+ ret = device_get_by_driver_info((struct driver_info *)cells->node,
+ &clk->dev);
if (ret)
return ret;
- clk->id = cells[0].arg[0];
+ clk->id = cells->arg[0];
return 0;
}
diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
index 61aa10e465..3c38681108 100644
--- a/drivers/misc/irq-uclass.c
+++ b/drivers/misc/irq-uclass.c
@@ -63,17 +63,15 @@ int irq_read_and_clear(struct irq *irq)
}
#if CONFIG_IS_ENABLED(OF_PLATDATA)
-int irq_get_by_index_platdata(struct udevice *dev, int index,
- struct phandle_1_arg *cells, struct irq *irq)
+int irq_get_by_driver_info(struct udevice *dev,
+ struct phandle_1_arg *cells, struct irq *irq)
{
int ret;
- if (index != 0)
- return -ENOSYS;
- ret = uclass_get_device(UCLASS_IRQ, 0, &irq->dev);
+ ret = device_get_by_driver_info(cells->node, &irq->dev);
if (ret)
return ret;
- irq->id = cells[0].arg[0];
+ irq->id = cells->arg[0];
return 0;
}
diff --git a/drivers/mmc/ftsdc010_mci.c b/drivers/mmc/ftsdc010_mci.c
index 9c15eb36d6..efa92d48be 100644
--- a/drivers/mmc/ftsdc010_mci.c
+++ b/drivers/mmc/ftsdc010_mci.c
@@ -437,7 +437,7 @@ static int ftsdc010_mmc_probe(struct udevice *dev)
chip->priv = dev;
chip->dev_index = 1;
memcpy(priv->minmax, dtplat->clock_freq_min_max, sizeof(priv->minmax));
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
#endif
diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
index a0e1be8794..7b4479543c 100644
--- a/drivers/mmc/rockchip_dw_mmc.c
+++ b/drivers/mmc/rockchip_dw_mmc.c
@@ -120,7 +120,7 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
priv->minmax[0] = 400000; /* 400 kHz */
priv->minmax[1] = dtplat->max_frequency;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
#else
diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index b440996b26..b073f1a08d 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -46,7 +46,7 @@ static int arasan_sdhci_probe(struct udevice *dev)
host->name = dev->name;
host->ioaddr = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
max_frequency = dtplat->max_frequency;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &clk);
#else
max_frequency = dev_read_u32_default(dev, "max-frequency", 0);
ret = clk_get_by_index(dev, 0, &clk);
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
index d69ef01d08..87ec25f893 100644
--- a/drivers/ram/rockchip/sdram_rk3399.c
+++ b/drivers/ram/rockchip/sdram_rk3399.c
@@ -3125,7 +3125,7 @@ static int rk3399_dmc_init(struct udevice *dev)
priv->cic, priv->pmugrf, priv->pmusgrf, priv->pmucru, priv->pmu);
#if CONFIG_IS_ENABLED(OF_PLATDATA)
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->ddr_clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->ddr_clk);
#else
ret = clk_get_by_index(dev, 0, &priv->ddr_clk);
#endif
diff --git a/drivers/spi/rk_spi.c b/drivers/spi/rk_spi.c
index 95eeb8307a..bd0337272e 100644
--- a/drivers/spi/rk_spi.c
+++ b/drivers/spi/rk_spi.c
@@ -181,7 +181,7 @@ static int conv_of_platdata(struct udevice *dev)
plat->base = dtplat->reg[0];
plat->frequency = 20000000;
- ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
+ ret = clk_get_by_driver_info(dev, dtplat->clocks, &priv->clk);
if (ret < 0)
return ret;
dev->req_seq = 0;
diff --git a/include/clk.h b/include/clk.h
index c6a2713f62..a62e2efa2c 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -89,8 +89,8 @@ struct clk_bulk {
#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
struct phandle_1_arg;
-int clk_get_by_index_platdata(struct udevice *dev, int index,
- struct phandle_1_arg *cells, struct clk *clk);
+int clk_get_by_driver_info(struct udevice *dev,
+ struct phandle_1_arg *cells, struct clk *clk);
/**
* clk_get_by_index - Get/request a clock by integer index.
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 0a54188348..59b720703c 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -146,6 +146,7 @@ class DtbPlatdata(object):
_aliases: Dict that hold aliases for compatible strings
_drivers: List of valid driver names found in drivers/
_driver_aliases: Dict that holds aliases for driver names
+ _links: List of links to be included in populate_phandle_data
"""
def __init__(self, dtb_fname, include_disabled, warning_disable):
self._fdt = None
@@ -158,6 +159,7 @@ class DtbPlatdata(object):
self._aliases = {}
self._drivers = []
self._driver_aliases = {}
+ self._links = []
def get_normalized_compat_name(self, node):
"""Get a node's normalized compat name
@@ -540,7 +542,7 @@ class DtbPlatdata(object):
"""
struct_name, _ = self.get_normalized_compat_name(node)
var_name = conv_name_to_c(node.name)
- self.buf('static const struct %s%s %s%s = {\n' %
+ self.buf('static struct %s%s %s%s = {\n' %
(STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
for pname in sorted(node.props):
prop = node.props[pname]
@@ -559,6 +561,7 @@ class DtbPlatdata(object):
if info:
# Process the list as pairs of (phandle, id)
pos = 0
+ item = 0
for args in info.args:
phandle_cell = prop.value[pos]
phandle = fdt_util.fdt32_to_cpu(phandle_cell)
@@ -568,8 +571,14 @@ class DtbPlatdata(object):
for i in range(args):
arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i])))
pos += 1 + args
- vals.append('\t{&%s%s, {%s}}' % (VAL_PREFIX, name,
- ', '.join(arg_values)))
+ # node member is filled with NULL as the real value
+ # will be update at run-time by populate_phandle_data()
+ vals.append('\t{NULL, {%s}}' % (', '.join(arg_values)))
+ var_node = '%s%s.%s[%d].node' % (VAL_PREFIX, var_name, member_name, item)
+ # Save the the link information to be use to define
+ # populate_phandle_data
+ self._links.append({'var_node': var_node, 'dev_name': name})
+ item += 1
for val in vals:
self.buf('\n\t\t%s,' % val)
else:
@@ -625,6 +634,15 @@ class DtbPlatdata(object):
self.output_node(node)
nodes_to_output.remove(node)
+ # Define populate_phandle_data which will add the linking between
+ # nodes using DM_GET_DEVICE
+ # dtv_dmc_at_xxx.clocks[0].node = DM_GET_DEVICE(clock_controller_at_xxx)
+ self.buf('void populate_phandle_data(void) {\n')
+ for l in self._links:
+ self.buf('\t%s = DM_GET_DEVICE(%s);\n' % (l['var_node'], l['dev_name']))
+ self.buf('}\n')
+
+ self.out(''.join(self.get_buf()))
def run_steps(args, dtb_file, include_disabled, output, warning_disable = False):
"""Run all the steps of the dtoc tool
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 07/10] dm: doc: update of-plat with new phandle support
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (5 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 08/10] dtoc: update tests to match new platdata Walter Lozano
` (3 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Update documentation to reflect the new phandle support when OF_PLATDATA
is used. Now phandles are implemented as pointers to U_BOOT_DEVICE,
which makes it possible to get a pointer to the actual device.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
doc/driver-model/of-plat.rst | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/doc/driver-model/of-plat.rst b/doc/driver-model/of-plat.rst
index aa0c8dde91..67477feec3 100644
--- a/doc/driver-model/of-plat.rst
+++ b/doc/driver-model/of-plat.rst
@@ -69,9 +69,8 @@ strictly necessary. Notable problems include:
- Correct relations between nodes are not implemented. This means that
parent/child relations (like bus device iteration) do not work yet.
Some phandles (those that are recognised as such) are converted into
- a pointer to platform data. This pointer can potentially be used to
- access the referenced device (by searching for the pointer value).
- This feature is not yet implemented, however.
+ a pointer to struct driver_info. This pointer can be used to access
+ the referenced device.
How it works
@@ -146,10 +145,10 @@ and the following device declaration:
.clock_freq_min_max = {0x61a80, 0x8f0d180},
.vmmc_supply = 0xb,
.num_slots = 0x1,
- .clocks = {{&dtv_clock_controller_at_ff760000, 456},
- {&dtv_clock_controller_at_ff760000, 68},
- {&dtv_clock_controller_at_ff760000, 114},
- {&dtv_clock_controller_at_ff760000, 118}},
+ .clocks = {{NULL, 456},
+ {NULL, 68},
+ {NULL, 114},
+ {NULL, 118}},
.cap_mmc_highspeed = true,
.disable_wp = true,
.bus_width = 0x4,
@@ -164,6 +163,13 @@ and the following device declaration:
.platdata_size = sizeof(dtv_dwmmc_at_ff0c0000),
};
+ void populate_phandle_data(void) {
+ dtv_dwmmc_at_ff0c0000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_ff760000);
+ dtv_dwmmc_at_ff0c0000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_ff760000);
+ dtv_dwmmc_at_ff0c0000.clocks[2].node = DM_GET_DEVICE(clock_controller_at_ff760000);
+ dtv_dwmmc_at_ff0c0000.clocks[3].node = DM_GET_DEVICE(clock_controller_at_ff760000);
+ }
+
The device is then instantiated at run-time and the platform data can be
accessed using:
@@ -329,7 +335,9 @@ prevents them being used inadvertently. All usage must be bracketed with
#if CONFIG_IS_ENABLED(OF_PLATDATA).
The dt-platdata.c file contains the device declarations and is is built in
-spl/dt-platdata.c.
+spl/dt-platdata.c. It additionally contains the definition of
+populate_phandle_data() which is responsible of filling the phandle
+information by adding references to U_BOOT_DEVICE by using DM_GET_DEVICE
The beginnings of a libfdt Python module are provided. So far this only
implements a subset of the features.
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 08/10] dtoc: update tests to match new platdata
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (6 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 07/10] dm: doc: update of-plat with new phandle support Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
` (2 subsequent siblings)
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
After using a new approach to link nodes when OF_PLATDATA is enabled
the test cases need to be update.
This patch updates the tests based on this new implementation.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/test_dtoc.py | 95 +++++++++++++++++++++++------------------
1 file changed, 54 insertions(+), 41 deletions(-)
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index a9b605cac8..f2155528f8 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -47,6 +47,9 @@ C_HEADER = '''/*
#include <dt-structs.h>
'''
+C_EMPTY_POPULATE_PHANDLE_DATA = '''void populate_phandle_data(void) {
+}
+'''
def get_dtb_file(dts_fname, capture_stderr=False):
@@ -162,7 +165,7 @@ class TestDtoc(unittest.TestCase):
dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
with open(output) as infile:
lines = infile.read().splitlines()
- self.assertEqual(C_HEADER.splitlines() + [''], lines)
+ self.assertEqual(C_HEADER.splitlines() + [''] + C_EMPTY_POPULATE_PHANDLE_DATA.splitlines(), lines)
def test_simple(self):
"""Test output from some simple nodes with various types of data"""
@@ -197,7 +200,7 @@ struct dtd_sandbox_spl_test_2 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_sandbox_spl_test dtv_spl_test = {
+static struct dtd_sandbox_spl_test dtv_spl_test = {
\t.boolval\t\t= true,
\t.bytearray\t\t= {0x6, 0x0, 0x0},
\t.byteval\t\t= 0x5,
@@ -215,7 +218,7 @@ U_BOOT_DEVICE(spl_test) = {
\t.platdata_size\t= sizeof(dtv_spl_test),
};
-static const struct dtd_sandbox_spl_test dtv_spl_test2 = {
+static struct dtd_sandbox_spl_test dtv_spl_test2 = {
\t.bytearray\t\t= {0x1, 0x23, 0x34},
\t.byteval\t\t= 0x8,
\t.intarray\t\t= {0x5, 0x0, 0x0, 0x0},
@@ -231,7 +234,7 @@ U_BOOT_DEVICE(spl_test2) = {
\t.platdata_size\t= sizeof(dtv_spl_test2),
};
-static const struct dtd_sandbox_spl_test dtv_spl_test3 = {
+static struct dtd_sandbox_spl_test dtv_spl_test3 = {
\t.stringarray\t\t= {"one", "", ""},
};
U_BOOT_DEVICE(spl_test3) = {
@@ -240,7 +243,7 @@ U_BOOT_DEVICE(spl_test3) = {
\t.platdata_size\t= sizeof(dtv_spl_test3),
};
-static const struct dtd_sandbox_spl_test_2 dtv_spl_test4 = {
+static struct dtd_sandbox_spl_test_2 dtv_spl_test4 = {
};
U_BOOT_DEVICE(spl_test4) = {
\t.name\t\t= "sandbox_spl_test_2",
@@ -248,7 +251,7 @@ U_BOOT_DEVICE(spl_test4) = {
\t.platdata_size\t= sizeof(dtv_spl_test4),
};
-static const struct dtd_sandbox_i2c_test dtv_i2c_at_0 = {
+static struct dtd_sandbox_i2c_test dtv_i2c_at_0 = {
};
U_BOOT_DEVICE(i2c_at_0) = {
\t.name\t\t= "sandbox_i2c_test",
@@ -256,7 +259,7 @@ U_BOOT_DEVICE(i2c_at_0) = {
\t.platdata_size\t= sizeof(dtv_i2c_at_0),
};
-static const struct dtd_sandbox_pmic_test dtv_pmic_at_9 = {
+static struct dtd_sandbox_pmic_test dtv_pmic_at_9 = {
\t.low_power\t\t= true,
\t.reg\t\t\t= {0x9, 0x0},
};
@@ -266,7 +269,7 @@ U_BOOT_DEVICE(pmic_at_9) = {
\t.platdata_size\t= sizeof(dtv_pmic_at_9),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_phandle(self):
"""Test output from a node containing a phandle reference"""
@@ -288,7 +291,7 @@ struct dtd_target {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_target dtv_phandle_target = {
+static struct dtd_target dtv_phandle_target = {
\t.intval\t\t\t= 0x0,
};
U_BOOT_DEVICE(phandle_target) = {
@@ -297,7 +300,7 @@ U_BOOT_DEVICE(phandle_target) = {
\t.platdata_size\t= sizeof(dtv_phandle_target),
};
-static const struct dtd_target dtv_phandle2_target = {
+static struct dtd_target dtv_phandle2_target = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(phandle2_target) = {
@@ -306,7 +309,7 @@ U_BOOT_DEVICE(phandle2_target) = {
\t.platdata_size\t= sizeof(dtv_phandle2_target),
};
-static const struct dtd_target dtv_phandle3_target = {
+static struct dtd_target dtv_phandle3_target = {
\t.intval\t\t\t= 0x2,
};
U_BOOT_DEVICE(phandle3_target) = {
@@ -315,12 +318,12 @@ U_BOOT_DEVICE(phandle3_target) = {
\t.platdata_size\t= sizeof(dtv_phandle3_target),
};
-static const struct dtd_source dtv_phandle_source = {
+static struct dtd_source dtv_phandle_source = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},
-\t\t\t{&dtv_phandle2_target, {11}},
-\t\t\t{&dtv_phandle3_target, {12, 13}},
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},
+\t\t\t{NULL, {11}},
+\t\t\t{NULL, {12, 13}},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source) = {
\t.name\t\t= "source",
@@ -328,9 +331,9 @@ U_BOOT_DEVICE(phandle_source) = {
\t.platdata_size\t= sizeof(dtv_phandle_source),
};
-static const struct dtd_source dtv_phandle_source2 = {
+static struct dtd_source dtv_phandle_source2 = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source2) = {
\t.name\t\t= "source",
@@ -338,6 +341,13 @@ U_BOOT_DEVICE(phandle_source2) = {
\t.platdata_size\t= sizeof(dtv_phandle_source2),
};
+void populate_phandle_data(void) {
+\tdtv_phandle_source.clocks[0].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source.clocks[1].node = DM_GET_DEVICE(phandle2_target);
+\tdtv_phandle_source.clocks[2].node = DM_GET_DEVICE(phandle3_target);
+\tdtv_phandle_source.clocks[3].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_single(self):
@@ -364,7 +374,7 @@ struct dtd_target {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_target dtv_phandle_target = {
+static struct dtd_target dtv_phandle_target = {
};
U_BOOT_DEVICE(phandle_target) = {
\t.name\t\t= "target",
@@ -372,9 +382,9 @@ U_BOOT_DEVICE(phandle_target) = {
\t.platdata_size\t= sizeof(dtv_phandle_target),
};
-static const struct dtd_source dtv_phandle_source2 = {
+static struct dtd_source dtv_phandle_source2 = {
\t.clocks\t\t\t= {
-\t\t\t{&dtv_phandle_target, {}},},
+\t\t\t{NULL, {}},},
};
U_BOOT_DEVICE(phandle_source2) = {
\t.name\t\t= "source",
@@ -382,6 +392,9 @@ U_BOOT_DEVICE(phandle_source2) = {
\t.platdata_size\t= sizeof(dtv_phandle_source2),
};
+void populate_phandle_data(void) {
+\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_bad(self):
@@ -423,7 +436,7 @@ struct dtd_compat1 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_compat1 dtv_spl_test = {
+static struct dtd_compat1 dtv_spl_test = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(spl_test) = {
@@ -432,7 +445,7 @@ U_BOOT_DEVICE(spl_test) = {
\t.platdata_size\t= sizeof(dtv_spl_test),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses64(self):
"""Test output from a node with a 'reg' property with na=2, ns=2"""
@@ -457,7 +470,7 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x5678},
};
U_BOOT_DEVICE(test1) = {
@@ -466,7 +479,7 @@ U_BOOT_DEVICE(test1) = {
\t.platdata_size\t= sizeof(dtv_test1),
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654},
};
U_BOOT_DEVICE(test2) = {
@@ -475,7 +488,7 @@ U_BOOT_DEVICE(test2) = {
\t.platdata_size\t= sizeof(dtv_test2),
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
@@ -484,7 +497,7 @@ U_BOOT_DEVICE(test3) = {
\t.platdata_size\t= sizeof(dtv_test3),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses32(self):
"""Test output from a node with a 'reg' property with na=1, ns=1"""
@@ -506,7 +519,7 @@ struct dtd_test2 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x5678},
};
U_BOOT_DEVICE(test1) = {
@@ -515,7 +528,7 @@ U_BOOT_DEVICE(test1) = {
\t.platdata_size\t= sizeof(dtv_test1),
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x12345678, 0x98765432, 0x2, 0x3},
};
U_BOOT_DEVICE(test2) = {
@@ -524,7 +537,7 @@ U_BOOT_DEVICE(test2) = {
\t.platdata_size\t= sizeof(dtv_test2),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses64_32(self):
"""Test output from a node with a 'reg' property with na=2, ns=1"""
@@ -549,7 +562,7 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x123400000000, 0x5678},
};
U_BOOT_DEVICE(test1) = {
@@ -558,7 +571,7 @@ U_BOOT_DEVICE(test1) = {
\t.platdata_size\t= sizeof(dtv_test1),
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x1234567890123456, 0x98765432},
};
U_BOOT_DEVICE(test2) = {
@@ -567,7 +580,7 @@ U_BOOT_DEVICE(test2) = {
\t.platdata_size\t= sizeof(dtv_test2),
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x1234567890123456, 0x98765432, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
@@ -576,7 +589,7 @@ U_BOOT_DEVICE(test3) = {
\t.platdata_size\t= sizeof(dtv_test3),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_addresses32_64(self):
"""Test output from a node with a 'reg' property with na=1, ns=2"""
@@ -601,7 +614,7 @@ struct dtd_test3 {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_test1 dtv_test1 = {
+static struct dtd_test1 dtv_test1 = {
\t.reg\t\t\t= {0x1234, 0x567800000000},
};
U_BOOT_DEVICE(test1) = {
@@ -610,7 +623,7 @@ U_BOOT_DEVICE(test1) = {
\t.platdata_size\t= sizeof(dtv_test1),
};
-static const struct dtd_test2 dtv_test2 = {
+static struct dtd_test2 dtv_test2 = {
\t.reg\t\t\t= {0x12345678, 0x9876543210987654},
};
U_BOOT_DEVICE(test2) = {
@@ -619,7 +632,7 @@ U_BOOT_DEVICE(test2) = {
\t.platdata_size\t= sizeof(dtv_test2),
};
-static const struct dtd_test3 dtv_test3 = {
+static struct dtd_test3 dtv_test3 = {
\t.reg\t\t\t= {0x12345678, 0x9876543210987654, 0x2, 0x3},
};
U_BOOT_DEVICE(test3) = {
@@ -628,7 +641,7 @@ U_BOOT_DEVICE(test3) = {
\t.platdata_size\t= sizeof(dtv_test3),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def test_bad_reg(self):
"""Test that a reg property with an invalid type generates an error"""
@@ -668,7 +681,7 @@ struct dtd_sandbox_spl_test {
with open(output) as infile:
data = infile.read()
self._CheckStrings(C_HEADER + '''
-static const struct dtd_sandbox_spl_test dtv_spl_test = {
+static struct dtd_sandbox_spl_test dtv_spl_test = {
\t.intval\t\t\t= 0x1,
};
U_BOOT_DEVICE(spl_test) = {
@@ -677,7 +690,7 @@ U_BOOT_DEVICE(spl_test) = {
\t.platdata_size\t= sizeof(dtv_spl_test),
};
-static const struct dtd_sandbox_spl_test dtv_spl_test2 = {
+static struct dtd_sandbox_spl_test dtv_spl_test2 = {
\t.intarray\t\t= 0x5,
};
U_BOOT_DEVICE(spl_test2) = {
@@ -686,7 +699,7 @@ U_BOOT_DEVICE(spl_test2) = {
\t.platdata_size\t= sizeof(dtv_spl_test2),
};
-''', data)
+''' + C_EMPTY_POPULATE_PHANDLE_DATA, data)
def testStdout(self):
"""Test output to stdout"""
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (7 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 08/10] dtoc: update tests to match new platdata Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 10/10] dtoc add test for cd-gpios Walter Lozano
2020-05-29 18:25 ` [PATCH 00/10] improve OF_PLATDATA support Jagan Teki
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Currently dtoc does not support the property cd-gpios used to declare
the gpios for card detect in mmc.
This patch adds support to cd-gpios property.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/dtb_platdata.py | 13 ++++++++-----
tools/dtoc/test_dtoc.py | 2 +-
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 59b720703c..3f7defb26e 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -250,7 +250,7 @@ class DtbPlatdata(object):
Return:
Number of argument cells is this is a phandle, else None
"""
- if prop.name in ['clocks']:
+ if prop.name in ['clocks', 'cd-gpios']:
if not isinstance(prop.value, list):
prop.value = [prop.value]
val = prop.value
@@ -270,11 +270,14 @@ class DtbPlatdata(object):
if not target:
raise ValueError("Cannot parse '%s' in node '%s'" %
(prop.name, node_name))
- prop_name = '#clock-cells'
- cells = target.props.get(prop_name)
+ cells = None
+ for prop_name in ['#clock-cells', '#gpio-cells']:
+ cells = target.props.get(prop_name)
+ if cells:
+ break
if not cells:
- raise ValueError("Node '%s' has no '%s' property" %
- (target.name, prop_name))
+ raise ValueError("Node '%s' has no cells property" %
+ (target.name))
num_args = fdt_util.fdt32_to_cpu(cells.value)
max_args = max(max_args, num_args)
args.append(num_args)
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index f2155528f8..4bfbab7d37 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -414,7 +414,7 @@ void populate_phandle_data(void) {
output = tools.GetOutputFilename('output')
with self.assertRaises(ValueError) as e:
dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
- self.assertIn("Node 'phandle-target' has no '#clock-cells' property",
+ self.assertIn("Node 'phandle-target' has no cells property",
str(e.exception))
def test_aliases(self):
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 10/10] dtoc add test for cd-gpios
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (8 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
@ 2020-05-29 18:15 ` Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:25 ` [PATCH 00/10] improve OF_PLATDATA support Jagan Teki
10 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:15 UTC (permalink / raw)
To: u-boot
Add a test for dtoc taking into account the cd-gpios property.
Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 ++++++++++++++
tools/dtoc/test_dtoc.py | 67 +++++++++++++++++++++++
2 files changed, 109 insertions(+)
create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
diff --git a/tools/dtoc/dtoc_test_phandle_cd_gpios.dts b/tools/dtoc/dtoc_test_phandle_cd_gpios.dts
new file mode 100644
index 0000000000..241743e73e
--- /dev/null
+++ b/tools/dtoc/dtoc_test_phandle_cd_gpios.dts
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test device tree file for dtoc
+ *
+ * Copyright 2020 Collabora Ltd.
+ */
+
+/dts-v1/;
+
+/ {
+ phandle: phandle-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <0>;
+ #gpio-cells = <0>;
+ };
+
+ phandle_1: phandle2-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <1>;
+ #gpio-cells = <1>;
+ };
+ phandle_2: phandle3-target {
+ u-boot,dm-pre-reloc;
+ compatible = "target";
+ intval = <2>;
+ #gpio-cells = <2>;
+ };
+
+ phandle-source {
+ u-boot,dm-pre-reloc;
+ compatible = "source";
+ cd-gpios = <&phandle &phandle_1 11 &phandle_2 12 13 &phandle>;
+ };
+
+ phandle-source2 {
+ u-boot,dm-pre-reloc;
+ compatible = "source";
+ cd-gpios = <&phandle>;
+ };
+};
diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
index 4bfbab7d37..a59f8aa9a6 100755
--- a/tools/dtoc/test_dtoc.py
+++ b/tools/dtoc/test_dtoc.py
@@ -395,6 +395,73 @@ U_BOOT_DEVICE(phandle_source2) = {
void populate_phandle_data(void) {
\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target);
}
+''', data)
+
+ def test_phandle_cd_gpio(self):
+ """Test that phandle targets are generated when unsing cd-gpios"""
+ dtb_file = get_dtb_file('dtoc_test_phandle_cd_gpios.dts')
+ output = tools.GetOutputFilename('output')
+ dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
+ with open(output) as infile:
+ data = infile.read()
+ self._CheckStrings(C_HEADER + '''
+static struct dtd_target dtv_phandle_target = {
+\t.intval\t\t\t= 0x0,
+};
+U_BOOT_DEVICE(phandle_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle_target,
+\t.platdata_size\t= sizeof(dtv_phandle_target),
+};
+
+static struct dtd_target dtv_phandle2_target = {
+\t.intval\t\t\t= 0x1,
+};
+U_BOOT_DEVICE(phandle2_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle2_target,
+\t.platdata_size\t= sizeof(dtv_phandle2_target),
+};
+
+static struct dtd_target dtv_phandle3_target = {
+\t.intval\t\t\t= 0x2,
+};
+U_BOOT_DEVICE(phandle3_target) = {
+\t.name\t\t= "target",
+\t.platdata\t= &dtv_phandle3_target,
+\t.platdata_size\t= sizeof(dtv_phandle3_target),
+};
+
+static struct dtd_source dtv_phandle_source = {
+\t.cd_gpios\t\t= {
+\t\t\t{NULL, {}},
+\t\t\t{NULL, {11}},
+\t\t\t{NULL, {12, 13}},
+\t\t\t{NULL, {}},},
+};
+U_BOOT_DEVICE(phandle_source) = {
+\t.name\t\t= "source",
+\t.platdata\t= &dtv_phandle_source,
+\t.platdata_size\t= sizeof(dtv_phandle_source),
+};
+
+static struct dtd_source dtv_phandle_source2 = {
+\t.cd_gpios\t\t= {
+\t\t\t{NULL, {}},},
+};
+U_BOOT_DEVICE(phandle_source2) = {
+\t.name\t\t= "source",
+\t.platdata\t= &dtv_phandle_source2,
+\t.platdata_size\t= sizeof(dtv_phandle_source2),
+};
+
+void populate_phandle_data(void) {
+\tdtv_phandle_source.cd_gpios[0].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source.cd_gpios[1].node = DM_GET_DEVICE(phandle2_target);
+\tdtv_phandle_source.cd_gpios[2].node = DM_GET_DEVICE(phandle3_target);
+\tdtv_phandle_source.cd_gpios[3].node = DM_GET_DEVICE(phandle_target);
+\tdtv_phandle_source2.cd_gpios[0].node = DM_GET_DEVICE(phandle_target);
+}
''', data)
def test_phandle_bad(self):
--
2.20.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 00/10] improve OF_PLATDATA support
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
` (9 preceding siblings ...)
2020-05-29 18:15 ` [PATCH 10/10] dtoc add test for cd-gpios Walter Lozano
@ 2020-05-29 18:25 ` Jagan Teki
2020-05-29 19:15 ` Walter Lozano
10 siblings, 1 reply; 42+ messages in thread
From: Jagan Teki @ 2020-05-29 18:25 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, May 29, 2020 at 11:45 PM Walter Lozano
<walter.lozano@collabora.com> wrote:
>
> When using OF_PLATDATA dtbs are converted to C structs in order to save
> space as we can remove both dtbs and libraries from TPL/SPL binaries.
>
> This patchset tries to improve its support by overcoming some limitations
> in the current implementation
>
> First, the support for scan and check for valid driver/aliases is added
> in order to generate U_BOOT_DEVICE entries with valid driver names.
>
> Secondly, the way information about linked noded (phandle) is generated
> in C structs is improved in order to make it easier to get a device
> associated to its data.
>
> Lastly the the suport for the property cd-gpios is added, which is used to
> configure the card detection gpio on MMC is added.
Does it impact the footprint? If yes any statistic about how much
space has been reduced with respect to current platdata?
Jagan.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 18:15 ` [PATCH 05/10] core: extend struct driver_info to point to device Walter Lozano
@ 2020-05-29 18:56 ` Walter Lozano
2020-05-29 19:00 ` Simon Glass
2020-06-04 15:59 ` Simon Glass
1 sibling, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 18:56 UTC (permalink / raw)
To: u-boot
On 29/5/20 15:15, Walter Lozano wrote:
> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> is declared, which contains the data needed to instantiate the device.
> However, the actual device is created at runtime and there is no proper
> way to get the device based on its struct driver_info.
>
> This patch extends struct driver_info adding a pointer to udevice which
> is populated during the bind process, allowing to generate a set of
> functions to get the device based on its struct driver_info.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/core/device.c | 26 +++++++++++++++++++++++---
> drivers/core/root.c | 4 ++++
> include/dm/device.h | 14 ++++++++++++++
> include/dm/platdata.h | 14 ++++++++++++++
> 4 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index a0ad080aaf..5adbc30849 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> {
> struct driver *drv;
> uint platdata_size = 0;
> + int ret = 0;
>
> drv = lists_driver_lookup_name(info->name);
> if (!drv)
> @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> platdata_size = info->platdata_size;
> #endif
> - return device_bind_common(parent, drv, info->name,
> - (void *)info->platdata, 0, ofnode_null(), platdata_size,
> - devp);
> + ret = device_bind_common(parent, drv, info->name,
> + (void *)info->platdata, 0, ofnode_null(),
> + platdata_size, devp);
> + if (ret)
> + return ret;
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + info->dev = *devp;
> +#endif
I have tried to test this using sandbox_spl_defconfig but I've received
a segmentation fault when trying to update info->dev, however this code
works on iMX6.
Could it be some kind of protection? Any thoughts?
> +
> + return ret;
> }
>
> static void *alloc_priv(int size, uint flags)
> @@ -727,6 +735,18 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> }
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +int device_get_by_driver_info(const struct driver_info *info,
> + struct udevice **devp)
> +{
> + struct udevice *dev;
> +
> + dev = info->dev;
> +
> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> +}
> +#endif
> +
> int device_find_first_child(const struct udevice *parent, struct udevice **devp)
> {
> if (list_empty(&parent->child_head)) {
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index c9ee56478a..8f47a6b356 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
> {
> int ret;
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + populate_phandle_data();
> +#endif
> +
> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
> if (ret) {
> debug("dm_init() failed: %d\n", ret);
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 2cfe10766f..a3b3e5bc46 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -538,6 +538,20 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
> */
> int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>
> +/**
> + * device_get_by_driver_info() - Get a device based on driver_info
> + *
> + * Locates a device by its struct driver_info.
> + *
> + * The device is probed to activate it ready for use.
> + *
> + * @info: Struct driver_info
> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
> + * @return 0 if OK, -ve on error
> + */
> +int device_get_by_driver_info(const struct driver_info *info,
> + struct udevice **devp);
> +
> /**
> * device_find_first_child() - Find the first child of a device
> *
> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
> index c972fa6936..238379b0e4 100644
> --- a/include/dm/platdata.h
> +++ b/include/dm/platdata.h
> @@ -22,12 +22,14 @@
> * @name: Driver name
> * @platdata: Driver-specific platform data
> * @platdata_size: Size of platform data structure
> + * @dev: Device created from this structure data
> */
> struct driver_info {
> const char *name;
> const void *platdata;
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> uint platdata_size;
> + struct udevice *dev;
> #endif
> };
>
> @@ -43,4 +45,16 @@ struct driver_info {
> #define U_BOOT_DEVICES(__name) \
> ll_entry_declare_list(struct driver_info, __name, driver_info)
>
> +/* Get a pointer to a given driver */
> +#define DM_GET_DEVICE(__name) \
> + ll_entry_get(struct driver_info, __name, driver_info)
> +
> +/**
> + * populate_phandle_data() - Populates phandle data in platda
> + *
> + * This populates phandle data with an U_BOOT_DEVICE entry get by
> + * DM_GET_DEVICE. The implementation of this function will be done
> + * by dtoc when parsing dtb.
> + */
> +void populate_phandle_data(void);
> #endif
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 18:56 ` Walter Lozano
@ 2020-05-29 19:00 ` Simon Glass
2020-05-29 19:20 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-05-29 19:00 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 29 May 2020 at 12:56, Walter Lozano <walter.lozano@collabora.com> wrote:
>
>
> On 29/5/20 15:15, Walter Lozano wrote:
> > Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> > is declared, which contains the data needed to instantiate the device.
> > However, the actual device is created at runtime and there is no proper
> > way to get the device based on its struct driver_info.
> >
> > This patch extends struct driver_info adding a pointer to udevice which
> > is populated during the bind process, allowing to generate a set of
> > functions to get the device based on its struct driver_info.
> >
> > Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> > ---
> > drivers/core/device.c | 26 +++++++++++++++++++++++---
> > drivers/core/root.c | 4 ++++
> > include/dm/device.h | 14 ++++++++++++++
> > include/dm/platdata.h | 14 ++++++++++++++
> > 4 files changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/core/device.c b/drivers/core/device.c
> > index a0ad080aaf..5adbc30849 100644
> > --- a/drivers/core/device.c
> > +++ b/drivers/core/device.c
> > @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> > {
> > struct driver *drv;
> > uint platdata_size = 0;
> > + int ret = 0;
> >
> > drv = lists_driver_lookup_name(info->name);
> > if (!drv)
> > @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> > #if CONFIG_IS_ENABLED(OF_PLATDATA)
> > platdata_size = info->platdata_size;
> > #endif
> > - return device_bind_common(parent, drv, info->name,
> > - (void *)info->platdata, 0, ofnode_null(), platdata_size,
> > - devp);
> > + ret = device_bind_common(parent, drv, info->name,
> > + (void *)info->platdata, 0, ofnode_null(),
> > + platdata_size, devp);
> > + if (ret)
> > + return ret;
> > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > + info->dev = *devp;
> > +#endif
>
> I have tried to test this using sandbox_spl_defconfig but I've received
> a segmentation fault when trying to update info->dev, however this code
> works on iMX6.
>
> Could it be some kind of protection? Any thoughts?
Yes, see u-boot-dm/dtoc-working - arch/sandbox/cpu/u-boot-spl.lds has
an attempt to move some of the list stuff into the data region.
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 00/10] improve OF_PLATDATA support
2020-05-29 18:25 ` [PATCH 00/10] improve OF_PLATDATA support Jagan Teki
@ 2020-05-29 19:15 ` Walter Lozano
0 siblings, 0 replies; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 19:15 UTC (permalink / raw)
To: u-boot
Hi Jagan
On 29/5/20 15:25, Jagan Teki wrote:
> Hi Walter,
>
> On Fri, May 29, 2020 at 11:45 PM Walter Lozano
> <walter.lozano@collabora.com> wrote:
>> When using OF_PLATDATA dtbs are converted to C structs in order to save
>> space as we can remove both dtbs and libraries from TPL/SPL binaries.
>>
>> This patchset tries to improve its support by overcoming some limitations
>> in the current implementation
>>
>> First, the support for scan and check for valid driver/aliases is added
>> in order to generate U_BOOT_DEVICE entries with valid driver names.
>>
>> Secondly, the way information about linked noded (phandle) is generated
>> in C structs is improved in order to make it easier to get a device
>> associated to its data.
>>
>> Lastly the the suport for the property cd-gpios is added, which is used to
>> configure the card detection gpio on MMC is added.
> Does it impact the footprint? If yes any statistic about how much
> space has been reduced with respect to current platdata?
>
This series tries to overcome some of the limitations of the OF_PLATDATA
support, it does not provide an improvement to the footprint.
Mainly it makes it easier to implement OF_PLATDATA, by improving the
better support to match compatible strings with a driver name and rising
warning in case some driver name is not found. Additionally, it
implements a way to access the device pointed by a phandle.
However, Simon Glass is working on footprint improvements with the
support of tiny DM based on this work.
https://patchwork.ozlabs.org/project/uboot/patch/20200525093539.1.Ibf2d19439cde35e39192a9d4a8dad23539fae2e6 at changeid/
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 19:00 ` Simon Glass
@ 2020-05-29 19:20 ` Walter Lozano
2020-05-29 20:42 ` Simon Glass
0 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-05-29 19:20 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 29/5/20 16:00, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 29 May 2020 at 12:56, Walter Lozano <walter.lozano@collabora.com> wrote:
>>
>> On 29/5/20 15:15, Walter Lozano wrote:
>>> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
>>> is declared, which contains the data needed to instantiate the device.
>>> However, the actual device is created at runtime and there is no proper
>>> way to get the device based on its struct driver_info.
>>>
>>> This patch extends struct driver_info adding a pointer to udevice which
>>> is populated during the bind process, allowing to generate a set of
>>> functions to get the device based on its struct driver_info.
>>>
>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>> ---
>>> drivers/core/device.c | 26 +++++++++++++++++++++++---
>>> drivers/core/root.c | 4 ++++
>>> include/dm/device.h | 14 ++++++++++++++
>>> include/dm/platdata.h | 14 ++++++++++++++
>>> 4 files changed, 55 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index a0ad080aaf..5adbc30849 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>>> {
>>> struct driver *drv;
>>> uint platdata_size = 0;
>>> + int ret = 0;
>>>
>>> drv = lists_driver_lookup_name(info->name);
>>> if (!drv)
>>> @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>> platdata_size = info->platdata_size;
>>> #endif
>>> - return device_bind_common(parent, drv, info->name,
>>> - (void *)info->platdata, 0, ofnode_null(), platdata_size,
>>> - devp);
>>> + ret = device_bind_common(parent, drv, info->name,
>>> + (void *)info->platdata, 0, ofnode_null(),
>>> + platdata_size, devp);
>>> + if (ret)
>>> + return ret;
>>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>>> + info->dev = *devp;
>>> +#endif
>> I have tried to test this using sandbox_spl_defconfig but I've received
>> a segmentation fault when trying to update info->dev, however this code
>> works on iMX6.
>>
>> Could it be some kind of protection? Any thoughts?
> Yes, see u-boot-dm/dtoc-working - arch/sandbox/cpu/u-boot-spl.lds has
> an attempt to move some of the list stuff into the data region.
Thanks for confirmed it and also for the quick response. I'm about to
start a deeper review to your work about tiny-dm now.
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 19:20 ` Walter Lozano
@ 2020-05-29 20:42 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-05-29 20:42 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 29 May 2020 at 13:21, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 29/5/20 16:00, Simon Glass wrote:
> > Hi Walter,
> >
> > On Fri, 29 May 2020 at 12:56, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>
> >> On 29/5/20 15:15, Walter Lozano wrote:
> >>> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> >>> is declared, which contains the data needed to instantiate the device.
> >>> However, the actual device is created at runtime and there is no proper
> >>> way to get the device based on its struct driver_info.
> >>>
> >>> This patch extends struct driver_info adding a pointer to udevice which
> >>> is populated during the bind process, allowing to generate a set of
> >>> functions to get the device based on its struct driver_info.
> >>>
> >>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>> ---
> >>> drivers/core/device.c | 26 +++++++++++++++++++++++---
> >>> drivers/core/root.c | 4 ++++
> >>> include/dm/device.h | 14 ++++++++++++++
> >>> include/dm/platdata.h | 14 ++++++++++++++
> >>> 4 files changed, 55 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >>> index a0ad080aaf..5adbc30849 100644
> >>> --- a/drivers/core/device.c
> >>> +++ b/drivers/core/device.c
> >>> @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> >>> {
> >>> struct driver *drv;
> >>> uint platdata_size = 0;
> >>> + int ret = 0;
> >>>
> >>> drv = lists_driver_lookup_name(info->name);
> >>> if (!drv)
> >>> @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> >>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> >>> platdata_size = info->platdata_size;
> >>> #endif
> >>> - return device_bind_common(parent, drv, info->name,
> >>> - (void *)info->platdata, 0, ofnode_null(), platdata_size,
> >>> - devp);
> >>> + ret = device_bind_common(parent, drv, info->name,
> >>> + (void *)info->platdata, 0, ofnode_null(),
> >>> + platdata_size, devp);
> >>> + if (ret)
> >>> + return ret;
> >>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> >>> + info->dev = *devp;
> >>> +#endif
> >> I have tried to test this using sandbox_spl_defconfig but I've received
> >> a segmentation fault when trying to update info->dev, however this code
> >> works on iMX6.
> >>
> >> Could it be some kind of protection? Any thoughts?
> > Yes, see u-boot-dm/dtoc-working - arch/sandbox/cpu/u-boot-spl.lds has
> > an attempt to move some of the list stuff into the data region.
>
> Thanks for confirmed it and also for the quick response. I'm about to
> start a deeper review to your work about tiny-dm now.
OK, but don't take too much notice of it as it is very rough.
One other thing that might be interesting is that I found a way to put
links in data structures:
#define DM_DECL_TINY_DRIVER(__name) \
ll_entry_decl(struct tiny_drv, __name, tiny_drv)
/*
* Get a pointer to a given tiny driver, for use in data structures. This
* requires that the symbol be declared with DM_DECL_TINY_DRIVER() first
*/
#define DM_REF_TINY_DRIVER(__name) \
ll_entry_ref(struct tiny_drv, __name, tiny_drv)
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-05-29 18:15 ` [PATCH 01/10] dtoc: add support to scan drivers Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
2020-06-08 15:49 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently dtoc scans dtbs to convert them to struct platdata and
> to generate U_BOOT_DEVICE entries. These entries need to be filled
> with the driver name, but at this moment the information used is the
> compatible name present in the dtb. This causes that only nodes with
> a compatible name that matches a driver name generate a working
> entry.
>
> In order to improve this behaviour, this patch adds to dtoc the
> capability of scan drivers source code to generate a list of valid driver
> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> entry will try to use a name not valid.
>
> Additionally, in order to add more flexibility to the solution, adds the
> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> easy way to declare driver name aliases. Thanks to this, dtoc can look
> for the driver name based on its alias when it populates the U_BOOT_DEVICE
> entry.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> include/dm/device.h | 7 ++++
> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
> 2 files changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 975eec5d0e..2cfe10766f 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -282,6 +282,13 @@ struct driver {
> #define DM_GET_DRIVER(__name) \
> ll_entry_get(struct driver, __name, driver)
>
> +/**
> + * Declare a macro to state a alias for a driver name. This macro will
> + * produce no code but its information will be parsed by tools like
> + * dtoc
> + */
> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> +
> /**
> * dev_get_platdata() - Get the platform data for a device
> *
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index ecfe0624d1..23cfda2f88 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
> @@ -13,6 +13,8 @@ static data.
>
> import collections
> import copy
> +import os
> +import re
> import sys
>
> from dtoc import fdt
> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
> _include_disabled: true to include nodes marked status = "disabled"
> _outfile: The current output file (sys.stdout or a real file)
> _lines: Stashed list of output lines for outputting in the future
> + _aliases: Dict that hold aliases for compatible strings
key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
value: ...
> + _drivers: List of valid driver names found in drivers/
> + _driver_aliases: Dict that holds aliases for driver names
key:
vaue:
> """
> def __init__(self, dtb_fname, include_disabled):
> self._fdt = None
> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
> self._outfile = None
> self._lines = []
> self._aliases = {}
> + self._drivers = []
> + self._driver_aliases = {}
> +
> + def get_normalized_compat_name(self, node):
> + """Get a node's normalized compat name
> +
> + Returns a valid driver name by retrieving node's first compatible
> + string as a C identifier and perfomrming a check against _drivers
performing
> + and a lookup in driver_aliases rising a warning in case of failure.
s/ rising/, printing/
> +
> + Args:
> + node: Node object to check
> + Return:
> + Tuple:
> + Driver name associated with the first compatible string
> + List of C identifiers for all the other compatible strings
> + (possibly empty)
Can you update this comment to explain what is returned when it is not found?
> + """
> + compat_c, aliases_c = get_compat_name(node)
> + if compat_c not in self._drivers:
> + compat_c_old = compat_c
> + compat_c = self._driver_aliases.get(compat_c)
> + if not compat_c:
> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
This creates lots of warnings at present. Either we need a patch to
clean up the differences in the source code, or we need to disable the
warning.
Also the warning is not really actionable. It needs to add mention of
U_BOOT_DEVICE and ...ALIAS.
In future we can scan the compatible strings and tell the user what
changes to make, I suppose.
> + compat_c = compat_c_old
> + else: # pragma: no cover
Need to fix the coverage here
> + aliases_c = [compat_c_old] + aliases_c
> +
> + return compat_c, aliases_c
>
> def setup_output(self, fname):
> """Set up the output destination
> @@ -243,6 +277,46 @@ class DtbPlatdata(object):
> return PhandleInfo(max_args, args)
> return None
>
> + def scan_driver(self, fn):
> + """Scan a driver file to build a list of driver names and aliases
> +
> + This procedure will populate self._drivers and self._driver_aliases
> +
> + Args
> + fn: Driver filename to scan
> + """
> + with open(fn) as fd:
> +
drop blank line
> + buff = fd.read()
> +
> + # The following re will search for driver names declared as
> + # U_BOOT_DRIVER(driver_name)
> + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', buff)
> +
> + for driver in drivers:
> + self._drivers.append(driver)
> +
> + # The following re will search for driver aliases declared as
> + # U_BOOT_DRIVER_ALIAS(alias, driver_name)
> + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', buff)
> +
> + for alias in driver_aliases: # pragma: no cover
> + if len(alias) != 2:
> + continue
> + self._driver_aliases[alias[1]] = alias[0]
> +
> + def scan_drivers(self):
> + """Scan the driver folders to build a list of driver names and aliases
> +
> + This procedure will populate self._drivers and self._driver_aliases
> +
> + """
> + for (dirpath, dirnames, filenames) in os.walk('./'):
This doesn't work for out-of-tree cases (make O=xxx). You may need to
pass $(srctree) to dtc as an argument.
> + for fn in filenames:
> + if not fn.endswith('.c'):
> + continue
> + self.scan_driver(dirpath + '/' + fn)
> +
> def scan_dtb(self):
> """Scan the device tree to obtain a tree of nodes and properties
>
> @@ -353,7 +427,7 @@ class DtbPlatdata(object):
> """
> structs = {}
> for node in self._valid_nodes:
> - node_name, _ = get_compat_name(node)
> + node_name, _ = self.get_normalized_compat_name(node)
> fields = {}
>
> # Get a list of all the valid properties in this node.
> @@ -377,14 +451,14 @@ class DtbPlatdata(object):
>
> upto = 0
> for node in self._valid_nodes:
> - node_name, _ = get_compat_name(node)
> + node_name, _ = self.get_normalized_compat_name(node)
> struct = structs[node_name]
> for name, prop in node.props.items():
> if name not in PROP_IGNORE_LIST and name[0] != '#':
> prop.Widen(struct[name])
> upto += 1
>
> - struct_name, aliases = get_compat_name(node)
> + struct_name, aliases = self.get_normalized_compat_name(node)
> for alias in aliases:
> self._aliases[alias] = struct_name
>
> @@ -461,7 +535,7 @@ class DtbPlatdata(object):
> Args:
> node: node to output
> """
> - struct_name, _ = get_compat_name(node)
> + struct_name, _ = self.get_normalized_compat_name(node)
> var_name = conv_name_to_c(node.name)
> self.buf('static const struct %s%s %s%s = {\n' %
> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
> @@ -562,6 +636,7 @@ def run_steps(args, dtb_file, include_disabled, output):
> raise ValueError('Please specify a command: struct, platdata')
>
> plat = DtbPlatdata(dtb_file, include_disabled)
> + plat.scan_drivers()
> plat.scan_dtb()
> plat.scan_tree()
> plat.scan_reg_sizes()
> --
> 2.20.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases
2020-05-29 18:15 ` [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Update the documentation with the support for driver aliases using
> U_BOOT_DRIVER_ALIAS.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> doc/driver-model/of-plat.rst | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 02/10] dtoc: add option to disable warnings
2020-05-29 18:15 ` [PATCH 02/10] dtoc: add option to disable warnings Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
2020-06-08 15:51 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> As dtoc now performs checks for valid driver names, when running dtoc
> tests several warnings arise as these tests don't use valid driver
> names.
>
> This patch adds an option to disable those warning, which is only
> intended for running tests.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/dtb_platdata.py | 11 +++++---
> tools/dtoc/test_dtoc.py | 54 +++++++++++++++++++-------------------
> 2 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index 23cfda2f88..0a54188348 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
> @@ -141,17 +141,19 @@ class DtbPlatdata(object):
> _valid_nodes: A list of Node object with compatible strings
> _include_disabled: true to include nodes marked status = "disabled"
> _outfile: The current output file (sys.stdout or a real file)
> + _warning_disabled: true to disable warnings about driver names not found
> _lines: Stashed list of output lines for outputting in the future
> _aliases: Dict that hold aliases for compatible strings
> _drivers: List of valid driver names found in drivers/
> _driver_aliases: Dict that holds aliases for driver names
> """
> - def __init__(self, dtb_fname, include_disabled):
> + def __init__(self, dtb_fname, include_disabled, warning_disable):
> self._fdt = None
> self._dtb_fname = dtb_fname
> self._valid_nodes = None
> self._include_disabled = include_disabled
> self._outfile = None
> + self._warning_disable = warning_disable
> self._lines = []
> self._aliases = {}
> self._drivers = []
> @@ -177,7 +179,8 @@ class DtbPlatdata(object):
> compat_c_old = compat_c
> compat_c = self._driver_aliases.get(compat_c)
> if not compat_c:
> - print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> + if not self._warning_disable: # pragma: no cover
Need coverage for this.
> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> compat_c = compat_c_old
> else: # pragma: no cover
> aliases_c = [compat_c_old] + aliases_c
> @@ -623,7 +626,7 @@ class DtbPlatdata(object):
> nodes_to_output.remove(node)
>
>
> -def run_steps(args, dtb_file, include_disabled, output):
> +def run_steps(args, dtb_file, include_disabled, output, warning_disable = False):
no spaces around =
> """Run all the steps of the dtoc tool
>
> Args:
> @@ -635,7 +638,7 @@ def run_steps(args, dtb_file, include_disabled, output):
> if not args:
> raise ValueError('Please specify a command: struct, platdata')
>
> - plat = DtbPlatdata(dtb_file, include_disabled)
> + plat = DtbPlatdata(dtb_file, include_disabled, warning_disable)
> plat.scan_drivers()
> plat.scan_dtb()
> plat.scan_tree()
> diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
> index 8498e8303c..a9b605cac8 100755
> --- a/tools/dtoc/test_dtoc.py
> +++ b/tools/dtoc/test_dtoc.py
> @@ -154,12 +154,12 @@ class TestDtoc(unittest.TestCase):
> """Test output from a device tree file with no nodes"""
> dtb_file = get_dtb_file('dtoc_test_empty.dts')
> output = tools.GetOutputFilename('output')
> - dtb_platdata.run_steps(['struct'], dtb_file, False, output)
> + dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
> with open(output) as infile:
> lines = infile.read().splitlines()
> self.assertEqual(HEADER.splitlines(), lines)
>
> - dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
> + dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
Can you create run_test which calls run_steps with that set that to
True, to avoid adding the param everywhere in this file?
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 04/10] core: drop const for struct driver_info
2020-05-29 18:15 ` [PATCH 04/10] core: drop const for struct driver_info Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> In order to prepare for a new support of phandle when OF_PLATDATA is used
> drop the const for struct driver_info as this struct will need to be
> updated on runtime.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/core/device.c | 2 +-
> drivers/core/root.c | 2 +-
> include/dm/device-internal.h | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-05-29 18:15 ` [PATCH 05/10] core: extend struct driver_info to point to device Walter Lozano
2020-05-29 18:56 ` Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
2020-06-08 15:53 ` Walter Lozano
1 sibling, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
> is declared, which contains the data needed to instantiate the device.
> However, the actual device is created at runtime and there is no proper
> way to get the device based on its struct driver_info.
>
> This patch extends struct driver_info adding a pointer to udevice which
> is populated during the bind process, allowing to generate a set of
> functions to get the device based on its struct driver_info.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/core/device.c | 26 +++++++++++++++++++++++---
> drivers/core/root.c | 4 ++++
> include/dm/device.h | 14 ++++++++++++++
> include/dm/platdata.h | 14 ++++++++++++++
> 4 files changed, 55 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
Nits below
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index a0ad080aaf..5adbc30849 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> {
> struct driver *drv;
> uint platdata_size = 0;
> + int ret = 0;
Can you drop = 0 ?
>
> drv = lists_driver_lookup_name(info->name);
> if (!drv)
> @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> platdata_size = info->platdata_size;
> #endif
> - return device_bind_common(parent, drv, info->name,
> - (void *)info->platdata, 0, ofnode_null(), platdata_size,
> - devp);
> + ret = device_bind_common(parent, drv, info->name,
> + (void *)info->platdata, 0, ofnode_null(),
> + platdata_size, devp);
> + if (ret)
> + return ret;
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + info->dev = *devp;
> +#endif
> +
> + return ret;
> }
>
> static void *alloc_priv(int size, uint flags)
> @@ -727,6 +735,18 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> }
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> +int device_get_by_driver_info(const struct driver_info *info,
> + struct udevice **devp)
> +{
> + struct udevice *dev;
> +
> + dev = info->dev;
> +
> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
> +}
> +#endif
> +
> int device_find_first_child(const struct udevice *parent, struct udevice **devp)
> {
> if (list_empty(&parent->child_head)) {
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index c9ee56478a..8f47a6b356 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
> {
> int ret;
>
> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> + populate_phandle_data();
> +#endif
> +
> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
> if (ret) {
> debug("dm_init() failed: %d\n", ret);
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 2cfe10766f..a3b3e5bc46 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -538,6 +538,20 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
> */
> int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>
> +/**
> + * device_get_by_driver_info() - Get a device based on driver_info
> + *
> + * Locates a device by its struct driver_info.
> + *
> + * The device is probed to activate it ready for use.
> + *
> + * @info: Struct driver_info
Here you should mention using DM_GET_DRIVER() to find it.
> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
> + * @return 0 if OK, -ve on error
> + */
> +int device_get_by_driver_info(const struct driver_info *info,
> + struct udevice **devp);
> +
> /**
> * device_find_first_child() - Find the first child of a device
> *
> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
> index c972fa6936..238379b0e4 100644
> --- a/include/dm/platdata.h
> +++ b/include/dm/platdata.h
> @@ -22,12 +22,14 @@
> * @name: Driver name
> * @platdata: Driver-specific platform data
> * @platdata_size: Size of platform data structure
> + * @dev: Device created from this structure data
> */
> struct driver_info {
> const char *name;
> const void *platdata;
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> uint platdata_size;
> + struct udevice *dev;
> #endif
> };
>
> @@ -43,4 +45,16 @@ struct driver_info {
> #define U_BOOT_DEVICES(__name) \
> ll_entry_declare_list(struct driver_info, __name, driver_info)
>
> +/* Get a pointer to a given driver */
> +#define DM_GET_DEVICE(__name) \
> + ll_entry_get(struct driver_info, __name, driver_info)
> +
> +/**
> + * populate_phandle_data() - Populates phandle data in platda
> + *
> + * This populates phandle data with an U_BOOT_DEVICE entry get by
> + * DM_GET_DEVICE. The implementation of this function will be done
> + * by dtoc when parsing dtb.
> + */
> +void populate_phandle_data(void);
dm_populate_phandle_data()
> #endif
> --
> 2.20.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes
2020-05-29 18:15 ` [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> In the current implementation, when dtoc parses a dtb to generate a struct
> platdata it converts the information related to linked nodes as pointers
> to struct platdata of destination nodes. By doing this, it makes
> difficult to get pointer to udevices created based on these
> information.
>
> This patch extends dtoc to use struct driver_info when populating
> information about linked nodes, which makes it easier to later get
> the devices created. In this context, reimplement functions like
> clk_get_by_index_platdata() which made use of the previous approach.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> drivers/clk/clk-uclass.c | 11 +++++------
> drivers/misc/irq-uclass.c | 10 ++++------
> drivers/mmc/ftsdc010_mci.c | 2 +-
> drivers/mmc/rockchip_dw_mmc.c | 2 +-
> drivers/mmc/rockchip_sdhci.c | 2 +-
> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
> drivers/spi/rk_spi.c | 2 +-
> include/clk.h | 4 ++--
> tools/dtoc/dtb_platdata.py | 24 +++++++++++++++++++++---
> 9 files changed, 37 insertions(+), 22 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 07/10] dm: doc: update of-plat with new phandle support
2020-05-29 18:15 ` [PATCH 07/10] dm: doc: update of-plat with new phandle support Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
2020-06-08 15:54 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Update documentation to reflect the new phandle support when OF_PLATDATA
> is used. Now phandles are implemented as pointers to U_BOOT_DEVICE,
> which makes it possible to get a pointer to the actual device.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> doc/driver-model/of-plat.rst | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But note dm_populate_...
I would like it to be easy to see what module/subsystem a function belongs to.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 08/10] dtoc: update tests to match new platdata
2020-05-29 18:15 ` [PATCH 08/10] dtoc: update tests to match new platdata Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> After using a new approach to link nodes when OF_PLATDATA is enabled
> the test cases need to be update.
>
> This patch updates the tests based on this new implementation.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/test_dtoc.py | 95 +++++++++++++++++++++++------------------
> 1 file changed, 54 insertions(+), 41 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 10/10] dtoc add test for cd-gpios
2020-05-29 18:15 ` [PATCH 10/10] dtoc add test for cd-gpios Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Add a test for dtoc taking into account the cd-gpios property.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/dtoc_test_phandle_cd_gpios.dts | 42 ++++++++++++++
> tools/dtoc/test_dtoc.py | 67 +++++++++++++++++++++++
> 2 files changed, 109 insertions(+)
> create mode 100644 tools/dtoc/dtoc_test_phandle_cd_gpios.dts
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios
2020-05-29 18:15 ` [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
@ 2020-06-04 15:59 ` Simon Glass
2020-06-08 16:01 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-04 15:59 UTC (permalink / raw)
To: u-boot
On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Currently dtoc does not support the property cd-gpios used to declare
> the gpios for card detect in mmc.
>
> This patch adds support to cd-gpios property.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
> tools/dtoc/dtb_platdata.py | 13 ++++++++-----
> tools/dtoc/test_dtoc.py | 2 +-
> 2 files changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But I think you should mention GPIOs, rather than just cd-gpios, as it
seems this will add support for any GPIO.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-04 15:59 ` Simon Glass
@ 2020-06-08 15:49 ` Walter Lozano
2020-06-11 16:15 ` Walter Lozano
2020-06-11 16:45 ` Simon Glass
0 siblings, 2 replies; 42+ messages in thread
From: Walter Lozano @ 2020-06-08 15:49 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 4/6/20 12:59, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Currently dtoc scans dtbs to convert them to struct platdata and
>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>> with the driver name, but at this moment the information used is the
>> compatible name present in the dtb. This causes that only nodes with
>> a compatible name that matches a driver name generate a working
>> entry.
>>
>> In order to improve this behaviour, this patch adds to dtoc the
>> capability of scan drivers source code to generate a list of valid driver
>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>> entry will try to use a name not valid.
>>
>> Additionally, in order to add more flexibility to the solution, adds the
>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
>> entry.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>> include/dm/device.h | 7 ++++
>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 86 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 975eec5d0e..2cfe10766f 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -282,6 +282,13 @@ struct driver {
>> #define DM_GET_DRIVER(__name) \
>> ll_entry_get(struct driver, __name, driver)
>>
>> +/**
>> + * Declare a macro to state a alias for a driver name. This macro will
>> + * produce no code but its information will be parsed by tools like
>> + * dtoc
>> + */
>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>> +
>> /**
>> * dev_get_platdata() - Get the platform data for a device
>> *
>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index ecfe0624d1..23cfda2f88 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
>> @@ -13,6 +13,8 @@ static data.
>>
>> import collections
>> import copy
>> +import os
>> +import re
>> import sys
>>
>> from dtoc import fdt
>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
>> _include_disabled: true to include nodes marked status = "disabled"
>> _outfile: The current output file (sys.stdout or a real file)
>> _lines: Stashed list of output lines for outputting in the future
>> + _aliases: Dict that hold aliases for compatible strings
> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
> value: ...
Noted.
>> + _drivers: List of valid driver names found in drivers/
>> + _driver_aliases: Dict that holds aliases for driver names
> key:
> vaue:
OK.
>
>> """
>> def __init__(self, dtb_fname, include_disabled):
>> self._fdt = None
>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
>> self._outfile = None
>> self._lines = []
>> self._aliases = {}
>> + self._drivers = []
>> + self._driver_aliases = {}
>> +
>> + def get_normalized_compat_name(self, node):
>> + """Get a node's normalized compat name
>> +
>> + Returns a valid driver name by retrieving node's first compatible
>> + string as a C identifier and perfomrming a check against _drivers
> performing
Noted.
>
>> + and a lookup in driver_aliases rising a warning in case of failure.
> s/ rising/, printing/
OK.
>
>> +
>> + Args:
>> + node: Node object to check
>> + Return:
>> + Tuple:
>> + Driver name associated with the first compatible string
>> + List of C identifiers for all the other compatible strings
>> + (possibly empty)
> Can you update this comment to explain what is returned when it is not found?
Sure.
>> + """
>> + compat_c, aliases_c = get_compat_name(node)
>> + if compat_c not in self._drivers:
>> + compat_c_old = compat_c
>> + compat_c = self._driver_aliases.get(compat_c)
>> + if not compat_c:
>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> This creates lots of warnings at present. Either we need a patch to
> clean up the differences in the source code, or we need to disable the
> warning.
Regarding this, maybe we should have a list of driver names we don't
expect to support, like simple_bus. For this to work probably the best
approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
so each config can add their owns.
> Also the warning is not really actionable. It needs to add mention of
> U_BOOT_DEVICE and ...ALIAS.
I agree with you. Thanks.
>
> In future we can scan the compatible strings and tell the user what
> changes to make, I suppose.
Yes, it will be a great improvement!
>
>> + compat_c = compat_c_old
>> + else: # pragma: no cover
> Need to fix the coverage here
Noted. I will a few more tests.
>
>> + aliases_c = [compat_c_old] + aliases_c
>> +
>> + return compat_c, aliases_c
>>
>> def setup_output(self, fname):
>> """Set up the output destination
>> @@ -243,6 +277,46 @@ class DtbPlatdata(object):
>> return PhandleInfo(max_args, args)
>> return None
>>
>> + def scan_driver(self, fn):
>> + """Scan a driver file to build a list of driver names and aliases
>> +
>> + This procedure will populate self._drivers and self._driver_aliases
>> +
>> + Args
>> + fn: Driver filename to scan
>> + """
>> + with open(fn) as fd:
>> +
> drop blank line
OK.
>> + buff = fd.read()
>> +
>> + # The following re will search for driver names declared as
>> + # U_BOOT_DRIVER(driver_name)
>> + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', buff)
>> +
>> + for driver in drivers:
>> + self._drivers.append(driver)
>> +
>> + # The following re will search for driver aliases declared as
>> + # U_BOOT_DRIVER_ALIAS(alias, driver_name)
>> + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', buff)
>> +
>> + for alias in driver_aliases: # pragma: no cover
>> + if len(alias) != 2:
>> + continue
>> + self._driver_aliases[alias[1]] = alias[0]
>> +
>> + def scan_drivers(self):
>> + """Scan the driver folders to build a list of driver names and aliases
>> +
>> + This procedure will populate self._drivers and self._driver_aliases
>> +
>> + """
>> + for (dirpath, dirnames, filenames) in os.walk('./'):
> This doesn't work for out-of-tree cases (make O=xxx). You may need to
> pass $(srctree) to dtc as an argument.
Thanks for pointing at this.
>> + for fn in filenames:
>> + if not fn.endswith('.c'):
>> + continue
>> + self.scan_driver(dirpath + '/' + fn)
>> +
>> def scan_dtb(self):
>> """Scan the device tree to obtain a tree of nodes and properties
>>
>> @@ -353,7 +427,7 @@ class DtbPlatdata(object):
>> """
>> structs = {}
>> for node in self._valid_nodes:
>> - node_name, _ = get_compat_name(node)
>> + node_name, _ = self.get_normalized_compat_name(node)
>> fields = {}
>>
>> # Get a list of all the valid properties in this node.
>> @@ -377,14 +451,14 @@ class DtbPlatdata(object):
>>
>> upto = 0
>> for node in self._valid_nodes:
>> - node_name, _ = get_compat_name(node)
>> + node_name, _ = self.get_normalized_compat_name(node)
>> struct = structs[node_name]
>> for name, prop in node.props.items():
>> if name not in PROP_IGNORE_LIST and name[0] != '#':
>> prop.Widen(struct[name])
>> upto += 1
>>
>> - struct_name, aliases = get_compat_name(node)
>> + struct_name, aliases = self.get_normalized_compat_name(node)
>> for alias in aliases:
>> self._aliases[alias] = struct_name
>>
>> @@ -461,7 +535,7 @@ class DtbPlatdata(object):
>> Args:
>> node: node to output
>> """
>> - struct_name, _ = get_compat_name(node)
>> + struct_name, _ = self.get_normalized_compat_name(node)
>> var_name = conv_name_to_c(node.name)
>> self.buf('static const struct %s%s %s%s = {\n' %
>> (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
>> @@ -562,6 +636,7 @@ def run_steps(args, dtb_file, include_disabled, output):
>> raise ValueError('Please specify a command: struct, platdata')
>>
>> plat = DtbPlatdata(dtb_file, include_disabled)
>> + plat.scan_drivers()
>> plat.scan_dtb()
>> plat.scan_tree()
>> plat.scan_reg_sizes()
>> --
>> 2.20.1
>>
Thanks once again for your review
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 02/10] dtoc: add option to disable warnings
2020-06-04 15:59 ` Simon Glass
@ 2020-06-08 15:51 ` Walter Lozano
0 siblings, 0 replies; 42+ messages in thread
From: Walter Lozano @ 2020-06-08 15:51 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 4/6/20 12:59, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>> As dtoc now performs checks for valid driver names, when running dtoc
>> tests several warnings arise as these tests don't use valid driver
>> names.
>>
>> This patch adds an option to disable those warning, which is only
>> intended for running tests.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>> tools/dtoc/dtb_platdata.py | 11 +++++---
>> tools/dtoc/test_dtoc.py | 54 +++++++++++++++++++-------------------
>> 2 files changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index 23cfda2f88..0a54188348 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
>> @@ -141,17 +141,19 @@ class DtbPlatdata(object):
>> _valid_nodes: A list of Node object with compatible strings
>> _include_disabled: true to include nodes marked status = "disabled"
>> _outfile: The current output file (sys.stdout or a real file)
>> + _warning_disabled: true to disable warnings about driver names not found
>> _lines: Stashed list of output lines for outputting in the future
>> _aliases: Dict that hold aliases for compatible strings
>> _drivers: List of valid driver names found in drivers/
>> _driver_aliases: Dict that holds aliases for driver names
>> """
>> - def __init__(self, dtb_fname, include_disabled):
>> + def __init__(self, dtb_fname, include_disabled, warning_disable):
>> self._fdt = None
>> self._dtb_fname = dtb_fname
>> self._valid_nodes = None
>> self._include_disabled = include_disabled
>> self._outfile = None
>> + self._warning_disable = warning_disable
>> self._lines = []
>> self._aliases = {}
>> self._drivers = []
>> @@ -177,7 +179,8 @@ class DtbPlatdata(object):
>> compat_c_old = compat_c
>> compat_c = self._driver_aliases.get(compat_c)
>> if not compat_c:
>> - print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
>> + if not self._warning_disable: # pragma: no cover
> Need coverage for this.
No problem!
>
>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
>> compat_c = compat_c_old
>> else: # pragma: no cover
>> aliases_c = [compat_c_old] + aliases_c
>> @@ -623,7 +626,7 @@ class DtbPlatdata(object):
>> nodes_to_output.remove(node)
>>
>>
>> -def run_steps(args, dtb_file, include_disabled, output):
>> +def run_steps(args, dtb_file, include_disabled, output, warning_disable = False):
> no spaces around =
Sure.
>> """Run all the steps of the dtoc tool
>>
>> Args:
>> @@ -635,7 +638,7 @@ def run_steps(args, dtb_file, include_disabled, output):
>> if not args:
>> raise ValueError('Please specify a command: struct, platdata')
>>
>> - plat = DtbPlatdata(dtb_file, include_disabled)
>> + plat = DtbPlatdata(dtb_file, include_disabled, warning_disable)
>> plat.scan_drivers()
>> plat.scan_dtb()
>> plat.scan_tree()
>> diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py
>> index 8498e8303c..a9b605cac8 100755
>> --- a/tools/dtoc/test_dtoc.py
>> +++ b/tools/dtoc/test_dtoc.py
>> @@ -154,12 +154,12 @@ class TestDtoc(unittest.TestCase):
>> """Test output from a device tree file with no nodes"""
>> dtb_file = get_dtb_file('dtoc_test_empty.dts')
>> output = tools.GetOutputFilename('output')
>> - dtb_platdata.run_steps(['struct'], dtb_file, False, output)
>> + dtb_platdata.run_steps(['struct'], dtb_file, False, output, True)
>> with open(output) as infile:
>> lines = infile.read().splitlines()
>> self.assertEqual(HEADER.splitlines(), lines)
>>
>> - dtb_platdata.run_steps(['platdata'], dtb_file, False, output)
>> + dtb_platdata.run_steps(['platdata'], dtb_file, False, output, True)
> Can you create run_test which calls run_steps with that set that to
> True, to avoid adding the param everywhere in this file?
Sure, no problem. Thanks!
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 05/10] core: extend struct driver_info to point to device
2020-06-04 15:59 ` Simon Glass
@ 2020-06-08 15:53 ` Walter Lozano
0 siblings, 0 replies; 42+ messages in thread
From: Walter Lozano @ 2020-06-08 15:53 UTC (permalink / raw)
To: u-boot
On 4/6/20 12:59, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Currently when creating an U_BOOT_DEVICE entry a struct driver_info
>> is declared, which contains the data needed to instantiate the device.
>> However, the actual device is created at runtime and there is no proper
>> way to get the device based on its struct driver_info.
>>
>> This patch extends struct driver_info adding a pointer to udevice which
>> is populated during the bind process, allowing to generate a set of
>> functions to get the device based on its struct driver_info.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>> drivers/core/device.c | 26 +++++++++++++++++++++++---
>> drivers/core/root.c | 4 ++++
>> include/dm/device.h | 14 ++++++++++++++
>> include/dm/platdata.h | 14 ++++++++++++++
>> 4 files changed, 55 insertions(+), 3 deletions(-)
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Nits below
Thanks for taking the time!
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index a0ad080aaf..5adbc30849 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -250,6 +250,7 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> {
>> struct driver *drv;
>> uint platdata_size = 0;
>> + int ret = 0;
> Can you drop = 0 ?
Noted!
>
>> drv = lists_driver_lookup_name(info->name);
>> if (!drv)
>> @@ -260,9 +261,16 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only,
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> platdata_size = info->platdata_size;
>> #endif
>> - return device_bind_common(parent, drv, info->name,
>> - (void *)info->platdata, 0, ofnode_null(), platdata_size,
>> - devp);
>> + ret = device_bind_common(parent, drv, info->name,
>> + (void *)info->platdata, 0, ofnode_null(),
>> + platdata_size, devp);
>> + if (ret)
>> + return ret;
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> + info->dev = *devp;
>> +#endif
>> +
>> + return ret;
>> }
>>
>> static void *alloc_priv(int size, uint flags)
>> @@ -727,6 +735,18 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp)
>> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>> }
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +int device_get_by_driver_info(const struct driver_info *info,
>> + struct udevice **devp)
>> +{
>> + struct udevice *dev;
>> +
>> + dev = info->dev;
>> +
>> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp);
>> +}
>> +#endif
>> +
>> int device_find_first_child(const struct udevice *parent, struct udevice **devp)
>> {
>> if (list_empty(&parent->child_head)) {
>> diff --git a/drivers/core/root.c b/drivers/core/root.c
>> index c9ee56478a..8f47a6b356 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only)
>> {
>> int ret;
>>
>> +#if CONFIG_IS_ENABLED(OF_PLATDATA)
>> + populate_phandle_data();
>> +#endif
>> +
>> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE));
>> if (ret) {
>> debug("dm_init() failed: %d\n", ret);
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 2cfe10766f..a3b3e5bc46 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -538,6 +538,20 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp);
>> */
>> int device_get_global_by_ofnode(ofnode node, struct udevice **devp);
>>
>> +/**
>> + * device_get_by_driver_info() - Get a device based on driver_info
>> + *
>> + * Locates a device by its struct driver_info.
>> + *
>> + * The device is probed to activate it ready for use.
>> + *
>> + * @info: Struct driver_info
> Here you should mention using DM_GET_DRIVER() to find it.
OK, I'll add a reference.
>> + * @devp: Returns pointer to device if found, otherwise this is set to NULL
>> + * @return 0 if OK, -ve on error
>> + */
>> +int device_get_by_driver_info(const struct driver_info *info,
>> + struct udevice **devp);
>> +
>> /**
>> * device_find_first_child() - Find the first child of a device
>> *
>> diff --git a/include/dm/platdata.h b/include/dm/platdata.h
>> index c972fa6936..238379b0e4 100644
>> --- a/include/dm/platdata.h
>> +++ b/include/dm/platdata.h
>> @@ -22,12 +22,14 @@
>> * @name: Driver name
>> * @platdata: Driver-specific platform data
>> * @platdata_size: Size of platform data structure
>> + * @dev: Device created from this structure data
>> */
>> struct driver_info {
>> const char *name;
>> const void *platdata;
>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> uint platdata_size;
>> + struct udevice *dev;
>> #endif
>> };
>>
>> @@ -43,4 +45,16 @@ struct driver_info {
>> #define U_BOOT_DEVICES(__name) \
>> ll_entry_declare_list(struct driver_info, __name, driver_info)
>>
>> +/* Get a pointer to a given driver */
>> +#define DM_GET_DEVICE(__name) \
>> + ll_entry_get(struct driver_info, __name, driver_info)
>> +
>> +/**
>> + * populate_phandle_data() - Populates phandle data in platda
>> + *
>> + * This populates phandle data with an U_BOOT_DEVICE entry get by
>> + * DM_GET_DEVICE. The implementation of this function will be done
>> + * by dtoc when parsing dtb.
>> + */
>> +void populate_phandle_data(void);
> dm_populate_phandle_data()
Sure!
>
>> #endif
>> --
>> 2.20.1
>>
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 07/10] dm: doc: update of-plat with new phandle support
2020-06-04 15:59 ` Simon Glass
@ 2020-06-08 15:54 ` Walter Lozano
0 siblings, 0 replies; 42+ messages in thread
From: Walter Lozano @ 2020-06-08 15:54 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 4/6/20 12:59, Simon Glass wrote:
> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Update documentation to reflect the new phandle support when OF_PLATDATA
>> is used. Now phandles are implemented as pointers to U_BOOT_DEVICE,
>> which makes it possible to get a pointer to the actual device.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>> doc/driver-model/of-plat.rst | 24 ++++++++++++++++--------
>> 1 file changed, 16 insertions(+), 8 deletions(-)
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But note dm_populate_...
>
> I would like it to be easy to see what module/subsystem a function belongs to.
Sure, I totally agree.
Thanks again for your deep review.
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios
2020-06-04 15:59 ` Simon Glass
@ 2020-06-08 16:01 ` Walter Lozano
2020-06-14 2:49 ` Simon Glass
0 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-06-08 16:01 UTC (permalink / raw)
To: u-boot
Hi Simon
On 4/6/20 12:59, Simon Glass wrote:
> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Currently dtoc does not support the property cd-gpios used to declare
>> the gpios for card detect in mmc.
>>
>> This patch adds support to cd-gpios property.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>> tools/dtoc/dtb_platdata.py | 13 ++++++++-----
>> tools/dtoc/test_dtoc.py | 2 +-
>> 2 files changed, 9 insertions(+), 6 deletions(-)
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But I think you should mention GPIOs, rather than just cd-gpios, as it
> seems this will add support for any GPIO.
Sorry, probably I am missing something, but in order to support GPIOs in
general we need to support a set of properties such as "cs-gpios",
"rb-gpios", "gpios" among others.
Do you agree?
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-08 15:49 ` Walter Lozano
@ 2020-06-11 16:15 ` Walter Lozano
2020-06-11 16:44 ` Simon Glass
2020-06-11 16:45 ` Simon Glass
1 sibling, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-06-11 16:15 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 8/6/20 12:49, Walter Lozano wrote:
> Hi Simon,
>
> On 4/6/20 12:59, Simon Glass wrote:
>> Hi Walter,
>>
>> On Fri, 29 May 2020 at 12:15, Walter Lozano
>> <walter.lozano@collabora.com> wrote:
>>> Currently dtoc scans dtbs to convert them to struct platdata and
>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>>> with the driver name, but at this moment the information used is the
>>> compatible name present in the dtb. This causes that only nodes with
>>> a compatible name that matches a driver name generate a working
>>> entry.
>>>
>>> In order to improve this behaviour, this patch adds to dtoc the
>>> capability of scan drivers source code to generate a list of valid
>>> driver
>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>>> entry will try to use a name not valid.
>>>
>>> Additionally, in order to add more flexibility to the solution, adds
>>> the
>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but
>>> allows an
>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>>> for the driver name based on its alias when it populates the
>>> U_BOOT_DEVICE
>>> entry.
>>>
>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>> ---
>>> ? include/dm/device.h??????? |? 7 ++++
>>> ? tools/dtoc/dtb_platdata.py | 83
>>> ++++++++++++++++++++++++++++++++++++--
>>> ? 2 files changed, 86 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>> index 975eec5d0e..2cfe10766f 100644
>>> --- a/include/dm/device.h
>>> +++ b/include/dm/device.h
>>> @@ -282,6 +282,13 @@ struct driver {
>>> ? #define DM_GET_DRIVER(__name) \
>>> ???????? ll_entry_get(struct driver, __name, driver)
>>>
>>> +/**
>>> + * Declare a macro to state a alias for a driver name. This macro will
>>> + * produce no code but its information will be parsed by tools like
>>> + * dtoc
>>> + */
>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>>> +
>>> ? /**
>>> ?? * dev_get_platdata() - Get the platform data for a device
>>> ?? *
>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>>> index ecfe0624d1..23cfda2f88 100644
>>> --- a/tools/dtoc/dtb_platdata.py
>>> +++ b/tools/dtoc/dtb_platdata.py
>>> @@ -13,6 +13,8 @@ static data.
>>>
>>> ? import collections
>>> ? import copy
>>> +import os
>>> +import re
>>> ? import sys
>>>
>>> ? from dtoc import fdt
>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
>>> ????????? _include_disabled: true to include nodes marked status =
>>> "disabled"
>>> ????????? _outfile: The current output file (sys.stdout or a real file)
>>> ????????? _lines: Stashed list of output lines for outputting in the
>>> future
>>> +??????? _aliases: Dict that hold aliases for compatible strings
>> key: The driver name, i.e. the part between brackets in
>> U_BOOT_DRIVER(xx)? ??
>> value: ...
> Noted.
>>> +??????? _drivers: List of valid driver names found in drivers/
>>> +??????? _driver_aliases: Dict that holds aliases for driver names
>> key:
>> vaue:
> OK.
>>
>>> ????? """
>>> ????? def __init__(self, dtb_fname, include_disabled):
>>> ????????? self._fdt = None
>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
>>> ????????? self._outfile = None
>>> ????????? self._lines = []
>>> ????????? self._aliases = {}
>>> +??????? self._drivers = []
>>> +??????? self._driver_aliases = {}
>>> +
>>> +??? def get_normalized_compat_name(self, node):
>>> +??????? """Get a node's normalized compat name
>>> +
>>> +??????? Returns a valid driver name by retrieving node's first
>>> compatible
>>> +??????? string as a C identifier and perfomrming a check against
>>> _drivers
>> performing
> Noted.
>>
>>> +??????? and a lookup in driver_aliases rising a warning in case of
>>> failure.
>> s/ rising/, printing/
> OK.
>>
>>> +
>>> +??????? Args:
>>> +??????????? node: Node object to check
>>> +??????? Return:
>>> +??????????? Tuple:
>>> +??????????????? Driver name associated with the first compatible
>>> string
>>> +??????????????? List of C identifiers for all the other compatible
>>> strings
>>> +??????????????????? (possibly empty)
>> Can you update this comment to explain what is returned when it is
>> not found?
> Sure.
>>> +??????? """
>>> +??????? compat_c, aliases_c = get_compat_name(node)
>>> +??????? if compat_c not in self._drivers:
>>> +??????????? compat_c_old = compat_c
>>> +??????????? compat_c = self._driver_aliases.get(compat_c)
>>> +??????????? if not compat_c:
>>> +??????????????? print('WARNING: the driver %s was not found in the
>>> driver list' % (compat_c_old))
>> This creates lots of warnings at present. Either we need a patch to
>> clean up the differences in the source code, or we need to disable the
>> warning.
>
>
> Regarding this, maybe we should have a list of driver names we don't
> expect to support, like simple_bus. For this to work probably the best
> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> so each config can add their owns.
I've been thinking about this issue, I think it would be better to only
print the warning if build is issue with
make V=1
What do you think?
>
>> Also the warning is not really actionable. It needs to add mention of
>> U_BOOT_DEVICE and ...ALIAS.
> I agree with you. Thanks.
>>
>> In future we can scan the compatible strings and tell the user what
>> changes to make, I suppose.
> Yes, it will be a great improvement!
>>
>>> +??????????????? compat_c = compat_c_old
>>> +??????????? else: # pragma: no cover
>> Need to fix the coverage here
> Noted. I will a few more tests.
>>
>>> +??????????????? aliases_c = [compat_c_old] + aliases_c
>>> +
>>> +??????? return compat_c, aliases_c
>>>
>>> ????? def setup_output(self, fname):
>>> ????????? """Set up the output destination
>>> @@ -243,6 +277,46 @@ class DtbPlatdata(object):
>>> ????????????? return PhandleInfo(max_args, args)
>>> ????????? return None
>>>
>>> +??? def scan_driver(self, fn):
>>> +??????? """Scan a driver file to build a list of driver names and
>>> aliases
>>> +
>>> +??????? This procedure will populate self._drivers and
>>> self._driver_aliases
>>> +
>>> +??????? Args
>>> +??????????? fn: Driver filename to scan
>>> +??????? """
>>> +??????? with open(fn) as fd:
>>> +
>> drop blank line
>
> OK.
>
>>> +??????????? buff = fd.read()
>>> +
>>> +??????????? # The following re will search for driver names
>>> declared as
>>> +??????????? # U_BOOT_DRIVER(driver_name)
>>> +??????????? drivers = re.findall('U_BOOT_DRIVER\((.*)\)', buff)
>>> +
>>> +??????????? for driver in drivers:
>>> +??????????????? self._drivers.append(driver)
>>> +
>>> +??????????? # The following re will search for driver aliases
>>> declared as
>>> +??????????? # U_BOOT_DRIVER_ALIAS(alias, driver_name)
>>> +??????????? driver_aliases =
>>> re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', buff)
>>> +
>>> +??????????? for alias in driver_aliases: # pragma: no cover
>>> +??????????????? if len(alias) != 2:
>>> +??????????????????? continue
>>> +??????????????? self._driver_aliases[alias[1]] = alias[0]
>>> +
>>> +??? def scan_drivers(self):
>>> +??????? """Scan the driver folders to build a list of driver names
>>> and aliases
>>> +
>>> +??????? This procedure will populate self._drivers and
>>> self._driver_aliases
>>> +
>>> +??????? """
>>> +??????? for (dirpath, dirnames, filenames) in os.walk('./'):
>> This doesn't work for out-of-tree cases (make O=xxx). You may need to
>> pass $(srctree) to dtc as an argument.
> Thanks for pointing at this.
>>> +??????????? for fn in filenames:
>>> +??????????????? if not fn.endswith('.c'):
>>> +??????????????????? continue
>>> +??????????????? self.scan_driver(dirpath + '/' + fn)
>>> +
>>> ????? def scan_dtb(self):
>>> ????????? """Scan the device tree to obtain a tree of nodes and
>>> properties
>>>
>>> @@ -353,7 +427,7 @@ class DtbPlatdata(object):
>>> ????????? """
>>> ????????? structs = {}
>>> ????????? for node in self._valid_nodes:
>>> -??????????? node_name, _ = get_compat_name(node)
>>> +??????????? node_name, _ = self.get_normalized_compat_name(node)
>>> ????????????? fields = {}
>>>
>>> ????????????? # Get a list of all the valid properties in this node.
>>> @@ -377,14 +451,14 @@ class DtbPlatdata(object):
>>>
>>> ????????? upto = 0
>>> ????????? for node in self._valid_nodes:
>>> -??????????? node_name, _ = get_compat_name(node)
>>> +??????????? node_name, _ = self.get_normalized_compat_name(node)
>>> ????????????? struct = structs[node_name]
>>> ????????????? for name, prop in node.props.items():
>>> ????????????????? if name not in PROP_IGNORE_LIST and name[0] != '#':
>>> ????????????????????? prop.Widen(struct[name])
>>> ????????????? upto += 1
>>>
>>> -??????????? struct_name, aliases = get_compat_name(node)
>>> +??????????? struct_name, aliases =
>>> self.get_normalized_compat_name(node)
>>> ????????????? for alias in aliases:
>>> ????????????????? self._aliases[alias] = struct_name
>>>
>>> @@ -461,7 +535,7 @@ class DtbPlatdata(object):
>>> ????????? Args:
>>> ????????????? node: node to output
>>> ????????? """
>>> -??????? struct_name, _ = get_compat_name(node)
>>> +??????? struct_name, _ = self.get_normalized_compat_name(node)
>>> ????????? var_name = conv_name_to_c(node.name)
>>> ????????? self.buf('static const struct %s%s %s%s = {\n' %
>>> ?????????????????? (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
>>> @@ -562,6 +636,7 @@ def run_steps(args, dtb_file, include_disabled,
>>> output):
>>> ????????? raise ValueError('Please specify a command: struct,
>>> platdata')
>>>
>>> ????? plat = DtbPlatdata(dtb_file, include_disabled)
>>> +??? plat.scan_drivers()
>>> ????? plat.scan_dtb()
>>> ????? plat.scan_tree()
>>> ????? plat.scan_reg_sizes()
>>> --
>>> 2.20.1
>>>
>
> Thanks once again for your review
>
> Regards,
>
> Walter
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-11 16:15 ` Walter Lozano
@ 2020-06-11 16:44 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-11 16:44 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Thu, 11 Jun 2020 at 10:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 8/6/20 12:49, Walter Lozano wrote:
> > Hi Simon,
> >
> > On 4/6/20 12:59, Simon Glass wrote:
> >> Hi Walter,
> >>
> >> On Fri, 29 May 2020 at 12:15, Walter Lozano
> >> <walter.lozano@collabora.com> wrote:
> >>> Currently dtoc scans dtbs to convert them to struct platdata and
> >>> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >>> with the driver name, but at this moment the information used is the
> >>> compatible name present in the dtb. This causes that only nodes with
> >>> a compatible name that matches a driver name generate a working
> >>> entry.
> >>>
> >>> In order to improve this behaviour, this patch adds to dtoc the
> >>> capability of scan drivers source code to generate a list of valid
> >>> driver
> >>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >>> entry will try to use a name not valid.
> >>>
> >>> Additionally, in order to add more flexibility to the solution, adds
> >>> the
> >>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but
> >>> allows an
> >>> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >>> for the driver name based on its alias when it populates the
> >>> U_BOOT_DEVICE
> >>> entry.
> >>>
> >>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>> ---
> >>> include/dm/device.h | 7 ++++
> >>> tools/dtoc/dtb_platdata.py | 83
> >>> ++++++++++++++++++++++++++++++++++++--
> >>> 2 files changed, 86 insertions(+), 4 deletions(-)
> >>>
[..]
> >>> + """
> >>> + compat_c, aliases_c = get_compat_name(node)
> >>> + if compat_c not in self._drivers:
> >>> + compat_c_old = compat_c
> >>> + compat_c = self._driver_aliases.get(compat_c)
> >>> + if not compat_c:
> >>> + print('WARNING: the driver %s was not found in the
> >>> driver list' % (compat_c_old))
> >> This creates lots of warnings at present. Either we need a patch to
> >> clean up the differences in the source code, or we need to disable the
> >> warning.
> >
> >
> > Regarding this, maybe we should have a list of driver names we don't
> > expect to support, like simple_bus. For this to work probably the best
> > approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> > so each config can add their owns.
>
> I've been thinking about this issue, I think it would be better to only
> print the warning if build is issue with
>
> make V=1
>
> What do you think?
Can we not fix the warnings?
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-08 15:49 ` Walter Lozano
2020-06-11 16:15 ` Walter Lozano
@ 2020-06-11 16:45 ` Simon Glass
2020-06-11 17:11 ` Walter Lozano
1 sibling, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-11 16:45 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 4/6/20 12:59, Simon Glass wrote:
> > Hi Walter,
> >
> > On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Currently dtoc scans dtbs to convert them to struct platdata and
> >> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >> with the driver name, but at this moment the information used is the
> >> compatible name present in the dtb. This causes that only nodes with
> >> a compatible name that matches a driver name generate a working
> >> entry.
> >>
> >> In order to improve this behaviour, this patch adds to dtoc the
> >> capability of scan drivers source code to generate a list of valid driver
> >> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >> entry will try to use a name not valid.
> >>
> >> Additionally, in order to add more flexibility to the solution, adds the
> >> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> >> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >> for the driver name based on its alias when it populates the U_BOOT_DEVICE
> >> entry.
> >>
> >> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >> ---
> >> include/dm/device.h | 7 ++++
> >> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
> >> 2 files changed, 86 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/dm/device.h b/include/dm/device.h
> >> index 975eec5d0e..2cfe10766f 100644
> >> --- a/include/dm/device.h
> >> +++ b/include/dm/device.h
> >> @@ -282,6 +282,13 @@ struct driver {
> >> #define DM_GET_DRIVER(__name) \
> >> ll_entry_get(struct driver, __name, driver)
> >>
> >> +/**
> >> + * Declare a macro to state a alias for a driver name. This macro will
> >> + * produce no code but its information will be parsed by tools like
> >> + * dtoc
> >> + */
> >> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> >> +
> >> /**
> >> * dev_get_platdata() - Get the platform data for a device
> >> *
> >> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> >> index ecfe0624d1..23cfda2f88 100644
> >> --- a/tools/dtoc/dtb_platdata.py
> >> +++ b/tools/dtoc/dtb_platdata.py
> >> @@ -13,6 +13,8 @@ static data.
> >>
> >> import collections
> >> import copy
> >> +import os
> >> +import re
> >> import sys
> >>
> >> from dtoc import fdt
> >> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
> >> _include_disabled: true to include nodes marked status = "disabled"
> >> _outfile: The current output file (sys.stdout or a real file)
> >> _lines: Stashed list of output lines for outputting in the future
> >> + _aliases: Dict that hold aliases for compatible strings
> > key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
> > value: ...
> Noted.
> >> + _drivers: List of valid driver names found in drivers/
> >> + _driver_aliases: Dict that holds aliases for driver names
> > key:
> > vaue:
> OK.
> >
> >> """
> >> def __init__(self, dtb_fname, include_disabled):
> >> self._fdt = None
> >> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
> >> self._outfile = None
> >> self._lines = []
> >> self._aliases = {}
> >> + self._drivers = []
> >> + self._driver_aliases = {}
> >> +
> >> + def get_normalized_compat_name(self, node):
> >> + """Get a node's normalized compat name
> >> +
> >> + Returns a valid driver name by retrieving node's first compatible
> >> + string as a C identifier and perfomrming a check against _drivers
> > performing
> Noted.
> >
> >> + and a lookup in driver_aliases rising a warning in case of failure.
> > s/ rising/, printing/
> OK.
> >
> >> +
> >> + Args:
> >> + node: Node object to check
> >> + Return:
> >> + Tuple:
> >> + Driver name associated with the first compatible string
> >> + List of C identifiers for all the other compatible strings
> >> + (possibly empty)
> > Can you update this comment to explain what is returned when it is not found?
> Sure.
> >> + """
> >> + compat_c, aliases_c = get_compat_name(node)
> >> + if compat_c not in self._drivers:
> >> + compat_c_old = compat_c
> >> + compat_c = self._driver_aliases.get(compat_c)
> >> + if not compat_c:
> >> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> > This creates lots of warnings at present. Either we need a patch to
> > clean up the differences in the source code, or we need to disable the
> > warning.
>
>
> Regarding this, maybe we should have a list of driver names we don't
> expect to support, like simple_bus. For this to work probably the best
> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> so each config can add their owns.
Or perhaps have another macro in the source code that indicates that
the driver cannot be used with of-platdata and should be ignored?
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-11 16:45 ` Simon Glass
@ 2020-06-11 17:11 ` Walter Lozano
2020-06-11 17:22 ` Simon Glass
0 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-06-11 17:11 UTC (permalink / raw)
To: u-boot
Hi Simon
On 11/6/20 13:45, Simon Glass wrote:
> Hi Walter,
>
> On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon,
>>
>> On 4/6/20 12:59, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Currently dtoc scans dtbs to convert them to struct platdata and
>>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>>>> with the driver name, but at this moment the information used is the
>>>> compatible name present in the dtb. This causes that only nodes with
>>>> a compatible name that matches a driver name generate a working
>>>> entry.
>>>>
>>>> In order to improve this behaviour, this patch adds to dtoc the
>>>> capability of scan drivers source code to generate a list of valid driver
>>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>>>> entry will try to use a name not valid.
>>>>
>>>> Additionally, in order to add more flexibility to the solution, adds the
>>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
>>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
>>>> entry.
>>>>
>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>> ---
>>>> include/dm/device.h | 7 ++++
>>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
>>>> 2 files changed, 86 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>>> index 975eec5d0e..2cfe10766f 100644
>>>> --- a/include/dm/device.h
>>>> +++ b/include/dm/device.h
>>>> @@ -282,6 +282,13 @@ struct driver {
>>>> #define DM_GET_DRIVER(__name) \
>>>> ll_entry_get(struct driver, __name, driver)
>>>>
>>>> +/**
>>>> + * Declare a macro to state a alias for a driver name. This macro will
>>>> + * produce no code but its information will be parsed by tools like
>>>> + * dtoc
>>>> + */
>>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>>>> +
>>>> /**
>>>> * dev_get_platdata() - Get the platform data for a device
>>>> *
>>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>>>> index ecfe0624d1..23cfda2f88 100644
>>>> --- a/tools/dtoc/dtb_platdata.py
>>>> +++ b/tools/dtoc/dtb_platdata.py
>>>> @@ -13,6 +13,8 @@ static data.
>>>>
>>>> import collections
>>>> import copy
>>>> +import os
>>>> +import re
>>>> import sys
>>>>
>>>> from dtoc import fdt
>>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
>>>> _include_disabled: true to include nodes marked status = "disabled"
>>>> _outfile: The current output file (sys.stdout or a real file)
>>>> _lines: Stashed list of output lines for outputting in the future
>>>> + _aliases: Dict that hold aliases for compatible strings
>>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
>>> value: ...
>> Noted.
>>>> + _drivers: List of valid driver names found in drivers/
>>>> + _driver_aliases: Dict that holds aliases for driver names
>>> key:
>>> vaue:
>> OK.
>>>> """
>>>> def __init__(self, dtb_fname, include_disabled):
>>>> self._fdt = None
>>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
>>>> self._outfile = None
>>>> self._lines = []
>>>> self._aliases = {}
>>>> + self._drivers = []
>>>> + self._driver_aliases = {}
>>>> +
>>>> + def get_normalized_compat_name(self, node):
>>>> + """Get a node's normalized compat name
>>>> +
>>>> + Returns a valid driver name by retrieving node's first compatible
>>>> + string as a C identifier and perfomrming a check against _drivers
>>> performing
>> Noted.
>>>> + and a lookup in driver_aliases rising a warning in case of failure.
>>> s/ rising/, printing/
>> OK.
>>>> +
>>>> + Args:
>>>> + node: Node object to check
>>>> + Return:
>>>> + Tuple:
>>>> + Driver name associated with the first compatible string
>>>> + List of C identifiers for all the other compatible strings
>>>> + (possibly empty)
>>> Can you update this comment to explain what is returned when it is not found?
>> Sure.
>>>> + """
>>>> + compat_c, aliases_c = get_compat_name(node)
>>>> + if compat_c not in self._drivers:
>>>> + compat_c_old = compat_c
>>>> + compat_c = self._driver_aliases.get(compat_c)
>>>> + if not compat_c:
>>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
>>> This creates lots of warnings at present. Either we need a patch to
>>> clean up the differences in the source code, or we need to disable the
>>> warning.
>>
>> Regarding this, maybe we should have a list of driver names we don't
>> expect to support, like simple_bus. For this to work probably the best
>> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
>> so each config can add their owns.
> Or perhaps have another macro in the source code that indicates that
> the driver cannot be used with of-platdata and should be ignored?
I don't fully understand your idea. As I see, the warning should help to
spot that you will be trying to create a U_BOOT_DEVICE without a proper
driver name, which means that compatible string does not match a driver
name. The most probably reason for this is that driver doesn't fully
support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
From my understanding by adding a another macro to indicate that a
driver cannot be used, or even better to add a macro which tells that a
driver supports of-platdata, will give us a cleaner dt-struct, which
will be nice, however my first sentence still makes sense.
Could you clarify?
Also I have mentioned
> > I've been thinking about this issue, I think it would be better to only
> > print the warning if build is issue with
> >
> > make V=1
> >
> > What do you think?
> Can we not fix the warnings?
So it is not clear to me which idea is better from your point if view.
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-11 17:11 ` Walter Lozano
@ 2020-06-11 17:22 ` Simon Glass
2020-06-11 19:07 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-11 17:22 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Thu, 11 Jun 2020 at 11:11, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon
>
> On 11/6/20 13:45, Simon Glass wrote:
> > Hi Walter,
> >
> > On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon,
> >>
> >> On 4/6/20 12:59, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Currently dtoc scans dtbs to convert them to struct platdata and
> >>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >>>> with the driver name, but at this moment the information used is the
> >>>> compatible name present in the dtb. This causes that only nodes with
> >>>> a compatible name that matches a driver name generate a working
> >>>> entry.
> >>>>
> >>>> In order to improve this behaviour, this patch adds to dtoc the
> >>>> capability of scan drivers source code to generate a list of valid driver
> >>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >>>> entry will try to use a name not valid.
> >>>>
> >>>> Additionally, in order to add more flexibility to the solution, adds the
> >>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> >>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
> >>>> entry.
> >>>>
> >>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>> ---
> >>>> include/dm/device.h | 7 ++++
> >>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
> >>>> 2 files changed, 86 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>>> index 975eec5d0e..2cfe10766f 100644
> >>>> --- a/include/dm/device.h
> >>>> +++ b/include/dm/device.h
> >>>> @@ -282,6 +282,13 @@ struct driver {
> >>>> #define DM_GET_DRIVER(__name) \
> >>>> ll_entry_get(struct driver, __name, driver)
> >>>>
> >>>> +/**
> >>>> + * Declare a macro to state a alias for a driver name. This macro will
> >>>> + * produce no code but its information will be parsed by tools like
> >>>> + * dtoc
> >>>> + */
> >>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> >>>> +
> >>>> /**
> >>>> * dev_get_platdata() - Get the platform data for a device
> >>>> *
> >>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> >>>> index ecfe0624d1..23cfda2f88 100644
> >>>> --- a/tools/dtoc/dtb_platdata.py
> >>>> +++ b/tools/dtoc/dtb_platdata.py
> >>>> @@ -13,6 +13,8 @@ static data.
> >>>>
> >>>> import collections
> >>>> import copy
> >>>> +import os
> >>>> +import re
> >>>> import sys
> >>>>
> >>>> from dtoc import fdt
> >>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
> >>>> _include_disabled: true to include nodes marked status = "disabled"
> >>>> _outfile: The current output file (sys.stdout or a real file)
> >>>> _lines: Stashed list of output lines for outputting in the future
> >>>> + _aliases: Dict that hold aliases for compatible strings
> >>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
> >>> value: ...
> >> Noted.
> >>>> + _drivers: List of valid driver names found in drivers/
> >>>> + _driver_aliases: Dict that holds aliases for driver names
> >>> key:
> >>> vaue:
> >> OK.
> >>>> """
> >>>> def __init__(self, dtb_fname, include_disabled):
> >>>> self._fdt = None
> >>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
> >>>> self._outfile = None
> >>>> self._lines = []
> >>>> self._aliases = {}
> >>>> + self._drivers = []
> >>>> + self._driver_aliases = {}
> >>>> +
> >>>> + def get_normalized_compat_name(self, node):
> >>>> + """Get a node's normalized compat name
> >>>> +
> >>>> + Returns a valid driver name by retrieving node's first compatible
> >>>> + string as a C identifier and perfomrming a check against _drivers
> >>> performing
> >> Noted.
> >>>> + and a lookup in driver_aliases rising a warning in case of failure.
> >>> s/ rising/, printing/
> >> OK.
> >>>> +
> >>>> + Args:
> >>>> + node: Node object to check
> >>>> + Return:
> >>>> + Tuple:
> >>>> + Driver name associated with the first compatible string
> >>>> + List of C identifiers for all the other compatible strings
> >>>> + (possibly empty)
> >>> Can you update this comment to explain what is returned when it is not found?
> >> Sure.
> >>>> + """
> >>>> + compat_c, aliases_c = get_compat_name(node)
> >>>> + if compat_c not in self._drivers:
> >>>> + compat_c_old = compat_c
> >>>> + compat_c = self._driver_aliases.get(compat_c)
> >>>> + if not compat_c:
> >>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> >>> This creates lots of warnings at present. Either we need a patch to
> >>> clean up the differences in the source code, or we need to disable the
> >>> warning.
> >>
> >> Regarding this, maybe we should have a list of driver names we don't
> >> expect to support, like simple_bus. For this to work probably the best
> >> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> >> so each config can add their owns.
> > Or perhaps have another macro in the source code that indicates that
> > the driver cannot be used with of-platdata and should be ignored?
>
> I don't fully understand your idea. As I see, the warning should help to
> spot that you will be trying to create a U_BOOT_DEVICE without a proper
> driver name, which means that compatible string does not match a driver
> name. The most probably reason for this is that driver doesn't fully
> support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
>
> From my understanding by adding a another macro to indicate that a
> driver cannot be used, or even better to add a macro which tells that a
> driver supports of-platdata, will give us a cleaner dt-struct, which
> will be nice, however my first sentence still makes sense.
>
> Could you clarify?
I just mean that you should fix all the warnings, so that none are
printed in the normal case. Then people can see the problems they
create. Perhaps then it could even be an error rather than a warning?
>
> Also I have mentioned
>
> > > I've been thinking about this issue, I think it would be better to only
> > > print the warning if build is issue with
> > >
> > > make V=1
> > >
> > > What do you think?
>
> > Can we not fix the warnings?
>
> So it is not clear to me which idea is better from your point if view.
I don't think V=1 helps. Then you suppress the warning for most
people. If it isn't important, we shouldn't print it. But see above -
hoping these warnings can be fixed by renaming drivers.
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-11 17:22 ` Simon Glass
@ 2020-06-11 19:07 ` Walter Lozano
2020-06-12 2:22 ` Simon Glass
0 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-06-11 19:07 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 11/6/20 14:22, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 11 Jun 2020 at 11:11, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon
>>
>> On 11/6/20 13:45, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 4/6/20 12:59, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>> Currently dtoc scans dtbs to convert them to struct platdata and
>>>>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>>>>>> with the driver name, but at this moment the information used is the
>>>>>> compatible name present in the dtb. This causes that only nodes with
>>>>>> a compatible name that matches a driver name generate a working
>>>>>> entry.
>>>>>>
>>>>>> In order to improve this behaviour, this patch adds to dtoc the
>>>>>> capability of scan drivers source code to generate a list of valid driver
>>>>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>>>>>> entry will try to use a name not valid.
>>>>>>
>>>>>> Additionally, in order to add more flexibility to the solution, adds the
>>>>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
>>>>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>>>>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
>>>>>> entry.
>>>>>>
>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>>>> ---
>>>>>> include/dm/device.h | 7 ++++
>>>>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
>>>>>> 2 files changed, 86 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>>>>> index 975eec5d0e..2cfe10766f 100644
>>>>>> --- a/include/dm/device.h
>>>>>> +++ b/include/dm/device.h
>>>>>> @@ -282,6 +282,13 @@ struct driver {
>>>>>> #define DM_GET_DRIVER(__name) \
>>>>>> ll_entry_get(struct driver, __name, driver)
>>>>>>
>>>>>> +/**
>>>>>> + * Declare a macro to state a alias for a driver name. This macro will
>>>>>> + * produce no code but its information will be parsed by tools like
>>>>>> + * dtoc
>>>>>> + */
>>>>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>>>>>> +
>>>>>> /**
>>>>>> * dev_get_platdata() - Get the platform data for a device
>>>>>> *
>>>>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>>>>>> index ecfe0624d1..23cfda2f88 100644
>>>>>> --- a/tools/dtoc/dtb_platdata.py
>>>>>> +++ b/tools/dtoc/dtb_platdata.py
>>>>>> @@ -13,6 +13,8 @@ static data.
>>>>>>
>>>>>> import collections
>>>>>> import copy
>>>>>> +import os
>>>>>> +import re
>>>>>> import sys
>>>>>>
>>>>>> from dtoc import fdt
>>>>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
>>>>>> _include_disabled: true to include nodes marked status = "disabled"
>>>>>> _outfile: The current output file (sys.stdout or a real file)
>>>>>> _lines: Stashed list of output lines for outputting in the future
>>>>>> + _aliases: Dict that hold aliases for compatible strings
>>>>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
>>>>> value: ...
>>>> Noted.
>>>>>> + _drivers: List of valid driver names found in drivers/
>>>>>> + _driver_aliases: Dict that holds aliases for driver names
>>>>> key:
>>>>> vaue:
>>>> OK.
>>>>>> """
>>>>>> def __init__(self, dtb_fname, include_disabled):
>>>>>> self._fdt = None
>>>>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
>>>>>> self._outfile = None
>>>>>> self._lines = []
>>>>>> self._aliases = {}
>>>>>> + self._drivers = []
>>>>>> + self._driver_aliases = {}
>>>>>> +
>>>>>> + def get_normalized_compat_name(self, node):
>>>>>> + """Get a node's normalized compat name
>>>>>> +
>>>>>> + Returns a valid driver name by retrieving node's first compatible
>>>>>> + string as a C identifier and perfomrming a check against _drivers
>>>>> performing
>>>> Noted.
>>>>>> + and a lookup in driver_aliases rising a warning in case of failure.
>>>>> s/ rising/, printing/
>>>> OK.
>>>>>> +
>>>>>> + Args:
>>>>>> + node: Node object to check
>>>>>> + Return:
>>>>>> + Tuple:
>>>>>> + Driver name associated with the first compatible string
>>>>>> + List of C identifiers for all the other compatible strings
>>>>>> + (possibly empty)
>>>>> Can you update this comment to explain what is returned when it is not found?
>>>> Sure.
>>>>>> + """
>>>>>> + compat_c, aliases_c = get_compat_name(node)
>>>>>> + if compat_c not in self._drivers:
>>>>>> + compat_c_old = compat_c
>>>>>> + compat_c = self._driver_aliases.get(compat_c)
>>>>>> + if not compat_c:
>>>>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
>>>>> This creates lots of warnings at present. Either we need a patch to
>>>>> clean up the differences in the source code, or we need to disable the
>>>>> warning.
>>>> Regarding this, maybe we should have a list of driver names we don't
>>>> expect to support, like simple_bus. For this to work probably the best
>>>> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
>>>> so each config can add their owns.
>>> Or perhaps have another macro in the source code that indicates that
>>> the driver cannot be used with of-platdata and should be ignored?
>> I don't fully understand your idea. As I see, the warning should help to
>> spot that you will be trying to create a U_BOOT_DEVICE without a proper
>> driver name, which means that compatible string does not match a driver
>> name. The most probably reason for this is that driver doesn't fully
>> support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
>>
>> From my understanding by adding a another macro to indicate that a
>> driver cannot be used, or even better to add a macro which tells that a
>> driver supports of-platdata, will give us a cleaner dt-struct, which
>> will be nice, however my first sentence still makes sense.
>>
>> Could you clarify?
> I just mean that you should fix all the warnings, so that none are
> printed in the normal case. Then people can see the problems they
> create. Perhaps then it could even be an error rather than a warning?
>
Thanks for taking the time to explain your point. Let me put an example
in order to check if we agree.
Currently, using sandbox_spl_defconfig several warnings arise, for instance
WARNING: the driver sandbox_serial was not found in the driver list
the driver is driver/serial/sandbox.c
The reason for this warning is that in sandbox_serial is not declared
neither as a driver nor as an alias. In this case, this device won't
work with of-platdata as it could not be bound. Am I correct?
To disable the warning is to rename the driver or to add an alias as
U_BOOT_DRIVER_ALIAS(serial_sandbox, sandbox_serial)
Would you like me to add U_BOOT_DRIVER_ALIAS for these kind of cases?
However removing the warning without properly testing the driver with
of-platdata might hide runtime issues, don't you think so?
Also, if you feel that this discussion will take time, I have no problem
in moving the warning to a different patchset, to avoid delay your work.
I totally open to your suggestions.
>> Also I have mentioned
>>
>>>> I've been thinking about this issue, I think it would be better to only
>>>> print the warning if build is issue with
>>>>
>>>> make V=1
>>>>
>>>> What do you think?
>>> Can we not fix the warnings?
>> So it is not clear to me which idea is better from your point if view.
> I don't think V=1 helps. Then you suppress the warning for most
> people. If it isn't important, we shouldn't print it. But see above -
> hoping these warnings can be fixed by renaming drivers.
>
Yes, I understand, thanks for clarifying?
Regards,
Walter
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-11 19:07 ` Walter Lozano
@ 2020-06-12 2:22 ` Simon Glass
2020-06-12 17:38 ` Walter Lozano
0 siblings, 1 reply; 42+ messages in thread
From: Simon Glass @ 2020-06-12 2:22 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Thu, 11 Jun 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 11/6/20 14:22, Simon Glass wrote:
> > Hi Walter,
> >
> > On Thu, 11 Jun 2020 at 11:11, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon
> >>
> >> On 11/6/20 13:45, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 4/6/20 12:59, Simon Glass wrote:
> >>>>> Hi Walter,
> >>>>>
> >>>>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>> Currently dtoc scans dtbs to convert them to struct platdata and
> >>>>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >>>>>> with the driver name, but at this moment the information used is the
> >>>>>> compatible name present in the dtb. This causes that only nodes with
> >>>>>> a compatible name that matches a driver name generate a working
> >>>>>> entry.
> >>>>>>
> >>>>>> In order to improve this behaviour, this patch adds to dtoc the
> >>>>>> capability of scan drivers source code to generate a list of valid driver
> >>>>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >>>>>> entry will try to use a name not valid.
> >>>>>>
> >>>>>> Additionally, in order to add more flexibility to the solution, adds the
> >>>>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> >>>>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >>>>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
> >>>>>> entry.
> >>>>>>
> >>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>>>> ---
> >>>>>> include/dm/device.h | 7 ++++
> >>>>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
> >>>>>> 2 files changed, 86 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>>>>> index 975eec5d0e..2cfe10766f 100644
> >>>>>> --- a/include/dm/device.h
> >>>>>> +++ b/include/dm/device.h
> >>>>>> @@ -282,6 +282,13 @@ struct driver {
> >>>>>> #define DM_GET_DRIVER(__name) \
> >>>>>> ll_entry_get(struct driver, __name, driver)
> >>>>>>
> >>>>>> +/**
> >>>>>> + * Declare a macro to state a alias for a driver name. This macro will
> >>>>>> + * produce no code but its information will be parsed by tools like
> >>>>>> + * dtoc
> >>>>>> + */
> >>>>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> >>>>>> +
> >>>>>> /**
> >>>>>> * dev_get_platdata() - Get the platform data for a device
> >>>>>> *
> >>>>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> >>>>>> index ecfe0624d1..23cfda2f88 100644
> >>>>>> --- a/tools/dtoc/dtb_platdata.py
> >>>>>> +++ b/tools/dtoc/dtb_platdata.py
> >>>>>> @@ -13,6 +13,8 @@ static data.
> >>>>>>
> >>>>>> import collections
> >>>>>> import copy
> >>>>>> +import os
> >>>>>> +import re
> >>>>>> import sys
> >>>>>>
> >>>>>> from dtoc import fdt
> >>>>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
> >>>>>> _include_disabled: true to include nodes marked status = "disabled"
> >>>>>> _outfile: The current output file (sys.stdout or a real file)
> >>>>>> _lines: Stashed list of output lines for outputting in the future
> >>>>>> + _aliases: Dict that hold aliases for compatible strings
> >>>>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
> >>>>> value: ...
> >>>> Noted.
> >>>>>> + _drivers: List of valid driver names found in drivers/
> >>>>>> + _driver_aliases: Dict that holds aliases for driver names
> >>>>> key:
> >>>>> vaue:
> >>>> OK.
> >>>>>> """
> >>>>>> def __init__(self, dtb_fname, include_disabled):
> >>>>>> self._fdt = None
> >>>>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
> >>>>>> self._outfile = None
> >>>>>> self._lines = []
> >>>>>> self._aliases = {}
> >>>>>> + self._drivers = []
> >>>>>> + self._driver_aliases = {}
> >>>>>> +
> >>>>>> + def get_normalized_compat_name(self, node):
> >>>>>> + """Get a node's normalized compat name
> >>>>>> +
> >>>>>> + Returns a valid driver name by retrieving node's first compatible
> >>>>>> + string as a C identifier and perfomrming a check against _drivers
> >>>>> performing
> >>>> Noted.
> >>>>>> + and a lookup in driver_aliases rising a warning in case of failure.
> >>>>> s/ rising/, printing/
> >>>> OK.
> >>>>>> +
> >>>>>> + Args:
> >>>>>> + node: Node object to check
> >>>>>> + Return:
> >>>>>> + Tuple:
> >>>>>> + Driver name associated with the first compatible string
> >>>>>> + List of C identifiers for all the other compatible strings
> >>>>>> + (possibly empty)
> >>>>> Can you update this comment to explain what is returned when it is not found?
> >>>> Sure.
> >>>>>> + """
> >>>>>> + compat_c, aliases_c = get_compat_name(node)
> >>>>>> + if compat_c not in self._drivers:
> >>>>>> + compat_c_old = compat_c
> >>>>>> + compat_c = self._driver_aliases.get(compat_c)
> >>>>>> + if not compat_c:
> >>>>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> >>>>> This creates lots of warnings at present. Either we need a patch to
> >>>>> clean up the differences in the source code, or we need to disable the
> >>>>> warning.
> >>>> Regarding this, maybe we should have a list of driver names we don't
> >>>> expect to support, like simple_bus. For this to work probably the best
> >>>> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> >>>> so each config can add their owns.
> >>> Or perhaps have another macro in the source code that indicates that
> >>> the driver cannot be used with of-platdata and should be ignored?
> >> I don't fully understand your idea. As I see, the warning should help to
> >> spot that you will be trying to create a U_BOOT_DEVICE without a proper
> >> driver name, which means that compatible string does not match a driver
> >> name. The most probably reason for this is that driver doesn't fully
> >> support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
> >>
> >> From my understanding by adding a another macro to indicate that a
> >> driver cannot be used, or even better to add a macro which tells that a
> >> driver supports of-platdata, will give us a cleaner dt-struct, which
> >> will be nice, however my first sentence still makes sense.
> >>
> >> Could you clarify?
> > I just mean that you should fix all the warnings, so that none are
> > printed in the normal case. Then people can see the problems they
> > create. Perhaps then it could even be an error rather than a warning?
> >
> Thanks for taking the time to explain your point. Let me put an example
> in order to check if we agree.
>
> Currently, using sandbox_spl_defconfig several warnings arise, for instance
>
> WARNING: the driver sandbox_serial was not found in the driver list
>
> the driver is driver/serial/sandbox.c
>
> The reason for this warning is that in sandbox_serial is not declared
> neither as a driver nor as an alias. In this case, this device won't
> work with of-platdata as it could not be bound. Am I correct?
>
> To disable the warning is to rename the driver or to add an alias as
>
> U_BOOT_DRIVER_ALIAS(serial_sandbox, sandbox_serial)
>
> Would you like me to add U_BOOT_DRIVER_ALIAS for these kind of cases?
I think it would be better to rename the driver. The names are a bit
arbitrary anyway at present.
>
> However removing the warning without properly testing the driver with
> of-platdata might hide runtime issues, don't you think so?
Well you can only make it better, I suspect, since you are correcting the name.
>
> Also, if you feel that this discussion will take time, I have no problem
> in moving the warning to a different patchset, to avoid delay your work.
> I totally open to your suggestions.
Sure I suppose we could start with what you have, with the warnings,
and then submit a fixup afterwards.
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-12 2:22 ` Simon Glass
@ 2020-06-12 17:38 ` Walter Lozano
2020-06-16 13:43 ` Simon Glass
0 siblings, 1 reply; 42+ messages in thread
From: Walter Lozano @ 2020-06-12 17:38 UTC (permalink / raw)
To: u-boot
On 11/6/20 23:22, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 11 Jun 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon,
>>
>> On 11/6/20 14:22, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Thu, 11 Jun 2020 at 11:11, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Hi Simon
>>>>
>>>> On 11/6/20 13:45, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 4/6/20 12:59, Simon Glass wrote:
>>>>>>> Hi Walter,
>>>>>>>
>>>>>>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>>>> Currently dtoc scans dtbs to convert them to struct platdata and
>>>>>>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
>>>>>>>> with the driver name, but at this moment the information used is the
>>>>>>>> compatible name present in the dtb. This causes that only nodes with
>>>>>>>> a compatible name that matches a driver name generate a working
>>>>>>>> entry.
>>>>>>>>
>>>>>>>> In order to improve this behaviour, this patch adds to dtoc the
>>>>>>>> capability of scan drivers source code to generate a list of valid driver
>>>>>>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
>>>>>>>> entry will try to use a name not valid.
>>>>>>>>
>>>>>>>> Additionally, in order to add more flexibility to the solution, adds the
>>>>>>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
>>>>>>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
>>>>>>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
>>>>>>>> entry.
>>>>>>>>
>>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>>>>>> ---
>>>>>>>> include/dm/device.h | 7 ++++
>>>>>>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
>>>>>>>> 2 files changed, 86 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>>>>>>> index 975eec5d0e..2cfe10766f 100644
>>>>>>>> --- a/include/dm/device.h
>>>>>>>> +++ b/include/dm/device.h
>>>>>>>> @@ -282,6 +282,13 @@ struct driver {
>>>>>>>> #define DM_GET_DRIVER(__name) \
>>>>>>>> ll_entry_get(struct driver, __name, driver)
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * Declare a macro to state a alias for a driver name. This macro will
>>>>>>>> + * produce no code but its information will be parsed by tools like
>>>>>>>> + * dtoc
>>>>>>>> + */
>>>>>>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
>>>>>>>> +
>>>>>>>> /**
>>>>>>>> * dev_get_platdata() - Get the platform data for a device
>>>>>>>> *
>>>>>>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>>>>>>>> index ecfe0624d1..23cfda2f88 100644
>>>>>>>> --- a/tools/dtoc/dtb_platdata.py
>>>>>>>> +++ b/tools/dtoc/dtb_platdata.py
>>>>>>>> @@ -13,6 +13,8 @@ static data.
>>>>>>>>
>>>>>>>> import collections
>>>>>>>> import copy
>>>>>>>> +import os
>>>>>>>> +import re
>>>>>>>> import sys
>>>>>>>>
>>>>>>>> from dtoc import fdt
>>>>>>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
>>>>>>>> _include_disabled: true to include nodes marked status = "disabled"
>>>>>>>> _outfile: The current output file (sys.stdout or a real file)
>>>>>>>> _lines: Stashed list of output lines for outputting in the future
>>>>>>>> + _aliases: Dict that hold aliases for compatible strings
>>>>>>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
>>>>>>> value: ...
>>>>>> Noted.
>>>>>>>> + _drivers: List of valid driver names found in drivers/
>>>>>>>> + _driver_aliases: Dict that holds aliases for driver names
>>>>>>> key:
>>>>>>> vaue:
>>>>>> OK.
>>>>>>>> """
>>>>>>>> def __init__(self, dtb_fname, include_disabled):
>>>>>>>> self._fdt = None
>>>>>>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
>>>>>>>> self._outfile = None
>>>>>>>> self._lines = []
>>>>>>>> self._aliases = {}
>>>>>>>> + self._drivers = []
>>>>>>>> + self._driver_aliases = {}
>>>>>>>> +
>>>>>>>> + def get_normalized_compat_name(self, node):
>>>>>>>> + """Get a node's normalized compat name
>>>>>>>> +
>>>>>>>> + Returns a valid driver name by retrieving node's first compatible
>>>>>>>> + string as a C identifier and perfomrming a check against _drivers
>>>>>>> performing
>>>>>> Noted.
>>>>>>>> + and a lookup in driver_aliases rising a warning in case of failure.
>>>>>>> s/ rising/, printing/
>>>>>> OK.
>>>>>>>> +
>>>>>>>> + Args:
>>>>>>>> + node: Node object to check
>>>>>>>> + Return:
>>>>>>>> + Tuple:
>>>>>>>> + Driver name associated with the first compatible string
>>>>>>>> + List of C identifiers for all the other compatible strings
>>>>>>>> + (possibly empty)
>>>>>>> Can you update this comment to explain what is returned when it is not found?
>>>>>> Sure.
>>>>>>>> + """
>>>>>>>> + compat_c, aliases_c = get_compat_name(node)
>>>>>>>> + if compat_c not in self._drivers:
>>>>>>>> + compat_c_old = compat_c
>>>>>>>> + compat_c = self._driver_aliases.get(compat_c)
>>>>>>>> + if not compat_c:
>>>>>>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
>>>>>>> This creates lots of warnings at present. Either we need a patch to
>>>>>>> clean up the differences in the source code, or we need to disable the
>>>>>>> warning.
>>>>>> Regarding this, maybe we should have a list of driver names we don't
>>>>>> expect to support, like simple_bus. For this to work probably the best
>>>>>> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
>>>>>> so each config can add their owns.
>>>>> Or perhaps have another macro in the source code that indicates that
>>>>> the driver cannot be used with of-platdata and should be ignored?
>>>> I don't fully understand your idea. As I see, the warning should help to
>>>> spot that you will be trying to create a U_BOOT_DEVICE without a proper
>>>> driver name, which means that compatible string does not match a driver
>>>> name. The most probably reason for this is that driver doesn't fully
>>>> support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
>>>>
>>>> From my understanding by adding a another macro to indicate that a
>>>> driver cannot be used, or even better to add a macro which tells that a
>>>> driver supports of-platdata, will give us a cleaner dt-struct, which
>>>> will be nice, however my first sentence still makes sense.
>>>>
>>>> Could you clarify?
>>> I just mean that you should fix all the warnings, so that none are
>>> printed in the normal case. Then people can see the problems they
>>> create. Perhaps then it could even be an error rather than a warning?
>>>
>> Thanks for taking the time to explain your point. Let me put an example
>> in order to check if we agree.
>>
>> Currently, using sandbox_spl_defconfig several warnings arise, for instance
>>
>> WARNING: the driver sandbox_serial was not found in the driver list
>>
>> the driver is driver/serial/sandbox.c
>>
>> The reason for this warning is that in sandbox_serial is not declared
>> neither as a driver nor as an alias. In this case, this device won't
>> work with of-platdata as it could not be bound. Am I correct?
>>
>> To disable the warning is to rename the driver or to add an alias as
>>
>> U_BOOT_DRIVER_ALIAS(serial_sandbox, sandbox_serial)
>>
>> Would you like me to add U_BOOT_DRIVER_ALIAS for these kind of cases?
> I think it would be better to rename the driver. The names are a bit
> arbitrary anyway at present.
>
Yes, I agree. Actually I rename some drivers for iMX6 for that reason.
Let me share some examples in order to check if we agree
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
index 3d96678a45..8cc288581c 100644
--- a/drivers/gpio/rk_gpio.c
+++ b/drivers/gpio/rk_gpio.c
@@ -172,8 +172,8 @@ static const struct udevice_id rockchip_gpio_ids[] = {
{ }
};
-U_BOOT_DRIVER(gpio_rockchip) = {
- .name = "gpio_rockchip",
+U_BOOT_DRIVER(rockchip_gpio_bank) = {
+ .name = "rockchip_gpio_bank",
.id = UCLASS_GPIO,
.of_match = rockchip_gpio_ids,
.ops = &gpio_rockchip_ops,
diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
index 9549c74c2b..ff46d3c8d1 100644
--- a/drivers/gpio/sandbox.c
+++ b/drivers/gpio/sandbox.c
@@ -243,8 +243,8 @@ static const struct udevice_id sandbox_gpio_ids[] = {
{ }
};
-U_BOOT_DRIVER(gpio_sandbox) = {
- .name = "gpio_sandbox",
+U_BOOT_DRIVER(sandbox_gpio) = {
+ .name = "sandbox_gpio",
.id = UCLASS_GPIO,
.of_match = sandbox_gpio_ids,
.ofdata_to_platdata = sandbox_gpio_ofdata_to_platdata,
diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
index 7ae147f304..c617d21b7a 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
@@ -240,7 +240,7 @@ static const struct udevice_id rk3288_pinctrl_ids[] = {
{ }
};
-U_BOOT_DRIVER(pinctrl_rk3288) = {
+U_BOOT_DRIVER(rockchip_rk3288_pinctrl) = {
.name = "rockchip_rk3288_pinctrl",
.id = UCLASS_PINCTRL,
.of_match = rk3288_pinctrl_ids,
diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
index 52e6d9d8c0..d870ed7113 100644
--- a/drivers/power/pmic/rk8xx.c
+++ b/drivers/power/pmic/rk8xx.c
@@ -182,8 +182,8 @@ static const struct udevice_id rk8xx_ids[] = {
{ }
};
-U_BOOT_DRIVER(pmic_rk8xx) = {
- .name = "rk8xx pmic",
+U_BOOT_DRIVER(rockchip_rk805) = {
+ .name = "rockchip_rk805",
.id = UCLASS_PMIC,
.of_match = rk8xx_ids,
#if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>> However removing the warning without properly testing the driver with
>> of-platdata might hide runtime issues, don't you think so?
> Well you can only make it better, I suspect, since you are correcting the name.
>> Also, if you feel that this discussion will take time, I have no problem
>> in moving the warning to a different patchset, to avoid delay your work.
>> I totally open to your suggestions.
> Sure I suppose we could start with what you have, with the warnings,
> and then submit a fixup afterwards.
>
I have no problem, let's see if we can agree in this patchset in order
to keep improving things.
Regards.
Walter
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios
2020-06-08 16:01 ` Walter Lozano
@ 2020-06-14 2:49 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-14 2:49 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Mon, 8 Jun 2020 at 10:01, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon
>
> On 4/6/20 12:59, Simon Glass wrote:
> > On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Currently dtoc does not support the property cd-gpios used to declare
> >> the gpios for card detect in mmc.
> >>
> >> This patch adds support to cd-gpios property.
> >>
> >> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >> ---
> >> tools/dtoc/dtb_platdata.py | 13 ++++++++-----
> >> tools/dtoc/test_dtoc.py | 2 +-
> >> 2 files changed, 9 insertions(+), 6 deletions(-)
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But I think you should mention GPIOs, rather than just cd-gpios, as it
> > seems this will add support for any GPIO.
>
> Sorry, probably I am missing something, but in order to support GPIOs in
> general we need to support a set of properties such as "cs-gpios",
> "rb-gpios", "gpios" among others.
>
> Do you agree?
OK yes I see. I was thinking of supporting any *-gpios but I think
what you have here is better for now.
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/10] dtoc: add support to scan drivers
2020-06-12 17:38 ` Walter Lozano
@ 2020-06-16 13:43 ` Simon Glass
0 siblings, 0 replies; 42+ messages in thread
From: Simon Glass @ 2020-06-16 13:43 UTC (permalink / raw)
To: u-boot
Hi Walter,
On Fri, 12 Jun 2020 at 11:38, Walter Lozano <walter.lozano@collabora.com> wrote:
>
>
> On 11/6/20 23:22, Simon Glass wrote:
> > Hi Walter,
> >
> > On Thu, 11 Jun 2020 at 13:07, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon,
> >>
> >> On 11/6/20 14:22, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Thu, 11 Jun 2020 at 11:11, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Hi Simon
> >>>>
> >>>> On 11/6/20 13:45, Simon Glass wrote:
> >>>>> Hi Walter,
> >>>>>
> >>>>> On Mon, 8 Jun 2020 at 09:49, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 4/6/20 12:59, Simon Glass wrote:
> >>>>>>> Hi Walter,
> >>>>>>>
> >>>>>>> On Fri, 29 May 2020 at 12:15, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>>>> Currently dtoc scans dtbs to convert them to struct platdata and
> >>>>>>>> to generate U_BOOT_DEVICE entries. These entries need to be filled
> >>>>>>>> with the driver name, but at this moment the information used is the
> >>>>>>>> compatible name present in the dtb. This causes that only nodes with
> >>>>>>>> a compatible name that matches a driver name generate a working
> >>>>>>>> entry.
> >>>>>>>>
> >>>>>>>> In order to improve this behaviour, this patch adds to dtoc the
> >>>>>>>> capability of scan drivers source code to generate a list of valid driver
> >>>>>>>> names. This allows to rise a warning in the case that an U_BOOT_DEVICE
> >>>>>>>> entry will try to use a name not valid.
> >>>>>>>>
> >>>>>>>> Additionally, in order to add more flexibility to the solution, adds the
> >>>>>>>> U_BOOT_DRIVER_ALIAS macro, which generates no code at all, but allows an
> >>>>>>>> easy way to declare driver name aliases. Thanks to this, dtoc can look
> >>>>>>>> for the driver name based on its alias when it populates the U_BOOT_DEVICE
> >>>>>>>> entry.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>>>>>> ---
> >>>>>>>> include/dm/device.h | 7 ++++
> >>>>>>>> tools/dtoc/dtb_platdata.py | 83 ++++++++++++++++++++++++++++++++++++--
> >>>>>>>> 2 files changed, 86 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/include/dm/device.h b/include/dm/device.h
> >>>>>>>> index 975eec5d0e..2cfe10766f 100644
> >>>>>>>> --- a/include/dm/device.h
> >>>>>>>> +++ b/include/dm/device.h
> >>>>>>>> @@ -282,6 +282,13 @@ struct driver {
> >>>>>>>> #define DM_GET_DRIVER(__name) \
> >>>>>>>> ll_entry_get(struct driver, __name, driver)
> >>>>>>>>
> >>>>>>>> +/**
> >>>>>>>> + * Declare a macro to state a alias for a driver name. This macro will
> >>>>>>>> + * produce no code but its information will be parsed by tools like
> >>>>>>>> + * dtoc
> >>>>>>>> + */
> >>>>>>>> +#define U_BOOT_DRIVER_ALIAS(__name, __alias)
> >>>>>>>> +
> >>>>>>>> /**
> >>>>>>>> * dev_get_platdata() - Get the platform data for a device
> >>>>>>>> *
> >>>>>>>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> >>>>>>>> index ecfe0624d1..23cfda2f88 100644
> >>>>>>>> --- a/tools/dtoc/dtb_platdata.py
> >>>>>>>> +++ b/tools/dtoc/dtb_platdata.py
> >>>>>>>> @@ -13,6 +13,8 @@ static data.
> >>>>>>>>
> >>>>>>>> import collections
> >>>>>>>> import copy
> >>>>>>>> +import os
> >>>>>>>> +import re
> >>>>>>>> import sys
> >>>>>>>>
> >>>>>>>> from dtoc import fdt
> >>>>>>>> @@ -140,6 +142,9 @@ class DtbPlatdata(object):
> >>>>>>>> _include_disabled: true to include nodes marked status = "disabled"
> >>>>>>>> _outfile: The current output file (sys.stdout or a real file)
> >>>>>>>> _lines: Stashed list of output lines for outputting in the future
> >>>>>>>> + _aliases: Dict that hold aliases for compatible strings
> >>>>>>> key: The driver name, i.e. the part between brackets in U_BOOT_DRIVER(xx) ??
> >>>>>>> value: ...
> >>>>>> Noted.
> >>>>>>>> + _drivers: List of valid driver names found in drivers/
> >>>>>>>> + _driver_aliases: Dict that holds aliases for driver names
> >>>>>>> key:
> >>>>>>> vaue:
> >>>>>> OK.
> >>>>>>>> """
> >>>>>>>> def __init__(self, dtb_fname, include_disabled):
> >>>>>>>> self._fdt = None
> >>>>>>>> @@ -149,6 +154,35 @@ class DtbPlatdata(object):
> >>>>>>>> self._outfile = None
> >>>>>>>> self._lines = []
> >>>>>>>> self._aliases = {}
> >>>>>>>> + self._drivers = []
> >>>>>>>> + self._driver_aliases = {}
> >>>>>>>> +
> >>>>>>>> + def get_normalized_compat_name(self, node):
> >>>>>>>> + """Get a node's normalized compat name
> >>>>>>>> +
> >>>>>>>> + Returns a valid driver name by retrieving node's first compatible
> >>>>>>>> + string as a C identifier and perfomrming a check against _drivers
> >>>>>>> performing
> >>>>>> Noted.
> >>>>>>>> + and a lookup in driver_aliases rising a warning in case of failure.
> >>>>>>> s/ rising/, printing/
> >>>>>> OK.
> >>>>>>>> +
> >>>>>>>> + Args:
> >>>>>>>> + node: Node object to check
> >>>>>>>> + Return:
> >>>>>>>> + Tuple:
> >>>>>>>> + Driver name associated with the first compatible string
> >>>>>>>> + List of C identifiers for all the other compatible strings
> >>>>>>>> + (possibly empty)
> >>>>>>> Can you update this comment to explain what is returned when it is not found?
> >>>>>> Sure.
> >>>>>>>> + """
> >>>>>>>> + compat_c, aliases_c = get_compat_name(node)
> >>>>>>>> + if compat_c not in self._drivers:
> >>>>>>>> + compat_c_old = compat_c
> >>>>>>>> + compat_c = self._driver_aliases.get(compat_c)
> >>>>>>>> + if not compat_c:
> >>>>>>>> + print('WARNING: the driver %s was not found in the driver list' % (compat_c_old))
> >>>>>>> This creates lots of warnings at present. Either we need a patch to
> >>>>>>> clean up the differences in the source code, or we need to disable the
> >>>>>>> warning.
> >>>>>> Regarding this, maybe we should have a list of driver names we don't
> >>>>>> expect to support, like simple_bus. For this to work probably the best
> >>>>>> approach is to have a config option similar to CONFIG_OF_REMOVE_PROPS,
> >>>>>> so each config can add their owns.
> >>>>> Or perhaps have another macro in the source code that indicates that
> >>>>> the driver cannot be used with of-platdata and should be ignored?
> >>>> I don't fully understand your idea. As I see, the warning should help to
> >>>> spot that you will be trying to create a U_BOOT_DEVICE without a proper
> >>>> driver name, which means that compatible string does not match a driver
> >>>> name. The most probably reason for this is that driver doesn't fully
> >>>> support of-platdata, or at least an U_BOOT_DRIVER_ALIAS is missing.
> >>>>
> >>>> From my understanding by adding a another macro to indicate that a
> >>>> driver cannot be used, or even better to add a macro which tells that a
> >>>> driver supports of-platdata, will give us a cleaner dt-struct, which
> >>>> will be nice, however my first sentence still makes sense.
> >>>>
> >>>> Could you clarify?
> >>> I just mean that you should fix all the warnings, so that none are
> >>> printed in the normal case. Then people can see the problems they
> >>> create. Perhaps then it could even be an error rather than a warning?
> >>>
> >> Thanks for taking the time to explain your point. Let me put an example
> >> in order to check if we agree.
> >>
> >> Currently, using sandbox_spl_defconfig several warnings arise, for instance
> >>
> >> WARNING: the driver sandbox_serial was not found in the driver list
> >>
> >> the driver is driver/serial/sandbox.c
> >>
> >> The reason for this warning is that in sandbox_serial is not declared
> >> neither as a driver nor as an alias. In this case, this device won't
> >> work with of-platdata as it could not be bound. Am I correct?
> >>
> >> To disable the warning is to rename the driver or to add an alias as
> >>
> >> U_BOOT_DRIVER_ALIAS(serial_sandbox, sandbox_serial)
> >>
> >> Would you like me to add U_BOOT_DRIVER_ALIAS for these kind of cases?
> > I think it would be better to rename the driver. The names are a bit
> > arbitrary anyway at present.
> >
> Yes, I agree. Actually I rename some drivers for iMX6 for that reason.
> Let me share some examples in order to check if we agree
>
>
> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> index 3d96678a45..8cc288581c 100644
> --- a/drivers/gpio/rk_gpio.c
> +++ b/drivers/gpio/rk_gpio.c
> @@ -172,8 +172,8 @@ static const struct udevice_id rockchip_gpio_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(gpio_rockchip) = {
> - .name = "gpio_rockchip",
> +U_BOOT_DRIVER(rockchip_gpio_bank) = {
> + .name = "rockchip_gpio_bank",
> .id = UCLASS_GPIO,
> .of_match = rockchip_gpio_ids,
> .ops = &gpio_rockchip_ops,
> diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
> index 9549c74c2b..ff46d3c8d1 100644
> --- a/drivers/gpio/sandbox.c
> +++ b/drivers/gpio/sandbox.c
> @@ -243,8 +243,8 @@ static const struct udevice_id sandbox_gpio_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(gpio_sandbox) = {
> - .name = "gpio_sandbox",
> +U_BOOT_DRIVER(sandbox_gpio) = {
> + .name = "sandbox_gpio",
> .id = UCLASS_GPIO,
> .of_match = sandbox_gpio_ids,
> .ofdata_to_platdata = sandbox_gpio_ofdata_to_platdata,
> diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> index 7ae147f304..c617d21b7a 100644
> --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> @@ -240,7 +240,7 @@ static const struct udevice_id rk3288_pinctrl_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(pinctrl_rk3288) = {
> +U_BOOT_DRIVER(rockchip_rk3288_pinctrl) = {
> .name = "rockchip_rk3288_pinctrl",
> .id = UCLASS_PINCTRL,
> .of_match = rk3288_pinctrl_ids,
> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> index 52e6d9d8c0..d870ed7113 100644
> --- a/drivers/power/pmic/rk8xx.c
> +++ b/drivers/power/pmic/rk8xx.c
> @@ -182,8 +182,8 @@ static const struct udevice_id rk8xx_ids[] = {
> { }
> };
>
> -U_BOOT_DRIVER(pmic_rk8xx) = {
> - .name = "rk8xx pmic",
> +U_BOOT_DRIVER(rockchip_rk805) = {
> + .name = "rockchip_rk805",
> .id = UCLASS_PMIC,
> .of_match = rk8xx_ids,
> #if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>
Yes that seems OK.
>
> >> However removing the warning without properly testing the driver with
> >> of-platdata might hide runtime issues, don't you think so?
> > Well you can only make it better, I suspect, since you are correcting the name.
> >> Also, if you feel that this discussion will take time, I have no problem
> >> in moving the warning to a different patchset, to avoid delay your work.
> >> I totally open to your suggestions.
> > Sure I suppose we could start with what you have, with the warnings,
> > and then submit a fixup afterwards.
> >
> I have no problem, let's see if we can agree in this patchset in order
> to keep improving things.
OK.
Regards,
Simon
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2020-06-16 13:43 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-29 18:15 [PATCH 00/10] improve OF_PLATDATA support Walter Lozano
2020-05-29 18:15 ` [PATCH 01/10] dtoc: add support to scan drivers Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-06-08 15:49 ` Walter Lozano
2020-06-11 16:15 ` Walter Lozano
2020-06-11 16:44 ` Simon Glass
2020-06-11 16:45 ` Simon Glass
2020-06-11 17:11 ` Walter Lozano
2020-06-11 17:22 ` Simon Glass
2020-06-11 19:07 ` Walter Lozano
2020-06-12 2:22 ` Simon Glass
2020-06-12 17:38 ` Walter Lozano
2020-06-16 13:43 ` Simon Glass
2020-05-29 18:15 ` [PATCH 02/10] dtoc: add option to disable warnings Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-06-08 15:51 ` Walter Lozano
2020-05-29 18:15 ` [PATCH 03/10] dm: doc: update of-plat with the suppor for driver aliases Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 04/10] core: drop const for struct driver_info Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 05/10] core: extend struct driver_info to point to device Walter Lozano
2020-05-29 18:56 ` Walter Lozano
2020-05-29 19:00 ` Simon Glass
2020-05-29 19:20 ` Walter Lozano
2020-05-29 20:42 ` Simon Glass
2020-06-04 15:59 ` Simon Glass
2020-06-08 15:53 ` Walter Lozano
2020-05-29 18:15 ` [PATCH 06/10] dtoc: extend dtoc to use struct driver_info when linking nodes Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 07/10] dm: doc: update of-plat with new phandle support Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-06-08 15:54 ` Walter Lozano
2020-05-29 18:15 ` [PATCH 08/10] dtoc: update tests to match new platdata Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:15 ` [PATCH 09/10] dtoc: update dtb_platdata to support cd-gpios Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-06-08 16:01 ` Walter Lozano
2020-06-14 2:49 ` Simon Glass
2020-05-29 18:15 ` [PATCH 10/10] dtoc add test for cd-gpios Walter Lozano
2020-06-04 15:59 ` Simon Glass
2020-05-29 18:25 ` [PATCH 00/10] improve OF_PLATDATA support Jagan Teki
2020-05-29 19:15 ` Walter Lozano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.