[PATCH net-next v2 0/3] FEC MDIO speedups

[PATCH net-next v2 0/3] FEC MDIO speedups

[Andrew Lunn]

This patchset gives a number of speedups for MDIO with the FEC.
Replacing interrupt driven with polled IO brings a big speedup due to
the overheads of interrupts compared to the short time interval.
Clocking the bus faster, when the MDIO targets supports it, can double
the transfer rate. And suppressing the preamble, if devices support
it, makes each transaction faster.

By default the MDIO clock remains 2.5MHz and preables are used. But
these can now be controlled from the device tree. Since these are
generic properties applicable to more than just FEC, these have been
added to the generic MDIO binding documentation.

Andrew Lunn (3):
  net: ethernet: fec: Replace interrupt driven MDIO with polled IO
  net: ethernet: fec: Allow configuration of MDIO bus speed
  net: ethernet: fec: Allow the MDIO preamble to be disabled

 .../devicetree/bindings/net/fsl-fec.txt       |  1 +
 .../devicetree/bindings/net/mdio.yaml         |  8 ++
 drivers/net/ethernet/freescale/fec.h          |  4 +-
 drivers/net/ethernet/freescale/fec_main.c     | 85 +++++++++++--------
 4 files changed, 59 insertions(+), 39 deletions(-)

-- 
2.26.1

[PATCH net-next v2 1/3] 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 | 67 ++++++++++++-----------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index e74dd1f86bba..a6cdd5b61921 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 */
@@ -543,7 +542,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 dc6f8763a5d4..6530829632b1 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -976,8 +976,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);
 
@@ -1123,7 +1123,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);
@@ -1652,6 +1652,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);
 
@@ -1659,16 +1663,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;
 }
 
@@ -1818,11 +1818,24 @@ 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)
+{
+	uint ievent;
+	int ret;
+
+	ret = readl_poll_timeout(fep->hwp + FEC_IEVENT, ievent,
+				 ievent & FEC_ENET_MII, 0, 30000);
+
+	if (!ret)
+		writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+
+	return ret;
+}
+
 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);
 
@@ -1830,8 +1843,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;
 
@@ -1843,11 +1854,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;
 		}
 
@@ -1866,11 +1875,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;
 	}
 
@@ -1888,7 +1895,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);
 
@@ -1898,8 +1904,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;
 
@@ -1911,11 +1915,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 {
@@ -1931,12 +1933,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);
@@ -2132,6 +2131,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;
@@ -3674,7 +3676,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.1

[PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Andrew Lunn]

MDIO busses typically operate at 2.5MHz. However many devices can
operate at faster speeds. This then allows more MDIO transactions per
second, useful for Ethernet switch statistics, or Ethernet PHY TDR
data. Allow the bus speed to be configured, using the standard
"clock-frequency" property, which i2c busses use to indicate the bus
speed.

