[PATCH net-next 0/2] net: Clean up some EEE code

[PATCH net-next 0/2] net: Clean up some EEE code

[Andrew Lunn]

Previous patches have reworked the API between phylib and MAC drivers
with respect to EEE, pushing most of the work into phylib. These two
patches rework two drivers to make use of the new API, and fix their
EEE implementation, so that EEE is configured in the MAC based on what
is actually negotiated during autoneg.

Compile tested only.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
Andrew Lunn (2):
      net: usb: lan78xx: Fixup EEE
      net: lan743x: Fixup EEE

 drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 ------------
 drivers/net/ethernet/microchip/lan743x_main.c    |  7 ++++
 drivers/net/usb/lan78xx.c                        | 42 +++++++++++++-----------
 3 files changed, 29 insertions(+), 41 deletions(-)
---
base-commit: 267e31750ae89f845cfe7df8f577b19482d9ef9b
change-id: 20240406-lan78xx-eee-6e40f57eaf04

Best regards,
-- 
Andrew Lunn <andrew@lunn.ch>

[PATCH net-next 1/2] net: usb: lan78xx: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
lan783xx_phy_link_status_change() which gets called by phylib when
there is a change in link status.

lan78xx_set_eee() now just programs the hardware with the LPI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

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

Call phy_support_eee() to indicate the MAC does actually support EEE.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/lan78xx.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 80ee4fcdfb36..0030be502daa 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1692,15 +1692,10 @@ static int lan78xx_get_eee(struct net_device *net, struct ethtool_keee *edata)
 
 	ret = lan78xx_read_reg(dev, MAC_CR, &buf);
 	if (buf & MAC_CR_EEE_EN_) {
-		edata->eee_enabled = true;
-		edata->tx_lpi_enabled = true;
 		/* 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->eee_enabled = false;
-		edata->eee_active = false;
-		edata->tx_lpi_enabled = false;
 		edata->tx_lpi_timer = 0;
 	}
 
@@ -1721,24 +1716,16 @@ static int lan78xx_set_eee(struct net_device *net, struct ethtool_keee *edata)
 	if (ret < 0)
 		return ret;
 
-	if (edata->eee_enabled) {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf |= MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-
-		phy_ethtool_set_eee(net->phydev, edata);
-
-		buf = (u32)edata->tx_lpi_timer;
-		ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
-	} else {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf &= ~MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-	}
+	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 0;
+	return ret;
 }
 
 static u32 lan78xx_get_link(struct net_device *net)
@@ -2114,7 +2101,20 @@ static void lan78xx_remove_mdio(struct lan78xx_net *dev)
 
 static void lan78xx_link_status_change(struct net_device *net)
 {
+	struct lan78xx_net *dev = netdev_priv(net);
 	struct phy_device *phydev = net->phydev;
+	u32 data;
+	int ret;
+
+	ret = lan78xx_read_reg(dev, MAC_CR, &data);
+	if (ret < 0)
+		return;
+
+	if (phydev->enable_tx_lpi)
+		data |=  MAC_CR_EEE_EN_;
+	else
+		data &= ~MAC_CR_EEE_EN_;
+	lan78xx_write_reg(dev, MAC_CR, data);
 
 	phy_print_status(phydev);
 }
@@ -2408,6 +2408,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	mii_adv_to_linkmode_adv_t(fc, mii_adv);
 	linkmode_or(phydev->advertising, fc, phydev->advertising);
 
+	phy_support_eee(phydev);
+
 	if (phydev->mdio.dev.of_node) {
 		u32 reg;
 		int len;

-- 
2.43.0

Re: [PATCH net-next 1/2] net: usb: lan78xx: Fixup EEE

[Oleksij Rempel]

Hi Andrew,

On Sat, Apr 06, 2024 at 03:15:59PM -0500, Andrew Lunn wrote:
.....
> ---
> -	if (edata->eee_enabled) {
> -		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
> -		buf |= MAC_CR_EEE_EN_;
> -		ret = lan78xx_write_reg(dev, MAC_CR, buf);
> -
> -		phy_ethtool_set_eee(net->phydev, edata);
> -
> -		buf = (u32)edata->tx_lpi_timer;
> -		ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
> -	} else {
> -		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
> -		buf &= ~MAC_CR_EEE_EN_;
> -		ret = lan78xx_write_reg(dev, MAC_CR, buf);
> -	}
> +	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);

According to the documentation:
"Host software should only change this field when Energy Efficient
Ethernet Enable (EEEEN) is cleared."

Even more: "A value of zero may adversely affect the ability of the TX
data path to support Gigabit operation. A reasonable value when the part
is operating at Gigabit speeds is 50 us."

Previous code seems to not care about it too. So, it would be not a
regression if something is broken.

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

I'll get some time to work on this driver in some months, in this case
I'll take a closer look on EEE too.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 2/2] net: lan743x: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
lan743x_phy_link_status_change() which gets called by phylib when
there is a change in link status.

lan743x_ethtool_set_eee() now just programs the hardware with the LTI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

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

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 ---------------------
 drivers/net/ethernet/microchip/lan743x_main.c    |  7 +++++++
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 8a6ae171e375..d0f4ff4ee075 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1076,15 +1076,10 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev,
 
 	buf = lan743x_csr_read(adapter, MAC_CR);
 	if (buf & MAC_CR_EEE_EN_) {
-		eee->eee_enabled = true;
-		eee->tx_lpi_enabled = true;
 		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
 		buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
 		eee->tx_lpi_timer = buf;
 	} else {
-		eee->eee_enabled = false;
-		eee->eee_active = false;
-		eee->tx_lpi_enabled = false;
 		eee->tx_lpi_timer = 0;
 	}
 
@@ -1097,7 +1092,6 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	struct lan743x_adapter *adapter;
 	struct phy_device *phydev;
 	u32 buf = 0;
-	int ret = 0;
 
 	if (!netdev)
 		return -EINVAL;
@@ -1114,23 +1108,8 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	}
 
 	if (eee->eee_enabled) {
-		ret = phy_init_eee(phydev, false);
-		if (ret) {
-			netif_err(adapter, drv, adapter->netdev,
-				  "EEE initialization failed\n");
-			return ret;
-		}
-
 		buf = (u32)eee->tx_lpi_timer;
 		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
-
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf |= MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
-	} else {
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf &= ~MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
 	}
 
 	return phy_ethtool_set_eee(phydev, eee);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 75a988c0bd79..d37a49cd5c69 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1462,6 +1462,13 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		    phydev->interface == PHY_INTERFACE_MODE_1000BASEX ||
 		    phydev->interface == PHY_INTERFACE_MODE_2500BASEX)
 			lan743x_sgmii_config(adapter);
+
+		data = lan743x_csr_read(adapter, MAC_CR);
+		if (phydev->enable_tx_lpi)
+			data |=  MAC_CR_EEE_EN_;
+		else
+			data &= ~MAC_CR_EEE_EN_;
+		lan743x_csr_write(adapter, MAC_CR, data);
 	}
 }
 

-- 
2.43.0

Re: [PATCH net-next 2/2] net: lan743x: Fixup EEE

[Oleksij Rempel]

On Sat, Apr 06, 2024 at 03:16:00PM -0500, Andrew Lunn wrote:
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So move the enable/disable into
> lan743x_phy_link_status_change() which gets called by phylib when
> there is a change in link status.
> 
> lan743x_ethtool_set_eee() now just programs the hardware with the LTI
> timer value, and passed everything else to phylib, so it can correctly
> setup the PHY.
> 
> lan743x_ethtool_get_eee() relies on phylib doing most of the work, the
> MAC driver just adds the LTI timer value.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Probably same as lan78xx - if something is wrong, it was before this
patch.

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 0/2] net: Clean up some EEE code

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 06 Apr 2024 15:15:58 -0500 you wrote:
> Previous patches have reworked the API between phylib and MAC drivers
> with respect to EEE, pushing most of the work into phylib. These two
> patches rework two drivers to make use of the new API, and fix their
> EEE implementation, so that EEE is configured in the MAC based on what
> is actually negotiated during autoneg.
> 
> Compile tested only.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: usb: lan78xx: Fixup EEE
    https://git.kernel.org/netdev/net-next/c/a00bbd15a5af
  - [net-next,2/2] net: lan743x: Fixup EEE
    https://git.kernel.org/netdev/net-next/c/ef460a8986fa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html