diff mbox series

[net-next,v4,09/10] net: usb: lan78xx: Integrate EEE support with phylink LPI API

Message ID 20250318093410.3047828-10-o.rempel@pengutronix.de (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series Convert LAN78xx to PHYLINK | expand

Commit Message

Oleksij Rempel March 18, 2025, 9:34 a.m. UTC
Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver to
fully integrate with the phylink Low Power Idle (LPI) API. This includes:

- Replacing direct calls to `phy_ethtool_get_eee` and `phy_ethtool_set_eee`
  with `phylink_ethtool_get_eee` and `phylink_ethtool_set_eee`.
- Implementing `.mac_enable_tx_lpi` and `.mac_disable_tx_lpi` to control
  LPI transitions via phylink.
- Configuring `lpi_timer_default` to align with recommended values from
  LAN7800 documentation.
- ensure EEE is disabled on controller reset

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v2:
- use latest PHYlink TX_LPI API
---
 drivers/net/usb/lan78xx.c | 120 ++++++++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 44 deletions(-)

Comments

Russell King (Oracle) March 18, 2025, 10:17 a.m. UTC | #1
On Tue, Mar 18, 2025 at 10:34:09AM +0100, Oleksij Rempel wrote:
> +static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> +				     bool tx_clk_stop)
> +{
> +	struct net_device *net = to_net_dev(config->dev);
> +	struct lan78xx_net *dev = netdev_priv(net);
> +	int ret;
> +
> +	/* Software should only change this field when Energy Efficient
> +	 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
> +	 * EEEEN during probe, and phylink itself guarantees that
> +	 * mac_disable_tx_lpi() will have been previously called.
> +	 */
> +	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, timer);
> +	if (ret < 0)
> +		goto tx_lpi_fail;
> +
> +	ret = lan78xx_mac_eee_enable(dev, true);
> +	if (ret < 0)
> +		goto tx_lpi_fail;
> +
> +	return 0;
> +
> +tx_lpi_fail:
> +	netdev_err(dev->net, "Failed to enable TX LPI with error %pe\n",
> +		   ERR_PTR(ret));

This function is called thusly:

        err = pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
                                             pl->mac_tx_clk_stop);
        if (err) {
                phylink_pcs_disable_eee(pl->pcs);
                phylink_err(pl, "%ps() failed: %pe\n",
                            pl->mac_ops->mac_enable_tx_lpi, ERR_PTR(err));

Is it necessary to report the error twice?
diff mbox series

Patch

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 58fc16d15a6f..c2ff358a0ec0 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1785,54 +1785,15 @@  static int lan78xx_set_wol(struct net_device *netdev,
 static int lan78xx_get_eee(struct net_device *net, struct ethtool_keee *edata)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	int ret;
-	u32 buf;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
-
-	ret = phy_ethtool_get_eee(phydev, edata);
-	if (ret < 0)
-		goto exit;
 
-	ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-	if (buf & MAC_CR_EEE_EN_) {
-		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
-		ret = lan78xx_read_reg(dev, EEE_TX_LPI_REQ_DLY, &buf);
-		edata->tx_lpi_timer = buf;
-	} else {
-		edata->tx_lpi_timer = 0;
-	}
-
-	ret = 0;
-exit:
-	usb_autopm_put_interface(dev->intf);
-
-	return ret;
+	return phylink_ethtool_get_eee(dev->phylink, edata);
 }
 
 static int lan78xx_set_eee(struct net_device *net, struct ethtool_keee *edata)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	int ret;
-	u32 buf;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
-
-	ret = phy_ethtool_set_eee(net->phydev, edata);
-	if (ret < 0)
-		goto out;
-
-	buf = (u32)edata->tx_lpi_timer;
-	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
-out:
-	usb_autopm_put_interface(dev->intf);
 
-	return ret;
+	return phylink_ethtool_set_eee(dev->phylink, edata);
 }
 
 static void lan78xx_get_drvinfo(struct net_device *net,
@@ -2470,10 +2431,59 @@  static void lan78xx_mac_link_up(struct phylink_config *config,
 		   ERR_PTR(ret));
 }
 
