[PATCH] fec: use interrupt for MDIO completion indication

[PATCH] fec: use interrupt for MDIO completion indication

[Baruch Siach]

With the move to phylib (commit e6b043d) I was seeing sporadic "MDIO write
timeout" messages. Measure of the actual time spent showed latency times of
more than 1600us.

This patch uses the MII event indication of the FEC hardware to detect
completion of MDIO transactions.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/fec.c |   55 ++++++++++++++++++++++++----------------------------
 1 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index edfff92..89f3393 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -187,6 +187,7 @@ struct fec_enet_private {
 	int	index;
 	int	link;
 	int	full_duplex;
+	struct	completion mdio_done;
 };
 
 static irqreturn_t fec_enet_interrupt(int irq, void * dev_id);
@@ -205,7 +206,7 @@ static void fec_stop(struct net_device *dev);
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 
-#define FEC_MII_TIMEOUT		10000
+#define FEC_MII_TIMEOUT		1000 /* us */
 
 /* Transmitter timeout */
 #define TX_TIMEOUT (2 * HZ)
@@ -334,6 +335,11 @@ fec_enet_interrupt(int irq, void * dev_id)
 			ret = IRQ_HANDLED;
 			fec_enet_tx(dev);
 		}
+
+		if (int_events & FEC_ENET_MII) {
+			ret = IRQ_HANDLED;
+			complete(&fep->mdio_done);
+		}
 	} while (int_events);
 
 	return ret;
@@ -608,18 +614,13 @@ spin_unlock:
 		phy_print_status(phy_dev);
 }
 
-/*
- * NOTE: a MII transaction is during around 25 us, so polling it...
- */
 static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct fec_enet_private *fep = bus->priv;
-	int timeout = FEC_MII_TIMEOUT;
+	unsigned long time_left;
 
 	fep->mii_timeout = 0;
-
-	/* clear MII end of transfer bit*/
-	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+	init_completion(&fep->mdio_done);
 
 	/* start a read op */
 	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
@@ -627,13 +628,12 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
 
 	/* wait for end of transfer */
-	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
-		cpu_relax();
-		if (timeout-- < 0) {
-			fep->mii_timeout = 1;
-			printk(KERN_ERR "FEC: MDIO read timeout\n");
-			return -ETIMEDOUT;
-		}
+	time_left = wait_for_completion_timeout(&fep->mdio_done,
+			usecs_to_jiffies(FEC_MII_TIMEOUT));
+	if (time_left == 0) {
+		fep->mii_timeout = 1;
+		printk(KERN_ERR "FEC: MDIO read timeout\n");
+		return -ETIMEDOUT;
 	}
 
 	/* return value */
@@ -644,12 +644,10 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct fec_enet_private *fep = bus->priv;
-	int timeout = FEC_MII_TIMEOUT;
+	unsigned long time_left;
 
 	fep->mii_timeout = 0;
-
-	/* clear MII end of transfer bit*/
-	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+	init_completion(&fep->mdio_done);
 
 	/* start a read op */
 	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
@@ -658,13 +656,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 		fep->hwp + FEC_MII_DATA);
 
 	/* wait for end of transfer */
-	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
-		cpu_relax();
-		if (timeout-- < 0) {
-			fep->mii_timeout = 1;
-			printk(KERN_ERR "FEC: MDIO write timeout\n");
-			return -ETIMEDOUT;
-		}
+	time_left = wait_for_completion_timeout(&fep->mdio_done,
+			usecs_to_jiffies(FEC_MII_TIMEOUT));
+	if (time_left == 0) {
+		fep->mii_timeout = 1;
+		printk(KERN_ERR "FEC: MDIO write timeout\n");
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -1222,7 +1219,8 @@ fec_restart(struct net_device *dev, int duplex)
 	writel(0, fep->hwp + FEC_R_DES_ACTIVE);
 
 	/* Enable interrupts we wish to service */
-	writel(FEC_ENET_TXF | FEC_ENET_RXF, fep->hwp + FEC_IMASK);
+	writel(FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII,
+			fep->hwp + FEC_IMASK);
 }
 
 static void
@@ -1242,9 +1240,6 @@ fec_stop(struct net_device *dev)
 	writel(1, fep->hwp + FEC_ECNTRL);
 	udelay(10);
 
