[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 at 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@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 at 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