[PATCH net-next V3 0/4] Add support to PHYLINK for LAN743x/PCI11x1x chips

[PATCH net-next V3 0/4] Add support to PHYLINK for LAN743x/PCI11x1x chips

[Raju Lakkaraju]

This is the follow-up patch series of
https://lkml.iu.edu/hypermail/linux/kernel/2310.2/02078.html

Divide the PHYLINK adaptation and SFP modifications into two separate patch
series.

The current patch series focuses on transitioning the LAN743x driver's PHY
support from phylib to phylink.

Tested on chip PCI11010 Rev-B with Bridgeport Evaluation board Rev-1

Change List:
============
V2 -> V3:
  - Remove the unwanted parens in each of these if() sub-blocks 
  - Replace "to_net_dev(config->dev)" with "netdev".
  - Add GMII_ID/RGMII_TXID/RGMII_RXID in supported_interfaces
  - Fix the lan743x_phy_handle_exists( ) return type

V1 -> V2:
  - Fix the Russell King's comments i.e. remove the speed, duplex update in 
    lan743x_phylink_mac_config( )
  - pre-March 2020 legacy support has been removed

V0 -> V1:
  - Integrate with Synopsys DesignWare XPCS drivers
  - Based on external review comments,
  - Changes made to SGMII interface support only 1G/100M/10M bps speed
  - Changes made to 2500Base-X interface support only 2.5Gbps speed
  - Add check for not is_sgmii_en with is_sfp_support_en support
  - Change the "pci11x1x_strap_get_status" function return type from void to
    int
  - Add ethtool phylink wol, eee, pause get/set functions


Raju Lakkaraju (4):
  net: lan743x: Create separate PCS power reset function
  net: lan743x: Create separate Link Speed Duplex state function
  net: lan743x: Migrate phylib to phylink
  net: lan743x: Add support to ethtool phylink get and set settings

 drivers/net/ethernet/microchip/Kconfig        |   5 +-
 .../net/ethernet/microchip/lan743x_ethtool.c  | 118 +---
 drivers/net/ethernet/microchip/lan743x_main.c | 657 +++++++++++-------
 drivers/net/ethernet/microchip/lan743x_main.h |   7 +
 4 files changed, 460 insertions(+), 327 deletions(-)

-- 
2.34.1

[PATCH net-next V3 1/4] net: lan743x: Create separate PCS power reset function

[Raju Lakkaraju]

Create separate PCS power reset function from lan743x_sgmii_config () to use
as subroutine.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V2 -> V3:
  - No change
V1 -> V2:
  - No change

 drivers/net/ethernet/microchip/lan743x_main.c | 55 ++++++++++---------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index e418539565b1..ce1e104adc20 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1147,12 +1147,39 @@ static int lan743x_pcs_seq_state(struct lan743x_adapter *adapter, u8 state)
 	return 0;
 }
 
