[PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Aminuddin Jamaluddin]

Add link status checking in m88e1510_loopback() for 1Gbps link speed
and delay for 100ms after phy loopback bit is set before send packet.
This is needed to ensure the stability and consistency when running
the phy loopback test.

Fixes: 020a45aff119 ("net: phy: marvell: add Marvell specific PHY loopback")
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Signed-off-by: Aminuddin Jamaluddin <aminuddin.jamaluddin@intel.com>
---
 drivers/net/phy/marvell.c | 22 ++++++++++++++++------
 include/linux/phy.h       |  3 +++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index a714150f5e8c..17403acf780d 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1992,6 +1992,7 @@ static int m88e1510_loopback(struct phy_device *phydev, bool enable)
 
 	if (enable) {
 		u16 bmcr_ctl, mscr2_ctl = 0;
+		int val = 0;
 
 		bmcr_ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
@@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct phy_device *phydev, bool enable)
 		if (err < 0)
 			return err;
 
-		/* FIXME: Based on trial and error test, it seem 1G need to have
-		 * delay between soft reset and loopback enablement.
-		 */
-		if (phydev->speed == SPEED_1000)
-			msleep(1000);
+		if (phydev->speed == SPEED_1000) {
+			err = phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS,
+						    PHY_LOOP_BACK_SLEEP,
+						    PHY_LOOP_BACK_TIMEOUT, true);
+			if (err)
+				return err;
+		}
 
-		return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
+		err =  phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
 				  BMCR_LOOPBACK);
+		if (!err) {
+			/* It takes some time for PHY device to switch
+			 * into/out-of loopback mode.
+			 */
+			msleep(100);
+		}
+		return err;
 	} else {
 		err = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
 		if (err < 0)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 87638c55d844..b4da968da8e6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -276,6 +276,9 @@ static inline const char *phy_modes(phy_interface_t interface)
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_FORCE_TIMEOUT	10
 
+#define PHY_LOOP_BACK_SLEEP	1000000
+#define PHY_LOOP_BACK_TIMEOUT	8000000
+
 #define PHY_MAX_ADDR	32
 
 /* Used when trying to connect to a specific phy (mii bus id:phy device id) */
-- 
2.17.1

Re: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Andrew Lunn]

> @@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct phy_device *phydev, bool enable)
>  		if (err < 0)
>  			return err;
>  
> -		/* FIXME: Based on trial and error test, it seem 1G need to have
> -		 * delay between soft reset and loopback enablement.
> -		 */
> -		if (phydev->speed == SPEED_1000)
> -			msleep(1000);
> +		if (phydev->speed == SPEED_1000) {
> +			err = phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS,
> +						    PHY_LOOP_BACK_SLEEP,
> +						    PHY_LOOP_BACK_TIMEOUT, true);

Is this link with itself?

Have you tested this with the cable unplugged?

> +			if (err)
> +				return err;

I'm just trying to ensure we don't end up here with -ETIMEDOUT.

>  
> +#define PHY_LOOP_BACK_SLEEP	1000000
> +#define PHY_LOOP_BACK_TIMEOUT	8000000

The kernel seems to be pretty consistent in having loopback as one
word.

	Andrew

RE: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Jamaluddin, Aminuddin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, 25 August, 2022 9:27 PM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min
> <tee.min.tan@intel.com>; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>
> Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
> enabling phy loopback
> 
> > @@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct
> phy_device *phydev, bool enable)
> >  		if (err < 0)
> >  			return err;
> >
> > -		/* FIXME: Based on trial and error test, it seem 1G need to
> have
> > -		 * delay between soft reset and loopback enablement.
> > -		 */
> > -		if (phydev->speed == SPEED_1000)
> > -			msleep(1000);
> > +		if (phydev->speed == SPEED_1000) {
> > +			err = phy_read_poll_timeout(phydev, MII_BMSR,
> val, val & BMSR_LSTATUS,
> > +						    PHY_LOOP_BACK_SLEEP,
> > +
> PHY_LOOP_BACK_TIMEOUT, true);
> 
> Is this link with itself?
 
Its required cabled plug in, back to back connection.

> 
> Have you tested this with the cable unplugged?

Yes we have and its expected to have the timeout. But the self-test required the link
to be up first before it can be run.

> 
> > +			if (err)
> > +				return err;
> 
> I'm just trying to ensure we don't end up here with -ETIMEDOUT.
> 
> >
> > +#define PHY_LOOP_BACK_SLEEP	1000000
> > +#define PHY_LOOP_BACK_TIMEOUT	8000000
> 
> The kernel seems to be pretty consistent in having loopback as one word.

Noted will update in v2.

> 
> 	Andrew

Re: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Andrew Lunn]

