diff mbox series

[net-next,V4,5/5] net: lan743x: Add support to ethtool phylink get and set settings

Message ID 20240829055132.79638-6-Raju.Lakkaraju@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add support to PHYLINK for LAN743x/PCI11x1x chips | 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: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: bryan.whitehead@microchip.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
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: 16 this patch: 16
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-29--15-00 (tests: 711)

Commit Message

Raju Lakkaraju - I30499 Aug. 29, 2024, 5:51 a.m. UTC 
Add support to ethtool phylink functions:
  - get/set settings like speed, duplex etc
  - get/set the wake-on-lan (WOL)
  - get/set the energy-efficient ethernet (EEE)
  - get/set the pause

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:                                                                    
============                                                                    
V3 -> V4:
  - Remove the EEE private variables from LAN743x adapter strcture and fix the   
    EEE's set/get functions
  - Change lan743x_set_eee( ) to lan743x_mac_eee_enable( )
  - Fix the EEE's tx lpi counter update
V2 -> V3:
  - No change
V1 -> V2:                                                                       
  - Fix the phylink changes                                                                  
 
 .../net/ethernet/microchip/lan743x_ethtool.c  | 117 ++++++------------
 drivers/net/ethernet/microchip/lan743x_main.c |  20 +++
 drivers/net/ethernet/microchip/lan743x_main.h |   1 +
 3 files changed, 59 insertions(+), 79 deletions(-)

Comments

Andrew Lunn Aug. 30, 2024, 8:55 p.m. UTC | #1 
> @@ -3055,6 +3071,10 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
>  					  cap & FLOW_CTRL_TX,
>  					  cap & FLOW_CTRL_RX);
>  
> +	if (phydev)
> +		lan743x_mac_eee_enable(adapter, phydev->enable_tx_lpi &&
> +				       phydev->eee_enabled);

This is wrong. The documentation says:

/**
 * phy_support_eee - Set initial EEE policy configuration
 * @phydev: Target phy_device struct
 *
 * This function configures the initial policy for Energy Efficient Ethernet
 * (EEE) on the specified PHY device, influencing that EEE capabilities are
 * advertised before the link is established. It should be called during PHY
 * registration by the MAC driver and/or the PHY driver (for SmartEEE PHYs)
 * if MAC supports LPI or PHY is capable to compensate missing LPI functionality
 * of the MAC.
 *
 * The function sets default EEE policy parameters, including preparing the PHY
 * to advertise EEE capabilities based on hardware support.
 *
 * It also sets the expected configuration for Low Power Idle (LPI) in the MAC
 * driver. If the PHY framework determines that both local and remote
 * advertisements support EEE, and the negotiated link mode is compatible with
 * EEE, it will set enable_tx_lpi = true. The MAC driver is expected to act on
 * this setting by enabling the LPI timer if enable_tx_lpi is set.
 */

So you should only be looking at enable_tx_lpi.

Also, do you actually call phy_support_eee() anywhere? I don't see it
in this patch, but maybe it was already there?

   	Adrew
Raju Lakkaraju - I30499 Sept. 4, 2024, 8:23 a.m. UTC | #2 
Hi Andrew,

Thank you for review the patches.

The 08/30/2024 22:55, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > @@ -3055,6 +3071,10 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> >                                         cap & FLOW_CTRL_TX,
> >                                         cap & FLOW_CTRL_RX);
> >
> > +     if (phydev)
> > +             lan743x_mac_eee_enable(adapter, phydev->enable_tx_lpi &&
> > +                                    phydev->eee_enabled);
> 
> This is wrong. The documentation says:
> 
> /**
>  * phy_support_eee - Set initial EEE policy configuration
>  * @phydev: Target phy_device struct
>  *
>  * This function configures the initial policy for Energy Efficient Ethernet
>  * (EEE) on the specified PHY device, influencing that EEE capabilities are
>  * advertised before the link is established. It should be called during PHY
>  * registration by the MAC driver and/or the PHY driver (for SmartEEE PHYs)
>  * if MAC supports LPI or PHY is capable to compensate missing LPI functionality
>  * of the MAC.
>  *
>  * The function sets default EEE policy parameters, including preparing the PHY
>  * to advertise EEE capabilities based on hardware support.
>  *
>  * It also sets the expected configuration for Low Power Idle (LPI) in the MAC
>  * driver. If the PHY framework determines that both local and remote
>  * advertisements support EEE, and the negotiated link mode is compatible with
>  * EEE, it will set enable_tx_lpi = true. The MAC driver is expected to act on
>  * this setting by enabling the LPI timer if enable_tx_lpi is set.
>  */
> 
> So you should only be looking at enable_tx_lpi.

