[v3] kbuild: Add support for DT binding schema checks

Message ID 20181210203225.11179-1-robh@kernel.org
State New
Headers show
Series
  • [v3] kbuild: Add support for DT binding schema checks
Related show

Commit Message

Rob Herring Dec. 10, 2018, 8:32 p.m.
This adds the build infrastructure for checking DT binding schema
documents and validating dts files using the binding schema.

Check DT binding schema documents:
make dt_binding_check

Build dts files and check using DT binding schema:
make dtbs_check

Optionally, DT_SCHEMA_FILES can passed in with a schema file(s) to use
for validation. This makes it easier to find and fix errors generated by
a specific schema.

Currently, the validation targets are separate from a normal build to
avoid a hard dependency on the external DT schema project and because
there are lots of warnings generated.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: linux-doc@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>

---
v3:
- Fix error causing only 1st schema file to get used.
- Add a more useful error message when dtc is missing YAML support 
telling the user they need to install libyaml devel package.

 
 .gitignore                                   |  1 +
 Documentation/Makefile                       |  2 +-
 Documentation/devicetree/bindings/.gitignore |  1 +
 Documentation/devicetree/bindings/Makefile   | 33 +++++++++++++++++
 Makefile                                     | 11 ++++--
 scripts/Makefile.lib                         | 37 ++++++++++++++++++--
 6 files changed, 80 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/.gitignore
 create mode 100644 Documentation/devicetree/bindings/Makefile

-- 
2.19.1

Comments

Masahiro Yamada Dec. 11, 2018, 4:02 p.m. | #1
On Wed, Dec 12, 2018 at 12:13 AM Rob Herring <robh@kernel.org> wrote:

>

> > > +$(obj)/%.example.dts: $(src)/%.yaml FORCE

> > > +       $(call if_changed,chk_binding)

> > > +

> > > +DT_TMP_SCHEMA := .schema.yaml.tmp

> >

> >

> > BTW, why does this file start with a period?

> > What is the meaning of '.tmp' extension?

>

> Nothing really. Just named it something so it gets cleaned and ignored by git.



It is cleaned whatever file name you use.


See scripts/Makefile.clean

__clean-files   := $(extra-y) $(extra-m) $(extra-)       \
                   $(always) $(targets) $(clean-files)   \
                   $(hostprogs-y) $(hostprogs-m) $(hostprogs-) \
                   $(hostlibs-y) $(hostlibs-m) $(hostlibs-) \
                   $(hostcxxlibs-y) $(hostcxxlibs-m)


$(extra-y) is cleaned.



You are adding *.example.dts to .gitignore

Why not "schema.yaml" ?




> > > +extra-y += $(DT_TMP_SCHEMA)

> > > +

> > > +quiet_cmd_mk_schema = SCHEMA  $@

> > > +      cmd_mk_schema = mkdir -p $(obj); \

> > > +                      rm -f $@; \

> > > +                      $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(filter-out FORCE, $^)

> >

> >

> > "mkdir -p $(obj)" is redundant.

> >

> >

> > Why is 'rm -f $@' necessary ?

> > Can't dt-mk-schema overwrite the output file?

>

> It is for error case when the output file is not generated. I can

> handle this within dt-mk-schema instead.

> > > +DT_DOCS = $(shell cd $(srctree)/$(src) && find * -name '*.yaml')

> > > +DT_SCHEMA_FILES ?= $(addprefix $(src)/,$(DT_DOCS))

> > > +

> > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dts, $(DT_SCHEMA_FILES))

> > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES))

> >

> >

> >

> > I assume you intentionally did not do like this:

> >

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

> >

> > From the commit description, DT_SCHEMA_FILES might be overridden by a user.

> > So, I think this is OK.

> >

> >

> >

> >

> > > +$(obj)/$(DT_TMP_SCHEMA): | $(addprefix $(obj)/,$(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES)))

> >

> > I do not understand this line.

> > Why is it necessary?

> >

> > *.example.dtb files are generated anyway

> > since they are listed in extra-y.

>

> It is enforcing the ordering. Without it, the binding checks and

> building .schema.yaml.tmp happen in parallel because both only have

> the source files as dependencies. The '|' keeps the dependencies out

> of the dependency list($^).



