* [PATCH v3] kbuild: Add support for DT binding schema checks
@ 2018-12-10 20:32 Rob Herring
2018-12-11 4:38 ` Masahiro Yamada
0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2018-12-10 20:32 UTC (permalink / raw)
To: devicetree, linux-kernel
Cc: Sean Hudson, Frank Rowand, linux-arm-kernel, linuxppc-dev,
Grant Likely, Kumar Gala, arm, Jonathan Corbet, Mark Rutland,
Masahiro Yamada, Michal Marek, linux-doc, linux-kbuild
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
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)
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] kbuild: Add support for DT binding schema checks
2018-12-10 20:32 [PATCH v3] kbuild: Add support for DT binding schema checks Rob Herring
@ 2018-12-11 4:38 ` Masahiro Yamada
2018-12-11 15:12 ` Rob Herring
0 siblings, 1 reply; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-11 4:38 UTC (permalink / raw)
To: Rob Herring
Cc: DTML, Linux Kernel Mailing List, darknighte, Frank Rowand,
linux-arm-kernel, linuxppc-dev, grant.likely, Kumar Gala,
arm-soc, Jonathan Corbet, Mark Rutland, Michal Marek,
open list:DOCUMENTATION, Linux Kbuild mailing list
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.
--
Best Regards
Masahiro Yamada
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] kbuild: Add support for DT binding schema checks
2018-12-11 4:38 ` Masahiro Yamada
@ 2018-12-11 15:12 ` Rob Herring
2018-12-11 16:02 ` Masahiro Yamada
0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2018-12-11 15:12 UTC (permalink / raw)
To: Masahiro Yamada
Cc: devicetree, linux-kernel, Sean Hudson, Frank Rowand,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linuxppc-dev, Grant Likely, Kumar Gala, ARM-SoC Maintainers,
Jonathan Corbet, Mark Rutland, Michal Marek,
Linux Doc Mailing List, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] kbuild: Add support for DT binding schema checks
2018-12-11 15:12 ` Rob Herring
@ 2018-12-11 16:02 ` Masahiro Yamada
2018-12-11 18:35 ` Rob Herring
0 siblings, 1 reply; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-11 16:02 UTC (permalink / raw)
To: Rob Herring
Cc: DTML, Linux Kernel Mailing List, darknighte, Frank Rowand,
linux-arm-kernel, linuxppc-dev, grant.likely, Kumar Gala,
arm-soc, Jonathan Corbet, Mark Rutland, Michal Marek,
open list:DOCUMENTATION, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] kbuild: Add support for DT binding schema checks
2018-12-11 16:02 ` Masahiro Yamada
@ 2018-12-11 18:35 ` Rob Herring
2018-12-12 3:04 ` Masahiro Yamada
0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2018-12-11 18:35 UTC (permalink / raw)
To: Masahiro Yamada
Cc: devicetree, linux-kernel, Sean Hudson, Frank Rowand,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linuxppc-dev, Grant Likely, Kumar Gala, ARM-SoC Maintainers,
Jonathan Corbet, Mark Rutland, Michal Marek,
Linux Doc Mailing List, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] kbuild: Add support for DT binding schema checks
2018-12-11 18:35 ` Rob Herring
@ 2018-12-12 3:04 ` Masahiro Yamada
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-12 3:04 UTC (permalink / raw)
To: Rob Herring
Cc: DTML, Linux Kernel Mailing List, darknighte, Frank Rowand,
linux-arm-kernel, linuxppc-dev, grant.likely, Kumar Gala,
arm-soc, Jonathan Corbet, Mark Rutland, Michal Marek,
open list:DOCUMENTATION, Linux Kbuild mailing list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-12 3:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 20:32 [PATCH v3] kbuild: Add support for DT binding schema checks Rob Herring
2018-12-11 4:38 ` Masahiro Yamada
2018-12-11 15:12 ` Rob Herring
2018-12-11 16:02 ` Masahiro Yamada
2018-12-11 18:35 ` Rob Herring
2018-12-12 3:04 ` Masahiro Yamada
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).