Re: [U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01
Joakim Tjernlund/Transmode wrote on 2013/01/24 09:58:35:
Joakim Tjernlund/Transmode wrote on 2013/01/24 09:40:45:
From: Joakim Tjernlund/Transmode To: Mats Kärrman Mats.Karrman@tritech.se, Cc: "u-boot@lists.denx.de" u-boot@lists.denx.de, Wolfgang Denk
Date: 2013/01/24 09:40 Subject: RE: [U-Boot] mpc512x: Trouble migrating from 2012.07 to
2013.01
Mats Kärrman Mats.Karrman@tritech.se wrote on 2013/01/23 22:58:56:
Dear Wolfgang Denk,
Found that it was looping endlessly in
arch/powerpc/lib/ticks.S::wait_ticks
(). Reverting commit "ppc: Create a stack frame for wait_ticks()"
made
everything work again.
This makes no sense to me - especially as it works on all other systems.
Me neither, there is not a lot details. I do recall having other
problems with
wait_ticks from time to time, sometimes the TB counter(mtfbu, mftb in
get_ticks)
would not increment so one would just loop forever in wait_ticks. This
problem
had nothing to do with my patch though. Never got around to finding out what caused it, we ended up blaming
unstable HW.
Some ideas though:
- Perhaps you have some alignment problem, try adding nop's here and
there.
- My patch uses r0(which is what one should use to make gdb happy)
instead of r8
to stash the LR. Possibly you have some odd dependency on r0, try
using r8 again.
Try getting LR from stack instead of trusting r0 to be valid:
mtlr r0 /* restore link register */
lwz r0, 20(r1) /* restore link register */mtlr r0This is what one should do but as we have complete control of r0 here we
don't need to.
hmm, do you have WATCHDOG_RESET defined? does it use r0? I guess the above patch would make wait_ticks safer from accidental use
by
WATCHDOG_RESET, if it works for you, please send a proper patch to u-boot.
Looking at the watchdog impl. I see it can be normal C code. This makes wait_ticks unsafe (even before my patch) as wait_ticks relies on r6 and r7 (and with my patch r0 too) to be unmodified.
This MIGHT be the fix: --- a/arch/powerpc/lib/ticks.S +++ b/arch/powerpc/lib/ticks.S @@ -56,13 +56,17 @@ wait_ticks: /* Calculate end time */ addc r7, r4, r7 /* Compute end time lower */ addze r6, r3 /* and end time upper */ - + stw r7, 4(r1) /* Stash r6 and r7 */ + stw r6, 8(r1) WATCHDOG_RESET /* Trigger watchdog, if needed */ + lwz r7, 4(r1) + lwz r6, 8(r1) 1: bl get_ticks /* Get current time */ subfc r4, r4, r7 /* Subtract current time from end time */ subfe. r3, r3, r6 bge 1b /* Loop until time expired */
- mtlr r0 /* restore link register */ + lwz r0, 20(r1) /* restore link register */ + mtlr r0 addi r1,r1,16 blr
Not sure about the 4 and 8 offsets though
Jocke
Joakim Tjernlund/Transmode wrote on 2013/01/24 09:58:35:
Me neither, there is not a lot details. I do recall having other
problems with
wait_ticks from time to time, sometimes the TB counter(mtfbu, mftb in
get_ticks)
would not increment so one would just loop forever in wait_ticks. This
problem
had nothing to do with my patch though. Never got around to finding out what caused it, we ended up blaming
unstable HW.
Some ideas though:
- Perhaps you have some alignment problem, try adding nop's here and
there.
- My patch uses r0(which is what one should use to make gdb happy)
instead of r8
to stash the LR. Possibly you have some odd dependency on r0, try
using r8 again.
Try getting LR from stack instead of trusting r0 to be valid:
mtlr r0 /* restore link register */
lwz r0, 20(r1) /* restore link register */mtlr r0This is what one should do but as we have complete control of r0 here we
don't need to.
hmm, do you have WATCHDOG_RESET defined? does it use r0? I guess the above patch would make wait_ticks safer from accidental use
by
WATCHDOG_RESET, if it works for you, please send a proper patch to u-boot.
Looking at the watchdog impl. I see it can be normal C code. This makes wait_ticks unsafe (even before my patch) as wait_ticks relies on r6 and r7 (and with my patch r0 too) to be unmodified.
This MIGHT be the fix: --- a/arch/powerpc/lib/ticks.S +++ b/arch/powerpc/lib/ticks.S @@ -56,13 +56,17 @@ wait_ticks: /* Calculate end time */ addc r7, r4, r7 /* Compute end time lower */ addze r6, r3 /* and end time upper */
stw r7, 4(r1) /* Stash r6 and r7 */stw r6, 8(r1) WATCHDOG_RESET /* Trigger watchdog, if needed */lwz r7, 4(r1)lwz r6, 8(r1)1: bl get_ticks /* Get current time */ subfc r4, r4, r7 /* Subtract current time from end time */ subfe. r3, r3, r6 bge 1b /* Loop until time expired */
mtlr r0 /* restore link register */
lwz r0, 20(r1) /* restore link register */mtlr r0 addi r1,r1,16 blrNot sure about the 4 and 8 offsets though
Jocke
Thanks for feedback! I will look into this ASAP (maybe not until tomorrow). I'll report back as soon as I have a result. BR, Mats
Joakim Tjernlund/Transmode wrote on 2013/01/24 09:21:
Looking at the watchdog impl. I see it can be normal C code. This makes wait_ticks unsafe (even before my patch) as wait_ticks relies on r6 and r7 (and with my patch r0 too) to be unmodified.
Yes! I can see in the assembly from my watchdog_reset() (arch/powerpc/cpu/mpc5125/cpu.c) that it does not touch r7 and r8 but indeed r0 -- for storing lr in fact so the endless loop is explained!
Many thanks! I'll prepare a patch as soon as I've got one tested.
BR // Mats
Mats Kärrman Mats.Karrman@tritech.se wrote on 2013/01/24 14:31:02:
Joakim Tjernlund/Transmode wrote on 2013/01/24 09:21:
Looking at the watchdog impl. I see it can be normal C code. This
makes
wait_ticks unsafe (even before my patch) as wait_ticks relies on r6 and r7 (and with my patch r0 too) to be unmodified.
Yes! I can see in the assembly from my watchdog_reset()
(arch/powerpc/cpu/mpc5125/
cpu.c) that it does not touch r7 and r8 but indeed r0 -- for storing lr
in
fact so the endless loop is explained!
Many thanks! I'll prepare a patch as soon as I've got one tested.
Great, just one thing though. I think the 4 resp.8 stack offsets should be 8 resp. 12 instead.
Jocke
participants (2)
-
Joakim Tjernlund -
Mats Kärrman