Suggested-by: Chris Healy <Chris.Healy@zii.aero>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |  1 +
 Documentation/devicetree/bindings/net/mdio.yaml   |  4 ++++
 drivers/net/ethernet/freescale/fec_main.c         | 11 ++++++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index ff8b0f211aa1..26c492a2e0e1 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -82,6 +82,7 @@ ethernet@83fec000 {
 	phy-supply = <&reg_fec_supply>;
 	phy-handle = <&ethphy>;
 	mdio {
+	        clock-frequency = <5000000>;
 		ethphy: ethernet-phy@6 {
 			compatible = "ethernet-phy-ieee802.3-c22";
 			reg = <6>;
diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
index 50c3397a82bc..bcd457c54cd7 100644
--- a/Documentation/devicetree/bindings/net/mdio.yaml
+++ b/Documentation/devicetree/bindings/net/mdio.yaml
@@ -39,6 +39,10 @@ properties:
       and must therefore be appropriately determined based on all PHY
       requirements (maximum value of all per-PHY RESET pulse widths).
 
+  clock-frequency:
+    description:
+      Desired MDIO bus clock frequency in Hz.
+
 patternProperties:
   "^ethernet-phy@[0-9a-f]+$":
     type: object
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 6530829632b1..28ef0abfa660 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2067,6 +2067,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	struct device_node *node;
 	int err = -ENXIO;
 	u32 mii_speed, holdtime;
+	u32 bus_freq;
 
 	/*
 	 * The i.MX28 dual fec interfaces are not equal.
@@ -2094,15 +2095,20 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	bus_freq = 2500000; /* 2.5MHz by default */
+	node = of_get_child_by_name(pdev->dev.of_node, "mdio");
+	if (node)
+		of_property_read_u32(node, "clock-frequency", &bus_freq);
+
 	/*
-	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
+	 * Set MII speed (= clk_get_rate() / 2 * phy_speed)
 	 *
 	 * The formula for FEC MDC is 'ref_freq / (MII_SPEED x 2)' while
 	 * for ENET-MAC is 'ref_freq / ((MII_SPEED + 1) x 2)'.  The i.MX28
 	 * Reference Manual has an error on this, and gets fixed on i.MX6Q
 	 * document.
 	 */
-	mii_speed = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 5000000);
+	mii_speed = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), bus_freq * 2);
 	if (fep->quirks & FEC_QUIRK_ENET_MAC)
 		mii_speed--;
 	if (mii_speed > 63) {
@@ -2148,7 +2154,6 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	fep->mii_bus->priv = fep;
 	fep->mii_bus->parent = &pdev->dev;
 
-	node = of_get_child_by_name(pdev->dev.of_node, "mdio");
 	err = of_mdiobus_register(fep->mii_bus, node);
 	of_node_put(node);
 	if (err)
-- 
2.26.1

[PATCH net-next v2 3/3] net: ethernet: fec: Allow the MDIO preamble to be disabled

[Andrew Lunn]

An MDIO transaction normally starts with 32 1s as a preamble. However
not all devices requires such a preamble. Add a device tree property
which allows the preamble to be suppressed. This will half the size of
the MDIO transaction, allowing faster transactions.

Suggested-by: Chris Healy <Chris.Healy@zii.aero>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
 drivers/net/ethernet/freescale/fec_main.c       | 9 ++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
index bcd457c54cd7..41ed4019f8ca 100644
--- a/Documentation/devicetree/bindings/net/mdio.yaml
+++ b/Documentation/devicetree/bindings/net/mdio.yaml
@@ -43,6 +43,10 @@ properties:
     description:
       Desired MDIO bus clock frequency in Hz.
 
+  suppress-preamble:
+        description: The 32 bit preamble should be suppressed.
+        type: boolean
+
 patternProperties:
   "^ethernet-phy@[0-9a-f]+$":
     type: object
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 28ef0abfa660..3404e085c657 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2064,6 +2064,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	static struct mii_bus *fec0_mii_bus;
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	bool suppress_preamble = false;
 	struct device_node *node;
 	int err = -ENXIO;
 	u32 mii_speed, holdtime;
@@ -2097,8 +2098,11 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 
 	bus_freq = 2500000; /* 2.5MHz by default */
 	node = of_get_child_by_name(pdev->dev.of_node, "mdio");
-	if (node)
+	if (node) {
 		of_property_read_u32(node, "clock-frequency", &bus_freq);
+		suppress_preamble = of_property_read_bool(node,
+							  "suppress-preamble");
+	}
 
 	/*
 	 * Set MII speed (= clk_get_rate() / 2 * phy_speed)
@@ -2135,6 +2139,9 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 
 	fep->phy_speed = mii_speed << 1 | holdtime << 8;
 
+	if (suppress_preamble)
+		fep->phy_speed |= BIT(7);
+
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
 	/* Clear any pending transaction complete indication */
-- 
2.26.1

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Florian Fainelli]

Hi Andrew,

On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> MDIO busses typically operate at 2.5MHz. However many devices can
> operate at faster speeds. This then allows more MDIO transactions per
> second, useful for Ethernet switch statistics, or Ethernet PHY TDR
> data. Allow the bus speed to be configured, using the standard
> "clock-frequency" property, which i2c busses use to indicate the bus
> speed.
> 
> Suggested-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