+static int lan743x_pcs_power_reset(struct lan743x_adapter *adapter)
+{
+	int mii_ctl;
+	int ret;
+
+	/* SGMII/1000/2500BASE-X PCS power down */
+	mii_ctl = lan743x_sgmii_read(adapter, MDIO_MMD_VEND2, MII_BMCR);
+	if (mii_ctl < 0)
+		return mii_ctl;
+
+	mii_ctl |= BMCR_PDOWN;
+	ret = lan743x_sgmii_write(adapter, MDIO_MMD_VEND2, MII_BMCR, mii_ctl);
+	if (ret < 0)
+		return ret;
+
+	ret = lan743x_pcs_seq_state(adapter, PCS_POWER_STATE_DOWN);
+	if (ret < 0)
+		return ret;
+
+	/* SGMII/1000/2500BASE-X PCS power up */
+	mii_ctl &= ~BMCR_PDOWN;
+	ret = lan743x_sgmii_write(adapter, MDIO_MMD_VEND2, MII_BMCR, mii_ctl);
+	if (ret < 0)
+		return ret;
+
+	return lan743x_pcs_seq_state(adapter, PCS_POWER_STATE_UP);
+}
+
 static int lan743x_sgmii_config(struct lan743x_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct phy_device *phydev = netdev->phydev;
 	enum lan743x_sgmii_lsd lsd = POWER_DOWN;
-	int mii_ctl;
 	bool status;
 	int ret;
 
@@ -1209,31 +1236,7 @@ static int lan743x_sgmii_config(struct lan743x_adapter *adapter)
 		netif_dbg(adapter, drv, adapter->netdev,
 			  "SGMII 1G mode enable\n");
 
-	/* SGMII/1000/2500BASE-X PCS power down */
-	mii_ctl = lan743x_sgmii_read(adapter, MDIO_MMD_VEND2, MII_BMCR);
-	if (mii_ctl < 0)
-		return mii_ctl;
-
-	mii_ctl |= BMCR_PDOWN;
-	ret = lan743x_sgmii_write(adapter, MDIO_MMD_VEND2, MII_BMCR, mii_ctl);
-	if (ret < 0)
-		return ret;
-
-	ret = lan743x_pcs_seq_state(adapter, PCS_POWER_STATE_DOWN);
-	if (ret < 0)
-		return ret;
-
-	/* SGMII/1000/2500BASE-X PCS power up */
-	mii_ctl &= ~BMCR_PDOWN;
-	ret = lan743x_sgmii_write(adapter, MDIO_MMD_VEND2, MII_BMCR, mii_ctl);
-	if (ret < 0)
-		return ret;
-
-	ret = lan743x_pcs_seq_state(adapter, PCS_POWER_STATE_UP);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return lan743x_pcs_power_reset(adapter);
 }
 
 static void lan743x_mac_set_address(struct lan743x_adapter *adapter,
-- 
2.34.1

[PATCH net-next V3 2/4] net: lan743x: Create separate Link Speed Duplex state function

[Raju Lakkaraju]

Create separate Link Speed Duplex (LSD) update state function from
lan743x_sgmii_config () to use as subroutine.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:                                                                    
============                                                                    
V2 -> V3:
  - No change
V1 -> V2:                                                                       
  - No change                                                                  
 
 drivers/net/ethernet/microchip/lan743x_main.c | 75 +++++++++++--------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index ce1e104adc20..b4a4c2840a83 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -992,6 +992,42 @@ static int lan743x_sgmii_write(struct lan743x_adapter *adapter,
 	return ret;
 }
 
+static int lan743x_get_lsd(int speed, int duplex, u8 mss)
+{
+	int lsd;
+
+	switch (speed) {
+	case SPEED_2500:
+		if (mss == MASTER_SLAVE_STATE_SLAVE)
+			lsd = LINK_2500_SLAVE;
+		else
+			lsd = LINK_2500_MASTER;
+		break;
+	case SPEED_1000:
+		if (mss == MASTER_SLAVE_STATE_SLAVE)
+			lsd = LINK_1000_SLAVE;
+		else
+			lsd = LINK_1000_MASTER;
+		break;
+	case SPEED_100:
+		if (duplex == DUPLEX_FULL)
+			lsd = LINK_100FD;
+		else
+			lsd = LINK_100HD;
+		break;
+	case SPEED_10:
+		if (duplex == DUPLEX_FULL)
+			lsd = LINK_10FD;
+		else
+			lsd = LINK_10HD;
+		break;
+	default:
+		lsd = -EINVAL;
+	}
+
+	return lsd;
+}
+
 static int lan743x_sgmii_mpll_set(struct lan743x_adapter *adapter,
 				  u16 baud)
 {
@@ -1179,42 +1215,21 @@ static int lan743x_sgmii_config(struct lan743x_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct phy_device *phydev = netdev->phydev;
-	enum lan743x_sgmii_lsd lsd = POWER_DOWN;
 	bool status;
 	int ret;
 
-	switch (phydev->speed) {
-	case SPEED_2500:
-		if (phydev->master_slave_state == MASTER_SLAVE_STATE_MASTER)
-			lsd = LINK_2500_MASTER;
-		else
-			lsd = LINK_2500_SLAVE;
-		break;
-	case SPEED_1000:
-		if (phydev->master_slave_state == MASTER_SLAVE_STATE_MASTER)
-			lsd = LINK_1000_MASTER;
-		else
-			lsd = LINK_1000_SLAVE;
-		break;
-	case SPEED_100:
-		if (phydev->duplex)
-			lsd = LINK_100FD;
-		else
-			lsd = LINK_100HD;
-		break;
-	case SPEED_10:
-		if (phydev->duplex)
-			lsd = LINK_10FD;
-		else
-			lsd = LINK_10HD;
-		break;
-	default:
+	ret = lan743x_get_lsd(phydev->speed, phydev->duplex,
+			      phydev->master_slave_state);
+	if (ret < 0) {
 		netif_err(adapter, drv, adapter->netdev,
-			  "Invalid speed %d\n", phydev->speed);
-		return -EINVAL;
+			  "error %d link-speed-duplex(LSD) invalid\n", ret);
+		return ret;
 	}
 
-	adapter->sgmii_lsd = lsd;
+	adapter->sgmii_lsd = ret;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "Link Speed Duplex (lsd) : 0x%X\n", adapter->sgmii_lsd);
+
 	ret = lan743x_sgmii_aneg_update(adapter);
 	if (ret < 0) {
 		netif_err(adapter, drv, adapter->netdev,
-- 
2.34.1

[PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Raju Lakkaraju]

Migrate phy support from phylib to phylink.
Fixed phy support is still used together with phylink since we need to support
dynamic fallback when a phy is not found over mdio. While phylink's FIXED mode
supports fixed phys that, it's dynamic and requires device tree entries which
are most of the time not present for LAN743x devices.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:                                                                    
============                                                                    
V2 -> V3:                                                                       
  - Remove the unwanted parens in each of these if() sub-blocks                 
  - Replace "to_net_dev(config->dev)" with "netdev".                            
  - Add GMII_ID/RGMII_TXID/RGMII_RXID in supported_interfaces                   
  - Fix the lan743x_phy_handle_exists( ) return type

V1 -> V2:                                                                       
  - Split the PHYLINK and SFP changes in 2 different patch series                                                                  
 
 drivers/net/ethernet/microchip/Kconfig        |   5 +-
 drivers/net/ethernet/microchip/lan743x_main.c | 574 +++++++++++-------
 drivers/net/ethernet/microchip/lan743x_main.h |   3 +
 3 files changed, 355 insertions(+), 227 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 43ba71e82260..4b7a0433b7e5 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -46,12 +46,13 @@ config LAN743X
 	tristate "LAN743x support"
 	depends on PCI
 	depends on PTP_1588_CLOCK_OPTIONAL
-	select PHYLIB
 	select FIXED_PHY
 	select CRC16
 	select CRC32
+	select PHYLINK
 	help
-	  Support for the Microchip LAN743x PCI Express Gigabit Ethernet chip
+	  Support for the Microchip LAN743x and PCI11x1x families of PCI
+	  Express Ethernet devices
 
 	  To compile this driver as a module, choose M here. The module will be
 	  called lan743x.
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index b4a4c2840a83..845a48ca497d 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -15,6 +15,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/iopoll.h>
 #include <linux/crc16.h>
+#include <linux/phylink.h>
 #include "lan743x_main.h"
 #include "lan743x_ethtool.h"
 
@@ -1077,26 +1078,7 @@ static int lan743x_sgmii_2_5G_mode_set(struct lan743x_adapter *adapter,
 					      VR_MII_BAUD_RATE_1P25GBPS);
 }
 
-static int lan743x_is_sgmii_2_5G_mode(struct lan743x_adapter *adapter,
-				      bool *status)
-{
-	int ret;
-
-	ret = lan743x_sgmii_read(adapter, MDIO_MMD_VEND2,
-				 VR_MII_GEN2_4_MPLL_CTRL1);
-	if (ret < 0)
-		return ret;
-
-	if (ret == VR_MII_MPLL_MULTIPLIER_125 ||
-	    ret == VR_MII_MPLL_MULTIPLIER_50)
-		*status = true;
-	else
-		*status = false;
-
-	return 0;
-}
-
-static int lan743x_sgmii_aneg_update(struct lan743x_adapter *adapter)
+static int lan743x_serdes_clock_and_aneg_update(struct lan743x_adapter *adapter)
 {
 	enum lan743x_sgmii_lsd lsd = adapter->sgmii_lsd;
 	int mii_ctrl;
@@ -1211,49 +1193,6 @@ static int lan743x_pcs_power_reset(struct lan743x_adapter *adapter)
 	return lan743x_pcs_seq_state(adapter, PCS_POWER_STATE_UP);
 }
 
-static int lan743x_sgmii_config(struct lan743x_adapter *adapter)
-{
-	struct net_device *netdev = adapter->netdev;
-	struct phy_device *phydev = netdev->phydev;
-	bool status;
-	int ret;
-
-	ret = lan743x_get_lsd(phydev->speed, phydev->duplex,
-			      phydev->master_slave_state);
-	if (ret < 0) {
-		netif_err(adapter, drv, adapter->netdev,
-			  "error %d link-speed-duplex(LSD) invalid\n", ret);
-		return ret;
-	}
-
-	adapter->sgmii_lsd = ret;
-	netif_dbg(adapter, drv, adapter->netdev,
-		  "Link Speed Duplex (lsd) : 0x%X\n", adapter->sgmii_lsd);
-
-	ret = lan743x_sgmii_aneg_update(adapter);
-	if (ret < 0) {
-		netif_err(adapter, drv, adapter->netdev,
-			  "error %d SGMII cfg failed\n", ret);
-		return ret;
-	}
-
-	ret = lan743x_is_sgmii_2_5G_mode(adapter, &status);
-	if (ret < 0) {
-		netif_err(adapter, drv, adapter->netdev,
-			  "error %d SGMII get mode failed\n", ret);
-		return ret;
-	}
-
-	if (status)
-		netif_dbg(adapter, drv, adapter->netdev,
-			  "SGMII 2.5G mode enable\n");
-	else
-		netif_dbg(adapter, drv, adapter->netdev,
-			  "SGMII 1G mode enable\n");
-
-	return lan743x_pcs_power_reset(adapter);
-}
-
 static void lan743x_mac_set_address(struct lan743x_adapter *adapter,
 				    u8 *addr)
 {
@@ -1407,103 +1346,11 @@ static int lan743x_phy_reset(struct lan743x_adapter *adapter)
 				  50000, 1000000);
 }
 
-static void lan743x_phy_update_flowcontrol(struct lan743x_adapter *adapter,
-					   u16 local_adv, u16 remote_adv)
-{
-	struct lan743x_phy *phy = &adapter->phy;
-	u8 cap;
-
-	if (phy->fc_autoneg)
-		cap = mii_resolve_flowctrl_fdx(local_adv, remote_adv);
-	else
-		cap = phy->fc_request_control;
-
-	lan743x_mac_flow_ctrl_set_enables(adapter,
-					  cap & FLOW_CTRL_TX,
-					  cap & FLOW_CTRL_RX);
-}
-
 static int lan743x_phy_init(struct lan743x_adapter *adapter)
 {
 	return lan743x_phy_reset(adapter);
 }
 
-static void lan743x_phy_link_status_change(struct net_device *netdev)
-{
-	struct lan743x_adapter *adapter = netdev_priv(netdev);
-	struct phy_device *phydev = netdev->phydev;
-	u32 data;
-
-	phy_print_status(phydev);
-	if (phydev->state == PHY_RUNNING) {
-		int remote_advertisement = 0;
-		int local_advertisement = 0;
-
-		data = lan743x_csr_read(adapter, MAC_CR);
-
-		/* set duplex mode */
-		if (phydev->duplex)
-			data |= MAC_CR_DPX_;
-		else
-			data &= ~MAC_CR_DPX_;
-
-		/* set bus speed */
-		switch (phydev->speed) {
-		case SPEED_10:
-			data &= ~MAC_CR_CFG_H_;
-			data &= ~MAC_CR_CFG_L_;
-		break;
-		case SPEED_100:
-			data &= ~MAC_CR_CFG_H_;
-			data |= MAC_CR_CFG_L_;
-		break;
-		case SPEED_1000:
-			data |= MAC_CR_CFG_H_;
-			data &= ~MAC_CR_CFG_L_;
-		break;
-		case SPEED_2500:
-			data |= MAC_CR_CFG_H_;
-			data |= MAC_CR_CFG_L_;
-		break;
-		}
-		lan743x_csr_write(adapter, MAC_CR, data);
-
-		local_advertisement =
-			linkmode_adv_to_mii_adv_t(phydev->advertising);
-		remote_advertisement =
-			linkmode_adv_to_mii_adv_t(phydev->lp_advertising);
-
-		lan743x_phy_update_flowcontrol(adapter, local_advertisement,
-					       remote_advertisement);
-		lan743x_ptp_update_latency(adapter, phydev->speed);
-		if (phydev->interface == PHY_INTERFACE_MODE_SGMII ||
-		    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->enable_tx_lpi)
-			data |=  MAC_CR_EEE_EN_;
-		else
-			data &= ~MAC_CR_EEE_EN_;
-		lan743x_csr_write(adapter, MAC_CR, data);
-	}
-}
-
-static void lan743x_phy_close(struct lan743x_adapter *adapter)
-{
-	struct net_device *netdev = adapter->netdev;
-	struct phy_device *phydev = netdev->phydev;
-
-	phy_stop(netdev->phydev);
-	phy_disconnect(netdev->phydev);
-
-	/* using phydev here as phy_disconnect NULLs netdev->phydev */
-	if (phy_is_pseudo_fixed_link(phydev))
-		fixed_phy_unregister(phydev);
-
-}
-
 static void lan743x_phy_interface_select(struct lan743x_adapter *adapter)
 {
 	u32 id_rev;
@@ -1520,65 +1367,9 @@ static void lan743x_phy_interface_select(struct lan743x_adapter *adapter)
 		adapter->phy_interface = PHY_INTERFACE_MODE_MII;
 	else
 		adapter->phy_interface = PHY_INTERFACE_MODE_RGMII;
-}
 
-static int lan743x_phy_open(struct lan743x_adapter *adapter)
-{
-	struct net_device *netdev = adapter->netdev;
-	struct lan743x_phy *phy = &adapter->phy;
-	struct fixed_phy_status fphy_status = {
-		.link = 1,
-		.speed = SPEED_1000,
-		.duplex = DUPLEX_FULL,
-	};
-	struct phy_device *phydev;
-	int ret = -EIO;
-
-	/* try devicetree phy, or fixed link */
-	phydev = of_phy_get_and_connect(netdev, adapter->pdev->dev.of_node,
-					lan743x_phy_link_status_change);
-
-	if (!phydev) {
-		/* try internal phy */
-		phydev = phy_find_first(adapter->mdiobus);
-		if (!phydev)	{
-			if ((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
-					ID_REV_ID_LAN7431_) {
-				phydev = fixed_phy_register(PHY_POLL,
-							    &fphy_status, NULL);
-				if (IS_ERR(phydev)) {
-					netdev_err(netdev, "No PHY/fixed_PHY found\n");
-					return PTR_ERR(phydev);
-				}
-			} else {
-				goto return_error;
-				}
-		}
-
-		lan743x_phy_interface_select(adapter);
-
-		ret = phy_connect_direct(netdev, phydev,
-					 lan743x_phy_link_status_change,
-					 adapter->phy_interface);
-		if (ret)
-			goto return_error;
-	}
-
-	/* MAC doesn't support 1000T Half */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	/* support both flow controls */
-	phy_support_asym_pause(phydev);
-	phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
-	phy->fc_autoneg = phydev->autoneg;
-
-	phy_start(phydev);
-	phy_start_aneg(phydev);
-	phy_attached_info(phydev);
-	return 0;
-
-return_error:
-	return ret;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "selected phy interface: 0x%X\n", adapter->phy_interface);
 }
 
 static void lan743x_rfe_open(struct lan743x_adapter *adapter)
@@ -3079,6 +2870,322 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
 	return ret;
 }
 
+static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
+{
+	u32 sgmii_ctl;
+	int ret;
+
+	ret = lan743x_get_lsd(SPEED_1000, DUPLEX_FULL,
+			      MASTER_SLAVE_STATE_MASTER);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d link-speed-duplex(LSD) invalid\n", ret);
+		return ret;
+	}
+
+	adapter->sgmii_lsd = ret;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "Link Speed Duplex (lsd) : 0x%X\n", adapter->sgmii_lsd);
+
+	/* LINK_STATUS_SOURCE from the External PHY via SGMII */
+	sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
+	sgmii_ctl &= ~SGMII_CTL_LINK_STATUS_SOURCE_;
+	lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
+
+	ret = lan743x_serdes_clock_and_aneg_update(adapter);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d sgmii aneg update failed\n", ret);
+		return ret;
+	}
+
+	return lan743x_pcs_power_reset(adapter);
+}
+
+static int lan743x_phylink_1000basex_config(struct lan743x_adapter *adapter)
+{
+	u32 sgmii_ctl;
+	int ret;
+
+	ret = lan743x_get_lsd(SPEED_1000, DUPLEX_FULL,
+			      MASTER_SLAVE_STATE_MASTER);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d link-speed-duplex(LSD) invalid\n", ret);
+		return ret;
+	}
+
+	adapter->sgmii_lsd = ret;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "Link Speed Duplex (lsd) : 0x%X\n", adapter->sgmii_lsd);
+
+	/* LINK_STATUS_SOURCE from 1000BASE-X PCS link status */
+	sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
+	sgmii_ctl |= SGMII_CTL_LINK_STATUS_SOURCE_;
+	lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
+
+	ret = lan743x_serdes_clock_and_aneg_update(adapter);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d 1000basex aneg update failed\n", ret);
+		return ret;
+	}
+
+	return lan743x_pcs_power_reset(adapter);
+}
+
+static int lan743x_phylink_2500basex_config(struct lan743x_adapter *adapter)
+{
+	u32 sgmii_ctl;
+	int ret;
+
+	ret = lan743x_get_lsd(SPEED_2500, DUPLEX_FULL,
+			      MASTER_SLAVE_STATE_MASTER);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d link-speed-duplex(LSD) invalid\n", ret);
+		return ret;
+	}
+
+	adapter->sgmii_lsd = ret;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "Link Speed Duplex (lsd) : 0x%X\n", adapter->sgmii_lsd);
+
+	/* LINK_STATUS_SOURCE from 2500BASE-X PCS link status */
+	sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
+	sgmii_ctl |= SGMII_CTL_LINK_STATUS_SOURCE_;
+	lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
+
+	ret = lan743x_serdes_clock_and_aneg_update(adapter);
+	if (ret < 0) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "error %d 2500basex aneg update failed\n", ret);
+		return ret;
+	}
+
+	return lan743x_pcs_power_reset(adapter);
+}
+
+static void lan743x_phylink_mac_config(struct phylink_config *config,
+				       unsigned int link_an_mode,
+				       const struct phylink_link_state *state)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	int ret;
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_2500BASEX:
+		ret = lan743x_phylink_2500basex_config(adapter);
+		if (ret < 0)
+			netif_err(adapter, drv, adapter->netdev,
+				  "2500BASEX config failed. Error %d\n", ret);
+		else
+			netif_dbg(adapter, drv, adapter->netdev,
+				  "2500BASEX mode selected and configured\n");
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+		ret = lan743x_phylink_1000basex_config(adapter);
+		if (ret < 0)
+			netif_err(adapter, drv, adapter->netdev,
+				  "1000BASEX config failed. Error %d\n", ret);
+		else
+			netif_dbg(adapter, drv, adapter->netdev,
+				  "1000BASEX mode selected and configured\n");
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+		ret = lan743x_phylink_sgmii_config(adapter);
+		if (ret < 0)
+			netif_err(adapter, drv, adapter->netdev,
+				  "SGMII config failed. Error %d\n", ret);
+		else
+			netif_dbg(adapter, drv, adapter->netdev,
+				  "SGMII mode selected and configured\n");
+		break;
+	default:
+		netif_dbg(adapter, drv, adapter->netdev,
+			  "RGMII/GMII/MII(0x%X) mode enable\n", state->interface);
+		break;
+	}
+}
+
+static void lan743x_phylink_mac_link_down(struct phylink_config *config,
+					  unsigned int link_an_mode,
+					  phy_interface_t interface)
+{
+	netif_tx_stop_all_queues(to_net_dev(config->dev));
+}
+
+static void lan743x_phylink_mac_link_up(struct phylink_config *config,
+					struct phy_device *phydev,
+					unsigned int link_an_mode,
+					phy_interface_t interface,
+					int speed, int duplex,
+					bool tx_pause, bool rx_pause)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	int mac_cr;
+	u8 cap;
+
+	mac_cr = lan743x_csr_read(adapter, MAC_CR);
+	/* Pre-initialize register bits.
+	 * Resulting value corresponds to SPEED_10
+	 */
+	mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
+	if (speed == SPEED_2500)
+		mac_cr |= MAC_CR_CFG_H_ | MAC_CR_CFG_L_;
+	else if (speed == SPEED_1000)
+		mac_cr |= MAC_CR_CFG_H_;
+	else if (speed == SPEED_100)
+		mac_cr |= MAC_CR_CFG_L_;
+
+	lan743x_csr_write(adapter, MAC_CR, mac_cr);
+
+	lan743x_ptp_update_latency(adapter, speed);
+
+	/* Flow Control operation */
+	cap = 0;
+	if (tx_pause)
+		cap |= FLOW_CTRL_TX;
+	if (rx_pause)
+		cap |= FLOW_CTRL_RX;
+
+	lan743x_mac_flow_ctrl_set_enables(adapter,
+					  cap & FLOW_CTRL_TX,
+					  cap & FLOW_CTRL_RX);
+
+	netif_tx_wake_all_queues(netdev);
+}
+
+static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
+	.mac_config = lan743x_phylink_mac_config,
+	.mac_link_down = lan743x_phylink_mac_link_down,
+	.mac_link_up = lan743x_phylink_mac_link_up,
+};
+
+static int lan743x_phylink_create(struct net_device *netdev)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	struct phylink *pl;
+
+	adapter->phylink_config.dev = &netdev->dev;
+	adapter->phylink_config.type = PHYLINK_NETDEV;
+	adapter->phylink_config.mac_managed_pm = false;
+
+	adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
+		MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
+
+	lan743x_phy_interface_select(adapter);
+
+	switch (adapter->phy_interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_GMII:
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	default:
+		__set_bit(PHY_INTERFACE_MODE_RGMII,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RGMII_ID,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
+			  adapter->phylink_config.supported_interfaces);
+	}
+
+	pl = phylink_create(&adapter->phylink_config, NULL,
+			    adapter->phy_interface, &lan743x_phylink_mac_ops);
+
+	if (IS_ERR(pl)) {
+		netdev_err(netdev, "Could not create phylink (%pe)\n", pl);
+		return PTR_ERR(pl);
+	}
+
+	adapter->phylink = pl;
+	netdev_dbg(netdev, "lan743x phylink created");
+
+	return 0;
+}
+
+static bool lan743x_phy_handle_exists(struct device_node *dn)
+{
+	dn = of_parse_phandle(dn, "phy-handle", 0);
+	of_node_put(dn);
+	return dn != NULL;
+}
+
+static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
+{
+	struct device_node *dn = adapter->pdev->dev.of_node;
+	struct net_device *dev = adapter->netdev;
+	struct fixed_phy_status fphy_status = {
+		.link = 1,
+		.speed = SPEED_1000,
+		.duplex = DUPLEX_FULL,
+	};
+	struct phy_device *phydev;
+	int ret;
+
+	if (dn)
+		ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
+
+	if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
+		phydev = phy_find_first(adapter->mdiobus);
+		if (!phydev) {
+			if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
+			      ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
+				phydev = fixed_phy_register(PHY_POLL,
+							    &fphy_status,
+							    NULL);
+				if (IS_ERR(phydev)) {
+					netdev_err(dev, "No PHY/fixed_PHY found\n");
+					return PTR_ERR(phydev);
+				}
+			} else {
+				netdev_err(dev, "no PHY found\n");
+				return -ENXIO;
+			}
+		}
+
+		/* attach the mac to the phy */
+		ret = phylink_connect_phy(adapter->phylink, phydev);
+		if (ret) {
+			netdev_err(dev, "Could not attach PHY (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	phylink_start(adapter->phylink);
+
+	return 0;
+}
+
+static void lan743x_phylink_disconnect(struct lan743x_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	struct phy_device *phydev = netdev->phydev;
+
+	phylink_stop(adapter->phylink);
+	phylink_disconnect_phy(adapter->phylink);
+
+	if (phydev)
+		if (phy_is_pseudo_fixed_link(phydev))
+			fixed_phy_unregister(phydev);
+}
+
 static int lan743x_netdev_close(struct net_device *netdev)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
@@ -3092,7 +3199,7 @@ static int lan743x_netdev_close(struct net_device *netdev)
 
 	lan743x_ptp_close(adapter);
 
-	lan743x_phy_close(adapter);
+	lan743x_phylink_disconnect(adapter);
 
 	lan743x_mac_close(adapter);
 
@@ -3115,13 +3222,13 @@ static int lan743x_netdev_open(struct net_device *netdev)
 	if (ret)
 		goto close_intr;
 
-	ret = lan743x_phy_open(adapter);
+	ret = lan743x_phylink_connect(adapter);
 	if (ret)
-		goto close_mac;
+		goto close_phylink;
 
 	ret = lan743x_ptp_open(adapter);
 	if (ret)
-		goto close_phy;
+		goto close_phylink;
 
 	lan743x_rfe_open(adapter);
 
@@ -3162,10 +3269,8 @@ static int lan743x_netdev_open(struct net_device *netdev)
 	}
 	lan743x_ptp_close(adapter);
 
