[RFC 0/4] drivers: footprint reduction proposal

[RFC 0/4] drivers: footprint reduction proposal

[Walter Lozano]

Based on several reports and discussions it is clear that U-Boot's
footprint is always a concern, and any kind of reduction is an
improvement.

This series is a proposal to  help reducing the footprint by parsing
information provided in DT and drivers in different ways and adding
additional intelligence to dtoc. The current version implements the basic
functionality in dtoc but this is no fully integrated, however it will allow
us to discuss this approach.

Firstly, based on the compatible strings found in drivers, include only DT nodes
which are supported by any driver present in U-Boot.

Secondly, generate struct udevice_id entries only for nodes present in DT,
which will allow to avoid including additional data.

These are the first steps for further improvements as proposed in the specific
patches in this series.

This work is based on the work of Simon Glass present in [1] which adds
support to dtoc for parsing compatible strings.

[1] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working

Walter Lozano (4):
  dtoc: add POC for dtb shrink
  dtoc: add initial support for deleting DTB nodes
  dtoc: add support for generate stuct udevice_id
  mmc: fsl_esdhc_imx: make use of dtoc to generate struct udevice_id

 drivers/mmc/fsl_esdhc_imx.c |  58 ++++++++++++++------
 tools/dtoc/dtb_platdata.py  | 102 ++++++++++++++++++++++++++++++++++--
 tools/dtoc/fdt.py           |   3 ++
 3 files changed, 143 insertions(+), 20 deletions(-)

-- 
2.20.1

[RFC 1/4] dtoc: add POC for dtb shrink

[Walter Lozano]

Based on several reports and discussions [1], [2] it is clear that U-Boot's
footprint is always a concern, and any kind of reduction is an
improvement.

In particular dtb is one of the sources of footprint increment, as
U-Boot uses the same dtb as Linux. However is interesting to note that
U-Boot does not require all the nodes and properties declared in it.
Some improvements in this sense are already present, such as
removing properties based on configuration and using specific "u-boot"
properties to keep only specific node in SPL. However, this require
manual configuration.

Additionally reducing dtb, will allow ATF for better handing FDT buffer, which
is an issue in some contexts [3].

In order to improve this situation, this patch adds a proof of concept
for dtb shrink. The idea behind this is simple, remove all the nodes
from dtb which compatible string is not supported by any driver present.
This approach makes sense for those configuration where Linux is
expected to have its own dtb.

This patch is based on the work of Simon Glass present in [4] which adds
support to dtoc for parsing compatible strings.

Some early untested results shows that the reduction in size is 50 % in
case of mx6_cuboxi_defconfig, which is promising.

Some additional reduction could be possible by only keeping the nodes for
whose compatible string is supported by any enabled driver. However,
this requires to add extra logic to parse config files and map
configuration to compatible strings.

This proof of concept uses fdtgrep to implement the node removal, but
the idea is to implement this logic inside the dtoc for better handling.

[1] http://patchwork.ozlabs.org/project/uboot/patch/20200525093539.1.Ibf2d19439cde35e39192a9d4a8dad23539fae2e6 at changeid/
[2] http://u-boot.10912.n7.nabble.com/PATCH-v1-00-15-add-basic-driver-support-for-broadcom-NS3-soc-tt412411.html#none
[3] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/4512
[4] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 tools/dtoc/dtb_platdata.py | 42 +++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 21cce5afb5..1df13b2cd2 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -399,7 +399,10 @@ class DtbPlatdata(object):
         """Scan the driver folders to build a list of driver names and possible
         aliases
         """
-        for (dirpath, dirnames, filenames) in os.walk('/home/sglass/u'):
+        basedir = sys.argv[0].replace('tools/dtoc/dtoc', '')
+        if basedir == '':
+            basedir = './'
+        for (dirpath, dirnames, filenames) in os.walk(basedir):
             for fn in filenames:
                 if not fn.endswith('.c'):
                     continue
@@ -802,6 +805,32 @@ class DtbPlatdata(object):
         self.out(''.join(self.get_buf()))
         self.close_output()
 
+    def shrink(self):
+        """Generate a shrunk version of DTB bases on valid drivers
+
+        This function removes nodes from dtb which compatible string is not
+        found in drivers. The output is saved in a file with suffix name -shrink.dtb
+        """
+        compat = []
+        cmd = './tools/fdtgrep '
+        #print(self._drivers)
+        for dr in self._drivers.values():
+            compat = compat + dr.compat
+
+        for cp in compat:
+            #print(cp)
+            cmd += ' -c ' + cp
+
+        cmd += ' -O dtb -o ' + self._dtb_fname.replace('.dtb', '') + '-shrink.dtb ' + self._dtb_fname
+
+        if False:
+            with open('dt_shrink.sh', 'w+') as script:
+                script.write(cmd)
+
+        os.system(cmd)
+
+        return
+
 def run_steps(args, dtb_file, config_file, include_disabled, output):
     """Run all the steps of the dtoc tool
 
@@ -816,6 +845,10 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
     if not args:
         raise ValueError('Please specify a command: struct, platdata')
 
+    skip_scan = False
+    if args == ['shrink']:
+        skip_scan = True
+
     plat = DtbPlatdata(dtb_file, config_file, include_disabled)
     plat.scan_drivers()
     plat.scan_dtb()
@@ -823,14 +856,17 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
     plat.scan_config()
     plat.scan_reg_sizes()
     plat.setup_output(output)
-    structs = plat.scan_structs()
-    plat.scan_phandles()
+    if not skip_scan:
+        structs = plat.scan_structs()
+        plat.scan_phandles()
 
     for cmd in args[0].split(','):
         if cmd == 'struct':
             plat.generate_structs(structs)
         elif cmd == 'platdata':
             plat.generate_tables()
+        elif cmd == 'shrink':
+            plat.shrink()
         else:
             raise ValueError("Unknown command '%s': (use: struct, platdata)" %
                              cmd)
-- 
2.20.1

[RFC 2/4] dtoc: add initial support for deleting DTB nodes

[Walter Lozano]

This patch introduce a test for deleting DTB nodes using Python library.

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 tools/dtoc/dtb_platdata.py | 28 ++++++++++++++++++++++++++++
 tools/dtoc/fdt.py          |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index 1df13b2cd2..d3fb4dcbf2 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -831,6 +831,30 @@ class DtbPlatdata(object):
 
         return
 
+    def test_del_node(self):
+        """Test the support of node deletion'
+
+        This function tests the support of node removal by deleting a specific
+        node name
+        """
+        for n in self._fdt.GetRoot().subnodes:
+            print('Name', n.name)
+            #print('Props', n.props)
+            if n.name == 'board-detect':
+                n.DelNode()
+                #self._fdt.GetRoot().subnodes.remove(n)
+        self._fdt.Scan()
+        print('')
+        for n in self._fdt.GetRoot().subnodes:
+            print('Name', n.name)
+            #print('Props', n.props)
+            if n.name == 'serial':
+                self._fdt.GetRoot().subnodes.remove(n)
+
+        self._fdt.Pack()
+        self._fdt.Flush()
+        self._fdt.Sync()
+
 def run_steps(args, dtb_file, config_file, include_disabled, output):
     """Run all the steps of the dtoc tool
 
@@ -848,6 +872,8 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
     skip_scan = False
     if args == ['shrink']:
         skip_scan = True
+    if args == ['test_del_node']:
+        skip_scan = True
 
     plat = DtbPlatdata(dtb_file, config_file, include_disabled)
     plat.scan_drivers()
@@ -867,6 +893,8 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
             plat.generate_tables()
         elif cmd == 'shrink':
             plat.shrink()
+        elif cmd == 'test_del_node':
+            plat.test_del_node()
         else:
             raise ValueError("Unknown command '%s': (use: struct, platdata)" %
                              cmd)
diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index 3d4bc3b2ef..b3b626cd4d 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -502,6 +502,9 @@ class Node:
         for prop in prop_list:
             prop.Sync(auto_resize)
 
+    def DelNode(self):
+        fdt_obj = self._fdt._fdt_obj
+        fdt_obj.del_node(self._offset)
 
 class Fdt:
     """Provides simple access to a flat device tree blob using libfdts.
-- 
2.20.1

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Based on several reports there is an increasing concern in the impact
of adding additional features to drivers based on compatible strings.
A good example of this situation is found in [1].

In order to reduce this impact and as an initial step for further
reduction, propose a new way to declare compatible strings, which allows
to only include the useful ones.

The idea is to define compatible strings in a way to be easily parsed by
dtoc, which will be responsible to build struct udevice_id [] based on
the compatible strings present in the dtb.

Additional features can be easily added, such as define constants
depending on the presence of compatible strings, which allows to enable
code blocks only in such cases without the need of adding additional
configuration options.

[1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index d3fb4dcbf2..e199caf8c9 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -170,6 +170,9 @@ class DtbPlatdata(object):
             key: alias name
             value: node path
         _tiny_uclasses: List of uclass names that are marked as 'tiny'
+        _compatible_strings: Dict of compatible strings read from drivres
+            key: driver name
+            value: list of struct udevice_id
     """
     def __init__(self, dtb_fname, config_fname, include_disabled):
         self._fdt = None
@@ -188,6 +191,7 @@ class DtbPlatdata(object):
         self._tiny_uclasses = []
         self._of_match = {}
         self._compat_to_driver = {}
+        self._compatible_strings = {}
 
     def get_normalized_compat_name(self, node):
         compat_c, aliases_c = get_compat_name(node)
@@ -395,6 +399,15 @@ class DtbPlatdata(object):
             except:
                 pass
 
+        # find entries declared with the form
+        # COMPATIBLE(driver_name, compatible_string)
+        compatible_strings = re.findall('COMPATIBLE\(\s*(\w+)\s*,\s*(\S+)\s*\)', b)
+
+        for co in compatible_strings:
+            if not self._compatible_strings.get(co[0]):
+                self._compatible_strings[co[0]] = []
+            self._compatible_strings[co[0]].append(co)
+
     def scan_drivers(self):
         """Scan the driver folders to build a list of driver names and possible
         aliases
@@ -805,6 +818,23 @@ class DtbPlatdata(object):
         self.out(''.join(self.get_buf()))
         self.close_output()
 
+    def generate_compatible(self):
+        """Generates the struct udevice_id[] to be used in drivers
+
+        This writes C code to implement struct udevice_id[] based on
+        COMPATIBLE(driver_name, compatible) entries found in drivers.
+
+        Additionally this function can filter entries in order to avoid
+        adding those that are not present in DT.
+        """
+        self.out('#define COMPATIBLE(__driver_name, __compatible, ...)\n\n')
+        for vals in self._compatible_strings.values():
+            st = ''
+            for comp in vals:
+                st += '{.compatible = %s},\\\n' % (comp[1])
+            st += '{ /* sentinel */ },\\\n'
+            self.out('#define COMPATIBLE_LIST_%s { \\\n%s}\n' % (comp[0], st))
+
     def shrink(self):
         """Generate a shrunk version of DTB bases on valid drivers
 
