diff mbox series

[V7,6/6] of: unittest: Statically apply overlays using fdtoverlay

Message ID 3683a542d4141cfcf9c2524a40a9ee75b657c1c2.1611904394.git.viresh.kumar@linaro.org
State Superseded
Headers show
Series dt: build overlays | expand

Commit Message

Viresh Kumar Jan. 29, 2021, 7:24 a.m. UTC 
Now that fdtoverlay is part of the kernel build, start using it to test
the unitest overlays we have by applying them statically. Create two new
base files static_base_1.dts and static_base_2.dts which includes other
.dtsi files.

Some unittest overlays deliberately contain errors that unittest checks
for. These overlays will cause fdtoverlay to fail, and are thus not
included for static builds.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/of/unittest-data/Makefile          | 56 ++++++++++++++++++++++
 drivers/of/unittest-data/static_base_1.dts |  4 ++
 drivers/of/unittest-data/static_base_2.dts |  4 ++
 3 files changed, 64 insertions(+)
 create mode 100644 drivers/of/unittest-data/static_base_1.dts
 create mode 100644 drivers/of/unittest-data/static_base_2.dts

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Rob Herring Feb. 4, 2021, 1:54 a.m. UTC | #1 
On Fri, Jan 29, 2021 at 12:54:10PM +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. Create two new

> base files static_base_1.dts and static_base_2.dts which includes other

> .dtsi files.

> 

> Some unittest overlays deliberately contain errors that unittest checks

> for. These overlays will cause fdtoverlay to fail, and are thus not

> included for static builds.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  drivers/of/unittest-data/Makefile          | 56 ++++++++++++++++++++++

>  drivers/of/unittest-data/static_base_1.dts |  4 ++

>  drivers/of/unittest-data/static_base_2.dts |  4 ++

>  3 files changed, 64 insertions(+)

>  create mode 100644 drivers/of/unittest-data/static_base_1.dts

>  create mode 100644 drivers/of/unittest-data/static_base_2.dts


The first 4 patches look good to me, no need to resend those if you 
don't want to.

> 

> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile

> index 009f4045c8e4..fc286224b2d1 100644

> --- a/drivers/of/unittest-data/Makefile

> +++ b/drivers/of/unittest-data/Makefile

> @@ -34,7 +34,63 @@ DTC_FLAGS_overlay += -@

>  DTC_FLAGS_overlay_bad_phandle += -@

>  DTC_FLAGS_overlay_bad_symbol += -@

>  DTC_FLAGS_overlay_base += -@

> +DTC_FLAGS_static_base_1 += -@

> +DTC_FLAGS_static_base_2 += -@

>  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 static_base_1.dtb and static_base_2.dtb to

> +# create static_test_1.dtb and static_test_2.dtb.  If fdtoverlay detects an

> +# error than the kernel build will fail.  static_test_1.dtb and

> +# static_test_2.dtb are 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_bad_add_dup_node.dtb \

> +#			  overlay_bad_add_dup_prop.dtb \

> +#			  overlay_bad_phandle.dtb \

> +#			  overlay_bad_symbol.dtb \

> +

> +apply_static_overlay_1 := 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


As these are overlays, .dtbo

> +

> +apply_static_overlay_2 := overlay.dtb

> +

> +quiet_cmd_fdtoverlay = FDTOVERLAY $@

> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^

> +

> +$(obj)/static_test_1.dtb: $(obj)/static_base_1.dtb $(addprefix $(obj)/,$(apply_static_overlay_1))

> +	$(call if_changed,fdtoverlay)

> +

> +$(obj)/static_test_2.dtb: $(obj)/static_base_2.dtb $(addprefix $(obj)/,$(apply_static_overlay_2))

> +	$(call if_changed,fdtoverlay)

> +

> +always-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb


We can't be leaving all this for ordinary folks to copy-n-paste in 
every directory with overlays. It needs to be simple for users like 
multi-file modules:

test1-dtbs := static_base_1.dtb $(apply_static_overlay_1)
test2-dtbs := static_base_2.dtb $(apply_static_overlay_2)

dtb-$(CONFIG_OF_OVERLAY) += test1.dtb test2.dtb


I've gotten something working with the patch below. Hopefully, Masahiro 
has some comments on it. I couldn't get multiple '-dtbs' to work without 
the '.SECONDEXPANSION'.

Rob

