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: [PATCH] net: ethernet: fec: Replace interrupt driven MDIO with polled IO

[Andrew Lunn]

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	

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

[Leonard Crestez]

On 2020-04-27 7:46 PM, Andrew Lunn wrote:
> 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";
> 
> 
> There is an interesting post from Fabio Estevam
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fu-boot.denx.narkive.com%2FPlutD3Rg%2Fpatch-1-3-phy-atheros-use-ar8035-config-for-ar8031&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6e84d8adeb644c51a86408d7eaca858d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637236027867051637&amp;sdata=s7825L%2BHQV%2FmPR2saYHcoSAgVTlZDr5gYT62kVgdeJA%3D&amp;reserved=0
> 
> Thanks
> 	Andrew
> 

Does not help. 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.

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);
+               }
+       }
+
         if (is_c45) {
                 frame_start = FEC_MMFR_ST_C45;

                 /* write address */
                 frame_addr = (regnum >> 16);


--
Regards,
Leonard

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

[Fabio Estevam]

Hi Andrew,

On Mon, Apr 27, 2020 at 1:44 PM Andrew Lunn <andrew@lunn.ch> wrote:

> You PHY is an ATH8031? If i'm reading these IDs correctly?

Yes, the imx8mm-evk has an AR8031.

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

[Andrew Lunn]

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";


There is an interesting post from Fabio Estevam

https://u-boot.denx.narkive.com/PlutD3Rg/patch-1-3-phy-atheros-use-ar8035-config-for-ar8031

Thanks
	Andrew

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

[Andrew Lunn]

On Mon, Apr 27, 2020 at 03:19:54PM +0000, Leonard Crestez wrote:

Hi Leanard

> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0xd074

You PHY is an ATH8031? If i'm reading these IDs correctly?

Thanks
	Andrew

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

[Andrew Lunn]

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.
> 
> Running with tp_printk trace_event=mdio:* shows the phy identifier is 
> not read correctly:
> 
> BAD:
> [    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x0000
> [    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0x0000
> 
> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
> val:0xd074
> 
> I don't have the original patch in my inbox, hope the manual In-Reply-To 
> header is OK.

Hi Leonard

Thanks for you message. You are the first to provide useful
debug information.

Are you using NFS root?

Do you see any ETIMEOUT messages in the bad case?

Thanks
	Andrew

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

[Andy Duan]

From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Monday, April 27, 2020 11:20 PM
> 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.
> 
> Running with tp_printk trace_event=mdio:* shows the phy identifier is not
> read correctly:
> 
> BAD:
> [    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x02
> val:0x0000
> [    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x03
> val:0x0000
> 
> GOOD:
> [    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x02
> val:0x004d
> [    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00
> reg:0x03
> val:0xd074
> 
> I don't have the original patch in my inbox, hope the manual In-Reply-To
> header is OK.
> 
> --
> Regards,
> Leonard

Leonard, I already sent out the revert patch to net mail list today.
Thanks for report this issue.

Regards,
Andy

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

[Leonard Crestez]

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.

Running with tp_printk trace_event=mdio:* shows the phy identifier is 
not read correctly:

BAD:
[    9.058457] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
val:0x0000
[    9.065857] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
val:0x0000

GOOD:
[    9.100485] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x02 
val:0x004d
[    9.108271] mdio_access: 30be0000.ethernet-1 read  phy:0x00 reg:0x03 
val:0xd074

I don't have the original patch in my inbox, hope the manual In-Reply-To 
header is OK.

--
Regards,
Leonard

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

[Andrew Lunn]

On Tue, Apr 14, 2020 at 04:38:30PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Tue, 14 Apr 2020 02:45:51 +0200
> 
> > Measurements of the MDIO bus have shown that driving the MDIO bus
> > using interrupts is slow. Back to back MDIO transactions take about
> > 90uS, with 25uS spent performing the transaction, and the remainder of
> > the time the bus is idle.
> > 
> > Replacing the completion interrupt with polled IO results in back to
> > back transactions of 40uS. The polling loop waiting for the hardware
> > to complete the transaction takes around 27uS. Which suggests
> > interrupt handling has an overhead of 50uS, and polled IO nearly
> > halves this overhead, and doubles the MDIO performance.
> > 
> > Suggested-by: Chris Heally <cphealy@gmail.com>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Where are we with this?
> 
> Andrew, do you intend to submit a version that via iopoll.h does
> cpu relax and usleeps?

Hi David

I do want to test such a version, yes.

  Andrew

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

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 14 Apr 2020 02:45:51 +0200

> Measurements of the MDIO bus have shown that driving the MDIO bus
> using interrupts is slow. Back to back MDIO transactions take about
> 90uS, with 25uS spent performing the transaction, and the remainder of
> the time the bus is idle.
> 
> Replacing the completion interrupt with polled IO results in back to
> back transactions of 40uS. The polling loop waiting for the hardware
> to complete the transaction takes around 27uS. Which suggests
> interrupt handling has an overhead of 50uS, and polled IO nearly
> halves this overhead, and doubles the MDIO performance.
> 
> Suggested-by: Chris Heally <cphealy@gmail.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Where are we with this?

Andrew, do you intend to submit a version that via iopoll.h does
cpu relax and usleeps?

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

[Andrew Lunn]

Measurements of the MDIO bus have shown that driving the MDIO bus
using interrupts is slow. Back to back MDIO transactions take about
90uS, with 25uS spent performing the transaction, and the remainder of
the time the bus is idle.

Replacing the completion interrupt with polled IO results in back to
back transactions of 40uS. The polling loop waiting for the hardware
to complete the transaction takes around 27uS. Which suggests
interrupt handling has an overhead of 50uS, and polled IO nearly
halves this overhead, and doubles the MDIO performance.

Suggested-by: Chris Heally <cphealy@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/freescale/fec.h      |  4 +-
 drivers/net/ethernet/freescale/fec_main.c | 69 ++++++++++++-----------
 2 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bd898f5b4da5..4c8e7f57957a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -376,8 +376,7 @@ struct bufdesc_ex {
 #define FEC_ENET_TS_AVAIL       ((uint)0x00010000)
 #define FEC_ENET_TS_TIMER       ((uint)0x00008000)
 
-#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
-#define FEC_NAPI_IMASK	FEC_ENET_MII
+#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF)
 #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
 /* ENET interrupt coalescing macro define */
@@ -537,7 +536,6 @@ struct fec_enet_private {
 	int	link;
 	int	full_duplex;
 	int	speed;
-	struct	completion mdio_done;
 	int	irq[FEC_IRQ_NUM];
 	bool	bufdesc_ex;
 	int	pause_flag;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c1c267b61647..48ac0a3a4eb0 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -938,8 +938,8 @@ fec_restart(struct net_device *ndev)
 	writel((__force u32)cpu_to_be32(temp_mac[1]),
 	       fep->hwp + FEC_ADDR_HIGH);
 
-	/* Clear any outstanding interrupt. */
-	writel(0xffffffff, fep->hwp + FEC_IEVENT);
+	/* Clear any outstanding interrupt, except MDIO. */
+	writel((0xffffffff & ~FEC_ENET_MII), fep->hwp + FEC_IEVENT);
 
 	fec_enet_bd_init(ndev);
 
@@ -1085,7 +1085,7 @@ fec_restart(struct net_device *ndev)
 	if (fep->link)
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 	else
-		writel(FEC_ENET_MII, fep->hwp + FEC_IMASK);
+		writel(0, fep->hwp + FEC_IMASK);
 
 	/* Init the interrupt coalescing */
 	fec_enet_itr_coal_init(ndev);
@@ -1599,6 +1599,10 @@ fec_enet_interrupt(int irq, void *dev_id)
 	irqreturn_t ret = IRQ_NONE;
 
 	int_events = readl(fep->hwp + FEC_IEVENT);
+
+	/* Don't clear MDIO events, we poll for those */
+	int_events &= ~FEC_ENET_MII;
+
 	writel(int_events, fep->hwp + FEC_IEVENT);
 	fec_enet_collect_events(fep, int_events);
 
@@ -1606,16 +1610,12 @@ fec_enet_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 
 		if (napi_schedule_prep(&fep->napi)) {
-			/* Disable the NAPI interrupts */
-			writel(FEC_NAPI_IMASK, fep->hwp + FEC_IMASK);
+			/* Disable interrupts */
+			writel(0, fep->hwp + FEC_IMASK);
 			__napi_schedule(&fep->napi);
 		}
 	}
 
-	if (int_events & FEC_ENET_MII) {
-		ret = IRQ_HANDLED;
-		complete(&fep->mdio_done);
-	}
 	return ret;
 }
 
@@ -1765,11 +1765,26 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 		phy_print_status(phy_dev);
 }
 
+static int fec_enet_mdio_wait(struct fec_enet_private *fep)
+{
+	int retries = 10000;
+	uint ievent;
+
+	while (retries--) {
+		ievent = readl(fep->hwp + FEC_IEVENT);
+		if (ievent & FEC_ENET_MII) {
+			writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+			return 0;
+		}
+	}
+
+	return -ETIMEDOUT;
+}
+
 static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct fec_enet_private *fep = bus->priv;
 	struct device *dev = &fep->pdev->dev;
-	unsigned long time_left;
 	int ret = 0, frame_start, frame_addr, frame_op;
 	bool is_c45 = !!(regnum & MII_ADDR_C45);
 
@@ -1777,8 +1792,6 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (ret < 0)
 		return ret;
 
-	reinit_completion(&fep->mdio_done);
-
 	if (is_c45) {
 		frame_start = FEC_MMFR_ST_C45;
 
@@ -1790,11 +1803,9 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		       fep->hwp + FEC_MII_DATA);
 
 		/* wait for end of transfer */
-		time_left = wait_for_completion_timeout(&fep->mdio_done,
-				usecs_to_jiffies(FEC_MII_TIMEOUT));
-		if (time_left == 0) {
+		ret = fec_enet_mdio_wait(fep);
+		if (ret) {
 			netdev_err(fep->netdev, "MDIO address write timeout\n");
-			ret = -ETIMEDOUT;
 			goto out;
 		}
 
@@ -1813,11 +1824,9 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
 
 	/* wait for end of transfer */
-	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-	if (time_left == 0) {
+	ret = fec_enet_mdio_wait(fep);
+	if (ret) {
 		netdev_err(fep->netdev, "MDIO read timeout\n");
-		ret = -ETIMEDOUT;
 		goto out;
 	}
 
@@ -1835,7 +1844,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 {
 	struct fec_enet_private *fep = bus->priv;
 	struct device *dev = &fep->pdev->dev;
-	unsigned long time_left;
 	int ret, frame_start, frame_addr;
 	bool is_c45 = !!(regnum & MII_ADDR_C45);
 
@@ -1845,8 +1853,6 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	else
 		ret = 0;
 
-	reinit_completion(&fep->mdio_done);
-
 	if (is_c45) {
 		frame_start = FEC_MMFR_ST_C45;
 
@@ -1858,11 +1864,9 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 		       fep->hwp + FEC_MII_DATA);
 
 		/* wait for end of transfer */
-		time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-		if (time_left == 0) {
+		ret = fec_enet_mdio_wait(fep);
+		if (ret) {
 			netdev_err(fep->netdev, "MDIO address write timeout\n");
-			ret = -ETIMEDOUT;
 			goto out;
 		}
 	} else {
@@ -1878,12 +1882,9 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 		fep->hwp + FEC_MII_DATA);
 
 	/* wait for end of transfer */
-	time_left = wait_for_completion_timeout(&fep->mdio_done,
-			usecs_to_jiffies(FEC_MII_TIMEOUT));
-	if (time_left == 0) {
+	ret = fec_enet_mdio_wait(fep);
+	if (ret)
 		netdev_err(fep->netdev, "MDIO write timeout\n");
-		ret  = -ETIMEDOUT;
-	}
 
 out:
 	pm_runtime_mark_last_busy(dev);
@@ -2079,6 +2080,9 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/* Clear any pending transaction complete indication */
+	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
+
 	fep->mii_bus = mdiobus_alloc();
 	if (fep->mii_bus == NULL) {
 		err = -ENOMEM;
@@ -3583,7 +3587,6 @@ fec_probe(struct platform_device *pdev)
 		fep->irq[i] = irq;
 	}
 
-	init_completion(&fep->mdio_done);
 	ret = fec_enet_mii_init(pdev);
 	if (ret)
 		goto failed_mii_init;
-- 
2.26.0