[PATCH net-next 0/4] net: add WoL from PHY support for stm32mp135f-dk

[PATCH net-next 0/4] net: add WoL from PHY support for stm32mp135f-dk

[Gatien Chevallier]

A previous patchset in drivers/net/phy/smsc.c introduced the WoL
from PHY capability of some PHYs. The Microchip LAN8742 PHY is
present on the stm32mp135f-dk board and posesses this capability.

Therefore, add the possibility to specify in the device tree that,
for a MAC instance, we want to use the WoL from PHY capability
instead of the MAC one.

However, when testing multiple power sequences with magic packets,
the first one succeeded but the following ones failed. This was
caused by uncleared flags in a PHY register. Therefore, I added
a patch to add suspend()/resume() callbacks that handle these.
These callbacks are only implemented for the Microchip LAN8742 as I
have no way of testing it for other WoL capable PHYs.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
Gatien Chevallier (4):
      dt-bindings: net: document st,phy-wol property
      net: stmmac: stm32: add WoL from PHY support
      net: phy: smsc: fix and improve WoL support
      arm: dts: st: activate ETH1 WoL from PHY on stm32mp135f-dk

 .../devicetree/bindings/net/stm32-dwmac.yaml       |  6 ++++
 arch/arm/boot/dts/st/stm32mp135f-dk.dts            |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  |  5 +++
 drivers/net/phy/smsc.c                             | 42 +++++++++++++++++++---
 include/linux/smscphy.h                            |  2 ++
 5 files changed, 52 insertions(+), 4 deletions(-)
---
base-commit: 4701ee5044fb3992f1c910630a9673c2dc600ce5
change-id: 20250721-wol-smsc-phy-ff3b40848852

Best regards,
-- 
Gatien Chevallier <gatien.chevallier@foss.st.com>

[PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien Chevallier]

The "st,phy-wol" property can be set to use the wakeup capability of
the PHY instead of the MAC.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
index 987254900d0da7aab81237f20b1540ad8a17bd21..985bd4c320b3e07fd1cd0aa398d6cce0b55a4e4d 100644
--- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
@@ -121,6 +121,12 @@ properties:
     minItems: 1
     maxItems: 2
 
+  st,phy-wol:
+    description:
+      set this property to use the wakeup capability from the PHY, if supported, instead of the
+      MAC.
+    type: boolean
+
 required:
   - compatible
   - clocks

-- 
2.35.3

[PATCH net-next 2/4] net: stmmac: stm32: add WoL from PHY support

[Gatien Chevallier]

If the "st,phy-wol" property is present in the device tree node,
set the STMMAC_FLAG_USE_PHY_WOL flag to use the WoL capability of
the PHY.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index 1eb16eec9c0d26eb21238433a77d77b4486f4ac3..443d4cec5d8c6bf074c2fabc75b97997b1020fe8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -106,6 +106,7 @@ struct stm32_dwmac {
 	u32 speed;
 	const struct stm32_ops *ops;
 	struct device *dev;
+	bool phy_wol;
 };
 
 struct stm32_ops {
@@ -433,6 +434,8 @@ static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
 		}
 	}
 
+	dwmac->phy_wol = of_property_read_bool(np, "st,phy-wol");
+
 	return err;
 }
 
@@ -535,6 +538,8 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
 
 	plat_dat->flags |= STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
 	plat_dat->bsp_priv = dwmac;
+	if (dwmac->phy_wol)
+		plat_dat->flags |= STMMAC_FLAG_USE_PHY_WOL;
 
 	ret = stm32_dwmac_init(plat_dat);
 	if (ret)

-- 
2.35.3

[PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Gatien Chevallier]

Add suspend()/resume() callbacks that do not shut down the PHY if the
WoL is supported and handle the WoL status flags.

If the WoL is supported by the PHY, indicate that the PHY device can
be a source of wake up for the platform. When setting the WoL
configuration, enable this capability.

Fixes: 8b305ee2a91c ("net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs")
Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/net/phy/smsc.c  | 42 ++++++++++++++++++++++++++++++++++++++----
 include/linux/smscphy.h |  2 ++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b6489da5cfcdfb405457058dc88575c0d84d259d..cf4e763907aefd2d725c734d3e0f2926128f770e 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -537,14 +537,45 @@ static int lan874x_set_wol(struct phy_device *phydev,
 		}
 	}
 
-	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
-			   val_wucsr);
+	/* Enable wakeup on PHY device if at least one WoL feature is configured */
+	device_set_wakeup_enable(&phydev->mdio.dev, !!(val_wucsr & MII_LAN874X_PHY_WOL_MASK));
+
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR, val_wucsr);
 	if (rc < 0)
 		return rc;
 
 	return 0;
 }
 
+static int smsc_phy_suspend(struct phy_device *phydev)
+{
+	if (!phydev->wol_enabled)
+		return genphy_suspend(phydev);
+
+	return 0;
+}
+
+static int smsc_phy_resume(struct phy_device *phydev)
+{
+	int rc;
+
+	if (!phydev->wol_enabled)
+		return genphy_resume(phydev);
+
+	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
+	if (rc < 0)
+		return rc;
+
+	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
+		return 0;
+
+	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");
+	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
+			   rc | MII_LAN874X_PHY_WOL_STATUS_MASK);
+
+	return rc;
+}
+
 static int smsc_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(smsc_hw_stats);
@@ -673,6 +704,9 @@ int smsc_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	if (phydev->drv->set_wol)
+		device_set_wakeup_capable(&phydev->mdio.dev, true);
+
 	/* Make clk optional to keep DTB backward compatibility. */
 	refclk = devm_clk_get_optional_enabled_with_rate(dev, NULL,
 							 50 * 1000 * 1000);
@@ -875,8 +909,8 @@ static struct phy_driver smsc_phy_driver[] = {
 	.set_wol	= lan874x_set_wol,
 	.get_wol	= lan874x_get_wol,
 
-	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	.suspend	= smsc_phy_suspend,
+	.resume		= smsc_phy_resume,
 } };
 
 module_phy_driver(smsc_phy_driver);
diff --git a/include/linux/smscphy.h b/include/linux/smscphy.h
index 1a6a851d2cf80d225bada7adeb79969e625964bd..cdf266960032609241afc8316da23f1c4834bfee 100644
--- a/include/linux/smscphy.h
+++ b/include/linux/smscphy.h
@@ -65,6 +65,8 @@ int smsc_phy_probe(struct phy_device *phydev);
 #define MII_LAN874X_PHY_WOL_WUEN		BIT(2)
 #define MII_LAN874X_PHY_WOL_MPEN		BIT(1)
 #define MII_LAN874X_PHY_WOL_BCSTEN		BIT(0)
+#define MII_LAN874X_PHY_WOL_MASK		GENMASK(4, 0)
+#define MII_LAN874X_PHY_WOL_STATUS_MASK		GENMASK(7, 4)
 
 #define MII_LAN874X_PHY_WOL_FILTER_EN		BIT(15)
 #define MII_LAN874X_PHY_WOL_FILTER_MCASTTEN	BIT(9)

-- 
2.35.3

[PATCH net-next 4/4] arm: dts: st: activate ETH1 WoL from PHY on stm32mp135f-dk

[Gatien Chevallier]

On this board, the ETH1 supports WoL from PHY. Add the "st,phy-wol"
property to support it.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 arch/arm/boot/dts/st/stm32mp135f-dk.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/st/stm32mp135f-dk.dts b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
index 9764a6bfa5b428c8524a5902c10b7807dda46b3d..d746424b039013759bfbcce5193a701ff775e715 100644
--- a/arch/arm/boot/dts/st/stm32mp135f-dk.dts
+++ b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
@@ -193,6 +193,7 @@ &ethernet1 {
 	pinctrl-names = "default", "sleep";
 	phy-mode = "rmii";
 	phy-handle = <&phy0_eth1>;
+	st,phy-wol;
 
 	mdio {
 		#address-cells = <1>;

-- 
2.35.3

Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Russell King (Oracle)]

On Mon, Jul 21, 2025 at 01:14:45PM +0200, Gatien Chevallier wrote:
> +static int smsc_phy_suspend(struct phy_device *phydev)
> +{
> +	if (!phydev->wol_enabled)
> +		return genphy_suspend(phydev);

This should not be necessary. Take a look at phy_suspend(). Notice:

        phydev->wol_enabled = phy_drv_wol_enabled(phydev) ||
                              (netdev && netdev->ethtool->wol_enabled);
        /* If the device has WOL enabled, we cannot suspend the PHY */
        if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
                return -EBUSY;

PHY_ALWAYS_CALL_SUSPEND is not set for this PHY, therefore if
phydev->wol_enabled is set by the above code, phydrv->suspend will
not be called.

> +
> +	return 0;
> +}
> +
> +static int smsc_phy_resume(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	if (!phydev->wol_enabled)
> +		return genphy_resume(phydev);
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
> +		return 0;
> +
> +	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");
> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
> +			   rc | MII_LAN874X_PHY_WOL_STATUS_MASK);
> +
> +	return rc;

Note that this will be called multiple times, e.g. during attachment of
the PHY to the network device, when the device is opened, etc even
without ->suspend having been called, and before ->wol_enabled has
been set. Make sure your code is safe for this.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Krzysztof Kozlowski]

On 21/07/2025 13:14, Gatien Chevallier wrote:
> The "st,phy-wol" property can be set to use the wakeup capability of
> the PHY instead of the MAC.


And why would that be property of a SoC or board? Word "can" suggests
you are documenting something which exists, but this does not exist.

Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

Hello Krzysztof,

