[PATCH V5 0/5] dt: build overlays

[PATCH V5 0/5] dt: build overlays

[Viresh Kumar]

Hi Frank/Rob,

I have picked all the related patches together into a single patchset,
so they can be properly reviewed/tested.

This patchset makes necessary changes to the kernel to add support for
building overlays (%.dtbo) and the required fdtoverlay tool. This also
builds static_test.dtb using some of the existing overlay tests present
in drivers/of/unittest-data/ for better test coverage.

Note that in order for anyone to test this stuff, you need to manually
run the ./update-dtc-source.sh script once to fetch the necessary
changes from the external DTC project (i.e. fdtoverlay.c and this[1]
patch).

Also note that Frank has already shared his concerns towards the error
reporting done by fdtoverlay tool [2], I have still included the patch
in this series for completeness, will see how to get that sorted out.

V5:

- Don't reuse DTC_SOURCE for fdtoverlay.c in patch 1/5 (Frank).

- Update .gitignore and scripts/Makefile.dtbinst, drop dtbo-y syntax and
  DTC_FLAGS += -@ in patch 4/5 (Masahiro).

- Remove the intermediate dtb, rename output to static_test.dtb, don't
  use overlay.dtb and overlay_base.dtb for static builds, improved
  layout/comments in Makefile for patch 5/5 (Frank).

--
Viresh

[1] https://github.com/dgibson/dtc/commit/163f0469bf2ed8b2fe5aa15bc796b93c70243ddc
[2] https://lore.kernel.org/lkml/74f8aa8f-ffab-3b0f-186f-31fb7395ebbb@gmail.com/

Viresh Kumar (5):
  scripts: dtc: Fetch fdtoverlay.c from external DTC project
  scripts: dtc: Build fdtoverlay tool
  scripts: dtc: Remove the unused fdtdump.c file
  kbuild: Add support to build overlays (%.dtbo)
  of: unittest: Statically apply overlays using fdtoverlay

 .gitignore                        |   3 +-
 Makefile                          |   4 +-
 drivers/of/unittest-data/Makefile |  50 +++++++++
 scripts/Makefile.dtbinst          |   3 +
 scripts/Makefile.lib              |   4 +-
 scripts/dtc/Makefile              |   6 +-
 scripts/dtc/fdtdump.c             | 163 ------------------------------
 scripts/dtc/update-dtc-source.sh  |   3 +-
 8 files changed, 66 insertions(+), 170 deletions(-)
 delete mode 100644 scripts/dtc/fdtdump.c

-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V5 1/5] scripts: dtc: Fetch fdtoverlay.c from external DTC project

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need fdtoverlay tool going forward. Lets start fetching it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/update-dtc-source.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
index bc704e2a6a4a..32ff17ffd089 100755
--- a/scripts/dtc/update-dtc-source.sh
+++ b/scripts/dtc/update-dtc-source.sh
@@ -37,6 +37,7 @@ DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c
 LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
 		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
 		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
+FDTOVERLAY_SOURCE=fdtoverlay.c
 
 get_last_dtc_version() {
 	git log --oneline scripts/dtc/ | grep 'upstream' | head -1 | sed -e 's/^.* \(.*\)/\1/'
@@ -54,7 +55,7 @@ dtc_log=$(git log --oneline ${last_dtc_ver}..)
 
 # Copy the files into the Linux tree
 cd $DTC_LINUX_PATH
-for f in $DTC_SOURCE; do
+for f in $DTC_SOURCE $FDTOVERLAY_SOURCE; do
 	cp ${DTC_UPSTREAM_PATH}/${f} ${f}
 	git add ${f}
 done
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V5 2/5] scripts: dtc: Build fdtoverlay tool

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need fdtoverlay going forward. Lets start building it.

The fdtoverlay program applies (or merges) one or more overlay dtb
blobs to a base dtb blob. The kernel build system would later use
fdtoverlay to generate the overlaid blobs based on platform specific
configurations.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index 4852bf44e913..5f19386a49eb 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -1,13 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0
 # scripts/dtc makefile
 
-hostprogs-always-$(CONFIG_DTC)		+= dtc
+hostprogs-always-$(CONFIG_DTC)		+= dtc fdtoverlay
 hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
 
 dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
 		   srcpos.o checks.o util.o
 dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
 
+libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
+libfdt		= $(addprefix libfdt/,$(libfdt-objs))
+fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
+
 # Source files need to get at the userspace version of libfdt_env.h to compile
 HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
 
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V5 3/5] scripts: dtc: Remove the unused fdtdump.c file

[Viresh Kumar]

This was copied from external DTC repository long back and isn't used
anymore. Over that the dtc tool can be used to generate the dts source
back from the dtb. Remove the unused fdtdump.c file.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/fdtdump.c | 163 ------------------------------------------
 1 file changed, 163 deletions(-)
 delete mode 100644 scripts/dtc/fdtdump.c

diff --git a/scripts/dtc/fdtdump.c b/scripts/dtc/fdtdump.c
deleted file mode 100644
index 7d460a50b513..000000000000
--- a/scripts/dtc/fdtdump.c
+++ /dev/null
@@ -1,163 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
- */
-
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <ctype.h>
-
-#include <fdt.h>
-#include <libfdt_env.h>
-
-#include "util.h"
-
-#define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
-#define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
-#define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
-
-static void print_data(const char *data, int len)
-{
-	int i;
-	const char *p = data;
-
-	/* no data, don't print */
-	if (len == 0)
-		return;
-
-	if (util_is_printable_string(data, len)) {
-		printf(" = \"%s\"", (const char *)data);
-	} else if ((len % 4) == 0) {
-		printf(" = <");
-		for (i = 0; i < len; i += 4)
-			printf("0x%08x%s", fdt32_to_cpu(GET_CELL(p)),
-			       i < (len - 4) ? " " : "");
-		printf(">");
-	} else {
-		printf(" = [");
-		for (i = 0; i < len; i++)
-			printf("%02x%s", *p++, i < len - 1 ? " " : "");
-		printf("]");
-	}
-}
-
-static void dump_blob(void *blob)
-{
-	struct fdt_header *bph = blob;
-	uint32_t off_mem_rsvmap = fdt32_to_cpu(bph->off_mem_rsvmap);
-	uint32_t off_dt = fdt32_to_cpu(bph->off_dt_struct);
-	uint32_t off_str = fdt32_to_cpu(bph->off_dt_strings);
-	struct fdt_reserve_entry *p_rsvmap =
-		(struct fdt_reserve_entry *)((char *)blob + off_mem_rsvmap);
-	const char *p_struct = (const char *)blob + off_dt;
-	const char *p_strings = (const char *)blob + off_str;
-	uint32_t version = fdt32_to_cpu(bph->version);
-	uint32_t totalsize = fdt32_to_cpu(bph->totalsize);
-	uint32_t tag;
-	const char *p, *s, *t;
-	int depth, sz, shift;
-	int i;
-	uint64_t addr, size;
-
-	depth = 0;
-	shift = 4;
-
-	printf("/dts-v1/;\n");
-	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
-	printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
-	printf("// off_dt_struct:\t0x%x\n", off_dt);
-	printf("// off_dt_strings:\t0x%x\n", off_str);
-	printf("// off_mem_rsvmap:\t0x%x\n", off_mem_rsvmap);
-	printf("// version:\t\t%d\n", version);
-	printf("// last_comp_version:\t%d\n",
-	       fdt32_to_cpu(bph->last_comp_version));
-	if (version >= 2)
-		printf("// boot_cpuid_phys:\t0x%x\n",
-		       fdt32_to_cpu(bph->boot_cpuid_phys));
-
-	if (version >= 3)
-		printf("// size_dt_strings:\t0x%x\n",
-		       fdt32_to_cpu(bph->size_dt_strings));
-	if (version >= 17)
-		printf("// size_dt_struct:\t0x%x\n",
-		       fdt32_to_cpu(bph->size_dt_struct));
-	printf("\n");
-
-	for (i = 0; ; i++) {
-		addr = fdt64_to_cpu(p_rsvmap[i].address);
-		size = fdt64_to_cpu(p_rsvmap[i].size);
-		if (addr == 0 && size == 0)
-			break;
-
-		printf("/memreserve/ %llx %llx;\n",
-		       (unsigned long long)addr, (unsigned long long)size);
-	}
-
-	p = p_struct;
-	while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
-
-		/* printf("tag: 0x%08x (%d)\n", tag, p - p_struct); */
-
-		if (tag == FDT_BEGIN_NODE) {
-			s = p;
-			p = PALIGN(p + strlen(s) + 1, 4);
-
-			if (*s == '\0')
-				s = "/";
-
-			printf("%*s%s {\n", depth * shift, "", s);
-
-			depth++;
-			continue;
-		}
-
-		if (tag == FDT_END_NODE) {
-			depth--;
-
-			printf("%*s};\n", depth * shift, "");
-			continue;
-		}
-
-		if (tag == FDT_NOP) {
-			printf("%*s// [NOP]\n", depth * shift, "");
-			continue;
-		}
-
-		if (tag != FDT_PROP) {
-			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n", depth * shift, "", tag);
-			break;
-		}
-		sz = fdt32_to_cpu(GET_CELL(p));
-		s = p_strings + fdt32_to_cpu(GET_CELL(p));
-		if (version < 16 && sz >= 8)
-			p = PALIGN(p, 8);
-		t = p;
-
-		p = PALIGN(p + sz, 4);
-
-		printf("%*s%s", depth * shift, "", s);
-		print_data(t, sz);
-		printf(";\n");
-	}
-}
-
-
-int main(int argc, char *argv[])
-{
-	char *buf;
-
-	if (argc < 2) {
-		fprintf(stderr, "supply input filename\n");
-		return 5;
-	}
-
-	buf = utilfdt_read(argv[1]);
-	if (buf)
-		dump_blob(buf);
-	else
-		return 10;
-
-	return 0;
-}
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Viresh Kumar]

