[RFC/RFTv3 00/24] net: ethernet: Rework EEE

[RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Andrew Lunn]

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, or
phylink mac_link_up callback.

MAC drivers are now expect to indicate to phylib/phylink if they
support EEE. If not, no EEE link modes are advertised. If the MAC does
support EEE, on phy_start()/phylink_start() EEE advertisement is
configured.

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

Andrew Lunn (24):
  net: phy: Add phydev->eee_active to simplify adjust link callbacks
  net: phylink: Add mac_set_eee() callback
  net: phy: Add helper to set EEE Clock stop enable bit
  net: phy: Keep track of EEE tx_lpi_enabled
  net: phy: Immediately call adjust_link if only tx_lpi_enabled changes
  net: phylink: Handle change in EEE from phylib
  net: marvell: mvneta: Simplify EEE configuration
  net: stmmac: Drop usage of phy_init_eee()
  net: stmmac: Simplify ethtool get eee
  net: lan743x: Fixup EEE
  net: fec: Move fec_enet_eee_mode_set() and helper earlier
  net: FEC: Fixup EEE
  net: genet: Fixup EEE
  net: sxgdb: Fixup EEE
  net: dsa: mt7530: Swap to using phydev->eee_active
  net: dsa: b53: Swap to using phydev->eee_active
  net: phylink: Remove unused phylink_init_eee()
  net: phy: remove unused phy_init_eee()
  net: usb: lan78xx: Fixup EEE
  net: phy: Add phy_support_eee() indicating MAC support EEE
  net: phylink: Add MAC_EEE to mac_capabilites
  net: phylink: Extend mac_capabilities in MAC drivers which support EEE
  net: phylib: call phy_support_eee() in MAC drivers which support EEE
  net: phy: Disable EEE advertisement by default

 drivers/net/dsa/b53/b53_common.c              |  5 +-
 drivers/net/dsa/mt7530.c                      |  2 +-
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 42 +++------
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  3 +-
 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  3 +
 drivers/net/ethernet/freescale/fec_main.c     | 88 ++++++++-----------
 drivers/net/ethernet/marvell/mvneta.c         | 28 +++---
 .../net/ethernet/microchip/lan743x_ethtool.c  | 22 -----
 drivers/net/ethernet/microchip/lan743x_main.c |  9 ++
 .../net/ethernet/samsung/sxgbe/sxgbe_common.h |  3 -
 .../ethernet/samsung/sxgbe/sxgbe_ethtool.c    | 21 +----
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 39 +++-----
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 -
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  7 --
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++-
 drivers/net/phy/phy-c45.c                     | 35 +++++++-
 drivers/net/phy/phy-core.c                    | 11 +++
 drivers/net/phy/phy.c                         | 83 ++++++++++-------
 drivers/net/phy/phy_device.c                  | 37 ++++----
 drivers/net/phy/phylink.c                     | 47 +++++-----
 drivers/net/usb/lan78xx.c                     | 44 +++++-----
 include/linux/phy.h                           | 11 ++-
 include/linux/phylink.h                       | 62 ++++++++-----
 include/uapi/linux/mdio.h                     |  1 +
 net/dsa/port.c                                |  3 +
 25 files changed, 308 insertions(+), 309 deletions(-)

-- 
2.40.0

[RFC/RFTv3 01/24] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Andrew Lunn]

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->eee_active 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.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2 Check for errors from genphy_c45_eee_is_active
v3 Refactor into helper which can be used in a later patch
---
 drivers/net/phy/phy.c | 21 +++++++++++++++++++++
 include/linux/phy.h   |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0c0df38cd1ab..150df69d9e08 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -902,6 +902,25 @@ int phy_config_aneg(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_config_aneg);
 
+/**
+ * phy_update_eee_active - Update phydev->eee_active statue
+ * @phydev: the phy_device struct
+ *
+ * Description: Read from the PHY is EEE is active. Use the
+ * information to set eee_active in phydev, which the MAC can then use
+ * to enable EEE in the MAC.
+ */
+static void phy_update_eee_active(struct phy_device *phydev)
+{
+	int err;
+
+	err = genphy_c45_eee_is_active(phydev, NULL, NULL, NULL);
+	if (err < 0)
+		phydev->eee_active = false;
+	else
+		phydev->eee_active = err;
+}
+
 /**
  * phy_check_link_status - check link status and set state accordingly
  * @phydev: the phy_device struct
@@ -928,9 +947,11 @@ 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;
+		phy_update_eee_active(phydev);
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
 		phydev->state = PHY_NOLINK;
+		phydev->eee_active = false;
 		phy_link_down(phydev);
 	}
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fefd5091bc24..5cc2dcb17eb0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -577,6 +577,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
+ * @eee_active: EEE is active for the current link mode
  * @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
@@ -691,6 +692,7 @@ struct phy_device {
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
+	bool eee_active;
 
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
-- 
2.40.0

Re: [RFC/RFTv3 01/24] net: phy: Add phydev->eee_active to simplify adjust link callbacks

[Florian Fainelli]

On 3/30/23 17:54, Andrew Lunn wrote:
> 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->eee_active 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.
> 
> 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>
-- 
Florian

[RFC/RFTv3 02/24] net: phylink: Add mac_set_eee() callback

[Andrew Lunn]

MAC drivers need to know the result of the auto negotiation of Energy
Efficient Ethernet. This is a simple boolean, it should be active or
not in the MAC. Add an additional callback to pass this information to
the MAC.

Currently the correct value should be passed, however no MAC drivers
have been modified to actually use it. Yet.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy-core.c | 11 +++++++++++
 drivers/net/phy/phylink.c  | 16 ++++++++++++++--
 include/linux/phy.h        |  1 +
 include/linux/phylink.h    | 22 +++++++++++++++++++---
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index a64186dc53f8..666b3b9b6f4d 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -76,6 +76,17 @@ const char *phy_duplex_to_str(unsigned int duplex)
 }
 EXPORT_SYMBOL_GPL(phy_duplex_to_str);
 
+/**
+ * phy_eee_active_to_str - Return string describing eee_active
+ *
+ * @eee_active: EEE active setting to describe
+ */
+const char *phy_eee_active_to_str(bool eee_active)
+{
+	return eee_active ? "EEE Active" : "EEE Inactive";
+}
+EXPORT_SYMBOL_GPL(phy_eee_active_to_str);
+
 /**
  * phy_rate_matching_to_str - Return a string describing the rate matching
  *
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f7da96f0c75b..453f80544792 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1142,6 +1142,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 		state->speed = SPEED_UNKNOWN;
 		state->duplex = DUPLEX_UNKNOWN;
 		state->pause = MLO_PAUSE_NONE;
+		state->eee_active = false;
 	} else {
 		state->speed =  pl->link_config.speed;
 		state->duplex = pl->link_config.duplex;
@@ -1223,10 +1224,12 @@ static void phylink_link_up(struct phylink *pl,
 	struct net_device *ndev = pl->netdev;
 	int speed, duplex;
 	bool rx_pause;
+	bool eee_active;
 
 	speed = link_state.speed;
 	duplex = link_state.duplex;
 	rx_pause = !!(link_state.pause & MLO_PAUSE_RX);
+	eee_active = link_state.eee_active;
 
 	switch (link_state.rate_matching) {
 	case RATE_MATCH_PAUSE:
@@ -1259,6 +1262,9 @@ static void phylink_link_up(struct phylink *pl,
 				 pl->cur_interface, speed, duplex,
 				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);
 
+	if (pl->phydev && pl->mac_ops->mac_set_eee)
+		pl->mac_ops->mac_set_eee(pl->config, eee_active);
+
 	if (ndev)
 		netif_carrier_on(ndev);
 
@@ -1529,6 +1535,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 	pl->link_config.pause = MLO_PAUSE_AN;
 	pl->link_config.speed = SPEED_UNKNOWN;
 	pl->link_config.duplex = DUPLEX_UNKNOWN;
+	pl->link_config.eee_active = false;
 	pl->mac_ops = mac_ops;
 	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
@@ -1593,6 +1600,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 	mutex_lock(&pl->state_mutex);
 	pl->phy_state.speed = phydev->speed;
 	pl->phy_state.duplex = phydev->duplex;
+	pl->phy_state.eee_active = phydev->eee_active;
 	pl->phy_state.rate_matching = phydev->rate_matching;
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	if (tx_pause)
@@ -1605,12 +1613,13 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 
 	phylink_run_resolve(pl);
 
-	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s\n", up ? "up" : "down",
+	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s/%s\n", up ? "up" : "down",
 		    phy_modes(phydev->interface),
 		    phy_speed_to_str(phydev->speed),
 		    phy_duplex_to_str(phydev->duplex),
 		    phy_rate_matching_to_str(phydev->rate_matching),
-		    phylink_pause_to_str(pl->phy_state.pause));
+		    phylink_pause_to_str(pl->phy_state.pause),
+		    phy_eee_active_to_str(phydev->eee_active));
 }
 
 static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
@@ -1684,6 +1693,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	pl->phy_state.pause = MLO_PAUSE_NONE;
 	pl->phy_state.speed = SPEED_UNKNOWN;
 	pl->phy_state.duplex = DUPLEX_UNKNOWN;
+	pl->phy_state.eee_active = false;
 	pl->phy_state.rate_matching = RATE_MATCH_NONE;
 	linkmode_copy(pl->supported, supported);
 	linkmode_copy(pl->link_config.advertising, config.advertising);
@@ -2929,6 +2939,7 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 	config.interface = PHY_INTERFACE_MODE_NA;
 	config.speed = SPEED_UNKNOWN;
 	config.duplex = DUPLEX_UNKNOWN;
+	config.eee_active = false;
 	config.pause = MLO_PAUSE_AN;
 
 	/* Ignore errors if we're expecting a PHY to attach later */
@@ -2997,6 +3008,7 @@ static int phylink_sfp_config_optical(struct phylink *pl)
 	linkmode_copy(config.advertising, pl->sfp_support);
 	config.speed = SPEED_UNKNOWN;
 	config.duplex = DUPLEX_UNKNOWN;
+	config.eee_active = false;
 	config.pause = MLO_PAUSE_AN;
 
 	/* For all the interfaces that are supported, reduce the sfp_support
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5cc2dcb17eb0..2508f1d99777 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1080,6 +1080,7 @@ struct phy_fixup {
 
 const char *phy_speed_to_str(int speed);
 const char *phy_duplex_to_str(unsigned int duplex);
+const char *phy_eee_active_to_str(bool eee_active);
 const char *phy_rate_matching_to_str(int rate_matching);
 
 int phy_interface_num_ports(phy_interface_t interface);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 9ff56b050584..7b5df55e2467 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -88,6 +88,7 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
  * @speed: link speed, one of the SPEED_* constants.
  * @duplex: link duplex mode, one of DUPLEX_* constants.
  * @pause: link pause state, described by MLO_PAUSE_* constants.
+ * @eee_active: true if EEE should be active
  * @rate_matching: rate matching being performed, one of the RATE_MATCH_*
  *   constants. If rate matching is taking place, then the speed/duplex of
  *   the medium link mode (@speed and @duplex) and the speed/duplex of the phy
@@ -105,6 +106,7 @@ struct phylink_link_state {
 	int rate_matching;
 	unsigned int link:1;
 	unsigned int an_complete:1;
+	unsigned int eee_active:1;
 };
 
 enum phylink_op_type {
@@ -152,6 +154,7 @@ struct phylink_config {
  * @mac_an_restart: restart 802.3z BaseX autonegotiation.
  * @mac_link_down: take the link down.
  * @mac_link_up: allow the link to come up.
+ * @mac_set_eee: enable/disable EEE in the MAC
  *
  * The individual methods are described more fully below.
  */
@@ -176,6 +179,7 @@ struct phylink_mac_ops {
 			    struct phy_device *phy, unsigned int mode,
 			    phy_interface_t interface, int speed, int duplex,
 			    bool tx_pause, bool rx_pause);
+	void (*mac_set_eee)(struct phylink_config *config, bool eee_active);
 };
 
 #if 0 /* For kernel-doc purposes only. */
@@ -408,6 +412,7 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
  * @duplex: link duplex
  * @tx_pause: link transmit pause enablement status
  * @rx_pause: link receive pause enablement status
+ * @eee_active: EEE should be enabled
  *
  * Configure the MAC for an established link.
  *
@@ -421,14 +426,25 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
  * that the user wishes to override the pause settings, and this should
  * be allowed when considering the implementation of this method.
  *
- * If in-band negotiation mode is disabled, allow the link to come up. If
- * @phy is non-%NULL, configure Energy Efficient Ethernet by calling
- * phy_init_eee() and perform appropriate MAC configuration for EEE.
+ * If in-band negotiation mode is disabled, allow the link to come up.
  * Interface type selection must be done in mac_config().
  */
 void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 unsigned int mode, phy_interface_t interface,
 		 int speed, int duplex, bool tx_pause, bool rx_pause);
