diff mbox series

[net-next,05/10] net: phylink: add EEE management

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

Checks

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

Commit Message

Russell King (Oracle) Dec. 9, 2024, 2:23 p.m. UTC
Add EEE management to phylink, making use of the phylib implementation.
This will only be used where a MAC driver populates the methods and
capabilities bitfield, otherwise we keep our old behaviour.

Phylink will keep track of the EEE configuration, including the clock
stop abilities at each end of the MAC to PHY link, programming the PHY
appropriately and preserving the EEE configuration should the PHY go
away.

It will also call into the MAC driver when LPI needs to be enabled or
disabled, with the expectation that the MAC have LPI disabled during
probe.

Support for phylink managed EEE is enabled by populating both tx_lpi
MAC operations method pointers.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |  44 ++++++++++++++
 2 files changed, 163 insertions(+), 4 deletions(-)

Comments

Andrew Lunn Dec. 10, 2024, 3:18 a.m. UTC | #1
> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.

How important is this expectation? I don't see it documented anywhere.

	Andrew
Simon Horman Dec. 13, 2024, 9:37 a.m. UTC | #2
On Mon, Dec 09, 2024 at 02:23:38PM +0000, Russell King (Oracle) wrote:
> Add EEE management to phylink, making use of the phylib implementation.
> This will only be used where a MAC driver populates the methods and
> capabilities bitfield, otherwise we keep our old behaviour.
> 
> Phylink will keep track of the EEE configuration, including the clock
> stop abilities at each end of the MAC to PHY link, programming the PHY
> appropriately and preserving the EEE configuration should the PHY go
> away.
> 
> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.
> 
> Support for phylink managed EEE is enabled by populating both tx_lpi
> MAC operations method pointers.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

...

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h

...

> @@ -143,11 +145,17 @@ enum phylink_op_type {
>   *                    possible and avoid stopping it during suspend events.
>   * @default_an_inband: if true, defaults to MLO_AN_INBAND rather than
>   *		       MLO_AN_PHY. A fixed-link specification will override.
> + * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI
>   * @get_fixed_state: callback to execute to determine the fixed link state,
>   *		     if MAC link is at %MLO_AN_FIXED mode.
>   * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
>   *                        are supported by the MAC/PCS.
> + * @lpi_interfaces: bitmap describing which PHY interface modes can support
> + *		    LPI signalling.
>   * @mac_capabilities: MAC pause/speed/duplex capabilities.
> + * @lpi_capabilities: MAC speeds which can support LPI signalling
> + * @eee: default EEE configuration.
> + * @lpi_timer_limit_us: Maximum (inclusive) value of the EEE LPI timer.
>   */
>  struct phylink_config {
>  	struct device *dev;
> @@ -156,10 +164,16 @@ struct phylink_config {
>  	bool mac_managed_pm;
>  	bool mac_requires_rxc;
>  	bool default_an_inband;
> +	bool eee_rx_clk_stop_enable;
>  	void (*get_fixed_state)(struct phylink_config *config,
>  				struct phylink_link_state *state);
>  	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
> +	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
>  	unsigned long mac_capabilities;
> +	unsigned long lpi_capabilities;
> +	u32 lpi_timer_limit_us;
> +	u32 lpi_timer_default;
> +	bool eee_enabled_default;
>  };

Hi Russell,

A minor nit from my side.

The Kernel doc updates don't correspond entirely to the structure updates:
- @eee is documented but not present in the structure
- Conversely, @lpi_timer_default and @ee_enabled_default are
  present in the structure but not documented.

...
Heiner Kallweit Dec. 14, 2024, 11:38 p.m. UTC | #3
On 09.12.2024 15:23, Russell King (Oracle) wrote:
> Add EEE management to phylink, making use of the phylib implementation.
> This will only be used where a MAC driver populates the methods and
> capabilities bitfield, otherwise we keep our old behaviour.
> 
> Phylink will keep track of the EEE configuration, including the clock
> stop abilities at each end of the MAC to PHY link, programming the PHY
> appropriately and preserving the EEE configuration should the PHY go
> away.
> 
> It will also call into the MAC driver when LPI needs to be enabled or
> disabled, with the expectation that the MAC have LPI disabled during
> probe.
> 
> Support for phylink managed EEE is enabled by populating both tx_lpi
> MAC operations method pointers.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 123 ++++++++++++++++++++++++++++++++++++--
>  include/linux/phylink.h   |  44 ++++++++++++++
>  2 files changed, 163 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 03509fdaa1ec..750356b6a2e9 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -81,12 +81,19 @@ struct phylink {
>  	unsigned int pcs_state;
>  
>  	bool link_failed;
> +	bool mac_supports_eee;
> +	bool phy_enable_tx_lpi;
> +	bool mac_enable_tx_lpi;
> +	bool mac_tx_clk_stop;
> +	u32 mac_tx_lpi_timer;
>  
>  	struct sfp_bus *sfp_bus;
>  	bool sfp_may_have_phy;
>  	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
>  	u8 sfp_port;
> +
> +	struct eee_config eee_cfg;
>  };
>  
>  #define phylink_printk(level, pl, fmt, ...) \
> @@ -1563,6 +1570,47 @@ static const char *phylink_pause_to_str(int pause)
>  	}
>  }
>  
> +static void phylink_deactivate_lpi(struct phylink *pl)
> +{
> +	if (pl->mac_enable_tx_lpi) {
> +		pl->mac_enable_tx_lpi = false;
> +
> +		phylink_dbg(pl, "disabling LPI\n");
> +
> +		pl->mac_ops->mac_disable_tx_lpi(pl->config);
> +	}
> +}
> +
> +static void phylink_activate_lpi(struct phylink *pl)
> +{
> +	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
> +		phylink_dbg(pl, "MAC does not support LPI with %s\n",
> +			    phy_modes(pl->cur_interface));
> +		return;
> +	}
> +
> +	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
> +		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
> +
> +	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
> +				       pl->mac_tx_clk_stop);
> +
> +	pl->mac_enable_tx_lpi = true;
> +}
> +
> +static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
> +
> +	/* Convert the MAC's LPI capabilities to linkmodes */
> +	linkmode_zero(eee_supported);
> +	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
> +
> +	/* Mask out EEE modes that are not supported */
> +	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
> +	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
> +}
> +

