[PATCH net-next 0/3] Add Possiblity to Reset PHY After Power-up

[PATCH net-next 0/3] Add Possiblity to Reset PHY After Power-up

[Philippe Schenker]

We do have a hardware design in which the ethernet phy regulator and
reset are controlled by software. The ethernet PHY is a Microchip
KSZ9131 [1] and the power sequencing requires a reset after the power
goes up.

In our case the ethernet PHY is connected to a Freescale FEC and the
driver is shutting down the regulator on suspend, however on the resume
path the reset signal is never asserted and because of that the
ethernet is not working anymore.

To solve this adds a new phy_reset_after_power_on() function, similar
to the existing phy_reset_after_clk_enable(), and call it in the fec
resume path after the regulator is switched on as suggested by
Joakim Zhang <qiangqing.zhang@nxp.com>.

[1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841C.pdf


Philippe Schenker (3):
  net: phy: add phy_reset_after_power_on() function
  net: phy: micrel: add reset-after-power-on flag to ksz9x31 phys
  net: fec: reset phy on resume after power-up

 drivers/net/ethernet/freescale/fec_main.c |  1 +
 drivers/net/phy/micrel.c                  |  2 ++
 drivers/net/phy/phy_device.c              | 24 +++++++++++++++++++++++
 include/linux/phy.h                       |  2 ++
 4 files changed, 29 insertions(+)

-- 
2.34.1

[PATCH net-next 1/3] net: phy: add phy_reset_after_power_on() function

[Philippe Schenker]

Some PHY requires a reset after being powered on (e.g. KSZ9131), add a
new function and related PHY_RST_AFTER_POWER_ON phy flag to be called
after the PHY regulator is enabled.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/net/phy/phy_device.c | 24 ++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74d8e1dc125f..bad836a7ee01 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1878,6 +1878,30 @@ int phy_reset_after_clk_enable(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_reset_after_clk_enable);
 
+/**
+ * phy_reset_after_power_on - perform a PHY reset if needed
+ * @phydev: target phy_device struct
+ *
+ * Description: Some PHYs or hardware design, need a reset after power was
+ *   enabled and rely on that software reset. This function evaluates the flags
+ *   and perform the reset if it's needed.
+ *   Returns < 0 on error, 0 if the phy wasn't reset and 1 if the phy was reset.
+ */
+int phy_reset_after_power_on(struct phy_device *phydev)
+{
+	if (!phydev || !phydev->drv)
+		return -ENODEV;
+
+	if (phydev->drv->flags & PHY_RST_AFTER_POWER_ON) {
+		phy_device_reset(phydev, 1);
+		phy_device_reset(phydev, 0);
+		return 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_reset_after_power_on);
+
 /* Generic PHY support and helper functions */
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index cbf03a5f9cf5..0d88cdc97dbd 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -80,6 +80,7 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_IS_INTERNAL		0x00000001
 #define PHY_RST_AFTER_CLK_EN	0x00000002
 #define PHY_POLL_CABLE_TEST	0x00000004
+#define PHY_RST_AFTER_POWER_ON	0x00000008
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /**
@@ -1499,6 +1500,7 @@ int phy_speed_up(struct phy_device *phydev);
 
 int phy_restart_aneg(struct phy_device *phydev);
 int phy_reset_after_clk_enable(struct phy_device *phydev);
+int phy_reset_after_power_on(struct phy_device *phydev);
 
 #if IS_ENABLED(CONFIG_PHYLIB)
 int phy_start_cable_test(struct phy_device *phydev,
-- 
2.34.1

[PATCH net-next 2/3] net: phy: micrel: add reset-after-power-on flag to ksz9x31 phys

[Philippe Schenker]

KSZ9031 and KSZ9131 do need a reset after power-on, set the
PHY_RST_AFTER_POWER_ON flag to enable the phylib to do it in case the
reset signal is controlled by software.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/net/phy/micrel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 44a24b99c894..85ee3a61017b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1777,6 +1777,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ9031 Gigabit PHY",
 	.driver_data	= &ksz9021_type,
+	.flags		= PHY_RST_AFTER_POWER_ON,
 	.probe		= kszphy_probe,
 	.get_features	= ksz9031_get_features,
 	.config_init	= ksz9031_config_init,
@@ -1822,6 +1823,7 @@ static struct phy_driver ksphy_driver[] = {
 	.name		= "Microchip KSZ9131 Gigabit PHY",
 	/* PHY_GBIT_FEATURES */
 	.driver_data	= &ksz9021_type,
+	.flags		= PHY_RST_AFTER_POWER_ON,
 	.probe		= kszphy_probe,
 	.config_init	= ksz9131_config_init,
 	.config_intr	= kszphy_config_intr,
-- 
2.34.1

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

[Philippe Schenker]

Reset the eth PHY after resume in case the power was switched off
during suspend, this is required by some PHYs if the reset signal
is controlled by software.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

---

 drivers/net/ethernet/freescale/fec_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1b1f7f2a6130..c29eddbb0155 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -4086,6 +4086,7 @@ static int __maybe_unused fec_resume(struct device *dev)
 		ret = regulator_enable(fep->reg_phy);
 		if (ret)
 			return ret;
+		phy_reset_after_power_on(ndev->phydev);
 	}
 
 	rtnl_lock();
-- 
2.34.1

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

[Andrew Lunn]

On Tue, Dec 14, 2021 at 01:16:38PM +0100, Philippe Schenker wrote:
> Reset the eth PHY after resume in case the power was switched off
> during suspend, this is required by some PHYs if the reset signal
> is controlled by software.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> 
> ---
> 
>  drivers/net/ethernet/freescale/fec_main.c | 1 +

Hi Philippe

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.

Is there a way we can put this into phylib? Maybe as part of
phy_init_hw()? Humm, actually, thinking aloud:

int phy_init_hw(struct phy_device *phydev)
{
	int ret = 0;

	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

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?

   Andrew

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

[Russell King (Oracle)]

On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> On Tue, Dec 14, 2021 at 01:16:38PM +0100, Philippe Schenker wrote:
> > Reset the eth PHY after resume in case the power was switched off
> > during suspend, this is required by some PHYs if the reset signal
> > is controlled by software.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > 
> > ---
> > 
> >  drivers/net/ethernet/freescale/fec_main.c | 1 +
> 
> Hi Philippe
> 
> 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.
> 
> Is there a way we can put this into phylib? Maybe as part of
> phy_init_hw()? Humm, actually, thinking aloud:
> 
> int phy_init_hw(struct phy_device *phydev)
> {
> 	int ret = 0;
> 
> 	/* Deassert the reset signal */
> 	phy_device_reset(phydev, 0);
> 
> 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?

It seems to be a combination issue - it's the fact that the power is
turned off and the fact that the reset needs to be applied.

If other PHYs such as AR8035 are subjected to this, they appear to
have a requirement that reset is asserted when power is applied and
kept asserted until the clock has stabilised and a certain time has
elapsed.

As I've already highlighted, we do not want to be asserting the reset
signal in phy_init_hw() - doing so would mean that any PHY with a GPIO
reset gets reset whenever the PHY is connected to the MAC - which can
be whenever the interface is brought up. That will introduce a multi-
second delay to bringing up the network.

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

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

[Francesco Dolcini]

Hello Andrew,

On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> 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.

> 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

Francesco

[1] https://lore.kernel.org/netdev/20171211121700.10200-1-dev@g0hl1n.net/
[2] 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")

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

[Andrew Lunn]

On Tue, Dec 14, 2021 at 11:35:48PM +0100, Francesco Dolcini wrote:
> Hello Andrew,
> 
> On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> > 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.
> 
> > 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).

Maybe i don't understand your hardware.

You have a regulator providing power of the PHY.

You have a reset, i guess a GPIO, connected to the reset pin of the
PHY.

What you could do is:

PHY driver suspend handler does a phy_device_reset(ndev->phydev, 1)
to put the PHY into reset.

MAC driver disables the regulator.

Power consumption should now be 0, since it does not have any power.

On resume, the MAC enables the regulator. At this point, the PHY gets
power, but is still held in reset. It is now consuming power, but not
doing anything. The MAC calls phy_hw_init(), which calls
phy_device_reset(ndev->phydev, 0), taking the PHY out of reset.

Hopefully, this release from reset is enough to make the PHY work.

Doing it like this also addresses Russell point. phy_hw_init() is not
putting the device into reset, it is only taking it out of reset, if
it happens to be already in reset. So we are not slowing down link up
for everybody.

    Andrew

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

[Russell King (Oracle)]

On Wed, Dec 15, 2021 at 10:36:52AM +0100, Andrew Lunn wrote:
> On Tue, Dec 14, 2021 at 11:35:48PM +0100, Francesco Dolcini wrote:
> > Hello Andrew,
> > 
> > On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> > > 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.
> > 
> > > 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).
> 
> Maybe i don't understand your hardware.
> 
> You have a regulator providing power of the PHY.
> 
> You have a reset, i guess a GPIO, connected to the reset pin of the
> PHY.
> 
> What you could do is:
> 
> PHY driver suspend handler does a phy_device_reset(ndev->phydev, 1)
> to put the PHY into reset.
> 
> MAC driver disables the regulator.
> 
> Power consumption should now be 0, since it does not have any power.
> 
> On resume, the MAC enables the regulator. At this point, the PHY gets
> power, but is still held in reset. It is now consuming power, but not
> doing anything. The MAC calls phy_hw_init(), which calls
> phy_device_reset(ndev->phydev, 0), taking the PHY out of reset.
> 
> Hopefully, this release from reset is enough to make the PHY work.
> 
> Doing it like this also addresses Russell point. phy_hw_init() is not
> putting the device into reset, it is only taking it out of reset, if
> it happens to be already in reset. So we are not slowing down link up
> for everybody.

Here's another question which no one seems to have considered. If the
PHY power source can be controlled, why doesn't the firmware describe
the power supply for the PHY, and why doesn't the PHY driver control
the PHY power source? Why is that in the SoC network driver?

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

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

[Francesco Dolcini]

On Wed, Dec 15, 2021 at 10:36:52AM +0100, Andrew Lunn wrote:
> On Tue, Dec 14, 2021 at 11:35:48PM +0100, Francesco Dolcini wrote:
> > Hello Andrew,
> > 
> > On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> > > 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.
> > 
> > > 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).
> 
> Maybe i don't understand your hardware.
> 
> You have a regulator providing power of the PHY.
> 
> You have a reset, i guess a GPIO, connected to the reset pin of the
> PHY.
> 
> What you could do is:
> 
> PHY driver suspend handler does a phy_device_reset(ndev->phydev, 1)
> to put the PHY into reset.
> 
> MAC driver disables the regulator.
> 
> Power consumption should now be 0, since it does not have any power.
> 
> On resume, the MAC enables the regulator. At this point, the PHY gets
> power, but is still held in reset. It is now consuming power, but not
> doing anything. The MAC calls phy_hw_init(), which calls
> phy_device_reset(ndev->phydev, 0), taking the PHY out of reset.
> 
> Hopefully, this release from reset is enough to make the PHY work.
This is all correct and will solve the issue, however ...

