[PATCH 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.
Simon Glass (6): dtc: 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 | 36 +++++++++++++++++++++++------------ scripts/dtc/pylibfdt/Makefile | 8 ++++++-- tools/dtoc/dtb_platdata.py | 23 ++++++++++++++++++---- tools/dtoc/test_dtoc.py | 2 +- 4 files changed, 50 insertions(+), 19 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 ---
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
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 ---
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)
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 ---
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
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 ---
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):
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 ---
scripts/Makefile.spl | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 5f37a82931e..cf04016ff99 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -120,6 +120,7 @@ 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 @@ -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)
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 ---
scripts/Makefile.spl | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index cf04016ff99..f2628026f9f 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -121,10 +121,15 @@ 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 +u-boot-spl-all-platdata := $(platdata-inst) $(platdata-noinst) +u-boot-spl-all-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-all-platdata)) + 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) else -u-boot-spl-platdata := $(obj)/dts/dt-plat.o +u-boot-spl-platdata := $(platdata-noinst) endif u-boot-spl-platdata_c := $(patsubst %.o,%.c,$(u-boot-spl-platdata)) endif @@ -334,6 +339,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
participants (1)
-
Simon Glass