[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Thomas Dörfler]

The mpc512x FEC network driver resets the FEC before transferring
packets. With this reset, the FEC clears the previous setting of MII
transfer speed. This patch ensures, that it is set again to a reasonable
value after a FEC reset.

Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
--
diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
index fb2c19a..f3d76ba 100644
--- a/drivers/net/mpc512x_fec.c
+++ b/drivers/net/mpc512x_fec.c
@@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
         * wait at least 16 clock cycles
         */
        udelay (10);
+       /*
+        * NOTE: reset will also clear the MII speed register
+        * we should reinitialize it ASAP
+        */
+       if (fec->xcv_type != SEVENWIRE) {
+               /*
+                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
+                * and do not drop the Preamble.
+                */
+               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
+                                       + 1)
+                                      << 1);
+       }
 #if (DEBUG & 0x3)
        printf ("Ethernet task stopped\n");
 #endif
-- 

--------------------------------------------
Embedded Brains GmbH
Thomas Doerfler        Obere Lagerstrasse 30
D-82178 Puchheim       Germany
email: Thomas.Doerfler@embedded-brains.de
Phone: +49-89-18908079-2
Fax:   +49-89-18908079-9

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Wolfgang Denk]

Dear =?ISO-8859-15?Q?Thomas_D=F6rfler?=,

In message <4A633D5A.3060803@embedded-brains.de> you wrote:
> The mpc512x FEC network driver resets the FEC before transferring
> packets. With this reset, the FEC clears the previous setting of MII
> transfer speed. This patch ensures, that it is set again to a reasonable
> value after a FEC reset.

Hmmm... what is the exact effect of the bug you are trying to fix
here? Do you have any example how to trigger erroneous behaviour?

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
If I can have honesty, it's easier to overlook mistakes.
	-- Kirk, "Space Seed", stardate 3141.9

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Thomas Dörfler]

Hello,

on our custom MPC5121 board I used the following sequence to see the bug:

- After U-Boot stated, I could inspect the status of the PHY with "mii
info". This works fine even if repeated.

- then I trigger some network activity, for example with "ping"

- if I try to get the PHY status with "mii info" AFTER that network
activity, the command fails (it does not even detect, that a PHY is
connected).

The reason I could track down is, that:
- the FEC driver performs a FEC reset before the initial packet is
transmitted
- this reset also clears the MII speed register, which defines the
communication speed on MDC/MDIO
- the PHY access works again, when the MII speed register is set to its
proper value.

I have tested this behaviour on a old (M34K) mask, but I am sure it
applies also to the current M36P.

wkr,
Thomas Doerfler.



Wolfgang Denk wrote:
> Dear =?ISO-8859-15?Q?Thomas_D=F6rfler?=,
> 
> In message <4A633D5A.3060803@embedded-brains.de> you wrote:
>> The mpc512x FEC network driver resets the FEC before transferring
>> packets. With this reset, the FEC clears the previous setting of MII
>> transfer speed. This patch ensures, that it is set again to a reasonable
>> value after a FEC reset.
> 
> Hmmm... what is the exact effect of the bug you are trying to fix
> here? Do you have any example how to trigger erroneous behaviour?
> 
> Best regards,
> 
> Wolfgang Denk
> 


-- 

--------------------------------------------
Embedded Brains GmbH
Thomas Doerfler        Obere Lagerstrasse 30
D-82178 Puchheim       Germany
email: Thomas.Doerfler at embedded-brains.de
Phone: +49-89-18908079-2
Fax:   +49-89-18908079-9

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Wolfgang Denk]

Dear =?ISO-8859-1?Q?Thomas_D=F6rfler?=,

In message <4A635D1F.9030402@embedded-brains.de> you wrote:
> 
> on our custom MPC5121 board I used the following sequence to see the bug:
> 
> - After U-Boot stated, I could inspect the status of the PHY with "mii
> info". This works fine even if repeated.
> 
> - then I trigger some network activity, for example with "ping"
> 
> - if I try to get the PHY status with "mii info" AFTER that network
> activity, the command fails (it does not even detect, that a PHY is
> connected).

Thanks for the explanation; I can reproduce this problem on the boards
we have in our lab, too.

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
"A great many people think they are thinking when they are merely re-
arranging their prejudices."                          - William James

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Wolfgang Denk]

