[PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

It is useful to be able to recheck dtbs files against a limited set of
DT schema files. This can be accomplished by using differnt
DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
for some reason if_changed_rule doesn't pick up the rule_dtc changes
(and doesn't retrigger the build).

Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
and dt-validate into a single new command. Then if_changed_dep triggers
on DT_SCHEMA_FILES changes and reruns the build/check.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 scripts/Makefile.lib | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index c88b98b5dc44..3df470289382 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
 DT_BINDING_DIR := Documentation/devicetree/bindings
 DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
 
-quiet_cmd_dtb_check =	CHECK   $@
-      cmd_dtb_check =	$(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
+quiet_cmd_dtb =	DTC/CHECK   $@
+      cmd_dtb =	$(cmd_dtc) ; $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
+else
+quiet_cmd_dtb = $(quiet_cmd_dtc)
+      cmd_dtb = $(cmd_dtc)
 endif
 
-define rule_dtc
-	$(call cmd_and_fixdep,dtc)
-	$(call cmd,dtb_check)
-endef
-
 $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
-	$(call if_changed_rule,dtc)
+	$(call if_changed_dep,dtb)
 
 $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
 	$(call if_changed_dep,dtc)
-- 
2.35.1

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Rob Herring]

On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> It is useful to be able to recheck dtbs files against a limited set of
> DT schema files. This can be accomplished by using differnt
> DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> for some reason if_changed_rule doesn't pick up the rule_dtc changes
> (and doesn't retrigger the build).
>
> Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> and dt-validate into a single new command. Then if_changed_dep triggers
> on DT_SCHEMA_FILES changes and reruns the build/check.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  scripts/Makefile.lib | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index c88b98b5dc44..3df470289382 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
>  DT_BINDING_DIR := Documentation/devicetree/bindings
>  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
>
> -quiet_cmd_dtb_check =  CHECK   $@
> -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> +quiet_cmd_dtb =        DTC/CHECK   $@

This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
always do just 'DTC'. I can fixup when applying.

I'll give it a few days for other comments.

> +      cmd_dtb =        $(cmd_dtc) ; $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> +else
> +quiet_cmd_dtb = $(quiet_cmd_dtc)
> +      cmd_dtb = $(cmd_dtc)
>  endif
>
> -define rule_dtc
> -       $(call cmd_and_fixdep,dtc)
> -       $(call cmd,dtb_check)
> -endef
> -
>  $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> -       $(call if_changed_rule,dtc)
> +       $(call if_changed_dep,dtb)
>
>  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>         $(call if_changed_dep,dtc)
> --
> 2.35.1
>

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Masahiro Yamada]

On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > It is useful to be able to recheck dtbs files against a limited set of
> > DT schema files. This can be accomplished by using differnt
> > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > (and doesn't retrigger the build).
> >
> > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > and dt-validate into a single new command. Then if_changed_dep triggers
> > on DT_SCHEMA_FILES changes and reruns the build/check.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  scripts/Makefile.lib | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index c88b98b5dc44..3df470289382 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> >  DT_BINDING_DIR := Documentation/devicetree/bindings
> >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> >
> > -quiet_cmd_dtb_check =  CHECK   $@
> > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > +quiet_cmd_dtb =        DTC/CHECK   $@
>
> This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> always do just 'DTC'. I can fixup when applying.
>
> I'll give it a few days for other comments.



When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
You do not need to re-run dtc.

I guess the strangeness comes from the fact that you are trying to do the
 two different things in a single rule.


--
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > It is useful to be able to recheck dtbs files against a limited set of
> > > DT schema files. This can be accomplished by using differnt
> > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > (and doesn't retrigger the build).
> > >
> > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  scripts/Makefile.lib | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index c88b98b5dc44..3df470289382 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > >
> > > -quiet_cmd_dtb_check =  CHECK   $@
> > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > +quiet_cmd_dtb =        DTC/CHECK   $@
> >
> > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > always do just 'DTC'. I can fixup when applying.
> >
> > I'll give it a few days for other comments.
>
>
>
> When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> You do not need to re-run dtc.
>
> I guess the strangeness comes from the fact that you are trying to do the
>  two different things in a single rule.

The issue is that with the current rules the dt-validate isn't
re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.

-- 
With best wishes
Dmitry

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Masahiro Yamada]

On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > DT schema files. This can be accomplished by using differnt
> > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > (and doesn't retrigger the build).
> > > >
> > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  scripts/Makefile.lib | 14 ++++++--------
> > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > index c88b98b5dc44..3df470289382 100644
> > > > --- a/scripts/Makefile.lib
> > > > +++ b/scripts/Makefile.lib
> > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > >
> > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > >
> > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > always do just 'DTC'. I can fixup when applying.
> > >
> > > I'll give it a few days for other comments.
> >
> >
> >
> > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > You do not need to re-run dtc.
> >
> > I guess the strangeness comes from the fact that you are trying to do the
> >  two different things in a single rule.
>
> The issue is that with the current rules the dt-validate isn't
> re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.

Correct.



What I said is like this.



# touch the timestamp file, %.dtb.checked
$(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
        $(call if_changed_rule,dtb_check)

$(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
        $(call if_changed_rule,dtc)

$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
        $(call if_changed_dep,dtc)


With the dtc/check split, we can avoid unneeded regeneration of
%.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
changed.


One drawback is we track %.dtb.checked and and %.dtb separately,
so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
may come back.






BTW, we do not check %.dtbo, why?

At least, 53182e81f47d4ea0c727c49ad23cb782173ab849
was trying to check %.dtbo




> --
> With best wishes
> Dmitry



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > DT schema files. This can be accomplished by using differnt
> > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > (and doesn't retrigger the build).
> > > > >
> > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > index c88b98b5dc44..3df470289382 100644
> > > > > --- a/scripts/Makefile.lib
> > > > > +++ b/scripts/Makefile.lib
> > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > >
> > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > >
> > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > always do just 'DTC'. I can fixup when applying.
> > > >
> > > > I'll give it a few days for other comments.
> > >
> > >
> > >
> > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > You do not need to re-run dtc.
> > >
> > > I guess the strangeness comes from the fact that you are trying to do the
> > >  two different things in a single rule.
> >
> > The issue is that with the current rules the dt-validate isn't
> > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
>
> Correct.
>
> What I said is like this.
>
> # touch the timestamp file, %.dtb.checked
> $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
>         $(call if_changed_rule,dtb_check)
>
> $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
>         $(call if_changed_rule,dtc)
>
> $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>         $(call if_changed_dep,dtc)
>
>
> With the dtc/check split, we can avoid unneeded regeneration of
> %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> changed.
>
>
> One drawback is we track %.dtb.checked and and %.dtb separately,
> so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> may come back.

It's up to you and Rob, but I'd really prefer a simpler solution here.
Regenerating dtbs sounds like a minor pain compared to hacking the
top-level Makefile again. What I really like is that if one has
CHECK_DTBS=y (for whatever reason), he can not generate dtb without
validation.

> BTW, we do not check %.dtbo, why?
>
> At least, 53182e81f47d4ea0c727c49ad23cb782173ab849
> was trying to check %.dtbo

The commit ef8795f3f1ce ("dt-bindings: kbuild: Use DTB files for
validation") separated .dtb and .dtbo paths. dt-validate is not
prepared to be executed on top of the .dtbo file. And this is quite
logical. The dtbo is an overlay, a patch. So it doesn't have to follow
the schema on its own. We should probably make sure that multi-dtb
files generated via fdtoverlay are also validated, but it can go in a
separate commit.

-- 
With best wishes
Dmitry

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Rob Herring]

On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > (and doesn't retrigger the build).
> > > > > >
> > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > --- a/scripts/Makefile.lib
> > > > > > +++ b/scripts/Makefile.lib
> > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > >
> > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > >
> > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > always do just 'DTC'. I can fixup when applying.
> > > > >
> > > > > I'll give it a few days for other comments.
> > > >
> > > >
> > > >
> > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > You do not need to re-run dtc.
> > > >
> > > > I guess the strangeness comes from the fact that you are trying to do the
> > > >  two different things in a single rule.
> > >
> > > The issue is that with the current rules the dt-validate isn't
> > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> >
> > Correct.
> >
> > What I said is like this.
> >
> > # touch the timestamp file, %.dtb.checked
> > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE

Not really a fan of the thousands of files that creates. Maybe if it
was turned into something useful like a list of schemas that apply to
the dtb. IOW, a dependency list. That would speed up re-running after
a schema change. Though if a schema change created new dependencies,
that wouldn't work.

> >         $(call if_changed_rule,dtb_check)
> >
> > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> >         $(call if_changed_rule,dtc)
> >
> > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> >         $(call if_changed_dep,dtc)
> >
> >
> > With the dtc/check split, we can avoid unneeded regeneration of
> > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > changed.
> >
> >
> > One drawback is we track %.dtb.checked and and %.dtb separately,
> > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > may come back.
>
> It's up to you and Rob, but I'd really prefer a simpler solution here.
> Regenerating dtbs sounds like a minor pain compared to hacking the
> top-level Makefile again. What I really like is that if one has
> CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> validation.

I lean towards just rebuilding the dtbs. That's pretty quick and
ensures we get dtc warnings with schema warnings. In the long run, I
would like to make the schema checks not optional to run. The
impediment to doing that is lots of warnings (but not not some
platforms), adding a tool dependency, and validation time.

> > BTW, we do not check %.dtbo, why?
> >
> > At least, 53182e81f47d4ea0c727c49ad23cb782173ab849
> > was trying to check %.dtbo
>
> The commit ef8795f3f1ce ("dt-bindings: kbuild: Use DTB files for
> validation") separated .dtb and .dtbo paths. dt-validate is not
> prepared to be executed on top of the .dtbo file. And this is quite
> logical. The dtbo is an overlay, a patch. So it doesn't have to follow
> the schema on its own. We should probably make sure that multi-dtb
> files generated via fdtoverlay are also validated, but it can go in a
> separate commit.

It needs some attention to see how well validation of overlays works or not.

Rob

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > >
> > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > (and doesn't retrigger the build).
> > > > > > >
> > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > ---
> > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > --- a/scripts/Makefile.lib
> > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > >
> > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > >
> > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > >
> > > > > > I'll give it a few days for other comments.
> > > > >
> > > > >
> > > > >
> > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > You do not need to re-run dtc.
> > > > >
> > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > >  two different things in a single rule.
> > > >
> > > > The issue is that with the current rules the dt-validate isn't
> > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > >
> > > Correct.
> > >
> > > What I said is like this.
> > >
> > > # touch the timestamp file, %.dtb.checked
> > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
>
> Not really a fan of the thousands of files that creates. Maybe if it
> was turned into something useful like a list of schemas that apply to
> the dtb. IOW, a dependency list. That would speed up re-running after
> a schema change. Though if a schema change created new dependencies,
> that wouldn't work.
>
> > >         $(call if_changed_rule,dtb_check)
> > >
> > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > >         $(call if_changed_rule,dtc)
> > >
> > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > >         $(call if_changed_dep,dtc)
> > >
> > >
> > > With the dtc/check split, we can avoid unneeded regeneration of
> > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > changed.
> > >
> > >
> > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > may come back.
> >
> > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > Regenerating dtbs sounds like a minor pain compared to hacking the
> > top-level Makefile again. What I really like is that if one has
> > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > validation.
>
> I lean towards just rebuilding the dtbs. That's pretty quick and
> ensures we get dtc warnings with schema warnings. In the long run, I
> would like to make the schema checks not optional to run. The
> impediment to doing that is lots of warnings (but not not some
> platforms), adding a tool dependency, and validation time.

Rob, Masahiro, do we have any conclusion here? I can change my patch,
but I'd like to understand in which way I should change it.
Fixing/testing yaml changes is a bit painful w/o this change.

-- 
With best wishes
Dmitry

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Masahiro Yamada]

On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov

<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > >
> > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > (and doesn't retrigger the build).
> > > > > > > >
> > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > ---
> > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > > >
> > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > >
> > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > >
> > > > > > > I'll give it a few days for other comments.
> > > > > >
> > > > > >
> > > > > >
> > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > You do not need to re-run dtc.
> > > > > >
> > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > >  two different things in a single rule.
> > > > >
> > > > > The issue is that with the current rules the dt-validate isn't
> > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > >
> > > > Correct.
> > > >
> > > > What I said is like this.
> > > >
> > > > # touch the timestamp file, %.dtb.checked
> > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> >
> > Not really a fan of the thousands of files that creates. Maybe if it
> > was turned into something useful like a list of schemas that apply to
> > the dtb. IOW, a dependency list. That would speed up re-running after
> > a schema change. Though if a schema change created new dependencies,
> > that wouldn't work.
> >
> > > >         $(call if_changed_rule,dtb_check)
> > > >
> > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > >         $(call if_changed_rule,dtc)
> > > >
> > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > >         $(call if_changed_dep,dtc)
> > > >
> > > >
> > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > changed.
> > > >
> > > >
> > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > may come back.
> > >
> > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > top-level Makefile again. What I really like is that if one has
> > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > validation.
> >
> > I lean towards just rebuilding the dtbs. That's pretty quick and
> > ensures we get dtc warnings with schema warnings. In the long run, I
> > would like to make the schema checks not optional to run. The
> > impediment to doing that is lots of warnings (but not not some
> > platforms), adding a tool dependency, and validation time.
>
> Rob, Masahiro, do we have any conclusion here? I can change my patch,
> but I'd like to understand in which way I should change it.
> Fixing/testing yaml changes is a bit painful w/o this change.
>
> --
> With best wishes
> Dmitry




I am fine with the patch as long as you fix DTC/CHECK to DTC.





Another idea is to re-check the schema every time,
like this:



diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 3fb6a99e78c4..9fc9f39fb12e 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -375,13 +375,9 @@ quiet_cmd_dtb_check =      CHECK   $@
       cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u
$(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA
) $@ || true
 endif

-define rule_dtc
-       $(call cmd_and_fixdep,dtc)
-       $(call cmd,dtb_check)
-endef
-
 $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
-       $(call if_changed_rule,dtc)
+       $(call if_changed_dep,dtc)
+       $(call cmd,dtb_check)

 $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
        $(call if_changed_dep,dtc)





Whatever.





--
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
>
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > >
> > > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > > (and doesn't retrigger the build).
> > > > > > > > >
> > > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > > > >
> > > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > > >
> > > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > > >
> > > > > > > > I'll give it a few days for other comments.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > > You do not need to re-run dtc.
> > > > > > >
> > > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > > >  two different things in a single rule.
> > > > > >
> > > > > > The issue is that with the current rules the dt-validate isn't
> > > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > > >
> > > > > Correct.
> > > > >
> > > > > What I said is like this.
> > > > >
> > > > > # touch the timestamp file, %.dtb.checked
> > > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> > >
> > > Not really a fan of the thousands of files that creates. Maybe if it
> > > was turned into something useful like a list of schemas that apply to
> > > the dtb. IOW, a dependency list. That would speed up re-running after
> > > a schema change. Though if a schema change created new dependencies,
> > > that wouldn't work.
> > >
> > > > >         $(call if_changed_rule,dtb_check)
> > > > >
> > > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > > >         $(call if_changed_rule,dtc)
> > > > >
> > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > > >         $(call if_changed_dep,dtc)
> > > > >
> > > > >
> > > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > > changed.
> > > > >
> > > > >
> > > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > > may come back.
> > > >
> > > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > > top-level Makefile again. What I really like is that if one has
> > > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > > validation.
> > >
> > > I lean towards just rebuilding the dtbs. That's pretty quick and
> > > ensures we get dtc warnings with schema warnings. In the long run, I
> > > would like to make the schema checks not optional to run. The
> > > impediment to doing that is lots of warnings (but not not some
> > > platforms), adding a tool dependency, and validation time.
> >
> > Rob, Masahiro, do we have any conclusion here? I can change my patch,
> > but I'd like to understand in which way I should change it.
> > Fixing/testing yaml changes is a bit painful w/o this change.
> >
> > --
> > With best wishes
> > Dmitry
>
>
>
>
> I am fine with the patch as long as you fix DTC/CHECK to DTC.

Ack, I completely forgot about this part. Please excuse me. I'll
change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.

>
> Another idea is to re-check the schema every time,
> like this:

I think this will add the external build dependency. We tried a
similar approach several releases ago and had to revert the change.

> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 3fb6a99e78c4..9fc9f39fb12e 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -375,13 +375,9 @@ quiet_cmd_dtb_check =      CHECK   $@
>        cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u
> $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA
> ) $@ || true
>  endif
>
> -define rule_dtc
> -       $(call cmd_and_fixdep,dtc)
> -       $(call cmd,dtb_check)
> -endef
> -
>  $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> -       $(call if_changed_rule,dtc)
> +       $(call if_changed_dep,dtc)
> +       $(call cmd,dtb_check)
>
>  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>         $(call if_changed_dep,dtc)
>
>

-- 
With best wishes
Dmitry

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Masahiro Yamada]

On Thu, Sep 15, 2022 at 8:40 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
> >
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > > > (and doesn't retrigger the build).
> > > > > > > > > >
> > > > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > > > > >
> > > > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > > > >
> > > > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > > > >
> > > > > > > > > I'll give it a few days for other comments.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > > > You do not need to re-run dtc.
> > > > > > > >
> > > > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > > > >  two different things in a single rule.
> > > > > > >
> > > > > > > The issue is that with the current rules the dt-validate isn't
> > > > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > > > >
> > > > > > Correct.
> > > > > >
> > > > > > What I said is like this.
> > > > > >
> > > > > > # touch the timestamp file, %.dtb.checked
> > > > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> > > >
> > > > Not really a fan of the thousands of files that creates. Maybe if it
> > > > was turned into something useful like a list of schemas that apply to
> > > > the dtb. IOW, a dependency list. That would speed up re-running after
> > > > a schema change. Though if a schema change created new dependencies,
> > > > that wouldn't work.
> > > >
> > > > > >         $(call if_changed_rule,dtb_check)
> > > > > >
> > > > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > > > >         $(call if_changed_rule,dtc)
> > > > > >
> > > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > > > >         $(call if_changed_dep,dtc)
> > > > > >
> > > > > >
> > > > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > > > changed.
> > > > > >
> > > > > >
> > > > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > > > may come back.
> > > > >
> > > > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > > > top-level Makefile again. What I really like is that if one has
> > > > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > > > validation.
> > > >
> > > > I lean towards just rebuilding the dtbs. That's pretty quick and
> > > > ensures we get dtc warnings with schema warnings. In the long run, I
> > > > would like to make the schema checks not optional to run. The
> > > > impediment to doing that is lots of warnings (but not not some
> > > > platforms), adding a tool dependency, and validation time.
> > >
> > > Rob, Masahiro, do we have any conclusion here? I can change my patch,
> > > but I'd like to understand in which way I should change it.
> > > Fixing/testing yaml changes is a bit painful w/o this change.
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> >
> >
> >
> > I am fine with the patch as long as you fix DTC/CHECK to DTC.
>
> Ack, I completely forgot about this part. Please excuse me. I'll
> change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.
>
> >
> > Another idea is to re-check the schema every time,
> > like this:
>
> I think this will add the external build dependency. We tried a
> similar approach several releases ago and had to revert the change.


If you are talking about
75e895343d5a2fcbdf4cb3d31ab7492bd65925f0,
you are completely misunderstanding.

Look at my code closely.
It is how the sparse checker works with C=2.





> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 3fb6a99e78c4..9fc9f39fb12e 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -375,13 +375,9 @@ quiet_cmd_dtb_check =      CHECK   $@
> >        cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u
> > $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA
> > ) $@ || true
> >  endif
> >
> > -define rule_dtc
> > -       $(call cmd_and_fixdep,dtc)
> > -       $(call cmd,dtb_check)
> > -endef
> > -
> >  $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> > -       $(call if_changed_rule,dtc)
> > +       $(call if_changed_dep,dtc)
> > +       $(call cmd,dtb_check)
> >
> >  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> >         $(call if_changed_dep,dtc)
> >
> >
>
> --
> With best wishes
> Dmitry

--
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On Fri, 16 Sept 2022 at 04:10, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Sep 15, 2022 at 8:40 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
> > >
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > > > > (and doesn't retrigger the build).
> > > > > > > > > > >
> > > > > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > > > > > >
> > > > > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > > > > >
> > > > > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > > > > >
> > > > > > > > > > I'll give it a few days for other comments.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > > > > You do not need to re-run dtc.
> > > > > > > > >
> > > > > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > > > > >  two different things in a single rule.
> > > > > > > >
> > > > > > > > The issue is that with the current rules the dt-validate isn't
> > > > > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > > > > >
> > > > > > > Correct.
> > > > > > >
> > > > > > > What I said is like this.
> > > > > > >
> > > > > > > # touch the timestamp file, %.dtb.checked
> > > > > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> > > > >
> > > > > Not really a fan of the thousands of files that creates. Maybe if it
> > > > > was turned into something useful like a list of schemas that apply to
> > > > > the dtb. IOW, a dependency list. That would speed up re-running after
> > > > > a schema change. Though if a schema change created new dependencies,
> > > > > that wouldn't work.
> > > > >
> > > > > > >         $(call if_changed_rule,dtb_check)
> > > > > > >
> > > > > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > > > > >         $(call if_changed_rule,dtc)
> > > > > > >
> > > > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > > > > >         $(call if_changed_dep,dtc)
> > > > > > >
> > > > > > >
> > > > > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > > > > changed.
> > > > > > >
> > > > > > >
> > > > > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > > > > may come back.
> > > > > >
> > > > > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > > > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > > > > top-level Makefile again. What I really like is that if one has
> > > > > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > > > > validation.
> > > > >
> > > > > I lean towards just rebuilding the dtbs. That's pretty quick and
> > > > > ensures we get dtc warnings with schema warnings. In the long run, I
> > > > > would like to make the schema checks not optional to run. The
> > > > > impediment to doing that is lots of warnings (but not not some
> > > > > platforms), adding a tool dependency, and validation time.
> > > >
> > > > Rob, Masahiro, do we have any conclusion here? I can change my patch,
> > > > but I'd like to understand in which way I should change it.
> > > > Fixing/testing yaml changes is a bit painful w/o this change.
> > > >
> > > > --
> > > > With best wishes
> > > > Dmitry
> > >
> > >
> > >
> > >
> > > I am fine with the patch as long as you fix DTC/CHECK to DTC.
> >
> > Ack, I completely forgot about this part. Please excuse me. I'll
> > change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.
> >
> > >
> > > Another idea is to re-check the schema every time,
> > > like this:
> >
> > I think this will add the external build dependency. We tried a
> > similar approach several releases ago and had to revert the change.
>
>
> If you are talking about
> 75e895343d5a2fcbdf4cb3d31ab7492bd65925f0,
> you are completely misunderstanding.
>
> Look at my code closely.
> It is how the sparse checker works with C=2.

Ah, I see. Then I'm fine with either your patch or mine v2 at your
(and Rob's) preference.

> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index 3fb6a99e78c4..9fc9f39fb12e 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -375,13 +375,9 @@ quiet_cmd_dtb_check =      CHECK   $@
> > >        cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u
> > > $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA
> > > ) $@ || true
> > >  endif
> > >
> > > -define rule_dtc
> > > -       $(call cmd_and_fixdep,dtc)
> > > -       $(call cmd,dtb_check)
> > > -endef
> > > -
> > >  $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> > > -       $(call if_changed_rule,dtc)
> > > +       $(call if_changed_dep,dtc)
> > > +       $(call cmd,dtb_check)
> > >
> > >  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > >         $(call if_changed_dep,dtc)
> > >
> > >


-- 
With best wishes
Dmitry

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Rob Herring]

On Fri, Sep 16, 2022 at 9:21 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 16 Sept 2022 at 04:10, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Thu, Sep 15, 2022 at 8:40 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
> > > >
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > > > > > (and doesn't retrigger the build).
> > > > > > > > > > > >
> > > > > > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > > > > > @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
> > > > > > > > > > > >  DT_BINDING_DIR := Documentation/devicetree/bindings
> > > > > > > > > > > >  DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
> > > > > > > > > > > >
> > > > > > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > > > > > >
> > > > > > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > > > > > >
> > > > > > > > > > > I'll give it a few days for other comments.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > > > > > You do not need to re-run dtc.
> > > > > > > > > >
> > > > > > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > > > > > >  two different things in a single rule.
> > > > > > > > >
> > > > > > > > > The issue is that with the current rules the dt-validate isn't
> > > > > > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > > > > > >
> > > > > > > > Correct.
> > > > > > > >
> > > > > > > > What I said is like this.
> > > > > > > >
> > > > > > > > # touch the timestamp file, %.dtb.checked
> > > > > > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> > > > > >
> > > > > > Not really a fan of the thousands of files that creates. Maybe if it
> > > > > > was turned into something useful like a list of schemas that apply to
> > > > > > the dtb. IOW, a dependency list. That would speed up re-running after
> > > > > > a schema change. Though if a schema change created new dependencies,
> > > > > > that wouldn't work.
> > > > > >
> > > > > > > >         $(call if_changed_rule,dtb_check)
> > > > > > > >
> > > > > > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > > > > > >         $(call if_changed_rule,dtc)
> > > > > > > >
> > > > > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > > > > > >         $(call if_changed_dep,dtc)
> > > > > > > >
> > > > > > > >
> > > > > > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > > > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > > > > > changed.
> > > > > > > >
> > > > > > > >
> > > > > > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > > > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > > > > > may come back.
> > > > > > >
> > > > > > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > > > > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > > > > > top-level Makefile again. What I really like is that if one has
> > > > > > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > > > > > validation.
> > > > > >
> > > > > > I lean towards just rebuilding the dtbs. That's pretty quick and
> > > > > > ensures we get dtc warnings with schema warnings. In the long run, I
> > > > > > would like to make the schema checks not optional to run. The
> > > > > > impediment to doing that is lots of warnings (but not not some
> > > > > > platforms), adding a tool dependency, and validation time.
> > > > >
> > > > > Rob, Masahiro, do we have any conclusion here? I can change my patch,
> > > > > but I'd like to understand in which way I should change it.
> > > > > Fixing/testing yaml changes is a bit painful w/o this change.
> > > > >
> > > > > --
> > > > > With best wishes
> > > > > Dmitry
> > > >
> > > >
> > > >
> > > >
> > > > I am fine with the patch as long as you fix DTC/CHECK to DTC.
> > >
> > > Ack, I completely forgot about this part. Please excuse me. I'll
> > > change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.
> > >
> > > >
> > > > Another idea is to re-check the schema every time,
> > > > like this:
> > >
> > > I think this will add the external build dependency. We tried a
> > > similar approach several releases ago and had to revert the change.
> >
> >
> > If you are talking about
> > 75e895343d5a2fcbdf4cb3d31ab7492bd65925f0,
> > you are completely misunderstanding.
> >
> > Look at my code closely.
> > It is how the sparse checker works with C=2.
>
> Ah, I see. Then I'm fine with either your patch or mine v2 at your
> (and Rob's) preference.

I'd rather not recheck every time. The user can remove the dtbs if
they want to do that.

Rob

Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while checking dtbs

[Dmitry Baryshkov]

On 16/09/2022 17:54, Rob Herring wrote:
> On Fri, Sep 16, 2022 at 9:21 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Fri, 16 Sept 2022 at 04:10, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>
>>> On Thu, Sep 15, 2022 at 8:40 PM Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>>
>>>>> On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
>>>>>
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>
>>>>>> On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@kernel.org> wrote:
>>>>>>>
>>>>>>> On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
>>>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>>>>>>
>>>>>>>>> On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
>>>>>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
>>>>>>>>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> It is useful to be able to recheck dtbs files against a limited set of
>>>>>>>>>>>>> DT schema files. This can be accomplished by using differnt
>>>>>>>>>>>>> DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
>>>>>>>>>>>>> for some reason if_changed_rule doesn't pick up the rule_dtc changes
>>>>>>>>>>>>> (and doesn't retrigger the build).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
>>>>>>>>>>>>> and dt-validate into a single new command. Then if_changed_dep triggers
>>>>>>>>>>>>> on DT_SCHEMA_FILES changes and reruns the build/check.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>   scripts/Makefile.lib | 14 ++++++--------
>>>>>>>>>>>>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>>>>>>>>>>>>> index c88b98b5dc44..3df470289382 100644
>>>>>>>>>>>>> --- a/scripts/Makefile.lib
>>>>>>>>>>>>> +++ b/scripts/Makefile.lib
>>>>>>>>>>>>> @@ -383,17 +383,15 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
>>>>>>>>>>>>>   DT_BINDING_DIR := Documentation/devicetree/bindings
>>>>>>>>>>>>>   DT_TMP_SCHEMA := $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
>>>>>>>>>>>>>
>>>>>>>>>>>>> -quiet_cmd_dtb_check =  CHECK   $@
>>>>>>>>>>>>> -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
>>>>>>>>>>>>> +quiet_cmd_dtb =        DTC/CHECK   $@
>>>>>>>>>>>>
>>>>>>>>>>>> This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
>>>>>>>>>>>> always do just 'DTC'. I can fixup when applying.
>>>>>>>>>>>>
>>>>>>>>>>>> I'll give it a few days for other comments.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
>>>>>>>>>>> You do not need to re-run dtc.
>>>>>>>>>>>
>>>>>>>>>>> I guess the strangeness comes from the fact that you are trying to do the
>>>>>>>>>>>   two different things in a single rule.
>>>>>>>>>>
>>>>>>>>>> The issue is that with the current rules the dt-validate isn't
>>>>>>>>>> re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
>>>>>>>>>
>>>>>>>>> Correct.
>>>>>>>>>
>>>>>>>>> What I said is like this.
>>>>>>>>>
>>>>>>>>> # touch the timestamp file, %.dtb.checked
>>>>>>>>> $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
>>>>>>>
>>>>>>> Not really a fan of the thousands of files that creates. Maybe if it
>>>>>>> was turned into something useful like a list of schemas that apply to
>>>>>>> the dtb. IOW, a dependency list. That would speed up re-running after
>>>>>>> a schema change. Though if a schema change created new dependencies,
>>>>>>> that wouldn't work.
>>>>>>>
>>>>>>>>>          $(call if_changed_rule,dtb_check)
>>>>>>>>>
>>>>>>>>> $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
>>>>>>>>>          $(call if_changed_rule,dtc)
>>>>>>>>>
>>>>>>>>> $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>>>>>>>>>          $(call if_changed_dep,dtc)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> With the dtc/check split, we can avoid unneeded regeneration of
>>>>>>>>> %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
>>>>>>>>> changed.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> One drawback is we track %.dtb.checked and and %.dtb separately,
>>>>>>>>> so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
>>>>>>>>> may come back.
>>>>>>>>
>>>>>>>> It's up to you and Rob, but I'd really prefer a simpler solution here.
>>>>>>>> Regenerating dtbs sounds like a minor pain compared to hacking the
>>>>>>>> top-level Makefile again. What I really like is that if one has
>>>>>>>> CHECK_DTBS=y (for whatever reason), he can not generate dtb without
>>>>>>>> validation.
>>>>>>>
>>>>>>> I lean towards just rebuilding the dtbs. That's pretty quick and
>>>>>>> ensures we get dtc warnings with schema warnings. In the long run, I
>>>>>>> would like to make the schema checks not optional to run. The
>>>>>>> impediment to doing that is lots of warnings (but not not some
>>>>>>> platforms), adding a tool dependency, and validation time.
>>>>>>
>>>>>> Rob, Masahiro, do we have any conclusion here? I can change my patch,
>>>>>> but I'd like to understand in which way I should change it.
>>>>>> Fixing/testing yaml changes is a bit painful w/o this change.
>>>>>>
>>>>>> --
>>>>>> With best wishes
>>>>>> Dmitry
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I am fine with the patch as long as you fix DTC/CHECK to DTC.
>>>>
>>>> Ack, I completely forgot about this part. Please excuse me. I'll
>>>> change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.
>>>>
>>>>>
>>>>> Another idea is to re-check the schema every time,
>>>>> like this:
>>>>
>>>> I think this will add the external build dependency. We tried a
>>>> similar approach several releases ago and had to revert the change.
>>>
>>>
>>> If you are talking about
>>> 75e895343d5a2fcbdf4cb3d31ab7492bd65925f0,
>>> you are completely misunderstanding.
>>>
>>> Look at my code closely.
>>> It is how the sparse checker works with C=2.
>>
>> Ah, I see. Then I'm fine with either your patch or mine v2 at your
>> (and Rob's) preference.
> 
> I'd rather not recheck every time. The user can remove the dtbs if
> they want to do that.

Then [1] should be the better fit.

[1] 
https://lore.kernel.org/linux-kernel/20220915114422.79378-1-dmitry.baryshkov@linaro.org/

-- 
With best wishes
Dmitry