-close_phy:
-	lan743x_phy_close(adapter);
-
-close_mac:
+close_phylink:
+	lan743x_phylink_disconnect(adapter);
 	lan743x_mac_close(adapter);
 
 close_intr:
@@ -3192,11 +3297,14 @@ static netdev_tx_t lan743x_netdev_xmit_frame(struct sk_buff *skb,
 static int lan743x_netdev_ioctl(struct net_device *netdev,
 				struct ifreq *ifr, int cmd)
 {
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
 	if (!netif_running(netdev))
 		return -EINVAL;
 	if (cmd == SIOCSHWTSTAMP)
 		return lan743x_ptp_ioctl(netdev, ifr, cmd);
-	return phy_mii_ioctl(netdev->phydev, ifr, cmd);
+
+	return phylink_mii_ioctl(adapter->phylink, ifr, cmd);
 }
 
 static void lan743x_netdev_set_multicast(struct net_device *netdev)
@@ -3301,10 +3409,17 @@ static void lan743x_mdiobus_cleanup(struct lan743x_adapter *adapter)
 	mdiobus_unregister(adapter->mdiobus);
 }
 
+static void lan743x_destroy_phylink(struct lan743x_adapter *adapter)
+{
+	phylink_destroy(adapter->phylink);
+	adapter->phylink = NULL;
+}
+
 static void lan743x_full_cleanup(struct lan743x_adapter *adapter)
 {
 	unregister_netdev(adapter->netdev);
 
+	lan743x_destroy_phylink(adapter);
 	lan743x_mdiobus_cleanup(adapter);
 	lan743x_hardware_cleanup(adapter);
 	lan743x_pci_cleanup(adapter);
@@ -3518,14 +3633,21 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
 				    NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	adapter->netdev->hw_features = adapter->netdev->features;
 
-	/* carrier off reporting is important to ethtool even BEFORE open */
-	netif_carrier_off(netdev);
+	ret = lan743x_phylink_create(adapter->netdev);
+	if (ret < 0) {
+		netif_err(adapter, probe, netdev,
+			  "failed to setup phylink (%d)\n", ret);
+		goto cleanup_mdiobus;
+	}
 
 	ret = register_netdev(adapter->netdev);
 	if (ret < 0)
-		goto cleanup_mdiobus;
+		goto cleanup_phylink;
 	return 0;
 
+cleanup_phylink:
+	lan743x_destroy_phylink(adapter);
+
 cleanup_mdiobus:
 	lan743x_mdiobus_cleanup(adapter);
 
@@ -3781,6 +3903,7 @@ static int lan743x_pm_resume(struct device *dev)
 	       MAC_WK_SRC_WK_FR_SAVED_;
 	lan743x_csr_write(adapter, MAC_WK_SRC, data);
 
+	rtnl_lock();
 	/* open netdev when netdev is at running state while resume.
 	 * For instance, it is true when system wakesup after pm-suspend
 	 * However, it is false when system wakes up after suspend GUI menu
@@ -3789,6 +3912,7 @@ static int lan743x_pm_resume(struct device *dev)
 		lan743x_netdev_open(netdev);
 
 	netif_device_attach(netdev);
+	rtnl_unlock();
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 3b2585a384e2..7f73d66854be 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -5,6 +5,7 @@
 #define _LAN743X_H
 
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
@@ -1083,6 +1084,8 @@ struct lan743x_adapter {
 	u32			flags;
 	u32			hw_cfg;
 	phy_interface_t		phy_interface;
+	struct phylink		*phylink;
+	struct phylink_config	phylink_config;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Russell King (Oracle)]

On Tue, Jul 30, 2024 at 07:36:18PM +0530, Raju Lakkaraju wrote:
> +	default:
> +		__set_bit(PHY_INTERFACE_MODE_RGMII,
> +			  adapter->phylink_config.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RGMII_ID,
> +			  adapter->phylink_config.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
> +			  adapter->phylink_config.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
> +			  adapter->phylink_config.supported_interfaces);

There's a shorter way to write this:

		phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);

> +static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
> +{
> +	struct device_node *dn = adapter->pdev->dev.of_node;
> +	struct net_device *dev = adapter->netdev;
> +	struct fixed_phy_status fphy_status = {
> +		.link = 1,
> +		.speed = SPEED_1000,
> +		.duplex = DUPLEX_FULL,
> +	};
> +	struct phy_device *phydev;
> +	int ret;
> +
> +	if (dn)
> +		ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
> +
> +	if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> +		phydev = phy_find_first(adapter->mdiobus);
> +		if (!phydev) {
> +			if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> +			      ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> +				phydev = fixed_phy_register(PHY_POLL,
> +							    &fphy_status,
> +							    NULL);

I thought something was going to happen with this?

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

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Raju Lakkaraju]

Hi Russell King,

Thank you for review the patches.

The 07/30/2024 15:56, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jul 30, 2024 at 07:36:18PM +0530, Raju Lakkaraju wrote:
> > +     default:
> > +             __set_bit(PHY_INTERFACE_MODE_RGMII,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_RGMII_ID,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
> > +                       adapter->phylink_config.supported_interfaces);
> 
> There's a shorter way to write this:
> 
>                 phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);

Ok. I will change.

> 
> > +static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
> > +{
> > +     struct device_node *dn = adapter->pdev->dev.of_node;
> > +     struct net_device *dev = adapter->netdev;
> > +     struct fixed_phy_status fphy_status = {
> > +             .link = 1,
> > +             .speed = SPEED_1000,
> > +             .duplex = DUPLEX_FULL,
> > +     };
> > +     struct phy_device *phydev;
> > +     int ret;
> > +
> > +     if (dn)
> > +             ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
> > +
> > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > +             phydev = phy_find_first(adapter->mdiobus);
> > +             if (!phydev) {
> > +                     if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > +                             phydev = fixed_phy_register(PHY_POLL,
> > +                                                         &fphy_status,
> > +                                                         NULL);
> 
> I thought something was going to happen with this?

Our SQA confirmed that it's working ping as expected (i.e Speed at 1Gbps
with full duplex) with Intel I210 NIC as link partner.

Do you suspect any corner case where it's fail?
 
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Russell King (Oracle)]

On Thu, Aug 01, 2024 at 04:33:13PM +0530, Raju Lakkaraju wrote:
> > > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > > +             phydev = phy_find_first(adapter->mdiobus);
> > > +             if (!phydev) {
> > > +                     if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > > +                             phydev = fixed_phy_register(PHY_POLL,
> > > +                                                         &fphy_status,
> > > +                                                         NULL);
> > 
> > I thought something was going to happen with this?
> 
> Our SQA confirmed that it's working ping as expected (i.e Speed at 1Gbps
> with full duplex) with Intel I210 NIC as link partner.
> 
> Do you suspect any corner case where it's fail?

Let me restate the review comment from V2:

"Eww. Given that phylink has its own internal fixed-PHY support, can we
not find some way to avoid the legacy fixed-PHY usage here?"

Yes, it may work, but fixed-phy is not supposed to be used with phylink.

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

RE: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Ronnie.Kunin]

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

Hi Russell,
Raju can comment more on it when he comes online tomorrow (IST time), but I recall this was discussed with you last year and my understanding is that the outcome was that as long as the need to use the dynamic fallback from phy to fixed_phy mode is explained in the commit message - which Raju did in the commit description - , it was acceptable to do this in phylink. Unless the "mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters" has been implemented since then (apologize if it has, I am not a linux expert by any means, but don't seem to see it), I would guess the reasons for doing this are still valid.

Attached is the email thread with that discussion and the relevant comments are copied below.

> The reason this should work is because the fixed-phy support does emulate a real PHY in software, and phylink will treat that exactly the same way as a real PHY (because when in phylink is in PHY mode, it delegates PHY management to phylib.)
>
>Using fixed-phy with phylink will certainly raise some eyebrows, so the reasons for it will need to be set out in the commit message - and as you've dropped Andrew from this thread, I suspect he will still raise some comments about it.
>
>In the longer term, it would probably make sense for phylink to provide a mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters.

Best regards,
Ronnie

-----Original Message-----
From: Russell King <linux@armlinux.org.uk> 
Sent: Thursday, August 1, 2024 12:27 PM
To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; andrew@lunn.ch; horms@kernel.org; hkallweit1@gmail.com; richardcochran@gmail.com; rdunlap@infradead.org; Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; edumazet@google.com; pabeni@redhat.com; linux-kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Thu, Aug 01, 2024 at 04:33:13PM +0530, Raju Lakkaraju wrote:
> > > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > > +             phydev = phy_find_first(adapter->mdiobus);
> > > +             if (!phydev) {
> > > +                     if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > > +                             phydev = fixed_phy_register(PHY_POLL,
> > > +                                                         &fphy_status,
> > > +                                                         NULL);
> >
> > I thought something was going to happen with this?
>
> Our SQA confirmed that it's working ping as expected (i.e Speed at 
> 1Gbps with full duplex) with Intel I210 NIC as link partner.
>
> Do you suspect any corner case where it's fail?

Let me restate the review comment from V2:

"Eww. Given that phylink has its own internal fixed-PHY support, can we not find some way to avoid the legacy fixed-PHY usage here?"

Yes, it may work, but fixed-phy is not supposed to be used with phylink.

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

[-- Attachment #2: Type: message/rfc822, Size: 13328 bytes --]

From: <linux@armlinux.org.uk>
To: <Raju.Lakkaraju@microchip.com>
Cc: <Ronnie.Kunin@microchip.com>, <Woojung.Huh@microchip.com>, <Horatiu.Vultur@microchip.com>
Subject: Re: [PATCH net-next 5/7] net: lan743x: Add support to the Phylink framework
Date: Tue, 26 Sep 2023 08:11:48 +0000
Message-ID: <ZRKSRG3L8fAZQD1i@shell.armlinux.org.uk>

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Tue, Sep 26, 2023 at 06:28:18AM +0000, Raju.Lakkaraju@microchip.com wrote:
> - As you mentioned in another thread (about phylink bindings) a few
> days ago, our current lan743x driver using phylib has a dynamic
> failover dt -> find_phy -> fixed phy sequence.

You mean this:

        /* try devicetree phy, or fixed link */
        phydev = of_phy_get_and_connect(netdev, adapter->pdev->dev.of_node,
                                        lan743x_phy_link_status_change);

        if (!phydev) {
                /* try internal phy */
                phydev = phy_find_first(adapter->mdiobus);
                if (!phydev)    {
                        if ((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
                                        ID_REV_ID_LAN7431_) {
                                phydev = fixed_phy_register(PHY_POLL,
                                                            &fphy_status, NULL);
...
                ret = phy_connect_direct(netdev, phydev,
                                         lan743x_phy_link_status_change,
                                         adapter->phy_interface);

> My understanding is that with the current phylink support you need to
> specify ahead of time whether you will be using PHY or FIXED_PHY
> support and cannot fail over from one to the other, which will break
> the current support we have for some of our customers.

This is not actually correct - the fixed PHY support hasn't been used
with phylink because we have a better solution in phylink itself for
fixed links. You rightly point out that this assumes that we've parsed
DT which tells us whether to use that or not.

However, this is possible:

        /* try devicetree phy, or fixed link */
        ret = phylink_of_phy_connect(pl, adapter->pdev->dev.of_node, 0);
        if (ret == -ENODEV) {
                /* try internal phy */
                phydev = phy_find_first(adapter->mdiobus);
                if (!phydev)    {
                        if ((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
                                        ID_REV_ID_LAN7431_) {
                                phydev = fixed_phy_register(PHY_POLL,
                                                            &fphy_status, NULL);
...
                ret = phylink_connect_phy(pl, phydev);

The reason this should work is because the fixed-phy support does
emulate a real PHY in software, and phylink will treat that exactly
the same way as a real PHY (because when in phylink is in PHY mode,
it delegates PHY management to phylib.)

Using fixed-phy with phylink will certainly raise some eyebrows, so
the reasons for it will need to be set out in the commit message -
and as you've dropped Andrew from this thread, I suspect he will
still raise some comments about it.

In the longer term, it would probably make sense for phylink to
provide a mechanism where a MAC driver can tell phylink to switch
to using a fixed-link with certain parameters.

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

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Russell King (Oracle)]

On Thu, Aug 01, 2024 at 11:46:41PM +0000, Ronnie.Kunin@microchip.com wrote:
> Hi Russell,
> Raju can comment more on it when he comes online tomorrow (IST time), but I recall this was discussed with you last year and my understanding is that the outcome was that as long as the need to use the dynamic fallback from phy to fixed_phy mode is explained in the commit message - which Raju did in the commit description - , it was acceptable to do this in phylink. Unless the "mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters" has been implemented since then (apologize if it has, I am not a linux expert by any means, but don't seem to see it), I would guess the reasons for doing this are still valid.
> 
> Attached is the email thread with that discussion and the relevant comments are copied below.
> 
> > The reason this should work is because the fixed-phy support does emulate a real PHY in software, and phylink will treat that exactly the same way as a real PHY (because when in phylink is in PHY mode, it delegates PHY management to phylib.)
> >
> >Using fixed-phy with phylink will certainly raise some eyebrows, so the reasons for it will need to be set out in the commit message - and as you've dropped Andrew from this thread, I suspect he will still raise some comments about it.
> >
> >In the longer term, it would probably make sense for phylink to provide a mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters.

Note the last two paragraphs...

What I never understand is why many software engineers working on Linux
are focused purely on the driver code and seem to have a complete
aversion to touching/improving anything outside of their driver,
prefering instead to come up with hacks.

Given that what you quote from me was from 26th September, I have to
wonder what you would define as "longer term" as we're 10 months on
from that statement.

I think it's time to implement the suggestion in the last paragraph
rather than using fixed-phy.

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

RE: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Ronnie.Kunin]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Friday, August 2, 2024 4:37 AM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>; netdev@vger.kernel.org;
> davem@davemloft.net; kuba@kernel.org; andrew@lunn.ch; horms@kernel.org; hkallweit1@gmail.com;
> richardcochran@gmail.com; rdunlap@infradead.org; Bryan Whitehead - C21958
> <Bryan.Whitehead@microchip.com>; edumazet@google.com; pabeni@redhat.com; linux-
> kernel@vger.kernel.org; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink
> ...
> > In the longer term, it would probably make sense for phylink to provide a mechanism where a MAC
> >driver can tell phylink to switch to using a fixed-link with certain parameters.
> ...
> I think it's time to implement the suggestion in the last paragraph rather than using fixed-phy.
>

We looked into an alternate way to migrate our lan743x driver from phylib to phylink continuing to support our existing hardware out in the field, without using the phylib's fixed-phy approach that you opposed to, but without modifying the phylib framework either. 
While investigating how to implement it we came across this which Raju borrowed ideas from: https://lore.kernel.org/linux-arm-kernel/YtGPO5SkMZfN8b%2Fs@shell.armlinux.org.uk/ . He is in the process of testing/cleaning it up and expects to submit it early next week.

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Russell King (Oracle)]

On Thu, Aug 08, 2024 at 08:23:38PM +0000, Ronnie.Kunin@microchip.com wrote:
> We looked into an alternate way to migrate our lan743x driver from phylib to phylink continuing to support our existing hardware out in the field, without using the phylib's fixed-phy approach that you opposed to, but without modifying the phylib framework either. 
> While investigating how to implement it we came across this which Raju borrowed ideas from: https://lore.kernel.org/linux-arm-kernel/YtGPO5SkMZfN8b%2Fs@shell.armlinux.org.uk/ . He is in the process of testing/cleaning it up and expects to submit it early next week.

That series died a death because it wasn't acceptable to the swnode
folk. In any case, that's clearly an over-complex solution for what is
a simple problem here.

The simplest solution would be for phylink to provide a new function,
e.g.

int phylink_set_fixed_link(struct phylink *pl,
			   const struct phylink_state *state)
{
	const struct phy_setting *s;
	unsigned long *adv;

	if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
		return -EINVAL;

	s = phy_lookup_setting(state->speed, state->duplex,
			       pl->supported, true);
	if (!s)
		return -EINVAL;

	adv = pl->link_config.advertising;
	linkmode_zero(adv);
	linkmode_set_bit(s->bit, adv);
	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);

	pl->link_config.speed = state->speed;
	pl->link_config.duplex = state->duplex;
	pl->link_config.link = 1;
	pl->link_config.an_complete = 1;

	pl->cfg_link_an_mode = MLO_AN_FIXED;
	pl->cur_link_an_mode = pl->cfg_link_an_mode;

	return 0;
}

You can then call this _instead_ of attaching a PHY to switch phylink
into fixed-link mode with the specified speed and duplex (assuming
they are supported by the MAC.)

Isn't this going to be simpler than trying to use swnodes that need
to be setup before phylink_create() gets called?

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

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Raju Lakkaraju]

Hi Russell King,

Thank you for quick response.

The 08/08/2024 22:07, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Aug 08, 2024 at 08:23:38PM +0000, Ronnie.Kunin@microchip.com wrote:
> > We looked into an alternate way to migrate our lan743x driver from phylib to phylink continuing to support our existing hardware out in the field, without using the phylib's fixed-phy approach that you opposed to, but without modifying the phylib framework either.
> > While investigating how to implement it we came across this which Raju borrowed ideas from: https://lore.kernel.org/linux-arm-kernel/YtGPO5SkMZfN8b%2Fs@shell.armlinux.org.uk/ . He is in the process of testing/cleaning it up and expects to submit it early next week.
> 
> That series died a death because it wasn't acceptable to the swnode
> folk. In any case, that's clearly an over-complex solution for what is
> a simple problem here.
> 
> The simplest solution would be for phylink to provide a new function,
> e.g.
> 
> int phylink_set_fixed_link(struct phylink *pl,
>                            const struct phylink_state *state)
> {
>         const struct phy_setting *s;
>         unsigned long *adv;
> 
>         if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
>             !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
>                 return -EINVAL;
> 
>         s = phy_lookup_setting(state->speed, state->duplex,
>                                pl->supported, true);
>         if (!s)
>                 return -EINVAL;
> 
>         adv = pl->link_config.advertising;
>         linkmode_zero(adv);
>         linkmode_set_bit(s->bit, adv);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
> 
>         pl->link_config.speed = state->speed;
>         pl->link_config.duplex = state->duplex;
>         pl->link_config.link = 1;
>         pl->link_config.an_complete = 1;
> 
>         pl->cfg_link_an_mode = MLO_AN_FIXED;
>         pl->cur_link_an_mode = pl->cfg_link_an_mode;
> 
>         return 0;
> }
> 
> You can then call this _instead_ of attaching a PHY to switch phylink
> into fixed-link mode with the specified speed and duplex (assuming
> they are supported by the MAC.)
> 
> Isn't this going to be simpler than trying to use swnodes that need
> to be setup before phylink_create() gets called?
> 

Your suggestion seems to be working well for us. I'm currently testing it on
different boards and checking for corner cases.
I plan to submit it for code review next week.

Quick question: Should I submit your suggested code along with our patches, or
will you be submitting it separately?

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

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink

[Russell King (Oracle)]

On Fri, Aug 16, 2024 at 11:08:59PM +0530, Raju Lakkaraju wrote:
> Hi Russell King,
> 
> Thank you for quick response.
> 
> The 08/08/2024 22:07, Russell King (Oracle) wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, Aug 08, 2024 at 08:23:38PM +0000, Ronnie.Kunin@microchip.com wrote:
> > > We looked into an alternate way to migrate our lan743x driver from phylib to phylink continuing to support our existing hardware out in the field, without using the phylib's fixed-phy approach that you opposed to, but without modifying the phylib framework either.
> > > While investigating how to implement it we came across this which Raju borrowed ideas from: https://lore.kernel.org/linux-arm-kernel/YtGPO5SkMZfN8b%2Fs@shell.armlinux.org.uk/ . He is in the process of testing/cleaning it up and expects to submit it early next week.
> > 
> > That series died a death because it wasn't acceptable to the swnode
> > folk. In any case, that's clearly an over-complex solution for what is
> > a simple problem here.
> > 
> > The simplest solution would be for phylink to provide a new function,
> > e.g.
> > 
> > int phylink_set_fixed_link(struct phylink *pl,
> >                            const struct phylink_state *state)
> > {
> >         const struct phy_setting *s;
> >         unsigned long *adv;
> > 
> >         if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
> >             !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
> >                 return -EINVAL;
> > 
> >         s = phy_lookup_setting(state->speed, state->duplex,
> >                                pl->supported, true);
> >         if (!s)
> >                 return -EINVAL;
> > 
> >         adv = pl->link_config.advertising;
> >         linkmode_zero(adv);
> >         linkmode_set_bit(s->bit, adv);
> >         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
> > 
> >         pl->link_config.speed = state->speed;
> >         pl->link_config.duplex = state->duplex;
> >         pl->link_config.link = 1;
> >         pl->link_config.an_complete = 1;
> > 
> >         pl->cfg_link_an_mode = MLO_AN_FIXED;
> >         pl->cur_link_an_mode = pl->cfg_link_an_mode;
> > 
> >         return 0;
> > }
> > 
> > You can then call this _instead_ of attaching a PHY to switch phylink
> > into fixed-link mode with the specified speed and duplex (assuming
> > they are supported by the MAC.)
> > 
> > Isn't this going to be simpler than trying to use swnodes that need
> > to be setup before phylink_create() gets called?
> > 
> 
> Your suggestion seems to be working well for us. I'm currently testing it on
> different boards and checking for corner cases.
> I plan to submit it for code review next week.
> 
> Quick question: Should I submit your suggested code along with our patches, or
> will you be submitting it separately?

Note the point in my signature, which means I won't be doing very much
likely for through the rest of August and - given the timeline I expect,
nothing at all through much of September.

So, please include it as a separate patch with my authorship. You'll
need to add a prototype to linux/phylink.h for it as well. I'm giving
you explicit permission to add my sign-off for such a patch. Thanks.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

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

[PATCH net-next V3 4/4] net: lan743x: Add support to ethtool phylink get and set settings

[Raju Lakkaraju]

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:                                                                    
============                                                                    
V2 -> V3:
  - No change
V1 -> V2:                                                                       
  - Fix the phylink changes                                                                  
 
 .../net/ethernet/microchip/lan743x_ethtool.c  | 118 ++++++------------
 drivers/net/ethernet/microchip/lan743x_main.c |  25 ++++
 drivers/net/ethernet/microchip/lan743x_main.h |   4 +
 3 files changed, 67 insertions(+), 80 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 3a63ec091413..a649ea7442a4 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1058,61 +1058,48 @@ 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;
-	}
 
-	ret = phy_ethtool_get_eee(phydev, eee);
-	if (ret < 0)
-		return ret;
-
-	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;
-	}
+	eee->tx_lpi_timer = lan743x_csr_read(adapter,
+					     MAC_EEE_TX_LPI_REQ_DLY_CNT);
+	eee->eee_enabled = adapter->eee_enabled;
+	eee->eee_active = adapter->eee_active;
+	eee->tx_lpi_enabled = adapter->tx_lpi_enabled;
 
-	return 0;
+	return phylink_ethtool_get_eee(adapter->phylink, eee);
 }
 
 static int lan743x_ethtool_set_eee(struct net_device *netdev,
 				   struct ethtool_keee *eee)
 {
-	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;
-	}
+	if (eee->tx_lpi_enabled)
+		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
+				  eee->tx_lpi_timer);
+	else
+		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0);
 
