[PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

[PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

[Xu Liang]

GPY2xx devices need 3 seconds to fully switch out of loopback mode
before it can safely re-enter loopback mode. Implement timeout mechanism
to guarantee 3 seconds waited before re-enter loopback mode.

Signed-off-by: Xu Liang <lxu@maxlinear.com>
---
v2 changes:
 Update comments.

v3 changes:
 Submit for net-next.

v4 changes:
 Implement timeout mechanism as re-enter loopback mode is quite rare use case.
 The delay is not required to resume normal function.

 drivers/net/phy/mxl-gpy.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index e5972b4ef6e8..7316d6ba0cb5 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -107,6 +107,14 @@ struct gpy_priv {
 
 	u8 fw_major;
 	u8 fw_minor;
+
+	/* It takes 3 seconds to fully switch out of loopback mode before
+	 * it can safely re-enter loopback mode. Record the time when
+	 * loopback is disabled. Check and wait if necessary before loopback
+	 * is enabled.
+	 */
+	bool lb_dis_chk;
+	u64 lb_dis_to;
 };
 
 static const struct {
@@ -769,18 +777,34 @@ static void gpy_get_wol(struct phy_device *phydev,
 
 static int gpy_loopback(struct phy_device *phydev, bool enable)
 {
+	struct gpy_priv *priv = phydev->priv;
+	u16 set = 0;
 	int ret;
 
-	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
-			 enable ? BMCR_LOOPBACK : 0);
-	if (!ret) {
-		/* It takes some time for PHY device to switch
-		 * into/out-of loopback mode.
+	if (enable) {
+		/* wait until 3 seconds from last disable */
+		if (priv->lb_dis_chk && time_is_after_jiffies64(priv->lb_dis_to))
+			msleep(jiffies64_to_msecs(priv->lb_dis_to - get_jiffies_64()));
+
+		priv->lb_dis_chk = false;
+		set = BMCR_LOOPBACK;
+	}
+
+	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, set);
+	if (ret)
+		return ret;
+
+	if (enable) {
+		/* It takes some time for PHY device to switch into
+		 * loopback mode.
 		 */
 		msleep(100);
+	} else {
+		priv->lb_dis_chk = true;
+		priv->lb_dis_to = get_jiffies_64() + HZ * 3;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int gpy115_loopback(struct phy_device *phydev, bool enable)
-- 
2.17.1

Re: [PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

[Andrew Lunn]

> +	/* It takes 3 seconds to fully switch out of loopback mode before
> +	 * it can safely re-enter loopback mode. Record the time when
> +	 * loopback is disabled. Check and wait if necessary before loopback
> +	 * is enabled.
> +	 */

Is there are restriction about entering loopback mode within the first
3 seconds after power on? 

> +	bool lb_dis_chk;
> +	u64 lb_dis_to;
>  };
>  
>  static const struct {
> @@ -769,18 +777,34 @@ static void gpy_get_wol(struct phy_device *phydev,
>  
>  static int gpy_loopback(struct phy_device *phydev, bool enable)
>  {
> +	struct gpy_priv *priv = phydev->priv;
> +	u16 set = 0;
>  	int ret;
>  
> -	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> -			 enable ? BMCR_LOOPBACK : 0);
> -	if (!ret) {
> -		/* It takes some time for PHY device to switch
> -		 * into/out-of loopback mode.
> +	if (enable) {
> +		/* wait until 3 seconds from last disable */
> +		if (priv->lb_dis_chk && time_is_after_jiffies64(priv->lb_dis_to))
> +			msleep(jiffies64_to_msecs(priv->lb_dis_to - get_jiffies_64()));
> +
> +		priv->lb_dis_chk = false;
> +		set = BMCR_LOOPBACK;

Maybe this can be simplified by setting priv->lb_dis_to =
get_jiffies_64() + HZ * 3 in _probe(). Then you don't need
priv->lb_dis_chk.

	Andrew

Re: [PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

[Michael Walle]

Am 2023-03-14 14:50, schrieb Andrew Lunn:
>> +	/* It takes 3 seconds to fully switch out of loopback mode before
>> +	 * it can safely re-enter loopback mode. Record the time when
>> +	 * loopback is disabled. Check and wait if necessary before loopback
>> +	 * is enabled.
>> +	 */
> 
> Is there are restriction about entering loopback mode within the first
> 3 seconds after power on?
> 
>> +	bool lb_dis_chk;
>> +	u64 lb_dis_to;
>>  };
>> 
>>  static const struct {
>> @@ -769,18 +777,34 @@ static void gpy_get_wol(struct phy_device 
>> *phydev,
>> 
>>  static int gpy_loopback(struct phy_device *phydev, bool enable)
>>  {
>> +	struct gpy_priv *priv = phydev->priv;
>> +	u16 set = 0;
>>  	int ret;
>> 
>> -	ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>> -			 enable ? BMCR_LOOPBACK : 0);
>> -	if (!ret) {
>> -		/* It takes some time for PHY device to switch
>> -		 * into/out-of loopback mode.
>> +	if (enable) {
>> +		/* wait until 3 seconds from last disable */
>> +		if (priv->lb_dis_chk && time_is_after_jiffies64(priv->lb_dis_to))
>> +			msleep(jiffies64_to_msecs(priv->lb_dis_to - get_jiffies_64()));
>> +
>> +		priv->lb_dis_chk = false;
>> +		set = BMCR_LOOPBACK;
> 
> Maybe this can be simplified by setting priv->lb_dis_to =
> get_jiffies_64() + HZ * 3 in _probe(). Then you don't need
> priv->lb_dis_chk.

First, I wonder if this is worth the effort and code complications.
phy_loopback() seem to be used very seldom. Anyway.

Can't we just save the jiffies on last enable as kind of a timestamp.
If it's 0 you know it wasn't called yet and if it's set, you have to at
least wait for until it is after "jiffies + HZ*3".

Also isn't that racy right now? "priv->lb_dis_to - get_jiffies_64())" 
can
get negative, no?

-michael

Re: [PATCH net-next v4] net: phy: mxl-gpy: enhance delay time required by loopback disable function

[Liang Xu]

On 14/3/2023 10:04 pm, Michael Walle wrote:
> This email was sent from outside of MaxLinear.
>
>
> Am 2023-03-14 14:50, schrieb Andrew Lunn:
>>> +    /* It takes 3 seconds to fully switch out of loopback mode before
>>> +     * it can safely re-enter loopback mode. Record the time when
>>> +     * loopback is disabled. Check and wait if necessary before 
>>> loopback
>>> +     * is enabled.
>>> +     */
>>
>> Is there are restriction about entering loopback mode within the first
>> 3 seconds after power on?
>>
>>> +    bool lb_dis_chk;
>>> +    u64 lb_dis_to;
>>>  };
>>>
>>>  static const struct {
>>> @@ -769,18 +777,34 @@ static void gpy_get_wol(struct phy_device
>>> *phydev,
>>>
>>>  static int gpy_loopback(struct phy_device *phydev, bool enable)
>>>  {
>>> +    struct gpy_priv *priv = phydev->priv;
>>> +    u16 set = 0;
>>>      int ret;
>>>
>>> -    ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
>>> -                     enable ? BMCR_LOOPBACK : 0);
>>> -    if (!ret) {
>>> -            /* It takes some time for PHY device to switch
>>> -             * into/out-of loopback mode.
>>> +    if (enable) {
>>> +            /* wait until 3 seconds from last disable */
>>> +            if (priv->lb_dis_chk && 
>>> time_is_after_jiffies64(priv->lb_dis_to))
>>> + msleep(jiffies64_to_msecs(priv->lb_dis_to - get_jiffies_64()));
>>> +
>>> +            priv->lb_dis_chk = false;
>>> +            set = BMCR_LOOPBACK;
>>
>> Maybe this can be simplified by setting priv->lb_dis_to =
>> get_jiffies_64() + HZ * 3 in _probe(). Then you don't need
>> priv->lb_dis_chk.
>
> First, I wonder if this is worth the effort and code complications.
> phy_loopback() seem to be used very seldom. Anyway.
>
> Can't we just save the jiffies on last enable as kind of a timestamp.
> If it's 0 you know it wasn't called yet and if it's set, you have to at
> least wait for until it is after "jiffies + HZ*3".
>
> Also isn't that racy right now? "priv->lb_dis_to - get_jiffies_64())"
> can
> get negative, no?
>
Yes, this is a bug. I will fix.

Also use jiffies only.

There is no restriction to enter loopback mode for first 3 seconds.

> -michael
>