The problem I see is that nor the phylib nor the PHY driver is aware
that the PHY was powered down, if we unconditionally assert the reset in
the suspend callback in the PHY driver/lib this will affect in a bad
case the most common use case in which we keep the PHY powered in
suspend.

We would have to move the regulator in the PHY driver (phy/micrel.c) to
do it properly.

The reason is that the power consumption in reset is higher in reset
compared to the normal PHY software power down.

This will create a power consumption regression for lot of users.

Doing this into the FEC driver would not have this issue, since we know
if we have a regulator (I guess you saw my one line patch for it).


On Wed, Dec 15, 2021 at 10:29:07AM +0000, Russell King (Oracle) wrote:
> Here's another question which no one seems to have considered. If the
> PHY power source can be controlled, why doesn't the firmware describe
> the power supply for the PHY, and why doesn't the PHY driver control
> the PHY power source? Why is that in the SoC network driver?
Legacy/historical reasons ...

In the first RFC patch for this issue this was mentioned by Philippe,
but than the discussion went into another direction.

As I wrote above if we handle both reset/regulator in the PHY driver it
should work, just a little bit tricky because phy/micrel.c handle a
whole family of phys.


On Wed, Dec 15, 2021 at 10:25:14AM +0000, Joakim Zhang wrote:
> As I mentioned before, both mac and phylib have not taken PHY reset
> into consideration during system suspend/resume scenario. As Andrew
> suggested, you could move this into phy driver suspend function, this
> is a corner case. One point I don't understand, why do you reject to
> assert reset signal during system suspended? 
See my answer to Andrew above, in short asserting the reset without
disabling the regulator will create a regression on the power
consumption.

