From patchwork Fri Jun 12 17:38:05 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Walter Lozano X-Patchwork-Id: 242201 List-Id: U-Boot discussion From: walter.lozano at collabora.com (Walter Lozano) Date: Fri, 12 Jun 2020 14:38:05 -0300 Subject: [PATCH 01/10] dtoc: add support to scan drivers In-Reply-To: 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> Message-ID: <6f03cdfa-446a-673c-7266-b068fce7eb14@collabora.com> 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 >> 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 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)