Date:   Mon, 18 Jan 2021 20:29:07 -0600
From:   Frank Rowand <frowand.list@...il.com>
To:     David Gibson <david@...son.dropbear.id.au>,
        Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Bill Mills <bill.mills@...aro.org>,
        Anmar Oueja <anmar.oueja@...aro.org>,
        Masahiro Yamada <masahiroy@...nel.org>
Subject: Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

On 1/18/21 12:30 AM, David Gibson wrote:
> On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>>>>
>>>> On 11-01-21, 09:46, Rob Herring wrote:
>>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>>>>>>
>>>>>> 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.
>>>>>
>>>>>> 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.
> 
>> 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.
> 
>> 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.
> 
>>> 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.
>>
>>> 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@...00 {
>>                 };
>>  
>>         };
>> -};
>> -
>> 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
>>
> 

Powered by blists - more mailing lists