Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer]

Hi Andrew,

Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt 
driven MDIO with polled IO") breaks the FEC driver on at least one of
the ColdFire platforms (the 5208). Maybe others, that is all I have
tested on so far.

Specifically the driver no longer finds any PHY devices when it probes
the MDIO bus at kernel start time.

I have pinned the problem down to this one specific change in this commit:

> @@ -2143,8 +2142,21 @@ 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);

At least by removing this I get the old behavior back and everything 
works as it did before.

With that write of the FEC_MII_DATA register in place it seems that
subsequent MDIO operations return immediately (that is FEC_IEVENT is
set) - even though it is obvious the MDIO transaction has not completed yet.

Any ideas?

Regards
Greg

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> Hi Andrew,
> 
> Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt driven
> MDIO with polled IO") breaks the FEC driver on at least one of
> the ColdFire platforms (the 5208). Maybe others, that is all I have
> tested on so far.
> 
> Specifically the driver no longer finds any PHY devices when it probes
> the MDIO bus at kernel start time.
> 
> I have pinned the problem down to this one specific change in this commit:
> 
> > @@ -2143,8 +2142,21 @@ 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);
> 
> At least by removing this I get the old behavior back and everything works
> as it did before.
> 
> With that write of the FEC_MII_DATA register in place it seems that
> subsequent MDIO operations return immediately (that is FEC_IEVENT is
> set) - even though it is obvious the MDIO transaction has not completed yet.
> 
> Any ideas?

Hi Greg

This has come up before, but the discussion fizzled out without a
final patch fixing the issue. NXP suggested this

writel(0, fep->hwp + FEC_MII_DATA);

Without it, some other FEC variants break because they do generate an
interrupt at the wrong time causing all following MDIO transactions to
fail.

At the moment, we don't seem to have a clear understanding of the
different FEC versions, and how their MDIO implementations vary.

	  Andrew

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, October 20, 2020 10:40 AM
> On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> > Hi Andrew,
> >
> > Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt
> > driven MDIO with polled IO") breaks the FEC driver on at least one of
> > the ColdFire platforms (the 5208). Maybe others, that is all I have
> > tested on so far.
> >
> > Specifically the driver no longer finds any PHY devices when it probes
> > the MDIO bus at kernel start time.
> >
> > I have pinned the problem down to this one specific change in this commit:
> >
> > > @@ -2143,8 +2142,21 @@ 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);
> >
> > At least by removing this I get the old behavior back and everything
> > works as it did before.
> >
> > With that write of the FEC_MII_DATA register in place it seems that
> > subsequent MDIO operations return immediately (that is FEC_IEVENT is
> > set) - even though it is obvious the MDIO transaction has not completed yet.
> >
> > Any ideas?
> 
> Hi Greg
> 
> This has come up before, but the discussion fizzled out without a final patch
> fixing the issue. NXP suggested this
> 
> writel(0, fep->hwp + FEC_MII_DATA);
> 
> Without it, some other FEC variants break because they do generate an
> interrupt at the wrong time causing all following MDIO transactions to fail.
> 
> At the moment, we don't seem to have a clear understanding of the different
> FEC versions, and how their MDIO implementations vary.
> 
>           Andrew

Andrew, different varants has little different behavior, so the line is required for
Imx6/7/7 platforms but should be removed in imx5 and ColdFire.

As we discuss one solution to resolve the issue, but it bring 30ms latency for kernel boot.

Now, I want to revert the polling mode to original interrupt mode, do you agree ?

Andy

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Chris Healy]

