Re: [PATCH net-next 1/4] dt-bindings: net: document st,phy-wol property

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

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()

>>
>> 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?
>>
>> 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?
>>
>> (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.)
>>