[PATCH net-next v1 0/7] make SmartEEE support controllable

[PATCH net-next v1 0/7] make SmartEEE support controllable

[Oleksij Rempel]

Some PHYs (for example AR8035) provide so called SmartEEE support. Which
allows to use EEE with MACs withotu EEE ability.

Since this functionality is usually enabled by default, it may have
negative impact in some use cases (for example PTP). Or even preventing use of
all link modes without PHY driver assistance (for example full range of
half-duplex modes).

To address at leas some of this issues we need to pass EEE ethtool access to
PHY drivers. Which is done in this patch set.

Oleksij Rempel (7):
  net: phy: add driver specific get/set_eee support
  net: phy: at803x: implement ethtool access to SmartEEE functionality
  net: phy: at803x: ar8035: fix EEE support for half duplex links
  net: phy: add PHY specifica flag to signal SmartEEE support
  net: phy: at803x: add PHY_SMART_EEE flag to AR8035
  net: phy: add phy_has_smarteee() helper
  net: fec: add support for PHYs with SmartEEE support

 drivers/net/ethernet/freescale/fec_main.c |  22 +++-
 drivers/net/phy/at803x.c                  | 142 +++++++++++++++++++++-
 drivers/net/phy/phy.c                     |   6 +
 include/linux/phy.h                       |  15 +++
 4 files changed, 175 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH net-next v1 1/7] net: phy: add driver specific get/set_eee support

[Oleksij Rempel]

Not all PHYs can be handled by generic phy_ethtool_get/set_eee()
functions. So, add driver specific get/set_eee support.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy.c | 6 ++++++
 include/linux/phy.h   | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2f1041a7211e..c42df62df302 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1520,6 +1520,9 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (phydev->drv->get_eee)
+		return phydev->drv->get_eee(phydev, data);
+
 	return genphy_c45_ethtool_get_eee(phydev, data);
 }
 EXPORT_SYMBOL(phy_ethtool_get_eee);
@@ -1536,6 +1539,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (phydev->drv->set_eee)
+		return phydev->drv->set_eee(phydev, data);
+
 	return genphy_c45_ethtool_set_eee(phydev, data);
 }
 EXPORT_SYMBOL(phy_ethtool_set_eee);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 727bff531a14..6709fbd72e10 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1052,6 +1052,11 @@ struct phy_driver {
 	/** @get_plca_status: Return the current PLCA status info */
 	int (*get_plca_status)(struct phy_device *dev,
 			       struct phy_plca_status *plca_st);
+
+	/** @get_eee: Return the current EEE configuration */
+	int (*get_eee)(struct phy_device *phydev, struct ethtool_eee *e);
+	/** @set_eee: Set the EEE configuration */
+	int (*set_eee)(struct phy_device *phydev, struct ethtool_eee *e);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.30.2

[PATCH net-next v1 2/7] net: phy: at803x: implement ethtool access to SmartEEE functionality

[Oleksij Rempel]

If AR8035 PHY is used with a MAC without EEE support
(iMX6, etc), then we need to process ethtool_eee::tx_lpi_timer and
tx_lpi_enabled by the PHY driver. So, add get/set_eee support for this
functionality.

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

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 22f4458274aa..9eb4439b0afc 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -166,8 +166,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
@@ -951,17 +961,26 @@ static int at803x_get_features(struct phy_device *phydev)
 	return 0;
 }
 
-static int at803x_smarteee_config(struct phy_device *phydev)
+static int at803x_smarteee_config(struct phy_device *phydev, bool enable,
+				  u32 tx_lpi_timer_us)
 {
 	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->flags & AT803X_DISABLE_SMARTEEE || !enable)
 		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
 				      AT803X_MMD3_SMARTEEE_CTL3,
 				      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN, 0);
 