> > > @@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct
> > phy_device *phydev, bool enable)
> > >  		if (err < 0)
> > >  			return err;
> > >
> > > -		/* FIXME: Based on trial and error test, it seem 1G need to
> > have
> > > -		 * delay between soft reset and loopback enablement.
> > > -		 */
> > > -		if (phydev->speed == SPEED_1000)
> > > -			msleep(1000);
> > > +		if (phydev->speed == SPEED_1000) {
> > > +			err = phy_read_poll_timeout(phydev, MII_BMSR,
> > val, val & BMSR_LSTATUS,
> > > +						    PHY_LOOP_BACK_SLEEP,
> > > +
> > PHY_LOOP_BACK_TIMEOUT, true);
> > 
> > Is this link with itself?
>  
> Its required cabled plug in, back to back connection.

Loopback should not require that. The whole point of loopback in the
PHY is you can do it without needing a cable.

> > 
> > Have you tested this with the cable unplugged?
> 
> Yes we have and its expected to have the timeout. But the self-test required the link
> to be up first before it can be run.

So you get an ETIMEDOUT, and then skip the code which actually sets
the LOOPBACK bit?

Please look at this again, and make it work without a cable.

Maybe you are addressing the wrong issue? Is the PHY actually
performing loopback, but reporting the link is down? Maybe you need to
fake a link up? Maybe you need the self test to not care about the
link state, all it really needs is that packets get looped?

       Andrew

RE: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Jamaluddin, Aminuddin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, 30 August, 2022 8:17 PM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min
> <tee.min.tan@intel.com>; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>
> Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
> enabling phy loopback
> 
> > > > @@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct
> > > phy_device *phydev, bool enable)
> > > >  		if (err < 0)
> > > >  			return err;
> > > >
> > > > -		/* FIXME: Based on trial and error test, it seem 1G need to
> > > have
> > > > -		 * delay between soft reset and loopback enablement.
> > > > -		 */
> > > > -		if (phydev->speed == SPEED_1000)
> > > > -			msleep(1000);
> > > > +		if (phydev->speed == SPEED_1000) {
> > > > +			err = phy_read_poll_timeout(phydev, MII_BMSR,
> > > val, val & BMSR_LSTATUS,
> > > > +						    PHY_LOOP_BACK_SLEEP,
> > > > +
> > > PHY_LOOP_BACK_TIMEOUT, true);
> > >
> > > Is this link with itself?
> >
> > Its required cabled plug in, back to back connection.
> 
> Loopback should not require that. The whole point of loopback in the PHY is
> you can do it without needing a cable.
> 
> > >
> > > Have you tested this with the cable unplugged?
> >
> > Yes we have and its expected to have the timeout. But the self-test
> > required the link to be up first before it can be run.
> 
> So you get an ETIMEDOUT, and then skip the code which actually sets the
> LOOPBACK bit?

If cable unplugged, test result will be displayed as 1. See comments below.

> 
> Please look at this again, and make it work without a cable.

Related to this the flow without cable, what we see in the codes during debugging.
After the phy loopback bit was set.
The test will be run through this __stmmac_test_loopback()
https://elixir.bootlin.com/linux/v5.19.8/source/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c#L320
Here, it will have another set of checking in dev_direct_xmit(), __dev_direct_xmit().
returning value 1(NET_XMIT_DROP)
https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4288
Which means the interface is not available or the interface link status is not up.
For this case the interface link status is not up. 
Thus failing the phy loopback test.
https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4296
Since we don't own this __stmmac_test_loopback(), we conclude the behaviour was as expected.

> 
> Maybe you are addressing the wrong issue? Is the PHY actually performing
> loopback, but reporting the link is down? Maybe you need to fake a link up?
> Maybe you need the self test to not care about the link state, all it really
> needs is that packets get looped?

When bit 14 was set, the link will be broken. 
But before the self-test was triggered it requires link to be up as stated above comments.

> 
Amin

Re: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Andrew Lunn]

