| Message ID | 20200619211140.5081-2-walter.lozano@collabora.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | drivers: footprint reduction proposal | expand |
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@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
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@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
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
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@collabora.com/ Do you think we can handle this in some other more efficient way? Regards, Walter
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@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
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@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 >
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)
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@collabora.com> --- tools/dtoc/dtb_platdata.py | 42 +++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)