On Mon, Oct 19, 2020 at 8:02 PM Andy Duan <fugang.duan@nxp.com> wrote:
>
> From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, October 20, 2020 10:40 AM
> > On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> > > Hi Andrew,
> > >
> > > Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt
> > > driven MDIO with polled IO") breaks the FEC driver on at least one of
> > > the ColdFire platforms (the 5208). Maybe others, that is all I have
> > > tested on so far.
> > >
> > > Specifically the driver no longer finds any PHY devices when it probes
> > > the MDIO bus at kernel start time.
> > >
> > > I have pinned the problem down to this one specific change in this commit:
> > >
> > > > @@ -2143,8 +2142,21 @@ 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);
> > >
> > > At least by removing this I get the old behavior back and everything
> > > works as it did before.
> > >
> > > With that write of the FEC_MII_DATA register in place it seems that
> > > subsequent MDIO operations return immediately (that is FEC_IEVENT is
> > > set) - even though it is obvious the MDIO transaction has not completed yet.
> > >
> > > Any ideas?
> >
> > Hi Greg
> >
> > This has come up before, but the discussion fizzled out without a final patch
> > fixing the issue. NXP suggested this
> >
> > writel(0, fep->hwp + FEC_MII_DATA);
> >
> > Without it, some other FEC variants break because they do generate an
> > interrupt at the wrong time causing all following MDIO transactions to fail.
> >
> > At the moment, we don't seem to have a clear understanding of the different
> > FEC versions, and how their MDIO implementations vary.
> >
> >           Andrew
>
> Andrew, different varants has little different behavior, so the line is required for
> Imx6/7/7 platforms but should be removed in imx5 and ColdFire.

Do we know which variants of i.MX6 and i.MX7 do and don't need this?
I'm successfully running with polling mode using the i.MX6q, i.MX6qp,
i.MX7d, and Vybrid, all of which benefit from the considerably higher
throughput achieved with polling.  (In all my use cases I'm working
with an Ethernet Switch attached via MDIO.)

>
> As we discuss one solution to resolve the issue, but it bring 30ms latency for kernel boot.
>
> Now, I want to revert the polling mode to original interrupt mode, do you agree ?
>
> Andy

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> Now, I want to revert the polling mode to original interrupt mode, do you agree ?

Hi Andy

I would like to see if fixed, rather than reverted. Lets try to figure
out the quirks needed to handle the different versions for the IP.

    Andrew

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Chris Healy <cphealy@gmail.com> Sent: Tuesday, October 20, 2020 9:07 PM
> On Mon, Oct 19, 2020 at 8:02 PM Andy Duan <fugang.duan@nxp.com> wrote:
> >
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, October 20, 2020
> > 10:40 AM
> > > On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> > > > Hi Andrew,
> > > >
> > > > Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace
> > > > interrupt driven MDIO with polled IO") breaks the FEC driver on at
> > > > least one of the ColdFire platforms (the 5208). Maybe others, that
> > > > is all I have tested on so far.
> > > >
> > > > Specifically the driver no longer finds any PHY devices when it
> > > > probes the MDIO bus at kernel start time.
> > > >
> > > > I have pinned the problem down to this one specific change in this commit:
> > > >
> > > > > @@ -2143,8 +2142,21 @@ 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);
> > > >
> > > > At least by removing this I get the old behavior back and
> > > > everything works as it did before.
> > > >
> > > > With that write of the FEC_MII_DATA register in place it seems
> > > > that subsequent MDIO operations return immediately (that is
> > > > FEC_IEVENT is
> > > > set) - even though it is obvious the MDIO transaction has not completed
> yet.
> > > >
> > > > Any ideas?
> > >
> > > Hi Greg
> > >
> > > This has come up before, but the discussion fizzled out without a
> > > final patch fixing the issue. NXP suggested this
> > >
> > > writel(0, fep->hwp + FEC_MII_DATA);
> > >
> > > Without it, some other FEC variants break because they do generate
> > > an interrupt at the wrong time causing all following MDIO transactions to
> fail.
> > >
> > > At the moment, we don't seem to have a clear understanding of the
> > > different FEC versions, and how their MDIO implementations vary.
> > >
> > >           Andrew
> >
> > Andrew, different varants has little different behavior, so the line
> > is required for
> > Imx6/7/7 platforms but should be removed in imx5 and ColdFire.
> 
> Do we know which variants of i.MX6 and i.MX7 do and don't need this?
> I'm successfully running with polling mode using the i.MX6q, i.MX6qp, i.MX7d,
> and Vybrid, all of which benefit from the considerably higher throughput
> achieved with polling.  (In all my use cases I'm working with an Ethernet Switch
> attached via MDIO.)
> 
I think the old version like imx53 and ColdFire don't need this.
Others like imx6/7/8 series required this.

