From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 16 Jun 2020 07:43:54 -0600 Subject: [PATCH 01/10] dtoc: add support to scan drivers In-Reply-To: <6f03cdfa-446a-673c-7266-b068fce7eb14@collabora.com> References: <20200529181521.22073-1-walter.lozano@collabora.com> <20200529181521.22073-2-walter.lozano@collabora.com> <023aedfc-cc83-ee7a-41eb-23238cb19e14@collabora.com> <7b0ff66e-4be6-296b-4190-822628f8d22e@collabora.com> <6f03cdfa-446a-673c-7266-b068fce7eb14@collabora.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Walter, On Fri, 12 Jun 2020 at 11:38, Walter Lozano wrote: > > > On 11/6/20 23:22, Simon Glass wrote: > > Hi Walter, > > > > On Thu, 11 Jun 2020 at 13:07, Walter Lozano wrote: > >> Hi Simon, > >> > >> On 11/6/20 14:22, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Thu, 11 Jun 2020 at 11:11, Walter Lozano wrote: > >>>> Hi Simon > >>>> > >>>> On 11/6/20 13:45, Simon Glass wrote: > >>>>> Hi Walter, > >>>>> > >>>>> On Mon, 8 Jun 2020 at 09:49, Walter Lozano wrote: > >>>>>> Hi Simon, > >>>>>> > >>>>>> On 4/6/20 12:59, Simon Glass wrote: > >>>>>>> Hi Walter, > >>>>>>> > >>>>>>> On Fri, 29 May 2020 at 12:15, Walter Lozano 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 > >>>>>>>> --- > >>>>>>>> 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