Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

Rengarajan.S
2025-01-17T10:50:53Z

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
>