What kind problem would you see if
the binding checks and building .schema.yaml.tmp
happen in parallel?



-- 
Best Regards
Masahiro Yamada
Rob Herring Dec. 11, 2018, 6:35 p.m. | #2
On Tue, Dec 11, 2018 at 10:03 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>

> On Wed, Dec 12, 2018 at 12:13 AM Rob Herring <robh@kernel.org> wrote:

>

> >

> > > > +$(obj)/%.example.dts: $(src)/%.yaml FORCE

> > > > +       $(call if_changed,chk_binding)

> > > > +

> > > > +DT_TMP_SCHEMA := .schema.yaml.tmp

> > >

> > >

> > > BTW, why does this file start with a period?

> > > What is the meaning of '.tmp' extension?

> >

> > Nothing really. Just named it something so it gets cleaned and ignored by git.

>

>

> It is cleaned whatever file name you use.

>

>

> See scripts/Makefile.clean

>

> __clean-files   := $(extra-y) $(extra-m) $(extra-)       \

>                    $(always) $(targets) $(clean-files)   \

>                    $(hostprogs-y) $(hostprogs-m) $(hostprogs-) \

>                    $(hostlibs-y) $(hostlibs-m) $(hostlibs-) \

>                    $(hostcxxlibs-y) $(hostcxxlibs-m)

>

>

> $(extra-y) is cleaned.


True.

>

>

> You are adding *.example.dts to .gitignore

>

> Why not "schema.yaml" ?


Okay. I'll do "processed-schema.yaml" to give a bit better name of
what it contains.

>

> > > > +extra-y += $(DT_TMP_SCHEMA)

> > > > +

> > > > +quiet_cmd_mk_schema = SCHEMA  $@

> > > > +      cmd_mk_schema = mkdir -p $(obj); \

> > > > +                      rm -f $@; \

> > > > +                      $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(filter-out FORCE, $^)

> > >

> > >

> > > "mkdir -p $(obj)" is redundant.

> > >

> > >

> > > Why is 'rm -f $@' necessary ?

> > > Can't dt-mk-schema overwrite the output file?

> >

> > It is for error case when the output file is not generated. I can

> > handle this within dt-mk-schema instead.

> > > > +DT_DOCS = $(shell cd $(srctree)/$(src) && find * -name '*.yaml')

> > > > +DT_SCHEMA_FILES ?= $(addprefix $(src)/,$(DT_DOCS))

> > > > +

> > > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dts, $(DT_SCHEMA_FILES))

> > > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES))

> > >

> > >

> > >

> > > I assume you intentionally did not do like this:

> > >

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

> > >

> > > From the commit description, DT_SCHEMA_FILES might be overridden by a user.

> > > So, I think this is OK.

> > >

> > >

> > >

> > >

> > > > +$(obj)/$(DT_TMP_SCHEMA): | $(addprefix $(obj)/,$(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES)))

> > >

> > > I do not understand this line.

> > > Why is it necessary?

> > >

> > > *.example.dtb files are generated anyway

> > > since they are listed in extra-y.

> >

> > It is enforcing the ordering. Without it, the binding checks and

> > building .schema.yaml.tmp happen in parallel because both only have

> > the source files as dependencies. The '|' keeps the dependencies out

> > of the dependency list($^).

>

>

> What kind problem would you see if

> the binding checks and building .schema.yaml.tmp

> happen in parallel?


In case of no errors in the binding docs, it doesn't matter. If there
are errors, I don't want the dtbs validation to run if any schema
doesn't validate. However, I played around with this a bit more and it
seems like having the examples' dts/dtb in extra-y prevents that from
happening. Does that match your expections? If so, then I think we can
remove the dependency.

Here's an example.

  SCHEMA  Documentation/devicetree/bindings/.schema.yaml.tmp
  CHKDT   Documentation/devicetree/bindings/arm/vt8500.yaml
  CHKDT   Documentation/devicetree/bindings/arm/zte.yaml
  CHKDT   Documentation/devicetree/bindings/arm/altera/socfpga-clk-manager.yaml
  CHKDT   Documentation/devicetree/bindings/arm/ti/ti,davinci.yaml
  CHKDT   Documentation/devicetree/bindings/arm/ti/nspire.yaml
  CHKDT   Documentation/devicetree/bindings/arm/spear.yaml
  CHKDT   Documentation/devicetree/bindings/arm/altera.yaml
