On 07/21/2012 10:06 AM, Joakim Tjernlund wrote:
>
> Scott Wood <scottw...@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.tjernl...@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@h
>>> - ori r1,r1,CONFIG_SYS_INIT_SP_OFFSET@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@h
> ori r0,RESET_VECTOR@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
