[PATCH net-next v2] net: ethernet: fec: Prevent MII event after MII_SPEED write

[PATCH net-next v2] net: ethernet: fec: Prevent MII event after MII_SPEED write

[Andrew Lunn]

The change to polled IO for MDIO completion assumes that MII events
are only generated for MDIO transactions. However on some SoCs writing
to the MII_SPEED register can also trigger an MII event. As a result,
the next MDIO read has a pending MII event, and immediately reads the
data registers before it contains useful data. When the read does
complete, another MII event is posted, which results in the next read
also going wrong, and the cycle continues.

By writing 0 to the MII_DATA register before writing to the speed
register, this MII event for the MII_SPEED is suppressed, and polled
IO works as expected.

v2 - Only infec_enet_mii_init()

Fixes: 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
Reported-by: Andy Duan <fugang.duan@nxp.com>
Suggested-by: Andy Duan <fugang.duan@nxp.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1ae075a246a3..2e209142f2d1 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2142,6 +2142,16 @@ 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);
 
 	/* Clear any pending transaction complete indication */
-- 
2.26.1

RE: [EXT] [PATCH net-next v2] net: ethernet: fec: Prevent MII event after MII_SPEED write

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Thursday, April 30, 2020 4:53 AM
> The change to polled IO for MDIO completion assumes that MII events are
> only generated for MDIO transactions. However on some SoCs writing to the
> MII_SPEED register can also trigger an MII event. As a result, the next MDIO
> read has a pending MII event, and immediately reads the data registers
> before it contains useful data. When the read does complete, another MII
> event is posted, which results in the next read also going wrong, and the cycle
> continues.
> 
> By writing 0 to the MII_DATA register before writing to the speed register, this
> MII event for the MII_SPEED is suppressed, and polled IO works as expected.
> 
> v2 - Only infec_enet_mii_init()
> 
> Fixes: 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt driven MDIO with
> polled IO")
> Reported-by: Andy Duan <fugang.duan@nxp.com>
> Suggested-by: Andy Duan <fugang.duan@nxp.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Acked-by: Fugang Duan <fugang.duan@nxp.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 1ae075a246a3..2e209142f2d1 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2142,6 +2142,16 @@ 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);
> 
>         /* Clear any pending transaction complete indication */
> --
> 2.26.1

Re: [PATCH net-next v2] net: ethernet: fec: Prevent MII event after MII_SPEED write

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 29 Apr 2020 22:53:23 +0200

> The change to polled IO for MDIO completion assumes that MII events
> are only generated for MDIO transactions. However on some SoCs writing
> to the MII_SPEED register can also trigger an MII event. As a result,
> the next MDIO read has a pending MII event, and immediately reads the
> data registers before it contains useful data. When the read does
> complete, another MII event is posted, which results in the next read
> also going wrong, and the cycle continues.
> 
> By writing 0 to the MII_DATA register before writing to the speed
> register, this MII event for the MII_SPEED is suppressed, and polled
> IO works as expected.
> 
> v2 - Only infec_enet_mii_init()
> 
> Fixes: 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
> Reported-by: Andy Duan <fugang.duan@nxp.com>
> Suggested-by: Andy Duan <fugang.duan@nxp.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Hmmm, I reverted the Fixes: tag patch so you'll need to respin this I think.

Thanks.

Re: [PATCH net-next v2] net: ethernet: fec: Prevent MII event after MII_SPEED write

[Andrew Lunn]

On Fri, May 01, 2020 at 03:29:54PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Wed, 29 Apr 2020 22:53:23 +0200
> 
> > The change to polled IO for MDIO completion assumes that MII events
> > are only generated for MDIO transactions. However on some SoCs writing
> > to the MII_SPEED register can also trigger an MII event. As a result,
> > the next MDIO read has a pending MII event, and immediately reads the
> > data registers before it contains useful data. When the read does
> > complete, another MII event is posted, which results in the next read
> > also going wrong, and the cycle continues.
> > 
> > By writing 0 to the MII_DATA register before writing to the speed
> > register, this MII event for the MII_SPEED is suppressed, and polled
> > IO works as expected.
> > 
> > v2 - Only infec_enet_mii_init()
> > 
> > Fixes: 29ae6bd1b0d8 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
> > Reported-by: Andy Duan <fugang.duan@nxp.com>
> > Suggested-by: Andy Duan <fugang.duan@nxp.com>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Hmmm, I reverted the Fixes: tag patch so you'll need to respin this I think.

Ah.

We wanted the fix reverting, but not that the fix was fixing. Sorry,
we were unclear.

   Andrew