warning: no schema found in file:
../Documentation/devicetree/bindings/arm/xilinx.yaml
/home/rob/proj/git/linux-2.6/Documentation/devicetree/bindings/arm/xilinx.yaml:
ignoring, error in schema 'onOf'
  CHKDT   Documentation/devicetree/bindings/arm/sti.yaml
  CHKDT   Documentation/devicetree/bindings/arm/qcom.yaml
  CHKDT   Documentation/devicetree/bindings/arm/primecell.yaml
  CHKDT   Documentation/devicetree/bindings/arm/cpus.yaml
  CHKDT   Documentation/devicetree/bindings/arm/sirf.yaml
  CHKDT   Documentation/devicetree/bindings/arm/calxeda.yaml
  CHKDT   Documentation/devicetree/bindings/arm/xilinx.yaml
  CHKDT   Documentation/devicetree/bindings/example-schema.yaml
/home/rob/proj/git/linux-2.6/Documentation/devicetree/bindings/arm/xilinx.yaml:
properties:compatible:onOf: 'onOf' is not one of ['$ref',
'additionalItems', 'allOf', 'const', 'contains', 'default',
'dependencies', 'description', 'enum', 'items', 'minItems', 'minimum',
'maxItems', 'maximum', 'not', 'oneOf', 'pattern', 'patternProperties',
'properties', 'required', 'type', 'typeSize']
make[2]: *** [../Documentation/devicetree/bindings/Makefile:12:
Documentation/devicetree/bindings/arm/xilinx.example.dts] Error 1

Rob
Masahiro Yamada Dec. 12, 2018, 3:04 a.m. | #3
On Wed, Dec 12, 2018 at 3:36 AM Rob Herring <robh@kernel.org> wrote:
>

> On Tue, Dec 11, 2018 at 10:03 AM Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

> >

> > On Wed, Dec 12, 2018 at 12:13 AM Rob Herring <robh@kernel.org> wrote:

> >

> > >

> > > > > +$(obj)/%.example.dts: $(src)/%.yaml FORCE

> > > > > +       $(call if_changed,chk_binding)

> > > > > +

> > > > > +DT_TMP_SCHEMA := .schema.yaml.tmp

> > > >

> > > >

> > > > BTW, why does this file start with a period?

> > > > What is the meaning of '.tmp' extension?

> > >

> > > Nothing really. Just named it something so it gets cleaned and ignored by git.

> >

> >

> > It is cleaned whatever file name you use.

> >

> >

> > See scripts/Makefile.clean

> >

> > __clean-files   := $(extra-y) $(extra-m) $(extra-)       \

> >                    $(always) $(targets) $(clean-files)   \

> >                    $(hostprogs-y) $(hostprogs-m) $(hostprogs-) \

> >                    $(hostlibs-y) $(hostlibs-m) $(hostlibs-) \

> >                    $(hostcxxlibs-y) $(hostcxxlibs-m)

> >

> >

> > $(extra-y) is cleaned.

>

> True.

>

> >

> >

> > You are adding *.example.dts to .gitignore

> >

> > Why not "schema.yaml" ?

>

> Okay. I'll do "processed-schema.yaml" to give a bit better name of

> what it contains.

>

> >

> > > > > +extra-y += $(DT_TMP_SCHEMA)

> > > > > +

> > > > > +quiet_cmd_mk_schema = SCHEMA  $@

> > > > > +      cmd_mk_schema = mkdir -p $(obj); \

> > > > > +                      rm -f $@; \

> > > > > +                      $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(filter-out FORCE, $^)

> > > >

> > > >

> > > > "mkdir -p $(obj)" is redundant.

> > > >

> > > >

> > > > Why is 'rm -f $@' necessary ?

> > > > Can't dt-mk-schema overwrite the output file?

> > >

> > > It is for error case when the output file is not generated. I can

> > > handle this within dt-mk-schema instead.

> > > > > +DT_DOCS = $(shell cd $(srctree)/$(src) && find * -name '*.yaml')

> > > > > +DT_SCHEMA_FILES ?= $(addprefix $(src)/,$(DT_DOCS))

