Patchwork [4/6] Add support for appended device tree blobs for arm

login
register
mail settings
Submitter Maxime Ripard
Date July 17, 2012, 12:27 p.m.
Message ID <1342528042-10038-5-git-send-email-maxime.ripard@free-electrons.com>
Download mbox | patch
Permalink /patch/171408/
State Superseded
Headers show

Comments

Maxime Ripard - July 17, 2012, 12:27 p.m.
This patch adds support for the ARM-only appended device tree
mechanism present in the kernel.

This option allows to add at the end of the kernel image the
device tree blob so that we can still boot device tree enabled
kernels with old bootloaders.

This patch also adds the needed logic to genereate such an image
when building zImages or uImages, also adding the necessary parts
to rebuild the uImage.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 linux/Config.in |   17 ++++++++++++++++-
 linux/linux.mk  |   43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 2 deletions(-)
Arnout Vandecappelle - July 28, 2012, 2:49 p.m.
On 07/17/12 14:27, Maxime Ripard wrote:
[snip]
> @@ -198,6 +200,13 @@ config BR2_LINUX_KERNEL_UIMAGE
>   	depends on BR2_arm || BR2_armeb || BR2_bfin || BR2_powerpc || BR2_avr32 || BR2_sh || BR2_sh64
>   	select BR2_LINUX_KERNEL_UBOOT_IMAGE
>
> +config BR2_LINUX_KERNEL_APPENDED_UIMAGE
> +	bool "uImage with appended DT"
> +	depends on BR2_arm || BR2_armeb
> +	select BR2_LINUX_KERNEL_DTS_SUPPORT

  Given that this option will automatically select DTS support,
I'd prefer to see the DTS menu options later, so after the image
names.  Otherwise, when you select this in menuconfig (and DTS
was not selected yet), it will suddenly move down because of the
new options that appear.  So this actually affects patch 2/6.


> +	select BR2_LINUX_KERNEL_APPENDED_DTB
> +	select BR2_LINUX_KERNEL_UBOOT_IMAGE
> +
>   config BR2_LINUX_KERNEL_BZIMAGE
>   	bool "bzImage"
>   	depends on BR2_i386 || BR2_x86_64
[snip]
> @@ -66,10 +73,14 @@ LINUX_IMAGE_NAME=vmImage
>   else
>   LINUX_IMAGE_NAME=uImage
>   endif
> +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
> +LINUX_IMAGE_NAME=uImage
>   else ifeq ($(BR2_LINUX_KERNEL_BZIMAGE),y)
>   LINUX_IMAGE_NAME=bzImage
>   else ifeq ($(BR2_LINUX_KERNEL_ZIMAGE),y)
>   LINUX_IMAGE_NAME=zImage
> +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
> +LINUX_IMAGE_NAME=zImage
>   else ifeq ($(BR2_LINUX_KERNEL_VMLINUX_BIN),y)
>   LINUX_IMAGE_NAME=vmlinux.bin
>   else ifeq ($(BR2_LINUX_KERNEL_VMLINUX),y)

  I think this whole blob should move to Config.in, like we
do it for many other strings...  But that's of course unrelated
to this patch.