+/**
+ * mac_set_eee() - enable/disable EEE in the MAC
+ * @config: a pointer to a &struct phylink_config.
+ *
+ * Enable or disable the MAC performing EEE.
+ *
+ * @eee_active indicates if the MAC should enable EEE. This callback
+ * can be called without the link going down, e.g after calls to
+ * ethtool_set_eee() which don't require a new auto-negotiation.
+ *
+ * This callback is optional.
+ */
+void mac_set_eee(struct phylink_config *config, bool eee_active);
 #endif
 
 struct phylink_pcs_ops;
-- 
2.40.0

[RFC/RFTv3 03/24] net: phy: Add helper to set EEE Clock stop enable bit

[Andrew Lunn]

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(). The helper
currently calls generic code, but it could in the future call a PHY
specific op if a PHY exports different registers to 802.3.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2: Add missing EXPORT_SYMBOL_GPL
v3: Move register access into phy-c45.c
---
 drivers/net/phy/phy-c45.c | 24 ++++++++++++++++++++++++
 drivers/net/phy/phy.c     | 19 +++++++++++++++++++
 include/linux/phy.h       |  2 ++
 include/uapi/linux/mdio.h |  1 +
 4 files changed, 46 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index fee514b96ab1..29479e142826 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1386,6 +1386,30 @@ int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
 }
 EXPORT_SYMBOL(genphy_c45_eee_is_active);
 
+/**
+ * genphy_c45_eee_clk_stop_enable - Clock should stop during LPI signalling
+ * @phydev: target phy_device struct
+ *
+ * Description: Program the MMD register 3.0 setting the "Clock stop enable"
+ * bit.
+ */
+int genphy_c45_eee_clk_stop_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	/* IEEE 802.3:2018 - 45.2.3.2.6 Clock stop capable (3.1.6) */
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_STAT1);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MDIO_STAT1_CLKSTOP_CAPABLE)
+		/* IEEE 802.3-2018 45.2.3.1.4 Clock stop enable (3.0.10) */
+		return phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+					MDIO_PCS_CTRL1_CLKSTOP_EN);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_eee_clk_stop_enable);
+
 /**
  * genphy_c45_ethtool_get_eee - get EEE supported and status
  * @phydev: target phy_device struct
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 150df69d9e08..6fc6a59d2f56 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1573,6 +1573,25 @@ int phy_get_eee_err(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_get_eee_err);
 
+/**
+ * phy_eee_clk_stop_enable - Clock should stop during LIP
+ * @phydev: target phy_device struct
+ *
+ * Description: Program the PHY to stop the clock when EEE is
+ * signalling LPI
+ */
+int phy_eee_clk_stop_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = genphy_c45_eee_clk_stop_enable(phydev);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_eee_clk_stop_enable);
+
 /**
  * phy_ethtool_get_eee - get EEE supported and status
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2508f1d99777..dea707b3189b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1773,6 +1773,7 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv);
 int genphy_c45_an_config_eee_aneg(struct phy_device *phydev);
 int genphy_c45_read_eee_adv(struct phy_device *phydev, unsigned long *adv);
+int genphy_c45_eee_clk_stop_enable(struct phy_device *phydev);
 
 /* Generic C45 PHY driver */
 extern struct phy_driver genphy_c45_driver;
@@ -1847,6 +1848,7 @@ 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_get_eee_err(struct phy_device *phydev);
+int phy_eee_clk_stop_enable(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data);
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data);
 int phy_ethtool_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol);
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 256b463e47a6..e810abb1a4d3 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -121,6 +121,7 @@
 /* Status register 1. */
 #define MDIO_STAT1_LPOWERABLE		0x0002	/* Low-power ability */
 #define MDIO_STAT1_LSTATUS		BMSR_LSTATUS
+#define MDIO_STAT1_CLKSTOP_CAPABLE	0x0040	/* Clock stop capable */
 #define MDIO_STAT1_FAULT		0x0080	/* Fault */
 #define MDIO_AN_STAT1_LPABLE		0x0001	/* Link partner AN ability */
 #define MDIO_AN_STAT1_ABLE		BMSR_ANEGCAPABLE
-- 
2.40.0

[RFC/RFTv3 04/24] net: phy: Keep track of EEE tx_lpi_enabled

[Andrew Lunn]

Have phylib keep track of the EEE tx_lpi_enabled 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.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 5 ++++-
 include/linux/phy.h   | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 6fc6a59d2f56..c7a5d0240211 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1598,7 +1598,7 @@ EXPORT_SYMBOL_GPL(phy_eee_clk_stop_enable);
  * @data: ethtool_eee data
  *
  * Description: it reportes the Supported/Advertisement/LP Advertisement
- * capabilities.
+ * capabilities, etc.
  */
 int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
@@ -1609,6 +1609,7 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_get_eee(phydev, data);
+	data->tx_lpi_enabled = phydev->tx_lpi_enabled;
 	mutex_unlock(&phydev->lock);
 
 	return ret;
@@ -1631,6 +1632,8 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
+	if (!ret)
+		phydev->tx_lpi_enabled = data->tx_lpi_enabled;
 	mutex_unlock(&phydev->lock);
 
 	return ret;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dea707b3189b..4c3d80311c04 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -578,6 +578,7 @@ struct macsec_ops;
  * @advertising_eee: Currently advertised EEE linkmodes
  * @eee_enabled: Flag indicating whether the EEE feature is enabled
  * @eee_active: EEE is active for the current link mode
+ * @tx_lpi_enabled: EEE should send LPI
  * @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
@@ -693,6 +694,7 @@ struct phy_device {
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
 	bool eee_active;
+	bool tx_lpi_enabled;
 
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;
-- 
2.40.0

Re: [RFC/RFTv3 04/24] net: phy: Keep track of EEE tx_lpi_enabled

[Florian Fainelli]

On 3/30/23 17:54, Andrew Lunn wrote:
> Have phylib keep track of the EEE tx_lpi_enabled 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.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [RFC/RFTv3 04/24] net: phy: Keep track of EEE tx_lpi_enabled

[Russell King (Oracle)]

On Tue, May 30, 2023 at 11:11:22AM -0700, Florian Fainelli wrote:
> On 3/30/23 17:54, Andrew Lunn wrote:
> > Have phylib keep track of the EEE tx_lpi_enabled 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.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

To Andrew: I'm not sure this is a good idea, having read 802.3 recently.
EEE is supported on optical as well, which means that LPI, being a MAC
thing, would need to be controlled without the PHY present.

A phylink using driver would need to store the LPI config when it's
used without a phylib PHY. Maybe phylink can store it on behalf of
of the driver instead though.

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

[RFC/RFTv3 05/24] net: phy: Immediately call adjust_link if only tx_lpi_enabled changes

[Andrew Lunn]

The MAC driver changes its EEE hardware configuration in its
adjust_link callback. This is called when auto-neg completes. If
set_eee is called with 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->eee_active, take tx_lpi_enabled into account, so
the MAC drivers don't need to consider tx_lpi_enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v3: Update phydev->eee_active
---
 drivers/net/phy/phy-c45.c | 11 ++++++++---
 drivers/net/phy/phy.c     | 16 +++++++++++++---
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 29479e142826..a16124668e1d 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1455,6 +1455,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
  *
  * Description: it reportes the Supported/Advertisement/LP Advertisement
  * capabilities.
+ * Returns either error code, 0 if there was no change, or positive if
+ * there was a change which triggered auto-neg.
  */
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_eee *data)
@@ -1488,9 +1490,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 c7a5d0240211..f4da7a5440e3 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -918,7 +918,7 @@ static void phy_update_eee_active(struct phy_device *phydev)
 	if (err < 0)
 		phydev->eee_active = false;
 	else
-		phydev->eee_active = err;
+		phydev->eee_active = (err & phydev->tx_lpi_enabled);
 }
 
 /**
@@ -1632,11 +1632,21 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (!ret)
+	if (ret >= 0) {
+		if (ret == 0) {
+			/* auto-neg not triggered */
+			if (phydev->tx_lpi_enabled != data->tx_lpi_enabled) {
+				phydev->tx_lpi_enabled = data->tx_lpi_enabled;
+				phy_update_eee_active(phydev);
+				if (phydev->link)
+					phy_link_up(phydev);
+			}
+		}
 		phydev->tx_lpi_enabled = data->tx_lpi_enabled;
+	}
 	mutex_unlock(&phydev->lock);
 
-	return ret;
+	return (ret < 0 ? ret : 0);
 }
 EXPORT_SYMBOL(phy_ethtool_set_eee);
 
-- 
2.40.0

[RFC/RFTv3 06/24] net: phylink: Handle change in EEE from phylib

[Andrew Lunn]

phylib can indicate a change in EEE outside of the link coming
up. When it does this, the link will already be up and remain
up. Detect this in the phylink adjust_link callback, and call the
mac_set_eee() callback, rather than running the resolver. This then
avoids violating phylinks guarantee of not calling mac_link_up()
without a corresponding mac_link_down().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phylink.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 453f80544792..cf26acc920bd 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1594,10 +1594,13 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 {
 	struct phylink *pl = phydev->phylink;
 	bool tx_pause, rx_pause;
+	bool eee_changed;
 
 	phy_get_pause(phydev, &tx_pause, &rx_pause);
 
 	mutex_lock(&pl->state_mutex);
+	eee_changed = (pl->phy_state.link == up);
+
 	pl->phy_state.speed = phydev->speed;
 	pl->phy_state.duplex = phydev->duplex;
 	pl->phy_state.eee_active = phydev->eee_active;
@@ -1609,9 +1612,14 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 		pl->phy_state.pause |= MLO_PAUSE_RX;
 	pl->phy_state.interface = phydev->interface;
 	pl->phy_state.link = up;
+
+	if (eee_changed && pl->mac_ops->mac_set_eee)
+		pl->mac_ops->mac_set_eee(pl->config, pl->phy_state.eee_active);
+
 	mutex_unlock(&pl->state_mutex);
 
-	phylink_run_resolve(pl);
+	if (!eee_changed)
+		phylink_run_resolve(pl);
 
 	phylink_dbg(pl, "phy link %s %s/%s/%s/%s/%s/%s\n", up ? "up" : "down",
 		    phy_modes(phydev->interface),
-- 
2.40.0

[RFC/RFTv3 07/24] net: marvell: mvneta: Simplify EEE configuration

[Andrew Lunn]

phylib already does most of the work. It will track eee_enabled,
eee_active and tx_lpi_enabled and correctly set them in the
ethtool_get_eee callback.

Replace the call to phy_init_eee() by looking at the value of
eee_active passed to the mac_set_eee callback.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2: Use eee_active parameter, which is race free.
    Remove handling of tx_lpi_enabled, leave it to phylib.
v3: Add mac_set_eee() callback.
---
 drivers/net/ethernet/marvell/mvneta.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 0e39d199ff06..fba9edad9e37 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -536,10 +536,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];
@@ -4170,7 +4166,6 @@ static void mvneta_mac_link_down(struct phylink_config *config,
 		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
 	}
 
-	pp->eee_active = false;
 	mvneta_set_eee(pp, false);
 }
 
@@ -4220,11 +4215,15 @@ static void mvneta_mac_link_up(struct phylink_config *config,
 	}
 
 	mvneta_port_up(pp);
+}
 
-	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_set_eee(struct phylink_config *config,
+			       bool eee_active)
+{
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct mvneta_port *pp = netdev_priv(ndev);
+
+	mvneta_set_eee(pp, eee_active);
 }
 
 static const struct phylink_mac_ops mvneta_phylink_ops = {
@@ -4234,6 +4233,7 @@ 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_set_eee = mvneta_mac_set_eee,
 };
 
 static int mvneta_mdio_probe(struct mvneta_port *pp)
@@ -5028,9 +5028,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev,
 
 	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);
@@ -5053,11 +5050,6 @@ static int mvneta_ethtool_set_eee(struct net_device *dev,
 	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);
 }
 
-- 
2.40.0

Re: [RFC/RFTv3 07/24] net: marvell: mvneta: Simplify EEE configuration

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> phylib already does most of the work. It will track eee_enabled,
> eee_active and tx_lpi_enabled and correctly set them in the
> ethtool_get_eee callback.
> 
> Replace the call to phy_init_eee() by looking at the value of
> eee_active passed to the mac_set_eee callback.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcon.com>
-- 
Florian

[RFC/RFTv3 08/24] net: stmmac: Drop usage of phy_init_eee()

[Andrew Lunn]

Replace this method by looking at the eee_active member of the phydev
structure. Additionally, call the phy_eee_clk_stop_enable() if the
platform indicates the clock should be stopped while LPI is active.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 17310ade88dd..dd2488998993 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1080,8 +1080,9 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active =
-			phy_init_eee(phy, !priv->plat->rx_clk_runs_in_lpi) >= 0;
+		priv->eee_active = phy->eee_active;
+		if (!priv->plat->rx_clk_runs_in_lpi)
+			phy_eee_clk_stop_enable(phy);
 		priv->eee_enabled = stmmac_eee_init(priv);
 		priv->tx_lpi_enabled = priv->eee_enabled;
 		stmmac_set_eee_pls(priv, priv->hw, true);
-- 
2.40.0

