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

[Florian Fainelli <f.fainelli@gmail.com>]

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