8<-------------------------------------------------------------------
From 3f3f1e478d0cc512050c70eda2e9e6f577bc3107 Mon Sep 17 00:00:00 2001
From: Rob Herring <robh@kernel.org>

Date: Wed, 3 Feb 2021 19:22:47 -0600
Subject: [PATCH] rework dtb overlay building

Signed-off-by: Rob Herring <robh@kernel.org>

---
 drivers/of/unittest-data/Makefile | 54 ++++++++++++++-----------------
 scripts/Makefile.lib              | 12 +++++++
 2 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index fc286224b2d1..749d04ee6dc3 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -60,37 +60,31 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
 #			  overlay_bad_phandle.dtb \
 #			  overlay_bad_symbol.dtb \
 
-apply_static_overlay_1 := 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
+apply_static_overlay_1 := overlay_0.dtbo \
+			  overlay_1.dtbo \
+			  overlay_2.dtbo \
+			  overlay_3.dtbo \
+			  overlay_4.dtbo \
+			  overlay_5.dtbo \
+			  overlay_6.dtbo \
+			  overlay_7.dtbo \
+			  overlay_8.dtbo \
+			  overlay_9.dtbo \
+			  overlay_10.dtbo \
+			  overlay_11.dtbo \
+			  overlay_12.dtbo \
+			  overlay_13.dtbo \
+			  overlay_15.dtbo \
+			  overlay_gpio_01.dtbo \
+			  overlay_gpio_02a.dtbo \
+			  overlay_gpio_02b.dtbo \
+			  overlay_gpio_03.dtbo \
+			  overlay_gpio_04a.dtbo \
+			  overlay_gpio_04b.dtbo
 
 apply_static_overlay_2 := overlay.dtb
 
-quiet_cmd_fdtoverlay = FDTOVERLAY $@
-      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
+test1-dtbs := static_base_1.dtb $(apply_static_overlay_1)
+test2-dtbs := static_base_2.dtb $(apply_static_overlay_2)
 
-$(obj)/static_test_1.dtb: $(obj)/static_base_1.dtb $(addprefix $(obj)/,$(apply_static_overlay_1))
-	$(call if_changed,fdtoverlay)
-
-$(obj)/static_test_2.dtb: $(obj)/static_base_2.dtb $(addprefix $(obj)/,$(apply_static_overlay_2))
-	$(call if_changed,fdtoverlay)
-
-always-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb
+dtb-$(CONFIG_OF_OVERLAY) += test1.dtb test2.dtb
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index b00855b247e0..886d2e6c58f0 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -66,6 +66,9 @@ multi-used   := $(multi-used-y) $(multi-used-m)
 real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))
 real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))
 
+real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))
+dtb-y += $(real-dtb-y)
+
 always-y += $(always-m)
 
 # hostprogs-always-y += foo
@@ -332,6 +335,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
 $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
 	$(call if_changed_dep,dtc)
 
+
+quiet_cmd_fdtoverlay = DTOVL   $@
+      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs)
+
+.SECONDEXPANSION:
+
+$(obj)/%.dtb: $$(addprefix $$(obj)/,$$(%-dtbs)) FORCE
+	$(call if_changed,fdtoverlay)
+
 DT_CHECKER ?= dt-validate
 DT_BINDING_DIR := Documentation/devicetree/bindings
 # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile
-- 
2.27.0
Viresh Kumar Feb. 4, 2021, 7:41 a.m. UTC | #2 
On 03-02-21, 19:54, Rob Herring wrote:
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib

> index b00855b247e0..886d2e6c58f0 100644

> --- a/scripts/Makefile.lib

> +++ b/scripts/Makefile.lib

> @@ -66,6 +66,9 @@ multi-used   := $(multi-used-y) $(multi-used-m)

>  real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))

>  real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))

>  

> +real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))

> +dtb-y += $(real-dtb-y)

> +

>  always-y += $(always-m)

>  

>  # hostprogs-always-y += foo

> @@ -332,6 +335,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE

>  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE

>  	$(call if_changed_dep,dtc)

>  

> +

> +quiet_cmd_fdtoverlay = DTOVL   $@

> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs)

> +

> +.SECONDEXPANSION:

> +

> +$(obj)/%.dtb: $$(addprefix $$(obj)/,$$(%-dtbs)) FORCE

> +	$(call if_changed,fdtoverlay)

> +

>  DT_CHECKER ?= dt-validate

>  DT_BINDING_DIR := Documentation/devicetree/bindings

>  # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile


Thanks, this simplifies it greatly for platform guys.

-- 
viresh
Viresh Kumar Feb. 8, 2021, 11:18 a.m. UTC | #3 
On 03-02-21, 19:54, Rob Herring wrote:
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib

> index b00855b247e0..886d2e6c58f0 100644

> --- a/scripts/Makefile.lib

> +++ b/scripts/Makefile.lib

> @@ -66,6 +66,9 @@ multi-used   := $(multi-used-y) $(multi-used-m)

>  real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))

>  real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))

>  

> +real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))


Sorry for my lack of knowledge, but what does (m:.dtb=-dtbs) do
exactly ?

> +dtb-y += $(real-dtb-y)

> +

>  always-y += $(always-m)

>  

>  # hostprogs-always-y += foo

> @@ -332,6 +335,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE

>  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE

>  	$(call if_changed_dep,dtc)

>  

> +

> +quiet_cmd_fdtoverlay = DTOVL   $@

> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs)

> +

> +.SECONDEXPANSION:

> +

> +$(obj)/%.dtb: $$(addprefix $$(obj)/,$$(%-dtbs)) FORCE

> +	$(call if_changed,fdtoverlay)

> +

>  DT_CHECKER ?= dt-validate

>  DT_BINDING_DIR := Documentation/devicetree/bindings

>  # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile


I tried to test a dtbo from arch/ code like this:

diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile
index f4d68caeba83..0ee9d7dc8e07 100644
--- a/arch/arm64/boot/dts/hisilicon/Makefile
+++ b/arch/arm64/boot/dts/hisilicon/Makefile
@@ -6,3 +6,8 @@ dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip05-d02.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip06-d03.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip07-d05.dtb
+
+DTC_FLAGS_hi3660-hikey960 += -@
+
+test1-dtbs := hi3660-hikey960.dtb hi3660-hikey960-overlay.dtbo
+dtb-y += test1.dtb
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
new file mode 100644
index 000000000000..1235a911caae
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+&dwmmc2 {
+       #address-cells = <0x1>;
+       #size-cells = <0x0>;
+
+       wlcore2: wlcore@5 {
+               compatible = "ti,wl1837";
+               reg = <2>;
+               interrupt-parent = <&gpio22>;
+               interrupts = <3 1>;
+               test = <1>;
+       };
+};

Even after your patch there are some issues I am facing:

1. dtbs_check doesn't test hi3660-hikey960-overlay.dts. I also tried
   to add it to dtbo-y +=, but that didn't do anything as well.

I expected this to work as we have this in scripts/Makefile.lib:

 ifneq ($(CHECK_DTBS),)
 extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
+extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))
 extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
+extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))
 endif


2. This fails dtbs_check as it tries to run it for the source file of
   test1.dtb

$ make ARCH=arm64 O=../barm64/ -j8 CROSS_COMPILE=aarch64-linux-gnu- dtbs_check
make[1]: Entering directory '/mnt/ssd/all/work/repos/devel/barm64'
make[3]: *** No rule to make target 'arch/arm64/boot/dts/hisilicon/test1.dt.yaml', needed by '__build'.  Stop.
/mnt/ssd/all/work/repos/devel/linux/scripts/Makefile.build:496: recipe for target 'arch/arm64/boot/dts/hisilicon' failed
make[2]: *** [arch/arm64/boot/dts/hisilicon] Error 2
make[2]: *** Waiting for unfinished jobs....
/mnt/ssd/all/work/repos/devel/linux/Makefile:1345: recipe for target 'dtbs' failed
make[1]: *** [dtbs] Error 2
make[1]: Leaving directory '/mnt/ssd/all/work/repos/devel/barm64'
Makefile:185: recipe for target '__sub-make' failed
make: *** [__sub-make] Error 2

I am not sure how to fix this.

-- 
viresh
Rob Herring Feb. 8, 2021, 2:21 p.m. UTC | #4 
On Mon, Feb 8, 2021 at 5:18 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> On 03-02-21, 19:54, Rob Herring wrote:

> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib

> > index b00855b247e0..886d2e6c58f0 100644

> > --- a/scripts/Makefile.lib

> > +++ b/scripts/Makefile.lib

> > @@ -66,6 +66,9 @@ multi-used   := $(multi-used-y) $(multi-used-m)

> >  real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))

> >  real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))

> >

> > +real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))

>

> Sorry for my lack of knowledge, but what does (m:.dtb=-dtbs) do

