| Message ID | 1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | of: unittest: Statically apply overlays using fdtoverlay | expand |
On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Now that fdtoverlay is part of the kernel build, start using it to test > the unitest overlays we have by applying them statically. Nice idea. > The file overlay_base.dtb have symbols of its own and we need to apply > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > us intermediate-overlay.dtb file. Okay? If restructuring things helps we should do that. Frank? > 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 <viresh.kumar@linaro.org> > > --- > Depends on: > > https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@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
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 <viresh.kumar@linaro.org> 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.kumar@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 >
On 1/11/21 9:46 AM, Rob Herring wrote: > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >> >> Now that fdtoverlay is part of the kernel build, start using it to test >> the unitest overlays we have by applying them statically. > > Nice idea. > >> The file overlay_base.dtb have symbols of its own and we need to apply >> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >> us intermediate-overlay.dtb file. > > Okay? If restructuring things helps we should do that. Frank? 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 <viresh.kumar@linaro.org> >> >> --- >> Depends on: >> >> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@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 >
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 <viresh.kumar@linaro.org> > > 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.kumar@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 >> >
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.
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.
On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> > > 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
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
On 1/12/21 8:04 AM, Rob Herring wrote: > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> >> >> 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
On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 1/12/21 8:04 AM, Rob Herring wrote: > > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> > >> > >> 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
On 1/12/21 1:41 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 1/12/21 8:04 AM, Rob Herring wrote: >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> >>>> >>>> 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 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. I'll have to go back and look at UML. I didn't recall that the base FDT for UML was an overlay. -Frank > > Rob >
On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 1/12/21 1:41 PM, Rob Herring wrote: > > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: > >> > >> On 1/12/21 8:04 AM, Rob Herring wrote: > >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> > >>>> > >>>> 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 least it should be. It's not really and that's why we have limited runtime applied overlay support. > 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. We're not changing nor constraining the data files. Just adding another sanity test on them. Rob
On 1/12/21 2:46 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 1/12/21 1:41 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>> >>>> On 1/12/21 8:04 AM, Rob Herring wrote: >>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> >>>>>> >>>>>> 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 already know that unittest overlays that are expected to be valid can apply successfully. The run time unittests already check for that. I don't see any value in adding a build time test for the same thing _for unittest overlay dts files_. And I do see an ongoing maintenance cost for _unittest overlay dts files_. If you want to add build time tests for all (or some) non-unittest overlay dts files, then I am not particularly opposed to that (but being aware that an overlay dtb could apply on top of more than one base dtb, so there is a possibility of an "explosion" of combinations to be maintained in the build system). I see value in having build time testing that overlay dtbs apply cleanly on a base dtb. I have heard frustration from the out of tree users of overlays that apply the overlays via the bootloader, because if the bootloader fails to apply an overlay it can be difficult to debug or fix on the target computer. Having a mechanism to specify what overlays are intended to be applied to a base dtb, and verify that they do apply would resolve some of those issues, assuming the boot loader and fdtoverlay are consistent with each other. > >> 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 least it should be. It's not really and that's why we have > limited runtime applied overlay support. > >> 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. > > We're not changing nor constraining the data files. Just adding > another sanity test on them. For _unittest_ dts files, I see no value add. And the cost of needing to track _in the build system_ which unittest dts files are expected fail to apply and which are expected to succeed. -Frank > > Rob >
On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 1/12/21 2:46 PM, Rob Herring wrote: > > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote: > >> > >> On 1/12/21 1:41 PM, Rob Herring wrote: > >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: > >>>> > >>>> On 1/12/21 8:04 AM, Rob Herring wrote: > >>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> > >>>>>> > >>>>>> 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 already know that unittest overlays that are expected to be valid > can apply successfully. The run time unittests already check for that. As soon as I apply a patch to one I don't know it's valid or can apply anymore. > I don't see any value in adding a build time test for the same thing > _for unittest overlay dts files_. And I do see an ongoing maintenance > cost for _unittest overlay dts files_. 0-day, kernelci, etc. will all build time test it. Actually, everyone doing allmodconfig builds will. Runtime testing requires *my* time. I'd like to say runtime testing is part of my highly automated workflow and unittests get run all the time, but they don't. > If you want to add build time tests for all (or some) non-unittest overlay > dts files, then I am not particularly opposed to that (but being aware that > an overlay dtb could apply on top of more than one base dtb, so there > is a possibility of an "explosion" of combinations to be maintained > in the build system). Yes, that could be a problem. I think reality is most overlays we're willing to accept will be board specific. > I see value in having build time testing that overlay dtbs apply cleanly > on a base dtb. I have heard frustration from the out of tree users of > overlays that apply the overlays via the bootloader, because if the > bootloader fails to apply an overlay it can be difficult to debug or > fix on the target computer. Having a mechanism to specify what overlays > are intended to be applied to a base dtb, and verify that they do > apply would resolve some of those issues, assuming the boot loader > and fdtoverlay are consistent with each other. Yes, that's one main reason to require applying them at build time. The other is if people want to refactor a current dtb into a base and overlay, then we should still produce the original combined dtb. > >> 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 least it should be. It's not really and that's why we have > > limited runtime applied overlay support. > > > >> 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. > > > > We're not changing nor constraining the data files. Just adding > > another sanity test on them. > > For _unittest_ dts files, I see no value add. And the cost of needing > to track _in the build system_ which unittest dts files are expected fail > to apply and which are expected to succeed. It costs nothing to add it (well, it would have been before this thread). If it becomes problematic, then we can drop it. Rob
On 1/13/21 9:05 AM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 1/12/21 2:46 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>> >>>> On 1/12/21 1:41 PM, Rob Herring wrote: >>>>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>>>> >>>>>> On 1/12/21 8:04 AM, Rob Herring wrote: >>>>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> 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 <viresh.kumar@linaro.org> >>>>>>>> >>>>>>>> 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 already know that unittest overlays that are expected to be valid >> can apply successfully. The run time unittests already check for that. > > As soon as I apply a patch to one I don't know it's valid or can apply anymore. > >> I don't see any value in adding a build time test for the same thing >> _for unittest overlay dts files_. And I do see an ongoing maintenance >> cost for _unittest overlay dts files_. > > 0-day, kernelci, etc. will all build time test it. Actually, everyone > doing allmodconfig builds will. Runtime testing requires *my* time. > I'd like to say runtime testing is part of my highly automated > workflow and unittests get run all the time, but they don't. I would like to think that if you apply a patch _to a unittest overlay_ that you would run the unittests afterward. But I can also see that applying a random patch to devicetree code could also unexpectedly result in a unittest failing. OK, I would say that helping your workflow is sufficient positive value to more than offset the slightly negative that I was noting. A simple one line comment at the list of unittest overlays static tested saying that some other overlays are not tested because they are designed to intentionally fail to apply would make me happy. And fortunately I do run the unittests, normally for each major release and each -rc1 at a minimum so at least that much unittest coverage is present. My big unittest gap is that I don't test all architectures. I withdraw my NACK. -Frank > >> If you want to add build time tests for all (or some) non-unittest overlay >> dts files, then I am not particularly opposed to that (but being aware that >> an overlay dtb could apply on top of more than one base dtb, so there >> is a possibility of an "explosion" of combinations to be maintained >> in the build system). > > Yes, that could be a problem. I think reality is most overlays we're > willing to accept will be board specific. > >> I see value in having build time testing that overlay dtbs apply cleanly >> on a base dtb. I have heard frustration from the out of tree users of >> overlays that apply the overlays via the bootloader, because if the >> bootloader fails to apply an overlay it can be difficult to debug or >> fix on the target computer. Having a mechanism to specify what overlays >> are intended to be applied to a base dtb, and verify that they do >> apply would resolve some of those issues, assuming the boot loader >> and fdtoverlay are consistent with each other. > > Yes, that's one main reason to require applying them at build time. > > The other is if people want to refactor a current dtb into a base and > overlay, then we should still produce the original combined dtb. > >>>> 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 least it should be. It's not really and that's why we have >>> limited runtime applied overlay support. >>> >>>> 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. >>> >>> We're not changing nor constraining the data files. Just adding >>> another sanity test on them. >> >> For _unittest_ dts files, I see no value add. And the cost of needing >> to track _in the build system_ which unittest dts files are expected fail >> to apply and which are expected to succeed. > > It costs nothing to add it (well, it would have been before this > thread). If it becomes problematic, then we can drop it. > > Rob >
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
On 11-01-21, 09:46, Rob Herring wrote: > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Now that fdtoverlay is part of the kernel build, start using it to test > > the unitest overlays we have by applying them statically. > > Nice idea. > > > The file overlay_base.dtb have symbols of its own and we need to apply > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > > us intermediate-overlay.dtb file. > > Okay? If restructuring things helps we should do that. Frank? Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi and include it from testcases.dts like the other ones ?
On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 11-01-21, 09:46, Rob Herring wrote: > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > Now that fdtoverlay is part of the kernel build, start using it to test > > > the unitest overlays we have by applying them statically. > > > > Nice idea. > > > > > The file overlay_base.dtb have symbols of its own and we need to apply > > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > > > us intermediate-overlay.dtb file. > > > > Okay? If restructuring things helps we should do that. Frank? > > Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi > and include it from testcases.dts like the other ones ? No, because overlay_base.dts is an overlay dt. 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
+David, On 14-01-21, 09:01, Rob Herring wrote: > On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 11-01-21, 09:46, Rob Herring wrote: > > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > Now that fdtoverlay is part of the kernel build, start using it to test > > > > the unitest overlays we have by applying them statically. > > > > > > Nice idea. > > > > > > > The file overlay_base.dtb have symbols of its own and we need to apply > > > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > > > > us intermediate-overlay.dtb file. > > > > > > Okay? If restructuring things helps we should do that. Frank? > > > > Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi > > and include it from testcases.dts like the other ones ? > > No, because overlay_base.dts is an overlay dt. 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 { + &overlay_base { #address-cells = <1>; #size-cells = <1>; @@ -89,5 +88,3 @@ retail_1: vending@50000 { }; }; -}; - diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts index a85b5e1c381a..539dc7d9eddc 100644 --- a/drivers/of/unittest-data/testcases.dts +++ b/drivers/of/unittest-data/testcases.dts @@ -11,6 +11,11 @@ node-remove { }; }; }; + + overlay_base: testcase-data-2 { + #address-cells = <1>; + #size-cells = <1>; + }; -------------------------8<------------------------- And then we can do this to the Makefile over my changes. -------------------------8<------------------------- diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 9f3eb30b78f1..8cc23311b778 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property # Apply all overlays (except overlay_bad_* as they are not supposed to apply and # fail build) statically with fdtoverlay -intermediate-overlay := overlay.dtb master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ overlay_3.dtb overlay_4.dtb overlay_5.dtb \ overlay_6.dtb overlay_7.dtb overlay_8.dtb \ @@ -50,15 +49,12 @@ master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ overlay_gpio_01.dtb overlay_gpio_02a.dtb \ overlay_gpio_02b.dtb overlay_gpio_03.dtb \ overlay_gpio_04a.dtb overlay_gpio_04b.dtb \ - intermediate-overlay.dtb + overlay_base.dtb overlay.dtb quiet_cmd_fdtoverlay = fdtoverlay $@ cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay)) - $(call if_changed,fdtoverlay) - $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) $(call if_changed,fdtoverlay) -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb +always-$(CONFIG_OF_OVERLAY) += master.dtb
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 <viresh.kumar@linaro.org> wrote: >>> >>> On 11-01-21, 09:46, Rob Herring wrote: >>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>>> >>>>> Now that fdtoverlay is part of the kernel build, start using it to test >>>>> the unitest overlays we have by applying them statically. >>>> >>>> Nice idea. >>>> >>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>>>> us intermediate-overlay.dtb file. >>>> >>>> Okay? If restructuring things helps we should do that. Frank? >>> >>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi >>> and include it from testcases.dts like the other ones ? 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 { > + &overlay_base { > #address-cells = <1>; > #size-cells = <1>; > > @@ -89,5 +88,3 @@ retail_1: vending@50000 { > }; > > }; > -}; > - > diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts > index a85b5e1c381a..539dc7d9eddc 100644 > --- a/drivers/of/unittest-data/testcases.dts > +++ b/drivers/of/unittest-data/testcases.dts > @@ -11,6 +11,11 @@ node-remove { > }; > }; > }; > + > + overlay_base: testcase-data-2 { > + #address-cells = <1>; > + #size-cells = <1>; > + }; > > -------------------------8<------------------------- > > And then we can do this to the Makefile over my changes. > > -------------------------8<------------------------- > > diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile > index 9f3eb30b78f1..8cc23311b778 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property > > # Apply all overlays (except overlay_bad_* as they are not supposed to apply and > # fail build) statically with fdtoverlay > -intermediate-overlay := overlay.dtb > master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ > overlay_3.dtb overlay_4.dtb overlay_5.dtb \ > overlay_6.dtb overlay_7.dtb overlay_8.dtb \ > @@ -50,15 +49,12 @@ master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ > overlay_gpio_01.dtb overlay_gpio_02a.dtb \ > overlay_gpio_02b.dtb overlay_gpio_03.dtb \ > overlay_gpio_04a.dtb overlay_gpio_04b.dtb \ > - intermediate-overlay.dtb > + overlay_base.dtb overlay.dtb > > quiet_cmd_fdtoverlay = fdtoverlay $@ > cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ > > -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay)) > - $(call if_changed,fdtoverlay) > - > $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) > $(call if_changed,fdtoverlay) > > -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb > +always-$(CONFIG_OF_OVERLAY) += master.dtb >
On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote: > +David, > > On 14-01-21, 09:01, Rob Herring wrote: > > On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 11-01-21, 09:46, Rob Herring wrote: > > > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > > > Now that fdtoverlay is part of the kernel build, start using it to test > > > > > the unitest overlays we have by applying them statically. > > > > > > > > Nice idea. > > > > > > > > > The file overlay_base.dtb have symbols of its own and we need to apply > > > > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > > > > > us intermediate-overlay.dtb file. > > > > > > > > Okay? If restructuring things helps we should do that. Frank? > > > > > > Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi > > > and include it from testcases.dts like the other ones ? > > > > No, because overlay_base.dts is an overlay dt. So.. I was confused for a bit here, because the overlay_base.dts you're discussing is the one in the kernel tree, not the one in the dtc tree. The kernel file is confusing to me. It has the /plugin/ tag which should be used for overlays, but the rest of the file looks like a base tree rather than an overlay. There's even a comment saying "Base device tree that overlays will be applied against". But it goes on to talk about __fixups__ and __local__fixups__ which will never be generated for a based tree. Oh.. and then there's terminology confusion. dtc has had compile time resolved overlays for a very long time. Those are usually .dtsi files, and should generally not have /plugin/. More recent is the support for run-time overlays - .dtbo output files, which are usually generated from a .dts with /plugin/. They usually should *not* have a "/ { ... } " stanza, since that indicates the base portion of the tree. > What property of a file makes it an overlay ? Just the presence of > /plugin/; ? Yes and no. Generally that's how it should work , but it looks to me like the presence of /plugin/ there is just wrong. /plugin/ basically just activates some extra dtc features, though, so it is possible to "manually" construct a valid overlay without it. > David, we are talking about the overlay base[1] file here. The > fdtoverlay tool > fails to apply it to testcases.dts file (in the same directory) because none of > its nodes have the __overlay__ label and the dtc routine overlay_merge() [2] > skips them intentionally. testcases.dts also has /plugin/ and again, it's not really clear if that's right. The point of /plugin/ is that it will automatically generate the __overlay__ subnodes from dtc's &label { ... } or &{/path} { ... } syntax, so you wouldn't expect to see __overlay__ in the source. > > I think we need an > > empty, minimal base.dtb to apply overlay_base.dtbo to. > > One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that > will make it work. > > > And then fdtoverlay needs a fix to apply overlays to the root node? > > It isn't just root node I think, but any node for which the __overlay__ label > isn't there. > > So this can make it all work if everyone is fine with it: > > diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts > index 99ab9d12d00b..59172c4c9e5a 100644 > --- a/drivers/of/unittest-data/overlay_base.dts > +++ b/drivers/of/unittest-data/overlay_base.dts > @@ -11,8 +11,7 @@ > * dtc will create nodes "/__symbols__" and "/__local_fixups__". > */ > > -/ { > - testcase-data-2 { > + &overlay_base { > #address-cells = <1>; > #size-cells = <1>; > > @@ -89,5 +88,3 @@ retail_1: vending@50000 { > }; > > }; > -}; > - > diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts > index a85b5e1c381a..539dc7d9eddc 100644 > --- a/drivers/of/unittest-data/testcases.dts > +++ b/drivers/of/unittest-data/testcases.dts > @@ -11,6 +11,11 @@ node-remove { > }; > }; > }; > + > + overlay_base: testcase-data-2 { > + #address-cells = <1>; > + #size-cells = <1>; > + }; > > -------------------------8<------------------------- > > And then we can do this to the Makefile over my changes. > > -------------------------8<------------------------- > > diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile > index 9f3eb30b78f1..8cc23311b778 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property > > # Apply all overlays (except overlay_bad_* as they are not supposed to apply and > # fail build) statically with fdtoverlay > -intermediate-overlay := overlay.dtb > master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ > overlay_3.dtb overlay_4.dtb overlay_5.dtb \ > overlay_6.dtb overlay_7.dtb overlay_8.dtb \ > @@ -50,15 +49,12 @@ master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ > overlay_gpio_01.dtb overlay_gpio_02a.dtb \ > overlay_gpio_02b.dtb overlay_gpio_03.dtb \ > overlay_gpio_04a.dtb overlay_gpio_04b.dtb \ > - intermediate-overlay.dtb > + overlay_base.dtb overlay.dtb > > quiet_cmd_fdtoverlay = fdtoverlay $@ > cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ > > -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay)) > - $(call if_changed,fdtoverlay) > - > $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) > $(call if_changed,fdtoverlay) > > -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb > +always-$(CONFIG_OF_OVERLAY) += master.dtb >
From: Frank Rowand <frank.rowand@sony.com>
These changes apply on top of the patches in:
[PATCH] of: unittest: Statically apply overlays using fdtoverlay
Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@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
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 >
On 1/18/21 12:30 AM, David Gibson wrote: > On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote: >> +David, >> >> On 14-01-21, 09:01, Rob Herring wrote: >>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>> >>>> On 11-01-21, 09:46, Rob Herring wrote: >>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>>>> >>>>>> Now that fdtoverlay is part of the kernel build, start using it to test >>>>>> the unitest overlays we have by applying them statically. >>>>> >>>>> Nice idea. >>>>> >>>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>>>>> us intermediate-overlay.dtb file. >>>>> >>>>> Okay? If restructuring things helps we should do that. Frank? >>>> >>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi >>>> and include it from testcases.dts like the other ones ? >>> >>> No, because overlay_base.dts is an overlay dt. > > So.. I was confused for a bit here, because the overlay_base.dts > you're discussing is the one in the kernel tree, not the one in the > dtc tree. > > The kernel file is confusing to me. It has the /plugin/ tag which It should be confusing to you, without having dug deeply into the evil things that unittest does. Unittest does a bunch of contortions and abuse of direct access into the kernel devicetree code internals to be able to create and test for abnormal conditions. No other code should emulate the bizarre things that unittest does. -Frank > should be used for overlays, but the rest of the file looks like a > base tree rather than an overlay. There's even a comment saying "Base > device tree that overlays will be applied against". But it goes on to > talk about __fixups__ and __local__fixups__ which will never be > generated for a based tree. > > Oh.. and then there's terminology confusion. dtc has had compile time > resolved overlays for a very long time. Those are usually .dtsi > files, and should generally not have /plugin/. > > More recent is the support for run-time overlays - .dtbo output files, > which are usually generated from a .dts with /plugin/. They usually > should *not* have a "/ { ... } " stanza, since that indicates the base > portion of the tree. > >> What property of a file makes it an overlay ? Just the presence of >> /plugin/; ? > > Yes and no. Generally that's how it should work , but it looks to me > like the presence of /plugin/ there is just wrong. /plugin/ basically > just activates some extra dtc features, though, so it is possible to > "manually" construct a valid overlay without it. > >> David, we are talking about the overlay base[1] file here. The >> fdtoverlay tool >> fails to apply it to testcases.dts file (in the same directory) because none of >> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2] >> skips them intentionally. > > testcases.dts also has /plugin/ and again, it's not really clear if > that's right. > > The point of /plugin/ is that it will automatically generate the > __overlay__ subnodes from dtc's &label { ... } or &{/path} { ... } > syntax, so you wouldn't expect to see __overlay__ in the source. > >>> I think we need an >>> empty, minimal base.dtb to apply overlay_base.dtbo to. >> >> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that >> will make it work. >> >>> And then fdtoverlay needs a fix to apply overlays to the root node? >> >> It isn't just root node I think, but any node for which the __overlay__ label >> isn't there. >> >> So this can make it all work if everyone is fine with it: >> >> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts >> index 99ab9d12d00b..59172c4c9e5a 100644 >> --- a/drivers/of/unittest-data/overlay_base.dts >> +++ b/drivers/of/unittest-data/overlay_base.dts >> @@ -11,8 +11,7 @@ >> * dtc will create nodes "/__symbols__" and "/__local_fixups__". >> */ >> >> -/ { >> - testcase-data-2 { >> + &overlay_base { >> #address-cells = <1>; >> #size-cells = <1>; >> >> @@ -89,5 +88,3 @@ retail_1: vending@50000 { >> }; >> >> }; >> -}; >> - >> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts >> index a85b5e1c381a..539dc7d9eddc 100644 >> --- a/drivers/of/unittest-data/testcases.dts >> +++ b/drivers/of/unittest-data/testcases.dts >> @@ -11,6 +11,11 @@ node-remove { >> }; >> }; >> }; >> + >> + overlay_base: testcase-data-2 { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + }; >> >> -------------------------8<------------------------- >> >> And then we can do this to the Makefile over my changes. >> >> -------------------------8<------------------------- >> >> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile >> index 9f3eb30b78f1..8cc23311b778 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property >> >> # Apply all overlays (except overlay_bad_* as they are not supposed to apply and >> # fail build) statically with fdtoverlay >> -intermediate-overlay := overlay.dtb >> master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ >> overlay_3.dtb overlay_4.dtb overlay_5.dtb \ >> overlay_6.dtb overlay_7.dtb overlay_8.dtb \ >> @@ -50,15 +49,12 @@ master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ >> overlay_gpio_01.dtb overlay_gpio_02a.dtb \ >> overlay_gpio_02b.dtb overlay_gpio_03.dtb \ >> overlay_gpio_04a.dtb overlay_gpio_04b.dtb \ >> - intermediate-overlay.dtb >> + overlay_base.dtb overlay.dtb >> >> quiet_cmd_fdtoverlay = fdtoverlay $@ >> cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >> >> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay)) >> - $(call if_changed,fdtoverlay) >> - >> $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) >> $(call if_changed,fdtoverlay) >> >> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb >> +always-$(CONFIG_OF_OVERLAY) += master.dtb >> >
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 <viresh.kumar@linaro.org> wrote: >>>> >>>> On 11-01-21, 09:46, Rob Herring wrote: >>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>>>> >>>>>> Now that fdtoverlay is part of the kernel build, start using it to test >>>>>> the unitest overlays we have by applying them statically. >>>>> >>>>> Nice idea. >>>>> >>>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>>>>> us intermediate-overlay.dtb file. >>>>> >>>>> Okay? If restructuring things helps we should do that. Frank? >>>> >>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi >>>> and include it from testcases.dts like the other ones ? > > 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 { >> + &overlay_base { >> #address-cells = <1>; >> #size-cells = <1>; >> >> @@ -89,5 +88,3 @@ retail_1: vending@50000 { >> }; >> >> }; >> -}; >> - >> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts >> index a85b5e1c381a..539dc7d9eddc 100644 >> --- a/drivers/of/unittest-data/testcases.dts >> +++ b/drivers/of/unittest-data/testcases.dts >> @@ -11,6 +11,11 @@ node-remove { >> }; >> }; >> }; >> + >> + overlay_base: testcase-data-2 { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + }; >> >> -------------------------8<------------------------- >> >> And then we can do this to the Makefile over my changes. >> >> -------------------------8<------------------------- >> >> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile >> index 9f3eb30b78f1..8cc23311b778 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property >> >> # Apply all overlays (except overlay_bad_* as they are not supposed to apply and >> # fail build) statically with fdtoverlay >> -intermediate-overlay := overlay.dtb >> master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ >> overlay_3.dtb overlay_4.dtb overlay_5.dtb \ >> overlay_6.dtb overlay_7.dtb overlay_8.dtb \ >> @@ -50,15 +49,12 @@ master := overlay_0.dtb overlay_1.dtb overlay_2.dtb \ >> overlay_gpio_01.dtb overlay_gpio_02a.dtb \ >> overlay_gpio_02b.dtb overlay_gpio_03.dtb \ >> overlay_gpio_04a.dtb overlay_gpio_04b.dtb \ >> - intermediate-overlay.dtb >> + overlay_base.dtb overlay.dtb >> >> quiet_cmd_fdtoverlay = fdtoverlay $@ >> cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >> >> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay)) >> - $(call if_changed,fdtoverlay) >> - >> $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) >> $(call if_changed,fdtoverlay) >> >> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb >> +always-$(CONFIG_OF_OVERLAY) += master.dtb >> > > . >
On 18-01-21, 20:21, frowand.list@gmail.com wrote: > From: Frank Rowand <frank.rowand@sony.com> > > These changes apply on top of the patches in: > > [PATCH] of: unittest: Statically apply overlays using fdtoverlay > Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@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.kumar@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 { + &overlay_base { #address-cells = <1>; #size-cells = <1>; @@ -89,5 +88,3 @@ retail_1: vending@50000 { }; }; -}; - diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts index a85b5e1c381a..539dc7d9eddc 100644 --- a/drivers/of/unittest-data/testcases.dts +++ b/drivers/of/unittest-data/testcases.dts @@ -11,6 +11,11 @@ node-remove { }; }; }; + + overlay_base: testcase-data-2 { + #address-cells = <1>; + #size-cells = <1>; + }; > -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb > +always-$(CONFIG_OF_OVERLAY) += static_test.dtb This is how static_test.dtb looks now with fdtdump /dts-v1/; // magic: 0xd00dfeed // totalsize: 0x261b (9755) // off_dt_struct: 0x38 // off_dt_strings: 0x22dc // off_mem_rsvmap: 0x28 // version: 17 // last_comp_version: 16 // boot_cpuid_phys: 0x0 // size_dt_strings: 0x33f // size_dt_struct: 0x22a4 / { #address-cells = <0x00000001>; #size-cells = <0x00000001>; testcase-data { security-password = "password"; duplicate-name = "duplicate"; #address-cells = <0x00000001>; #size-cells = <0x00000001>; ranges; phandle = <0x0000000b>; changeset { prop-update = "hello"; prop-remove = "world"; node-remove { }; }; duplicate-name { }; phandle-tests { provider0 { #phandle-cells = <0x00000000>; phandle = <0x00000002>; }; provider1 { #phandle-cells = <0x00000001>; phandle = <0x00000001>; }; provider2 { #phandle-cells = <0x00000002>; phandle = <0x00000004>; }; provider3 { #phandle-cells = <0x00000003>; phandle = <0x00000003>; }; provider4 { #phandle-cells = <0x00000002>; phandle-map = <0x00000000 0x00000001 0x00000001 0x00000003 0x00000004 0x00000000 0x00000002 0x00000010 0x00000005 0x00000003 0x00000003 0x00000005 0x00000000 0x000000c8 0x00000008 0x00000004 0x00000017 0x00000006 0x00000013 0x00000000 0x00000004 0x0000000f 0x00000000 0x00000002 0x00000003 0x00000003 0x00000002 0x00000005 0x00000003>; phandle-map-mask = <0x000000ff 0x0000000f>; phandle-map-pass-thru = <0x00000000 0x000000f0>; phandle = <0x00000005>; }; consumer-a { phandle-list = <0x00000001 0x00000001 0x00000004 0x00000002 0x00000000 0x00000000 0x00000003 0x00000004 0x00000004 0x00000003 0x00000004 0x00000005 0x00000064 0x00000002 0x00000001 0x00000007>; phandle-list-names = "first", "second", "third"; phandle-list-bad-phandle = <0x00bc614e 0x00000000 0x00000000>; phandle-list-bad-args = <0x00000004 0x00000001 0x00000000 0x00000003 0x00000000>; empty-property; string-property = "foobar"; unterminated-string = <0x40414243>; unterminated-string-list = [66 69 72 73 74 00 73 65 63 6f 6e 64 00 40 41 42 43]; }; consumer-b { phandle-list = <0x00000001 0x00000001 0x00000005 0x00000002 0x00000003 0x00000000 0x00000005 0x00000004 0x00000100 0x00000005 0x00000000 0x00000061 0x00000002 0x00000005 0x00000013 0x00000020>; phandle-list-bad-phandle = <0x00bc614e 0x00000000 0x00000000>; phandle-list-bad-args = <0x00000004 0x00000001 0x00000000 0x00000005 0x00000000>; }; }; interrupts { #address-cells = <0x00000001>; #size-cells = <0x00000001>; intc0 { interrupt-controller; #interrupt-cells = <0x00000001>; phandle = <0x00000006>; }; intc1 { interrupt-controller; #interrupt-cells = <0x00000003>; phandle = <0x00000007>; }; intc2 { interrupt-controller; #interrupt-cells = <0x00000002>; phandle = <0x00000008>; }; intmap0 { #interrupt-cells = <0x00000001>; #address-cells = <0x00000000>; interrupt-map = <0x00000001 0x00000006 0x00000009 0x00000002 0x00000007 0x0000000a 0x0000000b 0x0000000c 0x00000003 0x00000008 0x0000000d 0x0000000e 0x00000004 0x00000008 0x0000000f 0x00000010>; phandle = <0x00000009>; }; intmap1 { #interrupt-cells = <0x00000002>; interrupt-map = <0x00005000 0x00000001 0x00000002 0x00000006 0x0000000f>; phandle = <0x0000000a>; }; interrupts0 { interrupt-parent = <0x00000006>; interrupts = <0x00000001 0x00000002 0x00000003 0x00000004>; }; interrupts1 { interrupt-parent = <0x00000009>; interrupts = <0x00000001 0x00000002 0x00000003 0x00000004>; }; interrupts-extended0 { reg = <0x00005000 0x00000100>; interrupts-extended = <0x00000006 0x00000001 0x00000007 0x00000002 0x00000003 0x00000004 0x00000008 0x00000005 0x00000006 0x00000009 0x00000001 0x00000009 0x00000002 0x00000009 0x00000003 0x0000000a 0x00000001 0x00000002>; }; }; testcase-device1 { compatible = "testcase-device"; interrupt-parent = <0x00000006>; interrupts = <0x00000001>; }; testcase-device2 { compatible = "testcase-device"; interrupt-parent = <0x00000008>; interrupts = <0x00000001>; }; match-node { name0 { }; name1 { device_type = "type1"; }; a { name2 { device_type = "type1"; }; }; b { name2 { }; }; c { name2 { device_type = "type2"; }; }; name3 { compatible = "compat3"; }; name4 { compatible = "compat2", "compat3"; }; name5 { compatible = "compat2", "compat3"; }; name6 { compatible = "compat1", "compat2", "compat3"; }; name7 { compatible = "compat2"; device_type = "type1"; }; name8 { compatible = "compat2"; device_type = "type1"; }; name9 { compatible = "compat2"; }; }; address-tests { #address-cells = <0x00000001>; #size-cells = <0x00000001>; ranges = <0x70000000 0x70000000 0x40000000 0x00000000 0xd0000000 0x20000000>; dma-ranges = <0x00000000 0x20000000 0x40000000>; device@70000000 { reg = <0x70000000 0x00001000>; }; bus@80000000 { #address-cells = <0x00000002>; #size-cells = <0x00000002>; ranges = <0x00000000 0x00000000 0x80000000 0x00000000 0x00100000>; dma-ranges = <0x00000001 0x00000000 0x00000000 0x00000020 0x00000000>; device@1000 { reg = <0x00000000 0x00001000 0x00000000 0x00001000>; }; }; pci@90000000 { device_type = "pci"; #address-cells = <0x00000003>; #size-cells = <0x00000002>; reg = <0x90000000 0x00001000>; ranges = <0x42000000 0x00000000 0x40000000 0x40000000 0x00000000 0x10000000>; dma-ranges = <0x42000000 0x00000000 0x80000000 0x00000000 0x00000000 0x10000000 0x42000000 0x00000000 0xc0000000 0x20000000 0x00000000 0x10000000>; }; }; platform-tests { #address-cells = <0x00000001>; #size-cells = <0x00000000>; test-device@0 { compatible = "test-device"; reg = <0x00000000>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; dev@100 { compatible = "test-sub-device"; reg = <0x00000100>; }; }; test-device@1 { compatible = "test-device"; reg = <0x00000001>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; dev@100 { compatible = "test-sub-device", "test-compat2", "test-compat3"; reg = <0x00000100>; }; }; }; overlay-node { test-bus { compatible = "simple-bus"; #address-cells = <0x00000001>; #size-cells = <0x00000000>; phandle = <0x0000000c>; gpio@4 { gpio-line-names = "line-A", "line-B", "line-C", "line-D"; ngpios = <0x00000002>; #gpio-cells = <0x00000002>; gpio-controller; reg = <0x00000004>; compatible = "unittest-gpio"; line-c { line-name = "line-C-input"; input; gpios = <0x00000003 0x00000000>; gpio-hog; }; }; gpio@3 { gpio-line-names = "line-A", "line-B", "line-C", "line-D"; ngpios = <0x00000002>; #gpio-cells = <0x00000002>; gpio-controller; reg = <0x00000003>; compatible = "unittest-gpio"; line-d { line-name = "line-D-input"; input; gpios = <0x00000004 0x00000000>; gpio-hog; }; }; gpio@2 { gpio-line-names = "line-A", "line-B"; ngpios = <0x00000002>; #gpio-cells = <0x00000002>; gpio-controller; reg = <0x00000002>; compatible = "unittest-gpio"; line-a { line-name = "line-A-input"; input; gpios = <0x00000001 0x00000000>; gpio-hog; }; }; gpio@0 { gpio-line-names = "line-A", "line-B"; ngpios = <0x00000002>; #gpio-cells = <0x00000002>; gpio-controller; reg = <0x00000000>; compatible = "unittest-gpio"; line-b { line-name = "line-B-input"; input; gpios = <0x00000002 0x00000000>; gpio-hog; }; }; test-unittest11 { #size-cells = <0x00000000>; #address-cells = <0x00000001>; reg = <0x0000000b>; status = "okay"; compatible = "unittest"; test-unittest111 { reg = <0x00000001>; status = "okay"; compatible = "unittest"; }; }; test-unittest10 { #size-cells = <0x00000000>; #address-cells = <0x00000001>; reg = <0x0000000a>; status = "okay"; compatible = "unittest"; test-unittest101 { reg = <0x00000001>; status = "okay"; compatible = "unittest"; }; }; test-unittest4 { reg = <0x00000004>; status = "okay"; compatible = "unittest"; }; test-unittest100 { compatible = "unittest"; status = "okay"; reg = <0x00000064>; phandle = <0x0000000d>; }; test-unittest101 { compatible = "unittest"; status = "disabled"; reg = <0x00000065>; phandle = <0x0000000e>; }; test-unittest0 { compatible = "unittest"; status = "okay"; reg = <0x00000000>; phandle = <0x0000000f>; }; test-unittest1 { compatible = "unittest"; status = "disabled"; reg = <0x00000001>; phandle = <0x00000010>; }; test-unittest2 { compatible = "unittest"; status = "okay"; reg = <0x00000002>; phandle = <0x00000011>; }; test-unittest3 { compatible = "unittest"; status = "disabled"; reg = <0x00000003>; phandle = <0x00000012>; }; test-unittest5 { compatible = "unittest"; status = "okay"; reg = <0x00000005>; phandle = <0x00000013>; }; test-unittest6 { compatible = "unittest"; status = "okay"; reg = <0x00000006>; phandle = <0x00000014>; }; test-unittest7 { compatible = "unittest"; status = "okay"; reg = <0x00000007>; phandle = <0x00000015>; }; test-unittest8 { property-foo = "bar"; compatible = "unittest"; status = "okay"; reg = <0x00000008>; phandle = <0x00000016>; }; i2c-test-bus { compatible = "unittest-i2c-bus"; status = "okay"; reg = <0x00000032>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; phandle = <0x00000017>; test-unittest15 { #size-cells = <0x00000000>; #address-cells = <0x00000001>; status = "okay"; compatible = "unittest-i2c-mux"; reg = <0x0000000b>; i2c@0 { reg = <0x00000000>; #size-cells = <0x00000000>; #address-cells = <0x00000001>; test-mux-dev@20 { status = "okay"; compatible = "unittest-i2c-dev"; reg = <0x00000020>; }; }; }; test-unittest12 { reg = <0x00000008>; compatible = "unittest-i2c-dev"; status = "okay"; }; test-unittest13 { reg = <0x00000009>; compatible = "unittest-i2c-dev"; status = "disabled"; }; test-unittest14 { reg = <0x0000000a>; compatible = "unittest-i2c-mux"; status = "okay"; #address-cells = <0x00000001>; #size-cells = <0x00000000>; i2c@0 { #address-cells = <0x00000001>; #size-cells = <0x00000000>; reg = <0x00000000>; test-mux-dev@20 { reg = <0x00000020>; compatible = "unittest-i2c-dev"; status = "okay"; }; }; }; }; }; }; }; aliases { testcase-alias = "/testcase-data"; }; __symbols__ { testcase = "/testcase-data"; provider0 = "/testcase-data/phandle-tests/provider0"; provider1 = "/testcase-data/phandle-tests/provider1"; provider2 = "/testcase-data/phandle-tests/provider2"; provider3 = "/testcase-data/phandle-tests/provider3"; provider4 = "/testcase-data/phandle-tests/provider4"; test_intc0 = "/testcase-data/interrupts/intc0"; test_intc1 = "/testcase-data/interrupts/intc1"; test_intc2 = "/testcase-data/interrupts/intc2"; test_intmap0 = "/testcase-data/interrupts/intmap0"; test_intmap1 = "/testcase-data/interrupts/intmap1"; unittest_test_bus = "/testcase-data/overlay-node/test-bus"; unittest100 = "/testcase-data/overlay-node/test-bus/test-unittest100"; unittest101 = "/testcase-data/overlay-node/test-bus/test-unittest101"; unittest0 = "/testcase-data/overlay-node/test-bus/test-unittest0"; unittest1 = "/testcase-data/overlay-node/test-bus/test-unittest1"; unittest2 = "/testcase-data/overlay-node/test-bus/test-unittest2"; unittest3 = "/testcase-data/overlay-node/test-bus/test-unittest3"; unittest5 = "/testcase-data/overlay-node/test-bus/test-unittest5"; unittest6 = "/testcase-data/overlay-node/test-bus/test-unittest6"; unittest7 = "/testcase-data/overlay-node/test-bus/test-unittest7"; unittest8 = "/testcase-data/overlay-node/test-bus/test-unittest8"; unittest_i2c_test_bus = "/testcase-data/overlay-node/test-bus/i2c-test-bus"; }; __local_fixups__ { testcase-data { phandle-tests { provider4 { phandle-map = <0x00000008 0x00000018 0x00000024 0x0000003c 0x00000050 0x00000064>; }; consumer-a { phandle-list = <0x00000000 0x00000008 0x00000018 0x00000028 0x00000034 0x00000038>; phandle-list-bad-args = <0x00000000 0x0000000c>; }; consumer-b { phandle-list = <0x00000000 0x00000008 0x00000018 0x00000024 0x00000030 0x00000034>; phandle-list-bad-args = <0x00000000 0x0000000c>; }; }; interrupts { intmap0 { interrupt-map = <0x00000004 0x00000010 0x00000024 0x00000034>; }; intmap1 { interrupt-map = <0x0000000c>; }; interrupts0 { interrupt-parent = <0x00000000>; }; interrupts1 { interrupt-parent = <0x00000000>; }; interrupts-extended0 { interrupts-extended = <0x00000000 0x00000008 0x00000018 0x00000024 0x0000002c 0x00000034 0x0000003c>; }; }; testcase-device1 { interrupt-parent = <0x00000000>; }; testcase-device2 { interrupt-parent = <0x00000000>; }; }; }; };
On 1/19/21 2:05 AM, Viresh Kumar wrote: > On 18-01-21, 20:21, frowand.list@gmail.com wrote: >> From: Frank Rowand <frank.rowand@sony.com> >> >> These changes apply on top of the patches in: >> >> [PATCH] of: unittest: Statically apply overlays using fdtoverlay >> Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@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.kumar@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 { > + &overlay_base { > #address-cells = <1>; > #size-cells = <1>; > > @@ -89,5 +88,3 @@ retail_1: vending@50000 { > }; > > }; > -}; > - 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. > 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>; > + }; > >> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb >> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb > > This is how static_test.dtb looks now with fdtdump > < snip > -Frank
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.
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 ?
+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
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.
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 >
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
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 <viresh.kumar@linaro.org> --- Depends on: https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@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(+)