Message-ID: <9c9499e3-10c9-4245-938a-65831fe10c05@foss.st.com>
Date: Wed, 23 Jul 2025 10:53:55 +0200
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
        Andrew Lunn
	<andrew@...n.ch>
CC: 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 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.)
>>

Powered by blists - more mailing lists