From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Thu, 11 Jun 2020 16:07:14 -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> <7b0ff66e-4be6-296b-4190-822628f8d22e@collabora.com> Message-ID: 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 14:22, Simon Glass wrote: > Hi Walter, > > On Thu, 11 Jun 2020 at 11:11, Walter Lozano wrote: >> 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? > 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