-	/* Clear outstanding MII command interrupts. */
-	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
-
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 }
 
-- 
1.7.1

Re: [PATCH] fec: use interrupt for MDIO completion indication

[David Miller]

From: Baruch Siach <baruch@tkos.co.il>
Date: Mon, 12 Jul 2010 10:12:51 +0300

> With the move to phylib (commit e6b043d) I was seeing sporadic "MDIO write
> timeout" messages. Measure of the actual time spent showed latency times of
> more than 1600us.
> 
> This patch uses the MII event indication of the FEC hardware to detect
> completion of MDIO transactions.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Applied.

Re: [PATCH] fec: use interrupt for MDIO completion indication

[Bryan Wu]

Baruch,

Thanks for this patch, we tested on our i.MX51 board with Ubuntu. It works fine.

Wolfram, you can pick up this, too. -;)

-Bryan

On 07/12/2010 03:12 PM, Baruch Siach wrote:
> With the move to phylib (commit e6b043d) I was seeing sporadic "MDIO write
> timeout" messages. Measure of the actual time spent showed latency times of
> more than 1600us.
> 
> This patch uses the MII event indication of the FEC hardware to detect
> completion of MDIO transactions.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/fec.c |   55 ++++++++++++++++++++++++----------------------------
>  1 files changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index edfff92..89f3393 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -187,6 +187,7 @@ struct fec_enet_private {
>  	int	index;
>  	int	link;
>  	int	full_duplex;
> +	struct	completion mdio_done;
>  };
>  
>  static irqreturn_t fec_enet_interrupt(int irq, void * dev_id);
> @@ -205,7 +206,7 @@ static void fec_stop(struct net_device *dev);
>  #define FEC_MMFR_TA		(2 << 16)
>  #define FEC_MMFR_DATA(v)	(v & 0xffff)
>  
> -#define FEC_MII_TIMEOUT		10000
> +#define FEC_MII_TIMEOUT		1000 /* us */
>  
>  /* Transmitter timeout */
>  #define TX_TIMEOUT (2 * HZ)
> @@ -334,6 +335,11 @@ fec_enet_interrupt(int irq, void * dev_id)
>  			ret = IRQ_HANDLED;
>  			fec_enet_tx(dev);
>  		}
> +
> +		if (int_events & FEC_ENET_MII) {
> +			ret = IRQ_HANDLED;
> +			complete(&fep->mdio_done);
> +		}
>  	} while (int_events);
>  
>  	return ret;
> @@ -608,18 +614,13 @@ spin_unlock:
>  		phy_print_status(phy_dev);
>  }
>  
> -/*
> - * NOTE: a MII transaction is during around 25 us, so polling it...
> - */
>  static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct fec_enet_private *fep = bus->priv;
> -	int timeout = FEC_MII_TIMEOUT;
> +	unsigned long time_left;
>  
>  	fep->mii_timeout = 0;
> -
> -	/* clear MII end of transfer bit*/
> -	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +	init_completion(&fep->mdio_done);
>  
>  	/* start a read op */
>  	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
> @@ -627,13 +628,12 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>  
>  	/* wait for end of transfer */
> -	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -		cpu_relax();
> -		if (timeout-- < 0) {
> -			fep->mii_timeout = 1;
> -			printk(KERN_ERR "FEC: MDIO read timeout\n");
> -			return -ETIMEDOUT;
> -		}
> +	time_left = wait_for_completion_timeout(&fep->mdio_done,
> +			usecs_to_jiffies(FEC_MII_TIMEOUT));
> +	if (time_left == 0) {
> +		fep->mii_timeout = 1;
> +		printk(KERN_ERR "FEC: MDIO read timeout\n");
> +		return -ETIMEDOUT;
>  	}
>  
>  	/* return value */
> @@ -644,12 +644,10 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			   u16 value)
>  {
>  	struct fec_enet_private *fep = bus->priv;
> -	int timeout = FEC_MII_TIMEOUT;
> +	unsigned long time_left;
>  
>  	fep->mii_timeout = 0;
> -
> -	/* clear MII end of transfer bit*/
> -	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +	init_completion(&fep->mdio_done);
>  
>  	/* start a read op */
>  	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
> @@ -658,13 +656,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  		fep->hwp + FEC_MII_DATA);
>  
>  	/* wait for end of transfer */
> -	while (!(readl(fep->hwp + FEC_IEVENT) & FEC_ENET_MII)) {
> -		cpu_relax();
> -		if (timeout-- < 0) {
> -			fep->mii_timeout = 1;
> -			printk(KERN_ERR "FEC: MDIO write timeout\n");
> -			return -ETIMEDOUT;
> -		}
> +	time_left = wait_for_completion_timeout(&fep->mdio_done,
> +			usecs_to_jiffies(FEC_MII_TIMEOUT));
> +	if (time_left == 0) {
> +		fep->mii_timeout = 1;
> +		printk(KERN_ERR "FEC: MDIO write timeout\n");
> +		return -ETIMEDOUT;
>  	}
>  
>  	return 0;
> @@ -1222,7 +1219,8 @@ fec_restart(struct net_device *dev, int duplex)
>  	writel(0, fep->hwp + FEC_R_DES_ACTIVE);
>  
>  	/* Enable interrupts we wish to service */
> -	writel(FEC_ENET_TXF | FEC_ENET_RXF, fep->hwp + FEC_IMASK);
> +	writel(FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII,
> +			fep->hwp + FEC_IMASK);
>  }
>  
>  static void
> @@ -1242,9 +1240,6 @@ fec_stop(struct net_device *dev)
>  	writel(1, fep->hwp + FEC_ECNTRL);
>  	udelay(10);
>  
> -	/* Clear outstanding MII command interrupts. */
> -	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> -
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>  }
>  