[RFC/RFTv3 09/24] net: stmmac: Simplify ethtool get eee

[Andrew Lunn]

phylink_ethtool_get_eee() fills in eee_enabled, eee_active and
tx_lpi_enabled.  So there is no need for the MAC driver to do it as
well.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h         | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 7 -------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c    | 2 --
 3 files changed, 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 3d15e1e92e18..a0f6e58fc622 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -253,7 +253,6 @@ struct stmmac_priv {
 	int eee_enabled;
 	int eee_active;
 	int tx_lpi_timer;
-	int tx_lpi_enabled;
 	int eee_tw_timer;
 	bool eee_sw_timer_en;
 	unsigned int mode;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 35c8dd92d369..fd97cdbb6797 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -782,10 +782,7 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
-	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 
 	return phylink_ethtool_get_eee(priv->phylink, edata);
 }
@@ -799,10 +796,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	if (priv->tx_lpi_enabled != edata->tx_lpi_enabled)
-		netdev_warn(priv->dev,
-			    "Setting EEE tx-lpi is not supported\n");
-
 	if (!edata->eee_enabled)
 		stmmac_disable_eee_mode(priv);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index dd2488998993..190b74d7f4e7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -971,7 +971,6 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, false);
 	priv->eee_active = false;
-	priv->tx_lpi_enabled = false;
 	priv->eee_enabled = stmmac_eee_init(priv);
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
@@ -1084,7 +1083,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 		if (!priv->plat->rx_clk_runs_in_lpi)
 			phy_eee_clk_stop_enable(phy);
 		priv->eee_enabled = stmmac_eee_init(priv);
-		priv->tx_lpi_enabled = priv->eee_enabled;
 		stmmac_set_eee_pls(priv, priv->hw, true);
 	}
 
-- 
2.40.0

Re: [RFC/RFTv3 09/24] net: stmmac: Simplify ethtool get eee

[Oleksij Rempel]

On Fri, Mar 31, 2023 at 02:55:03AM +0200, Andrew Lunn wrote:
> phylink_ethtool_get_eee() fills in eee_enabled, eee_active and
> tx_lpi_enabled.  So there is no need for the MAC driver to do it as
> well.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Complete patch set is tested on stmmac interface of Polyhex Debix Model A
i.MX8MPlus board.

The only thing that's changed from before is that we're not advertising
EEE as a default anymore.

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h         | 1 -
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 7 -------
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c    | 2 --
>  3 files changed, 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 3d15e1e92e18..a0f6e58fc622 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -253,7 +253,6 @@ struct stmmac_priv {
>  	int eee_enabled;
>  	int eee_active;
>  	int tx_lpi_timer;
> -	int tx_lpi_enabled;
>  	int eee_tw_timer;
>  	bool eee_sw_timer_en;
>  	unsigned int mode;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 35c8dd92d369..fd97cdbb6797 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -782,10 +782,7 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
>  	if (!priv->dma_cap.eee)
>  		return -EOPNOTSUPP;
>  
> -	edata->eee_enabled = priv->eee_enabled;
> -	edata->eee_active = priv->eee_active;
>  	edata->tx_lpi_timer = priv->tx_lpi_timer;
> -	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
>  
>  	return phylink_ethtool_get_eee(priv->phylink, edata);
>  }
> @@ -799,10 +796,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
>  	if (!priv->dma_cap.eee)
>  		return -EOPNOTSUPP;
>  
> -	if (priv->tx_lpi_enabled != edata->tx_lpi_enabled)
> -		netdev_warn(priv->dev,
> -			    "Setting EEE tx-lpi is not supported\n");
> -
>  	if (!edata->eee_enabled)
>  		stmmac_disable_eee_mode(priv);
>  
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index dd2488998993..190b74d7f4e7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -971,7 +971,6 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>  
>  	stmmac_mac_set(priv, priv->ioaddr, false);
>  	priv->eee_active = false;
> -	priv->tx_lpi_enabled = false;
>  	priv->eee_enabled = stmmac_eee_init(priv);
>  	stmmac_set_eee_pls(priv, priv->hw, false);
>  
> @@ -1084,7 +1083,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
>  		if (!priv->plat->rx_clk_runs_in_lpi)
>  			phy_eee_clk_stop_enable(phy);
>  		priv->eee_enabled = stmmac_eee_init(priv);
> -		priv->tx_lpi_enabled = priv->eee_enabled;
>  		stmmac_set_eee_pls(priv, priv->hw, true);
>  	}
>  
> -- 
> 2.40.0
> 
> 

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

[RFC/RFTv3 10/24] net: lan743x: Fixup EEE

[Andrew Lunn]

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

lan743x_ethtool_set_eee() now just programs the hardware with the LTI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

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

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/microchip/lan743x_ethtool.c  | 22 -------------------
 drivers/net/ethernet/microchip/lan743x_main.c |  7 ++++++
 2 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 2db5949b4c7e..da2f1110e0db 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1073,16 +1073,10 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev,
 
 	buf = lan743x_csr_read(adapter, MAC_CR);
 	if (buf & MAC_CR_EEE_EN_) {
-		eee->eee_enabled = true;
-		eee->eee_active = !!(eee->advertised & eee->lp_advertised);
-		eee->tx_lpi_enabled = true;
 		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
 		buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
 		eee->tx_lpi_timer = buf;
 	} else {
-		eee->eee_enabled = false;
-		eee->eee_active = false;
-		eee->tx_lpi_enabled = false;
 		eee->tx_lpi_timer = 0;
 	}
 
@@ -1095,7 +1089,6 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	struct lan743x_adapter *adapter;
 	struct phy_device *phydev;
 	u32 buf = 0;
-	int ret = 0;
 
 	if (!netdev)
 		return -EINVAL;
@@ -1112,23 +1105,8 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
 	}
 
 	if (eee->eee_enabled) {
-		ret = phy_init_eee(phydev, false);
-		if (ret) {
-			netif_err(adapter, drv, adapter->netdev,
-				  "EEE initialization failed\n");
-			return ret;
-		}
-
 		buf = (u32)eee->tx_lpi_timer;
 		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
-
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf |= MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
-	} else {
-		buf = lan743x_csr_read(adapter, MAC_CR);
-		buf &= ~MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, buf);
 	}
 
 	return phy_ethtool_set_eee(phydev, eee);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 957d96a91a8a..7986f8fcf7d3 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1457,6 +1457,13 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		    phydev->interface == PHY_INTERFACE_MODE_1000BASEX ||
 		    phydev->interface == PHY_INTERFACE_MODE_2500BASEX)
 			lan743x_sgmii_config(adapter);
+
+		data = lan743x_csr_read(adapter, MAC_CR);
+		if (phydev->eee_active)
+			data |=  MAC_CR_EEE_EN_;
+		else
+			data &= ~MAC_CR_EEE_EN_;
+		lan743x_csr_write(adapter, MAC_CR, data);
 	}
 }
 
-- 
2.40.0

Re: [RFC/RFTv3 10/24] net: lan743x: Fixup EEE

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So move the enable/disable into
> lan743x_phy_link_status_change() which gets called by phylib when
> there is a change in link status.
> 
> lan743x_ethtool_set_eee() now just programs the hardware with the LTI
> timer value, and passed everything else to phylib, so it can correctly
> setup the PHY.
> 
> lan743x_ethtool_get_eee() relies on phylib doing most of the work, the
> MAC driver just adds the LTI timer value.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

LGTM:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[RFC/RFTv3 11/24] net: fec: Move fec_enet_eee_mode_set() and helper earlier

[Andrew Lunn]

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>
---
 drivers/net/ethernet/freescale/fec_main.c | 79 ++++++++++++-----------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f3b16a6673e2..462755f5d33e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1919,6 +1919,46 @@ 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_eee *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;
+	p->eee_enabled = enable;
+	p->eee_active = 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);
@@ -3057,45 +3097,6 @@ static int fec_enet_set_tunable(struct net_device *netdev,
 	return ret;
 }
 
-/* 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_eee *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;
-	p->eee_enabled = enable;
-	p->eee_active = 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_eee *edata)
 {
-- 
2.40.0

[RFC/RFTv3 12/24] net: FEC: Fixup EEE

[Andrew Lunn]

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 LTI 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 LTI timer value.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2: Only call fec_enet_eee_mode_set for those that support EEE
---
 drivers/net/ethernet/freescale/fec_main.c | 28 ++++-------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 462755f5d33e..fda1f9ff32b9 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
 	return us * (fep->clk_ref_rate / 1000) / 1000;
 }
 
-static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
+static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_eee *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;
 
+	if (eee_active) {
 		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
 		wake_cycle = sleep_cycle;
 	} else {
@@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
 		wake_cycle = 0;
 	}
 
-	p->tx_lpi_enabled = enable;
-	p->eee_enabled = enable;
-	p->eee_active = enable;
-
 	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
 	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
 
@@ -1997,6 +1988,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->eee_active);
 	} else {
 		if (fep->link) {
 			napi_disable(&fep->napi);
@@ -3109,10 +3102,7 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
 	if (!netif_running(ndev))
 		return -ENETDOWN;
 
-	edata->eee_enabled = p->eee_enabled;
-	edata->eee_active = p->eee_active;
 	edata->tx_lpi_timer = p->tx_lpi_timer;
-	edata->tx_lpi_enabled = p->tx_lpi_enabled;
 
 	return phy_ethtool_get_eee(ndev->phydev, edata);
 }
@@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct ethtool_eee *p = &fep->eee;
-	int ret = 0;
 
 	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
 		return -EOPNOTSUPP;
@@ -3132,15 +3121,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *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.40.0

Re: [RFC/RFTv3 12/24] net: FEC: Fixup EEE

[Oleksij Rempel]

On Fri, Mar 31, 2023 at 02:55:06AM +0200, Andrew Lunn wrote:
> 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 LTI 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 LTI timer value.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Tested on imx8mp based debix mode a board.

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
> v2: Only call fec_enet_eee_mode_set for those that support EEE
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 28 ++++-------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 462755f5d33e..fda1f9ff32b9 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us)
>  	return us * (fep->clk_ref_rate / 1000) / 1000;
>  }
>  
> -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
> +static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct ethtool_eee *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;
>  
> +	if (eee_active) {
>  		sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer);
>  		wake_cycle = sleep_cycle;
>  	} else {
> @@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
>  		wake_cycle = 0;
>  	}
>  
> -	p->tx_lpi_enabled = enable;
> -	p->eee_enabled = enable;
> -	p->eee_active = enable;
> -
>  	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
>  	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
>  
> @@ -1997,6 +1988,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->eee_active);
>  	} else {
>  		if (fep->link) {
>  			napi_disable(&fep->napi);
> @@ -3109,10 +3102,7 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  	if (!netif_running(ndev))
>  		return -ENETDOWN;
>  
> -	edata->eee_enabled = p->eee_enabled;
> -	edata->eee_active = p->eee_active;
>  	edata->tx_lpi_timer = p->tx_lpi_timer;
> -	edata->tx_lpi_enabled = p->tx_lpi_enabled;
>  
>  	return phy_ethtool_get_eee(ndev->phydev, edata);
>  }
> @@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct ethtool_eee *p = &fep->eee;
> -	int ret = 0;
>  
>  	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
>  		return -EOPNOTSUPP;
> @@ -3132,15 +3121,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *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.40.0
> 
> 

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

[RFC/RFTv3 13/24] net: genet: Fixup EEE

[Andrew Lunn]

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

bcmgenet_set_eee() now just writes the LTI timer value to the
hardware.  Everything else is passed to phylib, so it can correctly
setup the PHY.

bcmgenet_get_eee() relies on phylib doing most of the work, the MAC
driver just adds the LTI timer value from hardware.

The call to bcmgenet_eee_enable_set() in the resume function has been
removed. There is both unconditional calls to phy_init_hw() and
genphy_config_aneg, and a call to phy_resume(). As a result, the PHY
is going to perform auto-neg, and then it completes
bcmgenet_mii_setup() will be called, which will set the hardware to
the correct EEE mode.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 42 +++++--------------
 .../net/ethernet/broadcom/genet/bcmgenet.h    |  3 +-
 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  1 +
 3 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d937daa8ee88..035486304e31 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1272,19 +1272,21 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
-static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
+void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
+	u32 off;
 	u32 reg;
 
-	if (enable && !priv->clk_eee_enabled) {
+	off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
+
+	if (eee_active && !priv->clk_eee_enabled) {
 		clk_prepare_enable(priv->clk_eee);
 		priv->clk_eee_enabled = true;
 	}
 
 	reg = bcmgenet_umac_readl(priv, UMAC_EEE_CTRL);
-	if (enable)
+	if (eee_active)
 		reg |= EEE_EN;
 	else
 		reg &= ~EEE_EN;
@@ -1292,7 +1294,7 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
 
 	/* Enable EEE and switch to a 27Mhz clock automatically */
 	reg = bcmgenet_readl(priv->base + off);
-	if (enable)
+	if (eee_active)
 		reg |= TBUF_EEE_EN | TBUF_PM_EN;
 	else
 		reg &= ~(TBUF_EEE_EN | TBUF_PM_EN);
