[PATCH RFC 0/5] net: stmmac: fix resume failures due to RX clock

[PATCH RFC 0/5] net: stmmac: fix resume failures due to RX clock

[Russell King (Oracle)]

Hi,

This series is likely dependent on the "net: stmmac: cleanup transmit
clock setting" series which was submitted earlier today.

stmmac has a long history of failing to resume due to lack of receive
clock. NVidia have reported that as a result of the EEE changes, they
see a greater chance of resume failure with those patches applied than
before.

The issue is that the DesignWare core requires that the receive clock
is running in order to complete software reset, which causes
stmmac_reset() and stmmac_hw_setup() to fail.

There are several things that are wrong:

1. Calling phylink_start() early can result in a call to mac_link_up()
   which will set TE and RE bits before stmmac_hw_setup() has been
   called. This is evident in the debug logs that NVidia sent while
   debugging the problem.

   This is something I have pointed out in the past, but ithas been
   claimed to be necessary to do things this way to have the PHY
   receive clock running. Enabling RE before DMA is setup is against
   the DesignWare databook documentation.

2. Enabling LPI clock-stop at the PHY (as the driver has done prior
   to my patch set) allows the PHY to stop its receive clock when the
   link enters low-power mode. This means the completion of reset is
   dependent on the current EEE state, which is indeterminable, but
   is likely to be in low power mode on resume.

We solve (1) by moving the call to phylink_resume() later. This patch
on its own probably causes regressions as it may make it more likely
that the link will be in low power state, or maybe the PHY driver does
not respect the PHY_F_RXC_ALWAYS_ON flag - this needs to be tested on
as many different hardware setups that use this driver as possible,
and any issues addressed *without* moving phylink_resume() back.
If we need some way to resume the PHY early, then we need to work out
some way to do that with phylib without calling phy_start() early.

(2) is fixed by introducing phylink_prepare_resume(), which will
disable receive clock-stop in LPI mode at the PHY, and we will restore
the clock-stop setting in phylink_resume(). It is possible that this
solves some of the reason for the early placement of phylink_resume().

phylink_prepare_resume() also provides a convenient site should (1)
need further work.

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 27 +++++++--------
 drivers/net/phy/phylink.c                         | 40 ++++++++++++++++++++++-
 include/linux/phylink.h                           |  1 +
 3 files changed, 51 insertions(+), 17 deletions(-)

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

