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