| Message ID | 571E0683.4080306@laposte.net |
|---|---|
| State | Superseded |
| Headers | show |
Hi Thomas, On 04/25/2016 02:06 PM, Thomas Petazzoni wrote: > Hello, > > Thanks for reporting the problem, and thanks for your patch! Sure! That's what Open Source is for! :-) > >> diff --git a/linux/linux.mk b/linux/linux.mk >> index 317587f..1f93074 100644 >> --- a/linux/linux.mk >> +++ b/linux/linux.mk >> @@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) >> # appended DTBs. >> define LINUX_INSTALL_IMAGE >> mkdir -p $(1) >> - cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1) >> + cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1) > > I think you did this change for debugging (which is OK), but it should > be kept in your patch. Ok, I will remove it. > >> endef >> else >> # Otherwise, just install the unique image generated by the kernel >> @@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt: >> $(LINUX_DIR)/.stamp_target_installed $(LI >> # Build the kernel. >> $(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) >> $(LINUX_TARGET_NAME) This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? LINUX_BUILD_CMDS does a couple of more things than what is done here, like like running LINUX_BUILD_DTB or 'make modules', and maybe that is not wanted? >> $(LINUX_APPEND_DTB) >> - # Copy the kernel image to its final destination >> - cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR) >> + # Copy the kernel image(s) to its(their) final destination >> + $(LINUX_INSTALL_IMAGES_CMDS) > > I think I would prefer to have: > > $(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR)) > Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also handles the DTB copy when APPENDED_DTB is not set, right? Best regards, Sebastian
Hello, On Mon, 25 Apr 2016 14:17:50 +0200, Sebastian Frias wrote: > >> # Build the kernel. > >> $(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) > >> $(LINUX_TARGET_NAME) > > This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? > LINUX_BUILD_CMDS does a couple of more things than what is done here, > like like running LINUX_BUILD_DTB or 'make modules', and maybe that is > not wanted? We don't want to do "make modules" again, as this will change the root filesystem (surely by reinstalling the same thing, but still), while we are exactly in the process of adding the filesystem to the kernel image. > > I think I would prefer to have: > > > > $(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR)) > > Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also > handles the DTB copy when APPENDED_DTB is not set, right? It also copies the DTB to $(BINARIES_DIR), which is useless here. And in general, I don't like too much seeing the _CMDS variables called from other variables. Thomas
Hi Thomas, On 04/25/2016 02:23 PM, Thomas Petazzoni wrote: > Hello, > > On Mon, 25 Apr 2016 14:17:50 +0200, Sebastian Frias wrote: > >>>> # Build the kernel. >>>> $(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) >>>> $(LINUX_TARGET_NAME) >> >> This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? >> LINUX_BUILD_CMDS does a couple of more things than what is done here, >> like like running LINUX_BUILD_DTB or 'make modules', and maybe that is >> not wanted? > > We don't want to do "make modules" again, as this will change the root > filesystem (surely by reinstalling the same thing, but still), while we > are exactly in the process of adding the filesystem to the kernel image. Ok, thanks for the explanation. > >>> I think I would prefer to have: >>> >>> $(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR)) >> >> Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also >> handles the DTB copy when APPENDED_DTB is not set, right? > > It also copies the DTB to $(BINARIES_DIR), which is useless here. And in > general, I don't like too much seeing the _CMDS variables called from > other variables. Ok, just one last question because I seem to be missing something here. In your commit 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") you indicated: ----- <snip> Some of the tested configuration: - Normal uImage with several DTBs BR2_LINUX_KERNEL_DEFCONFIG="mvebu_v7" BR2_LINUX_KERNEL_UIMAGE=y BR2_LINUX_KERNEL_UIMAGE_LOADADDR="0x200000" BR2_LINUX_KERNEL_DTS_SUPPORT=y BR2_LINUX_KERNEL_INTREE_DTS_NAME="armada-xp-matrix armada-xp-gp armada-370-mirabox" Contents of output/images/: armada-370-mirabox.dtb armada-xp-gp.dtb armada-xp-matrix.dtb uImage <snip> ----- Hence, the DTB images are intended to be copied to output/images/ under some circumstances, right? How would that work? I mean, there's no call to LINUX_INSTALL_DTB on "$(LINUX_DIR)/.stamp_initramfs_rebuilt". Or does that happens through LINUX_INSTALL_TARGET_CMDS and package/pkg-generic.mk's "$(BUILD_DIR)/%/.stamp_target_installed" target? In that case I think I don't understand why we enter in "$(LINUX_DIR)/.stamp_initramfs_rebuilt" target, given there's a target $(BUILD_DIR)/%/.stamp_images_installed that calls LINUX_INSTALL_IMAGES_CMDS. Best regards, Sebastian
diff --git a/linux/linux.mk b/linux/linux.mk index 317587f..1f93074 100644 --- a/linux/linux.mk +++ b/linux/linux.mk @@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) # appended DTBs. define LINUX_INSTALL_IMAGE mkdir -p $(1) - cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1) + cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1) endef else # Otherwise, just install the unique image generated by the kernel @@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed $(LI # Build the kernel. $(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_TARGET_NAME) $(LINUX_APPEND_DTB) - # Copy the kernel image to its final destination - cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR) + # Copy the kernel image(s) to its(their) final destination + $(LINUX_INSTALL_IMAGES_CMDS) # If there is a .ub file copy it to the final destination
The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used. Indeed, it copies a single image, and does not copy the one with the DTB appended. This patch replaces the 'cp' command with $(LINUX_INSTALL_IMAGES_CMDS) which handles APPENDED_DTB. It also makes the copy verbose so that the user can see how many images were copied. Fixes: 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") Signed-off-by: Sebastian Frias <sf84@laposte.net> --- linux/linux.mk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) test ! -f $(LINUX_IMAGE_PATH).ub || cp $(LINUX_IMAGE_PATH).ub $(BINARIES_DIR) $(Q)touch $@