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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jul 22, 2025 at 11:00:34PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 22, 2025 at 10:39:53PM +0100, Russell King (Oracle) wrote:
> > rtl8211f_get_wol() does not take account of whether the PMEB pin is
> > wired or not. Thus, stmmac can't just forward the get_wol() and
> > set_wol() ops to the PHY driver and let it decide, as suggested
> > earlier. As stmmac gets used with multiple PHYs, (and hey, we can't
> > tell what they are, because DT doesn't list what the PHY actually is!)
> > we can't know how many other PHY drivers also have this problem.
> 
> I've just read a bit more of the RTL8211F datasheet, and looked at the
> code, and I'm now wondering whether WoL has even been tested with
> RTL8211F. What I'm about to state doesn't negate anything I've said
> in my previous reply.
> 
> 
> So, the RTL8211F doesn't have a separate PMEB pin. It has a pin that
> is shared between "interrupt" and "PMEB".
> 
> Register 22, page 0xd40, bit 5 determines whether this pin is used for
> PMEB (in which case it is pulsed on wake-up) or whether it is used as
> an interrupt. It's one or the other function, but can't be both.
> 
> rtl8211f_set_wol() manipulates this bit depending on whether
> WAKE_MAGIC is enabled or not.
> 
> The effect of this is...
> 
> If we're using PHY interrupts from the RTL8211F, and then userspace
> configures magic packet WoL on the PHY, then we reconfigure the
> interrupt pin to become a wakeup pin, disabling the interrupt
> function - we no longer receive interrupts from the RTL8211F !!!!!!!
> 
> Yes, the driver does support interrupts for this device!
> 
> This is surely wrong because it will break phylib's ability to track
> the link state as there will be no further interrupts _and_ phylib
> won't be expecting to poll the PHY.
> 
> The really funny thing is that the PHY does have the ability to
> raise an interrupt if a wakeup occurs through the interrupt pin
> (when configured as such) via register 18, page 0xa42, bit 7...
> but the driver doesn't touch that.
> 
> 
> Jetson Xavier NX uses interrupts from this PHY. Forwarding an
> ethtool .set_wol() op to the PHY driver which enables magic packet
> will, as things stand, switch the interrupt pin to wake-up only
> mode, preventing delivery of further link state change events to
> phylib, breaking phylib.
> 
> Maybe there's a need for this behaviour with which-ever network
> driver first used RTL8211F in the kernel. Maybe the set of network
> drivers that use interrupts from the RTL8211F don't use WoL and
> vice versa. If there's any network drivers that do forward WoL
> calls to the RTL8211F driver _and_ use interrupts from the PHY...
> that's just going to break if magic packet WoL is ever enabled at
> the PHY.

The only solutions I can think that may work with RTL8211F are:

Solution 1. move the control of RTL8211F_INTBCR_INTB_PMEB to a new
  rtl8211f_suspend() / rtl8211f_resume(), and switch the pin between
  interrupt mode and PMEB mode accordingly if WoL is enabled. This
  should be relatively low-risk, and not require DT changes.

Solution 2. don't switch to PMEB mode if phylib is using interrupts.
  Instead, enable WoL interrupt when in INTB mode. Also needs
  rtl8211f_handle_interrupt() modified to "handle" the interrupt
  to prevent an interrupt storm. May cause other problems - PMEB
  is pulsed, WoL over INTB is level-based, so higher risk.

Solution 3. introduce a DT flag for rtl8211f PHYs to tell the PHY
  driver "this platform should enable WoL interrupts in INTB mode
  and not switch to PMEB mode". Safest, as no change in behaviour
  without the flag being present... but arguable whether it truly
  describes hardware. However, what we currently have in DT
  *doesn't* actually describe hardware because of the mistakes made.
  (Maybe we can use the wakeup-source property to indicate this
  mode, which may be more acceptable to Krzysztof than a whole new
  flag.)

Maybe something else would be acceptable to DT folk - I think I've
provided enough of a description of the problem we currently have
to allow DT folk to digest the issue here.

Just random thoughts below... here's the description of the PHY on
the Jetson Xavier NX which I'll use as a basis for some scenarios.
(from arch/arm64/boot/dts/nvidia/tegra194-p3668.dtsi).

	phy: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x0>;
		interrupt-parent = <&gpio>;
		interrupts = <TEGRA194_MAIN_GPIO(G, 4) IRQ_TYPE_LEVEL_LOW>;
		#phy-cells = <0>;
	};

If WoL is supported through interrupts, then maybe we describe it
as:

	phy: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x0>;
		interrupt-parent = <&gpio>;
		interrupts = <TEGRA194_MAIN_GPIO(G, 4) IRQ_TYPE_LEVEL_LOW>;
		#phy-cells = <0>;
		wakeup-source;
	};

WoL is supported through PMEB, no interrupt support, then it gets
described as:

	phy: ethernet-phy@0 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x0>;
		#phy-cells = <0>;
		wakeup-source;
	};

The problem becomes how to describe the _existing_ behaviour going
forward, which we get with the current (first) description above. We
would need to preserve this for the existing description for backward
compatibility to avoid breaking existing setups. Do we try to come up
with something different that allows wakeup-source to be added?
Should we say that shared-interrupt and PMEB mode isn't something we
support except for legacy stuff, and thus not care about the missing
wakeup-source property? Something else, if so what (please suggest) ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!