[snip]
> +ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
> +define LINUX_APPEND_DTB
> +	cat $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb>  $(KERNEL_ARCH_PATH)/boot/zImage_dtb
> +	mv $(KERNEL_ARCH_PATH)/boot/zImage_dtb $(KERNEL_ARCH_PATH)/boot/zImage
> +	# 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.
> +	LOAD=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Load Address: \([0-9]*\)/\1/p'`; \
> +	ENTRY=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Entry Point: \([0-9]*\)/\1/p'`; \
> +	$(MKIMAGE) -A $(KERNEL_ARCH) -O linux -T kernel -C none -a $${LOAD} -e $${ENTRY} -n 'Linux Buildroot' \
> +		-d $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME)

  MKIMAGE isn't defined anywhere, so I wonder how it can work for you...
But anyway, with the amendment you posted later this shouldn't be an issue
anymore.

> +endef
> +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
> +define LINUX_APPEND_DTB
> +	cat $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb>  $(KERNEL_ARCH_PATH)/boot/zImage_dtb
> +	mv $(KERNEL_ARCH_PATH)/boot/zImage_dtb $(KERNEL_ARCH_PATH)/boot/zImage
> +endef
> +endif

  Does this work?

ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
define LINUX_APPEND_DTB
	cat ...
ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
	$(MAKE) ... uImage
endif
endef
endif

(I'm not sure if you can still use an ifeq inside a define.)


  Regards,
  Arnout

[snip]
Thomas Petazzoni - July 28, 2012, 7:29 p.m.
Le Sat, 28 Jul 2012 16:49:34 +0200,
Arnout Vandecappelle <arnout@mind.be> a écrit :

> > @@ -66,10 +73,14 @@ LINUX_IMAGE_NAME=vmImage
> >   else
> >   LINUX_IMAGE_NAME=uImage
> >   endif
> > +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
> > +LINUX_IMAGE_NAME=uImage
> >   else ifeq ($(BR2_LINUX_KERNEL_BZIMAGE),y)
> >   LINUX_IMAGE_NAME=bzImage
> >   else ifeq ($(BR2_LINUX_KERNEL_ZIMAGE),y)
> >   LINUX_IMAGE_NAME=zImage
> > +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
> > +LINUX_IMAGE_NAME=zImage
> >   else ifeq ($(BR2_LINUX_KERNEL_VMLINUX_BIN),y)
> >   LINUX_IMAGE_NAME=vmlinux.bin
> >   else ifeq ($(BR2_LINUX_KERNEL_VMLINUX),y)
> 
>   I think this whole blob should move to Config.in, like we
> do it for many other strings...  But that's of course unrelated
> to this patch.

I don't necessarily agree here. Generally speaking my preference is to
not use too much kconfig to define a bunch of strings when those are
not related to visible options.

> ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
> define LINUX_APPEND_DTB
> 	cat ...
> ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
> 	$(MAKE) ... uImage
> endif
> endef
> endif
> 
> (I'm not sure if you can still use an ifeq inside a define.)

No, you can't, and it's a shame :-(

Thomas
Arnout Vandecappelle - July 29, 2012, 3:16 p.m.
On 07/28/12 21:29, Thomas Petazzoni wrote:
> Le Sat, 28 Jul 2012 16:49:34 +0200,
> Arnout Vandecappelle<arnout@mind.be>  a écrit :
>
>>> @@ -66,10 +73,14 @@ LINUX_IMAGE_NAME=vmImage
>>>    else
>>>    LINUX_IMAGE_NAME=uImage
>>>    endif
>>> +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
>>> +LINUX_IMAGE_NAME=uImage
>>>    else ifeq ($(BR2_LINUX_KERNEL_BZIMAGE),y)
>>>    LINUX_IMAGE_NAME=bzImage
>>>    else ifeq ($(BR2_LINUX_KERNEL_ZIMAGE),y)
>>>    LINUX_IMAGE_NAME=zImage
>>> +else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
>>> +LINUX_IMAGE_NAME=zImage
>>>    else ifeq ($(BR2_LINUX_KERNEL_VMLINUX_BIN),y)
>>>    LINUX_IMAGE_NAME=vmlinux.bin
>>>    else ifeq ($(BR2_LINUX_KERNEL_VMLINUX),y)
>>
>>    I think this whole blob should move to Config.in, like we
>> do it for many other strings...  But that's of course unrelated
>> to this patch.
>
> I don't necessarily agree here. Generally speaking my preference is to
> not use too much kconfig to define a bunch of strings when those are
> not related to visible options.

  Putting it in Config.in is more concise. Imagine what the Makefile would
look like if all the target/Config.in.arch strings would have to be put
in "else ifeq" statements...

  Also, these strings are directly related to visible options: they are
the string representation of the BR2_LINUX_KERNEL_XXX option (well, it's
not a 1-to-1 mapping but it comes close).


>> ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
>> define LINUX_APPEND_DTB
>> 	cat ...
>> ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
>> 	$(MAKE) ... uImage
>> endif
>> endef
>> endif
>>
>> (I'm not sure if you can still use an ifeq inside a define.)
>
> No, you can't, and it's a shame :-(

  Darn.

  This would work I think:
ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
define LINUX_APPEND_DTB
	cat ...
endef
endif
ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
LINUX_APPEND_DTB += $(MAKE) ... uImage
endif

  Or else, define a LINUX_REBUILD_UIMAGE macro for the appended uImage.
But neither is much better than the original (especially if the cat-rm-mv
chain is replaced by a single cat) so let's keep it as it was proposed
by Maxime.


  Regards,
  Arnout
Fabio Porcedda - July 30, 2012, 8:25 a.m.
On Sat, Jul 28, 2012 at 4:49 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> ...
>>
>> +ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
>> +define LINUX_APPEND_DTB
>> +       cat $(KERNEL_ARCH_PATH)/boot/zImage
>> $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb>
>> $(KERNEL_ARCH_PATH)/boot/zImage_dtb
>> +       mv $(KERNEL_ARCH_PATH)/boot/zImage_dtb
>> $(KERNEL_ARCH_PATH)/boot/zImage
>> +       # 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.
>> +       LOAD=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Load Address:
>> \([0-9]*\)/\1/p'`; \
>> +       ENTRY=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Entry Point:
>> \([0-9]*\)/\1/p'`; \
>> +       $(MKIMAGE) -A $(KERNEL_ARCH) -O linux -T kernel -C none -a
>> $${LOAD} -e $${ENTRY} -n 'Linux Buildroot' \
>> +               -d $(KERNEL_ARCH_PATH)/boot/zImage
>> $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME)
>
>
>  MKIMAGE isn't defined anywhere, so I wonder how it can work for you...
> But anyway, with the amendment you posted later this shouldn't be an issue
> anymore.

