dt: build overlays

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 <[email protected]>

---
 scripts/dtc/fdtdump.c | 163 ------------------------------------------
 1 file changed, 163 deletions(-)
 delete mode 100644 scripts/dtc/fdtdump.c

scripts/dtc/fdtdump.c deleted +0 -163

deleted file mode 100644
index 7d460a50b513..000000000000
@@ -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;
-}

Quoted 5 lines · 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 <[email protected]>

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

Quoted 7 lines · David Gibson

Quoted 5 lines · 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 <[email protected]>

Quoted 2 lines

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.

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · David Gibson

Quoted 5 lines · 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 <[email protected]>

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

Quoted 4 lines

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.

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 <[email protected]>

---
 .gitignore               | 3 +--
 Makefile                 | 4 ++--
 scripts/Makefile.dtbinst | 3 +++
 scripts/Makefile.lib     | 4 +++-
 4 files changed, 9 insertions(+), 5 deletions(-)

.gitignore +0 -0

index d01cda8e1177..0458c36f3cb2 100644

.gitignore +0 -0

 *.dwo
 *.elf
 *.gcno

Makefile +0 -0

index 9e73f82e0d86..b84f5e0b46ab 100644

Makefile +1 -1

 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' \

scripts/Makefile.dtbinst +3 -0

index 50d580d77ae9..ba01f5ba2517 100644
@@ -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)/%,$@)

scripts/Makefile.lib +2 -0

index 213677a5ed33..30bc0a8e0087 100644
@@ -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

scripts/Makefile.lib +0 -1

 DT_CHECKER ?= dt-validate
--

2.25.0.rc1.19.g042ed3e048af

Quoted 2 lines

+%.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

Quoted 27 lines · Masahiro Yamada

Quoted 2 lines

+%.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"

Quoted 1 line

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

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.

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 <[email protected]>

---
 .gitignore               | 3 +--
 Makefile                 | 4 ++--
 scripts/Makefile.dtbinst | 3 +++
 scripts/Makefile.lib     | 4 +++-
 4 files changed, 9 insertions(+), 5 deletions(-)

.gitignore +1 -2

index d01cda8e1177..0458c36f3cb2 100644
@@ -17,8 +17,7 @@
 *.bz2
 *.c.[012]*.*
 *.dt.yaml
-*.dtb
-*.dtb.S
+*.dtb*
 *.dwo
 *.elf
 *.gcno

Makefile +2 -2

index 9e73f82e0d86..b84f5e0b46ab 100644
@@ -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' \

scripts/Makefile.dtbinst +3 -0

index 50d580d77ae9..ba01f5ba2517 100644
@@ -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)/%,$@)

scripts/Makefile.lib +2 -0

index 213677a5ed33..30bc0a8e0087 100644
@@ -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

scripts/Makefile.lib +0 -0

 DT_CHECKER ?= dt-validate

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.

Quoted 2 lines · David Gibson

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.

Quoted 2 lines · David Gibson

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.

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 <[email protected]>

---
 drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

drivers/of/unittest-data/Makefile +40 -0

index 009f4045c8e4..ece7dfd5cafa 100644
@@ -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 \

Quoted 65 lines · 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 <[email protected]>

---
 drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

drivers/of/unittest-data/Makefile +45 -0

index 009f4045c8e4..ece7dfd5cafa 100644
@@ -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 $^

drivers/of/unittest-data/Makefile DTC_FLAGS_testcases += -@ · line 90

Quoted 5 lines

drivers/of/unittest-data/Makefile +5 -0

+
+$(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.

Hi David,

Quoted 66 lines · David Gibson

Quoted 65 lines · 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 <[email protected]>

---
 drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

drivers/of/unittest-data/Makefile +45 -0

index 009f4045c8e4..ece7dfd5cafa 100644
@@ -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 $^

Quoted 9 lines

drivers/of/unittest-data/Makefile DTC_FLAGS_testcases += -@ · line 90

Quoted 5 lines

drivers/of/unittest-data/Makefile +5 -0

+
+$(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

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).

Hi Frank,

Quoted 4 lines · Frank Rowand

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.

Quoted 10 lines

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).

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

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.

Quoted 2 lines · Frank Rowand

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.

Quoted 3 lines

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.

Quoted 11 lines · Viresh Kumar

Quoted 2 lines · Frank Rowand

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 3 lines

Thanks for your feedback Frank.

Quoted 10 lines · Viresh Kumar

Hi Frank,

Quoted 4 lines · Frank Rowand

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

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 11 lines

Quoted 10 lines

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).

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.

Quoted 5 lines · Frank Rowand

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.

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?

Quoted 7 lines · Frank Rowand

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.

Quoted 1 line

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.

Quoted 5 lines · Frank Rowand

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.

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).

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

Quoted 4 lines · David Gibson

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.

Quoted 3 lines

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.

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.

Quoted 3 lines

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.

Quoted 10 lines · David Gibson

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.

Quoted 3 lines

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.

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).

Quoted 11 lines · 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.

Quoted 4 lines

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?