| Message ID | 20200529181521.22073-2-walter.lozano@collabora.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | improve OF_PLATDATA support | expand |
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
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
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 >
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
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
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
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
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
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
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
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
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()
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(-)