[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 
<wd@denx.de>
> > 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    r0
> This 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:

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    r0
> > This 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

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