Received-SPF: pass (google.com: domain of linux-kernel+bounces-21529-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 10 Jan 2024 00:28:38 +0100
From: Francesco Dolcini <francesco@dolcini.it>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
	Francesco Dolcini <francesco.dolcini@toradex.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>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] net: phy: micrel: reset KSZ9x31 when resuming
Message-ID: <20240109232838.GA3626@francesco-nb>
References: <20240109205223.40219-1-wsa+renesas@sang-engineering.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240109205223.40219-1-wsa+renesas@sang-engineering.com>

- 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@toradex.com/
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> 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@sang-engineering.com/
> 
>  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