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

[PATCH net-next] 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.

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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1ae075a246a3..aa5e744ec098 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -996,6 +996,9 @@ fec_restart(struct net_device *ndev)
 		writel(0x0, fep->hwp + FEC_X_CNTRL);
 	}
 
+	/* Prevent an MII event being report when changing speed */
+	writel(0, fep->hwp + FEC_MII_DATA);
+
 	/* Set MII speed */
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
@@ -1182,6 +1185,10 @@ fec_stop(struct net_device *ndev)
 		writel(val, fep->hwp + FEC_ECNTRL);
 		fec_enet_stop_mode(fep, true);
 	}
+
+	/* Prevent an MII event being report when changing speed */
+	writel(0, fep->hwp + FEC_MII_DATA);
+
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
 	/* We have to keep ENET enabled to have MII interrupt stay working */
@@ -2142,6 +2149,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] net: ethernet: fec: Prevent MII event after MII_SPEED write

[Andrew Lunn]

On Tue, Apr 28, 2020 at 07:58:33PM +0200, Andrew Lunn wrote:
> 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.

Hi Andy

Could you get your LAVA instances to test this? Or do we need to wait
for it to make its way into net-next?

Thanks
	Andrew

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

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Wednesday, April 29, 2020 2:02 AM
> On Tue, Apr 28, 2020 at 07:58:33PM +0200, Andrew Lunn wrote:
> > 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.
> 
> Hi Andy
> 
> Could you get your LAVA instances to test this? Or do we need to wait for it to
> make its way into net-next?
> 
> Thanks
>         Andrew

Yes, we need to wait for linux next.

Andy

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

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 28 Apr 2020 19:58:33 +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.
> 
> 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>

Applied to net-next, thanks.

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

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Wednesday, April 29, 2020 5:34 AM
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Tue, 28 Apr 2020 19:58:33 +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.
> >
> > 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>
> 
> Applied to net-next, thanks.

David, it is too early to apply the patch, it will introduce another
break issue as I explain in previous mail for the patch.

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

[David Miller]

From: Andy Duan <fugang.duan@nxp.com>
Date: Wed, 29 Apr 2020 01:55:35 +0000

> From: David Miller <davem@davemloft.net> Sent: Wednesday, April 29, 2020 5:34 AM
>> From: Andrew Lunn <andrew@lunn.ch>
>> Date: Tue, 28 Apr 2020 19:58:33 +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.
>> >
>> > 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>
>> 
>> Applied to net-next, thanks.
> 
> David, it is too early to apply the patch, it will introduce another
> break issue as I explain in previous mail for the patch.

So what should I do, revert?

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

[Andy Duan]

From: David Miller <davem@davemloft.net> Sent: Wednesday, April 29, 2020 11:35 AM
> From: Andy Duan <fugang.duan@nxp.com>
> Date: Wed, 29 Apr 2020 01:55:35 +0000
> 
> > From: David Miller <davem@davemloft.net> Sent: Wednesday, April 29,
> > 2020 5:34 AM
> >> From: Andrew Lunn <andrew@lunn.ch>
> >> Date: Tue, 28 Apr 2020 19:58:33 +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.
> >> >
> >> > 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>
> >>
> >> Applied to net-next, thanks.
> >
> > David, it is too early to apply the patch, it will introduce another
> > break issue as I explain in previous mail for the patch.
> 
> So what should I do, revert?

If you can revert the patch, please do it. 
Thanks, David.

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

[Andrew Lunn]

> > >> Applied to net-next, thanks.
> > >
> > > David, it is too early to apply the patch, it will introduce another
> > > break issue as I explain in previous mail for the patch.
> > 
> > So what should I do, revert?
> 
> If you can revert the patch, please do it. 
> Thanks, David.

Hi David

Please do revert. I will send a new version of the patch
soon. Probably RFC this time!

      Andrew

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

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 29 Apr 2020 16:11:02 +0200

>> > >> Applied to net-next, thanks.
>> > >
>> > > David, it is too early to apply the patch, it will introduce another
>> > > break issue as I explain in previous mail for the patch.
>> > 
>> > So what should I do, revert?
>> 
>> If you can revert the patch, please do it. 
>> Thanks, David.
> 
> Hi David
> 
> Please do revert. I will send a new version of the patch
> soon. Probably RFC this time!

Ok, done.

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

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Wednesday, April 29, 2020 1:59 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.
> 
> 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 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 1ae075a246a3..aa5e744ec098 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -996,6 +996,9 @@ fec_restart(struct net_device *ndev)
>                 writel(0x0, fep->hwp + FEC_X_CNTRL);
>         }
> 
> +       /* Prevent an MII event being report when changing speed */
> +       writel(0, fep->hwp + FEC_MII_DATA);
> +

Andrew, my suggestion is only add the change in .fec_enet_mii_init().
There have risk and may introduce MII event here when wirte value
into FEC_MII_DATA register.


As I said, if FEC_MII_SPEED register[7:0] is not zero, MII event will be generated
when write FEC_MII_DATA register. 
        * - writing MMFR:
        *      - mscr[7:0]_not_zero

>         /* Set MII speed */
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> @@ -1182,6 +1185,10 @@ fec_stop(struct net_device *ndev)
>                 writel(val, fep->hwp + FEC_ECNTRL);
>                 fec_enet_stop_mode(fep, true);
>         }
> +
> +       /* Prevent an MII event being report when changing speed */
> +       writel(0, fep->hwp + FEC_MII_DATA);
> +

The same here.
We should not add the change here.

I already consider normal case, suspend/resume with power on case,
Suspend/resume with power off (register value lost) case, only add
the change in . fec_enet_mii_init() seems safe.
The patch "net: ethernet: fec: Prevent MII event after MII_SPEED write"
brings some trouble here, we need to consider all cases when writing
FEC_MII_DATA, FEC_MII_SPEED registers.

Again, please note writing the two registers may trigger MII event with
Below logic:
        * 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
        */

If interrupt mode, we don't take care this logic and dependency.

>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>         /* We have to keep ENET enabled to have MII interrupt stay
> working */ @@ -2142,6 +2149,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