[U-Boot] [PATCH] armv7: Fix infinite loop for the spl boot
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com --- arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__ + cmp r0, r1 + beq skip_clbss #else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
/* * We are done. Do not return, instead branch to second part of board
Hi Albert,
Could you applied the patch to the arm tree?
Thanks, hongbo On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com
arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__
- cmp r0, r1
- beq skip_clbss
#else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
/*
- We are done. Do not return, instead branch to second part of board
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
Hi Albert,
Could you applied the patch to the arm tree?
Thanks, hongbo On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com
arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__
- cmp r0, r1
- beq skip_clbss
#else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".
I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Thanks in advance.
Amicalement,
Hi Albert, On 07/05/2012 08:19 PM, Albert ARIBAUD wrote:
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
Hi Albert,
Could you applied the patch to the arm tree?
Thanks, hongbo On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com
arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__
- cmp r0, r1
- beq skip_clbss
#else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".
Ok, Good to known, I will send V2
I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:
Ok,
Thanks, hongbo
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Thanks in advance.
Amicalement,
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
Hi Albert,
Could you applied the patch to the arm tree?
Thanks, hongbo On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com
arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__
- cmp r0, r1
- beq skip_clbss
#else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".
I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
Thanks in advance.
Amicalement,
On 07/06/2012 01:35 AM, Albert ARIBAUD albert.u.boot@aribaud.net (by way of Albert ARIBAUD wrote:
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
Hi Albert,
Could you applied the patch to the arm tree?
Thanks, hongbo On 07/03/2012 07:46 AM, Zhong Hongbo wrote:
From: Zhong Hongbo bocui107@gmail.com
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by: Hongbo Zhong bocui107@gmail.com
arch/arm/cpu/armv7/start.S | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index 76ccef1..c72f337 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -258,6 +258,8 @@ clear_bss: /* No relocation for SPL */ ldr r0, =__bss_start ldr r1, =__bss_end__
- cmp r0, r1
- beq skip_clbss
#else ldr r0, _bss_start_ofs ldr r1, _bss_end_ofs @@ -271,6 +273,7 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 bne clbss_l +skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".
Yes， My new patch have do this.
I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:
Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:
commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d Author: Christian Riesch christian.riesch@omicron.at Date: Wed Nov 30 22:27:37 2011 +0000
arm, arm926ejs: Fix clear bss loop for zero length bss
This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__.
Signed-off-by: Christian Riesch christian.riesch@omicron.at Cc: Albert Aribaud albert.u.boot@aribaud.net
diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 339c5ed..bb4d00b 100644 --- a/arch/arm/cpu/arm926ejs/start.S +++ b/arch/arm/cpu/arm926ejs/start.S @@ -301,10 +301,12 @@ clear_bss: #endif mov r2, #0x00000000 /* clear */
-clbss_l:str r2, [r0] /* clear loop... */ +clbss_l:cmp r0, r1 /* clear loop... */ + bhs clbss_e /* if reached end of bss, exit */ + str r2, [r0] add r0, r0, #4 - cmp r0, r1 - bne clbss_l + b clbss_l +clbss_e:
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
Ok，
Thanks, hongbo
Thanks in advance.
Amicalement,
Dear Zhong Hongbo,
On 06.07.2012 13:42, Zhong Hongbo wrote:
On 07/06/2012 01:35 AM, Albert ARIBAUD albert.u.boot@aribaud.net (by way of Albert ARIBAUD wrote:
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
<snip>
Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:
commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d Author: Christian Riesch christian.riesch@omicron.at Date: Wed Nov 30 22:27:37 2011 +0000
arm, arm926ejs: Fix clear bss loop for zero length bss This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__. Signed-off-by: Christian Riesch <christian.riesch@omicron.at> Cc: Albert Aribaud <albert.u.boot@aribaud.net>diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 339c5ed..bb4d00b 100644 --- a/arch/arm/cpu/arm926ejs/start.S +++ b/arch/arm/cpu/arm926ejs/start.S @@ -301,10 +301,12 @@ clear_bss: #endif mov r2, #0x00000000 /* clear */
-clbss_l:str r2, [r0] /* clear loop... */ +clbss_l:cmp r0, r1 /* clear loop... */
bhs clbss_e /* if reached end of bss, exit */str r2, [r0] add r0, r0, #4
cmp r0, r1bne clbss_l
b clbss_l+clbss_e:
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
Ok，
I think this is the thread we will proceed. So as I understand we had missed in 2011 to update the other start.S with a fix found by Christian Riesch for arm926ejs. So please do update _all_ start.S (of arm;) now. I think they do have all the same error cause we copied it over from one to the other when implementing the 'new relocation' stuff.
Best regards
Andreas Bießmann
On 07/06/2012 07:49 PM, Andreas Bießmann wrote:
Dear Zhong Hongbo,
On 06.07.2012 13:42, Zhong Hongbo wrote:
On 07/06/2012 01:35 AM, Albert ARIBAUD albert.u.boot@aribaud.net (by way of Albert ARIBAUD wrote:
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo bocui107@gmail.com wrote:
<snip>
Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:
commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d Author: Christian Riesch christian.riesch@omicron.at Date: Wed Nov 30 22:27:37 2011 +0000
arm, arm926ejs: Fix clear bss loop for zero length bss This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__. Signed-off-by: Christian Riesch <christian.riesch@omicron.at> Cc: Albert Aribaud <albert.u.boot@aribaud.net>diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 339c5ed..bb4d00b 100644 --- a/arch/arm/cpu/arm926ejs/start.S +++ b/arch/arm/cpu/arm926ejs/start.S @@ -301,10 +301,12 @@ clear_bss: #endif mov r2, #0x00000000 /* clear */
-clbss_l:str r2, [r0] /* clear loop... */ +clbss_l:cmp r0, r1 /* clear loop... */
bhs clbss_e /* if reached end of bss, exit */str r2, [r0] add r0, r0, #4
cmp r0, r1bne clbss_l
b clbss_l+clbss_e:
clbss_l:str r2, [r0] /* clear loop...*/ add r0, r0, #4 cmp r0, r1 bne clbss_l
Into something like
clbss_l:cmp r0, r1 blo clbss_d str r2, [r0] /* clear loop...*/ add r0, r0, #4 b clbss_l clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
Ok，
I think this is the thread we will proceed. So as I understand we had missed in 2011 to update the other start.S with a fix found by Christian Riesch for arm926ejs. So please do update _all_ start.S (of arm;) now. I think they do have all the same error cause we copied it over from one to the other when implementing the 'new relocation' stuff.
Agree, I will do it now.
Thanks, hongbo
Best regards
Andreas Bießmann
participants (4)
-
Albert ARIBAUD -
Albert ARIBAUD <albert.u.boot＠aribaud.net> (by way of Albert ARIBAUD
-
Andreas Bießmann -
Zhong Hongbo