Any agreement on how to move forward?

 1. add phy_reset_after_power_on() and call it from FEC driver (current
 patchset)
 2. assert phy reset in FEC driver suspend (one line patch from me in
 this thread)
 3. move regulator to phy/micrel.c and assert reset in the phy driver resume
 callback
 4. ?

?

Francesco

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

[Francesco Dolcini]

On Wed, Dec 15, 2021 at 12:01:39PM +0100, Francesco Dolcini wrote:
> Any agreement on how to move forward?
...
>  3. move regulator to phy/micrel.c and assert reset in the phy driver resume
>  callback
whoops, s/resume/suspend/

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

[Andrew Lunn]

> This is all correct and will solve the issue, however ...
> 
> The problem I see is that nor the phylib nor the PHY driver is aware
> that the PHY was powered down, if we unconditionally assert the reset in
> the suspend callback in the PHY driver/lib this will affect in a bad
> case the most common use case in which we keep the PHY powered in
> suspend.

We know if the PHY should be left up because of WoL. So that is not an
issue. We can also put the PHY into lower power mode, before making
the call to put the PHY into reset.  If the reset is not implemented,
the PHY stays in low power mode. If it is implemented, it is both in
lower power mode and held in reset. And if the regulator is provided,
the power will go off.

> The reason is that the power consumption in reset is higher in reset
> compared to the normal PHY software power down.

