[PATCH 01/10] dtoc: add support to scan drivers
Walter Lozano
walter.lozano at collabora.com
Thu Jun 11 21:07:14 CEST 2020
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 at 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 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?
> 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
More information about the U-Boot
mailing list