This does look good to me, however if we go down that road, it looks 
like we should also support a 'mdio-max-frequency' per MDIO child node 
in order to scale up and down the frequency accordingly, and do that on 
a per transfer basis. So this means that we would likely need to add a 
callback into the mii_bus structure to configure the MDIO bus controller 
clock rate based on the min between what the controller and the device 
supports.

It seems to me that everything works in your case because you have a 
single MDIO device which is a switch, right?
-- 
Florian

Re: [PATCH net-next v2 3/3] net: ethernet: fec: Allow the MDIO preamble to be disabled

[Florian Fainelli]

Hi Andrew,

On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> An MDIO transaction normally starts with 32 1s as a preamble. However
> not all devices requires such a preamble. Add a device tree property
> which allows the preamble to be suppressed. This will half the size of
> the MDIO transaction, allowing faster transactions.
> 
> Suggested-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
>   drivers/net/ethernet/freescale/fec_main.c       | 9 ++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
> index bcd457c54cd7..41ed4019f8ca 100644
> --- a/Documentation/devicetree/bindings/net/mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/mdio.yaml
> @@ -43,6 +43,10 @@ properties:
>       description:
>         Desired MDIO bus clock frequency in Hz.
>   
> +  suppress-preamble:
> +        description: The 32 bit preamble should be suppressed.
> +        type: boolean

This is a property of the MDIO device node and the MDIO bus controller 
as well, so I would assume that it has to be treated a little it like 
the 'broken-turn-around' property and it would have to be a bitmask per 
MDIO device address that is set/clear depending on what the device 
support. If it is set for the device and your controller supports it, 
then you an suppress preamble.
-- 
Florian

RE: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, April 18, 2020 8:04 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.
> 
> 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 | 67 ++++++++++++-----------
>  2 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index e74dd1f86bba..a6cdd5b61921 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 */ @@ -543,7 +542,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 dc6f8763a5d4..6530829632b1 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -976,8 +976,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);
> 
> @@ -1123,7 +1123,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);
> @@ -1652,6 +1652,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);
> 
> @@ -1659,16 +1663,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;
>  }
> 
> @@ -1818,11 +1818,24 @@ 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) {
> +       uint ievent;
> +       int ret;
> +
> +       ret = readl_poll_timeout(fep->hwp + FEC_IEVENT, ievent,
> +                                ievent & FEC_ENET_MII, 0, 30000);

sleep X us between reads ?

> +
> +       if (!ret)
> +               writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> +
> +       return ret;
> +}
> +
>  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);
> 
> @@ -1830,8 +1843,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;
> 
> @@ -1843,11 +1854,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;
>                 }
> 
> @@ -1866,11 +1875,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;
>         }
> 
> @@ -1888,7 +1895,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);
> 
> @@ -1898,8 +1904,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;
> 
> @@ -1911,11 +1915,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 {
> @@ -1931,12 +1933,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);
> @@ -2132,6 +2131,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;
> @@ -3674,7 +3676,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.1

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Andrew Lunn]

On Fri, Apr 17, 2020 at 05:34:56PM -0700, Florian Fainelli wrote:
> Hi Andrew,
> 
> On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> > MDIO busses typically operate at 2.5MHz. However many devices can
> > operate at faster speeds. This then allows more MDIO transactions per
> > second, useful for Ethernet switch statistics, or Ethernet PHY TDR
> > data. Allow the bus speed to be configured, using the standard
> > "clock-frequency" property, which i2c busses use to indicate the bus
> > speed.
> > 
> > Suggested-by: Chris Healy <Chris.Healy@zii.aero>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> This does look good to me, however if we go down that road, it looks like we
> should also support a 'mdio-max-frequency' per MDIO child node in order to
> scale up and down the frequency accordingly.

Hi Florian

I don't see how that would work. Each device on the bus needs to be
able to receiver the transaction in order to decode the device
address, and then either discard it, or act on it. So the same as I2C
where the device address is part of the transaction. You need the bus
to run as fast as the slowest device on the bus. So a bus property is
the simplest. You could have per device properties, and during the bus
scan, figure out what the slowest device is, but that seems to add
complexity for no real gain. I2C does not have this either.

If MDIO was more like SPI, with per device chip select lines, then a
per device frequency would make sense.

	   Andrew