> >
> > As we discuss one solution to resolve the issue, but it bring 30ms latency for
> kernel boot.
> >
> > Now, I want to revert the polling mode to original interrupt mode, do you
> agree ?
> >
> > Andy

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer]

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

Hi Andrew,

Thanks for the quick response.


On 20/10/20 12:40 pm, Andrew Lunn wrote:
> On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
>> Hi Andrew,
>>
>> Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt driven
>> MDIO with polled IO") breaks the FEC driver on at least one of
>> the ColdFire platforms (the 5208). Maybe others, that is all I have
>> tested on so far.
>>
>> Specifically the driver no longer finds any PHY devices when it probes
>> the MDIO bus at kernel start time.
>>
>> I have pinned the problem down to this one specific change in this commit:
>>
>>> @@ -2143,8 +2142,21 @@ 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);
>>
>> At least by removing this I get the old behavior back and everything works
>> as it did before.
>>
>> With that write of the FEC_MII_DATA register in place it seems that
>> subsequent MDIO operations return immediately (that is FEC_IEVENT is
>> set) - even though it is obvious the MDIO transaction has not completed yet.
>>
>> Any ideas?
> 
> Hi Greg
> 
> This has come up before, but the discussion fizzled out without a
> final patch fixing the issue. NXP suggested this
> 
> writel(0, fep->hwp + FEC_MII_DATA);
> 
> Without it, some other FEC variants break because they do generate an
> interrupt at the wrong time causing all following MDIO transactions to
> fail.
> 
> At the moment, we don't seem to have a clear understanding of the
> different FEC versions, and how their MDIO implementations vary.

Based on Andy and Chris' comments is something like the attached patch
what we need?

Regards
Greg



