[PATCH 01/10] dtoc: add support to scan drivers
Simon Glass
sjg at chromium.org
Tue Jun 16 15:43:54 CEST 2020
Hi Walter,
On Fri, 12 Jun 2020 at 11:38, Walter Lozano <walter.lozano at collabora.com> wrote:
>
>
> 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)
>
Yes that seems OK.
>
> >> 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.
OK.
Regards,
Simon
More information about the U-Boot
mailing list