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

[Scott Wood <scottwood@freescale.com>]

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