Add support for building DT overlays (%.dtbo). The overlay's source file
will have the usual extension, i.e. .dts, though the blob will have
.dtbo extension to distinguish it from normal blobs.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .gitignore               | 3 +--
 Makefile                 | 4 ++--
 scripts/Makefile.dtbinst | 3 +++
 scripts/Makefile.lib     | 4 +++-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/.gitignore b/.gitignore
index d01cda8e1177..0458c36f3cb2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,8 +17,7 @@
 *.bz2
 *.c.[012]*.*
 *.dt.yaml
-*.dtb
-*.dtb.S
+*.dtb*
 *.dwo
 *.elf
 *.gcno
diff --git a/Makefile b/Makefile
index 9e73f82e0d86..b84f5e0b46ab 100644
--- a/Makefile
+++ b/Makefile
@@ -1334,7 +1334,7 @@ endif
 
 ifneq ($(dtstree),)
 
-%.dtb: include/config/kernel.release scripts_dtc
+%.dtb %.dtbo: include/config/kernel.release scripts_dtc
 	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
 
 PHONY += dtbs dtbs_install dtbs_check
@@ -1816,7 +1816,7 @@ clean: $(clean-dirs)
 	@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 '*.dt.yaml' \
+		-o -name '*.dtb' -o -name '*.dtbo' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
 		-o -name '*.dwo' -o -name '*.lst' \
 		-o -name '*.su' -o -name '*.mod' \
 		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
diff --git a/scripts/Makefile.dtbinst b/scripts/Makefile.dtbinst
index 50d580d77ae9..ba01f5ba2517 100644
--- a/scripts/Makefile.dtbinst
+++ b/scripts/Makefile.dtbinst
@@ -29,6 +29,9 @@ quiet_cmd_dtb_install = INSTALL $@
 $(dst)/%.dtb: $(obj)/%.dtb
 	$(call cmd,dtb_install)
 
+$(dst)/%.dtbo: $(obj)/%.dtbo
+	$(call cmd,dtb_install)
+
 PHONY += $(subdirs)
 $(subdirs):
 	$(Q)$(MAKE) $(dtbinst)=$@ dst=$(patsubst $(obj)/%,$(dst)/%,$@)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 213677a5ed33..30bc0a8e0087 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -86,7 +86,9 @@ extra-$(CONFIG_OF_ALL_DTBS)	+= $(dtb-)
 
 ifneq ($(CHECK_DTBS),)
 extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
+extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))
 extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
+extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))
 endif
 
 # Add subdir path
@@ -324,7 +326,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ;
 		-d $(depfile).dtc.tmp $(dtc-tmp) ; \
 	cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
 
-$(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
+$(obj)/%.dtb $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
 	$(call if_changed_dep,dtc)
 
 DT_CHECKER ?= dt-validate
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

Now that fdtoverlay is part of the kernel build, start using it to test
the unitest overlays we have by applying them statically.

Some unittest overlays deliberately contain errors that unittest checks
for. These overlays will cause fdtoverlay to fail, and are thus not
included in the static_test.dtb.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 009f4045c8e4..ece7dfd5cafa 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@
 
 # suppress warnings about intentional errors
 DTC_FLAGS_testcases += -Wno-interrupts_property
+
+# Apply overlays statically with fdtoverlay.  This is a build time test that
+# the overlays can be applied successfully by fdtoverlay.  This does not
+# guarantee that the overlays can be applied successfully at run time by
+# unittest, but it provides a bit of build time test coverage for those
+# who do not execute unittest.
+#
+# The overlays are applied on top of testcases.dtb to create static_test.dtb
+# If fdtoverlay detects an error than the kernel build will fail.
+# static_test.dtb is not consumed by unittest.
+#
+# Some unittest overlays deliberately contain errors that unittest checks for.
+# These overlays will cause fdtoverlay to fail, and are thus not included
+# in the static test:
+#			overlay.dtb \
+#			overlay_bad_add_dup_node.dtb \
+#			overlay_bad_add_dup_prop.dtb \
+#			overlay_bad_phandle.dtb \
+#			overlay_bad_symbol.dtb \
+#			overlay_base.dtb \
+
+apply_static_overlay := overlay_0.dtb \
+			overlay_1.dtb \
+			overlay_2.dtb \
+			overlay_3.dtb \
+			overlay_4.dtb \
+			overlay_5.dtb \
+			overlay_6.dtb \
+			overlay_7.dtb \
+			overlay_8.dtb \
+			overlay_9.dtb \
+			overlay_10.dtb \
+			overlay_11.dtb \
+			overlay_12.dtb \
+			overlay_13.dtb \
+			overlay_15.dtb \
+			overlay_gpio_01.dtb \
+			overlay_gpio_02a.dtb \
+			overlay_gpio_02b.dtb \
+			overlay_gpio_03.dtb \
+			overlay_gpio_04a.dtb \
+			overlay_gpio_04b.dtb
+
+quiet_cmd_fdtoverlay = FDTOVERLAY $@
+      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
+
+$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
+	$(call if_changed,fdtoverlay)
+
+always-$(CONFIG_OF_OVERLAY) += static_test.dtb
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Masahiro Yamada]

On Wed, Jan 20, 2021 at 4:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Add support for building DT overlays (%.dtbo). The overlay's source file
> will have the usual extension, i.e. .dts, though the blob will have
> .dtbo extension to distinguish it from normal blobs.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  .gitignore               | 3 +--
>  Makefile                 | 4 ++--
>  scripts/Makefile.dtbinst | 3 +++
>  scripts/Makefile.lib     | 4 +++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index d01cda8e1177..0458c36f3cb2 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -17,8 +17,7 @@
>  *.bz2
>  *.c.[012]*.*
>  *.dt.yaml
> -*.dtb
> -*.dtb.S
> +*.dtb*


Personally, I prefer adding .dtbo explicitly


>  *.dwo
>  *.elf
>  *.gcno
> diff --git a/Makefile b/Makefile
> index 9e73f82e0d86..b84f5e0b46ab 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1334,7 +1334,7 @@ endif
>
>  ifneq ($(dtstree),)
>
> -%.dtb: include/config/kernel.release scripts_dtc
> +%.dtb %.dtbo: include/config/kernel.release scripts_dtc
>         $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@


No, this is wrong because it does not work
as grouped targets.

You need to separate them.



%.dtb: include/config/kernel.release scripts_dtc
         $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@

%.dtbo: include/config/kernel.release scripts_dtc
         $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@




See GNU make manual.


"Pattern rules may have more than one target; however, every target
must contain a % character.
Pattern rules are always treated as grouped targets"

https://www.gnu.org/software/make/manual/html_node/Pattern-Intro.html





