Message-ID: <0328797731d5ec0af580cf2f4f7e000212befa69.camel@microchip.com>
Date: Fri, 17 Jan 2025 10:50:48 +0000
From: <Rengarajan.S@...rochip.com>
To: <andrew+netdev@...n.ch>, <rmk+kernel@...linux.org.uk>,
	<davem@...emloft.net>, <Woojung.Huh@...rochip.com>, <pabeni@...hat.com>,
	<o.rempel@...gutronix.de>, <edumazet@...gle.com>, <kuba@...nel.org>
CC: <phil@...pberrypi.org>, <kernel@...gutronix.de>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for
 improved PHY and MAC management

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@...gutronix.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
> 

Powered by blists - more mailing lists