| Message ID | 20181210203225.11179-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] kbuild: Add support for DT binding schema checks | expand |
Hi Rob, On Tue, Dec 11, 2018 at 5:50 AM Rob Herring <robh@kernel.org> wrote: > > 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 Perhaps, "can be passed" ? > 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 > > 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 $< In convention, the short log displays the target name instead of the prerequisite. If O=foo/bar is given, $< shows the full path, then log will look like follows: CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/calxeda.yaml DTC Documentation/devicetree/bindings/arm/calxeda.example.dtb CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/qcom.yaml DTC Documentation/devicetree/bindings/arm/qcom.example.dtb CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/xilinx.yaml DTC Documentation/devicetree/bindings/arm/xilinx.example.dtb CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/nspire.yaml DTC Documentation/devicetree/bindings/arm/ti/nspire.example.dtb CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/ti,davinci.yaml DTC Documentation/devicetree/bindings/arm/ti/ti,davinci.example.dtb CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/altera.yaml You might think it is ugly. > + cmd_chk_binding = (set -e; \ > + $(DT_DOC_CHECKER) $< ; \ > + mkdir -p $(dir $@) ; \ > + $(DT_EXTRACT_EX) $< > $@ ) - 'set -e' is redundant because if_changed already has it. - 'mkdir mkdir -p $(dir $@)' is also redundant because scripts/Makefile.build automatically creates it. - You do not need to execute this in a sub-shell You can simplify like this: cmd_chk_binding = $(DT_DOC_CHECKER) $< ; \ $(DT_EXTRACT_EX) $< > $@ > +$(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? > +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? > +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. > +$(obj)/$(DT_TMP_SCHEMA): $(addprefix $(srctree)/, $(DT_SCHEMA_FILES)) FORCE > + $(call if_changed,mk_schema) You do not need to prefix $(srctree)/ because Kbuild uses VPATH. $(obj)/$(DT_TMP_SCHEMA): $(DT_SCHEMA_FILES) FORCE $(call if_changed,mk_schema) is fine. > +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) Hmm, this is kind of ugly. I'd rather check this in scripts/dtc/Makefile How about something like below? diff --git a/Makefile b/Makefile index ff59adf..a3e2db2 100644 --- a/Makefile +++ b/Makefile @@ -1233,11 +1233,11 @@ ifneq ($(dtstree),) $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ PHONY += dtbs dtbs_install dt_binding_check -dtbs: prepare3 scripts_dtc +dtbs dtbs_check: prepare3 scripts_dtc $(Q)$(MAKE) $(build)=$(dtstree) -dtbs_check: prepare3 dt_binding_check - $(Q)$(MAKE) $(build)=$(dtstree) CHECK_DTBS=1 +dtbs_check: export CHECK_DTBS=1 +dtbs_check: dt_binding_check dtbs_install: $(Q)$(MAKE) $(dtbinst)=$(dtstree) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 8a7d622..5017175 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -309,20 +309,7 @@ define rule_dtc_dt_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 +$(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc_dt_yaml) dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile index 056d5da..3e497f1 100644 --- a/scripts/dtc/Makefile +++ b/scripts/dtc/Makefile @@ -12,6 +12,10 @@ dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o HOST_EXTRACFLAGS := -I$(src)/libfdt ifeq ($(wildcard /usr/include/yaml.h),) +ifeq ($(CHECK_DTBS),1) +$(error dtc needs libyaml for DT schema validation support. \ + Install the necessary libyaml development package.) +endif HOST_EXTRACFLAGS += -DNO_YAML else dtc-objs += yamltree.o One drawback of this approach is, this is not checked if you are using out-of-tree DTC. Probably, Rob is the only person that overrides DTC.
On Mon, Dec 10, 2018 at 10:39 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Hi Rob, > > On Tue, Dec 11, 2018 at 5:50 AM Rob Herring <robh@kernel.org> wrote: > > > > 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 > > > Perhaps, "can be passed" ? Yes. [...] > > +quiet_cmd_chk_binding = CHKDT $< > > > In convention, the short log displays the target name > instead of the prerequisite. Yes, but here there is no target. We're just running a check on the source. > If O=foo/bar is given, $< shows the full path, > then log will look like follows: > > > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/calxeda.yaml > DTC Documentation/devicetree/bindings/arm/calxeda.example.dtb > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/qcom.yaml > DTC Documentation/devicetree/bindings/arm/qcom.example.dtb > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/xilinx.yaml > DTC Documentation/devicetree/bindings/arm/xilinx.example.dtb > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/nspire.yaml > DTC Documentation/devicetree/bindings/arm/ti/nspire.example.dtb > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/ti/ti,davinci.yaml > DTC Documentation/devicetree/bindings/arm/ti/ti,davinci.example.dtb > CHKDT /home/masahiro/ref/linux-devicetree/Documentation/devicetree/bindings/arm/altera.yaml > > You might think it is ugly. I've changed it to this: quiet_cmd_chk_binding = CHKDT $(patsubst $(srctree)/%,%,$<) [...] > > +$(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. > > +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($^). [...] > > +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) > > > Hmm, this is kind of ugly. > > I'd rather check this in scripts/dtc/Makefile > > > > How about something like below? Looks good. > ifeq ($(wildcard /usr/include/yaml.h),) > +ifeq ($(CHECK_DTBS),1) I'm going to change this to "ifneq ($CHECK_DTBS),)" in case we start supporting more than 1 value such as warning levels. > +$(error dtc needs libyaml for DT schema validation support. \ > + Install the necessary libyaml development package.) > +endif > HOST_EXTRACFLAGS += -DNO_YAML > else > dtc-objs += yamltree.o > > > > > > One drawback of this approach is, > this is not checked if you are using out-of-tree DTC. > Probably, Rob is the only person that overrides DTC. Sadly, that's probably true. Thanks for your thorough review. Rob
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?
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
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.
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)
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