[PATCH net-next v5 0/6] Convert LAN78xx to PHYLINK

[PATCH net-next v5 0/6] Convert LAN78xx to PHYLINK

[Oleksij Rempel]

changes v5:
- merge ethtool pause configuration patch with PHYlink patch
- merge some other small cleanup to a single patch

changes v4:
- split "Improve error handling in PHY initialization" patch and move
  some parts before PHYlink porting to address some of compile warning
  as early as possible.
- add cleanup patch to remove unused struct members

This patch series refactors the LAN78xx USB Ethernet driver to use the
PHYLINK API.


Oleksij Rempel (6):
  net: usb: lan78xx: Improve error handling in PHY initialization
  net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC
    management
  net: usb: lan78xx: Use ethtool_op_get_link to reflect current link
    status
  net: usb: lan78xx: port link settings to phylink API
  net: usb: lan78xx: Integrate EEE support with phylink LPI API
  net: usb: lan78xx: remove unused struct members

 drivers/net/usb/Kconfig   |   3 +-
 drivers/net/usb/lan78xx.c | 820 ++++++++++++++++++--------------------
 2 files changed, 394 insertions(+), 429 deletions(-)

--
2.39.5

[PATCH net-next v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[Oleksij Rempel]

Ensure that return values from `lan78xx_write_reg()`,
`lan78xx_read_reg()`, and `phy_find_first()` are properly checked and
propagated. Use `ERR_PTR(ret)` for error reporting in
`lan7801_phy_init()` and replace `-EIO` with `-ENODEV` where appropriate
to provide more accurate error codes.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v5:
- make sure lan7801_phy_init() caller is testing against IS_ERR
  instead of NULL.
changes v4:
- split the patch and move part of it before PHYlink migration
---
 drivers/net/usb/lan78xx.c | 47 ++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 137adf6d5b08..13b5da18850a 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2510,14 +2510,13 @@ static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
 
 static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 {
-	u32 buf;
-	int ret;
 	struct fixed_phy_status fphy_status = {
 		.link = 1,
 		.speed = SPEED_1000,
 		.duplex = DUPLEX_FULL,
 	};
 	struct phy_device *phydev;
+	int ret;
 
 	phydev = phy_find_first(dev->mdiobus);
 	if (!phydev) {
@@ -2525,30 +2524,40 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
 		if (IS_ERR(phydev)) {
 			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
-			return NULL;
+			return ERR_PTR(-ENODEV);
 		}
 		netdev_dbg(dev->net, "Registered FIXED PHY\n");
 		dev->interface = PHY_INTERFACE_MODE_RGMII;
 		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
 					MAC_RGMII_ID_TXC_DELAY_EN_);
+		if (ret < 0)
+			return ERR_PTR(ret);
+
 		ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
-		ret = lan78xx_read_reg(dev, HW_CFG, &buf);
-		buf |= HW_CFG_CLK125_EN_;
-		buf |= HW_CFG_REFCLK25_EN_;
-		ret = lan78xx_write_reg(dev, HW_CFG, buf);
+		if (ret < 0)
+			return ERR_PTR(ret);
+
+		ret = lan78xx_update_reg(dev, HW_CFG, HW_CFG_CLK125_EN_ |
+					 HW_CFG_REFCLK25_EN_,
+					 HW_CFG_CLK125_EN_ | HW_CFG_REFCLK25_EN_);
+		if (ret < 0)
+			return ERR_PTR(ret);
 	} else {
 		if (!phydev->drv) {
 			netdev_err(dev->net, "no PHY driver found\n");
-			return NULL;
+			return ERR_PTR(-EINVAL);
 		}
 		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
 		/* The PHY driver is responsible to configure proper RGMII
 		 * interface delays. Disable RGMII delays on MAC side.
 		 */
-		lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
+		ret = lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
+		if (ret < 0)
+			return ERR_PTR(ret);
 
 		phydev->is_internal = false;
 	}
+
 	return phydev;
 }
 
@@ -2562,9 +2571,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	switch (dev->chipid) {
 	case ID_REV_CHIP_ID_7801_:
 		phydev = lan7801_phy_init(dev);
-		if (!phydev) {
-			netdev_err(dev->net, "lan7801: PHY Init Failed");
-			return -EIO;
+		if (IS_ERR(phydev)) {
+			netdev_err(dev->net, "lan7801: failed to init PHY: %pe\n",
+				   phydev);
+			return PTR_ERR(phydev);
 		}
 		break;
 
@@ -2573,7 +2583,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		phydev = phy_find_first(dev->mdiobus);
 		if (!phydev) {
 			netdev_err(dev->net, "no PHY found\n");
-			return -EIO;
+			return -ENODEV;
 		}
 		phydev->is_internal = true;
 		dev->interface = PHY_INTERFACE_MODE_GMII;
@@ -2581,7 +2591,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 
 	default:
 		netdev_err(dev->net, "Unknown CHIP ID found\n");
-		return -EIO;
+		return -ENODEV;
 	}
 
 	/* if phyirq is not set, use polling mode in phylib */
@@ -2633,7 +2643,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 						      sizeof(u32));
 		if (len >= 0) {
 			/* Ensure the appropriate LEDs are enabled */
-			lan78xx_read_reg(dev, HW_CFG, &reg);
+			ret = lan78xx_read_reg(dev, HW_CFG, &reg);
+			if (ret < 0)
+				return ret;
+
 			reg &= ~(HW_CFG_LED0_EN_ |
 				 HW_CFG_LED1_EN_ |
 				 HW_CFG_LED2_EN_ |
@@ -2642,7 +2655,9 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 				(len > 1) * HW_CFG_LED1_EN_ |
 				(len > 2) * HW_CFG_LED2_EN_ |
 				(len > 3) * HW_CFG_LED3_EN_;
-			lan78xx_write_reg(dev, HW_CFG, reg);
+			ret = lan78xx_write_reg(dev, HW_CFG, reg);
+			if (ret < 0)
+				return ret;
 		}
 	}
 
-- 
2.39.5

[PATCH net-next v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Oleksij Rempel]

Convert the LAN78xx driver to use the PHYlink framework for managing
PHY and MAC interactions.

Key changes include:
- Replace direct PHY operations with phylink equivalents (e.g.,
  phylink_start, phylink_stop).
- Introduce lan78xx_phylink_setup for phylink initialization and
  configuration.
- Add phylink MAC operations (lan78xx_mac_config,
  lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
  settings and flow control.
- Remove redundant and now phylink-managed functions like
  `lan78xx_link_status_change`.
- update lan78xx_get/set_pause to use phylink helpers

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v5:
- merge ethtool pause interface changes to this patch
changes v4:
- add PHYLINK dependency
- remove PHYLIB and FIXED_PHY, both are replaced by PHYLINK
changes v3:
- lan78xx_phy_init: drop phy_suspend()
- lan78xx_phylink_setup: use phy_interface_set_rgmii()
changes v2:
- lan78xx_mac_config: remove unused rgmii_id
- lan78xx_mac_config: PHY_INTERFACE_MODE_RGMII* variants
- lan78xx_mac_config: remove auto-speed and duplex configuration
- lan78xx_phylink_setup: set link_interface to PHY_INTERFACE_MODE_RGMII_ID
  instead of PHY_INTERFACE_MODE_NA.
- lan78xx_phy_init: use phylink_set_fixed_link() instead of allocating
  fixed PHY.
- lan78xx_configure_usb: move function values to separate variables

20220427_lukas_polling_be_gone_on_lan95xx.cover
---
 drivers/net/usb/Kconfig   |   3 +-
 drivers/net/usb/lan78xx.c | 615 ++++++++++++++++++--------------------
 2 files changed, 298 insertions(+), 320 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 3c360d4f0635..71168e47a9b1 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -115,9 +115,8 @@ config USB_RTL8152
 config USB_LAN78XX
 	tristate "Microchip LAN78XX Based USB Ethernet Adapters"
 	select MII
-	select PHYLIB
+	select PHYLINK
 	select MICROCHIP_PHY
-	select FIXED_PHY
 	select CRC32
 	help
 	  This option adds support for Microchip LAN78XX based USB 2
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 13b5da18850a..fd6e80f9c35f 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -6,6 +6,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
+#include <linux/phylink.h>
 #include <linux/usb.h>
 #include <linux/crc32.h>
 #include <linux/signal.h>
@@ -384,7 +385,7 @@ struct skb_data {		/* skb->cb is one of these */
 #define EVENT_RX_HALT			1
 #define EVENT_RX_MEMORY			2
 #define EVENT_STS_SPLIT			3
-#define EVENT_LINK_RESET		4
+#define EVENT_PHY_INT_ACK		4
 #define EVENT_RX_PAUSED			5
 #define EVENT_DEV_WAKING		6
 #define EVENT_DEV_ASLEEP		7
@@ -470,6 +471,9 @@ struct lan78xx_net {
 	struct statstage	stats;
 
 	struct irq_domain_data	domain_data;
+
+	struct phylink		*phylink;
+	struct phylink_config	phylink_config;
 };
 
 /* use ethtool to change the level for any given device */
@@ -1554,40 +1558,6 @@ static void lan78xx_set_multicast(struct net_device *netdev)
 	schedule_work(&pdata->set_multicast);
 }
 
-static int lan78xx_update_flowcontrol(struct lan78xx_net *dev, u8 duplex,
-				      u16 lcladv, u16 rmtadv)
-{
-	u32 flow = 0, fct_flow = 0;
-	u8 cap;
-
-	if (dev->fc_autoneg)
-		cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
-	else
-		cap = dev->fc_request_control;
-
-	if (cap & FLOW_CTRL_TX)
-		flow |= (FLOW_CR_TX_FCEN_ | 0xFFFF);
-
-	if (cap & FLOW_CTRL_RX)
-		flow |= FLOW_CR_RX_FCEN_;
-
-	if (dev->udev->speed == USB_SPEED_SUPER)
-		fct_flow = FLOW_CTRL_THRESHOLD(FLOW_ON_SS, FLOW_OFF_SS);
-	else if (dev->udev->speed == USB_SPEED_HIGH)
-		fct_flow = FLOW_CTRL_THRESHOLD(FLOW_ON_HS, FLOW_OFF_HS);
-
-	netif_dbg(dev, link, dev->net, "rx pause %s, tx pause %s",
-		  (cap & FLOW_CTRL_RX ? "enabled" : "disabled"),
-		  (cap & FLOW_CTRL_TX ? "enabled" : "disabled"));
-
-	lan78xx_write_reg(dev, FCT_FLOW, fct_flow);
-
-	/* threshold value should be set before enabling flow */
-	lan78xx_write_reg(dev, FLOW, flow);
-
-	return 0;
-}
-
 static void lan78xx_rx_urb_submit_all(struct lan78xx_net *dev);
 
 static int lan78xx_mac_reset(struct lan78xx_net *dev)
@@ -1636,99 +1606,10 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev)
 	return ret;
 }
 
