[PATCH net-next v6 0/8] net: ethernet: Rework EEE

[PATCH net-next v6 0/8] net: ethernet: Rework EEE

[Oleksij Rempel]

Hello all,

with Andrew's permission I'll continue mainlining this patches:

==============================================================

Most MAC drivers get EEE wrong. The API to the PHY is not very
obvious, which is probably why. Rework the API, pushing most of the
EEE handling into phylib core, leaving the MAC drivers to just
enable/disable support for EEE in there change_link call back.

MAC drivers are now expect to indicate to phylib if they support
EEE. This will allow future patches to configure the PHY to advertise
no EEE link modes when EEE is not supported. The information could
also be used to enable SmartEEE if the PHY supports it.

With these changes, the uAPI configuration eee_enable becomes a global
on/off. tx-lpi must also be enabled before EEE is enabled. This fits
the discussion here:

https://lore.kernel.org/netdev/af880ce8-a7b8-138e-1ab9-8c89e662eecf@gmail.com/T/

This patchset puts in place all the infrastructure, and converts one
MAC driver to the new API. Following patchsets will convert other MAC
drivers, extend support into phylink, and when all MAC drivers are
converted to the new scheme, clean up some unneeded code.

v6:
--
Reword different comments. See per patch change comments.

v5:
--
Rebase against latest netdev-next
Use keee instead of eee struct

v4
--
Only convert one MAC driver
Drop all phylink code
Conform to the uAPI discision.

v3
--
Rework phylink code to add a new callback.
Rework function to indicate clock should be stopped during LPI

Andrew Lunn (7):
  net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks
  net: phy: Add helper to set EEE Clock stop enable bit
  net: phy: Keep track of EEE configuration
  net: phy: Immediately call adjust_link if only tx_lpi_enabled changes
  net: phy: Add phy_support_eee() indicating MAC support EEE
  net: fec: Move fec_enet_eee_mode_set() and helper earlier
  net: fec: Fixup EEE

Russell King (1):
  net: add helpers for EEE configuration

 drivers/net/ethernet/freescale/fec_main.c | 84 ++++++++++-------------
 drivers/net/phy/phy-c45.c                 | 11 ++-
 drivers/net/phy/phy.c                     | 55 ++++++++++++++-
 drivers/net/phy/phy_device.c              | 28 ++++++++
 include/linux/phy.h                       |  9 ++-
 include/net/eee.h                         | 38 ++++++++++
 6 files changed, 170 insertions(+), 55 deletions(-)
 create mode 100644 include/net/eee.h

-- 
2.39.2

[PATCH net-next v6 1/8] net: add helpers for EEE configuration

[Oleksij Rempel]

From: Russell King <rmk+kernel@armlinux.org.uk>

Add helpers that phylib and phylink can use to manage EEE configuration
and determine whether the MAC should be permitted to use LPI based on
that configuration.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/net/eee.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 include/net/eee.h

diff --git a/include/net/eee.h b/include/net/eee.h
new file mode 100644
index 000000000000..1232658b32f4
--- /dev/null
+++ b/include/net/eee.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _EEE_H
+#define _EEE_H
+
+#include <linux/types.h>
+
+struct eee_config {
+	u32 tx_lpi_timer;
+	bool tx_lpi_enabled;
+	bool eee_enabled;
+};
+
+static inline bool eeecfg_mac_can_tx_lpi(const struct eee_config *eeecfg)
+{
+	/* eee_enabled is the master on/off */
+	if (!eeecfg->eee_enabled || !eeecfg->tx_lpi_enabled)
+		return false;
+
+	return true;
+}
+
+static inline void eeecfg_to_eee(const struct eee_config *eeecfg,
+			  struct ethtool_keee *eee)
+{
+	eee->tx_lpi_timer = eeecfg->tx_lpi_timer;
+	eee->tx_lpi_enabled = eeecfg->tx_lpi_enabled;
+	eee->eee_enabled = eeecfg->eee_enabled;
+}
+
+static inline void eee_to_eeecfg(const struct ethtool_keee *eee,
+				 struct eee_config *eeecfg)
+{
+	eeecfg->tx_lpi_timer = eee->tx_lpi_timer;
+	eeecfg->tx_lpi_enabled = eee->tx_lpi_enabled;
+	eeecfg->eee_enabled = eee->eee_enabled;
+}
+
+#endif
-- 
2.39.2

[PATCH net-next v6 2/8] net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

MAC drivers which support EEE need to know the results of the EEE
auto-neg in order to program the hardware to perform EEE or not.  The
oddly named phy_init_eee() can be used to determine this, it returns 0
if EEE should be used, or a negative error code,
e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate
resulted in it not being used.

