When I was trying to fix commit 0f9da844d877 in a more correct way,
I found various problems in arch/mips/boot/Makefile.
ITS is always rebuilt when you rebuild the kernel without touching
anything. Many build rules are wrong.
If you look at the last patch in this series, you may realize
supporting ITB building in the kernel can cause nasty problems.
Personally, I do not like it, but I tried my best to fix the problems.
With this series, ITB is rebuilt only when necessary.
Masahiro Yamada (7):
Revert "MIPS: boot: Define __ASSEMBLY__ for its.S build"
MIPS: boot: do not include $(cpp_flags) for preprocessing ITS
MIPS: boot: fix build rule of vmlinux.its.S
MIPS: boot: correct prerequisite image of vmlinux.*.its
MIPS: boot: add missing targets for vmlinux.*.its
MIPS: boot: merge build rules of vmlinux.*.itb by using pattern rule
MIPS: boot: rebuild ITB when contained DTB is updated
arch/mips/boot/Makefile | 69 ++++++++++++++++++++++++++-----------------------
1 file changed, 37 insertions(+), 32 deletions(-)
--
2.7.4
Merge the build rule of vmlinux.{gz,bz2,lzma,lzo}.itb, and also move
'targets' close to the related code.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index 91d9fe8..d102d53 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -105,12 +105,6 @@ $(obj)/uImage: $(obj)/uImage.$(suffix-y)
# Flattened Image Tree (.itb) images
#
-targets += vmlinux.itb
-targets += vmlinux.gz.itb
-targets += vmlinux.bz2.itb
-targets += vmlinux.lzma.itb
-targets += vmlinux.lzo.itb
-
ifeq ($(ADDR_BITS),32)
itb_addr_cells = 1
endif
@@ -157,6 +151,12 @@ $(obj)/vmlinux.lzma.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.lzma FORCE
$(obj)/vmlinux.lzo.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.lzo FORCE
$(call if_changed,cpp_its_S,lzo,vmlinux.bin.lzo)
+targets += vmlinux.itb
+targets += vmlinux.gz.itb
+targets += vmlinux.bz2.itb
+targets += vmlinux.lzma.itb
+targets += vmlinux.lzo.itb
+
quiet_cmd_itb-image = ITB $@
cmd_itb-image = \
env PATH="$(objtree)/scripts/dtc:$(PATH)" \
@@ -169,14 +169,5 @@ quiet_cmd_itb-image = ITB $@
$(obj)/vmlinux.itb: $(obj)/vmlinux.its $(obj)/vmlinux.bin FORCE
$(call if_changed,itb-image,$<)
-$(obj)/vmlinux.gz.itb: $(obj)/vmlinux.gz.its $(obj)/vmlinux.bin.gz FORCE
- $(call if_changed,itb-image,$<)
-
-$(obj)/vmlinux.bz2.itb: $(obj)/vmlinux.bz2.its $(obj)/vmlinux.bin.bz2 FORCE
- $(call if_changed,itb-image,$<)
-
-$(obj)/vmlinux.lzma.itb: $(obj)/vmlinux.lzma.its $(obj)/vmlinux.bin.lzma FORCE
- $(call if_changed,itb-image,$<)
-
-$(obj)/vmlinux.lzo.itb: $(obj)/vmlinux.lzo.its $(obj)/vmlinux.bin.lzo FORCE
+$(obj)/vmlinux.%.itb: $(obj)/vmlinux.%.its $(obj)/vmlinux.bin.% FORCE
$(call if_changed,itb-image,$<)
--
2.7.4
As Documentation/kbuild/makefile.txt says, it is a typical mistake
to forget the FORCE prerequisite for the rule invoked by if_changed.
Add the FORCE to the prerequisite, but it must be filtered-out from
the files passed to the 'cat' command. Because this rule generates
.vmlinux.its.S.cmd, vmlinux.its.S must be specified as targets so
that the .cmd file is included.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index 981bcf8..6c7054e 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -118,10 +118,12 @@ ifeq ($(ADDR_BITS),64)
itb_addr_cells = 2
endif
+targets += vmlinux.its.S
+
quiet_cmd_its_cat = CAT $@
- cmd_its_cat = cat $^ >$@
+ cmd_its_cat = cat $(filter-out $(PHONY), $^) >$@
-$(obj)/vmlinux.its.S: $(addprefix $(srctree)/arch/mips/$(PLATFORM)/,$(ITS_INPUTS))
+$(obj)/vmlinux.its.S: $(addprefix $(srctree)/arch/mips/$(PLATFORM)/,$(ITS_INPUTS)) FORCE
$(call if_changed,its_cat)
quiet_cmd_cpp_its_S = ITS $@
--
2.7.4
The build rule of vmlinux.*.its is invoked by $(call if_changed,...)
but it always rebuilds the target needlessly due to missing targets.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index 30512ab..91d9fe8 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -126,6 +126,12 @@ quiet_cmd_its_cat = CAT $@
$(obj)/vmlinux.its.S: $(addprefix $(srctree)/arch/mips/$(PLATFORM)/,$(ITS_INPUTS)) FORCE
$(call if_changed,its_cat)
+targets += vmlinux.its
+targets += vmlinux.gz.its
+targets += vmlinux.bz2.its
+targets += vmlinux.lzmo.its
+targets += vmlinux.lzo.its
+
quiet_cmd_cpp_its_S = ITS $@
cmd_cpp_its_S = $(CPP) -P -C -o $@ $< \
-DKERNEL_NAME="\"Linux $(KERNELRELEASE)\"" \
--
2.7.4
This reverts commit 0f9da844d87796ac31b04e81ee95e155e9043132.
It is true that commit 0f9da844d877 ("MIPS: boot: Define __ASSEMBLY__
for its.S build") fixed the build error, but it should not have
defined __ASSEMBLY__ just for textual substitution in arbitrary data.
The file is image tree source in this case, but the purpose of using
CPP is to replace some macros.
I merged a better solution, commit a95b37e20db9 ("kbuild: get
<linux/compiler_types.h> out of <linux/kconfig.h>"). The original
fix-up is no longer needed.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index c22da16..1bd5c4f 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -126,7 +126,6 @@ $(obj)/vmlinux.its.S: $(addprefix $(srctree)/arch/mips/$(PLATFORM)/,$(ITS_INPUTS
quiet_cmd_cpp_its_S = ITS $@
cmd_cpp_its_S = $(CPP) $(cpp_flags) -P -C -o $@ $< \
- -D__ASSEMBLY__ \
-DKERNEL_NAME="\"Linux $(KERNELRELEASE)\"" \
-DVMLINUX_BINARY="\"$(3)\"" \
-DVMLINUX_COMPRESSION="\"$(2)\"" \
--
2.7.4
$(CPP) is used here to perform macro replacement in ITS. Do not
pass $(cpp_flags) because it pulls in more options for dependency
file generation etc. but none of which is necessary here. ITS files
do not include any header file, so $(call if_change,...) is enough.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index 1bd5c4f..981bcf8 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -125,7 +125,7 @@ $(obj)/vmlinux.its.S: $(addprefix $(srctree)/arch/mips/$(PLATFORM)/,$(ITS_INPUTS
$(call if_changed,its_cat)
quiet_cmd_cpp_its_S = ITS $@
- cmd_cpp_its_S = $(CPP) $(cpp_flags) -P -C -o $@ $< \
+ cmd_cpp_its_S = $(CPP) -P -C -o $@ $< \
-DKERNEL_NAME="\"Linux $(KERNELRELEASE)\"" \
-DVMLINUX_BINARY="\"$(3)\"" \
-DVMLINUX_COMPRESSION="\"$(2)\"" \
@@ -135,19 +135,19 @@ quiet_cmd_cpp_its_S = ITS $@
-DADDR_CELLS=$(itb_addr_cells)
$(obj)/vmlinux.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
- $(call if_changed_dep,cpp_its_S,none,vmlinux.bin)
+ $(call if_changed,cpp_its_S,none,vmlinux.bin)
$(obj)/vmlinux.gz.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
- $(call if_changed_dep,cpp_its_S,gzip,vmlinux.bin.gz)
+ $(call if_changed,cpp_its_S,gzip,vmlinux.bin.gz)
$(obj)/vmlinux.bz2.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
- $(call if_changed_dep,cpp_its_S,bzip2,vmlinux.bin.bz2)
+ $(call if_changed,cpp_its_S,bzip2,vmlinux.bin.bz2)
$(obj)/vmlinux.lzma.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
- $(call if_changed_dep,cpp_its_S,lzma,vmlinux.bin.lzma)
+ $(call if_changed,cpp_its_S,lzma,vmlinux.bin.lzma)
$(obj)/vmlinux.lzo.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
- $(call if_changed_dep,cpp_its_S,lzo,vmlinux.bin.lzo)
+ $(call if_changed,cpp_its_S,lzo,vmlinux.bin.lzo)
quiet_cmd_itb-image = ITB $@
cmd_itb-image = \
--
2.7.4
vmlinux.*.its does not directly depend on $(VMLINUX) but
vmlinux.bin.*
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index 6c7054e..30512ab 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -136,19 +136,19 @@ quiet_cmd_cpp_its_S = ITS $@
-DADDR_BITS=$(ADDR_BITS) \
-DADDR_CELLS=$(itb_addr_cells)
-$(obj)/vmlinux.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
+$(obj)/vmlinux.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin FORCE
$(call if_changed,cpp_its_S,none,vmlinux.bin)
-$(obj)/vmlinux.gz.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
+$(obj)/vmlinux.gz.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.gz FORCE
$(call if_changed,cpp_its_S,gzip,vmlinux.bin.gz)
-$(obj)/vmlinux.bz2.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
+$(obj)/vmlinux.bz2.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.bz2 FORCE
$(call if_changed,cpp_its_S,bzip2,vmlinux.bin.bz2)
-$(obj)/vmlinux.lzma.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
+$(obj)/vmlinux.lzma.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.lzma FORCE
$(call if_changed,cpp_its_S,lzma,vmlinux.bin.lzma)
-$(obj)/vmlinux.lzo.its: $(obj)/vmlinux.its.S $(VMLINUX) FORCE
+$(obj)/vmlinux.lzo.its: $(obj)/vmlinux.its.S $(obj)/vmlinux.bin.lzo FORCE
$(call if_changed,cpp_its_S,lzo,vmlinux.bin.lzo)
quiet_cmd_itb-image = ITB $@
--
2.7.4
Since now, the unnecessary rebuild of ITB has been fixed. Another
problem to be taken care of is, missed rebuild of ITB.
For example, board-boston.its.S includes boston.dtb by the /incbin/
directive. If boston.dtb is updated, vmlinux.*.dtb must be rebuilt.
Currently, the dependency between ITB and contained DTB files is not
described anywhere. Previously, this problem was hidden since
vmlinux.*.itb was always rebuilt even if nothing is updated. By
fixing the spurious rebuild, this is a real problem now.
Use the same strategy for automatic generation of the header file
dependency. DTC works as a backend of mkimage, and DTC supports -d
option. It outputs the dependencies, including binary files pulled
by the /incbin/ directive.
The implementation is simpler than cmd_dtc in scripts/Makefile.lib
since we do not need CPP here. Just pass -d $(depfile) to DTC, and
let the resulted $(depfile) processed by fixdep.
It might be unclear why "$(obj)/dts/%.dtb: ;" is needed. With this
commit, *.cmd files will contain dependency on DTB files. In the
next invocation of build, the *.cmd files will be included, then
Make will try to find a rule to update *.dtb files. Unfortunately,
it is found in scripts/Makefile.lib. The build rule of $(obj)/%.dtb
is invoked by if_changed_dep, so it needs to include *.cmd files
of DTB, but they are not included because we are in arch/mips/boot,
but those *.cmd files reside in arch/mips/boot/dts/*/. Cancel the
pattern rule in scripts/Makefile.lib to suppress unneeded rebuilding
of DTB.
Signed-off-by: Masahiro Yamada <[email protected]>
---
arch/mips/boot/Makefile | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/mips/boot/Makefile b/arch/mips/boot/Makefile
index d102d53..f8dce5b 100644
--- a/arch/mips/boot/Makefile
+++ b/arch/mips/boot/Makefile
@@ -163,11 +163,18 @@ quiet_cmd_itb-image = ITB $@
$(CONFIG_SHELL) $(MKIMAGE) \
-D "-I dts -O dtb -p 500 \
--include $(objtree)/arch/mips \
- --warning no-unit_address_vs_reg" \
+ --warning no-unit_address_vs_reg \
+ -d $(depfile)" \
-f $(2) $@
$(obj)/vmlinux.itb: $(obj)/vmlinux.its $(obj)/vmlinux.bin FORCE
- $(call if_changed,itb-image,$<)
+ $(call if_changed_dep,itb-image,$<)
$(obj)/vmlinux.%.itb: $(obj)/vmlinux.%.its $(obj)/vmlinux.bin.% FORCE
- $(call if_changed,itb-image,$<)
+ $(call if_changed_dep,itb-image,$<)
+
+# The -d option of DTC outputs dependencies of binaries included by the
+# /incbin/ directive. When .*.cmd files are included, Kbuild tries to
+# update *.dtb because it sees a pattern rule defined in scripts/Makefile.lib.
+# The rule must be cancelled by a more specific rule.
+$(obj)/dts/%.dtb: ;
--
2.7.4
Hi MIPS maintainers,
2018-04-16 23:47 GMT+09:00 Masahiro Yamada <[email protected]>:
>
> When I was trying to fix commit 0f9da844d877 in a more correct way,
> I found various problems in arch/mips/boot/Makefile.
>
> ITS is always rebuilt when you rebuild the kernel without touching
> anything. Many build rules are wrong.
>
> If you look at the last patch in this series, you may realize
> supporting ITB building in the kernel can cause nasty problems.
> Personally, I do not like it, but I tried my best to fix the problems.
>
> With this series, ITB is rebuilt only when necessary.
>
Is any part of this series applicable?
At least, patch 1-4 look simple to me.
Patch 5-7 should be applied together.
The last one is a bit tricky, so you can ignore 5-7 if you do not like them.
(Only the problem is, ITB files are always rebuilt even if nothing is changed.)
>
> Masahiro Yamada (7):
> Revert "MIPS: boot: Define __ASSEMBLY__ for its.S build"
> MIPS: boot: do not include $(cpp_flags) for preprocessing ITS
> MIPS: boot: fix build rule of vmlinux.its.S
> MIPS: boot: correct prerequisite image of vmlinux.*.its
> MIPS: boot: add missing targets for vmlinux.*.its
> MIPS: boot: merge build rules of vmlinux.*.itb by using pattern rule
> MIPS: boot: rebuild ITB when contained DTB is updated
>
> arch/mips/boot/Makefile | 69 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 37 insertions(+), 32 deletions(-)
>
> --
> 2.7.4
>
--
Best Regards
Masahiro Yamada
Hi Masahiro,
Thanks for these - I've applied patches 1-3, 5 & 6 to mips-next for
4.19.
On Mon, Apr 16, 2018 at 07:47:44AM -0700, Masahiro Yamada wrote:
> vmlinux.*.its does not directly depend on $(VMLINUX) but
> vmlinux.bin.*
This isn't really true - to generate the .its we actually do depend on
the ELF, which we read the entry address from (entry-y in
arch/mips/Makefile, provided to arch/mips/boot/Makefile as
VMLINUX_ENTRY_ADDRESS). In practice $(VMLINUX) is built before this
makefile is ever invoked anyway, but the dependency is there.
We don't need the vmlinux.bin.* file until we generate the .itb, so it
should be fine for the .its & .bin steps to be executed in parallel
which this patch would prevent.
Thanks,
Paul
Hi Masahiro,
On Mon, Apr 16, 2018 at 07:47:47AM -0700, Masahiro Yamada wrote:
> Since now, the unnecessary rebuild of ITB has been fixed. Another
> problem to be taken care of is, missed rebuild of ITB.
>
> For example, board-boston.its.S includes boston.dtb by the /incbin/
> directive. If boston.dtb is updated, vmlinux.*.dtb must be rebuilt.
> Currently, the dependency between ITB and contained DTB files is not
> described anywhere. Previously, this problem was hidden since
> vmlinux.*.itb was always rebuilt even if nothing is updated. By
> fixing the spurious rebuild, this is a real problem now.
>
> Use the same strategy for automatic generation of the header file
> dependency. DTC works as a backend of mkimage, and DTC supports -d
> option. It outputs the dependencies, including binary files pulled
> by the /incbin/ directive.
>
> The implementation is simpler than cmd_dtc in scripts/Makefile.lib
> since we do not need CPP here. Just pass -d $(depfile) to DTC, and
> let the resulted $(depfile) processed by fixdep.
>
> It might be unclear why "$(obj)/dts/%.dtb: ;" is needed. With this
> commit, *.cmd files will contain dependency on DTB files. In the
> next invocation of build, the *.cmd files will be included, then
> Make will try to find a rule to update *.dtb files. Unfortunately,
> it is found in scripts/Makefile.lib. The build rule of $(obj)/%.dtb
> is invoked by if_changed_dep, so it needs to include *.cmd files
> of DTB, but they are not included because we are in arch/mips/boot,
> but those *.cmd files reside in arch/mips/boot/dts/*/. Cancel the
> pattern rule in scripts/Makefile.lib to suppress unneeded rebuilding
> of DTB.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> arch/mips/boot/Makefile | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
Thanks - this looks good to me except that it starts outputting a "Don't
know how to preprocess itb-image" message after building the .itb.
I presume we just need an extra case adding to ksym_dep_filter. Do you
want to add that in & resubmit this one?
Thanks,
Paul
Hi Paul,
2018-06-20 9:22 GMT+09:00 Paul Burton <[email protected]>:
> Hi Masahiro,
>
> Thanks for these - I've applied patches 1-3, 5 & 6 to mips-next for
> 4.19.
>
> On Mon, Apr 16, 2018 at 07:47:44AM -0700, Masahiro Yamada wrote:
>> vmlinux.*.its does not directly depend on $(VMLINUX) but
>> vmlinux.bin.*
>
> This isn't really true - to generate the .its we actually do depend on
> the ELF, which we read the entry address from (entry-y in
> arch/mips/Makefile, provided to arch/mips/boot/Makefile as
> VMLINUX_ENTRY_ADDRESS). In practice $(VMLINUX) is built before this
> makefile is ever invoked anyway, but the dependency is there.
>
> We don't need the vmlinux.bin.* file until we generate the .itb, so it
> should be fine for the .its & .bin steps to be executed in parallel
> which this patch would prevent.
You are right.
Thanks for catching this.
--
Best Regards
Masahiro Yamada