On Tue, Dec 17, 2024 at 09:50:12PM +0100, Heiner Kallweit wrote:

On 17.12.2024 11:43, Andrew Lunn wrote:

quoted

quoted

@@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
 void phy_support_sym_pause(struct phy_device *phydev);
 void phy_support_asym_pause(struct phy_device *phydev);
 void phy_support_eee(struct phy_device *phydev);
+void phy_disable_eee(struct phy_device *phydev);

So we have three states:

MAC tells PHYLIB it does support EEE
MAC tells PHYLIB it does not support EEE
MAC says nothing.

Do we really want this?

For phylink, i think we have a nice new clean design and can say, if
the MAC does not indicate it supports EEE, we turn it off in the
PHY. For phylib, we have more of a mess, and there could be MACs
actually doing EEE by default using default setting but with no user
space control. Do we want to keep this, or should we say any MAC which
does not call phy_support_eee() before phy_start() would have EEE
disabled in the PHY?

The case "MAC says nothing" isn't desirable. However, if we did what
you mention, we'd silently change the behavior of several drivers,
resulting in disabled EEE and higher power consumption.
I briefly grepped the kernel source for phy_start() and found about
70 drivers. Some of them have the phylib EEE call, and in some cases
like cpsw the MAC doesn't support EEE. But what remains is IMO too
many drivers where we'd change the behavior.

So for phylib, we keep with the three states. But phylink? Can we
disable EEE when the MAC says nothing?

	Andrew