Phy read timeout in ibm_new_emac driver

Phy read timeout in ibm_new_emac driver

[M B]

The __emac_mdio_read function of the emac driver (core.c) will retry
100 times and wait 99us (until the last call of emac_phy_done). I
assume this is an off by one error.

	/* Wait for read to complete */
	n = 100;
	while (!emac_phy_done(dev, (r = in_be32(&p->stacr)))) {
		udelay(1);
		if (!--n) {
			DBG2(dev, " -> timeout wait complete\n");
			goto bail;
		}
	}

My Micrel/Kendin KSZ8721BT on my ppc405EP board needs one us longer to
finish. I was able to reproduce this all the time. So I wonder if the
timeout of 100us is defined by the MII standard, or by the author of
the driver?
If it's a standard I've still a bad feeling if we just correct the
timeout to 100us, maybe 110 should be fine. If it's not defined by the
standard, I would add 50% to the timeout. It won't slow down other
phys, but a scan on the phy bus might get slowed down.
Same applies for __emac_mdio_write.

Oh and we could save a us by putting the udelay(1) after the if section ;-)

Regards

Markus

Re: Phy read timeout in ibm_new_emac driver

[Benjamin Herrenschmidt]

> My Micrel/Kendin KSZ8721BT on my ppc405EP board needs one us longer to
> finish. I was able to reproduce this all the time. So I wonder if the
> timeout of 100us is defined by the MII standard, or by the author of
> the driver?
> If it's a standard I've still a bad feeling if we just correct the
> timeout to 100us, maybe 110 should be fine. If it's not defined by the
> standard, I would add 50% to the timeout. It won't slow down other
> phys, but a scan on the phy bus might get slowed down.
> Same applies for __emac_mdio_write.
> 
> Oh and we could save a us by putting the udelay(1) after the if section ;-)

Increasing the timeout is fine. In fact, EMAC specifically can sleep in
it's MDIO access routines (it already takes mutexes) so maybe a good
option here is to use longer sleeping delays and less iterations.

Somebody knows off hand what the standard says the timeout should be ? I
can check that tomorrow, I don't have it at hand right now and it's
getting late but feel free to beat me to it :-) 

Cheers,
Ben.

Re: Phy read timeout in ibm_new_emac driver

[Markus Brunner]

On Wednesday 16 April 2008, Benjamin Herrenschmidt wrote:

> Somebody knows off hand what the standard says the timeout should be ?

Anyone?

I didn't find any documentation on the standard, but I had a look at other 
drivers. 
au1000_eth.c waits 20 ms (20 * 1ms) in mdio_read.
bfin_mac.c waits 500 * 1us in mdio_poll. 
In both functions the last delay before the timeout is useless, like in 
new_emac. Not nice, but timeouts shouldn't occur anyway.

new emac probably doesn't wait long enough, but 20ms seems to be a bit too 
long.

Regards
Markus

Re: Phy read timeout in ibm_new_emac driver

[Benjamin Herrenschmidt]

On Wed, 2008-04-23 at 07:06 +0200, Markus Brunner wrote:
> On Wednesday 16 April 2008, Benjamin Herrenschmidt wrote:
> 
> > Somebody knows off hand what the standard says the timeout should be ?
> 
> Anyone?
> 
> I didn't find any documentation on the standard, but I had a look at other 
> drivers. 
> au1000_eth.c waits 20 ms (20 * 1ms) in mdio_read.
> bfin_mac.c waits 500 * 1us in mdio_poll. 
> In both functions the last delay before the timeout is useless, like in 
> new_emac. Not nice, but timeouts shouldn't occur anyway.
> 
> new emac probably doesn't wait long enough, but 20ms seems to be a bit too 
> long.

I just send Jeff a patch doubling the timeouts.

Cheers,
Ben.