-	if (eee->eee_enabled) {
-		buf = (u32)eee->tx_lpi_timer;
-		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
-	}
+	adapter->eee_enabled = eee->eee_enabled;
+	adapter->tx_lpi_enabled = eee->tx_lpi_enabled;
+	lan743x_set_eee(adapter, eee->tx_lpi_enabled && eee->eee_enabled);
 
-	return phy_ethtool_set_eee(phydev, eee);
+	return phylink_ethtool_set_eee(adapter->phylink, eee);
+}
+
+static int lan743x_ethtool_set_link_ksettings(struct net_device *netdev,
+					      const struct ethtool_link_ksettings *cmd)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	return phylink_ethtool_ksettings_set(adapter->phylink, cmd);
+}
+
+static int lan743x_ethtool_get_link_ksettings(struct net_device *netdev,
+					      struct ethtool_link_ksettings *cmd)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	return phylink_ethtool_ksettings_get(adapter->phylink, cmd);
 }
 
 #ifdef CONFIG_PM
@@ -1124,8 +1111,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 +1152,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 +1341,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 +1375,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 845a48ca497d..dc46686529a2 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_set_eee(struct lan743x_adapter *adapter, bool enable)
+{
+	u32 lpi_ctl1;
+
+	lpi_ctl1 = lan743x_csr_read(adapter, MAC_CR);
+	if (enable)
+		lpi_ctl1 |= MAC_CR_EEE_EN_;
+	else
+		lpi_ctl1 &= ~MAC_CR_EEE_EN_;
+	lan743x_csr_write(adapter, MAC_CR, lpi_ctl1);
+}
+
 static void lan743x_phylink_mac_config(struct phylink_config *config,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
@@ -3013,7 +3025,12 @@ 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));
+	adapter->eee_active = false;
+	lan743x_set_eee(adapter, false);
 }
 
 static void lan743x_phylink_mac_link_up(struct phylink_config *config,
@@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
 					  cap & FLOW_CTRL_TX,
 					  cap & FLOW_CTRL_RX);
 
