[U-Boot] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.

[Joakim Tjernlund]

>
>
> 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 :)
>
> >
> > > +   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 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).

So I got the BDI connected and did som experiments.
Having RESET_VECTOR in the initial stack chain makes gdb print this:
(gdb) bt
#0  cpu_init_early_f () at cpu_init_early.c:116
#1  0xeff800a4 in _start_cont () at start.S:877
Backtrace stopped: frame did not save the PC
(gdb)

So I don't think there is any benefit trying to squeeze RESET_VECTOR into the stack chain.

 Jocke