Ok. I will fix.

> 
> Also, do you actually call phy_support_eee() anywhere? I don't see it
> in this patch, but maybe it was already there?

We never call phy_support_eee() anywhere.
I will fix

> 
>         Adrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 3a63ec091413..b15b52d53e84 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1058,61 +1058,49 @@  static int lan743x_ethtool_get_eee(struct net_device *netdev,
 				   struct ethtool_keee *eee)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
-	struct phy_device *phydev = netdev->phydev;
-	u32 buf;
-	int ret;
 
-	if (!phydev)
-		return -EIO;
-	if (!phydev->drv) {
-		netif_err(adapter, drv, adapter->netdev,
-			  "Missing PHY Driver\n");
-		return -EIO;
-	}
+	eee->tx_lpi_timer = lan743x_csr_read(adapter,
+					     MAC_EEE_TX_LPI_REQ_DLY_CNT);
 
-	ret = phy_ethtool_get_eee(phydev, eee);
-	if (ret < 0)
-		return ret;
+	return phylink_ethtool_get_eee(adapter->phylink, eee);
+}
 
-	buf = lan743x_csr_read(adapter, MAC_CR);
-	if (buf & MAC_CR_EEE_EN_) {
-		/* 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->tx_lpi_timer = 0;
+static int lan743x_ethtool_set_eee(struct net_device *netdev,
+				   struct ethtool_keee *eee)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	u32 tx_lpi_timer;
+
+	tx_lpi_timer = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
+	if (tx_lpi_timer != eee->tx_lpi_timer) {
+		/* Software should only change this field when Energy Efficient
+		 * Ethernet Enable (EEEEN) is cleared.
+		 * This function will trigger an autonegotiation restart and
+		 * that eee will be reenabled during link up if eee was
+		 * negotiated.
+		 */
+		lan743x_mac_eee_enable(adapter, false);
+		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
+				  eee->tx_lpi_timer);
 	}
 
-	return 0;
+	return phylink_ethtool_set_eee(adapter->phylink, eee);
 }
 
-static int lan743x_ethtool_set_eee(struct net_device *netdev,
-				   struct ethtool_keee *eee)
+static int lan743x_ethtool_set_link_ksettings(struct net_device *netdev,
+					      const struct ethtool_link_ksettings *cmd)
 {
-	struct lan743x_adapter *adapter;
-	struct phy_device *phydev;
-	u32 buf = 0;
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
 
-	if (!netdev)
-		return -EINVAL;
-	adapter = netdev_priv(netdev);
-	if (!adapter)
-		return -EINVAL;
-	phydev = netdev->phydev;
-	if (!phydev)
-		return -EIO;
-	if (!phydev->drv) {
-		netif_err(adapter, drv, adapter->netdev,
-			  "Missing PHY Driver\n");
-		return -EIO;
-	}
+	return phylink_ethtool_ksettings_set(adapter->phylink, cmd);
+}
 
-	if (eee->eee_enabled) {
-		buf = (u32)eee->tx_lpi_timer;
-		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
-	}
+static int lan743x_ethtool_get_link_ksettings(struct net_device *netdev,
+					      struct ethtool_link_ksettings *cmd)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
 
-	return phy_ethtool_set_eee(phydev, eee);
+	return phylink_ethtool_ksettings_get(adapter->phylink, cmd);
 }
 
 #ifdef CONFIG_PM