@@ -1300,25 +1302,21 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
 
 	/* Do the same for thing for RBUF */
 	reg = bcmgenet_rbuf_readl(priv, RBUF_ENERGY_CTRL);
-	if (enable)
+	if (eee_active)
 		reg |= RBUF_EEE_EN | RBUF_PM_EN;
 	else
 		reg &= ~(RBUF_EEE_EN | RBUF_PM_EN);
 	bcmgenet_rbuf_writel(priv, reg, RBUF_ENERGY_CTRL);
 
-	if (!enable && priv->clk_eee_enabled) {
+	if (!eee_active && priv->clk_eee_enabled) {
 		clk_disable_unprepare(priv->clk_eee);
 		priv->clk_eee_enabled = false;
 	}
-
-	priv->eee.eee_enabled = enable;
-	priv->eee.eee_active = enable;
 }
 
 static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct ethtool_eee *p = &priv->eee;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1326,8 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!dev->phydev)
 		return -ENODEV;
 
-	e->eee_enabled = p->eee_enabled;
-	e->eee_active = p->eee_active;
 	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_get_eee(dev->phydev, e);
@@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	struct ethtool_eee *p = &priv->eee;
-	int ret = 0;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1345,20 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!dev->phydev)
 		return -ENODEV;
 
-	p->eee_enabled = e->eee_enabled;
-
-	if (!p->eee_enabled) {
-		bcmgenet_eee_enable_set(dev, false);
-	} else {
-		ret = phy_init_eee(dev->phydev, false);
-		if (ret) {
-			netif_err(priv, hw, dev, "EEE initialization failed\n");
-			return ret;
-		}
-
-		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
-		bcmgenet_eee_enable_set(dev, true);
-	}
+	bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_set_eee(dev->phydev, e);
 }
@@ -4278,9 +4259,6 @@ static int bcmgenet_resume(struct device *d)
 	if (!device_may_wakeup(d))
 		phy_resume(dev->phydev);
 
-	if (priv->eee.eee_enabled)
-		bcmgenet_eee_enable_set(dev, true);
-
 	bcmgenet_netif_start(dev);
 
 	netif_device_attach(dev);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 946f6e283c4e..8c9643ec738c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -644,8 +644,6 @@ struct bcmgenet_priv {
 	bool wol_active;
 
 	struct bcmgenet_mib_counters mib;
-
-	struct ethtool_eee eee;
 };
 
 #define GENET_IO_MACRO(name, offset)					\
@@ -703,4 +701,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode);
 
+void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active);
 #endif /* __BCMGENET_H__ */
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index be042905ada2..6c39839762a7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev)
 
 	if (phydev->link) {
 		bcmgenet_mac_config(dev);
+		bcmgenet_eee_enable_set(dev, phydev->eee_active);
 	} else {
 		reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
 		reg &= ~RGMII_LINK;
-- 
2.40.0

Re: [RFC/RFTv3 13/24] net: genet: Fixup EEE

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> The enabling/disabling of EEE in the MAC should happen as a result of
> auto negotiation. So move the enable/disable into bcmgenet_mii_setup()
> which gets called by phylib when there is a change in link status.
> 
> bcmgenet_set_eee() now just writes the LTI timer value to the
> hardware.  Everything else is passed to phylib, so it can correctly
> setup the PHY.
> 
> bcmgenet_get_eee() relies on phylib doing most of the work, the MAC
> driver just adds the LTI timer value from hardware.
> 
> The call to bcmgenet_eee_enable_set() in the resume function has been
> removed. There is both unconditional calls to phy_init_hw() and
> genphy_config_aneg, and a call to phy_resume(). As a result, the PHY
> is going to perform auto-neg, and then it completes
> bcmgenet_mii_setup() will be called, which will set the hardware to
> the correct EEE mode.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   .../net/ethernet/broadcom/genet/bcmgenet.c    | 42 +++++--------------
>   .../net/ethernet/broadcom/genet/bcmgenet.h    |  3 +-
>   drivers/net/ethernet/broadcom/genet/bcmmii.c  |  1 +
>   3 files changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d937daa8ee88..035486304e31 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1272,19 +1272,21 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
>   	}
>   }
>   
> -static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
> +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active)

Replacing the argument name here is a bit of noise in reviewing the 
patch, and it does not fundamentally change the behavior or semantics IMHO.

>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
> -	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
> +	u32 off;
>   	u32 reg;
>   
> -	if (enable && !priv->clk_eee_enabled) {
> +	off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
> +
> +	if (eee_active && !priv->clk_eee_enabled) {
>   		clk_prepare_enable(priv->clk_eee);
>   		priv->clk_eee_enabled = true;
>   	}
>   
>   	reg = bcmgenet_umac_readl(priv, UMAC_EEE_CTRL);
> -	if (enable)
> +	if (eee_active)
>   		reg |= EEE_EN;
>   	else
>   		reg &= ~EEE_EN;
> @@ -1292,7 +1294,7 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
>   
>   	/* Enable EEE and switch to a 27Mhz clock automatically */
>   	reg = bcmgenet_readl(priv->base + off);
> -	if (enable)
> +	if (eee_active)
>   		reg |= TBUF_EEE_EN | TBUF_PM_EN;
>   	else
>   		reg &= ~(TBUF_EEE_EN | TBUF_PM_EN);
> @@ -1300,25 +1302,21 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
>   
>   	/* Do the same for thing for RBUF */
>   	reg = bcmgenet_rbuf_readl(priv, RBUF_ENERGY_CTRL);
> -	if (enable)
> +	if (eee_active)
>   		reg |= RBUF_EEE_EN | RBUF_PM_EN;
>   	else
>   		reg &= ~(RBUF_EEE_EN | RBUF_PM_EN);
>   	bcmgenet_rbuf_writel(priv, reg, RBUF_ENERGY_CTRL);
>   
> -	if (!enable && priv->clk_eee_enabled) {
> +	if (!eee_active && priv->clk_eee_enabled) {
>   		clk_disable_unprepare(priv->clk_eee);
>   		priv->clk_eee_enabled = false;
>   	}
> -
> -	priv->eee.eee_enabled = enable;
> -	priv->eee.eee_active = enable;
>   }
>   
>   static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
> -	struct ethtool_eee *p = &priv->eee;
>   
>   	if (GENET_IS_V1(priv))
>   		return -EOPNOTSUPP;
> @@ -1326,8 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
>   	if (!dev->phydev)
>   		return -ENODEV;
>   
> -	e->eee_enabled = p->eee_enabled;
> -	e->eee_active = p->eee_active;
>   	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
>   
>   	return phy_ethtool_get_eee(dev->phydev, e);
> @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
>   static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
> -	struct ethtool_eee *p = &priv->eee;
> -	int ret = 0;
>   
>   	if (GENET_IS_V1(priv))
>   		return -EOPNOTSUPP;
> @@ -1345,20 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
>   	if (!dev->phydev)
>   		return -ENODEV;
>   
> -	p->eee_enabled = e->eee_enabled;
> -
> -	if (!p->eee_enabled) {
> -		bcmgenet_eee_enable_set(dev, false);
> -	} else {
> -		ret = phy_init_eee(dev->phydev, false);
> -		if (ret) {
> -			netif_err(priv, hw, dev, "EEE initialization failed\n");
> -			return ret;
> -		}
> -
> -		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
> -		bcmgenet_eee_enable_set(dev, true);
> -	}
> +	bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
>   
>   	return phy_ethtool_set_eee(dev->phydev, e);
>   }
> @@ -4278,9 +4259,6 @@ static int bcmgenet_resume(struct device *d)
>   	if (!device_may_wakeup(d))
>   		phy_resume(dev->phydev);
>   
> -	if (priv->eee.eee_enabled)
> -		bcmgenet_eee_enable_set(dev, true);
> -
>   	bcmgenet_netif_start(dev);
>   
>   	netif_device_attach(dev);
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 946f6e283c4e..8c9643ec738c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -644,8 +644,6 @@ struct bcmgenet_priv {
>   	bool wol_active;
>   
>   	struct bcmgenet_mib_counters mib;
> -
> -	struct ethtool_eee eee;
>   };
>   
>   #define GENET_IO_MACRO(name, offset)					\
> @@ -703,4 +701,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
>   void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
>   			       enum bcmgenet_power_mode mode);
>   
> +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active);
>   #endif /* __BCMGENET_H__ */
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index be042905ada2..6c39839762a7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev)
>   
>   	if (phydev->link) {
>   		bcmgenet_mac_config(dev);
> +		bcmgenet_eee_enable_set(dev, phydev->eee_active);

That part is a real bug fix, I do have a tentative patch that I should 
be able to submit to 'net' soon after I finish testing a few things with 
it. Thanks Andrew!
-- 
Florian

[RFC/RFTv3 14/24] net: sxgdb: Fixup EEE

[Andrew Lunn]

The enabling/disabling of EEE in the MAC should happen as a result of
auto negotiation. So rework sxgbe_eee_adjust() to take the result of
negotiation into account.

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

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

The hw_cap.eee is now used to control timers, rather than eee_enabled,
which was wrongly being set based on the value of phy_init_eee()
before auto-neg even completed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_common.h |  3 --
 .../ethernet/samsung/sxgbe/sxgbe_ethtool.c    | 21 ++---------
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 36 ++++++-------------
 3 files changed, 12 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
index 0f45107db8dd..c25588b90a13 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
@@ -502,8 +502,6 @@ struct sxgbe_priv_data {
 	struct timer_list eee_ctrl_timer;
 	bool tx_path_in_lpi_mode;
 	int lpi_irq;
-	int eee_enabled;
-	int eee_active;
 	int tx_lpi_timer;
 };
 
@@ -528,5 +526,4 @@ int sxgbe_restore(struct net_device *ndev);
 const struct sxgbe_mtl_ops *sxgbe_get_mtl_ops(void);
 
 void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv);
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv);
 #endif /* __SXGBE_COMMON_H__ */
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
index 8ba017ec9849..e7128864a3ef 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
@@ -140,8 +140,6 @@ static int sxgbe_get_eee(struct net_device *dev,
 	if (!priv->hw_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
 
 	return phy_ethtool_get_eee(dev->phydev, edata);
@@ -152,22 +150,7 @@ static int sxgbe_set_eee(struct net_device *dev,
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	priv->eee_enabled = edata->eee_enabled;
-
-	if (!priv->eee_enabled) {
-		sxgbe_disable_eee_mode(priv);
-	} else {
-		/* We are asking for enabling the EEE but it is safe
-		 * to verify all by invoking the eee_init function.
-		 * In case of failure it will return an error.
-		 */
-		priv->eee_enabled = sxgbe_eee_init(priv);
-		if (!priv->eee_enabled)
-			return -EOPNOTSUPP;
-
-		/* Do not change tx_lpi_timer in case of failure */
-		priv->tx_lpi_timer = edata->tx_lpi_timer;
-	}
+	priv->tx_lpi_timer = edata->tx_lpi_timer;
 
 	return phy_ethtool_set_eee(dev->phydev, edata);
 }
@@ -230,7 +213,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev,
 	int i;
 	char *p;
 
-	if (priv->eee_enabled) {
+	if (dev->phydev->eee_active) {
 		int val = phy_get_eee_err(dev->phydev);
 
 		if (val)
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 9664f029fa16..cf549a524674 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -119,18 +119,10 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t)
  *  phy can also manage EEE, so enable the LPI state and start the timer
  *  to verify if the tx path can enter in LPI state.
  */
-bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
+static void sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 {
-	struct net_device *ndev = priv->dev;
-	bool ret = false;
-
 	/* MAC core supports the EEE feature. */
 	if (priv->hw_cap.eee) {
-		/* Check if the PHY supports EEE */
-		if (phy_init_eee(ndev->phydev, true))
-			return false;
-
-		priv->eee_active = 1;
 		timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0);
 		priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer);
 		add_timer(&priv->eee_ctrl_timer);
@@ -140,23 +132,15 @@ bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 					     priv->tx_lpi_timer);
 
 		pr_info("Energy-Efficient Ethernet initialized\n");
-
-		ret = true;
 	}
-
-	return ret;
 }
 
-static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv)
+static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv,
+			     bool eee_active)
 {
-	struct net_device *ndev = priv->dev;
-
-	/* When the EEE has been already initialised we have to
-	 * modify the PLS bit in the LPI ctrl & status reg according
-	 * to the PHY link status. For this reason.
-	 */
-	if (priv->eee_enabled)
-		priv->hw->mac->set_eee_pls(priv->ioaddr, ndev->phydev->link);
+	if (priv->hw_cap.eee)
+		priv->hw->mac->set_eee_pls(priv->ioaddr, eee_active);
+	phy_eee_clk_stop_enable(priv->dev->phydev);
 }
 
 /**
@@ -250,7 +234,7 @@ static void sxgbe_adjust_link(struct net_device *dev)
 		phy_print_status(phydev);
 
 	/* Alter the MAC settings for EEE */