However, many MAC drivers get this wrong. Add phydev->enable_tx_lpi
which indicates the result of the autoneg for EEE, including if EEE is
administratively disabled with ethtool. The MAC driver can then access
this in the same way as link speed and duplex in the adjust link
callback. If enable_tx_lpi is true, the MAC should send low power
indications and does not need to consider anything else with respect
to EEE.

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v2: Check for errors from genphy_c45_eee_is_active
v5: Rename to enable_tx_lpi to fit better with phylink changes
v6: enable_tx_lpi = !!err
---
 drivers/net/phy/phy.c | 7 +++++++
 include/linux/phy.h   | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14224e06d69f..2bc0a7d51c63 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -983,9 +983,16 @@ static int phy_check_link_status(struct phy_device *phydev)
 	if (phydev->link && phydev->state != PHY_RUNNING) {
 		phy_check_downshift(phydev);
 		phydev->state = PHY_RUNNING;
+		err = genphy_c45_eee_is_active(phydev,
+					       NULL, NULL, NULL);
+		if (err < 0)
+			phydev->enable_tx_lpi = false;
+		else
+			phydev->enable_tx_lpi = !!err;
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
 		phydev->state = PHY_NOLINK;
+		phydev->enable_tx_lpi = false;
 		phy_link_down(phydev);
 	}
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e3ab2c347a59..a880f6d7170e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -594,6 +594,7 @@ struct macsec_ops;
  * @supported_eee: supported PHY EEE linkmodes
  * @advertising_eee: Currently advertised EEE linkmodes
  * @eee_enabled: Flag indicating whether the EEE feature is enabled
+ * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -713,6 +714,7 @@ struct phy_device {
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
+	bool enable_tx_lpi;
 
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
-- 
2.39.2

[PATCH net-next v6 3/8] net: phy: Add helper to set EEE Clock stop enable bit

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

The MAC driver can request that the PHY stops the clock during EEE
LPI. This has normally been does as part of phy_init_eee(), however
that function is overly complex and often wrongly used. Add a
standalone helper, to aid removing phy_init_eee().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy.c | 20 ++++++++++++++++++++
 include/linux/phy.h   |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2bc0a7d51c63..ab18b0d9beb4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1579,6 +1579,26 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
+/**
+ * phy_eee_clk_stop_enable - Clock should stop during LIP
+ * @phydev: target phy_device struct
+ *
+ * Description: Program the MMD register 3.0 setting the "Clock stop enable"
+ * bit.
+ */
+int phy_eee_clk_stop_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+			       MDIO_PCS_CTRL1_CLKSTOP_EN);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_eee_clk_stop_enable);
+
 /**
  * phy_init_eee - init and check the EEE feature
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a880f6d7170e..432c561f5809 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1995,6 +1995,7 @@ 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_init_eee(struct phy_device *phydev, bool clk_stop_enable);
+int phy_eee_clk_stop_enable(struct phy_device *phydev);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data);
-- 
2.39.2

[PATCH net-next v6 4/8] net: phy: Keep track of EEE configuration

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

Have phylib keep track of the EEE configuration. This simplifies the
MAC drivers, in that they don't need to store it.

Future patches to phylib will also make use of this information to
further simplify the MAC drivers.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v6: add @ in front of eee_cfg
---
 drivers/net/phy/phy.c | 7 +++++--
 include/linux/phy.h   | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ab18b0d9beb4..f0ed07c74a36 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1660,8 +1660,8 @@ EXPORT_SYMBOL(phy_get_eee_err);
  * @phydev: target phy_device struct
  * @data: ethtool_keee data
  *
- * Description: it reportes the Supported/Advertisement/LP Advertisement
- * capabilities.
+ * Description: reports the Supported/Advertisement/LP Advertisement
+ * capabilities, etc.
  */
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
@@ -1672,6 +1672,7 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_get_eee(phydev, data);
+	eeecfg_to_eee(&phydev->eee_cfg, data);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1694,6 +1695,8 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
+	if (!ret)
+		eee_to_eeecfg(data, &phydev->eee_cfg);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 432c561f5809..c315928357c8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -30,6 +30,7 @@
 #include <linux/refcount.h>
 
 #include <linux/atomic.h>
+#include <net/eee.h>
 
 #define PHY_DEFAULT_FEATURES	(SUPPORTED_Autoneg | \
 				 SUPPORTED_TP | \
@@ -595,6 +596,7 @@ struct macsec_ops;
  * @advertising_eee: Currently advertised EEE linkmodes
  * @eee_enabled: Flag indicating whether the EEE feature is enabled
  * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
+ * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -715,6 +717,7 @@ struct phy_device {
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
 	bool enable_tx_lpi;
+	struct eee_config eee_cfg;
 
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
-- 
2.39.2

[PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

The MAC driver changes its EEE hardware configuration in its
adjust_link callback. This is called when auto-neg
completes. Disabling EEE via eee_enabled false will trigger an
autoneg, and as a result the adjust_link callback will be called with
phydev->enable_tx_lpi set to false. Similarly, eee_enabled set to true
and with a change of advertised link modes will result in a new
autoneg, and a call the adjust_link call.

If set_eee is called with only a change to tx_lpi_enabled which does
not trigger an auto-neg, it is necessary to call the adjust_link
callback so that the MAC is reconfigured to take this change into
account.

When setting phydev->enable_tx_lpi, take both eee_enabled and
tx_lpi_enabled into account, so the MAC drivers just needs to act on
phydev->enable_tx_lpi and not the whole EEE configuration.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v7: remove ' comment and parenthesis or return
---
 drivers/net/phy/phy-c45.c | 11 ++++++++---
 drivers/net/phy/phy.c     | 25 ++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index fa5145c9328e..f98600ed3b35 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1550,6 +1550,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
  * advertised, but the previously advertised link modes are
  * retained. This allows EEE to be enabled/disabled in a
  * non-destructive way.
+ * Returns either error code, 0 if there was no change, or positive
+ * value if there was a change which triggered auto-neg.
  */
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_keee *data)
@@ -1581,9 +1583,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 	ret = genphy_c45_an_config_eee_aneg(phydev);
 	if (ret < 0)
 		return ret;
-	if (ret > 0)
-		return phy_restart_aneg(phydev);
-
+	if (ret > 0) {
+		ret = phy_restart_aneg(phydev);
+		if (ret < 0)
+			return ret;
+		return 1;
+	}
 	return 0;
 }
 EXPORT_SYMBOL(genphy_c45_ethtool_set_eee);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f0ed07c74a36..9e26508d5a31 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -988,7 +988,8 @@ static int phy_check_link_status(struct phy_device *phydev)
 		if (err < 0)
 			phydev->enable_tx_lpi = false;
 		else
-			phydev->enable_tx_lpi = !!err;
+			phydev->enable_tx_lpi = (err & phydev->eee_cfg.tx_lpi_enabled);
+
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
 		phydev->state = PHY_NOLINK;
@@ -1679,6 +1680,21 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_keee *data)
 }
 EXPORT_SYMBOL(phy_ethtool_get_eee);
 
+/* auto-neg not triggered, directly inform the MAC if something
+ * changed
+ */
+static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
+				      struct ethtool_keee *data)
+{
+	if (phydev->eee_cfg.tx_lpi_enabled !=
+	    data->tx_lpi_enabled) {
+		eee_to_eeecfg(data, &phydev->eee_cfg);
+		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
+		if (phydev->link)
+			phy_link_up(phydev);
+	}
+}
+
 /**
  * phy_ethtool_set_eee - set EEE supported and status
  * @phydev: target phy_device struct
@@ -1695,11 +1711,14 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (!ret)
+	if (ret >= 0) {
+		if (ret == 0)
+			phy_ethtool_set_eee_noneg(phydev, data);
 		eee_to_eeecfg(data, &phydev->eee_cfg);
+	}
 	mutex_unlock(&phydev->lock);
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 EXPORT_SYMBOL(phy_ethtool_set_eee);
 
-- 
2.39.2

[PATCH net-next v6 6/8] net: phy: Add phy_support_eee() indicating MAC support EEE

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

In order for EEE to operate, both the MAC and the PHY need to support
it, similar to how pause works. With some exception - a number of PHYs
have SmartEEE or AutoGrEEEn support in order to provide some EEE-like
power savings with non-EEE capable MACs.

Copy the pause concept and add the call phy_support_eee() which the MAC
makes after connecting the PHY to indicate it supports EEE. phylib will
then advertise EEE when auto-neg is performed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
v6: reword commit message and comment for phy_support_eee()
---
 drivers/net/phy/phy_device.c | 28 ++++++++++++++++++++++++++++
 include/linux/phy.h          |  3 ++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2eefee970851..72452e6a478c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2910,6 +2910,34 @@ void phy_advertise_eee_all(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(phy_advertise_eee_all);
 
+/**
+ * phy_support_eee - Set initial EEE policy configuration
+ * @phydev: Target phy_device struct
+ *
+ * This function configures the initial policy for Energy Efficient Ethernet
+ * (EEE) on the specified PHY device, influencing that EEE capabilities are
+ * advertised before the link is established. It should be called during PHY
+ * registration by the MAC driver and/or the PHY driver (for SmartEEE PHYs)
+ * if MAC supports LPI or PHY is capable to compensate missing LPI functionality
+ * of the MAC.
+ *
+ * The function sets default EEE policy parameters, including preparing the PHY
+ * to advertise EEE capabilities based on hardware support.
+ *
+ * It also sets the expected configuration for Low Power Idle (LPI) in the MAC
+ * driver. If the PHY framework determines that both local and remote
+ * advertisements support EEE, and the negotiated link mode is compatible with
+ * EEE, it will set enable_tx_lpi = true. The MAC driver is expected to act on
+ * this setting by enabling the LPI timer if enable_tx_lpi is set.
+ */
+void phy_support_eee(struct phy_device *phydev)
+{
+	linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
+	phydev->eee_cfg.tx_lpi_enabled = true;
+	phydev->eee_cfg.eee_enabled = true;
+}
+EXPORT_SYMBOL(phy_support_eee);
+
 /**
  * phy_support_sym_pause - Enable support of symmetrical pause
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c315928357c8..661c2c969b19 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -706,7 +706,7 @@ struct phy_device {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
 	/* used with phy_speed_down */
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
-	/* used for eee validation */
+	/* used for eee validation and configuration*/
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
 	bool eee_enabled;
@@ -1973,6 +1973,7 @@ void phy_advertise_supported(struct phy_device *phydev);
 void phy_advertise_eee_all(struct phy_device *phydev);
 void phy_support_sym_pause(struct phy_device *phydev);
 void phy_support_asym_pause(struct phy_device *phydev);
+void phy_support_eee(struct phy_device *phydev);
 void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
 		       bool autoneg);
 void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
-- 
2.39.2

[PATCH net-next v6 7/8] net: fec: Move fec_enet_eee_mode_set() and helper earlier

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

FEC is about to get its EEE code re-written. To allow this, move
fec_enet_eee_mode_set() before fec_enet_adjust_link() which will
need to call it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 75 ++++++++++++-----------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 207f1f66c117..a2c786550342 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2017,6 +2017,44 @@ static int fec_get_mac(struct net_device *ndev)
 /*
  * Phy section
  */
+
+/* LPI Sleep Ts count base on tx clk (clk_ref).
+ * The lpi sleep cnt value = X us / (cycle_ns).
+ */
+static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	return us * (fep->clk_ref_rate / 1000) / 1000;
+}
+
+static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct ethtool_keee *p = &fep->eee;
+	unsigned int sleep_cycle, wake_cycle;
+	int ret = 0;
+
+	if (enable) {
+		ret = phy_init_eee(ndev->phydev, false);
+		if (ret)
+			return ret;
+
+		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
+		wake_cycle = sleep_cycle;
+	} else {
+		sleep_cycle = 0;
+		wake_cycle = 0;
+	}
+
+	p->tx_lpi_enabled = enable;
+
+	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
+	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
+
+	return 0;
+}
+
 static void fec_enet_adjust_link(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -3121,43 +3159,6 @@ static int fec_enet_set_coalesce(struct net_device *ndev,
 	return 0;
 }
 
-/* LPI Sleep Ts count base on tx clk (clk_ref).
- * The lpi sleep cnt value = X us / (cycle_ns).
- */
-static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-
-	return us * (fep->clk_ref_rate / 1000) / 1000;
-}
-
-static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct ethtool_keee *p = &fep->eee;
-	unsigned int sleep_cycle, wake_cycle;
-	int ret = 0;
-
-	if (enable) {
-		ret = phy_init_eee(ndev->phydev, false);
-		if (ret)
-			return ret;
-
-		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
-		wake_cycle = sleep_cycle;
-	} else {
-		sleep_cycle = 0;
-		wake_cycle = 0;
-	}
-
-	p->tx_lpi_enabled = enable;
-
-	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
-	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
-
-	return 0;
-}
-
 static int
 fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
 {
-- 
2.39.2

[PATCH net-next v6 8/8] net: fec: Fixup EEE

[Oleksij Rempel]

From: Andrew Lunn <andrew@lunn.ch>

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So move the enable/disable into
fec_enet_adjust_link() which gets called by phylib when there is a
change in link status.

fec_enet_set_eee() now just stores away the LPI timer value.
Everything else is passed to phylib, so it can correctly setup the
PHY.

fec_enet_get_eee() relies on phylib doing most of the work,
the MAC driver just adds the LPI timer value.

Call phy_support_eee() if the quirk is present to indicate the MAC
actually supports EEE.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Oleksij Rempel <o.rempel@pengutronix.de> (On iMX8MP debix)
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
---
v2: Only call fec_enet_eee_mode_set for those that support EEE
v7: update against kernel v6.8-rc4
---
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index a2c786550342..d7693fdf640d 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2033,13 +2033,8 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_keee *p = &fep->eee;
 	unsigned int sleep_cycle, wake_cycle;
-	int ret = 0;
 
 	if (enable) {
-		ret = phy_init_eee(ndev->phydev, false);
-		if (ret)
-			return ret;
-
 		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
 		wake_cycle = sleep_cycle;
 	} else {
@@ -2047,8 +2042,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
 		wake_cycle = 0;
 	}
 
-	p->tx_lpi_enabled = enable;
-
 	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
 	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
 
@@ -2094,6 +2087,8 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 			netif_tx_unlock_bh(ndev);
 			napi_enable(&fep->napi);
 		}
+		if (fep->quirks & FEC_QUIRK_HAS_EEE)
+			fec_enet_eee_mode_set(ndev, phy_dev->enable_tx_lpi);
 	} else {
 		if (fep->link) {
 			netif_stop_queue(ndev);
@@ -2453,6 +2448,9 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 	else
 		phy_set_max_speed(phy_dev, 100);
 
+	if (fep->quirks & FEC_QUIRK_HAS_EEE)
+		phy_support_eee(phy_dev);
+
 	fep->link = 0;
 	fep->full_duplex = 0;
 
@@ -3172,7 +3170,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
 		return -ENETDOWN;
 
 	edata->tx_lpi_timer = p->tx_lpi_timer;
-	edata->tx_lpi_enabled = p->tx_lpi_enabled;
 
 	return phy_ethtool_get_eee(ndev->phydev, edata);
 }
@@ -3182,7 +3179,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_keee *p = &fep->eee;
-	int ret = 0;
 
 	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
 		return -EOPNOTSUPP;
@@ -3192,15 +3188,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata)
 
 	p->tx_lpi_timer = edata->tx_lpi_timer;
 
-	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
-	    !edata->tx_lpi_timer)
-		ret = fec_enet_eee_mode_set(ndev, false);
-	else
-		ret = fec_enet_eee_mode_set(ndev, true);
-
-	if (ret)
-		return ret;
-
 	return phy_ethtool_set_eee(ndev->phydev, edata);
 }
 
-- 
2.39.2

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Russell King (Oracle)]

On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> +				      struct ethtool_keee *data)
> +{
> +	if (phydev->eee_cfg.tx_lpi_enabled !=
> +	    data->tx_lpi_enabled) {
> +		eee_to_eeecfg(data, &phydev->eee_cfg);
> +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> +		if (phydev->link)
> +			phy_link_up(phydev);

I'm not convinced this is a good idea. Hasn't phylib previously had
the guarantee that the link will go down between two link-up events?
So calling phy_link_up() may result in either the MAC driver ignoring
it, or modifying registers that are only supposed to be modified while
the MAC side is down.

-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Oleksij Rempel]

On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > +				      struct ethtool_keee *data)
> > +{
> > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > +	    data->tx_lpi_enabled) {
> > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > +		if (phydev->link)
> > +			phy_link_up(phydev);
> 
> I'm not convinced this is a good idea. Hasn't phylib previously had
> the guarantee that the link will go down between two link-up events?
> So calling phy_link_up() may result in either the MAC driver ignoring
> it, or modifying registers that are only supposed to be modified while
> the MAC side is down.

Ok, I'll drop this patch.

Regards,
Oleksij
-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Andrew Lunn]

On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > +				      struct ethtool_keee *data)
> > +{
> > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > +	    data->tx_lpi_enabled) {
> > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > +		if (phydev->link)
> > +			phy_link_up(phydev);
> 
> I'm not convinced this is a good idea. Hasn't phylib previously had
> the guarantee that the link will go down between two link-up events?
> So calling phy_link_up() may result in either the MAC driver ignoring
> it, or modifying registers that are only supposed to be modified while
> the MAC side is down.

When auto-neg is used, we expect the link to go down and come back up
again.

Here we are dealing with the case that autoneg is not used. The MAC
needs informing somehow. If we want to preserve the down/up, we could
call phy_link_down() and then phy_link_up() back to back.

     Andrew

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Russell King (Oracle)]

On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
> On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > > +				      struct ethtool_keee *data)
> > > +{
> > > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > > +	    data->tx_lpi_enabled) {
> > > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > > +		if (phydev->link)
> > > +			phy_link_up(phydev);
> > 
> > I'm not convinced this is a good idea. Hasn't phylib previously had
> > the guarantee that the link will go down between two link-up events?
> > So calling phy_link_up() may result in either the MAC driver ignoring
> > it, or modifying registers that are only supposed to be modified while
> > the MAC side is down.
> 
> When auto-neg is used, we expect the link to go down and come back up
> again.
> 
> Here we are dealing with the case that autoneg is not used. The MAC
> needs informing somehow. If we want to preserve the down/up, we could
> call phy_link_down() and then phy_link_up() back to back.

Would it be better to have a separate callback for EEE state (as I
mentioned in another comment on this series?) That would be better
for future SmartEEE support.

-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On 2/23/24 05:26, Russell King (Oracle) wrote:
> On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
>> On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
>>> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
>>>> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>>>> +				      struct ethtool_keee *data)
>>>> +{
>>>> +	if (phydev->eee_cfg.tx_lpi_enabled !=
>>>> +	    data->tx_lpi_enabled) {
>>>> +		eee_to_eeecfg(data, &phydev->eee_cfg);
>>>> +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>>>> +		if (phydev->link)
>>>> +			phy_link_up(phydev);
>>>
>>> I'm not convinced this is a good idea. Hasn't phylib previously had
>>> the guarantee that the link will go down between two link-up events?
>>> So calling phy_link_up() may result in either the MAC driver ignoring
>>> it, or modifying registers that are only supposed to be modified while
>>> the MAC side is down.
>>
>> When auto-neg is used, we expect the link to go down and come back up
>> again.
>>
>> Here we are dealing with the case that autoneg is not used. The MAC
>> needs informing somehow. If we want to preserve the down/up, we could
>> call phy_link_down() and then phy_link_up() back to back.
> 
> Would it be better to have a separate callback for EEE state (as I
> mentioned in another comment on this series?) That would be better
> for future SmartEEE support.

That sounds like a good approach to me. The additional callback also 
helps figure out which drivers use the API and it should be simpler to 
audit for changes in the future, too.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Oleksij Rempel]

On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote:
> On 2/23/24 05:26, Russell King (Oracle) wrote:
> > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
> > > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> > > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > > > > +				      struct ethtool_keee *data)
> > > > > +{
> > > > > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > > > > +	    data->tx_lpi_enabled) {
> > > > > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > > > > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > > > > +		if (phydev->link)
> > > > > +			phy_link_up(phydev);
> > > > 
> > > > I'm not convinced this is a good idea. Hasn't phylib previously had
> > > > the guarantee that the link will go down between two link-up events?
> > > > So calling phy_link_up() may result in either the MAC driver ignoring
> > > > it, or modifying registers that are only supposed to be modified while
> > > > the MAC side is down.
> > > 
> > > When auto-neg is used, we expect the link to go down and come back up
> > > again.
> > > 
> > > Here we are dealing with the case that autoneg is not used. The MAC
> > > needs informing somehow. If we want to preserve the down/up, we could
> > > call phy_link_down() and then phy_link_up() back to back.
> > 
> > Would it be better to have a separate callback for EEE state (as I
> > mentioned in another comment on this series?) That would be better
> > for future SmartEEE support.
> 
> That sounds like a good approach to me. The additional callback also helps
> figure out which drivers use the API and it should be simpler to audit for
> changes in the future, too.

At this point I need help to understand how to proceed.
What exactly do you have in mind? Some description like following would
be helpful:
Add callback with name phy_link_set_eee(), which should be executed in
function bla/blup..

Regards,
Oleksij
-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Andrew Lunn]

On Sat, Feb 24, 2024 at 06:57:12PM +0100, Oleksij Rempel wrote:
> On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote:
> > On 2/23/24 05:26, Russell King (Oracle) wrote:
> > > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
> > > > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> > > > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > > > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > > > > > +				      struct ethtool_keee *data)
> > > > > > +{
> > > > > > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > > > > > +	    data->tx_lpi_enabled) {
> > > > > > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > > > > > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > > > > > +		if (phydev->link)
> > > > > > +			phy_link_up(phydev);
> > > > > 
> > > > > I'm not convinced this is a good idea. Hasn't phylib previously had
> > > > > the guarantee that the link will go down between two link-up events?
> > > > > So calling phy_link_up() may result in either the MAC driver ignoring
> > > > > it, or modifying registers that are only supposed to be modified while
> > > > > the MAC side is down.
> > > > 
> > > > When auto-neg is used, we expect the link to go down and come back up
> > > > again.
> > > > 
> > > > Here we are dealing with the case that autoneg is not used. The MAC
> > > > needs informing somehow. If we want to preserve the down/up, we could
> > > > call phy_link_down() and then phy_link_up() back to back.
> > > 
> > > Would it be better to have a separate callback for EEE state (as I
> > > mentioned in another comment on this series?) That would be better
> > > for future SmartEEE support.
> > 
> > That sounds like a good approach to me. The additional callback also helps
> > figure out which drivers use the API and it should be simpler to audit for
> > changes in the future, too.
> 
> At this point I need help to understand how to proceed.
> What exactly do you have in mind? Some description like following would
> be helpful:
> Add callback with name phy_link_set_eee(), which should be executed in
> function bla/blup..

When i first did this patchset, SmartEEE was out of scope. One
question we should decide is, is it still out of scope, and we should
first get 'dumb' EEE fixed everywhere, and than come back and look at
SmartEEE? Or do we want to consider SmartEEE now?

The idea of this patchset was to push as much as possible down into
phylib. The MAC needs to say it supports EEE. I left handling of the
tx_lpi_timer to the MAC driver, because the PHY has nothing it can do
with that value. phylib then just needs to tell the MAC to enable or
disable EEE when autoneg has completed. That i made part of the adjust
link callback because that is the only callback we have, and most
developers seem to understand it, and the locking around it. However,
it does get messy when EEE can change without an auto-neg, as pointed
out here.

If we are leaving SmartEEE out of scope for the moment, i would say
just doing a down/up is sufficient, lets get this merged and all
'dumb' EEE fixed.

If we want feature creep and to think about SmartEEE then we need a
few changes in the overall design.

We need to make eee_get and eee_set transparent to the MAC driver,
since the PHY could be doing it all. So phylib needs to track
tx_lpi_timer. If the MAC driver indicates it can do 'dumb' EEE we
probably want to use that in preference to SmartEEE, since i guess the
MAC can also save a little power in LPI mode. So the adjust link
callback needs to say: Enable MAC EEE with this value of tx_lpi_timer,
or turn off MAC EEE. When using SmartEEE it will never actually do
either.

The current phylib model is that adjust_link is the only callback, and
the MAC driver peeks into phydev to find what it needs. I would
probably stick to that model, and not add MAC callbacks. phylink is
slightly different, mac_link_up() passes everything the MAC needs to
know as parameters, so one of my patches adds an extra parameter to
indicate if EEE should be enabled or disabled. That would need
extending with the tx_lpi_timer value.

	  Andrew

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 5396 bytes --]

On 2/26/24 08:44, Andrew Lunn wrote:
> On Sat, Feb 24, 2024 at 06:57:12PM +0100, Oleksij Rempel wrote:
>> On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote:
>>> On 2/23/24 05:26, Russell King (Oracle) wrote:
>>>> On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
>>>>> On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
>>>>>> On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
>>>>>>> +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>>>>>>> +				      struct ethtool_keee *data)
>>>>>>> +{
>>>>>>> +	if (phydev->eee_cfg.tx_lpi_enabled !=
>>>>>>> +	    data->tx_lpi_enabled) {
>>>>>>> +		eee_to_eeecfg(data, &phydev->eee_cfg);
>>>>>>> +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>>>>>>> +		if (phydev->link)
>>>>>>> +			phy_link_up(phydev);
>>>>>>
>>>>>> I'm not convinced this is a good idea. Hasn't phylib previously had
>>>>>> the guarantee that the link will go down between two link-up events?
>>>>>> So calling phy_link_up() may result in either the MAC driver ignoring
>>>>>> it, or modifying registers that are only supposed to be modified while
>>>>>> the MAC side is down.
>>>>>
>>>>> When auto-neg is used, we expect the link to go down and come back up
>>>>> again.
>>>>>
>>>>> Here we are dealing with the case that autoneg is not used. The MAC
>>>>> needs informing somehow. If we want to preserve the down/up, we could
>>>>> call phy_link_down() and then phy_link_up() back to back.
>>>>
>>>> Would it be better to have a separate callback for EEE state (as I
>>>> mentioned in another comment on this series?) That would be better
>>>> for future SmartEEE support.
>>>
>>> That sounds like a good approach to me. The additional callback also helps
>>> figure out which drivers use the API and it should be simpler to audit for
>>> changes in the future, too.
>>
>> At this point I need help to understand how to proceed.
>> What exactly do you have in mind? Some description like following would
>> be helpful:
>> Add callback with name phy_link_set_eee(), which should be executed in
>> function bla/blup..
> 
> When i first did this patchset, SmartEEE was out of scope. One
> question we should decide is, is it still out of scope, and we should
> first get 'dumb' EEE fixed everywhere, and than come back and look at
> SmartEEE? Or do we want to consider SmartEEE now?

I believe the considerations about SmartEEE (which BTW requires a less 
QCA-centric name to be found) are somewhat orthogonal to the concerns 
from Russell here.

The concern is that MAC drivers are not expecting to see the following 
sequence of calls:

->adjust_link(link == 0)
->adjust_link(link == 1)
->adjust_link(link == 1)

The latter is somewhat unusual and could lead to some spectacular and 
unknown bugs to be discovered. This is even more surprising for drivers 
in that the TX LPI timer is typically programmed as part of the 
.set_eee() callback.

> 
> The idea of this patchset was to push as much as possible down into
> phylib. The MAC needs to say it supports EEE. I left handling of the
> tx_lpi_timer to the MAC driver, because the PHY has nothing it can do
> with that value. phylib then just needs to tell the MAC to enable or
> disable EEE when autoneg has completed. That i made part of the adjust
> link callback because that is the only callback we have, and most
> developers seem to understand it, and the locking around it. However,
> it does get messy when EEE can change without an auto-neg, as pointed
> out here.
> 
> If we are leaving SmartEEE out of scope for the moment, i would say
> just doing a down/up is sufficient, lets get this merged and all
> 'dumb' EEE fixed.
> 
> If we want feature creep and to think about SmartEEE then we need a
> few changes in the overall design.

I am fine with tackling Smart EEE later on, as a matter of fact, I don't 
believe many, if any changes at all should be required to your patch 
series here. We would need to find a less QCA-centric name since 
SmartEEE and AutoGrEEEn are both trade marks, though QCA definitively 
did a better job here at claiming a name people could understand.

> 
> We need to make eee_get and eee_set transparent to the MAC driver,
> since the PHY could be doing it all. So phylib needs to track
> tx_lpi_timer. If the MAC driver indicates it can do 'dumb' EEE we
> probably want to use that in preference to SmartEEE, since i guess the
> MAC can also save a little power in LPI mode. So the adjust link
> callback needs to say: Enable MAC EEE with this value of tx_lpi_timer,
> or turn off MAC EEE. When using SmartEEE it will never actually do
> either.

Agreed.

> 
> The current phylib model is that adjust_link is the only callback, and
> the MAC driver peeks into phydev to find what it needs. I would
> probably stick to that model, and not add MAC callbacks. phylink is
> slightly different, mac_link_up() passes everything the MAC needs to
> know as parameters, so one of my patches adds an extra parameter to
> indicate if EEE should be enabled or disabled. That would need
> extending with the tx_lpi_timer value.

This is the source of the concern, we don't know which MAC drivers we 
might end-up breaking by calling adjust_link(link == 1) twice in a row, 
hopefully none, because they should be well written to only update the 
parameters that need updating, but who knows?
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Russell King (Oracle)]

On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote:
> This is the source of the concern, we don't know which MAC drivers we might
> end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully
> none, because they should be well written to only update the parameters that
> need updating, but who knows?

Just quickly... There are some (I went through a bunch.) They don't
support EEE. I haven't been through all though, so there could be
some which support EEE and where adjust_link() with phydev->link=true
twice in a row could result in badness.

-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Andrew Lunn]

