| Message ID | 4C09FF4B.4050601@iki.fi |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Timo Teräs <timo.teras@iki.fi> Date: Sat, 05 Jun 2010 10:39:55 +0300 > Ok, I compared Realtek's and the in-tree driver. The only essential > difference is that Realtek driver uses udelay(100) in mdio_write()'s > busy polling loop where as the in-tree uses udelay(25). And that seems > to be the magic difference! Using udelay(100) fixes this! > > I'm guessing that the phy needs slight delay between consecutive > mdio_write's even if it has advertised that the write has been > completed. And yes, just adding a small delay in the end of mdio_write > does seem to work too. > > Francois, you think the below patch is ok? Should I send it as properly > formatted commit? Excellent detective work, Francois please review! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le samedi 05 juin 2010 à 10:39 +0300, Timo Teräs a écrit : > Ok, I compared Realtek's and the in-tree driver. The only essential > difference is that Realtek driver uses udelay(100) in mdio_write()'s > busy polling loop where as the in-tree uses udelay(25). And that seems > to be the magic difference! Using udelay(100) fixes this! > > I'm guessing that the phy needs slight delay between consecutive > mdio_write's even if it has advertised that the write has been > completed. And yes, just adding a small delay in the end of mdio_write > does seem to work too. > > Francois, you think the below patch is ok? Should I send it as properly > formatted commit? > > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c > index 217e709..6db62bf 100644 > --- a/drivers/net/r8169.c > +++ b/drivers/net/r8169.c > @@ -559,6 +559,7 @@ static void mdio_write(void __iomem *ioaddr, > break; > udelay(25); > } > + udelay(25); > } > > static int mdio_read(void __iomem *ioaddr, int reg_addr) > -- Sure this deserves an official patch with all prereqs, but please add a comment in mdio_write() why this extra udelay(25) is needed, especially since you say of udelay(100) 'fixes the bug'. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2010 12:13 PM, Eric Dumazet wrote: > Le samedi 05 juin 2010 à 10:39 +0300, Timo Teräs a écrit : >> Ok, I compared Realtek's and the in-tree driver. The only essential >> difference is that Realtek driver uses udelay(100) in mdio_write()'s >> busy polling loop where as the in-tree uses udelay(25). And that seems >> to be the magic difference! Using udelay(100) fixes this! >> >> I'm guessing that the phy needs slight delay between consecutive >> mdio_write's even if it has advertised that the write has been >> completed. And yes, just adding a small delay in the end of mdio_write >> does seem to work too. >> >> Francois, you think the below patch is ok? Should I send it as properly >> formatted commit? >> >> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c >> index 217e709..6db62bf 100644 >> --- a/drivers/net/r8169.c >> +++ b/drivers/net/r8169.c >> @@ -559,6 +559,7 @@ static void mdio_write(void __iomem *ioaddr, >> break; >> udelay(25); >> } >> + udelay(25); >> } >> >> static int mdio_read(void __iomem *ioaddr, int reg_addr) >> -- > > Sure this deserves an official patch with all prereqs, but please add a > comment in mdio_write() why this extra udelay(25) is needed, especially > since you say of udelay(100) 'fixes the bug'. Uh, yes. The intention was to get feedback if this is the proper place for the delay. I first thought that maybe we could just add the delay to the phy config function writing the values in tight loop... but as it appears even some other mdio_writes seem to fail, this seems to be the logical and correct place. Alternatively, I was thinking if someone had specs to check if they say anything about delay needed between mdio writes. Reason why adding new delay in the end of the function is better is because using udelay(100) in the loop means that anything between 0..100us has elapsed after the "write complete" is acked; if very unlucky the "write complete" happens just after our udelay has ended and there would be no delay between next mdio write. Adding the additional udelay(25) in the end guarantees small delay between "write complete" ack and the next write. And yes, it needs a comment. I'll send a new patch shortly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
difference is that Realtek driver uses udelay(100) in mdio_write()'s busy polling loop where as the in-tree uses udelay(25). And that seems to be the magic difference! Using udelay(100) fixes this! I'm guessing that the phy needs slight delay between consecutive mdio_write's even if it has advertised that the write has been completed. And yes, just adding a small delay in the end of mdio_write does seem to work too. Francois, you think the below patch is ok? Should I send it as properly formatted commit? diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index 217e709..6db62bf 100644 --- a/drivers/net/r8169.c +++ b/drivers/net/r8169.c @@ -559,6 +559,7 @@ static void mdio_write(void __iomem *ioaddr, break; udelay(25); } + udelay(25); } static int mdio_read(void __iomem *ioaddr, int reg_addr)