[Buildroot] [PATCH] linux: Fix uImage with appended DTs generation

[Buildroot] [PATCH] linux: Fix uImage with appended DTs generation

[Maxime Ripard]

Fixes bug #5516 - appended device tree blobs on uImage fails

Before version 3.7 of the kernel, building the zImage and then the
uImage will rewrite the zImage in the process, removing the device tree
we just appended.

Use mkimage to append the device tree to the uImage and rebuild the
headers directly.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 linux/linux.mk | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index 3877c35..a1166e5 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -228,9 +228,17 @@ define LINUX_APPEND_DTB
 	fi >> $(KERNEL_ARCH_PATH)/boot/zImage
 endef
 ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
-# We need to generate the uImage here after that so that the uImage is
-# generated with the right image size.
-LINUX_APPEND_DTB += $(sep)$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) uImage
+# We need to generate a new u-boot image that takes into
+# account the extra-size added by the device tree at the end
+# of the image. To do so, we first need to retrieve both load
+# address and entry point for the kernel from the already
+# generate uboot image before using mkimage -l.
+LINUX_APPEND_DTB += $(sep) LOAD=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
+                    sed -n 's/Load Address: \([0-9]*\)/\1/p'`; \
+                    ENTRY=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
+                    sed -n 's/Entry Point: \([0-9]*\)/\1/p'`; \
+                    $(HOST_DIR)/usr/bin/mkimage -A $(KERNEL_ARCH) -O linux -T kernel -C none -a $${LOAD} -e $${ENTRY} \
+                    -n 'Linux Buildroot' -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH);
 endif
 endif
 
-- 
1.8.2.3

[Buildroot] [PATCH] linux: Fix uImage with appended DTs generation

[Peter Korsgaard]

>>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes:

 Maxime> Fixes bug #5516 - appended device tree blobs on uImage fails
 Maxime> Before version 3.7 of the kernel, building the zImage and then the
 Maxime> uImage will rewrite the zImage in the process, removing the device tree
 Maxime> we just appended.

 Maxime> Use mkimage to append the device tree to the uImage and rebuild the
 Maxime> headers directly.

 Maxime> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
 Maxime> ---
 Maxime>  linux/linux.mk | 14 +++++++++++---
 Maxime>  1 file changed, 11 insertions(+), 3 deletions(-)

 Maxime> diff --git a/linux/linux.mk b/linux/linux.mk
 Maxime> index 3877c35..a1166e5 100644
 Maxime> --- a/linux/linux.mk
 Maxime> +++ b/linux/linux.mk
 Maxime> @@ -228,9 +228,17 @@ define LINUX_APPEND_DTB
 Maxime>  	fi >> $(KERNEL_ARCH_PATH)/boot/zImage
 Maxime>  endef
 Maxime>  ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
 Maxime> -# We need to generate the uImage here after that so that the uImage is
 Maxime> -# generated with the right image size.
 Maxime> -LINUX_APPEND_DTB += $(sep)$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) uImage
 Maxime> +# We need to generate a new u-boot image that takes into
 Maxime> +# account the extra-size added by the device tree at the end
 Maxime> +# of the image. To do so, we first need to retrieve both load
 Maxime> +# address and entry point for the kernel from the already
 Maxime> +# generate uboot image before using mkimage -l.

This doesn't seem right. APPENDED_UIMAGE implies APPENDED_DTB, which
only builds a zImage, so there isn't any uImage to get the load address
/ entry point from.


 Maxime> +LINUX_APPEND_DTB += $(sep) LOAD=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
 Maxime> +                    sed -n 's/Load Address: \([0-9]*\)/\1/p'`; \
 Maxime> +                    ENTRY=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
 Maxime> +                    sed -n 's/Entry Point: \([0-9]*\)/\1/p'`; \
 Maxime> +                    $(HOST_DIR)/usr/bin/mkimage -A $(KERNEL_ARCH) -O linux -T kernel -C none -a $${LOAD} -e $${ENTRY} \
 Maxime> +                    -n 'Linux Buildroot' -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH);

These regexps don't make much sense to me as the values are in
hexidecimal (E.G. prefixed with 0x) and the Entry Point: line as an
extra space before the numbers.

It would also be good to keep the previous name instead of hardcoding
Linux Buildroot.

