Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property
From: Gatien CHEVALLIER
Date: Wed Jul 23 2025 - 10:39:57 EST
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.