> > > > > +

> > > > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dts, $(DT_SCHEMA_FILES))

> > > > > +extra-y += $(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES))

> > > >

> > > >

> > > >

> > > > I assume you intentionally did not do like this:

> > > >

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

> > > >

> > > > From the commit description, DT_SCHEMA_FILES might be overridden by a user.

> > > > So, I think this is OK.

> > > >

> > > >

> > > >

> > > >

> > > > > +$(obj)/$(DT_TMP_SCHEMA): | $(addprefix $(obj)/,$(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES)))

> > > >

> > > > I do not understand this line.

> > > > Why is it necessary?

> > > >

> > > > *.example.dtb files are generated anyway

> > > > since they are listed in extra-y.

> > >

> > > It is enforcing the ordering. Without it, the binding checks and

> > > building .schema.yaml.tmp happen in parallel because both only have

> > > the source files as dependencies. The '|' keeps the dependencies out

> > > of the dependency list($^).

> >

> >

> > What kind problem would you see if

> > the binding checks and building .schema.yaml.tmp

> > happen in parallel?

>

> In case of no errors in the binding docs, it doesn't matter. If there

> are errors, I don't want the dtbs validation to run if any schema

> doesn't validate. However, I played around with this a bit more and it

> seems like having the examples' dts/dtb in extra-y prevents that from

> happening. Does that match your expections?


Exactly.

If any error occurs in Documentation/devicetree/bindings/Makefile,
Make terminates before proceeding to the dtbs_check stage.



-- 
Best Regards
Masahiro Yamada

Patch

diff --git a/.gitignore b/.gitignore
index 97ba6b79834c..a20ac26aa2f5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,7 @@ 
 *.bin
 *.bz2
 *.c.[012]*.*
+*.dt.yaml
 *.dtb
 *.dtb.S
 *.dwo
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 2ca77ad0f238..9786957c6a35 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -2,7 +2,7 @@ 
 # Makefile for Sphinx documentation
 #
 
-subdir-y :=
+subdir-y := devicetree/bindings/
 
 # You can set these variables from the command line.
 SPHINXBUILD   = sphinx-build
diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
new file mode 100644
index 000000000000..d9194c02dd08
--- /dev/null
+++ b/Documentation/devicetree/bindings/.gitignore
@@ -0,0 +1 @@ 
+*.example.dts
diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
new file mode 100644
index 000000000000..43f8657ab070
--- /dev/null
+++ b/Documentation/devicetree/bindings/Makefile
@@ -0,0 +1,33 @@ 
+# SPDX-License-Identifier: GPL-2.0
+DT_DOC_CHECKER ?= dt-doc-validate
+DT_EXTRACT_EX ?= dt-extract-example
+DT_MK_SCHEMA ?= dt-mk-schema
+DT_MK_SCHEMA_FLAGS := $(if $(DT_SCHEMA_FILES), -u)
+
+quiet_cmd_chk_binding = CHKDT   $<
+      cmd_chk_binding = (set -e; \
+                         $(DT_DOC_CHECKER) $< ; \
+                         mkdir -p $(dir $@) ; \
+                         $(DT_EXTRACT_EX) $< > $@ )
+
+$(obj)/%.example.dts: $(src)/%.yaml FORCE
+	$(call if_changed,chk_binding)
+
+DT_TMP_SCHEMA := .schema.yaml.tmp
+extra-y += $(DT_TMP_SCHEMA)
+
+quiet_cmd_mk_schema = SCHEMA  $@
+      cmd_mk_schema = mkdir -p $(obj); \
+                      rm -f $@; \
+                      $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(filter-out FORCE, $^)
+
+DT_DOCS = $(shell cd $(srctree)/$(src) && find * -name '*.yaml')
+DT_SCHEMA_FILES ?= $(addprefix $(src)/,$(DT_DOCS))
+
+extra-y += $(patsubst $(src)/%.yaml,%.example.dts, $(DT_SCHEMA_FILES))
+extra-y += $(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES))
+
+$(obj)/$(DT_TMP_SCHEMA): | $(addprefix $(obj)/,$(patsubst $(src)/%.yaml,%.example.dtb, $(DT_SCHEMA_FILES)))
+
+$(obj)/$(DT_TMP_SCHEMA): $(addprefix $(srctree)/, $(DT_SCHEMA_FILES)) FORCE
+	$(call if_changed,mk_schema)
diff --git a/Makefile b/Makefile
index 2f36db897895..ff59adf43300 100644
--- a/Makefile
+++ b/Makefile
@@ -1232,10 +1232,13 @@  ifneq ($(dtstree),)
 %.dtb: prepare3 scripts_dtc
 	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
 