+	if (phydev && adapter->eee_enabled) {
+		bool enable;
+
+		adapter->eee_active = phy_init_eee(phydev, false) >= 0;
+		enable = adapter->eee_active && adapter->tx_lpi_enabled;
+		lan743x_set_eee(adapter, enable);
+	}
+
 	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..79f21789eb32 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1086,6 +1086,9 @@ struct lan743x_adapter {
 	phy_interface_t		phy_interface;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
+	bool eee_enabled;
+	bool eee_active;
+	bool tx_lpi_enabled;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
@@ -1206,5 +1209,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_set_eee(struct lan743x_adapter *adapter, bool enable);
 
 #endif /* _LAN743X_H */
-- 
2.34.1

Re: [PATCH net-next V3 4/4] net: lan743x: Add support to ethtool phylink get and set settings

[Russell King (Oracle)]

On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote:
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index 3a63ec091413..a649ea7442a4 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1058,61 +1058,48 @@ 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;
> -	}
>  
> -	ret = phy_ethtool_get_eee(phydev, eee);
> -	if (ret < 0)
> -		return ret;
> -
> -	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;
> -	}
> +	eee->tx_lpi_timer = lan743x_csr_read(adapter,
> +					     MAC_EEE_TX_LPI_REQ_DLY_CNT);
> +	eee->eee_enabled = adapter->eee_enabled;
> +	eee->eee_active = adapter->eee_active;
> +	eee->tx_lpi_enabled = adapter->tx_lpi_enabled;

