Re: [PATCH net-next 2/2] net: stmmac: dwmac-imx: add support for PHY WOL

From: Florian Fainelli
Date: Fri Mar 08 2024 - 12:56:56 EST

On 3/8/24 09:48, POPESCU Catalin wrote:
On 07.03.24 19:52, Florian Fainelli wrote:
[Some people who received this message don't often get email from
f.fainelli@xxxxxxxxx. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]

This email is not from Hexagon’s Office 365 instance. Please be
careful while clicking links, opening attachments, or replying to this
email.


On 3/7/2024 1:13 AM, POPESCU Catalin wrote:
On 06.03.24 18:41, Florian Fainelli wrote:
[Some people who received this message don't often get email from
f.fainelli@xxxxxxxxx. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]

This email is not from Hexagon’s Office 365 instance. Please be
careful while clicking links, opening attachments, or replying to this
email.


On 3/6/24 09:24, Catalin Popescu wrote:
Add support for PHY WOL capability into dwmac-imx MAC driver.
This is required to enable WOL feature on a platform where MAC
WOL capability is not sufficient and WOL capability built into
the PHY is actually needed.

Signed-off-by: Catalin Popescu <catalin.popescu@xxxxxxxxxxxxxxxxxxxx>

Nope, this is not about how to do this. You use a Device Tree property
as a policy rather than properly describe your systems capabilities.
I'm not sure what policy means in that context.
BTW, dwmac-mediatek does the same with binding "mediatek,mac-wol" which
is a commit from 03/2022.

Policy here means you want a certain behavior from the OS that is
consuming the Device Tree, and that behavior is encoded via a Device
Tree property. This is different from describing how the hardware works
which does not make any provisions for getting a behavior out of the OS.

I understand this way of doing became "unacceptable" since then ??

It was not acceptable then, but there is only a limited reviewer time,
and it is easy unfortunately to sneak through reviewers.


What sort of Wake-on-LAN do you want to be done by the PHY exactly?
Does
the PHY have packet matching capabilities, or do you want to wake-up
from a PHY event like link up/down/any interrupt?

PHY is TI dp83826 and has secure magic packet capability. For the wakeup
we rely on a external MCU which is signaled through a PHY's GPIO which
toggles only on magic packet reception.
We want to wakeup _only_ on magic packet reception.

Then you need to represent that wake-up GPIO line in the Device Tree,
associate it with the PHY's Device Tree node for starters and add in a
'wakeup-source' property in the Device Tree.
The GPIO I was referring to is a PHY GPIO not a SOC GPIO, so there's no
way to describe it into the DT.

Well, technically there is, it's just that the PHY is not registered with Linux as a GPIO controller/provider, nothing prevents you from doing that, but it starts raising the bar.

The PHY is connected on the SOC MDIO bus, so the SOC programs the PHY,
but the PHY wakes up the external MCU which in turn wakes up the SOC.

OK, but that still needs to be described somehow, otherwise you are just cutting corners and assuming that the pin from the PHY to the external MCU is only driven when the PHY drives it, how about other wake-up sources to the MCU?



Now the PHY driver can know about the existence of a GPIO and it can
know the PHY is a system wake-up source, so the driver can call
device_set_wakeup_capable().

In user-space you have to configure the network interface with
WAKE_MAGICSECURE which needs to propagate to the PHY driver for adequate
configuration. Still in user-space you need to make the PHY device
wake-up *enabled* by doing:

echo "enable" > /sys/class/net/ethX/attached_phydev/power/wakeup

If both WAKE_MAGICSECURE is enabled and the PHY device in sysfs reports
that it is wake-up enabled would you wake-up from the PHY's GPIO. Your
PHY driver ought to be modified to check for both

device_wakeup_enabled() and wolopts being non-zero to call
enable_irq_wake() on the GPIO interrupt line.

That's how I would go about doing this, yes it's a tad more complicated
than adding an ad-hoc Device Tree property, but it's more flexible and
it's transposable to other configurations, too. Whether that sort of
encoding needs to be in the individual PHY drivers or somewhere in the
PHY library can be decided if we have more than one similar
configuration to support.

Yes, it's more complicated and it doesn't apply to our platform.
But, this doesn't really matter in the end, the problem I'm trying to
address here is to allow stmac for IMX to support PHY WOL.
Since the binding is not acceptable, I guess the only option here is to
remove flag STMMAC_FLAG_USE_PHY_WOL from stmac driver and replace it
with a call to phylink_ethtool_get_wol to identify whether the PHY is
WOL capable or not.
Then, how should we allow the platform to choose b/w MAC WOL and PHY WOL
if both are supported ?

We don't have a good way to configure that decision consistently and across *all* device drivers currently, what I can think of as the least bad solution is to intersect between the PHY supported WOL modes, the MAC supported WOL modes, and checking which of those is a wake-up enable device via device_wakeup_enabled() and use that one with a preference for the PHY since that is the closest to the wire. But this might be good for me and you, maybe not for others.

AFAIK ethtool only knows about MAC WOL capability since it interrogates
the MAC interface. ethtool doesn't know anything about the PHY, or does
it ??

No we don't, and until Maxime's patches about PHY topology land upstream:

https://lwn.net/Articles/961959/

we do not want to invent many different ways of specifying which of the MAC or the PHY should be used for WOL. FWIW, I have a similar need:

https://www.spinics.net/lists/netdev/msg751196.html

https://lore.kernel.org/all/20231026224509.112353-1-florian.fainelli@xxxxxxxxxxxx/
--
Florian