| Message ID | 20200513201345.26769-2-walter.lozano@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve OF_PLATDATA support | expand |
Hi Walter, On Wed, 13 May 2020 at 14:13, Walter Lozano <walter.lozano at 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 at collabora.com> > --- > include/dm/device.h | 6 +++++ > tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++--- > 2 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/include/dm/device.h b/include/dm/device.h > index 975eec5d0e..d5b3e732c3 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -282,6 +282,12 @@ 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 /* * Declare ... > + * 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..3f899a8bac 100644 > --- a/tools/dtoc/dtb_platdata.py > +++ b/tools/dtoc/dtb_platdata.py > @@ -14,6 +14,8 @@ static data. > import collections > import copy > import sys > +import os > +import re please keep sorted > > from dtoc import fdt > from dtoc import fdt_util > @@ -149,6 +151,20 @@ class DtbPlatdata(object): > self._outfile = None > self._lines = [] > self._aliases = {} > + self._drivers = [] > + self._driver_aliases = {} Please update comment above for what these new things are for. > + > + def get_normalized_compat_name(self, node): Needs function comment (also for those below). > + compat_c, aliases_c = get_compat_name(node) > + if compat_c not in self._drivers: > + try: # pragma: no cover Instead of an exception can you use .get() and check for None? > + compat_c_old = compat_c > + compat_c = self._driver_aliases[compat_c] > + aliases_c = [compat_c_old] + aliases > + except: > + print('WARNING: the driver %s was not found in the driver list' % (compat_c)) > + > + return compat_c, aliases_c > > def setup_output(self, fname): > """Set up the output destination > @@ -243,6 +259,34 @@ class DtbPlatdata(object): > return PhandleInfo(max_args, args) > return None > > + def scan_driver(self, fn): > + f = open(fn) Can you avoid single-char var names? Also how about either using tools.ReadFile() or: with open(fname) as fd: ...rest of function > + > + b = f.read() > + > + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b) > + > + for d in drivers: > + self._drivers.append(d) > + > + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b) Can you add a comment before this with an example of the line you are parsing? It helps make sense of the regex. > + > + for a in driver_aliases: # pragma: no cover > + try: > + self._driver_aliases[a[1]] = a[0] > + except: > + pass Again please avoid exceptions. > + > + def scan_drivers(self): > + """Scan the driver folders to build a list of driver names and possible > + 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 +397,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 = {} I wonder how long this scan takes? Should we cache the results somewhere? > > # Get a list of all the valid properties in this node. > @@ -377,14 +421,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 +505,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 +606,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 20/5/20 00:07, Simon Glass wrote: > Hi Walter, > > On Wed, 13 May 2020 at 14:13, Walter Lozano<walter.lozano at 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 at collabora.com> >> --- >> include/dm/device.h | 6 +++++ >> tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++--- >> 2 files changed, 55 insertions(+), 4 deletions(-) >> >> diff --git a/include/dm/device.h b/include/dm/device.h >> index 975eec5d0e..d5b3e732c3 100644 >> --- a/include/dm/device.h >> +++ b/include/dm/device.h >> @@ -282,6 +282,12 @@ 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 > /* > * Declare ... > Sure. >> + * 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..3f899a8bac 100644 >> --- a/tools/dtoc/dtb_platdata.py >> +++ b/tools/dtoc/dtb_platdata.py >> @@ -14,6 +14,8 @@ static data. >> import collections >> import copy >> import sys >> +import os >> +import re > please keep sorted Sure >> from dtoc import fdt >> from dtoc import fdt_util >> @@ -149,6 +151,20 @@ class DtbPlatdata(object): >> self._outfile = None >> self._lines = [] >> self._aliases = {} >> + self._drivers = [] >> + self._driver_aliases = {} > Please update comment above for what these new things are for. OK. >> + >> + def get_normalized_compat_name(self, node): > Needs function comment (also for those below). Sure, no problem. >> + compat_c, aliases_c = get_compat_name(node) >> + if compat_c not in self._drivers: >> + try: # pragma: no cover > Instead of an exception can you use .get() and check for None? Sure. >> + compat_c_old = compat_c >> + compat_c = self._driver_aliases[compat_c] >> + aliases_c = [compat_c_old] + aliases >> + except: >> + print('WARNING: the driver %s was not found in the driver list' % (compat_c)) This is something I would like to check with you. As you can see, a warning is raised when a driver name is not found, which I think is the right thing to do at this point. However, this warning will be trigger but almost all the tests as the drivers names are no valid (there is no U_BOOT_DRIVER). How do you think is best to fix this? I see different possibilities here - To add specific driver names to self._drivers when running a test - To change driver names on tests to match a valid one Maybe you have a more clean way to deal with this. >> + >> + return compat_c, aliases_c >> >> def setup_output(self, fname): >> """Set up the output destination >> @@ -243,6 +259,34 @@ class DtbPlatdata(object): >> return PhandleInfo(max_args, args) >> return None >> >> + def scan_driver(self, fn): >> + f = open(fn) > Can you avoid single-char var names? Also how about either using > tools.ReadFile() or: > > with open(fname) as fd: > ...rest of function Agree. >> + >> + b = f.read() >> + >> + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b) >> + >> + for d in drivers: >> + self._drivers.append(d) >> + >> + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b) > Can you add a comment before this with an example of the line you are > parsing? It helps make sense of the regex. No problem. >> + >> + for a in driver_aliases: # pragma: no cover >> + try: >> + self._driver_aliases[a[1]] = a[0] >> + except: >> + pass > Again please avoid exceptions. No problem at all. >> + >> + def scan_drivers(self): >> + """Scan the driver folders to build a list of driver names and possible >> + 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 +397,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 = {} > I wonder how long this scan takes? Should we cache the results somewhere? Probably you are worried because this task will be repeated every time we compile U-boot, and it could be a potential issue when testing several different configs. However, at least in my setup the scan takes less than 1 sec, which I think is promising. Also I think that this scan is only perform for those configs which enable OF_PLATDATA, which are 15, should not cause much overhead. What do you think? >> # Get a list of all the valid properties in this node. >> @@ -377,14 +421,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 +505,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 +606,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, Walter
Hi Walter, On Thu, 21 May 2020 at 07:15, Walter Lozano <walter.lozano at collabora.com> wrote: > > Hi Simon, > > On 20/5/20 00:07, Simon Glass wrote: > > Hi Walter, > > > > On Wed, 13 May 2020 at 14:13, Walter Lozano<walter.lozano at 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 at collabora.com> > >> --- > >> include/dm/device.h | 6 +++++ > >> tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++--- > >> 2 files changed, 55 insertions(+), 4 deletions(-) > >> [..] > >> + > >> + def get_normalized_compat_name(self, node): > > Needs function comment (also for those below). > Sure, no problem. > >> + compat_c, aliases_c = get_compat_name(node) > >> + if compat_c not in self._drivers: > >> + try: # pragma: no cover > > Instead of an exception can you use .get() and check for None? > Sure. > >> + compat_c_old = compat_c > >> + compat_c = self._driver_aliases[compat_c] > >> + aliases_c = [compat_c_old] + aliases > >> + except: > >> + print('WARNING: the driver %s was not found in the driver list' % (compat_c)) > > This is something I would like to check with you. As you can see, a > warning is raised when a driver name is not found, which I think is the > right thing to do at this point. However, this warning will be trigger > but almost all the tests as the drivers names are no valid (there is no > U_BOOT_DRIVER). How do you think is best to fix this? > > I see different possibilities here > > - To add specific driver names to self._drivers when running a test > > - To change driver names on tests to match a valid one > > Maybe you have a more clean way to deal with this. You could add a new method 'set_test_mode()' dtb_platdata.py You could add a flag to dtoc to disable the warning. You could capture the output with test_util.capture_sys_output() [..] > >> @@ -353,7 +397,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 = {} > > I wonder how long this scan takes? Should we cache the results somewhere? > > Probably you are worried because this task will be repeated every time > we compile U-boot, and it could be a potential issue when testing > several different configs. However, at least in my setup the scan takes > less than 1 sec, which I think is promising. Also I think that this scan > is only perform for those configs which enable OF_PLATDATA, which are > 15, should not cause much overhead. > > What do you think? I think that is OK for now. [..] Regards, Simon
diff --git a/include/dm/device.h b/include/dm/device.h index 975eec5d0e..d5b3e732c3 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -282,6 +282,12 @@ 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..3f899a8bac 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -14,6 +14,8 @@ static data. import collections import copy import sys +import os +import re from dtoc import fdt from dtoc import fdt_util @@ -149,6 +151,20 @@ class DtbPlatdata(object): self._outfile = None self._lines = [] self._aliases = {} + self._drivers = [] + self._driver_aliases = {} + + def get_normalized_compat_name(self, node): + compat_c, aliases_c = get_compat_name(node) + if compat_c not in self._drivers: + try: # pragma: no cover + compat_c_old = compat_c + compat_c = self._driver_aliases[compat_c] + aliases_c = [compat_c_old] + aliases + except: + print('WARNING: the driver %s was not found in the driver list' % (compat_c)) + + return compat_c, aliases_c def setup_output(self, fname): """Set up the output destination @@ -243,6 +259,34 @@ class DtbPlatdata(object): return PhandleInfo(max_args, args) return None + def scan_driver(self, fn): + f = open(fn) + + b = f.read() + + drivers = re.findall('U_BOOT_DRIVER\((.*)\)', b) + + for d in drivers: + self._drivers.append(d) + + driver_aliases = re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', b) + + for a in driver_aliases: # pragma: no cover + try: + self._driver_aliases[a[1]] = a[0] + except: + pass + + def scan_drivers(self): + """Scan the driver folders to build a list of driver names and possible + 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 +397,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 +421,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 +505,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 +606,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 at collabora.com> --- include/dm/device.h | 6 +++++ tools/dtoc/dtb_platdata.py | 53 +++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 4 deletions(-)