From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Mon, 23 Jul 2012 11:52:28 -0500 Subject: [U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong. In-Reply-To: References: <1342776046-25513-1-git-send-email-Joakim.Tjernlund@transmode.se> <1342776046-25513-2-git-send-email-Joakim.Tjernlund@transmode.se> <5009C985.9050408@freescale.com> Message-ID: <500D814C.3070205@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 07/21/2012 10:06 AM, Joakim Tjernlund wrote: > > Scott Wood 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 >>> --- >>> 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 at h >>> - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET at 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 at h > ori r0,RESET_VECTOR at 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) */ -Scott