@@ -895,6 +925,8 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
             plat.shrink()
         elif cmd == 'test_del_node':
             plat.test_del_node()
+        elif cmd == 'compatible':
+            plat.generate_compatible()
         else:
             raise ValueError("Unknown command '%s': (use: struct, platdata)" %
                              cmd)
-- 
2.20.1

[RFC 4/4] mmc: fsl_esdhc_imx: make use of dtoc to generate struct udevice_id

[Walter Lozano]

As an example of use, convert fsl_esdhc_imx to use COMPATIBLE entries
for declaring compatible strings.

As a result of these entries dtoc will generate code similar to

Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
---
 drivers/mmc/fsl_esdhc_imx.c | 58 ++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 588d6a9d76..16448a9ebe 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -33,6 +33,7 @@
 #include <dm.h>
 #include <asm-generic/gpio.h>
 #include <dm/pinctrl.h>
+#include <generated/compatible.h>
 
 #if !CONFIG_IS_ENABLED(BLK)
 #include "mmc_private.h"
@@ -1613,22 +1614,45 @@ static struct esdhc_soc_data usdhc_imx8qm_data = {
 		ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES,
 };
 
-static const struct udevice_id fsl_esdhc_ids[] = {
-	{ .compatible = "fsl,imx53-esdhc", },
-	{ .compatible = "fsl,imx6ul-usdhc", },
-	{ .compatible = "fsl,imx6sx-usdhc", },
-	{ .compatible = "fsl,imx6sl-usdhc", },
-	{ .compatible = "fsl,imx6q-usdhc", },
-	{ .compatible = "fsl,imx7d-usdhc", .data = (ulong)&usdhc_imx7d_data,},
-	{ .compatible = "fsl,imx7ulp-usdhc", },
-	{ .compatible = "fsl,imx8qm-usdhc", .data = (ulong)&usdhc_imx8qm_data,},
-	{ .compatible = "fsl,imx8mm-usdhc", .data = (ulong)&usdhc_imx8qm_data,},
-	{ .compatible = "fsl,imx8mn-usdhc", .data = (ulong)&usdhc_imx8qm_data,},
-	{ .compatible = "fsl,imx8mq-usdhc", .data = (ulong)&usdhc_imx8qm_data,},
-	{ .compatible = "fsl,imxrt-usdhc", },
-	{ .compatible = "fsl,esdhc", },
-	{ /* sentinel */ }
-};
+#if 0
+// As result of the COMPATIBLE entries, dtoc will produce
+// code similar to this
+
+#define COMPATIBLE(__driver_name, compatible, ...)
+
+#define COMPATIBLE_LIST_FSL_ESDHC { \
+{.compatible = "fsl,imx53-esdhc"},\
+{.compatible = "fsl,imx6ul-usdhc"},\
+{.compatible = "fsl,imx6sx-usdhc"},\
+{.compatible = "fsl,imx6sl-usdhc"},\
+{.compatible = "fsl,imx6q-usdhc"},\
+{.compatible = "fsl,imx7ulp-usdhc"},\
+{.compatible = "fsl,imxrt-usdhc"},\
+{.compatible = "fsl,esdhc"},\
+{ /* sentinel */ },\
+}
+#define COMPATIBLE_LIST_FSL_ESDHC2 { \
+{.compatible = "fsl,imxrt-usdhc2"},\
+{.compatible = "fsl,esdhc2"},\
+{ /* sentinel */ },\
+}
+#endif
+
+COMPATIBLE(FSL_ESDHC, "fsl,imx53-esdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx6ul-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx6sx-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx6sl-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx6q-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", (ulong)&usdhc_imx7d_data)
+COMPATIBLE(FSL_ESDHC, "fsl,imx7ulp-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,imx8qm-usdhc", &usdhc_imx8qm_data)
+COMPATIBLE(FSL_ESDHC, "fsl,imx8mm-usdhc", &usdhc_imx8qm_data)
+COMPATIBLE(FSL_ESDHC, "fsl,imx8mn-usdhc", &usdhc_imx8qm_data)
+COMPATIBLE(FSL_ESDHC, "fsl,imx8mq-usdhc", &usdhc_imx8qm_data)
+COMPATIBLE(FSL_ESDHC, "fsl,imxrt-usdhc")
+COMPATIBLE(FSL_ESDHC, "fsl,esdhc")
+
+static const struct udevice_id fsl_esdhc_ids[] = COMPATIBLE_LIST_FSL_ESDHC;
 
 #if CONFIG_IS_ENABLED(BLK)
 static int fsl_esdhc_bind(struct udevice *dev)
