Message-ID: <aICqi9eRi-vB1i1m@shell.armlinux.org.uk>
Date: Wed, 23 Jul 2025 10:25:31 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
Cc: Andrew Lunn <andrew@...n.ch>, Krzysztof Kozlowski <krzk@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Christophe Roullier <christophe.roullier@...s.st.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Simon Horman <horms@...nel.org>,
	Tristram Ha <Tristram.Ha@...rochip.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol
 property

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!

Powered by blists - more mailing lists