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 Jocke _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