-static int lan78xx_link_reset(struct lan78xx_net *dev)
+static int lan78xx_phy_int_ack(struct lan78xx_net *dev)
 {
-	struct phy_device *phydev = dev->net->phydev;
-	struct ethtool_link_ksettings ecmd;
-	int ladv, radv, ret, link;
-	u32 buf;
-
 	/* clear LAN78xx interrupt status */
-	ret = lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_);
-	if (unlikely(ret < 0))
-		return ret;
-
-	mutex_lock(&phydev->lock);
-	phy_read_status(phydev);
-	link = phydev->link;
-	mutex_unlock(&phydev->lock);
-
-	if (!link && dev->link_on) {
-		dev->link_on = false;
-
-		/* reset MAC */
-		ret = lan78xx_mac_reset(dev);
-		if (ret < 0)
-			return ret;
-
-		del_timer(&dev->stat_monitor);
-	} else if (link && !dev->link_on) {
-		dev->link_on = true;
-
-		phy_ethtool_ksettings_get(phydev, &ecmd);
-
-		if (dev->udev->speed == USB_SPEED_SUPER) {
-			if (ecmd.base.speed == 1000) {
-				/* disable U2 */
-				ret = lan78xx_read_reg(dev, USB_CFG1, &buf);
-				if (ret < 0)
-					return ret;
-				buf &= ~USB_CFG1_DEV_U2_INIT_EN_;
-				ret = lan78xx_write_reg(dev, USB_CFG1, buf);
-				if (ret < 0)
-					return ret;
-				/* enable U1 */
-				ret = lan78xx_read_reg(dev, USB_CFG1, &buf);
-				if (ret < 0)
-					return ret;
-				buf |= USB_CFG1_DEV_U1_INIT_EN_;
-				ret = lan78xx_write_reg(dev, USB_CFG1, buf);
-				if (ret < 0)
-					return ret;
-			} else {
-				/* enable U1 & U2 */
-				ret = lan78xx_read_reg(dev, USB_CFG1, &buf);
-				if (ret < 0)
-					return ret;
-				buf |= USB_CFG1_DEV_U2_INIT_EN_;
-				buf |= USB_CFG1_DEV_U1_INIT_EN_;
-				ret = lan78xx_write_reg(dev, USB_CFG1, buf);
-				if (ret < 0)
-					return ret;
-			}
-		}
-
-		ladv = phy_read(phydev, MII_ADVERTISE);
-		if (ladv < 0)
-			return ladv;
-
-		radv = phy_read(phydev, MII_LPA);
-		if (radv < 0)
-			return radv;
-
-		netif_dbg(dev, link, dev->net,
-			  "speed: %u duplex: %d anadv: 0x%04x anlpa: 0x%04x",
-			  ecmd.base.speed, ecmd.base.duplex, ladv, radv);
-
-		ret = lan78xx_update_flowcontrol(dev, ecmd.base.duplex, ladv,
-						 radv);
-		if (ret < 0)
-			return ret;
-
-		if (!timer_pending(&dev->stat_monitor)) {
-			dev->delta = 1;
-			mod_timer(&dev->stat_monitor,
-				  jiffies + STAT_UPDATE_TIMER);
-		}
-
-		lan78xx_rx_urb_submit_all(dev);
-
-		local_bh_disable();
-		napi_schedule(&dev->napi);
-		local_bh_enable();
-	}
-
-	return 0;
+	return lan78xx_write_reg(dev, INT_STS, INT_STS_PHY_INT_);
 }
 
 /* some work can't be done in tasklets, so we use keventd
@@ -1757,7 +1638,7 @@ static void lan78xx_status(struct lan78xx_net *dev, struct urb *urb)
 
 	if (intdata & INT_ENP_PHY_INT) {
 		netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n", intdata);
-		lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
+		lan78xx_defer_kevent(dev, EVENT_PHY_INT_ACK);
 
 		if (dev->domain_data.phyirq > 0)
 			generic_handle_irq_safe(dev->domain_data.phyirq);
@@ -2039,63 +1920,16 @@ static void lan78xx_get_pause(struct net_device *net,
 			      struct ethtool_pauseparam *pause)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	struct ethtool_link_ksettings ecmd;
-
-	phy_ethtool_ksettings_get(phydev, &ecmd);
-
-	pause->autoneg = dev->fc_autoneg;
 
-	if (dev->fc_request_control & FLOW_CTRL_TX)
-		pause->tx_pause = 1;
-
-	if (dev->fc_request_control & FLOW_CTRL_RX)
-		pause->rx_pause = 1;
+	phylink_ethtool_get_pauseparam(dev->phylink, pause);
 }
 
 static int lan78xx_set_pause(struct net_device *net,
 			     struct ethtool_pauseparam *pause)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	struct ethtool_link_ksettings ecmd;
-	int ret;
-
-	phy_ethtool_ksettings_get(phydev, &ecmd);
-
-	if (pause->autoneg && !ecmd.base.autoneg) {
-		ret = -EINVAL;
-		goto exit;
-	}
-
-	dev->fc_request_control = 0;
-	if (pause->rx_pause)
-		dev->fc_request_control |= FLOW_CTRL_RX;
 
-	if (pause->tx_pause)
-		dev->fc_request_control |= FLOW_CTRL_TX;
-
-	if (ecmd.base.autoneg) {
-		__ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
-		u32 mii_adv;
-
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-				   ecmd.link_modes.advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-				   ecmd.link_modes.advertising);
-		mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
-		mii_adv_to_linkmode_adv_t(fc, mii_adv);
-		linkmode_or(ecmd.link_modes.advertising, fc,
-			    ecmd.link_modes.advertising);
-
-		phy_ethtool_ksettings_set(phydev, &ecmd);
-	}
-
-	dev->fc_autoneg = pause->autoneg;
-
-	ret = 0;
-exit:
-	return ret;
+	return phylink_ethtool_set_pauseparam(dev->phylink, pause);
 }
 
 static int lan78xx_get_regs_len(struct net_device *netdev)
@@ -2356,26 +2190,6 @@ static void lan78xx_remove_mdio(struct lan78xx_net *dev)
 	mdiobus_free(dev->mdiobus);
 }
 
-static void lan78xx_link_status_change(struct net_device *net)
-{
-	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	u32 data;
-	int ret;
-
-	ret = lan78xx_read_reg(dev, MAC_CR, &data);
-	if (ret < 0)
-		return;
-
-	if (phydev->enable_tx_lpi)
-		data |=  MAC_CR_EEE_EN_;
-	else
-		data &= ~MAC_CR_EEE_EN_;
-	lan78xx_write_reg(dev, MAC_CR, data);
-
-	phy_print_status(phydev);
-}
-
 static int irq_map(struct irq_domain *d, unsigned int irq,
 		   irq_hw_number_t hwirq)
 {
@@ -2508,26 +2322,210 @@ static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
 	dev->domain_data.irqdomain = NULL;
 }
 
+static void lan78xx_mac_config(struct phylink_config *config, unsigned int mode,
+			       const struct phylink_link_state *state)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+	u32 mac_cr = 0;
+	int ret;
+
+	/* Check if the mode is supported */
+	if (mode != MLO_AN_FIXED && mode != MLO_AN_PHY) {
+		netdev_err(net, "Unsupported negotiation mode: %u\n", mode);
+		return;
+	}
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_INTERNAL:
+	case PHY_INTERFACE_MODE_GMII:
+		mac_cr |= MAC_CR_GMII_EN_;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		break;
+	default:
+		netdev_warn(net, "Unsupported interface mode: %d\n",
+			    state->interface);
+		return;
+	}
+
+	ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_GMII_EN_, mac_cr);
+	if (ret < 0)
+		netdev_err(net, "Failed to config MAC with error %pe\n",
+			   ERR_PTR(ret));
+}
+
+static void lan78xx_mac_link_down(struct phylink_config *config,
+				  unsigned int mode, phy_interface_t interface)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+	int ret;
+
+	/* MAC reset will not de-assert TXEN/RXEN, we need to stop them
+	 * manually before reset. TX and RX should be disabled before running
+	 * link_up sequence.
+	 */
+	ret = lan78xx_stop_tx_path(dev);
+	if (ret < 0)
+		goto link_down_fail;
+
+	ret = lan78xx_stop_rx_path(dev);
+	if (ret < 0)
+		goto link_down_fail;
+
+	/* MAC reset seems to not affect MAC configuration, no idea if it is
+	 * really needed, but it was done in previous driver version. So, leave
+	 * it here.
+	 */
+	ret = lan78xx_mac_reset(dev);
+	if (ret < 0)
+		goto link_down_fail;
+
+	return;
+
+link_down_fail:
+	netdev_err(dev->net, "Failed to set MAC down with error %pe\n",
+		   ERR_PTR(ret));
+}
+
+static int lan78xx_configure_usb(struct lan78xx_net *dev, int speed)
+{
+	u32 mask, val;
+	int ret;
+
+	/* Return early if USB speed is not SuperSpeed */
+	if (dev->udev->speed != USB_SPEED_SUPER)
+		return 0;
+
+	/* Update U1 and U2 settings based on speed */
+	if (speed != SPEED_1000) {
+		mask = USB_CFG1_DEV_U2_INIT_EN_ | USB_CFG1_DEV_U1_INIT_EN_;
+		val = USB_CFG1_DEV_U2_INIT_EN_ | USB_CFG1_DEV_U1_INIT_EN_;
+		return lan78xx_update_reg(dev, USB_CFG1, mask, val);
+	}
+
+	/* For 1000 Mbps: disable U2 and enable U1 */
+	mask = USB_CFG1_DEV_U2_INIT_EN_;
+	val = 0;
+	ret = lan78xx_update_reg(dev, USB_CFG1, mask, val);
+	if (ret < 0)
+		return ret;
+
+	mask = USB_CFG1_DEV_U1_INIT_EN_;
+	val = USB_CFG1_DEV_U1_INIT_EN_;
+	return lan78xx_update_reg(dev, USB_CFG1, mask, val);
+}
+
+static int lan78xx_configure_flowcontrol(struct lan78xx_net *dev,
+					 bool tx_pause, bool rx_pause)
+{
+	u32 flow = 0, fct_flow = 0;
+	int ret;
+
+	if (tx_pause)
+		flow |= (FLOW_CR_TX_FCEN_ | 0xFFFF);
+
+	if (rx_pause)
+		flow |= FLOW_CR_RX_FCEN_;
+
+	if (dev->udev->speed == USB_SPEED_SUPER)
+		fct_flow = FLOW_CTRL_THRESHOLD(FLOW_ON_SS, FLOW_OFF_SS);
+	else if (dev->udev->speed == USB_SPEED_HIGH)
+		fct_flow = FLOW_CTRL_THRESHOLD(FLOW_ON_HS, FLOW_OFF_HS);
+
+	ret = lan78xx_write_reg(dev, FCT_FLOW, fct_flow);
+	if (ret < 0)
+		return ret;
+
+	/* threshold value should be set before enabling flow */
+	return lan78xx_write_reg(dev, FLOW, flow);
+}
+
+static void lan78xx_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)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+	u32 mac_cr = 0;
+	int ret;
+
+	switch (speed) {
+	case SPEED_1000:
+		mac_cr |= MAC_CR_SPEED_1000_;
+		break;
+	case SPEED_100:
+		mac_cr |= MAC_CR_SPEED_100_;
+		break;
+	case SPEED_10:
+		mac_cr |= MAC_CR_SPEED_10_;
+		break;
+	default:
+		netdev_err(dev->net, "Unsupported speed %d\n", speed);
+		return;
+	}
+
+	if (duplex == DUPLEX_FULL)
+		mac_cr |= MAC_CR_FULL_DUPLEX_;
+
+	/* make sure TXEN and RXEN are disabled before reconfiguring MAC */
+	ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_SPEED_MASK_ |
+				 MAC_CR_FULL_DUPLEX_ | MAC_CR_EEE_EN_, mac_cr);
+	if (ret < 0)
+		goto link_up_fail;
+
+	ret = lan78xx_configure_flowcontrol(dev, tx_pause, rx_pause);
+	if (ret < 0)
+		goto link_up_fail;
+
+	ret = lan78xx_configure_usb(dev, speed);
+	if (ret < 0)
+		goto link_up_fail;
+
+	lan78xx_rx_urb_submit_all(dev);
+
+	ret = lan78xx_flush_rx_fifo(dev);
+	if (ret < 0)
+		goto link_up_fail;
+
+	ret = lan78xx_flush_tx_fifo(dev);
+	if (ret < 0)
+		goto link_up_fail;
+
+	ret = lan78xx_start_tx_path(dev);
+	if (ret < 0)
+		goto link_up_fail;
+
+	ret = lan78xx_start_rx_path(dev);
+	if (ret < 0)
+		goto link_up_fail;
+
+	return;
+link_up_fail:
+	netdev_err(dev->net, "Failed to set MAC up with error %pe\n",
+		   ERR_PTR(ret));
+}
+
+static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
+	.mac_config = lan78xx_mac_config,
+	.mac_link_down = lan78xx_mac_link_down,
+	.mac_link_up = lan78xx_mac_link_up,
+};
+
 static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 {
-	struct fixed_phy_status fphy_status = {
-		.link = 1,
-		.speed = SPEED_1000,
-		.duplex = DUPLEX_FULL,
-	};
 	struct phy_device *phydev;
 	int ret;
 
 	phydev = phy_find_first(dev->mdiobus);
 	if (!phydev) {
-		netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
-		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
-		if (IS_ERR(phydev)) {
-			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
-			return ERR_PTR(-ENODEV);
-		}
-		netdev_dbg(dev->net, "Registered FIXED PHY\n");
-		dev->interface = PHY_INTERFACE_MODE_RGMII;
+		netdev_dbg(dev->net, "PHY Not Found!! Forcing RGMII configuration\n");
 		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
 					MAC_RGMII_ID_TXC_DELAY_EN_);
 		if (ret < 0)
@@ -2547,7 +2545,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 			netdev_err(dev->net, "no PHY driver found\n");
 			return ERR_PTR(-EINVAL);
 		}
