On 1/18/21 12:30 AM, David Gibson wrote:

On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote:

quoted

+David,

On 14-01-21, 09:01, Rob Herring wrote:

quoted

On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar [off-list ref] wrote:

quoted

On 11-01-21, 09:46, Rob Herring wrote:

quoted

On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar [off-list ref] wrote:

quoted

Now that fdtoverlay is part of the kernel build, start using it to test
the unitest overlays we have by applying them statically.

Nice idea.

quoted

The file overlay_base.dtb have symbols of its own and we need to apply
overlay.dtb to overlay_base.dtb alone first to make it work, which gives
us intermediate-overlay.dtb file.

Okay? If restructuring things helps we should do that. Frank?

Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
and include it from testcases.dts like the other ones ?

No, because overlay_base.dts is an overlay dt.

So.. I was confused for a bit here, because the overlay_base.dts
you're discussing is the one in the kernel tree, not the one in the
dtc tree.

The kernel file is confusing to me.  It has the /plugin/ tag which

It should be confusing to you, without having dug deeply into the
evil things that unittest does.  Unittest does a bunch of contortions
and abuse of direct access into the kernel devicetree code internals
to be able to create and test for abnormal conditions.  No other code
should emulate the bizarre things that unittest does.

-Frank

should be used for overlays, but the rest of the file looks like a
base tree rather than an overlay.  There's even a comment saying "Base
device tree that overlays will be applied against".  But it goes on to
talk about __fixups__ and __local__fixups__ which will never be
generated for a based tree.

Oh.. and then there's terminology confusion.  dtc has had compile time
resolved overlays for a very long time.  Those are usually .dtsi
files, and should generally not have /plugin/.

More recent is the support for run-time overlays - .dtbo output files,
which are usually generated from a .dts with /plugin/.  They usually
should *not* have a "/ { ... } " stanza, since that indicates the base
portion of the tree.

quoted

What property of a file makes it an overlay ? Just the presence of
/plugin/; ?

Yes and no.  Generally that's how it should work , but it looks to me
like the presence of /plugin/ there is just wrong.  /plugin/ basically
just activates some extra dtc features, though, so it is possible to
"manually" construct a valid overlay without it.

quoted

David, we are talking about the overlay base[1] file here. The
fdtoverlay tool
fails to apply it to testcases.dts file (in the same directory) because none of
its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
skips them intentionally.

testcases.dts also has /plugin/ and again, it's not really clear if
that's right.

The point of /plugin/ is that it will automatically generate the
__overlay__ subnodes from dtc's &label { ... } or &{/path} { ... }
syntax, so you wouldn't expect to see __overlay__ in the source.

quoted

quoted

I think we need an
empty, minimal base.dtb to apply overlay_base.dtbo to.

One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
will make it work.

quoted

And then fdtoverlay needs a fix to apply overlays to the root node?

It isn't just root node I think, but any node for which the __overlay__ label
isn't there.

So this can make it all work if everyone is fine with it:

diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..59172c4c9e5a 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts

@@ -11,8 +11,7 @@
  * dtc will create nodes "/__symbols__" and "/__local_fixups__".
  */
 
-/ {
-       testcase-data-2 {
+       &overlay_base {
                #address-cells = <1>;
                #size-cells = <1>;
 

@@ -89,5 +88,3 @@ retail_1: vending@50000 {
                };
 
        };
-};
-

diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index a85b5e1c381a..539dc7d9eddc 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts

@@ -11,6 +11,11 @@ node-remove {
                        };
                };
        };
+
+       overlay_base: testcase-data-2 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+       };

-------------------------8<-------------------------

And then we can do this to the Makefile over my changes.

-------------------------8<-------------------------

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 9f3eb30b78f1..8cc23311b778 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile

@@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
 
 # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
 # fail build) statically with fdtoverlay
-intermediate-overlay   := overlay.dtb
 master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
                           overlay_3.dtb overlay_4.dtb overlay_5.dtb \
                           overlay_6.dtb overlay_7.dtb overlay_8.dtb \

@@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
                           overlay_gpio_01.dtb overlay_gpio_02a.dtb \
                           overlay_gpio_02b.dtb overlay_gpio_03.dtb \
                           overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
-                          intermediate-overlay.dtb
+                          overlay_base.dtb overlay.dtb
 
 quiet_cmd_fdtoverlay = fdtoverlay $@
       cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
 
-$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
-       $(call if_changed,fdtoverlay)
-
 $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
        $(call if_changed,fdtoverlay)
 
-always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
+always-$(CONFIG_OF_OVERLAY) += master.dtb