| Message ID | E1tLkSX-006qfS-Rx@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: clean up and fix EEE implementation | expand |
On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote: > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, > phy_init_eee(phy, !(priv->plat->flags & > STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; > priv->eee_enabled = stmmac_eee_init(priv); > + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; > priv->tx_lpi_enabled = priv->eee_enabled; > stmmac_set_eee_pls(priv, priv->hw, true); > } While looking deeper at stmmac, there's a bug in the above hunk - stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member needs to be set before calling this function. I'll post a v2 shortly.
On Fri, Dec 13, 2024 at 10:59:54AM +0000, Russell King (Oracle) wrote: > On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote: > > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, > > phy_init_eee(phy, !(priv->plat->flags & > > STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; > > priv->eee_enabled = stmmac_eee_init(priv); > > + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; > > priv->tx_lpi_enabled = priv->eee_enabled; > > stmmac_set_eee_pls(priv, priv->hw, true); > > } > > While looking deeper at stmmac, there's a bug in the above hunk - > stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member > needs to be set before calling this function. I'll post a v2 shortly. I'm going to hold off v2, there's a lot more that can be cleaned up here - the EEE code is rather horrid in stmmac, and there's definitely one race, and one logical error in it (e.g. why mark software EEE mode *enabled* when EEE mode is being disabled - which can lead to the EEE timer being added back onto the timer list.) There's also weirdness with dwmac4's EEE register fiddling. The stmmac driver uses hardware timed LPI entry if the timer is small enough to be programmed into hardware, otherwise it uses software mode. When software mode wants to enter LPI mode, it sets both: GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) When software mode wants to exit LPI mode, it clears both of these two bits. In hardware mode, when enabling LPI generation, we set the hardware LPI entry timer (separate register) to a non-zero value, and then set: GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) That seems logical. However, in hardware mode, when we want to then disable hardware LPI generation, we set the hardware LPI entry timer to zero, the following bits: GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) and clear: GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) So, hardware mode, disabled, ends up setting the same bits as software mode wanting to generate LPI state on the transmit side. This makes no sense to me, and looks like another bug in this driver. Can anyone suggest any hardware that I could source which uses the dwmac4 code and which supports EEE please, so that I have hardware to run some tests on. Thanks.
On Fri, Dec 13, 2024 at 08:06:22PM +0000, Russell King (Oracle) wrote: > On Fri, Dec 13, 2024 at 10:59:54AM +0000, Russell King (Oracle) wrote: > > On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote: > > > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, > > > phy_init_eee(phy, !(priv->plat->flags & > > > STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; > > > priv->eee_enabled = stmmac_eee_init(priv); > > > + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; > > > priv->tx_lpi_enabled = priv->eee_enabled; > > > stmmac_set_eee_pls(priv, priv->hw, true); > > > } > > > > While looking deeper at stmmac, there's a bug in the above hunk - > > stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member > > needs to be set before calling this function. I'll post a v2 shortly. > > I'm going to hold off v2, there's a lot more that can be cleaned up > here - the EEE code is rather horrid in stmmac, and there's definitely > one race, and one logical error in it (e.g. why mark software EEE mode > *enabled* when EEE mode is being disabled - which can lead to the EEE > timer being added back onto the timer list.) > > There's also weirdness with dwmac4's EEE register fiddling. > > The stmmac driver uses hardware timed LPI entry if the timer is small > enough to be programmed into hardware, otherwise it uses software mode. > > When software mode wants to enter LPI mode, it sets both: > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > > When software mode wants to exit LPI mode, it clears both of these > two bits. > > In hardware mode, when enabling LPI generation, we set the hardware LPI > entry timer (separate register) to a non-zero value, and then set: > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) > > That seems logical. However, in hardware mode, when we want to then > disable hardware LPI generation, we set the hardware LPI entry timer to > zero, the following bits: > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > > and clear: > > GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) > > So, hardware mode, disabled, ends up setting the same bits as > software mode wanting to generate LPI state on the transmit side. > This makes no sense to me, and looks like another bug in this driver. I've found a document that describes the GMAC: https://www.st.com/resource/en/reference_manual/rm0441-stm32mp151-advanced-armbased-32bit-mpus-stmicroelectronics.pdf Page 3302 gives the definitions for the ETH_MACLCSR, which is this register. LPITE (bit 20) - allows the ETH_MACLETR register to define how long to wait before the TX path enters LPI. Requires LPITXA and LPIEN to both be set. LPIEN is *not* automatically cleared when the TX path exits LPI. LPITXA (bit 19) - if this and LPIEN are set, then LPI is entered once all outstanding packets have been transmitted, and exits when there's a packet to be transmitted or the Tx FIFO is flushed. When it exits, it clears the LPIEN bit (making this a one-shot LPI enter-exit.) PLS (bit 17) needs to be programmed to reflect the PHY link status, and is used to hold off LPI state for the LS timer. LPIEN (bit 16) instructs the MAC to enter (when set) or exit (when cleared) LPI state. It doesn't say what the behaviour of this bit is if LPITXA is clear. So: LPIEN LPITXA LPITE Effect 0 x x No LPI, or LPI exited if active 1 0 0 Undocumented 1 1 0 Tx LPI entered if PLS has been set for the LS timer, and once all packets have been sent. LPIEN clears when Tx LPI exits. 1 1 1 Tx LPI entered if PLS has been set for the LS timer, and transmitter has been idle for ETH_MACLETR. Exits do not clear LPIEN, allowing for subsequent idle periods to enter LPI. Therefore, given this description, I believe the code to be wrong. In the case where we've set LPIEN=1 LPITXA=1 LPITE=1, and we want to exit/disable LPI, we should not be clearing LPIATE and leaving LPIEN and LPITXA as they were. According to my reading, this would cause LPI to remain active until there is a packet to be sent or the TX FIFO is flushed. At that point, Tx LPI will be exited, causing LPIEN to be cleared - but the code is wanting to disable Tx LPI (e.g. because the user configured through ethtool for LPI to be disabled.) The question is... does this get fixed? Is there anyone who can test this (beyond the "patch doesn't seem to cause regressions" but can actually confirm entry/exit from LPI state?)
On Thu, Jan 02, 2025 at 10:02:35PM +0000, Russell King (Oracle) wrote: > On Fri, Dec 13, 2024 at 08:06:22PM +0000, Russell King (Oracle) wrote: > > On Fri, Dec 13, 2024 at 10:59:54AM +0000, Russell King (Oracle) wrote: > > > On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote: > > > > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, > > > > phy_init_eee(phy, !(priv->plat->flags & > > > > STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; > > > > priv->eee_enabled = stmmac_eee_init(priv); > > > > + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; > > > > priv->tx_lpi_enabled = priv->eee_enabled; > > > > stmmac_set_eee_pls(priv, priv->hw, true); > > > > } > > > > > > While looking deeper at stmmac, there's a bug in the above hunk - > > > stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member > > > needs to be set before calling this function. I'll post a v2 shortly. > > > > I'm going to hold off v2, there's a lot more that can be cleaned up > > here - the EEE code is rather horrid in stmmac, and there's definitely > > one race, and one logical error in it (e.g. why mark software EEE mode > > *enabled* when EEE mode is being disabled - which can lead to the EEE > > timer being added back onto the timer list.) > > > > There's also weirdness with dwmac4's EEE register fiddling. > > > > The stmmac driver uses hardware timed LPI entry if the timer is small > > enough to be programmed into hardware, otherwise it uses software mode. > > > > When software mode wants to enter LPI mode, it sets both: > > > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > > > > When software mode wants to exit LPI mode, it clears both of these > > two bits. > > > > In hardware mode, when enabling LPI generation, we set the hardware LPI > > entry timer (separate register) to a non-zero value, and then set: > > > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > > GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) > > > > That seems logical. However, in hardware mode, when we want to then > > disable hardware LPI generation, we set the hardware LPI entry timer to > > zero, the following bits: > > > > GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable) > > GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate) > > > > and clear: > > > > GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable) > > > > So, hardware mode, disabled, ends up setting the same bits as > > software mode wanting to generate LPI state on the transmit side. > > This makes no sense to me, and looks like another bug in this driver. > > I've found a document that describes the GMAC: > > https://www.st.com/resource/en/reference_manual/rm0441-stm32mp151-advanced-armbased-32bit-mpus-stmicroelectronics.pdf > > Page 3302 gives the definitions for the ETH_MACLCSR, which is this > register. > > LPITE (bit 20) - allows the ETH_MACLETR register to define how long to > wait before the TX path enters LPI. Requires LPITXA and LPIEN to both > be set. LPIEN is *not* automatically cleared when the TX path exits > LPI. > > LPITXA (bit 19) - if this and LPIEN are set, then LPI is entered once > all outstanding packets have been transmitted, and exits when there's > a packet to be transmitted or the Tx FIFO is flushed. When it exits, > it clears the LPIEN bit (making this a one-shot LPI enter-exit.) > > PLS (bit 17) needs to be programmed to reflect the PHY link status, > and is used to hold off LPI state for the LS timer. > > LPIEN (bit 16) instructs the MAC to enter (when set) or exit (when > cleared) LPI state. It doesn't say what the behaviour of this bit is > if LPITXA is clear. > > So: > > LPIEN LPITXA LPITE Effect > 0 x x No LPI, or LPI exited if active > 1 0 0 Undocumented > 1 1 0 Tx LPI entered if PLS has been set for the LS > timer, and once all packets have been sent. > LPIEN clears when Tx LPI exits. > 1 1 1 Tx LPI entered if PLS has been set for the LS > timer, and transmitter has been idle for > ETH_MACLETR. Exits do not clear LPIEN, allowing > for subsequent idle periods to enter LPI. > > Therefore, given this description, I believe the code to be wrong. > In the case where we've set LPIEN=1 LPITXA=1 LPITE=1, and we want > to exit/disable LPI, we should not be clearing LPIATE and leaving > LPIEN and LPITXA as they were. According to my reading, this would > cause LPI to remain active until there is a packet to be sent or the > TX FIFO is flushed. At that point, Tx LPI will be exited, causing > LPIEN to be cleared - but the code is wanting to disable Tx LPI > (e.g. because the user configured through ethtool for LPI to be > disabled.) > > The question is... does this get fixed? Is there anyone who can test > this (beyond the "patch doesn't seem to cause regressions" but can > actually confirm entry/exit from LPI state?) Okay, given the lack of engagement on this patch set, I'm going to submit it in its latest form anyway (with a few more patches of the same ilk) so we can at least move forward cleaning up the code.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 1d77389ce953..5ce095a62feb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -898,7 +898,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev, if (!priv->dma_cap.eee) return -EOPNOTSUPP; - edata->tx_lpi_timer = priv->tx_lpi_timer; edata->tx_lpi_enabled = priv->tx_lpi_enabled; return phylink_ethtool_get_eee(priv->phylink, edata); @@ -908,7 +907,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev, struct ethtool_keee *edata) { struct stmmac_priv *priv = netdev_priv(dev); - int ret; if (!priv->dma_cap.eee) return -EOPNOTSUPP; @@ -920,17 +918,7 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev, if (!edata->eee_enabled) stmmac_disable_eee_mode(priv); - ret = phylink_ethtool_set_eee(priv->phylink, edata); - if (ret) - return ret; - - if (edata->eee_enabled && - priv->tx_lpi_timer != edata->tx_lpi_timer) { - priv->tx_lpi_timer = edata->tx_lpi_timer; - stmmac_eee_init(priv); - } - - return 0; + return phylink_ethtool_set_eee(priv->phylink, edata); } static u32 stmmac_usec2riwt(u32 usec, struct stmmac_priv *priv) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index d45fd7a3acd5..d202bee73b8f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, phy_init_eee(phy, !(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; priv->eee_enabled = stmmac_eee_init(priv); + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; priv->tx_lpi_enabled = priv->eee_enabled; stmmac_set_eee_pls(priv, priv->hw, true); } @@ -1190,6 +1191,15 @@ static int stmmac_init_phy(struct net_device *dev) ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0); } + if (ret == 0) { + struct ethtool_keee eee; + + if (phylink_ethtool_get_eee(priv->phylink, &eee)) { + eee.tx_lpi_timer = priv->tx_lpi_timer; + phylink_ethtool_set_eee(priv->phylink, &eee); + } + } + if (!priv->plat->pmt) { struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; @@ -3442,10 +3452,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) priv->eee_tw_timer = STMMAC_DEFAULT_TWT_LS; - /* Convert the timer from msec to usec */ - if (!priv->tx_lpi_timer) - priv->tx_lpi_timer = eee_timer * 1000; - if (priv->use_riwt) { u32 queue; @@ -3912,6 +3918,10 @@ static int __stmmac_open(struct net_device *dev, u32 chan; int ret; + /* Initialise the tx lpi timer, converting from msec to usec */ + if (!priv->tx_lpi_timer) + priv->tx_lpi_timer = eee_timer * 1000; + ret = pm_runtime_resume_and_get(priv->device); if (ret < 0) return ret;
When stmmac_ethtool_op_get_eee() is called, stmmac sets the tx_lpi_timer and tx_lpi_enabled members, and then calls into phylink and thus phylib. phylib overwrites these members. phylib will also cause a link down/link up transition when settings that impact the MAC have been changed. Convert stmmac to use the tx_lpi_timer setting in struct phy_device, updating priv->tx_lpi_timer each time when the link comes up, rather than trying to maintain this user setting itself. We initialise the phylib tx_lpi_timer setting by doing a get_ee-modify-set_eee sequence with the last known priv->tx_lpi_timer value. In order for this to work correctly, we also need this member to be initialised earlier. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 14 +------------- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 17 deletions(-)