You really need to start paying attention to the commits other people
make as a result of development to other parts of the kernel.

First, see:

commit ef460a8986fa0dae1cdcb158a06127f7af27c92d
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Sat Apr 6 15:16:00 2024 -0500

    net: lan743x: Fixup EEE

and note that the assignment of eee->eee_enabled, eee->eee_active, and
eee->tx_lpi_enabled were all removed.

Next...

>  
> -	return 0;
> +	return phylink_ethtool_get_eee(adapter->phylink, eee);

phylink_ethtool_get_eee() will call phy_ethtool_get_eee(), which
in turn calls genphy_c45_ethtool_get_eee() and eeecfg_to_eee().

genphy_c45_ethtool_get_eee() will do this:

        data->eee_enabled = is_enabled;
        data->eee_active = ret;

thus overwriting your assignment to eee->eee_enabled and
eee->eee_active.

eeecfg_to_eee() will overwrite eee->tx_lpi_enabled and
eee->eee_enabled.

Thus, writing to these fields and then calling
phylink_ethtool_get_eee() results in these fields being overwritten.

>  static int lan743x_ethtool_set_eee(struct net_device *netdev,
>  				   struct ethtool_keee *eee)
>  {
> -	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;
> -	}
> +	if (eee->tx_lpi_enabled)
> +		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
> +				  eee->tx_lpi_timer);
> +	else
> +		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0);
>  
> -	if (eee->eee_enabled) {
> -		buf = (u32)eee->tx_lpi_timer;
> -		lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> -	}
> +	adapter->eee_enabled = eee->eee_enabled;
> +	adapter->tx_lpi_enabled = eee->tx_lpi_enabled;

