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

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