Patchwork [2/2] fec: use interrupt for MDIO completion indication

login
register
mail settings
Submitter Tim Gardner
Date July 13, 2010, 4:20 p.m.
Message ID <4C3C9269.3040700@canonical.com>
Download mbox | patch
Permalink /patch/58802/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Tim Gardner - July 13, 2010, 4:20 p.m.
On 07/13/2010 09:20 AM, Bryan Wu wrote:
> From: Baruch Siach<baruch@tkos.co.il>
>
> 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.
>
> BugLink: http://bugs.launchpad.net/bugs/546649
> BugLink: http://bugs.launchpad.net/bugs/457878
>
> Signed-off-by: Baruch Siach<baruch@tkos.co.il>
> Signed-off-by: David S. Miller<davem@davemloft.net>
> Signed-off-by: Bryan Wu<bryan.wu@canonical.com>
> ---
>   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 5a4f611..be61573 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -198,6 +198,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);
> @@ -216,7 +217,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)
> @@ -347,6 +348,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;
> @@ -621,18 +627,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 |
> @@ -640,13 +641,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 */
> @@ -657,12 +657,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 |
> @@ -671,13 +669,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;
> @@ -1244,7 +1241,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
> @@ -1264,9 +1262,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);
>   }
>

I like this asynchronous completion much better since the delay doesn't 
hog the CPU.

There is a potential race in the interrupt handler which I think the 
attached patch closes. See what upstream thinks about it.

Acked-by: Tim Gardner <tim.gardner@canonical.com>

rtg

Patch

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index e0851a8..0604687 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -658,9 +658,12 @@  static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 {
 	struct fec_enet_private *fep = bus->priv;
 	unsigned long time_left;
+	unsigned long flags;
 
+	spin_lock_irqsave(&fep->hw_lock, flags);
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
+	spin_unlock_irqrestore(&fep->hw_lock, flags);
 
 	/* start a read op */
 	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |