* [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:43 ` Russell King (Oracle)
` (2 more replies)
2025-01-08 12:13 ` [PATCH net-next v1 2/7] net: usb: lan78xx: Move fixed PHY cleanup to lan78xx_unbind() Oleksij Rempel
` (5 subsequent siblings)
6 siblings, 3 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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`.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/usb/lan78xx.c | 525 +++++++++++++++++++++-----------------
1 file changed, 287 insertions(+), 238 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index a91bf9c7e31d..6dfd9301279f 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);
@@ -2356,26 +2237,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,6 +2369,207 @@ 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 rgmii_id = 0;
+ 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_ID:
+ break;
+ default:
+ netdev_warn(net, "Unsupported interface mode: %d\n",
+ state->interface);
+ return;
+ }
+
+ /* by the way, make sure auto speed and duplex are disabled */
+ ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_AUTO_DUPLEX_ |
+ MAC_CR_AUTO_SPEED_ | MAC_CR_GMII_EN_, mac_cr);
+ if (ret < 0)
+ goto mac_config_fail;
+
+ ret = lan78xx_write_reg(dev, MAC_RGMII_ID, rgmii_id);
+ if (ret < 0)
+ goto mac_config_fail;
+
+ return;
+
+mac_config_fail:
+ 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)
+{
+ int ret;
+
+ if (dev->udev->speed != USB_SPEED_SUPER)
+ return 0;
+
+ if (speed != SPEED_1000)
+ return lan78xx_update_reg(dev, USB_CFG1,
+ USB_CFG1_DEV_U2_INIT_EN_ |
+ USB_CFG1_DEV_U1_INIT_EN_,
+ USB_CFG1_DEV_U2_INIT_EN_ |
+ USB_CFG1_DEV_U1_INIT_EN_);
+
+ /* disable U2 */
+ ret = lan78xx_update_reg(dev, USB_CFG1,
+ USB_CFG1_DEV_U2_INIT_EN_, 0);
+ if (ret < 0)
+ return ret;
+ /* enable U1 */
+ return lan78xx_update_reg(dev, USB_CFG1,
+ USB_CFG1_DEV_U1_INIT_EN_,
+ USB_CFG1_DEV_U1_INIT_EN_);
+}
+
+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)
{
u32 buf;
@@ -2528,7 +2590,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
return NULL;
}
netdev_dbg(dev->net, "Registered FIXED PHY\n");
- dev->interface = PHY_INTERFACE_MODE_RGMII;
+ phydev->interface = PHY_INTERFACE_MODE_RGMII;
ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
MAC_RGMII_ID_TXC_DELAY_EN_);
ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
@@ -2541,7 +2603,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
netdev_err(dev->net, "no PHY driver found\n");
return NULL;
}
- 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.
*/
@@ -2552,12 +2614,47 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
return phydev;
}
+static int lan78xx_phylink_setup(struct lan78xx_net *dev)
+{
+ phy_interface_t link_interface;
+ struct phylink *phylink;
+
+ dev->phylink_config.dev = &dev->net->dev;
+ dev->phylink_config.type = PHYLINK_NETDEV;
+ dev->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
+ MAC_10 | MAC_100 | MAC_1000FD;
+ dev->phylink_config.mac_managed_pm = true;
+
+ if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+ __set_bit(PHY_INTERFACE_MODE_RGMII,
+ dev->phylink_config.supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_RGMII_ID,
+ dev->phylink_config.supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
+ dev->phylink_config.supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
+ dev->phylink_config.supported_interfaces);
+ link_interface = PHY_INTERFACE_MODE_NA;
+ } else {
+ __set_bit(PHY_INTERFACE_MODE_INTERNAL,
+ dev->phylink_config.supported_interfaces);
+ link_interface = PHY_INTERFACE_MODE_INTERNAL;
+ }
+
+ phylink = phylink_create(&dev->phylink_config, 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_:
@@ -2576,7 +2673,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
return -EIO;
}
phydev->is_internal = true;
- dev->interface = PHY_INTERFACE_MODE_GMII;
+ phydev->interface = PHY_INTERFACE_MODE_INTERNAL;
break;
default:
@@ -2591,12 +2688,7 @@ 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);
@@ -2609,18 +2701,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
return -EIO;
}
- /* 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_suspend(phydev);
phy_support_eee(phydev);
@@ -2646,10 +2727,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
}
}
- genphy_config_aneg(phydev);
-
- dev->fc_autoneg = phydev->autoneg;
-
return 0;
}
@@ -2989,7 +3066,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)
@@ -3146,22 +3222,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;
@@ -3211,9 +3277,11 @@ static int lan78xx_open(struct net_device *net)
mutex_lock(&dev->dev_mutex);
- phy_start(net->phydev);
+ lan78xx_init_stats(dev);
- netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
+ napi_enable(&dev->napi);
+
+ set_bit(EVENT_DEV_OPEN, &dev->flags);
/* for Link Check */
if (dev->urb_intr) {
@@ -3225,31 +3293,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);
@@ -3317,12 +3363,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);
@@ -3332,7 +3373,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);
@@ -4256,13 +4297,13 @@ 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);
+ if (lan78xx_phy_int_ack(dev) < 0) {
+ netdev_info(dev->net, "PHY INT ack failed (%pe)\n",
+ ERR_PTR(ret));
}
}
@@ -4344,26 +4385,29 @@ static void lan78xx_disconnect(struct usb_interface *intf)
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);
lan78xx_unbind(dev, intf);
@@ -4446,7 +4490,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);
@@ -4558,14 +4601,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);
@@ -4580,8 +4627,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:
@@ -5143,7 +5192,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-08 12:13 ` [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management Oleksij Rempel
@ 2025-01-08 12:43 ` Russell King (Oracle)
2025-01-17 10:50 ` Rengarajan.S
2025-01-22 4:02 ` Thangaraj.S
2 siblings, 0 replies; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-08 12:43 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 01:13:35PM +0100, Oleksij Rempel wrote:
> @@ -2508,6 +2369,207 @@ 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 rgmii_id = 0;
> + 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;
The indentation has gone a bit weird here.
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + break;
Normally, a MAC should support all RGMII interface modes, because these
define the RGMII delays at the PHY and have little dependence on the
MAC.
> +static int lan78xx_phylink_setup(struct lan78xx_net *dev)
> +{
> + phy_interface_t link_interface;
> + struct phylink *phylink;
> +
> + dev->phylink_config.dev = &dev->net->dev;
> + dev->phylink_config.type = PHYLINK_NETDEV;
> + dev->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000FD;
> + dev->phylink_config.mac_managed_pm = true;
> +
> + if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> + __set_bit(PHY_INTERFACE_MODE_RGMII,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_ID,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_RXID,
> + dev->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_RGMII_TXID,
> + dev->phylink_config.supported_interfaces);
The mac_config implementation conflicts with this.
> + link_interface = PHY_INTERFACE_MODE_NA;
This is supposed to be for DSA, not for general use. Is there any reason
you can't pass the real mode that is being used here?
> @@ -2576,7 +2673,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> return -EIO;
> }
> phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> + phydev->interface = PHY_INTERFACE_MODE_INTERNAL;
This needs to be explained.
As for continuing to use fixed-phy, please instead use
phylink_set_fixed_link() instead.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-08 12:13 ` [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management Oleksij Rempel
2025-01-08 12:43 ` Russell King (Oracle)
@ 2025-01-17 10:50 ` Rengarajan.S
2025-01-22 8:16 ` Oleksij Rempel
2025-01-22 4:02 ` Thangaraj.S
2 siblings, 1 reply; 33+ messages in thread
From: Rengarajan.S @ 2025-01-17 10:50 UTC (permalink / raw)
To: andrew+netdev, rmk+kernel, davem, Woojung.Huh, pabeni, o.rempel,
edumazet, kuba
Cc: phil, kernel, linux-kernel, netdev, UNGLinuxDriver
Hi Oleksji,
On Wed, 2025-01-08 at 13:13 +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`.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> drivers/net/usb/lan78xx.c | 525 +++++++++++++++++++++---------------
> --
> 1 file changed, 287 insertions(+), 238 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index a91bf9c7e31d..6dfd9301279f 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 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 rgmii_id = 0;
> + 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_ID:
> + break;
> + default:
> + netdev_warn(net, "Unsupported interface mode: %d\n",
> + state->interface);
> + return;
> + }
> +
> + /* by the way, make sure auto speed and duplex are disabled
> */
> + ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_AUTO_DUPLEX_ |
> + MAC_CR_AUTO_SPEED_ |
> MAC_CR_GMII_EN_, mac_cr);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + ret = lan78xx_write_reg(dev, MAC_RGMII_ID, rgmii_id);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + return;
> +
> +mac_config_fail:
> + netdev_err(net, "Failed to config MAC with error %pe\n",
> ERR_PTR(ret));
> +}
> +
> +
> +static int lan78xx_configure_usb(struct lan78xx_net *dev, int speed)
> +{
> + int ret;
> +
> + if (dev->udev->speed != USB_SPEED_SUPER)
> + return 0;
> +
> + if (speed != SPEED_1000)
> + return lan78xx_update_reg(dev, USB_CFG1,
> + USB_CFG1_DEV_U2_INIT_EN_ |
> + USB_CFG1_DEV_U1_INIT_EN_,
> + USB_CFG1_DEV_U2_INIT_EN_ |
> + USB_CFG1_DEV_U1_INIT_EN_);
> +
> + /* disable U2 */
> + ret = lan78xx_update_reg(dev, USB_CFG1,
> + USB_CFG1_DEV_U2_INIT_EN_, 0);
> + if (ret < 0)
> + return ret;
> + /* enable U1 */
> + return lan78xx_update_reg(dev, USB_CFG1,
> + USB_CFG1_DEV_U1_INIT_EN_,
> + USB_CFG1_DEV_U1_INIT_EN_);
Since, in the all the above cases we update the USB_CFG1 based on the
mask and value depending on various speeds, have a variable for mask
and value and use them to represent enabled flags instead of passing
the flags directly for better readability.
For Eg: have mask = USB_CFG1_DEV_U2_INIT_EN_ and val = 0 and pass them
as arguments.
> +}
> +
> +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);
The auto-speed and duplex are disabled in lan78xx_mac_config is it
necessary to disable this again here. You can disable it once to
avoid redundancy.
> + 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,
> +};
If possible, have MAC and phy related APIs in seperate patch.
>
> +
> 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_:
> @@ -2576,7 +2673,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> return -EIO;
> }
> phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> + phydev->interface = PHY_INTERFACE_MODE_INTERNAL;
> break;
>
> default:
> @@ -2591,12 +2688,7 @@ 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);
> @@ -2609,18 +2701,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> return -EIO;
> }
>
> - /* MAC doesn't support 1000T Half */
> - phy_remove_link_mode(phydev,
> ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
Since MAC doesn't support 1000Base-T half-duplex does phylink handle
its removal from the advertised link modes. Previously, it was done
using phy_remove_link_mode. If not, ensure that the phy doesn't end
up advertising an unsupported mode.
> -
> - /* 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_suspend(phydev);
>
> phy_support_eee(phydev);
>
> @@ -2646,10 +2727,6 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> }
> }
>
> - genphy_config_aneg(phydev);
> -
> - dev->fc_autoneg = phydev->autoneg;
> -
> return 0;
> }
>
> @@ -2989,7 +3066,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)
> @@ -3146,22 +3222,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;
> @@ -3211,9 +3277,11 @@ static int lan78xx_open(struct net_device
> *net)
>
> mutex_lock(&dev->dev_mutex);
>
> - phy_start(net->phydev);
> + lan78xx_init_stats(dev);
>
> - netif_dbg(dev, ifup, dev->net, "phy initialised
> successfully");
> + napi_enable(&dev->napi);
> +
> + set_bit(EVENT_DEV_OPEN, &dev->flags);
>
> /* for Link Check */
> if (dev->urb_intr) {
> @@ -3225,31 +3293,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);
>
> @@ -3317,12 +3363,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);
>
> @@ -3332,7 +3373,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);
> @@ -4256,13 +4297,13 @@ 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);
> + if (lan78xx_phy_int_ack(dev) < 0) {
> + netdev_info(dev->net, "PHY INT ack failed
> (%pe)\n",
> + ERR_PTR(ret));
> }
> }
>
> @@ -4344,26 +4385,29 @@ static void lan78xx_disconnect(struct
> usb_interface *intf)
> 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);
>
> lan78xx_unbind(dev, intf);
> @@ -4446,7 +4490,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);
> @@ -4558,14 +4601,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);
> @@ -4580,8 +4627,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:
> @@ -5143,7 +5192,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
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-17 10:50 ` Rengarajan.S
@ 2025-01-22 8:16 ` Oleksij Rempel
0 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-22 8:16 UTC (permalink / raw)
To: Rengarajan.S
Cc: andrew+netdev, rmk+kernel, davem, Woojung.Huh, pabeni, edumazet,
kuba, phil, kernel, linux-kernel, netdev, UNGLinuxDriver
Hi Rengarajan,
On Fri, Jan 17, 2025 at 10:50:48AM +0000, Rengarajan.S@microchip.com wrote:
> Hi Oleksji,
>
> On Wed, 2025-01-08 at 13:13 +0100, Oleksij Rempel wrote:
> > +static int lan78xx_configure_usb(struct lan78xx_net *dev, int speed)
> > +{
> > + int ret;
> > +
> > + if (dev->udev->speed != USB_SPEED_SUPER)
> > + return 0;
> > +
> > + if (speed != SPEED_1000)
> > + return lan78xx_update_reg(dev, USB_CFG1,
> > + USB_CFG1_DEV_U2_INIT_EN_ |
> > + USB_CFG1_DEV_U1_INIT_EN_,
> > + USB_CFG1_DEV_U2_INIT_EN_ |
> > + USB_CFG1_DEV_U1_INIT_EN_);
> > +
> > + /* disable U2 */
> > + ret = lan78xx_update_reg(dev, USB_CFG1,
> > + USB_CFG1_DEV_U2_INIT_EN_, 0);
> > + if (ret < 0)
> > + return ret;
> > + /* enable U1 */
> > + return lan78xx_update_reg(dev, USB_CFG1,
> > + USB_CFG1_DEV_U1_INIT_EN_,
> > + USB_CFG1_DEV_U1_INIT_EN_);
>
> Since, in the all the above cases we update the USB_CFG1 based on the
> mask and value depending on various speeds, have a variable for mask
> and value and use them to represent enabled flags instead of passing
> the flags directly for better readability.
>
> For Eg: have mask = USB_CFG1_DEV_U2_INIT_EN_ and val = 0 and pass them
> as arguments.
ok.
> +
> > + 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);
>
> The auto-speed and duplex are disabled in lan78xx_mac_config is it
> necessary to disable this again here. You can disable it once to
> avoid redundancy.
ok.
> > +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,
> > +};
>
> If possible, have MAC and phy related APIs in seperate patch.
I'll think about it, but I ques it will be not so good.
> > @@ -2609,18 +2701,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> > *dev)
> > return -EIO;
> > }
> >
> > - /* MAC doesn't support 1000T Half */
> > - phy_remove_link_mode(phydev,
> > ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
>
> Since MAC doesn't support 1000Base-T half-duplex does phylink handle
> its removal from the advertised link modes. Previously, it was done
> using phy_remove_link_mode. If not, ensure that the phy doesn't end
> up advertising an unsupported mode.
Yes, it is done by following line:
dev->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
MAC_10 | MAC_100 | MAC_1000FD;
Thank you,
Best regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-08 12:13 ` [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management Oleksij Rempel
2025-01-08 12:43 ` Russell King (Oracle)
2025-01-17 10:50 ` Rengarajan.S
@ 2025-01-22 4:02 ` Thangaraj.S
2025-01-22 6:06 ` Oleksij Rempel
2 siblings, 1 reply; 33+ messages in thread
From: Thangaraj.S @ 2025-01-22 4:02 UTC (permalink / raw)
To: andrew+netdev, rmk+kernel, davem, Woojung.Huh, pabeni, o.rempel,
edumazet, kuba
Cc: phil, kernel, linux-kernel, netdev, UNGLinuxDriver
Hi Oleksji,
On Wed, 2025-01-08 at 13:13 +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`.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> drivers/net/usb/lan78xx.c | 525 +++++++++++++++++++++---------------
> --
> 1 file changed, 287 insertions(+), 238 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index a91bf9c7e31d..6dfd9301279f 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_link_reset(struct lan78xx_net *dev)
> +static int lan78xx_phy_int_ack(struct lan78xx_net *dev)
> {
Is there any specific reason why this complete logic on phy interrupt
handling is removed?
> - 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);
> @@ -2356,26 +2237,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,6 +2369,207 @@ 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 rgmii_id = 0;
This variable is not modified anywhere in this function. Remove this
variable if not needed.
> + 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_ID:
> + break;
> + default:
> + netdev_warn(net, "Unsupported interface mode: %d\n",
> + state->interface);
> + return;
> + }
> +
> + /* by the way, make sure auto speed and duplex are disabled
> */
> + ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_AUTO_DUPLEX_ |
> + MAC_CR_AUTO_SPEED_ |
> MAC_CR_GMII_EN_, mac_cr);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + ret = lan78xx_write_reg(dev, MAC_RGMII_ID, rgmii_id);
> + if (ret < 0)
> + goto mac_config_fail;
> +
> + return;
> +
> +mac_config_fail:
> + netdev_err(net, "Failed to config MAC with error %pe\n",
> ERR_PTR(ret));
> +}
> +
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management
2025-01-22 4:02 ` Thangaraj.S
@ 2025-01-22 6:06 ` Oleksij Rempel
0 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-22 6:06 UTC (permalink / raw)
To: Thangaraj.S
Cc: andrew+netdev, rmk+kernel, davem, Woojung.Huh, pabeni, edumazet,
kuba, phil, kernel, linux-kernel, netdev, UNGLinuxDriver
Hi Thangaraj,
On Wed, Jan 22, 2025 at 04:02:42AM +0000, Thangaraj.S@microchip.com wrote:
> Hi Oleksji,
>
> On Wed, 2025-01-08 at 13:13 +0100, Oleksij Rempel wrote:
> > /* 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_link_reset(struct lan78xx_net *dev)
> > +static int lan78xx_phy_int_ack(struct lan78xx_net *dev)
> > {
>
> Is there any specific reason why this complete logic on phy interrupt
> handling is removed?
### Before: Old PHY Interrupt Handling
In the old implementation, the driver processed PHY interrupts as follows:
1. When a status URB was received, the driver checked for the `INT_ENP_PHY_INT`
flag to detect a PHY-related interrupt:
- Upon detecting the interrupt, it triggered a deferred kevent
(`lan78xx_defer_kevent`) with the `EVENT_LINK_RESET` event.
- It also called `generic_handle_irq_safe` to notify the PHY subsystem of
the interrupt, allowing it to update the PHY state.
2. The deferred kevent was handled by `lan78xx_link_reset`, which:
- It invoked `phy_read_status` to retrieve the PHY state.
- Based on the PHY state, it adjusted MAC settings (e.g., flow control,
USB state) and restarted the RX/TX paths if needed.
- Enabled/disrupted timers and submitted RX URBs when link changes occurred.
This design required the driver to manually handle both PHY and MAC state
management, leading to complex and redundant logic.
### Now: PHYlink Integration
In the updated code, the handling process is simplified:
1. When a status URB detects a PHY interrupt (`INT_ENP_PHY_INT`), the driver
still calls `lan78xx_defer_kevent(dev, EVENT_PHY_INT_ACK)`. However:
- The deferred event now serves only to acknowledge the interrupt by calling
`lan78xx_phy_int_ack`.
- This separation is necessary because the URB completion context cannot
directly perform register writes.
2. The interrupt is forwarded to the PHY subsystem, which updates the PHY state
as before. No additional logic is performed in this step.
3. Once the PHY state is updated, PHYlink invokes appropriate callbacks to
handle MAC reconfiguration:
- `mac_config` for initial setup.
- `mac_link_up` and `mac_link_down` to manage link transitions, flow
control, and USB state.
### Why the Old Logic is No Longer Needed
The MAC is now reconfigured only through PHYlink callbacks, eliminating the
need for deferred events to handle complex link reset logic.
With PHYlink coordinating PHY and MAC interactions, the driver no longer needs
custom logic to manage state transitions.
> > +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 rgmii_id = 0;
>
> This variable is not modified anywhere in this function. Remove this
> variable if not needed.
Thank you. I'll update it.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH net-next v1 2/7] net: usb: lan78xx: Move fixed PHY cleanup to lan78xx_unbind()
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 3/7] net: usb: lan78xx: Improve error handling for PHY init path Oleksij Rempel
` (4 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
Move the cleanup of the fixed PHY to the lan78xx_unbind() function,
which is invoked during both the probe and disconnect paths. This
change eliminates duplicate cleanup code in the disconnect routine and
ensures that the fixed PHY is properly freed during other probe steps.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/usb/lan78xx.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 6dfd9301279f..3d0097d07bcd 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -474,6 +474,8 @@ struct lan78xx_net {
struct phylink *phylink;
struct phylink_config phylink_config;
+
+ struct phy_device *fixed_phy;
};
/* use ethtool to change the level for any given device */
@@ -2589,6 +2591,8 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
netdev_err(dev->net, "No PHY/fixed_PHY found\n");
return NULL;
}
+
+ dev->fixed_phy = phydev;
netdev_dbg(dev->net, "Registered FIXED PHY\n");
phydev->interface = PHY_INTERFACE_MODE_RGMII;
ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
@@ -2690,14 +2694,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
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);
- }
- }
+ netdev_err(dev->net, "can't attach PHY to %s, error %pe\n",
+ dev->mdiobus->id, ERR_PTR(ret));
return -EIO;
}
@@ -3652,6 +3650,12 @@ static void lan78xx_unbind(struct lan78xx_net *dev, struct usb_interface *intf)
{
struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
+ if (dev->fixed_phy) {
+ fixed_phy_unregister(dev->fixed_phy);
+ phy_device_free(dev->fixed_phy);
+ dev->fixed_phy = NULL;
+ }
+
lan78xx_remove_irq_domain(dev);
lan78xx_remove_mdio(dev);
@@ -4378,7 +4382,6 @@ 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);
@@ -4401,11 +4404,6 @@ static void lan78xx_disconnect(struct usb_interface *intf)
set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
cancel_delayed_work_sync(&dev->wq);
- 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);
--
2.39.5
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH net-next v1 3/7] net: usb: lan78xx: Improve error handling for PHY init path
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 2/7] net: usb: lan78xx: Move fixed PHY cleanup to lan78xx_unbind() Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:52 ` Russell King (Oracle)
2025-01-08 12:13 ` [PATCH net-next v1 4/7] net: usb: lan78xx: Use ethtool_op_get_link to reflect current link status Oleksij Rempel
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
Make sure existing return values are actually used.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/usb/lan78xx.c | 58 +++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 18 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 3d0097d07bcd..dde127d18a07 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2574,22 +2574,24 @@ static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
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) {
netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
- if (IS_ERR(phydev)) {
+ if (PTR_ERR_OR_ZERO(phydev)) {
netdev_err(dev->net, "No PHY/fixed_PHY found\n");
- return NULL;
+ if (IS_ERR(phydev))
+ return phydev;
+ else
+ return ERR_PTR(-ENODEV);
}
dev->fixed_phy = phydev;
@@ -2597,24 +2599,34 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
phydev->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(-ENODEV);
}
phydev->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;
}
@@ -2663,9 +2675,9 @@ 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) {
+ if (IS_ERR(phydev)) {
netdev_err(dev->net, "lan7801: PHY Init Failed");
- return -EIO;
+ return PTR_ERR(phydev);
}
break;
@@ -2674,7 +2686,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;
phydev->interface = PHY_INTERFACE_MODE_INTERNAL;
@@ -2682,7 +2694,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 */
@@ -2696,10 +2708,15 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
if (ret) {
netdev_err(dev->net, "can't attach PHY to %s, error %pe\n",
dev->mdiobus->id, ERR_PTR(ret));
- return -EIO;
+ return ret;
}
- phy_suspend(phydev);
+ ret = phy_suspend(phydev);
+ if (ret) {
+ netdev_err(dev->net, "can't suspend PHY, error %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
phy_support_eee(phydev);
@@ -2712,7 +2729,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, ®);
+ ret = lan78xx_read_reg(dev, HW_CFG, ®);
+ if (ret < 0)
+ return ret;
+
reg &= ~(HW_CFG_LED0_EN_ |
HW_CFG_LED1_EN_ |
HW_CFG_LED2_EN_ |
@@ -2721,7 +2741,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 3/7] net: usb: lan78xx: Improve error handling for PHY init path
2025-01-08 12:13 ` [PATCH net-next v1 3/7] net: usb: lan78xx: Improve error handling for PHY init path Oleksij Rempel
@ 2025-01-08 12:52 ` Russell King (Oracle)
0 siblings, 0 replies; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-08 12:52 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 01:13:37PM +0100, Oleksij Rempel wrote:
> 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)) {
> + if (PTR_ERR_OR_ZERO(phydev)) {
Even though I've said to use phylink's fixed-phy support, I'm wondering
about this entire hunk.
> netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> - return NULL;
> + if (IS_ERR(phydev))
> + return phydev;
> + else
> + return ERR_PTR(-ENODEV);
When does fixed_phy_register() return NULL?
If there's no fixed-phy support enabled, then you get an ENODEV error
pointer. If support is enabled, then you may get an error pointer
or a valid phy_device structure. I can't see any case where it returns
NULL. So, I think this hunk is redundant.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH net-next v1 4/7] net: usb: lan78xx: Use ethtool_op_get_link to reflect current link status
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
` (2 preceding siblings ...)
2025-01-08 12:13 ` [PATCH net-next v1 3/7] net: usb: lan78xx: Improve error handling for PHY init path Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 5/7] net: usb: lan78xx: port link settings to phylink API Oleksij Rempel
` (2 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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>
---
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 dde127d18a07..8c3b199abd31 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1837,18 +1837,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)
{
@@ -2015,7 +2003,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH net-next v1 5/7] net: usb: lan78xx: port link settings to phylink API
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
` (3 preceding siblings ...)
2025-01-08 12:13 ` [PATCH net-next v1 4/7] net: usb: lan78xx: Use ethtool_op_get_link to reflect current link status Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 6/7] net: usb: lan78xx: Transition get/set_pause to phylink Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration Oleksij Rempel
6 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
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 8c3b199abd31..0c2e9976daa2 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1864,46 +1864,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH net-next v1 6/7] net: usb: lan78xx: Transition get/set_pause to phylink
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
` (4 preceding siblings ...)
2025-01-08 12:13 ` [PATCH net-next v1 5/7] net: usb: lan78xx: port link settings to phylink API Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:13 ` [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration Oleksij Rempel
6 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
Replace lan78xx_get_pause and lan78xx_set_pause implementations with
phylink-based functions. This transition aligns pause parameter handling
with the phylink API, simplifying the code and improving
maintainability.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/usb/lan78xx.c | 51 ++-------------------------------------
1 file changed, 2 insertions(+), 49 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 0c2e9976daa2..55488ddba269 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1880,63 +1880,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)
--
2.39.5
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-08 12:13 [PATCH net-next v1 0/7] Convert LAN78xx to PHYLINK Oleksij Rempel
` (5 preceding siblings ...)
2025-01-08 12:13 ` [PATCH net-next v1 6/7] net: usb: lan78xx: Transition get/set_pause to phylink Oleksij Rempel
@ 2025-01-08 12:13 ` Oleksij Rempel
2025-01-08 12:47 ` Russell King (Oracle)
6 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 12:13 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, Russell King
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
to integrate with phylink. This includes the following changes:
- Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
EEE settings, aligning with the phylink API.
- Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
request delay. Default it to 50 microseconds based on LAN7800 documentation
recommendations.
- Update EEE configuration during link_up and ensure proper disabling
during `link_down` to allow reconfiguration.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/usb/lan78xx.c | 81 ++++++++++++++++++++++-----------------
1 file changed, 46 insertions(+), 35 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 55488ddba269..b9958a0533de 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -476,6 +476,8 @@ struct lan78xx_net {
struct phylink_config phylink_config;
struct phy_device *fixed_phy;
+
+ u32 tx_lpi_timer;
};
/* use ethtool to change the level for any given device */
@@ -1787,54 +1789,24 @@ 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);
+ ret = phylink_ethtool_get_eee(dev->phylink, edata);
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);
+ edata->tx_lpi_timer = dev->tx_lpi_timer;
- return ret;
+ return 0;
}
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);
+ dev->tx_lpi_timer = edata->tx_lpi_timer;
- return ret;
+ return phylink_ethtool_set_eee(dev->phylink, edata);
}
static void lan78xx_get_drvinfo(struct net_device *net,
@@ -2345,6 +2317,13 @@ static void lan78xx_mac_link_down(struct phylink_config *config,
if (ret < 0)
goto link_down_fail;
+ /* at least MAC_CR_EEE_EN_ should be disable for proper configuration
+ * on link_up
+ */
+ ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_EEE_EN_, 0);
+ 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.
@@ -2418,6 +2397,7 @@ static void lan78xx_mac_link_up(struct phylink_config *config,
{
struct net_device *net = to_net_dev(config->dev);
struct lan78xx_net *dev = netdev_priv(net);
+ struct phy_device *phydev = net->phydev;
u32 mac_cr = 0;
int ret;
@@ -2439,6 +2419,18 @@ static void lan78xx_mac_link_up(struct phylink_config *config,
if (duplex == DUPLEX_FULL)
mac_cr |= MAC_CR_FULL_DUPLEX_;
+ if (phydev->enable_tx_lpi) {
+ /* EEE_TX_LPI_REQ_DLY should be written before MAC_CR_EEE_EN_
+ * is set
+ */
+ ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY,
+ dev->tx_lpi_timer);
+ if (ret < 0)
+ goto link_up_fail;
+
+ mac_cr |= MAC_CR_EEE_EN_;
+ }
+
/* 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);
@@ -4430,6 +4422,25 @@ static int lan78xx_probe(struct usb_interface *intf,
dev->msg_enable = netif_msg_init(msg_level, NETIF_MSG_DRV
| NETIF_MSG_PROBE | NETIF_MSG_LINK);
+ /*
+ * 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.
+ */
+ dev->tx_lpi_timer = 50;
+
skb_queue_head_init(&dev->rxq);
skb_queue_head_init(&dev->txq);
skb_queue_head_init(&dev->rxq_done);
--
2.39.5
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-08 12:13 ` [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration Oleksij Rempel
@ 2025-01-08 12:47 ` Russell King (Oracle)
2025-01-08 14:23 ` Oleksij Rempel
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-08 12:47 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote:
> Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
> to integrate with phylink. This includes the following changes:
>
> - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
> EEE settings, aligning with the phylink API.
> - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
> request delay. Default it to 50 microseconds based on LAN7800 documentation
> recommendations.
phylib maintains tx_lpi_timer for you. Please use that instead.
In any case, I've been submitting phylink EEE support which will help
driver authors get this correct, but I think it needs more feedback.
Please can you look at my patch set previously posted which is now
a bit out of date, review, and think about how this driver can make
use of it.
In particular, I'd like ideas on what phylink should be doing with
tx_lpi_timer values that are out of range of the hardware. Should it
limit the value itself? Should set_eee() error out? I asked these
questions in the cover message but I don't think *anyone* reads
cover messages anymore - as evidenced by my recent patch series that
made reference to "make it sew" and Singer sewing machines. No one
noticed. So I think patch series cover messages are now useless on
netdev.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-08 12:47 ` Russell King (Oracle)
@ 2025-01-08 14:23 ` Oleksij Rempel
2025-01-08 15:15 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-08 14:23 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote:
> > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
> > to integrate with phylink. This includes the following changes:
> >
> > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
> > EEE settings, aligning with the phylink API.
> > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
> > request delay. Default it to 50 microseconds based on LAN7800 documentation
> > recommendations.
>
> phylib maintains tx_lpi_timer for you. Please use that instead.
Just using this variable directly phydev->eee_cfg.tx_lpi_timer ?
> In any case, I've been submitting phylink EEE support which will help
> driver authors get this correct, but I think it needs more feedback.
> Please can you look at my patch set previously posted which is now
> a bit out of date, review, and think about how this driver can make
> use of it.
Ack, will do. It looks like your port of lan743x to the new API
looks exactly like what I need for this driver too.
> In particular, I'd like ideas on what phylink should be doing with
> tx_lpi_timer values that are out of range of the hardware. Should it
> limit the value itself?
Yes, otherwise every MAC driver will need to do it in the
ethtool_set_eee() function.
The other question is, should we allow absolute maximum values, or sane
maximum? At some point will come the question, why the EEE is even
enabled?
The same is about minimal value, too low value will cause strong speed
degradation. Should we allow set insane minimum, but use sane default
value?
> Should set_eee() error out?
Yes, please.
> I asked these questions in the cover message but I don't think *anyone*
> reads cover messages anymore - as evidenced by my recent patch series that
> made reference to "make it sew" and Singer sewing machines. No one
> noticed. So I think patch series cover messages are now useless on
> netdev.
lol :)
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-08 14:23 ` Oleksij Rempel
@ 2025-01-08 15:15 ` Russell King (Oracle)
2025-01-09 17:13 ` Oleksij Rempel
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-08 15:15 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote:
> > On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote:
> > > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
> > > to integrate with phylink. This includes the following changes:
> > >
> > > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
> > > EEE settings, aligning with the phylink API.
> > > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
> > > request delay. Default it to 50 microseconds based on LAN7800 documentation
> > > recommendations.
> >
> > phylib maintains tx_lpi_timer for you. Please use that instead.
>
> Just using this variable directly phydev->eee_cfg.tx_lpi_timer ?
Yes. We're already accessing phydev->enable_tx_lpi directly, and we
have no helpers for this. Maybe it's more a question for Andrew,
whether he wishes to see phylib helpers for this state rather than
directly dereferencing phydev.
> > In any case, I've been submitting phylink EEE support which will help
> > driver authors get this correct, but I think it needs more feedback.
> > Please can you look at my patch set previously posted which is now
> > a bit out of date, review, and think about how this driver can make
> > use of it.
>
> Ack, will do. It looks like your port of lan743x to the new API
> looks exactly like what I need for this driver too.
>
> > In particular, I'd like ideas on what phylink should be doing with
> > tx_lpi_timer values that are out of range of the hardware. Should it
> > limit the value itself?
>
> Yes, otherwise every MAC driver will need to do it in the
> ethtool_set_eee() function.
I've had several solutions, and my latest patch set actually has a
mixture of them in there (which is why I'm eager to try and find a way
forward on this, so I can fix the patch set):
1. the original idea to address this in Marvell platforms was to limit
the LPI timer to the maximum representable value in the hardware,
which would be 255us. This ignores that the hardware uses a 1us
tick rate for the timer at 1G speeds, and 10us for 100M speeds.
(So it limits it to 260us, even though the hardware can do 2550us
at 100M speed). This limit was applied by clamping the value passed
in from userspace without erroring out.
2. another solution was added the mac_validate_tx_lpi() method, and
implementations added _in addition_ to the above, with the idea
of erroring out for values > 255us on Marvell hardware.
3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
possible to allow e.g. falling back to a software timer (see stmmac
comments below.) Another reason for erroring out applies to Marvell
hardware, where PP2 hardware supports LPI on the GMAC but not the
XGMAC - so it only works at speeds at or below 2.5G. However, that
can be handled via the lpi_capabilities, so I don't think needs to
be a concern.
> The other question is, should we allow absolute maximum values, or sane
> maximum? At some point will come the question, why the EEE is even
> enabled?
As referenced above, stmmac uses the hardware timer for LPI timeouts up
to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
normal kernel timer which is:
- disabled (and EEE mode reset) when we have a packet to transmit, or
EEE is disabled
- is re-armed when cleaning up from packet transmission (although
it looks like we attempt to immediately enter LPI mode, and would
only wait for the timer if there are more packets to queue... maybe
this is a bug in stmmac's implementation?) or when EEE mode is first
enabled with a LPI timer longer than the above value.
So, should phylink have the capability to switch to a software LPI timer
implementation when the LPI timeout value exceeds what the hardware
supports? To put it another way, should the stmmac solution to this be
made generic?
Note that stmmac has this software timer implementation because not
only for the reason I've given above, but also because cores other than
GMAC4 that support LPI do not have support for the hardware timer.
> The same is about minimal value, too low value will cause strong speed
> degradation. Should we allow set insane minimum, but use sane default
> value?
We currently allow zero, and the behaviour of that depends on the
hardware. For example, in the last couple of days, it's been reported
that stmmac will never enter LPI with a value of zero.
Note that phylib defaults to zero, so imposing a minimum would cause
a read-modify-write of the EEE settings without setting the timer to
fail.
> > Should set_eee() error out?
>
> Yes, please.
If we are to convert stmmac, then we need to consider what it's doing
(as per the above) and whether that should be generic - and if it isn't
what we want in generic code, then how do we allow drivers to do this if
they wish.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-08 15:15 ` Russell King (Oracle)
@ 2025-01-09 17:13 ` Oleksij Rempel
2025-01-09 17:27 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-09 17:13 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > Yes, otherwise every MAC driver will need to do it in the
> > ethtool_set_eee() function.
>
> I've had several solutions, and my latest patch set actually has a
> mixture of them in there (which is why I'm eager to try and find a way
> forward on this, so I can fix the patch set):
>
> 1. the original idea to address this in Marvell platforms was to limit
> the LPI timer to the maximum representable value in the hardware,
> which would be 255us. This ignores that the hardware uses a 1us
> tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> (So it limits it to 260us, even though the hardware can do 2550us
> at 100M speed). This limit was applied by clamping the value passed
> in from userspace without erroring out.
>
> 2. another solution was added the mac_validate_tx_lpi() method, and
> implementations added _in addition_ to the above, with the idea
> of erroring out for values > 255us on Marvell hardware.
>
> 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> possible to allow e.g. falling back to a software timer (see stmmac
> comments below.) Another reason for erroring out applies to Marvell
> hardware, where PP2 hardware supports LPI on the GMAC but not the
> XGMAC - so it only works at speeds at or below 2.5G. However, that
> can be handled via the lpi_capabilities, so I don't think needs to
> be a concern.
>
> > The other question is, should we allow absolute maximum values, or sane
> > maximum? At some point will come the question, why the EEE is even
> > enabled?
>
> As referenced above, stmmac uses the hardware timer for LPI timeouts up
> to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> normal kernel timer which is:
>
> - disabled (and EEE mode reset) when we have a packet to transmit, or
> EEE is disabled
> - is re-armed when cleaning up from packet transmission (although
> it looks like we attempt to immediately enter LPI mode, and would
> only wait for the timer if there are more packets to queue... maybe
> this is a bug in stmmac's implementation?) or when EEE mode is first
> enabled with a LPI timer longer than the above value.
>
> So, should phylink have the capability to switch to a software LPI timer
> implementation when the LPI timeout value exceeds what the hardware
> supports?
No, i'll list my arguments later down.
> To put it another way, should the stmmac solution to this be
> made generic?
May be partially?
> Note that stmmac has this software timer implementation because not
> only for the reason I've given above, but also because cores other than
> GMAC4 that support LPI do not have support for the hardware timer.
There seems to be a samsung ethernet driver which implements software
based timer too.
> > The same is about minimal value, too low value will cause strong speed
> > degradation. Should we allow set insane minimum, but use sane default
> > value?
>
> We currently allow zero, and the behaviour of that depends on the
> hardware. For example, in the last couple of days, it's been reported
> that stmmac will never enter LPI with a value of zero.
>
> Note that phylib defaults to zero, so imposing a minimum would cause
> a read-modify-write of the EEE settings without setting the timer to
> fail.
>
> > > Should set_eee() error out?
> >
> > Yes, please.
>
> If we are to convert stmmac, then we need to consider what it's doing
> (as per the above) and whether that should be generic - and if it isn't
> what we want in generic code, then how do we allow drivers to do this if
> they wish.
I'll try to approach this from a user perspective. Currently, we have a single
`lpi_timer` interface for all link modes. If I start using it, I'm trying to
address a specific issue, but in most cases, I have no idea what link mode will
be active at any given time. To my knowledge, there are no user space tools
that allow users to configure different timer values for different link speeds.
So, what problems am I really trying to solve by adjusting this timer? I can
imagine the following:
1. Noticeable Speed Degradation:
This happens when the timer is configured to a value smaller than the time
needed to put the hardware to sleep and wake it up again. For interfaces
supporting multiple link speeds with EEE, the most plausible configuration to
avoid degradation would be to set the timer to the maximum sleep-wake time
across all supported EEE link speeds.
2. Other Use Cases:
Most other scenarios involve trying to work around specific constraints or
optimizing for particular use cases:
- Maximizing Power Savings: Setting the timer to the smallest possible
value to achieve aggressive power-saving. Why would a user do this? It seems
niche but might apply in low-traffic environments.
- Reducing Latency for Periodic Traffic: For example, in audio
streaming, frames might be sent every X milliseconds. In this case, the timer
could be set slightly higher than X to allow the interface to enter LPI mode
between frames. As soon as the audio stops and no other traffic is present, the
interface transitions to LPI mode entirely. If the hardware supports timers
with values ≥ X, no additional complexity is needed. However, if the hardware
timer is not supported or the supported range is lower than X, a
software-assisted timer would be required. This might introduce additional
latency, and users should be made aware of this potential impact.
In my expectation HW timer based latency can be different to software based
timer.
From my current user perspective, I would expect the following behavior from
the existing `lpi_timer` interface:
1. Subtle Disabling of LPI Should Be Prevented:
If setting the `lpi_timer` to 0 effectively disables LPI, then this value
should not be allowed. The interface should ensure that LPI remains functional
unless explicitly turned off by the user.
2. Maximum Timer Value Should Align with Timer Implementation:
The maximum value of the `lpi_timer` should correspond to the timer
implementation in use:
- No software and hardware timer should be mixed, otherwise it would
affect latency behavior depending on the timer value.
- If a hardware timer is supported but has a lower maximum range than
required, the interface should support either:
- Only the hardware timer within its valid range.
- A fallback to only a software timer (if feasible for the system).
However, for hardware like switches, software-based LPI implementations
are not feasible.
3. Sensible Maximum Timer Values:
Setting the timer to excessively high values (e.g., one or two seconds or
more) makes the behavior unpredictable. Such configurations seem more like a
"time bomb" or a workaround for another issue that should be addressed
differently. For example:
- If the use case requires such long timer values, it may make more sense to
disable `tx_lpi` entirely and manage power savings differently.
4. Errors for Unsupported Configurations:
If a configuration variation is not supported - whether due to hardware
constraints, a mismatch with the current link mode, or a similar limitation - the
user should receive a clear error message. EEE is already challenging to debug,
and silent failures or corner-case issues (e.g., a speed downshift from 1000
Mbps to 100 Mbps causing configuration to fail) would significantly degrade the
user experience.
Some HW or drivers (for example dsa/microchip/ driver) do no support
LPI timer configuration. In this case the error should be returned too.
5. Separate Handling of LPI Activation:
Some MACs support both automatic LPI activation (based on egress queue and
EEE/LPI activation bits) and forced activation for testing or software based
timers. Some PHYs, such as the Atheros AR8035, appear sensitive to premature
LPI activation, particularly during the transition from autonegotiation to an
active link. To address this:
- The MAC driver should expose controls for managing automatic versus forced
LPI activation where applicable. This will be needed for common software
based timer implementation.
- The PHYLINK API should provide separate control mechanisms for LPI
activation and link state transitions. (done)
6. Consideration for Link-Independent Modes:
Certain EEE-related configurations can be applied without a PHY, while
others are entirely dependent on the PHY being present. The system should
differentiate between these cases and handle them as follows:
- EEE On/Off:
Enabling or disabling EEE at the MAC level should be allowed without a
PHY. This can be treated as a user preference - "I prefer EEE to be on if
supported." If a PHY becomes available later and supports EEE, this preference
can then take effect.
- LPI On/Off:
Similar to EEE on/off, enabling or disabling Low Power Idle (LPI) can be
managed at the MAC level independently of the PHY. This setting reflects the
MAC's ability to enter LPI mode. In SmartEEE or similar modes, this could
potentially involve PHY-specific behavior, but the basic LPI on/off setting
remains primarily MAC-specific.
- LPI Timer:
The LPI timer is implementation-specific to the MAC driver and does not
inherently depend on the PHY. Yes, it depends at least on the link speed,
but this can't be addresses with existing interface.
- EEE Advertisement:
Advertising EEE capabilities is entirely dependent on the PHY. Without a
PHY, these settings cannot be determined or validated, as the PHY defines the
supported capabilities. Any attempt to configure EEE advertisement without an
attached PHY should fail immediately with an appropriate error, such as: "EEE
advertisement configuration not applied: no PHY available to validate
capabilities."
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 17:13 ` Oleksij Rempel
@ 2025-01-09 17:27 ` Russell King (Oracle)
2025-01-09 17:39 ` Oleksij Rempel
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-09 17:27 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Thu, Jan 09, 2025 at 06:13:10PM +0100, Oleksij Rempel wrote:
> On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> > On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > > Yes, otherwise every MAC driver will need to do it in the
> > > ethtool_set_eee() function.
> >
> > I've had several solutions, and my latest patch set actually has a
> > mixture of them in there (which is why I'm eager to try and find a way
> > forward on this, so I can fix the patch set):
> >
> > 1. the original idea to address this in Marvell platforms was to limit
> > the LPI timer to the maximum representable value in the hardware,
> > which would be 255us. This ignores that the hardware uses a 1us
> > tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> > (So it limits it to 260us, even though the hardware can do 2550us
> > at 100M speed). This limit was applied by clamping the value passed
> > in from userspace without erroring out.
> >
> > 2. another solution was added the mac_validate_tx_lpi() method, and
> > implementations added _in addition_ to the above, with the idea
> > of erroring out for values > 255us on Marvell hardware.
> >
> > 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> > possible to allow e.g. falling back to a software timer (see stmmac
> > comments below.) Another reason for erroring out applies to Marvell
> > hardware, where PP2 hardware supports LPI on the GMAC but not the
> > XGMAC - so it only works at speeds at or below 2.5G. However, that
> > can be handled via the lpi_capabilities, so I don't think needs to
> > be a concern.
> >
> > > The other question is, should we allow absolute maximum values, or sane
> > > maximum? At some point will come the question, why the EEE is even
> > > enabled?
> >
> > As referenced above, stmmac uses the hardware timer for LPI timeouts up
> > to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> > normal kernel timer which is:
> >
> > - disabled (and EEE mode reset) when we have a packet to transmit, or
> > EEE is disabled
> > - is re-armed when cleaning up from packet transmission (although
> > it looks like we attempt to immediately enter LPI mode, and would
> > only wait for the timer if there are more packets to queue... maybe
> > this is a bug in stmmac's implementation?) or when EEE mode is first
> > enabled with a LPI timer longer than the above value.
> >
> > So, should phylink have the capability to switch to a software LPI timer
> > implementation when the LPI timeout value exceeds what the hardware
> > supports?
>
> No, i'll list my arguments later down.
>
> > To put it another way, should the stmmac solution to this be
> > made generic?
>
> May be partially?
>
> > Note that stmmac has this software timer implementation because not
> > only for the reason I've given above, but also because cores other than
> > GMAC4 that support LPI do not have support for the hardware timer.
>
> There seems to be a samsung ethernet driver which implements software
> based timer too.
>
> > > The same is about minimal value, too low value will cause strong speed
> > > degradation. Should we allow set insane minimum, but use sane default
> > > value?
> >
> > We currently allow zero, and the behaviour of that depends on the
> > hardware. For example, in the last couple of days, it's been reported
> > that stmmac will never enter LPI with a value of zero.
> >
> > Note that phylib defaults to zero, so imposing a minimum would cause
> > a read-modify-write of the EEE settings without setting the timer to
> > fail.
> >
> > > > Should set_eee() error out?
> > >
> > > Yes, please.
> >
> > If we are to convert stmmac, then we need to consider what it's doing
> > (as per the above) and whether that should be generic - and if it isn't
> > what we want in generic code, then how do we allow drivers to do this if
> > they wish.
>
> I'll try to approach this from a user perspective. Currently, we have a single
> `lpi_timer` interface for all link modes. If I start using it, I'm trying to
> address a specific issue, but in most cases, I have no idea what link mode will
> be active at any given time. To my knowledge, there are no user space tools
> that allow users to configure different timer values for different link speeds.
>
> So, what problems am I really trying to solve by adjusting this timer? I can
> imagine the following:
>
> 1. Noticeable Speed Degradation:
>
> This happens when the timer is configured to a value smaller than the time
> needed to put the hardware to sleep and wake it up again. For interfaces
> supporting multiple link speeds with EEE, the most plausible configuration to
> avoid degradation would be to set the timer to the maximum sleep-wake time
> across all supported EEE link speeds.
>
> 2. Other Use Cases:
>
> Most other scenarios involve trying to work around specific constraints or
> optimizing for particular use cases:
>
> - Maximizing Power Savings: Setting the timer to the smallest possible
> value to achieve aggressive power-saving. Why would a user do this? It seems
> niche but might apply in low-traffic environments.
>
> - Reducing Latency for Periodic Traffic: For example, in audio
> streaming, frames might be sent every X milliseconds. In this case, the timer
> could be set slightly higher than X to allow the interface to enter LPI mode
> between frames. As soon as the audio stops and no other traffic is present, the
> interface transitions to LPI mode entirely. If the hardware supports timers
> with values ≥ X, no additional complexity is needed. However, if the hardware
> timer is not supported or the supported range is lower than X, a
> software-assisted timer would be required. This might introduce additional
> latency, and users should be made aware of this potential impact.
> In my expectation HW timer based latency can be different to software based
> timer.
>
> From my current user perspective, I would expect the following behavior from
> the existing `lpi_timer` interface:
>
> 1. Subtle Disabling of LPI Should Be Prevented:
>
> If setting the `lpi_timer` to 0 effectively disables LPI, then this value
> should not be allowed. The interface should ensure that LPI remains functional
> unless explicitly turned off by the user.
>
> 2. Maximum Timer Value Should Align with Timer Implementation:
>
> The maximum value of the `lpi_timer` should correspond to the timer
> implementation in use:
>
> - No software and hardware timer should be mixed, otherwise it would
> affect latency behavior depending on the timer value.
>
> - If a hardware timer is supported but has a lower maximum range than
> required, the interface should support either:
>
> - Only the hardware timer within its valid range.
> - A fallback to only a software timer (if feasible for the system).
>
> However, for hardware like switches, software-based LPI implementations
> are not feasible.
>
> 3. Sensible Maximum Timer Values:
>
> Setting the timer to excessively high values (e.g., one or two seconds or
> more) makes the behavior unpredictable. Such configurations seem more like a
> "time bomb" or a workaround for another issue that should be addressed
> differently. For example:
>
> - If the use case requires such long timer values, it may make more sense to
> disable `tx_lpi` entirely and manage power savings differently.
>
> 4. Errors for Unsupported Configurations:
>
> If a configuration variation is not supported - whether due to hardware
> constraints, a mismatch with the current link mode, or a similar limitation - the
> user should receive a clear error message. EEE is already challenging to debug,
> and silent failures or corner-case issues (e.g., a speed downshift from 1000
> Mbps to 100 Mbps causing configuration to fail) would significantly degrade the
> user experience.
> Some HW or drivers (for example dsa/microchip/ driver) do no support
> LPI timer configuration. In this case the error should be returned too.
>
> 5. Separate Handling of LPI Activation:
>
> Some MACs support both automatic LPI activation (based on egress queue and
> EEE/LPI activation bits) and forced activation for testing or software based
> timers. Some PHYs, such as the Atheros AR8035, appear sensitive to premature
> LPI activation, particularly during the transition from autonegotiation to an
> active link. To address this:
>
> - The MAC driver should expose controls for managing automatic versus forced
> LPI activation where applicable. This will be needed for common software
> based timer implementation.
>
> - The PHYLINK API should provide separate control mechanisms for LPI
> activation and link state transitions. (done)
>
> 6. Consideration for Link-Independent Modes:
>
> Certain EEE-related configurations can be applied without a PHY, while
> others are entirely dependent on the PHY being present. The system should
> differentiate between these cases and handle them as follows:
>
> - EEE On/Off:
>
> Enabling or disabling EEE at the MAC level should be allowed without a
> PHY. This can be treated as a user preference - "I prefer EEE to be on if
> supported." If a PHY becomes available later and supports EEE, this preference
> can then take effect.
>
> - LPI On/Off:
>
> Similar to EEE on/off, enabling or disabling Low Power Idle (LPI) can be
> managed at the MAC level independently of the PHY. This setting reflects the
> MAC's ability to enter LPI mode. In SmartEEE or similar modes, this could
> potentially involve PHY-specific behavior, but the basic LPI on/off setting
> remains primarily MAC-specific.
>
> - LPI Timer:
>
> The LPI timer is implementation-specific to the MAC driver and does not
> inherently depend on the PHY. Yes, it depends at least on the link speed,
> but this can't be addresses with existing interface.
>
> - EEE Advertisement:
>
> Advertising EEE capabilities is entirely dependent on the PHY. Without a
> PHY, these settings cannot be determined or validated, as the PHY defines the
> supported capabilities. Any attempt to configure EEE advertisement without an
> attached PHY should fail immediately with an appropriate error, such as: "EEE
> advertisement configuration not applied: no PHY available to validate
> capabilities."
Sorry, at this point, I give up with phylink managed EEE. What you
detail above is way too much for me to get involved with, and goes
well beyond simply:
1) Fixing the cockup with the phylib-managed EEE that has caused *user*
*regressions* that we need to resolve.
2) Providing core functionality so that newer implementations can have
a consistency of behaviour.
I have *no* interest in doing a total rewrite of kernel EEE
functionality - that goes well beyond my aims here.
So I'm afraid that I really lost interest in reading your email, sorry.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 17:27 ` Russell King (Oracle)
@ 2025-01-09 17:39 ` Oleksij Rempel
2025-01-09 18:10 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-09 17:39 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Thu, Jan 09, 2025 at 05:27:11PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 09, 2025 at 06:13:10PM +0100, Oleksij Rempel wrote:
> > On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > > > Yes, otherwise every MAC driver will need to do it in the
> > > > ethtool_set_eee() function.
> > >
> > > I've had several solutions, and my latest patch set actually has a
> > > mixture of them in there (which is why I'm eager to try and find a way
> > > forward on this, so I can fix the patch set):
> > >
> > > 1. the original idea to address this in Marvell platforms was to limit
> > > the LPI timer to the maximum representable value in the hardware,
> > > which would be 255us. This ignores that the hardware uses a 1us
> > > tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> > > (So it limits it to 260us, even though the hardware can do 2550us
> > > at 100M speed). This limit was applied by clamping the value passed
> > > in from userspace without erroring out.
> > >
> > > 2. another solution was added the mac_validate_tx_lpi() method, and
> > > implementations added _in addition_ to the above, with the idea
> > > of erroring out for values > 255us on Marvell hardware.
> > >
> > > 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> > > possible to allow e.g. falling back to a software timer (see stmmac
> > > comments below.) Another reason for erroring out applies to Marvell
> > > hardware, where PP2 hardware supports LPI on the GMAC but not the
> > > XGMAC - so it only works at speeds at or below 2.5G. However, that
> > > can be handled via the lpi_capabilities, so I don't think needs to
> > > be a concern.
> > >
> > > > The other question is, should we allow absolute maximum values, or sane
> > > > maximum? At some point will come the question, why the EEE is even
> > > > enabled?
> > >
> > > As referenced above, stmmac uses the hardware timer for LPI timeouts up
> > > to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> > > normal kernel timer which is:
> > >
> > > - disabled (and EEE mode reset) when we have a packet to transmit, or
> > > EEE is disabled
> > > - is re-armed when cleaning up from packet transmission (although
> > > it looks like we attempt to immediately enter LPI mode, and would
> > > only wait for the timer if there are more packets to queue... maybe
> > > this is a bug in stmmac's implementation?) or when EEE mode is first
> > > enabled with a LPI timer longer than the above value.
> > >
> > > So, should phylink have the capability to switch to a software LPI timer
> > > implementation when the LPI timeout value exceeds what the hardware
> > > supports?
> >
> > No, i'll list my arguments later down.
> >
> > > To put it another way, should the stmmac solution to this be
> > > made generic?
> >
> > May be partially?
> >
> > > Note that stmmac has this software timer implementation because not
> > > only for the reason I've given above, but also because cores other than
> > > GMAC4 that support LPI do not have support for the hardware timer.
> >
> > There seems to be a samsung ethernet driver which implements software
> > based timer too.
> >
> > > > The same is about minimal value, too low value will cause strong speed
> > > > degradation. Should we allow set insane minimum, but use sane default
> > > > value?
> > >
> > > We currently allow zero, and the behaviour of that depends on the
> > > hardware. For example, in the last couple of days, it's been reported
> > > that stmmac will never enter LPI with a value of zero.
> > >
> > > Note that phylib defaults to zero, so imposing a minimum would cause
> > > a read-modify-write of the EEE settings without setting the timer to
> > > fail.
> > >
> > > > > Should set_eee() error out?
> > > >
> > > > Yes, please.
> > >
> > > If we are to convert stmmac, then we need to consider what it's doing
> > > (as per the above) and whether that should be generic - and if it isn't
> > > what we want in generic code, then how do we allow drivers to do this if
> > > they wish.
> >
> > - EEE Advertisement:
> >
> > Advertising EEE capabilities is entirely dependent on the PHY. Without a
> > PHY, these settings cannot be determined or validated, as the PHY defines the
> > supported capabilities. Any attempt to configure EEE advertisement without an
> > attached PHY should fail immediately with an appropriate error, such as: "EEE
> > advertisement configuration not applied: no PHY available to validate
> > capabilities."
>
> Sorry, at this point, I give up with phylink managed EEE. What you
> detail above is way too much for me to get involved with, and goes
> well beyond simply:
>
> 1) Fixing the cockup with the phylib-managed EEE that has caused *user*
> *regressions* that we need to resolve.
>
> 2) Providing core functionality so that newer implementations can have
> a consistency of behaviour.
>
> I have *no* interest in doing a total rewrite of kernel EEE
> functionality - that goes well beyond my aims here.
>
> So I'm afraid that I really lost interest in reading your email, sorry.
Sorry for killing your motivation. I can feel your pain...
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 17:39 ` Oleksij Rempel
@ 2025-01-09 18:10 ` Russell King (Oracle)
2025-01-09 19:33 ` Andrew Lunn
2025-01-09 19:36 ` Oleksij Rempel
0 siblings, 2 replies; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-09 18:10 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Thu, Jan 09, 2025 at 06:39:45PM +0100, Oleksij Rempel wrote:
> On Thu, Jan 09, 2025 at 05:27:11PM +0000, Russell King (Oracle) wrote:
> > On Thu, Jan 09, 2025 at 06:13:10PM +0100, Oleksij Rempel wrote:
> > > On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> > > > On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > > > > Yes, otherwise every MAC driver will need to do it in the
> > > > > ethtool_set_eee() function.
> > > >
> > > > I've had several solutions, and my latest patch set actually has a
> > > > mixture of them in there (which is why I'm eager to try and find a way
> > > > forward on this, so I can fix the patch set):
> > > >
> > > > 1. the original idea to address this in Marvell platforms was to limit
> > > > the LPI timer to the maximum representable value in the hardware,
> > > > which would be 255us. This ignores that the hardware uses a 1us
> > > > tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> > > > (So it limits it to 260us, even though the hardware can do 2550us
> > > > at 100M speed). This limit was applied by clamping the value passed
> > > > in from userspace without erroring out.
> > > >
> > > > 2. another solution was added the mac_validate_tx_lpi() method, and
> > > > implementations added _in addition_ to the above, with the idea
> > > > of erroring out for values > 255us on Marvell hardware.
> > > >
> > > > 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> > > > possible to allow e.g. falling back to a software timer (see stmmac
> > > > comments below.) Another reason for erroring out applies to Marvell
> > > > hardware, where PP2 hardware supports LPI on the GMAC but not the
> > > > XGMAC - so it only works at speeds at or below 2.5G. However, that
> > > > can be handled via the lpi_capabilities, so I don't think needs to
> > > > be a concern.
> > > >
> > > > > The other question is, should we allow absolute maximum values, or sane
> > > > > maximum? At some point will come the question, why the EEE is even
> > > > > enabled?
> > > >
> > > > As referenced above, stmmac uses the hardware timer for LPI timeouts up
> > > > to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> > > > normal kernel timer which is:
> > > >
> > > > - disabled (and EEE mode reset) when we have a packet to transmit, or
> > > > EEE is disabled
> > > > - is re-armed when cleaning up from packet transmission (although
> > > > it looks like we attempt to immediately enter LPI mode, and would
> > > > only wait for the timer if there are more packets to queue... maybe
> > > > this is a bug in stmmac's implementation?) or when EEE mode is first
> > > > enabled with a LPI timer longer than the above value.
> > > >
> > > > So, should phylink have the capability to switch to a software LPI timer
> > > > implementation when the LPI timeout value exceeds what the hardware
> > > > supports?
> > >
> > > No, i'll list my arguments later down.
> > >
> > > > To put it another way, should the stmmac solution to this be
> > > > made generic?
> > >
> > > May be partially?
> > >
> > > > Note that stmmac has this software timer implementation because not
> > > > only for the reason I've given above, but also because cores other than
> > > > GMAC4 that support LPI do not have support for the hardware timer.
> > >
> > > There seems to be a samsung ethernet driver which implements software
> > > based timer too.
> > >
> > > > > The same is about minimal value, too low value will cause strong speed
> > > > > degradation. Should we allow set insane minimum, but use sane default
> > > > > value?
> > > >
> > > > We currently allow zero, and the behaviour of that depends on the
> > > > hardware. For example, in the last couple of days, it's been reported
> > > > that stmmac will never enter LPI with a value of zero.
> > > >
> > > > Note that phylib defaults to zero, so imposing a minimum would cause
> > > > a read-modify-write of the EEE settings without setting the timer to
> > > > fail.
> > > >
> > > > > > Should set_eee() error out?
> > > > >
> > > > > Yes, please.
> > > >
> > > > If we are to convert stmmac, then we need to consider what it's doing
> > > > (as per the above) and whether that should be generic - and if it isn't
> > > > what we want in generic code, then how do we allow drivers to do this if
> > > > they wish.
> > >
> > > - EEE Advertisement:
> > >
> > > Advertising EEE capabilities is entirely dependent on the PHY. Without a
> > > PHY, these settings cannot be determined or validated, as the PHY defines the
> > > supported capabilities. Any attempt to configure EEE advertisement without an
> > > attached PHY should fail immediately with an appropriate error, such as: "EEE
> > > advertisement configuration not applied: no PHY available to validate
> > > capabilities."
> >
> > Sorry, at this point, I give up with phylink managed EEE. What you
> > detail above is way too much for me to get involved with, and goes
> > well beyond simply:
> >
> > 1) Fixing the cockup with the phylib-managed EEE that has caused *user*
> > *regressions* that we need to resolve.
> >
> > 2) Providing core functionality so that newer implementations can have
> > a consistency of behaviour.
> >
> > I have *no* interest in doing a total rewrite of kernel EEE
> > functionality - that goes well beyond my aims here.
> >
> > So I'm afraid that I really lost interest in reading your email, sorry.
>
> Sorry for killing your motivation. I can feel your pain...
I just don't think it's right to throw a whole new load of problems
to be solved into the mix when we already have issues in the kernel
caused by inappropriately merged previous patches.
Andrew had a large patch set, which added the phylib core code, and
fixed up many drivers. This was taken by someone else, and only
Andrew's core phylib code was merged along with the code for their
driver, thus breaking a heck of a lot of other drivers.
Either this needs to be fixed, or why don't we just declare that we've
broken EEE in the kernel, declare that we don't support EEE at all, and
rip the whole sorry damn thing out and start again from scratch -
because what you're suggesting is basically changing *everything*
about EEE support.
Yes, what we currently do may be sub-optimal, but it's the API that
we've presented to userspace for a long time.
I just don't think it's right to decide to pile all these new issues
on top of the utter crap situation we currently have.
Oh, lookie... I just looked back in the git history to find the person
who submitted the subset of Andrew's code was... YOU. YOU broke lots of
drivers by doing so. Now you're torpedoing attempts to fix them by
trying to make it more complicated. Sorry, but your opinion has just
lost all credibility with me given the mess you previously created
and haven't been bothered to try to fix up. At least *I've* been
trying to fix you crap.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 18:10 ` Russell King (Oracle)
@ 2025-01-09 19:33 ` Andrew Lunn
2025-01-13 12:42 ` Oleksij Rempel
2025-01-09 19:36 ` Oleksij Rempel
1 sibling, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2025-01-09 19:33 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
> Andrew had a large patch set, which added the phylib core code, and
> fixed up many drivers. This was taken by someone else, and only
> Andrew's core phylib code was merged along with the code for their
> driver, thus breaking a heck of a lot of other drivers.
As Russell says, i did convert all existing drivers over the new
internal API, and removed some ugly parts of the old EEE core code.
I'm not too happy we only got part way with my patches. Having this in
between state makes the internal APIs much harder to deal with, and as
Russell says, we broke a few MAC drivers because the rest did not get
merged.
Before we think about extensions to the kAPI, we first need to finish
the refactor. Get all MAC drivers over to the newer internal API and
remove phy_init_eee() which really does need to die. My patches have
probably bit rotted a bit, but i doubt they are unusable. So i would
like to see them merged. I would however leave phylink drivers to
Russell. He went a slight different way than i did, and he should get
to decide how phylink should support this.
Andrew
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 19:33 ` Andrew Lunn
@ 2025-01-13 12:42 ` Oleksij Rempel
2025-01-13 13:29 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-13 12:42 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King (Oracle), David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Woojung Huh, Andrew Lunn, kernel,
linux-kernel, netdev, UNGLinuxDriver, Phil Elwell
On Thu, Jan 09, 2025 at 08:33:07PM +0100, Andrew Lunn wrote:
> > Andrew had a large patch set, which added the phylib core code, and
> > fixed up many drivers. This was taken by someone else, and only
> > Andrew's core phylib code was merged along with the code for their
> > driver, thus breaking a heck of a lot of other drivers.
>
> As Russell says, i did convert all existing drivers over the new
> internal API, and removed some ugly parts of the old EEE core code.
> I'm not too happy we only got part way with my patches. Having this in
> between state makes the internal APIs much harder to deal with, and as
> Russell says, we broke a few MAC drivers because the rest did not get
> merged.
>
> Before we think about extensions to the kAPI, we first need to finish
> the refactor. Get all MAC drivers over to the newer internal API and
> remove phy_init_eee() which really does need to die. My patches have
> probably bit rotted a bit, but i doubt they are unusable. So i would
> like to see them merged. I would however leave phylink drivers to
> Russell. He went a slight different way than i did, and he should get
> to decide how phylink should support this.
Hi Andrew,
Ok. If I see it correctly, all patches from the
v6.4-rc6-net-next-ethtool-eee-v7 branch, which I was working on, have been
merged by now. The missing parts are patches from the
v6.3-rc3-net-next-ethtool-eee-v5 branch.
More precisely, the following non-phylink drivers still need to be addressed:
drivers/net/ethernet/broadcom/asp2
drivers/net/ethernet/broadcom/genet
drivers/net/ethernet/samsung/sxgbe
drivers/net/usb/lan78xx - this one is in progress
Unfortunately, I won’t be able to accomplish this before the merge window, as I
am currently on sick leave. However, I promise to take care of it as soon as
possible.
Best regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-13 12:42 ` Oleksij Rempel
@ 2025-01-13 13:29 ` Russell King (Oracle)
2025-01-13 13:45 ` Andrew Lunn
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-13 13:29 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Mon, Jan 13, 2025 at 01:42:06PM +0100, Oleksij Rempel wrote:
> On Thu, Jan 09, 2025 at 08:33:07PM +0100, Andrew Lunn wrote:
> > > Andrew had a large patch set, which added the phylib core code, and
> > > fixed up many drivers. This was taken by someone else, and only
> > > Andrew's core phylib code was merged along with the code for their
> > > driver, thus breaking a heck of a lot of other drivers.
> >
> > As Russell says, i did convert all existing drivers over the new
> > internal API, and removed some ugly parts of the old EEE core code.
> > I'm not too happy we only got part way with my patches. Having this in
> > between state makes the internal APIs much harder to deal with, and as
> > Russell says, we broke a few MAC drivers because the rest did not get
> > merged.
> >
> > Before we think about extensions to the kAPI, we first need to finish
> > the refactor. Get all MAC drivers over to the newer internal API and
> > remove phy_init_eee() which really does need to die. My patches have
> > probably bit rotted a bit, but i doubt they are unusable. So i would
> > like to see them merged. I would however leave phylink drivers to
> > Russell. He went a slight different way than i did, and he should get
> > to decide how phylink should support this.
>
> Hi Andrew,
>
> Ok. If I see it correctly, all patches from the
> v6.4-rc6-net-next-ethtool-eee-v7 branch, which I was working on, have been
> merged by now. The missing parts are patches from the
> v6.3-rc3-net-next-ethtool-eee-v5 branch.
>
> More precisely, the following non-phylink drivers still need to be addressed:
> drivers/net/ethernet/broadcom/asp2
> drivers/net/ethernet/broadcom/genet
> drivers/net/ethernet/samsung/sxgbe
> drivers/net/usb/lan78xx - this one is in progress
>
> Unfortunately, I won’t be able to accomplish this before the merge window, as I
> am currently on sick leave. However, I promise to take care of it as soon as
> possible.
Does any of this include mvneta?
static int mvneta_ethtool_get_eee(struct net_device *dev,
struct ethtool_keee *eee)
{
struct mvneta_port *pp = netdev_priv(dev);
u32 lpi_ctl0;
lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0);
eee->eee_enabled = pp->eee_enabled;
eee->eee_active = pp->eee_active;
eee->tx_lpi_enabled = pp->tx_lpi_enabled;
eee->tx_lpi_timer = (lpi_ctl0) >> 8; // * scale;
return phylink_ethtool_get_eee(pp->phylink, eee);
}
Any driver that writes any members of struct ethtool_keee before
calling phylink_ethtool_get_eee() or phy_ethtool_get_eee() was
broken by the merging of phylib-managed EEE. I've been submitting
patches to fix these over time, such as my recently merged DSA
patches that remove the now useless get_mac_eee() method.
The biggest problem is that phylib overwrites eee->tx_lpi_timer,
which broke a lot of drivers. For example, I've fixed FEC:
3fa2540d93d8 net: fec: use phydev->eee_cfg.tx_lpi_timer
Anyway, I'm going to re-post a cut-down version of my phylink-
managed EEE support once Herner's EEE patch set has been merged.
This will convert mvneta, lan743x and stmmac, and add support for
mvpp2, thus resolving the breakage in at last some of those drivers..
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-13 13:29 ` Russell King (Oracle)
@ 2025-01-13 13:45 ` Andrew Lunn
2025-01-17 16:23 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2025-01-13 13:45 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Mon, Jan 13, 2025 at 01:29:37PM +0000, Russell King (Oracle) wrote:
> On Mon, Jan 13, 2025 at 01:42:06PM +0100, Oleksij Rempel wrote:
> > On Thu, Jan 09, 2025 at 08:33:07PM +0100, Andrew Lunn wrote:
> > > > Andrew had a large patch set, which added the phylib core code, and
> > > > fixed up many drivers. This was taken by someone else, and only
> > > > Andrew's core phylib code was merged along with the code for their
> > > > driver, thus breaking a heck of a lot of other drivers.
> > >
> > > As Russell says, i did convert all existing drivers over the new
> > > internal API, and removed some ugly parts of the old EEE core code.
> > > I'm not too happy we only got part way with my patches. Having this in
> > > between state makes the internal APIs much harder to deal with, and as
> > > Russell says, we broke a few MAC drivers because the rest did not get
> > > merged.
> > >
> > > Before we think about extensions to the kAPI, we first need to finish
> > > the refactor. Get all MAC drivers over to the newer internal API and
> > > remove phy_init_eee() which really does need to die. My patches have
> > > probably bit rotted a bit, but i doubt they are unusable. So i would
> > > like to see them merged. I would however leave phylink drivers to
> > > Russell. He went a slight different way than i did, and he should get
> > > to decide how phylink should support this.
> >
> > Hi Andrew,
> >
> > Ok. If I see it correctly, all patches from the
> > v6.4-rc6-net-next-ethtool-eee-v7 branch, which I was working on, have been
> > merged by now. The missing parts are patches from the
> > v6.3-rc3-net-next-ethtool-eee-v5 branch.
> >
> > More precisely, the following non-phylink drivers still need to be addressed:
> > drivers/net/ethernet/broadcom/asp2
> > drivers/net/ethernet/broadcom/genet
> > drivers/net/ethernet/samsung/sxgbe
> > drivers/net/usb/lan78xx - this one is in progress
> >
> > Unfortunately, I won’t be able to accomplish this before the merge window, as I
> > am currently on sick leave. However, I promise to take care of it as soon as
> > possible.
>
> Does any of this include mvneta?
Hi Russell
I asked Oleksij to skip MAC drivers using phylink. I'm not sure it
makes sense to merge my phylink changes for EEE and then replace them
with your EEE implementation.
Andrew
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-13 13:45 ` Andrew Lunn
@ 2025-01-17 16:23 ` Russell King (Oracle)
2025-01-18 7:22 ` Oleksij Rempel
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-17 16:23 UTC (permalink / raw)
To: Andrew Lunn
Cc: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Mon, Jan 13, 2025 at 02:45:14PM +0100, Andrew Lunn wrote:
> On Mon, Jan 13, 2025 at 01:29:37PM +0000, Russell King (Oracle) wrote:
> > On Mon, Jan 13, 2025 at 01:42:06PM +0100, Oleksij Rempel wrote:
> > > On Thu, Jan 09, 2025 at 08:33:07PM +0100, Andrew Lunn wrote:
> > > > > Andrew had a large patch set, which added the phylib core code, and
> > > > > fixed up many drivers. This was taken by someone else, and only
> > > > > Andrew's core phylib code was merged along with the code for their
> > > > > driver, thus breaking a heck of a lot of other drivers.
> > > >
> > > > As Russell says, i did convert all existing drivers over the new
> > > > internal API, and removed some ugly parts of the old EEE core code.
> > > > I'm not too happy we only got part way with my patches. Having this in
> > > > between state makes the internal APIs much harder to deal with, and as
> > > > Russell says, we broke a few MAC drivers because the rest did not get
> > > > merged.
> > > >
> > > > Before we think about extensions to the kAPI, we first need to finish
> > > > the refactor. Get all MAC drivers over to the newer internal API and
> > > > remove phy_init_eee() which really does need to die. My patches have
> > > > probably bit rotted a bit, but i doubt they are unusable. So i would
> > > > like to see them merged. I would however leave phylink drivers to
> > > > Russell. He went a slight different way than i did, and he should get
> > > > to decide how phylink should support this.
> > >
> > > Hi Andrew,
> > >
> > > Ok. If I see it correctly, all patches from the
> > > v6.4-rc6-net-next-ethtool-eee-v7 branch, which I was working on, have been
> > > merged by now. The missing parts are patches from the
> > > v6.3-rc3-net-next-ethtool-eee-v5 branch.
> > >
> > > More precisely, the following non-phylink drivers still need to be addressed:
> > > drivers/net/ethernet/broadcom/asp2
> > > drivers/net/ethernet/broadcom/genet
> > > drivers/net/ethernet/samsung/sxgbe
> > > drivers/net/usb/lan78xx - this one is in progress
> > >
> > > Unfortunately, I won’t be able to accomplish this before the merge window, as I
> > > am currently on sick leave. However, I promise to take care of it as soon as
> > > possible.
> >
> > Does any of this include mvneta?
>
> Hi Russell
>
> I asked Oleksij to skip MAC drivers using phylink. I'm not sure it
> makes sense to merge my phylink changes for EEE and then replace them
> with your EEE implementation.
Doing an audit on today's net-next after I've fixed up another driver
for the phylib-eee fallout, we have two obvious breakages remaining
in drivers that directly use phylib:
drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c:
edata->tx_lpi_timer = priv->tx_lpi_timer;
return phy_ethtool_get_eee(dev->phydev, edata);
drivers/net/ethernet/broadcom/genet/bcmgenet.c:
e->tx_lpi_enabled = p->tx_lpi_enabled;
e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
return phy_ethtool_get_eee(dev->phydev, e);
Two others change ->tx_lpi_timer after calling phy_ethtool_get_eee()
and thus are unaffected:
drivers/net/ethernet/realtek/r8169_main.c:
drivers/net/usb/lan78xx.c
Howeve,r I wonder whether lan78xx_set_eee() is racy:
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);
since phy_ethtool_set_eee() will have bounced the link if the timer
was changed.
I'm also wondering whether r8169 programs its LPI timer correctly.
I think all phylink using drivers network are now no longer affected.
I'm unsure about many DSA drivers. mt753x:
u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
if (e->tx_lpi_timer > 0xFFF)
return -EINVAL;
set = LPI_THRESH_SET(e->tx_lpi_timer);
if (!e->tx_lpi_enabled)
/* Force LPI Mode without a delay */
set |= LPI_MODE_EN;
mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
Why force LPI *without* a delay if tx_lpi_enabled is false? This
seems to go against the documented API:
* @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
* that eee was negotiated.
qca8k_set_mac_eee() sets the LPI enabled based off eee->eee_enabled.
It doesn't seem to change the register on link up/down, so I wonder
how the autoneg resolution is handled. Maybe it isn't, so maybe it's
buggy.
b53_set_mac_eee() looks similar to qca8k_set_mac_eee().
So there's still work to be done.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-17 16:23 ` Russell King (Oracle)
@ 2025-01-18 7:22 ` Oleksij Rempel
2025-01-18 9:04 ` Russell King (Oracle)
2025-01-18 10:03 ` Oleksij Rempel
0 siblings, 2 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-18 7:22 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> I'm unsure about many DSA drivers. mt753x:
>
> u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
>
> if (e->tx_lpi_timer > 0xFFF)
> return -EINVAL;
>
> set = LPI_THRESH_SET(e->tx_lpi_timer);
> if (!e->tx_lpi_enabled)
> /* Force LPI Mode without a delay */
> set |= LPI_MODE_EN;
> mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
>
> Why force LPI *without* a delay if tx_lpi_enabled is false? This
> seems to go against the documented API:
>
> * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> * that eee was negotiated.
According to MT7531 manual, I would say, the code is not correct:
https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
The LPI_MODE_EN_Px bit has following meaning:
When there is no packet to be transmitted, and the idle time is greater
than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
Power Idle) mode and send EEE LPI frame to the link partner.
0: LPI mode depends on the P2_LPI_THRESHOLD.
1: Let the system enter the LPI mode immediately and send EEE LPI frame
to the link partner.
This chip seems to not have support for tx_lpi_enabled != eee_enabled
configuration.
> qca8k_set_mac_eee() sets the LPI enabled based off eee->eee_enabled.
> It doesn't seem to change the register on link up/down, so I wonder
> how the autoneg resolution is handled. Maybe it isn't, so maybe it's
> buggy.
The QCA8K_REG_EEE_CTRL_LPI_EN() bit is supported only for ports with
integrated PHYs. There seems to be no validation for this case.
Other problem with the code, lpi_en bit can be removed only one time.
Executing tx_lpi off and tx_lpi on in a sequence will not work.
My chip documentation do not provide any information about LPI_EN bit
functionality. I can't say for sure how it works.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-18 7:22 ` Oleksij Rempel
@ 2025-01-18 9:04 ` Russell King (Oracle)
2025-01-18 10:01 ` Oleksij Rempel
2025-01-18 10:03 ` Oleksij Rempel
1 sibling, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-18 9:04 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > I'm unsure about many DSA drivers. mt753x:
> >
> > u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> >
> > if (e->tx_lpi_timer > 0xFFF)
> > return -EINVAL;
> >
> > set = LPI_THRESH_SET(e->tx_lpi_timer);
> > if (!e->tx_lpi_enabled)
> > /* Force LPI Mode without a delay */
> > set |= LPI_MODE_EN;
> > mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> >
> > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > seems to go against the documented API:
> >
> > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > * that eee was negotiated.
>
> According to MT7531 manual, I would say, the code is not correct:
> https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
>
> The LPI_MODE_EN_Px bit has following meaning:
>
> When there is no packet to be transmitted, and the idle time is greater
> than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> Power Idle) mode and send EEE LPI frame to the link partner.
> 0: LPI mode depends on the P2_LPI_THRESHOLD.
> 1: Let the system enter the LPI mode immediately and send EEE LPI frame
> to the link partner.
Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.
> This chip seems to not have support for tx_lpi_enabled != eee_enabled
> configuration.
Sorry, I don't see your reasoning there - and I think your
interpretation is different from the documentation (which is
the whole point of having a generic implementation in phylib
to avoid these kinds of different interpretation.)
* @eee_enabled: EEE configured mode (enabled/disabled).
* @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
* that eee was negotiated.
eee on|off
Enables/disables the device support of EEE.
tx-lpi on|off
Determines whether the device should assert its Tx LPI.
The way phylib interprets eee_enabled is whether EEE is advertised
to the remote device or not. If EEE is not advertised, then EEE is
not negotiated, and thus EEE will not become active. If EEE is not
active, then LPI must not be asserted. tx_lpi_enabled defines whether,
given that EEE has been negotiated, whether LPI should be signalled
after the LPI timer has expired.
phylib deals with all this logic, and its all encoded into the
phydev->enable_tx_lpi flag to give consistency of implementation.
Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
negotiated at the specified speed && tx_lpi_enabled. In that state,
LPI is expected to be signalled after the LPI timer has expired.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-18 9:04 ` Russell King (Oracle)
@ 2025-01-18 10:01 ` Oleksij Rempel
2025-01-18 10:40 ` Russell King (Oracle)
0 siblings, 1 reply; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-18 10:01 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Sat, Jan 18, 2025 at 09:04:07AM +0000, Russell King (Oracle) wrote:
> On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> > On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > > I'm unsure about many DSA drivers. mt753x:
> > >
> > > u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> > >
> > > if (e->tx_lpi_timer > 0xFFF)
> > > return -EINVAL;
> > >
> > > set = LPI_THRESH_SET(e->tx_lpi_timer);
> > > if (!e->tx_lpi_enabled)
> > > /* Force LPI Mode without a delay */
> > > set |= LPI_MODE_EN;
> > > mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> > >
> > > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > > seems to go against the documented API:
> > >
> > > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > > * that eee was negotiated.
> >
> > According to MT7531 manual, I would say, the code is not correct:
> > https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
> >
> > The LPI_MODE_EN_Px bit has following meaning:
> >
> > When there is no packet to be transmitted, and the idle time is greater
> > than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> > Power Idle) mode and send EEE LPI frame to the link partner.
> > 0: LPI mode depends on the P2_LPI_THRESHOLD.
> > 1: Let the system enter the LPI mode immediately and send EEE LPI frame
> > to the link partner.
>
> Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
> LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.
>
> > This chip seems to not have support for tx_lpi_enabled != eee_enabled
> > configuration.
>
> Sorry, I don't see your reasoning there - and I think your
> interpretation is different from the documentation (which is
> the whole point of having a generic implementation in phylib
> to avoid these kinds of different interpretation.)
>
> * @eee_enabled: EEE configured mode (enabled/disabled).
> * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> * that eee was negotiated.
>
> eee on|off
> Enables/disables the device support of EEE.
>
> tx-lpi on|off
> Determines whether the device should assert its Tx LPI.
>
> The way phylib interprets eee_enabled is whether EEE is advertised
> to the remote device or not. If EEE is not advertised, then EEE is
> not negotiated, and thus EEE will not become active. If EEE is not
> active, then LPI must not be asserted. tx_lpi_enabled defines whether,
> given that EEE has been negotiated, whether LPI should be signalled
> after the LPI timer has expired.
>
> phylib deals with all this logic, and its all encoded into the
> phydev->enable_tx_lpi flag to give consistency of implementation.
>
> Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
> negotiated at the specified speed && tx_lpi_enabled. In that state,
> LPI is expected to be signalled after the LPI timer has expired.
I mean, the configuration where EEE can be enabled and in active state,
but TX LPI is disabled: eee_enabled = true; eee_active = true;
enable_tx_lpi = false. UAPI allows this configuration and it seems to
work for 100Mbit/s. Atheros documentation call it asymmetric EEE
operation - where each link partner enters LPI mode independently. In
comparison, the same documentation calls 1000Mbit EEE mode, symmetric
operation - where both link partner must enter the LPI mode
simulatneously.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-18 10:01 ` Oleksij Rempel
@ 2025-01-18 10:40 ` Russell King (Oracle)
2025-01-18 11:23 ` Oleksij Rempel
0 siblings, 1 reply; 33+ messages in thread
From: Russell King (Oracle) @ 2025-01-18 10:40 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Sat, Jan 18, 2025 at 11:01:22AM +0100, Oleksij Rempel wrote:
> On Sat, Jan 18, 2025 at 09:04:07AM +0000, Russell King (Oracle) wrote:
> > On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> > > On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > > > I'm unsure about many DSA drivers. mt753x:
> > > >
> > > > u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> > > >
> > > > if (e->tx_lpi_timer > 0xFFF)
> > > > return -EINVAL;
> > > >
> > > > set = LPI_THRESH_SET(e->tx_lpi_timer);
> > > > if (!e->tx_lpi_enabled)
> > > > /* Force LPI Mode without a delay */
> > > > set |= LPI_MODE_EN;
> > > > mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> > > >
> > > > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > > > seems to go against the documented API:
> > > >
> > > > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > > > * that eee was negotiated.
> > >
> > > According to MT7531 manual, I would say, the code is not correct:
> > > https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
> > >
> > > The LPI_MODE_EN_Px bit has following meaning:
> > >
> > > When there is no packet to be transmitted, and the idle time is greater
> > > than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> > > Power Idle) mode and send EEE LPI frame to the link partner.
> > > 0: LPI mode depends on the P2_LPI_THRESHOLD.
> > > 1: Let the system enter the LPI mode immediately and send EEE LPI frame
> > > to the link partner.
> >
> > Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
> > LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.
> >
> > > This chip seems to not have support for tx_lpi_enabled != eee_enabled
> > > configuration.
> >
> > Sorry, I don't see your reasoning there - and I think your
> > interpretation is different from the documentation (which is
> > the whole point of having a generic implementation in phylib
> > to avoid these kinds of different interpretation.)
> >
> > * @eee_enabled: EEE configured mode (enabled/disabled).
> > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > * that eee was negotiated.
> >
> > eee on|off
> > Enables/disables the device support of EEE.
> >
> > tx-lpi on|off
> > Determines whether the device should assert its Tx LPI.
> >
> > The way phylib interprets eee_enabled is whether EEE is advertised
> > to the remote device or not. If EEE is not advertised, then EEE is
> > not negotiated, and thus EEE will not become active. If EEE is not
> > active, then LPI must not be asserted. tx_lpi_enabled defines whether,
> > given that EEE has been negotiated, whether LPI should be signalled
> > after the LPI timer has expired.
> >
> > phylib deals with all this logic, and its all encoded into the
> > phydev->enable_tx_lpi flag to give consistency of implementation.
> >
> > Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
> > negotiated at the specified speed && tx_lpi_enabled. In that state,
> > LPI is expected to be signalled after the LPI timer has expired.
>
> I mean, the configuration where EEE can be enabled and in active state,
> but TX LPI is disabled: eee_enabled = true; eee_active = true;
> enable_tx_lpi = false. UAPI allows this configuration and it seems to
enable_tx_lpi is the result of phylib's management, and not a uAPI
thing. I think you mean the uAPI tx_lpi_enabled.
> work for 100Mbit/s. Atheros documentation call it asymmetric EEE
> operation - where each link partner enters LPI mode independently. In
> comparison, the same documentation calls 1000Mbit EEE mode, symmetric
> operation - where both link partner must enter the LPI mode
> simulatneously.
I'm not sure you are entirely correct.
FORCE_MODE_EEE100_P2
FORCE_MODE_EEE1G_P2
These bits seem to control whether the MT753x uses the result of polling
the PHY or the two force bits below to determine whether "EEE ability"
is determined.
FORCE_EEE1G_P2
FORCE_EEE100_P2
These bits determine whether, when their respective FORCE_MODE_EEE*_P2
bit is set, "EEE ability" is set or not.
"EEE ability" in this case would seem to basically be what we call
"EEE active" in kernel speak.
So, an implementation that would support our current uAPI fully would
be:
- Set FORCE_MODE_EEE*_P2 bits (thus making the "EEE ability" be
under software control rather than the result of the PHY polling
unit.)
- Set/clear FORCE_EEE*_P2 bits depending on phydev->enable_tx_lpi
- Set the timer according to phydev->eee_cfg.tx_lpi_timer
and that will support the user API in the way that its intended to be.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-18 10:40 ` Russell King (Oracle)
@ 2025-01-18 11:23 ` Oleksij Rempel
0 siblings, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-18 11:23 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Sat, Jan 18, 2025 at 10:40:26AM +0000, Russell King (Oracle) wrote:
> On Sat, Jan 18, 2025 at 11:01:22AM +0100, Oleksij Rempel wrote:
> > On Sat, Jan 18, 2025 at 09:04:07AM +0000, Russell King (Oracle) wrote:
> > > On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> > > > On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > > > > I'm unsure about many DSA drivers. mt753x:
> > > > >
> > > > > u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> > > > >
> > > > > if (e->tx_lpi_timer > 0xFFF)
> > > > > return -EINVAL;
> > > > >
> > > > > set = LPI_THRESH_SET(e->tx_lpi_timer);
> > > > > if (!e->tx_lpi_enabled)
> > > > > /* Force LPI Mode without a delay */
> > > > > set |= LPI_MODE_EN;
> > > > > mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> > > > >
> > > > > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > > > > seems to go against the documented API:
> > > > >
> > > > > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > > > > * that eee was negotiated.
> > > >
> > > > According to MT7531 manual, I would say, the code is not correct:
> > > > https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
> > > >
> > > > The LPI_MODE_EN_Px bit has following meaning:
> > > >
> > > > When there is no packet to be transmitted, and the idle time is greater
> > > > than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> > > > Power Idle) mode and send EEE LPI frame to the link partner.
> > > > 0: LPI mode depends on the P2_LPI_THRESHOLD.
> > > > 1: Let the system enter the LPI mode immediately and send EEE LPI frame
> > > > to the link partner.
> > >
> > > Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
> > > LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.
> > >
> > > > This chip seems to not have support for tx_lpi_enabled != eee_enabled
> > > > configuration.
> > >
> > > Sorry, I don't see your reasoning there - and I think your
> > > interpretation is different from the documentation (which is
> > > the whole point of having a generic implementation in phylib
> > > to avoid these kinds of different interpretation.)
> > >
> > > * @eee_enabled: EEE configured mode (enabled/disabled).
> > > * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > > * that eee was negotiated.
> > >
> > > eee on|off
> > > Enables/disables the device support of EEE.
> > >
> > > tx-lpi on|off
> > > Determines whether the device should assert its Tx LPI.
> > >
> > > The way phylib interprets eee_enabled is whether EEE is advertised
> > > to the remote device or not. If EEE is not advertised, then EEE is
> > > not negotiated, and thus EEE will not become active. If EEE is not
> > > active, then LPI must not be asserted. tx_lpi_enabled defines whether,
> > > given that EEE has been negotiated, whether LPI should be signalled
> > > after the LPI timer has expired.
> > >
> > > phylib deals with all this logic, and its all encoded into the
> > > phydev->enable_tx_lpi flag to give consistency of implementation.
> > >
> > > Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
> > > negotiated at the specified speed && tx_lpi_enabled. In that state,
> > > LPI is expected to be signalled after the LPI timer has expired.
> >
> > I mean, the configuration where EEE can be enabled and in active state,
> > but TX LPI is disabled: eee_enabled = true; eee_active = true;
> > enable_tx_lpi = false. UAPI allows this configuration and it seems to
>
> enable_tx_lpi is the result of phylib's management, and not a uAPI
> thing. I think you mean the uAPI tx_lpi_enabled.
yes.
> > work for 100Mbit/s. Atheros documentation call it asymmetric EEE
> > operation - where each link partner enters LPI mode independently. In
> > comparison, the same documentation calls 1000Mbit EEE mode, symmetric
> > operation - where both link partner must enter the LPI mode
> > simulatneously.
>
> I'm not sure you are entirely correct.
>
> FORCE_MODE_EEE100_P2
> FORCE_MODE_EEE1G_P2
>
> These bits seem to control whether the MT753x uses the result of polling
> the PHY or the two force bits below to determine whether "EEE ability"
> is determined.
>
> FORCE_EEE1G_P2
> FORCE_EEE100_P2
>
> These bits determine whether, when their respective FORCE_MODE_EEE*_P2
> bit is set, "EEE ability" is set or not.
>
> "EEE ability" in this case would seem to basically be what we call
> "EEE active" in kernel speak.
>
> So, an implementation that would support our current uAPI fully would
> be:
>
> - Set FORCE_MODE_EEE*_P2 bits (thus making the "EEE ability" be
> under software control rather than the result of the PHY polling
> unit.)
> - Set/clear FORCE_EEE*_P2 bits depending on phydev->enable_tx_lpi
> - Set the timer according to phydev->eee_cfg.tx_lpi_timer
>
> and that will support the user API in the way that its intended to be.
Ack, I see. It make sense.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-18 7:22 ` Oleksij Rempel
2025-01-18 9:04 ` Russell King (Oracle)
@ 2025-01-18 10:03 ` Oleksij Rempel
1 sibling, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-18 10:03 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn, kernel, linux-kernel,
netdev, UNGLinuxDriver, Phil Elwell
On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > qca8k_set_mac_eee() sets the LPI enabled based off eee->eee_enabled.
> > It doesn't seem to change the register on link up/down, so I wonder
> > how the autoneg resolution is handled. Maybe it isn't, so maybe it's
> > buggy.
>
> The QCA8K_REG_EEE_CTRL_LPI_EN() bit is supported only for ports with
> integrated PHYs. There seems to be no validation for this case.
> Other problem with the code, lpi_en bit can be removed only one time.
> Executing tx_lpi off and tx_lpi on in a sequence will not work.
Please ignore this lat part, I'm wrong here.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration
2025-01-09 18:10 ` Russell King (Oracle)
2025-01-09 19:33 ` Andrew Lunn
@ 2025-01-09 19:36 ` Oleksij Rempel
1 sibling, 0 replies; 33+ messages in thread
From: Oleksij Rempel @ 2025-01-09 19:36 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Thu, Jan 09, 2025 at 06:10:15PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 09, 2025 at 06:39:45PM +0100, Oleksij Rempel wrote:
> > On Thu, Jan 09, 2025 at 05:27:11PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Jan 09, 2025 at 06:13:10PM +0100, Oleksij Rempel wrote:
> > > > On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> > > > > On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > > > > > Yes, otherwise every MAC driver will need to do it in the
> > > > > > ethtool_set_eee() function.
> > > > >
> > > > > I've had several solutions, and my latest patch set actually has a
> > > > > mixture of them in there (which is why I'm eager to try and find a way
> > > > > forward on this, so I can fix the patch set):
> > > > >
> > > > > 1. the original idea to address this in Marvell platforms was to limit
> > > > > the LPI timer to the maximum representable value in the hardware,
> > > > > which would be 255us. This ignores that the hardware uses a 1us
> > > > > tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> > > > > (So it limits it to 260us, even though the hardware can do 2550us
> > > > > at 100M speed). This limit was applied by clamping the value passed
> > > > > in from userspace without erroring out.
> > > > >
> > > > > 2. another solution was added the mac_validate_tx_lpi() method, and
> > > > > implementations added _in addition_ to the above, with the idea
> > > > > of erroring out for values > 255us on Marvell hardware.
> > > > >
> > > > > 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> > > > > possible to allow e.g. falling back to a software timer (see stmmac
> > > > > comments below.) Another reason for erroring out applies to Marvell
> > > > > hardware, where PP2 hardware supports LPI on the GMAC but not the
> > > > > XGMAC - so it only works at speeds at or below 2.5G. However, that
> > > > > can be handled via the lpi_capabilities, so I don't think needs to
> > > > > be a concern.
> > > > >
> > > > > > The other question is, should we allow absolute maximum values, or sane
> > > > > > maximum? At some point will come the question, why the EEE is even
> > > > > > enabled?
> > > > >
> > > > > As referenced above, stmmac uses the hardware timer for LPI timeouts up
> > > > > to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> > > > > normal kernel timer which is:
> > > > >
> > > > > - disabled (and EEE mode reset) when we have a packet to transmit, or
> > > > > EEE is disabled
> > > > > - is re-armed when cleaning up from packet transmission (although
> > > > > it looks like we attempt to immediately enter LPI mode, and would
> > > > > only wait for the timer if there are more packets to queue... maybe
> > > > > this is a bug in stmmac's implementation?) or when EEE mode is first
> > > > > enabled with a LPI timer longer than the above value.
> > > > >
> > > > > So, should phylink have the capability to switch to a software LPI timer
> > > > > implementation when the LPI timeout value exceeds what the hardware
> > > > > supports?
> > > >
> > > > No, i'll list my arguments later down.
> > > >
> > > > > To put it another way, should the stmmac solution to this be
> > > > > made generic?
> > > >
> > > > May be partially?
> > > >
> > > > > Note that stmmac has this software timer implementation because not
> > > > > only for the reason I've given above, but also because cores other than
> > > > > GMAC4 that support LPI do not have support for the hardware timer.
> > > >
> > > > There seems to be a samsung ethernet driver which implements software
> > > > based timer too.
> > > >
> > > > > > The same is about minimal value, too low value will cause strong speed
> > > > > > degradation. Should we allow set insane minimum, but use sane default
> > > > > > value?
> > > > >
> > > > > We currently allow zero, and the behaviour of that depends on the
> > > > > hardware. For example, in the last couple of days, it's been reported
> > > > > that stmmac will never enter LPI with a value of zero.
> > > > >
> > > > > Note that phylib defaults to zero, so imposing a minimum would cause
> > > > > a read-modify-write of the EEE settings without setting the timer to
> > > > > fail.
> > > > >
> > > > > > > Should set_eee() error out?
> > > > > >
> > > > > > Yes, please.
> > > > >
> > > > > If we are to convert stmmac, then we need to consider what it's doing
> > > > > (as per the above) and whether that should be generic - and if it isn't
> > > > > what we want in generic code, then how do we allow drivers to do this if
> > > > > they wish.
> > > >
> > > > - EEE Advertisement:
> > > >
> > > > Advertising EEE capabilities is entirely dependent on the PHY. Without a
> > > > PHY, these settings cannot be determined or validated, as the PHY defines the
> > > > supported capabilities. Any attempt to configure EEE advertisement without an
> > > > attached PHY should fail immediately with an appropriate error, such as: "EEE
> > > > advertisement configuration not applied: no PHY available to validate
> > > > capabilities."
> > >
> > > Sorry, at this point, I give up with phylink managed EEE. What you
> > > detail above is way too much for me to get involved with, and goes
> > > well beyond simply:
> > >
> > > 1) Fixing the cockup with the phylib-managed EEE that has caused *user*
> > > *regressions* that we need to resolve.
> > >
> > > 2) Providing core functionality so that newer implementations can have
> > > a consistency of behaviour.
> > >
> > > I have *no* interest in doing a total rewrite of kernel EEE
> > > functionality - that goes well beyond my aims here.
> > >
> > > So I'm afraid that I really lost interest in reading your email, sorry.
> >
> > Sorry for killing your motivation. I can feel your pain...
>
> I just don't think it's right to throw a whole new load of problems
> to be solved into the mix when we already have issues in the kernel
> caused by inappropriately merged previous patches.
>
> Andrew had a large patch set, which added the phylib core code, and
> fixed up many drivers. This was taken by someone else, and only
> Andrew's core phylib code was merged along with the code for their
> driver, thus breaking a heck of a lot of other drivers.
>
> Either this needs to be fixed, or why don't we just declare that we've
> broken EEE in the kernel, declare that we don't support EEE at all, and
> rip the whole sorry damn thing out and start again from scratch -
> because what you're suggesting is basically changing *everything*
> about EEE support.
>
> Yes, what we currently do may be sub-optimal, but it's the API that
> we've presented to userspace for a long time.
>
> I just don't think it's right to decide to pile all these new issues
> on top of the utter crap situation we currently have.
>
> Oh, lookie... I just looked back in the git history to find the person
> who submitted the subset of Andrew's code was... YOU. YOU broke lots of
> drivers by doing so. Now you're torpedoing attempts to fix them by
> trying to make it more complicated. Sorry, but your opinion has just
> lost all credibility with me given the mess you previously created
> and haven't been bothered to try to fix up. At least *I've* been
> trying to fix you crap.
First, I want to say that I understand your frustration with the current
situation and appreciate the effort you've put into improving EEE.
However, I find the tone of your message inappropriate. Personal and
highly emotional comments make constructive communication much harder.
While we may not be working as a team, we share a common interest in
improving the Linux kernel. I acknowledge that there have been issues
with some of my patches in the past, and I take responsibility for them.
My goal has always been to contribute meaningful improvements and learn
from any mistakes.
Let’s focus on the shared goal of making the kernel better. A respectful
and technical discussion will benefit everyone involved and ensure
progress is made.
Best regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 33+ messages in thread