[U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.
Joakim Tjernlund
joakim.tjernlund at transmode.se
Mon Jul 23 19:11:38 CEST 2012
Scott Wood <scottwood at freescale.com> wrote on 2012/07/23 18:52:28:
> From: Scott Wood <scottwood at freescale.com>
> To: Joakim Tjernlund <joakim.tjernlund at transmode.se>,
> Cc: <u-boot at 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 at 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 at 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 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) */
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
More information about the U-Boot
mailing list