RE: [PATCH net-next 3/3] net: fec: reset phy on resume after power-up

-----Original Message-----
From: Francesco Dolcini <redacted>
Sent: 2021年12月15日 6:36
To: Andrew Lunn <andrew@lunn.ch>
Cc: Philippe Schenker <redacted>;
netdev@vger.kernel.org; Joakim Zhang [off-list ref]; David
S . Miller [off-list ref]; Russell King [off-list ref];
Heiner Kallweit [off-list ref]; Francesco Dolcini
[off-list ref]; Jakub Kicinski [off-list ref]; Fabio
Estevam [off-list ref]; Fugang Duan [off-list ref];
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 3/3] net: fec: reset phy on resume after
power-up

Hello Andrew,

On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:

quoted

What i don't particularly like about this is that the MAC driver is
doing it. Meaning if this PHY is used with any other MAC, the same
code needs adding there.

This is exactly the same case as phy_reset_after_clk_enable() [1][2], to me it
does not look that bad.

quoted

So maybe in the phy driver, add a suspend handler, which asserts the
reset. This call here will take it out of reset, so applying the reset
you need?

Asserting the reset in the phylib in suspend path is a bad idea, in the general
case in which the PHY is powered in suspend the power-consumption is likely
to be higher if the device is in reset compared to software power-down using
the BMCR register (at least for the PHY datasheet I checked).

What we could do is to call phy_device_reset in the fec driver suspend path
when we know we are going to disable the regulator, I do not like it, but it
would solve the issue.

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -4064,7 +4064,11 @@ static int __maybe_unused fec_suspend(struct

device *dev)
        rtnl_unlock();

        if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE))
+       {
                regulator_disable(fep->reg_phy);
+               phy_device_reset(ndev->phydev, 1);
+       }
+

        /* SOC supply clock to phy, when clock is disabled, phy link down
         * SOC control phy regulator, when regulator is disabled, phy link
down