Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Florian Fainelli <f.fainelli@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

Hi Andrew, Russell,

On 3/30/23 17:54, Andrew Lunn wrote:
> Most MAC drivers get EEE wrong. The API to the PHY is not very
> obvious, which is probably why. Rework the API, pushing most of the
> EEE handling into phylib core, leaving the MAC drivers to just
> enable/disable support for EEE in there change_link call back, or
> phylink mac_link_up callback.
> 
> MAC drivers are now expect to indicate to phylib/phylink if they
> support EEE. If not, no EEE link modes are advertised. If the MAC does
> support EEE, on phy_start()/phylink_start() EEE advertisement is
> configured.

Thanks for doing this work, because it really is a happy mess out there. 
A few questions as I have been using mvneta as the reference for fixing 
GENET and its shortcomings.

In your new patches the decision to enable EEE is purely based upon the 
eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what 
mvneta useed to do.

Russell, is there an use case for having eee_enabled while not having 
tx_lpi_enabled?

With the candidate patch attached, I have the following behavior on boot 
with no specific ethtool operation:

# ethtool --show-eee eth0
EEE Settings for eth0:
         EEE status: enabled - active
         Tx LPI: disabled
         Supported EEE link modes:  100baseT/Full
                                    1000baseT/Full
         Advertised EEE link modes:  100baseT/Full
                                     1000baseT/Full
         Link partner advertised EEE link modes:  100baseT/Full
                                                  1000baseT/Full
#

however EEE is not *really* enabled at the hardware level unless I do 
another:

# ethtool --set-eee eth0 tx-lpi on
# ethtool --show-eee eth0
EEE Settings for eth0:
         EEE status: enabled - active
         Tx LPI: 34 (us)
         Supported EEE link modes:  100baseT/Full
                                    1000baseT/Full
         Advertised EEE link modes:  100baseT/Full
                                     1000baseT/Full
         Link partner advertised EEE link modes:  100baseT/Full
                                                  1000baseT/Full

We have a global EEE enable bit which is described as "If set, the TX 
LPI policy control engine is enabled and the MAC inserts LPI_idle codes 
if the link is idle", so it would seem to me that we should require 
users to set both "eee on" and "tx-lpi on" in their ethtool command, but 
then unless we intentionally override tx_lpi_enabled in the driver upon 
probe, there will be any automagical configuration happening, and no 
power savings being realized.

Do you have any recommendations on what drivers should do? As you could 
see from the need of this patch series, we already all created our own 
little schisms of the cargo cult.

Thanks!
-- 
Florian

[-- Attachment #2: 0001-net-bcmgenet-Fix-EEE-implementation.patch --]
[-- Type: text/x-patch, Size: 4226 bytes --]

From ff5ed48c008e052a3005e81001748058045d09d2 Mon Sep 17 00:00:00 2001
From: Florian Fainelli <florian.fainelli@broadcom.com>
Date: Tue, 30 May 2023 11:07:57 -0700
Subject: [PATCH] net: bcmgenet: Fix EEE implementation

We had a number of short comings:

- EEE must be re-evaluated whenever the state machine detects a link
change as wight be switching from a link partner with EEE
enabled/disabled

- We do not need to forcibly enable EEE upon system resume, as the PHY
state machine will trigger a link event that will do that, too

- EEE shall be enabled if both eee_active and tx_lpi_enabled are set

refs #SWLINUX-6655

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
Change-Id: Ie9c3ff63458f08f00b0d19625205396c7a3d5306
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 18 ++++++------------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  2 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  4 ++++
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9d770b76f9bb..f31930215099 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1291,7 +1291,7 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
-static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
+void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
@@ -1347,6 +1347,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 
 	e->eee_enabled = p->eee_enabled;
 	e->eee_active = p->eee_active;
+	e->tx_lpi_enabled = p->tx_lpi_enabled;
 	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_get_eee(dev->phydev, e);
@@ -1356,7 +1357,6 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct ethtool_eee *p = &priv->eee;
-	int ret = 0;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1369,14 +1369,11 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!p->eee_enabled) {
 		bcmgenet_eee_enable_set(dev, false);
 	} else {
-		ret = phy_init_eee(dev->phydev, 0);
-		if (ret) {
-			netif_err(priv, hw, dev, "EEE initialization failed\n");
-			return ret;
-		}
-
+		p->eee_active = phy_init_eee(dev->phydev, 0) >= 0;
+		p->tx_lpi_enabled = e->tx_lpi_enabled;
 		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
-		bcmgenet_eee_enable_set(dev, true);
+		bcmgenet_eee_enable_set(dev,
+					p->eee_active && p->tx_lpi_enabled);
 	}
 
 	return phy_ethtool_set_eee(dev->phydev, e);
@@ -4310,9 +4307,6 @@ static int bcmgenet_resume(struct device *d)
 	if (!device_may_wakeup(d))
 		phy_resume(dev->phydev);
 
-	if (priv->eee.eee_enabled)
-		bcmgenet_eee_enable_set(dev, true);
-
 	bcmgenet_netif_start(dev);
 
 	netif_device_attach(dev);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 47b8326c2f39..ce6b9f29f3d8 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -758,4 +758,6 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode);
 
+void bcmgenet_eee_enable_set(struct net_device *dev, bool enable);
+
 #endif /* __BCMGENET_H__ */
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 613e40202915..90cdd54f8a8d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -108,6 +108,10 @@ static void bcmgenet_mac_config(struct net_device *dev)
 		reg |= CMD_TX_EN | CMD_RX_EN;
 	}
 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+
+	priv->eee.eee_active = phy_init_eee(phydev, 0) >= 0;
+	bcmgenet_eee_enable_set(dev, priv->eee.eee_active &&
+				priv->eee.tx_lpi_enabled);
 }
 
 /* setup netdev link state when PHY link status change and
-- 
2.25.1