>  PHONY += dtbs dtbs_install dtbs_check
> @@ -1816,7 +1816,7 @@ clean: $(clean-dirs)
>         @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 '*.dt.yaml' \
> +               -o -name '*.dtb' -o -name '*.dtbo' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
>                 -o -name '*.dwo' -o -name '*.lst' \
>                 -o -name '*.su' -o -name '*.mod' \
>                 -o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
> diff --git a/scripts/Makefile.dtbinst b/scripts/Makefile.dtbinst
> index 50d580d77ae9..ba01f5ba2517 100644
> --- a/scripts/Makefile.dtbinst
> +++ b/scripts/Makefile.dtbinst
> @@ -29,6 +29,9 @@ quiet_cmd_dtb_install = INSTALL $@
>  $(dst)/%.dtb: $(obj)/%.dtb
>         $(call cmd,dtb_install)
>
> +$(dst)/%.dtbo: $(obj)/%.dtbo
> +       $(call cmd,dtb_install)
> +
>  PHONY += $(subdirs)
>  $(subdirs):
>         $(Q)$(MAKE) $(dtbinst)=$@ dst=$(patsubst $(obj)/%,$(dst)/%,$@)
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 213677a5ed33..30bc0a8e0087 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -86,7 +86,9 @@ extra-$(CONFIG_OF_ALL_DTBS)   += $(dtb-)
>
>  ifneq ($(CHECK_DTBS),)
>  extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
> +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))
>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> +extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))
>  endif
>
>  # Add subdir path
> @@ -324,7 +326,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ;
>                 -d $(depfile).dtc.tmp $(dtc-tmp) ; \
>         cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
>
> -$(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> +$(obj)/%.dtb $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>         $(call if_changed_dep,dtc)


Same here.

You need to duplicate the rules everywhere, unfortunately.







>  DT_CHECKER ?= dt-validate
> --
> 2.25.0.rc1.19.g042ed3e048af
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Viresh Kumar]

On 20-01-21, 17:58, Masahiro Yamada wrote:
> > +%.dtb %.dtbo: include/config/kernel.release scripts_dtc
> >         $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> 
> 
> No, this is wrong because it does not work
> as grouped targets.
> 
> You need to separate them.
> 
> 
> 
> %.dtb: include/config/kernel.release scripts_dtc
>          $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> 
> %.dtbo: include/config/kernel.release scripts_dtc
>          $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> 
> 
> 
> 
> See GNU make manual.
> 
> 
> "Pattern rules may have more than one target; however, every target
> must contain a % character.
> Pattern rules are always treated as grouped targets"
> 
> https://www.gnu.org/software/make/manual/html_node/Pattern-Intro.html

Hmm, okay I will do that.

I did it this way because I saw similar stuff at some other places. I
am not a regular Makefile hacker, there is every chance I am reading
it wrong.

$ git grep "%.*%.*:" | grep Makefile
Makefile:%/config/auto.conf %/config/auto.conf.cmd %/generated/autoconf.h: $(KCONFIG_CONFIG)
scripts/Makefile.build:$(obj)/%.asn1.c $(obj)/%.asn1.h: $(src)/%.asn1 $(objtree)/scripts/asn1_compiler
scripts/Makefile.host:$(obj)/%.tab.c $(obj)/%.tab.h: $(src)/%.y FORCE
scripts/genksyms/Makefile:$(obj)/pars%.tab.c $(obj)/pars%.tab.h: $(src)/pars%.y FORCE
tools/perf/Documentation/Makefile:$(OUTPUT)%.1 $(OUTPUT)%.5 $(OUTPUT)%.7 : %.txt
tools/perf/Documentation/Makefile:$(OUTPUT)%.1 $(OUTPUT)%.5 $(OUTPUT)%.7 : $(OUTPUT)%.xml

-- 
viresh

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Masahiro Yamada]

On Wed, Jan 20, 2021 at 6:55 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 20-01-21, 17:58, Masahiro Yamada wrote:
> > > +%.dtb %.dtbo: include/config/kernel.release scripts_dtc
> > >         $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> >
> >
> > No, this is wrong because it does not work
> > as grouped targets.
> >
> > You need to separate them.
> >
> >
> >
> > %.dtb: include/config/kernel.release scripts_dtc
> >          $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> >
> > %.dtbo: include/config/kernel.release scripts_dtc
> >          $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> >
> >
> >
> >
> > See GNU make manual.
> >
> >
> > "Pattern rules may have more than one target; however, every target
> > must contain a % character.
> > Pattern rules are always treated as grouped targets"
> >
> > https://www.gnu.org/software/make/manual/html_node/Pattern-Intro.html
>
> Hmm, okay I will do that.
>
> I did it this way because I saw similar stuff at some other places. I
> am not a regular Makefile hacker, there is every chance I am reading
> it wrong.
>


In grouped targets, a recipe must be able to create
all the targets in a single invocation.



> $ git grep "%.*%.*:" | grep Makefile
> Makefile:%/config/auto.conf %/config/auto.conf.cmd %/generated/autoconf.h: $(KCONFIG_CONFIG)

One single invocation of Kconfig generates
three files, auto.conf, auto.conf.cmd, autoconf.h

So, this is correct.


> scripts/Makefile.build:$(obj)/%.asn1.c $(obj)/%.asn1.h: $(src)/%.asn1 $(objtree)/scripts/asn1_compiler

asn1_compiler takes one source file,
then outputs two files %.asn1.c and %.asn1.h

So, this is correct.


> scripts/Makefile.host:$(obj)/%.tab.c $(obj)/%.tab.h: $(src)/%.y FORCE

bison takes one source file,
then outputs two files %.tab.c and %.tab.h

So, this is correct.


> scripts/genksyms/Makefile:$(obj)/pars%.tab.c $(obj)/pars%.tab.h: $(src)/pars%.y FORCE

This is also a bison rule. Correct.


> tools/perf/Documentation/Makefile:$(OUTPUT)%.1 $(OUTPUT)%.5 $(OUTPUT)%.7 : %.txt
> tools/perf/Documentation/Makefile:$(OUTPUT)%.1 $(OUTPUT)%.5 $(OUTPUT)%.7 : $(OUTPUT)%.xml


These are out of scope of my maintenance coverage.
I do not know whether they are correct or not.



The DTC rule takes one source input, and one output file.

It cannot generate .dtb and .dtbo at the same time.

That is why a grouped target does not fit here.



> --
> viresh



-- 
Best Regards
Masahiro Yamada

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Viresh Kumar]

On 20-01-21, 20:04, Masahiro Yamada wrote:
> The DTC rule takes one source input, and one output file.
> 
> It cannot generate .dtb and .dtbo at the same time.
> 
> That is why a grouped target does not fit here.

Okay, thanks for taking time to explain this. Will fix this in the
next version.

-- 
viresh

Re: [PATCH V5 0/5] dt: build overlays

[Rob Herring]

On Wed, Jan 20, 2021 at 1:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hi Frank/Rob,
>
> I have picked all the related patches together into a single patchset,
> so they can be properly reviewed/tested.
>
> This patchset makes necessary changes to the kernel to add support for
> building overlays (%.dtbo) and the required fdtoverlay tool. This also
> builds static_test.dtb using some of the existing overlay tests present
> in drivers/of/unittest-data/ for better test coverage.
>
> Note that in order for anyone to test this stuff, you need to manually
> run the ./update-dtc-source.sh script once to fetch the necessary
> changes from the external DTC project (i.e. fdtoverlay.c and this[1]
> patch).

Do we need a fdtoverlay fix for applying root node changes?

Rob

Re: [PATCH V5 3/5] scripts: dtc: Remove the unused fdtdump.c file

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5383 bytes --]

On Wed, Jan 20, 2021 at 12:36:45PM +0530, Viresh Kumar wrote:
> This was copied from external DTC repository long back and isn't used
> anymore. Over that the dtc tool can be used to generate the dts source
> back from the dtb. Remove the unused fdtdump.c file.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Doesn't this make updating the kernel dtc from upstream needlessly
more difficult?