Re: [PATCH net-next v2 3/3] net: ethernet: fec: Allow the MDIO preamble to be disabled

[Andrew Lunn]

> This is a property of the MDIO device node and the MDIO bus controller as
> well, so I would assume that it has to be treated a little it like the
> 'broken-turn-around' property and it would have to be a bitmask per MDIO
> device address that is set/clear depending on what the device support. If it
> is set for the device and your controller supports it, then you an suppress
> preamble.

Again, i don't see how this can work. You need all the devices on the
bus to support preamble suppression, otherwise you cannot use it. As
with a maximum clock frequency, we could add the complexity to check
as bus scan time that all devices have the necessary property, but i
don't think it brings anything.

    Andrew

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Florian Fainelli]

On 4/18/2020 7:23 AM, Andrew Lunn wrote:
> On Fri, Apr 17, 2020 at 05:34:56PM -0700, Florian Fainelli wrote:
>> Hi Andrew,
>>
>> On 4/17/2020 5:03 PM, Andrew Lunn wrote:
>>> MDIO busses typically operate at 2.5MHz. However many devices can
>>> operate at faster speeds. This then allows more MDIO transactions per
>>> second, useful for Ethernet switch statistics, or Ethernet PHY TDR
>>> data. Allow the bus speed to be configured, using the standard
>>> "clock-frequency" property, which i2c busses use to indicate the bus
>>> speed.
>>>
>>> Suggested-by: Chris Healy <Chris.Healy@zii.aero>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>
>> This does look good to me, however if we go down that road, it looks like we
>> should also support a 'mdio-max-frequency' per MDIO child node in order to
>> scale up and down the frequency accordingly.
> 
> Hi Florian
> 
> I don't see how that would work. Each device on the bus needs to be
> able to receiver the transaction in order to decode the device
> address, and then either discard it, or act on it. So the same as I2C
> where the device address is part of the transaction. You need the bus
> to run as fast as the slowest device on the bus. So a bus property is
> the simplest. You could have per device properties, and during the bus
> scan, figure out what the slowest device is, but that seems to add
> complexity for no real gain. I2C does not have this either.
> 
> If MDIO was more like SPI, with per device chip select lines, then a
> per device frequency would make sense.

OK, that is a good point, but then again, just like patch #3 you need to 
ensure that you are setting a MDIO bus controller frequency that is the 
lowest common denominator of all MDIO slaves on the bus, which means 
that you need to know about what devices do support.

Your current patch makes it possible for someone to set a 
'clock-frequency' which is not going to guarantee that all MDIO devices 
will work, and the same is arguably true for every other MDIO controller 
that supports having its bus frequency defined/controlled. So maybe we 
can make this in a progressive manner: start with your patches, but add 
support for specifying the MDIO devices' maximum supported frequency and 
later add code in mdio_bus.c which is responsible for selecting the 
lowest frequency and telling the use that a frequency lower than that 
specified in 'clock-frequency' has been selected.
-- 
Florian

Re: [PATCH net-next v2 3/3] net: ethernet: fec: Allow the MDIO preamble to be disabled

[Florian Fainelli]

On 4/18/2020 7:27 AM, Andrew Lunn wrote:
>> This is a property of the MDIO device node and the MDIO bus controller as
>> well, so I would assume that it has to be treated a little it like the
>> 'broken-turn-around' property and it would have to be a bitmask per MDIO
>> device address that is set/clear depending on what the device support. If it
>> is set for the device and your controller supports it, then you an suppress
>> preamble.
> 
> Again, i don't see how this can work. You need all the devices on the
> bus to support preamble suppression, otherwise you cannot use it. As
> with a maximum clock frequency, we could add the complexity to check
> as bus scan time that all devices have the necessary property, but i
> don't think it brings anything.

With your current patch I can define 'suppress-preamble' for the FEC 
MDIO node, and if there is at least one device on the bus that does not 
support preamble suppression, they are not going to work, so if nothing 
else you need to intersect between the device capability and the MDIO 
bus controller capability. Same comments as patch #2 about we could 
approach this.
-- 
Florian

Re: [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Fabio Estevam]

On Fri, Apr 17, 2020 at 9:06 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> 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

