From: Andrew Lunn <and...@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.d...@nxp.com>
> Suggested-by: Andy Duan <fugang.d...@nxp.com>
> Signed-off-by: Andrew Lunn <and...@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