> ---
>  scripts/dtc/fdtdump.c | 163 ------------------------------------------
>  1 file changed, 163 deletions(-)
>  delete mode 100644 scripts/dtc/fdtdump.c
> 
> diff --git a/scripts/dtc/fdtdump.c b/scripts/dtc/fdtdump.c
> deleted file mode 100644
> index 7d460a50b513..000000000000
> --- a/scripts/dtc/fdtdump.c
> +++ /dev/null
> @@ -1,163 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * fdtdump.c - Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> - */
> -
> -#include <stdint.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <ctype.h>
> -
> -#include <fdt.h>
> -#include <libfdt_env.h>
> -
> -#include "util.h"
> -
> -#define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
> -#define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
> -#define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
> -
> -static void print_data(const char *data, int len)
> -{
> -	int i;
> -	const char *p = data;
> -
> -	/* no data, don't print */
> -	if (len == 0)
> -		return;
> -
> -	if (util_is_printable_string(data, len)) {
> -		printf(" = \"%s\"", (const char *)data);
> -	} else if ((len % 4) == 0) {
> -		printf(" = <");
> -		for (i = 0; i < len; i += 4)
> -			printf("0x%08x%s", fdt32_to_cpu(GET_CELL(p)),
> -			       i < (len - 4) ? " " : "");
> -		printf(">");
> -	} else {
> -		printf(" = [");
> -		for (i = 0; i < len; i++)
> -			printf("%02x%s", *p++, i < len - 1 ? " " : "");
> -		printf("]");
> -	}
> -}
> -
> -static void dump_blob(void *blob)
> -{
> -	struct fdt_header *bph = blob;
> -	uint32_t off_mem_rsvmap = fdt32_to_cpu(bph->off_mem_rsvmap);
> -	uint32_t off_dt = fdt32_to_cpu(bph->off_dt_struct);
> -	uint32_t off_str = fdt32_to_cpu(bph->off_dt_strings);
> -	struct fdt_reserve_entry *p_rsvmap =
> -		(struct fdt_reserve_entry *)((char *)blob + off_mem_rsvmap);
> -	const char *p_struct = (const char *)blob + off_dt;
> -	const char *p_strings = (const char *)blob + off_str;
> -	uint32_t version = fdt32_to_cpu(bph->version);
> -	uint32_t totalsize = fdt32_to_cpu(bph->totalsize);
> -	uint32_t tag;
> -	const char *p, *s, *t;
> -	int depth, sz, shift;
> -	int i;
> -	uint64_t addr, size;
> -
> -	depth = 0;
> -	shift = 4;
> -
> -	printf("/dts-v1/;\n");
> -	printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
> -	printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
> -	printf("// off_dt_struct:\t0x%x\n", off_dt);
> -	printf("// off_dt_strings:\t0x%x\n", off_str);
> -	printf("// off_mem_rsvmap:\t0x%x\n", off_mem_rsvmap);
> -	printf("// version:\t\t%d\n", version);
> -	printf("// last_comp_version:\t%d\n",
> -	       fdt32_to_cpu(bph->last_comp_version));
> -	if (version >= 2)
> -		printf("// boot_cpuid_phys:\t0x%x\n",
> -		       fdt32_to_cpu(bph->boot_cpuid_phys));
> -
> -	if (version >= 3)
> -		printf("// size_dt_strings:\t0x%x\n",
> -		       fdt32_to_cpu(bph->size_dt_strings));
> -	if (version >= 17)
> -		printf("// size_dt_struct:\t0x%x\n",
> -		       fdt32_to_cpu(bph->size_dt_struct));
> -	printf("\n");
> -
> -	for (i = 0; ; i++) {
> -		addr = fdt64_to_cpu(p_rsvmap[i].address);
> -		size = fdt64_to_cpu(p_rsvmap[i].size);
> -		if (addr == 0 && size == 0)
> -			break;
> -
> -		printf("/memreserve/ %llx %llx;\n",
> -		       (unsigned long long)addr, (unsigned long long)size);
> -	}
> -
> -	p = p_struct;
> -	while ((tag = fdt32_to_cpu(GET_CELL(p))) != FDT_END) {
> -
> -		/* printf("tag: 0x%08x (%d)\n", tag, p - p_struct); */
> -
> -		if (tag == FDT_BEGIN_NODE) {
> -			s = p;
> -			p = PALIGN(p + strlen(s) + 1, 4);
> -
> -			if (*s == '\0')
> -				s = "/";
> -
> -			printf("%*s%s {\n", depth * shift, "", s);
> -
> -			depth++;
> -			continue;
> -		}
> -
> -		if (tag == FDT_END_NODE) {
> -			depth--;
> -
> -			printf("%*s};\n", depth * shift, "");
> -			continue;
> -		}
> -
> -		if (tag == FDT_NOP) {
> -			printf("%*s// [NOP]\n", depth * shift, "");
> -			continue;
> -		}
> -
> -		if (tag != FDT_PROP) {
> -			fprintf(stderr, "%*s ** Unknown tag 0x%08x\n", depth * shift, "", tag);
> -			break;
> -		}
> -		sz = fdt32_to_cpu(GET_CELL(p));
> -		s = p_strings + fdt32_to_cpu(GET_CELL(p));
> -		if (version < 16 && sz >= 8)
> -			p = PALIGN(p, 8);
> -		t = p;
> -
> -		p = PALIGN(p + sz, 4);
> -
> -		printf("%*s%s", depth * shift, "", s);
> -		print_data(t, sz);
> -		printf(";\n");
> -	}
> -}
> -
> -
> -int main(int argc, char *argv[])
> -{
> -	char *buf;
> -
> -	if (argc < 2) {
> -		fprintf(stderr, "supply input filename\n");
> -		return 5;
> -	}
> -
> -	buf = utilfdt_read(argv[1]);
> -	if (buf)
> -		dump_blob(buf);
> -	else
> -		return 10;
> -
> -	return 0;
> -}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

On Wed, Jan 20, 2021 at 12:36:46PM +0530, Viresh Kumar wrote:
> Add support for building DT overlays (%.dtbo). The overlay's source file
> will have the usual extension, i.e. .dts, though the blob will have
> .dtbo extension to distinguish it from normal blobs.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  .gitignore               | 3 +--
>  Makefile                 | 4 ++--
>  scripts/Makefile.dtbinst | 3 +++
>  scripts/Makefile.lib     | 4 +++-
>  4 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index d01cda8e1177..0458c36f3cb2 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -17,8 +17,7 @@
>  *.bz2
>  *.c.[012]*.*
>  *.dt.yaml
> -*.dtb
> -*.dtb.S
> +*.dtb*
>  *.dwo
>  *.elf
>  *.gcno
> diff --git a/Makefile b/Makefile
> index 9e73f82e0d86..b84f5e0b46ab 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1334,7 +1334,7 @@ endif
>  
>  ifneq ($(dtstree),)
>  
> -%.dtb: include/config/kernel.release scripts_dtc
> +%.dtb %.dtbo: include/config/kernel.release scripts_dtc
>  	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
>  
>  PHONY += dtbs dtbs_install dtbs_check
> @@ -1816,7 +1816,7 @@ clean: $(clean-dirs)
>  	@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 '*.dt.yaml' \
> +		-o -name '*.dtb' -o -name '*.dtbo' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
>  		-o -name '*.dwo' -o -name '*.lst' \
>  		-o -name '*.su' -o -name '*.mod' \
>  		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
> diff --git a/scripts/Makefile.dtbinst b/scripts/Makefile.dtbinst
> index 50d580d77ae9..ba01f5ba2517 100644
> --- a/scripts/Makefile.dtbinst
> +++ b/scripts/Makefile.dtbinst
> @@ -29,6 +29,9 @@ quiet_cmd_dtb_install = INSTALL $@
>  $(dst)/%.dtb: $(obj)/%.dtb
>  	$(call cmd,dtb_install)
>  
> +$(dst)/%.dtbo: $(obj)/%.dtbo
> +	$(call cmd,dtb_install)
> +
>  PHONY += $(subdirs)
>  $(subdirs):
>  	$(Q)$(MAKE) $(dtbinst)=$@ dst=$(patsubst $(obj)/%,$(dst)/%,$@)
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 213677a5ed33..30bc0a8e0087 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -86,7 +86,9 @@ extra-$(CONFIG_OF_ALL_DTBS)	+= $(dtb-)
>  
>  ifneq ($(CHECK_DTBS),)
>  extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
> +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))
>  extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> +extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))
>  endif
>  
>  # Add subdir path
> @@ -324,7 +326,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ;
>  		-d $(depfile).dtc.tmp $(dtc-tmp) ; \
>  	cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
>  
> -$(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> +$(obj)/%.dtb $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>  	$(call if_changed_dep,dtc)

If you're using overlays, you probably need the -@ flag, for both the
base file and the overlays, which AFAICT is not already the case.

>  
>  DT_CHECKER ?= dt-validate

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]

