| Message ID | 20200428175833.30517-1-andrew@lunn.ch |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write | expand |
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.
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
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
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.
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?
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.
> > >> 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
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.
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 */
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(+)