| Message ID | E1tLkSX-006qfS-Rx@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| 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.
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(-)