On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:
> Now that fdtoverlay is part of the kernel build, start using it to test
> the unitest overlays we have by applying them statically.
> 
> Some unittest overlays deliberately contain errors that unittest checks
> for. These overlays will cause fdtoverlay to fail, and are thus not
> included in the static_test.dtb.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 009f4045c8e4..ece7dfd5cafa 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@
>  
>  # suppress warnings about intentional errors
>  DTC_FLAGS_testcases += -Wno-interrupts_property
> +
> +# Apply overlays statically with fdtoverlay.  This is a build time test that
> +# the overlays can be applied successfully by fdtoverlay.  This does not
> +# guarantee that the overlays can be applied successfully at run time by
> +# unittest, but it provides a bit of build time test coverage for those
> +# who do not execute unittest.
> +#
> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
> +# If fdtoverlay detects an error than the kernel build will fail.
> +# static_test.dtb is not consumed by unittest.
> +#
> +# Some unittest overlays deliberately contain errors that unittest checks for.
> +# These overlays will cause fdtoverlay to fail, and are thus not included
> +# in the static test:
> +#			overlay.dtb \
> +#			overlay_bad_add_dup_node.dtb \
> +#			overlay_bad_add_dup_prop.dtb \
> +#			overlay_bad_phandle.dtb \
> +#			overlay_bad_symbol.dtb \
> +#			overlay_base.dtb \
> +
> +apply_static_overlay := overlay_0.dtb \
> +			overlay_1.dtb \
> +			overlay_2.dtb \
> +			overlay_3.dtb \
> +			overlay_4.dtb \
> +			overlay_5.dtb \
> +			overlay_6.dtb \
> +			overlay_7.dtb \
> +			overlay_8.dtb \
> +			overlay_9.dtb \
> +			overlay_10.dtb \
> +			overlay_11.dtb \
> +			overlay_12.dtb \
> +			overlay_13.dtb \
> +			overlay_15.dtb \
> +			overlay_gpio_01.dtb \
> +			overlay_gpio_02a.dtb \
> +			overlay_gpio_02b.dtb \
> +			overlay_gpio_03.dtb \
> +			overlay_gpio_04a.dtb \
> +			overlay_gpio_04b.dtb
> +
> +quiet_cmd_fdtoverlay = FDTOVERLAY $@
> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
> +
> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
> +	$(call if_changed,fdtoverlay)
> +
> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb

The fact that testcases.dts includes /plugin/ still seems completely
wrong, if it's being used as the base tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Viresh Kumar]

On 21-01-21, 11:49, David Gibson wrote:
> If you're using overlays, you probably need the -@ flag, for both the
> base file and the overlays, which AFAICT is not already the case.

I think the idea was to do that in the platform specific Makefiles,
unless I have misunderstood that from earlier discussions. So a
platform may want to do that per-file or just enable it for the entire
platform.

-- 
viresh

Re: [PATCH V5 0/5] dt: build overlays

[Viresh Kumar]

On 20-01-21, 09:43, Rob Herring wrote:
> On Wed, Jan 20, 2021 at 1:07 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Hi Frank/Rob,
> >
> > I have picked all the related patches together into a single patchset,
> > so they can be properly reviewed/tested.
> >
> > This patchset makes necessary changes to the kernel to add support for
> > building overlays (%.dtbo) and the required fdtoverlay tool. This also
> > builds static_test.dtb using some of the existing overlay tests present
> > in drivers/of/unittest-data/ for better test coverage.
> >
> > Note that in order for anyone to test this stuff, you need to manually
> > run the ./update-dtc-source.sh script once to fetch the necessary
> > changes from the external DTC project (i.e. fdtoverlay.c and this[1]
> > patch).
> 
> Do we need a fdtoverlay fix for applying root node changes?

I have dropped the overlay files which were updating the root-node as
it looks like it shouldn't be done. Frank suggested (in his patch) to
drop overlay.dtb and I dropped overlay_base.dtb as well.

With that no other fixes are required.

-- 
viresh

Re: [PATCH V5 3/5] scripts: dtc: Remove the unused fdtdump.c file

[Viresh Kumar]

On 21-01-21, 11:44, David Gibson wrote:
> On Wed, Jan 20, 2021 at 12:36:45PM +0530, Viresh Kumar wrote:
> > This was copied from external DTC repository long back and isn't used
> > anymore. Over that the dtc tool can be used to generate the dts source
> > back from the dtb. Remove the unused fdtdump.c file.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Doesn't this make updating the kernel dtc from upstream needlessly
> more difficult?

Hmm, I am not sure I understand the concern well. The kernel keeps a
list of files[1] it needs to automatically copy (using a script) from
the upstream dtc repo and fdtdump.c was never part of that. Keeping it
there isn't going to make any difficulty I believe.

-- 
viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/dtc/update-dtc-source.sh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

Hi David,

On 1/20/21 6:51 PM, David Gibson wrote:
> On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:
>> Now that fdtoverlay is part of the kernel build, start using it to test
>> the unitest overlays we have by applying them statically.
>>
>> Some unittest overlays deliberately contain errors that unittest checks
>> for. These overlays will cause fdtoverlay to fail, and are thus not
>> included in the static_test.dtb.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index 009f4045c8e4..ece7dfd5cafa 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@
>>  
>>  # suppress warnings about intentional errors
>>  DTC_FLAGS_testcases += -Wno-interrupts_property
>> +
>> +# Apply overlays statically with fdtoverlay.  This is a build time test that
>> +# the overlays can be applied successfully by fdtoverlay.  This does not
>> +# guarantee that the overlays can be applied successfully at run time by
>> +# unittest, but it provides a bit of build time test coverage for those
>> +# who do not execute unittest.
>> +#
>> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
>> +# If fdtoverlay detects an error than the kernel build will fail.
>> +# static_test.dtb is not consumed by unittest.
>> +#
>> +# Some unittest overlays deliberately contain errors that unittest checks for.
>> +# These overlays will cause fdtoverlay to fail, and are thus not included
>> +# in the static test:
>> +#			overlay.dtb \
>> +#			overlay_bad_add_dup_node.dtb \
>> +#			overlay_bad_add_dup_prop.dtb \
>> +#			overlay_bad_phandle.dtb \
>> +#			overlay_bad_symbol.dtb \
>> +#			overlay_base.dtb \
>> +
>> +apply_static_overlay := overlay_0.dtb \
>> +			overlay_1.dtb \
>> +			overlay_2.dtb \
>> +			overlay_3.dtb \
>> +			overlay_4.dtb \
>> +			overlay_5.dtb \
>> +			overlay_6.dtb \
>> +			overlay_7.dtb \
>> +			overlay_8.dtb \
>> +			overlay_9.dtb \
>> +			overlay_10.dtb \
>> +			overlay_11.dtb \
>> +			overlay_12.dtb \
>> +			overlay_13.dtb \
>> +			overlay_15.dtb \
>> +			overlay_gpio_01.dtb \
>> +			overlay_gpio_02a.dtb \
>> +			overlay_gpio_02b.dtb \
>> +			overlay_gpio_03.dtb \
>> +			overlay_gpio_04a.dtb \
>> +			overlay_gpio_04b.dtb
>> +
>> +quiet_cmd_fdtoverlay = FDTOVERLAY $@
>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>> +
>> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
>> +	$(call if_changed,fdtoverlay)
>> +
>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb
> 
> The fact that testcases.dts includes /plugin/ still seems completely
> wrong, if it's being used as the base tree.
> 

Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT.
It is a convenient FDT to use because it provides the frame that the overlays
require to be applied.  It is fortunate that fdtoverlay does not reject the use
of an FDT with overlay metadata as the base blob.

If Viresh wants to test a more realistic data set then he could create a build
rule that copies testcases.dts into (for example) testcases_base.dts, strip
out the '/plugin/;" then compile that into testcases_base.dtb and use that for
fdtoverlay.

   pseudo makefile rule for testcases_base.dts:
       sed -e 's|/plugin/;||' > testcases_base.dts

   add testcases_base.dtb to the list of objects to build

   change the rule for static_test.dtb to use testcases_base.dtb instead of
   testcases.dtb

This is probably a good idea instead of depending on the leniency of fdtoverlay.

-Frank

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 20-01-21, 23:14, Frank Rowand wrote:
> It is a convenient FDT to use because it provides the frame that the overlays
> require to be applied.  It is fortunate that fdtoverlay does not reject the use
> of an FDT with overlay metadata as the base blob.

> This is probably a good idea instead of depending on the leniency of fdtoverlay.