[PATCH RFC net-next 1/5] net: phylink: add config of PHY receive clock-stop in phylink_resume()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a3b186ab3854..0aae0bb2a254 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2264,7 +2264,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->mac_tx_clk_stop = phy_eee_tx_clock_stop_capable(phy) > 0;
 
 	if (pl->mac_supports_eee_ops) {
-		/* Explicitly configure whether the PHY is allowed to stop it's
+		/* Explicitly configure whether the PHY is allowed to stop its
 		 * receive clock.
 		 */
 		ret = phy_eee_rx_clock_stop(phy,
@@ -2645,8 +2645,22 @@ EXPORT_SYMBOL_GPL(phylink_suspend);
  */
 void phylink_resume(struct phylink *pl)
 {
+	int ret;
+
 	ASSERT_RTNL();
 
+	if (pl->mac_supports_eee_ops && pl->phydev) {
+		/* Explicitly configure whether the PHY is allowed to stop its
+		 * receive clock on resume to ensure that it is correctly
+		 * configured.
+		 */
+		ret = phy_eee_rx_clock_stop(pl->phydev,
+					    pl->config->eee_rx_clk_stop_enable);
+		if (ret == -EOPNOTSUPP)
+			phylink_warn(pl, "failed to set rx clock stop: %pe\n",
+				     ERR_PTR(ret));
+	}
+
 	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
 		/* Wake-on-Lan enabled, MAC handling */
 
-- 
2.30.2

Re: [PATCH RFC net-next 1/5] net: phylink: add config of PHY receive clock-stop in phylink_resume()

[Andrew Lunn]

On Thu, Feb 27, 2025 at 02:37:47PM +0000, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH RFC net-next 2/5] net: phylink: add phylink_prepare_resume()

[Russell King (Oracle)]

Add a resume preparation function, which will ensure that the receive
clock from the PHY is appropriately configured while resuming.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 24 ++++++++++++++++++++++++
 include/linux/phylink.h   |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0aae0bb2a254..976e569feb70 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2636,6 +2636,30 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 }
 EXPORT_SYMBOL_GPL(phylink_suspend);
 
+/**
+ * phylink_prepare_resume() - prepare to resume a network device
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Optional, thus must be called prior to phylink_resume().
+ *
+ * Prepare to resume a network device, preparing the PHY as necessary.
+ */
+void phylink_prepare_resume(struct phylink *pl)
+{
+	ASSERT_RTNL();
+
+	/* If the MAC requires the receive clock, but receive clock
+	 * stop was enabled at the PHY, we need to ensure that the
+	 * receive clock is running. Disable receive clock stop.
+	 * phylink_resume() will re-enable it if necessary.
+	 */
+	if (pl->mac_supports_eee_ops && pl->phydev &&
+	    pl->config->mac_requires_rxc &&
+	    pl->config->eee_rx_clk_stop_enable)
+		phy_eee_rx_clock_stop(pl->phydev, false);
+}
+EXPORT_SYMBOL_GPL(phylink_prepare_resume);
+
 /**
  * phylink_resume() - handle a network device resume event
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 08df65f6867a..071ed4683c8c 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -699,6 +699,7 @@ void phylink_start(struct phylink *);
 void phylink_stop(struct phylink *);
 
 void phylink_suspend(struct phylink *pl, bool mac_wol);
+void phylink_prepare_resume(struct phylink *pl);
 void phylink_resume(struct phylink *pl);
 
 void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
-- 
2.30.2

Re: [PATCH RFC net-next 2/5] net: phylink: add phylink_prepare_resume()

[Andrew Lunn]

On Thu, Feb 27, 2025 at 02:37:53PM +0000, Russell King (Oracle) wrote:
> Add a resume preparation function, which will ensure that the receive
> clock from the PHY is appropriately configured while resuming.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH RFC net-next 3/5] net: stmmac: simplify phylink_suspend() and phylink_resume() calls

[Russell King (Oracle)]

Currently, the calls to phylink's suspend and resume functions are
inside overly complex tests, and boil down to:

	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
		call phylink
	} else {
		call phylink and
		if (device_may_wakeup(priv->device))
			do something else
	}

This results in phylink always being called, possibly with differing
arguments for phylink_suspend().

Simplify this code, noting that each site is slightly different due to
the order in which phylink is called and the "something else".

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index aec230353ac4..fbcba6c71f12 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7831,13 +7831,11 @@ int stmmac_suspend(struct device *dev)
 	mutex_unlock(&priv->lock);
 
 	rtnl_lock();
-	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
-		phylink_suspend(priv->phylink, true);
-	} else {
-		if (device_may_wakeup(priv->device))
-			phylink_speed_down(priv->phylink, false);
-		phylink_suspend(priv->phylink, false);
-	}
+	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+		phylink_speed_down(priv->phylink, false);
+
+	phylink_suspend(priv->phylink,
+			device_may_wakeup(priv->device) && priv->plat->pmt);
 	rtnl_unlock();
 
 	if (stmmac_fpe_supported(priv))
@@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev)
 	}
 
 	rtnl_lock();
-	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
-		phylink_resume(priv->phylink);
-	} else {
-		phylink_resume(priv->phylink);
-		if (device_may_wakeup(priv->device))
-			phylink_speed_up(priv->phylink);
-	}
+	phylink_resume(priv->phylink);
+	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+		phylink_speed_up(priv->phylink);
 	rtnl_unlock();
 
 	rtnl_lock();
-- 
2.30.2

Re: [PATCH RFC net-next 3/5] net: stmmac: simplify phylink_suspend() and phylink_resume() calls

[Andrew Lunn]

> @@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev)
>  	}
>  
>  	rtnl_lock();
> -	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
> -		phylink_resume(priv->phylink);
> -	} else {
> -		phylink_resume(priv->phylink);
> -		if (device_may_wakeup(priv->device))
> -			phylink_speed_up(priv->phylink);
> -	}
> +	phylink_resume(priv->phylink);
> +	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> +		phylink_speed_up(priv->phylink);
>  	rtnl_unlock();
>  
>  	rtnl_lock();

Unrelated to this patch, but unlock() followed by lock()? Seems like
some more code which could be cleaned up?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 3/5] net: stmmac: simplify phylink_suspend() and phylink_resume() calls

[Russell King (Oracle)]

On Thu, Feb 27, 2025 at 05:45:35PM +0100, Andrew Lunn wrote:
> > @@ -7927,13 +7925,9 @@ int stmmac_resume(struct device *dev)
> >  	}
> >  
> >  	rtnl_lock();
> > -	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
> > -		phylink_resume(priv->phylink);
> > -	} else {
> > -		phylink_resume(priv->phylink);
> > -		if (device_may_wakeup(priv->device))
> > -			phylink_speed_up(priv->phylink);
> > -	}
> > +	phylink_resume(priv->phylink);
> > +	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> > +		phylink_speed_up(priv->phylink);
> >  	rtnl_unlock();
> >  
> >  	rtnl_lock();
> 
> Unrelated to this patch, but unlock() followed by lock()? Seems like
> some more code which could be cleaned up?

Indeed, this vanishes in the next patch due to phylink_resume()
moving later.

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

[PATCH RFC net-next 4/5] net: stmmac: move phylink_resume() after resume setup is complete

[Russell King (Oracle)]

Move phylink_resume() to be after the setup in stmmac_resume() has
completed, as phylink_resume() may result in an immediate call to the
.mac_link_up method, which will enable the transmitter and receiver,
and enable the transmit queues.

This behaviour has been witnessed by Jon Hunter on the Jetson TX2
platform (Tegra 186).

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fbcba6c71f12..23c610f7c779 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7924,12 +7924,6 @@ int stmmac_resume(struct device *dev)
 			return ret;
 	}
 
-	rtnl_lock();
-	phylink_resume(priv->phylink);
-	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
-		phylink_speed_up(priv->phylink);
-	rtnl_unlock();
-
 	rtnl_lock();
 	mutex_lock(&priv->lock);
 
@@ -7948,6 +7942,11 @@ int stmmac_resume(struct device *dev)
 	stmmac_enable_all_dma_irq(priv);
 
 	mutex_unlock(&priv->lock);
+
+	phylink_resume(priv->phylink);
+	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+		phylink_speed_up(priv->phylink);
+
 	rtnl_unlock();
 
 	netif_device_attach(ndev);
-- 
2.30.2

Re: [PATCH RFC net-next 4/5] net: stmmac: move phylink_resume() after resume setup is complete

[Andrew Lunn]

On Thu, Feb 27, 2025 at 02:38:03PM +0000, Russell King (Oracle) wrote:
> Move phylink_resume() to be after the setup in stmmac_resume() has
> completed, as phylink_resume() may result in an immediate call to the
> .mac_link_up method, which will enable the transmitter and receiver,
> and enable the transmit queues.
> 
> This behaviour has been witnessed by Jon Hunter on the Jetson TX2
> platform (Tegra 186).
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH RFC net-next 5/5] net: stmmac: fix resume when media is in low-power mode

[Russell King (Oracle)]

The Synopsys Designwavre GMAC core databook requires all clocks to be
active in order to complete software reset.

This means if the PHY receive clock has been stopped due to the media
being in EEE low-power state, and the PHY being permitted to stop its
clock, then software reset will not complete.

Phylink now provides a way to work around this by calling
phylink_prepare_resume() before attempting to issue a reset. This will
prepare any attached PHY by disabling its permission to stop the clock.
phylink_resume() will restore the receive clock stop setting according
to the configuration passed from the netdev driver.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 23c610f7c779..13796b1c8d7c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7925,6 +7925,8 @@ int stmmac_resume(struct device *dev)
 	}
 
 	rtnl_lock();
+	phylink_prepare_resume(priv->phylink);
+
 	mutex_lock(&priv->lock);
 
 	stmmac_reset_queues_param(priv);
-- 
2.30.2

Re: [PATCH RFC net-next 5/5] net: stmmac: fix resume when media is in low-power mode

[Andrew Lunn]

On Thu, Feb 27, 2025 at 02:38:08PM +0000, Russell King (Oracle) wrote:
> The Synopsys Designwavre GMAC core databook requires all clocks to be
> active in order to complete software reset.
> 
> This means if the PHY receive clock has been stopped due to the media
> being in EEE low-power state, and the PHY being permitted to stop its
> clock, then software reset will not complete.
> 
> Phylink now provides a way to work around this by calling
> phylink_prepare_resume() before attempting to issue a reset. This will
> prepare any attached PHY by disabling its permission to stop the clock.
> phylink_resume() will restore the receive clock stop setting according
> to the configuration passed from the netdev driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC 0/5] net: stmmac: fix resume failures due to RX clock

[Jon Hunter]

Hi Russell,

On 27/02/2025 14:37, Russell King (Oracle) wrote:
> Hi,
> 
> This series is likely dependent on the "net: stmmac: cleanup transmit
> clock setting" series which was submitted earlier today.

I tested this series without the above on top of mainline and I still 
saw some issues with suspend. However, when testing this on top of -next 
(which has the referenced series) it works like a charm. So yes it does 
appear to be dependent indeed.

I have tested this on Tegra186, Tegra194 and Tegra234 with -next and all 
are working fine. So with that feel free to add my ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

Re: [PATCH RFC 0/5] net: stmmac: fix resume failures due to RX clock

[Russell King (Oracle)]

On Thu, Mar 06, 2025 at 11:30:53AM +0000, Jon Hunter wrote:
> Hi Russell,
> 
> On 27/02/2025 14:37, Russell King (Oracle) wrote:
> > Hi,
> > 
> > This series is likely dependent on the "net: stmmac: cleanup transmit
> > clock setting" series which was submitted earlier today.
> 
> I tested this series without the above on top of mainline and I still saw
> some issues with suspend. However, when testing this on top of -next (which
> has the referenced series) it works like a charm. So yes it does appear to
> be dependent indeed.
> 
> I have tested this on Tegra186, Tegra194 and Tegra234 with -next and all are
> working fine. So with that feel free to add my ...
> 
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Hi Jon,

I came up with an alternative approach which should make this safer -
for example, if the PHY remains linked with the partner over an
ifdown or module remove/re-insert.

Please see v2 of "net: stmmac: approach 2 to solve EEE LPI reset
issues" which replaces this series.

https://lore.kernel.org/r/Z8m-CRucPxDW5zZK@shell.armlinux.org.uk

Thanks.

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