> exactly ?


In string 'm' replace '.dtb' with '-dtbs'. Then we get the value of
that variable.

>

> > +dtb-y += $(real-dtb-y)

> > +

> >  always-y += $(always-m)

> >

> >  # hostprogs-always-y += foo

> > @@ -332,6 +335,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE

> >  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE

> >       $(call if_changed_dep,dtc)

> >

> > +

> > +quiet_cmd_fdtoverlay = DTOVL   $@

> > +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs)

> > +

> > +.SECONDEXPANSION:

> > +

> > +$(obj)/%.dtb: $$(addprefix $$(obj)/,$$(%-dtbs)) FORCE

> > +     $(call if_changed,fdtoverlay)

> > +

> >  DT_CHECKER ?= dt-validate

> >  DT_BINDING_DIR := Documentation/devicetree/bindings

> >  # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile

>

> I tried to test a dtbo from arch/ code like this:

>

> diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile

> index f4d68caeba83..0ee9d7dc8e07 100644

> --- a/arch/arm64/boot/dts/hisilicon/Makefile

> +++ b/arch/arm64/boot/dts/hisilicon/Makefile

> @@ -6,3 +6,8 @@ dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb

>  dtb-$(CONFIG_ARCH_HISI) += hip05-d02.dtb

>  dtb-$(CONFIG_ARCH_HISI) += hip06-d03.dtb

>  dtb-$(CONFIG_ARCH_HISI) += hip07-d05.dtb

> +

> +DTC_FLAGS_hi3660-hikey960 += -@

> +

> +test1-dtbs := hi3660-hikey960.dtb hi3660-hikey960-overlay.dtbo

> +dtb-y += test1.dtb

> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts

> new file mode 100644

> index 000000000000..1235a911caae

> --- /dev/null

> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts

> @@ -0,0 +1,16 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/dts-v1/;

> +/plugin/;

> +

> +&dwmmc2 {

> +       #address-cells = <0x1>;

> +       #size-cells = <0x0>;

> +

> +       wlcore2: wlcore@5 {

> +               compatible = "ti,wl1837";

> +               reg = <2>;

> +               interrupt-parent = <&gpio22>;

> +               interrupts = <3 1>;

> +               test = <1>;

> +       };

> +};

>

> Even after your patch there are some issues I am facing:

>

> 1. dtbs_check doesn't test hi3660-hikey960-overlay.dts. I also tried

>    to add it to dtbo-y +=, but that didn't do anything as well.

>

> I expected this to work as we have this in scripts/Makefile.lib:

>

>  ifneq ($(CHECK_DTBS),)

>  extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))

> +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))

>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))

> +extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))

>  endif


I'll have to try that out. I think that should work.

> 2. This fails dtbs_check as it tries to run it for the source file of

>    test1.dtb

>

> $ make ARCH=arm64 O=../barm64/ -j8 CROSS_COMPILE=aarch64-linux-gnu- dtbs_check

> make[1]: Entering directory '/mnt/ssd/all/work/repos/devel/barm64'

> make[3]: *** No rule to make target 'arch/arm64/boot/dts/hisilicon/test1.dt.yaml', needed by '__build'.  Stop.

> /mnt/ssd/all/work/repos/devel/linux/scripts/Makefile.build:496: recipe for target 'arch/arm64/boot/dts/hisilicon' failed

> make[2]: *** [arch/arm64/boot/dts/hisilicon] Error 2

> make[2]: *** Waiting for unfinished jobs....

> /mnt/ssd/all/work/repos/devel/linux/Makefile:1345: recipe for target 'dtbs' failed

> make[1]: *** [dtbs] Error 2

> make[1]: Leaving directory '/mnt/ssd/all/work/repos/devel/barm64'

> Makefile:185: recipe for target '__sub-make' failed

> make: *** [__sub-make] Error 2

>

> I am not sure how to fix this.


Even if we fixed the make rules, it's not going to work with
validation. There's some information from source files that we
maintain in yaml output, but is lost in dtb output. For example, the
sizes of /bits/ syntax are maintained. For now, I think we'll want to
just validate base and overlays separately. We may need to turn off
checks in overlays for required properties as they may be incomplete.
We already do that on disabled nodes.

Rob
Viresh Kumar Feb. 9, 2021, 4:04 a.m. UTC | #5 
On 08-02-21, 08:21, Rob Herring wrote:
> In string 'm' replace '.dtb' with '-dtbs'. Then we get the value of