-	sxgbe_eee_adjust(priv);
+	sxgbe_eee_adjust(priv, phydev->eee_active);
 }
 
 /**
@@ -803,7 +787,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv)
 		sxgbe_tx_queue_clean(tqueue);
 	}
 
-	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
+	if (priv->hw_cap.eee && !priv->tx_path_in_lpi_mode) {
 		sxgbe_enable_eee_mode(priv);
 		mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
 	}
@@ -1181,7 +1165,7 @@ static int sxgbe_open(struct net_device *dev)
 	}
 
 	priv->tx_lpi_timer = SXGBE_DEFAULT_LPI_TIMER;
-	priv->eee_enabled = sxgbe_eee_init(priv);
+	sxgbe_eee_init(priv);
 
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
@@ -1208,7 +1192,7 @@ static int sxgbe_release(struct net_device *dev)
 {
 	struct sxgbe_priv_data *priv = netdev_priv(dev);
 
-	if (priv->eee_enabled)
+	if (priv->hw_cap.eee)
 		del_timer_sync(&priv->eee_ctrl_timer);
 
 	/* Stop and disconnect the PHY */
-- 
2.40.0

[RFC/RFTv3 15/24] net: dsa: mt7530: Swap to using phydev->eee_active

[Andrew Lunn]

Rather than calling phy_init_eee() retrieve the same information from
within the phydev structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a0d99af897ac..19d089eadcd0 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2753,7 +2753,7 @@ static void mt753x_phylink_mac_link_up(struct dsa_switch *ds, int port,
 			mcr |= PMCR_RX_FC_EN;
 	}
 
