[PATCH 01/10] dtoc: add support to scan drivers

[Walter Lozano]

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 at 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 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        |  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