2024-04-06 20:52:24

by Andrew Lunn

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

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 <[email protected]>
---
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 <[email protected]>



2024-04-06 20:52:26

by Andrew Lunn

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

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 <[email protected]>
---
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


2024-04-07 17:51:29

by Oleksij Rempel

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

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 <[email protected]>

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 |

2024-04-08 13:10:41

by patchwork-bot+netdevbpf

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

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:

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