Re: [RFC PATCH] net: phy: micrel: reset KSZ9x31 when resuming

[Francesco Dolcini]

- Philippe, email address is no longer valid.

On Tue, Jan 09, 2024 at 09:52:22PM +0100, Wolfram Sang wrote:
> On a Renesas Ebisu board, the KSZ9031 PHY is stalled after resuming if
> the interface has not been brought up before suspending. If it had been
> brought up before, phylib ensures that reset is asserted before
> suspending. But if it had never been brought up, there is no instance
> which could ensure that reset is asserted. And upon resume, the PHY is
> in an unknown state without reset being asserted. To bring it back to a
> known state, simply reset it when it is about to be resumed.
> 
> This likely also helps another issue [1] where a KSZ9131 can be powered
> using regulators. After switching power on again in resume, a reset is
> also needed.
> 
> [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211214121638.138784-4-philippe.schenker@xxxxxxxxxxx/
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
> This is a different solution to a problem I already tried to solve
> here[2]. Back then, I added code to the MAC, but I now believe it should
> be solved on PHY level. We never saw the problem with other PHYs.
> Looking at [1], it seems that KSZ9x31 is also sensitive to being
> powered down without reset being asserted. I know it is not a perfect
> proof, but I guess these assumptions are all we have.
> 
> Philippe, Francesco: do you still have access to machines with this
> issue? Could you kindly test if so?

I have access, however
 - Philippe is long gone from Toradex and he was the one looking into
   this topic
 - we did solve the issue in a different way, e.g. we no longer
   power-off the phy in suspend

Therefore is not straightforward to provide valuable feedback to you
now.


> 
> Patch is based on 6.7. Looking forward for comments if this is the
> correct layer to tackle the problem. Thanks!
> 
> 
> [2] https://lore.kernel.org/all/20230321103357.18940-1-wsa+renesas@xxxxxxxxxxxxxxxxxxxx/
> 
>  drivers/net/phy/micrel.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 08e3915001c3..c38d7982c06c 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1984,6 +1984,14 @@ static int kszphy_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int ksz9x31_resume(struct phy_device *phydev)
> +{
> +	phy_device_reset(phydev, 1);
> +	phy_device_reset(phydev, 0);

Is something like that fine?
Don't we need to reconfigure the ethernet phy completely on resume
if we do reset it? kszphy_config_reset() is taking care of something,
but I think that the phy being reset on resume is not handled
correctly.

Francesco