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

[<Thangaraj.S@microchip.com>]

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));
> +}
> +
>