@@ -1640,7 +1664,7 @@ static int fsl_esdhc_bind(struct udevice *dev)
 #endif
 
 U_BOOT_DRIVER(fsl_esdhc) = {
-	.name	= "fsl-esdhc-mmc",
+	.name	= "fsl_esdhc",
 	.id	= UCLASS_MMC,
 	.of_match = fsl_esdhc_ids,
 	.ops	= &fsl_esdhc_ops,
-- 
2.20.1

[RFC 0/4] drivers: footprint reduction proposal

[Tom Rini]

On Fri, Jun 19, 2020 at 06:11:36PM -0300, Walter Lozano wrote:

> Based on several reports and discussions it is clear that U-Boot's
> footprint is always a concern, and any kind of reduction is an
> improvement.
> 
> This series is a proposal to  help reducing the footprint by parsing
> information provided in DT and drivers in different ways and adding
> additional intelligence to dtoc. The current version implements the basic
> functionality in dtoc but this is no fully integrated, however it will allow
> us to discuss this approach.
> 
> Firstly, based on the compatible strings found in drivers, include only DT nodes
> which are supported by any driver present in U-Boot.
> 
> Secondly, generate struct udevice_id entries only for nodes present in DT,
> which will allow to avoid including additional data.
> 
> These are the first steps for further improvements as proposed in the specific
> patches in this series.
> 
> This work is based on the work of Simon Glass present in [1] which adds
> support to dtoc for parsing compatible strings.
> 
> [1] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working

I applied this series on top of the above tree, but there's no rule for
<generated/compatible.h> so is something missing?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200619/779fbe95/attachment.sig>

[RFC 0/4] drivers: footprint reduction proposal

[Walter Lozano]

Hi Tom,

On 19/6/20 18:48, Tom Rini wrote:
> On Fri, Jun 19, 2020 at 06:11:36PM -0300, Walter Lozano wrote:
>
>> Based on several reports and discussions it is clear that U-Boot's
>> footprint is always a concern, and any kind of reduction is an
>> improvement.
>>
>> This series is a proposal to  help reducing the footprint by parsing
>> information provided in DT and drivers in different ways and adding
>> additional intelligence to dtoc. The current version implements the basic
>> functionality in dtoc but this is no fully integrated, however it will allow
>> us to discuss this approach.
>>
>> Firstly, based on the compatible strings found in drivers, include only DT nodes
>> which are supported by any driver present in U-Boot.
>>
>> Secondly, generate struct udevice_id entries only for nodes present in DT,
>> which will allow to avoid including additional data.
>>
>> These are the first steps for further improvements as proposed in the specific
>> patches in this series.
>>
>> This work is based on the work of Simon Glass present in [1] which adds
>> support to dtoc for parsing compatible strings.
>>
>> [1] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
> I applied this series on top of the above tree, but there's no rule for
> <generated/compatible.h> so is something missing?  Thanks!
>
Thanks for taking the time to check this RFC.

As you pointed, the Makefile needs to be tweaked in order to be able to 
run a build, that is what I meant by "not fully integrated", also some 
additional stuff are missing. However, I thought that sending this RFC 
explaining the idea will be nice in order to confirm if the approaches 
proposed make sense for the community and at least the one to handle 
compatible strings is different from the linker list suggestion.

Regards,

Walter

[RFC 0/4] drivers: footprint reduction proposal

[Tom Rini]

On Mon, Jun 22, 2020 at 11:12:40AM -0300, Walter Lozano wrote:
> Hi Tom,
> 
> On 19/6/20 18:48, Tom Rini wrote:
> > On Fri, Jun 19, 2020 at 06:11:36PM -0300, Walter Lozano wrote:
> > 
> > > Based on several reports and discussions it is clear that U-Boot's
> > > footprint is always a concern, and any kind of reduction is an
> > > improvement.
> > > 
> > > This series is a proposal to  help reducing the footprint by parsing
> > > information provided in DT and drivers in different ways and adding
> > > additional intelligence to dtoc. The current version implements the basic
> > > functionality in dtoc but this is no fully integrated, however it will allow
> > > us to discuss this approach.
> > > 
> > > Firstly, based on the compatible strings found in drivers, include only DT nodes
> > > which are supported by any driver present in U-Boot.
> > > 
> > > Secondly, generate struct udevice_id entries only for nodes present in DT,
> > > which will allow to avoid including additional data.
> > > 
> > > These are the first steps for further improvements as proposed in the specific
> > > patches in this series.
> > > 
> > > This work is based on the work of Simon Glass present in [1] which adds
> > > support to dtoc for parsing compatible strings.
> > > 
> > > [1] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
> > I applied this series on top of the above tree, but there's no rule for
> > <generated/compatible.h> so is something missing?  Thanks!
> > 
> Thanks for taking the time to check this RFC.
> 
> As you pointed, the Makefile needs to be tweaked in order to be able to run
> a build, that is what I meant by "not fully integrated", also some
> additional stuff are missing. However, I thought that sending this RFC
> explaining the idea will be nice in order to confirm if the approaches
> proposed make sense for the community and at least the one to handle
> compatible strings is different from the linker list suggestion.

I think I like the idea, but I need to give a build a spin and poke
things harder.  What do I need to do to manually have this build+link?
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200622/08acb115/attachment.sig>

[RFC 0/4] drivers: footprint reduction proposal

[Walter Lozano]

Hi Tom,

On 22/6/20 11:20, Tom Rini wrote:
> On Mon, Jun 22, 2020 at 11:12:40AM -0300, Walter Lozano wrote:
>> Hi Tom,
>>
>> On 19/6/20 18:48, Tom Rini wrote:
>>> On Fri, Jun 19, 2020 at 06:11:36PM -0300, Walter Lozano wrote:
>>>
>>>> Based on several reports and discussions it is clear that U-Boot's
>>>> footprint is always a concern, and any kind of reduction is an
>>>> improvement.
>>>>
>>>> This series is a proposal to  help reducing the footprint by parsing
>>>> information provided in DT and drivers in different ways and adding
>>>> additional intelligence to dtoc. The current version implements the basic
>>>> functionality in dtoc but this is no fully integrated, however it will allow
>>>> us to discuss this approach.
>>>>
>>>> Firstly, based on the compatible strings found in drivers, include only DT nodes
>>>> which are supported by any driver present in U-Boot.
>>>>
>>>> Secondly, generate struct udevice_id entries only for nodes present in DT,
>>>> which will allow to avoid including additional data.
>>>>
>>>> These are the first steps for further improvements as proposed in the specific
>>>> patches in this series.
>>>>
>>>> This work is based on the work of Simon Glass present in [1] which adds
>>>> support to dtoc for parsing compatible strings.
>>>>
>>>> [1] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
>>> I applied this series on top of the above tree, but there's no rule for
>>> <generated/compatible.h> so is something missing?  Thanks!
>>>
>> Thanks for taking the time to check this RFC.
>>
>> As you pointed, the Makefile needs to be tweaked in order to be able to run
>> a build, that is what I meant by "not fully integrated", also some
>> additional stuff are missing. However, I thought that sending this RFC
>> explaining the idea will be nice in order to confirm if the approaches
>> proposed make sense for the community and at least the one to handle
>> compatible strings is different from the linker list suggestion.
> I think I like the idea, but I need to give a build a spin and poke
> things harder.  What do I need to do to manually have this build+link?
> Thanks!
>
Well, I don't think this version will give you something to fully test, 
as there are several pieces missing, but the fact you think you like the 
idea is good starting point.

Just to do some testing you can try


Shrink a dtb with

./tools/dtoc/dtoc shrink -d u-boot.dtb

this will shrink u-boot.dtb and create u-boot-shrink.dtb by only include 
nodes with compatible strings present in drivers


Generate include/generated/compatible.h

./tools/dtoc/dtoc compatible -d u-boot.dtb -o include/generated/compatible.h

this will generate compatible.h but the code does not yet support struct 
udevice_id with data in struct udevice_id and does not filter anything.


I will continue working on these features but any early feedback is 
welcome.

Regards,

[RFC 0/4] drivers: footprint reduction proposal

[Walter Lozano]

Hi Tom,

On 22/6/20 12:25, Walter Lozano wrote:
> Hi Tom,
>
> On 22/6/20 11:20, Tom Rini wrote:
>> On Mon, Jun 22, 2020 at 11:12:40AM -0300, Walter Lozano wrote:
>>> Hi Tom,
>>>
>>> On 19/6/20 18:48, Tom Rini wrote:
>>>> On Fri, Jun 19, 2020 at 06:11:36PM -0300, Walter Lozano wrote:
>>>>
>>>>> Based on several reports and discussions it is clear that U-Boot's
>>>>> footprint is always a concern, and any kind of reduction is an
>>>>> improvement.
>>>>>
>>>>> This series is a proposal to? help reducing the footprint by parsing
>>>>> information provided in DT and drivers in different ways and adding
>>>>> additional intelligence to dtoc. The current version implements 
>>>>> the basic
>>>>> functionality in dtoc but this is no fully integrated, however it 
>>>>> will allow
>>>>> us to discuss this approach.
>>>>>
>>>>> Firstly, based on the compatible strings found in drivers, include 
>>>>> only DT nodes
>>>>> which are supported by any driver present in U-Boot.
>>>>>
>>>>> Secondly, generate struct udevice_id entries only for nodes 
>>>>> present in DT,
>>>>> which will allow to avoid including additional data.
>>>>>
>>>>> These are the first steps for further improvements as proposed in 
>>>>> the specific
>>>>> patches in this series.
>>>>>
>>>>> This work is based on the work of Simon Glass present in [1] which 
>>>>> adds
>>>>> support to dtoc for parsing compatible strings.
>>>>>
>>>>> [1] 
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working 
>>>>>
>>>> I applied this series on top of the above tree, but there's no rule 
>>>> for
>>>> <generated/compatible.h> so is something missing? Thanks!
>>>>
>>> Thanks for taking the time to check this RFC.
>>>
>>> As you pointed, the Makefile needs to be tweaked in order to be able 
>>> to run
>>> a build, that is what I meant by "not fully integrated", also some
>>> additional stuff are missing. However, I thought that sending this RFC
>>> explaining the idea will be nice in order to confirm if the approaches
>>> proposed make sense for the community and at least the one to handle
>>> compatible strings is different from the linker list suggestion.
>> I think I like the idea, but I need to give a build a spin and poke
>> things harder.? What do I need to do to manually have this build+link?
>> Thanks!
>>
> Well, I don't think this version will give you something to fully 
> test, as there are several pieces missing, but the fact you think you 
> like the idea is good starting point.
>
> Just to do some testing you can try
>
>
> Shrink a dtb with
>
> ./tools/dtoc/dtoc shrink -d u-boot.dtb
>
> this will shrink u-boot.dtb and create u-boot-shrink.dtb by only 
> include nodes with compatible strings present in drivers
>
>
> Generate include/generated/compatible.h
>
> ./tools/dtoc/dtoc compatible -d u-boot.dtb -o 
> include/generated/compatible.h
>
> this will generate compatible.h but the code does not yet support 
> struct udevice_id with data in struct udevice_id and does not filter 
> anything.
>
>
> I will continue working on these features but any early feedback is 
> welcome.
>

To be able to test this a bit more I reworked and back ported the 
patches and add a bit more of work on top, such as

- Add Makefile rules (need to be improved)

- Add support for checking enabled drivers for dtb shrink (need to be 
improved as parsing probably does not take into account no common 
situations)

- Add support for defining constants based on compatible strings enabled

This work can be found in

https://gitlab.collabora.com/wlozano/u-boot/-/tree/wip

The drawback is that this implementation doesn't take advantage of the 
new abstractions found in the Simon's work, but I think it could still 
be useful to test the idea, and discuss if it makes sense.

I have done some testing in my iMX6 Hummingboard but I have found some 
issues not related to this work in MMC, which I need to debug.

Regards,

Walter

[RFC 2/4] dtoc: add initial support for deleting DTB nodes

[Simon Glass]

Hi Walter,

On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> This patch introduce a test for deleting DTB nodes using Python library.
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  tools/dtoc/dtb_platdata.py | 28 ++++++++++++++++++++++++++++
>  tools/dtoc/fdt.py          |  3 +++
>  2 files changed, 31 insertions(+)

This test should go in test_dtoc.py

Regards,
Simon

[RFC 1/4] dtoc: add POC for dtb shrink

[Simon Glass]

Hi Walter,

On Fri, 19 Jun 2020 at 15:11, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Based on several reports and discussions [1], [2] it is clear that U-Boot's
> footprint is always a concern, and any kind of reduction is an
> improvement.
>
> In particular dtb is one of the sources of footprint increment, as
> U-Boot uses the same dtb as Linux. However is interesting to note that
> U-Boot does not require all the nodes and properties declared in it.
> Some improvements in this sense are already present, such as
> removing properties based on configuration and using specific "u-boot"
> properties to keep only specific node in SPL. However, this require
> manual configuration.
>
> Additionally reducing dtb, will allow ATF for better handing FDT buffer, which
> is an issue in some contexts [3].
>
> In order to improve this situation, this patch adds a proof of concept
> for dtb shrink. The idea behind this is simple, remove all the nodes
> from dtb which compatible string is not supported by any driver present.
> This approach makes sense for those configuration where Linux is
> expected to have its own dtb.
>
> This patch is based on the work of Simon Glass present in [4] which adds
> support to dtoc for parsing compatible strings.
>
> Some early untested results shows that the reduction in size is 50 % in
> case of mx6_cuboxi_defconfig, which is promising.
>
> Some additional reduction could be possible by only keeping the nodes for
> whose compatible string is supported by any enabled driver. However,
> this requires to add extra logic to parse config files and map
> configuration to compatible strings.
>
> This proof of concept uses fdtgrep to implement the node removal, but
> the idea is to implement this logic inside the dtoc for better handling.
>
> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525093539.1.Ibf2d19439cde35e39192a9d4a8dad23539fae2e6 at changeid/
> [2] http://u-boot.10912.n7.nabble.com/PATCH-v1-00-15-add-basic-driver-support-for-broadcom-NS3-soc-tt412411.html#none
> [3] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/4512
> [4] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  tools/dtoc/dtb_platdata.py | 42 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
> index 21cce5afb5..1df13b2cd2 100644
> --- a/tools/dtoc/dtb_platdata.py
> +++ b/tools/dtoc/dtb_platdata.py
> @@ -399,7 +399,10 @@ class DtbPlatdata(object):
>          """Scan the driver folders to build a list of driver names and possible
>          aliases
>          """
> -        for (dirpath, dirnames, filenames) in os.walk('/home/sglass/u'):
> +        basedir = sys.argv[0].replace('tools/dtoc/dtoc', '')

Instead of this logic, can we pass the source-tree location into dtoc
with a flag? It could default to the current dir perhaps.

> +        if basedir == '':
> +            basedir = './'
> +        for (dirpath, dirnames, filenames) in os.walk(basedir):
>              for fn in filenames:
>                  if not fn.endswith('.c'):
>                      continue
> @@ -802,6 +805,32 @@ class DtbPlatdata(object):
>          self.out(''.join(self.get_buf()))
>          self.close_output()
>
> +    def shrink(self):
> +        """Generate a shrunk version of DTB bases on valid drivers
> +
> +        This function removes nodes from dtb which compatible string is not
> +        found in drivers. The output is saved in a file with suffix name -shrink.dtb
> +        """
> +        compat = []
> +        cmd = './tools/fdtgrep '
> +        #print(self._drivers)
> +        for dr in self._drivers.values():
> +            compat = compat + dr.compat
> +
> +        for cp in compat:
> +            #print(cp)
> +            cmd += ' -c ' + cp
> +
> +        cmd += ' -O dtb -o ' + self._dtb_fname.replace('.dtb', '') + '-shrink.dtb ' + self._dtb_fname
> +
> +        if False:
> +            with open('dt_shrink.sh', 'w+') as script:
> +                script.write(cmd)
> +
> +        os.system(cmd)
> +
> +        return
> +
>  def run_steps(args, dtb_file, config_file, include_disabled, output):
>      """Run all the steps of the dtoc tool
>
> @@ -816,6 +845,10 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
>      if not args:
>          raise ValueError('Please specify a command: struct, platdata')
>
> +    skip_scan = False
> +    if args == ['shrink']:
> +        skip_scan = True

I think that would be better as a positive variable, like 'scan'.

> +
>      plat = DtbPlatdata(dtb_file, config_file, include_disabled)
>      plat.scan_drivers()
>      plat.scan_dtb()
> @@ -823,14 +856,17 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
>      plat.scan_config()
>      plat.scan_reg_sizes()

Are those two needed with this new command?

>      plat.setup_output(output)
> -    structs = plat.scan_structs()
> -    plat.scan_phandles()
> +    if not skip_scan:
> +        structs = plat.scan_structs()
> +        plat.scan_phandles()
>
>      for cmd in args[0].split(','):
>          if cmd == 'struct':
>              plat.generate_structs(structs)
>          elif cmd == 'platdata':
>              plat.generate_tables()
> +        elif cmd == 'shrink':
> +            plat.shrink()
>          else:
>              raise ValueError("Unknown command '%s': (use: struct, platdata)" %
>                               cmd)
> --
> 2.20.1
>

Regards,
Simon

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Simon Glass]

Hi Walter,

On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Based on several reports there is an increasing concern in the impact
> of adding additional features to drivers based on compatible strings.
> A good example of this situation is found in [1].
>
> In order to reduce this impact and as an initial step for further
> reduction, propose a new way to declare compatible strings, which allows
> to only include the useful ones.

What are the useful ones?

>
> The idea is to define compatible strings in a way to be easily parsed by
> dtoc, which will be responsible to build struct udevice_id [] based on
> the compatible strings present in the dtb.
>
> Additional features can be easily added, such as define constants
> depending on the presence of compatible strings, which allows to enable
> code blocks only in such cases without the need of adding additional
> configuration options.
>
> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>
> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> ---
>  tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

I think dtoc should be able to parse the compatible strings as they
are today - e.g. see the tiny-dm stuff.

Regards,
Simon

[RFC 2/4] dtoc: add initial support for deleting DTB nodes

[Walter Lozano]

Hi Simon,

On 6/7/20 16:21, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>> This patch introduce a test for deleting DTB nodes using Python library.
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>>   tools/dtoc/dtb_platdata.py | 28 ++++++++++++++++++++++++++++
>>   tools/dtoc/fdt.py          |  3 +++
>>   2 files changed, 31 insertions(+)
> This test should go in test_dtoc.py

Thanks for checking this series. Yes, you are? right, this was an early 
version in order to check with you and Tom if the idea makes any sense.

Regards,

Walter

[RFC 1/4] dtoc: add POC for dtb shrink

[Walter Lozano]

Hi Simon,

Thanks for your time.

On 6/7/20 16:21, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 19 Jun 2020 at 15:11, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Based on several reports and discussions [1], [2] it is clear that U-Boot's
>> footprint is always a concern, and any kind of reduction is an
>> improvement.
>>
>> In particular dtb is one of the sources of footprint increment, as
>> U-Boot uses the same dtb as Linux. However is interesting to note that
>> U-Boot does not require all the nodes and properties declared in it.
>> Some improvements in this sense are already present, such as
>> removing properties based on configuration and using specific "u-boot"
>> properties to keep only specific node in SPL. However, this require
>> manual configuration.
>>
>> Additionally reducing dtb, will allow ATF for better handing FDT buffer, which
>> is an issue in some contexts [3].
>>
>> In order to improve this situation, this patch adds a proof of concept
>> for dtb shrink. The idea behind this is simple, remove all the nodes
>> from dtb which compatible string is not supported by any driver present.
>> This approach makes sense for those configuration where Linux is
>> expected to have its own dtb.
>>
>> This patch is based on the work of Simon Glass present in [4] which adds
>> support to dtoc for parsing compatible strings.
>>
>> Some early untested results shows that the reduction in size is 50 % in
>> case of mx6_cuboxi_defconfig, which is promising.
>>
>> Some additional reduction could be possible by only keeping the nodes for
>> whose compatible string is supported by any enabled driver. However,
>> this requires to add extra logic to parse config files and map
>> configuration to compatible strings.
>>
>> This proof of concept uses fdtgrep to implement the node removal, but
>> the idea is to implement this logic inside the dtoc for better handling.
>>
>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525093539.1.Ibf2d19439cde35e39192a9d4a8dad23539fae2e6 at changeid/
>> [2] http://u-boot.10912.n7.nabble.com/PATCH-v1-00-15-add-basic-driver-support-for-broadcom-NS3-soc-tt412411.html#none
>> [3] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/4512
>> [4] https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>>   tools/dtoc/dtb_platdata.py | 42 +++++++++++++++++++++++++++++++++++---
>>   1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
>> index 21cce5afb5..1df13b2cd2 100644
>> --- a/tools/dtoc/dtb_platdata.py
>> +++ b/tools/dtoc/dtb_platdata.py
>> @@ -399,7 +399,10 @@ class DtbPlatdata(object):
>>           """Scan the driver folders to build a list of driver names and possible
>>           aliases
>>           """
>> -        for (dirpath, dirnames, filenames) in os.walk('/home/sglass/u'):
>> +        basedir = sys.argv[0].replace('tools/dtoc/dtoc', '')
> Instead of this logic, can we pass the source-tree location into dtoc
> with a flag? It could default to the current dir perhaps.


Sure, no problem at all.


>
>> +        if basedir == '':
>> +            basedir = './'
>> +        for (dirpath, dirnames, filenames) in os.walk(basedir):
>>               for fn in filenames:
>>                   if not fn.endswith('.c'):
>>                       continue
>> @@ -802,6 +805,32 @@ class DtbPlatdata(object):
>>           self.out(''.join(self.get_buf()))
>>           self.close_output()
>>
>> +    def shrink(self):
>> +        """Generate a shrunk version of DTB bases on valid drivers
>> +
>> +        This function removes nodes from dtb which compatible string is not
>> +        found in drivers. The output is saved in a file with suffix name -shrink.dtb
>> +        """
>> +        compat = []
>> +        cmd = './tools/fdtgrep '
>> +        #print(self._drivers)
>> +        for dr in self._drivers.values():
>> +            compat = compat + dr.compat
>> +
>> +        for cp in compat:
>> +            #print(cp)
>> +            cmd += ' -c ' + cp
>> +
>> +        cmd += ' -O dtb -o ' + self._dtb_fname.replace('.dtb', '') + '-shrink.dtb ' + self._dtb_fname
>> +
>> +        if False:
>> +            with open('dt_shrink.sh', 'w+') as script:
>> +                script.write(cmd)
>> +
>> +        os.system(cmd)
>> +
>> +        return
>> +
>>   def run_steps(args, dtb_file, config_file, include_disabled, output):
>>       """Run all the steps of the dtoc tool
>>
>> @@ -816,6 +845,10 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
>>       if not args:
>>           raise ValueError('Please specify a command: struct, platdata')
>>
>> +    skip_scan = False
>> +    if args == ['shrink']:
>> +        skip_scan = True
> I think that would be better as a positive variable, like 'scan'.


Yes, I agree. The idea was to test the general idea, and check if it 
could be useful.


>
>> +
>>       plat = DtbPlatdata(dtb_file, config_file, include_disabled)
>>       plat.scan_drivers()
>>       plat.scan_dtb()
>> @@ -823,14 +856,17 @@ def run_steps(args, dtb_file, config_file, include_disabled, output):
>>       plat.scan_config()
>>       plat.scan_reg_sizes()
> Are those two needed with this new command?

Probably not in this version, but I was planning to use scan_config to 
check which drivers are enabled. Actually a there is a newer version in 
the link I previously left. However, as I had some issues working on top 
of the dtoc-working branch this version is back ported, in order to make 
is user to run some early/quick tests.

https://gitlab.collabora.com/wlozano/u-boot/-/tree/wip


>>       plat.setup_output(output)
>> -    structs = plat.scan_structs()
>> -    plat.scan_phandles()
>> +    if not skip_scan:
>> +        structs = plat.scan_structs()
>> +        plat.scan_phandles()
>>
>>       for cmd in args[0].split(','):
>>           if cmd == 'struct':
>>               plat.generate_structs(structs)
>>           elif cmd == 'platdata':
>>               plat.generate_tables()
>> +        elif cmd == 'shrink':
>> +            plat.shrink()
>>           else:
>>               raise ValueError("Unknown command '%s': (use: struct, platdata)" %
>>                                cmd)
>> --
>> 2.20.1
>>
Regards,

Walter

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon

On 6/7/20 16:21, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Based on several reports there is an increasing concern in the impact
>> of adding additional features to drivers based on compatible strings.
>> A good example of this situation is found in [1].
>>
>> In order to reduce this impact and as an initial step for further
>> reduction, propose a new way to declare compatible strings, which allows
>> to only include the useful ones.
> What are the useful ones?

The useful ones would be those that are used by the selected DTB by the 
current configuration. The idea of this patch is to declare all the 
possible compatible strings in a way that dtoc can generate code for 
only those which are going to be used, and in this way avoid lots of 
#ifdef like the ones shows in

http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/ 


>> The idea is to define compatible strings in a way to be easily parsed by
>> dtoc, which will be responsible to build struct udevice_id [] based on
>> the compatible strings present in the dtb.
>>
>> Additional features can be easily added, such as define constants
>> depending on the presence of compatible strings, which allows to enable
>> code blocks only in such cases without the need of adding additional
>> configuration options.
>>
>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>
>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>> ---
>>   tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
> I think dtoc should be able to parse the compatible strings as they
> are today - e.g. see the tiny-dm stuff.


Yes, I agree. My idea is that dtoc parses compatible strings as they are 
today but also in this new way. The reason for this is to allow dtoc to 
generate the code to include the useful compatible strings. Of course, 
this only makes sense if the idea of generating the compatible string 
associated? code is accepted.

What do you think?

Regards,

Walter


>
> Regards,
> Simon

[RFC 1/4] dtoc: add POC for dtb shrink

[Rasmus Villemoes]

On 19/06/2020 23.11, Walter Lozano wrote:

> Some additional reduction could be possible by only keeping the nodes for
> whose compatible string is supported by any enabled driver. However,
> this requires to add extra logic to parse config files and map
> configuration to compatible strings.

If this can be done after building the U-Boot (or SPL) ELF, can't it
just be done by doing 'grep -a' on that? Or, probably a little more
efficient, running "strings | grep -E '^[a-zA-Z0-9,._-]*' | sort -u",
slurping in the output of that in a python set() and just looking there.

Rasmus

[RFC 1/4] dtoc: add POC for dtb shrink

[Walter Lozano]

Hi Rasmus,

On 7/7/20 11:15, Rasmus Villemoes wrote:
> On 19/06/2020 23.11, Walter Lozano wrote:
>
>> Some additional reduction could be possible by only keeping the nodes for
>> whose compatible string is supported by any enabled driver. However,
>> this requires to add extra logic to parse config files and map
>> configuration to compatible strings.
> If this can be done after building the U-Boot (or SPL) ELF, can't it
> just be done by doing 'grep -a' on that? Or, probably a little more
> efficient, running "strings | grep -E '^[a-zA-Z0-9,._-]*' | sort -u",
> slurping in the output of that in a python set() and just looking there.
>
Thanks for your review and suggestion. Your approach is interesting, 
however, I wonder, won't we get a lot of strings which are not 
compatible strings? How could be filter this list to only include those 
strings that are compatible strings?

Also the idea if parsing config and Makefiles would be useful to only 
process file drivers which are going to be used, and prepare for 
instance the compatible string list as described in "[RFC 3/4] dtoc: add 
support for generate stuct udevice_id", which can be found in

https://patchwork.ozlabs.org/project/uboot/patch/20200619211140.5081-4-walter.lozano at collabora.com/

Do you think we can handle this in some other more efficient way?

Regards,

Walter

[RFC 1/4] dtoc: add POC for dtb shrink

[Rasmus Villemoes]

On 07/07/2020 16.32, Walter Lozano wrote:
> Hi Rasmus,
> 
> On 7/7/20 11:15, Rasmus Villemoes wrote:
>> On 19/06/2020 23.11, Walter Lozano wrote:
>>
>>> Some additional reduction could be possible by only keeping the nodes
>>> for
>>> whose compatible string is supported by any enabled driver. However,
>>> this requires to add extra logic to parse config files and map
>>> configuration to compatible strings.
>> If this can be done after building the U-Boot (or SPL) ELF, can't it
>> just be done by doing 'grep -a' on that? Or, probably a little more
>> efficient, running "strings | grep -E '^[a-zA-Z0-9,._-]*' | sort -u",
>> slurping in the output of that in a python set() and just looking there.
>>
> Thanks for your review and suggestion. Your approach is interesting,
> however, I wonder, won't we get a lot of strings which are not
> compatible strings? How could be filter this list to only include those
> strings that are compatible strings?

Does it matter? You have a dt node containing 'compatible =
"acme,frobnozzle"', so you want to know if any enabled driver has
"acme,frobnozzle". Sure, the brute-force grep'ing will produce lots and
lots of irrelevant strings, but the chance of a false positive
(acme,frobnozzle appearing, but not from some driver's compatible
strings) should be quite low, and false negatives can't (and must not,
of course) happen AFAICS.