-	if (mode == MLO_AN_PHY && phydev && phy_init_eee(phydev, false) >= 0) {
+	if (mode == MLO_AN_PHY && phydev && phydev->eee_active) {
 		switch (speed) {
 		case SPEED_1000:
 			mcr |= PMCR_FORCE_EEE1G;
-- 
2.40.0

[RFC/RFTv3 16/24] net: dsa: b53: Swap to using phydev->eee_active

[Andrew Lunn]

Rather than calling phy_init_eee() retrieve the same information from
within the phydev structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/b53/b53_common.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 3464ce5e7470..5984733e4d0d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2218,10 +2218,7 @@ EXPORT_SYMBOL(b53_eee_enable_set);
  */
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
-	int ret;
-
-	ret = phy_init_eee(phy, false);
-	if (ret)
+	if (!phy->eee_active)
 		return 0;
 
 	b53_eee_enable_set(ds, port, true);
-- 
2.40.0

Re: [RFC/RFTv3 16/24] net: dsa: b53: Swap to using phydev->eee_active

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> Rather than calling phy_init_eee() retrieve the same information from
> within the phydev structure.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[RFC/RFTv3 17/24] net: phylink: Remove unused phylink_init_eee()

[Andrew Lunn]

This is not used in tree, and the phylib equivalent phy_init_eee() is
about to be removed.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index cf26acc920bd..61e4502086c3 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2517,24 +2517,6 @@ int phylink_get_eee_err(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_get_eee_err);
 
-/**
- * phylink_init_eee() - init and check the EEE features
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- * @clk_stop_enable: allow PHY to stop receive clock
- *
- * Must be called either with RTNL held or within mac_link_up()
- */
-int phylink_init_eee(struct phylink *pl, bool clk_stop_enable)
-{
-	int ret = -EOPNOTSUPP;
-
-	if (pl->phydev)
-		ret = phy_init_eee(pl->phydev, clk_stop_enable);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phylink_init_eee);
-
 /**
  * phylink_ethtool_get_eee() - read the energy efficient ethernet parameters
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 7b5df55e2467..52f29b648853 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -617,7 +617,6 @@ void phylink_ethtool_get_pauseparam(struct phylink *,
 int phylink_ethtool_set_pauseparam(struct phylink *,
 				   struct ethtool_pauseparam *);
 int phylink_get_eee_err(struct phylink *);
-int phylink_init_eee(struct phylink *, bool);
 int phylink_ethtool_get_eee(struct phylink *, struct ethtool_eee *);
 int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
 int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
-- 
2.40.0

[RFC/RFTv3 18/24] net: phy: remove unused phy_init_eee()

[Andrew Lunn]

There are no users left of phy_init_eee(), and it is often wrongly
used. So remove it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 34 ----------------------------------
 include/linux/phy.h   |  1 -
 2 files changed, 35 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f4da7a5440e3..a70de8dbe6b6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1517,40 +1517,6 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
-/**
- * phy_init_eee - init and check the EEE feature
- * @phydev: target phy_device struct
- * @clk_stop_enable: PHY may stop the clock during LPI
- *
- * Description: it checks if the Energy-Efficient Ethernet (EEE)
- * is supported by looking at the MMD registers 3.20 and 7.60/61
- * and it programs the MMD register 3.0 setting the "Clock stop enable"
- * bit if required.
- */
-int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
-{
-	int ret;
-
-	if (!phydev->drv)
-		return -EIO;
-
-	ret = genphy_c45_eee_is_active(phydev, NULL, NULL, NULL);
-	if (ret < 0)
-		return ret;
-	if (!ret)
-		return -EPROTONOSUPPORT;
-
-	if (clk_stop_enable)
-		/* Configure the PHY to stop receiving xMII
-		 * clock while it is signaling LPI.
-		 */
-		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
-				       MDIO_PCS_CTRL1_CLKSTOP_EN);
-
-	return ret < 0 ? ret : 0;
-}
-EXPORT_SYMBOL(phy_init_eee);
-
 /**
  * phy_get_eee_err - report the EEE wake error count
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4c3d80311c04..af199fcc52b2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1848,7 +1848,6 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask);
 int phy_unregister_fixup_for_id(const char *bus_id);
 int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
 
-int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_eee_clk_stop_enable(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data);
-- 
2.40.0

[RFC/RFTv3 19/24] net: usb: lan78xx: Fixup EEE

[Andrew Lunn]

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

lan78xx_set_eee() now just programs the hardware with the LTI
timer value, and passed everything else to phylib, so it can correctly
setup the PHY.

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

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v3: Fixup compiler error, missing read in read/modify/write.
---
 drivers/net/usb/lan78xx.c | 42 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index c458c030fadf..729183e57080 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1690,17 +1690,10 @@ static int lan78xx_get_eee(struct net_device *net, struct ethtool_eee *edata)
 
 	ret = lan78xx_read_reg(dev, MAC_CR, &buf);
 	if (buf & MAC_CR_EEE_EN_) {
-		edata->eee_enabled = true;
-		edata->eee_active = !!(edata->advertised &
-				       edata->lp_advertised);
-		edata->tx_lpi_enabled = true;
 		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
 		ret = lan78xx_read_reg(dev, EEE_TX_LPI_REQ_DLY, &buf);
 		edata->tx_lpi_timer = buf;
 	} else {
-		edata->eee_enabled = false;
-		edata->eee_active = false;
-		edata->tx_lpi_enabled = false;
 		edata->tx_lpi_timer = 0;
 	}
 
@@ -1721,24 +1714,16 @@ static int lan78xx_set_eee(struct net_device *net, struct ethtool_eee *edata)
 	if (ret < 0)
 		return ret;
 
-	if (edata->eee_enabled) {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf |= MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-
-		phy_ethtool_set_eee(net->phydev, edata);
-
-		buf = (u32)edata->tx_lpi_timer;
-		ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
-	} else {
-		ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-		buf &= ~MAC_CR_EEE_EN_;
-		ret = lan78xx_write_reg(dev, MAC_CR, buf);
-	}
+	ret = phy_ethtool_set_eee(net->phydev, edata);
+	if (ret < 0)
+		goto out;
 
+	buf = (u32)edata->tx_lpi_timer;
+	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
+out:
 	usb_autopm_put_interface(dev->intf);
 
-	return 0;
+	return ret;
 }
 
 static u32 lan78xx_get_link(struct net_device *net)
@@ -2114,7 +2099,20 @@ static void lan78xx_remove_mdio(struct lan78xx_net *dev)
 
 static void lan78xx_link_status_change(struct net_device *net)
 {
+	struct lan78xx_net *dev = netdev_priv(net);
 	struct phy_device *phydev = net->phydev;
+	u32 data;
+	int ret;
+
+	ret = lan78xx_read_reg(dev, MAC_CR, &data);
+	if (ret < 0)
+		return;
+
+	if (phydev->eee_active)
+		data |=  MAC_CR_EEE_EN_;
+	else
+		data &= ~MAC_CR_EEE_EN_;
+	lan78xx_write_reg(dev, MAC_CR, data);
 
 	phy_print_status(phydev);
 }
-- 
2.40.0

[RFC/RFTv3 20/24] net: phy: Add phy_support_eee() indicating MAC support EEE

[Andrew Lunn]

In order for EEE to operate, both the MAC and the PHY need to support
it, similar to how pause works. 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>
---
 drivers/net/phy/phy_device.c | 16 ++++++++++++++++
 include/linux/phy.h          |  3 ++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c0760cbf534b..30f07623637b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2760,6 +2760,22 @@ void phy_advertise_supported(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_advertise_supported);
 
+/**
+ * phy_support_eee - Enable support of EEE
+ * @phydev: target phy_device struct
+ *
+ * Description: Called by the MAC to indicate is supports Energy
+ * Efficient Ethernet. This should be called before phy_start() in
+ * order that EEE is negotiated when the link comes up as part of
+ * phy_start().
+ */
+void phy_support_eee(struct phy_device *phydev)
+{
+	linkmode_copy(phydev->advertising_eee, phydev->supported_eee);
+	phydev->tx_lpi_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 af199fcc52b2..85f66c905fef 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -683,7 +683,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;
@@ -1824,6 +1824,7 @@ void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
 void phy_advertise_supported(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.40.0

Re: [RFC/RFTv3 20/24] net: phy: Add phy_support_eee() indicating MAC support EEE

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> In order for EEE to operate, both the MAC and the PHY need to support
> it, similar to how pause works. 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.

Nit: there is SmartEEE/AutogrEEEn which are specifically designed for 
MACs that do not support EEE, but in spirit, there is still the 
auto-negotiation aspect that needs to be looked at the MAC driver to 
enable SmartEEE/AutogrEEEn at the PHY level.

With that added/reworded a bit:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[RFC/RFTv3 21/24] net: phylink: Add MAC_EEE to mac_capabilites

[Andrew Lunn]

If the MAC supports Energy Efficient Ethernet, it should indicate this
by setting the MAC_EEE bit in the config.mac_capabilities
bitmap. phylink will then enable EEE in the PHY, if it supports it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v3: Move MAC_EEE to BIT(2) and renumber link modes.
---
 drivers/net/phy/phylink.c |  3 +++
 include/linux/phylink.h   | 39 ++++++++++++++++++++++-----------------
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 61e4502086c3..191cafd37e62 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1647,6 +1647,9 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	 */
 	phy_support_asym_pause(phy);
 
+	if (pl->config->mac_capabilities & MAC_EEE)
+		phy_support_eee(phy);
+
 	memset(&config, 0, sizeof(config));
 	linkmode_copy(supported, phy->supported);
 	linkmode_copy(config.advertising, phy->advertising);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 52f29b648853..cb5204fb9106 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -52,26 +52,31 @@ enum {
 	 */
 	MAC_SYM_PAUSE	= BIT(0),
 	MAC_ASYM_PAUSE	= BIT(1),
-	MAC_10HD	= BIT(2),
-	MAC_10FD	= BIT(3),
+
+	/* MAC_EEE indicates that the MAC is Energy Efficient capable
+	 * and that the PHY should negotiate its use, if possible
+	 */
+	MAC_EEE		= BIT(2),
+	MAC_10HD	= BIT(3),
+	MAC_10FD	= BIT(4),
 	MAC_10		= MAC_10HD | MAC_10FD,
-	MAC_100HD	= BIT(4),
-	MAC_100FD	= BIT(5),
+	MAC_100HD	= BIT(5),
+	MAC_100FD	= BIT(6),
 	MAC_100		= MAC_100HD | MAC_100FD,
-	MAC_1000HD	= BIT(6),
-	MAC_1000FD	= BIT(7),
+	MAC_1000HD	= BIT(7),
+	MAC_1000FD	= BIT(8),
 	MAC_1000	= MAC_1000HD | MAC_1000FD,
-	MAC_2500FD	= BIT(8),
-	MAC_5000FD	= BIT(9),
-	MAC_10000FD	= BIT(10),
-	MAC_20000FD	= BIT(11),
-	MAC_25000FD	= BIT(12),
-	MAC_40000FD	= BIT(13),
-	MAC_50000FD	= BIT(14),
-	MAC_56000FD	= BIT(15),
-	MAC_100000FD	= BIT(16),
-	MAC_200000FD	= BIT(17),
-	MAC_400000FD	= BIT(18),
+	MAC_2500FD	= BIT(9),
+	MAC_5000FD	= BIT(10),
+	MAC_10000FD	= BIT(11),
+	MAC_20000FD	= BIT(12),
+	MAC_25000FD	= BIT(13),
+	MAC_40000FD	= BIT(14),
+	MAC_50000FD	= BIT(15),
+	MAC_56000FD	= BIT(16),
+	MAC_100000FD	= BIT(17),
+	MAC_200000FD	= BIT(18),
+	MAC_400000FD	= BIT(19),
 };
 
 static inline bool phylink_autoneg_inband(unsigned int mode)
-- 
2.40.0

Re: [RFC/RFTv3 21/24] net: phylink: Add MAC_EEE to mac_capabilites

[Florian Fainelli]

On 3/30/23 17:55, Andrew Lunn wrote:
> If the MAC supports Energy Efficient Ethernet, it should indicate this
> by setting the MAC_EEE bit in the config.mac_capabilities
> bitmap. phylink will then enable EEE in the PHY, if it supports it.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[RFC/RFTv3 22/24] net: phylink: Extend mac_capabilities in MAC drivers which support EEE

[Andrew Lunn]

For MAC drivers making use of phylink, and which support EEE, set the
MAC_EEE bit in the mac_capabilities.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/marvell/mvneta.c             | 2 +-
 drivers/net/ethernet/microchip/lan743x_main.c     | 2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 net/dsa/port.c                                    | 3 +++
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index fba9edad9e37..efe751da899b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5450,7 +5450,7 @@ static int mvneta_probe(struct platform_device *pdev)
 
 	pp->phylink_config.dev = &dev->dev;
 	pp->phylink_config.type = PHYLINK_NETDEV;
-	pp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 |
+	pp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_EEE | MAC_10 |
 		MAC_100 | MAC_1000FD | MAC_2500FD;
 
 	phy_interface_set_rgmii(pp->phylink_config.supported_interfaces);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 7986f8fcf7d3..ad76be484536 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1543,6 +1543,8 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
 	phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
 	phy->fc_autoneg = phydev->autoneg;
 
+	phy_support_eee(phydev);
+
 	phy_start(phydev);
 	phy_start_aneg(phydev);
 	phy_attached_info(phydev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 190b74d7f4e7..522e6bb9e1fd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1232,6 +1232,9 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	if (priv->dma_cap.eee)
+		priv->phylink_config.mac_capabilities |= MAC_EEE;
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 67ad1adec2a2..68e52abadd7e 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1696,6 +1696,9 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 	if (ds->ops->phylink_get_caps)
 		ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config);
 
+	if (ds->ops->set_mac_eee && ds->ops->get_mac_eee)
+		dp->pl_config.mac_capabilities |= MAC_EEE;
+
 	pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn),
 			    mode, &dsa_port_phylink_mac_ops);
 	if (IS_ERR(pl)) {
-- 
2.40.0

[RFC/RFTv3 23/24] net: phylib: call phy_support_eee() in MAC drivers which support EEE

[Andrew Lunn]

For MAC drivers making use of phylib, and which support EEE, call
phy_support_eee() before starting the PHY.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c    | 2 ++
 drivers/net/ethernet/freescale/fec_main.c       | 3 +++
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 3 +++
 drivers/net/usb/lan78xx.c                       | 2 ++
 4 files changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 6c39839762a7..4175042865ec 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -376,6 +376,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
 		}
 	}
 
+	phy_support_eee(dev->phydev);
+
 	/* Configure port multiplexer based on what the probed PHY device since
 	 * reading the 'max-speed' property determines the maximum supported
 	 * PHY speed which is needed for bcmgenet_mii_config() to configure
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index fda1f9ff32b9..09c55430c614 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2348,6 +2348,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;
 
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index cf549a524674..060280c4bc0a 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -286,6 +286,9 @@ static int sxgbe_init_phy(struct net_device *ndev)
 		return -ENODEV;
 	}
 
+	if (priv->hw_cap.eee)
+		phy_support_eee(phydev);
+
 	netdev_dbg(ndev, "%s: attached to PHY (UID 0x%x) Link = %d\n",
 		   __func__, phydev->phy_id, phydev->link);
 
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 729183e57080..fcf367ad9b0b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2406,6 +2406,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	mii_adv_to_linkmode_adv_t(fc, mii_adv);
 	linkmode_or(phydev->advertising, fc, phydev->advertising);
 
+	phy_support_eee(phydev);
+
 	if (phydev->mdio.dev.of_node) {
 		u32 reg;
 		int len;
-- 
2.40.0

[RFC/RFTv3 24/24] net: phy: Disable EEE advertisement by default

[Andrew Lunn]

EEE should only be advertised if the MAC supports it. Clear
advertising_eee by default. If the MAC indicates it supports EEE by
calling phy_support_eee() advertising_eee will be set to
supported_eee. When the PHY is started, EEE registers will then be
configured.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy_device.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 30f07623637b..0a5936a81ee6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3153,24 +3153,11 @@ static int phy_probe(struct device *dev)
 	of_set_phy_supported(phydev);
 	phy_advertise_supported(phydev);
 
-	/* Get PHY default EEE advertising modes and handle them as potentially
-	 * safe initial configuration.
+	/* Clear EEE advertising until the MAC indicates it also
+	 * supports EEE.
 	 */
-	err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
-	if (err)
-		goto out;
-
-	/* There is no "enabled" flag. If PHY is advertising, assume it is
-	 * kind of enabled.
-	 */
-	phydev->eee_enabled = !linkmode_empty(phydev->advertising_eee);
-
-	/* Some PHYs may advertise, by default, not support EEE modes. So,
-	 * we need to clean them.
-	 */
-	if (phydev->eee_enabled)
-		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
-			     phydev->advertising_eee);
+	linkmode_zero(phydev->advertising_eee);
+	phydev->eee_enabled = false;
 
 	/* Get the EEE modes we want to prohibit. We will ask
 	 * the PHY stop advertising these mode later on
-- 
2.40.0

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Oleksij Rempel]

Hi Andrew,

On Fri, Mar 31, 2023 at 02:54:54AM +0200, Andrew Lunn wrote:
> 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, or
> phylink mac_link_up callback.
> 
> MAC drivers are now expect to indicate to phylib/phylink if they
> support EEE. If not, no EEE link modes are advertised. If the MAC does
> support EEE, on phy_start()/phylink_start() EEE advertisement is
> configured.
> 
> v3
> --
 
I was able to test some drivers and things seems to work ok so far. Do you
need more tests for a non RFC version?

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: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Andrew Lunn]

On Fri, May 26, 2023 at 10:56:04AM +0200, Oleksij Rempel wrote:
> Hi Andrew,
> 
> On Fri, Mar 31, 2023 at 02:54:54AM +0200, Andrew Lunn wrote:
> > 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, or
> > phylink mac_link_up callback.
> > 
> > MAC drivers are now expect to indicate to phylib/phylink if they
> > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > configured.
> > 
> > v3
> > --
>  
> I was able to test some drivers and things seems to work ok so far. Do you
> need more tests for a non RFC version?

No, i just need time to rebase and post them. Plus check if there are
more drivers which added support for EEE and fix them up. There is a
new Broadcom driver which i think will need work.

Hopefully next week i can do this.

	  Andrew

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Florian Fainelli]

On 5/26/23 05:08, Andrew Lunn wrote:
> On Fri, May 26, 2023 at 10:56:04AM +0200, Oleksij Rempel wrote:
>> Hi Andrew,
>>
>> On Fri, Mar 31, 2023 at 02:54:54AM +0200, Andrew Lunn wrote:
>>> 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, or
>>> phylink mac_link_up callback.
>>>
>>> MAC drivers are now expect to indicate to phylib/phylink if they
>>> support EEE. If not, no EEE link modes are advertised. If the MAC does
>>> support EEE, on phy_start()/phylink_start() EEE advertisement is
>>> configured.
>>>
>>> v3
>>> --
>>   
>> I was able to test some drivers and things seems to work ok so far. Do you
>> need more tests for a non RFC version?
> 
> No, i just need time to rebase and post them. Plus check if there are
> more drivers which added support for EEE and fix them up. There is a
> new Broadcom driver which i think will need work.

The Broadcom ASP driver should have EEE stripped off as we just found 
out that the MAC does not drive the PHY signals properly yet. This 
should allow your series to proceed through, without having to care 
about that particular driver.

Thanks for doing this work Andrew!
-- 
Florian

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Florian Fainelli]

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

Hi Andrew, Russell,

On 3/30/23 17:54, Andrew Lunn wrote:
> 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, or
> phylink mac_link_up callback.
> 
> MAC drivers are now expect to indicate to phylib/phylink if they
> support EEE. If not, no EEE link modes are advertised. If the MAC does
> support EEE, on phy_start()/phylink_start() EEE advertisement is
> configured.

Thanks for doing this work, because it really is a happy mess out there. 
A few questions as I have been using mvneta as the reference for fixing 
GENET and its shortcomings.

In your new patches the decision to enable EEE is purely based upon the 
eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what 
mvneta useed to do.

Russell, is there an use case for having eee_enabled while not having 
tx_lpi_enabled?

With the candidate patch attached, I have the following behavior on boot 
with no specific ethtool operation:

# ethtool --show-eee eth0
EEE Settings for eth0:
         EEE status: enabled - active
         Tx LPI: disabled
         Supported EEE link modes:  100baseT/Full
                                    1000baseT/Full
         Advertised EEE link modes:  100baseT/Full
                                     1000baseT/Full
         Link partner advertised EEE link modes:  100baseT/Full
                                                  1000baseT/Full
#

however EEE is not *really* enabled at the hardware level unless I do 
another:

# ethtool --set-eee eth0 tx-lpi on
# ethtool --show-eee eth0
EEE Settings for eth0:
         EEE status: enabled - active
         Tx LPI: 34 (us)
         Supported EEE link modes:  100baseT/Full
                                    1000baseT/Full
         Advertised EEE link modes:  100baseT/Full
                                     1000baseT/Full
         Link partner advertised EEE link modes:  100baseT/Full
                                                  1000baseT/Full

We have a global EEE enable bit which is described as "If set, the TX 
LPI policy control engine is enabled and the MAC inserts LPI_idle codes 
if the link is idle", so it would seem to me that we should require 
users to set both "eee on" and "tx-lpi on" in their ethtool command, but 
then unless we intentionally override tx_lpi_enabled in the driver upon 
probe, there will be any automagical configuration happening, and no 
power savings being realized.

Do you have any recommendations on what drivers should do? As you could 
see from the need of this patch series, we already all created our own 
little schisms of the cargo cult.

Thanks!
-- 
Florian

[-- Attachment #2: 0001-net-bcmgenet-Fix-EEE-implementation.patch --]
[-- Type: text/x-patch, Size: 4226 bytes --]

From ff5ed48c008e052a3005e81001748058045d09d2 Mon Sep 17 00:00:00 2001
From: Florian Fainelli <florian.fainelli@broadcom.com>
Date: Tue, 30 May 2023 11:07:57 -0700
Subject: [PATCH] net: bcmgenet: Fix EEE implementation

We had a number of short comings:

- EEE must be re-evaluated whenever the state machine detects a link
change as wight be switching from a link partner with EEE
enabled/disabled

- We do not need to forcibly enable EEE upon system resume, as the PHY
state machine will trigger a link event that will do that, too

- EEE shall be enabled if both eee_active and tx_lpi_enabled are set

refs #SWLINUX-6655

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
Change-Id: Ie9c3ff63458f08f00b0d19625205396c7a3d5306
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 18 ++++++------------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  2 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  4 ++++
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9d770b76f9bb..f31930215099 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1291,7 +1291,7 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
-static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
+void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
@@ -1347,6 +1347,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e)
 
 	e->eee_enabled = p->eee_enabled;
 	e->eee_active = p->eee_active;
+	e->tx_lpi_enabled = p->tx_lpi_enabled;
 	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
 
 	return phy_ethtool_get_eee(dev->phydev, e);
@@ -1356,7 +1357,6 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct ethtool_eee *p = &priv->eee;
-	int ret = 0;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1369,14 +1369,11 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e)
 	if (!p->eee_enabled) {
 		bcmgenet_eee_enable_set(dev, false);
 	} else {
-		ret = phy_init_eee(dev->phydev, 0);
-		if (ret) {
-			netif_err(priv, hw, dev, "EEE initialization failed\n");
-			return ret;
-		}
-
+		p->eee_active = phy_init_eee(dev->phydev, 0) >= 0;
+		p->tx_lpi_enabled = e->tx_lpi_enabled;
 		bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
-		bcmgenet_eee_enable_set(dev, true);
+		bcmgenet_eee_enable_set(dev,
+					p->eee_active && p->tx_lpi_enabled);
 	}
 
 	return phy_ethtool_set_eee(dev->phydev, e);
@@ -4310,9 +4307,6 @@ static int bcmgenet_resume(struct device *d)
 	if (!device_may_wakeup(d))
 		phy_resume(dev->phydev);
 
-	if (priv->eee.eee_enabled)
-		bcmgenet_eee_enable_set(dev, true);
-
 	bcmgenet_netif_start(dev);
 
 	netif_device_attach(dev);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 47b8326c2f39..ce6b9f29f3d8 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -758,4 +758,6 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode);
 
+void bcmgenet_eee_enable_set(struct net_device *dev, bool enable);
+
 #endif /* __BCMGENET_H__ */
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 613e40202915..90cdd54f8a8d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -108,6 +108,10 @@ static void bcmgenet_mac_config(struct net_device *dev)
 		reg |= CMD_TX_EN | CMD_RX_EN;
 	}
 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+
