| Message ID | 696c137461be8ec4395c733c559c269bb4ad586e.1611124778.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | dt: build overlays | expand |
On Wed, Jan 20, 2021 at 12:36:47PM +0530, 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. > > 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.dtb. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile > index 009f4045c8e4..ece7dfd5cafa 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@ > > # suppress warnings about intentional errors > DTC_FLAGS_testcases += -Wno-interrupts_property > + > +# 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 \ > +# overlay_base.dtb \ > + > +apply_static_overlay := 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 $@ > + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ > + > +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay)) > + $(call if_changed,fdtoverlay) > + > +always-$(CONFIG_OF_OVERLAY) += static_test.dtb The fact that testcases.dts includes /plugin/ still seems completely wrong, if it's being used as the base tree. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Hi David, On 1/20/21 6:51 PM, David Gibson wrote: > On Wed, Jan 20, 2021 at 12:36:47PM +0530, 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. >> >> 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.dtb. >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile >> index 009f4045c8e4..ece7dfd5cafa 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@ >> >> # suppress warnings about intentional errors >> DTC_FLAGS_testcases += -Wno-interrupts_property >> + >> +# 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 \ >> +# overlay_base.dtb \ >> + >> +apply_static_overlay := 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 $@ >> + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >> + >> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay)) >> + $(call if_changed,fdtoverlay) >> + >> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb > > The fact that testcases.dts includes /plugin/ still seems completely > wrong, if it's being used as the base tree. > Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT. It is a convenient FDT to use because it provides the frame that the overlays require to be applied. It is fortunate that fdtoverlay does not reject the use of an FDT with overlay metadata as the base blob. If Viresh wants to test a more realistic data set then he could create a build rule that copies testcases.dts into (for example) testcases_base.dts, strip out the '/plugin/;" then compile that into testcases_base.dtb and use that for fdtoverlay. pseudo makefile rule for testcases_base.dts: sed -e 's|/plugin/;||' > testcases_base.dts add testcases_base.dtb to the list of objects to build change the rule for static_test.dtb to use testcases_base.dtb instead of testcases.dtb This is probably a good idea instead of depending on the leniency of fdtoverlay. -Frank
On 20-01-21, 23:14, Frank Rowand wrote: > It is a convenient FDT to use because it provides the frame that the overlays > require to be applied. It is fortunate that fdtoverlay does not reject the use > of an FDT with overlay metadata as the base blob. > This is probably a good idea instead of depending on the leniency of fdtoverlay. I believe fdtoverlay allows that intentionally, that would be required for the cases where we have a hierarchy of extension boards or overlays. A platform can have a base dtb (with /plugin/;), then we can have an overlay (1) for an extension board (with /plugin/;) and then an overlay (2) for an extension board for the previous extension board. In such a case overlay-(2) can't be applied directly to the base dtb as it may not find all the nodes it is trying to update. And so overlay-(2) needs to be applied to overlay-(1) and then the output of this can be applied to the base dtb. This is very similar to what I tried with the intermediate.dtb earlier. -- viresh
Hi Viresh, On 1/20/21 11:14 PM, Frank Rowand wrote: > Hi David, > > On 1/20/21 6:51 PM, David Gibson wrote: >> On Wed, Jan 20, 2021 at 12:36:47PM +0530, 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. >>> >>> 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.dtb. >>> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >>> drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> >>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile >>> index 009f4045c8e4..ece7dfd5cafa 100644 >>> --- a/drivers/of/unittest-data/Makefile >>> +++ b/drivers/of/unittest-data/Makefile >>> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@ >>> >>> # suppress warnings about intentional errors >>> DTC_FLAGS_testcases += -Wno-interrupts_property >>> + >>> +# 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 \ >>> +# overlay_base.dtb \ >>> + >>> +apply_static_overlay := 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 $@ >>> + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >>> + >>> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay)) >>> + $(call if_changed,fdtoverlay) >>> + >>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb >> >> The fact that testcases.dts includes /plugin/ still seems completely >> wrong, if it's being used as the base tree. >> > > Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT. > It is a convenient FDT to use because it provides the frame that the overlays > require to be applied. It is fortunate that fdtoverlay does not reject the use > of an FDT with overlay metadata as the base blob. > > If Viresh wants to test a more realistic data set then he could create a build > rule that copies testcases.dts into (for example) testcases_base.dts, strip > out the '/plugin/;" then compile that into testcases_base.dtb and use that for > fdtoverlay. > > pseudo makefile rule for testcases_base.dts: > sed -e 's|/plugin/;||' > testcases_base.dts > > add testcases_base.dtb to the list of objects to build > > change the rule for static_test.dtb to use testcases_base.dtb instead of > testcases.dtb > > This is probably a good idea instead of depending on the leniency of fdtoverlay. It should be possible to apply this same concept to copying overlay_base.dts to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts and using an additional rule to use fdtoverlay to apply overlay.dtb on top of overlay_base_base.dtb. Yes, overlay_base_base is a terrible name. Just used to illustrate the point. I tried this by hand and am failing miserably. But I am not using the proper environment (just a quick hack to see if the method might work). So I would have to set things up properly to really test this. If this does work, it would remove my objections to you trying to transform the existing unittest .dts test data files (because you would not have to actually modify the existing .dts files). > > -Frank >
Hi Frank, On 20-01-21, 23:34, Frank Rowand wrote: > It should be possible to apply this same concept to copying overlay_base.dts > to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts > and using an additional rule to use fdtoverlay to apply overlay.dtb on top > of overlay_base_base.dtb. Are you suggesting to then merge this with testcases.dtb to get static_test.dtb or keep two output files (static_test.dtb from testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb and overlay.dtb) ? Asking because as I mentioned earlier, overlay_base.dtb doesn't have __overlay__ property for its nodes and we can't apply that to testcases.dtb using fdtoverlay. > Yes, overlay_base_base is a terrible name. Just used to illustrate the point. > > I tried this by hand and am failing miserably. But I am not using the proper > environment (just a quick hack to see if the method might work). So I would > have to set things up properly to really test this. > > If this does work, it would remove my objections to you trying to transform > the existing unittest .dts test data files (because you would not have to > actually modify the existing .dts files). -- viresh
On 1/20/21 11:34 PM, Viresh Kumar wrote: > On 20-01-21, 23:14, Frank Rowand wrote: >> It is a convenient FDT to use because it provides the frame that the overlays >> require to be applied. It is fortunate that fdtoverlay does not reject the use >> of an FDT with overlay metadata as the base blob. > >> This is probably a good idea instead of depending on the leniency of fdtoverlay. > > I believe fdtoverlay allows that intentionally, that would be required > for the cases where we have a hierarchy of extension boards or > overlays. > > A platform can have a base dtb (with /plugin/;), then we can have an > overlay (1) for an extension board (with /plugin/;) and then an > overlay (2) for an extension board for the previous extension board. > > In such a case overlay-(2) can't be applied directly to the base dtb > as it may not find all the nodes it is trying to update. And so > overlay-(2) needs to be applied to overlay-(1) and then the output of > this can be applied to the base dtb. I have only the most surface knowledge of fdtoverlay, mostly from "fdtoverlay --help", but you can apply multiple overlays with a single invocation of fdtoverlay. My _assumption_ was that the overlays would be applied in order, and after any given overlay was applied, subsequent overlays could reference the previously applied overlay. Is my assumption incorrect? > > This is very similar to what I tried with the intermediate.dtb > earlier. >
On 20-01-21, 23:45, Frank Rowand wrote: > I have only the most surface knowledge of fdtoverlay, mostly from > "fdtoverlay --help", but you can apply multiple overlays with a > single invocation of fdtoverlay. My _assumption_ was that the > overlays would be applied in order, and after any given overlay > was applied, subsequent overlays could reference the previously > applied overlay. > > Is my assumption incorrect? I think yes, if everything is in order then it should work just fine. I was only suggesting that fdtoverlay accepting the base overlay with /plugin/; may well be a requirement and so intentionally done. -- viresh
On 1/20/21 11:43 PM, Viresh Kumar wrote: > Hi Frank, > > On 20-01-21, 23:34, Frank Rowand wrote: >> It should be possible to apply this same concept to copying overlay_base.dts >> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts >> and using an additional rule to use fdtoverlay to apply overlay.dtb on top >> of overlay_base_base.dtb. > > Are you suggesting to then merge this with testcases.dtb to get > static_test.dtb no > or keep two output files (static_test.dtb from > testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb > and overlay.dtb) ? yes, but using the modified versions ("/plugin/;" removed) of testcases.dtb and overlay_base.dtb. > > Asking because as I mentioned earlier, overlay_base.dtb doesn't have > __overlay__ property for its nodes and we can't apply that to > testcases.dtb using fdtoverlay. Correct. I apologize in advance if I get confused in these threads or cause confusion. I find myself going in circles and losing track of how things fit together as I move through the various pieces of unittest. -Frank > >> Yes, overlay_base_base is a terrible name. Just used to illustrate the point. >> >> I tried this by hand and am failing miserably. But I am not using the proper >> environment (just a quick hack to see if the method might work). So I would >> have to set things up properly to really test this. >> >> If this does work, it would remove my objections to you trying to transform >> the existing unittest .dts test data files (because you would not have to >> actually modify the existing .dts files). >
On 20-01-21, 23:55, Frank Rowand wrote: > yes, but using the modified versions ("/plugin/;" removed) of > testcases.dtb and overlay_base.dtb. Okay, that would work fine I guess. I will try to implement this in the new version. > I apologize in advance if I get confused in these threads or cause confusion. > I find myself going in circles and losing track of how things fit together as > I move through the various pieces of unittest. Me too :) Today is the first time where we have some overlap in our work hours (probably you working late :)), and we are able to get this sorted out quickly enough. Thanks for your feedback Frank. -- viresh
On Thu, Jan 21, 2021 at 11:04:26AM +0530, Viresh Kumar wrote: > On 20-01-21, 23:14, Frank Rowand wrote: > > It is a convenient FDT to use because it provides the frame that the overlays > > require to be applied. It is fortunate that fdtoverlay does not reject the use > > of an FDT with overlay metadata as the base blob. > > > This is probably a good idea instead of depending on the leniency of fdtoverlay. > > I believe fdtoverlay allows that intentionally, that would be required > for the cases where we have a hierarchy of extension boards or > overlays. Um.. no. > A platform can have a base dtb (with /plugin/;), then we can have an > overlay (1) for an extension board (with /plugin/;) and then an > overlay (2) for an extension board for the previous extension board. > > In such a case overlay-(2) can't be applied directly to the base dtb > as it may not find all the nodes it is trying to update. And so > overlay-(2) needs to be applied to overlay-(1) and then the output of > this can be applied to the base dtb. No, this is the wrong way around. The expected operation here is that you apply overlay (1) to the base tree, giving you, say, output1.dtb. output1.dtb is (effectively) a base tree itself, to which you can then apply overlay-(2). What you're talking about is "merging" overlays: combingin overlay (1) and (2) into overlay-(X) which would have the same effect applied to base.dtb as (1) and (2) applied in sequence. Merging overlays is something that could make sense, but fdtoverlay will not do it at present. > This is very similar to what I tried with the intermediate.dtb > earlier. > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Jan 21, 2021 at 11:20:40AM +0530, Viresh Kumar wrote: > On 20-01-21, 23:45, Frank Rowand wrote: > > I have only the most surface knowledge of fdtoverlay, mostly from > > "fdtoverlay --help", but you can apply multiple overlays with a > > single invocation of fdtoverlay. My _assumption_ was that the > > overlays would be applied in order, and after any given overlay > > was applied, subsequent overlays could reference the previously > > applied overlay. > > > > Is my assumption incorrect? > > I think yes, if everything is in order then it should work just fine. > > I was only suggesting that fdtoverlay accepting the base overlay with > /plugin/; may well be a requirement and so intentionally done. No. It's simply the result of the fact that a dtbo is still a dtb. So, you can still apply an overlay to it. However, a dtbo is a weirdly structured dtb, so applying an overlay to it is very unlikely to give you something useful. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Wed, Jan 20, 2021 at 11:55:08PM -0600, Frank Rowand wrote: > On 1/20/21 11:43 PM, Viresh Kumar wrote: > > Hi Frank, > > > > On 20-01-21, 23:34, Frank Rowand wrote: > >> It should be possible to apply this same concept to copying overlay_base.dts > >> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts > >> and using an additional rule to use fdtoverlay to apply overlay.dtb on top > >> of overlay_base_base.dtb. > > > > Are you suggesting to then merge this with testcases.dtb to get > > static_test.dtb > > no > > > or keep two output files (static_test.dtb from > > testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb > > and overlay.dtb) ? > > yes, but using the modified versions ("/plugin/;" removed) of > testcases.dtb and overlay_base.dtb. I really don't understand why you want /plugin/ in *any* version of testcases.dtb. > > > > Asking because as I mentioned earlier, overlay_base.dtb doesn't have > > __overlay__ property for its nodes and we can't apply that to > > testcases.dtb using fdtoverlay. > > Correct. > > I apologize in advance if I get confused in these threads or cause confusion. > I find myself going in circles and losing track of how things fit together as > I move through the various pieces of unittest. > > -Frank > > > > >> Yes, overlay_base_base is a terrible name. Just used to illustrate the point. > >> > >> I tried this by hand and am failing miserably. But I am not using the proper > >> environment (just a quick hack to see if the method might work). So I would > >> have to set things up properly to really test this. > >> > >> If this does work, it would remove my objections to you trying to transform > >> the existing unittest .dts test data files (because you would not have to > >> actually modify the existing .dts files). > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 21-01-21, 17:34, David Gibson wrote: > No, this is the wrong way around. The expected operation here is that > you apply overlay (1) to the base tree, giving you, say, output1.dtb. > output1.dtb is (effectively) a base tree itself, to which you can then > apply overlay-(2). Thanks for the confirmation about this. > Merging overlays is > something that could make sense, but fdtoverlay will not do it at > present. FWIW, I think it works fine right now even if it not intentional. I did inspect the output dtb (made by merging two overlays) using fdtdump and it looked okay. But yeah, I understand that we shouldn't do it. -- viresh
On 1/20/21 11:58 PM, Viresh Kumar wrote: > On 20-01-21, 23:55, Frank Rowand wrote: >> yes, but using the modified versions ("/plugin/;" removed) of >> testcases.dtb and overlay_base.dtb. > > Okay, that would work fine I guess. I will try to implement this in > the new version. > >> I apologize in advance if I get confused in these threads or cause confusion. >> I find myself going in circles and losing track of how things fit together as >> I move through the various pieces of unittest. > > Me too :) > > Today is the first time where we have some overlap in our work hours > (probably you working late :)), and we are able to get this sorted out > quickly enough. Working quite late. I swear I stopped working 3 hours ago and that was already late. I reacted quite negatively to the attempt to restructure the unittest .dts file in the original patch. Now after walking around the problem space a bit, and reviewing the ugly things that unittest.c does, and coming up with the approach of sed to copy and modify the two base .dts files, I think I finally have my head wrapped around an easier and cleaner approach than sed. I'll describe it for just one of the two base files, but the same concept would apply to the other. Don't take my file names as good suggestions, I am avoiding using the brain power to come up with good names at the moment. 1) rename overlay_base.dts to overlay_base.dtsi 2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi 3) create a new overlay_base.dts: // SPDX-License-Identifier: GPL-2.0 /dts-v1/; /plugin/; #include overlay_base_dtsi 4) create a new file overlay_base_static.dts: // SPDX-License-Identifier: GPL-2.0 /dts-v1/; #include overlay_base_dtsi 5) then use overlay_base_static.dtb as the base blob for ftdoverlay applying overlay.dtb Untested, hand typed, hopefully no typos. -Frank > > Thanks for your feedback Frank. >
On Thu, Jan 21, 2021 at 12:27:28PM +0530, Viresh Kumar wrote: > On 21-01-21, 17:34, David Gibson wrote: > > No, this is the wrong way around. The expected operation here is that > > you apply overlay (1) to the base tree, giving you, say, output1.dtb. > > output1.dtb is (effectively) a base tree itself, to which you can then > > apply overlay-(2). > > Thanks for the confirmation about this. > > > Merging overlays is > > something that could make sense, but fdtoverlay will not do it at > > present. > > FWIW, I think it works fine right now even if it not intentional. No, it definitely will not work in general. It might kinda work in a few trivial cases, but it absolutely will not do the neccessary handling in some cases. > I > did inspect the output dtb (made by merging two overlays) using > fdtdump and it looked okay. Ok.. but if you're using these bizarre messed up "dtbs" that this test code seems to be, I don't really trust that tells you much. > But yeah, I understand that we shouldn't > do it. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Jan 21, 2021 at 01:04:46AM -0600, Frank Rowand wrote: > On 1/20/21 11:58 PM, Viresh Kumar wrote: > > On 20-01-21, 23:55, Frank Rowand wrote: > >> yes, but using the modified versions ("/plugin/;" removed) of > >> testcases.dtb and overlay_base.dtb. > > > > Okay, that would work fine I guess. I will try to implement this in > > the new version. > > > >> I apologize in advance if I get confused in these threads or cause confusion. > >> I find myself going in circles and losing track of how things fit together as > >> I move through the various pieces of unittest. > > > > Me too :) > > > > Today is the first time where we have some overlap in our work hours > > (probably you working late :)), and we are able to get this sorted out > > quickly enough. > > Working quite late. I swear I stopped working 3 hours ago and that was > already late. > > I reacted quite negatively to the attempt to restructure the unittest > .dts file in the original patch. Now after walking around the problem > space a bit, and reviewing the ugly things that unittest.c does, and > coming up with the approach of sed to copy and modify the two base > .dts files, I think I finally have my head wrapped around an easier > and cleaner approach than sed. > > I'll describe it for just one of the two base files, but the same > concept would apply to the other. Don't take my file names as > good suggestions, I am avoiding using the brain power to come up > with good names at the moment. > > 1) rename overlay_base.dts to overlay_base.dtsi > > 2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi > > 3) create a new overlay_base.dts: > // SPDX-License-Identifier: GPL-2.0 > /dts-v1/; > /plugin/; > #include overlay_base_dtsi "overlay_base" is a terrible name - it's not clear if it's supposed to be a base tree or an overlay. I'm still not seeing any reasonable use for the plugin version either, fwiw. > > 4) create a new file overlay_base_static.dts: > // SPDX-License-Identifier: GPL-2.0 > /dts-v1/; > #include overlay_base_dtsi > > 5) then use overlay_base_static.dtb as the base blob for ftdoverlay > applying overlay.dtb > > Untested, hand typed, hopefully no typos. > > -Frank > > > > > Thanks for your feedback Frank. > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 22-01-21, 10:39, David Gibson wrote: > No, it definitely will not work in general. It might kinda work in a > few trivial cases, but it absolutely will not do the neccessary > handling in some cases. > > > I > > did inspect the output dtb (made by merging two overlays) using > > fdtdump and it looked okay. > > Ok.. but if you're using these bizarre messed up "dtbs" that this test > code seems to be, I don't really trust that tells you much. I only looked if the changes from the second overlay were present in the merge and they were. And so I assumed that it must have worked. What about checking the base dtb for /plugin/; in fdtoverlay and fail the whole thing in case it is present ? I think it is possible for people to get confused otherwise, like I did. -- viresh
On Fri, Jan 22, 2021 at 08:40:49AM +0530, Viresh Kumar wrote: > On 22-01-21, 10:39, David Gibson wrote: > > No, it definitely will not work in general. It might kinda work in a > > few trivial cases, but it absolutely will not do the neccessary > > handling in some cases. > > > > > I > > > did inspect the output dtb (made by merging two overlays) using > > > fdtdump and it looked okay. > > > > Ok.. but if you're using these bizarre messed up "dtbs" that this test > > code seems to be, I don't really trust that tells you much. > > I only looked if the changes from the second overlay were present in > the merge and they were. And so I assumed that it must have worked. > > What about checking the base dtb for /plugin/; in fdtoverlay and fail > the whole thing in case it is present ? I think it is possible for > people to get confused otherwise, like I did. /plugin/ doesn't exist in the dtb, only in the dts. From the dtb encoding point of view, there's no difference between a dtb and a dtbo, a dtbo is just a dtb that follows some conventions for its content. If we were doing this from scratch, it would be better for dtbos to have a different magic number from regular dtbs. I think I actually suggested that sometime in the past, but by the time that came up, dtbos were already in pretty widespread use with the existing format. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..ece7dfd5cafa 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# 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 \ +# overlay_base.dtb \ + +apply_static_overlay := 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 $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ + +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay)) + $(call if_changed,fdtoverlay) + +always-$(CONFIG_OF_OVERLAY) += static_test.dtb
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. 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.dtb. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) -- 2.25.0.rc1.19.g042ed3e048af