[-- Attachment #2: 0001-net-fec-fix-MDIO-probing-for-some-FEC-hardware-block.patch --]
[-- Type: text/x-patch, Size: 3935 bytes --]

From 6149e75aca3bd26b158d06e6e6f10dda8fa138de Mon Sep 17 00:00:00 2001
From: Greg Ungerer <gerg@linux-m68k.org>
Date: Wed, 21 Oct 2020 11:35:21 +1000
Subject: [PATCH] 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.

Fixes: f166f890c8f0 ("net: ethernet: fec: Replace interrupt driven MDIO with polled IO")
Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 drivers/net/ethernet/freescale/fec.h      |  6 +++++
 drivers/net/ethernet/freescale/fec_main.c | 27 +++++++++++++----------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 832a2175636d..c527f4ee1d3a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -456,6 +456,12 @@ struct bufdesc_ex {
  */
 #define FEC_QUIRK_HAS_FRREG		(1 << 16)
 
+/* Some FEC hardware blocks need the MMFR cleared at setup time to avoid
+ * the generation of an MII event. This must be avoided in the older
+ * FEC blocks where it will stop MII events being generated.
+ */
+#define FEC_QUIRK_CLEAR_SETUP_MII	(1 << 17)
+
 struct bufdesc_prop {
 	int qid;
 	/* Address of Rx and Tx buffers */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fb37816a74db..e67b60de2f8d 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -107,7 +107,7 @@ static const struct fec_devinfo fec_imx6q_info = {
 	.quirks = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
 		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_ERR006358 |
-		  FEC_QUIRK_HAS_RACC,
+		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_CLEAR_SETUP_MII,
 };
 
 static const struct fec_devinfo fec_mvf600_info = {
@@ -119,7 +119,8 @@ static const struct fec_devinfo fec_imx6x_info = {
 		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
 		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
-		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE,
+		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
+		  FEC_QUIRK_CLEAR_SETUP_MII,
 };
 
 static const struct fec_devinfo fec_imx6ul_info = {
@@ -127,7 +128,7 @@ static const struct fec_devinfo fec_imx6ul_info = {
 		  FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_ERR007885 |
 		  FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_HAS_RACC |
-		  FEC_QUIRK_HAS_COALESCE,
+		  FEC_QUIRK_HAS_COALESCE | FEC_QUIRK_CLEAR_SETUP_MII,
 };
 
 static struct platform_device_id fec_devtype[] = {
@@ -2114,15 +2115,17 @@ 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);
+	if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
+		/* 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);
 
-- 
2.25.1

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Greg Ungerer <gerg@linux-m68k.org> Sent: Wednesday, October 21, 2020 9:52 AM
> Hi Andrew,
> 
> Thanks for the quick response.
> 
> 
> On 20/10/20 12:40 pm, Andrew Lunn wrote:
> > On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
> >> Hi Andrew,
> >>
> >> Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt
> >> driven MDIO with polled IO") breaks the FEC driver on at least one of
> >> the ColdFire platforms (the 5208). Maybe others, that is all I have
> >> tested on so far.
> >>
> >> Specifically the driver no longer finds any PHY devices when it
> >> probes the MDIO bus at kernel start time.
> >>
> >> I have pinned the problem down to this one specific change in this commit:
> >>
> >>> @@ -2143,8 +2142,21 @@ 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);
> >>
> >> At least by removing this I get the old behavior back and everything
> >> works as it did before.
> >>
> >> With that write of the FEC_MII_DATA register in place it seems that
> >> subsequent MDIO operations return immediately (that is FEC_IEVENT is
> >> set) - even though it is obvious the MDIO transaction has not completed yet.
> >>
> >> Any ideas?
> >
> > Hi Greg
> >
> > This has come up before, but the discussion fizzled out without a
> > final patch fixing the issue. NXP suggested this
> >
> > writel(0, fep->hwp + FEC_MII_DATA);
> >
> > Without it, some other FEC variants break because they do generate an
> > interrupt at the wrong time causing all following MDIO transactions to
> > fail.
> >
> > At the moment, we don't seem to have a clear understanding of the
> > different FEC versions, and how their MDIO implementations vary.
> 
> Based on Andy and Chris' comments is something like the attached patch what
> we need?

Greg, imx28 platform also requires the flag.
> 
> Regards
> Greg
> 

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer]

Hi Andy,

On 21/10/20 12:19 pm, Andy Duan wrote:
> From: Greg Ungerer <gerg@linux-m68k.org> Sent: Wednesday, October 21, 2020 9:52 AM
>> Hi Andrew,
>>
>> Thanks for the quick response.
>>
>>
>> On 20/10/20 12:40 pm, Andrew Lunn wrote:
>>> On Tue, Oct 20, 2020 at 12:14:04PM +1000, Greg Ungerer wrote:
>>>> Hi Andrew,
>>>>
>>>> Commit f166f890c8f0 ("[PATCH] net: ethernet: fec: Replace interrupt
>>>> driven MDIO with polled IO") breaks the FEC driver on at least one of
>>>> the ColdFire platforms (the 5208). Maybe others, that is all I have
>>>> tested on so far.
>>>>
>>>> Specifically the driver no longer finds any PHY devices when it
>>>> probes the MDIO bus at kernel start time.
>>>>
>>>> I have pinned the problem down to this one specific change in this commit:
>>>>
>>>>> @@ -2143,8 +2142,21 @@ 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);
>>>>
>>>> At least by removing this I get the old behavior back and everything
>>>> works as it did before.
>>>>
>>>> With that write of the FEC_MII_DATA register in place it seems that
>>>> subsequent MDIO operations return immediately (that is FEC_IEVENT is
>>>> set) - even though it is obvious the MDIO transaction has not completed yet.
>>>>
>>>> Any ideas?
>>>
>>> Hi Greg
>>>
>>> This has come up before, but the discussion fizzled out without a
>>> final patch fixing the issue. NXP suggested this
>>>
>>> writel(0, fep->hwp + FEC_MII_DATA);
>>>
>>> Without it, some other FEC variants break because they do generate an
>>> interrupt at the wrong time causing all following MDIO transactions to
>>> fail.
>>>
>>> At the moment, we don't seem to have a clear understanding of the
>>> different FEC versions, and how their MDIO implementations vary.
>>
>> Based on Andy and Chris' comments is something like the attached patch what
>> we need?
> 
> Greg, imx28 platform also requires the flag.

Got it, thanks. I will update the patch. I won't resend a v2 just yet, 
wait and see if there is any other comments.

Regards
Greg

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> +	if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
> +		/* 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);
> +	}

Hi Greg

The last time we discussed this, we decided that if you cannot do the
quirk, you need to wait around for an MDIO interrupt, e.g. call
fec_enet_mdio_wait() after setting FEC_MII_SPEED register.

>  
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

	Andrew

Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer]

Hi Andrew,

On 21/10/20 11:37 pm, Andrew Lunn wrote:
>> +	if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
>> +		/* 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);
>> +	}
> 
> Hi Greg
> 
> The last time we discussed this, we decided that if you cannot do the
> quirk, you need to wait around for an MDIO interrupt, e.g. call
> fec_enet_mdio_wait() after setting FEC_MII_SPEED register.
> 
>>   
>>   	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

The code following this is:

         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

         /* Clear any pending transaction complete indication */
         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);


So this is forcing a clear of the event here. Is that not good enough?

For me on my ColdFire test target I always get a timeout if I wait for a 
FEC_IEVENT after the FEC_MII_SPEED write.

Regards
Greg

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Greg Ungerer <gerg@linux-m68k.org> Sent: Thursday, October 22, 2020 9:14 AM
> Hi Andrew,
> 
> On 21/10/20 11:37 pm, Andrew Lunn wrote:
> >> +    if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
> >> +            /* 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);
> >> +    }
> >
> > Hi Greg
> >
> > The last time we discussed this, we decided that if you cannot do the
> > quirk, you need to wait around for an MDIO interrupt, e.g. call
> > fec_enet_mdio_wait() after setting FEC_MII_SPEED register.
> >
> >>
> >>      writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> The code following this is:
> 
>          writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>          /* Clear any pending transaction complete indication */
>          writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> 
> 
> So this is forcing a clear of the event here. Is that not good enough?
> 
> For me on my ColdFire test target I always get a timeout if I wait for a
> FEC_IEVENT after the FEC_MII_SPEED write.

It is what I raised in previous mail about 30ms latency for boot. 
> 
> Regards
> Greg

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Greg Ungerer <gerg@linux-m68k.org> Sent: Thursday, October 22, 2020 9:14 AM
> Hi Andrew,
> 
> On 21/10/20 11:37 pm, Andrew Lunn wrote:
> >> +    if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
> >> +            /* 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);
> >> +    }
> >
> > Hi Greg
> >
> > The last time we discussed this, we decided that if you cannot do the
> > quirk, you need to wait around for an MDIO interrupt, e.g. call
> > fec_enet_mdio_wait() after setting FEC_MII_SPEED register.
> >
> >>
> >>      writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
> The code following this is:
> 
>          writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>          /* Clear any pending transaction complete indication */
>          writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> 
> 
> So this is forcing a clear of the event here. Is that not good enough?
> 
> For me on my ColdFire test target I always get a timeout if I wait for a
> FEC_IEVENT after the FEC_MII_SPEED write.
> 
> Regards
> Greg

Dave Karr's last patch can fix the issue, but it may introduce 30ms delay during boot.
Greg's patch is to add quirk flag to distinguish platform before clearing mmfr operation,
which also can fix the issue. 

Andy

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Greg Ungerer]

Hi Andy,

On 22/10/20 7:04 pm, Andy Duan wrote:
> From: Greg Ungerer <gerg@linux-m68k.org> Sent: Thursday, October 22, 2020 9:14 AM
>> Hi Andrew,
>>
>> On 21/10/20 11:37 pm, Andrew Lunn wrote:
>>>> +    if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
>>>> +            /* 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);
>>>> +    }
>>>
>>> Hi Greg
>>>
>>> The last time we discussed this, we decided that if you cannot do the
>>> quirk, you need to wait around for an MDIO interrupt, e.g. call
>>> fec_enet_mdio_wait() after setting FEC_MII_SPEED register.
>>>
>>>>
>>>>       writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>>
>> The code following this is:
>>
>>           writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>>
>>           /* Clear any pending transaction complete indication */
>>           writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>>
>>
>> So this is forcing a clear of the event here. Is that not good enough?
>>
>> For me on my ColdFire test target I always get a timeout if I wait for a
>> FEC_IEVENT after the FEC_MII_SPEED write.
>>
>> Regards
>> Greg
> 
> Dave Karr's last patch can fix the issue, but it may introduce 30ms delay during boot.
> Greg's patch is to add quirk flag to distinguish platform before clearing mmfr operation,
> which also can fix the issue.

Do you mean that we can use either fix - and that is ok for all hardware 
types?

Regards
Greg

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Greg Ungerer <gerg@linux-m68k.org> Sent: Tuesday, October 27, 2020 8:18 AM
> Hi Andy,
> 
> On 22/10/20 7:04 pm, Andy Duan wrote:
> > From: Greg Ungerer <gerg@linux-m68k.org> Sent: Thursday, October 22,
> > 2020 9:14 AM
> >> Hi Andrew,
> >>
> >> On 21/10/20 11:37 pm, Andrew Lunn wrote:
> >>>> +    if (fep->quirks & FEC_QUIRK_CLEAR_SETUP_MII) {
> >>>> +            /* 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);
> >>>> +    }
> >>>
> >>> Hi Greg
> >>>
> >>> The last time we discussed this, we decided that if you cannot do
> >>> the quirk, you need to wait around for an MDIO interrupt, e.g. call
> >>> fec_enet_mdio_wait() after setting FEC_MII_SPEED register.
> >>>
> >>>>
> >>>>       writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> >>
> >> The code following this is:
> >>
> >>           writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> >>
> >>           /* Clear any pending transaction complete indication */
> >>           writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> >>
> >>
> >> So this is forcing a clear of the event here. Is that not good enough?
> >>
> >> For me on my ColdFire test target I always get a timeout if I wait
> >> for a FEC_IEVENT after the FEC_MII_SPEED write.
> >>
> >> Regards
> >> Greg
> >
> > Dave Karr's last patch can fix the issue, but it may introduce 30ms delay during
> boot.
> > Greg's patch is to add quirk flag to distinguish platform before
> > clearing mmfr operation, which also can fix the issue.
> 
> Do you mean that we can use either fix - and that is ok for all hardware types?
> 
> Regards
> Greg

Yes, I think so.

Thanks,
Andy

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> > Hi Andy
> > 
> > Thanks for digging into the internal of the FEC. Just to make sure i understand
> > this correctly:
> > 
> > In fec_enet_mii_init() we have:
> > 
> >         holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000)
> > - 1;
> > 
> >         fep->phy_speed = mii_speed << 1 | holdtime << 8;
> > 
> >         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> > 
> >         /* Clear any pending transaction complete indication */
> >         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> > 
> > You are saying this write to the FEC_MII_SPEED register can on some SoCs
> > trigger an FEC_ENET_MII event. And because it does not happen immediately,
> > it happens after the clear which is performed here?
> 
> Correct.
> Before write FEC_MII_SPEED register, FEC_MII_DATA register is not zero, and
> the current value of FEC_MII_SPEED register is zero, once write non zero value
> to FEC_MII_SPEED register, it trigger MII event.
> 
> > Sometime later we then go into fec_enet_mdio_wait(), the event is still
> > pending, so we read the FEC_MII_DATA register too early?
> 
> Correct.
> The first mdio operation is mdio read, read FEC_MII_DATA register is too early,
> it get invalid value. 
> > 
> > But this does not fully explain the problem. This should only affect the first
> > MDIO transaction, because as we exit fec_enet_mdio_wait() the event is
> > cleared. But Leonard reported that all reads return 0, not just the first.
> 
> Of course, it impact subsequent mdio read/write operations.
> After you clear MII event that is pending before.
> Then, after mdio read data back, MII event is set again.
> 
> cpu instruction is much faster than mdio read/write operation.

Ah. Now i get it....

The flow is...

Write FEC_MII_SPEED register
Clear event register

A little latter

event bit set because of FEC_MII_SPEED

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from FEC_MII_SPEED
Clear FEC_MII_SPEED event
Read data register too early

A little later

event bit set because read complete

A little later
Setup read
fec_enet_mdio_wait()
exit immediately, because of pending event from read complete
Clear previous read complete event
Read data register too early

A little later

event bit set because read complete

And the cycle continues...

I will make a formal patch from your email.

  Andrew

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 9:35 PM
> > Andrew, after investigate the issue, there have one MII event coming
> > later then clearing MII pending event when writing MSCR register
> (MII_SPEED).
> >
> > Check the rtl design by co-working with our IC designer, the 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
> >
> > mmfr[31:0]: current MMFR register value
> > mscr[7:0]: current MSCR register value
> > mscr_reg_data_in[7:0]: the value wrote to MSCR
> >
> >
> > Below patch can fix the block issue:
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -2142,6 +2142,15 @@ 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);
> 
> Hi Andy
> 
> Thanks for digging into the internal of the FEC. Just to make sure i understand
> this correctly:
> 
> In fec_enet_mii_init() we have:
> 
>         holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000)
> - 1;
> 
>         fep->phy_speed = mii_speed << 1 | holdtime << 8;
> 
>         writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> 
>         /* Clear any pending transaction complete indication */
>         writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
> 
> You are saying this write to the FEC_MII_SPEED register can on some SoCs
> trigger an FEC_ENET_MII event. And because it does not happen immediately,
> it happens after the clear which is performed here?

Correct.
Before write FEC_MII_SPEED register, FEC_MII_DATA register is not zero, and
the current value of FEC_MII_SPEED register is zero, once write non zero value
to FEC_MII_SPEED register, it trigger MII event.

> Sometime later we then go into fec_enet_mdio_wait(), the event is still
> pending, so we read the FEC_MII_DATA register too early?

Correct.
The first mdio operation is mdio read, read FEC_MII_DATA register is too early,
it get invalid value. 
> 
> But this does not fully explain the problem. This should only affect the first
> MDIO transaction, because as we exit fec_enet_mdio_wait() the event is
> cleared. But Leonard reported that all reads return 0, not just the first.

Of course, it impact subsequent mdio read/write operations.
After you clear MII event that is pending before.
Then, after mdio read data back, MII event is set again.

cpu instruction is much faster than mdio read/write operation.

> 
>     Andrew

Re: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

> Andrew, after investigate the issue, there have one MII event coming later then
> clearing MII pending event when writing MSCR register (MII_SPEED).
> 
> Check the rtl design by co-working with our IC designer, the 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
> 	
> mmfr[31:0]: current MMFR register value
> mscr[7:0]: current MSCR register value
> mscr_reg_data_in[7:0]: the value wrote to MSCR
> 
> 
> Below patch can fix the block issue:
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2142,6 +2142,15 @@ 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);

