| Message ID | 1275733273-28321-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Timo Teräs <timo.teras@iki.fi> : [...] > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c > index 217e709..03a8318 100644 > --- a/drivers/net/r8169.c > +++ b/drivers/net/r8169.c > @@ -559,6 +559,11 @@ static void mdio_write(void __iomem *ioaddr, int reg_addr, int value) > break; > udelay(25); > } > + /* > + * Some configurations require a small delay even after the write > + * completed indication or the next write might fail. > + */ > + udelay(25); Acked-off-by: Francois Romieu <romieu@fr.zoreil.com> Good work. I wonder if increasing the in-loop delay as well would help the write succeed faster (or slower ?).
From: Francois Romieu <romieu@fr.zoreil.com> Date: Sat, 5 Jun 2010 14:41:03 +0200 > Timo Teräs <timo.teras@iki.fi> : > [...] >> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c >> index 217e709..03a8318 100644 >> --- a/drivers/net/r8169.c >> +++ b/drivers/net/r8169.c >> @@ -559,6 +559,11 @@ static void mdio_write(void __iomem *ioaddr, int reg_addr, int value) >> break; >> udelay(25); >> } >> + /* >> + * Some configurations require a small delay even after the write >> + * completed indication or the next write might fail. >> + */ >> + udelay(25); > > Acked-off-by: Francois Romieu <romieu@fr.zoreil.com> Applied, thanks everyone. -- 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
Our hardware engineer suggests that check the completed indication per 100 micro seconds. And it needs 20 micro seconds delay after the completed indication for the next command. Best Regards, Hayes -----Original Message----- From: Francois Romieu [mailto:romieu@fr.zoreil.com] Sent: Saturday, June 05, 2010 8:41 PM To: Timo Teräs Cc: netdev@vger.kernel.org; Edward Hsu; Hayeswang; davem@davemloft.net Subject: Re: [PATCH] r8169: fix random mdio_write failures Timo Teräs <timo.teras@iki.fi> : [...] > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index > 217e709..03a8318 100644 > --- a/drivers/net/r8169.c > +++ b/drivers/net/r8169.c > @@ -559,6 +559,11 @@ static void mdio_write(void __iomem *ioaddr, int reg_addr, int value) > break; > udelay(25); > } > + /* > + * Some configurations require a small delay even after the write > + * completed indication or the next write might fail. > + */ > + udelay(25); Acked-off-by: Francois Romieu <romieu@fr.zoreil.com> Good work. I wonder if increasing the in-loop delay as well would help the write succeed faster (or slower ?). -- Ueimor ------Please consider the environment before printing this e-mail. -- 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
hayeswang <hayeswang@realtek.com> : > Our hardware engineer suggests that check the completed indication > per 100 micro seconds. And it needs 20 micro seconds delay after the > completed indication for the next command. Should we do the same for mdio_read as well (100 us per iteration + an extra 20 us) ?
On 06/08/2010 12:51 AM, Francois Romieu wrote: > hayeswang <hayeswang@realtek.com> : >> Our hardware engineer suggests that check the completed indication >> per 100 micro seconds. And it needs 20 micro seconds delay after the >> completed indication for the next command. > > Should we do the same for mdio_read as well (100 us per iteration + > an extra 20 us) ? Well, doing 100us per iteration will increase the latency that the code notices "write complete" which slows down things. It'll also slightly decrease bus traffic which is good. But I'd be just fine with 25us per iteration. It sounds unlikely that polling the status register would slow down the actual write operation (if that is the case then 100us would be desirable). Changing my 25us to 20us would good. The original 25us was just a guess. The comment should be probably also updated that those delays are from realtek hw specs then. Would you like me to send a patch? - Timo -- 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
Timo Teräs <timo.teras@iki.fi> : [ok] > iteration. It sounds unlikely that polling the status register would > slow down the actual write operation (if that is the case then 100us > would be desirable). I would not be that surprized. > Changing my 25us to 20us would good. The original 25us was just a guess. > The comment should be probably also updated that those delays are from > realtek hw specs then. Yes. > Would you like me to send a patch? Of course. Some comment from Hayes regarding mdio_read would be welcome beforehand though.
I think the major point is that it needs a delay (20 us) after both read and write completed indiacation before next command. It needs almost 100 us for the indication to be completed, so our engineer suggests us to check the indication per 100 us. However, it is ok to reduce the timer from 100 us to 25 us for checking . Best Regards, Hayes -----Original Message----- From: Timo Teräs [mailto:timo.teras@gmail.com] On Behalf Of Timo Teras Sent: Tuesday, June 08, 2010 2:06 PM To: Francois Romieu Cc: Hayeswang; netdev@vger.kernel.org; davem@davemloft.net Subject: Re: [PATCH] r8169: fix random mdio_write failures On 06/08/2010 12:51 AM, Francois Romieu wrote: > hayeswang <hayeswang@realtek.com> : >> Our hardware engineer suggests that check the completed indication >> per 100 micro seconds. And it needs 20 micro seconds delay after the >> completed indication for the next command. > > Should we do the same for mdio_read as well (100 us per iteration + an > extra 20 us) ? Well, doing 100us per iteration will increase the latency that the code notices "write complete" which slows down things. It'll also slightly decrease bus traffic which is good. But I'd be just fine with 25us per iteration. It sounds unlikely that polling the status register would slow down the actual write operation (if that is the case then 100us would be desirable). Changing my 25us to 20us would good. The original 25us was just a guess. The comment should be probably also updated that those delays are from realtek hw specs then. Would you like me to send a patch? - Timo ------Please consider the environment before printing this e-mail. -- 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
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index 217e709..03a8318 100644 --- a/drivers/net/r8169.c +++ b/drivers/net/r8169.c @@ -559,6 +559,11 @@ static void mdio_write(void __iomem *ioaddr, int reg_addr, int value) break; udelay(25); } + /* + * Some configurations require a small delay even after the write + * completed indication or the next write might fail. + */ + udelay(25); } static int mdio_read(void __iomem *ioaddr, int reg_addr)
Some configurations need delay between the "write completed" indication and new write to work reliably. Realtek driver seems to use longer delay when polling the "write complete" bit, so it waits long enough between writes with high probability (but could probably break too). This patch adds a new udelay to make sure we wait unconditionally some time after the write complete indication. This caused a regression with XID 18000000 boards when the board specific phy configuration writing many mdio registers was added in commit 2e955856ff (r8169: phy init for the 8169scd). Some of the configration mdio writes would almost always fail, and depending on failure might leave the PHY in non-working state. Signed-off-by: Timo Teräs <timo.teras@iki.fi> Cc: françois romieu <romieu@fr.zoreil.com> Cc: Edward Hsu <edward_hsu@realtek.com.tw> --- drivers/net/r8169.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)