-PHONY += dtbs dtbs_install
+PHONY += dtbs dtbs_install dt_binding_check
 dtbs: prepare3 scripts_dtc
 	$(Q)$(MAKE) $(build)=$(dtstree)
 
+dtbs_check: prepare3 dt_binding_check
+	$(Q)$(MAKE) $(build)=$(dtstree) CHECK_DTBS=1
+
 dtbs_install:
 	$(Q)$(MAKE) $(dtbinst)=$(dtstree)
 
@@ -1249,6 +1252,9 @@  PHONY += scripts_dtc
 scripts_dtc: scripts_basic
 	$(Q)$(MAKE) $(build)=scripts/dtc
 
+dt_binding_check: scripts_dtc
+	$(Q)$(MAKE) $(build)=Documentation/devicetree/bindings
+
 # ---------------------------------------------------------------------------
 # Modules
 
@@ -1611,7 +1617,8 @@  clean: $(clean-dirs)
 	$(call cmd,rmfiles)
 	@find $(if $(KBUILD_EXTMOD), $(KBUILD_EXTMOD), .) $(RCS_FIND_IGNORE) \
 		\( -name '*.[aios]' -o -name '*.ko' -o -name '.*.cmd' \
-		-o -name '*.ko.*' -o -name '*.dtb' -o -name '*.dtb.S' \
+		-o -name '*.ko.*' \
+		-o -name '*.dtb' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
 		-o -name '*.dwo' -o -name '*.lst' \
 		-o -name '*.su'  \
 		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 8fe4468f9bda..6a3450b20041 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -61,6 +61,11 @@  real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y))
 extra-y				+= $(dtb-y)
 extra-$(CONFIG_OF_ALL_DTBS)	+= $(dtb-)
 
+ifneq ($(CHECK_DTBS),)
+extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
+extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
+endif
+
 # Add subdir path
 
 extra-y		:= $(addprefix $(obj)/,$(extra-y))
@@ -284,13 +289,41 @@  $(obj)/%.dtb.S: $(obj)/%.dtb FORCE
 quiet_cmd_dtc = DTC     $@
 cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 	$(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; \
-	$(DTC) -O dtb -o $@ -b 0 \
+	$(DTC) -O $(2) -o $@ -b 0 \
 		$(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \
 		-d $(depfile).dtc.tmp $(dtc-tmp) ; \
 	cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
 
 $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
-	$(call if_changed_dep,dtc)
+	$(call if_changed_dep,dtc,dtb)
+
+DT_CHECKER ?= dt-validate
+DT_BINDING_DIR := Documentation/devicetree/bindings
+DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/.schema.yaml.tmp
+
+quiet_cmd_dtb_check =	CHECK   $@
+      cmd_dtb_check =	$(DT_CHECKER) -p $(DT_TMP_SCHEMA) $@ ;
+
+define rule_dtc_dt_yaml
+	$(call cmd_and_fixdep,dtc,yaml)		\
+	$(call echo-cmd,dtb_check) $(cmd_dtb_check)
+endef
+
+cmd_dtc_yaml_chk = \
+	if ! echo "/dts-v1/; /{};" | $(DTC) -I dts -O yaml > /dev/null ; then \
+		echo "*"; \
+		echo "* dtc needs libyaml for DT schema validation support."; \
+		echo "* Install the necessary libyaml development package from your distro."; \
+		echo "*"; \
+		false; \
+	fi; \
+	touch $@
+
+$(objtree)/scripts/dtc/.dtc-yaml-chk.tmp: $(DTC) FORCE
+	$(call if_changed,dtc_yaml_chk)
+
+$(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE | $(objtree)/scripts/dtc/.dtc-yaml-chk.tmp
+	$(call if_changed_rule,dtc_dt_yaml)
 
 dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)