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

[<Rengarajan.S@microchip.com>]

Hi Oleksji,

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

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

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

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

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