> Also the idea if parsing config and Makefiles would be useful to only
> process file drivers which are going to be used, and prepare for
> instance the compatible string list as described in "[RFC 3/4] dtoc: add
> support for generate stuct udevice_id", which can be found in
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20200619211140.5081-4-walter.lozano at collabora.com/
> 
> 
> Do you think we can handle this in some other more efficient way?

I haven't read these patches very closely, just stumbled on the above. I
do think that the job of parsing Kconfig files is best left to Kconfig,
the job of parsing Makefiles is best left to make, and the job of
processing all the #ifdefery/CONFIG_IS_ENABLED stuff is best left to the
compiler (preprocessor). Trying to predict which code will or will not
be included in a given image in any way other than by building that
image sounds quite error-prone. But I may very well not have understood
what you're proposing.

Rasmus

[RFC 1/4] dtoc: add POC for dtb shrink

[Walter Lozano]

Hi Rasmus,

On 7/7/20 11:53, Rasmus Villemoes wrote:
> On 07/07/2020 16.32, Walter Lozano wrote:
>> Hi Rasmus,
>>
>> On 7/7/20 11:15, Rasmus Villemoes wrote:
>>> On 19/06/2020 23.11, Walter Lozano wrote:
>>>
>>>> Some additional reduction could be possible by only keeping the nodes
>>>> for
>>>> whose compatible string is supported by any enabled driver. However,
>>>> this requires to add extra logic to parse config files and map
>>>> configuration to compatible strings.
>>> If this can be done after building the U-Boot (or SPL) ELF, can't it
>>> just be done by doing 'grep -a' on that? Or, probably a little more
>>> efficient, running "strings | grep -E '^[a-zA-Z0-9,._-]*' | sort -u",
>>> slurping in the output of that in a python set() and just looking there.
>>>
>> Thanks for your review and suggestion. Your approach is interesting,
>> however, I wonder, won't we get a lot of strings which are not
>> compatible strings? How could be filter this list to only include those
>> strings that are compatible strings?
> Does it matter? You have a dt node containing 'compatible =
> "acme,frobnozzle"', so you want to know if any enabled driver has
> "acme,frobnozzle". Sure, the brute-force grep'ing will produce lots and
> lots of irrelevant strings, but the chance of a false positive
> (acme,frobnozzle appearing, but not from some driver's compatible
> strings) should be quite low, and false negatives can't (and must not,
> of course) happen AFAICS.