I believe fdtoverlay allows that intentionally, that would be required
for the cases where we have a hierarchy of extension boards or
overlays.

A platform can have a base dtb (with /plugin/;), then we can have an
overlay (1) for an extension board (with /plugin/;) and then an
overlay (2) for an extension board for the previous extension board.

In such a case overlay-(2) can't be applied directly to the base dtb
as it may not find all the nodes it is trying to update. And so
overlay-(2) needs to be applied to overlay-(1) and then the output of
this can be applied to the base dtb.

This is very similar to what I tried with the intermediate.dtb
earlier.

-- 
viresh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

Hi Viresh,

On 1/20/21 11:14 PM, Frank Rowand wrote:
> Hi David,
> 
> On 1/20/21 6:51 PM, David Gibson wrote:
>> On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:
>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>> the unitest overlays we have by applying them statically.
>>>
>>> Some unittest overlays deliberately contain errors that unittest checks
>>> for. These overlays will cause fdtoverlay to fail, and are thus not
>>> included in the static_test.dtb.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
>>>  1 file changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>>> index 009f4045c8e4..ece7dfd5cafa 100644
>>> --- a/drivers/of/unittest-data/Makefile
>>> +++ b/drivers/of/unittest-data/Makefile
>>> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@
>>>  
>>>  # suppress warnings about intentional errors
>>>  DTC_FLAGS_testcases += -Wno-interrupts_property
>>> +
>>> +# Apply overlays statically with fdtoverlay.  This is a build time test that
>>> +# the overlays can be applied successfully by fdtoverlay.  This does not
>>> +# guarantee that the overlays can be applied successfully at run time by
>>> +# unittest, but it provides a bit of build time test coverage for those
>>> +# who do not execute unittest.
>>> +#
>>> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
>>> +# If fdtoverlay detects an error than the kernel build will fail.
>>> +# static_test.dtb is not consumed by unittest.
>>> +#
>>> +# Some unittest overlays deliberately contain errors that unittest checks for.
>>> +# These overlays will cause fdtoverlay to fail, and are thus not included
>>> +# in the static test:
>>> +#			overlay.dtb \
>>> +#			overlay_bad_add_dup_node.dtb \
>>> +#			overlay_bad_add_dup_prop.dtb \
>>> +#			overlay_bad_phandle.dtb \
>>> +#			overlay_bad_symbol.dtb \
>>> +#			overlay_base.dtb \
>>> +
>>> +apply_static_overlay := overlay_0.dtb \
>>> +			overlay_1.dtb \
>>> +			overlay_2.dtb \
>>> +			overlay_3.dtb \
>>> +			overlay_4.dtb \
>>> +			overlay_5.dtb \
>>> +			overlay_6.dtb \
>>> +			overlay_7.dtb \
>>> +			overlay_8.dtb \
>>> +			overlay_9.dtb \
>>> +			overlay_10.dtb \
>>> +			overlay_11.dtb \
>>> +			overlay_12.dtb \
>>> +			overlay_13.dtb \
>>> +			overlay_15.dtb \
>>> +			overlay_gpio_01.dtb \
>>> +			overlay_gpio_02a.dtb \
>>> +			overlay_gpio_02b.dtb \
>>> +			overlay_gpio_03.dtb \
>>> +			overlay_gpio_04a.dtb \
>>> +			overlay_gpio_04b.dtb
>>> +
>>> +quiet_cmd_fdtoverlay = FDTOVERLAY $@
>>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>>> +
>>> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
>>> +	$(call if_changed,fdtoverlay)
>>> +
>>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb
>>
>> The fact that testcases.dts includes /plugin/ still seems completely
>> wrong, if it's being used as the base tree.
>>
> 
> Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT.
> It is a convenient FDT to use because it provides the frame that the overlays
> require to be applied.  It is fortunate that fdtoverlay does not reject the use
> of an FDT with overlay metadata as the base blob.
> 
> If Viresh wants to test a more realistic data set then he could create a build
> rule that copies testcases.dts into (for example) testcases_base.dts, strip
> out the '/plugin/;" then compile that into testcases_base.dtb and use that for
> fdtoverlay.
> 
>    pseudo makefile rule for testcases_base.dts:
>        sed -e 's|/plugin/;||' > testcases_base.dts
> 
>    add testcases_base.dtb to the list of objects to build
> 
>    change the rule for static_test.dtb to use testcases_base.dtb instead of
>    testcases.dtb
> 
> This is probably a good idea instead of depending on the leniency of fdtoverlay.

It should be possible to apply this same concept to copying overlay_base.dts
to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts
and using an additional rule to use fdtoverlay to apply overlay.dtb on top
of overlay_base_base.dtb.

Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.

I tried this by hand and am failing miserably.  But I am not using the proper
environment (just a quick hack to see if the method might work).  So I would
have to set things up properly to really test this.

If this does work, it would remove my objections to you trying to transform
the existing unittest .dts test data files (because you would not have to
actually modify the existing .dts files).

> 
> -Frank
> 

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[Frank Rowand]

On 1/20/21 10:13 PM, Viresh Kumar wrote:
> On 21-01-21, 11:49, David Gibson wrote:
>> If you're using overlays, you probably need the -@ flag, for both the
>> base file and the overlays, which AFAICT is not already the case.
> 
> I think the idea was to do that in the platform specific Makefiles,
> unless I have misunderstood that from earlier discussions. So a
> platform may want to do that per-file or just enable it for the entire
> platform.
> 

Yes, that is correct.  For drivers/of/unitest-data/Makefile, we have
entries like:

DTC_FLAGS_overlay += -@
DTC_FLAGS_overlay_bad_phandle += -@
DTC_FLAGS_overlay_bad_symbol += -@
DTC_FLAGS_overlay_base += -@
DTC_FLAGS_testcases += -@

-Frank

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

Hi Frank,

On 20-01-21, 23:34, Frank Rowand wrote:
> It should be possible to apply this same concept to copying overlay_base.dts
> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts
> and using an additional rule to use fdtoverlay to apply overlay.dtb on top
> of overlay_base_base.dtb.

Are you suggesting to then merge this with testcases.dtb to get
static_test.dtb or keep two output files (static_test.dtb from
testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb
and overlay.dtb) ?

Asking because as I mentioned earlier, overlay_base.dtb doesn't have
__overlay__ property for its nodes and we can't apply that to
testcases.dtb using fdtoverlay.

> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.
> 
> I tried this by hand and am failing miserably.  But I am not using the proper
> environment (just a quick hack to see if the method might work).  So I would
> have to set things up properly to really test this.
> 
> If this does work, it would remove my objections to you trying to transform
> the existing unittest .dts test data files (because you would not have to
> actually modify the existing .dts files).

-- 
viresh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/20/21 11:34 PM, Viresh Kumar wrote:
> On 20-01-21, 23:14, Frank Rowand wrote:
>> It is a convenient FDT to use because it provides the frame that the overlays
>> require to be applied.  It is fortunate that fdtoverlay does not reject the use
>> of an FDT with overlay metadata as the base blob.
> 
>> This is probably a good idea instead of depending on the leniency of fdtoverlay.
> 
> I believe fdtoverlay allows that intentionally, that would be required
> for the cases where we have a hierarchy of extension boards or
> overlays.
> 
> A platform can have a base dtb (with /plugin/;), then we can have an
> overlay (1) for an extension board (with /plugin/;) and then an
> overlay (2) for an extension board for the previous extension board.
> 
> In such a case overlay-(2) can't be applied directly to the base dtb
> as it may not find all the nodes it is trying to update. And so
> overlay-(2) needs to be applied to overlay-(1) and then the output of
> this can be applied to the base dtb.

I have only the most surface knowledge of fdtoverlay, mostly from
"fdtoverlay --help", but you can apply multiple overlays with a
single invocation of fdtoverlay.  My _assumption_ was that the
overlays would be applied in order, and after any given overlay
was applied, subsequent overlays could reference the previously
applied overlay.

Is my assumption incorrect?

> 
> This is very similar to what I tried with the intermediate.dtb
> earlier.
> 

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 20-01-21, 23:45, Frank Rowand wrote:
> I have only the most surface knowledge of fdtoverlay, mostly from
> "fdtoverlay --help", but you can apply multiple overlays with a
> single invocation of fdtoverlay.  My _assumption_ was that the
> overlays would be applied in order, and after any given overlay
> was applied, subsequent overlays could reference the previously
> applied overlay.
> 
> Is my assumption incorrect?

