[BUG] net: phy: genphy_loopback: add link speed configuration

[BUG] net: phy: genphy_loopback: add link speed configuration

[Ismail, Mohammad Athari]

Hi Oleksij,

"net: phy: genphy_loopback: add link speed configuration" patch causes Marvell 88E1510 PHY not able to perform PHY loopback using ethtool command (ethtool -t eth0 offline). Below is the error message: 

"Marvell 88E1510 stmmac-3:01: genphy_loopback failed: -110" 

Tested on Intel Elkhart Lake platform (Synopsys Designware QoS MAC and Marvell88E1510 PHY). Kernel version is 5.15.x.

Reverting the patch able to fix the issue.

Thank you.

Regards,
Athari

Re: [BUG] net: phy: genphy_loopback: add link speed configuration

[Andrew Lunn]

On Tue, Dec 14, 2021 at 07:00:37AM +0000, Ismail, Mohammad Athari wrote:
> Hi Oleksij,
> 
> "net: phy: genphy_loopback: add link speed configuration" patch causes Marvell 88E1510 PHY not able to perform PHY loopback using ethtool command (ethtool -t eth0 offline). Below is the error message: 
> 
> "Marvell 88E1510 stmmac-3:01: genphy_loopback failed: -110" 

-110 is ETIMEDOUT. So that points to the phy_read_poll_timeout().

Ah, that points to the fact the Marvell PHYs are odd. You need to
perform a software reset after changing some registers to actually
execute the change.

As a quick test, please could you try:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74d8e1dc125f..b45f3ffc7c7f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2625,6 +2625,10 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 
                phy_modify(phydev, MII_BMCR, ~0, ctl);
 
+               ret = genphy_soft_reset(phydev);
+               if (ret < 0)
+                       return ret;
+
                ret = phy_read_poll_timeout(phydev, MII_BMSR, val,
                                            val & BMSR_LSTATUS,
                                    5000, 500000, true);

If this fixes it for you, the actual fix will be more complex, Marvell
cannot use genphy_loopback, it will need its own implementation.

       Andrew

RE: [BUG] net: phy: genphy_loopback: add link speed configuration

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, December 14, 2021 5:39 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> Wong, Vee Khee <vee.khee.wong@intel.com>
> Subject: Re: [BUG] net: phy: genphy_loopback: add link speed configuration
> 
> On Tue, Dec 14, 2021 at 07:00:37AM +0000, Ismail, Mohammad Athari wrote:
> > Hi Oleksij,
> >
> > "net: phy: genphy_loopback: add link speed configuration" patch causes
> Marvell 88E1510 PHY not able to perform PHY loopback using ethtool
> command (ethtool -t eth0 offline). Below is the error message:
> >
> > "Marvell 88E1510 stmmac-3:01: genphy_loopback failed: -110"
> 
> -110 is ETIMEDOUT. So that points to the phy_read_poll_timeout().
> 
> Ah, that points to the fact the Marvell PHYs are odd. You need to perform a
> software reset after changing some registers to actually execute the change.
> 
> As a quick test, please could you try:
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 74d8e1dc125f..b45f3ffc7c7f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2625,6 +2625,10 @@ int genphy_loopback(struct phy_device *phydev,
> bool enable)
> 
>                 phy_modify(phydev, MII_BMCR, ~0, ctl);
> 
> +               ret = genphy_soft_reset(phydev);
> +               if (ret < 0)
> +                       return ret;
> +
>                 ret = phy_read_poll_timeout(phydev, MII_BMSR, val,
>                                             val & BMSR_LSTATUS,
>                                     5000, 500000, true);
> 
> If this fixes it for you, the actual fix will be more complex, Marvell cannot use
> genphy_loopback, it will need its own implementation.

Thanks for the suggestion. The proposed solution also doesn't work. Still get -110 error.

-Athari-

> 
>        Andrew

Re: [BUG] net: phy: genphy_loopback: add link speed configuration

[Andrew Lunn]

> Thanks for the suggestion. The proposed solution also doesn't work. Still get -110 error.

Please can you trace where this -110 comes from. Am i looking at the
wrong poll call?

Thanks
	Andrew

RE: [BUG] net: phy: genphy_loopback: add link speed configuration

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, December 15, 2021 5:23 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> Wong, Vee Khee <vee.khee.wong@intel.com>
> Subject: Re: [BUG] net: phy: genphy_loopback: add link speed configuration
> 
> > Thanks for the suggestion. The proposed solution also doesn't work. Still
> get -110 error.
> 
> Please can you trace where this -110 comes from. Am i looking at the wrong
> poll call?

I did read the ret value from genphy_soft_reset() and phy_read_poll_timeout().
The -110 came from phy_read_poll_timeout().

-Athari-

> 
> Thanks
> 	Andrew

Re: [BUG] net: phy: genphy_loopback: add link speed configuration

[Andrew Lunn]

> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, December 15, 2021 5:23 PM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> > Wong, Vee Khee <vee.khee.wong@intel.com>
> > Subject: Re: [BUG] net: phy: genphy_loopback: add link speed configuration
> > 
> > > Thanks for the suggestion. The proposed solution also doesn't work. Still
> > get -110 error.
> > 
> > Please can you trace where this -110 comes from. Am i looking at the wrong
> > poll call?
> 
> I did read the ret value from genphy_soft_reset() and phy_read_poll_timeout().
> The -110 came from phy_read_poll_timeout().

O.K.

Does the PHY actually do loopback, despite the -110?