Hi Andy

Thanks for digging into the internal of the FEC. Just to make sure i
understand this correctly:

In fec_enet_mii_init() we have:

        holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000) - 1;

        fep->phy_speed = mii_speed << 1 | holdtime << 8;

        writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);

        /* Clear any pending transaction complete indication */
        writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);

You are saying this write to the FEC_MII_SPEED register can on some
SoCs trigger an FEC_ENET_MII event. And because it does not happen
immediately, it happens after the clear which is performed here?
Sometime later we then go into fec_enet_mdio_wait(), the event is
still pending, so we read the FEC_MII_DATA register too early?

But this does not fully explain the problem. This should only affect
the first MDIO transaction, because as we exit fec_enet_mdio_wait()
the event is cleared. But Leonard reported that all reads return 0,
not just the first.

    Andrew

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 4:14 AM
> Hi Leonard
> 
> > Does not help.
> 
> Thanks for testing it.
> 
> > What does seem to help is inserting prints after the FEC_ENET_MII
> > check but that's probably because it inject a long delay equivalent to
> > the long udelay Andy has mentioned.
> 
> Yes, serial ports are slow...
> 
> > I found that in my case FEC_ENET_MII is already set on entry to
> > fec_enet_mdio_read, doesn't this make fec_enet_mdio_wait pointless?
> > Perhaps the problem is that the MII Interrupt pending bit is not
> > cleared. I can fix the problem like this:
> >
> > diff --git drivers/net/ethernet/freescale/fec_main.c
> > drivers/net/ethernet/freescale/fec_main.c
> > index 1ae075a246a3..f1330071647c 100644
> > --- drivers/net/ethernet/freescale/fec_main.c
> > +++ drivers/net/ethernet/freescale/fec_main.c
> > @@ -1841,10 +1841,19 @@ static int fec_enet_mdio_read(struct mii_bus
> > *bus, int mii_id, int regnum)
> >
> >          ret = pm_runtime_get_sync(dev);
> >          if (ret < 0)
> >                  return ret;
> >
> > +       if (1) {
> > +               u32 ievent;
> > +               ievent = readl(fep->hwp + FEC_IEVENT);
> > +               if (ievent & FEC_ENET_MII) {
> > +                       dev_warn(dev, "found FEC_ENET_MII
> pending\n");
> > +                       writel(FEC_ENET_MII, fep->hwp +
> FEC_IEVENT);
> > +               }
> 
> How often do you see this warning?
> 
> The patch which is causing the regression clears any pending events in
> fec_enet_mii_init() and after each time we wait. So the bit should not be set
> here. If it is set, the question is why?
> 
> The other option is that the hardware is broken. It is setting the event bit way
> too soon, before we can actually read the data from the register.
> 
>         Andrew

Andrew, after investigate the issue, there have one MII event coming later then
clearing MII pending event when writing MSCR register (MII_SPEED).

Check the rtl design by co-working with our IC designer, the 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
	
mmfr[31:0]: current MMFR register value
mscr[7:0]: current MSCR register value
mscr_reg_data_in[7:0]: the value wrote to MSCR


Below patch can fix the block issue:
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2142,6 +2142,15 @@ 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);

RE: [EXT] Re: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andy Duan]

From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, April 28, 2020 12:46 AM
> On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:
> > Hello,
> >
> > This patch breaks network boot on at least imx8mm-evk. Boot works if I
> > revert just commit 29ae6bd1b0d8 ("net: ethernet: fec: Replace
> > interrupt driven MDIO with polled IO") on top of next-20200424.
> 
> Hi Leonard
> 
> Please could you try this:
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> index 951e14a3de0e..3c1adaf7affa 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
> @@ -109,6 +109,7 @@ &fec1 {
>         phy-handle = <&ethphy0>;
>         phy-reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
>         phy-reset-duration = <10>;
> +       phy-reset-post-delay = <100>;
>         fsl,magic-packet;
>         status = "okay";
> 
Add the similar change as below on i.MX6SX sdb, it still doesn't work.
As my previous mail, udelay(50) can work.(50us can be optimized)

--- a/arch/arm/boot/dts/imx6sx-sdb.dtsi
+++ b/arch/arm/boot/dts/imx6sx-sdb.dtsi
@@ -194,6 +194,8 @@
        phy-mode = "rgmii-id";
        phy-handle = <&ethphy1>;
        phy-reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
+       phy-reset-duration = <10>;
+       phy-reset-post-delay = <100>;