On 07/23/2012 12:11 PM, Joakim Tjernlund wrote: > > > Scott Wood <scottw...@freescale.com> wrote on 2012/07/23 18:52:28: > >> From: Scott Wood <scottw...@freescale.com> >> To: Joakim Tjernlund <joakim.tjernl...@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 <scottw...@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.tjernl...@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
Well, we do want it to stop the backtrace at that point. :-) ...but I was a bit confused when I thought it would help terminate things. The NULL LR only helps prevent finding something worse, if something happens to do a backtrace immediately after the first stwu but before a real LR is saved. -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