Given that phylib stores these and overwrites your copies in the get_eee
method above, do you need to store these?

> @@ -3013,7 +3025,12 @@ 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));
> +	adapter->eee_active = false;

phylib tracks this for you.

> +	lan743x_set_eee(adapter, false);
>  }
>  
>  static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
>  					  cap & FLOW_CTRL_TX,
>  					  cap & FLOW_CTRL_RX);
>  
> +	if (phydev && adapter->eee_enabled) {
> +		bool enable;
> +
> +		adapter->eee_active = phy_init_eee(phydev, false) >= 0;
> +		enable = adapter->eee_active && adapter->tx_lpi_enabled;

No need. Your enable should be conditional on phydev->enable_tx_lpi
here. See Andrew's commit and understand his changes, rather than
just ignoring Andrew's work and continuing with your old pattern of
EEE support. Things have moved forward.

Thanks.

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

Re: [PATCH net-next V3 4/4] net: lan743x: Add support to ethtool phylink get and set settings

[Raju Lakkaraju]

Hi Russell King,

The 07/30/2024 16:11, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote:
> > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > index 3a63ec091413..a649ea7442a4 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > @@ -1058,61 +1058,48 @@ 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;
> > -     }
> >
> > -     ret = phy_ethtool_get_eee(phydev, eee);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     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;
> > -     }
> > +     eee->tx_lpi_timer = lan743x_csr_read(adapter,
> > +                                          MAC_EEE_TX_LPI_REQ_DLY_CNT);
> > +     eee->eee_enabled = adapter->eee_enabled;
> > +     eee->eee_active = adapter->eee_active;
> > +     eee->tx_lpi_enabled = adapter->tx_lpi_enabled;
> 
> You really need to start paying attention to the commits other people
> make as a result of development to other parts of the kernel.
> 
> First, see:
> 
> commit ef460a8986fa0dae1cdcb158a06127f7af27c92d
> Author: Andrew Lunn <andrew@lunn.ch>
> Date:   Sat Apr 6 15:16:00 2024 -0500
> 
>     net: lan743x: Fixup EEE
> 
> and note that the assignment of eee->eee_enabled, eee->eee_active, and
> eee->tx_lpi_enabled were all removed.
> 

