diff mbox series

[net-next,v2,6/8] net: phy: at803x: Make SmartEEE support optional and configurable via ethtool

Message ID 20230327142202.3754446-7-o.rempel@pengutronix.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Make SmartEEE support controllable | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
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: 21 this patch: 21
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang fail Errors and warnings before: 21 this patch: 21
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: 21 this patch: 21
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 187 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Oleksij Rempel March 27, 2023, 2:22 p.m. UTC 
This commit makes SmartEEE support in the AR8035 PHY optional and
configurable through the ethtool eee_set/get interface. Before this
patch, SmartEEE was always enabled except when a device tree option was
preventing it. Since EEE support not only provides advantages in power
management, but can also uncover compatibility issues and other bugs, it
is beneficial to allow users to control this functionality.

By making SmartEEE support optional and configurable via ethtool, the
at803x driver can adapt to different MAC configurations and properly
handle EEE and LPI features. This flexibility empowers users to manage
the trade-offs between power management, compatibility, and overall
performance as needed.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/at803x.c | 126 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 8 deletions(-)

Comments

Oleksij Rempel March 28, 2023, 12:05 p.m. UTC | #1 
On Mon, Mar 27, 2023 at 04:22:00PM +0200, Oleksij Rempel wrote:
> This commit makes SmartEEE support in the AR8035 PHY optional and
> configurable through the ethtool eee_set/get interface. Before this
> patch, SmartEEE was always enabled except when a device tree option was
> preventing it. Since EEE support not only provides advantages in power
> management, but can also uncover compatibility issues and other bugs, it
> is beneficial to allow users to control this functionality.
> 
> By making SmartEEE support optional and configurable via ethtool, the
> at803x driver can adapt to different MAC configurations and properly
> handle EEE and LPI features. This flexibility empowers users to manage
> the trade-offs between power management, compatibility, and overall
> performance as needed.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/at803x.c | 126 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 118 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 653d27a2e62b..4f65b3ebf806 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -165,8 +165,18 @@
>  
>  #define AT803X_MMD3_SMARTEEE_CTL1		0x805b
>  #define AT803X_MMD3_SMARTEEE_CTL2		0x805c
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_LOW	GENMASK(15, 0)
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_15_0	GENMASK(15, 0)
>  #define AT803X_MMD3_SMARTEEE_CTL3		0x805d
>  #define AT803X_MMD3_SMARTEEE_CTL3_LPI_EN	BIT(8)
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH	GENMASK(7, 0)
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_23_16	GENMASK(23, 16)
> +/* Tx LPI timer resolution */
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS	163840
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_MAX_US	\
> +	((GENMASK(23, 0) * AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS) / \
> +	       NSEC_PER_USEC)
> +#define AT803X_MMD3_SMARTEEE_LPI_TIME_DEF_US	335544
>  
>  #define ATH9331_PHY_ID				0x004dd041
>  #define ATH8030_PHY_ID				0x004dd076
> @@ -302,6 +312,8 @@ struct at803x_priv {
>  	u8 smarteee_lpi_tw_100m;
>  	bool is_fiber;
>  	bool is_1000basex;
> +	bool tx_lpi_on;

@Andrew, this variable can be replace by your phydev->tx_lpi_enabled
variable. Should I wait for your patches went mainline?

Regards,
Oleksij
Andrew Lunn March 28, 2023, 12:49 p.m. UTC | #2 
> > @@ -302,6 +312,8 @@ struct at803x_priv {
> >  	u8 smarteee_lpi_tw_100m;
> >  	bool is_fiber;
> >  	bool is_1000basex;
> > +	bool tx_lpi_on;
> 
> @Andrew, this variable can be replace by your phydev->tx_lpi_enabled
> variable. Should I wait for your patches went mainline?

I was wondering about the overlap and the best way to address it.

My patchset is also a bit big, and getting bigger. So it might make
sense to split out some of the bits you need and get them merged
first.

You need the MAC indicating it is EEE capable. I don't store that
information explicitly, but that is a quick simple change. You also
need phydev->tx_lpi_enabled. Anything else?

     Andrew
Oleksij Rempel March 28, 2023, 1 p.m. UTC | #3 
On Tue, Mar 28, 2023 at 02:49:58PM +0200, Andrew Lunn wrote:
> > > @@ -302,6 +312,8 @@ struct at803x_priv {
> > >  	u8 smarteee_lpi_tw_100m;
> > >  	bool is_fiber;
> > >  	bool is_1000basex;
> > > +	bool tx_lpi_on;
> > 
> > @Andrew, this variable can be replace by your phydev->tx_lpi_enabled
> > variable. Should I wait for your patches went mainline?
> 
> I was wondering about the overlap and the best way to address it.
> 
> My patchset is also a bit big, and getting bigger. So it might make
> sense to split out some of the bits you need and get them merged
> first.
> 
> You need the MAC indicating it is EEE capable. I don't store that
> information explicitly, but that is a quick simple change. You also
> need phydev->tx_lpi_enabled. Anything else?

No. It's fine. Thx!

Regards,
Oleksij
diff mbox series

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 653d27a2e62b..4f65b3ebf806 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -165,8 +165,18 @@ 
 
 #define AT803X_MMD3_SMARTEEE_CTL1		0x805b
 #define AT803X_MMD3_SMARTEEE_CTL2		0x805c
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_LOW	GENMASK(15, 0)
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_15_0	GENMASK(15, 0)
 #define AT803X_MMD3_SMARTEEE_CTL3		0x805d
 #define AT803X_MMD3_SMARTEEE_CTL3_LPI_EN	BIT(8)
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH	GENMASK(7, 0)
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_23_16	GENMASK(23, 16)
+/* Tx LPI timer resolution */
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS	163840
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_MAX_US	\
+	((GENMASK(23, 0) * AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS) / \
+	       NSEC_PER_USEC)
+#define AT803X_MMD3_SMARTEEE_LPI_TIME_DEF_US	335544
 
 #define ATH9331_PHY_ID				0x004dd041
 #define ATH8030_PHY_ID				0x004dd076
@@ -302,6 +312,8 @@  struct at803x_priv {
 	u8 smarteee_lpi_tw_100m;
 	bool is_fiber;
 	bool is_1000basex;
+	bool tx_lpi_on;
+	u32 tx_lpi_timer;
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
@@ -858,8 +870,12 @@  static int at803x_probe(struct phy_device *phydev)
 
 	if (phydev->drv->phy_id == ATH8035_PHY_ID ||
 	    phydev->drv->phy_id == ATH8031_PHY_ID) {
-		if (!(priv->flags & AT803X_DISABLE_SMARTEEE))
+		if (!(priv->flags & AT803X_DISABLE_SMARTEEE)) {
 			phydev->is_smart_eee_phy = true;
+			priv->tx_lpi_on = true;
+			priv->tx_lpi_timer =
+				AT803X_MMD3_SMARTEEE_LPI_TIME_DEF_US;
+		}
 	}
 
 	if (priv->vddio) {
@@ -959,10 +975,12 @@  static int at803x_get_features(struct phy_device *phydev)
 static int at803x_smarteee_config(struct phy_device *phydev)
 {
 	struct at803x_priv *priv = phydev->priv;
+	u64 tx_lpi_timer_raw;
+	u64 tx_lpi_timer_ns;
 	u16 mask = 0, val = 0;
 	int ret;
 
-	if (priv->flags & AT803X_DISABLE_SMARTEEE)
+	if (!priv->tx_lpi_on)
 		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
 				      AT803X_MMD3_SMARTEEE_CTL3,
 				      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN, 0);
@@ -983,9 +1001,27 @@  static int at803x_smarteee_config(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	tx_lpi_timer_ns = priv->tx_lpi_timer * NSEC_PER_USEC;
+	tx_lpi_timer_raw =
+		DIV_ROUND_CLOSEST_ULL(tx_lpi_timer_ns,
+				      AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS);
+	val = FIELD_PREP(AT803X_MMD3_SMARTEEE_LPI_TIME_LOW,
+			 FIELD_GET(AT803X_MMD3_SMARTEEE_LPI_TIME_15_0,
+				   tx_lpi_timer_raw));
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_MMD3_SMARTEEE_CTL2,
+			    val);
+	if (ret)
+		return ret;
+
+	val = AT803X_MMD3_SMARTEEE_CTL3_LPI_EN |
+		FIELD_PREP(AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH,
+			   FIELD_GET(AT803X_MMD3_SMARTEEE_LPI_TIME_23_16,
+				     tx_lpi_timer_raw));
+
 	return phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_MMD3_SMARTEEE_CTL3,
-			      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN,
-			      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN);
+			      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN |
+			      AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH, val);
 }
 
 static int at803x_clk_out_config(struct phy_device *phydev)
@@ -1072,10 +1108,6 @@  static int at803x_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	ret = at803x_smarteee_config(phydev);
-	if (ret < 0)
-		return ret;
-
 	ret = at803x_clk_out_config(phydev);
 	if (ret < 0)
 		return ret;
@@ -1359,6 +1391,16 @@  static int at803x_config_aneg(struct phy_device *phydev)
 			return ret;
 	}
 
+	/* only after PHY is attached we will be able to see if MAC supports
+	 * EEE
+	 */
+	if (phydev->mac_supports_eee)
+		priv->tx_lpi_on = false;
+
+	ret = at803x_smarteee_config(phydev);
+	if (ret < 0)
+		return ret;
+
 	return __genphy_config_aneg(phydev, ret);
 }
 
