[PATCH net-next v1 1/1] net: sxgbe: rework EEE handling to use PHY negotiation results

[PATCH net-next v1 1/1] net: sxgbe: rework EEE handling to use PHY negotiation results

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So rework sxgbe_eee_adjust() to take the result of
negotiation into account.

sxgbe_set_eee() now just stores LTI timer value. Everything else is
passed to phylib, so it can correctly setup the PHY.

sxgbe_get_eee() relies on phylib doing most of the work, the MAC
driver just adds the LTI timer value.

The hw_cap.eee is now used to control timers, rather than eee_enabled,
which was wrongly being set based on the value of phy_init_eee()
before auto-neg even completed.

WARNING: This patch is only compile tested.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_common.h |  2 -
 .../ethernet/samsung/sxgbe/sxgbe_ethtool.c    | 19 +---------
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 38 +++++++------------
 3 files changed, 15 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
index 1458939c3bf5..a9e964142c9c 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
@@ -502,7 +502,6 @@ struct sxgbe_priv_data {
 	struct timer_list eee_ctrl_timer;
 	bool tx_path_in_lpi_mode;
 	int lpi_irq;
-	int eee_enabled;
 	int tx_lpi_timer;
 };
 
@@ -527,5 +526,4 @@ int sxgbe_restore(struct net_device *ndev);
 const struct sxgbe_mtl_ops *sxgbe_get_mtl_ops(void);
 
 void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv);
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv);
 #endif /* __SXGBE_COMMON_H__ */
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
index 4a439b34114d..263f1070cbed 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
@@ -150,22 +150,7 @@ static int sxgbe_set_eee(struct net_device *dev,
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	priv->eee_enabled = edata->eee_enabled;
-
-	if (!priv->eee_enabled) {
-		sxgbe_disable_eee_mode(priv);
-	} else {
-		/* We are asking for enabling the EEE but it is safe
-		 * to verify all by invoking the eee_init function.
-		 * In case of failure it will return an error.
-		 */
-		priv->eee_enabled = sxgbe_eee_init(priv);
-		if (!priv->eee_enabled)
-			return -EOPNOTSUPP;
-
-		/* Do not change tx_lpi_timer in case of failure */
-		priv->tx_lpi_timer = edata->tx_lpi_timer;
-	}
+	priv->tx_lpi_timer = edata->tx_lpi_timer;
 
 	return phy_ethtool_set_eee(dev->phydev, edata);
 }
@@ -228,7 +213,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev,
 	int i;
 	char *p;
 
-	if (priv->eee_enabled) {
+	if (dev->phydev->eee_active) {
 		int val = phy_get_eee_err(dev->phydev);
 
 		if (val)
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 12c8396b6942..b306e12ac6bf 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -119,17 +119,10 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t)
  *  phy can also manage EEE, so enable the LPI state and start the timer
  *  to verify if the tx path can enter in LPI state.
  */
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
+static void sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 {
-	struct net_device *ndev = priv->dev;
-	bool ret = false;
-
 	/* MAC core supports the EEE feature. */
 	if (priv->hw_cap.eee) {
-		/* Check if the PHY supports EEE */
-		if (phy_init_eee(ndev->phydev, true))
-			return false;
-
 		timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0);
 		priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer);
 		add_timer(&priv->eee_ctrl_timer);
@@ -139,23 +132,15 @@ bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 					     priv->tx_lpi_timer);
 
 		pr_info("Energy-Efficient Ethernet initialized\n");
-
-		ret = true;
 	}
-
-	return ret;
 }
 
-static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv)
+static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv,
+			     bool eee_active)
 {
-	struct net_device *ndev = priv->dev;
-
-	/* When the EEE has been already initialised we have to
-	 * modify the PLS bit in the LPI ctrl & status reg according
-	 * to the PHY link status. For this reason.
-	 */
-	if (priv->eee_enabled)
-		priv->hw->mac->set_eee_pls(priv->ioaddr, ndev->phydev->link);
+	if (priv->hw_cap.eee)
+		priv->hw->mac->set_eee_pls(priv->ioaddr, eee_active);
+	phy_eee_rx_clock_stop(priv->dev->phydev, true);
 }
 
 /**
@@ -249,7 +234,7 @@ static void sxgbe_adjust_link(struct net_device *dev)
 		phy_print_status(phydev);
 
 	/* Alter the MAC settings for EEE */
-	sxgbe_eee_adjust(priv);
+	sxgbe_eee_adjust(priv, phydev->eee_active);
 }
 
 /**
@@ -296,6 +281,9 @@ static int sxgbe_init_phy(struct net_device *ndev)
 	    (phy_iface == PHY_INTERFACE_MODE_RMII))
 		phy_set_max_speed(phydev, SPEED_1000);
 
+	if (priv->hw_cap.eee)
+		phy_support_eee(phydev);
+
 	if (phydev->phy_id == 0) {
 		phy_disconnect(phydev);
 		return -ENODEV;
@@ -802,7 +790,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv)
 		sxgbe_tx_queue_clean(tqueue);
 	}
 
-	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
+	if (priv->hw_cap.eee && !priv->tx_path_in_lpi_mode) {
 		sxgbe_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
 	}
@@ -1180,7 +1168,7 @@ static int sxgbe_open(struct net_device *dev)
 	}
 
 	priv->tx_lpi_timer = SXGBE_DEFAULT_LPI_TIMER;
-	priv->eee_enabled = sxgbe_eee_init(priv);
+	sxgbe_eee_init(priv);
 
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
@@ -1207,7 +1195,7 @@ static int sxgbe_release(struct net_device *dev)
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	if (priv->eee_enabled)
+	if (priv->hw_cap.eee)
 		del_timer_sync(&priv->eee_ctrl_timer);
 
 	/* Stop and disconnect the PHY */
-- 
2.39.5

Re: [PATCH net-next v1 1/1] net: sxgbe: rework EEE handling to use PHY negotiation results

[Russell King (Oracle)]

On Thu, Feb 06, 2025 at 08:58:56AM +0100, Oleksij Rempel wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So rework sxgbe_eee_adjust() to take the result of
> negotiation into account.

PLS, at least in stmmac terminology, is "phy link status" and there it
is used with a separate timer which prevents LPI mode being entered
near to the link coming up. Given that the method is called
priv->hw->mac->set_eee_pls, and what was being passed to it was the
link status, I think this is the same thing in this driver.

So, I think repurposing this bit to indicate whether EEE has been
negotiated is not correct.

> sxgbe_set_eee() now just stores LTI timer value. Everything else is
> passed to phylib, so it can correctly setup the PHY.

Not sure why you don't use phydev->eee_cfg.lpi_timer which will be
updated correctly - if phy_ethtool_set_eee() reutrns an error, then
priv->tx_lpi_timer will have been updated yet an error will be
returned to userspace. Sure, have priv->tx_lpi_timer, but update
this on link-up.

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