From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Thu, 11 Jun 2020 14:11:47 -0300 Subject: [PATCH 01/10] dtoc: add support to scan drivers In-Reply-To: References: <20200529181521.22073-1-walter.lozano@collabora.com> <20200529181521.22073-2-walter.lozano@collabora.com> <023aedfc-cc83-ee7a-41eb-23238cb19e14@collabora.com> Message-ID: <7b0ff66e-4be6-296b-4190-822628f8d22e@collabora.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon On 11/6/20 13:45, Simon Glass wrote: > Hi Walter, > > On Mon, 8 Jun 2020 at 09: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 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 >>>> --- >>>> 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