@@ -1124,8 +1112,7 @@  static void lan743x_ethtool_get_wol(struct net_device *netdev,
 	wol->supported = 0;
 	wol->wolopts = 0;
 
-	if (netdev->phydev)
-		phy_ethtool_get_wol(netdev->phydev, wol);
+	phylink_ethtool_get_wol(adapter->phylink, wol);
 
 	if (wol->supported != adapter->phy_wol_supported)
 		netif_warn(adapter, drv, adapter->netdev,
@@ -1166,7 +1153,7 @@  static int lan743x_ethtool_set_wol(struct net_device *netdev,
 		    !(adapter->phy_wol_supported & WAKE_MAGICSECURE))
 			phy_wol.wolopts &= ~WAKE_MAGIC;
 
-		ret = phy_ethtool_set_wol(netdev->phydev, &phy_wol);
+		ret = phylink_ethtool_set_wol(adapter->phylink, wol);
 		if (ret && (ret != -EOPNOTSUPP))
 			return ret;
 
@@ -1355,44 +1342,16 @@  static void lan743x_get_pauseparam(struct net_device *dev,
 				   struct ethtool_pauseparam *pause)
 {
 	struct lan743x_adapter *adapter = netdev_priv(dev);
-	struct lan743x_phy *phy = &adapter->phy;
 
-	if (phy->fc_request_control & FLOW_CTRL_TX)
-		pause->tx_pause = 1;
-	if (phy->fc_request_control & FLOW_CTRL_RX)
-		pause->rx_pause = 1;
-	pause->autoneg = phy->fc_autoneg;
+	phylink_ethtool_get_pauseparam(adapter->phylink, pause);
 }
 
 static int lan743x_set_pauseparam(struct net_device *dev,
 				  struct ethtool_pauseparam *pause)
 {
 	struct lan743x_adapter *adapter = netdev_priv(dev);
-	struct phy_device *phydev = dev->phydev;
-	struct lan743x_phy *phy = &adapter->phy;
 
-	if (!phydev)
-		return -ENODEV;
-
-	if (!phy_validate_pause(phydev, pause))
-		return -EINVAL;
-
-	phy->fc_request_control = 0;
-	if (pause->rx_pause)
-		phy->fc_request_control |= FLOW_CTRL_RX;
-
-	if (pause->tx_pause)
-		phy->fc_request_control |= FLOW_CTRL_TX;
-
-	phy->fc_autoneg = pause->autoneg;
-
-	if (pause->autoneg == AUTONEG_DISABLE)
-		lan743x_mac_flow_ctrl_set_enables(adapter, pause->tx_pause,
-						  pause->rx_pause);
-	else
-		phy_set_asym_pause(phydev, pause->rx_pause,  pause->tx_pause);
-
-	return 0;
+	return phylink_ethtool_set_pauseparam(adapter->phylink, pause);
 }
 
 const struct ethtool_ops lan743x_ethtool_ops = {
@@ -1417,8 +1376,8 @@  const struct ethtool_ops lan743x_ethtool_ops = {
 	.get_ts_info = lan743x_ethtool_get_ts_info,
 	.get_eee = lan743x_ethtool_get_eee,
 	.set_eee = lan743x_ethtool_set_eee,
-	.get_link_ksettings = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings = lan743x_ethtool_get_link_ksettings,
+	.set_link_ksettings = lan743x_ethtool_set_link_ksettings,
 	.get_regs_len = lan743x_get_regs_len,
 	.get_regs = lan743x_get_regs,
 	.get_pauseparam = lan743x_get_pauseparam,
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 91e74e231251..9c18e025a28d 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -2966,6 +2966,18 @@  static int lan743x_phylink_2500basex_config(struct lan743x_adapter *adapter)
 	return lan743x_pcs_power_reset(adapter);
 }
 
+void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
+{
+	u32 mac_cr;
+
+	mac_cr = lan743x_csr_read(adapter, MAC_CR);
+	if (enable)
+		mac_cr |= MAC_CR_EEE_EN_;
+	else
+		mac_cr &= ~MAC_CR_EEE_EN_;
+	lan743x_csr_write(adapter, MAC_CR, mac_cr);
+}
+
 static void lan743x_phylink_mac_config(struct phylink_config *config,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
@@ -3013,7 +3025,11 @@  static void lan743x_phylink_mac_link_down(struct phylink_config *config,
 					  unsigned int link_an_mode,
 					  phy_interface_t interface)
 {
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
 	netif_tx_stop_all_queues(to_net_dev(config->dev));
+	lan743x_mac_eee_enable(adapter, false);
 }
 
 static void lan743x_phylink_mac_link_up(struct phylink_config *config,
@@ -3055,6 +3071,10 @@  static void lan743x_phylink_mac_link_up(struct phylink_config *config,
 					  cap & FLOW_CTRL_TX,
 					  cap & FLOW_CTRL_RX);
 
+	if (phydev)
+		lan743x_mac_eee_enable(adapter, phydev->enable_tx_lpi &&
+				       phydev->eee_enabled);
+
 	netif_tx_wake_all_queues(netdev);
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 7f73d66854be..8ef897c114d3 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1206,5 +1206,6 @@  void lan743x_hs_syslock_release(struct lan743x_adapter *adapter);
 void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
 				       bool tx_enable, bool rx_enable);
 int lan743x_sgmii_read(struct lan743x_adapter *adapter, u8 mmd, u16 addr);
+void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable);
 
 #endif /* _LAN743X_H */