[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Mats Kärrman]

> Now when I'm trying to migrate to U-Boot 2013.01 I face problems with a sudden hang. I see console output until the "Flash:" from arch/powerpc/lib/board.c::board_init_r() but nothing more until the board reboots on a watchdog reset after 30s.

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.

I am not fluent enough in powerpc assembly or gcc stack frames for the same to see exactly what is wrong. Perhaps someone with the necessary mileage could take a look?

BR // Mats

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Wolfgang Denk]

Dear Mats K?rrman,

In message <ED3E0BCACD909541BA94A34C4A164D4C427B4AA5@post.tritech.se> you wrote:
>
> 
> 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.

> I am not fluent enough in powerpc assembly or gcc stack frames for the same to see exactly what is wrong. Perhaps someone with the necessary mileage could take a look?

Well, you mentioned you were using a MPC5125 processor which is not
supported in mainline U-Boot, and which is known to have enough
differences in it's register layout that anything might be the cause
for such issues.

Is it correct to assume that you have a _lot_ of local modifiucations
in your tree?  I bet the problem lies there...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anything free is worth what you pay for it.

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Mats Kärrman]

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.

If you say it works, it makes no sense to me either. I thought it might be some subtle difference in the way the e300c4 core or GCC works compared to other PPC cores. If there are no such differences, then a patch with no intended functional difference beyond keeping gdb happy should not make any difference, good or bad.

> Well, you mentioned you were using a MPC5125 processor which is not
> supported in mainline U-Boot, and which is known to have enough
> differences in it's register layout that anything might be the cause
> for such issues.
>
> Is it correct to assume that you have a _lot_ of local modifiucations
> in your tree?  I bet the problem lies there...

I wouldn't say a lot. I'm using only the serial console to be able to download and flash images (using CFI flash) and to boot Linux so the primary patches are the serial driver and the DRAM initialization. On top of that there are some minimal I/O initialization and of course I went through and updated the register mappings for the relevant peripherals. It worked fine with 2009.03 and with 2012.07 but of course I might have overlooked something.

I will report back when / if I find out something more.
I'm not much for betting though ;)

Best regards,
Mats

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Joakim Tjernlund]

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.

None of the above is a real fix tough.

> If you say it works, it makes no sense to me either. I thought it might 
be 
> some subtle difference in the way the e300c4 core or GCC works compared 
to 
> other PPC cores. If there are no such differences, then a patch with no 
> intended functional difference beyond keeping gdb happy should not make 
any 
> difference, good or bad.

hmm, we use e300c2 with no problem.

 Jocke

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Joakim Tjernlund]

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

 Jocke

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Joakim Tjernlund]

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 at 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:
--- 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

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Mats Kärrman]

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

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Mats Kärrman]

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

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Joakim Tjernlund]

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

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Mats Kärrman]

Hi!

I have a system using a MPC5125 processor working fine using U-Boot 2012.07. I have based my board adaption etc on the mpc5121ads board and also patched the necessary parts of the serial port driver.

Now when I'm trying to migrate to U-Boot 2013.01 I face problems with a sudden hang. I see console output until the "Flash:" from arch/powerpc/lib/board.c::board_init_r() but nothing more until the board reboots on a watchdog reset after 30s.

I have looked for relevant changes in the U-Boot code and tried some printf debugging. So far I have been unable to pin down exactly where things go wrong.

Question: To help me decide where to look I would like to know if anyone has built the mpc5121ads board configuration using U-Boot 2013.01 and actually tested that it is still working?

Thanks & best regards,
Mats

[U-Boot] mpc512x: Trouble migrating from 2012.07 to 2013.01

[Wolfgang Denk]

Dear Mats,

In message <ED3E0BCACD909541BA94A34C4A164D4C427B39B4@post.tritech.se> you wrote:
> 
> Question: To help me decide where to look I would like to know if anyone has built the mpc5121ads board configuration using U-Boot 2013.01 and actually tested that it is still working?

Yes, v2012.01 is working on the MPC5121ADS board:

U-Boot 2013.01 (Jan 23 2013 - 21:37:51)MPC512X

CPU:   MPC5121e rev. 2.0, Core e300c4 at 400 MHz, CSB at 200 MHz
(RSR=0x1003)
Board: MPC5121ADS rev. 0x0400 (CPLD rev. 0x05)
I2C:   ready
DRAM:  512 MiB
Flash: 64 MiB
NAND:  1024 MiB
PCI:   Bus Dev VenId DevId Class Int
In:    serial
Out:   serial
Err:   serial
Net:   FEC
IDE:   Bus 0:
............................................................** Timeout **

Type run flash_nfs to mount root filesystem over NFS

Hit any key to stop autoboot:  0 
=> 



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In any group of employed individuals the only naturally  early  riser
is  _always_  the office manager, who will _always_ leave reproachful
little notes ... on the desks of their subordinates.
                                - Terry Pratchett, _Lords and Ladies_