I agree with you regarding the fact that the chances of a false positive 
are low, and also causes no big issue, just some node we don't remove. 
However as much of the support in dtoc is already there for other 
features I I think is cleaner in that way. Not sure what other people think.

>> Also the idea if parsing config and Makefiles would be useful to only
>> process file drivers which are going to be used, and prepare for
>> instance the compatible string list as described in "[RFC 3/4] dtoc: add
>> support for generate stuct udevice_id", which can be found in
>>
>> https://patchwork.ozlabs.org/project/uboot/patch/20200619211140.5081-4-walter.lozano at collabora.com/
>>
>>
>> Do you think we can handle this in some other more efficient way?
> I haven't read these patches very closely, just stumbled on the above. I
> do think that the job of parsing Kconfig files is best left to Kconfig,
> the job of parsing Makefiles is best left to make, and the job of
> processing all the #ifdefery/CONFIG_IS_ENABLED stuff is best left to the
> compiler (preprocessor). Trying to predict which code will or will not
> be included in a given image in any way other than by building that
> image sounds quite error-prone. But I may very well not have understood
> what you're proposing.
>
Yes, I also agree with you, that this could be error-prone, that is my 
main concern, and that is the reason for sending this RFC, to explore 
this (and possible other) ideas or approaches to reduce the footprint.

To elaborate a bit more, I think the scanning the Makefiles could be 
error-prone, and could lead to some file driver not being taken into 
account, which could lead to runtime issues. On the other hand, I don't 
see much of a problem to use some macros which will be implemented in 
code generated by dtoc, as this idea is already used for OF_PLATDATA.

If you see some specific issue please let me know.

Thanks for sharing you thoughts, it is very useful. I will keep thinking 
about your suggestions and different alternatives.

Regards,

Walter

> Rasmus
>

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Simon Glass]

Hi Walter,

On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon
>
> On 6/7/20 16:21, Simon Glass wrote:
> > Hi Walter,
> >
> > On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Based on several reports there is an increasing concern in the impact
> >> of adding additional features to drivers based on compatible strings.
> >> A good example of this situation is found in [1].
> >>
> >> In order to reduce this impact and as an initial step for further
> >> reduction, propose a new way to declare compatible strings, which allows
> >> to only include the useful ones.
> > What are the useful ones?
>
> The useful ones would be those that are used by the selected DTB by the
> current configuration. The idea of this patch is to declare all the
> possible compatible strings in a way that dtoc can generate code for
> only those which are going to be used, and in this way avoid lots of
> #ifdef like the ones shows in
>
> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>
>
> >> The idea is to define compatible strings in a way to be easily parsed by
> >> dtoc, which will be responsible to build struct udevice_id [] based on
> >> the compatible strings present in the dtb.
> >>
> >> Additional features can be easily added, such as define constants
> >> depending on the presence of compatible strings, which allows to enable
> >> code blocks only in such cases without the need of adding additional
> >> configuration options.
> >>
> >> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>
> >> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >> ---
> >>   tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 32 insertions(+)
> > I think dtoc should be able to parse the compatible strings as they
> > are today - e.g. see the tiny-dm stuff.
>
>
> Yes, I agree. My idea is that dtoc parses compatible strings as they are
> today but also in this new way. The reason for this is to allow dtoc to
> generate the code to include the useful compatible strings. Of course,
> this only makes sense if the idea of generating the compatible string
> associated  code is accepted.
>
> What do you think?

I think this is useful and better than using #ifdef in the source code
for this sort of thing. We need a way to specify the driver_data value
as well, right?

Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

Regards,
Simon

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon,