-		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
+		phydev->interface = PHY_INTERFACE_MODE_RGMII_ID;
 		/* The PHY driver is responsible to configure proper RGMII
 		 * interface delays. Disable RGMII delays on MAC side.
 		 */
@@ -2561,21 +2559,64 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 	return phydev;
 }
 
+static int lan78xx_phylink_setup(struct lan78xx_net *dev)
+{
+	struct phylink_config *pc = &dev->phylink_config;
+	phy_interface_t link_interface;
+	struct phylink *phylink;
+
+	pc->dev = &dev->net->dev;
+	pc->type = PHYLINK_NETDEV;
+	pc->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
+			       MAC_100 | MAC_1000FD;
+	pc->mac_managed_pm = true;
+
+	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+		phy_interface_set_rgmii(pc->supported_interfaces);
+		link_interface = PHY_INTERFACE_MODE_RGMII_ID;
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  pc->supported_interfaces);
+		link_interface = PHY_INTERFACE_MODE_INTERNAL;
+	}
+
+	phylink = phylink_create(pc, dev->net->dev.fwnode,
+				 link_interface, &lan78xx_phylink_mac_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
+
+	dev->phylink = phylink;
+
+	return 0;
+}
+
 static int lan78xx_phy_init(struct lan78xx_net *dev)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
-	int ret;
-	u32 mii_adv;
 	struct phy_device *phydev;
+	int ret;
 
 	switch (dev->chipid) {
 	case ID_REV_CHIP_ID_7801_:
 		phydev = lan7801_phy_init(dev);
+		/* If no PHY found, set fixed link, probably there is no
+		 * device or some kind of different device like switch.
+		 * For example: EVB-KSZ9897-1 (KSZ9897 switch evaluation board
+		 * with LAN7801 & KSZ9031)
+		 */
 		if (IS_ERR(phydev)) {
-			netdev_err(dev->net, "lan7801: failed to init PHY: %pe\n",
-				   phydev);
-			return PTR_ERR(phydev);
+			struct phylink_link_state state = {
+				.speed = SPEED_1000,
+				.duplex = DUPLEX_FULL,
+				.interface = PHY_INTERFACE_MODE_RGMII,
+			};
+
+			ret = phylink_set_fixed_link(dev->phylink, &state);
+			if (ret)
+				netdev_err(dev->net, "Could not set fixed link\n");
+
+			return ret;
 		}
+
 		break;
 
 	case ID_REV_CHIP_ID_7800_:
@@ -2586,7 +2627,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 			return -ENODEV;
 		}
 		phydev->is_internal = true;
-		dev->interface = PHY_INTERFACE_MODE_GMII;
+		phydev->interface = PHY_INTERFACE_MODE_GMII;
 		break;
 
 	default:
@@ -2601,37 +2642,13 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		phydev->irq = PHY_POLL;
 	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
 