Nit: please use 's' instead of 'S' for seconds.

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Andrew Lunn]

> > I don't see how that would work. Each device on the bus needs to be
> > able to receiver the transaction in order to decode the device
> > address, and then either discard it, or act on it. So the same as I2C
> > where the device address is part of the transaction. You need the bus
> > to run as fast as the slowest device on the bus. So a bus property is
> > the simplest. You could have per device properties, and during the bus
> > scan, figure out what the slowest device is, but that seems to add
> > complexity for no real gain. I2C does not have this either.
> > 
> > If MDIO was more like SPI, with per device chip select lines, then a
> > per device frequency would make sense.
> 
> OK, that is a good point, but then again, just like patch #3 you need to
> ensure that you are setting a MDIO bus controller frequency that is the
> lowest common denominator of all MDIO slaves on the bus, which means that
> you need to know about what devices do support.

Hi Florian

I've been following what I2C does, since MDIO and I2C is very similar.
I2C has none of what you are asking for. If I2C does not need any of
this, does MDIO? I2C assumes what whoever writes the DT knows what
they are doing and will set a valid clock frequency which works for
all devices on the bus. This seems to work for I2C, so why should it
not work for MDIO?

My preference is KISS.

    Andrew

Re: [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Florian Fainelli]

On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> 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>

You should support both modes and let an user decide whether they want 
to use interrupts or polling mode by specifying or omitting the 
'interrupts' property in their Device Tree.
-- 
Florian

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Florian Fainelli]

On 4/18/2020 9:49 AM, Andrew Lunn wrote:
>>> I don't see how that would work. Each device on the bus needs to be
>>> able to receiver the transaction in order to decode the device
>>> address, and then either discard it, or act on it. So the same as I2C
>>> where the device address is part of the transaction. You need the bus
>>> to run as fast as the slowest device on the bus. So a bus property is
>>> the simplest. You could have per device properties, and during the bus
>>> scan, figure out what the slowest device is, but that seems to add
>>> complexity for no real gain. I2C does not have this either.
>>>
>>> If MDIO was more like SPI, with per device chip select lines, then a
>>> per device frequency would make sense.
>>
>> OK, that is a good point, but then again, just like patch #3 you need to
>> ensure that you are setting a MDIO bus controller frequency that is the
>> lowest common denominator of all MDIO slaves on the bus, which means that
>> you need to know about what devices do support.
> 
> Hi Florian
> 
> I've been following what I2C does, since MDIO and I2C is very similar.
> I2C has none of what you are asking for. If I2C does not need any of
> this, does MDIO? I2C assumes what whoever writes the DT knows what
> they are doing and will set a valid clock frequency which works for
> all devices on the bus. This seems to work for I2C, so why should it
> not work for MDIO?
> 
> My preference is KISS.

OK, you have convinced me.
-- 
Florian

Re: [PATCH net-next v2 2/3] net: ethernet: fec: Allow configuration of MDIO bus speed

[Florian Fainelli]

On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> MDIO busses typically operate at 2.5MHz. However many devices can
> operate at faster speeds. This then allows more MDIO transactions per
> second, useful for Ethernet switch statistics, or Ethernet PHY TDR
> data. Allow the bus speed to be configured, using the standard
> "clock-frequency" property, which i2c busses use to indicate the bus
> speed.
> 
> Suggested-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next v2 3/3] net: ethernet: fec: Allow the MDIO preamble to be disabled

[Florian Fainelli]

On 4/17/2020 5:03 PM, Andrew Lunn wrote:
> An MDIO transaction normally starts with 32 1s as a preamble. However
> not all devices requires such a preamble. Add a device tree property
> which allows the preamble to be suppressed. This will half the size of
> the MDIO transaction, allowing faster transactions.
> 
> Suggested-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Chris Healy]

I did some profiling using an oscilloscope with my NXP Vybrid based
platform to see what different "sleep_us" values resulted in for start
of MDIO to start of MDIO transaction times.  Here's what I found:

0  - ~38us to ~40us
1  - ~48us to ~64us
2  - ~48us to ~64us
3  - ~48us to ~64us
4  - ~48us to ~64us
4  - ~48us to ~64us
5  - ~48us to ~64us
6  - ~48us to ~64us
7  - ~48us to ~64us
8  - ~48us to ~64us
9  - ~56us to ~88us
10 - ~56us to ~112us

Basically, with the "sleep_us" value set to 0, I would get the
shortest inter transaction times with a very low variance.  Once I
went to a non-zero value, the inter transaction time went up, as well
as the variance, which I suppose makes sense....


On Sat, Apr 18, 2020 at 6:55 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, April 18, 2020 8:04 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.
> >
> > 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 | 67 ++++++++++++-----------
> >  2 files changed, 35 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index e74dd1f86bba..a6cdd5b61921 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 */ @@ -543,7 +542,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 dc6f8763a5d4..6530829632b1 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -976,8 +976,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);
> >
> > @@ -1123,7 +1123,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);
> > @@ -1652,6 +1652,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);
> >
> > @@ -1659,16 +1663,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;
> >  }
> >
> > @@ -1818,11 +1818,24 @@ 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) {
> > +       uint ievent;
> > +       int ret;
> > +
> > +       ret = readl_poll_timeout(fep->hwp + FEC_IEVENT, ievent,
> > +                                ievent & FEC_ENET_MII, 0, 30000);
>
> sleep X us between reads ?
>
> > +
> > +       if (!ret)
> > +               writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> > +
> > +       return ret;
> > +}
> > +
> >  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);
> >
> > @@ -1830,8 +1843,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;
> >
> > @@ -1843,11 +1854,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;
> >                 }
> >
> > @@ -1866,11 +1875,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;
> >         }
> >
> > @@ -1888,7 +1895,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);
> >
> > @@ -1898,8 +1904,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;
> >
> > @@ -1911,11 +1915,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 {
> > @@ -1931,12 +1933,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);
> > @@ -2132,6 +2131,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;
> > @@ -3674,7 +3676,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.1
>

Re: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Sat, Apr 18, 2020 at 03:39:17PM -0700, Chris Healy wrote:
> I did some profiling using an oscilloscope with my NXP Vybrid based
> platform to see what different "sleep_us" values resulted in for start
> of MDIO to start of MDIO transaction times.  Here's what I found:
> 
> 0  - ~38us to ~40us
> 1  - ~48us to ~64us
> 2  - ~48us to ~64us
> 3  - ~48us to ~64us
> 4  - ~48us to ~64us
> 4  - ~48us to ~64us
> 5  - ~48us to ~64us
> 6  - ~48us to ~64us
> 7  - ~48us to ~64us
> 8  - ~48us to ~64us
> 9  - ~56us to ~88us
> 10 - ~56us to ~112us
> 
> Basically, with the "sleep_us" value set to 0, I would get the
> shortest inter transaction times with a very low variance.  Once I
> went to a non-zero value, the inter transaction time went up, as well
> as the variance, which I suppose makes sense....

Thanks for these numbers. They are suggesting that udelay() is quite
expensive, and is probably taking longer than we expect. I might
instrument the function to see what is happening.

	   Andrew

RE: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Chris Healy <cphealy@gmail.com> Sent: Sunday, April 19, 2020 6:39 AM
> I did some profiling using an oscilloscope with my NXP Vybrid based platform

Vybrid platform is low-end platform, can you test the value on i.MX6/MX8 ?
I think i.MX6/MX8 add some sleep_us has no MDIO transaction performance drop.