Something similar we may need in phylib too. An example is cpsw MAC driver which
doesn't support EEE. Issues have been reported if the PHY's on both sides negotiate
EEE, workaround is to use property eee-broken-xxx in the respective DT's to disable
PHY EEE advertisement. I'm thinking of adding a simple phy_disable_eee() which can
be called by MAC drivers to clear EEE supported and advertising bitmaps.

A similar case is enetc (using phylink) which doesn't support EEE. See following in
enetc.c:

/* disable EEE autoneg, until ENETC driver supports it */
memset(&edata, 0, sizeof(struct ethtool_keee));
phylink_ethtool_set_eee(priv->phylink, &edata);

Russell, do you plan to change this driver too, based on phylink extensions?
I think already now, based on the quoted code piece, several (all?) eee-broken-xxx
properties can be removed under arch/arm64/boot/dts/freescale .
diff mbox series

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 03509fdaa1ec..750356b6a2e9 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -81,12 +81,19 @@  struct phylink {
 	unsigned int pcs_state;
 
 	bool link_failed;
+	bool mac_supports_eee;
+	bool phy_enable_tx_lpi;
+	bool mac_enable_tx_lpi;
+	bool mac_tx_clk_stop;
+	u32 mac_tx_lpi_timer;
 
 	struct sfp_bus *sfp_bus;
 	bool sfp_may_have_phy;
 	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
 	u8 sfp_port;
+
+	struct eee_config eee_cfg;
 };
 
 #define phylink_printk(level, pl, fmt, ...) \
@@ -1563,6 +1570,47 @@  static const char *phylink_pause_to_str(int pause)
 	}
 }
 
+static void phylink_deactivate_lpi(struct phylink *pl)
+{
+	if (pl->mac_enable_tx_lpi) {
+		pl->mac_enable_tx_lpi = false;
+
+		phylink_dbg(pl, "disabling LPI\n");
+
+		pl->mac_ops->mac_disable_tx_lpi(pl->config);
+	}
+}
+
+static void phylink_activate_lpi(struct phylink *pl)
+{
+	if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {
+		phylink_dbg(pl, "MAC does not support LPI with %s\n",
+			    phy_modes(pl->cur_interface));
+		return;
+	}
+
+	phylink_dbg(pl, "LPI timer %uus, tx clock stop %u\n",
+		    pl->mac_tx_lpi_timer, pl->mac_tx_clk_stop);
+
+	pl->mac_ops->mac_enable_tx_lpi(pl->config, pl->mac_tx_lpi_timer,
+				       pl->mac_tx_clk_stop);
+
+	pl->mac_enable_tx_lpi = true;
+}
+
+static void phylink_phy_restrict_eee(struct phylink *pl, struct phy_device *phy)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_supported);
+
+	/* Convert the MAC's LPI capabilities to linkmodes */
+	linkmode_zero(eee_supported);
+	phylink_caps_to_linkmodes(eee_supported, pl->config->lpi_capabilities);
+
+	/* Mask out EEE modes that are not supported */
+	linkmode_and(phy->supported_eee, phy->supported_eee, eee_supported);
+	linkmode_and(phy->advertising_eee, phy->advertising_eee, eee_supported);
+}
+
 static void phylink_link_up(struct phylink *pl,
 			    struct phylink_link_state link_state)
 {
@@ -1609,6 +1657,9 @@  static void phylink_link_up(struct phylink *pl,
 				 pl->cur_interface, speed, duplex,
 				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);
 
+	if (pl->mac_supports_eee && pl->phy_enable_tx_lpi)
+		phylink_activate_lpi(pl);
+
 	if (ndev)
 		netif_carrier_on(ndev);
 
@@ -1625,6 +1676,9 @@  static void phylink_link_down(struct phylink *pl)
 
 	if (ndev)
 		netif_carrier_off(ndev);
+
+	phylink_deactivate_lpi(pl);
+
 	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
 				   pl->cur_interface);
 	phylink_info(pl, "Link is Down\n");
