diff mbox series

[net-next,07/10] net: mvneta: convert to phylink EEE implementation

Message ID E1tKefs-006SN1-PG@rmk-PC.armlinux.org.uk (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: add phylink managed EEE support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 21
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 0 this patch: 19
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1 this patch: 21
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 196 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Russell King (Oracle) Dec. 9, 2024, 2:23 p.m. UTC 
Convert mvneta to use phylink's EEE implementation, which means we just
need to implement the two methods for LPI control, and adding the
initial configuration.

Disabling LPI requires clearing a single bit. Enabling LPI needs a full
configuration of several values, as the timer values are dependent on
the MAC operating speed.

As Armada 388 states that EEE is only supported in "SGMII" modes, mark
this in lpi_interfaces. Testing with RGMII on the Clearfog platform
indicates that the receive path fails to detect LPI over RGMII.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
 1 file changed, 79 insertions(+), 48 deletions(-)

Comments

Andrew Lunn Dec. 10, 2024, 3:25 a.m. UTC | #1 
On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> Convert mvneta to use phylink's EEE implementation, which means we just
> need to implement the two methods for LPI control, and adding the
> initial configuration.
> 
> Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> configuration of several values, as the timer values are dependent on
> the MAC operating speed.
> 
> As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> indicates that the receive path fails to detect LPI over RGMII.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Simon Horman Dec. 13, 2024, 10:04 a.m. UTC | #2 
On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> Convert mvneta to use phylink's EEE implementation, which means we just
> need to implement the two methods for LPI control, and adding the
> initial configuration.
> 
> Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> configuration of several values, as the timer values are dependent on
> the MAC operating speed.
> 
> As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> indicates that the receive path fails to detect LPI over RGMII.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
>  1 file changed, 79 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c

...

> +static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> +				     bool tx_clk_stop)
> +{
> +	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
> +	u32 ts, tw, lpi0, lpi1, status;
> +
> +	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
> +	if (status & MVNETA_GMAC_SPEED_1000) {
> +		/* At 1G speeds, the timer resolution are 1us, and
> +		 * 802.3 says tw is 16.5us. Round up to 17us.
> +		 */
> +		tw = 17;
> +		ts = timer;
> +	} else {
> +		/* At 100M speeds, the timer resolutions are 10us, and
> +		 * 802.3 says tw is 30us.
> +		 */
> +		tw = 3;
> +		ts = DIV_ROUND_UP(timer, 10);
>  	}
> +
> +	if (ts > 255)
> +		ts = 255;
> +
> +	/* Configure ts */
> +	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
> +	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);

Hi Russell,

I think that the val and field arguments to u32_replace_bits() are
inverted here and this should be:

	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);

> +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> +
> +	/* Configure tw and enable LPI generation */
> +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);

Ditto.

> +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
>  }
>  
>  static const struct phylink_mac_ops mvneta_phylink_ops = {

Flagged by clang-19 and gcc-14 W=1 builds.

...
Simon Horman Dec. 13, 2024, 10:22 a.m. UTC | #3 
On Fri, Dec 13, 2024 at 10:04:15AM +0000, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 02:23:48PM +0000, Russell King (Oracle) wrote:
> > Convert mvneta to use phylink's EEE implementation, which means we just
> > need to implement the two methods for LPI control, and adding the
> > initial configuration.
> > 
> > Disabling LPI requires clearing a single bit. Enabling LPI needs a full
> > configuration of several values, as the timer values are dependent on
> > the MAC operating speed.
> > 
> > As Armada 388 states that EEE is only supported in "SGMII" modes, mark
> > this in lpi_interfaces. Testing with RGMII on the Clearfog platform
> > indicates that the receive path fails to detect LPI over RGMII.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 127 ++++++++++++++++----------
> >  1 file changed, 79 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> 
> ...
> 
> > +static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> > +				     bool tx_clk_stop)
> > +{
> > +	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
> > +	u32 ts, tw, lpi0, lpi1, status;
> > +
> > +	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
> > +	if (status & MVNETA_GMAC_SPEED_1000) {
> > +		/* At 1G speeds, the timer resolution are 1us, and
> > +		 * 802.3 says tw is 16.5us. Round up to 17us.
> > +		 */
> > +		tw = 17;
> > +		ts = timer;
> > +	} else {
> > +		/* At 100M speeds, the timer resolutions are 10us, and
> > +		 * 802.3 says tw is 30us.
> > +		 */
> > +		tw = 3;
> > +		ts = DIV_ROUND_UP(timer, 10);
> >  	}
> > +
> > +	if (ts > 255)
> > +		ts = 255;
> > +
> > +	/* Configure ts */
> > +	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
> > +	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);
> 
> Hi Russell,
> 
> I think that the val and field arguments to u32_replace_bits() are
> inverted here and this should be:
> 
> 	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);
> 
> > +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> > +
> > +	/* Configure tw and enable LPI generation */
> > +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> > +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
> 
> Ditto.
> 
> > +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> > +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
> >  }
> >  
> >  static const struct phylink_mac_ops mvneta_phylink_ops = {
> 
> Flagged by clang-19 and gcc-14 W=1 builds.
> 
> ...

Sorry for more noise, and perhaps this is obvious.
But a similar problem seems to also exists in the following patch,
[PATCH] net: mvpp2: add EEE implementation.
Russell King (Oracle) Dec. 13, 2024, 10:51 a.m. UTC | #4 
On Fri, Dec 13, 2024 at 10:22:11AM +0000, Simon Horman wrote:
> > Hi Russell,
> > 
> > I think that the val and field arguments to u32_replace_bits() are
> > inverted here and this should be:
> > 
> > 	lpi0 = u32_replace_bits(lpi0, ts, MVNETA_LPI_CTRL_0_TS);
> > 
> > > +	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
> > > +
> > > +	/* Configure tw and enable LPI generation */
> > > +	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
> > > +	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
> > 
> > Ditto.
> > 
> > > +	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
> > > +	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
> > >  }
> > >  
> > >  static const struct phylink_mac_ops mvneta_phylink_ops = {
> > 
> > Flagged by clang-19 and gcc-14 W=1 builds.
> > 
> > ...
> 
> Sorry for more noise, and perhaps this is obvious.
> But a similar problem seems to also exists in the following patch,
> [PATCH] net: mvpp2: add EEE implementation.

Thanks Simon, the 0-day bot flagged them and have already been fixed.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index fe6261b81540..2b0758c11664 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -284,8 +284,12 @@ 
 					  MVNETA_TXQ_BUCKET_REFILL_PERIOD))
 
 #define MVNETA_LPI_CTRL_0                        0x2cc0
