diff mbox series

[net-next,v2] net: phy: marvell: add sleep time after enabling the loopback bit

Message ID 20221108074005.28229-1-aminuddin.jamaluddin@intel.com
State New
Headers
Series [net-next,v2] net: phy: marvell: add sleep time after enabling the loopback bit |

Commit Message

Aminuddin Jamaluddin Nov. 8, 2022, 7:40 a.m. UTC 
  Sleep time is added to ensure the phy to be ready after loopback
bit was set. This to prevent the phy loopback test from failing.

---
V1: https://patchwork.kernel.org/project/netdevbpf/patch/20220825082238.11056-1-aminuddin.jamaluddin@intel.com/
---

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 | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Jakub Kicinski Nov. 10, 2022, 2:41 a.m. UTC | #1 
On Tue,  8 Nov 2022 15:40:05 +0800 Aminuddin Jamaluddin wrote:
> Subject: [PATCH net-next v2] net: phy: marvell: add sleep time after enabling the loopback bit

Looks like v1 was tagged for net, why switch to net-next?
It's either a fix or not, we don't do gray scales in netdev.

> Sleep time is added to ensure the phy to be ready after loopback
> bit was set. This to prevent the phy loopback test from failing.
> 
> ---
> V1: https://patchwork.kernel.org/project/netdevbpf/patch/20220825082238.11056-1-aminuddin.jamaluddin@intel.com/
> ---

git am will cut off at the first --- it finds, so the v1 link and all
the tags below we'll be lost when the patch is applied. Please move 
this section after the tags.

> 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>
Florian Fainelli Nov. 10, 2022, 3:54 a.m. UTC | #2 
On 11/7/2022 11:40 PM, Aminuddin Jamaluddin wrote:
> Sleep time is added to ensure the phy to be ready after loopback
> bit was set. This to prevent the phy loopback test from failing.
> 
> ---
> V1: https://patchwork.kernel.org/project/netdevbpf/patch/20220825082238.11056-1-aminuddin.jamaluddin@intel.com/
> ---
> 
> 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 | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index a3e810705ce2..860610ba4d00 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2015,14 +2015,16 @@ 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);
> +		err = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> +				 BMCR_LOOPBACK);
>   
> -		return 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(1000);

Is not there a better indication than waiting a full second to ensure 
the PHY exited loopback?
Aminuddin Jamaluddin Nov. 11, 2022, 3:40 a.m. UTC | #3 
> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Thursday, 10 November, 2022 11:54 AM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com>; Andrew
> Lunn <andrew@lunn.ch>; 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>
> Cc: 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>; Looi, Hong
> Aun <hong.aun.looi@intel.com>
> Subject: Re: [PATCH net-next v2] net: phy: marvell: add sleep time after
> enabling the loopback bit
> 
> 
> 
> On 11/7/2022 11:40 PM, Aminuddin Jamaluddin wrote:
> > Sleep time is added to ensure the phy to be ready after loopback bit
> > was set. This to prevent the phy loopback test from failing.
> >
> > ---
> > V1:
> >
> https://patchwork.kernel.org/project/netdevbpf/patch/20220825082238.11
> > 056-1-aminuddin.jamaluddin@intel.com/
> > ---
> >
> > 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 | 16 +++++++++-------
> >   1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index a3e810705ce2..860610ba4d00 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -2015,14 +2015,16 @@ 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);
> > +		err = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> > +				 BMCR_LOOPBACK);
> >
> > -		return 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(1000);
> 
> Is not there a better indication than waiting a full second to ensure the PHY
> exited loopback?

Previously we implemented the link status check that waiting phy to be ready
before the loopback bit being set in V1. But we removed it due to simpler
behaviour as that can be achieve with this. And previous discussion with Marvell
stated the delay timing was needed after loopback bit is set. 

> --

Amin
Aminuddin Jamaluddin Nov. 11, 2022, 3:43 a.m. UTC | #4 
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, 10 November, 2022 10:42 AM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S .
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> 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>; Looi, Hong Aun
> <hong.aun.looi@intel.com>
> Subject: Re: [PATCH net-next v2] net: phy: marvell: add sleep time after
> enabling the loopback bit
> 
> On Tue,  8 Nov 2022 15:40:05 +0800 Aminuddin Jamaluddin wrote:
> > Subject: [PATCH net-next v2] net: phy: marvell: add sleep time after
> > enabling the loopback bit
> 
> Looks like v1 was tagged for net, why switch to net-next?
> It's either a fix or not, we don't do gray scales in netdev.
> 
> > Sleep time is added to ensure the phy to be ready after loopback bit
> > was set. This to prevent the phy loopback test from failing.
> >
> > ---
> > V1:
> >
> https://patchwork.kernel.org/project/netdevbpf/patch/20220825082238.11
> > 056-1-aminuddin.jamaluddin@intel.com/
> > ---
> 
> git am will cut off at the first --- it finds, so the v1 link and all the tags below
> we'll be lost when the patch is applied. Please move this section after the
> tags.
> 

Ok noted will correct this with V3

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

Amin
diff mbox series

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index a3e810705ce2..860610ba4d00 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2015,14 +2015,16 @@  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);
+		err = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
+				 BMCR_LOOPBACK);
 
-		return 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(1000);
+		}
+		return err;
 	} else {
 		err = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
 		if (err < 0)