On 26/7/20 11:53, Simon Glass wrote:
> Hi Walter,
>
> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon
>>
>> On 6/7/20 16:21, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Based on several reports there is an increasing concern in the impact
>>>> of adding additional features to drivers based on compatible strings.
>>>> A good example of this situation is found in [1].
>>>>
>>>> In order to reduce this impact and as an initial step for further
>>>> reduction, propose a new way to declare compatible strings, which allows
>>>> to only include the useful ones.
>>> What are the useful ones?
>> The useful ones would be those that are used by the selected DTB by the
>> current configuration. The idea of this patch is to declare all the
>> possible compatible strings in a way that dtoc can generate code for
>> only those which are going to be used, and in this way avoid lots of
>> #ifdef like the ones shows in
>>
>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>
>>
>>>> The idea is to define compatible strings in a way to be easily parsed by
>>>> dtoc, which will be responsible to build struct udevice_id [] based on
>>>> the compatible strings present in the dtb.
>>>>
>>>> Additional features can be easily added, such as define constants
>>>> depending on the presence of compatible strings, which allows to enable
>>>> code blocks only in such cases without the need of adding additional
>>>> configuration options.
>>>>
>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>
>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>> ---
>>>>    tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>> I think dtoc should be able to parse the compatible strings as they
>>> are today - e.g. see the tiny-dm stuff.
>>
>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
>> today but also in this new way. The reason for this is to allow dtoc to
>> generate the code to include the useful compatible strings. Of course,
>> this only makes sense if the idea of generating the compatible string
>> associated  code is accepted.
>>
>> What do you think?
> I think this is useful and better than using #ifdef in the source code
> for this sort of thing. We need a way to specify the driver_data value
> as well, right?

Yes, I agree, it is better than #ifdef and c/ould give us some extra 
functionality.

What doe you mean by driver_data value? Are you referring to the data 
field? like

static struct esdhc_soc_data usdhc_imx7d_data = {
         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
                         | ESDHC_FLAG_HS400,
};

If that is the case, I was thinking in defining a constant when specific 
compatible strings are enabled by dtoc, based in the above case

#ifdef FSL_ESDHC_IMX_V2
static struct esdhc_soc_data usdhc_imx7d_data = {
         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
                         | ESDHC_FLAG_HS400,
};
#endif

COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

So when dtoc parses COMPATIBLE and determines that compatible 
"fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

This is alsoAs I comment you in the tread about tiny-dm I think that we 
can save some space following your suggestions, and for instance implement


> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
>
I totally agree, naming is very important, and DT_COMPAT() is much better.

What I don't fully understand is what are the cases for DT_OPT_COMPAT(), 
could you please clarify?

Regards,

Walter

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Simon Glass]

Hi Walter,

On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 26/7/20 11:53, Simon Glass wrote:
> > Hi Walter,
> >
> > On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon
> >>
> >> On 6/7/20 16:21, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Based on several reports there is an increasing concern in the impact
> >>>> of adding additional features to drivers based on compatible strings.
> >>>> A good example of this situation is found in [1].
> >>>>
> >>>> In order to reduce this impact and as an initial step for further
> >>>> reduction, propose a new way to declare compatible strings, which allows
> >>>> to only include the useful ones.
> >>> What are the useful ones?
> >> The useful ones would be those that are used by the selected DTB by the
> >> current configuration. The idea of this patch is to declare all the
> >> possible compatible strings in a way that dtoc can generate code for
> >> only those which are going to be used, and in this way avoid lots of
> >> #ifdef like the ones shows in
> >>
> >> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>
> >>
> >>>> The idea is to define compatible strings in a way to be easily parsed by
> >>>> dtoc, which will be responsible to build struct udevice_id [] based on
> >>>> the compatible strings present in the dtb.
> >>>>
> >>>> Additional features can be easily added, such as define constants
> >>>> depending on the presence of compatible strings, which allows to enable
> >>>> code blocks only in such cases without the need of adding additional
> >>>> configuration options.
> >>>>
> >>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>>>
> >>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>> ---
> >>>>    tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 32 insertions(+)
> >>> I think dtoc should be able to parse the compatible strings as they
> >>> are today - e.g. see the tiny-dm stuff.
> >>
> >> Yes, I agree. My idea is that dtoc parses compatible strings as they are
> >> today but also in this new way. The reason for this is to allow dtoc to
> >> generate the code to include the useful compatible strings. Of course,
> >> this only makes sense if the idea of generating the compatible string
> >> associated  code is accepted.
> >>
> >> What do you think?
> > I think this is useful and better than using #ifdef in the source code
> > for this sort of thing. We need a way to specify the driver_data value
> > as well, right?
>
> Yes, I agree, it is better than #ifdef and c/ould give us some extra
> functionality.
>
> What doe you mean by driver_data value? Are you referring to the data
> field? like
>
> static struct esdhc_soc_data usdhc_imx7d_data = {
>          .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>                          | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>                          | ESDHC_FLAG_HS400,
> };
>

Actually I was talking about the .data member in struct udevice_id.

> If that is the case, I was thinking in defining a constant when specific
> compatible strings are enabled by dtoc, based in the above case
>
> #ifdef FSL_ESDHC_IMX_V2
> static struct esdhc_soc_data usdhc_imx7d_data = {
>          .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>                          | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>                          | ESDHC_FLAG_HS400,
> };
> #endif
>
> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
>
> So when dtoc parses COMPATIBLE and determines that compatible
> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

I think we can put that data in the dt-platdata.c file perhaps.

>
> This is alsoAs I comment you in the tread about tiny-dm I think that we
> can save some space following your suggestions, and for instance implement
>
>
> > Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
> > even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
> >
> I totally agree, naming is very important, and DT_COMPAT() is much better.
>
> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
> could you please clarify?

It's just an alternative name, with OPT meaning optional. But I think
we can leave out the OPT.

Regards,
Simon

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon,

On 28/7/20 23:42, Simon Glass wrote:
> Hi Walter,
>
> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon,
>>
>> On 26/7/20 11:53, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Hi Simon
>>>>
>>>> On 6/7/20 16:21, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>> Based on several reports there is an increasing concern in the impact
>>>>>> of adding additional features to drivers based on compatible strings.
>>>>>> A good example of this situation is found in [1].
>>>>>>
>>>>>> In order to reduce this impact and as an initial step for further
>>>>>> reduction, propose a new way to declare compatible strings, which allows
>>>>>> to only include the useful ones.
>>>>> What are the useful ones?
>>>> The useful ones would be those that are used by the selected DTB by the
>>>> current configuration. The idea of this patch is to declare all the
>>>> possible compatible strings in a way that dtoc can generate code for
>>>> only those which are going to be used, and in this way avoid lots of
>>>> #ifdef like the ones shows in
>>>>
>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>
>>>>
>>>>>> The idea is to define compatible strings in a way to be easily parsed by
>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
>>>>>> the compatible strings present in the dtb.
>>>>>>
>>>>>> Additional features can be easily added, such as define constants
>>>>>> depending on the presence of compatible strings, which allows to enable
>>>>>> code blocks only in such cases without the need of adding additional
>>>>>> configuration options.
>>>>>>
>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>>>
>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>>>> ---
>>>>>>     tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>> I think dtoc should be able to parse the compatible strings as they
>>>>> are today - e.g. see the tiny-dm stuff.
>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
>>>> today but also in this new way. The reason for this is to allow dtoc to
>>>> generate the code to include the useful compatible strings. Of course,
>>>> this only makes sense if the idea of generating the compatible string
>>>> associated  code is accepted.
>>>>
>>>> What do you think?
>>> I think this is useful and better than using #ifdef in the source code
>>> for this sort of thing. We need a way to specify the driver_data value
>>> as well, right?
>> Yes, I agree, it is better than #ifdef and c/ould give us some extra
>> functionality.
>>
>> What doe you mean by driver_data value? Are you referring to the data
>> field? like
>>
>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>                           | ESDHC_FLAG_HS400,
>> };
>>
> Actually I was talking about the .data member in struct udevice_id.
So my example is correct, as usdhc_imx7d_data is the value for .data in 
one case as shown bellow.
>> If that is the case, I was thinking in defining a constant when specific
>> compatible strings are enabled by dtoc, based in the above case
>>
>> #ifdef FSL_ESDHC_IMX_V2
>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>                           | ESDHC_FLAG_HS400,
>> };
>> #endif
>>
>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
>>
>> So when dtoc parses COMPATIBLE and determines that compatible
>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
> I think we can put that data in the dt-platdata.c file perhaps.

I thought the same at the beginning, but then I changed my mind, because

1- in order to work dt-platdata.c will need to include several different 
.h, in this example, only for fsl_esdhc_imx to work, we will need to 
include fsl_esdhc_imx.h where all the flags are defined.

2- it case we use #define to avoid having to include several different 
.h probably the errors will be more difficult to catch/debug

What do you think?

>
>> This is alsoAs I comment you in the tread about tiny-dm I think that we
>> can save some space following your suggestions, and for instance implement
>>
>>
>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
>>>
>> I totally agree, naming is very important, and DT_COMPAT() is much better.
>>
>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
>> could you please clarify?
> It's just an alternative name, with OPT meaning optional. But I think
> we can leave out the OPT.

Thanks for clarifying.

Regards,

Walter

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Simon Glass]

Hi Walter,