> that variable.


Ah, thanks. I was able to get everything to work as expected now :)

> >  ifneq ($(CHECK_DTBS),)

> >  extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))

> > +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))

> >  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))

> > +extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))

> >  endif

> 

> I'll have to try that out. I think that should work.


It works with your patch itself, just that it was done after the
failure and so wasn't happening.

> > 2. This fails dtbs_check as it tries to run it for the source file of

> >    test1.dtb

> >

> > $ make ARCH=arm64 O=../barm64/ -j8 CROSS_COMPILE=aarch64-linux-gnu- dtbs_check

> > make[1]: Entering directory '/mnt/ssd/all/work/repos/devel/barm64'

> > make[3]: *** No rule to make target 'arch/arm64/boot/dts/hisilicon/test1.dt.yaml', needed by '__build'.  Stop.

> > /mnt/ssd/all/work/repos/devel/linux/scripts/Makefile.build:496: recipe for target 'arch/arm64/boot/dts/hisilicon' failed

> > make[2]: *** [arch/arm64/boot/dts/hisilicon] Error 2

> > make[2]: *** Waiting for unfinished jobs....

> > /mnt/ssd/all/work/repos/devel/linux/Makefile:1345: recipe for target 'dtbs' failed

> > make[1]: *** [dtbs] Error 2

> > make[1]: Leaving directory '/mnt/ssd/all/work/repos/devel/barm64'

> > Makefile:185: recipe for target '__sub-make' failed

> > make: *** [__sub-make] Error 2

> >

> > I am not sure how to fix this.

> 

> Even if we fixed the make rules, it's not going to work with

> validation. There's some information from source files that we

> maintain in yaml output, but is lost in dtb output. For example, the

> sizes of /bits/ syntax are maintained. For now, I think we'll want to

> just validate base and overlays separately. We may need to turn off

> checks in overlays for required properties as they may be incomplete.

> We already do that on disabled nodes.


I did this instead and it made everything work, we don't try dt.yaml
for the test1.dtb file anymore, is this acceptable ?

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 886d2e6c58f0..b86ff1b3de14 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -66,7 +66,7 @@ multi-used   := $(multi-used-y) $(multi-used-m)
 real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))
 real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m)))
 
-real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))
+real-dtb-y := $(foreach m,$(overlay-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))
 dtb-y += $(real-dtb-y)
 
 always-y += $(always-m)

diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile
index f4d68caeba83..69ca27014e89 100644
--- a/arch/arm64/boot/dts/hisilicon/Makefile
+++ b/arch/arm64/boot/dts/hisilicon/Makefile
@@ -6,3 +6,8 @@ dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip05-d02.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip06-d03.dtb
 dtb-$(CONFIG_ARCH_HISI) += hip07-d05.dtb
+
+DTC_FLAGS_hi3660-hikey960 += -@
+
+test1-dtbs := hi3660-hikey960.dtb hi3660-hikey960-overlay.dtbo
+overlay-y += test1.dtb
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
new file mode 100644
index 000000000000..1235a911caae
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+&dwmmc2 {
+       #address-cells = <0x1>;
+       #size-cells = <0x0>;
+
+       wlcore2: wlcore@5 {
+               compatible = "ti,wl1837";
+               reg = <2>;
+               interrupt-parent = <&gpio22>;
+               interrupts = <3 1>;
+               test = <1>;
+       };
+};

-- 
viresh
Viresh Kumar Feb. 9, 2021, 10:10 a.m. UTC | #6 
On 08-02-21, 08:21, Rob Herring wrote:
> We may need to turn off

> checks in overlays for required properties as they may be incomplete.

> We already do that on disabled nodes.


And after decent amount of effort understanding how to do this, I
finally did it in a not so efficient way, I am sure you can help
improving it :)

Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Tue Feb 9 12:19:50 2021 +0530

    dt-validate: Skip "required property" checks for overlays
    
    The overlays may not carry the required properties and would depend on
    the base dtb to carry those, there is no point raising those errors
    here.
    
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 tools/dt-validate | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/dt-validate b/tools/dt-validate
index 410b0538ef47..c6117504f1d1 100755
--- a/tools/dt-validate
+++ b/tools/dt-validate
@@ -80,6 +80,23 @@ show_unmatched = False
                                   (filename, line, col, fullname, node['compatible']), file=sys.stderr)
                             continue
 