> > > Its required cabled plug in, back to back connection.
> > 
> > Loopback should not require that. The whole point of loopback in the PHY is
> > you can do it without needing a cable.
> > 
> > > >
> > > > Have you tested this with the cable unplugged?
> > >
> > > Yes we have and its expected to have the timeout. But the self-test
> > > required the link to be up first before it can be run.
> > 
> > So you get an ETIMEDOUT, and then skip the code which actually sets the
> > LOOPBACK bit?
> 
> If cable unplugged, test result will be displayed as 1. See comments below.
> 
> > 
> > Please look at this again, and make it work without a cable.
> 
> Related to this the flow without cable, what we see in the codes during debugging.
> After the phy loopback bit was set.
> The test will be run through this __stmmac_test_loopback()
> https://elixir.bootlin.com/linux/v5.19.8/source/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c#L320
> Here, it will have another set of checking in dev_direct_xmit(), __dev_direct_xmit().
> returning value 1(NET_XMIT_DROP)
> https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4288
> Which means the interface is not available or the interface link status is not up.
> For this case the interface link status is not up. 
> Thus failing the phy loopback test.
> https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4296
> Since we don't own this __stmmac_test_loopback(), we conclude the behaviour was as expected.
> 
> > 
> > Maybe you are addressing the wrong issue? Is the PHY actually performing
> > loopback, but reporting the link is down? Maybe you need to fake a link up?
> > Maybe you need the self test to not care about the link state, all it really
> > needs is that packets get looped?
> 
> When bit 14 was set, the link will be broken. 
> But before the self-test was triggered it requires link to be up as stated above comments.

You have not said anything about my comment:

> Maybe you need to fake a link up?

My guess is, some PHYs are going to report link up when put into
loopback. Others might not. For the Marvell PHY, it looks like you
need to make marvell_read_status() return that the link is up if
loopback is enabled.

	 Andrew

RE: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Jamaluddin, Aminuddin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, 20 September, 2022 7:46 AM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min
> <tee.min.tan@intel.com>; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>
> Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
> enabling phy loopback
> 
> > > > Its required cabled plug in, back to back connection.
> > >
> > > Loopback should not require that. The whole point of loopback in the
> > > PHY is you can do it without needing a cable.
> > >
> > > > >
> > > > > Have you tested this with the cable unplugged?
> > > >
> > > > Yes we have and its expected to have the timeout. But the
> > > > self-test required the link to be up first before it can be run.
> > >
> > > So you get an ETIMEDOUT, and then skip the code which actually sets
> > > the LOOPBACK bit?
> >
> > If cable unplugged, test result will be displayed as 1. See comments below.
> >
> > >
> > > Please look at this again, and make it work without a cable.
> >
> > Related to this the flow without cable, what we see in the codes during
> debugging.
> > After the phy loopback bit was set.
> > The test will be run through this __stmmac_test_loopback()
> > https://elixir.bootlin.com/linux/v5.19.8/source/drivers/net/ethernet/s
> > tmicro/stmmac/stmmac_selftests.c#L320
> > Here, it will have another set of checking in dev_direct_xmit(),
> __dev_direct_xmit().
> > returning value 1(NET_XMIT_DROP)
> > https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4288
> > Which means the interface is not available or the interface link status is not
> up.
> > For this case the interface link status is not up.
> > Thus failing the phy loopback test.
> > https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4296
> > Since we don't own this __stmmac_test_loopback(), we conclude the
> behaviour was as expected.
> >
> > >
> > > Maybe you are addressing the wrong issue? Is the PHY actually
> > > performing loopback, but reporting the link is down? Maybe you need to
> fake a link up?
> > > Maybe you need the self test to not care about the link state, all
> > > it really needs is that packets get looped?
> >
> > When bit 14 was set, the link will be broken.
> > But before the self-test was triggered it requires link to be up as stated
> above comments.
> 
> You have not said anything about my comment:
> 
> > Maybe you need to fake a link up?
> 
> My guess is, some PHYs are going to report link up when put into loopback.
> Others might not. For the Marvell PHY, it looks like you need to make
> marvell_read_status() return that the link is up if loopback is enabled.
> 

We able to do the PHY loopback test, after fake link up without 
cable plugged on as suggested above. 
We will provide version 2 patch with minimum code changes 
without having the status link check.
Only need to increase the msleep(200) to msleep(1000) inside
m88e1510_loopback() function.

Amin