+static int lan78xx_mac_eee_enable(struct lan78xx_net *dev, bool enable)
+{
+	u32 mac_cr = 0;
+
+	if (enable)
+		mac_cr |= MAC_CR_EEE_EN_;
+
+	/* make sure TXEN and RXEN are disabled before reconfiguring MAC */
+	return lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_, mac_cr);
+}
+
+static void lan78xx_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+
+	lan78xx_mac_eee_enable(dev, false);
+}
+
+static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+				     bool tx_clk_stop)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+	int ret;
+
+	/* Software should only change this field when Energy Efficient
+	 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
+	 * EEEEN during probe, and phylink itself guarantees that
+	 * mac_disable_tx_lpi() will have been previously called.
+	 */
+	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, timer);
+	if (ret < 0)
+		goto tx_lpi_fail;
+
+	ret = lan78xx_mac_eee_enable(dev, true);
+	if (ret < 0)
+		goto tx_lpi_fail;
+
+	return 0;
+
+tx_lpi_fail:
+	netdev_err(dev->net, "Failed to enable TX LPI with error %pe\n",
+		   ERR_PTR(ret));
+	return ret;
+}
+
 static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
 	.mac_config = lan78xx_mac_config,
 	.mac_link_down = lan78xx_mac_link_down,
 	.mac_link_up = lan78xx_mac_link_up,
+	.mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
 };
 
 static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
@@ -2528,6 +2538,26 @@  static int lan78xx_phylink_setup(struct lan78xx_net *dev)
 	pc->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
 			       MAC_100 | MAC_1000FD;
 	pc->mac_managed_pm = true;
+	pc->lpi_capabilities = MAC_100FD | MAC_1000FD;
+	/*
+	 * Default TX LPI (Low Power Idle) request delay count is set to 50us.
+	 *
+	 * Source: LAN7800 Documentation, DS00001992H, Section 15.1.57, Page 204.
+	 *
+	 * Reasoning:
+	 * According to the application note in the LAN7800 documentation, a
+	 * zero delay may negatively impact the TX data path’s ability to
+	 * support Gigabit operation. A value of 50us is recommended as a
+	 * reasonable default when the part operates at Gigabit speeds,
+	 * balancing stability and power efficiency in EEE mode. This delay can
+	 * be increased based on performance testing, as EEE is designed for
+	 * scenarios with mostly idle links and occasional bursts of full
+	 * bandwidth transmission. The goal is to ensure reliable Gigabit
+	 * performance without overly aggressive power optimization during
+	 * inactive periods.
+	 */
+	pc->lpi_timer_default = 50;
+	pc->eee_enabled_default = true;
 
 	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
 		phy_interface_set_rgmii(pc->supported_interfaces);
@@ -2538,6 +2568,10 @@  static int lan78xx_phylink_setup(struct lan78xx_net *dev)
 		link_interface = PHY_INTERFACE_MODE_INTERNAL;
 	}
 
+	memcpy(dev->phylink_config.lpi_interfaces,
+	       dev->phylink_config.supported_interfaces,
+	       sizeof(dev->phylink_config.lpi_interfaces));
+
 	phylink = phylink_create(pc, dev->net->dev.fwnode,
 				 link_interface, &lan78xx_phylink_mac_ops);
 	if (IS_ERR(phylink))
@@ -2607,8 +2641,6 @@  static int lan78xx_phy_init(struct lan78xx_net *dev)
 		return ret;
 	}
 
-	phy_support_eee(phydev);
-
 	if (phydev->mdio.dev.of_node) {
 		u32 reg;
 		int len;
@@ -3131,7 +3163,7 @@  static int lan78xx_reset(struct lan78xx_net *dev)
 	if (ret < 0)
 		return ret;
 
-	buf &= ~(MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_);
+	buf &= ~(MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_ | MAC_CR_EEE_EN_);
 
 	/* LAN7801 only has RGMII mode */
 	if (dev->chipid == ID_REV_CHIP_ID_7801_)