+#define      MVNETA_LPI_CTRL_0_TS                (0xff << 8)
 #define MVNETA_LPI_CTRL_1                        0x2cc4
-#define      MVNETA_LPI_REQUEST_ENABLE           BIT(0)
+#define      MVNETA_LPI_CTRL_1_REQUEST_ENABLE    BIT(0)
+#define      MVNETA_LPI_CTRL_1_REQUEST_FORCE     BIT(1)
+#define      MVNETA_LPI_CTRL_1_MANUAL_MODE       BIT(2)
+#define      MVNETA_LPI_CTRL_1_TW                (0xfff << 4)
 #define MVNETA_LPI_CTRL_2                        0x2cc8
 #define MVNETA_LPI_STATUS                        0x2ccc
 
@@ -541,10 +545,6 @@  struct mvneta_port {
 	struct mvneta_bm_pool *pool_short;
 	int bm_win_id;
 
-	bool eee_enabled;
-	bool eee_active;
-	bool tx_lpi_enabled;
-
 	u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)];
 
 	u32 indir[MVNETA_RSS_LU_TABLE_SIZE];
@@ -1354,6 +1354,13 @@  static void mvneta_port_enable(struct mvneta_port *pp)
 	val = mvreg_read(pp, MVNETA_GMAC_CTRL_0);
 	val |= MVNETA_GMAC0_PORT_ENABLE;
 	mvreg_write(pp, MVNETA_GMAC_CTRL_0, val);
+
+	/* Ensure LPI is disabled */
+	val = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	val &= ~(MVNETA_LPI_CTRL_1_REQUEST_ENABLE |
+		 MVNETA_LPI_CTRL_1_REQUEST_FORCE |
+		 MVNETA_LPI_CTRL_1_MANUAL_MODE);
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, val);
 }
 
 /* Disable the port and wait for about 200 usec before retuning */
@@ -4213,18 +4220,6 @@  static int mvneta_mac_finish(struct phylink_config *config, unsigned int mode,
 	return 0;
 }
 
-static void mvneta_set_eee(struct mvneta_port *pp, bool enable)
-{
-	u32 lpi_ctl1;
-
-	lpi_ctl1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
-	if (enable)
-		lpi_ctl1 |= MVNETA_LPI_REQUEST_ENABLE;
-	else
-		lpi_ctl1 &= ~MVNETA_LPI_REQUEST_ENABLE;
-	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi_ctl1);
-}
-
 static void mvneta_mac_link_down(struct phylink_config *config,
 				 unsigned int mode, phy_interface_t interface)
 {
@@ -4240,9 +4235,6 @@  static void mvneta_mac_link_down(struct phylink_config *config,
 		val |= MVNETA_GMAC_FORCE_LINK_DOWN;
 		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
 	}
-
-	pp->eee_active = false;
-	mvneta_set_eee(pp, false);
 }
 
 static void mvneta_mac_link_up(struct phylink_config *config,
@@ -4291,11 +4283,61 @@  static void mvneta_mac_link_up(struct phylink_config *config,
 	}
 
 	mvneta_port_up(pp);
+}
+
+static int mvneta_mac_validate_tx_lpi(struct phylink_config *config,
+				      struct ethtool_keee *eee)
+{
+	if (eee->tx_lpi_timer > 255)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void mvneta_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
+	u32 lpi1;
+
+	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	lpi1 &= ~MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
+}
 