-	/* set to AUTOMDIX */
-	phydev->mdix = ETH_TP_MDI_AUTO;
-
-	ret = phy_connect_direct(dev->net, phydev,
-				 lan78xx_link_status_change,
-				 dev->interface);
+	ret = phylink_connect_phy(dev->phylink, phydev);
 	if (ret) {
-		netdev_err(dev->net, "can't attach PHY to %s\n",
-			   dev->mdiobus->id);
-		if (dev->chipid == ID_REV_CHIP_ID_7801_) {
-			if (phy_is_pseudo_fixed_link(phydev)) {
-				fixed_phy_unregister(phydev);
-				phy_device_free(phydev);
-			}
-		}
-		return -EIO;
+		netdev_err(dev->net, "can't attach PHY to %s, error %pe\n",
+			   dev->mdiobus->id, ERR_PTR(ret));
+		return ret;
 	}
 
-	/* MAC doesn't support 1000T Half */
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	/* support both flow controls */
-	dev->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-			   phydev->advertising);
-	linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-			   phydev->advertising);
-	mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
-	mii_adv_to_linkmode_adv_t(fc, mii_adv);
-	linkmode_or(phydev->advertising, fc, phydev->advertising);
-
 	phy_support_eee(phydev);
 
 	if (phydev->mdio.dev.of_node) {
@@ -2661,10 +2678,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		}
 	}
 
-	genphy_config_aneg(phydev);
-
-	dev->fc_autoneg = phydev->autoneg;
-
 	return 0;
 }
 
@@ -3004,7 +3017,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 	unsigned long timeout;
 	int ret;
 	u32 buf;
-	u8 sig;
 
 	ret = lan78xx_read_reg(dev, HW_CFG, &buf);
 	if (ret < 0)
@@ -3161,22 +3173,12 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 	if (ret < 0)
 		return ret;
 
+	buf &= ~(MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_);
+
 	/* LAN7801 only has RGMII mode */
-	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+	if (dev->chipid == ID_REV_CHIP_ID_7801_)
 		buf &= ~MAC_CR_GMII_EN_;
-		/* Enable Auto Duplex and Auto speed */
-		buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
-	}
 
-	if (dev->chipid == ID_REV_CHIP_ID_7800_ ||
-	    dev->chipid == ID_REV_CHIP_ID_7850_) {
-		ret = lan78xx_read_raw_eeprom(dev, 0, 1, &sig);
-		if (!ret && sig != EEPROM_INDICATOR) {
-			/* Implies there is no external eeprom. Set mac speed */
-			netdev_info(dev->net, "No External EEPROM. Setting MAC Speed\n");
-			buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
-		}
-	}
 	ret = lan78xx_write_reg(dev, MAC_CR, buf);
 	if (ret < 0)
 		return ret;
@@ -3226,9 +3228,11 @@ static int lan78xx_open(struct net_device *net)
 
 	mutex_lock(&dev->dev_mutex);
 
-	phy_start(net->phydev);
+	lan78xx_init_stats(dev);
+
+	napi_enable(&dev->napi);
 
-	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
+	set_bit(EVENT_DEV_OPEN, &dev->flags);
 
 	/* for Link Check */
 	if (dev->urb_intr) {
@@ -3240,31 +3244,9 @@ static int lan78xx_open(struct net_device *net)
 		}
 	}
 
-	ret = lan78xx_flush_rx_fifo(dev);
-	if (ret < 0)
-		goto done;
-	ret = lan78xx_flush_tx_fifo(dev);
-	if (ret < 0)
-		goto done;
-
-	ret = lan78xx_start_tx_path(dev);
-	if (ret < 0)
-		goto done;
-	ret = lan78xx_start_rx_path(dev);
-	if (ret < 0)
-		goto done;
-
-	lan78xx_init_stats(dev);
-
-	set_bit(EVENT_DEV_OPEN, &dev->flags);
+	phylink_start(dev->phylink);
 
 	netif_start_queue(net);
-
-	dev->link_on = false;
-
-	napi_enable(&dev->napi);
-
-	lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
 done:
 	mutex_unlock(&dev->dev_mutex);
 
@@ -3332,12 +3314,7 @@ static int lan78xx_stop(struct net_device *net)
 		   net->stats.rx_packets, net->stats.tx_packets,
 		   net->stats.rx_errors, net->stats.tx_errors);
 
-	/* ignore errors that occur stopping the Tx and Rx data paths */
-	lan78xx_stop_tx_path(dev);
-	lan78xx_stop_rx_path(dev);
-
-	if (net->phydev)
-		phy_stop(net->phydev);
+	phylink_stop(dev->phylink);
 
 	usb_kill_urb(dev->urb_intr);
 
@@ -3347,7 +3324,7 @@ static int lan78xx_stop(struct net_device *net)
 	 */
 	clear_bit(EVENT_TX_HALT, &dev->flags);
 	clear_bit(EVENT_RX_HALT, &dev->flags);
-	clear_bit(EVENT_LINK_RESET, &dev->flags);
+	clear_bit(EVENT_PHY_INT_ACK, &dev->flags);
 	clear_bit(EVENT_STAT_UPDATE, &dev->flags);
 
 	cancel_delayed_work_sync(&dev->wq);
@@ -4271,14 +4248,14 @@ static void lan78xx_delayedwork(struct work_struct *work)
 		}
 	}
 
