[PATCH 01/10] dtoc: add support to scan drivers
Walter Lozano
walter.lozano at collabora.com
Fri Jun 12 19:38:05 CEST 2020
On 11/6/20 23:22, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 11 Jun 2020 at 13:07, Walter Lozano <walter.lozano at 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 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?
> I think it would be better to rename the driver. The names are a bit
> arbitrary anyway at present.
>
Yes, I agree. Actually I rename some drivers for iMX6 for that reason.
Let me share some examples in order to check if we agree
diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
index 3d96678a45..8cc288581c 100644
--- a/drivers/gpio/rk_gpio.c
+++ b/drivers/gpio/rk_gpio.c
@@ -172,8 +172,8 @@ static const struct udevice_id rockchip_gpio_ids[] = {
{ }
};
-U_BOOT_DRIVER(gpio_rockchip) = {
- .name = "gpio_rockchip",
+U_BOOT_DRIVER(rockchip_gpio_bank) = {
+ .name = "rockchip_gpio_bank",
.id = UCLASS_GPIO,
.of_match = rockchip_gpio_ids,
.ops = &gpio_rockchip_ops,
diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
index 9549c74c2b..ff46d3c8d1 100644
--- a/drivers/gpio/sandbox.c
+++ b/drivers/gpio/sandbox.c
@@ -243,8 +243,8 @@ static const struct udevice_id sandbox_gpio_ids[] = {
{ }
};
-U_BOOT_DRIVER(gpio_sandbox) = {
- .name = "gpio_sandbox",
+U_BOOT_DRIVER(sandbox_gpio) = {
+ .name = "sandbox_gpio",
.id = UCLASS_GPIO,
.of_match = sandbox_gpio_ids,
.ofdata_to_platdata = sandbox_gpio_ofdata_to_platdata,
diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
index 7ae147f304..c617d21b7a 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
@@ -240,7 +240,7 @@ static const struct udevice_id rk3288_pinctrl_ids[] = {
{ }
};
-U_BOOT_DRIVER(pinctrl_rk3288) = {
+U_BOOT_DRIVER(rockchip_rk3288_pinctrl) = {
.name = "rockchip_rk3288_pinctrl",
.id = UCLASS_PINCTRL,
.of_match = rk3288_pinctrl_ids,
diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
index 52e6d9d8c0..d870ed7113 100644
--- a/drivers/power/pmic/rk8xx.c
+++ b/drivers/power/pmic/rk8xx.c
@@ -182,8 +182,8 @@ static const struct udevice_id rk8xx_ids[] = {
{ }
};
-U_BOOT_DRIVER(pmic_rk8xx) = {
- .name = "rk8xx pmic",
+U_BOOT_DRIVER(rockchip_rk805) = {
+ .name = "rockchip_rk805",
.id = UCLASS_PMIC,
.of_match = rk8xx_ids,
#if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>> 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.
>
I have no problem, let's see if we can agree in this patchset in order
to keep improving things.
Regards.
Walter
More information about the U-Boot
mailing list