-	if (phy && pp->eee_enabled) {
-		pp->eee_active = phy_init_eee(phy, false) >= 0;
-		mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled);
+static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+				     bool tx_clk_stop)
+{
+	struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev));
+	u32 ts, tw, lpi0, lpi1, status;
+
+	status = mvreg_read(pp, MVNETA_GMAC_STATUS);
+	if (status & MVNETA_GMAC_SPEED_1000) {
+		/* At 1G speeds, the timer resolution are 1us, and
+		 * 802.3 says tw is 16.5us. Round up to 17us.
+		 */
+		tw = 17;
+		ts = timer;
+	} else {
+		/* At 100M speeds, the timer resolutions are 10us, and
+		 * 802.3 says tw is 30us.
+		 */
+		tw = 3;
+		ts = DIV_ROUND_UP(timer, 10);
 	}
+
+	if (ts > 255)
+		ts = 255;
+
+	/* Configure ts */
+	lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
+	lpi0 = u32_replace_bits(lpi0, MVNETA_LPI_CTRL_0_TS, ts);
+	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0);
+
+	/* Configure tw and enable LPI generation */
+	lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1);
+	lpi1 = u32_replace_bits(lpi1, MVNETA_LPI_CTRL_1_TW, tw);
+	lpi1 |= MVNETA_LPI_CTRL_1_REQUEST_ENABLE;
+	mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1);
 }
 
 static const struct phylink_mac_ops mvneta_phylink_ops = {
@@ -4305,6 +4347,9 @@  static const struct phylink_mac_ops mvneta_phylink_ops = {
 	.mac_finish = mvneta_mac_finish,
 	.mac_link_down = mvneta_mac_link_down,
 	.mac_link_up = mvneta_mac_link_up,
+	.mac_validate_tx_lpi = mvneta_mac_validate_tx_lpi,
+	.mac_disable_tx_lpi = mvneta_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = mvneta_mac_enable_tx_lpi,
 };
 
 static int mvneta_mdio_probe(struct mvneta_port *pp)
@@ -5106,14 +5151,6 @@  static int mvneta_ethtool_get_eee(struct net_device *dev,
 				  struct ethtool_keee *eee)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	u32 lpi_ctl0;
-
-	lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
-
-	eee->eee_enabled = pp->eee_enabled;
-	eee->eee_active = pp->eee_active;
-	eee->tx_lpi_enabled = pp->tx_lpi_enabled;
-	eee->tx_lpi_timer = (lpi_ctl0) >> 8; // * scale;
 
 	return phylink_ethtool_get_eee(pp->phylink, eee);
 }
@@ -5122,23 +5159,6 @@  static int mvneta_ethtool_set_eee(struct net_device *dev,
 				  struct ethtool_keee *eee)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
-	u32 lpi_ctl0;
-
-	/* The Armada 37x documents do not give limits for this other than
-	 * it being an 8-bit register.
-	 */
-	if (eee->tx_lpi_enabled && eee->tx_lpi_timer > 255)
-		return -EINVAL;
-
-	lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
-	lpi_ctl0 &= ~(0xff << 8);
-	lpi_ctl0 |= eee->tx_lpi_timer << 8;
-	mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi_ctl0);
-
-	pp->eee_enabled = eee->eee_enabled;
-	pp->tx_lpi_enabled = eee->tx_lpi_enabled;
-
-	mvneta_set_eee(pp, eee->tx_lpi_enabled && eee->eee_enabled);
 
 	return phylink_ethtool_set_eee(pp->phylink, eee);
 }
@@ -5453,6 +5473,9 @@  static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
 	    !phy_interface_mode_is_rgmii(phy_mode))
 		return -EINVAL;
 
+	/* Ensure LPI is disabled */
+	mvneta_mac_disable_tx_lpi(&pp->phylink_config);
+
 	return 0;
 }
 
@@ -5544,6 +5567,14 @@  static int mvneta_probe(struct platform_device *pdev)
 	pp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 |
 		MAC_100 | MAC_1000FD | MAC_2500FD;
 
+	/* Setup EEE. Choose 250us idle. Only supported in SGMII modes. */
+	__set_bit(PHY_INTERFACE_MODE_QSGMII, pp->phylink_config.lpi_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_SGMII, pp->phylink_config.lpi_interfaces);
+	pp->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
+	pp->phylink_config.lpi_timer_limit_us = 255;
+	pp->phylink_config.lpi_timer_default = 250;
+	pp->phylink_config.eee_enabled_default = true;
+
 	phy_interface_set_rgmii(pp->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_QSGMII,
 		  pp->phylink_config.supported_interfaces);