+                        if nodename == '/':
+                            is_fragment = False
+                            for name in node.items():
+                                if name[0] == 'fragment@0':
+                                    is_fragment = True
+                                    break;
+
+                            if is_fragment == True:
+                                if 'required property' in error.message:
+                                    continue
+                                elif error.context:
+                                    for e in error.context:
+                                        if not 'required property' in e.message:
+                                            break
+                                    else:
+                                        continue
+
                         print(dtschema.format_error(filename, error, nodename=nodename, verbose=verbose) +
                             '\n\tFrom schema: ' + schema['$filename'],
                             file=sys.stderr)

-- 
viresh
Viresh Kumar Feb. 16, 2021, 10:39 a.m. UTC | #7 
On 09-02-21, 15:40, Viresh Kumar wrote:
> And after decent amount of effort understanding how to do this, I

> finally did it in a not so efficient way, I am sure you can help

> improving it :)


Ping!

Also, where do we send patches for dt-schema ? Which list ?

> Author: Viresh Kumar <viresh.kumar@linaro.org>

> Date:   Tue Feb 9 12:19:50 2021 +0530

> 

>     dt-validate: Skip "required property" checks for overlays

>     

>     The overlays may not carry the required properties and would depend on

>     the base dtb to carry those, there is no point raising those errors

>     here.

>     

>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  tools/dt-validate | 17 +++++++++++++++++

>  1 file changed, 17 insertions(+)

> 

> diff --git a/tools/dt-validate b/tools/dt-validate

> index 410b0538ef47..c6117504f1d1 100755

> --- a/tools/dt-validate

> +++ b/tools/dt-validate

> @@ -80,6 +80,23 @@ show_unmatched = False

>                                    (filename, line, col, fullname, node['compatible']), file=sys.stderr)

>                              continue

>  

> +                        if nodename == '/':

> +                            is_fragment = False

> +                            for name in node.items():

> +                                if name[0] == 'fragment@0':

> +                                    is_fragment = True

> +                                    break;

> +

> +                            if is_fragment == True:

> +                                if 'required property' in error.message:

> +                                    continue

> +                                elif error.context:

> +                                    for e in error.context:

> +                                        if not 'required property' in e.message:

> +                                            break

> +                                    else:

> +                                        continue

> +

>                          print(dtschema.format_error(filename, error, nodename=nodename, verbose=verbose) +

>                              '\n\tFrom schema: ' + schema['$filename'],

>                              file=sys.stderr)


-- 
viresh
diff mbox series

Patch

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 009f4045c8e4..fc286224b2d1 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -34,7 +34,63 @@  DTC_FLAGS_overlay += -@
 DTC_FLAGS_overlay_bad_phandle += -@
 DTC_FLAGS_overlay_bad_symbol += -@
 DTC_FLAGS_overlay_base += -@
+DTC_FLAGS_static_base_1 += -@
+DTC_FLAGS_static_base_2 += -@
 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 static_base_1.dtb and static_base_2.dtb to
+# create static_test_1.dtb and static_test_2.dtb.  If fdtoverlay detects an
+# error than the kernel build will fail.  static_test_1.dtb and
+# static_test_2.dtb are 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_bad_add_dup_node.dtb \
+#			  overlay_bad_add_dup_prop.dtb \
+#			  overlay_bad_phandle.dtb \
+#			  overlay_bad_symbol.dtb \
+
+apply_static_overlay_1 := 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
+
+apply_static_overlay_2 := overlay.dtb
+
+quiet_cmd_fdtoverlay = FDTOVERLAY $@
+      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
+
+$(obj)/static_test_1.dtb: $(obj)/static_base_1.dtb $(addprefix $(obj)/,$(apply_static_overlay_1))
+	$(call if_changed,fdtoverlay)
+
+$(obj)/static_test_2.dtb: $(obj)/static_base_2.dtb $(addprefix $(obj)/,$(apply_static_overlay_2))
+	$(call if_changed,fdtoverlay)
+
+always-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb
diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts
new file mode 100644
index 000000000000..10556cb3f01f
--- /dev/null
+++ b/drivers/of/unittest-data/static_base_1.dts
@@ -0,0 +1,4 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+#include "testcases_common.dtsi"
diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts
new file mode 100644
index 000000000000..b0ea9504d6f3
--- /dev/null
+++ b/drivers/of/unittest-data/static_base_2.dts
@@ -0,0 +1,4 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+#include "overlay_common.dtsi"