Date:   Sat, 11 Dec 2021 14:15:54 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Francesco Dolcini <francesco.dolcini@toradex.com>
Cc:     philippe.schenker@toradex.com, andrew@lunn.ch,
        qiangqing.zhang@nxp.com, davem@davemloft.net, festevam@gmail.com,
        kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH net-next] net: phy: perform a PHY reset on resume
Message-ID: <YbSymkxlslW2DqLW@shell.armlinux.org.uk>
References: <7a4830b495e5e819a2b2b39dd01785aa3eba4ce7.camel@toradex.com>
 <20211211130146.357794-1-francesco.dolcini@toradex.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211211130146.357794-1-francesco.dolcini@toradex.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Sat, Dec 11, 2021 at 02:01:46PM +0100, Francesco Dolcini wrote:
> Perform a PHY reset in phy_init_hw() to ensure that the PHY is working
> after resume. This is required if the PHY was powered down in suspend
> like it is done by the freescale FEC driver in fec_suspend().
> 
> Link: https://lore.kernel.org/netdev/20211206101326.1022527-1-philippe.schenker@toradex.com/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> ---
> 
> Philippe: what about something like that? Only compile tested, but I see no reason for this not solving the issue.
> 
> Any delay required on the reset can be specified using reset-assert-us/reset-deassert-us.
> 
> ---
>  drivers/net/phy/phy_device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 74d8e1dc125f..7eab0c054adf 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1158,7 +1158,8 @@ int phy_init_hw(struct phy_device *phydev)
>  {
>  	int ret = 0;
>  
> -	/* Deassert the reset signal */
> +	/* phy reset required if the phy was powered down during suspend */
> +	phy_device_reset(phydev, 1);
>  	phy_device_reset(phydev, 0);
>  
>  	if (!phydev->drv)

I don't particularly like this - this impacts everyone who is using
phylib at this point, whereas no reset was happening if the reset was
already deasserted here.

In the opening remarks to this series, it is stated:

  If a hardware-design is able to control power to the Ethernet PHY and
  relying on software to do a reset, the PHY does no longer work after
  resuming from suspend, given the PHY does need a hardware-reset.

This requirement is conditional on the hardware design, it isn't a
universal requirement and won't apply everywhere. I think it needs to
be described in firmware that this action is required. That said...

Please check the datasheet on the PHY regarding application of power and
reset. You may find that the PHY datasheet requires the reset to be held
active from power up until the clock input is stable - this could mean
you need some other arrangement to assert the PHY reset signal after
re-applying power sooner than would happen by the proposed point in the
kernel.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!