I think it is simpler and cleaner to simply create the mkimage command
line with a single sed invocation, E.G. something like:

LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
	sed -n -e 's/Image Name:[ ]*\(.*\)/-n "\1"/p' -e 's/Load Address:/-a/' -e 's/Entry Point:/-e/'`; \
	$(HOST_DIR)/usr/bin/mkimage -A $(KERNEL_ARCH) -O linux \
	-T kernel -C none $${MKIMAGE_ARGS} \
	-d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH);

(Construct the needed -n / -a / -e options in place).

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH] linux: Fix uImage with appended DTs generation

[Maxime Ripard]

Hi Peter,

On Thu, May 30, 2013 at 02:07:10PM +0200, Peter Korsgaard wrote:
> >>>>> "Maxime" == Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
>  Maxime> Fixes bug #5516 - appended device tree blobs on uImage fails
>  Maxime> Before version 3.7 of the kernel, building the zImage and then the
>  Maxime> uImage will rewrite the zImage in the process, removing the device tree
>  Maxime> we just appended.
> 
>  Maxime> Use mkimage to append the device tree to the uImage and rebuild the
>  Maxime> headers directly.
> 
>  Maxime> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>  Maxime> ---
>  Maxime>  linux/linux.mk | 14 +++++++++++---
>  Maxime>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
>  Maxime> diff --git a/linux/linux.mk b/linux/linux.mk
>  Maxime> index 3877c35..a1166e5 100644
>  Maxime> --- a/linux/linux.mk
>  Maxime> +++ b/linux/linux.mk
>  Maxime> @@ -228,9 +228,17 @@ define LINUX_APPEND_DTB
>  Maxime>  	fi >> $(KERNEL_ARCH_PATH)/boot/zImage
>  Maxime>  endef
>  Maxime>  ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
>  Maxime> -# We need to generate the uImage here after that so that the uImage is
>  Maxime> -# generated with the right image size.
>  Maxime> -LINUX_APPEND_DTB += $(sep)$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) uImage
>  Maxime> +# We need to generate a new u-boot image that takes into
>  Maxime> +# account the extra-size added by the device tree at the end
>  Maxime> +# of the image. To do so, we first need to retrieve both load
>  Maxime> +# address and entry point for the kernel from the already
>  Maxime> +# generate uboot image before using mkimage -l.
> 
> This doesn't seem right. APPENDED_UIMAGE implies APPENDED_DTB, which
> only builds a zImage, so there isn't any uImage to get the load address
> / entry point from.

Hmmm, right.

I don't see how it could have worked in the first place during my tests,
except maybe because of an uncleaned workspace. I'll send a v2.

> 
>  Maxime> +LINUX_APPEND_DTB += $(sep) LOAD=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
>  Maxime> +                    sed -n 's/Load Address: \([0-9]*\)/\1/p'`; \
>  Maxime> +                    ENTRY=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
>  Maxime> +                    sed -n 's/Entry Point: \([0-9]*\)/\1/p'`; \
>  Maxime> +                    $(HOST_DIR)/usr/bin/mkimage -A $(KERNEL_ARCH) -O linux -T kernel -C none -a $${LOAD} -e $${ENTRY} \
>  Maxime> +                    -n 'Linux Buildroot' -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH);
> 
> These regexps don't make much sense to me as the values are in
> hexidecimal (E.G. prefixed with 0x) and the Entry Point: line as an
> extra space before the numbers.
> 
> It would also be good to keep the previous name instead of hardcoding
> Linux Buildroot.
> 
> I think it is simpler and cleaner to simply create the mkimage command
> line with a single sed invocation, E.G. something like:
> 
> LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(HOST_DIR)/usr/bin/mkimage -l $(LINUX_IMAGE_PATH) |\
> 	sed -n -e 's/Image Name:[ ]*\(.*\)/-n "\1"/p' -e 's/Load Address:/-a/' -e 's/Entry Point:/-e/'`; \
> 	$(HOST_DIR)/usr/bin/mkimage -A $(KERNEL_ARCH) -O linux \
> 	-T kernel -C none $${MKIMAGE_ARGS} \
> 	-d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH);
> 
> (Construct the needed -n / -a / -e options in place).

Yep, Ok, will do.

Thanks!
Maxime