Re: [PATCH] fec: use interrupt for MDIO completion indication

[Baruch Siach]

Hi Bryan,

On Thu, Jul 15, 2010 at 11:31:56AM +0800, Bryan Wu wrote:
> Thanks for this patch, we tested on our i.MX51 board with Ubuntu. It works 
> fine.
> 
> Wolfram, you can pick up this, too. -;)

Dave has already applied this patch to his net-next tree.

baruch

> On 07/12/2010 03:12 PM, Baruch Siach wrote:
> > With the move to phylib (commit e6b043d) I was seeing sporadic "MDIO write
> > timeout" messages. Measure of the actual time spent showed latency times of
> > more than 1600us.
> > 
> > This patch uses the MII event indication of the FEC hardware to detect
> > completion of MDIO transactions.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  drivers/net/fec.c |   55 ++++++++++++++++++++++++----------------------------
> >  1 files changed, 25 insertions(+), 30 deletions(-)
> > 

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] fec: use interrupt for MDIO completion indication

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]


> > Thanks for this patch, we tested on our i.MX51 board with Ubuntu. It works 
> > fine.
> > 
> > Wolfram, you can pick up this, too. -;)
> 
> Dave has already applied this patch to his net-next tree.

Bryan, thanks for letting me know, I missed this one.

However, have you guys ever tried pulling the cable off/on or restarting
the interface with 'ifconfig down/up'? This always caused a stalled PHY
for me. This patch helps:

==========================

From: Wolfram Sang <w.sang@pengutronix.de>
Subject: [PATCH] net/fec: restore interrupt mask after software-reset in fec_stop()

After the change from mdio polling to irq, it became necessary to
restore the interrupt mask after resetting the chip in fec_stop().
Otherwise, with all irqs disabled, no communication with the PHY will be
possible after e.g. un-/replugging the cable and the device gets
stalled.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/net/fec.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 391a553..768b840 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -118,6 +118,8 @@ static unsigned char	fec_mac_default[] = {
 #define FEC_ENET_MII	((uint)0x00800000)	/* MII interrupt */
 #define FEC_ENET_EBERR	((uint)0x00400000)	/* SDMA bus error */
 
+#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
+
 /* The FEC stores dest/src/type, data, and checksum for receive packets.
  */
 #define PKT_MAXBUF_SIZE		1518
@@ -1213,8 +1215,7 @@ fec_restart(struct net_device *dev, int duplex)
 	writel(0, fep->hwp + FEC_R_DES_ACTIVE);
 
 	/* Enable interrupts we wish to service */
-	writel(FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII,
-			fep->hwp + FEC_IMASK);
+	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 }
 
 static void
