[PATCH 1/4] ARM:boot:device tree: Allow the device tree binary to be appended to zImage

[John Bonesio]

I have looked through, and I'm reworking the patch set. I have just one
comment below.

- John

On 02/28/2011 10:39 PM, Nicolas Pitre wrote:
> On Mon, 28 Feb 2011, John Bonesio wrote:
> 
>> This patch provides the ability to boot using a device tree that is appended
>> to the raw binary zImage (e.g. cat zImage <filename>.dtb > zImage_w_dtb).
>>
>> Signed-off-by: John Bonesio <bones at secretlab.ca>
> 
> Comments below.
> 
>> ---
>>
>>  arch/arm/Kconfig                |    7 +++
>>  arch/arm/boot/compressed/head.S |   93 ++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 94 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index d8a330f..68fc640 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1593,6 +1593,13 @@ config USE_OF
>>  	help
>>  	  Include support for flattened device tree machine descriptions.
>>  
>> +config ARM_APPENDED_DTB
>> +       bool "Use appended device tree blob" if OF
>> +       default n
> 
> The default is n by default, so you don't need to mention it.
> 
> Also this should depend on OF (CONFIG_OF).
> 
>> +       help
>> +         With this option, the boot code will look for a dtb bianry
> 
> s/bianry/binary/
> 
> Since this is an help text for people who might not have a clue about 
> "dtb", it would be better to spell it out.
> 
>> +         appended to zImage.
>> +
>>  # Compressed boot loader in ROM.  Yes, we really want to ask about
>>  # TEXT and BSS so we preserve their values in the config files.
>>  config ZBOOT_ROM_TEXT
>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>> index 200625c..ae9f8c6 100644
>> --- a/arch/arm/boot/compressed/head.S
>> +++ b/arch/arm/boot/compressed/head.S
>> @@ -210,6 +210,46 @@ restart:	adr	r0, LC0
>>  		 */
>>  		mov	r10, r6
>>  #endif
>> +#ifdef CONFIG_ARM_APPENDED_DTB
>> +/*
>> + *   r0  = delta
>> + *   r2  = BSS start
>> + *   r3  = BSS end
>> + *   r4  = final kernel address
>> + *   r5  = start of this image
>> + *   r6  = _edata
>> + *   r7  = architecture ID
>> + *   r8  = atags/device tree pointer
>> + *   r9  = size of decompressed image
>> + *   r10 = end of this image, including  bss/stack/malloc space if non XIP
>> + *   r11 = GOT start
>> + *   r12 = GOT end
>> + *
>> + * if there are device trees (dtb) appended to zImage, advance r10 so that the
>> + * dtb data will get relocated along with the kernel if necessary.
>> + */
>> +
>> +		ldr	r12, [r6, #0]
>> +		ldr	r1, =0xedfe0dd0		@ sig is 0xdoodfeed big endian
>> +		cmp	r12, r1
>> +		bne	dtb_check_done
>> +
>> +		/* Get the dtb's size */
>> +		ldr	r12, [r6, #4]		@ device tree size
>> +
>> +		/* convert r12 (dtb size) to little endian */
>> +		eor	r1, r12, r12, ror #16
>> +		bic	r1, r1, #0x00ff0000
>> +		mov	r12, r12, ror #8
>> +		eor	r12, r12, r1, lsr #8
>> +
>> +		add	r10, r10, r12
>> +		add	r6, r6, r12
>> +
>> +dtb_check_done:
>> +		adr	r1, LC0
>> +		ldr	r12, [r1, #28]		@ restore r12 (GOT end)
>> +#endif
> 
> Instead of clobbering r12, you could use lr instead.
> 
> The byte swap on the size should be done only if __ARMEB__ is not 
> defined i.e. #ifndef __ARMEB__ ...
> 
> Also the DT signature should be endian aware.
> 
>>  /*
>>   * Check to see if we will overwrite ourselves.
>> @@ -223,8 +263,8 @@ restart:	adr	r0, LC0
>>   */
>>  		cmp	r4, r10
>>  		bhs	wont_overwrite
>> -		add	r10, r4, r9
>> -		cmp	r10, r5
>> +		add	r1, r4, r9
>> +		cmp	r1, r5
>>  		bls	wont_overwrite
>>  
>>  /*
>> @@ -272,8 +312,10 @@ wont_overwrite:
>>   *   r12 = GOT end
>>   *   sp  = stack pointer
>>   */
>> -		teq	r0, #0
>> -		beq	not_relocated
>> +		adr	r1, LC0
>> +		ldr	r6, [r1, #16]		@ reload _edata value
> 
> Why?
> 
>> +
>> +		add	r6, r6, r0
>>  		add	r11, r11, r0
>>  		add	r12, r12, r0
>>  
>> @@ -288,12 +330,34 @@ wont_overwrite:
>>  
>>  		/*
>>  		 * Relocate all entries in the GOT table.
>> +		 * Bump bss entries to past image end (r10)
>>  		 */
>> +		sub	r5, r10, r6		@ delta of image end and _edata
>> +		add	r5, r5, #7		@ ... rounded up to a multiple
>> +		bic	r5, r5, #7		@ ... of 8 bytes, so misaligned
>> +		             	 		@ ... GOT entry doesn't
>> +		             	 		@ ... overwrite end of image
> 
> This is wrong. You are going to displace the .bss pointers even if they 
> don't need that in the case where no dtb was found.  And if a dtb was 
> found the displacement is going to be the size of the dtb _plus_ the 
> size of the .bss_stack instead of only the dtb size.
> 
> At this point you should only keep track of the .bss displacement in 
> addition to the delta offset in r0.  And if both are equal to zero then 
> skip over the fixup loop as before.
> 
>>  1:		ldr	r1, [r11, #0]		@ relocate entries in the GOT
>>  		add	r1, r1, r0		@ table.  This fixes up the
>> +		cmp	r1, r2
>> +		movcc	r9, #0			@ r9 = entry < bss_start ? 0 :
>> +		movcs	r9, #1			@      1;
>> +		cmp	r1, r3
>> +		movcs	r9, #0			@ r9 = entry >= end ? 0 : t1;
>> +		cmp	r9, #0
>> +		addne	r1, r5			@ entry += r9 ? bss delta : 0;
> 
> The above would be much more elegant if written like this:
> 
> 		cmp	r1, r2
> 		cmphs	r3, r1
> 		addhi	r1, r5
> 
>>  		str	r1, [r11], #4		@ C references.
>>  		cmp	r11, r12
>>  		blo	1b
>> +
>> +		/* bump our bss registers too */
>> +		add	r2, r2, r5
>> +		add	r3, r3, r5
>> +
>> +		/* bump the stack pinter, if at or above _edata */
>> +		cmp	sp, r6
>> +		addcs	sp, sp, r5
> 
> This will always be true as this is within #ifndef CONFIG_ZBOOT_ROM.

Right now sp will always be above r6. I thought it might be prudent to
add the test in case the linker script changed and the stack was placed
elsewhere. It might save someone a headache later.

> 
>>  #else
>>  
>>  		/*
>> @@ -309,7 +373,7 @@ wont_overwrite:
>>  		blo	1b
>>  #endif
>>  
>> -not_relocated:	mov	r0, #0
>> +		mov	r0, #0
>>  1:		str	r0, [r2], #4		@ clear bss
>>  		str	r0, [r2], #4
>>  		str	r0, [r2], #4
>> @@ -317,8 +381,25 @@ not_relocated:	mov	r0, #0
>>  		cmp	r2, r3
>>  		blo	1b
>>  
>> +#ifdef CONFIG_ARM_APPENDED_DTB
>> +/*
>> + * The C runtime environment should now be set up sufficiently.
>> + *   r4  = kernel execution address
>> + *   r6  = _edata
>> + *   r7  = architecture ID
>> + *   r8  = atags pointer
>> + *
>> + * if there is a device tree (dtb) appended to zImage, set up to use this dtb.
>> + */
>> +		ldr	r0, [r6, #0]
>> +		ldr	r1, =0xedfe0dd0		@ sig is 0xdoodfeed big endian
>> +		cmp	r0, r1
>> +		bne	keep_atags
>> +
>> +		mov	r8, r6			@ use the appended device tree
> 
> You should have updated r8 the moment you knew you have a valid dtb 
> earlier instead of duplicating this test here.
> 
>> +keep_atags:
>> +#endif
>>  /*
>> - * The C runtime environment should now be setup sufficiently.
>>   * Set up some pointers, and start decompressing.
>>   *   r4  = kernel execution address
>>   *   r7  = architecture ID
>>