On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote:
> > This is the source of the concern, we don't know which MAC drivers we might
> > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully
> > none, because they should be well written to only update the parameters that
> > need updating, but who knows?
> 
> Just quickly... There are some (I went through a bunch.) They don't
> support EEE. I haven't been through all though, so there could be
> some which support EEE and where adjust_link() with phydev->link=true
> twice in a row could result in badness.

So i think we all agree the MAC needs to see a down/up, even if the
link itself never went down. Anything else is too risky and will
probably break something somewhere.

	 Andrew

Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Oleksij Rempel]

On Mon, Feb 26, 2024 at 07:59:28PM +0100, Andrew Lunn wrote:
> On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote:
> > On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote:
> > > This is the source of the concern, we don't know which MAC drivers we might
> > > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully
> > > none, because they should be well written to only update the parameters that
> > > need updating, but who knows?
> > 
> > Just quickly... There are some (I went through a bunch.) They don't
> > support EEE. I haven't been through all though, so there could be
> > some which support EEE and where adjust_link() with phydev->link=true
> > twice in a row could result in badness.
> 
> So i think we all agree the MAC needs to see a down/up, even if the
> link itself never went down. Anything else is too risky and will
> probably break something somewhere.

Means, this patch should be dropped. Are there other changes required?

Regards,
Oleksij
-- 
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 v6 5/8] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Andrew Lunn]

On Tue, Feb 27, 2024 at 06:35:38AM +0100, Oleksij Rempel wrote:
> On Mon, Feb 26, 2024 at 07:59:28PM +0100, Andrew Lunn wrote:
> > On Mon, Feb 26, 2024 at 05:53:31PM +0000, Russell King (Oracle) wrote:
> > > On Mon, Feb 26, 2024 at 09:50:02AM -0800, Florian Fainelli wrote:
> > > > This is the source of the concern, we don't know which MAC drivers we might
> > > > end-up breaking by calling adjust_link(link == 1) twice in a row, hopefully
> > > > none, because they should be well written to only update the parameters that
> > > > need updating, but who knows?
> > > 
> > > Just quickly... There are some (I went through a bunch.) They don't
> > > support EEE. I haven't been through all though, so there could be
> > > some which support EEE and where adjust_link() with phydev->link=true
> > > twice in a row could result in badness.
> > 
> > So i think we all agree the MAC needs to see a down/up, even if the
> > link itself never went down. Anything else is too risky and will
> > probably break something somewhere.
> 
> Means, this patch should be dropped.

No.

This patch handles the case that EEE is changed, but does not require
an auto-neg cycle. If you drop this patch, that use case breaks.

You need to extend this patch to signal to the MAC a down followed by
an up. It is a fake down, the media side never goes down, but the MAC
needs to think it has in order to keep with the usual convention that
we never call adjust_link() twice with phydev->link not changing.

   Andrew