@@ -1233,8 +1234,8 @@ fec_stop(struct net_device *dev)
 	/* Whack a reset.  We should wait for this. */
 	writel(1, fep->hwp + FEC_ECNTRL);
 	udelay(10);
-
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
+	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 }
 
 static int __devinit
-- 
1.7.1

==========================

BUT, while it helps and may possibly be a quick fix for 2.6.35,
resetting the chip in fec_stop() looks like a wrong thing to do for me.
In the long run, it probably is better to make sure the chip is set up
correctly during initialization, so the reset in fec_stop() is not
needed at all. I had a quick shot at this, but seem to have missed
something as it didn't work. As I will be away from the computers for
two weeks in about 24 hours, I at least wanted to bring up the issue.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] fec: use interrupt for MDIO completion indication

[Baruch Siach]

Hi Wolfram, Bryan,

On Wed, Jul 21, 2010 at 02:51:13PM +0200, Wolfram Sang wrote:
> > > Thanks for this patch, we tested on our i.MX51 board with Ubuntu. It 
> > > works fine.
> > > 
> > > Wolfram, you can pick up this, too. -;)
> > 
> > Dave has already applied this patch to his net-next tree.
> 
> Bryan, thanks for letting me know, I missed this one.
> 
> However, have you guys ever tried pulling the cable off/on or restarting
> the interface with 'ifconfig down/up'? This always caused a stalled PHY
> for me. This patch helps:

I can confirm the problem and the fix on a i.MX25 based system. Thanks 
Wolfram.

baruch

> From: Wolfram Sang <w.sang@pengutronix.de>
> Subject: [PATCH] net/fec: restore interrupt mask after software-reset in fec_stop()
> 
> After the change from mdio polling to irq, it became necessary to
> restore the interrupt mask after resetting the chip in fec_stop().
> Otherwise, with all irqs disabled, no communication with the PHY will be
> possible after e.g. un-/replugging the cable and the device gets
> stalled.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/net/fec.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 391a553..768b840 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -118,6 +118,8 @@ static unsigned char	fec_mac_default[] = {
>  #define FEC_ENET_MII	((uint)0x00800000)	/* MII interrupt */
>  #define FEC_ENET_EBERR	((uint)0x00400000)	/* SDMA bus error */
>  
> +#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
> +
>  /* The FEC stores dest/src/type, data, and checksum for receive packets.
>   */
>  #define PKT_MAXBUF_SIZE		1518
> @@ -1213,8 +1215,7 @@ fec_restart(struct net_device *dev, int duplex)
>  	writel(0, fep->hwp + FEC_R_DES_ACTIVE);
>  
>  	/* Enable interrupts we wish to service */
> -	writel(FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII,
> -			fep->hwp + FEC_IMASK);
> +	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>  }
>  
>  static void
> @@ -1233,8 +1234,8 @@ fec_stop(struct net_device *dev)
>  	/* Whack a reset.  We should wait for this. */
>  	writel(1, fep->hwp + FEC_ECNTRL);
>  	udelay(10);
> -
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> +	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>  }
>  
>  static int __devinit
> -- 
> 1.7.1
> 
> ==========================
> 
> BUT, while it helps and may possibly be a quick fix for 2.6.35,
> resetting the chip in fec_stop() looks like a wrong thing to do for me.
> In the long run, it probably is better to make sure the chip is set up
> correctly during initialization, so the reset in fec_stop() is not
> needed at all. I had a quick shot at this, but seem to have missed
> something as it didn't work. As I will be away from the computers for
> two weeks in about 24 hours, I at least wanted to bring up the issue.
> 
> Regards,
> 
>    Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] fec: use interrupt for MDIO completion indication

[David Miller]

From: Wolfram Sang <w.sang@pengutronix.de>
Date: Wed, 21 Jul 2010 14:51:13 +0200

> From: Wolfram Sang <w.sang@pengutronix.de>
> Subject: [PATCH] net/fec: restore interrupt mask after software-reset in fec_stop()
> 
> After the change from mdio polling to irq, it became necessary to
> restore the interrupt mask after resetting the chip in fec_stop().
> Otherwise, with all irqs disabled, no communication with the PHY will be
> possible after e.g. un-/replugging the cable and the device gets
> stalled.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Applied.