Does the datasheet have numbers for in lower power mode and held in
reset? We only have an issue if held in reset when in low power mode
consumes more power than simply in low power mode.

	Andrew

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

[Francesco Dolcini]

On Wed, Dec 15, 2021 at 08:34:00PM +0100, Andrew Lunn wrote:
> > The reason is that the power consumption in reset is higher in reset
> > compared to the normal PHY software power down.
> 
> Does the datasheet have numbers for in lower power mode and held in
> reset? We only have an issue if held in reset when in low power mode
> consumes more power than simply in low power mode.

The numbers for KSZ9131, Table 6-3: Power Consumption from datasheet [0]

61.2mW in reset, 24.4mW in software power down (3.3VDD)
40.9mW in reset, 12.5mW in software power down (2.5VDD)

Francesco

[0] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841B.pdf

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

[Joakim Zhang]

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2021年12月15日 19:02
> To: Andrew Lunn <andrew@lunn.ch>; Russell King (Oracle)
> <linux@armlinux.org.uk>; Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: Russell King (Oracle) <linux@armlinux.org.uk>; Francesco Dolcini
> <francesco.dolcini@toradex.com>; Philippe Schenker
> <philippe.schenker@toradex.com>; netdev@vger.kernel.org; Joakim Zhang
> <qiangqing.zhang@nxp.com>; David S . Miller <davem@davemloft.net>;
> Heiner Kallweit <hkallweit1@gmail.com>; Jakub Kicinski <kuba@kernel.org>;
> Fabio Estevam <festevam@gmail.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 3/3] net: fec: reset phy on resume after
> power-up
> 
[...]

> On Wed, Dec 15, 2021 at 10:25:14AM +0000, Joakim Zhang wrote:
> > As I mentioned before, both mac and phylib have not taken PHY reset
> > into consideration during system suspend/resume scenario. As Andrew
> > suggested, you could move this into phy driver suspend function, this
> > is a corner case. One point I don't understand, why do you reject to
> > assert reset signal during system suspended?
> See my answer to Andrew above, in short asserting the reset without
> disabling the regulator will create a regression on the power consumption.

As I can see, when system suspended, PHY is totally powered down, since you disable the 
regulator. At this situation, if you assert reset signal, you mean it will increase the power
consumption? PHY is totally powered down, why assert reset signal still affect PHY? 
 
Best Regards,
Joakim Zhang

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

[Francesco Dolcini]

On Thu, Dec 16, 2021 at 04:52:39AM +0000, Joakim Zhang wrote:
> As I can see, when system suspended, PHY is totally powered down,
> since you disable the regulator. At this situation, if you
> assert reset signal, you mean it will increase the power
> consumption? PHY is totally powered down, why assert reset
> signal still affect PHY? 
In general there are *other* use cases in which the PHY is powered in
suspend. We should not create a regression there.

Francesco

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

[Andrew Lunn]

On Thu, Dec 16, 2021 at 08:52:16AM +0100, Francesco Dolcini wrote:
> On Thu, Dec 16, 2021 at 04:52:39AM +0000, Joakim Zhang wrote:
> > As I can see, when system suspended, PHY is totally powered down,
> > since you disable the regulator. At this situation, if you
> > assert reset signal, you mean it will increase the power
> > consumption? PHY is totally powered down, why assert reset
> > signal still affect PHY? 

> In general there are *other* use cases in which the PHY is powered in
> suspend. We should not create a regression there.

Yes, this is the sticking point. We can do what you want, but
potentially, the change affects others.

