From mboxrd@z Thu Jan  1 00:00:00 1970
From: Walter Lozano <walter.lozano@collabora.com>
Date: Thu, 11 Jun 2020 13:15:06 -0300
Subject: [PATCH 01/10] dtoc: add support to scan drivers
In-Reply-To: <023aedfc-cc83-ee7a-41eb-23238cb19e14@collabora.com>
References: <20200529181521.22073-1-walter.lozano@collabora.com>
 <20200529181521.22073-2-walter.lozano@collabora.com>
 <CAPnjgZ0WLRhB0A05+QEYO6QhM_XTZUGyvbeR=yov76Aeag-GDg@mail.gmail.com>
 <023aedfc-cc83-ee7a-41eb-23238cb19e14@collabora.com>
Message-ID: <7bf6a08d-00eb-a6d8-ea4c-239ccf577169@collabora.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On 8/6/20 12: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 
>> <walter.lozano@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@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.

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?

>
>> Also the warning is not really actionable. It needs to add mention of
>> U_BOOT_DEVICE and ...ALIAS.
> I agree with you. Thanks.
>>
>> In future we can scan the compatible strings and tell the user what
>> changes to make, I suppose.
> Yes, it will be a great improvement!
>>
>>> +??????????????? compat_c = compat_c_old
>>> +??????????? else: # pragma: no cover
>> Need to fix the coverage here
> Noted. I will a few more tests.
>>
>>> +??????????????? aliases_c = [compat_c_old] + aliases_c
>>> +
>>> +??????? return compat_c, aliases_c
>>>
>>> ????? def setup_output(self, fname):
>>> ????????? """Set up the output destination
>>> @@ -243,6 +277,46 @@ class DtbPlatdata(object):
>>> ????????????? return PhandleInfo(max_args, args)
>>> ????????? return None
>>>
>>> +??? def scan_driver(self, fn):
>>> +??????? """Scan a driver file to build a list of driver names and 
>>> aliases
>>> +
>>> +??????? This procedure will populate self._drivers and 
>>> self._driver_aliases
>>> +
>>> +??????? Args
>>> +??????????? fn: Driver filename to scan
>>> +??????? """
>>> +??????? with open(fn) as fd:
>>> +
>> drop blank line
>
> OK.
>
>>> +??????????? buff = fd.read()
>>> +
>>> +??????????? # The following re will search for driver names 
>>> declared as
>>> +??????????? # U_BOOT_DRIVER(driver_name)
>>> +??????????? drivers = re.findall('U_BOOT_DRIVER\((.*)\)', buff)
>>> +
>>> +??????????? for driver in drivers:
>>> +??????????????? self._drivers.append(driver)
>>> +
>>> +??????????? # The following re will search for driver aliases 
>>> declared as
>>> +??????????? # U_BOOT_DRIVER_ALIAS(alias, driver_name)
>>> +??????????? driver_aliases = 
>>> re.findall('U_BOOT_DRIVER_ALIAS\(\s*(\w+)\s*,\s*(\w+)\s*\)', buff)
>>> +
>>> +??????????? for alias in driver_aliases: # pragma: no cover
>>> +??????????????? if len(alias) != 2:
>>> +??????????????????? continue
>>> +??????????????? self._driver_aliases[alias[1]] = alias[0]
>>> +
>>> +??? def scan_drivers(self):
>>> +??????? """Scan the driver folders to build a list of driver names 
>>> and aliases
>>> +
>>> +??????? This procedure will populate self._drivers and 
>>> self._driver_aliases
>>> +
>>> +??????? """
>>> +??????? for (dirpath, dirnames, filenames) in os.walk('./'):
>> This doesn't work for out-of-tree cases (make O=xxx). You may need to
>> pass $(srctree) to dtc as an argument.
> Thanks for pointing at this.
>>> +??????????? for fn in filenames:
>>> +??????????????? if not fn.endswith('.c'):
>>> +??????????????????? continue
>>> +??????????????? self.scan_driver(dirpath + '/' + fn)
>>> +
>>> ????? def scan_dtb(self):
>>> ????????? """Scan the device tree to obtain a tree of nodes and 
>>> properties
>>>
>>> @@ -353,7 +427,7 @@ class DtbPlatdata(object):
>>> ????????? """
>>> ????????? structs = {}
>>> ????????? for node in self._valid_nodes:
>>> -??????????? node_name, _ = get_compat_name(node)
>>> +??????????? node_name, _ = self.get_normalized_compat_name(node)
>>> ????????????? fields = {}
>>>
>>> ????????????? # Get a list of all the valid properties in this node.
>>> @@ -377,14 +451,14 @@ class DtbPlatdata(object):
>>>
>>> ????????? upto = 0
>>> ????????? for node in self._valid_nodes:
>>> -??????????? node_name, _ = get_compat_name(node)
>>> +??????????? node_name, _ = self.get_normalized_compat_name(node)
>>> ????????????? struct = structs[node_name]
>>> ????????????? for name, prop in node.props.items():
>>> ????????????????? if name not in PROP_IGNORE_LIST and name[0] != '#':
>>> ????????????????????? prop.Widen(struct[name])
>>> ????????????? upto += 1
>>>
>>> -??????????? struct_name, aliases = get_compat_name(node)
>>> +??????????? struct_name, aliases = 
>>> self.get_normalized_compat_name(node)
>>> ????????????? for alias in aliases:
>>> ????????????????? self._aliases[alias] = struct_name
>>>
>>> @@ -461,7 +535,7 @@ class DtbPlatdata(object):
>>> ????????? Args:
>>> ????????????? node: node to output
>>> ????????? """
>>> -??????? struct_name, _ = get_compat_name(node)
>>> +??????? struct_name, _ = self.get_normalized_compat_name(node)
>>> ????????? var_name = conv_name_to_c(node.name)
>>> ????????? self.buf('static const struct %s%s %s%s = {\n' %
>>> ?????????????????? (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name))
>>> @@ -562,6 +636,7 @@ def run_steps(args, dtb_file, include_disabled, 
>>> output):
>>> ????????? raise ValueError('Please specify a command: struct, 
>>> platdata')
>>>
>>> ????? plat = DtbPlatdata(dtb_file, include_disabled)
>>> +??? plat.scan_drivers()
>>> ????? plat.scan_dtb()
>>> ????? plat.scan_tree()
>>> ????? plat.scan_reg_sizes()
>>> -- 
>>> 2.20.1
>>>
>
> Thanks once again for your review
>
> Regards,
>
> Walter
>