@@ -1888,6 +1942,14 @@  struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
+	pl->mac_supports_eee = mac_ops->mac_disable_tx_lpi &&
+			       mac_ops->mac_enable_tx_lpi;
+
+	/* Set the default EEE configuration */
+	pl->eee_cfg.eee_enabled = pl->config->eee_enabled_default;
+	pl->eee_cfg.tx_lpi_enabled = pl->eee_cfg.eee_enabled;
+	pl->eee_cfg.tx_lpi_timer = pl->config->lpi_timer_default;
+
 	pl->phy_state.interface = iface;
 	pl->link_interface = iface;
 	if (iface == PHY_INTERFACE_MODE_MOCA)
@@ -1992,16 +2054,22 @@  static void phylink_phy_change(struct phy_device *phydev, bool up)
 	pl->phy_state.link = up;
 	if (!up)
 		pl->link_failed = true;
+
+	/* Get the LPI state from phylib */
+	pl->phy_enable_tx_lpi = phydev->enable_tx_lpi;
+	pl->mac_tx_lpi_timer = phydev->eee_cfg.tx_lpi_timer;
 	mutex_unlock(&pl->state_mutex);
 
 	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/%slpi\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),
+		    phydev->enable_tx_lpi ? "" : "no");
 }
 
 static int phylink_validate_phy(struct phylink *pl, struct phy_device *phy,
@@ -2131,6 +2199,28 @@  static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 
 	/* Restrict the phy advertisement according to the MAC support. */
 	linkmode_copy(phy->advertising, config.advertising);
+
+	/* If the MAC supports phylink managed EEE, restrict the EEE
+	 * advertisement according to the MAC's LPI capabilities.
+	 */
+	if (pl->mac_supports_eee) {
+		phy->eee_cfg.eee_enabled = pl->eee_cfg.eee_enabled;
+
+		/* If EEE is enabled, then we need to call phy_support_eee()
+		 * to ensure that the advertising mask is appropriately set.
+		 */
+		if (pl->eee_cfg.eee_enabled)
+			phy_support_eee(phy);
+
+		phy->eee_cfg.tx_lpi_enabled = pl->eee_cfg.tx_lpi_enabled;
+		phy->eee_cfg.tx_lpi_timer = pl->eee_cfg.tx_lpi_timer;
+
+		/* Restrict the PHYs EEE support/advertisement to the modes
+		 * that the MAC supports.
+		 */
+		phylink_phy_restrict_eee(pl, phy);
+	}
+
 	mutex_unlock(&pl->state_mutex);
 	mutex_unlock(&phy->lock);
 
@@ -2146,7 +2236,13 @@  static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->config->mac_managed_pm)
 		phy->mac_managed_pm = true;
 
-	return 0;
+	/* Allow the MAC to stop its clock if the PHY has the capability */
+	pl->mac_tx_clk_stop = phy_eee_tx_clock_stop_capable(phy) > 0;
+
+	/* Explicitly configure whether the PHY is allowed to stop it's
+	 * receive clock.
+	 */
+	return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable);
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
@@ -2303,6 +2399,8 @@  void phylink_disconnect_phy(struct phylink *pl)
 		mutex_lock(&phy->lock);
 		mutex_lock(&pl->state_mutex);
 		pl->phydev = NULL;
+		pl->phy_enable_tx_lpi = false;
+		pl->mac_tx_clk_stop = false;
 		mutex_unlock(&pl->state_mutex);
 		mutex_unlock(&phy->lock);
 		flush_work(&pl->resolve);