I tested the patch and doesn't work.
The MKIMAGE isn't defined anywhere.

output log error:
# 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.
LOAD=` -l /home/fabiopo/porting-pro3/ge863-pro3-linux-3.6/buildroot-dt/output/build/linux-custom/arch/arm/boot/uImage
| sed -n 's/Load Address: \([0-9]*\)/\1/p'`; ENTRY=` -l
/home/fabiopo/porting-pro3/ge863-pro3-linux-3.6/buildroot-dt/output/build/linux-custom/arch/arm/boot/uImage
| sed -n 's/Entry Point: \([0-9]*\)/\1/p'`;  -A arm -O linux -T kernel
-C none -a ${LOAD} -e ${ENTRY} -n 'Linux Buildroot' -d
/home/fabiopo/porting-pro3/ge863-pro3-linux-3.6/buildroot-dt/output/build/linux-custom/arch/arm/boot/zImage
/home/fabiopo/porting-pro3/ge863-pro3-linux-3.6/buildroot-dt/output/build/linux-custom/arch/arm/boot/uImage
/bin/bash: -l: command not found
/bin/bash: -l: command not found
/bin/bash: -A: command not found
make: *** [/home/fabiopo/porting-pro3/ge863-pro3-linux-3.6/buildroot-dt/output/build/linux-custom/.stamp_built]
Error 127

Regards
Thomas Petazzoni - July 30, 2012, 8:31 a.m.
Le Mon, 30 Jul 2012 10:25:51 +0200,
Fabio Porcedda <fabio.porcedda@gmail.com> a écrit :

> I tested the patch and doesn't work.
> The MKIMAGE isn't defined anywhere.

Yes, please apply the followup patch that Maxime has posted to you on:

Date: Fri, 27 Jul 2012 20:59:15 +0200
Message-ID: <5012E503.2070005@free-electrons.com>

It should fix this problem.

Best regards,

