[U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 aligned

[U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 aligned

[Joakim Tjernlund]

The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 arch/powerpc/lib/board.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git arch/powerpc/lib/board.c arch/powerpc/lib/board.c
index d5b75e5..dc7cc40 100644
--- arch/powerpc/lib/board.c
+++ arch/powerpc/lib/board.c
@@ -521,9 +521,8 @@ void board_init_f(ulong bootflag)
 	addr_sp -= 16;
 	addr_sp &= ~0xF;
 	s = (ulong *) addr_sp;
-	*s-- = 0;
-	*s-- = 0;
-	addr_sp = (ulong) s;
+	*s = 0;   /* NULL Back Chain */
+	*--s = 0; /* NULL LR */
 	debug("Stack Pointer at: %08lx\n", addr_sp);
 
 	/*
-- 
1.7.3.4

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

[Joakim Tjernlund]

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@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 */
 	li	r0,0
-	stwu	r0,-4(r1)
-	stwu	r0,-4(r1)		/* Terminate call chain */
-
-	stwu	r1,-8(r1)		/* Save back chain and move SP */
-	lis	r0,RESET_VECTOR at h	/* Address of reset vector */
-	ori	r0,r0,RESET_VECTOR at l
-	stwu	r1,-8(r1)		/* Save back chain and move SP */
-	stw	r0,+12(r1)		/* Save return addr (underflow vect) */
+	stw	r0,-4(r2)	/* Store a NULL LR */
+	stw	r0,0(r2)	/* Store a NULL Back Chain */
+	mr	r1,r2		/* Transfer to SP(r1) */
 
 	GET_GOT
 	bl	cpu_init_early_f
-- 
1.7.3.4

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

[Scott Wood]

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@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.

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

-Scott

[U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 aligned

[Scott Wood]

On 07/20/2012 04:20 AM, Joakim Tjernlund wrote:
> The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  arch/powerpc/lib/board.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git arch/powerpc/lib/board.c arch/powerpc/lib/board.c
> index d5b75e5..dc7cc40 100644
> --- arch/powerpc/lib/board.c
> +++ arch/powerpc/lib/board.c
> @@ -521,9 +521,8 @@ void board_init_f(ulong bootflag)
>  	addr_sp -= 16;
>  	addr_sp &= ~0xF;
>  	s = (ulong *) addr_sp;
> -	*s-- = 0;
> -	*s-- = 0;
> -	addr_sp = (ulong) s;
> +	*s = 0;   /* NULL Back Chain */
> +	*--s = 0; /* NULL LR */

Same problems as in patch 2/2.

-Scott

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

[Joakim Tjernlund]

Scott Wood <scottwood@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@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).

 Jocke

[U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 aligned

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2012/07/20 23:12:49:

> From: Scott Wood <scottwood@freescale.com>
> To: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>,
> Cc: <u-boot@lists.denx.de>
> Date: 2012/07/20 23:12
> Subject: Re: [U-Boot] [PATCH 1/2] powerpc: Stack Pointer must be 16 aligned
>
> On 07/20/2012 04:20 AM, Joakim Tjernlund wrote:
> > The PowerPC ABI mandates the SP to be 16 bytes aligned, make is so.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  arch/powerpc/lib/board.c |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git arch/powerpc/lib/board.c arch/powerpc/lib/board.c
> > index d5b75e5..dc7cc40 100644
> > --- arch/powerpc/lib/board.c
> > +++ arch/powerpc/lib/board.c
> > @@ -521,9 +521,8 @@ void board_init_f(ulong bootflag)
> >     addr_sp -= 16;
> >     addr_sp &= ~0xF;
> >     s = (ulong *) addr_sp;
> > -   *s-- = 0;
> > -   *s-- = 0;
> > -   addr_sp = (ulong) s;
> > +   *s = 0;   /* NULL Back Chain */
> > +   *--s = 0; /* NULL LR */
>
> Same problems as in patch 2/2.

Right, that should be
    *s = 0;		/* NULL Back Chain */
    *++s = 0;	/* NULL LR */

 Jocke

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

[Joakim Tjernlund]

>
>
> Scott Wood <scottwood@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@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

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

[Scott Wood]

On 07/21/2012 10:06 AM, Joakim Tjernlund wrote:
> 
> Scott Wood <scottwood@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@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) */

-Scott

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

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2012/07/23 18:52:28:

> From: Scott Wood <scottwood@freescale.com>
> To: Joakim Tjernlund <joakim.tjernlund@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 <scottwood@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@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

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

[Scott Wood]

On 07/23/2012 12:11 PM, Joakim Tjernlund wrote:
> 
> 
> Scott Wood <scottwood@freescale.com> wrote on 2012/07/23 18:52:28:
> 
>> From: Scott Wood <scottwood@freescale.com>
>> To: Joakim Tjernlund <joakim.tjernlund@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 <scottwood@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@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

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] [PATCH 2/2] mpc85xx: Initial SP alignment is wrong.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2012/07/23 19:28:20:
>
> On 07/23/2012 12:11 PM, Joakim Tjernlund wrote:
> >
> >


> ...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.

So I just sent v2, now I am offline for a few days/a week.

 Jocke