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

[Gatien CHEVALLIER <gatien.chevallier@foss.st.com>]

On 9/17/25 18:31, Russell King (Oracle) wrote:
> On Wed, Sep 17, 2025 at 05:36:37PM +0200, Gatien Chevallier wrote:
>> 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 77a04c4579c9dbae886a0b387f69610a932b7b9e..6f197789cc2e8018d6959158b795e4bca46869c5 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;
>>   }
>>   
>> @@ -557,6 +560,8 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
>>   	plat_dat->bsp_priv = dwmac;
>>   	plat_dat->suspend = stm32_dwmac_suspend;
>>   	plat_dat->resume = stm32_dwmac_resume;
>> +	if (dwmac->phy_wol)
>> +		plat_dat->flags |= STMMAC_FLAG_USE_PHY_WOL;
> 
> I would much rather we found a different approach, rather than adding
> custom per-driver DT properties to figure this out.
> 
> Andrew has previously suggested that MAC drivers should ask the PHY
> whether WoL is supported, but this pre-supposes that PHY drivers are
> coded correctly to only report WoL capabilities if they are really
> capable of waking the system. As shown in your smsc PHY driver patch,
> this may not be the case.

So how can we distinguish whether a PHY that implements WoL features
is actually able (wired) to wake up the system? By adding the
"wakeup-source" property to the PHY node?

Therefore, only set the "can wakeup" capability when both the PHY
supports WoL and the property is present in the PHY node?

However, this does not solve the actual static pin function
configuration for pins that can, if correct alternate function is
selected, generate interrupts, in PHY drivers.

It would be nice to be able to apply some kind of pinctrl to configure
the PHY pins over the MDIO bus thanks to some kind of pinctrl hogging.
This suggests modifying relevant PHY drivers and documentation to be
able to handle an optional pinctrl.

Disregarding syntax issues, could be something like:

phy0_eth1: ethernet-phy@0 {
	compatible = "ethernet-phy-id0007.c131";
	reg = <0>;
	reset-gpios = <&mcp23017 9 GPIO_ACTIVE_LOW>;
	wakeup-source;
	pinctrl-0 = <&phy_pin_npme_hog &phy_pin_nint_hog>;

	phy_pin_npme_hog: nPME {
		pins = "LED2/nINT/nPME/nINTSEL";
		function = "nPME";
	};

	phy_pin_nint_hog: nINT {
		pins = "LED1/nINT/nPME/nINTSEL";
		function = "nINT";
	};
};

What do you think of that?

Otherwise, the idea below looks promising to me.

> 
> Given that we have historically had PHY drivers reporting WoL
> capabilities without being able to wake the system, we can't
> implement Andrew's suggestion easily.
> 
> The only approach I can think that would allow us to transition is
> to add:
> 
> static inline bool phy_can_wakeup(struct phy_device *phy_dev)
> {
> 	return device_can_wakeup(&phy_dev->mdio.dev);
> }
> 
> to include/linux/phy.h, and a corresponding wrapper for phylink.
> This can then be used to determine whether to attempt to use PHY-based
> Wol in stmmac_get_wol() and rtl8211f_set_wol(), falling back to
> PMT-based WoL if supported at the MAC.
> 
> So, maybe something like:
> 
> static u32 stmmac_wol_support(struct stmmac_priv *priv)
> {
> 	u32 support = 0;
> 
> 	if (priv->plat->pmt && device_can_wakeup(priv->device)) {
> 		support = WAKE_UCAST;
> 		if (priv->hw_cap_support && priv->dma_cap.pmt_magic_frame)
> 			support |= WAKE_MAGIC;
> 	}
> 
> 	return support;
> }
> 
> static void stmmac_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> {
> 	struct stmmac_priv *priv = netdev_priv(dev);
> 	int err;
> 
> 	/* Check STMMAC_FLAG_USE_PHY_WOL for legacy */
> 	if (phylink_can_wakeup(priv->phylink) ||
> 	    priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL) {
> 		err = phylink_ethtool_get_wol(priv->phylink, wol);
> 		if (err != 0 && err != -EOPNOTSUPP)
> 			return;
> 	}
> 
> 	wol->supported |= stmmac_wol_support(priv);
> 
> 	/* A read of priv->wolopts is single-copy atomic. Locking
> 	 * doesn't add any benefit.
> 	 */
> 	wol->wolopts |= priv->wolopts;
> }
> 
> static int stmmac_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
> {
> 	struct stmmac_priv *priv = netdev_priv(dev);
> 	u32 support, wolopts;
> 	int err;
> 
> 	wolopts = wol->wolopts;
> 
> 	/* Check STMMAC_FLAG_USE_PHY_WOL for legacy */
> 	if (phylink_can_wakeup(priv->phylink) ||
> 	    priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL) {
> 		struct ethtool_wolinfo w;
> 
> 		err = phylink_ethtool_set_wol(priv->phylink, wol);
> 		if (err != -EOPNOTSUPP)
> 			return err;
> 
> 		/* Remove the WoL modes that the PHY is handling */
> 		if (!phylink_ethtool_get_wol(priv->phylink, &w))
> 			wolopts &= ~w.wolopts;
> 	}
> 
> 	support = stmmac_wol_support(priv);
> 
> 	mutex_lock(&priv->lock);
> 	priv->wolopts = wolopts & support;
> 	device_set_wakeup_enable(priv->device, !!priv->wolopts);
> 	mutex_unlock(&priv->lock);
> 
> 	return 0;
> }
> 
> ... and now I'm wondering whether this complexity is something that
> phylink should handle internally, presenting a mac_set_wol() method
> to configure the MAC-side WoL settings. What makes it difficult to
> just move into phylink is the STMMAC_FLAG_USE_PHY_WOL flag, but
> that could be a "force_phy_wol" flag in struct phylink_config as
> a transitionary measure... so long as PHY drivers get fixed.
>