I think yes, if everything is in order then it should work just fine.

I was only suggesting that fdtoverlay accepting the base overlay with
/plugin/; may well be a requirement and so intentionally done.

-- 
viresh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/20/21 11:43 PM, Viresh Kumar wrote:
> Hi Frank,
> 
> On 20-01-21, 23:34, Frank Rowand wrote:
>> It should be possible to apply this same concept to copying overlay_base.dts
>> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts
>> and using an additional rule to use fdtoverlay to apply overlay.dtb on top
>> of overlay_base_base.dtb.
> 
> Are you suggesting to then merge this with testcases.dtb to get
> static_test.dtb

no

> or keep two output files (static_test.dtb from
> testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb
> and overlay.dtb) ?

yes, but using the modified versions ("/plugin/;" removed) of
testcases.dtb and overlay_base.dtb.

> 
> Asking because as I mentioned earlier, overlay_base.dtb doesn't have
> __overlay__ property for its nodes and we can't apply that to
> testcases.dtb using fdtoverlay.

Correct.

I apologize in advance if I get confused in these threads or cause confusion.
I find myself going in circles and losing track of how things fit together as
I move through the various pieces of unittest.

-Frank

> 
>> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.
>>
>> I tried this by hand and am failing miserably.  But I am not using the proper
>> environment (just a quick hack to see if the method might work).  So I would
>> have to set things up properly to really test this.
>>
>> If this does work, it would remove my objections to you trying to transform
>> the existing unittest .dts test data files (because you would not have to
>> actually modify the existing .dts files).
> 

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 20-01-21, 23:55, Frank Rowand wrote:
> yes, but using the modified versions ("/plugin/;" removed) of
> testcases.dtb and overlay_base.dtb.

Okay, that would work fine I guess. I will try to implement this in
the new version.

> I apologize in advance if I get confused in these threads or cause confusion.
> I find myself going in circles and losing track of how things fit together as
> I move through the various pieces of unittest.

Me too :)

Today is the first time where we have some overlap in our work hours
(probably you working late :)), and we are able to get this sorted out
quickly enough.

Thanks for your feedback Frank.

-- 
viresh

Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Thu, Jan 21, 2021 at 09:43:00AM +0530, Viresh Kumar wrote:
> On 21-01-21, 11:49, David Gibson wrote:
> > If you're using overlays, you probably need the -@ flag, for both the
> > base file and the overlays, which AFAICT is not already the case.
> 
> I think the idea was to do that in the platform specific Makefiles,
> unless I have misunderstood that from earlier discussions. So a
> platform may want to do that per-file or just enable it for the entire
> platform.

Hm, ok.  Any platform that does anything with dtbo files is likely to
want it, though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 3/5] scripts: dtc: Remove the unused fdtdump.c file

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Thu, Jan 21, 2021 at 09:47:57AM +0530, Viresh Kumar wrote:
> On 21-01-21, 11:44, David Gibson wrote:
> > On Wed, Jan 20, 2021 at 12:36:45PM +0530, Viresh Kumar wrote:
> > > This was copied from external DTC repository long back and isn't used
> > > anymore. Over that the dtc tool can be used to generate the dts source
> > > back from the dtb. Remove the unused fdtdump.c file.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > Doesn't this make updating the kernel dtc from upstream needlessly
> > more difficult?
> 
> Hmm, I am not sure I understand the concern well. The kernel keeps a
> list of files[1] it needs to automatically copy (using a script) from
> the upstream dtc repo and fdtdump.c was never part of that. Keeping it
> there isn't going to make any difficulty I believe.

Hm, ok.  Seems a bit clunky compared to embedding the whole directory,
but whatever.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

On Thu, Jan 21, 2021 at 11:04:26AM +0530, Viresh Kumar wrote:
> On 20-01-21, 23:14, Frank Rowand wrote:
> > It is a convenient FDT to use because it provides the frame that the overlays
> > require to be applied.  It is fortunate that fdtoverlay does not reject the use
> > of an FDT with overlay metadata as the base blob.
> 
> > This is probably a good idea instead of depending on the leniency of fdtoverlay.
> 
> I believe fdtoverlay allows that intentionally, that would be required
> for the cases where we have a hierarchy of extension boards or
> overlays.

Um.. no.

> A platform can have a base dtb (with /plugin/;), then we can have an
> overlay (1) for an extension board (with /plugin/;) and then an
> overlay (2) for an extension board for the previous extension board.
> 
> In such a case overlay-(2) can't be applied directly to the base dtb
> as it may not find all the nodes it is trying to update. And so
> overlay-(2) needs to be applied to overlay-(1) and then the output of
> this can be applied to the base dtb.

No, this is the wrong way around.  The expected operation here is that
you apply overlay (1) to the base tree, giving you, say, output1.dtb.
output1.dtb is (effectively) a base tree itself, to which you can then
apply overlay-(2).

What you're talking about is "merging" overlays: combingin overlay (1)
and (2) into overlay-(X) which would have the same effect applied to
base.dtb as (1) and (2) applied in sequence.  Merging overlays is
something that could make sense, but fdtoverlay will not do it at
present.

> This is very similar to what I tried with the intermediate.dtb
> earlier.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, Jan 21, 2021 at 11:20:40AM +0530, Viresh Kumar wrote:
> On 20-01-21, 23:45, Frank Rowand wrote:
> > I have only the most surface knowledge of fdtoverlay, mostly from
> > "fdtoverlay --help", but you can apply multiple overlays with a
> > single invocation of fdtoverlay.  My _assumption_ was that the
> > overlays would be applied in order, and after any given overlay
> > was applied, subsequent overlays could reference the previously
> > applied overlay.
> > 
> > Is my assumption incorrect?
> 
> I think yes, if everything is in order then it should work just fine.
> 
> I was only suggesting that fdtoverlay accepting the base overlay with
> /plugin/; may well be a requirement and so intentionally done.

No.  It's simply the result of the fact that a dtbo is still a dtb.
So, you can still apply an overlay to it.  However, a dtbo is a
weirdly structured dtb, so applying an overlay to it is very unlikely
to give you something useful.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

On Wed, Jan 20, 2021 at 11:55:08PM -0600, Frank Rowand wrote:
> On 1/20/21 11:43 PM, Viresh Kumar wrote:
> > Hi Frank,
> > 
> > On 20-01-21, 23:34, Frank Rowand wrote:
> >> It should be possible to apply this same concept to copying overlay_base.dts
> >> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts
> >> and using an additional rule to use fdtoverlay to apply overlay.dtb on top
> >> of overlay_base_base.dtb.
> > 
> > Are you suggesting to then merge this with testcases.dtb to get
> > static_test.dtb
> 
> no
> 
> > or keep two output files (static_test.dtb from
> > testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb
> > and overlay.dtb) ?
> 
> yes, but using the modified versions ("/plugin/;" removed) of
> testcases.dtb and overlay_base.dtb.

I really don't understand why you want /plugin/ in *any* version of
testcases.dtb.
> > 
> > Asking because as I mentioned earlier, overlay_base.dtb doesn't have
> > __overlay__ property for its nodes and we can't apply that to
> > testcases.dtb using fdtoverlay.
> 
> Correct.
> 
> I apologize in advance if I get confused in these threads or cause confusion.
> I find myself going in circles and losing track of how things fit together as
> I move through the various pieces of unittest.
> 
> -Frank
> 
> > 
> >> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.
> >>
> >> I tried this by hand and am failing miserably.  But I am not using the proper
> >> environment (just a quick hack to see if the method might work).  So I would
> >> have to set things up properly to really test this.
> >>
> >> If this does work, it would remove my objections to you trying to transform
> >> the existing unittest .dts test data files (because you would not have to
> >> actually modify the existing .dts files).
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 21-01-21, 17:34, David Gibson wrote:
> No, this is the wrong way around.  The expected operation here is that
> you apply overlay (1) to the base tree, giving you, say, output1.dtb.
> output1.dtb is (effectively) a base tree itself, to which you can then
> apply overlay-(2).

Thanks for the confirmation about this.

> Merging overlays is
> something that could make sense, but fdtoverlay will not do it at
> present.

FWIW, I think it works fine right now even if it not intentional. I
did inspect the output dtb (made by merging two overlays) using
fdtdump and it looked okay. But yeah, I understand that we shouldn't
do it.