Thomas
Fabio Porcedda - July 30, 2012, 10:10 a.m.
On Mon, Jul 30, 2012 at 10:31 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Le Mon, 30 Jul 2012 10:25:51 +0200,
> Fabio Porcedda <fabio.porcedda@gmail.com> a écrit :
>
>> I tested the patch and doesn't work.
>> The MKIMAGE isn't defined anywhere.
>
> Yes, please apply the followup patch that Maxime has posted to you on:
>
> Date: Fri, 27 Jul 2012 20:59:15 +0200
> Message-ID: <5012E503.2070005@free-electrons.com>
>
> It should fix this problem.

It does fix this problem, thanks.

Best regards

Patch

diff --git a/linux/Config.in b/linux/Config.in
index 6da50f5..606bf8e 100644
--- a/linux/Config.in
+++ b/linux/Config.in
@@ -149,6 +149,9 @@  if BR2_LINUX_KERNEL_DTS_SUPPORT
 config BR2_LINUX_KERNEL_DTB_IS_SELF_BUILT
        bool
 
+config BR2_LINUX_KERNEL_APPENDED_DTB
+	bool
+
 choice
 	prompt "Device tree source"
 	default BR2_LINUX_KERNEL_USE_INTREE_DTS
@@ -180,7 +183,6 @@  config BR2_LINUX_KERNEL_CUSTOM_DTS_PATH
 	depends on BR2_LINUX_KERNEL_USE_CUSTOM_DTS
 	help
 	  Path to the device tree source file
-
 endif
 
 #
@@ -198,6 +200,13 @@  config BR2_LINUX_KERNEL_UIMAGE
 	depends on BR2_arm || BR2_armeb || BR2_bfin || BR2_powerpc || BR2_avr32 || BR2_sh || BR2_sh64
 	select BR2_LINUX_KERNEL_UBOOT_IMAGE
 
+config BR2_LINUX_KERNEL_APPENDED_UIMAGE
+	bool "uImage with appended DT"
+	depends on BR2_arm || BR2_armeb
+	select BR2_LINUX_KERNEL_DTS_SUPPORT
+	select BR2_LINUX_KERNEL_APPENDED_DTB
+	select BR2_LINUX_KERNEL_UBOOT_IMAGE
+
 config BR2_LINUX_KERNEL_BZIMAGE
 	bool "bzImage"
 	depends on BR2_i386 || BR2_x86_64
@@ -206,6 +215,12 @@  config BR2_LINUX_KERNEL_ZIMAGE
 	bool "zImage"
 	depends on BR2_arm || BR2_armeb || BR2_powerpc || BR2_sparc || BR2_sh || BR2_sh64 || BR2_xtensa
 
+config BR2_LINUX_KERNEL_APPENDED_ZIMAGE
+	bool "zImage with appended DT"
+	depends on BR2_arm || BR2_armeb
+	select BR2_LINUX_KERNEL_DTS_SUPPORT
+	select BR2_LINUX_KERNEL_APPENDED_DTB
+
 config BR2_LINUX_KERNEL_VMLINUX_BIN
 	bool "vmlinux.bin"
 	depends on BR2_mips || BR2_mipsel || BR2_sh || BR2_sh64
diff --git a/linux/linux.mk b/linux/linux.mk
index 7d632f6..011474e 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -56,6 +56,13 @@  else ifeq ($(BR2_LINUX_KERNEL_USE_CUSTOM_DTS),y)
 KERNEL_DTS_NAME = $(basename $(notdir $(BR2_LINUX_KERNEL_CUSTOM_DTS_PATH)))
 endif
 
+ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
+ifneq ($(words $(KERNEL_DTS_NAME)),1)
+$(error Kernel with appended device tree needs exactly one DTS source.\
+  Check BR2_LINUX_KERNEL_INTREE_DTS_NAME or BR2_LINUX_KERNEL_CUSTOM_DTS_PATH.)
+endif
+endif
+
 ifeq ($(BR2_LINUX_KERNEL_IMAGE_TARGET_CUSTOM),y)
 LINUX_IMAGE_NAME=$(call qstrip,$(BR2_LINUX_KERNEL_IMAGE_TARGET_NAME))
 else
