[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 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.


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