+	if (tx_lpi_timer_us > 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;
+	}
+
 	if (priv->smarteee_lpi_tw_1g) {
 		mask |= 0xff00;
 		val |= priv->smarteee_lpi_tw_1g << 8;
@@ -978,9 +997,27 @@ static int at803x_smarteee_config(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	tx_lpi_timer_ns = tx_lpi_timer_us * 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)
@@ -1067,7 +1104,8 @@ static int at803x_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	ret = at803x_smarteee_config(phydev);
+	ret = at803x_smarteee_config(phydev, true,
+				     AT803X_MMD3_SMARTEEE_LPI_TIME_DEF_US);
 	if (ret < 0)
 		return ret;
 
@@ -1612,6 +1650,65 @@ 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 (priv->flags & AT803X_DISABLE_SMARTEEE)
+		return genphy_c45_ethtool_set_eee(phydev, data);
+
+	/* Changing Tx LPI on/off or Tx LPI timer settings
+	 * do not require link reset.
+	 */
+	ret = at803x_smarteee_config(phydev, data->tx_lpi_enabled,
+				     data->tx_lpi_timer);
+	if (ret)
+		return ret;
+
+	return genphy_c45_ethtool_set_eee(phydev, data);
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -2038,6 +2135,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,
-- 
2.30.2

[PATCH net-next v1 3/7] net: phy: at803x: ar8035: fix EEE support for half duplex links

[Oleksij Rempel]

If AR8035 is running with enabled EEE and LPI, it will not be able to
establish an 100BaseTX/Half or 1000BaseT/Half link. Similar issue we
will have with 100BaseTX/Full and LPI TX timer configured to less then
80msec.

To avoid this issue, we need to keep LPI disabled before link is
establish and enable it only we detected supported link configuration.

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

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 9eb4439b0afc..5ab43eb63581 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -313,6 +313,7 @@ struct at803x_priv {
 	u8 smarteee_lpi_tw_100m;
 	bool is_fiber;
 	bool is_1000basex;
+	bool tx_lpi_on;
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
@@ -970,6 +971,8 @@ static int at803x_smarteee_config(struct phy_device *phydev, bool enable,
 	u16 mask = 0, val = 0;
 	int ret;
 
+	priv->tx_lpi_on = enable;
+
 	if (priv->flags & AT803X_DISABLE_SMARTEEE || !enable)
 		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
 				      AT803X_MMD3_SMARTEEE_CTL3,
@@ -1010,10 +1013,15 @@ static int at803x_smarteee_config(struct phy_device *phydev, bool enable,
 	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));
+	val = FIELD_PREP(AT803X_MMD3_SMARTEEE_LPI_TIME_HIGH,
+			 FIELD_GET(AT803X_MMD3_SMARTEEE_LPI_TIME_23_16,
+				   tx_lpi_timer_raw));
+
+	if (phydev->state == PHY_RUNNING &&
+	    phy_check_valid(phydev->speed, phydev->duplex,
+			    phydev->supported_eee)) {
+		val |= AT803X_MMD3_SMARTEEE_CTL3_LPI_EN;
+	}
 
 	return phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_MMD3_SMARTEEE_CTL3,
 			      AT803X_MMD3_SMARTEEE_CTL3_LPI_EN |
@@ -1682,7 +1690,7 @@ static int at803x_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	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);
+	data->tx_lpi_enabled = priv->tx_lpi_on;
 
 	return genphy_c45_ethtool_get_eee(phydev, data);
 }
@@ -1709,6 +1717,28 @@ static int at803x_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	return genphy_c45_ethtool_set_eee(phydev, data);
 }
 