On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
> On 28/7/20 23:42, Simon Glass wrote:
> > Hi Walter,
> >
> > On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon,
> >>
> >> On 26/7/20 11:53, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Hi Simon
> >>>>
> >>>> On 6/7/20 16:21, Simon Glass wrote:
> >>>>> Hi Walter,
> >>>>>
> >>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>> Based on several reports there is an increasing concern in the impact
> >>>>>> of adding additional features to drivers based on compatible strings.
> >>>>>> A good example of this situation is found in [1].
> >>>>>>
> >>>>>> In order to reduce this impact and as an initial step for further
> >>>>>> reduction, propose a new way to declare compatible strings, which allows
> >>>>>> to only include the useful ones.
> >>>>> What are the useful ones?
> >>>> The useful ones would be those that are used by the selected DTB by the
> >>>> current configuration. The idea of this patch is to declare all the
> >>>> possible compatible strings in a way that dtoc can generate code for
> >>>> only those which are going to be used, and in this way avoid lots of
> >>>> #ifdef like the ones shows in
> >>>>
> >>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>>>
> >>>>
> >>>>>> The idea is to define compatible strings in a way to be easily parsed by
> >>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
> >>>>>> the compatible strings present in the dtb.
> >>>>>>
> >>>>>> Additional features can be easily added, such as define constants
> >>>>>> depending on the presence of compatible strings, which allows to enable
> >>>>>> code blocks only in such cases without the need of adding additional
> >>>>>> configuration options.
> >>>>>>
> >>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>>>>>
> >>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>>>> ---
> >>>>>>     tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 32 insertions(+)
> >>>>> I think dtoc should be able to parse the compatible strings as they
> >>>>> are today - e.g. see the tiny-dm stuff.
> >>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
> >>>> today but also in this new way. The reason for this is to allow dtoc to
> >>>> generate the code to include the useful compatible strings. Of course,
> >>>> this only makes sense if the idea of generating the compatible string
> >>>> associated  code is accepted.
> >>>>
> >>>> What do you think?
> >>> I think this is useful and better than using #ifdef in the source code
> >>> for this sort of thing. We need a way to specify the driver_data value
> >>> as well, right?
> >> Yes, I agree, it is better than #ifdef and c/ould give us some extra
> >> functionality.
> >>
> >> What doe you mean by driver_data value? Are you referring to the data
> >> field? like
> >>
> >> static struct esdhc_soc_data usdhc_imx7d_data = {
> >>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> >>                           | ESDHC_FLAG_HS400,
> >> };
> >>
> > Actually I was talking about the .data member in struct udevice_id.
> So my example is correct, as usdhc_imx7d_data is the value for .data in
> one case as shown bellow.
> >> If that is the case, I was thinking in defining a constant when specific
> >> compatible strings are enabled by dtoc, based in the above case
> >>
> >> #ifdef FSL_ESDHC_IMX_V2
> >> static struct esdhc_soc_data usdhc_imx7d_data = {
> >>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> >>                           | ESDHC_FLAG_HS400,
> >> };
> >> #endif
> >>
> >> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
> >>
> >> So when dtoc parses COMPATIBLE and determines that compatible
> >> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
> > I think we can put that data in the dt-platdata.c file perhaps.
>
> I thought the same at the beginning, but then I changed my mind, because
>
> 1- in order to work dt-platdata.c will need to include several different
> .h, in this example, only for fsl_esdhc_imx to work, we will need to
> include fsl_esdhc_imx.h where all the flags are defined.

Yes I hit that problem with the tiny-dm experiment and ended up adding
a macro to specify the header.

Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
usdhc_imx7d_data not being used? If so, we could use _maybe_unused

>
> 2- it case we use #define to avoid having to include several different
> .h probably the errors will be more difficult to catch/debug

Yes we would have to include the real header, not just copy bits out of it.
>
> What do you think?

I'm not sure overall. On the one hand I don't really like hiding C
code inside macros. On the other, it avoids the horrible manual
#ifdefs. So on balance I think your idea is the best approach. We can
always refine it later and it is easier to iterate on this sort of
thing if it is actually being used by some boards.


>
> >
> >> This is alsoAs I comment you in the tread about tiny-dm I think that we
> >> can save some space following your suggestions, and for instance implement
> >>
> >>
> >>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
> >>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
> >>>
> >> I totally agree, naming is very important, and DT_COMPAT() is much better.
> >>
> >> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
> >> could you please clarify?
> > It's just an alternative name, with OPT meaning optional. But I think
> > we can leave out the OPT.
>
> Thanks for clarifying.

Regards,
SImon

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon

On 7/8/20 13:23, Simon Glass wrote:
> Hi Walter,
>
> On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon,
>>
>> On 28/7/20 23:42, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 26/7/20 11:53, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>> Hi Simon
>>>>>>
>>>>>> On 6/7/20 16:21, Simon Glass wrote:
>>>>>>> Hi Walter,
>>>>>>>
>>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>>>> Based on several reports there is an increasing concern in the impact
>>>>>>>> of adding additional features to drivers based on compatible strings.
>>>>>>>> A good example of this situation is found in [1].
>>>>>>>>
>>>>>>>> In order to reduce this impact and as an initial step for further
>>>>>>>> reduction, propose a new way to declare compatible strings, which allows
>>>>>>>> to only include the useful ones.
>>>>>>> What are the useful ones?
>>>>>> The useful ones would be those that are used by the selected DTB by the
>>>>>> current configuration. The idea of this patch is to declare all the
>>>>>> possible compatible strings in a way that dtoc can generate code for
>>>>>> only those which are going to be used, and in this way avoid lots of
>>>>>> #ifdef like the ones shows in
>>>>>>
>>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>>>
>>>>>>
>>>>>>>> The idea is to define compatible strings in a way to be easily parsed by
>>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
>>>>>>>> the compatible strings present in the dtb.
>>>>>>>>
>>>>>>>> Additional features can be easily added, such as define constants
>>>>>>>> depending on the presence of compatible strings, which allows to enable
>>>>>>>> code blocks only in such cases without the need of adding additional
>>>>>>>> configuration options.
>>>>>>>>
>>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>>>>>
>>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>>>>>> ---
>>>>>>>>      tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 32 insertions(+)
>>>>>>> I think dtoc should be able to parse the compatible strings as they
>>>>>>> are today - e.g. see the tiny-dm stuff.
>>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
>>>>>> today but also in this new way. The reason for this is to allow dtoc to
>>>>>> generate the code to include the useful compatible strings. Of course,
>>>>>> this only makes sense if the idea of generating the compatible string
>>>>>> associated  code is accepted.
>>>>>>
>>>>>> What do you think?
>>>>> I think this is useful and better than using #ifdef in the source code
>>>>> for this sort of thing. We need a way to specify the driver_data value
>>>>> as well, right?
>>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra
>>>> functionality.
>>>>
>>>> What doe you mean by driver_data value? Are you referring to the data
>>>> field? like
>>>>
>>>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>>>                            | ESDHC_FLAG_HS400,
>>>> };
>>>>
>>> Actually I was talking about the .data member in struct udevice_id.
>> So my example is correct, as usdhc_imx7d_data is the value for .data in
>> one case as shown bellow.
>>>> If that is the case, I was thinking in defining a constant when specific
>>>> compatible strings are enabled by dtoc, based in the above case
>>>>
>>>> #ifdef FSL_ESDHC_IMX_V2
>>>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>>>                            | ESDHC_FLAG_HS400,
>>>> };
>>>> #endif
>>>>
>>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
>>>>
>>>> So when dtoc parses COMPATIBLE and determines that compatible
>>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
>>> I think we can put that data in the dt-platdata.c file perhaps.
>> I thought the same at the beginning, but then I changed my mind, because
>>
>> 1- in order to work dt-platdata.c will need to include several different
>> .h, in this example, only for fsl_esdhc_imx to work, we will need to
>> include fsl_esdhc_imx.h where all the flags are defined.
> Yes I hit that problem with the tiny-dm experiment and ended up adding
> a macro to specify the header.

I haven't seen that. I will check it. Thanks.


> Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
> usdhc_imx7d_data not being used? If so, we could use _maybe_unused

Well, it is not that I really need it, but I try to give the possibility 
to add some #ifdef or similar based on compatible strings, the 
usdhc_imx7d_data was just an example. A more interesting example could 
be some code that makes sense only on specific "compatible string" cases 
and using #ifdef or if would save some bytes in other cases.

>> 2- it case we use #define to avoid having to include several different
>> .h probably the errors will be more difficult to catch/debug
> Yes we would have to include the real header, not just copy bits out of it.

Yes, for that reason I feel it could lead to more issues than benefits. 
However, it is only a personal opinion, I'm not completely sure.


>> What do you think?
> I'm not sure overall. On the one hand I don't really like hiding C
> code inside macros. On the other, it avoids the horrible manual
> #ifdefs. So on balance I think your idea is the best approach. We can
> always refine it later and it is easier to iterate on this sort of
> thing if it is actually being used by some boards.


It is exactly what I feel, we need to find the best balance here, and it 
always easy to improve it if this is used by some boards.

>>>> This is alsoAs I comment you in the tread about tiny-dm I think that we
>>>> can save some space following your suggestions, and for instance implement
>>>>
>>>>
>>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
>>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
>>>>>
>>>> I totally agree, naming is very important, and DT_COMPAT() is much better.
>>>>
>>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
>>>> could you please clarify?
>>> It's just an alternative name, with OPT meaning optional. But I think
>>> we can leave out the OPT.
>> Thanks for clarifying.

Thanks for your review and comments.

BTW, as this work is based in some of the improvements you developed for 
tiny-dm, I was wondering what are your plans regarding it.

Regards,

Walter

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Simon Glass]

Hi Walter,

