[PATCH RFC net-next 1/7] net: stmmac: remove unnecessary checks in ethtool eee ops

[PATCH RFC net-next 1/7] net: stmmac: remove unnecessary checks in ethtool eee ops

[Russell King (Oracle)]

Phylink will check whether the MAC supports the LPI methods in
struct phylink_mac_ops, and return -EOPNOTSUPP if the LPI capabilities
are not provided. stmmac doesn't provide LPI capabilities if
priv->dma_cap.eee is not set.

Therefore, checking the state of priv->dma_cap.eee in the ethtool ops
and returning -EOPNOTSUPP is redundant - let phylink handle this.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 77758a7299b4..dda7ba1f524d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -852,9 +852,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	if (!priv->dma_cap.eee)
-		return -EOPNOTSUPP;
-
 	return phylink_ethtool_get_eee(priv->phylink, edata);
 }
 
@@ -863,9 +860,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	if (!priv->dma_cap.eee)
-		return -EOPNOTSUPP;
-
 	return phylink_ethtool_set_eee(priv->phylink, edata);
 }
 
-- 
2.30.2

[PATCH RFC net-next 2/7] net: stmmac: remove write-only mac->pmt

[Russell King (Oracle)]

mac_device_info->pmt is only ever written, nothing reads it. Remove
this struct member.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index cbffccb3b9af..eaa1f2e1c5a5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -602,7 +602,6 @@ struct mac_device_info {
 	unsigned int mcast_bits_log2;
 	unsigned int rx_csum;
 	unsigned int pcs;
-	unsigned int pmt;
 	unsigned int ps;
 	unsigned int xlgmac;
 	unsigned int num_vlan;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f350a6662880..6a4ef32f57ec 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7240,7 +7240,6 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 		priv->plat->enh_desc = priv->dma_cap.enh_desc;
 		priv->plat->pmt = priv->dma_cap.pmt_remote_wake_up &&
 				!(priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL);
-		priv->hw->pmt = priv->plat->pmt;
 		if (priv->dma_cap.hash_tb_sz) {
 			priv->hw->multicast_filter_bins =
 					(BIT(priv->dma_cap.hash_tb_sz) << 5);
-- 
2.30.2

[PATCH RFC net-next 3/7] net: stmmac: remove redundant WoL option validation

[Russell King (Oracle)]

The core ethtool API validates the WoL options passed from userspace
against the support which the driver reports from its get_wol() method,
returning EINVAL if an unsupported mode is requested.

Therefore, there is no need for stmmac to implement its own validation.
Remove this unnecessary code.

See ethnl_set_wol() in net/ethtool/wol.c and ethtool_set_wol() in
net/ethtool/ioctl.c.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index dda7ba1f524d..cd2fb92ac84c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -803,7 +803,6 @@ static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
-	u32 support = WAKE_MAGIC | WAKE_UCAST;
 
 	if (!device_can_wakeup(priv->device))
 		return -EOPNOTSUPP;
@@ -816,15 +815,6 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 		return ret;
 	}
 
-	/* By default almost all GMAC devices support the WoL via
-	 * magic frame but we can disable it if the HW capability
-	 * register shows no support for pmt_magic_frame. */
-	if ((priv->hw_cap_support) && (!priv->dma_cap.pmt_magic_frame))
-		wol->wolopts &= ~WAKE_MAGIC;
-
-	if (wol->wolopts & ~support)
-		return -EINVAL;
-
 	if (wol->wolopts) {
 		pr_info("stmmac: wakeup enable\n");
 		device_set_wakeup_enable(priv->device, 1);
-- 
2.30.2

[PATCH RFC net-next 4/7] net: stmmac: remove unnecessary "stmmac: wakeup enable" print

[Russell King (Oracle)]

Printing "stmmac: wakeup enable" to the kernel log isn't useful - it
doesn't identify the adapter, and is effectively nothing more than a
debugging print. This information can be discovered by looking at
/sys/device.../power/wakeup as the device_set_wakeup_enable() call
updates this sysfs file.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index cd2fb92ac84c..58542b72cc01 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -816,7 +816,6 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	}
 
 	if (wol->wolopts) {
-		pr_info("stmmac: wakeup enable\n");
 		device_set_wakeup_enable(priv->device, 1);
 		/* Avoid unbalanced enable_irq_wake calls */
 		if (priv->wol_irq_disabled)
-- 
2.30.2

[PATCH RFC net-next 0/7] net: stmmac: EEE and WoL cleanups

[Russell King (Oracle)]

Hi,

This series contains a series of cleanup patches for the EEE and WoL
code in stmmac, prompted by issues raised during the last three weeks.

 drivers/net/ethernet/stmicro/stmmac/common.h       |  1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       | 11 +++++++-
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 31 +---------------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 21 ++++++++-------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  4 +--
 5 files changed, 25 insertions(+), 43 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 5/7] net: stmmac: use core wake IRQ support

[Russell King (Oracle)]

The PM core provides management of wake IRQs along side setting the
device wake enable state. In order to use this, we need to register
the interrupt used to wakeup the system using devm_pm_set_wake_irq()
or dev_pm_set_wake_irq(). The core will then enable or disable IRQ
wake state on this interrupt as appropriate.

Make use of this functionality, rather than explicitly managing the
IRQ enable state in the set_wol() ethtool op. This removes the IRQ
wake state management from stmmac.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  1 -
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 14 +-------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  4 ++--
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index cda09cf5dcca..e1df59a643e3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -289,7 +289,6 @@ struct stmmac_priv {
 	u32 msg_enable;
 	int wolopts;
 	int wol_irq;
-	bool wol_irq_disabled;
 	int clk_csr;
 	struct timer_list eee_ctrl_timer;
 	int lpi_irq;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 58542b72cc01..39fa1ec92f82 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -815,19 +815,7 @@ static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 		return ret;
 	}
 
-	if (wol->wolopts) {
-		device_set_wakeup_enable(priv->device, 1);
-		/* Avoid unbalanced enable_irq_wake calls */
-		if (priv->wol_irq_disabled)
-			enable_irq_wake(priv->wol_irq);
-		priv->wol_irq_disabled = false;
-	} else {
-		device_set_wakeup_enable(priv->device, 0);
-		/* Avoid unbalanced disable_irq_wake calls */
-		if (!priv->wol_irq_disabled)
-			disable_irq_wake(priv->wol_irq);
-		priv->wol_irq_disabled = true;
-	}
+	device_set_wakeup_enable(priv->device, !!wol->wolopts);
 
 	mutex_lock(&priv->lock);
 	priv->wolopts = wol->wolopts;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6a4ef32f57ec..7d467b494685 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -29,6 +29,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/prefetch.h>
 #include <linux/pinctrl/consumer.h>
 #ifdef CONFIG_DEBUG_FS
@@ -3724,7 +3725,6 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
 	/* Request the Wake IRQ in case of another line
 	 * is used for WoL
 	 */
-	priv->wol_irq_disabled = true;
 	if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
 		int_name = priv->int_name_wol;
 		sprintf(int_name, "%s:%s", dev->name, "wol");
@@ -3885,7 +3885,6 @@ static int stmmac_request_irq_single(struct net_device *dev)
 	/* Request the Wake IRQ in case of another line
 	 * is used for WoL
 	 */
-	priv->wol_irq_disabled = true;
 	if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
 		ret = request_irq(priv->wol_irq, stmmac_interrupt,
 				  IRQF_SHARED, dev->name, dev);
@@ -7277,6 +7276,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (priv->plat->pmt) {
 		dev_info(priv->device, "Wake-Up On Lan supported\n");
 		device_set_wakeup_capable(priv->device, 1);
+		devm_pm_set_wake_irq(priv->device, priv->wol_irq);
 	}
 
 	if (priv->dma_cap.tsoen)
-- 
2.30.2

[PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

Add two helpers to abstract the WoL enable status at the PHY and MAC to
make the code easier to read.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h          | 10 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 11 +++++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  4 ++--
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index e1df59a643e3..2ae7174ec4b8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -373,6 +373,16 @@ enum stmmac_state {
 	STMMAC_SERVICE_SCHED,
 };
 
+static inline bool stmmac_wol_enabled_mac(struct stmmac_priv *priv)
+{
+	return priv->plat->pmt && device_may_wakeup(priv->device);
+}
+
+static inline bool stmmac_wol_enabled_phy(struct stmmac_priv *priv)
+{
+	return !priv->plat->pmt && device_may_wakeup(priv->device);
+}
+
 int stmmac_mdio_unregister(struct net_device *ndev);
 int stmmac_mdio_register(struct net_device *ndev);
 int stmmac_mdio_reset(struct mii_bus *mii);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7d467b494685..f44f8b1b0efa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7857,7 +7857,7 @@ int stmmac_suspend(struct device *dev)
 		priv->plat->serdes_powerdown(ndev, priv->plat->bsp_priv);
 
 	/* Enable Power down mode by programming the PMT regs */
-	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
+	if (stmmac_wol_enabled_mac(priv)) {
 		stmmac_pmt(priv, priv->hw, priv->wolopts);
 		priv->irq_wake = 1;
 	} else {
@@ -7868,11 +7868,10 @@ int stmmac_suspend(struct device *dev)
 	mutex_unlock(&priv->lock);
 
 	rtnl_lock();
-	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+	if (stmmac_wol_enabled_phy(priv))
 		phylink_speed_down(priv->phylink, false);
 
-	phylink_suspend(priv->phylink,
-			device_may_wakeup(priv->device) && priv->plat->pmt);
+	phylink_suspend(priv->phylink, stmmac_wol_enabled_mac(priv));
 	rtnl_unlock();
 
 	if (stmmac_fpe_supported(priv))
@@ -7939,7 +7938,7 @@ int stmmac_resume(struct device *dev)
 	 * this bit because it can generate problems while resuming
 	 * from another devices (e.g. serial console).
 	 */
-	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
+	if (stmmac_wol_enabled_mac(priv)) {
 		mutex_lock(&priv->lock);
 		stmmac_pmt(priv, priv->hw, 0);
 		mutex_unlock(&priv->lock);
@@ -7992,7 +7991,7 @@ int stmmac_resume(struct device *dev)
 	 * workqueue thread, which will race with initialisation.
 	 */
 	phylink_resume(priv->phylink);
-	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+	if (stmmac_wol_enabled_phy(priv))
 		phylink_speed_up(priv->phylink);
 
 	rtnl_unlock();
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 38b1c04c92a2..19f370bb934c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -954,7 +954,7 @@ static int __maybe_unused stmmac_pltfr_noirq_suspend(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
-	if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
+	if (!stmmac_wol_enabled_mac(priv)) {
 		/* Disable clock in case of PWM is off */
 		clk_disable_unprepare(priv->plat->clk_ptp_ref);
 
@@ -975,7 +975,7 @@ static int __maybe_unused stmmac_pltfr_noirq_resume(struct device *dev)
 	if (!netif_running(ndev))
 		return 0;
 
-	if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
+	if (!stmmac_wol_enabled_mac(priv)) {
 		/* enable the clk previously disabled */
 		ret = pm_runtime_force_resume(dev);
 		if (ret)
-- 
2.30.2

[PATCH RFC net-next 7/7] net: stmmac: explain the phylink_speed_down() call in stmmac_release()

[Russell King (Oracle)]

The call to phylink_speed_down() looks odd on the face of it. Add a
comment to explain why this call is there.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f44f8b1b0efa..0da5c29b8cb0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4138,8 +4138,13 @@ static int stmmac_release(struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	u32 chan;
 
+	/* If the PHY or MAC has WoL enabled, then the PHY will not be
+	 * suspended when phylink_stop() is called below. Set the PHY
+	 * to its slowest speed to save power.
+	 */
 	if (device_may_wakeup(priv->device))
 		phylink_speed_down(priv->phylink, false);
+
 	/* Stop and disconnect the PHY */
 	phylink_stop(priv->phylink);
 	phylink_disconnect_phy(priv->phylink);
-- 
2.30.2

Re: [PATCH RFC net-next 1/7] net: stmmac: remove unnecessary checks in ethtool eee ops

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:45:32PM +0100, Russell King (Oracle) wrote:
> Phylink will check whether the MAC supports the LPI methods in
> struct phylink_mac_ops, and return -EOPNOTSUPP if the LPI capabilities
> are not provided. stmmac doesn't provide LPI capabilities if
> priv->dma_cap.eee is not set.
> 
> Therefore, checking the state of priv->dma_cap.eee in the ethtool ops
> and returning -EOPNOTSUPP is redundant - let phylink handle this.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH RFC net-next 2/7] net: stmmac: remove write-only mac->pmt

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:45:37PM +0100, Russell King (Oracle) wrote:
> mac_device_info->pmt is only ever written, nothing reads it. Remove
> this struct member.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH RFC net-next 3/7] net: stmmac: remove redundant WoL option validation

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:45:42PM +0100, Russell King (Oracle) wrote:
> The core ethtool API validates the WoL options passed from userspace
> against the support which the driver reports from its get_wol() method,
> returning EINVAL if an unsupported mode is requested.
> 
> Therefore, there is no need for stmmac to implement its own validation.
> Remove this unnecessary code.
> 
> See ethnl_set_wol() in net/ethtool/wol.c and ethtool_set_wol() in
> net/ethtool/ioctl.c.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH RFC net-next 4/7] net: stmmac: remove unnecessary "stmmac: wakeup enable" print

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:45:47PM +0100, Russell King (Oracle) wrote:
> Printing "stmmac: wakeup enable" to the kernel log isn't useful - it
> doesn't identify the adapter, and is effectively nothing more than a
> debugging print. This information can be discovered by looking at
> /sys/device.../power/wakeup as the device_set_wakeup_enable() call
> updates this sysfs file.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH RFC net-next 5/7] net: stmmac: use core wake IRQ support

[Andrew Lunn]

> -	if (wol->wolopts) {
> -		device_set_wakeup_enable(priv->device, 1);
> -		/* Avoid unbalanced enable_irq_wake calls */
> -		if (priv->wol_irq_disabled)
> -			enable_irq_wake(priv->wol_irq);
> -		priv->wol_irq_disabled = false;
> -	} else {
> -		device_set_wakeup_enable(priv->device, 0);
> -		/* Avoid unbalanced disable_irq_wake calls */
> -		if (!priv->wol_irq_disabled)
> -			disable_irq_wake(priv->wol_irq);
> -		priv->wol_irq_disabled = true;
> -	}
> +	device_set_wakeup_enable(priv->device, !!wol->wolopts);

It might be worth mentioning in the commit message that
device_set_wakeup_enable() does not care about unbalanced calls to
enable/disable. This obviously caused issues in the past with
enable_irq_wake()/disable_irq_wake().

	Andrew

Re: [PATCH RFC net-next 7/7] net: stmmac: explain the phylink_speed_down() call in stmmac_release()

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:46:02PM +0100, Russell King (Oracle) wrote:
> The call to phylink_speed_down() looks odd on the face of it. Add a
> comment to explain why this call is there.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Andrew Lunn]

> +static inline bool stmmac_wol_enabled_mac(struct stmmac_priv *priv)
> +{
> +	return priv->plat->pmt && device_may_wakeup(priv->device);
> +}
> +
> +static inline bool stmmac_wol_enabled_phy(struct stmmac_priv *priv)
> +{
> +	return !priv->plat->pmt && device_may_wakeup(priv->device);
> +}

I agree this is a direct translation into a helper.

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

I'm guessing at some point you want to change these two
helpers. e.g. at some point, you want to try getting the PHY to do the
WoL, independent of !priv->plat->pmt? 

> -	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> +	if (stmmac_wol_enabled_phy(priv))
>  		phylink_speed_down(priv->phylink, false);

This might be related to the next patch. But why only do speed down
when PHY is doing WoL? If the MAC is doing WoL, you could also do a
speed_down.

	Andrew

Re: [PATCH RFC net-next 7/7] net: stmmac: explain the phylink_speed_down() call in stmmac_release()

[Andrew Lunn]

On Mon, Jul 28, 2025 at 04:46:02PM +0100, Russell King (Oracle) wrote:
> The call to phylink_speed_down() looks odd on the face of it. Add a
> comment to explain why this call is there.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f44f8b1b0efa..0da5c29b8cb0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4138,8 +4138,13 @@ static int stmmac_release(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u32 chan;
>  
> +	/* If the PHY or MAC has WoL enabled, then the PHY will not be
> +	 * suspended when phylink_stop() is called below. Set the PHY
> +	 * to its slowest speed to save power.
> +	 */
>  	if (device_may_wakeup(priv->device))
>  		phylink_speed_down(priv->phylink, false);
> +

Is there a corresponding phylink_speed_up() somewhere else? Does that
need a similar comment?

	Andrew

Re: [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Mon, Jul 28, 2025 at 07:28:01PM +0200, Andrew Lunn wrote:
> > +static inline bool stmmac_wol_enabled_mac(struct stmmac_priv *priv)
> > +{
> > +	return priv->plat->pmt && device_may_wakeup(priv->device);
> > +}
> > +
> > +static inline bool stmmac_wol_enabled_phy(struct stmmac_priv *priv)
> > +{
> > +	return !priv->plat->pmt && device_may_wakeup(priv->device);
> > +}
> 
> I agree this is a direct translation into a helper.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> I'm guessing at some point you want to change these two
> helpers. e.g. at some point, you want to try getting the PHY to do the
> WoL, independent of !priv->plat->pmt? 
> 
> > -	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> > +	if (stmmac_wol_enabled_phy(priv))
> >  		phylink_speed_down(priv->phylink, false);
> 
> This might be related to the next patch. But why only do speed down
> when PHY is doing WoL? If the MAC is doing WoL, you could also do a
> speed_down.

No idea, but that's what the code currently does, and, as ever with
a cleanup series, I try to avoid functional changes in cleanup series.

Also, bear in mind that I can't test any of this.

We haven't yet been successful in getting WoL working in mainline. It
_seems_ that the Jetson Xaiver NX platform should be using PHY mode,
but the Realtek PHY driver is definitely broken for WoL. Even with
that hacked, and along with other fixes that I've been given, I still
can't get the SoC to wake up via WoL. In fact, the changes to change
DT to specify the PHY interrupt as being routed through the PM
controller results in normal PHY link up/down interrupts no longer
working.

I'd like someone else to test functional changes!

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Gatien CHEVALLIER]

On 7/28/25 19:54, Russell King (Oracle) wrote:
> On Mon, Jul 28, 2025 at 07:28:01PM +0200, Andrew Lunn wrote:
>>> +static inline bool stmmac_wol_enabled_mac(struct stmmac_priv *priv)
>>> +{
>>> +	return priv->plat->pmt && device_may_wakeup(priv->device);
>>> +}
>>> +
>>> +static inline bool stmmac_wol_enabled_phy(struct stmmac_priv *priv)
>>> +{
>>> +	return !priv->plat->pmt && device_may_wakeup(priv->device);
>>> +}
>>
>> I agree this is a direct translation into a helper.
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>
>> I'm guessing at some point you want to change these two
>> helpers. e.g. at some point, you want to try getting the PHY to do the
>> WoL, independent of !priv->plat->pmt?
>>
>>> -	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
>>> +	if (stmmac_wol_enabled_phy(priv))
>>>   		phylink_speed_down(priv->phylink, false);
>>
>> This might be related to the next patch. But why only do speed down
>> when PHY is doing WoL? If the MAC is doing WoL, you could also do a
>> speed_down.
> 
> No idea, but that's what the code currently does, and, as ever with
> a cleanup series, I try to avoid functional changes in cleanup series.
> 
> Also, bear in mind that I can't test any of this.
> 
> We haven't yet been successful in getting WoL working in mainline. It
> _seems_ that the Jetson Xaiver NX platform should be using PHY mode,
> but the Realtek PHY driver is definitely broken for WoL. Even with
> that hacked, and along with other fixes that I've been given, I still
> can't get the SoC to wake up via WoL. In fact, the changes to change
> DT to specify the PHY interrupt as being routed through the PM
> controller results in normal PHY link up/down interrupts no longer
> working.
> 
> I'd like someone else to test functional changes!
> 

Hello Russel,

First of all, thank you for taking the time to improve this code.
What exactly do you mean by hacking? Forcing !priv->plat->pmt?

Re: [PATCH RFC net-next 7/7] net: stmmac: explain the phylink_speed_down() call in stmmac_release()

[Russell King (Oracle)]

On Mon, Jul 28, 2025 at 07:29:09PM +0200, Andrew Lunn wrote:
> On Mon, Jul 28, 2025 at 04:46:02PM +0100, Russell King (Oracle) wrote:
> > The call to phylink_speed_down() looks odd on the face of it. Add a
> > comment to explain why this call is there.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f44f8b1b0efa..0da5c29b8cb0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4138,8 +4138,13 @@ static int stmmac_release(struct net_device *dev)
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	u32 chan;
> >  
> > +	/* If the PHY or MAC has WoL enabled, then the PHY will not be
> > +	 * suspended when phylink_stop() is called below. Set the PHY
> > +	 * to its slowest speed to save power.
> > +	 */
> >  	if (device_may_wakeup(priv->device))
> >  		phylink_speed_down(priv->phylink, false);
> > +
> 
> Is there a corresponding phylink_speed_up() somewhere else? Does that
> need a similar comment?

__stmmac_open() does:

        phylink_start(priv->phylink);
        /* We may have called phylink_speed_down before */
        phylink_speed_up(priv->phylink);

So yes, there is a corresponding call, and it's unconditional, so such
a comment there wouldn't make sense.

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

> On 7/28/25 19:54, Russell King (Oracle) wrote:
> > On Mon, Jul 28, 2025 at 07:28:01PM +0200, Andrew Lunn wrote:
> > > > +static inline bool stmmac_wol_enabled_mac(struct stmmac_priv *priv)
> > > > +{
> > > > +	return priv->plat->pmt && device_may_wakeup(priv->device);
> > > > +}
> > > > +
> > > > +static inline bool stmmac_wol_enabled_phy(struct stmmac_priv *priv)
> > > > +{
> > > > +	return !priv->plat->pmt && device_may_wakeup(priv->device);
> > > > +}
> > > 
> > > I agree this is a direct translation into a helper.
> > > 
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > 
> > > I'm guessing at some point you want to change these two
> > > helpers. e.g. at some point, you want to try getting the PHY to do the
> > > WoL, independent of !priv->plat->pmt?
> > > 
> > > > -	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> > > > +	if (stmmac_wol_enabled_phy(priv))
> > > >   		phylink_speed_down(priv->phylink, false);
> > > 
> > > This might be related to the next patch. But why only do speed down
> > > when PHY is doing WoL? If the MAC is doing WoL, you could also do a
> > > speed_down.
> > 
> > No idea, but that's what the code currently does, and, as ever with
> > a cleanup series, I try to avoid functional changes in cleanup series.
> > 
> > Also, bear in mind that I can't test any of this.
> > 
> > We haven't yet been successful in getting WoL working in mainline. It
> > _seems_ that the Jetson Xaiver NX platform should be using PHY mode,
> > but the Realtek PHY driver is definitely broken for WoL. Even with
> > that hacked, and along with other fixes that I've been given, I still
> > can't get the SoC to wake up via WoL. In fact, the changes to change
> > DT to specify the PHY interrupt as being routed through the PM
> > controller results in normal PHY link up/down interrupts no longer
> > working.
> > 
> > I'd like someone else to test functional changes!
> > 
> 
> Hello Russel,

That's "Russell" please.

> First of all, thank you for taking the time to improve this code.
> What exactly do you mean by hacking? Forcing !priv->plat->pmt?

No. There's a cleaner way to clear ->pmt which isn't hacky. Thierry's
patch to .dts also isn't hacky.

With Thierry's .dts patch, PHY interrupts completely stop working, so
we don't get link change notifications anymore - and we still don't
seem to be capable of waking the system up with the PHY interrupt
being asserted.

Without Thierry's .dts patch, as I predicted, enabling WoL at the
PHY results in Bad Stuff happening - the code in the realtek driver
for WoL is quite simply broken and wrong.

Switching the pin from INTB mode to PMEB mode results in:
- No link change interrupts once WoL is enabled
- The interrupt output being stuck at active level, causing an
  interrupt storm and the interrupt is eventually disabled.
  The PHY can be configured to pulse the PMEB or hold at an active
  level until the WoL is cleared - and by default it's the latter.

So, switching the interrupt pin to PMEB mode is simply wrong and
breaks phylib. I guess the original WoL support was only tested on
a system which didn't use the PHY interrupt, only using the interrupt
pin for wake-up purposes.

I was working on the realtek driver to fix this, but it's pointless
spending time on this until the rest of the system can wake up -
and thus the changes can be tested. This is where I got to (and
includes work from both Thierry and myself, so please don't pick
this up as-is, because I can guarantee that you'll get the sign-offs
wrong! It's a work-in-progress, and should be a series for submission.)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
index a410fc335fa3..f7c9c14b095e 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
@@ -39,8 +39,8 @@ mdio {
 				phy: ethernet-phy@0 {
 					compatible = "ethernet-phy-ieee802.3-c22";
 					reg = <0x0>;
-					interrupt-parent = <&gpio>;
-					interrupts = <TEGRA194_MAIN_GPIO(G, 4) IRQ_TYPE_LEVEL_LOW>;
+					interrupt-parent = <&pmc>;
+					interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
 					#phy-cells = <0>;
 				};
 			};
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index 09ae16e026eb..bcaa37e08345 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -261,7 +261,8 @@ static int tegra_eqos_probe(struct platform_device *pdev,
 	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
 	plat_dat->bsp_priv = eqos;
 	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE |
-			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
+			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP |
+			   STMMAC_FLAG_USE_PHY_WOL;
 
 	return 0;
 
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index dd0d675149ad..fdd926d3ab83 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -31,6 +31,7 @@
 #define RTL821x_INER				0x12
 #define RTL8211B_INER_INIT			0x6400
 #define RTL8211E_INER_LINK_STATUS		BIT(10)
+#define RTL8211F_INER_WOL			BIT(7)
 #define RTL8211F_INER_LINK_STATUS		BIT(4)
 
 #define RTL821x_INSR				0x13
@@ -426,12 +427,15 @@ static irqreturn_t rtl8211f_handle_interrupt(struct phy_device *phydev)
 		return IRQ_NONE;
 	}
 
-	if (!(irq_status & RTL8211F_INER_LINK_STATUS))
-		return IRQ_NONE;
+	if (irq_status & RTL8211F_INER_LINK_STATUS) {
+		phy_trigger_machine(phydev);
+		return IRQ_HANDLED;
+	}
 
-	phy_trigger_machine(phydev);
+	if (irq_status & RTL8211F_INER_WOL)
+		return IRQ_HANDLED;
 
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static void rtl8211f_get_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
@@ -451,6 +455,7 @@ static void rtl8211f_get_wol(struct phy_device *dev, struct ethtool_wolinfo *wol
 static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
 {
 	const u8 *mac_addr = dev->attached_dev->dev_addr;
+	struct rtl821x_priv *priv = phydev->priv;
 	int oldpage;
 
 	oldpage = phy_save_page(dev);
@@ -470,23 +475,44 @@ static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
 		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
 
 		/* Enable the WOL interrupt */
-		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
-		__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
+		//__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		rtl821x_write_page(dev, 0xa42);
+		__phy_set_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
 	} else {
 		/* Disable the WOL interrupt */
-		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
-		__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
+		//__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		rtl821x_write_page(dev, 0xa42);
+		__phy_clear_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
 
 		/* Disable magic packet matching and reset WOL status */
 		rtl821x_write_page(dev, RTL8211F_WOL_SETTINGS_PAGE);
 		__phy_write(dev, RTL8211F_WOL_SETTINGS_EVENTS, 0);
 		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
 	}
+	priv->saved_wolopts = wolopts;
 
 err:
 	return phy_restore_page(dev, oldpage, 0);
 }
 
+static int rtl8211f_suspend(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret = rtl821x_suspend(phydev);
+
+	return ret;
+}
+
+static int rtl8211f_resume(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret = rtl821x_resume(phydev);
+
+	return ret;
+}
+
 static int rtl8211_config_aneg(struct phy_device *phydev)
 {
 	int ret;
@@ -1619,8 +1645,8 @@ static struct phy_driver realtek_drvs[] = {
 		.handle_interrupt = rtl8211f_handle_interrupt,
 		.set_wol	= rtl8211f_set_wol,
 		.get_wol	= rtl8211f_get_wol,
-		.suspend	= rtl821x_suspend,
-		.resume		= rtl821x_resume,
+		.suspend	= rtl8211f_suspend,
+		.resume		= rtl8211f_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 		.flags		= PHY_ALWAYS_CALL_SUSPEND,

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
> Without Thierry's .dts patch, as I predicted, enabling WoL at the
> PHY results in Bad Stuff happening - the code in the realtek driver
> for WoL is quite simply broken and wrong.
> 
> Switching the pin from INTB mode to PMEB mode results in:
> - No link change interrupts once WoL is enabled
> - The interrupt output being stuck at active level, causing an
>   interrupt storm and the interrupt is eventually disabled.
>   The PHY can be configured to pulse the PMEB or hold at an active
>   level until the WoL is cleared - and by default it's the latter.
> 
> So, switching the interrupt pin to PMEB mode is simply wrong and
> breaks phylib. I guess the original WoL support was only tested on
> a system which didn't use the PHY interrupt, only using the interrupt
> pin for wake-up purposes.

I will also state that the only way the WoL support for Realtek that
was merged can possibly work is if there is some other agent in the
system which configures the PHY such that PMEB only pulses on WoL
packet reception. Unless this is the case, the PMEB pin will be
pulled active on the first matching WoL packet, and remain there.
That means WoL will work for the first attempt and not after.

This makes the WoL support that was merged completely broken for the
general case where there isn't some other agent configuring the PHY
e.g. at boot time.

I am in two minds whether it should be reverted until it can be
correctly implemented. It's a relatively recent addition to the
kernel - the patch is dated 29th April 2025. See
https://patch.msgid.link/20250429-realtek_wol-v2-1-8f84def1ef2c@kuka.com

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
> With Thierry's .dts patch, PHY interrupts completely stop working, so
> we don't get link change notifications anymore - and we still don't
> seem to be capable of waking the system up with the PHY interrupt
> being asserted.
> 
> Without Thierry's .dts patch, as I predicted, enabling WoL at the
> PHY results in Bad Stuff happening - the code in the realtek driver
> for WoL is quite simply broken and wrong.
> 
> Switching the pin from INTB mode to PMEB mode results in:
> - No link change interrupts once WoL is enabled
> - The interrupt output being stuck at active level, causing an
>   interrupt storm and the interrupt is eventually disabled.
>   The PHY can be configured to pulse the PMEB or hold at an active
>   level until the WoL is cleared - and by default it's the latter.
> 
> So, switching the interrupt pin to PMEB mode is simply wrong and
> breaks phylib. I guess the original WoL support was only tested on
> a system which didn't use the PHY interrupt, only using the interrupt
> pin for wake-up purposes.
> 
> I was working on the realtek driver to fix this, but it's pointless
> spending time on this until the rest of the system can wake up -
> and thus the changes can be tested. This is where I got to (and
> includes work from both Thierry and myself, so please don't pick
> this up as-is, because I can guarantee that you'll get the sign-offs
> wrong! It's a work-in-progress, and should be a series for submission.)

Okay, with this patch, wake-up now works on the PHY interrupt line, but
because normal interrupts aren't processed, the interrupt output from
the PHY is stuck at active level, so the system immediately wakes up
from suspend.

Without the normal interrupt problem solved, there's nothing further
I can do on this.

Some of the open questions are:
- whether we should configure the WoL interrupt in the suspend/resume
  function
- the interaction between the WoL interrupt configuration and the
  config_intr method (whether on resume, the WoL interrupt enable
  could get cleared, effectively disabling future WoL, despite it
  being reported as enabled to userspace)
- if we don't mark the PHY as wake-up capable, should we indicate
  that the PHY does not support WoL in .get_wol() and return
  -EOPNOTSUPP for .set_wol() given that we've had broken WoL support
  merged in the recent v6.16 release.

I'm pretty sure that we want all PHYs that support WoL to mark
themselves as a wakeup capable device, so the core wakeup code knows
that the PHY has capabilities, and control the device wakeup enable.
Thus, I think we want to have some of this wakeup handling in the
core phylib code.

However, as normal PHY interrupts don't work, this isn't something I
can pursue further.

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
index a410fc335fa3..8ceba83614ed 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
@@ -39,9 +39,10 @@ mdio {
 				phy: ethernet-phy@0 {
 					compatible = "ethernet-phy-ieee802.3-c22";
 					reg = <0x0>;
-					interrupt-parent = <&gpio>;
-					interrupts = <TEGRA194_MAIN_GPIO(G, 4) IRQ_TYPE_LEVEL_LOW>;
+					interrupt-parent = <&pmc>;
+					interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
 					#phy-cells = <0>;
+					wakeup-source;
 				};
 			};
 		};
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index 09ae16e026eb..bcaa37e08345 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -261,7 +261,8 @@ static int tegra_eqos_probe(struct platform_device *pdev,
 	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
 	plat_dat->bsp_priv = eqos;
 	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE |
-			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
+			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP |
+			   STMMAC_FLAG_USE_PHY_WOL;
 
 	return 0;
 
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index dd0d675149ad..ef10e2c32318 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/of.h>
 #include <linux/phy.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/netdevice.h>
 #include <linux/module.h>
 #include <linux/delay.h>
@@ -31,6 +32,7 @@
 #define RTL821x_INER				0x12
 #define RTL8211B_INER_INIT			0x6400
 #define RTL8211E_INER_LINK_STATUS		BIT(10)
+#define RTL8211F_INER_WOL			BIT(7)
 #define RTL8211F_INER_LINK_STATUS		BIT(4)
 
 #define RTL821x_INSR				0x13
@@ -255,6 +257,28 @@ static int rtl821x_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int rtl8211f_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	int ret;
+
+	ret = rtl821x_probe(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* Mark this PHY as wakeup capable and register the interrupt as a
+	 * wakeup IRQ if the PHY is marked as a wakeup source in firmware,
+	 * and the interrupt is valid.
+	 */
+	if (device_property_read_bool(dev, "wakeup-source") &&
+	    phy_interrupt_is_valid(phydev)) {
+		device_set_wakeup_capable(dev, true);
+		devm_pm_set_wake_irq(dev, phydev->irq);
+	}
+
+	return ret;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -426,12 +450,17 @@ static irqreturn_t rtl8211f_handle_interrupt(struct phy_device *phydev)
 		return IRQ_NONE;
 	}
 
-	if (!(irq_status & RTL8211F_INER_LINK_STATUS))
-		return IRQ_NONE;
+	if (irq_status & RTL8211F_INER_LINK_STATUS) {
+		phy_trigger_machine(phydev);
+		return IRQ_HANDLED;
+	}
 
-	phy_trigger_machine(phydev);
+	if (irq_status & RTL8211F_INER_WOL) {
+		pm_wakeup_event(&phydev->mdio.dev, 0);
+		return IRQ_HANDLED;
+	}
 
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static void rtl8211f_get_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
@@ -470,12 +499,16 @@ static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
 		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
 
 		/* Enable the WOL interrupt */
-		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
-		__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
+		//__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		rtl821x_write_page(dev, 0xa42);
+		__phy_set_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
 	} else {
 		/* Disable the WOL interrupt */
-		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
-		__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
+		//__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
+		rtl821x_write_page(dev, 0xa42);
+		__phy_clear_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
 
 		/* Disable magic packet matching and reset WOL status */
 		rtl821x_write_page(dev, RTL8211F_WOL_SETTINGS_PAGE);
@@ -483,10 +516,30 @@ static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
 		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
 	}
 
+	device_set_wakeup_enable(&dev->mdio.dev, !!(wol->wolopts & WAKE_MAGIC));
+
 err:
 	return phy_restore_page(dev, oldpage, 0);
 }
 
+static int rtl821x_suspend(struct phy_device *phydev);
+static int rtl8211f_suspend(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret = rtl821x_suspend(phydev);
+
+	return ret;
+}
+
+static int rtl821x_resume(struct phy_device *phydev);
+static int rtl8211f_resume(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret = rtl821x_resume(phydev);
+
+	return ret;
+}
+
 static int rtl8211_config_aneg(struct phy_device *phydev)
 {
 	int ret;
@@ -1612,15 +1665,15 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
-		.probe		= rtl821x_probe,
+		.probe		= rtl8211f_probe,
 		.config_init	= &rtl8211f_config_init,
 		.read_status	= rtlgen_read_status,
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
 		.set_wol	= rtl8211f_set_wol,
 		.get_wol	= rtl8211f_get_wol,
-		.suspend	= rtl821x_suspend,
-		.resume		= rtl821x_resume,
+		.suspend	= rtl8211f_suspend,
+		.resume		= rtl8211f_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 		.flags		= PHY_ALWAYS_CALL_SUSPEND,

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Gatien CHEVALLIER]

On 7/29/25 14:45, Russell King (Oracle) wrote:
> On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
>> With Thierry's .dts patch, PHY interrupts completely stop working, so
>> we don't get link change notifications anymore - and we still don't
>> seem to be capable of waking the system up with the PHY interrupt
>> being asserted.
>>
>> Without Thierry's .dts patch, as I predicted, enabling WoL at the
>> PHY results in Bad Stuff happening - the code in the realtek driver
>> for WoL is quite simply broken and wrong.
>>
>> Switching the pin from INTB mode to PMEB mode results in:
>> - No link change interrupts once WoL is enabled
>> - The interrupt output being stuck at active level, causing an
>>    interrupt storm and the interrupt is eventually disabled.
>>    The PHY can be configured to pulse the PMEB or hold at an active
>>    level until the WoL is cleared - and by default it's the latter.
>>
>> So, switching the interrupt pin to PMEB mode is simply wrong and
>> breaks phylib. I guess the original WoL support was only tested on
>> a system which didn't use the PHY interrupt, only using the interrupt
>> pin for wake-up purposes.
>>
>> I was working on the realtek driver to fix this, but it's pointless
>> spending time on this until the rest of the system can wake up -
>> and thus the changes can be tested. This is where I got to (and
>> includes work from both Thierry and myself, so please don't pick
>> this up as-is, because I can guarantee that you'll get the sign-offs
>> wrong! It's a work-in-progress, and should be a series for submission.)
> 
> Okay, with this patch, wake-up now works on the PHY interrupt line, but
> because normal interrupts aren't processed, the interrupt output from
> the PHY is stuck at active level, so the system immediately wakes up
> from suspend.
> 

If I'm following correctly, you do not use the PMEB mode and share
the same pin for WoL and regular interrupts (INTB mode)?

> Without the normal interrupt problem solved, there's nothing further
> I can do on this.
> 
> Some of the open questions are:
> - whether we should configure the WoL interrupt in the suspend/resume
>    function

For the LAN8742 PHY with which I worked with, the recommendation when
using the same pin for WoL and regular interrupt management is to mask
regular interrupt and enable the WoL IIRC.
This prevents the PHY from waking up from undesired events while still
being able use the WoL capability and should be done in suspend() /
resume() callbacks. I guess this means also that you share the same
interrupt handler that must manage both WoL events and regular events.

On the other hand, on the stm32mp135f-dk, the nPME pin (equivalent to
PMEB IIUC) is wired and is different from the nINT pin. Therefore, I
guess it should not be done during suspend()/resume() and it really
depends on how the PHY is wired. Because if a WoL event is received at
runtime, then the PHY must clear the flags otherwise the WoL event won't
trigger a system wakeup afterwards.

I need to look at how the PHYs can handle two different interrupts.

> - the interaction between the WoL interrupt configuration and the
>    config_intr method (whether on resume, the WoL interrupt enable
>    could get cleared, effectively disabling future WoL, despite it
>    being reported as enabled to userspace)
> - if we don't mark the PHY as wake-up capable, should we indicate
>    that the PHY does not support WoL in .get_wol() and return
>    -EOPNOTSUPP for .set_wol() given that we've had broken WoL support
>    merged in the recent v6.16 release.
> 
> I'm pretty sure that we want all PHYs that support WoL to mark
> themselves as a wakeup capable device, so the core wakeup code knows
> that the PHY has capabilities, and control the device wakeup enable.

I agree.

> Thus, I think we want to have some of this wakeup handling in the
> core phylib code.
> 
> However, as normal PHY interrupts don't work, this isn't something I
> can pursue further.
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
> index a410fc335fa3..8ceba83614ed 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi
> @@ -39,9 +39,10 @@ mdio {
>   				phy: ethernet-phy@0 {
>   					compatible = "ethernet-phy-ieee802.3-c22";
>   					reg = <0x0>;
> -					interrupt-parent = <&gpio>;
> -					interrupts = <TEGRA194_MAIN_GPIO(G, 4) IRQ_TYPE_LEVEL_LOW>;
> +					interrupt-parent = <&pmc>;
> +					interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
>   					#phy-cells = <0>;
> +					wakeup-source;
>   				};
>   			};
>   		};
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> index 09ae16e026eb..bcaa37e08345 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> @@ -261,7 +261,8 @@ static int tegra_eqos_probe(struct platform_device *pdev,
>   	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
>   	plat_dat->bsp_priv = eqos;
>   	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE |
> -			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
> +			   STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP |
> +			   STMMAC_FLAG_USE_PHY_WOL;
>   
>   	return 0;
>   
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index dd0d675149ad..ef10e2c32318 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -10,6 +10,7 @@
>   #include <linux/bitops.h>
>   #include <linux/of.h>
>   #include <linux/phy.h>
> +#include <linux/pm_wakeirq.h>
>   #include <linux/netdevice.h>
>   #include <linux/module.h>
>   #include <linux/delay.h>
> @@ -31,6 +32,7 @@
>   #define RTL821x_INER				0x12
>   #define RTL8211B_INER_INIT			0x6400
>   #define RTL8211E_INER_LINK_STATUS		BIT(10)
> +#define RTL8211F_INER_WOL			BIT(7)
>   #define RTL8211F_INER_LINK_STATUS		BIT(4)
>   
>   #define RTL821x_INSR				0x13
> @@ -255,6 +257,28 @@ static int rtl821x_probe(struct phy_device *phydev)
>   	return 0;
>   }
>   
> +static int rtl8211f_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	int ret;
> +
> +	ret = rtl821x_probe(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Mark this PHY as wakeup capable and register the interrupt as a
> +	 * wakeup IRQ if the PHY is marked as a wakeup source in firmware,
> +	 * and the interrupt is valid.
> +	 */
> +	if (device_property_read_bool(dev, "wakeup-source") &&
> +	    phy_interrupt_is_valid(phydev)) {
> +		device_set_wakeup_capable(dev, true);
> +		devm_pm_set_wake_irq(dev, phydev->irq);
> +	}
> +
> +	return ret;
> +}
> +
>   static int rtl8201_ack_interrupt(struct phy_device *phydev)
>   {
>   	int err;
> @@ -426,12 +450,17 @@ static irqreturn_t rtl8211f_handle_interrupt(struct phy_device *phydev)
>   		return IRQ_NONE;
>   	}
>   
> -	if (!(irq_status & RTL8211F_INER_LINK_STATUS))
> -		return IRQ_NONE;
> +	if (irq_status & RTL8211F_INER_LINK_STATUS) {
> +		phy_trigger_machine(phydev);
> +		return IRQ_HANDLED;
> +	}
>   
> -	phy_trigger_machine(phydev);
> +	if (irq_status & RTL8211F_INER_WOL) {
> +		pm_wakeup_event(&phydev->mdio.dev, 0);
> +		return IRQ_HANDLED;
> +	}
>   
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static void rtl8211f_get_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
> @@ -470,12 +499,16 @@ static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
>   		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
>   
>   		/* Enable the WOL interrupt */
> -		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
> -		__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
> +		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
> +		//__phy_set_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
> +		rtl821x_write_page(dev, 0xa42);
> +		__phy_set_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
>   	} else {
>   		/* Disable the WOL interrupt */
> -		rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
> -		__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
> +		//rtl821x_write_page(dev, RTL8211F_INTBCR_PAGE);
> +		//__phy_clear_bits(dev, RTL8211F_INTBCR, RTL8211F_INTBCR_INTB_PMEB);
> +		rtl821x_write_page(dev, 0xa42);
> +		__phy_clear_bits(dev, RTL821x_INER, RTL8211F_INER_WOL);
>   
>   		/* Disable magic packet matching and reset WOL status */
>   		rtl821x_write_page(dev, RTL8211F_WOL_SETTINGS_PAGE);
> @@ -483,10 +516,30 @@ static int rtl8211f_set_wol(struct phy_device *dev, struct ethtool_wolinfo *wol)
>   		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);
>   	}
>   
> +	device_set_wakeup_enable(&dev->mdio.dev, !!(wol->wolopts & WAKE_MAGIC));
> +
>   err:
>   	return phy_restore_page(dev, oldpage, 0);
>   }
>   
> +static int rtl821x_suspend(struct phy_device *phydev);
> +static int rtl8211f_suspend(struct phy_device *phydev)
> +{
> +	struct rtl821x_priv *priv = phydev->priv;
> +	int ret = rtl821x_suspend(phydev);
> +
> +	return ret;
> +}
> +
> +static int rtl821x_resume(struct phy_device *phydev);
> +static int rtl8211f_resume(struct phy_device *phydev)
> +{
> +	struct rtl821x_priv *priv = phydev->priv;
> +	int ret = rtl821x_resume(phydev);
> +
> +	return ret;
> +}
> +
>   static int rtl8211_config_aneg(struct phy_device *phydev)
>   {
>   	int ret;
> @@ -1612,15 +1665,15 @@ static struct phy_driver realtek_drvs[] = {
>   	}, {
>   		PHY_ID_MATCH_EXACT(0x001cc916),
>   		.name		= "RTL8211F Gigabit Ethernet",
> -		.probe		= rtl821x_probe,
> +		.probe		= rtl8211f_probe,
>   		.config_init	= &rtl8211f_config_init,
>   		.read_status	= rtlgen_read_status,
>   		.config_intr	= &rtl8211f_config_intr,
>   		.handle_interrupt = rtl8211f_handle_interrupt,
>   		.set_wol	= rtl8211f_set_wol,
>   		.get_wol	= rtl8211f_get_wol,
> -		.suspend	= rtl821x_suspend,
> -		.resume		= rtl821x_resume,
> +		.suspend	= rtl8211f_suspend,
> +		.resume		= rtl8211f_resume,
>   		.read_page	= rtl821x_read_page,
>   		.write_page	= rtl821x_write_page,
>   		.flags		= PHY_ALWAYS_CALL_SUSPEND,
> 

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 03:10:56PM +0200, Gatien CHEVALLIER wrote:
> 
> 
> On 7/29/25 14:45, Russell King (Oracle) wrote:
> > On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
> > > With Thierry's .dts patch, PHY interrupts completely stop working, so
> > > we don't get link change notifications anymore - and we still don't
> > > seem to be capable of waking the system up with the PHY interrupt
> > > being asserted.
> > > 
> > > Without Thierry's .dts patch, as I predicted, enabling WoL at the
> > > PHY results in Bad Stuff happening - the code in the realtek driver
> > > for WoL is quite simply broken and wrong.
> > > 
> > > Switching the pin from INTB mode to PMEB mode results in:
> > > - No link change interrupts once WoL is enabled
> > > - The interrupt output being stuck at active level, causing an
> > >    interrupt storm and the interrupt is eventually disabled.
> > >    The PHY can be configured to pulse the PMEB or hold at an active
> > >    level until the WoL is cleared - and by default it's the latter.
> > > 
> > > So, switching the interrupt pin to PMEB mode is simply wrong and
> > > breaks phylib. I guess the original WoL support was only tested on
> > > a system which didn't use the PHY interrupt, only using the interrupt
> > > pin for wake-up purposes.
> > > 
> > > I was working on the realtek driver to fix this, but it's pointless
> > > spending time on this until the rest of the system can wake up -
> > > and thus the changes can be tested. This is where I got to (and
> > > includes work from both Thierry and myself, so please don't pick
> > > this up as-is, because I can guarantee that you'll get the sign-offs
> > > wrong! It's a work-in-progress, and should be a series for submission.)
> > 
> > Okay, with this patch, wake-up now works on the PHY interrupt line, but
> > because normal interrupts aren't processed, the interrupt output from
> > the PHY is stuck at active level, so the system immediately wakes up
> > from suspend.
> > 
> 
> If I'm following correctly, you do not use the PMEB mode and share
> the same pin for WoL and regular interrupts (INTB mode)?

As the driver is currently structured, switching the pin to PMEB mode
in .set_wol() breaks phylib, since as soon as one enables WoL at the
PHY, link state interrupts are no longer delivered.

It may be appropriate to switch the pin to PMEB mode in the suspend
method while waiting for a wakeup but we need code in place to deal
with the resulting interrupt storm (by clearing the wakeup) and that
code is missing.

The other approach would be to leave the pin in INTB mode, and
reconfigure the interrupt enable register (INER) to allow WoL
interrupts, maybe disabling link state interrupts (more on that
below.) This has the advantage that reading the interrupt status
register clears the interrupt - and that code already exists so we
avoid the interrupt storm.

> > Without the normal interrupt problem solved, there's nothing further
> > I can do on this.
> > 
> > Some of the open questions are:
> > - whether we should configure the WoL interrupt in the suspend/resume
> >    function
> 
> For the LAN8742 PHY with which I worked with, the recommendation when
> using the same pin for WoL and regular interrupt management is to mask
> regular interrupt and enable the WoL IIRC.

That's only really appropriate if the MAC isn't involved in WoL. Let's
say that the PHY can support magic-packet WoL, but the MAC can also
support unicast frame WoL, and the user has enabled both.

The system goes to sleep (e.g. overnight) and during the night, there's
a hiccup which causes the link to drop and re-establish at a slower
speed.

Since the MAC has not been reconfigured (because the link state
interrupt is disabled, and thus won't wake the system) the MAC can now
no longer receive unicast frames to check whether they match the
despired system wakeup condition.

So, this poses another question: do we really want to support
simultaneous PHY and MAC level WoL support, or should we only allow
one or other device to support WoL?

If we explicitly deny the WoL at both approach, then we don't have
to care about link state interrupts, because the PHY will be able
to cope with the different link speed without needing to wake the
iystem to reconfigure the network interface for the new link
parameters.

> This prevents the PHY from waking up from undesired events while still
> being able use the WoL capability and should be done in suspend() /
> resume() callbacks. I guess this means also that you share the same
> interrupt handler that must manage both WoL events and regular events.
> 
> On the other hand, on the stm32mp135f-dk, the nPME pin (equivalent to
> PMEB IIUC) is wired and is different from the nINT pin. Therefore, I
> guess it should not be done during suspend()/resume() and it really
> depends on how the PHY is wired. Because if a WoL event is received at
> runtime, then the PHY must clear the flags otherwise the WoL event won't
> trigger a system wakeup afterwards.
> 
> I need to look at how the PHYs can handle two different interrupts.

The RTL8211F only has one pin (pin 31) which can be used in INTB mode
or PMEB mode. You can't have independent interrupt and wakeup signals
with this PHY.

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 10:14:18AM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
> > Without Thierry's .dts patch, as I predicted, enabling WoL at the
> > PHY results in Bad Stuff happening - the code in the realtek driver
> > for WoL is quite simply broken and wrong.
> > 
> > Switching the pin from INTB mode to PMEB mode results in:
> > - No link change interrupts once WoL is enabled
> > - The interrupt output being stuck at active level, causing an
> >   interrupt storm and the interrupt is eventually disabled.
> >   The PHY can be configured to pulse the PMEB or hold at an active
> >   level until the WoL is cleared - and by default it's the latter.
> > 
> > So, switching the interrupt pin to PMEB mode is simply wrong and
> > breaks phylib. I guess the original WoL support was only tested on
> > a system which didn't use the PHY interrupt, only using the interrupt
> > pin for wake-up purposes.
> 
> I will also state that the only way the WoL support for Realtek that
> was merged can possibly work is if there is some other agent in the
> system which configures the PHY such that PMEB only pulses on WoL
> packet reception. Unless this is the case, the PMEB pin will be
> pulled active on the first matching WoL packet, and remain there.
> That means WoL will work for the first attempt and not after.
> 
> This makes the WoL support that was merged completely broken for the
> general case where there isn't some other agent configuring the PHY
> e.g. at boot time.
> 
> I am in two minds whether it should be reverted until it can be
> correctly implemented. It's a relatively recent addition to the
> kernel - the patch is dated 29th April 2025. See
> https://patch.msgid.link/20250429-realtek_wol-v2-1-8f84def1ef2c@kuka.com

Oh, and it gets better...

		/* Disable magic packet matching and reset WOL status */
		rtl821x_write_page(dev, RTL8211F_WOL_SETTINGS_PAGE);
		__phy_write(dev, RTL8211F_WOL_SETTINGS_EVENTS, 0);
		__phy_write(dev, RTL8211F_WOL_SETTINGS_STATUS, RTL8211F_WOL_STATUS_RESET);

where RTL8211F_WOL_STATUS_RESET is defined as:

#define RTL8211F_WOL_STATUS_RESET              (BIT(15) | 0x1fff)

Now, this does nothing of the sort. Yes, bit 15 is the WoL reset bit,
but if one bothers to read the application note, one discovers that
bit 15 is _active_ _low_ !

This bit is required to be written as zero to reset the PMEB output
to inactive state. No where in the driver is this done.

Ergo, the addition of WoL support for RTL8211F, basically, is totally
and utterly broken, even if pin 31 is used solely for PMEB
functionality.

Therefore, the only conclusion at this point is that the patch adding
WoL support was likely hardly functionally tested, if at all. Given
everything I've stated about the current code, I think it's safe to
ignore the "functionality" provided by existing code, and not worry
about breaking anyone's setup: it's already completely broken.

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Gatien CHEVALLIER]

On 7/29/25 16:44, Russell King (Oracle) wrote:
> On Tue, Jul 29, 2025 at 03:10:56PM +0200, Gatien CHEVALLIER wrote:
>>
>>
>> On 7/29/25 14:45, Russell King (Oracle) wrote:
>>> On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
>>>> With Thierry's .dts patch, PHY interrupts completely stop working, so
>>>> we don't get link change notifications anymore - and we still don't
>>>> seem to be capable of waking the system up with the PHY interrupt
>>>> being asserted.
>>>>
>>>> Without Thierry's .dts patch, as I predicted, enabling WoL at the
>>>> PHY results in Bad Stuff happening - the code in the realtek driver
>>>> for WoL is quite simply broken and wrong.
>>>>
>>>> Switching the pin from INTB mode to PMEB mode results in:
>>>> - No link change interrupts once WoL is enabled
>>>> - The interrupt output being stuck at active level, causing an
>>>>     interrupt storm and the interrupt is eventually disabled.
>>>>     The PHY can be configured to pulse the PMEB or hold at an active
>>>>     level until the WoL is cleared - and by default it's the latter.
>>>>
>>>> So, switching the interrupt pin to PMEB mode is simply wrong and
>>>> breaks phylib. I guess the original WoL support was only tested on
>>>> a system which didn't use the PHY interrupt, only using the interrupt
>>>> pin for wake-up purposes.
>>>>
>>>> I was working on the realtek driver to fix this, but it's pointless
>>>> spending time on this until the rest of the system can wake up -
>>>> and thus the changes can be tested. This is where I got to (and
>>>> includes work from both Thierry and myself, so please don't pick
>>>> this up as-is, because I can guarantee that you'll get the sign-offs
>>>> wrong! It's a work-in-progress, and should be a series for submission.)
>>>
>>> Okay, with this patch, wake-up now works on the PHY interrupt line, but
>>> because normal interrupts aren't processed, the interrupt output from
>>> the PHY is stuck at active level, so the system immediately wakes up
>>> from suspend.
>>>
>>
>> If I'm following correctly, you do not use the PMEB mode and share
>> the same pin for WoL and regular interrupts (INTB mode)?
> 
> As the driver is currently structured, switching the pin to PMEB mode
> in .set_wol() breaks phylib, since as soon as one enables WoL at the
> PHY, link state interrupts are no longer delivered.
> 
> It may be appropriate to switch the pin to PMEB mode in the suspend
> method while waiting for a wakeup but we need code in place to deal
> with the resulting interrupt storm (by clearing the wakeup) and that
> code is missing.
> 
> The other approach would be to leave the pin in INTB mode, and
> reconfigure the interrupt enable register (INER) to allow WoL
> interrupts, maybe disabling link state interrupts (more on that
> below.) This has the advantage that reading the interrupt status
> register clears the interrupt - and that code already exists so we
> avoid the interrupt storm.
> 
>>> Without the normal interrupt problem solved, there's nothing further
>>> I can do on this.
>>>
>>> Some of the open questions are:
>>> - whether we should configure the WoL interrupt in the suspend/resume
>>>     function
>>
>> For the LAN8742 PHY with which I worked with, the recommendation when
>> using the same pin for WoL and regular interrupt management is to mask
>> regular interrupt and enable the WoL IIRC.
> 
> That's only really appropriate if the MAC isn't involved in WoL. Let's
> say that the PHY can support magic-packet WoL, but the MAC can also
> support unicast frame WoL, and the user has enabled both.
> 

Oops, just saw that I mispronounced your name in a previous message,
sorry for that.

For STMMAC:
I'm a bit lost there. I may be missing something. I thought that using
PHY WoL (therefore having STMMAC_FLAG_USE_PHY_WOL) superseded the MAC
WoL usage.

In stmmac_hw_init():

if (priv->plat->pmt) {
         dev_info(priv->device, "Wake-Up On Lan supported\n");
         device_set_wakeup_capable(priv->device, 1);
}

but above:

priv->plat->pmt = priv->dma_cap.pmt_remote_wake_up &&
!(priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL);

Then, in stmmac_set_wol(), it's either set the WoL for the
PHY or the MAC?

Can you point me to the part I'm missing please?

> The system goes to sleep (e.g. overnight) and during the night, there's
> a hiccup which causes the link to drop and re-establish at a slower
> speed.
> 
> Since the MAC has not been reconfigured (because the link state
> interrupt is disabled, and thus won't wake the system) the MAC can now
> no longer receive unicast frames to check whether they match the
> despired system wakeup condition.
> 
> So, this poses another question: do we really want to support
> simultaneous PHY and MAC level WoL support, or should we only allow
> one or other device to support WoL?
> 
> If we explicitly deny the WoL at both approach, then we don't have
> to care about link state interrupts, because the PHY will be able
> to cope with the different link speed without needing to wake the
> iystem to reconfigure the network interface for the new link
> parameters.
> 
>> This prevents the PHY from waking up from undesired events while still
>> being able use the WoL capability and should be done in suspend() /
>> resume() callbacks. I guess this means also that you share the same
>> interrupt handler that must manage both WoL events and regular events.
>>
>> On the other hand, on the stm32mp135f-dk, the nPME pin (equivalent to
>> PMEB IIUC) is wired and is different from the nINT pin. Therefore, I
>> guess it should not be done during suspend()/resume() and it really
>> depends on how the PHY is wired. Because if a WoL event is received at
>> runtime, then the PHY must clear the flags otherwise the WoL event won't
>> trigger a system wakeup afterwards.
>>
>> I need to look at how the PHYs can handle two different interrupts.
> 
> The RTL8211F only has one pin (pin 31) which can be used in INTB mode
> or PMEB mode. You can't have independent interrupt and wakeup signals
> with this PHY.
> 

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 05:34:49PM +0200, Gatien CHEVALLIER wrote:
> For STMMAC:
> I'm a bit lost there. I may be missing something. I thought that using
> PHY WoL (therefore having STMMAC_FLAG_USE_PHY_WOL) superseded the MAC
> WoL usage.

I'll simply point you to Andrew's message:

https://lore.kernel.org/r/5b8608cb-1369-4638-9cda-1cf90412fc0f@lunn.ch

The PHY and the MAC are supposed to inter-operate so that one ends
up with the union of the WoL capabilities.

stmmac gets this wrong right now, but (as I've written previously)
this is going to be a *very* difficult problem to solve, because
the PHY drivers are - to put it bluntly - "utter crap" when it
comes to WoL.

I'll take the rtl8211f again as an example - its get_wol()
implementation is quite typical of many PHY drivers. Someone comes
along and decides to implement WoL support at the PHY. They add the
.get_wol() method, which unconditionally returns the PHY's hardware
capabilities without regards for the rest of the system.

Consider the case where a PHY supports WoL, but the signalling for
WoL to wake up the system is not wired. The .get_wol() method happily
says that WoL is supported. Let's say that the PHY supports magic
packet, and so does the MAC, and the MAC WoL is functional.

Now, with what Andrew said in his email, and consider what this means.
.set_wol() is called, requesting magic packet. The PHY driver says "oh
yes, the PHY hardware supports this, I'll program the PHY and return
zero". At this point, the MAC thinks the PHY has accepted the WoL
configuration.

The user suspends the system. The user sends the correct magic
packet. The system does not wake up. The user is now confused.

However, if the PHY driver were to behave correctly according to what
Andrew says, and not allow WoL if it can't wake the system, then
instead we would program the MAC to allow magic packet, and the user
would be happy because their system would wake up as expected.

This is why we can't simply "fix" stmmac - not without all the PHY
drivers that are being used with stmmac behaving properly. Can it
ever be fixed to work as Andrew suggests? I really don't know. I
suspect not, because that will probably involve regressing a lot of
setups that work today (fixing the PHY drivers will likely cause
user visible regressions.)

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Andrew Lunn]

> stmmac gets this wrong right now, but (as I've written previously)
> this is going to be a *very* difficult problem to solve, because
> the PHY drivers are - to put it bluntly - "utter crap" when it
> comes to WoL.

Agreed.

> I'll take the rtl8211f again as an example - its get_wol()
> implementation is quite typical of many PHY drivers. Someone comes
> along and decides to implement WoL support at the PHY. They add the
> .get_wol() method, which unconditionally returns the PHY's hardware
> capabilities without regards for the rest of the system.
> 
> Consider the case where a PHY supports WoL, but the signalling for
> WoL to wake up the system is not wired. The .get_wol() method happily
> says that WoL is supported. Let's say that the PHY supports magic
> packet, and so does the MAC, and the MAC WoL is functional.
> 
> Now, with what Andrew said in his email, and consider what this means.
> .set_wol() is called, requesting magic packet. The PHY driver says "oh
> yes, the PHY hardware supports this, I'll program the PHY and return
> zero". At this point, the MAC thinks the PHY has accepted the WoL
> configuration.
> 
> The user suspends the system. The user sends the correct magic
> packet. The system does not wake up. The user is now confused.

There are some MAC drivers which simply trust the PHY. They pass
.get_wol() and .set_wol() direct to the PHY. They don't attempt to
perform MAC WoL, or the MAC driver does not have any hardware support
for it. Such systems are going to end up with a confused user when the
driver says WoL is enabled, but it does not wake.

So while i agree we cannot simply 'fix' stmmac, the issue of PHY
drivers not behaving properly is a bigger problem across a wide range
of MAC drivers.

I think we could quickly improve the situation to some degree by
reviewing the PHY drivers. e.g. the current code in mxl-gpy.c makes it
clear WoL is just another interrupt source. There is no special
pin. So get_wol() needs a call to phy_interrupt_is_valid(phydev) and
return not return any WoL modes if there is not a valid interrupt.

This will not work for all PHYs, e.g. the Marvell 1G PHYs can
repurposed LED2 for WoL indication.

motorcomm.c looks broken. The code suggests WoL is just another
interrupt source, but the driver lacks interrupt handling...

The broadcom code looks like it gets it correct.
bcm54xx_phy_can_wakeup() checks if there is an interrupt or a
dedicated GPIO, and return no wakeup modes if not. KUDOS to Florians
team.

dp83822.c appears to be missing a phy_interrupt_is_valid(phydev),
since WoL appears to be just another interrupt source.

Same for dp83867.c.

And i did notice that the Broadcom code is the only one doing anything
with enable_irq_wake()/disable_irq_wake(). We need to scatter these
into the drivers.

	Andrew

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Russell King (Oracle)]

On Tue, Jul 29, 2025 at 07:27:11PM +0200, Andrew Lunn wrote:
> And i did notice that the Broadcom code is the only one doing anything
> with enable_irq_wake()/disable_irq_wake(). We need to scatter these
> into the drivers.

It's better to use devm_pm_set_wake_irq() in the probe function, and
then let the core code (drivers/base/power/wakeup.c and
drivers/base/power/wakeirq.c) handle it. This is what I'm doing for
the rtl8211f.

IRQ wake gets enabled/disabled at suspend/resume time, rather than
when the device wakeup state changes, which I believe is what is
preferred.

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

Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Florian Fainelli]

On 7/29/25 11:19, Russell King (Oracle) wrote:
> On Tue, Jul 29, 2025 at 07:27:11PM +0200, Andrew Lunn wrote:
>> And i did notice that the Broadcom code is the only one doing anything
>> with enable_irq_wake()/disable_irq_wake(). We need to scatter these
>> into the drivers.
> 
> It's better to use devm_pm_set_wake_irq() in the probe function, and
> then let the core code (drivers/base/power/wakeup.c and
> drivers/base/power/wakeirq.c) handle it. This is what I'm doing for
> the rtl8211f.
> 
> IRQ wake gets enabled/disabled at suspend/resume time, rather than
> when the device wakeup state changes, which I believe is what is
> preferred.
> 

Sounds reasonable, I will go test that instead of doing the 
enable_irq_wake()/disable_irq_wake() dance. Thanks!
-- 
Florian