I'm wondering if we should ignore the return value from
phy_read_poll_timeout().

	Andrew

RE: [BUG] net: phy: genphy_loopback: add link speed configuration

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, December 15, 2021 5:55 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> Wong, Vee Khee <vee.khee.wong@intel.com>
> Subject: Re: [BUG] net: phy: genphy_loopback: add link speed configuration
> 
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Wednesday, December 15, 2021 5:23 PM
> > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>;
> > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Voon, Weifeng
> > > <weifeng.voon@intel.com>; Wong, Vee Khee
> <vee.khee.wong@intel.com>
> > > Subject: Re: [BUG] net: phy: genphy_loopback: add link speed
> > > configuration
> > >
> > > > Thanks for the suggestion. The proposed solution also doesn't
> > > > work. Still
> > > get -110 error.
> > >
> > > Please can you trace where this -110 comes from. Am i looking at the
> > > wrong poll call?
> >
> > I did read the ret value from genphy_soft_reset() and
> phy_read_poll_timeout().
> > The -110 came from phy_read_poll_timeout().
> 
> O.K.
> 
> Does the PHY actually do loopback, despite the -110?

As Intel Elkhart Lake is using stmmac driver, in stmmac_selftest, return value of phy_loopback() is checked as well. If it return -110, the selftest that using PHY loopback will be recorded as -110 (fail).

> 
> I'm wondering if we should ignore the return value from
> phy_read_poll_timeout().

Removing/ignoring the return value from phy_read_poll_timeout() can work. But, the -110 error message will be displayed in dmesg. It is because there is phydev_err() as part of phy_read_poll_timeout() definition.

-Athari-

> 
> 	Andrew

RE: [BUG] net: phy: genphy_loopback: add link speed configuration

[Ismail, Mohammad Athari]

Hi Andrew,

As the current genphy_loopback() is not applicable for Marvell 88E1510 PHY, should we implement Marvell specific PHY loopback function as below?

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 4fcfca4e1702..2a73a959b48b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1932,6 +1932,12 @@ static void marvell_get_stats(struct phy_device *phydev,
                data[i] = marvell_get_stat(phydev, i);
 }
 
+static int marvell_loopback(struct phy_device *phydev, bool enable)
+{
+       return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
+                         enable ? BMCR_LOOPBACK : 0);
+}
+
 static int marvell_vct5_wait_complete(struct phy_device *phydev)
 {
        int i;
@@ -3078,7 +3084,7 @@ static struct phy_driver marvell_drivers[] = {
                .get_sset_count = marvell_get_sset_count,
                .get_strings = marvell_get_strings,
                .get_stats = marvell_get_stats,
-               .set_loopback = genphy_loopback,
+               .set_loopback = marvell_loopback,
                .get_tunable = m88e1011_get_tunable,
                .set_tunable = m88e1011_set_tunable,
                .cable_test_start = marvell_vct7_cable_test_start,

-Athari-

> -----Original Message-----
> From: Ismail, Mohammad Athari
> Sent: Wednesday, December 15, 2021 11:04 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> Wong, Vee Khee <Vee.Khee.Wong@intel.com>
> Subject: RE: [BUG] net: phy: genphy_loopback: add link speed configuration
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Wednesday, December 15, 2021 5:55 PM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Voon, Weifeng <weifeng.voon@intel.com>;
> > Wong, Vee Khee <vee.khee.wong@intel.com>
> > Subject: Re: [BUG] net: phy: genphy_loopback: add link speed
> > configuration
> >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@lunn.ch>
> > > > Sent: Wednesday, December 15, 2021 5:23 PM
> > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>;
> > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Voon,
> > > > Weifeng <weifeng.voon@intel.com>; Wong, Vee Khee
> > <vee.khee.wong@intel.com>
> > > > Subject: Re: [BUG] net: phy: genphy_loopback: add link speed
> > > > configuration
> > > >
> > > > > Thanks for the suggestion. The proposed solution also doesn't
> > > > > work. Still
> > > > get -110 error.
> > > >
> > > > Please can you trace where this -110 comes from. Am i looking at
> > > > the wrong poll call?
> > >
> > > I did read the ret value from genphy_soft_reset() and
> > phy_read_poll_timeout().
> > > The -110 came from phy_read_poll_timeout().
> >
> > O.K.
> >
> > Does the PHY actually do loopback, despite the -110?
> 
> As Intel Elkhart Lake is using stmmac driver, in stmmac_selftest, return value
> of phy_loopback() is checked as well. If it return -110, the selftest that using
> PHY loopback will be recorded as -110 (fail).
> 
> >
> > I'm wondering if we should ignore the return value from
> > phy_read_poll_timeout().
> 
> Removing/ignoring the return value from phy_read_poll_timeout() can work.
> But, the -110 error message will be displayed in dmesg. It is because there is
> phydev_err() as part of phy_read_poll_timeout() definition.
> 
> -Athari-
> 
> >
> > 	Andrew

Re: [BUG] net: phy: genphy_loopback: add link speed configuration

[Andrew Lunn]

On Mon, Dec 20, 2021 at 11:11:32AM +0000, Ismail, Mohammad Athari wrote:
> Hi Andrew,
> 

> As the current genphy_loopback() is not applicable for Marvell
> 88E1510 PHY, should we implement Marvell specific PHY loopback
> function as below?

Yes, that is probably a good solution. We will have to see if other
PHY drivers need this as well. If they do, we might move this simple
implementation into the core. But for the moment, it can be in the
Marvell driver.

	Andrew