@@ -1617,6 +1659,72 @@  static int at803x_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
+static int at803x_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
+{
+	struct at803x_priv *priv = phydev->priv;
+	u32 tx_timer_raw;
+	u64 tx_timer_ns;
+	int ret;
+
+	/* If SmartEEE is not enabled, it is expected that tx_lpi_* fields
+	 * are processed by the MAC driver.
+	 */
+	if (priv->flags & AT803X_DISABLE_SMARTEEE)
+		return genphy_c45_ethtool_get_eee(phydev, data);
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+			   AT803X_MMD3_SMARTEEE_CTL2);
+	tx_timer_raw = FIELD_PREP(AT803X_MMD3_SMARTEEE_LPI_TIME_15_0,
+				  FIELD_GET(AT803X_MMD3_SMARTEEE_LPI_TIME_LOW,
+					    ret));
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
+			   AT803X_MMD3_SMARTEEE_CTL3);
+	if (ret < 0)
+		return ret;
+
+	tx_timer_raw |= FIELD_PREP(AT803X_MMD3_SMARTEEE_LPI_TIME_23_16,
+				   FIELD_GET(AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH,
+					     ret));
+	tx_timer_ns = tx_timer_raw * AT803X_MMD3_SMARTEEE_LPI_TIME_RESOL_NS;
+	data->tx_lpi_timer = DIV_ROUND_CLOSEST_ULL(tx_timer_ns, NSEC_PER_USEC);
+
+	data->tx_lpi_enabled = !!(ret & AT803X_MMD3_SMARTEEE_CTL3_LPI_EN);
+
+	return genphy_c45_ethtool_get_eee(phydev, data);
+}
+
+static int at803x_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
+{
+	struct at803x_priv *priv = phydev->priv;
+	int ret;
+
+	/* If SmartEEE is not enabled, it is expected that tx_lpi_* fields
+	 * are processed by the MAC driver.
+	 */
+	if (phydev->mac_supports_eee || !phydev->is_smart_eee_phy)
+		return genphy_c45_ethtool_set_eee(phydev, data);
+
+	if (data->tx_lpi_timer > AT803X_MMD3_SMARTEEE_LPI_TIME_MAX_US) {
+		phydev_err(phydev, "Max LPI timer is %lu microsecs\n",
+			   AT803X_MMD3_SMARTEEE_LPI_TIME_MAX_US);
+		return -EINVAL;
+	}
+
+	/* Changing Tx LPI on/off or Tx LPI timer settings
+	 * do not require link reset.
+	 */
+	priv->tx_lpi_timer = data->tx_lpi_timer;
+	priv->tx_lpi_on = data->tx_lpi_enabled;
+	ret = at803x_smarteee_config(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_c45_ethtool_set_eee(phydev, data);
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -2043,6 +2151,8 @@  static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.get_eee		= at803x_get_eee,
+	.set_eee		= at803x_set_eee,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,