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

[Simon Glass <sjg@chromium.org>]

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