+	priv->eee.eee_active = phy_init_eee(phydev, 0) >= 0;
+	bcmgenet_eee_enable_set(dev, priv->eee.eee_active &&
+				priv->eee.tx_lpi_enabled);
 }
 
 /* setup netdev link state when PHY link status change and
-- 
2.25.1

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Russell King (Oracle)]

On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
> Hi Andrew, Russell,
> 
> On 3/30/23 17:54, Andrew Lunn wrote:
> > 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, or
> > phylink mac_link_up callback.
> > 
> > MAC drivers are now expect to indicate to phylib/phylink if they
> > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > configured.
> 
> Thanks for doing this work, because it really is a happy mess out there. A
> few questions as I have been using mvneta as the reference for fixing GENET
> and its shortcomings.
> 
> In your new patches the decision to enable EEE is purely based upon the
> eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
> useed to do.
> 
> Russell, is there an use case for having eee_enabled while not having
> tx_lpi_enabled?

As I've been stating recently, LPI is entirely to do with the MAC, so
the LPI delay and the enable boolean are about controlling the MAC end
of things.

I've never really understood what "eee_enabled" is supposed to be doing,
since there's nowhere really to enable or disable EEE per-se. Through
recent patches, phylib has since defined "eee_enabled" to mean that the
advertisement is non-empty, which came as a surprise to me, but again
seems rather redundant and strange.

It's strange because as far as I can see from what documentation there
is, "eee_enabled" is supposed to be a control that users can set
independently of the advertisement, but with the phylib implementation,
eee_enabled read from get_eee() as I say is defined as whether the
advertisement is non-zero or not.

With fibre setups, there is no EEE advertisement, so in that situation
phylib's interpretation of eee_enabled via get_eee/set_eee would be
very much incorrect. The only control one has is whether LPI is enabled
and its timer setting.

That said, the current mvneta implementation doesn't actually enable
LPI for fibre... but there is a bug in that one can get the MAC to
enable LPI via ethtool's set_eee() !

I think for a fibre setup, eee_enabled && tx_lpi_enabled is reasonable,
given that eee_enabled is a seperate control from the advertisement
which will be zero in that case.

Going back to phylib, given this, things get even more "fun" if you have
a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
advertisement with a dual-media PHY that would only affect the copper
side, and EEE may still be possible in the fibre side... which makes
phylib's new interpretation of "eee_enabled" rather odd.

In any case, "eee_enabled" I don't think has much meaning for the fibre
case because there's definitely no control beyond what "tx_lpi_enabled"
already offers.

I think this is a classic case where the EEE interface has been designed
solely around copper without checking what the situation is for fibre!

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

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Russell King (Oracle)]

On Tue, May 30, 2023 at 08:35:55PM +0100, Russell King (Oracle) wrote:
> Going back to phylib, given this, things get even more "fun" if you have
> a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
> a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
> advertisement with a dual-media PHY that would only affect the copper
> side, and EEE may still be possible in the fibre side... which makes
> phylib's new interpretation of "eee_enabled" rather odd.
> 
> In any case, "eee_enabled" I don't think has much meaning for the fibre
> case because there's definitely no control beyond what "tx_lpi_enabled"
> already offers.
> 
> I think this is a classic case where the EEE interface has been designed
> solely around copper without checking what the situation is for fibre!

Let me be a bit more explicit on this. If one does (e.g.) this:

# ethtool --set-eee eth0 advertise 0 tx-lpi on tx-timer 100

with a dual-media PHY, if the MAC is programmed to enable LPI, the
dual-media PHY is linked via fibre, and the remote end supports fibre
EEE, phylib will force "eee" to "off" purely on the grounds that the
advertisement was empty.

If one looks at the man page for ethtool, it says:

           eee on|off
                  Enables/disables the device support of EEE.

What does that mean, exactly, and how is it different from:

           tx-lpi on|off
                  Determines whether the device should assert its Tx LPI.

since the only control at the MAC is whether LPI can be asserted or
not and what the timer is.

The only control at the PHY end of things is what the advertisement
is, if an advertisement even exists for the media type in use.

So, honestly, I don't get what this ethtool interface actually intends
the "eee_enabled" control to do.

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

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Oleksij Rempel]

Hi Russell,

On Tue, May 30, 2023 at 08:49:50PM +0100, Russell King (Oracle) wrote:
> On Tue, May 30, 2023 at 08:35:55PM +0100, Russell King (Oracle) wrote:
> > Going back to phylib, given this, things get even more "fun" if you have
> > a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
> > a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
> > advertisement with a dual-media PHY that would only affect the copper
> > side, and EEE may still be possible in the fibre side... which makes
> > phylib's new interpretation of "eee_enabled" rather odd.
> > 
> > In any case, "eee_enabled" I don't think has much meaning for the fibre
> > case because there's definitely no control beyond what "tx_lpi_enabled"
> > already offers.
> > 
> > I think this is a classic case where the EEE interface has been designed
> > solely around copper without checking what the situation is for fibre!
> 
> Let me be a bit more explicit on this. If one does (e.g.) this:
> 
> # ethtool --set-eee eth0 advertise 0 tx-lpi on tx-timer 100
> 
> with a dual-media PHY, if the MAC is programmed to enable LPI, the
> dual-media PHY is linked via fibre, and the remote end supports fibre
> EEE, phylib will force "eee" to "off" purely on the grounds that the
> advertisement was empty.
> 
> If one looks at the man page for ethtool, it says:
> 
>            eee on|off
>                   Enables/disables the device support of EEE.
> 
> What does that mean, exactly, and how is it different from:
> 
>            tx-lpi on|off
>                   Determines whether the device should assert its Tx LPI.
> 
> since the only control at the MAC is whether LPI can be asserted or
> not and what the timer is.
> 
> The only control at the PHY end of things is what the advertisement
> is, if an advertisement even exists for the media type in use.
> 
> So, honestly, I don't get what this ethtool interface actually intends
> the "eee_enabled" control to do.

Thank you for your insightful observations on the EEE interface and its
related complexities, particularly in the case of fiber interfaces.

Your comments regarding the functionality of eee_enabled and
tx_lpi_enabled commands have sparked a good amount of thought on the
topic. Based on my understanding and observations, I've put together a
table that outlines the interactions between these commands, and their
influence on the MAC LPI status, PHY EEE advertisement, and the overall
EEE status on the link level.

For Copper assuming link partner advertise EEE as well:
+------+--------+------------+----------------+--------------------------------+---------------------------------+
| eee  | tx-lpi | advertise  | MAC LPI Status | PHY EEE Advertisement Status  | EEE Status on Link Level        |
+------+--------+------------+----------------+--------------------------------+---------------------------------+
| on   | on     |   !=0      | Enabled        | Advertise EEE for supported   | EEE enabled for supported       |
|      |        |            |                | speeds                        | speeds (Full EEE operation)     |
| on   | off    |   !=0      | Disabled       | Advertise EEE for supported   | EEE enabled for RX, disabled    |
|      |        |            |                | speeds                        | for TX (Partial EEE operation)  |
| off  | on     |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
| off  | off    |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
| on   | on     |    0       | Enabled        | No EEE advertisement          | EEE TX enabled, RX depends on   |
|      |        |            |                |                               | link partner                    |
| on   | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
| off  | on     |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
| off  | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
+------+--------+------------+----------------+--------------------------------+---------------------------------+

For Fiber:
+-----------+-----------+-----------------+---------------------+-------------------------+
|     eee   |   tx-lpi  | PHY EEE Adv.    | MAC LPI Status      | EEE Status on Link Level|
+-----------+-----------+-----------------+---------------------+-------------------------+
|     on    |     on    |         NA      | Enabled             | EEE supported           |
|     on    |     off   |         NA      | Disabled            | EEE not supported       |
|     off   |     on    |         NA      | Disabled            | EEE not supported       |
|     off   |     off   |         NA      | Disabled            | EEE not supported       |
+-----------+-----------+-----------------+---------------------+-------------------------+

In my perspective, eee_enabled serves as a global administrative control for
all EEE properties, including PHY EEE advertisement and MAC LPI status. When
EEE is turned off (eee_enabled = off), both PHY EEE advertisement and MAC LPI
status should be disabled, regardless of the tx_lpi_enabled setting.

On the other hand, advertise retains the EEE advertisement configuration, even
when EEE is turned off. This way, users can temporarily disable EEE without
losing their specific advertisement settings, which can then be reinstated when
EEE is turned back on.

In the context of fiber interfaces, where there is no concept of advertisement,
the eee and tx-lpi commands may appear redundant. However, maintaining both
commands could offer consistency across different media types in the ethtool
interface.

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: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Russell King (Oracle)]

On Wed, May 31, 2023 at 09:14:19AM +0200, Oleksij Rempel wrote:
> Hi Russell,
> 
> On Tue, May 30, 2023 at 08:49:50PM +0100, Russell King (Oracle) wrote:
> > On Tue, May 30, 2023 at 08:35:55PM +0100, Russell King (Oracle) wrote:
> > > Going back to phylib, given this, things get even more "fun" if you have
> > > a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
> > > a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
> > > advertisement with a dual-media PHY that would only affect the copper
> > > side, and EEE may still be possible in the fibre side... which makes
> > > phylib's new interpretation of "eee_enabled" rather odd.
> > > 
> > > In any case, "eee_enabled" I don't think has much meaning for the fibre
> > > case because there's definitely no control beyond what "tx_lpi_enabled"
> > > already offers.
> > > 
> > > I think this is a classic case where the EEE interface has been designed
> > > solely around copper without checking what the situation is for fibre!
> > 
> > Let me be a bit more explicit on this. If one does (e.g.) this:
> > 
> > # ethtool --set-eee eth0 advertise 0 tx-lpi on tx-timer 100
> > 
> > with a dual-media PHY, if the MAC is programmed to enable LPI, the
> > dual-media PHY is linked via fibre, and the remote end supports fibre
> > EEE, phylib will force "eee" to "off" purely on the grounds that the
> > advertisement was empty.
> > 
> > If one looks at the man page for ethtool, it says:
> > 
> >            eee on|off
> >                   Enables/disables the device support of EEE.
> > 
> > What does that mean, exactly, and how is it different from:
> > 
> >            tx-lpi on|off
> >                   Determines whether the device should assert its Tx LPI.
> > 
> > since the only control at the MAC is whether LPI can be asserted or
> > not and what the timer is.
> > 
> > The only control at the PHY end of things is what the advertisement
> > is, if an advertisement even exists for the media type in use.
> > 
> > So, honestly, I don't get what this ethtool interface actually intends
> > the "eee_enabled" control to do.
> 
> Thank you for your insightful observations on the EEE interface and its
> related complexities, particularly in the case of fiber interfaces.
> 
> Your comments regarding the functionality of eee_enabled and
> tx_lpi_enabled commands have sparked a good amount of thought on the
> topic. Based on my understanding and observations, I've put together a
> table that outlines the interactions between these commands, and their
> influence on the MAC LPI status, PHY EEE advertisement, and the overall
> EEE status on the link level.
> 
> For Copper assuming link partner advertise EEE as well:
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> | eee  | tx-lpi | advertise  | MAC LPI Status | PHY EEE Advertisement Status  | EEE Status on Link Level        |
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> | on   | on     |   !=0      | Enabled        | Advertise EEE for supported   | EEE enabled for supported       |
> |      |        |            |                | speeds                        | speeds (Full EEE operation)     |
> | on   | off    |   !=0      | Disabled       | Advertise EEE for supported   | EEE enabled for RX, disabled    |
> |      |        |            |                | speeds                        | for TX (Partial EEE operation)  |
> | off  | on     |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | off    |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
> | on   | on     |    0       | Enabled        | No EEE advertisement          | EEE TX enabled, RX depends on   |
> |      |        |            |                |                               | link partner                    |
> | on   | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | on     |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> 
> For Fiber:
> +-----------+-----------+-----------------+---------------------+-------------------------+
> |     eee   |   tx-lpi  | PHY EEE Adv.    | MAC LPI Status      | EEE Status on Link Level|
> +-----------+-----------+-----------------+---------------------+-------------------------+
> |     on    |     on    |         NA      | Enabled             | EEE supported           |
> |     on    |     off   |         NA      | Disabled            | EEE not supported       |
> |     off   |     on    |         NA      | Disabled            | EEE not supported       |
> |     off   |     off   |         NA      | Disabled            | EEE not supported       |
> +-----------+-----------+-----------------+---------------------+-------------------------+
> 
> In my perspective, eee_enabled serves as a global administrative control for
> all EEE properties, including PHY EEE advertisement and MAC LPI status. When
> EEE is turned off (eee_enabled = off), both PHY EEE advertisement and MAC LPI
> status should be disabled, regardless of the tx_lpi_enabled setting.
> 
> On the other hand, advertise retains the EEE advertisement configuration, even
> when EEE is turned off. This way, users can temporarily disable EEE without
> losing their specific advertisement settings, which can then be reinstated when
> EEE is turned back on.

I agree. I've found phylib's interpretation to be weird, particularly
that the advertisement settings are lost if one sets eee_enabled to be
false. That alone makes eee_enabled entirely redundant.

To me, sane behaviour is exactly as you describe - if the user sets
eee_enabled false, but sets an advertisement, the kernel should store
the advertisement setting so it can be retrieved via get_eee(), ready
for use when eee_enabled is subsequently set true.

> In the context of fiber interfaces, where there is no concept of advertisement,
> the eee and tx-lpi commands may appear redundant. However, maintaining both
> commands could offer consistency across different media types in the ethtool
> interface.

Yes, because having a consistent behaviour is important for a user API.
Having a user API randomly change behaviour depending on what's behind
it leads to user confusion.

Consider the case of a dual-media PHY - should the behaviour of the
EEE setter/getter change depending on what media is in use? Clearly
not, since one normally configures the EEE _policy_ when configuring
the network device, rather than each time the link comes up.

Thanks for your input. I now have some patches that add:

1. generic EEE configuration helpers, which I hope phylib and phylink
   can both use for consistent behaviour.
2. phylink gaining infrastructure for EEE management both of existing
   phylib and also fibre setups.
3. mvneta converted to use phylink's implementation. I may also do
   mvpp2 as well as I have patches for EEE support there as well.

I hope to post the patches later today.

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

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Andrew Lunn]

On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
> Hi Andrew, Russell,
> 
> On 3/30/23 17:54, Andrew Lunn wrote:
> > 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, or
> > phylink mac_link_up callback.
> > 
> > MAC drivers are now expect to indicate to phylib/phylink if they
> > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > configured.
> 
> Thanks for doing this work, because it really is a happy mess out there. A
> few questions as I have been using mvneta as the reference for fixing GENET
> and its shortcomings.
> 
> In your new patches the decision to enable EEE is purely based upon the
> eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
> useed to do.

I don't really care much what we decide means 'enabled'. I just want
it moved out of MAC drivers and into the core so it is consistent.

Russel, if you want to propose something which works for both Copper
and Fibre, i'm happy to implement it. But as you pointed out, we need
to decide where. Maybe phylib handles copper, and phylink is layered
on top and handles fibre?

	  Andrew

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Florian Fainelli]

On 5/30/23 12:48, Andrew Lunn wrote:
> On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
>> Hi Andrew, Russell,
>>
>> On 3/30/23 17:54, Andrew Lunn wrote:
>>> 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, or
>>> phylink mac_link_up callback.
>>>
>>> MAC drivers are now expect to indicate to phylib/phylink if they
>>> support EEE. If not, no EEE link modes are advertised. If the MAC does
>>> support EEE, on phy_start()/phylink_start() EEE advertisement is
>>> configured.
>>
>> Thanks for doing this work, because it really is a happy mess out there. A
>> few questions as I have been using mvneta as the reference for fixing GENET
>> and its shortcomings.
>>
>> In your new patches the decision to enable EEE is purely based upon the
>> eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
>> useed to do.
> 
> I don't really care much what we decide means 'enabled'. I just want
> it moved out of MAC drivers and into the core so it is consistent.

Understood this is slightly out of the scope of what you are doing which 
is to have an unified behavior, but we also have a shot at defining a 
correct behavior.

> 
> Russel, if you want to propose something which works for both Copper
> and Fibre, i'm happy to implement it. But as you pointed out, we need
> to decide where. Maybe phylib handles copper, and phylink is layered
> on top and handles fibre?
> 
> 	  Andrew

-- 
Florian

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Russell King (Oracle)]

On Tue, May 30, 2023 at 01:03:15PM -0700, Florian Fainelli wrote:
> On 5/30/23 12:48, Andrew Lunn wrote:
> > I don't really care much what we decide means 'enabled'. I just want
> > it moved out of MAC drivers and into the core so it is consistent.
> 
> Understood this is slightly out of the scope of what you are doing which is
> to have an unified behavior, but we also have a shot at defining a correct
> behavior.

Yes, having unified behaviour is good only if we don't end up boxing
ourselves into a corner when trying to unify it. That said, I suspect
many are just broken in one way or another.

What always makes me rather nervous is that trying to fix this kind of
thing will inevitably change the user visible behaviour, and then we
hope that that doesn't cause people's setups to regress.

So, IMHO, if we can decide what the correct behaviour and use should
be before we unify it, the better chance we stand at not having
differing behaviours from future kernel versions as we end up revising
the unified behaviour.

I hope that makes sense! :)

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

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Russell King (Oracle)]

On Tue, May 30, 2023 at 09:48:00PM +0200, Andrew Lunn wrote:
> On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
> > Hi Andrew, Russell,
> > 
> > On 3/30/23 17:54, Andrew Lunn wrote:
> > > 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, or
> > > phylink mac_link_up callback.
> > > 
> > > MAC drivers are now expect to indicate to phylib/phylink if they
> > > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > > configured.
> > 
> > Thanks for doing this work, because it really is a happy mess out there. A
> > few questions as I have been using mvneta as the reference for fixing GENET
> > and its shortcomings.
> > 
> > In your new patches the decision to enable EEE is purely based upon the
> > eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
> > useed to do.
> 
> I don't really care much what we decide means 'enabled'. I just want
> it moved out of MAC drivers and into the core so it is consistent.
> 
> Russel, if you want to propose something which works for both Copper
> and Fibre, i'm happy to implement it. But as you pointed out, we need
> to decide where. Maybe phylib handles copper, and phylink is layered
> on top and handles fibre?

Phylib also handles fibre too with dual-media PHYs (such as 88E151x
and 88X3310), and as I've just pointed out, the recent attempts at
"fixing" phylib's handling particularly with eee_enabled have made it
rather odd.

That said, the 88E151x resolution of 1000BASE-X negotiation is also
rather odd, particularly with pause modes. So I don't trust one bit
that anyone is even using 88E151x in fibre setups - or if they are
they don't care about this odd behaviour.

Before we go any further, I think we need to hammer out eactly how the
ethtool EEE interface is supposed to work, because right now I can't
say that I fully understand it - and as I've said in my replies to
Florian recently, phylib's EEE implementation becomes utterly silly
when it comes to fibre.

In particular, we need to hammer out what the difference exactly is
between "eee_enabled" and "tx_lpi_enabled", and what they control,
and I suggest we look at it from the point of view of both copper
(where EEE is negotiated) and fibre (were EEE is optional, no
capability bits, no negotiation, so no advertisement.)

It seems fairly obvious to me that tx_lpi* are about the MAC
configuration, since that's the entity which is responsible for
signalling LPI towards the PHY(PCS) over GMII.

eee_active... what does "active" actually mean? From the API doc, it
means the "Result of the eee negotiation" which is fine for copper
links where EEE is negotiated, but in the case of fibre, there isn't
EEE negotiation, and EEE is optionally implemented in the PCS.

eee_enabled... doesn't seem to have a meaning as far as IEEE 802.3
goes, it's a Linux invention. Documentation says "EEE configured mode"
which is just as useful as a chocolate teapot for making tea, that
comment might as well be deleted for what use it is. To this day, I
have no idea what this setting is actually supposed to be doing.
It seemed sane to me that if eee_enabled is false, then we should
not report eee_active as true, nor should we allow the MAC to
generate LPI. Whether the advertisement gets programmed into the PHY
or not is something I never thought about, and I can't remember
phylib's old behaviour. Modern phylib treats eee_enabled = false to
program a zero advertisement, which means when reading back via
get_eee(), you get a zero advertisement back. Effectively, eee_active
in modern phylib means "allow the advertisement to be programmed
if enabled, otherwise clear the advertisement".

If it's simply there to zero the advertisement, then what if the
media type has no capability for EEE advertisement, but intrinsically
supports EEE. That's where phylib's interpretation falls down IMHO.

Maybe this ethtool interface doesn't work very well for cases where
there is EEE ability but no EEE advertisement? Not sure.

Until we get that settled, we can't begin to fathom how phylib (or
phylink) should make a decision as to whether the MAC should signal
LPI towards the media or not.

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

Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

[Florian Fainelli]

On 5/30/23 13:28, Russell King (Oracle) wrote:
> On Tue, May 30, 2023 at 09:48:00PM +0200, Andrew Lunn wrote:
>> On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
>>> Hi Andrew, Russell,
>>>
>>> On 3/30/23 17:54, Andrew Lunn wrote:
>>>> 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, or
>>>> phylink mac_link_up callback.
>>>>
>>>> MAC drivers are now expect to indicate to phylib/phylink if they
>>>> support EEE. If not, no EEE link modes are advertised. If the MAC does
>>>> support EEE, on phy_start()/phylink_start() EEE advertisement is
>>>> configured.
>>>
>>> Thanks for doing this work, because it really is a happy mess out there. A
>>> few questions as I have been using mvneta as the reference for fixing GENET
>>> and its shortcomings.
>>>
>>> In your new patches the decision to enable EEE is purely based upon the
>>> eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
>>> useed to do.
>>
>> I don't really care much what we decide means 'enabled'. I just want
>> it moved out of MAC drivers and into the core so it is consistent.
>>
>> Russel, if you want to propose something which works for both Copper
>> and Fibre, i'm happy to implement it. But as you pointed out, we need
>> to decide where. Maybe phylib handles copper, and phylink is layered
>> on top and handles fibre?
> 
> Phylib also handles fibre too with dual-media PHYs (such as 88E151x
> and 88X3310), and as I've just pointed out, the recent attempts at
> "fixing" phylib's handling particularly with eee_enabled have made it
> rather odd.
> 
> That said, the 88E151x resolution of 1000BASE-X negotiation is also
> rather odd, particularly with pause modes. So I don't trust one bit
> that anyone is even using 88E151x in fibre setups - or if they are
> they don't care about this odd behaviour.
> 
> Before we go any further, I think we need to hammer out eactly how the
> ethtool EEE interface is supposed to work, because right now I can't
> say that I fully understand it - and as I've said in my replies to
> Florian recently, phylib's EEE implementation becomes utterly silly
> when it comes to fibre.
> 
> In particular, we need to hammer out what the difference exactly is
> between "eee_enabled" and "tx_lpi_enabled", and what they control,
> and I suggest we look at it from the point of view of both copper
> (where EEE is negotiated) and fibre (were EEE is optional, no
> capability bits, no negotiation, so no advertisement.)
> 
> It seems fairly obvious to me that tx_lpi* are about the MAC
> configuration, since that's the entity which is responsible for
> signalling LPI towards the PHY(PCS) over GMII.

Yes that much we can agree on that tx_lpi* is from the perspective of 
the MAC.

> 
> eee_active... what does "active" actually mean? From the API doc, it
> means the "Result of the eee negotiation" which is fine for copper
> links where EEE is negotiated, but in the case of fibre, there isn't
> EEE negotiation, and EEE is optionally implemented in the PCS.

Is there any way to feed back whether EEE is actually being used at the 
time on a fiber link? In which case eee_active would reflect that state.

> 
> eee_enabled... doesn't seem to have a meaning as far as IEEE 802.3
> goes, it's a Linux invention. Documentation says "EEE configured mode"
> which is just as useful as a chocolate teapot for making tea, that
> comment might as well be deleted for what use it is. To this day, I
> have no idea what this setting is actually supposed to be doing.
> It seemed sane to me that if eee_enabled is false, then we should
> not report eee_active as true, nor should we allow the MAC to
> generate LPI. Whether the advertisement gets programmed into the PHY
> or not is something I never thought about, and I can't remember
> phylib's old behaviour. Modern phylib treats eee_enabled = false to
> program a zero advertisement, which means when reading back via
> get_eee(), you get a zero advertisement back. Effectively, eee_active
> in modern phylib means "allow the advertisement to be programmed
> if enabled, otherwise clear the advertisement". >
> If it's simply there to zero the advertisement, then what if the
> media type has no capability for EEE advertisement, but intrinsically
> supports EEE. That's where phylib's interpretation falls down IMHO.
> 
> Maybe this ethtool interface doesn't work very well for cases where
> there is EEE ability but no EEE advertisement? Not sure.

At the time it was introduced, there certainly was not much care being 
given to fiber use cases.

> 
> Until we get that settled, we can't begin to fathom how phylib (or
> phylink) should make a decision as to whether the MAC should signal
> LPI towards the media or not.

Now that we have SmartEEE and AutogrEEEn as additional supported modes 
by certain PHY drivers for MACs that lack any LPI signaling capability 
towards the PHY, there may be a reason for re-purposing or clarifying 
the meaning of eee_enabled=true with tx_lpi_enabled=false. Although in 
those cases, I would just issue a warning that tx_lpi_enabled is ignored 
because the MAC is incapable of LPI signaling.

It seems that eee_enabled is intended to be a local administrative 
parameter that provides an additional gating level to the local & remote 
advertisement resolution. As a driver you can only enable EEE at the MAC 
and/or PHY level if local & remote & enabled = true.
-- 
Florian