[PATCH v2 0/6] Makefile: Tidy up of-platdata file generation rules
The recent of-platdata implementation has caused an occasional error in CI, possibly due to the transition between !OF_PLATDATA_INST and OF_PLATDATA_INST.
The problems seems to be due to a generated file (C or object file) not being regenerated when the setting changes.
This series take two steps aimed at correct this problem.
Firstly it makes the set of files generated (in each case) mutually exclusive, except for the header files which remain common. This means that the build will fail if new files are not generated when the setting changes.
Secondly it removes the old generated files before building new ones, since that could trip things up if the flag changes back again in a subsequent build.
In addition, dtoc is currently running on every of-platadata build, even if nothing has changed. Also pylibfdt is always built due to a change in file naming with Python 3. Both of these problems are fixed.
Changes in v2: - Adjust tag so this patch doesn't got to dtc list - Add a comment to the endif - Only remove the old (and to be unused) files, so avoid confusing make
Simon Glass (6): libfdt: Tidy up pylibfdt build rule Makefile: Avoid running dtoc every time Makefile: Depend only on required of-platdata files dtoc: Only generate the required files Makefile: Use a variable for generated of-platdata headers Makefile: Remove old of-platdata files before regenerating
scripts/Makefile.spl | 43 ++++++++++++++++++++++++----------- scripts/dtc/pylibfdt/Makefile | 8 +++++-- tools/dtoc/dtb_platdata.py | 23 +++++++++++++++---- tools/dtoc/test_dtoc.py | 2 +- 4 files changed, 56 insertions(+), 20 deletions(-)
At present the build rule for pylibfdt depends on _libfdt.so but modern Python versions add a different suffix to the output file, resulting in something like _libfdt.cpython-38-x86_64-linux-gnu.so
The result is that pylibfdt is rebuilt every time.
Rename the file the standard name so that the rule works correctly. Also add libfdt.py to the dependencies, so that file is always created if missing.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust tag so this patch doesn't got to dtc list
scripts/dtc/pylibfdt/Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/dtc/pylibfdt/Makefile b/scripts/dtc/pylibfdt/Makefile index 80b6ad2ae71..112198df9de 100644 --- a/scripts/dtc/pylibfdt/Makefile +++ b/scripts/dtc/pylibfdt/Makefile @@ -23,12 +23,16 @@ quiet_cmd_pymod = PYMOD $@ SWIG_OPTS="-I$(LIBFDT_srcdir) -I$(LIBFDT_srcdir)/.." \ $(PYTHON3) $< --quiet build_ext --inplace
-$(obj)/_libfdt.so: $(src)/setup.py $(PYLIBFDT_srcs) FORCE +$(obj)/_libfdt.so $(obj)/libfdt.py &: $(src)/setup.py $(PYLIBFDT_srcs) @# Remove the library since otherwise Python doesn't seem to regenerate @# the libfdt.py file if it is missing. rm -f $(obj)/_libfdt*.so $(call if_changed,pymod) + @# Rename the file to _libfdt.so so this Makefile doesn't run every time + @if [ ! -e $(obj)/_libfdt.so ]; then \ + mv -v $(obj)/_libfdt*.so $(obj)/_libfdt.so; \ + fi
-always += _libfdt.so +always += _libfdt.so libfdt.py
clean-files += libfdt.i _libfdt.so libfdt.py libfdt_wrap.c
At present the build rule for pylibfdt depends on _libfdt.so but modern Python versions add a different suffix to the output file, resulting in something like _libfdt.cpython-38-x86_64-linux-gnu.so
The result is that pylibfdt is rebuilt every time.
Rename the file the standard name so that the rule works correctly. Also add libfdt.py to the dependencies, so that file is always created if missing.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust tag so this patch doesn't got to dtc list
scripts/dtc/pylibfdt/Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Applied to u-boot-dm/next, thanks!
Since the dst_dir rule always runs, it causes a rebuild of the of-platdata files even if not needed.
Create the directory inside the rule instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
scripts/Makefile.spl | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 849e9c7060e..4f5876dad95 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -330,12 +330,10 @@ $(obj)/dts/dt-%.o: $(obj)/dts/dt-%.c \ include/generated/dt-structs-gen.h prepare FORCE $(call if_changed,plat)
-PHONY += dts_dir -dts_dir: - $(shell [ -d $(obj)/dts ] || mkdir -p $(obj)/dts) - +# Don't use dts_dir here, since it forces running this expensive rule every time include/generated/dt-structs-gen.h $(u-boot-spl-platdata_c) &: \ - $(obj)/$(SPL_BIN).dtb dts_dir FORCE + $(obj)/$(SPL_BIN).dtb + @[ -d $(obj)/dts ] || mkdir -p $(obj)/dts $(call if_changed,dtoc)
ifdef CONFIG_SAMSUNG @@ -476,6 +474,10 @@ FORCE: $(obj)/dts/dt-$(SPL_NAME).dtb: dts/dt.dtb $(Q)$(MAKE) $(build)=$(obj)/dts spl_dtbs
+PHONY += dts_dir +dts_dir: + $(shell [ -d $(obj)/dts ] || mkdir -p $(obj)/dts) + # Declare the contents of the .PHONY variable as phony. We keep that # information in a variable so we can use it in if_changed and friends. .PHONY: $(PHONY)
Since the dst_dir rule always runs, it causes a rebuild of the of-platdata files even if not needed.
Create the directory inside the rule instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
scripts/Makefile.spl | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Applied to u-boot-dm/next, thanks!
When OF_PLATDATA_INST is enabled, we need dt-uclass.c and dt-device.c for the build to work. When OF_PLATDATA_INST is not enabled, we only need dt-plat.c
Update the Makefile rules to indicate this.
At present all files are generated and compiled regardless of which are actually needed.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
scripts/Makefile.spl | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 4f5876dad95..5f37a82931e 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -120,8 +120,11 @@ endif u-boot-spl-init := $(head-y) u-boot-spl-main := $(libs-y) ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA -u-boot-spl-platdata := $(obj)/dts/dt-plat.o $(obj)/dts/dt-uclass.o \ - $(obj)/dts/dt-device.o +ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA_INST +u-boot-spl-platdata := $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o +else +u-boot-spl-platdata := $(obj)/dts/dt-plat.o +endif u-boot-spl-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-platdata)) endif
When OF_PLATDATA_INST is enabled, we need dt-uclass.c and dt-device.c for the build to work. When OF_PLATDATA_INST is not enabled, we only need dt-plat.c
Update the Makefile rules to indicate this.
At present all files are generated and compiled regardless of which are actually needed.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
scripts/Makefile.spl | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Applied to u-boot-dm/next, thanks!
At present all possible files are generated, even if some of them just have a header and an empty body. It is better to generate only the files that are needed, so that the two types of build (based on the setting of OF_PLATDATA_INST) can be mutually exclusive.
This is intended to fix a strange problem sometimes found with CI:
Building current source for 1 boards (1 thread, 40 jobs per thread) sandbox: + sandbox_spl +drivers/built-in.o: In function `dm_setup_inst': +drivers/core/root.c:135: undefined reference to `_u_boot_list_2_udevice_2_root' +dts/dt-uclass.o:(.u_boot_list_2_uclass_2_serial+0x10): undefined reference to `_u_boot_list_2_udevice_2_serial' ...
This likely happens when switching from !OF_PLATDATA_INST to OF_PLATDATA_INST since running 'make xxx_defconfig" does not currently cause any change in which files are generated. With !OF_PLATDATA_INST the dt-device.c file has no declarations and this is assumed to be the starting state. The error above seems to indicate that, after changing to OF_PLATDATA_INST, the dt-uclass.c file is regenerated but the dt-device.c files is not. This does not seem possible from the relevant Makefile.spl rule:
u-boot-spl-platdata := $(obj)/dts/dt-plat.o $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o
cmd_dtoc = $(DTOC_ARGS) -c $(obj)/dts -C include/generated all
include/generated/dt-structs-gen.h $(u-boot-spl-platdata_c) &: \ $(obj)/$(SPL_BIN).dtb @[ -d $(obj)/dts ] || mkdir -p $(obj)/dts $(call if_changed,dtoc)
It seems that this cannot regenerate dt-uclass.c without dt-device.c since 'dtoc all' is used. So here the trail ends for now.
In any case it seems better to generate files that are uses and not bother with those that serve no purpose. So update dtoc to do this automatically.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
tools/dtoc/dtb_platdata.py | 23 +++++++++++++++++++---- tools/dtoc/test_dtoc.py | 2 +- 2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index b5c449ebb47..c9c657cb9a9 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -1128,7 +1128,7 @@ class DtbPlatdata(): # Types of output file we understand # key: Command used to generate this file # value: OutputFile for this command -OUTPUT_FILES = { +OUTPUT_FILES_COMMON = { 'decl': OutputFile(Ftype.HEADER, 'dt-decl.h', DtbPlatdata.generate_decl, 'Declares externs for all device/uclass instances'), @@ -1136,9 +1136,17 @@ OUTPUT_FILES = { OutputFile(Ftype.HEADER, 'dt-structs-gen.h', DtbPlatdata.generate_structs, 'Defines the structs used to hold devicetree data'), + } + +# File generated without instantiate +OUTPUT_FILES_NOINST = { 'platdata': OutputFile(Ftype.SOURCE, 'dt-plat.c', DtbPlatdata.generate_plat, 'Declares the U_BOOT_DRIVER() records and platform data'), + } + +# File generated with instantiate +OUTPUT_FILES_INST = { 'device': OutputFile(Ftype.SOURCE, 'dt-device.c', DtbPlatdata.generate_device, 'Declares the DM_DEVICE_INST() records'), @@ -1204,14 +1212,21 @@ def run_steps(args, dtb_file, include_disabled, output, output_dirs, phase, plat.read_aliases() plat.assign_seqs()
+ # Figure out what output files we plan to generate + output_files = OUTPUT_FILES_COMMON + if instantiate: + output_files.update(OUTPUT_FILES_INST) + else: + output_files.update(OUTPUT_FILES_NOINST) + cmds = args[0].split(',') if 'all' in cmds: - cmds = sorted(OUTPUT_FILES.keys()) + cmds = sorted(output_files.keys()) for cmd in cmds: - outfile = OUTPUT_FILES.get(cmd) + outfile = output_files.get(cmd) if not outfile: raise ValueError("Unknown command '%s': (use: %s)" % - (cmd, ', '.join(sorted(OUTPUT_FILES.keys())))) + (cmd, ', '.join(sorted(output_files.keys())))) plat.setup_output(outfile.ftype, outfile.fname if output_dirs else output) plat.out_header(outfile) diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 1912a8723f2..e9512834574 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -1554,7 +1554,7 @@ U_BOOT_DRVINFO(spl_test2) = { with self.assertRaises(ValueError) as exc: self.run_test(['invalid-cmd'], dtb_file, output) self.assertIn( - "Unknown command 'invalid-cmd': (use: decl, device, platdata, struct, uclass)", + "Unknown command 'invalid-cmd': (use: decl, platdata, struct)", str(exc.exception))
def test_output_conflict(self):
At present all possible files are generated, even if some of them just have a header and an empty body. It is better to generate only the files that are needed, so that the two types of build (based on the setting of OF_PLATDATA_INST) can be mutually exclusive.
This is intended to fix a strange problem sometimes found with CI:
Building current source for 1 boards (1 thread, 40 jobs per thread) sandbox: + sandbox_spl +drivers/built-in.o: In function `dm_setup_inst': +drivers/core/root.c:135: undefined reference to `_u_boot_list_2_udevice_2_root' +dts/dt-uclass.o:(.u_boot_list_2_uclass_2_serial+0x10): undefined reference to `_u_boot_list_2_udevice_2_serial' ...
This likely happens when switching from !OF_PLATDATA_INST to OF_PLATDATA_INST since running 'make xxx_defconfig" does not currently cause any change in which files are generated. With !OF_PLATDATA_INST the dt-device.c file has no declarations and this is assumed to be the starting state. The error above seems to indicate that, after changing to OF_PLATDATA_INST, the dt-uclass.c file is regenerated but the dt-device.c files is not. This does not seem possible from the relevant Makefile.spl rule:
u-boot-spl-platdata := $(obj)/dts/dt-plat.o $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o
cmd_dtoc = $(DTOC_ARGS) -c $(obj)/dts -C include/generated all
include/generated/dt-structs-gen.h $(u-boot-spl-platdata_c) &: \ $(obj)/$(SPL_BIN).dtb @[ -d $(obj)/dts ] || mkdir -p $(obj)/dts $(call if_changed,dtoc)
It seems that this cannot regenerate dt-uclass.c without dt-device.c since 'dtoc all' is used. So here the trail ends for now.
In any case it seems better to generate files that are uses and not bother with those that serve no purpose. So update dtoc to do this automatically.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
tools/dtoc/dtb_platdata.py | 23 +++++++++++++++++++---- tools/dtoc/test_dtoc.py | 2 +- 2 files changed, 20 insertions(+), 5 deletions(-)
Applied to u-boot-dm/next, thanks!
There are actually two generated files but only one is currently mentioned in the Makefile as a dependency. Put them both in a Makefile variable and use that instead, to avoid inconsistency.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a comment to the endif
scripts/Makefile.spl | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 5f37a82931e..11e9f6a17eb 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -120,13 +120,14 @@ endif u-boot-spl-init := $(head-y) u-boot-spl-main := $(libs-y) ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA +platdata-hdr := include/generated/dt-structs-gen.h include/generated/dt-decl.h ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA_INST u-boot-spl-platdata := $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o else u-boot-spl-platdata := $(obj)/dts/dt-plat.o endif u-boot-spl-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-platdata)) -endif +endif # OF_PLATDATA
# Linker Script # First test whether there's a linker-script for the specific stage defined... @@ -327,15 +328,11 @@ cmd_dtoc = $(DTOC_ARGS) -c $(obj)/dts -C include/generated all quiet_cmd_plat = PLAT $@ cmd_plat = $(CC) $(c_flags) -c $< -o $(filter-out $(PHONY),$@)
-targets += $(u-boot-spl-platdata) - -$(obj)/dts/dt-%.o: $(obj)/dts/dt-%.c \ - include/generated/dt-structs-gen.h prepare FORCE +$(obj)/dts/dt-%.o: $(obj)/dts/dt-%.c $(platdata-hdr) $(call if_changed,plat)
# Don't use dts_dir here, since it forces running this expensive rule every time -include/generated/dt-structs-gen.h $(u-boot-spl-platdata_c) &: \ - $(obj)/$(SPL_BIN).dtb +$(platdata-hdr) $(u-boot-spl-platdata_c) &: $(obj)/$(SPL_BIN).dtb @[ -d $(obj)/dts ] || mkdir -p $(obj)/dts $(call if_changed,dtoc)
There are actually two generated files but only one is currently mentioned in the Makefile as a dependency. Put them both in a Makefile variable and use that instead, to avoid inconsistency.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a comment to the endif
scripts/Makefile.spl | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
Applied to u-boot-dm/next, thanks!
Which files we generate depends on the setting of OF_PLATDATA_INST in the build. This might change between builds, but the build directory may be reused.
Leaving old files around is confusing and switching the OF_PLATDATA_INST setting does not necessarily regenerate the files, e.g. if the devicetree has not changed.
Remove all the files before regenerating new ones.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Only remove the old (and to be unused) files, so avoid confusing make
scripts/Makefile.spl | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 11e9f6a17eb..ca988224dad 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -121,12 +121,22 @@ u-boot-spl-init := $(head-y) u-boot-spl-main := $(libs-y) ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA platdata-hdr := include/generated/dt-structs-gen.h include/generated/dt-decl.h +platdata-inst := $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o +platdata-noinst := $(obj)/dts/dt-plat.o + ifdef CONFIG_$(SPL_TPL_)OF_PLATDATA_INST -u-boot-spl-platdata := $(obj)/dts/dt-uclass.o $(obj)/dts/dt-device.o +u-boot-spl-platdata := $(platdata-inst) +u-boot-spl-old-platdata := $(platdata-noinst) else -u-boot-spl-platdata := $(obj)/dts/dt-plat.o +u-boot-spl-platdata := $(platdata-noinst) +u-boot-spl-old-platdata := $(platdata-inst) endif + +# Files we need to generate u-boot-spl-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-platdata)) + +# Files we won't generate and should remove +u-boot-spl-old-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-old-platdata)) endif # OF_PLATDATA
# Linker Script @@ -334,6 +344,11 @@ $(obj)/dts/dt-%.o: $(obj)/dts/dt-%.c $(platdata-hdr) # Don't use dts_dir here, since it forces running this expensive rule every time $(platdata-hdr) $(u-boot-spl-platdata_c) &: $(obj)/$(SPL_BIN).dtb @[ -d $(obj)/dts ] || mkdir -p $(obj)/dts + @# Remove old files since which ones we generate depends on the setting + @# of OF_PLATDATA_INST and this might change between builds. Leaving old + @# ones around is confusing and it is possible that switching the + @# setting again will use the old one instead of regenerating it. + @rm -f $(u-boot-spl-all-platdata_c) $(u-boot-spl-all-platdata) $(call if_changed,dtoc)
ifdef CONFIG_SAMSUNG
Which files we generate depends on the setting of OF_PLATDATA_INST in the build. This might change between builds, but the build directory may be reused.
Leaving old files around is confusing and switching the OF_PLATDATA_INST setting does not necessarily regenerate the files, e.g. if the devicetree has not changed.
Remove all the files before regenerating new ones.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Only remove the old (and to be unused) files, so avoid confusing make
scripts/Makefile.spl | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
Applied to u-boot-dm/next, thanks!
participants (1)
-
Simon Glass