[PATCH net-next 0/7] net: stmmac: clean up and fix EEE implementation

[PATCH net-next 0/7] net: stmmac: clean up and fix EEE implementation

[Russell King (Oracle)]

Hi,

This is a rework of stmmac's EEE support in light of the addition of EEE
management to phylib.

Patch 1 adds configuration of the receive clock phy_eee_rx_clock_stop()
(which was part of another series.)

Patch 2 moves the tracking of tx_lpi_timer to phylib.

Patch 3 makes stmmac EEE state depend on phylib's enable_tx_lpi flag.

Patch 4 removes redundant code from the ethtool EEE operations.

Patch 5 removes the driver private tx_lpi_enabled, which will now be
managed by phylib.

Patch 6 removes the dependence of EEE error statistics on the EEE
enable state, instead depending on whether EEE is supported by the
hardware.

Patch 7 removes phy_init_eee(), instead using phy_eee_rx_clock_stop()
to configure whether the PHY may stop the receive clock.

 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  1 -
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   | 25 ++------------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 26 +++++++++++++--------
 drivers/net/phy/phy.c                              | 27 ++++++++++++++++++----
 include/linux/phy.h                                |  1 +
 5 files changed, 42 insertions(+), 38 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/7] net: phy: add configuration of rx clock stop mode

[Russell King (Oracle)]

Add a function to allow configuration of the PCS's clock stop enable
bit, used to configure whether the xMII receive clock can be stopped
during LPI mode.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 27 ++++++++++++++++++++++-----
 include/linux/phy.h   |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e4b04cdaa995..a4b9fcc2503a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1640,6 +1640,27 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
+/**
+ * phy_eee_rx_clock_stop() - configure PHY receive clock in LPI
+ * @phydev: target phy_device struct
+ * @clk_stop_enable: flag to indicate whether the clock can be stopped
+ *
+ * Configure whether the PHY can disable its receive clock during LPI mode,
+ * See IEEE 802.3 sections 22.2.2.2, 35.2.2.10, and 45.2.3.1.4.
+ *
+ * Returns: 0 or negative error.
+ */
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable)
+{
+	/* Configure the PHY to stop receiving xMII
+	 * clock while it is signaling LPI.
+	 */
+	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+			      MDIO_PCS_CTRL1_CLKSTOP_EN,
+			      clk_stop_enable ? MDIO_PCS_CTRL1_CLKSTOP_EN : 0);
+}
+EXPORT_SYMBOL_GPL(phy_eee_rx_clock_stop);
+
 /**
  * phy_init_eee - init and check the EEE feature
  * @phydev: target phy_device struct
@@ -1664,11 +1685,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 		return -EPROTONOSUPPORT;
 
 	if (clk_stop_enable)
-		/* Configure the PHY to stop receiving xMII
-		 * clock while it is signaling LPI.
-		 */
-		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
-				       MDIO_PCS_CTRL1_CLKSTOP_EN);
+		ret = phy_eee_rx_clock_stop(phydev, true);
 
 	return ret < 0 ? ret : 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e597a32cc787..ba9d16fc69b8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2095,6 +2095,7 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask);
 int phy_unregister_fixup_for_id(const char *bus_id);
 int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
 
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
-- 
2.30.2

[PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

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(-)

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;
-- 
2.30.2

[PATCH net-next 3/7] net: stmmac: make EEE depend on phy->enable_tx_lpi

[Russell King (Oracle)]

Make stmmac EEE depend on phylib's evaluation of user settings and PHY
negotiation, as indicated by phy->enable_tx_lpi. This will ensure when
phylib has evaluated that the user has disabled LPI, phy_init_eee()
will not be called, and priv->eee_active will be false, causing LPI/EEE
to be disabled.

This is an interim measure - phy_init_eee() will be removed in a later
patch.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d202bee73b8f..75f540d7ec7d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1088,7 +1088,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active =
+		priv->eee_active = phy->enable_tx_lpi &&
 			phy_init_eee(phy, !(priv->plat->flags &
 				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
 		priv->eee_enabled = stmmac_eee_init(priv);
-- 
2.30.2

[PATCH net-next 4/7] net: stmmac: remove redundant code from ethtool EEE ops

[Russell King (Oracle)]

Setting edata->tx_lpi_enabled in stmmac_ethtool_op_get_eee() gets
overwritten by phylib, so there's no point setting this.

In stmmac_ethtool_op_set_eee(), now that stmmac is using the result of
phylib's evaluation of EEE, there is no need to handle anything in the
ethtool EEE ops other than calling through to the appropriate phylink
function, which will pass on to phylib the users request.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 5ce095a62feb..3a10788bb210 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -898,8 +898,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
-
 	return phylink_ethtool_get_eee(priv->phylink, edata);
 }
 
@@ -911,13 +909,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	if (priv->tx_lpi_enabled != edata->tx_lpi_enabled)
-		netdev_warn(priv->dev,
-			    "Setting EEE tx-lpi is not supported\n");
-
-	if (!edata->eee_enabled)
-		stmmac_disable_eee_mode(priv);
-
 	return phylink_ethtool_set_eee(priv->phylink, edata);
 }
 
-- 
2.30.2

[PATCH net-next 5/7] net: stmmac: remove priv->tx_lpi_enabled

[Russell King (Oracle)]

Through using phylib's EEE state, priv->tx_lpi_enabled has become a
write-only variable. Remove it.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 1d86439b8a14..77a7a098ebd1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -310,7 +310,6 @@ struct stmmac_priv {
 	int eee_enabled;
 	int eee_active;
 	int tx_lpi_timer;
-	int tx_lpi_enabled;
 	int eee_tw_timer;
 	bool eee_sw_timer_en;
 	unsigned int mode;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 75f540d7ec7d..3acc6f6e2190 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -978,7 +978,6 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, false);
 	priv->eee_active = false;
-	priv->tx_lpi_enabled = false;
 	priv->eee_enabled = stmmac_eee_init(priv);
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
@@ -1093,7 +1092,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 				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);
 	}
 
-- 
2.30.2

[PATCH net-next 6/7] net: stmmac: report EEE error statistics if EEE is supported

[Russell King (Oracle)]

Report the number of EEE error statistics in the xstats even when EEE
is not enabled in hardware, but is supported. The PHY maintains this
counter even when EEE is not enabled.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 3a10788bb210..a33d7db810f0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -654,7 +654,7 @@ static void stmmac_get_ethtool_stats(struct net_device *dev,
 					     (*(u32 *)p);
 			}
 		}
-		if (priv->eee_enabled) {
+		if (priv->dma_cap.eee) {
 			int val = phylink_get_eee_err(priv->phylink);
 			if (val)
 				priv->xstats.phy_eee_wakeup_error_n = val;
-- 
2.30.2

[PATCH net-next 7/7] net: stmmac: convert to use phy_eee_rx_clock_stop()

[Russell King (Oracle)]

Convert stmmac to use phy_eee_rx_clock_stop() to set the PHY receive
clock stop in LPI setting, rather than calling the legacy
phy_init_eee() function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3acc6f6e2190..d5516ef0f098 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1087,9 +1087,9 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active = phy->enable_tx_lpi &&
-			phy_init_eee(phy, !(priv->plat->flags &
-				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
+		phy_eee_rx_clock_stop(phy, !(priv->plat->flags &
+					     STMMAC_FLAG_RX_CLK_RUNS_IN_LPI));
+		priv->eee_active = phy->enable_tx_lpi;
 		priv->eee_enabled = stmmac_eee_init(priv);
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
 		stmmac_set_eee_pls(priv, priv->hw, true);
-- 
2.30.2

Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

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?)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!