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@xxxxxxx>
> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Voon, Weifeng <weifeng.voon@xxxxxxxxx>;
> Wong, Vee Khee <Vee.Khee.Wong@xxxxxxxxx>
> Subject: RE: [BUG] net: phy: genphy_loopback: add link speed configuration
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Wednesday, December 15, 2021 5:55 PM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; Voon, Weifeng <weifeng.voon@xxxxxxxxx>;
> > Wong, Vee Khee <vee.khee.wong@xxxxxxxxx>
> > Subject: Re: [BUG] net: phy: genphy_loopback: add link speed
> > configuration
> >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@xxxxxxx>
> > > > Sent: Wednesday, December 15, 2021 5:23 PM
> > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> > > > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>;
> > > > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Voon,
> > > > Weifeng <weifeng.voon@xxxxxxxxx>; Wong, Vee Khee
> > <vee.khee.wong@xxxxxxxxx>
> > > > 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