| Message ID | E1tVCRZ-007Y35-9N@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: stmmac: clean up and fix EEE implementation | expand |
On 8/1/2025 12:28 am, Russell King (Oracle) wrote: > 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. > > As stmmac_eee_init() is no longer called outside of stmmac_main.c, make > it static. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Hi Russell, I have completed the sanity test on the EEE changes to the stmmac driver. It seems that most of the changes are acceptable with respect to EEE behavior. However, I noticed that this part of the code requires a minor change to fix the logic: /* Configure phylib's copy of the LPI timer */ if (phylink_ethtool_get_eee(priv->phylink, &eee) == 0) { eee.tx_lpi_timer = priv->tx_lpi_timer; phylink_ethtool_set_eee(priv->phylink, &eee); } Otherwise, the "tx_lpi_timer" will not be set correctly during the initial state. Tested-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
Hi, On Wed, Jan 08, 2025 at 03:36:57PM +0800, Choong Yong Liang wrote: > I have completed the sanity test on the EEE changes to the stmmac driver. > > It seems that most of the changes are acceptable with respect to EEE behavior. > > However, I noticed that this part of the code requires a minor change to fix > the logic: > > /* Configure phylib's copy of the LPI timer */ > if (phylink_ethtool_get_eee(priv->phylink, &eee) == 0) { > eee.tx_lpi_timer = priv->tx_lpi_timer; > phylink_ethtool_set_eee(priv->phylink, &eee); > } > > Otherwise, the "tx_lpi_timer" will not be set correctly during the initial > state. > > Tested-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> Thanks for testing. I can't update the series as there has been a power failure at home, hence the machine that has my git trees on is inaccessible at the moment.
Hi,
On Wed, Jan 08, 2025 at 03:36:57PM +0800, Choong Yong Liang wrote:
> Tested-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
Please let me know if this is for the entire series.
Thanks.
On 8/1/2025 6:40 pm, Russell King (Oracle) wrote: > Hi, > > On Wed, Jan 08, 2025 at 03:36:57PM +0800, Choong Yong Liang wrote: >> Tested-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > Please let me know if this is for the entire series. > > Thanks. > Yes, the test is for the entire series. The only issue that was discovered is that during the initial state, the "tx_lpi_timer" is not set correctly. The "tx_lpi_timer" with a value of 0 causes the EEE to be unable to enter LPI mode.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index b8d631e559c0..df386fc948ec 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -406,7 +406,6 @@ int stmmac_dvr_probe(struct device *device, struct plat_stmmacenet_data *plat_dat, struct stmmac_resources *res); void stmmac_disable_eee_mode(struct stmmac_priv *priv); -bool stmmac_eee_init(struct stmmac_priv *priv); int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt); int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size); int stmmac_bus_clks_config(struct stmmac_priv *priv, bool enabled); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 16b4d8c21c90..0429a99a8114 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 2f518ec845ec..ddb79e8880e9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -469,7 +469,7 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t) * can also manage EEE, this function enable the LPI state and start related * timer. */ -bool stmmac_eee_init(struct stmmac_priv *priv) +static bool stmmac_eee_init(struct stmmac_priv *priv) { int eee_tw_timer = priv->eee_tw_timer; @@ -1088,6 +1088,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, priv->eee_active = phy_init_eee(phy, !(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0; + priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer; priv->eee_enabled = stmmac_eee_init(priv); priv->tx_lpi_enabled = priv->eee_enabled; stmmac_set_eee_pls(priv, priv->hw, true); @@ -1187,6 +1188,16 @@ 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; + + /* Configure phylib's copy of the LPI timer */ + 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 }; @@ -3446,10 +3457,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; @@ -3916,6 +3923,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;