@@ -66,10 +73,14 @@  LINUX_IMAGE_NAME=vmImage
 else
 LINUX_IMAGE_NAME=uImage
 endif
+else ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
+LINUX_IMAGE_NAME=uImage
 else ifeq ($(BR2_LINUX_KERNEL_BZIMAGE),y)
 LINUX_IMAGE_NAME=bzImage
 else ifeq ($(BR2_LINUX_KERNEL_ZIMAGE),y)
 LINUX_IMAGE_NAME=zImage
+else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
+LINUX_IMAGE_NAME=zImage
 else ifeq ($(BR2_LINUX_KERNEL_VMLINUX_BIN),y)
 LINUX_IMAGE_NAME=vmlinux.bin
 else ifeq ($(BR2_LINUX_KERNEL_VMLINUX),y)
@@ -79,6 +90,12 @@  LINUX_IMAGE_NAME=vmlinuz
 endif
 endif
 
+ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
+LINUX_IMAGE_TARGET=zImage
+else
+LINUX_IMAGE_TARGET=$(LINUX_IMAGE_NAME)
+endif
+
 # Compute the arch path, since i386 and x86_64 are in arch/x86 and not
 # in arch/$(KERNEL_ARCH). Even if the kernel creates symbolic links
 # for bzImage, arch/i386 and arch/x86_64 do not exist when copying the
@@ -159,6 +176,8 @@  define LINUX_CONFIGURE_CMDS
 		$(call KCONFIG_SET_OPT,CONFIG_UEVENT_HELPER_PATH,\"/sbin/mdev\",$(@D)/.config))
 	$(if $(BR2_PACKAGE_SYSTEMD),
 		$(call KCONFIG_ENABLE_OPT,CONFIG_CGROUPS,$(@D)/.config))
+	$(if $(BR2_LINUX_KERNEL_APPENDED_DTB),
+		$(call KCONFIG_ENABLE_OPT,CONFIG_ARM_APPENDED_DTB,$(@D)/.config))
 	yes '' | $(TARGET_MAKE_ENV) $(MAKE1) $(LINUX_MAKE_FLAGS) -C $(@D) oldconfig
 endef
 
@@ -173,16 +192,38 @@  endef
 endif
 endif
 
+ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y)
+define LINUX_APPEND_DTB
+	cat $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb > $(KERNEL_ARCH_PATH)/boot/zImage_dtb
+	mv $(KERNEL_ARCH_PATH)/boot/zImage_dtb $(KERNEL_ARCH_PATH)/boot/zImage
+	# 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.
+	LOAD=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Load Address: \([0-9]*\)/\1/p'`; \
+	ENTRY=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) | sed -n 's/Entry Point: \([0-9]*\)/\1/p'`; \
+	$(MKIMAGE) -A $(KERNEL_ARCH) -O linux -T kernel -C none -a $${LOAD} -e $${ENTRY} -n 'Linux Buildroot' \
+		-d $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME)
+endef
+else ifeq ($(BR2_LINUX_KERNEL_APPENDED_ZIMAGE),y)
+define LINUX_APPEND_DTB
+	cat $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb > $(KERNEL_ARCH_PATH)/boot/zImage_dtb
+	mv $(KERNEL_ARCH_PATH)/boot/zImage_dtb $(KERNEL_ARCH_PATH)/boot/zImage
+endef
+endif
+
 # Compilation. We make sure the kernel gets rebuilt when the
 # configuration has changed.
 define LINUX_BUILD_CMDS
 	$(if $(BR2_LINUX_KERNEL_USE_CUSTOM_DTS),
 		cp $(BR2_LINUX_KERNEL_CUSTOM_DTS_PATH) $(KERNEL_ARCH_PATH)/boot/dts/)
-	$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_IMAGE_NAME)
+	$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_IMAGE_TARGET)
 	@if grep -q "CONFIG_MODULES=y" $(@D)/.config; then 	\
 		$(TARGET_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) modules ;	\
 	fi
 	$(LINUX_BUILD_DTB)
+	$(LINUX_APPEND_DTB)
 endef