-	if (test_bit(EVENT_LINK_RESET, &dev->flags)) {
+	if (test_bit(EVENT_PHY_INT_ACK, &dev->flags)) {
 		int ret = 0;
 
-		clear_bit(EVENT_LINK_RESET, &dev->flags);
-		if (lan78xx_link_reset(dev) < 0) {
-			netdev_info(dev->net, "link reset failed (%d)\n",
-				    ret);
-		}
+		clear_bit(EVENT_PHY_INT_ACK, &dev->flags);
+		ret = lan78xx_phy_int_ack(dev);
+		if (ret)
+			netdev_info(dev->net, "PHY INT ack failed (%pe)\n",
+				    ERR_PTR(ret));
 	}
 
 	if (test_bit(EVENT_STAT_UPDATE, &dev->flags)) {
@@ -4352,32 +4329,29 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 	struct lan78xx_net *dev;
 	struct usb_device *udev;
 	struct net_device *net;
-	struct phy_device *phydev;
 
 	dev = usb_get_intfdata(intf);
 	usb_set_intfdata(intf, NULL);
 	if (!dev)
 		return;
 
-	netif_napi_del(&dev->napi);
-
 	udev = interface_to_usbdev(intf);
 	net = dev->net;
 
+	rtnl_lock();
+	phylink_stop(dev->phylink);
+	phylink_disconnect_phy(dev->phylink);
+	rtnl_unlock();
+
+	netif_napi_del(&dev->napi);
+
 	unregister_netdev(net);
 
 	timer_shutdown_sync(&dev->stat_monitor);
 	set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
 	cancel_delayed_work_sync(&dev->wq);
 
-	phydev = net->phydev;
-
-	phy_disconnect(net->phydev);
-
-	if (phy_is_pseudo_fixed_link(phydev)) {
-		fixed_phy_unregister(phydev);
-		phy_device_free(phydev);
-	}
+	phylink_destroy(dev->phylink);
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
@@ -4461,7 +4435,6 @@ static int lan78xx_probe(struct usb_interface *intf,
 		goto out1;
 	}
 
-	/* netdev_printk() needs this */
 	SET_NETDEV_DEV(netdev, &intf->dev);
 
 	dev = netdev_priv(netdev);
@@ -4573,14 +4546,18 @@ static int lan78xx_probe(struct usb_interface *intf,
 	/* driver requires remote-wakeup capability during autosuspend. */
 	intf->needs_remote_wakeup = 1;
 
-	ret = lan78xx_phy_init(dev);
+	ret = lan78xx_phylink_setup(dev);
 	if (ret < 0)
 		goto free_urbs;
 
+	ret = lan78xx_phy_init(dev);
+	if (ret < 0)
+		goto destroy_phylink;
+
 	ret = register_netdev(netdev);
 	if (ret != 0) {
 		netif_err(dev, probe, netdev, "couldn't register the device\n");
-		goto out8;
+		goto disconnect_phy;
 	}
 
 	usb_set_intfdata(intf, dev);
@@ -4595,8 +4572,10 @@ static int lan78xx_probe(struct usb_interface *intf,
 
 	return 0;
 
-out8:
-	phy_disconnect(netdev->phydev);
+disconnect_phy:
+	phylink_disconnect_phy(dev->phylink);
+destroy_phylink:
+	phylink_destroy(dev->phylink);
 free_urbs:
 	usb_free_urb(dev->urb_intr);
 out5:
@@ -5158,7 +5137,7 @@ static int lan78xx_reset_resume(struct usb_interface *intf)
 	if (ret < 0)
 		return ret;
 
-	phy_start(dev->net->phydev);
+	phylink_start(dev->phylink);
 
 	ret = lan78xx_resume(intf);
 
-- 
2.39.5

[PATCH net-next v5 3/6] net: usb: lan78xx: Use ethtool_op_get_link to reflect current link status

[Oleksij Rempel]

Replace the custom lan78xx_get_link implementation with the standard
ethtool_op_get_link helper, which uses netif_carrier_ok to reflect
the current link status accurately.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/usb/lan78xx.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index fd6e80f9c35f..6a6e965ec27f 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1835,18 +1835,6 @@ static int lan78xx_set_eee(struct net_device *net, struct ethtool_keee *edata)
 	return ret;
 }
 
-static u32 lan78xx_get_link(struct net_device *net)
-{
-	u32 link;
-
-	mutex_lock(&net->phydev->lock);
-	phy_read_status(net->phydev);
-	link = net->phydev->link;
-	mutex_unlock(&net->phydev->lock);
-
-	return link;
-}
-
 static void lan78xx_get_drvinfo(struct net_device *net,
 				struct ethtool_drvinfo *info)
 {
@@ -1966,7 +1954,7 @@ lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
 }
 
 static const struct ethtool_ops lan78xx_ethtool_ops = {
-	.get_link	= lan78xx_get_link,
+	.get_link	= ethtool_op_get_link,
 	.nway_reset	= phy_ethtool_nway_reset,
 	.get_drvinfo	= lan78xx_get_drvinfo,
 	.get_msglevel	= lan78xx_get_msglevel,
-- 
2.39.5

[PATCH net-next v5 4/6] net: usb: lan78xx: port link settings to phylink API

[Oleksij Rempel]

Refactor lan78xx_get_link_ksettings and lan78xx_set_link_ksettings to
use the phylink API (phylink_ethtool_ksettings_get and
phylink_ethtool_ksettings_set) instead of directly interfacing with the
PHY. This change simplifies the code and ensures better integration with
the phylink framework for link management.

Additionally, the explicit calls to usb_autopm_get_interface() and
usb_autopm_put_interface() have been removed. These were originally
needed to manage USB power management during register accesses. However,
lan78xx_mdiobus_read() and lan78xx_mdiobus_write() already handle USB
auto power management internally, ensuring that the interface remains
active when necessary. Since there are no other direct register accesses
in these functions that require explicit power management handling, the
extra calls have become redundant and are no longer needed.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
changes v4:
- add explanation why we do not care about usb_autopm in this functions
---
 drivers/net/usb/lan78xx.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 6a6e965ec27f..9ff8e7850e1e 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1862,46 +1862,16 @@ static int lan78xx_get_link_ksettings(struct net_device *net,
 				      struct ethtool_link_ksettings *cmd)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	int ret;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
 
-	phy_ethtool_ksettings_get(phydev, cmd);
-
-	usb_autopm_put_interface(dev->intf);
-
-	return ret;
+	return phylink_ethtool_ksettings_get(dev->phylink, cmd);
 }
 
 static int lan78xx_set_link_ksettings(struct net_device *net,
 				      const struct ethtool_link_ksettings *cmd)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	int ret = 0;
-	int temp;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
-
-	/* change speed & duplex */
-	ret = phy_ethtool_ksettings_set(phydev, cmd);
 
-	if (!cmd->base.autoneg) {
-		/* force link down */
-		temp = phy_read(phydev, MII_BMCR);
-		phy_write(phydev, MII_BMCR, temp | BMCR_LOOPBACK);
-		mdelay(1);
-		phy_write(phydev, MII_BMCR, temp);
-	}
-
-	usb_autopm_put_interface(dev->intf);
-
-	return ret;
+	return phylink_ethtool_ksettings_set(dev->phylink, cmd);
 }
 
 static void lan78xx_get_pause(struct net_device *net,
-- 
2.39.5

[PATCH net-next v5 5/6] net: usb: lan78xx: Integrate EEE support with phylink LPI API

[Oleksij Rempel]

Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver to
fully integrate with the phylink Low Power Idle (LPI) API. This includes:

- Replacing direct calls to `phy_ethtool_get_eee` and `phy_ethtool_set_eee`
  with `phylink_ethtool_get_eee` and `phylink_ethtool_set_eee`.
- Implementing `.mac_enable_tx_lpi` and `.mac_disable_tx_lpi` to control
  LPI transitions via phylink.
- Configuring `lpi_timer_default` to align with recommended values from
  LAN7800 documentation.
- ensure EEE is disabled on controller reset

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v5:
- remove redundant error prints
changes v2:
- use latest PHYlink TX_LPI API
---
 drivers/net/usb/lan78xx.c | 111 +++++++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 44 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 9ff8e7850e1e..074ac4d1cbcb 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1785,54 +1785,15 @@ static int lan78xx_set_wol(struct net_device *netdev,
 static int lan78xx_get_eee(struct net_device *net, struct ethtool_keee *edata)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	struct phy_device *phydev = net->phydev;
-	int ret;
-	u32 buf;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
-
-	ret = phy_ethtool_get_eee(phydev, edata);
-	if (ret < 0)
-		goto exit;
-
-	ret = lan78xx_read_reg(dev, MAC_CR, &buf);
-	if (buf & MAC_CR_EEE_EN_) {
-		/* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
-		ret = lan78xx_read_reg(dev, EEE_TX_LPI_REQ_DLY, &buf);
-		edata->tx_lpi_timer = buf;
-	} else {
-		edata->tx_lpi_timer = 0;
-	}
-
-	ret = 0;
-exit:
-	usb_autopm_put_interface(dev->intf);
 
-	return ret;
+	return phylink_ethtool_get_eee(dev->phylink, edata);
 }
 
 static int lan78xx_set_eee(struct net_device *net, struct ethtool_keee *edata)
 {
 	struct lan78xx_net *dev = netdev_priv(net);
-	int ret;
-	u32 buf;
-
-	ret = usb_autopm_get_interface(dev->intf);
-	if (ret < 0)
-		return ret;
 
-	ret = phy_ethtool_set_eee(net->phydev, edata);
-	if (ret < 0)
-		goto out;
-
-	buf = (u32)edata->tx_lpi_timer;
-	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);
-out:
-	usb_autopm_put_interface(dev->intf);
-
-	return ret;
+	return phylink_ethtool_set_eee(dev->phylink, edata);
 }
 
 static void lan78xx_get_drvinfo(struct net_device *net,
@@ -2470,10 +2431,50 @@ static void lan78xx_mac_link_up(struct phylink_config *config,
 		   ERR_PTR(ret));
 }
 
+static int lan78xx_mac_eee_enable(struct lan78xx_net *dev, bool enable)
+{
+	u32 mac_cr = 0;
+
+	if (enable)
+		mac_cr |= MAC_CR_EEE_EN_;
+
+	/* make sure TXEN and RXEN are disabled before reconfiguring MAC */
+	return lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_, mac_cr);
+}
+
+static void lan78xx_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+
+	lan78xx_mac_eee_enable(dev, false);
+}
+
+static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
+				     bool tx_clk_stop)
+{
+	struct net_device *net = to_net_dev(config->dev);
+	struct lan78xx_net *dev = netdev_priv(net);
+	int ret;
+
+	/* Software should only change this field when Energy Efficient
+	 * Ethernet Enable (EEEEN) is cleared. We ensure that by clearing
+	 * EEEEN during probe, and phylink itself guarantees that
+	 * mac_disable_tx_lpi() will have been previously called.
+	 */
+	ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, timer);
+	if (ret < 0)
+		return ret;
+
+	return lan78xx_mac_eee_enable(dev, true);
+}
+
 static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
 	.mac_config = lan78xx_mac_config,
 	.mac_link_down = lan78xx_mac_link_down,
 	.mac_link_up = lan78xx_mac_link_up,
+	.mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
 };
 
 static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
@@ -2528,6 +2529,26 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
 	pc->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
 			       MAC_100 | MAC_1000FD;
 	pc->mac_managed_pm = true;
+	pc->lpi_capabilities = MAC_100FD | MAC_1000FD;
+	/*
+	 * Default TX LPI (Low Power Idle) request delay count is set to 50us.
+	 *
+	 * Source: LAN7800 Documentation, DS00001992H, Section 15.1.57, Page 204.
+	 *
+	 * Reasoning:
+	 * According to the application note in the LAN7800 documentation, a
+	 * zero delay may negatively impact the TX data path’s ability to
+	 * support Gigabit operation. A value of 50us is recommended as a
+	 * reasonable default when the part operates at Gigabit speeds,
+	 * balancing stability and power efficiency in EEE mode. This delay can
+	 * be increased based on performance testing, as EEE is designed for
+	 * scenarios with mostly idle links and occasional bursts of full
+	 * bandwidth transmission. The goal is to ensure reliable Gigabit
+	 * performance without overly aggressive power optimization during
+	 * inactive periods.
+	 */
+	pc->lpi_timer_default = 50;
+	pc->eee_enabled_default = true;
 
 	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
 		phy_interface_set_rgmii(pc->supported_interfaces);
@@ -2538,6 +2559,10 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
 		link_interface = PHY_INTERFACE_MODE_INTERNAL;
 	}
 
+	memcpy(dev->phylink_config.lpi_interfaces,
+	       dev->phylink_config.supported_interfaces,
+	       sizeof(dev->phylink_config.lpi_interfaces));
+
 	phylink = phylink_create(pc, dev->net->dev.fwnode,
 				 link_interface, &lan78xx_phylink_mac_ops);
 	if (IS_ERR(phylink))
@@ -2607,8 +2632,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		return ret;
 	}
 