I think you need to move the regulator into phylib, so the PHY driver
can do the right thing. It is really the only entity which knows what
is the correct thing to do.

   Andrew

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

[Francesco Dolcini]

On Thu, Dec 16, 2021 at 11:24:24AM +0100, Andrew Lunn wrote:
> I think you need to move the regulator into phylib, so the PHY driver
> can do the right thing. It is really the only entity which knows what
> is the correct thing to do.
Do you believe that the right place is the phylib and not the phy driver?
Is this generic enough?

Francesco

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

[Andrew Lunn]

On Thu, Dec 16, 2021 at 12:24:33PM +0100, Francesco Dolcini wrote:
> On Thu, Dec 16, 2021 at 11:24:24AM +0100, Andrew Lunn wrote:
> > I think you need to move the regulator into phylib, so the PHY driver
> > can do the right thing. It is really the only entity which knows what
> > is the correct thing to do.

> Do you believe that the right place is the phylib and not the phy driver?
> Is this generic enough?

It is split. phylib can do the lookup in DT, get the regulator and
provide a helper to enable/disable it. So very similar to the reset.

The phy driver would then use the helpers. It probably needs to look
into the phydev structure to see what is actually available, is there
a reset, a regulator etc, and then decide what is best to do given the
available resources.

	  Andrew

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

[Francesco Dolcini]

On Thu, Dec 16, 2021 at 12:28:19PM +0100, Andrew Lunn wrote:
> On Thu, Dec 16, 2021 at 12:24:33PM +0100, Francesco Dolcini wrote:
> > On Thu, Dec 16, 2021 at 11:24:24AM +0100, Andrew Lunn wrote:
> > > I think you need to move the regulator into phylib, so the PHY driver
> > > can do the right thing. It is really the only entity which knows what
> > > is the correct thing to do.
> 
> > Do you believe that the right place is the phylib and not the phy driver?
> > Is this generic enough?
> 
> It is split. phylib can do the lookup in DT, get the regulator and
> provide a helper to enable/disable it. So very similar to the reset.
Sounds good.

Can we safely assume that we do have at most one regulator for the phy?

Francesco

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

[Andrew Lunn]

> Can we safely assume that we do have at most one regulator for the phy?

Seems like a reasonable assumption.

      Andrew

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

[Joakim Zhang]

Hi Francesco,

> -----Original Message-----
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> Sent: 2021年12月15日 6:36
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Philippe Schenker <philippe.schenker@toradex.com>;
> netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>; David
> S . Miller <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>;
> Heiner Kallweit <hkallweit1@gmail.com>; Francesco Dolcini
> <francesco.dolcini@toradex.com>; Jakub Kicinski <kuba@kernel.org>; Fabio
> Estevam <festevam@gmail.com>; Fugang Duan <fugang.duan@nxp.com>;
> 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:
> > 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.
> 
> > 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

As I mentioned before, both mac and phylib have not taken PHY reset into consideration during
system suspend/resume scenario. As Andrew suggested, you could move this into phy driver suspend
function, this is a corner case. One point I don't understand, why do you reject to assert reset signal during
system suspended? 

Best Regards,
Joakim Zhang
> Francesco
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Fnetdev%2F20171211121700.10200-1-dev%40g0hl1n.net%2F&a
> mp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7Cf7140fe971544fe8d2
> 2608d9bf521517%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6377
> 51181527979233%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=itV
> m0jroQ0MzDG5Ipqs3OY0F5SY%2FkbdFRWauNKq2XiQ%3D&amp;reserved=0
> [2] 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")

Re: [PATCH net-next 0/3] Add Possiblity to Reset PHY After Power-up

[Francesco Dolcini]

On Tue, Dec 14, 2021 at 01:16:35PM +0100, Philippe Schenker wrote:
> We do have a hardware design in which the ethernet phy regulator and
> reset are controlled by software. The ethernet PHY is a Microchip
> KSZ9131 [1] and the power sequencing requires a reset after the power
> goes up.
> 
> In our case the ethernet PHY is connected to a Freescale FEC and the
> driver is shutting down the regulator on suspend, however on the resume
> path the reset signal is never asserted and because of that the
> ethernet is not working anymore.
> 
> To solve this adds a new phy_reset_after_power_on() function, similar
> to the existing phy_reset_after_clk_enable(), and call it in the fec
> resume path after the regulator is switched on as suggested by
> Joakim Zhang <qiangqing.zhang@nxp.com>.
> 
> [1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841C.pdf

For the whole series.

Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>