Dear =?ISO-8859-15?Q?Thomas_D=F6rfler?=,

In message <4A633D5A.3060803@embedded-brains.de> you wrote:
> The mpc512x FEC network driver resets the FEC before transferring
> packets. With this reset, the FEC clears the previous setting of MII
> transfer speed. This patch ensures, that it is set again to a reasonable
> value after a FEC reset.
> 
> Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
> --
> diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
> index fb2c19a..f3d76ba 100644
> --- a/drivers/net/mpc512x_fec.c
> +++ b/drivers/net/mpc512x_fec.c
> @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
>          * wait at least 16 clock cycles
>          */
>         udelay (10);
> +       /*
> +        * NOTE: reset will also clear the MII speed register
> +        * we should reinitialize it ASAP
> +        */
> +       if (fec->xcv_type != SEVENWIRE) {
> +               /*
> +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
> +                * and do not drop the Preamble.
> +                */
> +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
> +                                       + 1)
> +                                      << 1);
> +       }
>  #if (DEBUG & 0x3)
>         printf ("Ethernet task stopped\n");
>  #endif
> -- 

This patch is white-space corrupted. Please fix your mailer settings.

Also, you should use I/O accessors instead of direct pointer accesses
like we do it in the rest of the code (maybe you are using an obsolete
code base?).

Finally I disagree with the solution: I think it is actually correct
to disable the MII clock, too, when we stop the Ethernet interface.

However, we should make sure that the PHY gets (re-) initialized (by
running mpc512x_fec_init_phy()) if needed when a MII command is run.


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
A person who is more than casually interested in computers should  be
well  schooled in machine language, since it is a fundamental part of
a computer.                                           -- Donald Knuth

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Thomas Dörfler]

Hello,

thank you for your feedback. Sorry for the "newbie" problems, I should
have checked this before sending the patch.

But I doubt that (re-)calling mpc512x_fec_init_phy before a MII command
is run is really a good idea. Using the same code for initial init and
repeated init makes sense, but this function in its current
implementation also resets the PHY, which forces a new line negotiation
to occur. IMHO this is NOT needed when we e.g. query the PHY status and
it would even be contraproductive, if we would change some PHY register
settings, because these changes would also be reset.

What COULD make sense is to extract the MII speed register setting
operation from mpc512x_fec_init_phy() and move it to the
mpc512x_fec_mii_read/write functions. because this is where it is really
needed.

Does this make sense to you?

wkr,
Thomas Doerfler.




Wolfgang Denk wrote:
> Dear =?ISO-8859-15?Q?Thomas_D=F6rfler?=,
> 
> In message <4A633D5A.3060803@embedded-brains.de> you wrote:
>> The mpc512x FEC network driver resets the FEC before transferring
>> packets. With this reset, the FEC clears the previous setting of MII
>> transfer speed. This patch ensures, that it is set again to a reasonable
>> value after a FEC reset.
>>
>> Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
>> --
>> diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
>> index fb2c19a..f3d76ba 100644
>> --- a/drivers/net/mpc512x_fec.c
>> +++ b/drivers/net/mpc512x_fec.c
>> @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
>>          * wait at least 16 clock cycles
>>          */
>>         udelay (10);
>> +       /*
>> +        * NOTE: reset will also clear the MII speed register
>> +        * we should reinitialize it ASAP
>> +        */
>> +       if (fec->xcv_type != SEVENWIRE) {
>> +               /*
>> +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
>> +                * and do not drop the Preamble.
>> +                */
>> +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
>> +                                       + 1)
>> +                                      << 1);
>> +       }
>>  #if (DEBUG & 0x3)
>>         printf ("Ethernet task stopped\n");
>>  #endif
>> -- 
> 
> This patch is white-space corrupted. Please fix your mailer settings.
> 
> Also, you should use I/O accessors instead of direct pointer accesses
> like we do it in the rest of the code (maybe you are using an obsolete
> code base?).
> 
> Finally I disagree with the solution: I think it is actually correct
> to disable the MII clock, too, when we stop the Ethernet interface.
> 
> However, we should make sure that the PHY gets (re-) initialized (by
> running mpc512x_fec_init_phy()) if needed when a MII command is run.
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 


-- 

--------------------------------------------
Embedded Brains GmbH
Thomas Doerfler        Obere Lagerstrasse 30
D-82178 Puchheim       Germany
email: Thomas.Doerfler at embedded-brains.de
Phone: +49-89-18908079-2
Fax:   +49-89-18908079-9

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Ben Warren]

