Scott Wood scottwood@freescale.com wrote on 2012/07/23 18:52:28:
From: Scott Wood scottwood@freescale.com To: Joakim Tjernlund joakim.tjernlund@transmode.se, Cc: u-boot@lists.denx.de Date: 2012/07/23 18:52 Subject: Re: [U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.
On 07/21/2012 10:06 AM, Joakim Tjernlund wrote:
Scott Wood scottwood@freescale.com wrote on 2012/07/20 23:11:33:
On 07/20/2012 04:20 AM, Joakim Tjernlund wrote:
PowerPC mandates SP to be 16 bytes aligned. Furthermore, a stack frame is added, pointing to the reset vector which is just get in the way when gdb is walking the stack because the reset vector may not accessible depending on emulator settings. Also use a temp register so gdb doesn't pick up intermediate values.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc85xx/start.S | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)
diff --git arch/powerpc/cpu/mpc85xx/start.S arch/powerpc/cpu/mpc85xx/start.S index 8d66cf1..8c75af9 100644 --- arch/powerpc/cpu/mpc85xx/start.S +++ arch/powerpc/cpu/mpc85xx/start.S @@ -848,18 +848,12 @@ version_string: .globl _start_cont _start_cont: /* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/
- lis r1,CONFIG_SYS_INIT_RAM_ADDR@h
- ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET@l
- lis r2,(CONFIG_SYS_INIT_RAM_ADDR)@h
- ori r2,r2,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
Please don't use r2 as a general purpose register -- even if it's early enough that we haven't started using it for its fixed purpose yet.
I guess that would be a tiny improvement, but it is nothing compared with the r1 abuse in this file :)
Yeah, I just wanted to discourage making it worse.
- stw r0,-4(r2) /* Store a NULL LR */
- stw r0,0(r2) /* Store a NULL Back Chain */
- mr r1,r2 /* Transfer to SP(r1) */
Why are you storing anything below the stack pointer? LR goes above the backchain, not below -- and it's not valid anyway for the current stack frame. If you want to have a terminating stack frame with a NULL LR, you need to create a second stack frame, like the code currently does.
Right, it should be +4 where the NULL LR should be. As you said, it doesn't matter because it is not valid for this stack frame and could be left out.
I do not want to create a frame as the current code does, it might create an memory access to 0xfffffffc which I don't want.
How would that happen?
How about this then:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ mr r1,r3 /* Transfer to SP(r1) */
or if that reset address is wanted: lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,+4(r3) /* NULL LR, to be nice. */ stwu r3,-16(r3) /* save back chain and move SP */ lis r0,RESET_VECTOR@h ori r0,RESET_VECTOR@l stw r0,+20(r3) /* save return address */ mr r1,r3 /* Transfer to SP(r1) */
I would have to test with the emulator if both are OK or if there will be an access to RESET_VECTOR(I don't have that connected ATM).
How about:
lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ li r0,0 stw r0,0(r3) /* Terminate Back Chain */ stw r0,4(r3) /* NULL LR, to be nice. */stwu r3,-16(r3) /* Create new stack frame * so NULL LR is valid */ mr r1,r3 /* Transfer to SP(r1) */
This also confuses gdb (gdb) bt #0 cpu_init_early_f () at cpu_init_early.c:96 #1 0xeff80098 in _start_cont () at start.S:863 Backtrace stopped: frame did not save the PC
that extra stwu creates an improper call chain. We have the initial stack frame already without the stwu
Jocke