Re: [PATCH 01/10] dtoc: add support to scan drivers

11 Jun 2020

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@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@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@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.
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?
However removing the warning without properly testing the driver with 
of-platdata might hide runtime issues, don't you think so?
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.
...
...
Also I have mentioned
...
...
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?
Can we not fix the warnings?
So it is not clear to me which idea is better from your point if view.
I don't think V=1 helps. Then you suppress the warning for most
people. If it isn't important, we shouldn't print it. But see above -
hoping these warnings can be fixed by renaming drivers.
Yes, I understand, thanks for clarifying?
Regards,
Walter

    