Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Wed, Jan 20, 2021 at 11:00:17PM -0600, Frank Rowand wrote: > > +David > > so I don't have to repeat this in another thread > > On 1/19/21 11:06 PM, Viresh Kumar wrote: > > On 19-01-21, 09:44, Frank Rowand wrote: > >> No. overlay_base.dts is intentionally compiled into a base FDT, not > >> an overlay. Unittest intentionally unflattens this FDT in early boot, > >> in association with unflattening the system FDT. One key intent > >> behind this is to use the same memory allocation method that is > >> used for the system FDT. > >> > >> Do not try to convert overlay_base.dts into an overlay. > > > > Okay, but why does it have /plugin/; specified in it then ? > > OK, so I sortof lied about overlay_base.dts not being an overlay. It is > a Frankenstein monster or a Schrodinger's dts/dtb. It is not a normal > object. Nobody who is not looking at how it is abused inside unittest.c > should be trying to touch it or understand it. In that case, it absolutely should not be used as your standard base dtb. Note that overlays in general rely on particular details of the base dtb they apply to - they'll need certain symbols and expect certain paths to be there. So applying random overlays to a "standard" base dtb sounds destined to failure anyway. Also, whatever they hell you're doing with testcases.dts sounds like a terrible idea to begin with. > unittest.c first unflattens overlay_base.dtb during early boot. Then later > it does some phandle resolution using the overlay metadata from overlay_base. > Then it removes the overlay metadata from the in kernel devicetree data > structure. It is a hack, it is ugly, but it enables some overlay unit > tests. > > Quit trying to change overlay_base.dts. > > In my suggested changes to the base patch I put overlay_base.dtb in the > list of overlays for fdtoverlay to apply (apply_static_overlay in the > Makefile) because overlay_base.dts is compiled as an overlay into > overlay_base.dtb and it can be applied on top of the base tree > testcases.dtb. This gives a little bit more testcase data for > fdtoverlay from an existing dtb. > > If you keep trying to change overlay_base.dts I will just tell you > to remove overlay_base.dtb from apply_static_overlay, and then the > test coverage will become smaller. I do not see that as a good change. > > If you want more extensive testing of fdtoverlay, then create your > own specific test cases from scratch and submit patches for them > to the kernel or to the dtc compiler project. > > > > > And shouldn't we create two separate dtb-s now, static_test.dtb and > > static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with > > testcase.dtb anyway. > > > > Or maybe we can create another file static_overlay.dts (like testcases.dts) > > which can include both testcases.dts and overlay_base.dts, and then we can > > create static_test.dtb out of it ? That won't impact the runtime tests at > > all. > > > > Stop trying to use all of the unittest .dts test data files. It is convenient > that so many of them can be used in their current form. That is goodness > and nice leveraging. Just ignore the .dts test data files that are not > easily consumed by fdtoverlay. > > The email threads around the various versions of this patch series show how > normal devicetree knowledgeable people look at the contents of some of the > .dts test data files and think that they are incorrect. That is because > the way that unittest uses them is not normal. Trying to modify one or two > of the many unittest .dts test data files so that they are usable by both > the static fdtoverlay and the run time unittest is not worth it. > > -Frank > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 20-01-21, 23:00, Frank Rowand wrote: > unittest.c first unflattens overlay_base.dtb during early boot. Then later > it does some phandle resolution using the overlay metadata from overlay_base. > Then it removes the overlay metadata from the in kernel devicetree data > structure. It is a hack, it is ugly, but it enables some overlay unit > tests. > > Quit trying to change overlay_base.dts. I have already done so (in the latest series I sent yesterday). > In my suggested changes to the base patch I put overlay_base.dtb in the > list of overlays for fdtoverlay to apply (apply_static_overlay in the > Makefile) because overlay_base.dts is compiled as an overlay into > overlay_base.dtb and it can be applied on top of the base tree > testcases.dtb. This gives a little bit more testcase data for > fdtoverlay from an existing dtb. Okay, but fdtoverlay tool can't apply overlay_base.dtb to testcases.dtb as none of its node have the __overlay__ property and so I have entirely skipped overlay_base.dtb and overlay.dtb now. Yes this reduces the test coverage a bit as you said, but I don't see a way to make it work right now. And I am not even sure if it is a fdtoverlay bug, it expects the __overlay__ thing to be there for each node, otherwise it can't figure out where this node should be applied. -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
+David so I don't have to repeat this in another thread On 1/19/21 11:06 PM, Viresh Kumar wrote: > On 19-01-21, 09:44, Frank Rowand wrote: >> No. overlay_base.dts is intentionally compiled into a base FDT, not >> an overlay. Unittest intentionally unflattens this FDT in early boot, >> in association with unflattening the system FDT. One key intent >> behind this is to use the same memory allocation method that is >> used for the system FDT. >> >> Do not try to convert overlay_base.dts into an overlay. > > Okay, but why does it have /plugin/; specified in it then ? OK, so I sortof lied about overlay_base.dts not being an overlay. It is a Frankenstein monster or a Schrodinger's dts/dtb. It is not a normal object. Nobody who is not looking at how it is abused inside unittest.c should be trying to touch it or understand it. unittest.c first unflattens overlay_base.dtb during early boot. Then later it does some phandle resolution using the overlay metadata from overlay_base. Then it removes the overlay metadata from the in kernel devicetree data structure. It is a hack, it is ugly, but it enables some overlay unit tests. Quit trying to change overlay_base.dts. In my suggested changes to the base patch I put overlay_base.dtb in the list of overlays for fdtoverlay to apply (apply_static_overlay in the Makefile) because overlay_base.dts is compiled as an overlay into overlay_base.dtb and it can be applied on top of the base tree testcases.dtb. This gives a little bit more testcase data for fdtoverlay from an existing dtb. If you keep trying to change overlay_base.dts I will just tell you to remove overlay_base.dtb from apply_static_overlay, and then the test coverage will become smaller. I do not see that as a good change. If you want more extensive testing of fdtoverlay, then create your own specific test cases from scratch and submit patches for them to the kernel or to the dtc compiler project. > > And shouldn't we create two separate dtb-s now, static_test.dtb and > static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with > testcase.dtb anyway. > > Or maybe we can create another file static_overlay.dts (like testcases.dts) > which can include both testcases.dts and overlay_base.dts, and then we can > create static_test.dtb out of it ? That won't impact the runtime tests at all. > Stop trying to use all of the unittest .dts test data files. It is convenient that so many of them can be used in their current form. That is goodness and nice leveraging. Just ignore the .dts test data files that are not easily consumed by fdtoverlay. The email threads around the various versions of this patch series show how normal devicetree knowledgeable people look at the contents of some of the .dts test data files and think that they are incorrect. That is because the way that unittest uses them is not normal. Trying to modify one or two of the many unittest .dts test data files so that they are usable by both the static fdtoverlay and the run time unittest is not worth it. -Frank
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 20-01-21, 10:36, Viresh Kumar wrote: > On 19-01-21, 09:44, Frank Rowand wrote: > > No. overlay_base.dts is intentionally compiled into a base FDT, not > > an overlay. Unittest intentionally unflattens this FDT in early boot, > > in association with unflattening the system FDT. One key intent > > behind this is to use the same memory allocation method that is > > used for the system FDT. > > > > Do not try to convert overlay_base.dts into an overlay. > > Okay, but why does it have /plugin/; specified in it then ? > > And shouldn't we create two separate dtb-s now, static_test.dtb and > static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with > testcase.dtb anyway. > > Or maybe we can create another file static_overlay.dts (like testcases.dts) > which can include both testcases.dts and overlay_base.dts, and then we can > create static_test.dtb out of it ? That won't impact the runtime tests at all. Hmm, I noticed just now that you have kept overlay.dtb out of the build, probably we should then drop overlay_base.dtb as well ? -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 19-01-21, 09:44, Frank Rowand wrote: > No. overlay_base.dts is intentionally compiled into a base FDT, not > an overlay. Unittest intentionally unflattens this FDT in early boot, > in association with unflattening the system FDT. One key intent > behind this is to use the same memory allocation method that is > used for the system FDT. > > Do not try to convert overlay_base.dts into an overlay. Okay, but why does it have /plugin/; specified in it then ? And shouldn't we create two separate dtb-s now, static_test.dtb and static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with testcase.dtb anyway. Or maybe we can create another file static_overlay.dts (like testcases.dts) which can include both testcases.dts and overlay_base.dts, and then we can create static_test.dtb out of it ? That won't impact the runtime tests at all. -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/19/21 2:05 AM, Viresh Kumar wrote: > On 18-01-21, 20:21, frowand.l...@gmail.com wrote: >> From: Frank Rowand >> >> These changes apply on top of the patches in: >> >> [PATCH] of: unittest: Statically apply overlays using fdtoverlay >> Message-Id: >> <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.ku...@linaro.org> >> >> There are still some issues to be cleaned up, so not ready for acceptance. > > Are you talking about the missing __overlay__ thing ? (more below) No. I am referencing my comments below (I'll copy them up here): I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and have not looked into the proper usage of it. [Tested using my own fdtoverlay instead of the one supplied by your patches that added fdtoverlay and fdtdump to the kernel tree.] I have not run this through checkpatch, or my checks for build warnings. I have not run unittests on my target with these patches applied. I will have to get the updated patch, test it more fully, and fill in a gap in my knowledge (use of "always-$(CONFIG_xxx)". > >> I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and >> have not looked into the proper usage of it. > > I wasn't sure either, maybe Masahiro can suggest the best fit. > >> I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler >> upstream project. For my testing I added LD_LIBRARY_PATH to the body of >> "cmd_fdtoverlay" to reference my hand built libfdt. The kernel build >> system will have to instead use a libfdt that is built in the kernel >> tree. > > I tested it with this patchset: > > https://lore.kernel.org/lkml/cover.1610431620.git.viresh.ku...@linaro.org/ > >> I have not run this through checkpatch, or my checks for build warnings. >> I have not run unittests on my target with these patches applied. >> >> --- >> drivers/of/unittest-data/Makefile | 67 ++- >> 1 file changed, 48 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index f17bce85f65f..28614a123d1e 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@ >> # suppress warnings about intentional errors >> DTC_FLAGS_testcases += -Wno-interrupts_property >> >> -# Apply overlays 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 \ >> - overlay_9.dtb overlay_10.dtb overlay_11.dtb \ >> - overlay_12.dtb overlay_13.dtb overlay_15.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 >> - >> -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) >> +# Apply overlays statically with fdtoverlay. This is a build time test that >> +# the overlays can be applied successfully by fdtoverlay. This does not >> +# guarantee that the overlays can be applied successfully at run time by >> +# unittest, but it provides a bit of build time test coverage for those >> +# who do not execute unittest. >> +# >> +# The overlays are applied on top of testcases.dtb to create static_test.dtb >> +# If fdtoverlay detects an error than the kernel build will fail. >> +# static_test.dtb is not consumed by unittest. >> +# >> +# Some unittest overlays deliberately contain errors that unittest checks >> for. >> +# These overlays will cause fdtoverlay to fail, and are thus not included >> +# in the static test: >> +# overlay.dtb \ >> +# overlay_bad_add_dup_node.dtb \ >> +# overlay_bad_add_dup_prop.dtb \ >> +# overlay_bad_phandle.dtb \ >> +# overlay_bad_symbol.dtb \ >> + >> +apply_static_overlay := overlay_base.dtb \ > > This won't work because of the issues I ment
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 18-01-21, 20:21, frowand.l...@gmail.com wrote:
> From: Frank Rowand
>
> These changes apply on top of the patches in:
>
> [PATCH] of: unittest: Statically apply overlays using fdtoverlay
> Message-Id:
> <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.ku...@linaro.org>
>
> There are still some issues to be cleaned up, so not ready for acceptance.
Are you talking about the missing __overlay__ thing ? (more below)
> I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and
> have not looked into the proper usage of it.
I wasn't sure either, maybe Masahiro can suggest the best fit.
> I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler
> upstream project. For my testing I added LD_LIBRARY_PATH to the body of
> "cmd_fdtoverlay" to reference my hand built libfdt. The kernel build
> system will have to instead use a libfdt that is built in the kernel
> tree.
I tested it with this patchset:
https://lore.kernel.org/lkml/cover.1610431620.git.viresh.ku...@linaro.org/
> I have not run this through checkpatch, or my checks for build warnings.
> I have not run unittests on my target with these patches applied.
>
> ---
> drivers/of/unittest-data/Makefile | 67 ++-
> 1 file changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/unittest-data/Makefile
> b/drivers/of/unittest-data/Makefile
> index f17bce85f65f..28614a123d1e 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@
> # suppress warnings about intentional errors
> DTC_FLAGS_testcases += -Wno-interrupts_property
>
> -# Apply overlays 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 \
> -overlay_9.dtb overlay_10.dtb overlay_11.dtb \
> -overlay_12.dtb overlay_13.dtb overlay_15.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
> -
> -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)
> +# Apply overlays statically with fdtoverlay. This is a build time test that
> +# the overlays can be applied successfully by fdtoverlay. This does not
> +# guarantee that the overlays can be applied successfully at run time by
> +# unittest, but it provides a bit of build time test coverage for those
> +# who do not execute unittest.
> +#
> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
> +# If fdtoverlay detects an error than the kernel build will fail.
> +# static_test.dtb is not consumed by unittest.
> +#
> +# Some unittest overlays deliberately contain errors that unittest checks
> for.
> +# These overlays will cause fdtoverlay to fail, and are thus not included
> +# in the static test:
> +#overlay.dtb \
> +#overlay_bad_add_dup_node.dtb \
> +#overlay_bad_add_dup_prop.dtb \
> +#overlay_bad_phandle.dtb \
> +#overlay_bad_symbol.dtb \
> +
> +apply_static_overlay := overlay_base.dtb \
This won't work because of the issues I mentioned earlier. This file
doesn't have __overlay__. One way to fix that is to do this:
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 {
+ _base {
#address-cells = <1>;
#size-cells = <1>;
@@ -89,5 +88,3 @@ retail_1: vending@5 {
};
};
-};
-
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-dat
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/17/21 9:54 PM, Frank Rowand wrote:
> Hi Viresh,
>
> On 1/14/21 11:44 PM, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar
>>> wrote:
On 11-01-21, 09:46, Rob Herring wrote:
> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar
> 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 ?
>
> I was not able to look at this until tonight. The unittest world is somewhat
> convoluted and complex. Not at all a normal OF environment since it is
> directly
> using both dynamic OF code and overlay apply/remove code. Not to mention
> deliberately misformed devicetree (.dts) data. And totally hacking the
> loading
> of FDTs in additional ways.
>
> It is late Sunday night here (almost 10:00pm), so I am going to look at this
> first thing Monday morning.
I sent comments in the form of a patch to the original patch email.
-Frank
>
>>>
>>> No, because overlay_base.dts is an overlay dt.
>>
>> What property of a file makes it an overlay ? Just the presence of /plugin/;
>> ?
>
> The "/plugin/;" in a dts file is what tells the dtc compiler to process the
> source
> file as an overlay instead of as a base.
>
>>
>> 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.
>>
>>> 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:
>
> I'll look this over Monday morning to see what the side effects are in the
> bizarre world of unittest.
>
>>
>> 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 {
>> + _base {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> @@ -89,5 +88,3 @@ retail_1: vending@5 {
>> };
>>
>> };
>> -};
>> -
>> 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
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
>>> wrote:
On 11-01-21, 09:46, Rob Herring wrote:
> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar
> 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 { ... } 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 {
>> + _base {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> @@ -89,5 +88,3 @@ retail_1: vending@5 {
>> };
>>
>> };
>> -};
>> -
>> 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
>>
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/13/21 11:00 PM, Viresh Kumar wrote: > Frank/Rob. > > On 08-01-21, 14:11, Viresh Kumar wrote: >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index 009f4045c8e4..f17bce85f65f 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ >> >> # suppress warnings about intentional errors >> DTC_FLAGS_testcases += -Wno-interrupts_property >> + >> +# Apply overlays statically with fdtoverlay > > I will update this part to mention about the dtbs we are not using in the > build > as they will fail (as per Frank's comment). > > Is there anything else you guys want me to change before I send the next > version > ? I sent some changes in the form of a patch, in reply to your original patch. If you can fold the changes into your patch, and look into the comments that I put into the patch, that would be great. -Frank > >> +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 \ >> + overlay_9.dtb overlay_10.dtb overlay_11.dtb \ >> + overlay_12.dtb overlay_13.dtb overlay_15.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 >> + >> +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 >
re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
From: Frank Rowand These changes apply on top of the patches in: [PATCH] of: unittest: Statically apply overlays using fdtoverlay Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.ku...@linaro.org> There are still some issues to be cleaned up, so not ready for acceptance. I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and have not looked into the proper usage of it. I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler upstream project. For my testing I added LD_LIBRARY_PATH to the body of "cmd_fdtoverlay" to reference my hand built libfdt. The kernel build system will have to instead use a libfdt that is built in the kernel tree. I have not run this through checkpatch, or my checks for build warnings. I have not run unittests on my target with these patches applied. --- drivers/of/unittest-data/Makefile | 67 ++- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index f17bce85f65f..28614a123d1e 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property -# Apply overlays 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 \ - overlay_9.dtb overlay_10.dtb overlay_11.dtb \ - overlay_12.dtb overlay_13.dtb overlay_15.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 - -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) +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of testcases.dtb to create static_test.dtb +# If fdtoverlay detects an error than the kernel build will fail. +# static_test.dtb is not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +# overlay.dtb \ +# overlay_bad_add_dup_node.dtb \ +# overlay_bad_add_dup_prop.dtb \ +# overlay_bad_phandle.dtb \ +# overlay_bad_symbol.dtb \ + +apply_static_overlay := overlay_base.dtb \ + 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 \ + overlay_9.dtb \ + overlay_10.dtb \ + overlay_11.dtb \ + overlay_12.dtb \ + overlay_13.dtb \ + overlay_15.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 \ + +quiet_cmd_fdtoverlay = FDTOVERLAY $@ + +## This is not correct, need to use libfdt from the kernel tree: +cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ -$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay)) $(call if_changed,fdtoverlay) -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb +always-$(CONFIG_OF_OVERLAY) += static_test.dtb -- Frank Rowand
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
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
> > wrote:
> > >
> > > On 11-01-21, 09:46, Rob Herring wrote:
> > > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar
> > > > 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
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 { ... } 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 {
> + _base {
> #address-cells = <1>;
> #size-cells = <1>;
>
> @@ -89,5 +88,3 @@ retail_1: vending@5 {
> };
>
> };
> -};
> -
> 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
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
Hi Viresh,
On 1/14/21 11:44 PM, Viresh Kumar wrote:
> +David,
>
> On 14-01-21, 09:01, Rob Herring wrote:
>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar
>> wrote:
>>>
>>> On 11-01-21, 09:46, Rob Herring wrote:
On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar
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 ?
I was not able to look at this until tonight. The unittest world is somewhat
convoluted and complex. Not at all a normal OF environment since it is directly
using both dynamic OF code and overlay apply/remove code. Not to mention
deliberately misformed devicetree (.dts) data. And totally hacking the loading
of FDTs in additional ways.
It is late Sunday night here (almost 10:00pm), so I am going to look at this
first thing Monday morning.
>>
>> No, because overlay_base.dts is an overlay dt.
>
> What property of a file makes it an overlay ? Just the presence of /plugin/; ?
The "/plugin/;" in a dts file is what tells the dtc compiler to process the
source
file as an overlay instead of as a base.
>
> 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.
>
>> 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:
I'll look this over Monday morning to see what the side effects are in the
bizarre world of unittest.
>
> 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 {
> + _base {
> #address-cells = <1>;
> #size-cells = <1>;
>
> @@ -89,5 +88,3 @@ retail_1: vending@5 {
> };
>
> };
> -};
> -
> 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)
> -
>
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
+David,
On 14-01-21, 09:01, Rob Herring wrote:
> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar wrote:
> >
> > On 11-01-21, 09:46, Rob Herring wrote:
> > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar
> > > 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.
What property of a file makes it an overlay ? Just the presence of /plugin/; ?
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.
> 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 {
+ _base {
#address-cells = <1>;
#size-cells = <1>;
@@ -89,5 +88,3 @@ retail_1: vending@5 {
};
};
-};
-
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
--
viresh
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/unittest-data/overlay_base.dts
[2]
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt/fdt_overlay.c#n628
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar wrote: > > On 11-01-21, 09:46, Rob Herring wrote: > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar 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. I think we need an empty, minimal base.dtb to apply overlay_base.dtbo to. And then fdtoverlay needs a fix to apply overlays to the root node? Rob
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 11-01-21, 09:46, Rob Herring wrote: > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar 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 ? -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
Frank/Rob. On 08-01-21, 14:11, Viresh Kumar wrote: > diff --git a/drivers/of/unittest-data/Makefile > b/drivers/of/unittest-data/Makefile > index 009f4045c8e4..f17bce85f65f 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ > > # suppress warnings about intentional errors > DTC_FLAGS_testcases += -Wno-interrupts_property > + > +# Apply overlays statically with fdtoverlay I will update this part to mention about the dtbs we are not using in the build as they will fail (as per Frank's comment). Is there anything else you guys want me to change before I send the next version ? > +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 \ > +overlay_9.dtb overlay_10.dtb overlay_11.dtb \ > +overlay_12.dtb overlay_13.dtb overlay_15.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 > + > +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 -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/13/21 9:05 AM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand wrote: >> >> On 1/12/21 2:46 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: On 1/12/21 1:41 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand > wrote: >> >> On 1/12/21 8:04 AM, Rob Herring wrote: >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand >>> wrote: On 1/8/21 2:41 AM, Viresh Kumar wrote: > Now that fdtoverlay is part of the kernel build, start using it to > test > the unitest overlays we have by applying them statically. > > 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. > > The intermediate-overlay.dtb file along with all other overlays is > them > applied to testcases.dtb to generate the master.dtb file. > > Signed-off-by: Viresh Kumar NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. >>> >>> This patch doesn't take away from any of that and it completely >>> orthogonal. >> >> Mea culpa. I took the patch header comment at face value, and read more >> into >> the header comment than what was written there. I then skimmed the patch >> instead of actually reading what it was doing. >> >> I incorrectly _assumed_ (bad!) that the intent was to replace applying >> the >> individual overlay dtb's with the master.dtb. Reading more closely, I >> see >> that the assumed final step of actually _using_ master.dtb does not >> exist. >> >> So, yes, I agree that the patch as written is orthogonal to my concern. >> >> My updated understanding is that this patch is attempting to use the >> existing >> unittest overlay dts files as source to test fdtoverlay. And that the >> resulting >> dtb from fdtoverlay is not intended to be consumed by the kernel >> unittest. > > The goal is not to test fdtoverlay. dtc unittests do that. The goal is > testing overlays we expect to be able to apply can actually apply and > doing this at build time. That's also the goal for all the 'real' > overlays which get added. > >> I do not agree that this is a good approach to testing fdtoverlay. The >> unittest overlay dts files are constructed specifically to test various >> parts of the kernel overlay code and dynamic OF code. Some of the >> content >> of the overlays is constructed to trigger error conditions in that code, >> and thus will not be able to be processed without error by fdtoverlay. > > Then those should be omitted. > >> Trying to use overlay dts files that are constructed to test runtime >> kernel >> code as fdtoverlay input data mixes two different test environments and >> objectives. If fdtoverlay test cases are desired, then fdtoverlay >> specific >> dts files should be created. >> >>> I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. >>> >>> If you (or more importantly someone else sending us patches) make >>> changes to the overlays, you can test that they apply at build time >>> rather than runtime. I'll take it! So please help on fixing the issue >>> because I want to apply this. >> >> If the tests can be added to the upstream project, I would much prefer >> they reside there. If there is some reason a certain test is more >> suited to be in the Linux kernel source tree then I also would like >> it to be accepted here. > > Again, this is just about doing sanity checks at build time rather > than *only* rely on runtime. I'm fine with adding tests for applying overlays at build time (in other words, tests of fdtoverlay). >>> >> >>> Again, it's not tests of fdtoverlay. It's a test of the dts files. We >>> are testing that an overlay dts can apply to the base dts we claim it >>> applies. If
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand wrote: > > On 1/12/21 2:46 PM, Rob Herring wrote: > > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: > >> > >> On 1/12/21 1:41 PM, Rob Herring wrote: > >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand > >>> wrote: > > On 1/12/21 8:04 AM, Rob Herring wrote: > > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand > > wrote: > >> > >> On 1/8/21 2:41 AM, Viresh Kumar wrote: > >>> Now that fdtoverlay is part of the kernel build, start using it to > >>> test > >>> the unitest overlays we have by applying them statically. > >>> > >>> 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. > >>> > >>> The intermediate-overlay.dtb file along with all other overlays is > >>> them > >>> applied to testcases.dtb to generate the master.dtb file. > >>> > >>> Signed-off-by: Viresh Kumar > >> > >> NACK to this specific patch, in its current form. > >> > >> There are restrictions on applying an overlay at runtime that do not > >> apply > >> to applying an overlay to an FDT that will be loaded by the kernel > >> during > >> early boot. Thus the unittest overlays _must_ be applied using the > >> kernel > >> overlay loading methods to test the kernel runtime overlay loading > >> feature. > > > > This patch doesn't take away from any of that and it completely > > orthogonal. > > Mea culpa. I took the patch header comment at face value, and read more > into > the header comment than what was written there. I then skimmed the patch > instead of actually reading what it was doing. > > I incorrectly _assumed_ (bad!) that the intent was to replace applying > the > individual overlay dtb's with the master.dtb. Reading more closely, I > see > that the assumed final step of actually _using_ master.dtb does not > exist. > > So, yes, I agree that the patch as written is orthogonal to my concern. > > My updated understanding is that this patch is attempting to use the > existing > unittest overlay dts files as source to test fdtoverlay. And that the > resulting > dtb from fdtoverlay is not intended to be consumed by the kernel > unittest. > >>> > >>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is > >>> testing overlays we expect to be able to apply can actually apply and > >>> doing this at build time. That's also the goal for all the 'real' > >>> overlays which get added. > >>> > I do not agree that this is a good approach to testing fdtoverlay. The > unittest overlay dts files are constructed specifically to test various > parts of the kernel overlay code and dynamic OF code. Some of the > content > of the overlays is constructed to trigger error conditions in that code, > and thus will not be able to be processed without error by fdtoverlay. > >>> > >>> Then those should be omitted. > >>> > Trying to use overlay dts files that are constructed to test runtime > kernel > code as fdtoverlay input data mixes two different test environments and > objectives. If fdtoverlay test cases are desired, then fdtoverlay > specific > dts files should be created. > > > > >> I agree that testing fdtoverlay is a good idea. I have not looked at > >> the > >> parent project to see how much testing of fdtoverlay occurs there, but > >> I > >> would prefer that fdtoverlay tests reside in the parent project if > >> practical > >> and reasonable. If there is some reason that some fdtoverlay tests are > >> more practical in the Linux kernel repository then I am open to adding > >> them to the Linux kernel tree. > > > > If you (or more importantly someone else sending us patches) make > > changes to the overlays, you can test that they apply at build time > > rather than runtime. I'll take it! So please help on fixing the issue > > because I want to apply this. > > If the tests can be added to the upstream project, I would much prefer > they reside there. If there is some reason a certain test is more > suited to be in the Linux kernel source tree then I also would like > it to be accepted here. > >>> > >>> Again, this is just about doing sanity checks at build time rather > >>> than *only* rely on runtime. > >> > >> I'm fine with adding tests for applying overlays at build time (in > >> other words, tests of fdtoverlay). > > > > > Again, it's not tests of fdtoverlay. It's a test of the dts files. We > > are testing that an overlay dts can apply to the base dts we claim it > > applies. If the overlay dts has crap then we'll catch it.
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 2:46 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: >> >> On 1/12/21 1:41 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: On 1/12/21 8:04 AM, Rob Herring wrote: > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand > wrote: >> >> On 1/8/21 2:41 AM, Viresh Kumar wrote: >>> Now that fdtoverlay is part of the kernel build, start using it to test >>> the unitest overlays we have by applying them statically. >>> >>> 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. >>> >>> The intermediate-overlay.dtb file along with all other overlays is them >>> applied to testcases.dtb to generate the master.dtb file. >>> >>> Signed-off-by: Viresh Kumar >> >> NACK to this specific patch, in its current form. >> >> There are restrictions on applying an overlay at runtime that do not >> apply >> to applying an overlay to an FDT that will be loaded by the kernel during >> early boot. Thus the unittest overlays _must_ be applied using the >> kernel >> overlay loading methods to test the kernel runtime overlay loading >> feature. > > This patch doesn't take away from any of that and it completely > orthogonal. Mea culpa. I took the patch header comment at face value, and read more into the header comment than what was written there. I then skimmed the patch instead of actually reading what it was doing. I incorrectly _assumed_ (bad!) that the intent was to replace applying the individual overlay dtb's with the master.dtb. Reading more closely, I see that the assumed final step of actually _using_ master.dtb does not exist. So, yes, I agree that the patch as written is orthogonal to my concern. My updated understanding is that this patch is attempting to use the existing unittest overlay dts files as source to test fdtoverlay. And that the resulting dtb from fdtoverlay is not intended to be consumed by the kernel unittest. >>> >>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is >>> testing overlays we expect to be able to apply can actually apply and >>> doing this at build time. That's also the goal for all the 'real' >>> overlays which get added. >>> I do not agree that this is a good approach to testing fdtoverlay. The unittest overlay dts files are constructed specifically to test various parts of the kernel overlay code and dynamic OF code. Some of the content of the overlays is constructed to trigger error conditions in that code, and thus will not be able to be processed without error by fdtoverlay. >>> >>> Then those should be omitted. >>> Trying to use overlay dts files that are constructed to test runtime kernel code as fdtoverlay input data mixes two different test environments and objectives. If fdtoverlay test cases are desired, then fdtoverlay specific dts files should be created. > >> I agree that testing fdtoverlay is a good idea. I have not looked at the >> parent project to see how much testing of fdtoverlay occurs there, but I >> would prefer that fdtoverlay tests reside in the parent project if >> practical >> and reasonable. If there is some reason that some fdtoverlay tests are >> more practical in the Linux kernel repository then I am open to adding >> them to the Linux kernel tree. > > If you (or more importantly someone else sending us patches) make > changes to the overlays, you can test that they apply at build time > rather than runtime. I'll take it! So please help on fixing the issue > because I want to apply this. If the tests can be added to the upstream project, I would much prefer they reside there. If there is some reason a certain test is more suited to be in the Linux kernel source tree then I also would like it to be accepted here. >>> >>> Again, this is just about doing sanity checks at build time rather >>> than *only* rely on runtime. >> >> I'm fine with adding tests for applying overlays at build time (in >> other words, tests of fdtoverlay). > > Again, it's not tests of fdtoverlay. It's a test of the dts files. We > are testing that an overlay dts can apply to the base dts we claim it > applies. If the overlay dts has crap then we'll catch it. > > We shouldn't accept overlays that can't apply to a base in the kernel > tree. That's either because it's broken or because the base doesn't > exist. With the exception of overlays designed to fail for tests, > unittest overlays should not be any different. I understood the goal to be testing fdtoverlay. I'll switch my mind set to the goal being a test of dts files. We
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 1:41 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: >> >> On 1/12/21 8:04 AM, Rob Herring wrote: >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: On 1/8/21 2:41 AM, Viresh Kumar wrote: > Now that fdtoverlay is part of the kernel build, start using it to test > the unitest overlays we have by applying them statically. > > 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. > > The intermediate-overlay.dtb file along with all other overlays is them > applied to testcases.dtb to generate the master.dtb file. > > Signed-off-by: Viresh Kumar NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. >>> >>> This patch doesn't take away from any of that and it completely orthogonal. >> >> Mea culpa. I took the patch header comment at face value, and read more into >> the header comment than what was written there. I then skimmed the patch >> instead of actually reading what it was doing. >> >> I incorrectly _assumed_ (bad!) that the intent was to replace applying the >> individual overlay dtb's with the master.dtb. Reading more closely, I see >> that the assumed final step of actually _using_ master.dtb does not exist. >> >> So, yes, I agree that the patch as written is orthogonal to my concern. >> >> My updated understanding is that this patch is attempting to use the existing >> unittest overlay dts files as source to test fdtoverlay. And that the >> resulting >> dtb from fdtoverlay is not intended to be consumed by the kernel unittest. > > The goal is not to test fdtoverlay. dtc unittests do that. The goal is > testing overlays we expect to be able to apply can actually apply and > doing this at build time. That's also the goal for all the 'real' > overlays which get added. > >> I do not agree that this is a good approach to testing fdtoverlay. The >> unittest overlay dts files are constructed specifically to test various >> parts of the kernel overlay code and dynamic OF code. Some of the content >> of the overlays is constructed to trigger error conditions in that code, >> and thus will not be able to be processed without error by fdtoverlay. > > Then those should be omitted. > >> Trying to use overlay dts files that are constructed to test runtime kernel >> code as fdtoverlay input data mixes two different test environments and >> objectives. If fdtoverlay test cases are desired, then fdtoverlay specific >> dts files should be created. >> >>> I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. >>> >>> If you (or more importantly someone else sending us patches) make >>> changes to the overlays, you can test that they apply at build time >>> rather than runtime. I'll take it! So please help on fixing the issue >>> because I want to apply this. >> >> If the tests can be added to the upstream project, I would much prefer >> they reside there. If there is some reason a certain test is more >> suited to be in the Linux kernel source tree then I also would like >> it to be accepted here. > > Again, this is just about doing sanity checks at build time rather > than *only* rely on runtime. I'm fine with adding tests for applying overlays at build time (in other words, tests of fdtoverlay). But the constraints on applying an overlay at build time are different than the runtime constraints. The existing unittest overlay dts files are not designed to test applying overlays at build time. Tests for fdtoverlay should be designed to test that overlays that meet the build time constraints can be applied properly by fdtoverlay, and that overlays that fail to meet those constraints are rejected by fdtoverlay. Trying to use the same data (dts) files for tests that have different constraints is likely to make both tests more fragile when a data file is modified for one environment without careful consideration of the other environment. > >>> >>> And yes, dtc has fdtoverlay tests. But this patch shows there's at >>> least 2 issues, >> >> >>> fdtoverlay can't apply overlays to the root >> >> A test of that definitely belongs in the upstream project. > > Yes, agreed. > >>> and
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: > > On 1/12/21 1:41 PM, Rob Herring wrote: > > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: > >> > >> On 1/12/21 8:04 AM, Rob Herring wrote: > >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand > >>> wrote: > > On 1/8/21 2:41 AM, Viresh Kumar wrote: > > Now that fdtoverlay is part of the kernel build, start using it to test > > the unitest overlays we have by applying them statically. > > > > 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. > > > > The intermediate-overlay.dtb file along with all other overlays is them > > applied to testcases.dtb to generate the master.dtb file. > > > > Signed-off-by: Viresh Kumar > > NACK to this specific patch, in its current form. > > There are restrictions on applying an overlay at runtime that do not > apply > to applying an overlay to an FDT that will be loaded by the kernel during > early boot. Thus the unittest overlays _must_ be applied using the > kernel > overlay loading methods to test the kernel runtime overlay loading > feature. > >>> > >>> This patch doesn't take away from any of that and it completely > >>> orthogonal. > >> > >> Mea culpa. I took the patch header comment at face value, and read more > >> into > >> the header comment than what was written there. I then skimmed the patch > >> instead of actually reading what it was doing. > >> > >> I incorrectly _assumed_ (bad!) that the intent was to replace applying the > >> individual overlay dtb's with the master.dtb. Reading more closely, I see > >> that the assumed final step of actually _using_ master.dtb does not exist. > >> > >> So, yes, I agree that the patch as written is orthogonal to my concern. > >> > >> My updated understanding is that this patch is attempting to use the > >> existing > >> unittest overlay dts files as source to test fdtoverlay. And that the > >> resulting > >> dtb from fdtoverlay is not intended to be consumed by the kernel unittest. > > > > The goal is not to test fdtoverlay. dtc unittests do that. The goal is > > testing overlays we expect to be able to apply can actually apply and > > doing this at build time. That's also the goal for all the 'real' > > overlays which get added. > > > >> I do not agree that this is a good approach to testing fdtoverlay. The > >> unittest overlay dts files are constructed specifically to test various > >> parts of the kernel overlay code and dynamic OF code. Some of the content > >> of the overlays is constructed to trigger error conditions in that code, > >> and thus will not be able to be processed without error by fdtoverlay. > > > > Then those should be omitted. > > > >> Trying to use overlay dts files that are constructed to test runtime kernel > >> code as fdtoverlay input data mixes two different test environments and > >> objectives. If fdtoverlay test cases are desired, then fdtoverlay specific > >> dts files should be created. > >> > >>> > I agree that testing fdtoverlay is a good idea. I have not looked at the > parent project to see how much testing of fdtoverlay occurs there, but I > would prefer that fdtoverlay tests reside in the parent project if > practical > and reasonable. If there is some reason that some fdtoverlay tests are > more practical in the Linux kernel repository then I am open to adding > them to the Linux kernel tree. > >>> > >>> If you (or more importantly someone else sending us patches) make > >>> changes to the overlays, you can test that they apply at build time > >>> rather than runtime. I'll take it! So please help on fixing the issue > >>> because I want to apply this. > >> > >> If the tests can be added to the upstream project, I would much prefer > >> they reside there. If there is some reason a certain test is more > >> suited to be in the Linux kernel source tree then I also would like > >> it to be accepted here. > > > > Again, this is just about doing sanity checks at build time rather > > than *only* rely on runtime. > > I'm fine with adding tests for applying overlays at build time (in > other words, tests of fdtoverlay). Again, it's not tests of fdtoverlay. It's a test of the dts files. We are testing that an overlay dts can apply to the base dts we claim it applies. If the overlay dts has crap then we'll catch it. We shouldn't accept overlays that can't apply to a base in the kernel tree. That's either because it's broken or because the base doesn't exist. With the exception of overlays designed to fail for tests, unittest overlays should not be any different. > But the constraints on applying an overlay at build time are different > than the runtime constraints. Like what specifically? Runtime is more constrained than build time. Or at
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: > > On 1/12/21 8:04 AM, Rob Herring wrote: > > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: > >> > >> On 1/8/21 2:41 AM, Viresh Kumar wrote: > >>> Now that fdtoverlay is part of the kernel build, start using it to test > >>> the unitest overlays we have by applying them statically. > >>> > >>> 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. > >>> > >>> The intermediate-overlay.dtb file along with all other overlays is them > >>> applied to testcases.dtb to generate the master.dtb file. > >>> > >>> Signed-off-by: Viresh Kumar > >> > >> NACK to this specific patch, in its current form. > >> > >> There are restrictions on applying an overlay at runtime that do not apply > >> to applying an overlay to an FDT that will be loaded by the kernel during > >> early boot. Thus the unittest overlays _must_ be applied using the kernel > >> overlay loading methods to test the kernel runtime overlay loading feature. > > > > This patch doesn't take away from any of that and it completely orthogonal. > > Mea culpa. I took the patch header comment at face value, and read more into > the header comment than what was written there. I then skimmed the patch > instead of actually reading what it was doing. > > I incorrectly _assumed_ (bad!) that the intent was to replace applying the > individual overlay dtb's with the master.dtb. Reading more closely, I see > that the assumed final step of actually _using_ master.dtb does not exist. > > So, yes, I agree that the patch as written is orthogonal to my concern. > > My updated understanding is that this patch is attempting to use the existing > unittest overlay dts files as source to test fdtoverlay. And that the > resulting > dtb from fdtoverlay is not intended to be consumed by the kernel unittest. The goal is not to test fdtoverlay. dtc unittests do that. The goal is testing overlays we expect to be able to apply can actually apply and doing this at build time. That's also the goal for all the 'real' overlays which get added. > I do not agree that this is a good approach to testing fdtoverlay. The > unittest overlay dts files are constructed specifically to test various > parts of the kernel overlay code and dynamic OF code. Some of the content > of the overlays is constructed to trigger error conditions in that code, > and thus will not be able to be processed without error by fdtoverlay. Then those should be omitted. > Trying to use overlay dts files that are constructed to test runtime kernel > code as fdtoverlay input data mixes two different test environments and > objectives. If fdtoverlay test cases are desired, then fdtoverlay specific > dts files should be created. > > > > >> I agree that testing fdtoverlay is a good idea. I have not looked at the > >> parent project to see how much testing of fdtoverlay occurs there, but I > >> would prefer that fdtoverlay tests reside in the parent project if > >> practical > >> and reasonable. If there is some reason that some fdtoverlay tests are > >> more practical in the Linux kernel repository then I am open to adding > >> them to the Linux kernel tree. > > > > If you (or more importantly someone else sending us patches) make > > changes to the overlays, you can test that they apply at build time > > rather than runtime. I'll take it! So please help on fixing the issue > > because I want to apply this. > > If the tests can be added to the upstream project, I would much prefer > they reside there. If there is some reason a certain test is more > suited to be in the Linux kernel source tree then I also would like > it to be accepted here. Again, this is just about doing sanity checks at build time rather than *only* rely on runtime. > > > > And yes, dtc has fdtoverlay tests. But this patch shows there's at > > least 2 issues, > > > > fdtoverlay can't apply overlays to the root > > A test of that definitely belongs in the upstream project. Yes, agreed. > > and using an overlay as the base tree in UML is odd IMO. > > Am I still not fully understanding the patch? I'm missing how > this patch changes what dtb is used as the base tree in UML. This was more my theorising why Viresh is having problems in that perhaps fdtoverlay can't take an overlay as a base DT while the kernel can and does for UML. The fact that it works for UML seems wrong to me. Rob
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 8:04 AM, Rob Herring wrote: > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: >> >> On 1/8/21 2:41 AM, Viresh Kumar wrote: >>> Now that fdtoverlay is part of the kernel build, start using it to test >>> the unitest overlays we have by applying them statically. >>> >>> 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. >>> >>> The intermediate-overlay.dtb file along with all other overlays is them >>> applied to testcases.dtb to generate the master.dtb file. >>> >>> Signed-off-by: Viresh Kumar >> >> NACK to this specific patch, in its current form. >> >> There are restrictions on applying an overlay at runtime that do not apply >> to applying an overlay to an FDT that will be loaded by the kernel during >> early boot. Thus the unittest overlays _must_ be applied using the kernel >> overlay loading methods to test the kernel runtime overlay loading feature. > > This patch doesn't take away from any of that and it completely orthogonal. Mea culpa. I took the patch header comment at face value, and read more into the header comment than what was written there. I then skimmed the patch instead of actually reading what it was doing. I incorrectly _assumed_ (bad!) that the intent was to replace applying the individual overlay dtb's with the master.dtb. Reading more closely, I see that the assumed final step of actually _using_ master.dtb does not exist. So, yes, I agree that the patch as written is orthogonal to my concern. My updated understanding is that this patch is attempting to use the existing unittest overlay dts files as source to test fdtoverlay. And that the resulting dtb from fdtoverlay is not intended to be consumed by the kernel unittest. I do not agree that this is a good approach to testing fdtoverlay. The unittest overlay dts files are constructed specifically to test various parts of the kernel overlay code and dynamic OF code. Some of the content of the overlays is constructed to trigger error conditions in that code, and thus will not be able to be processed without error by fdtoverlay. Trying to use overlay dts files that are constructed to test runtime kernel code as fdtoverlay input data mixes two different test environments and objectives. If fdtoverlay test cases are desired, then fdtoverlay specific dts files should be created. > >> I agree that testing fdtoverlay is a good idea. I have not looked at the >> parent project to see how much testing of fdtoverlay occurs there, but I >> would prefer that fdtoverlay tests reside in the parent project if practical >> and reasonable. If there is some reason that some fdtoverlay tests are >> more practical in the Linux kernel repository then I am open to adding >> them to the Linux kernel tree. > > If you (or more importantly someone else sending us patches) make > changes to the overlays, you can test that they apply at build time > rather than runtime. I'll take it! So please help on fixing the issue > because I want to apply this. If the tests can be added to the upstream project, I would much prefer they reside there. If there is some reason a certain test is more suited to be in the Linux kernel source tree then I also would like it to be accepted here. > > And yes, dtc has fdtoverlay tests. But this patch shows there's at > least 2 issues, > fdtoverlay can't apply overlays to the root A test of that definitely belongs in the upstream project. > and using an overlay as the base tree in UML is odd IMO. Am I still not fully understanding the patch? I'm missing how this patch changes what dtb is used as the base tree in UML. > > Rob > -Frank
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 4:16 AM, Bill Mills wrote: > > > On 1/12/21 3:37 AM, Viresh Kumar wrote: >> On 11-01-21, 20:22, Bill Mills wrote: >>> On 1/11/21 5:06 PM, Frank Rowand wrote: NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. >> >> I wasn't looking to add any testing for fdtoverlay in the kernel, but >> then I stumbled upon unit-tests here and thought it would be a good >> idea to get this built using static tools as well, as we aren't >> required to add any new source files for this and the existing tests >> already cover a lot of nodes. >> >> And so I am fine if we don't want to do this stuff in kernel. >> >>> I thought we were aligned that any new overlays into the kernel today would >>> only be for boot loader applied case. Applying overlays at kernel runtime >>> was out of scope at your request. >>> >>> Rob had requested that the overlays be test applied at build time. I don't >>> think there is any way to test the kernel runtime method at build time >>> correct? >>> >>> Please clarify your concern and your suggested way forward. >> >> The kernel does some overlay testing currently (at kernel boot only, >> not later), to see if the overlays get applied correctly or not, these >> are the unit tests. >> >> What Frank is probably saying is that the unit-tests dtbs shouldn't >> get used for testing fdtoverlay stuff. He isn't asking to support >> runtime application of overlays, but to not do fdtoverlay testing in >> the kernel. Yes, Viresh is understanding my point. >> > > Thanks Viresh, that makes sense. Sorry for the confusion Frank. No problem Bill, communication by email is hard, and we end up spending a lot of time getting to the point of common understanding, it is the nature of the process. -Frank
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: > > On 1/8/21 2:41 AM, Viresh Kumar wrote: > > Now that fdtoverlay is part of the kernel build, start using it to test > > the unitest overlays we have by applying them statically. > > > > 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. > > > > The intermediate-overlay.dtb file along with all other overlays is them > > applied to testcases.dtb to generate the master.dtb file. > > > > Signed-off-by: Viresh Kumar > > NACK to this specific patch, in its current form. > > There are restrictions on applying an overlay at runtime that do not apply > to applying an overlay to an FDT that will be loaded by the kernel during > early boot. Thus the unittest overlays _must_ be applied using the kernel > overlay loading methods to test the kernel runtime overlay loading feature. This patch doesn't take away from any of that and it completely orthogonal. > I agree that testing fdtoverlay is a good idea. I have not looked at the > parent project to see how much testing of fdtoverlay occurs there, but I > would prefer that fdtoverlay tests reside in the parent project if practical > and reasonable. If there is some reason that some fdtoverlay tests are > more practical in the Linux kernel repository then I am open to adding > them to the Linux kernel tree. If you (or more importantly someone else sending us patches) make changes to the overlays, you can test that they apply at build time rather than runtime. I'll take it! So please help on fixing the issue because I want to apply this. And yes, dtc has fdtoverlay tests. But this patch shows there's at least 2 issues. fdtoverlay can't apply overlays to the root and using an overlay as the base tree in UML is odd IMO. Rob
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 3:37 AM, Viresh Kumar wrote: On 11-01-21, 20:22, Bill Mills wrote: On 1/11/21 5:06 PM, Frank Rowand wrote: NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. I wasn't looking to add any testing for fdtoverlay in the kernel, but then I stumbled upon unit-tests here and thought it would be a good idea to get this built using static tools as well, as we aren't required to add any new source files for this and the existing tests already cover a lot of nodes. And so I am fine if we don't want to do this stuff in kernel. I thought we were aligned that any new overlays into the kernel today would only be for boot loader applied case. Applying overlays at kernel runtime was out of scope at your request. Rob had requested that the overlays be test applied at build time. I don't think there is any way to test the kernel runtime method at build time correct? Please clarify your concern and your suggested way forward. The kernel does some overlay testing currently (at kernel boot only, not later), to see if the overlays get applied correctly or not, these are the unit tests. What Frank is probably saying is that the unit-tests dtbs shouldn't get used for testing fdtoverlay stuff. He isn't asking to support runtime application of overlays, but to not do fdtoverlay testing in the kernel. Thanks Viresh, that makes sense. Sorry for the confusion Frank. -- Bill Mills
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 11-01-21, 20:22, Bill Mills wrote: > On 1/11/21 5:06 PM, Frank Rowand wrote: > > NACK to this specific patch, in its current form. > > > > There are restrictions on applying an overlay at runtime that do not apply > > to applying an overlay to an FDT that will be loaded by the kernel during > > early boot. Thus the unittest overlays _must_ be applied using the kernel > > overlay loading methods to test the kernel runtime overlay loading feature. > > > > I agree that testing fdtoverlay is a good idea. I have not looked at the > > parent project to see how much testing of fdtoverlay occurs there, but I > > would prefer that fdtoverlay tests reside in the parent project if practical > > and reasonable. If there is some reason that some fdtoverlay tests are > > more practical in the Linux kernel repository then I am open to adding > > them to the Linux kernel tree. I wasn't looking to add any testing for fdtoverlay in the kernel, but then I stumbled upon unit-tests here and thought it would be a good idea to get this built using static tools as well, as we aren't required to add any new source files for this and the existing tests already cover a lot of nodes. And so I am fine if we don't want to do this stuff in kernel. > I thought we were aligned that any new overlays into the kernel today would > only be for boot loader applied case. Applying overlays at kernel runtime > was out of scope at your request. > > Rob had requested that the overlays be test applied at build time. I don't > think there is any way to test the kernel runtime method at build time > correct? > > Please clarify your concern and your suggested way forward. The kernel does some overlay testing currently (at kernel boot only, not later), to see if the overlays get applied correctly or not, these are the unit tests. What Frank is probably saying is that the unit-tests dtbs shouldn't get used for testing fdtoverlay stuff. He isn't asking to support runtime application of overlays, but to not do fdtoverlay testing in the kernel. -- viresh
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/11/21 5:06 PM, Frank Rowand wrote: On 1/8/21 2:41 AM, Viresh Kumar wrote: Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. 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. The intermediate-overlay.dtb file along with all other overlays is them applied to testcases.dtb to generate the master.dtb file. Signed-off-by: Viresh Kumar NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. Frank, I thought we were aligned that any new overlays into the kernel today would only be for boot loader applied case. Applying overlays at kernel runtime was out of scope at your request. Rob had requested that the overlays be test applied at build time. I don't think there is any way to test the kernel runtime method at build time correct? Please clarify your concern and your suggested way forward. Thanks, Bill -Frank --- Depends on: https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ I have kept the .dtb naming for overlays for now, lets see how we do it eventually. Rob/Frank, this doesn't work properly right now. Maybe I missed how these overlays must be applied or there is a bug in fdtoverlay. The master.dtb doesn't include any nodes from overlay_base.dtb or overlay.dtb probably because 'testcase-data-2' node isn't present in testcases.dtb and fdtoverlay doesn't allow applying new nodes to the root node, i.e. allows new sub-nodes once it gets phandle to the parent but nothing can be added to the root node itself. Though I get a feel that it works while applying the nodes dynamically and it is expected to work here as well. (And yeah, this is my first serious attempt at updating Makefiles, I am sure there is a scope of improvement here :)) --- drivers/of/unittest-data/Makefile | 23 +++ 1 file changed, 23 insertions(+) diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..f17bce85f65f 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays 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 \ + overlay_9.dtb overlay_10.dtb overlay_11.dtb \ + overlay_12.dtb overlay_13.dtb overlay_15.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 + +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 -- Bill Mills Principal Technical Consultant, Linaro +1-240-643-0836 TZ: US Eastern Work Schedule: Tues/Wed/Thur
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/11/21 9:46 AM, Rob Herring wrote: > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar 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? I'm a little slow responding to this thread. After seeing this question, I responded to the patch email with a NACK (with further explanation in that response). -Frank > >> The intermediate-overlay.dtb file along with all other overlays is them > > s/them/then/ > >> applied to testcases.dtb to generate the master.dtb file. >> >> Signed-off-by: Viresh Kumar >> >> --- >> Depends on: >> >> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ >> >> I have kept the .dtb naming for overlays for now, lets see how we do it >> eventually. >> >> Rob/Frank, this doesn't work properly right now. Maybe I missed how >> these overlays must be applied or there is a bug in fdtoverlay. >> >> The master.dtb doesn't include any nodes from overlay_base.dtb or >> overlay.dtb probably because 'testcase-data-2' node isn't present in >> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the >> root node, i.e. allows new sub-nodes once it gets phandle to the parent >> but nothing can be added to the root node itself. Though I get a feel >> that it works while applying the nodes dynamically and it is expected to >> work here as well. > > Sounds like a bug in fdtoverlay to me. Though maybe you need an empty > base tree. An overlay serving as the base is a bit odd so it's > somewhat understandable fdtoverlay couldn't handle that. OTOH, > combining 2 overlays together seems like a valid use. > >> >> (And yeah, this is my first serious attempt at updating Makefiles, I am >> sure there is a scope of improvement here :)) > > Usually I write something and Masahiro rewrites it for me. :) > > Rob >
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/8/21 2:41 AM, Viresh Kumar wrote: > Now that fdtoverlay is part of the kernel build, start using it to test > the unitest overlays we have by applying them statically. > > 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. > > The intermediate-overlay.dtb file along with all other overlays is them > applied to testcases.dtb to generate the master.dtb file. > > Signed-off-by: Viresh Kumar NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. -Frank > > --- > Depends on: > > https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ > > I have kept the .dtb naming for overlays for now, lets see how we do it > eventually. > > Rob/Frank, this doesn't work properly right now. Maybe I missed how > these overlays must be applied or there is a bug in fdtoverlay. > > The master.dtb doesn't include any nodes from overlay_base.dtb or > overlay.dtb probably because 'testcase-data-2' node isn't present in > testcases.dtb and fdtoverlay doesn't allow applying new nodes to the > root node, i.e. allows new sub-nodes once it gets phandle to the parent > but nothing can be added to the root node itself. Though I get a feel > that it works while applying the nodes dynamically and it is expected to > work here as well. > > (And yeah, this is my first serious attempt at updating Makefiles, I am > sure there is a scope of improvement here :)) > > --- > drivers/of/unittest-data/Makefile | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/of/unittest-data/Makefile > b/drivers/of/unittest-data/Makefile > index 009f4045c8e4..f17bce85f65f 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ > > # suppress warnings about intentional errors > DTC_FLAGS_testcases += -Wno-interrupts_property > + > +# Apply overlays 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 \ > +overlay_9.dtb overlay_10.dtb overlay_11.dtb \ > +overlay_12.dtb overlay_13.dtb overlay_15.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 > + > +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 >
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar 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? > The intermediate-overlay.dtb file along with all other overlays is them s/them/then/ > applied to testcases.dtb to generate the master.dtb file. > > Signed-off-by: Viresh Kumar > > --- > Depends on: > > https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ > > I have kept the .dtb naming for overlays for now, lets see how we do it > eventually. > > Rob/Frank, this doesn't work properly right now. Maybe I missed how > these overlays must be applied or there is a bug in fdtoverlay. > > The master.dtb doesn't include any nodes from overlay_base.dtb or > overlay.dtb probably because 'testcase-data-2' node isn't present in > testcases.dtb and fdtoverlay doesn't allow applying new nodes to the > root node, i.e. allows new sub-nodes once it gets phandle to the parent > but nothing can be added to the root node itself. Though I get a feel > that it works while applying the nodes dynamically and it is expected to > work here as well. Sounds like a bug in fdtoverlay to me. Though maybe you need an empty base tree. An overlay serving as the base is a bit odd so it's somewhat understandable fdtoverlay couldn't handle that. OTOH, combining 2 overlays together seems like a valid use. > > (And yeah, this is my first serious attempt at updating Makefiles, I am > sure there is a scope of improvement here :)) Usually I write something and Masahiro rewrites it for me. :) Rob
[PATCH] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. 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. The intermediate-overlay.dtb file along with all other overlays is them applied to testcases.dtb to generate the master.dtb file. Signed-off-by: Viresh Kumar --- Depends on: https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ I have kept the .dtb naming for overlays for now, lets see how we do it eventually. Rob/Frank, this doesn't work properly right now. Maybe I missed how these overlays must be applied or there is a bug in fdtoverlay. The master.dtb doesn't include any nodes from overlay_base.dtb or overlay.dtb probably because 'testcase-data-2' node isn't present in testcases.dtb and fdtoverlay doesn't allow applying new nodes to the root node, i.e. allows new sub-nodes once it gets phandle to the parent but nothing can be added to the root node itself. Though I get a feel that it works while applying the nodes dynamically and it is expected to work here as well. (And yeah, this is my first serious attempt at updating Makefiles, I am sure there is a scope of improvement here :)) --- drivers/of/unittest-data/Makefile | 23 +++ 1 file changed, 23 insertions(+) diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..f17bce85f65f 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays 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 \ + overlay_9.dtb overlay_10.dtb overlay_11.dtb \ + overlay_12.dtb overlay_13.dtb overlay_15.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 + +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 -- 2.25.0.rc1.19.g042ed3e048af
