kbuild fixes

PATCH Superseded Masahiro Yamada 4 participants 11 messages Feb 3, 2016 – Feb 5, 2016 Kbuild

Share & tools

Masahiro Yamada

author

2016-02-03 12:05 UTC #0 lore raw

cover kbuild fixes

Masahiro Yamada (5):
kbuild: remove unneeded ifdef conditionals around build rules
kbuild: sunxi: fix build rule of sunxi-spl.bin
kbuild: use $(call cmd,) rather than $(call if_changed,) where
possible
kbuild: add missing FORCE where $(call if_changed,) is used
kbuild: fix build rule of u-boot-spl.dtb

 Makefile             | 36 ++++++++++++++++++------------------
 scripts/Makefile.spl | 24 ++++++++++--------------
 2 files changed, 28 insertions(+), 32 deletions(-)

Signature · 1 line

1.9.1

Masahiro Yamada

author

2016-02-03 12:05 UTC #1 lore raw

PATCH 1/5 kbuild: remove unneeded ifdef conditionals around build rules

These rules are only used for SOCFPGA, SUNXI, but no need to hide them from other SoCs.

Signed-off-by Masahiro Yamada <[email protected]>

---

 scripts/Makefile.spl | 4 ----
 1 file changed, 4 deletions(-)

scripts/Makefile.spl +0 -4

index f486feb..adabfcf 100644
@@ -228,18 +228,14 @@ ifneq ($(CONFIG_SPL_TEXT_BASE),)
 LDFLAGS_$(SPL_BIN) += -Ttext $(CONFIG_SPL_TEXT_BASE)
 endif
 
-ifdef CONFIG_ARCH_SOCFPGA
 MKIMAGEFLAGS_$(SPL_BIN).sfp = -T socfpgaimage
 $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
 	$(call if_changed,mkimage)
-endif
 
