| Message ID | 20250108121341.2689130-8-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Convert LAN78xx to PHYLINK | expand |
On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote: > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver > to integrate with phylink. This includes the following changes: > > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage > EEE settings, aligning with the phylink API. > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle) > request delay. Default it to 50 microseconds based on LAN7800 documentation > recommendations. phylib maintains tx_lpi_timer for you. Please use that instead. In any case, I've been submitting phylink EEE support which will help driver authors get this correct, but I think it needs more feedback. Please can you look at my patch set previously posted which is now a bit out of date, review, and think about how this driver can make use of it. In particular, I'd like ideas on what phylink should be doing with tx_lpi_timer values that are out of range of the hardware. Should it limit the value itself? Should set_eee() error out? I asked these questions in the cover message but I don't think *anyone* reads cover messages anymore - as evidenced by my recent patch series that made reference to "make it sew" and Singer sewing machines. No one noticed. So I think patch series cover messages are now useless on netdev. Thanks.
On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote: > On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote: > > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver > > to integrate with phylink. This includes the following changes: > > > > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage > > EEE settings, aligning with the phylink API. > > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle) > > request delay. Default it to 50 microseconds based on LAN7800 documentation > > recommendations. > > phylib maintains tx_lpi_timer for you. Please use that instead. Just using this variable directly phydev->eee_cfg.tx_lpi_timer ? > In any case, I've been submitting phylink EEE support which will help > driver authors get this correct, but I think it needs more feedback. > Please can you look at my patch set previously posted which is now > a bit out of date, review, and think about how this driver can make > use of it. Ack, will do. It looks like your port of lan743x to the new API looks exactly like what I need for this driver too. > In particular, I'd like ideas on what phylink should be doing with > tx_lpi_timer values that are out of range of the hardware. Should it > limit the value itself? Yes, otherwise every MAC driver will need to do it in the ethtool_set_eee() function. The other question is, should we allow absolute maximum values, or sane maximum? At some point will come the question, why the EEE is even enabled? The same is about minimal value, too low value will cause strong speed degradation. Should we allow set insane minimum, but use sane default value? > Should set_eee() error out? Yes, please. > I asked these questions in the cover message but I don't think *anyone* > reads cover messages anymore - as evidenced by my recent patch series that > made reference to "make it sew" and Singer sewing machines. No one > noticed. So I think patch series cover messages are now useless on > netdev. lol :)
On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote: > On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote: > > On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote: > > > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver > > > to integrate with phylink. This includes the following changes: > > > > > > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage > > > EEE settings, aligning with the phylink API. > > > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle) > > > request delay. Default it to 50 microseconds based on LAN7800 documentation > > > recommendations. > > > > phylib maintains tx_lpi_timer for you. Please use that instead. > > Just using this variable directly phydev->eee_cfg.tx_lpi_timer ? Yes. We're already accessing phydev->enable_tx_lpi directly, and we have no helpers for this. Maybe it's more a question for Andrew, whether he wishes to see phylib helpers for this state rather than directly dereferencing phydev. > > In any case, I've been submitting phylink EEE support which will help > > driver authors get this correct, but I think it needs more feedback. > > Please can you look at my patch set previously posted which is now > > a bit out of date, review, and think about how this driver can make > > use of it. > > Ack, will do. It looks like your port of lan743x to the new API > looks exactly like what I need for this driver too. > > > In particular, I'd like ideas on what phylink should be doing with > > tx_lpi_timer values that are out of range of the hardware. Should it > > limit the value itself? > > Yes, otherwise every MAC driver will need to do it in the > ethtool_set_eee() function. I've had several solutions, and my latest patch set actually has a mixture of them in there (which is why I'm eager to try and find a way forward on this, so I can fix the patch set): 1. the original idea to address this in Marvell platforms was to limit the LPI timer to the maximum representable value in the hardware, which would be 255us. This ignores that the hardware uses a 1us tick rate for the timer at 1G speeds, and 10us for 100M speeds. (So it limits it to 260us, even though the hardware can do 2550us at 100M speed). This limit was applied by clamping the value passed in from userspace without erroring out. 2. another solution was added the mac_validate_tx_lpi() method, and implementations added _in addition_ to the above, with the idea of erroring out for values > 255us on Marvell hardware. 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't possible to allow e.g. falling back to a software timer (see stmmac comments below.) Another reason for erroring out applies to Marvell hardware, where PP2 hardware supports LPI on the GMAC but not the XGMAC - so it only works at speeds at or below 2.5G. However, that can be handled via the lpi_capabilities, so I don't think needs to be a concern. > The other question is, should we allow absolute maximum values, or sane > maximum? At some point will come the question, why the EEE is even > enabled? As referenced above, stmmac uses the hardware timer for LPI timeouts up to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a normal kernel timer which is: - disabled (and EEE mode reset) when we have a packet to transmit, or EEE is disabled - is re-armed when cleaning up from packet transmission (although it looks like we attempt to immediately enter LPI mode, and would only wait for the timer if there are more packets to queue... maybe this is a bug in stmmac's implementation?) or when EEE mode is first enabled with a LPI timer longer than the above value. So, should phylink have the capability to switch to a software LPI timer implementation when the LPI timeout value exceeds what the hardware supports? To put it another way, should the stmmac solution to this be made generic? Note that stmmac has this software timer implementation because not only for the reason I've given above, but also because cores other than GMAC4 that support LPI do not have support for the hardware timer. > The same is about minimal value, too low value will cause strong speed > degradation. Should we allow set insane minimum, but use sane default > value? We currently allow zero, and the behaviour of that depends on the hardware. For example, in the last couple of days, it's been reported that stmmac will never enter LPI with a value of zero. Note that phylib defaults to zero, so imposing a minimum would cause a read-modify-write of the EEE settings without setting the timer to fail. > > Should set_eee() error out? > > Yes, please. If we are to convert stmmac, then we need to consider what it's doing (as per the above) and whether that should be generic - and if it isn't what we want in generic code, then how do we allow drivers to do this if they wish.
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 55488ddba269..b9958a0533de 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -476,6 +476,8 @@ struct lan78xx_net { struct phylink_config phylink_config; struct phy_device *fixed_phy; + + u32 tx_lpi_timer; }; /* use ethtool to change the level for any given device */ @@ -1787,54 +1789,24 @@ 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); + ret = phylink_ethtool_get_eee(dev->phylink, edata); 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); + edata->tx_lpi_timer = dev->tx_lpi_timer; - return ret; + return 0; } 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); + dev->tx_lpi_timer = edata->tx_lpi_timer; - return ret; + return phylink_ethtool_set_eee(dev->phylink, edata); } static void lan78xx_get_drvinfo(struct net_device *net, @@ -2345,6 +2317,13 @@ static void lan78xx_mac_link_down(struct phylink_config *config, if (ret < 0) goto link_down_fail; + /* at least MAC_CR_EEE_EN_ should be disable for proper configuration + * on link_up + */ + ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_, 0); + if (ret < 0) + goto link_down_fail; + /* MAC reset seems to not affect MAC configuration, no idea if it is * really needed, but it was done in previous driver version. So, leave * it here. @@ -2418,6 +2397,7 @@ static void lan78xx_mac_link_up(struct phylink_config *config, { struct net_device *net = to_net_dev(config->dev); struct lan78xx_net *dev = netdev_priv(net); + struct phy_device *phydev = net->phydev; u32 mac_cr = 0; int ret; @@ -2439,6 +2419,18 @@ static void lan78xx_mac_link_up(struct phylink_config *config, if (duplex == DUPLEX_FULL) mac_cr |= MAC_CR_FULL_DUPLEX_; + if (phydev->enable_tx_lpi) { + /* EEE_TX_LPI_REQ_DLY should be written before MAC_CR_EEE_EN_ + * is set + */ + ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, + dev->tx_lpi_timer); + if (ret < 0) + goto link_up_fail; + + mac_cr |= MAC_CR_EEE_EN_; + } + /* make sure TXEN and RXEN are disabled before reconfiguring MAC */ ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_SPEED_MASK_ | MAC_CR_FULL_DUPLEX_ | MAC_CR_EEE_EN_, mac_cr); @@ -4430,6 +4422,25 @@ static int lan78xx_probe(struct usb_interface *intf, dev->msg_enable = netif_msg_init(msg_level, NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK); + /* + * 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. + */ + dev->tx_lpi_timer = 50; + skb_queue_head_init(&dev->rxq); skb_queue_head_init(&dev->txq); skb_queue_head_init(&dev->rxq_done);
Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver to integrate with phylink. This includes the following changes: - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage EEE settings, aligning with the phylink API. - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle) request delay. Default it to 50 microseconds based on LAN7800 documentation recommendations. - Update EEE configuration during link_up and ensure proper disabling during `link_down` to allow reconfiguration. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/usb/lan78xx.c | 81 ++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 35 deletions(-)