> to see what different "sleep_us" values resulted in for start of MDIO to start
> of MDIO transaction times.  Here's what I found:
> 
> 0  - ~38us to ~40us
> 1  - ~48us to ~64us
> 2  - ~48us to ~64us
> 3  - ~48us to ~64us
> 4  - ~48us to ~64us
> 4  - ~48us to ~64us
> 5  - ~48us to ~64us
> 6  - ~48us to ~64us
> 7  - ~48us to ~64us
> 8  - ~48us to ~64us
> 9  - ~56us to ~88us
> 10 - ~56us to ~112us
> 
> Basically, with the "sleep_us" value set to 0, I would get the shortest inter
> transaction times with a very low variance.  Once I went to a non-zero value,
> the inter transaction time went up, as well as the variance, which I suppose
> makes sense....
> 
> 
> On Sat, Apr 18, 2020 at 6:55 AM Andy Duan <fugang.duan@nxp.com> wrote:
> >
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Saturday, April 18, 2020 8:04
> > 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.
> > >
> > > 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 | 67
> > > ++++++++++++-----------
> > >  2 files changed, 35 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/fec.h
> > > b/drivers/net/ethernet/freescale/fec.h
> > > index e74dd1f86bba..a6cdd5b61921 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 */ @@ -543,7 +542,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 dc6f8763a5d4..6530829632b1 100644
> > > --- a/drivers/net/ethernet/freescale/fec_main.c
> > > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > > @@ -976,8 +976,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);
> > >
> > > @@ -1123,7 +1123,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); @@ -1652,6 +1652,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);
> > >
> > > @@ -1659,16 +1663,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;
> > >  }
> > >
> > > @@ -1818,11 +1818,24 @@ 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) {
> > > +       uint ievent;
> > > +       int ret;
> > > +
> > > +       ret = readl_poll_timeout(fep->hwp + FEC_IEVENT, ievent,
> > > +                                ievent & FEC_ENET_MII, 0,
> 30000);
> >
> > sleep X us between reads ?
> >
> > > +
> > > +       if (!ret)
> > > +               writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  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);
> > >
> > > @@ -1830,8 +1843,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;
> > >
> > > @@ -1843,11 +1854,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;
> > >                 }
> > >
> > > @@ -1866,11 +1875,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;
> > >         }
> > >
> > > @@ -1888,7 +1895,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);
> > >
> > > @@ -1898,8 +1904,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;
> > >
> > > @@ -1911,11 +1915,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 {
> > > @@ -1931,12 +1933,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); @@ -2132,6 +2131,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;
> > > @@ -3674,7 +3676,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.1
> >

Re: [EXT] [PATCH net-next v2 1/3] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Sat, Apr 18, 2020 at 03:39:17PM -0700, Chris Healy wrote:
> I did some profiling using an oscilloscope with my NXP Vybrid based
> platform to see what different "sleep_us" values resulted in for start
> of MDIO to start of MDIO transaction times.  Here's what I found:
> 
> 0  - ~38us to ~40us
> 1  - ~48us to ~64us
> 2  - ~48us to ~64us
> 3  - ~48us to ~64us
> 4  - ~48us to ~64us
> 4  - ~48us to ~64us
> 5  - ~48us to ~64us
> 6  - ~48us to ~64us
> 7  - ~48us to ~64us
> 8  - ~48us to ~64us
> 9  - ~56us to ~88us
> 10 - ~56us to ~112us
> 
> Basically, with the "sleep_us" value set to 0, I would get the
> shortest inter transaction times with a very low variance.  Once I
> went to a non-zero value, the inter transaction time went up, as well
> as the variance, which I suppose makes sense....

Hi All

I dug into this, adding some instrumentation. I've been testing on a
Vybrid platform. During boot it does over 6500 transactions in order
to configure 3 switches and one PHY.

With a delay of 0, it polls an average of 62 times, and it needs 29us
to exit the loop. This means one poll takes 0.5uS.

With a delay of 1uS, it polls on average 2 times, and takes on average
45uS to exit the loop. Which suggests the delay takes around 22uS,
despite the request for only 1uS. Looking at the code, it is actually
performing a usleep_range(1, 1). So i'm assuming sleeping is very
expensive, maybe it is using a timer? So we end up with just as much
interrupt work as waiting for the MDIO interrupt?

By swapping to readl_poll_timeout_atomic() i got much better
behaviour. This uses udelay(). Using a delay of 2uS, it loops polling
the completion bit an average of 10 times. It takes 29uS to exist the
loop. This suggests that udelay(2) is pretty accurate.

This seems like a reasonable compromise. The bus load has been reduced
from 62 to 10 poll loops, without increasing the time it takes to exit
the loop by much. And 10 polls allows for significantly faster
completions when using a faster bus clock and preamble suppression.

So i plan to resubmit using readl_poll_timeout_atomic().

   Andrew