[PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Uwe Kleine-König]

For some mii transfers the MII bit in the event register is already set
before a read or write transfer is started. This breaks evaluating the
transfer's result because it is checked too early.

Before MII transfers were switched from irq to polling this was not an
issue because then it just resulted in an irq which completed the
mdio_done completion. This completion however was reset before each
transfer and so the event didn't hurt.

This fixes NFS booting on an i.MX25 based machine.

Fixes: f166f890c8f0 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I tried (shortly) to find out what actually results in this bit being
set because looking at f166f890c8f0 I'd say it cares enough. It's just
proven by the real world that it's not good enough :-)

Best regards
Uwe

 drivers/net/ethernet/freescale/fec_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2e209142f2d1..ab21d2bcda75 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1869,6 +1869,8 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		frame_addr = regnum;
 	}
 
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+
 	/* start a read op */
 	writel(frame_start | frame_op |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
@@ -1926,6 +1928,8 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 		frame_addr = regnum;
 	}
 
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+
 	/* start a write op */
 	writel(frame_start | FEC_MMFR_OP_WRITE |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(frame_addr) |
-- 
2.20.1

Re: [PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Andrew Lunn]

On Wed, Dec 09, 2020 at 11:29:59AM +0100, Uwe Kleine-König wrote:
> For some mii transfers the MII bit in the event register is already set
> before a read or write transfer is started. This breaks evaluating the
> transfer's result because it is checked too early.
> 
> Before MII transfers were switched from irq to polling this was not an
> issue because then it just resulted in an irq which completed the
> mdio_done completion. This completion however was reset before each
> transfer and so the event didn't hurt.
> 
> This fixes NFS booting on an i.MX25 based machine.
> 
> Fixes: f166f890c8f0 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I tried (shortly) to find out what actually results in this bit being
> set because looking at f166f890c8f0 I'd say it cares enough. It's just
> proven by the real world that it's not good enough :-)
> 
> Best regards
> Uwe

Hi Uwe

Humm. This should of been fixed already. Has a patch been dropped?

Let me go look at the history.

    Andrew

Re: [PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Andrew Lunn]

On Wed, Dec 09, 2020 at 11:29:59AM +0100, Uwe Kleine-König wrote:
> For some mii transfers the MII bit in the event register is already set
> before a read or write transfer is started. This breaks evaluating the
> transfer's result because it is checked too early.
> 
> Before MII transfers were switched from irq to polling this was not an
> issue because then it just resulted in an irq which completed the
> mdio_done completion. This completion however was reset before each
> transfer and so the event didn't hurt.
> 
> This fixes NFS booting on an i.MX25 based machine.
> 
> Fixes: f166f890c8f0 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I tried (shortly) to find out what actually results in this bit being
> set because looking at f166f890c8f0 I'd say it cares enough. It's just
> proven by the real world that it's not good enough :-)

Hi Uwe

Do you have

ommit 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc
Author: Greg Ungerer <gerg@linux-m68k.org>
Date:   Wed Oct 28 15:22:32 2020 +1000

    net: fec: fix MDIO probing for some FEC hardware blocks
    
    Some (apparently older) versions of the FEC hardware block do not like
    the MMFR register being cleared to avoid generation of MII events at
    initialization time. The action of clearing this register results in no
    future MII events being generated at all on the problem block. This means
    the probing of the MDIO bus will find no PHYs.
    
    Create a quirk that can be checked at the FECs MII init time so that
    the right thing is done. The quirk is set as appropriate for the FEC
    hardware blocks that are known to need this.

in your tree?

   Andrew

Re: [PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

Hi Andrew,

On Wed, Dec 09, 2020 at 03:44:13PM +0100, Andrew Lunn wrote:
> On Wed, Dec 09, 2020 at 11:29:59AM +0100, Uwe Kleine-König wrote:
> Do you have
> 
> ommit 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc
> Author: Greg Ungerer <gerg@linux-m68k.org>
> Date:   Wed Oct 28 15:22:32 2020 +1000
> 
>     net: fec: fix MDIO probing for some FEC hardware blocks
>     
>     Some (apparently older) versions of the FEC hardware block do not like
>     the MMFR register being cleared to avoid generation of MII events at
>     initialization time. The action of clearing this register results in no
>     future MII events being generated at all on the problem block. This means
>     the probing of the MDIO bus will find no PHYs.
>     
>     Create a quirk that can be checked at the FECs MII init time so that
>     the right thing is done. The quirk is set as appropriate for the FEC
>     hardware blocks that are known to need this.
> 
> in your tree?

Unless I did something wrong I also saw the failure with v5.10-rc$latest
earlier today.

... some time later ...

Argh, I checked my git reflog and the newest release I tested was
5.9-rc8.

I wonder if my patch is a simpler and more straight forward fix for the
problem however, but that might also be because I don't understand the
comment touched by 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc without
checking the reference manual (which I didn't).

@Marian: As it's you who has to work on this i.MX25 machine, can you
maybe test if using a kernel > 5.10-rc3 (or cherry-picking
1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc) fixes the problem for you?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Marian Cichy]

On 12/9/20 5:51 PM, Uwe Kleine-König wrote:
> Hi Andrew,
>
> On Wed, Dec 09, 2020 at 03:44:13PM +0100, Andrew Lunn wrote:
>> On Wed, Dec 09, 2020 at 11:29:59AM +0100, Uwe Kleine-König wrote:
>> Do you have
>>
>> ommit 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc
>> Author: Greg Ungerer <gerg@linux-m68k.org>
>> Date:   Wed Oct 28 15:22:32 2020 +1000
>>
>>      net: fec: fix MDIO probing for some FEC hardware blocks
>>      
>>      Some (apparently older) versions of the FEC hardware block do not like
>>      the MMFR register being cleared to avoid generation of MII events at
>>      initialization time. The action of clearing this register results in no
>>      future MII events being generated at all on the problem block. This means
>>      the probing of the MDIO bus will find no PHYs.
>>      
>>      Create a quirk that can be checked at the FECs MII init time so that
>>      the right thing is done. The quirk is set as appropriate for the FEC
>>      hardware blocks that are known to need this.
>>
>> in your tree?
> Unless I did something wrong I also saw the failure with v5.10-rc$latest
> earlier today.
>
> ... some time later ...
>
> Argh, I checked my git reflog and the newest release I tested was
> 5.9-rc8.
>
> I wonder if my patch is a simpler and more straight forward fix for the
> problem however, but that might also be because I don't understand the
> comment touched by 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc without
> checking the reference manual (which I didn't).
>
> @Marian: As it's you who has to work on this i.MX25 machine, can you
> maybe test if using a kernel > 5.10-rc3 (or cherry-picking
> 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc) fixes the problem for you?

Tested it on 5.10-rc7 and the problem is fixed without your previous patch.

Best regards,
Marian

>
> Best regards
> Uwe
>

Re: [PATCH] net: ethernet: fec: Clear stale flag in IEVENT register before MII transfers

[Andrew Lunn]

> > @Marian: As it's you who has to work on this i.MX25 machine, can you
> > maybe test if using a kernel > 5.10-rc3 (or cherry-picking
> > 1e6114f51f9d4090390fcec2f5d67d8cc8dc4bfc) fixes the problem for you?
> 
> Tested it on 5.10-rc7 and the problem is fixed without your previous patch.

Hi Marian

Thanks for testing. It keeps surprising me how much breakage this
change i made caused :-(

       Andrew