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