[PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

Measurements of the MDIO bus have shown that driving the MDIO bus
using interrupts is slow. Back to back MDIO transactions take about
90uS, with 25uS spent performing the transaction, and the remainder of
the time the bus is idle.

Replacing the completion interrupt with polled IO results in back to
back transactions of 40uS. The polling loop waiting for the hardware
to complete the transaction takes around 27uS. Which suggests
interrupt handling has an overhead of 50uS, and polled IO nearly
halves this overhead, and doubles the MDIO performance.

Suggested-by: Chris Heally <cphealy@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/freescale/fec.h      |  4 +-
 drivers/net/ethernet/freescale/fec_main.c | 69 ++++++++++++-----------
 2 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bd898f5b4da5..4c8e7f57957a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -376,8 +376,7 @@ struct bufdesc_ex {
 #define FEC_ENET_TS_AVAIL       ((uint)0x00010000)
 #define FEC_ENET_TS_TIMER       ((uint)0x00008000)
 
-#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
-#define FEC_NAPI_IMASK	FEC_ENET_MII
+#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF)
 #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
 /* ENET interrupt coalescing macro define */
@@ -537,7 +536,6 @@ struct fec_enet_private {
 	int	link;
 	int	full_duplex;
 	int	speed;
-	struct	completion mdio_done;
 	int	irq[FEC_IRQ_NUM];
 	bool	bufdesc_ex;
 	int	pause_flag;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c1c267b61647..48ac0a3a4eb0 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -938,8 +938,8 @@ fec_restart(struct net_device *ndev)
 	writel((__force u32)cpu_to_be32(temp_mac[1]),
 	       fep->hwp + FEC_ADDR_HIGH);
 
-	/* Clear any outstanding interrupt. */
-	writel(0xffffffff, fep->hwp + FEC_IEVENT);
+	/* Clear any outstanding interrupt, except MDIO. */
+	writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
 
 	fec_enet_bd_init(ndev);
 
@@ -1085,7 +1085,7 @@ fec_restart(struct net_device *ndev)
 	if (fep->link)
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 	else
-		writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
+		writel(0, fep->hwp + FEC_IMASK);
 
 	/* Init the interrupt coalescing */
 	fec_enet_itr_coal_init(ndev);
@@ -1599,6 +1599,10 @@ fec_enet_interrupt(int irq, void *dev_id)
 	irqreturn_t ret = IRQ_NONE;
 
 	int_events = readl(fep->hwp + FEC_IEVENT);
+
+	/* Don't clear MDIO events, we poll for those */
+	int_events &= ~FEC_ENET_MII;
+
 	writel(int_events, fep->hwp + FEC_IEVENT);
 	fec_enet_collect_events(fep, int_events);
 
@@ -1606,16 +1610,12 @@ fec_enet_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 
 		if (napi_schedule_prep(&fep->napi)) {
-			/* Disable the NAPI interrupts */
-			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
+			/* Disable interrupts */
+			writel(0, fep->hwp + FEC_IMASK);
 			__napi_schedule(&fep->napi);
 		}
 	}
 
-	if (int_events & FEC_ENET_MII) {
-		ret = IRQ_HANDLED;
-		complete(&fep->mdio_done);
-	}
 	return ret;
 }
 
@@ -1765,11 +1765,26 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 		phy_print_status(phy_dev);
 }
 
+static int fec_enet_mdio_wait(struct fec_enet_private *fep)
+{
+	int retries = 10000;
+	uint ievent;
+
+	while (retries--) {
+		ievent = readl(fep->hwp + FEC_IEVENT);
+		if (ievent & FEC_ENET_MII) {
+			writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+			return 0;
+		}
+	}
+
+	return -ETIMEDOUT;
+}
+
 static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct fec_enet_private *fep = bus->priv;
 	struct device *dev = &fep->pdev->dev;
-	unsigned long time_left;
 	int ret = 0, frame_start, frame_addr, frame_op;
 	bool is_c45 = !!(regnum & MII_ADDR_C45);
 
@@ -1777,8 +1792,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret < 0)
 		return ret;
 
-	reinit_completion(&fep->mdio_done);
-
 	if (is_c45) {
 		frame_start = FEC_MMFR_ST_C45;
 
@@ -1790,11 +1803,9 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		       fep->hwp + FEC_MII_DATA);
 
 		/* wait for end of transfer */
