* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
@ 2011-03-09 21:58 John Bonesio
2011-03-09 23:05 ` Ben Dooks
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: John Bonesio @ 2011-03-09 21:58 UTC (permalink / raw)
To: linux-arm-kernel
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@secretlab.ca>
---
arch/arm/Kconfig | 7 ++++
arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
2 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d8a330f..1a55e3e 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"
+ depends on OF
+ help
+ With this option, the boot code will look for a device tree binary
+ (dtb) 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..611719e 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -210,6 +210,58 @@ 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 lr, [r6, #0]
+#ifndef __ARMEB__
+ ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
+#else
+ ldr r1, =0xd00dfeed
+#endif
+ cmp lr, r1
+ bne dtb_check_done
+
+ mov r8, r6 @ use the appended device tree
+
+ /* Get the dtb's size */
+ ldr lr, [r6, #4]
+
+#ifndef __ARMEB__
+ /* convert lr (dtb size) to little endian */
+ eor r1, lr, lr, ror #16
+ bic r1, r1, #0x00ff0000
+ mov lr, lr, ror #8
+ eor lr, lr, r1, lsr #8
+#endif
+ /*
+ * dtb size rounded up to a multiple of 8 bytes so a
+ * misaligned GOT entry doesn't cause the end of the
+ * dtb binary to be overwritten.
+ */
+ add lr, lr, #7
+ bic lr, lr, #7
+
+ add r10, r10, lr
+ add r6, r6, lr
+dtb_check_done:
+#endif
/*
* Check to see if we will overwrite ourselves.
@@ -272,7 +324,8 @@ wont_overwrite:
* r12 = GOT end
* sp = stack pointer
*/
- teq r0, #0
+ add r1, r0, lr
+ teq r1, #0
beq not_relocated
add r11, r11, r0
add r12, r12, r0
@@ -288,12 +341,28 @@ wont_overwrite:
/*
* Relocate all entries in the GOT table.
+ * Bump bss entries to _edata + dtb size
*/
1: ldr r1, [r11, #0] @ relocate entries in the GOT
- add r1, r1, r0 @ table. This fixes up the
- str r1, [r11], #4 @ C references.
+ add r1, r1, r0 @ This fixes up C references
+ cmp r1, r2 @ if entry >= bss_start &&
+ cmphs r3, r1 @ bss_end > entry
+ addhi r1, lr @ entry += dtb size
+ str r1, [r11], #4 @ next entry
cmp r11, r12
blo 1b
+
+ /* bump our bss registers too */
+ add r2, r2, lr
+ add r3, r3, lr
+
+ /*
+ * bump the stack pinter
+ *
+ * If the linker script changes so the stack is not after
+ * the bss section, this code will be wrong.
+ */
+ add sp, sp, lr
#else
/*
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-09 21:58 [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage John Bonesio
@ 2011-03-09 23:05 ` Ben Dooks
2011-03-12 8:44 ` Grant Likely
2011-03-15 3:44 ` Shawn Guo
2011-03-17 18:42 ` Nicolas Pitre
2 siblings, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2011-03-09 23:05 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 09, 2011 at 01:58:00PM -0800, 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).
I'd much rather see something that wrappers the kernel and passes the
DT through an ATAG. So possibly a pre-amble as well?
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
>
> arch/arm/Kconfig | 7 ++++
> arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 79 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d8a330f..1a55e3e 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"
> + depends on OF
> + help
> + With this option, the boot code will look for a device tree binary
> + (dtb) 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..611719e 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -210,6 +210,58 @@ 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 lr, [r6, #0]
> +#ifndef __ARMEB__
> + ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> +#else
> + ldr r1, =0xd00dfeed
> +#endif
> + cmp lr, r1
> + bne dtb_check_done
> +
> + mov r8, r6 @ use the appended device tree
> +
> + /* Get the dtb's size */
> + ldr lr, [r6, #4]
> +
> +#ifndef __ARMEB__
> + /* convert lr (dtb size) to little endian */
> + eor r1, lr, lr, ror #16
> + bic r1, r1, #0x00ff0000
> + mov lr, lr, ror #8
> + eor lr, lr, r1, lsr #8
> +#endif
> + /*
> + * dtb size rounded up to a multiple of 8 bytes so a
> + * misaligned GOT entry doesn't cause the end of the
> + * dtb binary to be overwritten.
> + */
> + add lr, lr, #7
> + bic lr, lr, #7
> +
> + add r10, r10, lr
> + add r6, r6, lr
> +dtb_check_done:
> +#endif
>
> /*
> * Check to see if we will overwrite ourselves.
> @@ -272,7 +324,8 @@ wont_overwrite:
> * r12 = GOT end
> * sp = stack pointer
> */
> - teq r0, #0
> + add r1, r0, lr
> + teq r1, #0
> beq not_relocated
> add r11, r11, r0
> add r12, r12, r0
> @@ -288,12 +341,28 @@ wont_overwrite:
>
> /*
> * Relocate all entries in the GOT table.
> + * Bump bss entries to _edata + dtb size
> */
> 1: ldr r1, [r11, #0] @ relocate entries in the GOT
> - add r1, r1, r0 @ table. This fixes up the
> - str r1, [r11], #4 @ C references.
> + add r1, r1, r0 @ This fixes up C references
> + cmp r1, r2 @ if entry >= bss_start &&
> + cmphs r3, r1 @ bss_end > entry
> + addhi r1, lr @ entry += dtb size
> + str r1, [r11], #4 @ next entry
> cmp r11, r12
> blo 1b
> +
> + /* bump our bss registers too */
> + add r2, r2, lr
> + add r3, r3, lr
> +
> + /*
> + * bump the stack pinter
> + *
> + * If the linker script changes so the stack is not after
> + * the bss section, this code will be wrong.
> + */
> + add sp, sp, lr
> #else
>
> /*
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-09 23:05 ` Ben Dooks
@ 2011-03-12 8:44 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2011-03-12 8:44 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 09, 2011 at 11:05:42PM +0000, Ben Dooks wrote:
> On Wed, Mar 09, 2011 at 01:58:00PM -0800, 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).
>
> I'd much rather see something that wrappers the kernel and passes the
> DT through an ATAG. So possibly a pre-amble as well?
When DT support was initially implemented for ARM, it did exactly
that. It passed the dt via an ATAG. However, that leaves a number of
questions about which data is authoritative, ATAG or DT data. Nicolas
argued rightly and successfully that if DT was going to be used, then
we should go at it whole hog and make passing a dtb the full boot
interface.
I disagreed strongly at first, but I'm glad to say he convinced me.
In the end the implementation of passing dt directly turned out to be
far more simple and elegant than the hybrid approach was. In the
current code, a single kernel image can accept either a .dtb or an
ATAGs list (so no backwards compatibility issues) and it detects which
data structure to parse early in setup_arch().
As for this patch, this functionality was implemented for a client and
it is being posted for wider review in case anyone else finds it
useful. I wasn't originally planning to push for mainline merging of
it, but after positive feedback on the first posting I though it was
probably worth the effort to refine it more and get it into acceptable
shape for mainline.
Regarding using a wrapper; that was the approach I initially wanted to
pursue, but modifying the zImage wrapper was much simpler to implement
because the wrapper has knowledge about how much RAM the kernel needs
at boot and can therefore make a good decision about where to locate
the .dtb before jumping into the kernel.
g.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-09 21:58 [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage John Bonesio
2011-03-09 23:05 ` Ben Dooks
@ 2011-03-15 3:44 ` Shawn Guo
2011-03-15 7:52 ` Shawn Guo
2011-03-17 18:42 ` Nicolas Pitre
2 siblings, 1 reply; 10+ messages in thread
From: Shawn Guo @ 2011-03-15 3:44 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 09, 2011 at 01:58:00PM -0800, 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@secretlab.ca>
> ---
>
> arch/arm/Kconfig | 7 ++++
> arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 79 insertions(+), 3 deletions(-)
>
I played this patch on mx51 babbage a little bit, and noticed that it
introduces one difference on both non-dt and dt kernel booting. The
serial terminal stalls for a few seconds after printing
'Uncompressing Linux...', and then continue to work with all message
output. That's something like:
Uncompressing Linux... (stall for seconds) done, booting the kernel.
Linux version 2.6.38-rc6+ (r65073 at S2101-09) (gcc version 4.4.5 (Ubuntu/Linaro 4.
4.4-14ubuntu4) ) #91 Tue Mar 15 11:29:03 CST 2011
CPU: ARMv7 Processor [412fc085] revision 5 (ARMv7), cr=10c53c7f
CPU: VIPT nonaliasing data cache, VIPT aliasing instruction cache
Machine: Freescale MX51 Babbage Board
Machine: Freescale MX51 Babbage Board
Memory policy: ECC disabled, Data cache writeback
Built 1 zonelists in Zone order, mobility grouping on. Total pages: 130048
......
I'm unsure if this is expected ...
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-15 3:44 ` Shawn Guo
@ 2011-03-15 7:52 ` Shawn Guo
2011-03-15 8:03 ` Grant Likely
0 siblings, 1 reply; 10+ messages in thread
From: Shawn Guo @ 2011-03-15 7:52 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 15, 2011 at 11:44:22AM +0800, Shawn Guo wrote:
> On Wed, Mar 09, 2011 at 01:58:00PM -0800, 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@secretlab.ca>
> > ---
> >
> > arch/arm/Kconfig | 7 ++++
> > arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 79 insertions(+), 3 deletions(-)
> >
> I played this patch on mx51 babbage a little bit, and noticed that it
> introduces one difference on both non-dt and dt kernel booting. The
To be clear, the difference is being seen when I do not even enable
CONFIG_ARM_APPENDED_DTB.
--
Regards,
Shawn
> serial terminal stalls for a few seconds after printing
> 'Uncompressing Linux...', and then continue to work with all message
> output. That's something like:
>
> Uncompressing Linux... (stall for seconds) done, booting the kernel.
> Linux version 2.6.38-rc6+ (r65073 at S2101-09) (gcc version 4.4.5 (Ubuntu/Linaro 4.
> 4.4-14ubuntu4) ) #91 Tue Mar 15 11:29:03 CST 2011
> CPU: ARMv7 Processor [412fc085] revision 5 (ARMv7), cr=10c53c7f
> CPU: VIPT nonaliasing data cache, VIPT aliasing instruction cache
> Machine: Freescale MX51 Babbage Board
> Machine: Freescale MX51 Babbage Board
> Memory policy: ECC disabled, Data cache writeback
> Built 1 zonelists in Zone order, mobility grouping on. Total pages: 130048
> ......
>
> I'm unsure if this is expected ...
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-15 7:52 ` Shawn Guo
@ 2011-03-15 8:03 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2011-03-15 8:03 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 15, 2011 at 03:52:32PM +0800, Shawn Guo wrote:
> On Tue, Mar 15, 2011 at 11:44:22AM +0800, Shawn Guo wrote:
> > On Wed, Mar 09, 2011 at 01:58:00PM -0800, 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@secretlab.ca>
> > > ---
> > >
> > > arch/arm/Kconfig | 7 ++++
> > > arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
> > > 2 files changed, 79 insertions(+), 3 deletions(-)
> > >
> > I played this patch on mx51 babbage a little bit, and noticed that it
> > introduces one difference on both non-dt and dt kernel booting. The
>
> To be clear, the difference is being seen when I do not even enable
> CONFIG_ARM_APPENDED_DTB.
That would definitely be a bug then.
g.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-09 21:58 [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage John Bonesio
2011-03-09 23:05 ` Ben Dooks
2011-03-15 3:44 ` Shawn Guo
@ 2011-03-17 18:42 ` Nicolas Pitre
2011-03-21 17:35 ` John Bonesio
2 siblings, 1 reply; 10+ messages in thread
From: Nicolas Pitre @ 2011-03-17 18:42 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 9 Mar 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@secretlab.ca>
You're almost there, but there are still remaining issues with this
patch.
> ---
>
> arch/arm/Kconfig | 7 ++++
> arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 79 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d8a330f..1a55e3e 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"
> + depends on OF
> + help
> + With this option, the boot code will look for a device tree binary
> + (dtb) appended to zImage.
> +
I'd put this option below the ZBOOT_ROM option, and make it dependent on
!ZBOOT_ROM, e.g.:
depends on OF && !ZBOOT_ROM
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 200625c..611719e 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -210,6 +210,58 @@ 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 lr, [r6, #0]
> +#ifndef __ARMEB__
> + ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> +#else
> + ldr r1, =0xd00dfeed
> +#endif
> + cmp lr, r1
> + bne dtb_check_done
> +
> + mov r8, r6 @ use the appended device tree
> +
> + /* Get the dtb's size */
> + ldr lr, [r6, #4]
> +
> +#ifndef __ARMEB__
> + /* convert lr (dtb size) to little endian */
> + eor r1, lr, lr, ror #16
> + bic r1, r1, #0x00ff0000
> + mov lr, lr, ror #8
> + eor lr, lr, r1, lsr #8
> +#endif
> + /*
> + * dtb size rounded up to a multiple of 8 bytes so a
> + * misaligned GOT entry doesn't cause the end of the
> + * dtb binary to be overwritten.
> + */
> + add lr, lr, #7
> + bic lr, lr, #7
> +
> + add r10, r10, lr
> + add r6, r6, lr
> +dtb_check_done:
> +#endif
Now if no dtb was found, or if that code is not configured, then lr
contains garbage and that may have random consequences with the code
that follows, which would explain the reported bad behaviors with this
patch applied even if not configured in the kernel.
> /*
> * Check to see if we will overwrite ourselves.
> @@ -272,7 +324,8 @@ wont_overwrite:
> * r12 = GOT end
> * sp = stack pointer
> */
> - teq r0, #0
> + add r1, r0, lr
> + teq r1, #0
> beq not_relocated
A better way to write the above is simply:
orrs r1, r0, lr
beq not_relocated
> add r11, r11, r0
> add r12, r12, r0
> @@ -288,12 +341,28 @@ wont_overwrite:
>
> /*
> * Relocate all entries in the GOT table.
> + * Bump bss entries to _edata + dtb size
> */
> 1: ldr r1, [r11, #0] @ relocate entries in the GOT
> - add r1, r1, r0 @ table. This fixes up the
> - str r1, [r11], #4 @ C references.
> + add r1, r1, r0 @ This fixes up C references
> + cmp r1, r2 @ if entry >= bss_start &&
> + cmphs r3, r1 @ bss_end > entry
> + addhi r1, lr @ entry += dtb size
> + str r1, [r11], #4 @ next entry
> cmp r11, r12
> blo 1b
> +
> + /* bump our bss registers too */
> + add r2, r2, lr
> + add r3, r3, lr
> +
> + /*
> + * bump the stack pinter
> + *
> + * If the linker script changes so the stack is not after
> + * the bss section, this code will be wrong.
> + */
> + add sp, sp, lr
> #else
>
> /*
>
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-17 18:42 ` Nicolas Pitre
@ 2011-03-21 17:35 ` John Bonesio
2011-03-21 18:22 ` Nicolas Pitre
0 siblings, 1 reply; 10+ messages in thread
From: John Bonesio @ 2011-03-21 17:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Nicolas,
Thanks for the comments. I have a question (below).
Thanks for your help,
- John
On 03/17/2011 11:42 AM, Nicolas Pitre wrote:
> On Wed, 9 Mar 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@secretlab.ca>
>
> You're almost there, but there are still remaining issues with this
> patch.
>
>> ---
>>
>> arch/arm/Kconfig | 7 ++++
>> arch/arm/boot/compressed/head.S | 75 +++++++++++++++++++++++++++++++++++++--
>> 2 files changed, 79 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index d8a330f..1a55e3e 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"
>> + depends on OF
>> + help
>> + With this option, the boot code will look for a device tree binary
>> + (dtb) appended to zImage.
>> +
>
> I'd put this option below the ZBOOT_ROM option, and make it dependent on
> !ZBOOT_ROM, e.g.:
>
> depends on OF && !ZBOOT_ROM
>
>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>> index 200625c..611719e 100644
>> --- a/arch/arm/boot/compressed/head.S
>> +++ b/arch/arm/boot/compressed/head.S
>> @@ -210,6 +210,58 @@ 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 lr, [r6, #0]
>> +#ifndef __ARMEB__
>> + ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
>> +#else
>> + ldr r1, =0xd00dfeed
>> +#endif
>> + cmp lr, r1
>> + bne dtb_check_done
>> +
>> + mov r8, r6 @ use the appended device tree
>> +
>> + /* Get the dtb's size */
>> + ldr lr, [r6, #4]
>> +
>> +#ifndef __ARMEB__
>> + /* convert lr (dtb size) to little endian */
>> + eor r1, lr, lr, ror #16
>> + bic r1, r1, #0x00ff0000
>> + mov lr, lr, ror #8
>> + eor lr, lr, r1, lsr #8
>> +#endif
>> + /*
>> + * dtb size rounded up to a multiple of 8 bytes so a
>> + * misaligned GOT entry doesn't cause the end of the
>> + * dtb binary to be overwritten.
>> + */
>> + add lr, lr, #7
>> + bic lr, lr, #7
>> +
>> + add r10, r10, lr
>> + add r6, r6, lr
>> +dtb_check_done:
>> +#endif
>
> Now if no dtb was found, or if that code is not configured, then lr
> contains garbage and that may have random consequences with the code
> that follows, which would explain the reported bad behaviors with this
> patch applied even if not configured in the kernel.
I'm probably not understanding something here. When I look at the code
prior to my patch, it looks like lr is not initialized by head.S. It
also looks like this code makes use of lr when it relocates the code and
leaves it uninitialized when it jumps back to 'restart'.
Is this a bug in the previous code too?
What should lr be initialized to, or should we just be preserving it's
value?
I saw the comment about the performance is different with this patch -
something about a pause between 'Uncompressing Linux...' and the kernel
boot output. I'm not sure what's going on here. By the time
'Uncompressing Linux...' comes out all relocations and dtb discovery is
complete. Do you really think having lr uninitialized would cause this?
>
>
[snipped rest of patch out]
>
> Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-21 17:35 ` John Bonesio
@ 2011-03-21 18:22 ` Nicolas Pitre
2011-03-21 18:36 ` John Bonesio
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Pitre @ 2011-03-21 18:22 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 21 Mar 2011, John Bonesio wrote:
> Hi Nicolas,
>
> Thanks for the comments. I have a question (below).
>
[...]
> >> +#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 lr, [r6, #0]
> >> +#ifndef __ARMEB__
> >> + ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> >> +#else
> >> + ldr r1, =0xd00dfeed
> >> +#endif
> >> + cmp lr, r1
> >> + bne dtb_check_done
> >> +
> >> + mov r8, r6 @ use the appended device tree
> >> +
> >> + /* Get the dtb's size */
> >> + ldr lr, [r6, #4]
> >> +
> >> +#ifndef __ARMEB__
> >> + /* convert lr (dtb size) to little endian */
> >> + eor r1, lr, lr, ror #16
> >> + bic r1, r1, #0x00ff0000
> >> + mov lr, lr, ror #8
> >> + eor lr, lr, r1, lsr #8
> >> +#endif
> >> + /*
> >> + * dtb size rounded up to a multiple of 8 bytes so a
> >> + * misaligned GOT entry doesn't cause the end of the
> >> + * dtb binary to be overwritten.
> >> + */
> >> + add lr, lr, #7
> >> + bic lr, lr, #7
> >> +
> >> + add r10, r10, lr
> >> + add r6, r6, lr
> >> +dtb_check_done:
> >> +#endif
> >
> > Now if no dtb was found, or if that code is not configured, then lr
> > contains garbage and that may have random consequences with the code
> > that follows, which would explain the reported bad behaviors with this
> > patch applied even if not configured in the kernel.
>
> I'm probably not understanding something here. When I look at the code
> prior to my patch, it looks like lr is not initialized by head.S.
Exact.
> It also looks like this code makes use of lr when it relocates the
> code and leaves it uninitialized when it jumps back to 'restart'.
Exact.
> Is this a bug in the previous code too?
No.
> What should lr be initialized to, or should we just be preserving it's
> value?
No. The problem is that you do use lr further down in your patch when
adjusting GOT entries, assuming it contains the size of the dtb. If no
DTB is found then lr contains random stuff that will mess up the GOT for
.bss references.
> I saw the comment about the performance is different with this patch -
> something about a pause between 'Uncompressing Linux...' and the kernel
> boot output. I'm not sure what's going on here. By the time
> 'Uncompressing Linux...' comes out all relocations and dtb discovery is
> complete. Do you really think having lr uninitialized would cause this?
Certainly. See above.
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage
2011-03-21 18:22 ` Nicolas Pitre
@ 2011-03-21 18:36 ` John Bonesio
0 siblings, 0 replies; 10+ messages in thread
From: John Bonesio @ 2011-03-21 18:36 UTC (permalink / raw)
To: linux-arm-kernel
Comments below.
- John
On 03/21/2011 11:22 AM, Nicolas Pitre wrote:
> On Mon, 21 Mar 2011, John Bonesio wrote:
>
>> Hi Nicolas,
>>
>> Thanks for the comments. I have a question (below).
>>
> [...]
[snip]
>>>
>>> Now if no dtb was found, or if that code is not configured, then lr
>>> contains garbage and that may have random consequences with the code
>>> that follows, which would explain the reported bad behaviors with this
>>> patch applied even if not configured in the kernel.
>>
>> I'm probably not understanding something here. When I look at the code
>> prior to my patch, it looks like lr is not initialized by head.S.
>
> Exact.
>
>> It also looks like this code makes use of lr when it relocates the
>> code and leaves it uninitialized when it jumps back to 'restart'.
>
> Exact.
>
>> Is this a bug in the previous code too?
>
> No.
>
>> What should lr be initialized to, or should we just be preserving it's
>> value?
>
> No. The problem is that you do use lr further down in your patch when
> adjusting GOT entries, assuming it contains the size of the dtb. If no
> DTB is found then lr contains random stuff that will mess up the GOT for
> .bss references.
Oh! Good catch. Thanks.
>
>> I saw the comment about the performance is different with this patch -
>> something about a pause between 'Uncompressing Linux...' and the kernel
>> boot output. I'm not sure what's going on here. By the time
>> 'Uncompressing Linux...' comes out all relocations and dtb discovery is
>> complete. Do you really think having lr uninitialized would cause this?
>
> Certainly. See above.
>
>
> Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-03-21 18:36 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-09 21:58 [PATCH] ARM:boot:device tree: Allow the device tree binary to be appended to zImage John Bonesio
2011-03-09 23:05 ` Ben Dooks
2011-03-12 8:44 ` Grant Likely
2011-03-15 3:44 ` Shawn Guo
2011-03-15 7:52 ` Shawn Guo
2011-03-15 8:03 ` Grant Likely
2011-03-17 18:42 ` Nicolas Pitre
2011-03-21 17:35 ` John Bonesio
2011-03-21 18:22 ` Nicolas Pitre
2011-03-21 18:36 ` John Bonesio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).