Message-ID: <599388D0.9050108@gmail.com>
Date:   Tue, 15 Aug 2017 16:50:40 -0700
From:   Frank Rowand <frowand.list@...il.com>
To:     Tom Rini <trini@...sulko.com>, devicetree@...r.kernel.org
Cc:     Tero Kristo <t-kristo@...com>, Nishanth Menon <nm@...com>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Sekhar Nori <nsekhar@...com>, Rob Herring <robh+dt@...nel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Michal Marek <mmarek@...e.com>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH] devicetree: Enable generation of __symbols__ in all dtb
 files

On 08/15/17 14:15, Tom Rini wrote:
> With support for stacked overlays being part of libfdt it is now
> possible and likely that overlays which require __symbols__ will be
> applied to the dtb files generated by the kernel.  This is done by
> passing -@ to dtc.  This does increase the filesize (and resident memory
> usage) based on the number of __symbol__ entries added to match the
> contents of the dts.
> 
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: Frank Rowand <frowand.list@...il.com>
> Cc: Masahiro Yamada <yamada.masahiro@...ionext.com>
> Cc: Michal Marek <mmarek@...e.com>
> Cc: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
> Cc: devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> CC: linux-kbuild@...r.kernel.org
> Signed-off-by: Tom Rini <trini@...sulko.com>
> ---
> In order for a dtb file to be useful with all types of overlays, it
> needs to be generated with the -@ flag passed to dtc so that __symbols__
> are generated.  This however is not free, and increases the resulting
> dtb file by up to approximately 50% today.  In the current worst case
> this is moving from 88KiB to 133KiB.  In talking with Frank about this,
> he outlined 3 possible ways (with the 4th option of something else
> entirely).
> 
> 1. Make passing -@ to dtc be dependent upon some CONFIG symbol.
> 2. In the kernel, if the kernel does not have overlay support, discard
> the __symbols__ information that we've been passed.
> 3. Have the bootloader pass in, or not, __symbols__ information.

I also was hoping that other people might have ideas for additional
approaches.


> This patch is an attempt to implement something between the 3rd option
> and a different, 4th option.  Frank was thinking that we might introduce
> a new symbol to control generation of __symbol__ information for option
> 1.  I think this gets the usage backwards and will lead to confusion
> among users and developers.
> 
> My proposal is that we do not want __symbols__ existence to be dependent
> on some part of the kernel configuration for a number of reasons.
> First, this is out of step with the rest of how dtbs are created today
> and more importantly, thought about.  Today, all dtb content is
> independent of CONFIG options.  If you build a dtb from a given kernel
> tree, everyone will agree on the result.  This is part of the "contract"
> on passing old kernels and new dtb files even.

I hope that dtb contents are independent of CONFIG options, but I don't
feel confident is stating that there is not such dependency.  (Of course,
whether to build a dtb can be dependent on a CONFIG option in the Makefile,
but that is not the same concept.)

The only existing rule that I am aware of that helps avoid a dts dependency
on kernel CONFIG options is that included files can not be from general kernel
header files; they must be in include/dt-bindings/.

Should we add text to Documentation/devicetree/bindings/submitting-patches.txt
that explicitly states that dts files are not allowed to contain any
dependency on kernel CONFIG options?


> Second, I think this is out of step with how a lot of overlay usage will
> occur.  My thinking is that with maximally useful overlays being
> available in mainline, lots of use-cases that we have today that result
> in a number of DTS files being included can become just overlays.  This

I disagree with this.  My _opinion_ is that overlays should be the exception,
not the common case.  Overlays require extra complexity in the various
subsystems that interact with device trees.  For an overlay to work, these
subsystems must be able to react to changes made to the device tree by
an overlay.  The current mechanism is via notifiers, which only exist
for a few subsystems.

This extra complexity has the potential to be rather fragile, and receive
less extensive testing than the normal boot path.  My preference is that
the number of subsystems supporting overlays is minimized, to minimize
the fragility.

I suspect that drivers that support overlays will also be more fragile.

And it is not just subsystems and drivers.  I suspect that overlays will
also expose more of the boot time ordering issues that we keep running
into.


> is true in terms of not only evaluation kits but also when these systems
> are turned into custom hardware.  This is even more true for SoM based
> systems where a physical widget would be a SoM + carrier overlay +
> custom parts overlay.  These cases are going to be resolved with
> overlays being applied outside of the kernel.
> 
> Signed-off-by: Tom Rini <trini@...sulko.com>
> ---
>  drivers/of/unittest-data/Makefile | 5 -----
>  scripts/Makefile.lib              | 3 +++
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 6e00a9c69e58..70731cfe8900 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -11,8 +11,3 @@ targets += overlay_base.dtb overlay_base.dtb.S
>  .PRECIOUS: \
>  	$(obj)/%.dtb.S \
>  	$(obj)/%.dtb
> -
> -# enable creation of __symbols__ node
> -DTC_FLAGS_overlay := -@
> -DTC_FLAGS_overlay_bad_phandle := -@
> -DTC_FLAGS_overlay_base := -@
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib

No to that change.  I want to be able to control whether or not
__SYMBOLS__ is generated for each of those files.


> index 58c05e5d9870..a1f4a6b29d75 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -293,6 +293,9 @@ DTC_FLAGS += -Wnode_name_chars_strict \
>  	-Wproperty_name_chars_strict
>  endif
>  
> +# enable creation of __symbols__ node
> +DTC_FLAGS += -@
> +
>  DTC_FLAGS += $(DTC_FLAGS_$(basetarget))
>  
>  # Generate an assembly file to wrap the output of the device tree compiler
> 

And no to that change.  A dtb should not have __SYMBOLS__ enabled unless it is
needed (or plausibly needed).  The default case for the majority of dtbs is that
there is not need to support overlays and thus the overhead of the __SYMBOLS__
node is all overhead with no benefit.  This method turns on __SYMBOLS__ for
all dtbs.

-Frank

Powered by blists - more mailing lists