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

[Thangaraj.S]

Hi Oleksji,

On Fri, 2025-03-07 at 19:24 +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@xxxxxxxxxxxxxx>
> ---
> 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
> ---
>  drivers/net/usb/lan78xx.c | 565 ++++++++++++++++++++--------------
> ----
>  1 file changed, 300 insertions(+), 265 deletions(-)
> 
>  static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
> -       __ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
> -       int ret;
> -       u32 mii_adv;
>         struct phy_device *phydev;
> +       int ret;
> 
>         switch (dev->chipid) {
>         case ID_REV_CHIP_ID_7801_:
>                 phydev = lan7801_phy_init(dev);
> +               /* If no PHY found, set fixed link, probably there is
> no
> +                * device or some kind of different device like
> switch.
> +                * For example: EVB-KSZ9897-1 (KSZ9897 switch
> evaluation board
> +                * with LAN7801 & KSZ9031)
> +                */
>                 if (!phydev) {
> -                       netdev_err(dev->net, "lan7801: PHY Init
> Failed");
> -                       return -EIO;
> +                       struct phylink_link_state state = {
> +                               .speed = SPEED_1000,
> +                               .duplex = DUPLEX_FULL,
> +                               .interface =
> PHY_INTERFACE_MODE_RGMII,
> +                       };
> +
> +                       ret = phylink_set_fixed_link(dev->phylink,
> &state);
> +                       if (ret)
> +                               netdev_err(dev->net, "Could not set
> fixed link\n");
> +
> +                       return ret;
>                 }
> +
>                 break;
> 
>         case ID_REV_CHIP_ID_7800_:
> @@ -2576,7 +2671,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_GMII;
>                 break;
> 
>         default:
> @@ -2591,36 +2686,14 @@ static int lan78xx_phy_init(struct
> lan78xx_net *dev)
>                 phydev->irq = PHY_POLL;
>         netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
> 
> -       /* set to AUTOMDIX */
> -       phydev->mdix = ETH_TP_MDI_AUTO;
> -
> -       ret = phy_connect_direct(dev->net, phydev,
> -                                lan78xx_link_status_change,
> -                                dev->interface);
> +       ret = phylink_connect_phy(dev->phylink, phydev);
>         if (ret) {
>                 netdev_err(dev->net, "can't attach PHY to %s\n",
>                            dev->mdiobus->id);
> -               if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> -                       if (phy_is_pseudo_fixed_link(phydev)) {
> -                               fixed_phy_unregister(phydev);
> -                               phy_device_free(phydev);
> -                       }
> -               }
>                 return -EIO;
>         }
> 
> -       /* 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);
> 

Why phy_suspend called in the init? Is there any specific reason?

>         phy_support_eee(phydev);
> 
> @@ -2646,10 +2719,6 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                 }
>         }
> 
> -       genphy_config_aneg(phydev);
> -
> -       dev->fc_autoneg = phydev->autoneg;
> -
>         return 0;
>  }
> 
>