-- 
viresh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/20/21 11:58 PM, Viresh Kumar wrote:
> On 20-01-21, 23:55, Frank Rowand wrote:
>> yes, but using the modified versions ("/plugin/;" removed) of
>> testcases.dtb and overlay_base.dtb.
> 
> Okay, that would work fine I guess. I will try to implement this in
> the new version.
> 
>> I apologize in advance if I get confused in these threads or cause confusion.
>> I find myself going in circles and losing track of how things fit together as
>> I move through the various pieces of unittest.
> 
> Me too :)
> 
> Today is the first time where we have some overlap in our work hours
> (probably you working late :)), and we are able to get this sorted out
> quickly enough.

Working quite late.  I swear I stopped working 3 hours ago and that was
already late.

I reacted quite negatively to the attempt to restructure the unittest
.dts file in the original patch.  Now after walking around the problem
space a bit, and reviewing the ugly things that unittest.c does, and
coming up with the approach of sed to copy and modify the two base
.dts files, I think I finally have my head wrapped around an easier
and cleaner approach than sed.

I'll describe it for just one of the two base files, but the same
concept would apply to the other.  Don't take my file names as
good suggestions, I am avoiding using the brain power to come up
with good names at the moment.

1) rename overlay_base.dts to overlay_base.dtsi

2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi

3) create a new overlay_base.dts:
   // SPDX-License-Identifier: GPL-2.0
   /dts-v1/;
   /plugin/;
   #include overlay_base_dtsi

4) create a new file overlay_base_static.dts:
   // SPDX-License-Identifier: GPL-2.0
   /dts-v1/;
   #include overlay_base_dtsi

5) then use overlay_base_static.dtb as the base blob for ftdoverlay
   applying overlay.dtb

Untested, hand typed, hopefully no typos.

-Frank

> 
> Thanks for your feedback Frank.
> 

Re: [PATCH V5 3/5] scripts: dtc: Remove the unused fdtdump.c file

[Rob Herring]

On Thu, Jan 21, 2021 at 12:43 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Thu, Jan 21, 2021 at 09:47:57AM +0530, Viresh Kumar wrote:
> > On 21-01-21, 11:44, David Gibson wrote:
> > > On Wed, Jan 20, 2021 at 12:36:45PM +0530, Viresh Kumar wrote:
> > > > This was copied from external DTC repository long back and isn't used
> > > > anymore. Over that the dtc tool can be used to generate the dts source
> > > > back from the dtb. Remove the unused fdtdump.c file.
> > > >
> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >
> > > Doesn't this make updating the kernel dtc from upstream needlessly
> > > more difficult?
> >
> > Hmm, I am not sure I understand the concern well. The kernel keeps a
> > list of files[1] it needs to automatically copy (using a script) from
> > the upstream dtc repo and fdtdump.c was never part of that. Keeping it
> > there isn't going to make any difficulty I believe.
>
> Hm, ok.  Seems a bit clunky compared to embedding the whole directory,
> but whatever.

Either way, it's a list of what to keep or what to omit as we don't
want build files nor tests. If we were to take the whole thing, then
we should do a submodule, but so far no one wants submodules in the
kernel tree. There is a git subtree feature now that could do the same
thing as the script. But the script is good enough only needing small
tweaks occasionally, and anything else is work.

Rob

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Thu, Jan 21, 2021 at 12:27:28PM +0530, Viresh Kumar wrote:
> On 21-01-21, 17:34, David Gibson wrote:
> > No, this is the wrong way around.  The expected operation here is that
> > you apply overlay (1) to the base tree, giving you, say, output1.dtb.
> > output1.dtb is (effectively) a base tree itself, to which you can then
> > apply overlay-(2).
> 
> Thanks for the confirmation about this.
> 
> > Merging overlays is
> > something that could make sense, but fdtoverlay will not do it at
> > present.
> 
> FWIW, I think it works fine right now even if it not intentional.

No, it definitely will not work in general.  It might kinda work in a
few trivial cases, but it absolutely will not do the neccessary
handling in some cases.

> I
> did inspect the output dtb (made by merging two overlays) using
> fdtdump and it looked okay.

Ok.. but if you're using these bizarre messed up "dtbs" that this test
code seems to be, I don't really trust that tells you much.

> But yeah, I understand that we shouldn't
> do it.


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On Thu, Jan 21, 2021 at 01:04:46AM -0600, Frank Rowand wrote:
> On 1/20/21 11:58 PM, Viresh Kumar wrote:
> > On 20-01-21, 23:55, Frank Rowand wrote:
> >> yes, but using the modified versions ("/plugin/;" removed) of
> >> testcases.dtb and overlay_base.dtb.
> > 
> > Okay, that would work fine I guess. I will try to implement this in
> > the new version.
> > 
> >> I apologize in advance if I get confused in these threads or cause confusion.
> >> I find myself going in circles and losing track of how things fit together as
> >> I move through the various pieces of unittest.
> > 
> > Me too :)
> > 
> > Today is the first time where we have some overlap in our work hours
> > (probably you working late :)), and we are able to get this sorted out
> > quickly enough.
> 
> Working quite late.  I swear I stopped working 3 hours ago and that was
> already late.
> 
> I reacted quite negatively to the attempt to restructure the unittest
> .dts file in the original patch.  Now after walking around the problem
> space a bit, and reviewing the ugly things that unittest.c does, and
> coming up with the approach of sed to copy and modify the two base
> .dts files, I think I finally have my head wrapped around an easier
> and cleaner approach than sed.
> 
> I'll describe it for just one of the two base files, but the same
> concept would apply to the other.  Don't take my file names as
> good suggestions, I am avoiding using the brain power to come up
> with good names at the moment.
> 
> 1) rename overlay_base.dts to overlay_base.dtsi
> 
> 2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi
> 
> 3) create a new overlay_base.dts:
>    // SPDX-License-Identifier: GPL-2.0
>    /dts-v1/;
>    /plugin/;
>    #include overlay_base_dtsi

"overlay_base" is a terrible name - it's not clear if it's supposed to
be a base tree or an overlay.  I'm still not seeing any reasonable
use for the plugin version either, fwiw.

> 
> 4) create a new file overlay_base_static.dts:
>    // SPDX-License-Identifier: GPL-2.0
>    /dts-v1/;
>    #include overlay_base_dtsi
> 
> 5) then use overlay_base_static.dtb as the base blob for ftdoverlay
>    applying overlay.dtb
> 
> Untested, hand typed, hopefully no typos.
> 
> -Frank
> 
> > 
> > Thanks for your feedback Frank.
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 22-01-21, 10:39, David Gibson wrote:
> No, it definitely will not work in general.  It might kinda work in a
> few trivial cases, but it absolutely will not do the neccessary
> handling in some cases.
> 
> > I
> > did inspect the output dtb (made by merging two overlays) using
> > fdtdump and it looked okay.
> 
> Ok.. but if you're using these bizarre messed up "dtbs" that this test
> code seems to be, I don't really trust that tells you much.

I only looked if the changes from the second overlay were present in
the merge and they were. And so I assumed that it must have worked.

What about checking the base dtb for /plugin/; in fdtoverlay and fail
the whole thing in case it is present ? I think it is possible for
people to get confused otherwise, like I did.

-- 
viresh

Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Fri, Jan 22, 2021 at 08:40:49AM +0530, Viresh Kumar wrote:
> On 22-01-21, 10:39, David Gibson wrote:
> > No, it definitely will not work in general.  It might kinda work in a
> > few trivial cases, but it absolutely will not do the neccessary
> > handling in some cases.
> > 
> > > I
> > > did inspect the output dtb (made by merging two overlays) using
> > > fdtdump and it looked okay.
> > 
> > Ok.. but if you're using these bizarre messed up "dtbs" that this test
> > code seems to be, I don't really trust that tells you much.
> 
> I only looked if the changes from the second overlay were present in
> the merge and they were. And so I assumed that it must have worked.
> 
> What about checking the base dtb for /plugin/; in fdtoverlay and fail
> the whole thing in case it is present ? I think it is possible for
> people to get confused otherwise, like I did.

/plugin/ doesn't exist in the dtb, only in the dts.  From the dtb
encoding point of view, there's no difference between a dtb and a
dtbo, a dtbo is just a dtb that follows some conventions for its
content.

If we were doing this from scratch, it would be better for dtbos to
have a different magic number from regular dtbs.  I think I actually
suggested that sometime in the past, but by the time that came up,
dtbos were already in pretty widespread use with the existing format.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]