-	phy_support_eee(phydev);
-
 	if (phydev->mdio.dev.of_node) {
 		u32 reg;
 		int len;
@@ -3131,7 +3154,7 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 	if (ret < 0)
 		return ret;
 
-	buf &= ~(MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_);
+	buf &= ~(MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_ | MAC_CR_EEE_EN_);
 
 	/* LAN7801 only has RGMII mode */
 	if (dev->chipid == ID_REV_CHIP_ID_7801_)
-- 
2.39.5

[PATCH net-next v5 6/6] net: usb: lan78xx: remove unused struct members

[Oleksij Rempel]

Remove unused members from struct lan78xx_net, including:

    driver_priv
    suspend_count
    link_on
    mdix_ctrl
    interface
    fc_autoneg
    fc_request_control

These fields are no longer used in the driver and can be safely removed
as part of a cleanup.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 074ac4d1cbcb..fc6517bb3671 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -414,7 +414,6 @@ struct lan78xx_net {
 	struct net_device	*net;
 	struct usb_device	*udev;
 	struct usb_interface	*intf;
-	void			*driver_priv;
 
 	unsigned int		tx_pend_data_len;
 	size_t			n_tx_urbs;
@@ -449,23 +448,15 @@ struct lan78xx_net {
 	unsigned long		flags;
 
 	wait_queue_head_t	*wait;
-	unsigned char		suspend_count;
 
 	unsigned int		maxpacket;
 	struct timer_list	stat_monitor;
 
 	unsigned long		data[5];
 
-	int			link_on;
-	u8			mdix_ctrl;
-
 	u32			chipid;
 	u32			chiprev;
 	struct mii_bus		*mdiobus;
-	phy_interface_t		interface;
-
-	int			fc_autoneg;
-	u8			fc_request_control;
 
 	int			delta;
 	struct statstage	stats;
-- 
2.39.5

Re: [PATCH net-next v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Maxime Chevallier]

Hi Oleksij,

On Wed, 19 Mar 2025 09:49:48 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Convert the LAN78xx driver to use the PHYlink framework for managing
> PHY and MAC interactions.
> 
> Key changes include:
> - Replace direct PHY operations with phylink equivalents (e.g.,
>   phylink_start, phylink_stop).
> - Introduce lan78xx_phylink_setup for phylink initialization and
>   configuration.
> - Add phylink MAC operations (lan78xx_mac_config,
>   lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
>   settings and flow control.
> - Remove redundant and now phylink-managed functions like
>   `lan78xx_link_status_change`.
> - update lan78xx_get/set_pause to use phylink helpers
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>


[...]

> @@ -5158,7 +5137,7 @@ static int lan78xx_reset_resume(struct usb_interface *intf)
>  	if (ret < 0)
>  		return ret;
>  
> -	phy_start(dev->net->phydev);
> +	phylink_start(dev->phylink);

You need RTNL to be held when calling this function.

I'm not familiar with USB but from what I get, this function is part of
the resume path (resume by resetting). I think you also need to
address the suspend path, it still has calls to
netif_carrier_off(dev->net), and you may need to use
phylink_suspend() / phylink_resume() ? (not sure)

Maxime

Re: [PATCH net-next v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 11:48:02AM +0100, Maxime Chevallier wrote:
> Hi Oleksij,
> 
> On Wed, 19 Mar 2025 09:49:48 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > Convert the LAN78xx driver to use the PHYlink framework for managing
> > PHY and MAC interactions.
> > 
> > Key changes include:
> > - Replace direct PHY operations with phylink equivalents (e.g.,
> >   phylink_start, phylink_stop).
> > - Introduce lan78xx_phylink_setup for phylink initialization and
> >   configuration.
> > - Add phylink MAC operations (lan78xx_mac_config,
> >   lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
> >   settings and flow control.
> > - Remove redundant and now phylink-managed functions like
> >   `lan78xx_link_status_change`.
> > - update lan78xx_get/set_pause to use phylink helpers
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> 
> [...]
> 
> > @@ -5158,7 +5137,7 @@ static int lan78xx_reset_resume(struct usb_interface *intf)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	phy_start(dev->net->phydev);
> > +	phylink_start(dev->phylink);
> 
> You need RTNL to be held when calling this function.
> 
> I'm not familiar with USB but from what I get, this function is part of
> the resume path (resume by resetting). I think you also need to
> address the suspend path, it still has calls to
> netif_carrier_off(dev->net), and you may need to use
> phylink_suspend() / phylink_resume() ? (not sure)

phylink_suspend/resume are very much preferred in suspend/resume paths
over stop/start.

Remember that phylink_resume() (and phylink_start()) may result in the
link coming up *immediately* so should be the last step. stmmac gets
this wrong, which can cause problems (as well as solving the lack of
RXC issue.)

-- 
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 v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 09:49:47AM +0100, Oleksij Rempel wrote:
>  	} else {
>  		if (!phydev->drv) {
>  			netdev_err(dev->net, "no PHY driver found\n");
> -			return NULL;
> +			return ERR_PTR(-EINVAL);
>  		}

Idly wondering why this driver cares whether the PHY has a driver or
not (it seemingly wants to exclude the generic driver.) Not a reason
to reject the patch, just curious.

-- 
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 v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 09:49:48AM +0100, Oleksij Rempel wrote:
> Convert the LAN78xx driver to use the PHYlink framework for managing
> PHY and MAC interactions.
> 
> Key changes include:
> - Replace direct PHY operations with phylink equivalents (e.g.,
>   phylink_start, phylink_stop).
> - Introduce lan78xx_phylink_setup for phylink initialization and
>   configuration.
> - Add phylink MAC operations (lan78xx_mac_config,
>   lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
>   settings and flow control.
> - Remove redundant and now phylink-managed functions like
>   `lan78xx_link_status_change`.
> - update lan78xx_get/set_pause to use phylink helpers

I don't think this goes into enough detail - there's some subtle changes
going on in this patch.

>  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  {
> -	struct fixed_phy_status fphy_status = {
> -		.link = 1,
> -		.speed = SPEED_1000,
> -		.duplex = DUPLEX_FULL,
> -	};
>  	struct phy_device *phydev;
>  	int ret;
>  
>  	phydev = phy_find_first(dev->mdiobus);
>  	if (!phydev) {
> -		netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> -		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> -		if (IS_ERR(phydev)) {
> -			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> -			return ERR_PTR(-ENODEV);
> -		}
> -		netdev_dbg(dev->net, "Registered FIXED PHY\n");
> -		dev->interface = PHY_INTERFACE_MODE_RGMII;
> +		netdev_dbg(dev->net, "PHY Not Found!! Forcing RGMII configuration\n");

dev->interface is removed.

>  		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
>  					MAC_RGMII_ID_TXC_DELAY_EN_);
>  		if (ret < 0)
> @@ -2547,7 +2545,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  			netdev_err(dev->net, "no PHY driver found\n");
>  			return ERR_PTR(-EINVAL);
>  		}
> -		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;

Here too.

> +		phydev->interface = PHY_INTERFACE_MODE_RGMII_ID;

I'm not sure why this is being set here - the PHY has been found, but
hasn't had phy_connect*() or phy_attach*() called on it yet (which will
write this member of phy_device.)

> +static int lan78xx_phylink_setup(struct lan78xx_net *dev)
> +{
> +	struct phylink_config *pc = &dev->phylink_config;
> +	phy_interface_t link_interface;
> +	struct phylink *phylink;
> +
> +	pc->dev = &dev->net->dev;
> +	pc->type = PHYLINK_NETDEV;
> +	pc->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
> +			       MAC_100 | MAC_1000FD;
> +	pc->mac_managed_pm = true;
> +
> +	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> +		phy_interface_set_rgmii(pc->supported_interfaces);
> +		link_interface = PHY_INTERFACE_MODE_RGMII_ID;
> +	} else {
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  pc->supported_interfaces);
> +		link_interface = PHY_INTERFACE_MODE_INTERNAL;
> +	}

Hmm. This seems to me to be a functional change. lan78xx_phy_init() had
a switch() statement that:

1. for ID_REV_CHIP_ID_7801_, calls lan7801_phy_init().

   For a fixed PHY, sets dev->interface to PHY_INTERFACE_MODE_RGMII for
   a fixed-PHY (and it seems to configure the RGMII interface delays).

   For a normal PHY, sets dev->interface to PHY_INTERFACE_MODE_RGMII_ID
   and apparently disables the MAC-side RGMII delays.

2. for ID_REV_CHIP_ID_7800_ and ID_REV_CHIP_ID_7850_, uses GMII mode
   with an internal PHY. Maybe the internal connection is GMII. Note
   that with PHY_INTERFACE_MODE_INTERNAL, phylink will not restrict
   the speeds, whereas with PHY_INTERFACE_MODE_GMII it will.

So, I think it would make sense to first make this functional change as
a separate patch.

>  		if (IS_ERR(phydev)) {
> -			netdev_err(dev->net, "lan7801: failed to init PHY: %pe\n",
> -				   phydev);
> -			return PTR_ERR(phydev);
> +			struct phylink_link_state state = {
> +				.speed = SPEED_1000,
> +				.duplex = DUPLEX_FULL,
> +				.interface = PHY_INTERFACE_MODE_RGMII,

This member has no effect here. phylink_set_fixed_link() just
reconfigures for a fixed link as if it had been specified by firmware.
It doesn't support changing the interface at this point.

> @@ -2586,7 +2627,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  			return -ENODEV;
>  		}
>  		phydev->is_internal = true;
> -		dev->interface = PHY_INTERFACE_MODE_GMII;
> +		phydev->interface = PHY_INTERFACE_MODE_GMII;

Same as the case above with PHY_INTERFACE_MODE_RGMII_ID.

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 v5 6/6] net: usb: lan78xx: remove unused struct members

[Russell King (Oracle)]

On Wed, Mar 19, 2025 at 09:49:52AM +0100, Oleksij Rempel wrote:
> Remove unused members from struct lan78xx_net, including:
> 
>     driver_priv
>     suspend_count
>     link_on
>     mdix_ctrl
>     interface
>     fc_autoneg
>     fc_request_control
> 
> These fields are no longer used in the driver and can be safely removed
> as part of a cleanup.

Shouldn't these members be removed in the patches that removed their
users? For example, I think "interface" becomes unused in patch 2.

-- 
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 v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Rengarajan.S]

Hi Oleksji,

On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Convert the LAN78xx driver to use the PHYlink framework for managing
> PHY and MAC interactions.
> 
> Key changes include:
> - Replace direct PHY operations with phylink equivalents (e.g.,
>   phylink_start, phylink_stop).
> - Introduce lan78xx_phylink_setup for phylink initialization and
>   configuration.
> - Add phylink MAC operations (lan78xx_mac_config,
>   lan78xx_mac_link_down, lan78xx_mac_link_up) for managing link
>   settings and flow control.
> - Remove redundant and now phylink-managed functions like
>   `lan78xx_link_status_change`.
> - update lan78xx_get/set_pause to use phylink helpers
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v5:
> - merge ethtool pause interface changes to this patch
> changes v4:
> - add PHYLINK dependency
> - remove PHYLIB and FIXED_PHY, both are replaced by PHYLINK
> changes v3:
> - lan78xx_phy_init: drop phy_suspend()
> - lan78xx_phylink_setup: use phy_interface_set_rgmii()
> changes v2:
> - lan78xx_mac_config: remove unused rgmii_id
> - lan78xx_mac_config: PHY_INTERFACE_MODE_RGMII* variants
> - lan78xx_mac_config: remove auto-speed and duplex configuration
> - lan78xx_phylink_setup: set link_interface to
> PHY_INTERFACE_MODE_RGMII_ID
>   instead of PHY_INTERFACE_MODE_NA.
> - lan78xx_phy_init: use phylink_set_fixed_link() instead of
> allocating
>   fixed PHY.
> - lan78xx_configure_usb: move function values to separate variables
> 
> 20220427_lukas_polling_be_gone_on_lan95xx.cover
> ---
>  drivers/net/usb/Kconfig   |   3 +-
>  drivers/net/usb/lan78xx.c | 615 ++++++++++++++++++----------------
> ----
>  2 files changed, 298 insertions(+), 320 deletions(-)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 3c360d4f0635..71168e47a9b1 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -115,9 +115,8 @@ config USB_RTL8152
>  config USB_LAN78XX
>         tristate "Microchip LAN78XX Based USB Ethernet Adapters"
>         select MII
> -       select PHYLIB
> +       select PHYLINK
>         select MICROCHIP_PHY
> -       select FIXED_PHY
>         select CRC32
>         help
>           This option adds support for Microchip LAN78XX based USB 2
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 13b5da18850a..fd6e80f9c35f 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> +static int lan78xx_configure_usb(struct lan78xx_net *dev, int speed)
> +{
> +       u32 mask, val;
> +       int ret;
> +
> +       /* Return early if USB speed is not SuperSpeed */
> +       if (dev->udev->speed != USB_SPEED_SUPER)
> +               return 0;
> +
> +       /* Update U1 and U2 settings based on speed */
> +       if (speed != SPEED_1000) {
> +               mask = USB_CFG1_DEV_U2_INIT_EN_ |
> USB_CFG1_DEV_U1_INIT_EN_;
> +               val = USB_CFG1_DEV_U2_INIT_EN_ |
> USB_CFG1_DEV_U1_INIT_EN_;
> +               return lan78xx_update_reg(dev, USB_CFG1, mask, val);
> +       }
> +
> +       /* For 1000 Mbps: disable U2 and enable U1 */
> +       mask = USB_CFG1_DEV_U2_INIT_EN_;
> +       val = 0;
> +       ret = lan78xx_update_reg(dev, USB_CFG1, mask, val);
> +       if (ret < 0)
> +               return ret;
> +
> +       mask = USB_CFG1_DEV_U1_INIT_EN_;
> +       val = USB_CFG1_DEV_U1_INIT_EN_;
> +       return lan78xx_update_reg(dev, USB_CFG1, mask, val);

Is it possible to combine the mask and val used above into a single
statement and pass it as argument to lan78xx_update_reg. You can have
it as else part.

> +}
> +
> @@ -4352,32 +4329,29 @@ static void lan78xx_disconnect(struct
> usb_interface *intf)
>         struct lan78xx_net *dev;
>         struct usb_device *udev;
>         struct net_device *net;
> -       struct phy_device *phydev;
> 
>         dev = usb_get_intfdata(intf);
>         usb_set_intfdata(intf, NULL);
>         if (!dev)
>                 return;
> 
> -       netif_napi_del(&dev->napi);
> -
>         udev = interface_to_usbdev(intf);
>         net = dev->net;
> 
> +       rtnl_lock();
> +       phylink_stop(dev->phylink);
> +       phylink_disconnect_phy(dev->phylink);
> +       rtnl_unlock();
> +
> +       netif_napi_del(&dev->napi);
> +
>         unregister_netdev(net);
> 
>         timer_shutdown_sync(&dev->stat_monitor);
>         set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
>         cancel_delayed_work_sync(&dev->wq);
> 
> -       phydev = net->phydev;
> -
> -       phy_disconnect(net->phydev);
> -
> -       if (phy_is_pseudo_fixed_link(phydev)) {
> -               fixed_phy_unregister(phydev);
> -               phy_device_free(phydev);
> -       }
> +       phylink_destroy(dev->phylink);

Before destroying phylink here is it possible to add a check to make
sure dev->phylink is allocated properly.

> 
>         usb_scuttle_anchored_urbs(&dev->deferred);
> 
> 
> @@ -5158,7 +5137,7 @@ static int lan78xx_reset_resume(struct
> usb_interface *intf)
>         if (ret < 0)
>                 return ret;
> 
> -       phy_start(dev->net->phydev);
> +       phylink_start(dev->phylink);
> 
>         ret = lan78xx_resume(intf);
> 
> --
> 2.39.5
> 

Re: [PATCH net-next v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[Rengarajan.S]

Hi Oleksij,

On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Ensure that return values from `lan78xx_write_reg()`,
> `lan78xx_read_reg()`, and `phy_find_first()` are properly checked and
> propagated. Use `ERR_PTR(ret)` for error reporting in
> `lan7801_phy_init()` and replace `-EIO` with `-ENODEV` where
> appropriate
> to provide more accurate error codes.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v5:
> - make sure lan7801_phy_init() caller is testing against IS_ERR
>   instead of NULL.
> changes v4:
> - split the patch and move part of it before PHYlink migration
> ---
>  drivers/net/usb/lan78xx.c | 47 ++++++++++++++++++++++++++-----------
> --
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 137adf6d5b08..13b5da18850a 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2510,14 +2510,13 @@ static void lan78xx_remove_irq_domain(struct
> lan78xx_net *dev)
> 
>  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  {
> -       u32 buf;
> -       int ret;
>         struct fixed_phy_status fphy_status = {
>                 .link = 1,
>                 .speed = SPEED_1000,
>                 .duplex = DUPLEX_FULL,
>         };
>         struct phy_device *phydev;
> +       int ret;
> 
>         phydev = phy_find_first(dev->mdiobus);
>         if (!phydev) {
> @@ -2525,30 +2524,40 @@ static struct phy_device
> *lan7801_phy_init(struct lan78xx_net *dev)
>                 phydev = fixed_phy_register(PHY_POLL, &fphy_status,
> NULL);
>                 if (IS_ERR(phydev)) {
>                         netdev_err(dev->net, "No PHY/fixed_PHY
> found\n");
> -                       return NULL;
> +                       return ERR_PTR(-ENODEV);
>                 }
>                 netdev_dbg(dev->net, "Registered FIXED PHY\n");
>                 dev->interface = PHY_INTERFACE_MODE_RGMII;
>                 ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
>                                         MAC_RGMII_ID_TXC_DELAY_EN_);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> +

I noticed that fixed_phy_register is removed in later patches. However,
in the above implementation, if a failure occurs we exit without
unregistering it. To avoid this issue, can we include the patch that
removes fixed_phy_register first to avoid the cleanup scenario?

>                 ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL,
> 0x3D00);
> -               ret = lan78xx_read_reg(dev, HW_CFG, &buf);
> -               buf |= HW_CFG_CLK125_EN_;
> -               buf |= HW_CFG_REFCLK25_EN_;
> -               ret = lan78xx_write_reg(dev, HW_CFG, buf);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> +
> +               ret = lan78xx_update_reg(dev, HW_CFG,
> HW_CFG_CLK125_EN_ |
> +                                        HW_CFG_REFCLK25_EN_,
> +                                        HW_CFG_CLK125_EN_ |
> HW_CFG_REFCLK25_EN_);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
>         } else {
>                 if (!phydev->drv) {
>                         netdev_err(dev->net, "no PHY driver
> found\n");
> -                       return NULL;
> +                       return ERR_PTR(-EINVAL);
>                 }
>                 dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
>                 /* The PHY driver is responsible to configure proper
> RGMII
>                  * interface delays. Disable RGMII delays on MAC
> side.
>                  */
> -               lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
> +               ret = lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> 
>                 phydev->is_internal = false;
>         }
> +
>         return phydev;
>  }
> 
> @@ -2562,9 +2571,10 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>         switch (dev->chipid) {
>         case ID_REV_CHIP_ID_7801_:
>                 phydev = lan7801_phy_init(dev);
> -               if (!phydev) {
> -                       netdev_err(dev->net, "lan7801: PHY Init
> Failed");
> -                       return -EIO;
> +               if (IS_ERR(phydev)) {
> +                       netdev_err(dev->net, "lan7801: failed to init
> PHY: %pe\n",
> +                                  phydev);
> +                       return PTR_ERR(phydev);
>                 }
>                 break;
> 
> @@ -2573,7 +2583,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                 phydev = phy_find_first(dev->mdiobus);
>                 if (!phydev) {
>                         netdev_err(dev->net, "no PHY found\n");
> -                       return -EIO;
> +                       return -ENODEV;
>                 }
>                 phydev->is_internal = true;
>                 dev->interface = PHY_INTERFACE_MODE_GMII;
> @@ -2581,7 +2591,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> 
>         default:
>                 netdev_err(dev->net, "Unknown CHIP ID found\n");
> -               return -EIO;
> +               return -ENODEV;
>         }
> 
>         /* if phyirq is not set, use polling mode in phylib */
> @@ -2633,7 +2643,10 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                                                       sizeof(u32));
>                 if (len >= 0) {
>                         /* Ensure the appropriate LEDs are enabled */
> -                       lan78xx_read_reg(dev, HW_CFG, &reg);
> +                       ret = lan78xx_read_reg(dev, HW_CFG, &reg);
> +                       if (ret < 0)
> +                               return ret;
> +
>                         reg &= ~(HW_CFG_LED0_EN_ |
>                                  HW_CFG_LED1_EN_ |
>                                  HW_CFG_LED2_EN_ |
> @@ -2642,7 +2655,9 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                                 (len > 1) * HW_CFG_LED1_EN_ |
>                                 (len > 2) * HW_CFG_LED2_EN_ |
>                                 (len > 3) * HW_CFG_LED3_EN_;
> -                       lan78xx_write_reg(dev, HW_CFG, reg);
> +                       ret = lan78xx_write_reg(dev, HW_CFG, reg);
> +                       if (ret < 0)
> +                               return ret;
>                 }
>         }
> 
> --
> 2.39.5
> 

Re: [PATCH net-next v5 5/6] net: usb: lan78xx: Integrate EEE support with phylink LPI API

[Rengarajan.S]

Hi Oleksij,

On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx
> driver to
> fully integrate with the phylink Low Power Idle (LPI) API. This
> includes:
> 
> - Replacing direct calls to `phy_ethtool_get_eee` and
> `phy_ethtool_set_eee`
>   with `phylink_ethtool_get_eee` and `phylink_ethtool_set_eee`.
> - Implementing `.mac_enable_tx_lpi` and `.mac_disable_tx_lpi` to
> control
>   LPI transitions via phylink.
> - Configuring `lpi_timer_default` to align with recommended values
> from
>   LAN7800 documentation.
> - ensure EEE is disabled on controller reset
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v5:
> - remove redundant error prints
> changes v2:
> - use latest PHYlink TX_LPI API
> ---
>  drivers/net/usb/lan78xx.c | 111 +++++++++++++++++++++++-------------
> --
>  1 file changed, 67 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 9ff8e7850e1e..074ac4d1cbcb 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> 
> +static int lan78xx_mac_eee_enable(struct lan78xx_net *dev, bool
> enable)
> +{
> +       u32 mac_cr = 0;
> +
> +       if (enable)
> +               mac_cr |= MAC_CR_EEE_EN_;
> +
> +       /* make sure TXEN and RXEN are disabled before reconfiguring
> MAC */
> +       return lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_,
> mac_cr);

Is it possible to add a check to make sure TXEN and RXEN are disabled
before updating the MAC. Is it taken care by phylink itself?

> --
> 2.39.5
> 

Re: [PATCH net-next v5 2/6] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Russell King (Oracle)]

On Tue, Mar 25, 2025 at 05:37:53PM +0000, Rengarajan.S@microchip.com wrote:
> Hi Oleksji,
> 
> On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> >         udev = interface_to_usbdev(intf);
> >         net = dev->net;
> > 
> > +       rtnl_lock();
> > +       phylink_stop(dev->phylink);
> > +       phylink_disconnect_phy(dev->phylink);
> > +       rtnl_unlock();
> > +
> > +       netif_napi_del(&dev->napi);
> > +
> >         unregister_netdev(net);
> > 
> >         timer_shutdown_sync(&dev->stat_monitor);
> >         set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
> >         cancel_delayed_work_sync(&dev->wq);
> > 
> > -       phydev = net->phydev;
> > -
> > -       phy_disconnect(net->phydev);
> > -
> > -       if (phy_is_pseudo_fixed_link(phydev)) {
> > -               fixed_phy_unregister(phydev);
> > -               phy_device_free(phydev);
> > -       }
> > +       phylink_destroy(dev->phylink);
> 
> Before destroying phylink here is it possible to add a check to make
> sure dev->phylink is allocated properly.

Not necessary.

How would dev->phylink not be "allocated properly" ?

If it isn't "allocated properly" then lan78xx_phylink_setup() will
return an error, which will cause lan78xx_probe() to fail. If the
probe function fails, then the driver won't be bound.

Having additional nonsense checks for things that shouldn't ever
happen is annying and detracts from code readability.

-- 
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 v5 5/6] net: usb: lan78xx: Integrate EEE support with phylink LPI API

[Russell King (Oracle)]

On Tue, Mar 25, 2025 at 05:51:05PM +0000, Rengarajan.S@microchip.com wrote:
> Hi Oleksij,
> 
> On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx
> > driver to
> > fully integrate with the phylink Low Power Idle (LPI) API. This
> > includes:
> > 
> > - Replacing direct calls to `phy_ethtool_get_eee` and
> > `phy_ethtool_set_eee`
> >   with `phylink_ethtool_get_eee` and `phylink_ethtool_set_eee`.
> > - Implementing `.mac_enable_tx_lpi` and `.mac_disable_tx_lpi` to
> > control
> >   LPI transitions via phylink.
> > - Configuring `lpi_timer_default` to align with recommended values
> > from
> >   LAN7800 documentation.
> > - ensure EEE is disabled on controller reset
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > changes v5:
> > - remove redundant error prints
> > changes v2:
> > - use latest PHYlink TX_LPI API
> > ---
> >  drivers/net/usb/lan78xx.c | 111 +++++++++++++++++++++++-------------
> > --
> >  1 file changed, 67 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index 9ff8e7850e1e..074ac4d1cbcb 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > 
> > +static int lan78xx_mac_eee_enable(struct lan78xx_net *dev, bool
> > enable)
> > +{
> > +       u32 mac_cr = 0;
> > +
> > +       if (enable)
> > +               mac_cr |= MAC_CR_EEE_EN_;
> > +
> > +       /* make sure TXEN and RXEN are disabled before reconfiguring
> > MAC */
> > +       return lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_,
> > mac_cr);
> 
> Is it possible to add a check to make sure TXEN and RXEN are disabled
> before updating the MAC. Is it taken care by phylink itself?

I suspect this is not true.

On link up, the order of calls that phylink makes is:

	pcs_link_up()
	mac_link_up()
	pcs_enable_eee()
	mac_enable_tx_lpi()

From what I can see, TXEN and RXEN are set by lan78xx_start_tx_path()
and lan78xx_start_rx_path(). These are called from lan78xx_open(),
which *might* happen way before the link comes up. Given the placement
of phy_start() (now phylink_start()) then the above sequence can happen
at *any* moment from that call to phy*_start() onwards, maybe midway
through these two functions.

-- 
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 v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[Andrew Lunn]

> >  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> >  {
> > -       u32 buf;
> > -       int ret;
> >         struct fixed_phy_status fphy_status = {
> >                 .link = 1,
> >                 .speed = SPEED_1000,
> >                 .duplex = DUPLEX_FULL,
> >         };
> >         struct phy_device *phydev;
> > +       int ret;
> > 
> >         phydev = phy_find_first(dev->mdiobus);
> >         if (!phydev) {
> > @@ -2525,30 +2524,40 @@ static struct phy_device
> > *lan7801_phy_init(struct lan78xx_net *dev)
> >                 phydev = fixed_phy_register(PHY_POLL, &fphy_status,
> > NULL);
> >                 if (IS_ERR(phydev)) {
> >                         netdev_err(dev->net, "No PHY/fixed_PHY
> > found\n");
> > -                       return NULL;
> > +                       return ERR_PTR(-ENODEV);
> >                 }
> >                 netdev_dbg(dev->net, "Registered FIXED PHY\n");
> >                 dev->interface = PHY_INTERFACE_MODE_RGMII;
> >                 ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> >                                         MAC_RGMII_ID_TXC_DELAY_EN_);
> > +               if (ret < 0)
> > +                       return ERR_PTR(ret);
> > +
> 
> I noticed that fixed_phy_register is removed in later patches. However,
> in the above implementation, if a failure occurs we exit without
> unregistering it. To avoid this issue, can we include the patch that
> removes fixed_phy_register first to avoid the cleanup scenario?

phylink itself implements fixed phy. So it is being removed later
because it is not needed once the conversation to phylink is
performed. If you remove it here, before the conversion to phylink,
you break the driver when it is using fixed phy.

With this sort of refactoring, you should not break the normal
case. But there is some wiggle room for error cases, which should not
happen, so long as by the end of the patch series, it is all clean.

So i personally don't care about this leak of a fixed link, at this
stage.

	Andrew