Thomas,

Thomas D?rfler wrote:
> The mpc512x FEC network driver resets the FEC before transferring
> packets. With this reset, the FEC clears the previous setting of MII
> transfer speed. This patch ensures, that it is set again to a reasonable
> value after a FEC reset.
>
> Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
> --
> diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
> index fb2c19a..f3d76ba 100644
> --- a/drivers/net/mpc512x_fec.c
> +++ b/drivers/net/mpc512x_fec.c
> @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
>          * wait at least 16 clock cycles
>          */
>         udelay (10);
> +       /*
> +        * NOTE: reset will also clear the MII speed register
> +        * we should reinitialize it ASAP
> +        */
> +       if (fec->xcv_type != SEVENWIRE) {
> +               /*
> +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
> +                * and do not drop the Preamble.
> +                */
> +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
> +                                       + 1)
> +                                      << 1);
> +       }
>  #if (DEBUG & 0x3)
>         printf ("Ethernet task stopped\n");
>  #endif
>   
Applied to net repo (after some tweaking)

thanks,
Ben

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Wolfgang Denk]

Dear Ben Warren,

In message <4A6565D4.7050400@gmail.com> you wrote:
> 
> Thomas D?rfler wrote:
> > The mpc512x FEC network driver resets the FEC before transferring
> > packets. With this reset, the FEC clears the previous setting of MII
> > transfer speed. This patch ensures, that it is set again to a reasonable
> > value after a FEC reset.
> >
> > Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
> > --
> > diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
> > index fb2c19a..f3d76ba 100644
> > --- a/drivers/net/mpc512x_fec.c
> > +++ b/drivers/net/mpc512x_fec.c
> > @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
> >          * wait at least 16 clock cycles
> >          */
> >         udelay (10);
> > +       /*
> > +        * NOTE: reset will also clear the MII speed register
> > +        * we should reinitialize it ASAP
> > +        */
> > +       if (fec->xcv_type != SEVENWIRE) {
> > +               /*
> > +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
> > +                * and do not drop the Preamble.
> > +                */
> > +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
> > +                                       + 1)
> > +                                      << 1);
> > +       }
> >  #if (DEBUG & 0x3)
> >         printf ("Ethernet task stopped\n");
> >  #endif
> >   
> Applied to net repo (after some tweaking)

Sorry Ben, but I am not convinced that this is a good way to  address
the problem. Please see the previous discussion of this patch.

I think it  is  perfectly  reasonable  that  mpc512x_fec_halt()  also
disables  the  MII  clock.  After  all,  when the network intrface is
stopped, there is no need that the clocks keep  running  and  drawing
current on battery powered devices.

I'd rather see that the MII clock gets only started when needed, i. e.
before running any MII commands.

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
I've already got a female to worry about. Her name is the Enterprise.
	-- Kirk, "The Corbomite Maneuver", stardate 1514.0

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Ben Warren]

Wolfgang Denk wrote:
> Dear Ben Warren,
>
> In message <4A6565D4.7050400@gmail.com> you wrote:
>   
>> Thomas D?rfler wrote:
>>     
>>> The mpc512x FEC network driver resets the FEC before transferring
>>> packets. With this reset, the FEC clears the previous setting of MII
>>> transfer speed. This patch ensures, that it is set again to a reasonable
>>> value after a FEC reset.
>>>
>>> Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
>>> --
>>> diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
>>> index fb2c19a..f3d76ba 100644
>>> --- a/drivers/net/mpc512x_fec.c
>>> +++ b/drivers/net/mpc512x_fec.c
>>> @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
>>>          * wait at least 16 clock cycles
>>>          */
>>>         udelay (10);
>>> +       /*
>>> +        * NOTE: reset will also clear the MII speed register
>>> +        * we should reinitialize it ASAP
>>> +        */
>>> +       if (fec->xcv_type != SEVENWIRE) {
>>> +               /*
>>> +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
>>> +                * and do not drop the Preamble.
>>> +                */
>>> +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
>>> +                                       + 1)
>>> +                                      << 1);
>>> +       }
>>>  #if (DEBUG & 0x3)
>>>         printf ("Ethernet task stopped\n");
>>>  #endif
>>>   
>>>       
>> Applied to net repo (after some tweaking)
>>     
>
> Sorry Ben, but I am not convinced that this is a good way to  address
> the problem. Please see the previous discussion of this patch.
>
> I think it  is  perfectly  reasonable  that  mpc512x_fec_halt()  also
> disables  the  MII  clock.  After  all,  when the network intrface is
> stopped, there is no need that the clocks keep  running  and  drawing
> current on battery powered devices.
>
> I'd rather see that the MII clock gets only started when needed, i. e.
> before running any MII commands.
>
> Best regards,
>
> Wolfgang Denk
>
>   
Yikes, sorry again.  I saw the one where you said 'thanks for the 
explanation, I can reproduce...'