-		time_left = wait_for_completion_timeout(&fep->mdio_done,
-				usecs_to_jiffies(FEC_MII_TIMEOUT));
-		if (time_left == 0) {
+		ret = fec_enet_mdio_wait(fep);
+		if (ret) {
 			netdev_err(fep->netdev, "MDIO address write timeout\n");
-			ret = -ETIMEDOUT;
 			goto out;
 		}
 
@@ -1813,11 +1824,9 @@ 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 */
-	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-	if (time_left == 0) {
+	ret = fec_enet_mdio_wait(fep);
+	if (ret) {
 		netdev_err(fep->netdev, "MDIO read timeout\n");
-		ret = -ETIMEDOUT;
 		goto out;
 	}
 
@@ -1835,7 +1844,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 {
 	struct fec_enet_private *fep = bus->priv;
 	struct device *dev = &fep->pdev->dev;
-	unsigned long time_left;
 	int ret, frame_start, frame_addr;
 	bool is_c45 = !!(regnum & MII_ADDR_C45);
 
@@ -1845,8 +1853,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	else
 		ret = 0;
 
-	reinit_completion(&fep->mdio_done);
-
 	if (is_c45) {
 		frame_start = FEC_MMFR_ST_C45;
 
@@ -1858,11 +1864,9 @@ 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 */
-		time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-		if (time_left == 0) {
+		ret = fec_enet_mdio_wait(fep);
+		if (ret) {
 			netdev_err(fep->netdev, "MDIO address write timeout\n");
-			ret = -ETIMEDOUT;
 			goto out;
 		}
 	} else {
@@ -1878,12 +1882,9 @@ 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 */
-	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-	if (time_left == 0) {
+	ret = fec_enet_mdio_wait(fep);
+	if (ret)
 		netdev_err(fep->netdev, "MDIO write timeout\n");
-		ret  = -ETIMEDOUT;
-	}
 
 out:
 	pm_runtime_mark_last_busy(dev);
@@ -2079,6 +2080,9 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/* Clear any pending transaction complete indication */
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+
 	fep->mii_bus = mdiobus_alloc();
 	if (fep->mii_bus == NULL) {
 		err = -ENOMEM;
@@ -3583,7 +3587,6 @@ fec_probe(struct platform_device *pdev)
 		fep->irq[i] = irq;
 	}
 
-	init_completion(&fep->mdio_done);
 	ret = fec_enet_mii_init(pdev);
 	if (ret)
 		goto failed_mii_init;
-- 
2.26.0

RE: [EXT] [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 14, 2020 8:46 AM
> Measurements of the MDIO bus have shown that driving the MDIO bus using
> interrupts is slow. Back to back MDIO transactions take about 90uS, with
> 25uS spent performing the transaction, and the remainder of the time the bus
> is idle.
> 
> Replacing the completion interrupt with polled IO results in back to back
> transactions of 40uS. The polling loop waiting for the hardware to complete
> the transaction takes around 27uS. Which suggests interrupt handling has an
> overhead of 50uS, and polled IO nearly halves this overhead, and doubles the
> MDIO performance.
> 

Although the mdio performance is better, but polling IO by reading register
cause system/bus loading more heavy.

So I don't think it is valuable to change interrupt mode to polling mode.

Thanks,
Andy
> Suggested-by: Chris Heally <cphealy@gmail.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  4 +-
>  drivers/net/ethernet/freescale/fec_main.c | 69 ++++++++++++-----------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index bd898f5b4da5..4c8e7f57957a 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -376,8 +376,7 @@ struct bufdesc_ex {
>  #define FEC_ENET_TS_AVAIL       ((uint)0x00010000)
>  #define FEC_ENET_TS_TIMER       ((uint)0x00008000)
> 
> -#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF |
> FEC_ENET_MII) -#define FEC_NAPI_IMASK FEC_ENET_MII
> +#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF)
>  #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK &
> (~FEC_ENET_RXF))
> 
>  /* ENET interrupt coalescing macro define */ @@ -537,7 +536,6 @@ struct
> fec_enet_private {
>         int     link;
>         int     full_duplex;
>         int     speed;
> -       struct  completion mdio_done;
>         int     irq[FEC_IRQ_NUM];
>         bool    bufdesc_ex;
>         int     pause_flag;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c1c267b61647..48ac0a3a4eb0 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -938,8 +938,8 @@ fec_restart(struct net_device *ndev)
>         writel((__force u32)cpu_to_be32(temp_mac[1]),
>                fep->hwp + FEC_ADDR_HIGH);
> 
> -       /* Clear any outstanding interrupt. */
> -       writel(0xffffffff, fep->hwp + FEC_IEVENT);
> +       /* Clear any outstanding interrupt, except MDIO. */
> +       writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
> 
>         fec_enet_bd_init(ndev);
> 
> @@ -1085,7 +1085,7 @@ fec_restart(struct net_device *ndev)
>         if (fep->link)
>                 writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>         else
> -               writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
> +               writel(0, fep->hwp + FEC_IMASK);
> 
>         /* Init the interrupt coalescing */
>         fec_enet_itr_coal_init(ndev);
> @@ -1599,6 +1599,10 @@ fec_enet_interrupt(int irq, void *dev_id)
>         irqreturn_t ret = IRQ_NONE;
> 
>         int_events = readl(fep->hwp + FEC_IEVENT);
> +
> +       /* Don't clear MDIO events, we poll for those */
> +       int_events &= ~FEC_ENET_MII;
> +
>         writel(int_events, fep->hwp + FEC_IEVENT);
>         fec_enet_collect_events(fep, int_events);
> 
> @@ -1606,16 +1610,12 @@ fec_enet_interrupt(int irq, void *dev_id)
>                 ret = IRQ_HANDLED;
> 
>                 if (napi_schedule_prep(&fep->napi)) {
> -                       /* Disable the NAPI interrupts */
> -                       writel(FEC_NAPI_IMASK, fep->hwp +
> FEC_IMASK);
> +                       /* Disable interrupts */
> +                       writel(0, fep->hwp + FEC_IMASK);
>                         __napi_schedule(&fep->napi);
>                 }
>         }
> 
> -       if (int_events & FEC_ENET_MII) {
> -               ret = IRQ_HANDLED;
> -               complete(&fep->mdio_done);
> -       }
>         return ret;
>  }
> 
> @@ -1765,11 +1765,26 @@ static void fec_enet_adjust_link(struct
> net_device *ndev)
>                 phy_print_status(phy_dev);  }
> 
> +static int fec_enet_mdio_wait(struct fec_enet_private *fep) {
> +       int retries = 10000;
> +       uint ievent;
> +
> +       while (retries--) {
> +               ievent = readl(fep->hwp + FEC_IEVENT);
> +               if (ievent & FEC_ENET_MII) {
> +                       writel(FEC_ENET_MII, fep->hwp +
> FEC_IEVENT);
> +                       return 0;
> +               }
> +       }
> +
> +       return -ETIMEDOUT;
> +}
> +
>  static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)  {
>         struct fec_enet_private *fep = bus->priv;
>         struct device *dev = &fep->pdev->dev;
> -       unsigned long time_left;
>         int ret = 0, frame_start, frame_addr, frame_op;
>         bool is_c45 = !!(regnum & MII_ADDR_C45);
> 
> @@ -1777,8 +1792,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus,
> int mii_id, int regnum)
>         if (ret < 0)
>                 return ret;
> 
> -       reinit_completion(&fep->mdio_done);
> -
>         if (is_c45) {
>                 frame_start = FEC_MMFR_ST_C45;
> 
> @@ -1790,11 +1803,9 @@ static int fec_enet_mdio_read(struct mii_bus
> *bus, int mii_id, int regnum)
>                        fep->hwp + FEC_MII_DATA);
> 
>                 /* wait for end of transfer */
> -               time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> -                               usecs_to_jiffies(FEC_MII_TIMEOUT));
> -               if (time_left == 0) {
> +               ret = fec_enet_mdio_wait(fep);
> +               if (ret) {
>                         netdev_err(fep->netdev, "MDIO address write
> timeout\n");
> -                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
> 
> @@ -1813,11 +1824,9 @@ 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 */
> -       time_left = wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -       if (time_left == 0) {
> +       ret = fec_enet_mdio_wait(fep);
> +       if (ret) {
>                 netdev_err(fep->netdev, "MDIO read timeout\n");
> -               ret = -ETIMEDOUT;
>                 goto out;
>         }
> 
> @@ -1835,7 +1844,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,  {
>         struct fec_enet_private *fep = bus->priv;
>         struct device *dev = &fep->pdev->dev;
> -       unsigned long time_left;
>         int ret, frame_start, frame_addr;
>         bool is_c45 = !!(regnum & MII_ADDR_C45);
> 
> @@ -1845,8 +1853,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
> int mii_id, int regnum,
>         else
>                 ret = 0;
> 
> -       reinit_completion(&fep->mdio_done);
> -
>         if (is_c45) {
>                 frame_start = FEC_MMFR_ST_C45;
> 
> @@ -1858,11 +1864,9 @@ 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 */
> -               time_left =
> wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -               if (time_left == 0) {
> +               ret = fec_enet_mdio_wait(fep);
> +               if (ret) {
>                         netdev_err(fep->netdev, "MDIO address write
> timeout\n");
> -                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
>         } else {
> @@ -1878,12 +1882,9 @@ 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 */
> -       time_left = wait_for_completion_timeout(&fep->mdio_done,
> -                       usecs_to_jiffies(FEC_MII_TIMEOUT));
> -       if (time_left == 0) {
> +       ret = fec_enet_mdio_wait(fep);
> +       if (ret)
>                 netdev_err(fep->netdev, "MDIO write timeout\n");
> -               ret  = -ETIMEDOUT;
> -       }
> 
>  out:
>         pm_runtime_mark_last_busy(dev);
> @@ -2079,6 +2080,9 @@ static int fec_enet_mii_init(struct platform_device
> *pdev)
> 
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> +       /* Clear any pending transaction complete indication */
> +       writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +
>         fep->mii_bus = mdiobus_alloc();
>         if (fep->mii_bus == NULL) {
>                 err = -ENOMEM;
> @@ -3583,7 +3587,6 @@ fec_probe(struct platform_device *pdev)
>                 fep->irq[i] = irq;
>         }
> 
> -       init_completion(&fep->mdio_done);
>         ret = fec_enet_mii_init(pdev);
>         if (ret)
>                 goto failed_mii_init;
> --
> 2.26.0

Re: [EXT] [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Tue, Apr 14, 2020 at 03:07:09AM +0000, Andy Duan wrote:
> From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 14, 2020 8:46 AM
> > Measurements of the MDIO bus have shown that driving the MDIO bus using
> > interrupts is slow. Back to back MDIO transactions take about 90uS, with
> > 25uS spent performing the transaction, and the remainder of the time the bus
> > is idle.
> > 
> > Replacing the completion interrupt with polled IO results in back to back
> > transactions of 40uS. The polling loop waiting for the hardware to complete
> > the transaction takes around 27uS. Which suggests interrupt handling has an
> > overhead of 50uS, and polled IO nearly halves this overhead, and doubles the
> > MDIO performance.
> > 
> 
> Although the mdio performance is better, but polling IO by reading register
> cause system/bus loading more heavy.

Hi Andy

I actually think is reduces the system bus load. With interrupts we
have 27uS waiting for the interrupt when the bus is idle, followed by
63uS the CPU is busy handling the interrupt and setting up the next
transfer, which will case the bus to be loaded. So the system bus is
busy for 63uS per transaction. With polled IO, yes the system bus is
busy for 27uS polling while the transaction happens, and then another
13uS setting up the next transaction. But in total, that is only 40uS.

So with interrupts we have 63uS of load per transaction, vs 40uS of
load per transaction for polled IO. Polled IO is better for the bus.

I also have follow up patches which allows the bus to be run at higher
speeds. The Ethernet switch i have on the bus is happy to run a 5MHz
rather than the default 2.5MHz. That reduces the transaction time by a
1/2. The switch will also work without the MDIO preamble, again
reducing the size of the MDIO transaction by 1/2. Combining all these,
interrupt handling becomes very expensive. You do not want to be doing
interrupts every 7uS.

     Andrew

RE: [EXT] [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 14, 2020 11:49 AM
> On Tue, Apr 14, 2020 at 03:07:09AM +0000, Andy Duan wrote:
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 14, 2020 8:46
> > AM
> > > Measurements of the MDIO bus have shown that driving the MDIO bus
> > > using interrupts is slow. Back to back MDIO transactions take about
> > > 90uS, with 25uS spent performing the transaction, and the remainder
> > > of the time the bus is idle.
> > >
> > > Replacing the completion interrupt with polled IO results in back to
> > > back transactions of 40uS. The polling loop waiting for the hardware
> > > to complete the transaction takes around 27uS. Which suggests
> > > interrupt handling has an overhead of 50uS, and polled IO nearly
> > > halves this overhead, and doubles the MDIO performance.
> > >
> >
> > Although the mdio performance is better, but polling IO by reading
> > register cause system/bus loading more heavy.
> 
> Hi Andy
> 
> I actually think is reduces the system bus load. With interrupts we have 27uS
> waiting for the interrupt when the bus is idle, followed by 63uS the CPU is
> busy handling the interrupt and setting up the next transfer, which will case
> the bus to be loaded. So the system bus is busy for 63uS per transaction. With
> polled IO, yes the system bus is busy for 27uS polling while the transaction
> happens, and then another 13uS setting up the next transaction. But in total,
> that is only 40uS.
>
We cannot calculate the bus loading like this number. As you know, interrupt handler
may cost many instructions on cacheable memory accessing, but IO register
accessing is non-cacheable, which bring heavy AIPS bus loading when a flood of
MDIO read/write operations.  But I don't deny your conclusion that system bus
is more busy in interrupt mode than polling mode. 

> So with interrupts we have 63uS of load per transaction, vs 40uS of load per
> transaction for polled IO. Polled IO is better for the bus.
If we switch to polling mode, it is better to add usleep or cpu relax between IO
Polling.

> 
> I also have follow up patches which allows the bus to be run at higher speeds.
> The Ethernet switch i have on the bus is happy to run a 5MHz rather than the
> default 2.5MHz. 

Please compatible with 2.5Hz for your following patches.

Thanks,
Andy 

Re: [EXT] [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> If we switch to polling mode, it is better to add usleep or cpu relax between IO
> Polling.

Hi Andy

Yes. I can do that. I've been wanting to try iopoll.h. I will see what
the performance impacts are.

> > I also have follow up patches which allows the bus to be run at higher speeds.
> > The Ethernet switch i have on the bus is happy to run a 5MHz rather than the
> > default 2.5MHz. 
> 
> Please compatible with 2.5Hz for your following patches.

Yes. I'm adding device tree properties. If the property is not
present, it will default to 2.5MHz. Same for preamble suppression, if
the boolean DT property is not present, it will not suppress it, so
keeping backwards compatibility.

	Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 14 Apr 2020 02:45:51 +0200

> Measurements of the MDIO bus have shown that driving the MDIO bus
> using interrupts is slow. Back to back MDIO transactions take about
> 90uS, with 25uS spent performing the transaction, and the remainder of
> the time the bus is idle.
> 
> Replacing the completion interrupt with polled IO results in back to
> back transactions of 40uS. The polling loop waiting for the hardware
> to complete the transaction takes around 27uS. Which suggests
> interrupt handling has an overhead of 50uS, and polled IO nearly
> halves this overhead, and doubles the MDIO performance.
> 
> Suggested-by: Chris Heally <cphealy@gmail.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Where are we with this?

Andrew, do you intend to submit a version that via iopoll.h does
cpu relax and usleeps?

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Tue, Apr 14, 2020 at 04:38:30PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Tue, 14 Apr 2020 02:45:51 +0200
> 
> > Measurements of the MDIO bus have shown that driving the MDIO bus
> > using interrupts is slow. Back to back MDIO transactions take about
> > 90uS, with 25uS spent performing the transaction, and the remainder of
> > the time the bus is idle.
> > 
> > Replacing the completion interrupt with polled IO results in back to
> > back transactions of 40uS. The polling loop waiting for the hardware
> > to complete the transaction takes around 27uS. Which suggests
> > interrupt handling has an overhead of 50uS, and polled IO nearly
> > halves this overhead, and doubles the MDIO performance.
> > 
> > Suggested-by: Chris Heally <cphealy@gmail.com>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Where are we with this?
> 
> Andrew, do you intend to submit a version that via iopoll.h does
> cpu relax and usleeps?

Hi David

I do want to test such a version, yes.

  Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Leonard Crestez]

Hello,

This patch breaks network boot on at least imx8mm-evk. Boot works if I 
revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt 
driven MDIO with polled IO") on top of next-20200424.

Running with tp_printk trace_event=mdio:* shows the phy identifier is 
not read correctly:

BAD:
[    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
val:0x0000
[    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
val:0x0000

GOOD:
[    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
val:0x004d
[    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
val:0xd074

I don't have the original patch in my inbox, hope the manual In-Reply-To 
header is OK.

--
Regards,
Leonard

RE: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Monday, April 27, 2020 11:20 PM
> Hello,
> 
> This patch breaks network boot on at least imx8mm-evk. Boot works if I
> revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt
> driven MDIO with polled IO") on top of next-20200424.
> 
> Running with tp_printk trace_event=mdio:* shows the phy identifier is not
> read correctly:
> 
> BAD:
> [    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x02
> val:0x0000
> [    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x03
> val:0x0000
> 
> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x02
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x03
> val:0xd074
> 
> I don't have the original patch in my inbox, hope the manual In-Reply-To
> header is OK.
> 
> --
> Regards,
> Leonard

Leonard, I already sent out the revert patch to net mail list today.
Thanks for report this issue.

Regards,
Andy

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:
> Hello,
> 
> This patch breaks network boot on at least imx8mm-evk. Boot works if I 
> revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt 
> driven MDIO with polled IO") on top of next-20200424.
> 
> Running with tp_printk trace_event=mdio:* shows the phy identifier is 
> not read correctly:
> 
> BAD:
> [    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x0000
> [    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0x0000
> 
> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0xd074
> 
> I don't have the original patch in my inbox, hope the manual In-Reply-To 
> header is OK.

Hi Leonard

Thanks for you message. You are the first to provide useful
debug information.

Are you using NFS root?

Do you see any ETIMEOUT messages in the bad case?

Thanks
	Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:

Hi Leanard

> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0xd074

You PHY is an ATH8031? If i'm reading these IDs correctly?

Thanks
	Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:
> Hello,
> 
> This patch breaks network boot on at least imx8mm-evk. Boot works if I 
> revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt 
> driven MDIO with polled IO") on top of next-20200424.

Hi Leonard

Please could you try this:

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
index 951e14a3de0e..3c1adaf7affa 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
@@ -109,6 +109,7 @@ &fec1 {
        phy-handle = <&ethphy0>;
        phy-reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
        phy-reset-duration = <10>;
+       phy-reset-post-delay = <100>;
        fsl,magic-packet;
        status = "okay";


There is an interesting post from Fabio Estevam

https://u-boot.denx.narkive.com/PlutD3Rg/patch-1-3-phy-atheros-use-ar8035-config-for-ar8031

Thanks
	Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Fabio Estevam]

Hi Andrew,

On Mon, Apr 27, 2020 at 1:44 PM Andrew Lunn <andrew@lunn.ch> wrote:

> You PHY is an ATH8031? If i'm reading these IDs correctly?

Yes, the imx8mm-evk has an AR8031.

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 12:46 AM
> On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:
> > Hello,
> >
> > This patch breaks network boot on at least imx8mm-evk. Boot works if I
> > revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace
> > interrupt driven MDIO with polled IO") on top of next-20200424.
> 
> Hi Leonard
> 
> Please could you try this:
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> index 951e14a3de0e..3c1adaf7affa 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> @@ -109,6 +109,7 @@ &fec1 {
>         phy-handle = <&ethphy0>;
>         phy-reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
>         phy-reset-duration = <10>;
> +       phy-reset-post-delay = <100>;
>         fsl,magic-packet;
>         status = "okay";
> 
Add the similar change as below on i.MX6SX sdb, it still doesn't work.
As my previous mail, udelay(50) can work.(50us can be optimized)

--- a/arch/arm/boot/dts/imx6sx-sdb.dtsi
+++ b/arch/arm/boot/dts/imx6sx-sdb.dtsi
@@ -194,6 +194,8 @@
        phy-mode = "rgmii-id";
        phy-handle = <&ethphy1>;
        phy-reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
+       phy-reset-duration = <10>;
+       phy-reset-post-delay = <100>;

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Leonard Crestez]

On 2020-04-27 7:46 PM, Andrew Lunn wrote:
> On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:
>> Hello,
>>
>> This patch breaks network boot on at least imx8mm-evk. Boot works if I
>> revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt
>> driven MDIO with polled IO") on top of next-20200424.
> 
> Hi Leonard
> 
> Please could you try this:
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> index 951e14a3de0e..3c1adaf7affa 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> @@ -109,6 +109,7 @@ &fec1 {
>          phy-handle = <&ethphy0>;
>          phy-reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
>          phy-reset-duration = <10>;
> +       phy-reset-post-delay = <100>;
>          fsl,magic-packet;
>          status = "okay";
> 
> 
> There is an interesting post from Fabio Estevam
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fu-boot.denx.narkive.com%2FPlutD3Rg%2Fpatch-1-3-phy-atheros-use-ar8035-config-for-ar8031&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6e84d8adeb644c51a86408d7eaca858d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637236027867051637&amp;sdata=s7825L%2BHQV%2FmPR2saYHcoSAgVTlZDr5gYT62kVgdeJA%3D&amp;reserved=0
> 
> Thanks
> 	Andrew
> 

Does not help. What does seem to help is inserting prints after the 
FEC_ENET_MII check but that's probably because it inject a long delay 
equivalent to the long udelay Andy has mentioned.

I found that in my case FEC_ENET_MII is already set on entry to 
fec_enet_mdio_read, doesn't this make fec_enet_mdio_wait pointless?

Perhaps the problem is that the MII Interrupt pending bit is not 
cleared. I can fix the problem like this:

diff --git drivers/net/ethernet/freescale/fec_main.c 
drivers/net/ethernet/freescale/fec_main.c
index 1ae075a246a3..f1330071647c 100644
--- drivers/net/ethernet/freescale/fec_main.c
+++ drivers/net/ethernet/freescale/fec_main.c
@@ -1841,10 +1841,19 @@ static int fec_enet_mdio_read(struct mii_bus 
*bus, int mii_id, int regnum)

         ret = pm_runtime_get_sync(dev);
         if (ret < 0)
                 return ret;

+       if (1) {
+               u32 ievent;
+               ievent = readl(fep->hwp + FEC_IEVENT);
+               if (ievent & FEC_ENET_MII) {
+                       dev_warn(dev, "found FEC_ENET_MII pending\n");
+                       writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+               }
+       }
+
         if (is_c45) {
                 frame_start = FEC_MMFR_ST_C45;

                 /* write address */
                 frame_addr = (regnum >> 16);


--
Regards,
Leonard

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

Hi Leonard

> Does not help.

Thanks for testing it.

> What does seem to help is inserting prints after the 
> FEC_ENET_MII check but that's probably because it inject a long delay 
> equivalent to the long udelay Andy has mentioned.

Yes, serial ports are slow...

> I found that in my case FEC_ENET_MII is already set on entry to 
> fec_enet_mdio_read, doesn't this make fec_enet_mdio_wait pointless?
> Perhaps the problem is that the MII Interrupt pending bit is not 
> cleared. I can fix the problem like this:
> 
> diff --git drivers/net/ethernet/freescale/fec_main.c 
> drivers/net/ethernet/freescale/fec_main.c
> index 1ae075a246a3..f1330071647c 100644
> --- drivers/net/ethernet/freescale/fec_main.c
> +++ drivers/net/ethernet/freescale/fec_main.c
> @@ -1841,10 +1841,19 @@ static int fec_enet_mdio_read(struct mii_bus 
> *bus, int mii_id, int regnum)
> 
>          ret = pm_runtime_get_sync(dev);
>          if (ret < 0)
>                  return ret;
> 
> +       if (1) {
> +               u32 ievent;
> +               ievent = readl(fep->hwp + FEC_IEVENT);
> +               if (ievent & FEC_ENET_MII) {
> +                       dev_warn(dev, "found FEC_ENET_MII pending\n");
> +                       writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +               }

How often do you see this warning?

The patch which is causing the regression clears any pending events in
fec_enet_mii_init() and after each time we wait. So the bit should not
be set here. If it is set, the question is why?

The other option is that the hardware is broken. It is setting the
event bit way too soon, before we can actually read the data from the
register.

	Andrew	

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 4:14 AM
> Hi Leonard
> 
> > Does not help.
> 
> Thanks for testing it.
> 
> > What does seem to help is inserting prints after the FEC_ENET_MII
> > check but that's probably because it inject a long delay equivalent to
> > the long udelay Andy has mentioned.
> 
> Yes, serial ports are slow...
> 
> > I found that in my case FEC_ENET_MII is already set on entry to
> > fec_enet_mdio_read, doesn't this make fec_enet_mdio_wait pointless?
> > Perhaps the problem is that the MII Interrupt pending bit is not
> > cleared. I can fix the problem like this:
> >
> > diff --git drivers/net/ethernet/freescale/fec_main.c
> > drivers/net/ethernet/freescale/fec_main.c
> > index 1ae075a246a3..f1330071647c 100644
> > --- drivers/net/ethernet/freescale/fec_main.c
> > +++ drivers/net/ethernet/freescale/fec_main.c
> > @@ -1841,10 +1841,19 @@ static int fec_enet_mdio_read(struct mii_bus
> > *bus, int mii_id, int regnum)
> >
> >          ret = pm_runtime_get_sync(dev);
> >          if (ret < 0)
> >                  return ret;
> >
> > +       if (1) {
> > +               u32 ievent;
> > +               ievent = readl(fep->hwp + FEC_IEVENT);
> > +               if (ievent & FEC_ENET_MII) {
> > +                       dev_warn(dev, "found FEC_ENET_MII
> pending\n");
> > +                       writel(FEC_ENET_MII, fep->hwp +
> FEC_IEVENT);
> > +               }
> 
> How often do you see this warning?
> 
> The patch which is causing the regression clears any pending events in
> fec_enet_mii_init() and after each time we wait. So the bit should not be set
> here. If it is set, the question is why?
> 
> The other option is that the hardware is broken. It is setting the event bit way
> too soon, before we can actually read the data from the register.
> 
>         Andrew

Andrew, after investigate the issue, there have one MII event coming later then
clearing MII pending event when writing MSCR register (MII_SPEED).

Check the rtl design by co-working with our IC designer, the MII event generation
condition:
- writing MSCR:
	- mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
- writing MMFR:
	- mscr[7:0]_not_zero
	
mmfr[31:0]: current MMFR register value
mscr[7:0]: current MSCR register value
mscr_reg_data_in[7:0]: the value wrote to MSCR


Below patch can fix the block issue:
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2142,6 +2142,15 @@ static int fec_enet_mii_init(struct platform_device *pdev)
        if (suppress_preamble)
                fep->phy_speed |= BIT(7);

+       /*
+        * Clear MMFR to avoid to generate MII event by writing MSCR.
+        * MII event generation condition:
+        * - writing MSCR:
+        *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
+        * - writing MMFR:
+        *      - mscr[7:0]_not_zero
+        */
+       writel(0, fep->hwp + FEC_MII_DATA);
        writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> Andrew, after investigate the issue, there have one MII event coming later then
> clearing MII pending event when writing MSCR register (MII_SPEED).
> 
> Check the rtl design by co-working with our IC designer, the MII event generation
> condition:
> - writing MSCR:
> 	- mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
> - writing MMFR:
> 	- mscr[7:0]_not_zero
> 	
> mmfr[31:0]: current MMFR register value
> mscr[7:0]: current MSCR register value
> mscr_reg_data_in[7:0]: the value wrote to MSCR
> 
> 
> Below patch can fix the block issue:
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2142,6 +2142,15 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>         if (suppress_preamble)
>                 fep->phy_speed |= BIT(7);
> 
> +       /*
> +        * Clear MMFR to avoid to generate MII event by writing MSCR.
> +        * MII event generation condition:
> +        * - writing MSCR:
> +        *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero & mscr_reg_data_in[7:0] != 0
> +        * - writing MMFR:
> +        *      - mscr[7:0]_not_zero
> +        */
> +       writel(0, fep->hwp + FEC_MII_DATA);
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

Hi Andy

Thanks for digging into the internal of the FEC. Just to make sure i
understand this correctly:

In fec_enet_mii_init() we have:

        holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000) - 1;

        fep->phy_speed = mii_speed << 1 | holdtime << 8;

        writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

        /* Clear any pending transaction complete indication */
        writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);

You are saying this write to the FEC_MII_SPEED register can on some
SoCs trigger an FEC_ENET_MII event. And because it does not happen
immediately, it happens after the clear which is performed here?
Sometime later we then go into fec_enet_mdio_wait(), the event is
still pending, so we read the FEC_MII_DATA register too early?

But this does not fully explain the problem. This should only affect
the first MDIO transaction, because as we exit fec_enet_mdio_wait()
the event is cleared. But Leonard reported that all reads return 0,
not just the first.

    Andrew

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 9:35 PM
> > Andrew, after investigate the issue, there have one MII event coming
> > later then clearing MII pending event when writing MSCR register
> (MII_SPEED).
> >
> > Check the rtl design by co-working with our IC designer, the MII event
> > generation
> > condition:
> > - writing MSCR:
> >       - mmfr[31:0]_not_zero & mscr[7:0]_is_zero &
> > mscr_reg_data_in[7:0] != 0
> > - writing MMFR:
> >       - mscr[7:0]_not_zero
> >
> > mmfr[31:0]: current MMFR register value
> > mscr[7:0]: current MSCR register value
> > mscr_reg_data_in[7:0]: the value wrote to MSCR
> >
> >
> > Below patch can fix the block issue:
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -2142,6 +2142,15 @@ static int fec_enet_mii_init(struct
> platform_device *pdev)
> >         if (suppress_preamble)
> >                 fep->phy_speed |= BIT(7);
> >
> > +       /*
> > +        * Clear MMFR to avoid to generate MII event by writing MSCR.
> > +        * MII event generation condition:
> > +        * - writing MSCR:
> > +        *      - mmfr[31:0]_not_zero & mscr[7:0]_is_zero &
> mscr_reg_data_in[7:0] != 0
> > +        * - writing MMFR:
> > +        *      - mscr[7:0]_not_zero
> > +        */
> > +       writel(0, fep->hwp + FEC_MII_DATA);
> >         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> Hi Andy
> 
> Thanks for digging into the internal of the FEC. Just to make sure i understand
> this correctly:
> 
> In fec_enet_mii_init() we have:
> 
>         holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000)
> - 1;
> 
>         fep->phy_speed = mii_speed << 1 | holdtime << 8;
> 
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>         /* Clear any pending transaction complete indication */
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> 
> You are saying this write to the FEC_MII_SPEED register can on some SoCs
> trigger an FEC_ENET_MII event. And because it does not happen immediately,
> it happens after the clear which is performed here?

Correct.
Before write FEC_MII_SPEED register, FEC_MII_DATA register is not zero, and
the current value of FEC_MII_SPEED register is zero, once write non zero value
to FEC_MII_SPEED register, it trigger MII event.

> Sometime later we then go into fec_enet_mdio_wait(), the event is still
> pending, so we read the FEC_MII_DATA register too early?

Correct.
The first mdio operation is mdio read, read FEC_MII_DATA register is too early,
it get invalid value. 
> 
> But this does not fully explain the problem. This should only affect the first
> MDIO transaction, because as we exit fec_enet_mdio_wait() the event is
> cleared. But Leonard reported that all reads return 0, not just the first.

Of course, it impact subsequent mdio read/write operations.
After you clear MII event that is pending before.
Then, after mdio read data back, MII event is set again.

cpu instruction is much faster than mdio read/write operation.

> 
>     Andrew

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> > Hi Andy
> > 
> > Thanks for digging into the internal of the FEC. Just to make sure i understand
> > this correctly:
> > 
> > In fec_enet_mii_init() we have:
> > 
> >         holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000)
> > - 1;
> > 
> >         fep->phy_speed = mii_speed << 1 | holdtime << 8;
> > 
> >         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> > 
> >         /* Clear any pending transaction complete indication */
> >         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> > 
> > You are saying this write to the FEC_MII_SPEED register can on some SoCs
> > trigger an FEC_ENET_MII event. And because it does not happen immediately,
> > it happens after the clear which is performed here?
> 
> Correct.
> Before write FEC_MII_SPEED register, FEC_MII_DATA register is not zero, and
> the current value of FEC_MII_SPEED register is zero, once write non zero value
> to FEC_MII_SPEED register, it trigger MII event.
> 
> > Sometime later we then go into fec_enet_mdio_wait(), the event is still
> > pending, so we read the FEC_MII_DATA register too early?
> 
> Correct.
> The first mdio operation is mdio read, read FEC_MII_DATA register is too early,
> it get invalid value. 
> > 
> > But this does not fully explain the problem. This should only affect the first
> > MDIO transaction, because as we exit fec_enet_mdio_wait() the event is
> > cleared. But Leonard reported that all reads return 0, not just the first.
> 
> Of course, it impact subsequent mdio read/write operations.
> After you clear MII event that is pending before.
> Then, after mdio read data back, MII event is set again.
> 
> cpu instruction is much faster than mdio read/write operation.

Ah. Now i get it....

The flow is...

Write FEC_MII_SPEED register
Clear event register

A little latter

event bit set because of FEC_MII_SPEED

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from FEC_MII_SPEED
Clear FEC_MII_SPEED event
Read data register too early

A little later

event bit set because read complete

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from read complete
Clear previous read complete event
Read data register too early

A little later

event bit set because read complete

And the cycle continues...

I will make a formal patch from your email.

  Andrew