On Fri, 7 Aug 2020 at 11:23, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon
>
> On 7/8/20 13:23, Simon Glass wrote:
> > Hi Walter,
> >
> > On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:
> >> Hi Simon,
> >>
> >> On 28/7/20 23:42, Simon Glass wrote:
> >>> Hi Walter,
> >>>
> >>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 26/7/20 11:53, Simon Glass wrote:
> >>>>> Hi Walter,
> >>>>>
> >>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>> Hi Simon
> >>>>>>
> >>>>>> On 6/7/20 16:21, Simon Glass wrote:
> >>>>>>> Hi Walter,
> >>>>>>>
> >>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
> >>>>>>>> Based on several reports there is an increasing concern in the impact
> >>>>>>>> of adding additional features to drivers based on compatible strings.
> >>>>>>>> A good example of this situation is found in [1].
> >>>>>>>>
> >>>>>>>> In order to reduce this impact and as an initial step for further
> >>>>>>>> reduction, propose a new way to declare compatible strings, which allows
> >>>>>>>> to only include the useful ones.
> >>>>>>> What are the useful ones?
> >>>>>> The useful ones would be those that are used by the selected DTB by the
> >>>>>> current configuration. The idea of this patch is to declare all the
> >>>>>> possible compatible strings in a way that dtoc can generate code for
> >>>>>> only those which are going to be used, and in this way avoid lots of
> >>>>>> #ifdef like the ones shows in
> >>>>>>
> >>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>>>>>
> >>>>>>
> >>>>>>>> The idea is to define compatible strings in a way to be easily parsed by
> >>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
> >>>>>>>> the compatible strings present in the dtb.
> >>>>>>>>
> >>>>>>>> Additional features can be easily added, such as define constants
> >>>>>>>> depending on the presence of compatible strings, which allows to enable
> >>>>>>>> code blocks only in such cases without the need of adding additional
> >>>>>>>> configuration options.
> >>>>>>>>
> >>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
> >>>>>>>>
> >>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
> >>>>>>>> ---
> >>>>>>>>      tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
> >>>>>>>>      1 file changed, 32 insertions(+)
> >>>>>>> I think dtoc should be able to parse the compatible strings as they
> >>>>>>> are today - e.g. see the tiny-dm stuff.
> >>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
> >>>>>> today but also in this new way. The reason for this is to allow dtoc to
> >>>>>> generate the code to include the useful compatible strings. Of course,
> >>>>>> this only makes sense if the idea of generating the compatible string
> >>>>>> associated  code is accepted.
> >>>>>>
> >>>>>> What do you think?
> >>>>> I think this is useful and better than using #ifdef in the source code
> >>>>> for this sort of thing. We need a way to specify the driver_data value
> >>>>> as well, right?
> >>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra
> >>>> functionality.
> >>>>
> >>>> What doe you mean by driver_data value? Are you referring to the data
> >>>> field? like
> >>>>
> >>>> static struct esdhc_soc_data usdhc_imx7d_data = {
> >>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> >>>>                            | ESDHC_FLAG_HS400,
> >>>> };
> >>>>
> >>> Actually I was talking about the .data member in struct udevice_id.
> >> So my example is correct, as usdhc_imx7d_data is the value for .data in
> >> one case as shown bellow.
> >>>> If that is the case, I was thinking in defining a constant when specific
> >>>> compatible strings are enabled by dtoc, based in the above case
> >>>>
> >>>> #ifdef FSL_ESDHC_IMX_V2
> >>>> static struct esdhc_soc_data usdhc_imx7d_data = {
> >>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> >>>>                            | ESDHC_FLAG_HS400,
> >>>> };
> >>>> #endif
> >>>>
> >>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
> >>>>
> >>>> So when dtoc parses COMPATIBLE and determines that compatible
> >>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
> >>> I think we can put that data in the dt-platdata.c file perhaps.
> >> I thought the same at the beginning, but then I changed my mind, because
> >>
> >> 1- in order to work dt-platdata.c will need to include several different
> >> .h, in this example, only for fsl_esdhc_imx to work, we will need to
> >> include fsl_esdhc_imx.h where all the flags are defined.
> > Yes I hit that problem with the tiny-dm experiment and ended up adding
> > a macro to specify the header.
>
> I haven't seen that. I will check it. Thanks.
>
>
> > Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
> > usdhc_imx7d_data not being used? If so, we could use _maybe_unused
>
> Well, it is not that I really need it, but I try to give the possibility
> to add some #ifdef or similar based on compatible strings, the
> usdhc_imx7d_data was just an example. A more interesting example could
> be some code that makes sense only on specific "compatible string" cases
> and using #ifdef or if would save some bytes in other cases.
>
> >> 2- it case we use #define to avoid having to include several different
> >> .h probably the errors will be more difficult to catch/debug
> > Yes we would have to include the real header, not just copy bits out of it.
>
> Yes, for that reason I feel it could lead to more issues than benefits.
> However, it is only a personal opinion, I'm not completely sure.
>
>
> >> What do you think?
> > I'm not sure overall. On the one hand I don't really like hiding C
> > code inside macros. On the other, it avoids the horrible manual
> > #ifdefs. So on balance I think your idea is the best approach. We can
> > always refine it later and it is easier to iterate on this sort of
> > thing if it is actually being used by some boards.
>
>
> It is exactly what I feel, we need to find the best balance here, and it
> always easy to improve it if this is used by some boards.
>
> >>>> This is alsoAs I comment you in the tread about tiny-dm I think that we
> >>>> can save some space following your suggestions, and for instance implement
> >>>>
> >>>>
> >>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
> >>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
> >>>>>
> >>>> I totally agree, naming is very important, and DT_COMPAT() is much better.
> >>>>
> >>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
> >>>> could you please clarify?
> >>> It's just an alternative name, with OPT meaning optional. But I think
> >>> we can leave out the OPT.
> >> Thanks for clarifying.
>
> Thanks for your review and comments.
>
> BTW, as this work is based in some of the improvements you developed for
> tiny-dm, I was wondering what are your plans regarding it.

I haven't had a lot of time recently. Don't let my side hold you up,
though. I am hoping to look at the platdata parent stuff soon.

Regards,
SImon

[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon,

On 6/9/20 22:44, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 7 Aug 2020 at 11:23, Walter Lozano <walter.lozano@collabora.com> wrote:
>> Hi Simon
>>
>> On 7/8/20 13:23, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 28/7/20 23:42, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 26/7/20 11:53, Simon Glass wrote:
>>>>>>> Hi Walter,
>>>>>>>
>>>>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>>>> Hi Simon
>>>>>>>>
>>>>>>>> On 6/7/20 16:21, Simon Glass wrote:
>>>>>>>>> Hi Walter,
>>>>>>>>>
>>>>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:
>>>>>>>>>> Based on several reports there is an increasing concern in the impact
>>>>>>>>>> of adding additional features to drivers based on compatible strings.
>>>>>>>>>> A good example of this situation is found in [1].
>>>>>>>>>>
>>>>>>>>>> In order to reduce this impact and as an initial step for further
>>>>>>>>>> reduction, propose a new way to declare compatible strings, which allows
>>>>>>>>>> to only include the useful ones.
>>>>>>>>> What are the useful ones?
>>>>>>>> The useful ones would be those that are used by the selected DTB by the
>>>>>>>> current configuration. The idea of this patch is to declare all the
>>>>>>>> possible compatible strings in a way that dtoc can generate code for
>>>>>>>> only those which are going to be used, and in this way avoid lots of
>>>>>>>> #ifdef like the ones shows in
>>>>>>>>
>>>>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>>>>>
>>>>>>>>
>>>>>>>>>> The idea is to define compatible strings in a way to be easily parsed by
>>>>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
>>>>>>>>>> the compatible strings present in the dtb.
>>>>>>>>>>
>>>>>>>>>> Additional features can be easily added, such as define constants
>>>>>>>>>> depending on the presence of compatible strings, which allows to enable
>>>>>>>>>> code blocks only in such cases without the need of adding additional
>>>>>>>>>> configuration options.
>>>>>>>>>>
>>>>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>
>>>>>>>>>> ---
>>>>>>>>>>       tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>>>       1 file changed, 32 insertions(+)
>>>>>>>>> I think dtoc should be able to parse the compatible strings as they
>>>>>>>>> are today - e.g. see the tiny-dm stuff.
>>>>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
>>>>>>>> today but also in this new way. The reason for this is to allow dtoc to
>>>>>>>> generate the code to include the useful compatible strings. Of course,
>>>>>>>> this only makes sense if the idea of generating the compatible string
>>>>>>>> associated  code is accepted.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>> I think this is useful and better than using #ifdef in the source code
>>>>>>> for this sort of thing. We need a way to specify the driver_data value
>>>>>>> as well, right?
>>>>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra
>>>>>> functionality.
>>>>>>
>>>>>> What doe you mean by driver_data value? Are you referring to the data
>>>>>> field? like
>>>>>>
>>>>>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>>>>>             .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>>>>                             | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>>>>>                             | ESDHC_FLAG_HS400,
>>>>>> };
>>>>>>
>>>>> Actually I was talking about the .data member in struct udevice_id.
>>>> So my example is correct, as usdhc_imx7d_data is the value for .data in
>>>> one case as shown bellow.
>>>>>> If that is the case, I was thinking in defining a constant when specific
>>>>>> compatible strings are enabled by dtoc, based in the above case
>>>>>>
>>>>>> #ifdef FSL_ESDHC_IMX_V2
>>>>>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>>>>>             .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>>>>                             | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>>>>>                             | ESDHC_FLAG_HS400,
>>>>>> };
>>>>>> #endif
>>>>>>
>>>>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
>>>>>>
>>>>>> So when dtoc parses COMPATIBLE and determines that compatible
>>>>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
>>>>> I think we can put that data in the dt-platdata.c file perhaps.
>>>> I thought the same at the beginning, but then I changed my mind, because
>>>>
>>>> 1- in order to work dt-platdata.c will need to include several different
>>>> .h, in this example, only for fsl_esdhc_imx to work, we will need to
>>>> include fsl_esdhc_imx.h where all the flags are defined.
>>> Yes I hit that problem with the tiny-dm experiment and ended up adding
>>> a macro to specify the header.
>> I haven't seen that. I will check it. Thanks.
>>
>>
>>> Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
>>> usdhc_imx7d_data not being used? If so, we could use _maybe_unused
>> Well, it is not that I really need it, but I try to give the possibility
>> to add some #ifdef or similar based on compatible strings, the
>> usdhc_imx7d_data was just an example. A more interesting example could
>> be some code that makes sense only on specific "compatible string" cases
>> and using #ifdef or if would save some bytes in other cases.
>>
>>>> 2- it case we use #define to avoid having to include several different
>>>> .h probably the errors will be more difficult to catch/debug
>>> Yes we would have to include the real header, not just copy bits out of it.
>> Yes, for that reason I feel it could lead to more issues than benefits.
>> However, it is only a personal opinion, I'm not completely sure.
>>
>>
>>>> What do you think?
>>> I'm not sure overall. On the one hand I don't really like hiding C
>>> code inside macros. On the other, it avoids the horrible manual
>>> #ifdefs. So on balance I think your idea is the best approach. We can
>>> always refine it later and it is easier to iterate on this sort of
>>> thing if it is actually being used by some boards.
>>
>> It is exactly what I feel, we need to find the best balance here, and it
>> always easy to improve it if this is used by some boards.
>>
>>>>>> This is alsoAs I comment you in the tread about tiny-dm I think that we
>>>>>> can save some space following your suggestions, and for instance implement
>>>>>>
>>>>>>
>>>>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
>>>>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
>>>>>>>
>>>>>> I totally agree, naming is very important, and DT_COMPAT() is much better.
>>>>>>
>>>>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
>>>>>> could you please clarify?
>>>>> It's just an alternative name, with OPT meaning optional. But I think
>>>>> we can leave out the OPT.
>>>> Thanks for clarifying.
>> Thanks for your review and comments.
>>
>> BTW, as this work is based in some of the improvements you developed for
>> tiny-dm, I was wondering what are your plans regarding it.
> I haven't had a lot of time recently. Don't let my side hold you up,
> though. I am hoping to look at the platdata parent stuff soon.


Thanks for the warning. I have planed to send a patch for this but I 
have also been a bit busy. However, I hope to have some time this week 
to prepare and send it.

Regards,

Walter