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 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.
>