From patchwork Tue Jul 13 16:20:57 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Gardner X-Patchwork-Id: 58802 X-Patchwork-Delegate: stefan.bader@canonical.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from chlorine.canonical.com (chlorine.canonical.com [91.189.94.204]) by ozlabs.org (Postfix) with ESMTP id 7D33A1007D1 for ; Wed, 14 Jul 2010 02:21:16 +1000 (EST) Received: from localhost ([127.0.0.1] helo=chlorine.canonical.com) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1OYiEF-0006OU-VO; Tue, 13 Jul 2010 17:21:08 +0100 Received: from mail.tpi.com ([70.99.223.143]) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1OYiEB-0006NC-0z for kernel-team@lists.ubuntu.com; Tue, 13 Jul 2010 17:21:05 +0100 Received: from [10.0.2.5] (unknown [10.0.2.5]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mail.tpi.com (Postfix) with ESMTP id B9C1D2564EC; Tue, 13 Jul 2010 09:20:14 -0700 (PDT) Message-ID: <4C3C9269.3040700@canonical.com> Date: Tue, 13 Jul 2010 10:20:57 -0600 From: Tim Gardner User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100528 Thunderbird/3.0.5 MIME-Version: 1.0 To: Bryan Wu Subject: Re: [PATCH 2/2] fec: use interrupt for MDIO completion indication References: <1279034433-7791-1-git-send-email-bryan.wu@canonical.com> <1279034433-7791-3-git-send-email-bryan.wu@canonical.com> In-Reply-To: <1279034433-7791-3-git-send-email-bryan.wu@canonical.com> Cc: kernel-team@lists.ubuntu.com X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.9 Precedence: list Reply-To: tim.gardner@canonical.com List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kernel-team-bounces@lists.ubuntu.com Errors-To: kernel-team-bounces@lists.ubuntu.com On 07/13/2010 09:20 AM, Bryan Wu wrote: > From: 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. > > BugLink: http://bugs.launchpad.net/bugs/546649 > BugLink: http://bugs.launchpad.net/bugs/457878 > > Signed-off-by: Baruch Siach > Signed-off-by: David S. Miller > Signed-off-by: Bryan Wu > --- > 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 rtg 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 |