Message-ID: <aICpe63iqldl9NFd@shell.armlinux.org.uk>
Date: Wed, 23 Jul 2025 10:20:59 +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: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!

Powered by blists - more mailing lists