@@ -3071,12 +3169,29 @@  EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee);
  */
 int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee)
 {
+	bool mac_eee = pl->mac_supports_eee;
 	int ret = -EOPNOTSUPP;
 
 	ASSERT_RTNL();
 
-	if (pl->phydev)
+	phylink_dbg(pl, "mac %s phylink EEE%s, adv %*pbl, LPI%s timer %uus\n",
+		    mac_eee ? "supports" : "does not support",
+		    eee->eee_enabled ? ", enabled" : "",
+		    __ETHTOOL_LINK_MODE_MASK_NBITS, eee->advertised,
+		    eee->tx_lpi_enabled ? " enabled" : "", eee->tx_lpi_timer);
+
+	/* Clamp the LPI timer maximum value */
+	if (mac_eee && eee->tx_lpi_timer > pl->config->lpi_timer_limit_us) {
+		eee->tx_lpi_timer = pl->config->lpi_timer_limit_us;
+		phylink_dbg(pl, "LPI timer limited to %uus\n",
+			    eee->tx_lpi_timer);
+	}
+
+	if (pl->phydev) {
 		ret = phy_ethtool_set_eee(pl->phydev, eee);
+		if (ret == 0)
+			eee_to_eeecfg(&pl->eee_cfg, eee);
+	}
 
 	return ret;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 5462cc6a37dc..f72f0a1feb89 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -5,6 +5,8 @@ 
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
+#include <net/eee.h>
+
 struct device_node;
 struct ethtool_cmd;
 struct fwnode_handle;
@@ -143,11 +145,17 @@  enum phylink_op_type {
  *                    possible and avoid stopping it during suspend events.
  * @default_an_inband: if true, defaults to MLO_AN_INBAND rather than
  *		       MLO_AN_PHY. A fixed-link specification will override.
+ * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
  *                        are supported by the MAC/PCS.
+ * @lpi_interfaces: bitmap describing which PHY interface modes can support
+ *		    LPI signalling.
  * @mac_capabilities: MAC pause/speed/duplex capabilities.
+ * @lpi_capabilities: MAC speeds which can support LPI signalling
+ * @eee: default EEE configuration.
+ * @lpi_timer_limit_us: Maximum (inclusive) value of the EEE LPI timer.
  */
 struct phylink_config {
 	struct device *dev;
@@ -156,10 +164,16 @@  struct phylink_config {
 	bool mac_managed_pm;
 	bool mac_requires_rxc;
 	bool default_an_inband;
+	bool eee_rx_clk_stop_enable;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
+	DECLARE_PHY_INTERFACE_MASK(lpi_interfaces);
 	unsigned long mac_capabilities;
+	unsigned long lpi_capabilities;
+	u32 lpi_timer_limit_us;
+	u32 lpi_timer_default;
+	bool eee_enabled_default;
 };
 
 void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
@@ -173,6 +187,8 @@  void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed);
  * @mac_finish: finish a major reconfiguration of the interface.
  * @mac_link_down: take the link down.
  * @mac_link_up: allow the link to come up.
+ * @mac_disable_tx_lpi: disable LPI.
+ * @mac_enable_tx_lpi: enable and configure LPI.
  *
  * The individual methods are described more fully below.
  */
@@ -193,6 +209,9 @@  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_disable_tx_lpi)(struct phylink_config *config);
+	void (*mac_enable_tx_lpi)(struct phylink_config *config, u32 timer,
+				  bool tx_clk_stop);
 };
 
 #if 0 /* For kernel-doc purposes only. */
@@ -387,6 +406,31 @@  void mac_link_down(struct phylink_config *config, unsigned int mode,
 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_disable_tx_lpi() - disable LPI generation at the MAC
+ * @config: a pointer to a &struct phylink_config.
+ *
+ * Disable generation of LPI at the MAC, effectively preventing the MAC
+ * from indicating that it is idle.
+ */
+void mac_disable_tx_lpi(struct phylink_config *config);
+
+/**
+ * mac_enable_tx_lpi() - configure and enable LPI generation at the MAC
+ * @config: a pointer to a &struct phylink_config.
+ * @timer: LPI timeout in microseconds.
+ * @tx_clk_stop: allow xMII transmit clock to be stopped during LPI
+ *
+ * Configure the LPI timeout accordingly. This will only be called when
+ * the link is already up, to cater for situations where the hardware
+ * needs to be programmed according to the link speed.
+ *
+ * Enable LPI generation at the MAC, and configure whether the xMII transmit
+ * clock may be stopped.
+ */
+void mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+		       bool tx_clk_stop);
 #endif
 
 struct phylink_pcs_ops;