Accepted. I will fix

> Next...
> 
> >
> > -     return 0;
> > +     return phylink_ethtool_get_eee(adapter->phylink, eee);
> 
> phylink_ethtool_get_eee() will call phy_ethtool_get_eee(), which
> in turn calls genphy_c45_ethtool_get_eee() and eeecfg_to_eee().
> 
> genphy_c45_ethtool_get_eee() will do this:
> 
>         data->eee_enabled = is_enabled;
>         data->eee_active = ret;
> 
> thus overwriting your assignment to eee->eee_enabled and
> eee->eee_active.
> 
> eeecfg_to_eee() will overwrite eee->tx_lpi_enabled and
> eee->eee_enabled.
> 
> Thus, writing to these fields and then calling
> phylink_ethtool_get_eee() results in these fields being overwritten.
> 

Ok. I will fix.

> >  static int lan743x_ethtool_set_eee(struct net_device *netdev,
> >                                  struct ethtool_keee *eee)
> >  {
> > -     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;
> > -     }
> > +     if (eee->tx_lpi_enabled)
> > +             lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
> > +                               eee->tx_lpi_timer);
> > +     else
> > +             lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0);
> >
> > -     if (eee->eee_enabled) {
> > -             buf = (u32)eee->tx_lpi_timer;
> > -             lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> > -     }
> > +     adapter->eee_enabled = eee->eee_enabled;
> > +     adapter->tx_lpi_enabled = eee->tx_lpi_enabled;
> 
> Given that phylib stores these and overwrites your copies in the get_eee
> method above, do you need to store these?
> 

OK

> > @@ -3013,7 +3025,12 @@ 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));
> > +     adapter->eee_active = false;
> 
> phylib tracks this for you.
> 

Ok

> > +     lan743x_set_eee(adapter, false);
> >  }
> >
> >  static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> > @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> >                                         cap & FLOW_CTRL_TX,
> >                                         cap & FLOW_CTRL_RX);
> >
> > +     if (phydev && adapter->eee_enabled) {
> > +             bool enable;
> > +
> > +             adapter->eee_active = phy_init_eee(phydev, false) >= 0;
> > +             enable = adapter->eee_active && adapter->tx_lpi_enabled;
> 
> No need. Your enable should be conditional on phydev->enable_tx_lpi
> here. See Andrew's commit and understand his changes, rather than
> just ignoring Andrew's work and continuing with your old pattern of
> EEE support. Things have moved forward.

Ok. I will fix

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

-- 
Thanks,                                                                         
Raju