Better hold off on my pull request :)

Ben

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Ben Warren]

Wolfgang Denk wrote:
> Dear Ben Warren,
>
> In message <4A6565D4.7050400@gmail.com> you wrote:
>   
>> Thomas D?rfler wrote:
>>     
>>> The mpc512x FEC network driver resets the FEC before transferring
>>> packets. With this reset, the FEC clears the previous setting of MII
>>> transfer speed. This patch ensures, that it is set again to a reasonable
>>> value after a FEC reset.
>>>
>>> Signed-off-by: Thomas Doerfler <Thomas.Doerfler@embedded-brains.de>
>>> --
>>> diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
>>> index fb2c19a..f3d76ba 100644
>>> --- a/drivers/net/mpc512x_fec.c
>>> +++ b/drivers/net/mpc512x_fec.c
>>> @@ -446,6 +446,19 @@ static void mpc512x_fec_halt (struct eth_device *dev)
>>>          * wait at least 16 clock cycles
>>>          */
>>>         udelay (10);
>>> +       /*
>>> +        * NOTE: reset will also clear the MII speed register
>>> +        * we should reinitialize it ASAP
>>> +        */
>>> +       if (fec->xcv_type != SEVENWIRE) {
>>> +               /*
>>> +                * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
>>> +                * and do not drop the Preamble.
>>> +                */
>>> +               fec->eth->mii_speed = ((((gd->ips_clk / 1000000) / 5)
>>> +                                       + 1)
>>> +                                      << 1);
>>> +       }
>>>  #if (DEBUG & 0x3)
>>>         printf ("Ethernet task stopped\n");
>>>  #endif
>>>   
>>>       
>> Applied to net repo (after some tweaking)
>>     
>
> Sorry Ben, but I am not convinced that this is a good way to  address
> the problem. Please see the previous discussion of this patch.
>
> I think it  is  perfectly  reasonable  that  mpc512x_fec_halt()  also
> disables  the  MII  clock.  After  all,  when the network intrface is
> stopped, there is no need that the clocks keep  running  and  drawing
> current on battery powered devices.
>
> I'd rather see that the MII clock gets only started when needed, i. e.
> before running any MII commands.
>
> Best regards,
>
> Wolfgang Denk
>
>   
Yikes, sorry again.  I saw the one where you said 'thanks for the
explanation, I can reproduce...'

Better hold off on my pull request :)

On that note, I'm off to bed.  Hopefully the brain is better engaged 
tomorrow.

Ben

[U-Boot] [PATCH] Bugfix: mpc512x_fec MII accesses fail after ethernet transfers

[Wolfgang Denk]

Dear Thomas D?rfler,

In message <4A637B05.4030306@embedded-brains.de> you wrote:
>
> But I doubt that (re-)calling mpc512x_fec_init_phy before a MII command
> is run is really a good idea. Using the same code for initial init and
> repeated init makes sense, but this function in its current
> implementation also resets the PHY, which forces a new line negotiation
> to occur. IMHO this is NOT needed when we e.g. query the PHY status and
> it would even be contraproductive, if we would change some PHY register
> settings, because these changes would also be reset.

Agreed.

> What COULD make sense is to extract the MII speed register setting
> operation from mpc512x_fec_init_phy() and move it to the
> mpc512x_fec_mii_read/write functions. because this is where it is really
> needed.
>
> Does this make sense to you?

Yes, it does. But we should double-check if setting the MII_SPEED
register is all that should be (re-) initialized.

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
"We all agree on the necessity of compromise. We just can't agree  on
when it's necessary to compromise."
                - Larry Wall in  <1991Nov13.194420.28091@netlabs.com>