-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Thursday, 25 August, 2022 9:27 PM
To: Jamaluddin, Aminuddin <redacted>
Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
[off-list ref]; David S . Miller [off-list ref]; Eric
Dumazet [off-list ref]; Jakub Kicinski [off-list ref];
Paolo Abeni [off-list ref]; Ismail, Mohammad Athari
[off-list ref]; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min
[off-list ref]; Zulkifli, Muhammad Husaini
[off-list ref]
Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
enabling phy loopback

quoted

@@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct

phy_device *phydev, bool enable)

quoted

 		if (err < 0)
 			return err;

-		/* FIXME: Based on trial and error test, it seem 1G need to

have

quoted

-		 * 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,

quoted

+						    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.

quoted

+			if (err)
+				return err;

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

quoted

+#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