On 7/21/25 13:30, Krzysztof Kozlowski wrote:
> On 21/07/2025 13:14, Gatien Chevallier wrote:
>> The "st,phy-wol" property can be set to use the wakeup capability of
>> the PHY instead of the MAC.
> 
> 
> And why would that be property of a SoC or board? Word "can" suggests
> you are documenting something which exists, but this does not exist.
Can you elaborate a bit more on the "not existing" part please?

For the WoL from PHY to be supported, the PHY line that is raised
(On nPME pin for this case) when receiving a wake up event has to be
wired to a wakeup event input of the Extended interrupt and event
controller(EXTI), and that's implementation dependent.

Best regards,
Gatien

> 
> Best regards,
> Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Krzysztof Kozlowski]

On 21/07/2025 14:10, Gatien CHEVALLIER wrote:
> Hello Krzysztof,
> 
> On 7/21/25 13:30, Krzysztof Kozlowski wrote:
>> On 21/07/2025 13:14, Gatien Chevallier wrote:
>>> The "st,phy-wol" property can be set to use the wakeup capability of
>>> the PHY instead of the MAC.
>>
>>
>> And why would that be property of a SoC or board? Word "can" suggests
>> you are documenting something which exists, but this does not exist.
> Can you elaborate a bit more on the "not existing" part please?


Where does this property exist that you suggest that a new binding "can"
use it?

> 
> For the WoL from PHY to be supported, the PHY line that is raised
> (On nPME pin for this case) when receiving a wake up event has to be
> wired to a wakeup event input of the Extended interrupt and event
> controller(EXTI), and that's implementation dependent.


So it is not "can" but some implementations do not have proper wiring.
You need to justify your commits and changes.



Best regards,
Krzysztof

Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Gatien CHEVALLIER]

Hello Russel,

On 7/21/25 13:28, Russell King (Oracle) wrote:
> On Mon, Jul 21, 2025 at 01:14:45PM +0200, Gatien Chevallier wrote:
>> +static int smsc_phy_suspend(struct phy_device *phydev)
>> +{
>> +	if (!phydev->wol_enabled)
>> +		return genphy_suspend(phydev);
> 
> This should not be necessary. Take a look at phy_suspend(). Notice:
> 
>          phydev->wol_enabled = phy_drv_wol_enabled(phydev) ||
>                                (netdev && netdev->ethtool->wol_enabled);
>          /* If the device has WOL enabled, we cannot suspend the PHY */
>          if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
>                  return -EBUSY;
> 
> PHY_ALWAYS_CALL_SUSPEND is not set for this PHY, therefore if
> phydev->wol_enabled is set by the above code, phydrv->suspend will
> not be called.
> 

Indeed, thank you for pointing this out. I will remove this callback for
v2.

>> +
>> +	return 0;
>> +}
>> +
>> +static int smsc_phy_resume(struct phy_device *phydev)
>> +{
>> +	int rc;
>> +
>> +	if (!phydev->wol_enabled)
>> +		return genphy_resume(phydev);
>> +
>> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
>> +		return 0;
>> +
>> +	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");
>> +	rc = phy_write_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR,
>> +			   rc | MII_LAN874X_PHY_WOL_STATUS_MASK);
>> +
>> +	return rc;
> 
> Note that this will be called multiple times, e.g. during attachment of
> the PHY to the network device, when the device is opened, etc even
> without ->suspend having been called, and before ->wol_enabled has
> been set. Make sure your code is safe for this.
> 

If ->wol_enabled isn't set, then we should fallback to the previous
implementation so I expect it to be fine for that matter.
Then, I expect flags to be set only in case of WoL event received.
Nevertheless, I will double check the phy_* API used in this sequence
for V2, thank you.

Best regards,
Gatien

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/21/25 14:16, Krzysztof Kozlowski wrote:
> On 21/07/2025 14:10, Gatien CHEVALLIER wrote:
>> Hello Krzysztof,
>>
>> On 7/21/25 13:30, Krzysztof Kozlowski wrote:
>>> On 21/07/2025 13:14, Gatien Chevallier wrote:
>>>> The "st,phy-wol" property can be set to use the wakeup capability of
>>>> the PHY instead of the MAC.
>>>
>>>
>>> And why would that be property of a SoC or board? Word "can" suggests
>>> you are documenting something which exists, but this does not exist.
>> Can you elaborate a bit more on the "not existing" part please?
> 
> 
> Where does this property exist that you suggest that a new binding "can"
> use it?
> 

I'm not sure if you're only targeting the wording here?

I found this property: "mediatek,mac-wol". Its non-presence sets the
same flag AFAICT.

I need to set the flag so that the stmmac_set_wol() in
drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c eventually
reaches the PHY driver.

>>
>> For the WoL from PHY to be supported, the PHY line that is raised
>> (On nPME pin for this case) when receiving a wake up event has to be
>> wired to a wakeup event input of the Extended interrupt and event
>> controller(EXTI), and that's implementation dependent.
> 
> 
> So it is not "can" but some implementations do not have proper wiring.
> You need to justify your commits and changes.
> 

Ok, does:
In case of proper wiring from the PHY to a wake up event input,
set the "st,phy-wol" to indicate that the PHY WoL capability is
preferred over the MAC one.

seems better?

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

On Mon, Jul 21, 2025 at 02:10:48PM +0200, Gatien CHEVALLIER wrote:
> Hello Krzysztof,
> 
> On 7/21/25 13:30, Krzysztof Kozlowski wrote:
> > On 21/07/2025 13:14, Gatien Chevallier wrote:
> > > The "st,phy-wol" property can be set to use the wakeup capability of
> > > the PHY instead of the MAC.
> > 
> > 
> > And why would that be property of a SoC or board? Word "can" suggests
> > you are documenting something which exists, but this does not exist.
> Can you elaborate a bit more on the "not existing" part please?
> 
> For the WoL from PHY to be supported, the PHY line that is raised
> (On nPME pin for this case) when receiving a wake up event has to be
> wired to a wakeup event input of the Extended interrupt and event
> controller(EXTI), and that's implementation dependent.

How does this differ from normal interrupts from the PHY? Isn't the
presence of an interrupt in DT sufficient to indicate the PHY can wake
the system?

	Andrew

Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Andrew Lunn]

> +static int smsc_phy_suspend(struct phy_device *phydev)
> +{
> +	if (!phydev->wol_enabled)
> +		return genphy_suspend(phydev);
> +
> +	return 0;
> +}

Suspend/resume is somewhat complex, and i don't know all the
details. But this looks odd. Why does the phylib core call suspend
when phydev->wol_enabled is true? That at least needs an explanation
in the commit message.

> +static int smsc_phy_resume(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	if (!phydev->wol_enabled)
> +		return genphy_resume(phydev);
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
> +		return 0;
> +
> +	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");

Please don't spam the log. It is clear the system woke up, there are
messages in the log...

	Andrew

Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Gatien CHEVALLIER]

Hello Andrew,

On 7/21/25 15:26, Andrew Lunn wrote:
>> +static int smsc_phy_suspend(struct phy_device *phydev)
>> +{
>> +	if (!phydev->wol_enabled)
>> +		return genphy_suspend(phydev);
>> +
>> +	return 0;
>> +}
> 
> Suspend/resume is somewhat complex, and i don't know all the
> details. But this looks odd. Why does the phylib core call suspend
> when phydev->wol_enabled is true? That at least needs an explanation
> in the commit message.
> 

As stated by Russel, this callback is not needed because phy_suspend()
will not call this suspend() callback if phydev->wol_enabled is set.
Therefore, I'm removing it vor V2.

>> +static int smsc_phy_resume(struct phy_device *phydev)
>> +{
>> +	int rc;
>> +
>> +	if (!phydev->wol_enabled)
>> +		return genphy_resume(phydev);
>> +
>> +	rc = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_LAN874X_PHY_MMD_WOL_WUCSR);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	if (!(rc & MII_LAN874X_PHY_WOL_STATUS_MASK))
>> +		return 0;
>> +
>> +	dev_info(&phydev->mdio.dev, "Woke up from LAN event.\n");
> 
> Please don't spam the log. It is clear the system woke up, there are
> messages in the log...

I wanted to state clearly that the wake up happended because of a WoL
event but sure, I understand that it's best if log isn't spammed. Do you
prefer it completely removed or dev_info()->dev_dbg() ?

Best regards,
Gatien

> 
> 	Andrew

Re: [PATCH net-next 3/4] net: phy: smsc: fix and improve WoL support

[Andrew Lunn]

> I wanted to state clearly that the wake up happended because of a WoL
> event but sure, I understand that it's best if log isn't spammed. Do you
> prefer it completely removed or dev_info()->dev_dbg() ?

phydev_dbg() is fine.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

Hello Andrew,

On 7/21/25 15:18, Andrew Lunn wrote:
> On Mon, Jul 21, 2025 at 02:10:48PM +0200, Gatien CHEVALLIER wrote:
>> Hello Krzysztof,
>>
>> On 7/21/25 13:30, Krzysztof Kozlowski wrote:
>>> On 21/07/2025 13:14, Gatien Chevallier wrote:
>>>> The "st,phy-wol" property can be set to use the wakeup capability of
>>>> the PHY instead of the MAC.
>>>
>>>
>>> And why would that be property of a SoC or board? Word "can" suggests
>>> you are documenting something which exists, but this does not exist.
>> Can you elaborate a bit more on the "not existing" part please?
>>
>> For the WoL from PHY to be supported, the PHY line that is raised
>> (On nPME pin for this case) when receiving a wake up event has to be
>> wired to a wakeup event input of the Extended interrupt and event
>> controller(EXTI), and that's implementation dependent.
> 
> How does this differ from normal interrupts from the PHY? Isn't the
> presence of an interrupt in DT sufficient to indicate the PHY can wake
> the system?
> 
> 	Andrew

Here's an extract from the Microchip datasheet for the LAN8742A PHY:

"In addition to the main interrupts described in this section, an nPME
pin is provided exclusively for WoL specific interrupts."

I'm not an expert of the different PHYs, but I guess there may be a
distinction needed between some "main" PHY interrupts and the WoL one
at least for deep low-power use cases.

Because this line is wired to a peripheral managed by our
TEE, the ultimate goal here would be to declare the OP-TEE node as
an interrupt provider and to forward the interrupt to the kernel using
the asynchronous notification mechanism. Then, reference the OP-TEE
node in the "interrupts-extended" property in the PHY node so that it
can be acked by the PHY driver. As of now, this patch set at least allow
to wakeup from a deep low power mode using the WoL capability of the
PHY.

Regarding this property, somewhat similar to "mediatek,mac-wol",
I need to position a flag at the mac driver level. I thought I'd go
using the same approach.

Best regards,
Gatien

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

> Regarding this property, somewhat similar to "mediatek,mac-wol",
> I need to position a flag at the mac driver level. I thought I'd go
> using the same approach.

Ideally, you don't need such a flag. WoL should be done as low as
possible. If the PHY can do the WoL, the PHY should be used. If not,
fall back to MAC.

Many MAC drivers don't support this, or they get the implementation
wrong. So it could be you need to fix the MAC driver.

MAC get_wol() should ask the PHY what it supports, and then OR in what
the MAC supports.

When set_wol() is called, the MAC driver should ask the PHY driver to
do it. If it return 0, all is good, and the MAC driver can be
suspended when times comes. If the PHY driver returns EOPNOTSUPP, it
means it cannot support all the enabled WoL operations, so the MAC
driver needs to do some of them. The MAC driver then needs to ensure
it is not suspended.

If the PHY driver is missing the interrupt used to wake the system,
the get_wol() call should not return any supported WoL modes. The MAC
will then do WoL. Your "vendor,mac-wol" property is then pointless.

Correctly describe the PHY in DT, list the interrupt it uses for
waking the system.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Florian Fainelli]

On 7/21/2025 10:07 AM, Andrew Lunn wrote:
>> Regarding this property, somewhat similar to "mediatek,mac-wol",
>> I need to position a flag at the mac driver level. I thought I'd go
>> using the same approach.
> 
> Ideally, you don't need such a flag. WoL should be done as low as
> possible. If the PHY can do the WoL, the PHY should be used. If not,
> fall back to MAC.
> 
> Many MAC drivers don't support this, or they get the implementation
> wrong. So it could be you need to fix the MAC driver.
> 
> MAC get_wol() should ask the PHY what it supports, and then OR in what
> the MAC supports.
> 
> When set_wol() is called, the MAC driver should ask the PHY driver to
> do it. If it return 0, all is good, and the MAC driver can be
> suspended when times comes. If the PHY driver returns EOPNOTSUPP, it
> means it cannot support all the enabled WoL operations, so the MAC
> driver needs to do some of them. The MAC driver then needs to ensure
> it is not suspended.
> 
> If the PHY driver is missing the interrupt used to wake the system,
> the get_wol() call should not return any supported WoL modes. The MAC
> will then do WoL. Your "vendor,mac-wol" property is then pointless.
> 
> Correctly describe the PHY in DT, list the interrupt it uses for
> waking the system.

+1
-- 
Florian

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Mon, Jul 21, 2025 at 05:56:17PM +0200, Gatien CHEVALLIER wrote:
> Here's an extract from the Microchip datasheet for the LAN8742A PHY:
> 
> "In addition to the main interrupts described in this section, an nPME
> pin is provided exclusively for WoL specific interrupts."

So the pin on the PHY for WoL is called nPME? If this pin isn't wired
to an interrupt controller, then the PHY doesn't support WoL. If it is
wired, then could it be inferred that WoL is supported?

If so, then it seems to me the simple solution here is for the PHY
driver to say "if the nPME pin is connected to an interrupt controller,
then PHY-side WoL is supported, otherwise PHY-side WoL is not
supported".

Then, I wonder if the detection of the WoL capabilities of the PHY
in stmmac_init_phy() could be used to determine whether PHY WoL
should be used or not.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/21/25 19:07, Andrew Lunn wrote:
>> Regarding this property, somewhat similar to "mediatek,mac-wol",
>> I need to position a flag at the mac driver level. I thought I'd go
>> using the same approach.
> 
> Ideally, you don't need such a flag. WoL should be done as low as
> possible. If the PHY can do the WoL, the PHY should be used. If not,
> fall back to MAC.
> 
> Many MAC drivers don't support this, or they get the implementation
> wrong. So it could be you need to fix the MAC driver.
> 
> MAC get_wol() should ask the PHY what it supports, and then OR in what
> the MAC supports.
> 
> When set_wol() is called, the MAC driver should ask the PHY driver to
> do it. If it return 0, all is good, and the MAC driver can be
> suspended when times comes. If the PHY driver returns EOPNOTSUPP, it
> means it cannot support all the enabled WoL operations, so the MAC
> driver needs to do some of them. The MAC driver then needs to ensure
> it is not suspended.
> 
> If the PHY driver is missing the interrupt used to wake the system,
> the get_wol() call should not return any supported WoL modes. The MAC
> will then do WoL. Your "vendor,mac-wol" property is then pointless.
> 

Seems like a fair and logical approach. It seems reasonable that the
MAC driver relies on the get_wol() API to know what's supported.

The tricky thing for the PHY used in this patchset is to get this
information:

Extract from the documentation of the LAN8742A PHY:
"The WoL detection can be configured to assert the nINT interrupt pin
or nPME pin"

This PHY proposes several pins with alternate configurations so they
can act as either nINT, nPME or other type of pin. While the nPME
is dedicated to raise a signal on a WoL event, WoL event can also,
if configured, raise a signal on a nINT pin. However, the latter
case expect (extract again):
"While waiting for a WoL event to occur, it is possible that other
interrupts may be triggered. To prevent such conditions, all other
interrupts shall be masked by system software, or the alternative nPME
pin may be used"
therefore preventing other types of interrupt from triggering.

Today, the WoL is statically configured so that the nPME pin is
asserted on such event in lan874x_phy_config_init(). For it to
be functional, the nPME pin has to be wired to a wake up input of
a wake up capable interrupt controller.
On the stm32mp135f-dk board, e.g, that's the case for only one of the
two ethernet ports.

Overall it's both a combination of what pin is asserted on a WoL
and what pin is wired to a wake up capable interrupt controller.
What would be a correct approach to get the information from the PHY
driver that the WoL is indeed supported considering all of this?

Tristram, can you tell me if what I'm saying here makes any
sense?

> Correctly describe the PHY in DT, list the interrupt it uses for
> waking the system.
> 
> 	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/22/25 09:32, Russell King (Oracle) wrote:
> On Mon, Jul 21, 2025 at 05:56:17PM +0200, Gatien CHEVALLIER wrote:
>> Here's an extract from the Microchip datasheet for the LAN8742A PHY:
>>
>> "In addition to the main interrupts described in this section, an nPME
>> pin is provided exclusively for WoL specific interrupts."
> 
> So the pin on the PHY for WoL is called nPME? If this pin isn't wired
> to an interrupt controller, then the PHY doesn't support WoL. If it is
> wired, then could it be inferred that WoL is supported?
> 

For this PHY yes, but it's a bit more tricky. In my response to Andrew,
I added a bit more information.

> If so, then it seems to me the simple solution here is for the PHY
> driver to say "if the nPME pin is connected to an interrupt controller,
> then PHY-side WoL is supported, otherwise PHY-side WoL is not
> supported".
> 

If there's a proper way to do this, sure!

> Then, I wonder if the detection of the WoL capabilities of the PHY
> in stmmac_init_phy() could be used to determine whether PHY WoL
> should be used or not.
> 

Yes, sure.

Best regards,
Gatien

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Mon, Jul 21, 2025 at 07:07:11PM +0200, Andrew Lunn wrote:
> > Regarding this property, somewhat similar to "mediatek,mac-wol",
> > I need to position a flag at the mac driver level. I thought I'd go
> > using the same approach.
> 
> Ideally, you don't need such a flag. WoL should be done as low as
> possible. If the PHY can do the WoL, the PHY should be used. If not,
> fall back to MAC.
> 
> Many MAC drivers don't support this, or they get the implementation
> wrong. So it could be you need to fix the MAC driver.
> 
> MAC get_wol() should ask the PHY what it supports, and then OR in what
> the MAC supports.
> 
> When set_wol() is called, the MAC driver should ask the PHY driver to
> do it. If it return 0, all is good, and the MAC driver can be
> suspended when times comes. If the PHY driver returns EOPNOTSUPP, it
> means it cannot support all the enabled WoL operations, so the MAC
> driver needs to do some of them. The MAC driver then needs to ensure
> it is not suspended.
> 
> If the PHY driver is missing the interrupt used to wake the system,
> the get_wol() call should not return any supported WoL modes. The MAC
> will then do WoL. Your "vendor,mac-wol" property is then pointless.
> 
> Correctly describe the PHY in DT, list the interrupt it uses for
> waking the system.

This would be a good idea if we were talking about a new PHY and MAC
driver, but we aren't.

Given the number of platform drivers that stmmac has with numerous
PHY drivers, changing how this works _now_ will likely break current
setups. Whether PHY-side WoL is used is dependent on a
priv->plat->pmt flag which depends on MAC capabilties and also
whether the platform glue sets/clears the STMMAC_FLAG_USE_PHY_WOL
flag.

Yes, it's a mess, and it could do with being improved, which will
likely take considerable time to do to shake out any regressions
caused - both in stmmac and PHY drivers.

I bet there are _numerous_ PHY drivers that report and accept WoL
even when the hardware isn't wired to support WoL. For example,
AT8031 reports that it supports WAKE_KAGIC irrespective of whether
WOL_INT is wired, and whether or not the INT pin is capable of
waking the system. I don't think we have any way that a driver can
determine whether a particular interrupt _can_ wake the system.

The problem for stmmac is that the PHY driver may support WAKE_MAGIC,
but so can the MAC core. If the PHY isn't electrically capable of
waking the system for whatever reason, but the PHY driver still
reports that it can (like AT803x) and we don't program the MAC core,
then under your idea, WoL will no longer work.

The only thing I can think we can now do is to have yet another
STMMAC_FLAG_xxx which platform glue can set to enable a new behaviour.
Yay, a driver with multiple different behaviours depending on flags
for the same feature.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

I know Russell has also replied about issues with stmmac. Please
consider that when reading what i say... It might be not applicable.

> Seems like a fair and logical approach. It seems reasonable that the
> MAC driver relies on the get_wol() API to know what's supported.
> 
> The tricky thing for the PHY used in this patchset is to get this
> information:
> 
> Extract from the documentation of the LAN8742A PHY:
> "The WoL detection can be configured to assert the nINT interrupt pin
> or nPME pin"

https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt

It is a bit messy, but in the device tree, you could have:

    interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
                 <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
    interrupt-names = "nINT", "wake";
    wakeup-source

You could also have:

    interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
    interrupt-names = "wake";
    wakeup-source

In the first example, since there are two interrupts listed, it must
be using the nPME. For the second, since there is only one, it must be
using nINT.

Where this does not work so well is when you have a board which does
not have nINT wired, but does have nPME. The phylib core will see
there is an interrupt and request it, and disable polling. And then
nothing will work. We might be able to delay solving that until such a
board actually exists?

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
> I know Russell has also replied about issues with stmmac. Please
> consider that when reading what i say... It might be not applicable.
> 
> > Seems like a fair and logical approach. It seems reasonable that the
> > MAC driver relies on the get_wol() API to know what's supported.
> > 
> > The tricky thing for the PHY used in this patchset is to get this
> > information:
> > 
> > Extract from the documentation of the LAN8742A PHY:
> > "The WoL detection can be configured to assert the nINT interrupt pin
> > or nPME pin"
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> 
> It is a bit messy, but in the device tree, you could have:
> 
>     interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
>                  <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
>     interrupt-names = "nINT", "wake";
>     wakeup-source
> 
> You could also have:
> 
>     interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
>     interrupt-names = "wake";
>     wakeup-source
> 
> In the first example, since there are two interrupts listed, it must
> be using the nPME. For the second, since there is only one, it must be
> using nINT.
> 
> Where this does not work so well is when you have a board which does
> not have nINT wired, but does have nPME. The phylib core will see
> there is an interrupt and request it, and disable polling. And then
> nothing will work. We might be able to delay solving that until such a
> board actually exists?

(Officially, I'm still on vacation...)

At this point, I'd like to kick off a discussion about PHY-based
wakeup that is relevant to this thread.

The kernel has device-based wakeup support. We have:

- device_set_wakeup_capable(dev, flag) - indicates that the is
  capable of waking the system depending on the flag.

- device_set_wakeup_enable(dev, flag) - indicates whether "dev"
  has had wake-up enabled or disabled depending on the flag.

- dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
  the indicated IRQ is capable of waking the system, and the core
  will handle enabling/disabling irq wake capabilities on the IRQ
  as appropriate (dependent on device_set_wakeup_enable()). Other
  functions are available for wakeup IRQs that are dedicated to
  only waking up the system (e.g. the WOL_INT pin on AR8031).

Issue 1. In stmmac_init_phy(), we have this code:

        if (!priv->plat->pmt) {
                struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };

                phylink_ethtool_get_wol(priv->phylink, &wol);
                device_set_wakeup_capable(priv->device, !!wol.supported);
                device_set_wakeup_enable(priv->device, !!wol.wolopts);
        }

This reads the WoL state from the PHY (a different struct device)
and sets the wakeup capability and enable state for the _stmmac_
device accordingly, but in the case of PHY based WoL, it's the PHY
doing the wakeup, not the MAC. So this seems wrong on the face of
it.

Issue 2. no driver in phylib, nor the core, ever uses any of the
device_set_wakeup_*() functions. As PHYs on their own are capable
of WoL, isn't this an oversight? Shouldn't phylib be supporting
this rather than leaving it to MAC drivers to figure something out?

Issue 3. should pins like WOL_INT or nPME be represented as an
interrupt, and dev_pm_set_dedicated_wake_irq() used to manage that
interrupt signal if listed as an IRQ in the PHY's DT description?

(Side note: I have tried WoL on the Jetson Xavier NX board I have
which uses stmmac-based WoL, but it seems non-functional. I've
dropped a private email to Jon and Thierry to see whether this is
expected or something that needs fixing. I'm intending to convert
stmmac to use core wakeirq support, rather than managing
the enable_irq_wake()/disable_irq_wake() by itself.)

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Florian Fainelli]

On 7/22/25 13:20, Russell King (Oracle) wrote:
> On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
>> I know Russell has also replied about issues with stmmac. Please
>> consider that when reading what i say... It might be not applicable.
>>
>>> Seems like a fair and logical approach. It seems reasonable that the
>>> MAC driver relies on the get_wol() API to know what's supported.
>>>
>>> The tricky thing for the PHY used in this patchset is to get this
>>> information:
>>>
>>> Extract from the documentation of the LAN8742A PHY:
>>> "The WoL detection can be configured to assert the nINT interrupt pin
>>> or nPME pin"
>>
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
>>
>> It is a bit messy, but in the device tree, you could have:
>>
>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
>>                   <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
>>      interrupt-names = "nINT", "wake";
>>      wakeup-source
>>
>> You could also have:
>>
>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
>>      interrupt-names = "wake";
>>      wakeup-source
>>
>> In the first example, since there are two interrupts listed, it must
>> be using the nPME. For the second, since there is only one, it must be
>> using nINT.
>>
>> Where this does not work so well is when you have a board which does
>> not have nINT wired, but does have nPME. The phylib core will see
>> there is an interrupt and request it, and disable polling. And then
>> nothing will work. We might be able to delay solving that until such a
>> board actually exists?
> 
> (Officially, I'm still on vacation...)
> 
> At this point, I'd like to kick off a discussion about PHY-based
> wakeup that is relevant to this thread.
> 
> The kernel has device-based wakeup support. We have:
> 
> - device_set_wakeup_capable(dev, flag) - indicates that the is
>    capable of waking the system depending on the flag.
> 
> - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
>    has had wake-up enabled or disabled depending on the flag.
> 
> - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
>    the indicated IRQ is capable of waking the system, and the core
>    will handle enabling/disabling irq wake capabilities on the IRQ
>    as appropriate (dependent on device_set_wakeup_enable()). Other
>    functions are available for wakeup IRQs that are dedicated to
>    only waking up the system (e.g. the WOL_INT pin on AR8031).
> 
> Issue 1. In stmmac_init_phy(), we have this code:
> 
>          if (!priv->plat->pmt) {
>                  struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> 
>                  phylink_ethtool_get_wol(priv->phylink, &wol);
>                  device_set_wakeup_capable(priv->device, !!wol.supported);
>                  device_set_wakeup_enable(priv->device, !!wol.wolopts);
>          }
> 
> This reads the WoL state from the PHY (a different struct device)
> and sets the wakeup capability and enable state for the _stmmac_
> device accordingly, but in the case of PHY based WoL, it's the PHY
> doing the wakeup, not the MAC. So this seems wrong on the face of
> it.

Yes, this looks like the wrong driver to be doing the 
device_set_{wakeup,capable}, those calls should be in the PHY driver 
where the knowledge of whether WoL is possible should reside.

> 
> Issue 2. no driver in phylib, nor the core, ever uses any of the
> device_set_wakeup_*() functions. As PHYs on their own are capable
> of WoL, isn't this an oversight? Shouldn't phylib be supporting
> this rather than leaving it to MAC drivers to figure something out?

The Broadcom PHY driver calls device_init_wakeup() when we have 
determined that the GPIO used for wake-up is available as an interrupt 
resource.

> 
> Issue 3. should pins like WOL_INT or nPME be represented as an
> interrupt, and dev_pm_set_dedicated_wake_irq() used to manage that
> interrupt signal if listed as an IRQ in the PHY's DT description?

Yes they should be IMHO.

> 
> (Side note: I have tried WoL on the Jetson Xavier NX board I have
> which uses stmmac-based WoL, but it seems non-functional. I've
> dropped a private email to Jon and Thierry to see whether this is
> expected or something that needs fixing. I'm intending to convert
> stmmac to use core wakeirq support, rather than managing
> the enable_irq_wake()/disable_irq_wake() by itself.)
> 


-- 
Florian

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

>         if (!priv->plat->pmt) {

Let me start with maybe a dumb question. What does pmt mean? Why would
it be true?

>                 struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> 
>                 phylink_ethtool_get_wol(priv->phylink, &wol);
>                 device_set_wakeup_capable(priv->device, !!wol.supported);
>                 device_set_wakeup_enable(priv->device, !!wol.wolopts);

Without knowing what pmt means, this is pure speculation....  Maybe it
means the WoL output from the PHY is connected to a pin of the stmmac.
It thus needs stmmac to perform the actual wakeup of the system, as a
proxy for the PHY?

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Tue, Jul 22, 2025 at 10:59:51PM +0200, Andrew Lunn wrote:
> >         if (!priv->plat->pmt) {
> 
> Let me start with maybe a dumb question. What does pmt mean? Why would
> it be true?

->pmt being true means the MAC supports remote wakeup (in other words,
magic packet or unicast), and the glue driver has not set the
STMMAC_FLAG_USE_PHY_WOL flag to force the use of PHY WoL.

->pmt being false means that the MAC doesn't support remote wakeup
(in other words, has no ability to wake the system itself) _or_ the
platform wishes to force the use of PHY WoL when the MAC does support
remote wakeup.

> 
> >                 struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> > 
> >                 phylink_ethtool_get_wol(priv->phylink, &wol);
> >                 device_set_wakeup_capable(priv->device, !!wol.supported);
> >                 device_set_wakeup_enable(priv->device, !!wol.wolopts);
> 
> Without knowing what pmt means, this is pure speculation....  Maybe it
> means the WoL output from the PHY is connected to a pin of the stmmac.
> It thus needs stmmac to perform the actual wakeup of the system, as a
> proxy for the PHY?

stmmac itself doesn't have an input for PHY interrupts, so a PHY
which signals WoL via its interrupt pin (e.g. AR8035) wouldn't go
through stmmac.


I mentioned above the STMMAC_FLAG_USE_PHY_WOL flag. To see why this
flag is needed, on the Jetson Xavier NX platform, a RTL8211 PHY is used
with stmmac - I think it's RTL8211F (without powering it back up, I
can't check.) This PHY supports WoL, but signals wake-up through a
dedicated PMEB pin. If a platform decides not to wire the PMEB pin, WoL
on the PHY is unusable.

rtl8211f_get_wol() does not take account of whether the PMEB pin is
wired or not. Thus, stmmac can't just forward the get_wol() and
set_wol() ops to the PHY driver and let it decide, as suggested
earlier. As stmmac gets used with multiple PHYs, (and hey, we can't
tell what they are, because DT doesn't list what the PHY actually is!)
we can't know how many other PHY drivers also have this problem.

So, the idea put forward that ethernet drivers should forward get_wol()
and set_wol() to the PHY driver and only do WoL at the MAC if the PHY
doesn't support it is, I'm afraid, now fundamentally flawed.

We can't retrofit such detection into PHY drivers - if we do so, we'll
break WoL on lots of boards (we'd need to e.g. describe PMEB in DT for
RTL8211F PHYs. While we can add it, if a newer kernel that expects
PMEB to be described to allow WoL is run with an older DT, then that
will be a regression.) Thus, I don't see how we could retrofit PHY
WoL support detection to MAC drivers.

So, while it is undesirable to have a flag in DT to say "we can use
PHY WoL on this platform" and we can whinge that it isn't "describing
hardware", DT _hasn't_ been describing the hardware, and trying to fix
DT to properly describe the hardware now is going to cause _lots_ of
breakage.

I can't see a way forward on this unless someone is willing to relax
over what seem to be hard requirements (e.g. no we don't want a flag
in DT to say use PHY WoL.) We have collectively boxed ourselves into
a corner on this.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Tue, Jul 22, 2025 at 10:39:53PM +0100, Russell King (Oracle) wrote:
> rtl8211f_get_wol() does not take account of whether the PMEB pin is
> wired or not. Thus, stmmac can't just forward the get_wol() and
> set_wol() ops to the PHY driver and let it decide, as suggested
> earlier. As stmmac gets used with multiple PHYs, (and hey, we can't
> tell what they are, because DT doesn't list what the PHY actually is!)
> we can't know how many other PHY drivers also have this problem.

I've just read a bit more of the RTL8211F datasheet, and looked at the
code, and I'm now wondering whether WoL has even been tested with
RTL8211F. What I'm about to state doesn't negate anything I've said
in my previous reply.


So, the RTL8211F doesn't have a separate PMEB pin. It has a pin that
is shared between "interrupt" and "PMEB".

Register 22, page 0xd40, bit 5 determines whether this pin is used for
PMEB (in which case it is pulsed on wake-up) or whether it is used as
an interrupt. It's one or the other function, but can't be both.

rtl8211f_set_wol() manipulates this bit depending on whether
WAKE_MAGIC is enabled or not.

The effect of this is...

If we're using PHY interrupts from the RTL8211F, and then userspace
configures magic packet WoL on the PHY, then we reconfigure the
interrupt pin to become a wakeup pin, disabling the interrupt
function - we no longer receive interrupts from the RTL8211F !!!!!!!

Yes, the driver does support interrupts for this device!

This is surely wrong because it will break phylib's ability to track
the link state as there will be no further interrupts _and_ phylib
won't be expecting to poll the PHY.

The really funny thing is that the PHY does have the ability to
raise an interrupt if a wakeup occurs through the interrupt pin
(when configured as such) via register 18, page 0xa42, bit 7...
but the driver doesn't touch that.


Jetson Xavier NX uses interrupts from this PHY. Forwarding an
ethtool .set_wol() op to the PHY driver which enables magic packet
will, as things stand, switch the interrupt pin to wake-up only
mode, preventing delivery of further link state change events to
phylib, breaking phylib.

Maybe there's a need for this behaviour with which-ever network
driver first used RTL8211F in the kernel. Maybe the set of network
drivers that use interrupts from the RTL8211F don't use WoL and
vice versa. If there's any network drivers that do forward WoL
calls to the RTL8211F driver _and_ use interrupts from the PHY...
that's just going to break if magic packet WoL is ever enabled at
the PHY.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Tue, Jul 22, 2025 at 11:00:34PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 22, 2025 at 10:39:53PM +0100, Russell King (Oracle) wrote:
> > rtl8211f_get_wol() does not take account of whether the PMEB pin is
> > wired or not. Thus, stmmac can't just forward the get_wol() and
> > set_wol() ops to the PHY driver and let it decide, as suggested
> > earlier. As stmmac gets used with multiple PHYs, (and hey, we can't
> > tell what they are, because DT doesn't list what the PHY actually is!)
> > we can't know how many other PHY drivers also have this problem.
> 
> I've just read a bit more of the RTL8211F datasheet, and looked at the
> code, and I'm now wondering whether WoL has even been tested with
> RTL8211F. What I'm about to state doesn't negate anything I've said
> in my previous reply.
> 
> 
> So, the RTL8211F doesn't have a separate PMEB pin. It has a pin that
> is shared between "interrupt" and "PMEB".
> 
> Register 22, page 0xd40, bit 5 determines whether this pin is used for
> PMEB (in which case it is pulsed on wake-up) or whether it is used as
> an interrupt. It's one or the other function, but can't be both.
> 
> rtl8211f_set_wol() manipulates this bit depending on whether
> WAKE_MAGIC is enabled or not.
> 
> The effect of this is...
> 
> If we're using PHY interrupts from the RTL8211F, and then userspace
> configures magic packet WoL on the PHY, then we reconfigure the
> interrupt pin to become a wakeup pin, disabling the interrupt
> function - we no longer receive interrupts from the RTL8211F !!!!!!!
> 
> Yes, the driver does support interrupts for this device!
> 
> This is surely wrong because it will break phylib's ability to track
> the link state as there will be no further interrupts _and_ phylib
> won't be expecting to poll the PHY.
> 
> The really funny thing is that the PHY does have the ability to
> raise an interrupt if a wakeup occurs through the interrupt pin
> (when configured as such) via register 18, page 0xa42, bit 7...
> but the driver doesn't touch that.
> 
> 
> Jetson Xavier NX uses interrupts from this PHY. Forwarding an
> ethtool .set_wol() op to the PHY driver which enables magic packet
> will, as things stand, switch the interrupt pin to wake-up only
> mode, preventing delivery of further link state change events to
> phylib, breaking phylib.
> 
> Maybe there's a need for this behaviour with which-ever network
> driver first used RTL8211F in the kernel. Maybe the set of network
> drivers that use interrupts from the RTL8211F don't use WoL and
> vice versa. If there's any network drivers that do forward WoL
> calls to the RTL8211F driver _and_ use interrupts from the PHY...
> that's just going to break if magic packet WoL is ever enabled at
> the PHY.

The only solutions I can think that may work with RTL8211F are:

Solution 1. move the control of RTL8211F_INTBCR_INTB_PMEB to a new
  rtl8211f_suspend() / rtl8211f_resume(), and switch the pin between
  interrupt mode and PMEB mode accordingly if WoL is enabled. This
  should be relatively low-risk, and not require DT changes.

Solution 2. don't switch to PMEB mode if phylib is using interrupts.
  Instead, enable WoL interrupt when in INTB mode. Also needs
  rtl8211f_handle_interrupt() modified to "handle" the interrupt
  to prevent an interrupt storm. May cause other problems - PMEB
  is pulsed, WoL over INTB is level-based, so higher risk.

Solution 3. introduce a DT flag for rtl8211f PHYs to tell the PHY
  driver "this platform should enable WoL interrupts in INTB mode
  and not switch to PMEB mode". Safest, as no change in behaviour
  without the flag being present... but arguable whether it truly
  describes hardware. However, what we currently have in DT
  *doesn't* actually describe hardware because of the mistakes made.
  (Maybe we can use the wakeup-source property to indicate this
  mode, which may be more acceptable to Krzysztof than a whole new
  flag.)

Maybe something else would be acceptable to DT folk - I think I've
provided enough of a description of the problem we currently have
to allow DT folk to digest the issue here.

Just random thoughts below... here's the description of the PHY on
the Jetson Xavier NX which I'll use as a basis for some scenarios.
(from arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi).

	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>;
		#phy-cells = <0>;
	};

If WoL is supported through interrupts, then maybe we describe it
as:

	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>;
		#phy-cells = <0>;
		wakeup-source;
	};

WoL is supported through PMEB, no interrupt support, then it gets
described as:

	phy: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x0>;
		#phy-cells = <0>;
		wakeup-source;
	};

The problem becomes how to describe the _existing_ behaviour going
forward, which we get with the current (first) description above. We
would need to preserve this for the existing description for backward
compatibility to avoid breaking existing setups. Do we try to come up
with something different that allows wakeup-source to be added?
Should we say that shared-interrupt and PMEB mode isn't something we
support except for legacy stuff, and thus not care about the missing
wakeup-source property? Something else, if so what (please suggest) ?

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/22/25 22:20, Russell King (Oracle) wrote:
> On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
>> I know Russell has also replied about issues with stmmac. Please
>> consider that when reading what i say... It might be not applicable.
>>
>>> Seems like a fair and logical approach. It seems reasonable that the
>>> MAC driver relies on the get_wol() API to know what's supported.
>>>
>>> The tricky thing for the PHY used in this patchset is to get this
>>> information:
>>>
>>> Extract from the documentation of the LAN8742A PHY:
>>> "The WoL detection can be configured to assert the nINT interrupt pin
>>> or nPME pin"
>>
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
>>
>> It is a bit messy, but in the device tree, you could have:
>>
>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
>>                   <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
>>      interrupt-names = "nINT", "wake";
>>      wakeup-source
>>
>> You could also have:
>>
>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
>>      interrupt-names = "wake";
>>      wakeup-source
>>
>> In the first example, since there are two interrupts listed, it must
>> be using the nPME. For the second, since there is only one, it must be
>> using nINT.
>>
>> Where this does not work so well is when you have a board which does
>> not have nINT wired, but does have nPME. The phylib core will see
>> there is an interrupt and request it, and disable polling. And then
>> nothing will work. We might be able to delay solving that until such a
>> board actually exists?
> 
> (Officially, I'm still on vacation...)
> 
> At this point, I'd like to kick off a discussion about PHY-based
> wakeup that is relevant to this thread.
> 
> The kernel has device-based wakeup support. We have:
> 
> - device_set_wakeup_capable(dev, flag) - indicates that the is
>    capable of waking the system depending on the flag.
> 
> - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
>    has had wake-up enabled or disabled depending on the flag.
> 
> - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
>    the indicated IRQ is capable of waking the system, and the core
>    will handle enabling/disabling irq wake capabilities on the IRQ
>    as appropriate (dependent on device_set_wakeup_enable()). Other
>    functions are available for wakeup IRQs that are dedicated to
>    only waking up the system (e.g. the WOL_INT pin on AR8031).
> 
> Issue 1. In stmmac_init_phy(), we have this code:
> 
>          if (!priv->plat->pmt) {
>                  struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> 
>                  phylink_ethtool_get_wol(priv->phylink, &wol);
>                  device_set_wakeup_capable(priv->device, !!wol.supported);
>                  device_set_wakeup_enable(priv->device, !!wol.wolopts);
>          }
> 
> This reads the WoL state from the PHY (a different struct device)
> and sets the wakeup capability and enable state for the _stmmac_
> device accordingly, but in the case of PHY based WoL, it's the PHY
> doing the wakeup, not the MAC. So this seems wrong on the face of
> it.

2 cents: Maybe even remove in stmmac_set_wol() if !priv->plat->pmt.

> 
> Issue 2. no driver in phylib, nor the core, ever uses any of the
> device_set_wakeup_*() functions. As PHYs on their own are capable
> of WoL, isn't this an oversight? Shouldn't phylib be supporting
> this rather than leaving it to MAC drivers to figure something out?
> 
> Issue 3. should pins like WOL_INT or nPME be represented as an
> interrupt, and dev_pm_set_dedicated_wake_irq() used to manage that
> interrupt signal if listed as an IRQ in the PHY's DT description?
> 
> (Side note: I have tried WoL on the Jetson Xavier NX board I have
> which uses stmmac-based WoL, but it seems non-functional. I've
> dropped a private email to Jon and Thierry to see whether this is
> expected or something that needs fixing. I'm intending to convert
> stmmac to use core wakeirq support, rather than managing
> the enable_irq_wake()/disable_irq_wake() by itself.)
> 

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/23/25 10:50, Gatien CHEVALLIER wrote:
> 
> 
> On 7/22/25 22:20, Russell King (Oracle) wrote:
>> On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
>>> I know Russell has also replied about issues with stmmac. Please
>>> consider that when reading what i say... It might be not applicable.
>>>
>>>> Seems like a fair and logical approach. It seems reasonable that the
>>>> MAC driver relies on the get_wol() API to know what's supported.
>>>>
>>>> The tricky thing for the PHY used in this patchset is to get this
>>>> information:
>>>>
>>>> Extract from the documentation of the LAN8742A PHY:
>>>> "The WoL detection can be configured to assert the nINT interrupt pin
>>>> or nPME pin"
>>>
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
>>>
>>> It is a bit messy, but in the device tree, you could have:
>>>
>>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
>>>                   <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
>>>      interrupt-names = "nINT", "wake";
>>>      wakeup-source
>>>
>>> You could also have:
>>>
>>>      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
>>>      interrupt-names = "wake";
>>>      wakeup-source
>>>
>>> In the first example, since there are two interrupts listed, it must
>>> be using the nPME. For the second, since there is only one, it must be
>>> using nINT.
>>>
>>> Where this does not work so well is when you have a board which does
>>> not have nINT wired, but does have nPME. The phylib core will see
>>> there is an interrupt and request it, and disable polling. And then
>>> nothing will work. We might be able to delay solving that until such a
>>> board actually exists?
>>
>> (Officially, I'm still on vacation...)
>>
>> At this point, I'd like to kick off a discussion about PHY-based
>> wakeup that is relevant to this thread.
>>
>> The kernel has device-based wakeup support. We have:
>>
>> - device_set_wakeup_capable(dev, flag) - indicates that the is
>>    capable of waking the system depending on the flag.
>>
>> - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
>>    has had wake-up enabled or disabled depending on the flag.
>>
>> - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
>>    the indicated IRQ is capable of waking the system, and the core
>>    will handle enabling/disabling irq wake capabilities on the IRQ
>>    as appropriate (dependent on device_set_wakeup_enable()). Other
>>    functions are available for wakeup IRQs that are dedicated to
>>    only waking up the system (e.g. the WOL_INT pin on AR8031).
>>
>> Issue 1. In stmmac_init_phy(), we have this code:
>>
>>          if (!priv->plat->pmt) {
>>                  struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>>
>>                  phylink_ethtool_get_wol(priv->phylink, &wol);
>>                  device_set_wakeup_capable(priv->device, 
>> !!wol.supported);
>>                  device_set_wakeup_enable(priv->device, !!wol.wolopts);
>>          }
>>
>> This reads the WoL state from the PHY (a different struct device)
>> and sets the wakeup capability and enable state for the _stmmac_
>> device accordingly, but in the case of PHY based WoL, it's the PHY
>> doing the wakeup, not the MAC. So this seems wrong on the face of
>> it.
> 
> 2 cents: Maybe even remove in stmmac_set_wol() if !priv->plat->pmt.
> 

Sorry, that's not very clear. I was thinking of removing:
device_set_wakeup_enable(priv->device, !!wol->wolopts); in
stmmac_set_wol()

>>
>> Issue 2. no driver in phylib, nor the core, ever uses any of the
>> device_set_wakeup_*() functions. As PHYs on their own are capable
>> of WoL, isn't this an oversight? Shouldn't phylib be supporting
>> this rather than leaving it to MAC drivers to figure something out?
>>
>> Issue 3. should pins like WOL_INT or nPME be represented as an
>> interrupt, and dev_pm_set_dedicated_wake_irq() used to manage that
>> interrupt signal if listed as an IRQ in the PHY's DT description?
>>
>> (Side note: I have tried WoL on the Jetson Xavier NX board I have
>> which uses stmmac-based WoL, but it seems non-functional. I've
>> dropped a private email to Jon and Thierry to see whether this is
>> expected or something that needs fixing. I'm intending to convert
>> stmmac to use core wakeirq support, rather than managing
>> the enable_irq_wake()/disable_irq_wake() by itself.)
>>

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Wed, Jul 23, 2025 at 10:50:12AM +0200, Gatien CHEVALLIER wrote:
> On 7/22/25 22:20, Russell King (Oracle) wrote:
> > On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
> > > I know Russell has also replied about issues with stmmac. Please
> > > consider that when reading what i say... It might be not applicable.
> > > 
> > > > Seems like a fair and logical approach. It seems reasonable that the
> > > > MAC driver relies on the get_wol() API to know what's supported.
> > > > 
> > > > The tricky thing for the PHY used in this patchset is to get this
> > > > information:
> > > > 
> > > > Extract from the documentation of the LAN8742A PHY:
> > > > "The WoL detection can be configured to assert the nINT interrupt pin
> > > > or nPME pin"
> > > 
> > > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> > > 
> > > It is a bit messy, but in the device tree, you could have:
> > > 
> > >      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
> > >                   <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
> > >      interrupt-names = "nINT", "wake";
> > >      wakeup-source
> > > 
> > > You could also have:
> > > 
> > >      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
> > >      interrupt-names = "wake";
> > >      wakeup-source
> > > 
> > > In the first example, since there are two interrupts listed, it must
> > > be using the nPME. For the second, since there is only one, it must be
> > > using nINT.
> > > 
> > > Where this does not work so well is when you have a board which does
> > > not have nINT wired, but does have nPME. The phylib core will see
> > > there is an interrupt and request it, and disable polling. And then
> > > nothing will work. We might be able to delay solving that until such a
> > > board actually exists?
> > 
> > (Officially, I'm still on vacation...)
> > 
> > At this point, I'd like to kick off a discussion about PHY-based
> > wakeup that is relevant to this thread.
> > 
> > The kernel has device-based wakeup support. We have:
> > 
> > - device_set_wakeup_capable(dev, flag) - indicates that the is
> >    capable of waking the system depending on the flag.
> > 
> > - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
> >    has had wake-up enabled or disabled depending on the flag.
> > 
> > - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
> >    the indicated IRQ is capable of waking the system, and the core
> >    will handle enabling/disabling irq wake capabilities on the IRQ
> >    as appropriate (dependent on device_set_wakeup_enable()). Other
> >    functions are available for wakeup IRQs that are dedicated to
> >    only waking up the system (e.g. the WOL_INT pin on AR8031).
> > 
> > Issue 1. In stmmac_init_phy(), we have this code:
> > 
> >          if (!priv->plat->pmt) {
> >                  struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> > 
> >                  phylink_ethtool_get_wol(priv->phylink, &wol);
> >                  device_set_wakeup_capable(priv->device, !!wol.supported);
> >                  device_set_wakeup_enable(priv->device, !!wol.wolopts);
> >          }
> > 
> > This reads the WoL state from the PHY (a different struct device)
> > and sets the wakeup capability and enable state for the _stmmac_
> > device accordingly, but in the case of PHY based WoL, it's the PHY
> > doing the wakeup, not the MAC. So this seems wrong on the face of
> > it.
> 
> 2 cents: Maybe even remove in stmmac_set_wol() if !priv->plat->pmt.

On the face of it, that seems like a sane solution, but sadly we
can't do that. If we did, then at least Jetson Xavier NX would break if
WoL were enabled, because DT describes the PHY interrupt, and currently
the PHY driver switches the interrupt pin from interrupt mode (where it
signals link changes) to PMEB mode (where it only pulses when a wake-up
packet is received) when WoL is enabled at the PHY. This will mean
phylib won't receive link state interrupts anymore, so unplugging/
replugging the cable won't be detected by phylib.

Even my further suggestions do not get us to a state where we can pass
the set_wol() to the PHY, and then work out what the MAC needs to do,
because e.g. in the case of RTL8211F, even if we use the "wakeup-source"
to fix the above, we still have the problem of "is wake-up supported
by the PHY wiring or is it not". We just have not described that in the
DT trees for platforms, so this is basically unknown.

Adding any logic to the kernel to make that decision will cause
regressions - either by making WoL unavailable on platforms where it
previously worked (e.g. because PHY WoL was being used) or the opposite
(where MAC WoL was used but now PHY WoL gets used but isn't wired.)

This is why I say we've boxed ourselves into a corner on this - this is
not phylib maintainers fault, because we can't know everything about
every device out there, and we don't have time to read every damn data
sheet which may or may not be publically available. We have to go by
what submitters provide us, which leads to problems like this.

So, we need flexibility from everyone to try and find a way forward.
We can't have people sticking to ideals - such as DT maintainers being
rigid about "DT describes hardware only". As I've already stated, our
DT currently does *not* describe hardware with respect to PHY wake-up,
and because this has been overlooked, we're now in a mess where there
is *no* easy solutions, and no solution that could be said to purely
describe hardware.

If we didn't have any PHY drivers, and were starting afresh, then we
would have the latitude to come up with something clean. That boat has
long since sailed.

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Russell King (Oracle)]

On Wed, Jul 23, 2025 at 10:53:55AM +0200, Gatien CHEVALLIER wrote:
> On 7/23/25 10:50, Gatien CHEVALLIER wrote:
> > On 7/22/25 22:20, Russell King (Oracle) wrote:
> > > On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
> > > > I know Russell has also replied about issues with stmmac. Please
> > > > consider that when reading what i say... It might be not applicable.
> > > > 
> > > > > Seems like a fair and logical approach. It seems reasonable that the
> > > > > MAC driver relies on the get_wol() API to know what's supported.
> > > > > 
> > > > > The tricky thing for the PHY used in this patchset is to get this
> > > > > information:
> > > > > 
> > > > > Extract from the documentation of the LAN8742A PHY:
> > > > > "The WoL detection can be configured to assert the nINT interrupt pin
> > > > > or nPME pin"
> > > > 
> > > > https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
> > > > 
> > > > It is a bit messy, but in the device tree, you could have:
> > > > 
> > > >      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
> > > >                   <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
> > > >      interrupt-names = "nINT", "wake";
> > > >      wakeup-source
> > > > 
> > > > You could also have:
> > > > 
> > > >      interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
> > > >      interrupt-names = "wake";
> > > >      wakeup-source
> > > > 
> > > > In the first example, since there are two interrupts listed, it must
> > > > be using the nPME. For the second, since there is only one, it must be
> > > > using nINT.
> > > > 
> > > > Where this does not work so well is when you have a board which does
> > > > not have nINT wired, but does have nPME. The phylib core will see
> > > > there is an interrupt and request it, and disable polling. And then
> > > > nothing will work. We might be able to delay solving that until such a
> > > > board actually exists?
> > > 
> > > (Officially, I'm still on vacation...)
> > > 
> > > At this point, I'd like to kick off a discussion about PHY-based
> > > wakeup that is relevant to this thread.
> > > 
> > > The kernel has device-based wakeup support. We have:
> > > 
> > > - device_set_wakeup_capable(dev, flag) - indicates that the is
> > >    capable of waking the system depending on the flag.
> > > 
> > > - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
> > >    has had wake-up enabled or disabled depending on the flag.
> > > 
> > > - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
> > >    the indicated IRQ is capable of waking the system, and the core
> > >    will handle enabling/disabling irq wake capabilities on the IRQ
> > >    as appropriate (dependent on device_set_wakeup_enable()). Other
> > >    functions are available for wakeup IRQs that are dedicated to
> > >    only waking up the system (e.g. the WOL_INT pin on AR8031).
> > > 
> > > Issue 1. In stmmac_init_phy(), we have this code:
> > > 
> > >          if (!priv->plat->pmt) {
> > >                  struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> > > 
> > >                  phylink_ethtool_get_wol(priv->phylink, &wol);
> > >                  device_set_wakeup_capable(priv->device,
> > > !!wol.supported);
> > >                  device_set_wakeup_enable(priv->device, !!wol.wolopts);
> > >          }
> > > 
> > > This reads the WoL state from the PHY (a different struct device)
> > > and sets the wakeup capability and enable state for the _stmmac_
> > > device accordingly, but in the case of PHY based WoL, it's the PHY
> > > doing the wakeup, not the MAC. So this seems wrong on the face of
> > > it.
> > 
> > 2 cents: Maybe even remove in stmmac_set_wol() if !priv->plat->pmt.
> > 
> 
> Sorry, that's not very clear. I was thinking of removing:
> device_set_wakeup_enable(priv->device, !!wol->wolopts); in
> stmmac_set_wol()

Yes, I think that's something which should be looked into, along with
the code at the bottom of stmmac_init_phy() calling
device_set_wakeup_capable() and device_set_wakeup_enable() depending on
the PHY state. However, that's something which needs testing by folk
who have stmmac setups that use PHY-side WoL.

It appears that my Jetson Xavier NX currently doesn't, although
MAC-side WoL also doesn't appear to work, so I've asked nVidia folk
for assistance. It could be it's supposed to use PHY-side, or maybe
there's something missing to support MAC-side (e.g. clk_rx_i is
being turned off in suspend despite WoL being enabled.)

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

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

> I've just read a bit more of the RTL8211F datasheet, and looked at the
> code, and I'm now wondering whether WoL has even been tested with
> RTL8211F. What I'm about to state doesn't negate anything I've said
> in my previous reply.
> 
> 
> So, the RTL8211F doesn't have a separate PMEB pin. It has a pin that
> is shared between "interrupt" and "PMEB".
> 
> Register 22, page 0xd40, bit 5 determines whether this pin is used for
> PMEB (in which case it is pulsed on wake-up) or whether it is used as
> an interrupt. It's one or the other function, but can't be both.

This sounds familiar.

> rtl8211f_set_wol() manipulates this bit depending on whether
> WAKE_MAGIC is enabled or not.
> 
> The effect of this is...
> 
> If we're using PHY interrupts from the RTL8211F, and then userspace
> configures magic packet WoL on the PHY, then we reconfigure the
> interrupt pin to become a wakeup pin, disabling the interrupt
> function - we no longer receive interrupts from the RTL8211F !!!!!!!

Ah. I thought that switch happened in the PHY driver suspend() call,
and it gets restored in the resume() call? That does required that
suspend/resume actually gets called despite WoL being enabled...

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Andrew Lunn]

> We can't retrofit such detection into PHY drivers - if we do so, we'll
> break WoL on lots of boards (we'd need to e.g. describe PMEB in DT for
> RTL8211F PHYs. While we can add it, if a newer kernel that expects
> PMEB to be described to allow WoL is run with an older DT, then that
> will be a regression.) Thus, I don't see how we could retrofit PHY
> WoL support detection to MAC drivers.

WoL is a mess. I wounder on how many boards it actually works
correctly? How often is it tested?

I actually think this is similar to pause and EEE. Those were also a
mess, and mostly wrongly implemented. The solution to that was to take
as much as possible out of the driver and put it into the core,
phylink.

We probably want a similar solution. The MAC driver indicates its WoL
capabilities to phylink. The PHY driver indicates its capabilities to
phylink. phylink implements all the business logic, and just tells the
PHY what it should enable, and stay awake. phylink tells the MAC what
is should enable, and that it should stay awake.

Is this going to happen? Given Russell limited availability, i guess
not. It needs somebody else to step up and take this on. Are we going
to break working systems? Probably. But given how broken this is
overall, how much should we actually care? We can just fix up systems
as they are reported broken.

I also think WoL, pause and EEE is a space we should have more tests
for. To fully test WoL and pause you need a link partner, but i
suspect you can do some basic API tests without one. WoL you
definitely need a link partner. So this makes testing a bit more
difficult. But that should not stop the community from writing such
tests and making them available for developers to use.

	Andrew

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Gatien CHEVALLIER]

On 7/23/25 11:20, Russell King (Oracle) wrote:
> On Wed, Jul 23, 2025 at 10:50:12AM +0200, Gatien CHEVALLIER wrote:
>> On 7/22/25 22:20, Russell King (Oracle) wrote:
>>> On Tue, Jul 22, 2025 at 03:40:16PM +0200, Andrew Lunn wrote:
>>>> I know Russell has also replied about issues with stmmac. Please
>>>> consider that when reading what i say... It might be not applicable.
>>>>
>>>>> Seems like a fair and logical approach. It seems reasonable that the
>>>>> MAC driver relies on the get_wol() API to know what's supported.
>>>>>
>>>>> The tricky thing for the PHY used in this patchset is to get this
>>>>> information:
>>>>>
>>>>> Extract from the documentation of the LAN8742A PHY:
>>>>> "The WoL detection can be configured to assert the nINT interrupt pin
>>>>> or nPME pin"
>>>>
>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/power/wakeup-source.txt
>>>>
>>>> It is a bit messy, but in the device tree, you could have:
>>>>
>>>>       interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>
>>>>                    <&pmic 42 IRQ_TYPE_LEVEL_LOW>;
>>>>       interrupt-names = "nINT", "wake";
>>>>       wakeup-source
>>>>
>>>> You could also have:
>>>>
>>>>       interrupts = <&sirq 0 IRQ_TYPE_LEVEL_LOW>;
>>>>       interrupt-names = "wake";
>>>>       wakeup-source
>>>>
>>>> In the first example, since there are two interrupts listed, it must
>>>> be using the nPME. For the second, since there is only one, it must be
>>>> using nINT.
>>>>
>>>> Where this does not work so well is when you have a board which does
>>>> not have nINT wired, but does have nPME. The phylib core will see
>>>> there is an interrupt and request it, and disable polling. And then
>>>> nothing will work. We might be able to delay solving that until such a
>>>> board actually exists?
>>>
>>> (Officially, I'm still on vacation...)
>>>
>>> At this point, I'd like to kick off a discussion about PHY-based
>>> wakeup that is relevant to this thread.
>>>
>>> The kernel has device-based wakeup support. We have:
>>>
>>> - device_set_wakeup_capable(dev, flag) - indicates that the is
>>>     capable of waking the system depending on the flag.
>>>
>>> - device_set_wakeup_enable(dev, flag) - indicates whether "dev"
>>>     has had wake-up enabled or disabled depending on the flag.
>>>
>>> - dev*_pm_set_wake_irq(dev, irq) - indicates to the wake core that
>>>     the indicated IRQ is capable of waking the system, and the core
>>>     will handle enabling/disabling irq wake capabilities on the IRQ
>>>     as appropriate (dependent on device_set_wakeup_enable()). Other
>>>     functions are available for wakeup IRQs that are dedicated to
>>>     only waking up the system (e.g. the WOL_INT pin on AR8031).
>>>
>>> Issue 1. In stmmac_init_phy(), we have this code:
>>>
>>>           if (!priv->plat->pmt) {
>>>                   struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>>>
>>>                   phylink_ethtool_get_wol(priv->phylink, &wol);
>>>                   device_set_wakeup_capable(priv->device, !!wol.supported);
>>>                   device_set_wakeup_enable(priv->device, !!wol.wolopts);
>>>           }
>>>
>>> This reads the WoL state from the PHY (a different struct device)
>>> and sets the wakeup capability and enable state for the _stmmac_
>>> device accordingly, but in the case of PHY based WoL, it's the PHY
>>> doing the wakeup, not the MAC. So this seems wrong on the face of
>>> it.
>>
>> 2 cents: Maybe even remove in stmmac_set_wol() if !priv->plat->pmt.
> 
> On the face of it, that seems like a sane solution, but sadly we
> can't do that. If we did, then at least Jetson Xavier NX would break if
> WoL were enabled, because DT describes the PHY interrupt, and currently
> the PHY driver switches the interrupt pin from interrupt mode (where it
> signals link changes) to PMEB mode (where it only pulses when a wake-up
> packet is received) when WoL is enabled at the PHY. This will mean
> phylib won't receive link state interrupts anymore, so unplugging/
> replugging the cable won't be detected by phylib.
> 
> Even my further suggestions do not get us to a state where we can pass
> the set_wol() to the PHY, and then work out what the MAC needs to do,
> because e.g. in the case of RTL8211F, even if we use the "wakeup-source"
> to fix the above, we still have the problem of "is wake-up supported
> by the PHY wiring or is it not". We just have not described that in the
> DT trees for platforms, so this is basically unknown.
> 

Yes but the PHY is described as board level, so for each board, each
vendor/industrial/amateur has the knowledge of the pin being wired.
or not. Therefore, the "wakeup-source" property could be added only
if that's the case, couldn't it? Maybe I'm missing something.

This way, the PHY driver could ultimately return an AND of the property
being present and the PHY WoL capabilities for wol.supported field.
However, that's seems a bit messy and suggests not relying on pmt being
present when setting WoL for stmmac and other reworks in other places.

TBH I'm not sure how to handle this without massive changes...

> Adding any logic to the kernel to make that decision will cause
> regressions - either by making WoL unavailable on platforms where it
> previously worked (e.g. because PHY WoL was being used) or the opposite
> (where MAC WoL was used but now PHY WoL gets used but isn't wired.)
> 
> This is why I say we've boxed ourselves into a corner on this - this is
> not phylib maintainers fault, because we can't know everything about
> every device out there, and we don't have time to read every damn data
> sheet which may or may not be publically available. We have to go by
> what submitters provide us, which leads to problems like this.
> 
> So, we need flexibility from everyone to try and find a way forward.
> We can't have people sticking to ideals - such as DT maintainers being
> rigid about "DT describes hardware only". As I've already stated, our
> DT currently does *not* describe hardware with respect to PHY wake-up,
> and because this has been overlooked, we're now in a mess where there
> is *no* easy solutions, and no solution that could be said to purely
> describe hardware.
> 
> If we didn't have any PHY drivers, and were starting afresh, then we
> would have the latitude to come up with something clean. That boat has
> long since sailed.
> 

Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

[Florian Fainelli]

On 7/23/25 07:23, Andrew Lunn wrote:
>> We can't retrofit such detection into PHY drivers - if we do so, we'll
>> break WoL on lots of boards (we'd need to e.g. describe PMEB in DT for
>> RTL8211F PHYs. While we can add it, if a newer kernel that expects
>> PMEB to be described to allow WoL is run with an older DT, then that
>> will be a regression.) Thus, I don't see how we could retrofit PHY
>> WoL support detection to MAC drivers.
> 
> WoL is a mess. I wounder on how many boards it actually works
> correctly? How often is it tested?
 > > I actually think this is similar to pause and EEE. Those were also a
> mess, and mostly wrongly implemented. The solution to that was to take
> as much as possible out of the driver and put it into the core,
> phylink.
> 
> We probably want a similar solution. The MAC driver indicates its WoL
> capabilities to phylink. The PHY driver indicates its capabilities to
> phylink. phylink implements all the business logic, and just tells the
> PHY what it should enable, and stay awake. phylink tells the MAC what
> is should enable, and that it should stay awake.

We would need both a phylib and a phylink set of helpers because not all 
of the drivers need to be converted to phylink.

> 
> Is this going to happen? Given Russell limited availability, i guess
> not. It needs somebody else to step up and take this on. Are we going
> to break working systems? Probably. But given how broken this is
> overall, how much should we actually care? We can just fix up systems
> as they are reported broken.

Please just refrain from making such statements, it really just does not 
help, and if you have no direct hands on experience with Wake-on-LAN, it 
becomes purely gratuitous. I agree that there is a lack of adequate 
consistency and guidelines for developers to implement Wake-on-LAN 
properly, but I don't agree with the message and the way it is 
delivered. It's just completely antagonistic to people like me and my 
colleagues who have spent a great deal of time implementing Wake-on-LAN 
for actively used systems, and I am talking hundred of millions of STBs 
deployed each of them doing hundreds of system suspend/resume involving 
Wake-on-LAN per day.

> 
> I also think WoL, pause and EEE is a space we should have more tests
> for. To fully test WoL and pause you need a link partner, but i
> suspect you can do some basic API tests without one. WoL you
> definitely need a link partner. So this makes testing a bit more
> difficult. But that should not stop the community from writing such
> tests and making them available for developers to use.

The tests are in premise very simple, but you need a link partner and 
you need to be ideally on the same physical network and you need to have 
a system that supports system wide wake-up, or if nothing else s2idle. 
Then you need a secondary wake-up source like a RTC or GPIO in order to 
ensure that there is an upper bound on when you timeout for not 
receiving a proper wake-on-LAN trigger.

It's not clear to me what needs to be contributed to the community, but 
essentially the pseudo code is something like:

- wait for DUT to boot
for each support Wake-on-LAN mode:
	- configure wake-on-LAN on DUT
	- snapshot /sys/*/wakeup_count for the MAC/PHY device
	- enter standby with e.g.: rtcwake -s <TIMEOUT> -m mem
	- send Wake-on-LAN trigger from external device
	- ensure DUT woke-up before <TIMEOUT> and check that /sys/*wakeup_count 
is +1 compared to the previous snapshot
-- 
Florian