+static void at8035_link_change_notify(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	if (priv->flags & AT803X_DISABLE_SMARTEEE)
+		return;
+
+	if (phydev->state == PHY_RUNNING) {
+		if (priv->tx_lpi_on && phy_check_valid(phydev->speed,
+						       phydev->duplex,
+						       phydev->supported_eee))
+			phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
+					 AT803X_MMD3_SMARTEEE_CTL3,
+					 AT803X_MMD3_SMARTEEE_CTL3_LPI_EN);
+	} else {
+		if (priv->tx_lpi_on)
+			phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
+					   AT803X_MMD3_SMARTEEE_CTL3,
+					   AT803X_MMD3_SMARTEEE_CTL3_LPI_EN);
+	}
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -2137,6 +2167,7 @@ static struct phy_driver at803x_driver[] = {
 	.cable_test_get_status	= at803x_cable_test_get_status,
 	.get_eee		= at803x_get_eee,
 	.set_eee		= at803x_set_eee,
+	.link_change_notify	= at8035_link_change_notify,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,
-- 
2.30.2

[PATCH net-next v1 4/7] net: phy: add PHY specifica flag to signal SmartEEE support

[Oleksij Rempel]

Typical EEE support need cooperation of MAC and PHY, so both parts should
be able to do EEE. But, there also PHYs compatible with normal 802.3az
standard working with legacy MAC without EEE ability, acting as a complete
EEE power saving system.

To identify this PHYs we need a PHY specific flag. Since the PHY
specification implementing this functionality calls it SmartEEE, use
the same flag name - PHY_SMART_EEE.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6709fbd72e10..e6b12653c655 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -85,6 +85,7 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_IS_INTERNAL		0x00000001
 #define PHY_RST_AFTER_CLK_EN	0x00000002
 #define PHY_POLL_CABLE_TEST	0x00000004
+#define PHY_SMART_EEE		0x00000008 /* EEE done by PHY without MAC */
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /**
-- 
2.30.2

[PATCH net-next v1 5/7] net: phy: at803x: add PHY_SMART_EEE flag to AR8035

[Oleksij Rempel]

AR8035 is one of the PHYs with SmartEEE functionality. This flag will be
used by one of next patches on the i.MX FEC driver.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/at803x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 5ab43eb63581..94dbec0a992c 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -2147,7 +2147,7 @@ static struct phy_driver at803x_driver[] = {
 	/* Qualcomm Atheros AR8035 */
 	PHY_ID_MATCH_EXACT(ATH8035_PHY_ID),
 	.name			= "Qualcomm Atheros AR8035",
-	.flags			= PHY_POLL_CABLE_TEST,
+	.flags			= PHY_POLL_CABLE_TEST | PHY_SMART_EEE,
 	.probe			= at803x_probe,
 	.remove			= at803x_remove,
 	.config_aneg		= at803x_config_aneg,
-- 
2.30.2

[PATCH net-next v1 6/7] net: phy: add phy_has_smarteee() helper

[Oleksij Rempel]

Add helper to identify PHYs with SmartEEE support.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index e6b12653c655..2e726450d3c3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1408,6 +1408,15 @@ static inline bool phy_polling_mode(struct phy_device *phydev)
 	return phydev->irq == PHY_POLL;
 }
 
