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/[email protected]/
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
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 <[email protected]>
---
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
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
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
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(-)
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
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(+)
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
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 <[email protected]>
---
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
On Wed, Jan 20, 2021 at 4:07 PM Viresh Kumar <[email protected]> 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 <[email protected]>
> ---
> .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
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
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
On Wed, Jan 20, 2021 at 6:55 PM Viresh Kumar <[email protected]> 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
On Wed, Jan 20, 2021 at 1:07 AM Viresh Kumar <[email protected]> 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
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 <[email protected]>
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
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 <[email protected]>
> ---
> .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
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 <[email protected]>
> ---
> 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
On 20-01-21, 09:43, Rob Herring wrote:
> On Wed, Jan 20, 2021 at 1:07 AM Viresh Kumar <[email protected]> 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
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 <[email protected]>
>
> 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
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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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
>
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
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
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
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.
>
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
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).
>
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
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 <[email protected]>
> >
> > 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
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
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
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
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
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.
>
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
On Thu, Jan 21, 2021 at 12:43 AM David Gibson
<[email protected]> 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 <[email protected]>
> > >
> > > 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
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
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
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
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