-ifdef CONFIG_SUNXI
 quiet_cmd_mksunxiboot = MKSUNXI $@
 cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $< $@
 $(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin
 	$(call if_changed,mksunxiboot)
-endif
 
 quiet_cmd_u-boot-spl = LD      $@
       cmd_u-boot-spl = (cd $(obj) && $(LD) $(LDFLAGS) $(LDFLAGS_$(@F)) \

Signature · 1 line

1.9.1

Masahiro Yamada

author

2016-02-03 12:05 UTC #2 lore raw

PATCH 2/5 kbuild: sunxi: fix build rule of sunxi-spl.bin

In reply to Masahiro Yamada

The build command for sunxi-spl.bin is constant, so it is pointless to use $(call if_changed,...). $(call cmd,...) is enough.

On the other hand, if the tools/mksunxiboot is updated, the resulted output may be different. Make the output image dependent on tools/mksunxiboot.

Signed-off-by Masahiro Yamada <[email protected]>

---

 scripts/Makefile.spl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

scripts/Makefile.spl +2 -2

index adabfcf..d8b3947 100644
@@ -234,8 +234,8 @@ $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE
 
 quiet_cmd_mksunxiboot = MKSUNXI $@
 cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $< $@
-$(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin
-	$(call if_changed,mksunxiboot)
+$(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin $(objtree)/tools/mksunxiboot
+	$(call cmd,mksunxiboot)
 
 quiet_cmd_u-boot-spl = LD      $@
       cmd_u-boot-spl = (cd $(obj) && $(LD) $(LDFLAGS) $(LDFLAGS_$(@F)) \

Signature · 1 line

1.9.1

Masahiro Yamada

author

2016-02-03 12:05 UTC #3 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

In reply to Masahiro Yamada

These build commands are constant (mostly, just concatenating images, or just copying). No need to use $(call if_changed,...) for them.

Signed-off-by Masahiro Yamada <[email protected]>

---

 Makefile             | 26 +++++++++++++-------------
 scripts/Makefile.spl |  4 ++--
 2 files changed, 15 insertions(+), 15 deletions(-)

Makefile +13 ~~-13~~

index 430dd4f..aaa5b44 100644
@@ -827,8 +827,8 @@ quiet_cmd_copy = COPY    $@
       cmd_copy = cp $< $@
 
 ifeq ($(CONFIG_OF_SEPARATE),y)
-u-boot-dtb.bin: u-boot-nodtb.bin dts/dt.dtb FORCE
-	$(call if_changed,cat)
+u-boot-dtb.bin: u-boot-nodtb.bin dts/dt.dtb
+	$(call cmd,cat)
 
 u-boot.bin: u-boot-dtb.bin FORCE
 	$(call if_changed,copy)

@@ -847,8 +847,8 @@ OBJCOPYFLAGS_u-boot.hex := -O ihex
 
 OBJCOPYFLAGS_u-boot.srec := -O srec
 
-u-boot.hex u-boot.srec: u-boot FORCE
-	$(call if_changed,objcopy)
+u-boot.hex u-boot.srec: u-boot
+	$(call cmd,objcopy)
 
 OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
 		$(if $(CONFIG_X86_RESET_VECTOR),-R .start16 -R .resetvec)

@@ -881,8 +881,8 @@ OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex
 
 OBJCOPYFLAGS_u-boot.ldr.srec := -I binary -O srec
 
-u-boot.ldr.hex u-boot.ldr.srec: u-boot.ldr FORCE
-	$(call if_changed,objcopy)
+u-boot.ldr.hex u-boot.ldr.srec: u-boot.ldr
+	$(call cmd,objcopy)
 
 #
 # U-Boot entry point, needed for booting of full-blown U-Boot

@@ -954,7 +954,7 @@ lpc32xx-boot-1.bin: lpc32xx-spl.img
 	$(call if_changed,objcopy)
 
 lpc32xx-full.bin: lpc32xx-boot-0.bin lpc32xx-boot-1.bin u-boot.img
-	$(call if_changed,cat)
+	$(call cmd,cat)
 
 CLEAN_FILES += lpc32xx-*

@@ -1060,8 +1060,8 @@ u-boot.rom: u-boot-x86-16bit.bin u-boot.bin
 	$(call if_changed,ifdtool)
 
 OBJCOPYFLAGS_u-boot-x86-16bit.bin := -O binary -j .start16 -j .resetvec
-u-boot-x86-16bit.bin: u-boot FORCE
-	$(call if_changed,objcopy)
+u-boot-x86-16bit.bin: u-boot
+	$(call cmd,objcopy)
 endif
 
 ifneq ($(CONFIG_SUNXI),)

@@ -1080,8 +1080,8 @@ OBJCOPYFLAGS_u-boot-tegra.bin = -O binary --pad-to=$(CONFIG_SYS_TEXT_BASE)
 u-boot-tegra.bin: spl/u-boot-spl u-boot.bin FORCE
 	$(call if_changed,pad_cat)
 
-u-boot-dtb-tegra.bin: u-boot-tegra.bin FORCE
-	$(call if_changed,copy)
+u-boot-dtb-tegra.bin: u-boot-tegra.bin
+	$(call cmd,copy)
 endif
 
 OBJCOPYFLAGS_u-boot-app.efi := $(OBJCOPYFLAGS_EFI)

@@ -1108,8 +1108,8 @@ OBJCOPYFLAGS_u-boot-payload.efi := $(OBJCOPYFLAGS_EFI)
 u-boot-payload.efi: u-boot-payload FORCE
 	$(call if_changed,zobjcopy)
 
-u-boot-img.bin: spl/u-boot-spl.bin u-boot.img FORCE
-	$(call if_changed,cat)
+u-boot-img.bin: spl/u-boot-spl.bin u-boot.img
+	$(call cmd,cat)
 
 #Add a target to create boot binary having SPL binary in PBI format
 #concatenated with u-boot binary. It is need by PowerPC SoC having

scripts/Makefile.spl +2 -2

index d8b3947..2668ef3 100644
@@ -167,8 +167,8 @@ quiet_cmd_copy = COPY    $@
 
 ifeq ($(CONFIG_SPL_OF_CONTROL),y)
 $(obj)/$(SPL_BIN)-dtb.bin: $(obj)/$(SPL_BIN)-nodtb.bin $(obj)/$(SPL_BIN)-pad.bin \
-		$(obj)/$(SPL_BIN).dtb FORCE
-	$(call if_changed,cat)
+		$(obj)/$(SPL_BIN).dtb
+	$(call cmd,cat)
 
 $(obj)/$(SPL_BIN).bin: $(obj)/$(SPL_BIN)-dtb.bin FORCE
 	$(call if_changed,copy)

Signature · 1 line

1.9.1

Albert ARIBAUD

2016-02-03 12:18 UTC #4 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

Hello Masahiro,

Quoted 2 lines · Masahiro Yamada

These build commands are constant (mostly, just concatenating images, or just copying). No need to use $(call if_changed,...) for them.

I am a total kbuild ignorant, so I'm probably asking a stupid question, but hey.

What are the exact semantics of $(call if_changed,)? I thought it meant "call the function if any of the dependencies has changed. If it does, then won't this patch make the corresponding action systematic, causing unneeded concatenations or copies?

Amicalement,

Signature · 1 line

Albert.

Albert ARIBAUD

2016-02-03 12:20 UTC #5 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

Hello Albert,

Quoted 7 lines · Albert ARIBAUD

Hello Masahiro,
Quoted 2 lines · Masahiro Yamada
These build commands are constant (mostly, just concatenating images, or just copying). No need to use $(call if_changed,...) for them.

Quoted 7 lines

I am a total kbuild ignorant, so I'm probably asking a stupid question, but hey.
What are the exact semantics of $(call if_changed,)? I thought it meant "call the function if any of the dependencies has changed. If it does, then won't this patch make the corresponding action systematic, causing unneeded concatenations or copies?

NVM, the answer could be found in later patches in the series.

Amicalement,

Signature · 1 line

Albert.

Stephen Warren

2016-02-03 15:57 UTC #6 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

In reply to Masahiro Yamada

Quoted 2 lines · Masahiro Yamada

These build commands are constant (mostly, just concatenating images, or just copying). No need to use $(call if_changed,...) for them.

I disagree, since I believe this change means that if someone /does/ change the command in the future (e.g. to replace it with more complex processing, or add additional dependencies), then the Makefile will/may not automatically rebuild those targets, which is the entire point of using if_changed, and is a huge benefit of using Kbuild.

In my opinion, every rule should use if_changed, and contain a single command at the Makefile level; i.e. I noticed the following somewhere, which also doesn't rebuild the target in all cases if the commands are changed, which is bad:

target: sources FORCE

     $(call if_changed,xxx)
     something_else
     yet_more_cmds

If "something_else" or "yet_more_cmds" are edited, the target won't get rebuilt unless some other modification causes it to be.

Masahiro Yamada

author

2016-02-05 05:33 UTC #7 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

Hi Stephen,

2016-02-04 0:57 GMT+09:00 Stephen Warren <[email protected]>:

Quoted 11 lines · Masahiro Yamada

Quoted 3 lines
These build commands are constant (mostly, just concatenating images, or just copying). No need to use $(call if_changed,...) for them.
I disagree, since I believe this change means that if someone /does/ change the command in the future (e.g. to replace it with more complex processing, or add additional dependencies), then the Makefile will/may not automatically rebuild those targets, which is the entire point of using if_changed, and is a huge benefit of using Kbuild.

I do not a strong opinion about this, so
I will drop 3/5 and submit v2.

Quoted 5 lines

In my opinion, every rule should use if_changed, and contain a single command at the Makefile level; i.e. I noticed the following somewhere, which also doesn't rebuild the target in all cases if the commands are changed, which is bad:

Quoted 7 lines

target: sources FORCE
    $(call if_changed,xxx)
    something_else
    yet_more_cmds
If "something_else" or "yet_more_cmds" are edited, the target won't get rebuilt unless some other modification causes it to be.

Patch are welcome to clean up them.

Signature · 2 lines

Best Regards
Masahiro Yamada

Ian Campbell

2016-02-05 08:11 UTC #8 lore raw

PATCH 3/5 kbuild: use $(call cmd, ) rather than $(call if_changed, ) where possible

Quoted 5 lines · Masahiro Yamada

Quoted 1 line
Quoted 1 line
These build commands are constant (mostly, just concatenating
images,
Quoted 3 lines
Quoted 1 line
or just copying). No need to use $(call if_changed,...) for them.

Quoted 8 lines

Quoted 5 lines
I disagree, since I believe this change means that if someone /does/ change the command in the future (e.g. to replace it with more complex processing, or add additional dependencies), then the Makefile will/may not automatically rebuild those targets, which is the entire point of using if_changed, and is a huge benefit of using Kbuild.
I do not a strong opinion about this, so
I will drop 3/5 and submit v2.

I think the same logic applies to that part of the change in patch 2 as well.

Ian.

Masahiro Yamada

author

2016-02-03 12:05 UTC #9 lore raw

PATCH 4/5 kbuild: add missing FORCE where $(call if_changed, ) is used

In reply to Masahiro Yamada

FORCE is needed for $(call if_changed,...) to be evaluated every time. Otherwise, the command is not executed when the command line has changed but any prerequisite has not been updated.

Signed-off-by Masahiro Yamada <[email protected]>

---

 Makefile             | 10 +++++-----
 scripts/Makefile.spl |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

Makefile +5 -5

index aaa5b44..ee29c1c 100644
@@ -924,7 +924,7 @@ u-boot.sha1:	u-boot.bin
 u-boot.dis:	u-boot
 		$(OBJDUMP) -d $< > $@
 
-u-boot.cfg:	include/config.h
+u-boot.cfg:	include/config.h FORCE
 	$(call if_changed,cpp_cfg)
 
 ifdef CONFIG_TPL

@@ -945,12 +945,12 @@ lpc32xx-spl.img: spl/u-boot-spl.bin FORCE
 
 OBJCOPYFLAGS_lpc32xx-boot-0.bin = -I binary -O binary --pad-to=$(CONFIG_SPL_PAD_TO)
 
-lpc32xx-boot-0.bin: lpc32xx-spl.img
+lpc32xx-boot-0.bin: lpc32xx-spl.img FORCE
 	$(call if_changed,objcopy)
 
 OBJCOPYFLAGS_lpc32xx-boot-1.bin = -I binary -O binary --pad-to=$(CONFIG_SPL_PAD_TO)
 
-lpc32xx-boot-1.bin: lpc32xx-spl.img
+lpc32xx-boot-1.bin: lpc32xx-spl.img FORCE
 	$(call if_changed,objcopy)
 
 lpc32xx-full.bin: lpc32xx-boot-0.bin lpc32xx-boot-1.bin u-boot.img

@@ -1056,7 +1056,7 @@ endif
 cmd_ifdtool += $(IFDTOOL) $(IFDTOOL_FLAGS) u-boot.tmp;
 cmd_ifdtool += mv u-boot.tmp $@
 
-u-boot.rom: u-boot-x86-16bit.bin u-boot.bin
+u-boot.rom: u-boot-x86-16bit.bin u-boot.bin FORCE
 	$(call if_changed,ifdtool)
 
 OBJCOPYFLAGS_u-boot-x86-16bit.bin := -O binary -j .start16 -j .resetvec

@@ -1171,7 +1171,7 @@ cmd_smap = \
 	$(CC) $(c_flags) -DSYSTEM_MAP="\"$${smap}\"" \
 		-c $(srctree)/common/system_map.c -o common/system_map.o
 
-u-boot:	$(u-boot-init) $(u-boot-main) u-boot.lds
+u-boot:	$(u-boot-init) $(u-boot-main) u-boot.lds FORCE
 	$(call if_changed,u-boot__)
 ifeq ($(CONFIG_KALLSYMS),y)
 	$(call cmd,smap)

scripts/Makefile.spl +4 -4

index 2668ef3..e1af6c8 100644
@@ -114,7 +114,7 @@ MKIMAGEFLAGS_MLO = -T omapimage -a $(CONFIG_SPL_TEXT_BASE)
 
 MKIMAGEFLAGS_MLO.byteswap = -T omapimage -n byteswap -a $(CONFIG_SPL_TEXT_BASE)
 
-MLO MLO.byteswap: $(obj)/u-boot-spl.bin
+MLO MLO.byteswap: $(obj)/u-boot-spl.bin FORCE
 	$(call if_changed,mkimage)
 
 ifeq ($(CONFIG_SYS_SOC),"at91")

@@ -126,12 +126,12 @@ MKIMAGEFLAGS_boot.bin += -n $(shell $(obj)/../tools/atmel_pmecc_params)
 boot.bin: $(obj)/../tools/atmel_pmecc_params
 endif
 
-boot.bin: $(obj)/u-boot-spl.bin
+boot.bin: $(obj)/u-boot-spl.bin FORCE
 	$(call if_changed,mkimage)
 else
 MKIMAGEFLAGS_boot.bin = -T zynqimage
 
-spl/boot.bin: $(obj)/u-boot-spl.bin
+spl/boot.bin: $(obj)/u-boot-spl.bin FORCE
 	$(call if_changed,mkimage)
 endif

@@ -200,7 +200,7 @@ quiet_cmd_cpp_cfg = CFG     $@
 cmd_cpp_cfg = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) -ansi \
 	-DDO_DEPS_ONLY -D__ASSEMBLY__ -x assembler-with-cpp -P -dM -E -o $@ $<
 
-$(obj)/$(SPL_BIN).cfg:	include/config.h
+$(obj)/$(SPL_BIN).cfg:	include/config.h FORCE
 	$(call if_changed,cpp_cfg)
 
 ifdef CONFIG_SAMSUNG

Signature · 1 line

1.9.1

Masahiro Yamada

author

2016-02-03 12:05 UTC #10 lore raw

PATCH 5/5 kbuild: fix build rule of u-boot-spl.dtb

In reply to Masahiro Yamada

The build command of u-boot-spl.dtb is not constant, but dependent on CONFIG_OF_SPL_REMOVE_PROPS. Use $(call if_changed,...) so that the change of CONFIG_OF_SPL_REMOVE_PROPS is detected.

Signed-off-by Masahiro Yamada <[email protected]>

---

 scripts/Makefile.spl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

scripts/Makefile.spl +2 -2

index e1af6c8..8942ffb 100644
@@ -193,8 +193,8 @@ quiet_cmd_fdtgrep = FDTGREP $@
 	$(objtree)/tools/fdtgrep -r -O dtb - -o $@ \
 		$(addprefix -P ,$(subst $\",,$(CONFIG_OF_SPL_REMOVE_PROPS)))
 
-$(obj)/$(SPL_BIN).dtb: dts/dt.dtb
-	$(call cmd,fdtgrep)
+$(obj)/$(SPL_BIN).dtb: dts/dt.dtb FORCE
+	$(call if_changed,fdtgrep)
 
 quiet_cmd_cpp_cfg = CFG     $@
 cmd_cpp_cfg = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) -ansi \

Signature · 1 line

1.9.1