* [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
@ 2020-04-28 17:58 Andrew Lunn
2020-04-28 18:01 ` Andrew Lunn
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andrew Lunn @ 2020-04-28 17:58 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Chris Healy, Andy Duan, Leonard Crestez, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-28 17:58 [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write Andrew Lunn
@ 2020-04-28 18:01 ` Andrew Lunn
2020-04-29 1:39 ` [EXT] " Andy Duan
2020-04-28 21:33 ` David Miller
2020-04-29 1:53 ` [EXT] " Andy Duan
2 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2020-04-28 18:01 UTC (permalink / raw)
To: Andy Duan; +Cc: netdev, Chris Healy, Andy Duan, Leonard Crestez
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-28 17:58 [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write Andrew Lunn
2020-04-28 18:01 ` Andrew Lunn
@ 2020-04-28 21:33 ` David Miller
2020-04-29 1:55 ` [EXT] " Andy Duan
2020-04-29 1:53 ` [EXT] " Andy Duan
2 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-04-28 21:33 UTC (permalink / raw)
To: andrew; +Cc: netdev, cphealy, fugang.duan, leonard.crestez
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-28 18:01 ` Andrew Lunn
@ 2020-04-29 1:39 ` Andy Duan
0 siblings, 0 replies; 10+ messages in thread
From: Andy Duan @ 2020-04-29 1:39 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, Chris Healy, Leonard Crestez
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-28 17:58 [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write Andrew Lunn
2020-04-28 18:01 ` Andrew Lunn
2020-04-28 21:33 ` David Miller
@ 2020-04-29 1:53 ` Andy Duan
2 siblings, 0 replies; 10+ messages in thread
From: Andy Duan @ 2020-04-29 1:53 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev, Chris Healy, Leonard Crestez
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-28 21:33 ` David Miller
@ 2020-04-29 1:55 ` Andy Duan
2020-04-29 3:34 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Andy Duan @ 2020-04-29 1:55 UTC (permalink / raw)
To: David Miller, andrew; +Cc: netdev, cphealy, Leonard Crestez
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-29 1:55 ` [EXT] " Andy Duan
@ 2020-04-29 3:34 ` David Miller
2020-04-29 3:43 ` Andy Duan
0 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-04-29 3:34 UTC (permalink / raw)
To: fugang.duan; +Cc: andrew, netdev, cphealy, leonard.crestez
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-29 3:34 ` David Miller
@ 2020-04-29 3:43 ` Andy Duan
2020-04-29 14:11 ` Andrew Lunn
0 siblings, 1 reply; 10+ messages in thread
From: Andy Duan @ 2020-04-29 3:43 UTC (permalink / raw)
To: David Miller; +Cc: andrew, netdev, cphealy, Leonard Crestez
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-29 3:43 ` Andy Duan
@ 2020-04-29 14:11 ` Andrew Lunn
2020-04-29 19:16 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2020-04-29 14:11 UTC (permalink / raw)
To: Andy Duan; +Cc: David Miller, netdev, cphealy, Leonard Crestez
> > >> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] Re: [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write
2020-04-29 14:11 ` Andrew Lunn
@ 2020-04-29 19:16 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-04-29 19:16 UTC (permalink / raw)
To: andrew; +Cc: fugang.duan, netdev, cphealy, leonard.crestez
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-29 19:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28 17:58 [PATCH net-next] net: ethernet: fec: Prevent MII event after MII_SPEED write Andrew Lunn
2020-04-28 18:01 ` Andrew Lunn
2020-04-29 1:39 ` [EXT] " Andy Duan
2020-04-28 21:33 ` David Miller
2020-04-29 1:55 ` [EXT] " Andy Duan
2020-04-29 3:34 ` David Miller
2020-04-29 3:43 ` Andy Duan
2020-04-29 14:11 ` Andrew Lunn
2020-04-29 19:16 ` David Miller
2020-04-29 1:53 ` [EXT] " Andy Duan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).