+/**
+ * phy_has_smarteee - Tests whether a PHY supports SmartEEE.
+ * @phydev: the phy_device struct
+ */
+static inline bool phy_has_smarteee(struct phy_device *phydev)
+{
+	return phydev && phydev->drv && !!(phydev->drv->flags & PHY_SMART_EEE);
+}
+
 /**
  * phy_has_hwtstamp - Tests whether a PHY time stamp configuration.
  * @phydev: the phy_device struct
-- 
2.30.2

[PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Oleksij Rempel]

Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
But this chips are used sometimes in combinations with SmartEEE capable
PHYs.
So, instead of aborting get/set_eee access on MACs without EEE support,
ask PHY if it is able to do the EEE job by using SmartEEE.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c73e25f8995e..00f3703db69d 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_eee *p = &fep->eee;
 
-	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
-		return -EOPNOTSUPP;
+	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
+		if (!netif_running(ndev))
+			return -ENETDOWN;
+
+		if (!phy_has_smarteee(ndev->phydev))
+			return -EOPNOTSUPP;
+
+		return phy_ethtool_get_eee(ndev->phydev, edata);
+	}
 
 	if (!netif_running(ndev))
 		return -ENETDOWN;
@@ -3123,8 +3130,15 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
 	struct ethtool_eee *p = &fep->eee;
 	int ret = 0;
 
-	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
-		return -EOPNOTSUPP;
+	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
+		if (!netif_running(ndev))
+			return -ENETDOWN;
+
+		if (!phy_has_smarteee(ndev->phydev))
+			return -EOPNOTSUPP;
+
+		return phy_ethtool_set_eee(ndev->phydev, edata);
+	}
 
 	if (!netif_running(ndev))
 		return -ENETDOWN;
-- 
2.30.2

Re: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Andrew Lunn]

On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote:
> Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
> But this chips are used sometimes in combinations with SmartEEE capable
> PHYs.
> So, instead of aborting get/set_eee access on MACs without EEE support,
> ask PHY if it is able to do the EEE job by using SmartEEE.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index c73e25f8995e..00f3703db69d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct ethtool_eee *p = &fep->eee;
>  
> -	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
> -		return -EOPNOTSUPP;
> +	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
> +		if (!netif_running(ndev))
> +			return -ENETDOWN;
> +
> +		if (!phy_has_smarteee(ndev->phydev))
> +			return -EOPNOTSUPP;
> +
> +		return phy_ethtool_get_eee(ndev->phydev, edata);
> +	}

I can see two different ways we do this. As you have here, we modify
every MAC driver which is paired to a SmartEEE PHY and have it call
into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP,
and there is an ndev->phydev call directly into phylib. That should
make all boards with SmartEEE supported. We do this for a few calls,
TS Info, and SFP module info.

Either way, i don't think we need phy_has_smarteee() exposed outside
of phylib. SmartEEE is supposed to be transparent to the MAC, so it
should not need to care. Same as WOL, the MAC does not care if the PHY
supports WoL, it should just call the APIs to get and set WoL and
assume they do the right thing.

What is also unclear to me is how we negotiate between EEE and
SmartEEE. I assume if the MAC is EEE capable, we prefer that over
SmartEEE. But i don't think i've seen anything in these patches which
addresses this. Maybe we want phy_init_eee() to disable SmartEEE?

	  Andrew

Re: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Larysa Zaremba]

On Tue, Feb 14, 2023 at 02:26:36PM +0100, Andrew Lunn wrote:
> On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote:
> > Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
> > But this chips are used sometimes in combinations with SmartEEE capable
> > PHYs.
> > So, instead of aborting get/set_eee access on MACs without EEE support,
> > ask PHY if it is able to do the EEE job by using SmartEEE.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index c73e25f8995e..00f3703db69d 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> >  	struct ethtool_eee *p = &fep->eee;
> >  
> > -	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
> > -		return -EOPNOTSUPP;
> > +	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
> > +		if (!netif_running(ndev))
> > +			return -ENETDOWN;
> > +
> > +		if (!phy_has_smarteee(ndev->phydev))
> > +			return -EOPNOTSUPP;
> > +
> > +		return phy_ethtool_get_eee(ndev->phydev, edata);
> > +	}
> 
> I can see two different ways we do this. As you have here, we modify
> every MAC driver which is paired to a SmartEEE PHY and have it call
> into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP,
> and there is an ndev->phydev call directly into phylib. That should
> make all boards with SmartEEE supported. We do this for a few calls,
> TS Info, and SFP module info.
> 
> Either way, i don't think we need phy_has_smarteee() exposed outside
> of phylib. SmartEEE is supposed to be transparent to the MAC, so it
> should not need to care. Same as WOL, the MAC does not care if the PHY
> supports WoL, it should just call the APIs to get and set WoL and
> assume they do the right thing.
> 
> What is also unclear to me is how we negotiate between EEE and
> SmartEEE. I assume if the MAC is EEE capable, we prefer that over
> SmartEEE. But i don't think i've seen anything in these patches which
> addresses this. Maybe we want phy_init_eee() to disable SmartEEE?
> 
> 	  Andrew

I agree with the attitude that we shouldn't expect legacy MAC driver to handle 
fancy features of PHY, so modifying core code instead of driver code would be a 
much better solution.

Also, I too would like to see the case when both MAC EEE and SmartEEE are 
enabled, to be prevented or acknowledged, if it's actually useful for something.

Other than that, patchset look fine to me, so in v2, you can put my Reviewed-By 
(Larysa Zaremba <larysa.zaremba@intel.com>) under any of the previous patches 
which stay unchanged (except the 4th one, you have to correct 'specifica'
in the title).

Re: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Oleksij Rempel]

Hi Andrew,

On Tue, Feb 14, 2023 at 02:26:36PM +0100, Andrew Lunn wrote:
> On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote:
> I can see two different ways we do this. As you have here, we modify
> every MAC driver which is paired to a SmartEEE PHY and have it call
> into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP,
> and there is an ndev->phydev call directly into phylib. That should
> make all boards with SmartEEE supported. We do this for a few calls,
> TS Info, and SFP module info.

ACK. I'm working on this.

> Either way, i don't think we need phy_has_smarteee() exposed outside
> of phylib. SmartEEE is supposed to be transparent to the MAC, so it
> should not need to care. Same as WOL, the MAC does not care if the PHY
> supports WoL, it should just call the APIs to get and set WoL and
> assume they do the right thing.
> 
> What is also unclear to me is how we negotiate between EEE and
> SmartEEE. I assume if the MAC is EEE capable, we prefer that over
> SmartEEE. But i don't think i've seen anything in these patches which
> addresses this. Maybe we want phy_init_eee() to disable SmartEEE?
> 
> 	  Andrew
> 

I would prefer to not touch phy_init_eee(). At least not in this patch
set. With this function we have following situation:
drivers/net/dsa/b53/b53_common.c:2173:

This driver will enable EEE if link partners agreed to do so. But never
disable it, if link partner decided to turn off EEE or other link partner
without EEE support was attached.

drivers/net/dsa/mt7530.c:2862:

Seems to be ok.

drivers/net/ethernet/broadcom/genet/bcmgenet.c:1353:

EEE is not enabled link up. It will work only with ethtool and only if
link was already active.

drivers/net/ethernet/freescale/fec_main.c:3078:

EEE is not enabled link up. It will work only with ethtool and only if
link was already active.

drivers/net/ethernet/marvell/mvneta.c:4225:

Seems to be ok.

drivers/net/ethernet/microchip/lan743x_ethtool.c:1115:

EEE is not enabled link up. It will work only with ethtool and only if
link was already active.

drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c:130:

EEE will be enabled on open, but only if PHY was fast enough to detect
the link.

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:1084:

May partially work, by driver has many reason to not enable EEE, even if
PHY will continue to advertise it.

In all broken or partially broken cases, the PHY will continue to advertise
EEE support. And the link partner will even potentially try to make use of it.
No Idea if this works good.

Hm.. I need to admit, EEE should not be advertised by default. Only
if MAC driver calls something like phy_support_eee(), we should start doing it.

In case some Intel Ethernet drivers developer read this. There are some issue
too. For example:
net/ethernet/intel/igb/igb_ethtool.c
  igb_get_eee()
	if (adapter->link_duplex == HALF_DUPLEX) {
		edata->eee_enabled = false;
		edata->eee_active = false;
		edata->tx_lpi_enabled = false;
		edata->advertised &= ~edata->advertised;
	}

This part of code will make EEE permanently disabled if link partner switched
to HALF duplex and then back to full duplex.

It can be reproduce with following steps:
system B:
ethtool -s end0 advertise 0x008
system A:
ethtool --show-eee enp1s0f1
	EEE status: enabled - active
system B:
ethtool -s end0 advertise 0x004
system A:
ethtool --show-eee enp1s0f1
	EEE status: disabled
system B:
ethtool -s end0 advertise 0x008
ethtool --show-eee enp1s0f1
	EEE status: disabled

drivers/net/ethernet/intel/igc/igc_ethtool.c is affected as well.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Andrew Lunn]

> I would prefer to not touch phy_init_eee(). At least not in this patch
> set. With this function we have following situation:

We have a complete mess :-(

I spent yesterday re-writing the MAC driver side of EEE. Most get it
completely wrong, as you point out. So i changed the API a bit, making
it more like other negotiated things, so i hope developers will get it
correct in the future.  I will post an RFC/RFT series soon.

> Hm.. I need to admit, EEE should not be advertised by default. Only
> if MAC driver calls something like phy_support_eee(), we should start doing it.

This i've not looked at yet. But i agree.

     Andrew