[v2,1/5] net: macb: add phylink support
diff mbox series

Message ID 1560933636-29684-1-git-send-email-pthombar@cadence.com
State Changes Requested
Headers show
Series
  • net: macb: cover letter
Related show

Commit Message

Parshuram Raju Thombare June 19, 2019, 8:40 a.m. UTC
This patch replace phylib API's by phylink API's.

Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
 drivers/net/ethernet/cadence/Kconfig     |   2 +-
 drivers/net/ethernet/cadence/macb.h      |   3 +
 drivers/net/ethernet/cadence/macb_main.c | 312 +++++++++++++----------
 3 files changed, 182 insertions(+), 135 deletions(-)

Comments

Russell King - ARM Linux admin June 19, 2019, 9:22 a.m. UTC | #1
On Wed, Jun 19, 2019 at 09:40:36AM +0100, Parshuram Thombare wrote:
> This patch replace phylib API's by phylink API's.
> 
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
>  drivers/net/ethernet/cadence/Kconfig     |   2 +-
>  drivers/net/ethernet/cadence/macb.h      |   3 +
>  drivers/net/ethernet/cadence/macb_main.c | 312 +++++++++++++----------
>  3 files changed, 182 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 1766697c9c5a..d71411a71587 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -22,7 +22,7 @@ if NET_VENDOR_CADENCE
>  config MACB
>  	tristate "Cadence MACB/GEM support"
>  	depends on HAS_DMA
> -	select PHYLIB
> +	select PHYLINK
>  	---help---
>  	  The Cadence MACB ethernet interface is found on many Atmel AT32 and
>  	  AT91 parts.  This driver also supports the Cadence GEM (Gigabit
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 00ee5e8e0ff0..35ed13236c8b 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -14,6 +14,7 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/interrupt.h>
> +#include <linux/phylink.h>
>  
>  #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
>  #define MACB_EXT_DESC
> @@ -1227,6 +1228,8 @@ struct macb {
>  	u32	rx_intr_mask;
>  
>  	struct macb_pm_data pm_data;
> +	struct phylink *pl;
> +	struct phylink_config pl_config;
>  };
>  
>  #ifdef CONFIG_MACB_USE_HWSTAMP
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c545c5b435d8..830af86d3c65 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -39,6 +39,7 @@
>  #include <linux/tcp.h>
>  #include <linux/iopoll.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/phylink.h>
>  #include "macb.h"
>  
>  /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -438,115 +439,150 @@ static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
>  		netdev_err(dev, "adjusting tx_clk failed.\n");
>  }
>  
> -static void macb_handle_link_change(struct net_device *dev)
> +static void gem_phylink_validate(struct phylink_config *pl_config,
> +				 unsigned long *supported,
> +				 struct phylink_link_state *state)
>  {
> -	struct macb *bp = netdev_priv(dev);
> -	struct phy_device *phydev = dev->phydev;
> -	unsigned long flags;
> -	int status_change = 0;
> +	struct net_device *netdev = to_net_dev(pl_config->dev);
> +	struct macb *bp = netdev_priv(netdev);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_GMII:
> +	case PHY_INTERFACE_MODE_RGMII:
> +		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> +			phylink_set(mask, 1000baseT_Full);
> +			phylink_set(mask, 1000baseX_Full);
> +			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) {
> +				phylink_set(mask, 1000baseT_Half);
> +				phylink_set(mask, 1000baseT_Half);
> +			}
> +		}
> +	/* fallthrough */
> +	case PHY_INTERFACE_MODE_MII:
> +	case PHY_INTERFACE_MODE_RMII:
> +		phylink_set(mask, 10baseT_Half);
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Half);
> +		phylink_set(mask, 100baseT_Full);
> +		break;
> +	default:
> +		break;
> +	}
>  
> -	spin_lock_irqsave(&bp->lock, flags);
> +	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_and(state->advertising, state->advertising, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);

Consider using linkmode_and() here.

> +}
>  
> -	if (phydev->link) {
> -		if ((bp->speed != phydev->speed) ||
> -		    (bp->duplex != phydev->duplex)) {
> -			u32 reg;
> +static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
> +				      struct phylink_link_state *state)
> +{
> +	struct net_device *netdev = to_net_dev(pl_config->dev);
> +	struct macb *bp = netdev_priv(netdev);
>  
> -			reg = macb_readl(bp, NCFGR);
> -			reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> -			if (macb_is_gem(bp))
> -				reg &= ~GEM_BIT(GBE);
> +	state->speed = bp->speed;
> +	state->duplex = bp->duplex;
> +	state->link = bp->link;

You can't read from the hardware what the actual MAC is doing?

> +	return 1;
> +}
>  
> -			if (phydev->duplex)
> -				reg |= MACB_BIT(FD);
> -			if (phydev->speed == SPEED_100)
> -				reg |= MACB_BIT(SPD);
> -			if (phydev->speed == SPEED_1000 &&
> -			    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> -				reg |= GEM_BIT(GBE);
> +static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
> +			   const struct phylink_link_state *state)
> +{
> +	struct net_device *netdev = to_net_dev(pl_config->dev);
> +	struct macb *bp = netdev_priv(netdev);
> +	unsigned long flags;
>  
> -			macb_or_gem_writel(bp, NCFGR, reg);
> +	spin_lock_irqsave(&bp->lock, flags);
>  
> -			bp->speed = phydev->speed;
> -			bp->duplex = phydev->duplex;
> -			status_change = 1;
> -		}
> -	}
> +	if (bp->speed != state->speed ||
> +	    bp->duplex != state->duplex) {

Please read the updated phylink documentation - state->{speed,duplex}
are not always valid depending on the negotiation mode.

> +		u32 reg;
>  
> -	if (phydev->link != bp->link) {
> -		if (!phydev->link) {
> -			bp->speed = 0;
> -			bp->duplex = -1;
> +		reg = macb_readl(bp, NCFGR);
> +		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> +		if (macb_is_gem(bp))
> +			reg &= ~GEM_BIT(GBE);
> +		if (state->duplex)
> +			reg |= MACB_BIT(FD);
> +
> +		switch (state->speed) {
> +		case SPEED_1000:
> +			reg |= GEM_BIT(GBE);
> +			break;
> +		case SPEED_100:
> +			reg |= MACB_BIT(SPD);
> +			break;
> +		default:
> +			break;
>  		}
> -		bp->link = phydev->link;
> +		macb_or_gem_writel(bp, NCFGR, reg);
> +
> +		bp->speed = state->speed;
> +		bp->duplex = state->duplex;
>  
> -		status_change = 1;
> +		if (state->link)
> +			macb_set_tx_clk(bp->tx_clk, state->speed, netdev);
>  	}
>  
>  	spin_unlock_irqrestore(&bp->lock, flags);
> +}
>  
> -	if (status_change) {
> -		if (phydev->link) {
> -			/* Update the TX clock rate if and only if the link is
> -			 * up and there has been a link change.
> -			 */
> -			macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
> +static void gem_mac_link_up(struct phylink_config *pl_config, unsigned int mode,
> +			    phy_interface_t interface, struct phy_device *phy)
> +{
> +	struct net_device *netdev = to_net_dev(pl_config->dev);
> +	struct macb *bp = netdev_priv(netdev);
>  
> -			netif_carrier_on(dev);
> -			netdev_info(dev, "link up (%d/%s)\n",
> -				    phydev->speed,
> -				    phydev->duplex == DUPLEX_FULL ?
> -				    "Full" : "Half");
> -		} else {
> -			netif_carrier_off(dev);
> -			netdev_info(dev, "link down\n");
> -		}
> -	}
> +	bp->link = 1;
> +	/* Enable TX and RX */
> +	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
> +}
> +
> +static void gem_mac_link_down(struct phylink_config *pl_config,
> +			      unsigned int mode, phy_interface_t interface)
> +{
> +	struct net_device *netdev = to_net_dev(pl_config->dev);
> +	struct macb *bp = netdev_priv(netdev);
> +
> +	bp->link = 0;
> +	/* Disable TX and RX */
> +	macb_writel(bp, NCR,
> +		    macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE)));
>  }
>  
> +static const struct phylink_mac_ops gem_phylink_ops = {
> +	.validate = gem_phylink_validate,
> +	.mac_link_state = gem_phylink_mac_link_state,
> +	.mac_config = gem_mac_config,
> +	.mac_link_up = gem_mac_link_up,
> +	.mac_link_down = gem_mac_link_down,
> +};
> +
>  /* based on au1000_eth. c*/
>  static int macb_mii_probe(struct net_device *dev)
>  {
>  	struct macb *bp = netdev_priv(dev);
>  	struct phy_device *phydev;
>  	struct device_node *np;
> -	int ret, i;
> +	int ret;
>  
>  	np = bp->pdev->dev.of_node;
>  	ret = 0;
>  
> -	if (np) {
> -		if (of_phy_is_fixed_link(np)) {
> -			bp->phy_node = of_node_get(np);
> -		} else {
> -			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
> -			/* fallback to standard phy registration if no
> -			 * phy-handle was found nor any phy found during
> -			 * dt phy registration
> -			 */
> -			if (!bp->phy_node && !phy_find_first(bp->mii_bus)) {
> -				for (i = 0; i < PHY_MAX_ADDR; i++) {
> -					phydev = mdiobus_scan(bp->mii_bus, i);
> -					if (IS_ERR(phydev) &&
> -					    PTR_ERR(phydev) != -ENODEV) {
> -						ret = PTR_ERR(phydev);
> -						break;
> -					}
> -				}
> -
> -				if (ret)
> -					return -ENODEV;
> -			}
> -		}
> +	bp->pl_config.dev = &dev->dev;
> +	bp->pl_config.type = PHYLINK_NETDEV;
> +	bp->pl = phylink_create(&bp->pl_config, of_fwnode_handle(np),
> +				bp->phy_interface, &gem_phylink_ops);
> +	if (IS_ERR(bp->pl)) {
> +		netdev_err(dev,
> +			   "error creating PHYLINK: %ld\n", PTR_ERR(bp->pl));
> +		return PTR_ERR(bp->pl);
>  	}

At this point bp->pl can never be NULL.

>  
> -	if (bp->phy_node) {
> -		phydev = of_phy_connect(dev, bp->phy_node,
> -					&macb_handle_link_change, 0,
> -					bp->phy_interface);
> -		if (!phydev)
> -			return -ENODEV;
> -	} else {
> +	ret = phylink_of_phy_connect(bp->pl, np, 0);
> +	if (ret == -ENODEV && bp->mii_bus) {
>  		phydev = phy_find_first(bp->mii_bus);
>  		if (!phydev) {
>  			netdev_err(dev, "no PHY found\n");
> @@ -554,29 +590,18 @@ static int macb_mii_probe(struct net_device *dev)
>  		}
>  
>  		/* attach the mac to the phy */
> -		ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
> -					 bp->phy_interface);
> +		ret = phylink_connect_phy(bp->pl, phydev);
>  		if (ret) {
>  			netdev_err(dev, "Could not attach to PHY\n");
>  			return ret;
>  		}
>  	}
>  
> -	/* mask with MAC supported features */
> -	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> -		phy_set_max_speed(phydev, SPEED_1000);
> -	else
> -		phy_set_max_speed(phydev, SPEED_100);
> -
> -	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
> -		phy_remove_link_mode(phydev,
> -				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -
>  	bp->link = 0;
>  	bp->speed = 0;
>  	bp->duplex = -1;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int macb_mii_init(struct macb *bp)
> @@ -604,17 +629,7 @@ static int macb_mii_init(struct macb *bp)
>  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>  
>  	np = bp->pdev->dev.of_node;
> -	if (np && of_phy_is_fixed_link(np)) {
> -		if (of_phy_register_fixed_link(np) < 0) {
> -			dev_err(&bp->pdev->dev,
> -				"broken fixed-link specification %pOF\n", np);
> -			goto err_out_free_mdiobus;
> -		}
> -
> -		err = mdiobus_register(bp->mii_bus);
> -	} else {
> -		err = of_mdiobus_register(bp->mii_bus, np);
> -	}
> +	err = of_mdiobus_register(bp->mii_bus, np);
>  
>  	if (err)
>  		goto err_out_free_fixed_link;
> @@ -630,7 +645,6 @@ static int macb_mii_init(struct macb *bp)
>  err_out_free_fixed_link:
>  	if (np && of_phy_is_fixed_link(np))
>  		of_phy_deregister_fixed_link(np);
> -err_out_free_mdiobus:
>  	of_node_put(bp->phy_node);
>  	mdiobus_free(bp->mii_bus);
>  err_out:
> @@ -2422,7 +2436,7 @@ static int macb_open(struct net_device *dev)
>  	netif_carrier_off(dev);
>  
>  	/* if the phy is not yet register, retry later*/
> -	if (!dev->phydev) {
> +	if (!bp->pl) {

So this check is unnecessary.

>  		err = -EAGAIN;
>  		goto pm_exit;
>  	}
> @@ -2444,7 +2458,7 @@ static int macb_open(struct net_device *dev)
>  	macb_init_hw(bp);
>  
>  	/* schedule a link state check */
> -	phy_start(dev->phydev);
> +	phylink_start(bp->pl);
>  
>  	netif_tx_start_all_queues(dev);
>  
> @@ -2471,8 +2485,8 @@ static int macb_close(struct net_device *dev)
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
>  		napi_disable(&queue->napi);
>  
> -	if (dev->phydev)
> -		phy_stop(dev->phydev);
> +	if (bp->pl)
> +		phylink_stop(bp->pl);

Ditto.

>  
>  	spin_lock_irqsave(&bp->lock, flags);
>  	macb_reset_hw(bp);
> @@ -3161,6 +3175,29 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
>  	return ret;
>  }
>  
> +static int gem_ethtool_get_link_ksettings(struct net_device *netdev,
> +					  struct ethtool_link_ksettings *cmd)
> +{
> +	struct macb *bp = netdev_priv(netdev);
> +
> +	if (!bp->pl)
> +		return -ENOTSUPP;

Ditto.

> +
> +	return phylink_ethtool_ksettings_get(bp->pl, cmd);
> +}
> +
> +static int
> +gem_ethtool_set_link_ksettings(struct net_device *netdev,
> +			       const struct ethtool_link_ksettings *cmd)
> +{
> +	struct macb *bp = netdev_priv(netdev);
> +
> +	if (!bp->pl)
> +		return -ENOTSUPP;

Ditto.

> +
> +	return phylink_ethtool_ksettings_set(bp->pl, cmd);
> +}
> +
>  static const struct ethtool_ops macb_ethtool_ops = {
>  	.get_regs_len		= macb_get_regs_len,
>  	.get_regs		= macb_get_regs,
> @@ -3168,8 +3205,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  	.get_wol		= macb_get_wol,
>  	.set_wol		= macb_set_wol,
> -	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
> -	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
> +	.get_link_ksettings     = gem_ethtool_get_link_ksettings,
> +	.set_link_ksettings     = gem_ethtool_set_link_ksettings,
>  	.get_ringparam		= macb_get_ringparam,
>  	.set_ringparam		= macb_set_ringparam,
>  };
> @@ -3182,8 +3219,8 @@ static const struct ethtool_ops gem_ethtool_ops = {
>  	.get_ethtool_stats	= gem_get_ethtool_stats,
>  	.get_strings		= gem_get_ethtool_strings,
>  	.get_sset_count		= gem_get_sset_count,
> -	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
> -	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
> +	.get_link_ksettings     = gem_ethtool_get_link_ksettings,
> +	.set_link_ksettings     = gem_ethtool_set_link_ksettings,
>  	.get_ringparam		= macb_get_ringparam,
>  	.set_ringparam		= macb_set_ringparam,
>  	.get_rxnfc			= gem_get_rxnfc,
> @@ -3192,17 +3229,16 @@ static const struct ethtool_ops gem_ethtool_ops = {
>  
>  static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  {
> -	struct phy_device *phydev = dev->phydev;
>  	struct macb *bp = netdev_priv(dev);
>  
>  	if (!netif_running(dev))
>  		return -EINVAL;
>  
> -	if (!phydev)
> +	if (!bp->pl)
>  		return -ENODEV;

Ditto.

>  
>  	if (!bp->ptp_info)
> -		return phy_mii_ioctl(phydev, rq, cmd);
> +		return phylink_mii_ioctl(bp->pl, rq, cmd);
>  
>  	switch (cmd) {
>  	case SIOCSHWTSTAMP:
> @@ -3210,7 +3246,7 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	case SIOCGHWTSTAMP:
>  		return bp->ptp_info->get_hwtst(dev, rq);
>  	default:
> -		return phy_mii_ioctl(phydev, rq, cmd);
> +		return phylink_mii_ioctl(bp->pl, rq, cmd);
>  	}
>  }
>  
> @@ -3710,7 +3746,7 @@ static int at91ether_open(struct net_device *dev)
>  			     MACB_BIT(HRESP));
>  
>  	/* schedule a link state check */
> -	phy_start(dev->phydev);
> +	phylink_start(lp->pl);
>  
>  	netif_start_queue(dev);
>  
> @@ -4183,13 +4219,12 @@ static int macb_probe(struct platform_device *pdev)
>  	struct clk *tsu_clk = NULL;
>  	unsigned int queue_mask, num_queues;
>  	bool native_io;
> -	struct phy_device *phydev;
>  	struct net_device *dev;
>  	struct resource *regs;
>  	void __iomem *mem;
>  	const char *mac;
>  	struct macb *bp;
> -	int err, val;
> +	int err, val, phy_mode;
>  
>  	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	mem = devm_ioremap_resource(&pdev->dev, regs);
> @@ -4310,12 +4345,12 @@ static int macb_probe(struct platform_device *pdev)
>  		macb_get_hwaddr(bp);
>  	}
>  
> -	err = of_get_phy_mode(np);
> -	if (err < 0)
> +	phy_mode = of_get_phy_mode(np);
> +	if (phy_mode < 0)
>  		/* not found in DT, MII by default */
>  		bp->phy_interface = PHY_INTERFACE_MODE_MII;
>  	else
> -		bp->phy_interface = err;
> +		bp->phy_interface = phy_mode;

The phy interface mode is managed by phylink - and there are phys out
there that dynamically change their link mode.  You may wish to update
the link mode in your mac_config() implementation too.

>  
>  	/* IP specific init */
>  	err = init(pdev);
> @@ -4326,8 +4361,6 @@ static int macb_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_out_free_netdev;
>  
> -	phydev = dev->phydev;
> -
>  	netif_carrier_off(dev);
>  
>  	err = register_netdev(dev);
> @@ -4339,7 +4372,8 @@ static int macb_probe(struct platform_device *pdev)
>  	tasklet_init(&bp->hresp_err_tasklet, macb_hresp_error_task,
>  		     (unsigned long)bp);
>  
> -	phy_attached_info(phydev);
> +	if (dev->phydev)
> +		phy_attached_info(dev->phydev);

phylink already prints information about the attached phy, why do we
need another print here?

>  
>  	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
>  		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
> @@ -4351,7 +4385,9 @@ static int macb_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_out_unregister_mdio:
> -	phy_disconnect(dev->phydev);
> +	rtnl_lock();
> +	phylink_disconnect_phy(bp->pl);
> +	rtnl_unlock();
>  	mdiobus_unregister(bp->mii_bus);
>  	of_node_put(bp->phy_node);
>  	if (np && of_phy_is_fixed_link(np))
> @@ -4385,13 +4421,18 @@ static int macb_remove(struct platform_device *pdev)
>  
>  	if (dev) {
>  		bp = netdev_priv(dev);
> -		if (dev->phydev)
> -			phy_disconnect(dev->phydev);
> +		if (bp->pl) {
> +			rtnl_lock();
> +			phylink_disconnect_phy(bp->pl);
> +			rtnl_unlock();
> +		}
>  		mdiobus_unregister(bp->mii_bus);
>  		if (np && of_phy_is_fixed_link(np))
>  			of_phy_deregister_fixed_link(np);
>  		dev->phydev = NULL;
>  		mdiobus_free(bp->mii_bus);
> +		if (bp->pl)
> +			phylink_destroy(bp->pl);
>  
>  		unregister_netdev(dev);
>  		pm_runtime_disable(&pdev->dev);
> @@ -4434,8 +4475,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  		for (q = 0, queue = bp->queues; q < bp->num_queues;
>  		     ++q, ++queue)
>  			napi_disable(&queue->napi);
> -		phy_stop(netdev->phydev);
> -		phy_suspend(netdev->phydev);
> +		phylink_stop(bp->pl);
> +		if (netdev->phydev)
> +			phy_suspend(netdev->phydev);

When the attached phy is stopped, the state machine suspends the phy.
Why do we need an explicit call to phy_suspend() here, bypassing
phylink?

>  		spin_lock_irqsave(&bp->lock, flags);
>  		macb_reset_hw(bp);
>  		spin_unlock_irqrestore(&bp->lock, flags);
> @@ -4483,9 +4525,11 @@ static int __maybe_unused macb_resume(struct device *dev)
>  		for (q = 0, queue = bp->queues; q < bp->num_queues;
>  		     ++q, ++queue)
>  			napi_enable(&queue->napi);
> -		phy_resume(netdev->phydev);
> -		phy_init_hw(netdev->phydev);
> -		phy_start(netdev->phydev);
> +		if (netdev->phydev) {
> +			phy_resume(netdev->phydev);
> +			phy_init_hw(netdev->phydev);
> +		}
> +		phylink_start(bp->pl);

When the phy is started, the phy state machine will resume the phy.
Same question as above.

>  	}
>  
>  	bp->macbgem_ops.mog_init_rings(bp);
> -- 
> 2.17.1
> 
>
Parshuram Raju Thombare June 19, 2019, 10:51 a.m. UTC | #2
Hi Russel,

Thanks for review comments.

>On Wed, Jun 19, 2019 at 09:40:36AM +0100, Parshuram Thombare wrote:
>

>> +	bitmap_and(supported, supported, mask,
>__ETHTOOL_LINK_MODE_MASK_NBITS);
>
>> +	bitmap_and(state->advertising, state->advertising, mask,
>
>> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
>
>
>
>Consider using linkmode_and() here.
>
Ok

>> +static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
>
>> +				      struct phylink_link_state *state)
>
>> +	state->speed = bp->speed;
>
>> +	state->duplex = bp->duplex;
>
>> +	state->link = bp->link;
>
>
>
>You can't read from the hardware what the actual MAC is doing?

As  mostly PHY mode was used in driver and this method is called only for
in band mode, I added this is just as place holder and used in next patch set
where SGMII support is added. Should I remove it from this patch ?


>> +static void gem_mac_config(struct phylink_config *pl_config, unsigned int
>mode,
>> +			   const struct phylink_link_state *state)
>
>> +{
>> +	if (bp->speed != state->speed ||
>
>> +	    bp->duplex != state->duplex) {
>
>Please read the updated phylink documentation - state->{speed,duplex}
>are not always valid depending on the negotiation mode.

At least for PHY and FIXED mode I see mac_config is called after state is updated in phylink_resolve().
In case of IN BAND mode, I see mac_config may not get called after state is updated in mac_link_state()
method. Are you suggesting to configure MAC here only for FIXED and PHY mode ?


>> +	bp->pl_config.type = PHYLINK_NETDEV;
>
>> +	bp->pl = phylink_create(&bp->pl_config, of_fwnode_handle(np),
>
>> +				bp->phy_interface, &gem_phylink_ops);
>
>> +	if (IS_ERR(bp->pl)) {
>
>> +		netdev_err(dev,
>
>> +			   "error creating PHYLINK: %ld\n", PTR_ERR(bp->pl));
>
>> +		return PTR_ERR(bp->pl);
>
>>  	}
>
>At this point bp->pl can never be NULL.

phylink_create() does return failure also. I think this comment is not for above snippet.

>
>> -	if (!dev->phydev) {
>
>> +	if (!bp->pl) {
>
>
>
>So this check is unnecessary.
>

Ok, I will remove this check.

>> -	if (dev->phydev)
>
>> -		phy_stop(dev->phydev);
>
>> +	if (bp->pl)
>
>> +		phylink_stop(bp->pl);
>
>
>
>Ditto.

Ok, I will remove this redundant check.


>> +	if (!bp->pl)
>
>> +		return -ENOTSUPP;
>
>
>
>Ditto.

Ok, I will remove this redundant check.

>
>> +	if (!bp->pl)
>
>> +		return -ENOTSUPP;
>
>
>
>Ditto.

Ok, I will remove this redundant check.

>
>> +	if (!bp->pl)
>
>>  		return -ENODEV;
>
>
>
>Ditto.

Ok, I will remove this redundant check.


>> @@ -4183,13 +4219,12 @@ static int macb_probe(struct platform_device
>*pdev)
>
>>  	struct clk *tsu_clk = NULL;
>
>>  	unsigned int queue_mask, num_queues;
>> +	phy_mode = of_get_phy_mode(np);
>
>> +	if (phy_mode < 0)
>
>>  		/* not found in DT, MII by default */
>
>>  		bp->phy_interface = PHY_INTERFACE_MODE_MII;
>
>>  	else
>
>> -		bp->phy_interface = err;
>
>> +		bp->phy_interface = phy_mode;
>
>The phy interface mode is managed by phylink - and there are phys out
>there that dynamically change their link mode.  You may wish to update
>the link mode in your mac_config() implementation too.
>
Ok, I will modify mac_config to check phy_mode and program MAC accordingly.


>> +	if (dev->phydev)
>
>> +		phy_attached_info(dev->phydev);[] 
>
>phylink already prints information about the attached phy, why do we
>need another print here?
>

Ok, I will remove this.

>> -		phy_stop(netdev->phydev);
>> -		phy_suspend(netdev->phydev);
>> +		phylink_stop(bp->pl);
>> +		if (netdev->phydev)
>> +			phy_suspend(netdev->phydev);
>
>When the attached phy is stopped, the state machine suspends the phy.
>Why do we need an explicit call to phy_suspend() here, bypassing
>phylink?
>

Here I am just trying to keep functionality unchanged, just replacing 
Phylib API's with phylink API's.

>> -		phy_resume(netdev->phydev);
>
>> -		phy_init_hw(netdev->phydev);
>
>> -		phy_start(netdev->phydev);
>
>> +		if (netdev->phydev) {
>
>> +			phy_resume(netdev->phydev);
>
>> +			phy_init_hw(netdev->phydev);
>
>> +		}
>
>> +		phylink_start(bp->pl);
>
>
>
>When the phy is started, the phy state machine will resume the phy.
>Same question as above.
>

Here I am just trying to keep functionality unchanged, just replacing 
Phylib API's with phylink API's.


Regards,
Parshuram Thombare

Patch
diff mbox series

diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 1766697c9c5a..d71411a71587 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -22,7 +22,7 @@  if NET_VENDOR_CADENCE
 config MACB
 	tristate "Cadence MACB/GEM support"
 	depends on HAS_DMA
-	select PHYLIB
+	select PHYLINK
 	---help---
 	  The Cadence MACB ethernet interface is found on many Atmel AT32 and
 	  AT91 parts.  This driver also supports the Cadence GEM (Gigabit
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 00ee5e8e0ff0..35ed13236c8b 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -14,6 +14,7 @@ 
 #include <linux/ptp_clock_kernel.h>
 #include <linux/net_tstamp.h>
 #include <linux/interrupt.h>
+#include <linux/phylink.h>
 
 #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
 #define MACB_EXT_DESC
@@ -1227,6 +1228,8 @@  struct macb {
 	u32	rx_intr_mask;
 
 	struct macb_pm_data pm_data;
+	struct phylink *pl;
+	struct phylink_config pl_config;
 };
 
 #ifdef CONFIG_MACB_USE_HWSTAMP
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c545c5b435d8..830af86d3c65 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -39,6 +39,7 @@ 
 #include <linux/tcp.h>
 #include <linux/iopoll.h>
 #include <linux/pm_runtime.h>
+#include <linux/phylink.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -438,115 +439,150 @@  static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
 		netdev_err(dev, "adjusting tx_clk failed.\n");
 }
 
-static void macb_handle_link_change(struct net_device *dev)
+static void gem_phylink_validate(struct phylink_config *pl_config,
+				 unsigned long *supported,
+				 struct phylink_link_state *state)
 {
-	struct macb *bp = netdev_priv(dev);
-	struct phy_device *phydev = dev->phydev;
-	unsigned long flags;
-	int status_change = 0;
+	struct net_device *netdev = to_net_dev(pl_config->dev);
+	struct macb *bp = netdev_priv(netdev);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_GMII:
+	case PHY_INTERFACE_MODE_RGMII:
+		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_Full);
+			if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) {
+				phylink_set(mask, 1000baseT_Half);
+				phylink_set(mask, 1000baseT_Half);
+			}
+		}
+	/* fallthrough */
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_RMII:
+		phylink_set(mask, 10baseT_Half);
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Half);
+		phylink_set(mask, 100baseT_Full);
+		break;
+	default:
+		break;
+	}
 
-	spin_lock_irqsave(&bp->lock, flags);
+	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
+	bitmap_and(state->advertising, state->advertising, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+}
 
-	if (phydev->link) {
-		if ((bp->speed != phydev->speed) ||
-		    (bp->duplex != phydev->duplex)) {
-			u32 reg;
+static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
+				      struct phylink_link_state *state)
+{
+	struct net_device *netdev = to_net_dev(pl_config->dev);
+	struct macb *bp = netdev_priv(netdev);
 
-			reg = macb_readl(bp, NCFGR);
-			reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
-			if (macb_is_gem(bp))
-				reg &= ~GEM_BIT(GBE);
+	state->speed = bp->speed;
+	state->duplex = bp->duplex;
+	state->link = bp->link;
+	return 1;
+}
 
-			if (phydev->duplex)
-				reg |= MACB_BIT(FD);
-			if (phydev->speed == SPEED_100)
-				reg |= MACB_BIT(SPD);
-			if (phydev->speed == SPEED_1000 &&
-			    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
-				reg |= GEM_BIT(GBE);
+static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
+			   const struct phylink_link_state *state)
+{
+	struct net_device *netdev = to_net_dev(pl_config->dev);
+	struct macb *bp = netdev_priv(netdev);
+	unsigned long flags;
 
-			macb_or_gem_writel(bp, NCFGR, reg);
+	spin_lock_irqsave(&bp->lock, flags);
 
-			bp->speed = phydev->speed;
-			bp->duplex = phydev->duplex;
-			status_change = 1;
-		}
-	}
+	if (bp->speed != state->speed ||
+	    bp->duplex != state->duplex) {
+		u32 reg;
 
-	if (phydev->link != bp->link) {
-		if (!phydev->link) {
-			bp->speed = 0;
-			bp->duplex = -1;
+		reg = macb_readl(bp, NCFGR);
+		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
+		if (macb_is_gem(bp))
+			reg &= ~GEM_BIT(GBE);
+		if (state->duplex)
+			reg |= MACB_BIT(FD);
+
+		switch (state->speed) {
+		case SPEED_1000:
+			reg |= GEM_BIT(GBE);
+			break;
+		case SPEED_100:
+			reg |= MACB_BIT(SPD);
+			break;
+		default:
+			break;
 		}
-		bp->link = phydev->link;
+		macb_or_gem_writel(bp, NCFGR, reg);
+
+		bp->speed = state->speed;
+		bp->duplex = state->duplex;
 
-		status_change = 1;
+		if (state->link)
+			macb_set_tx_clk(bp->tx_clk, state->speed, netdev);
 	}
 
 	spin_unlock_irqrestore(&bp->lock, flags);
+}
 
-	if (status_change) {
-		if (phydev->link) {
-			/* Update the TX clock rate if and only if the link is
-			 * up and there has been a link change.
-			 */
-			macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
+static void gem_mac_link_up(struct phylink_config *pl_config, unsigned int mode,
+			    phy_interface_t interface, struct phy_device *phy)
+{
+	struct net_device *netdev = to_net_dev(pl_config->dev);
+	struct macb *bp = netdev_priv(netdev);
 
-			netif_carrier_on(dev);
-			netdev_info(dev, "link up (%d/%s)\n",
-				    phydev->speed,
-				    phydev->duplex == DUPLEX_FULL ?
-				    "Full" : "Half");
-		} else {
-			netif_carrier_off(dev);
-			netdev_info(dev, "link down\n");
-		}
-	}
+	bp->link = 1;
+	/* Enable TX and RX */
+	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
+}
+
+static void gem_mac_link_down(struct phylink_config *pl_config,
+			      unsigned int mode, phy_interface_t interface)
+{
+	struct net_device *netdev = to_net_dev(pl_config->dev);
+	struct macb *bp = netdev_priv(netdev);
+
+	bp->link = 0;
+	/* Disable TX and RX */
+	macb_writel(bp, NCR,
+		    macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE)));
 }
 
+static const struct phylink_mac_ops gem_phylink_ops = {
+	.validate = gem_phylink_validate,
+	.mac_link_state = gem_phylink_mac_link_state,
+	.mac_config = gem_mac_config,
+	.mac_link_up = gem_mac_link_up,
+	.mac_link_down = gem_mac_link_down,
+};
+
 /* based on au1000_eth. c*/
 static int macb_mii_probe(struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
 	struct phy_device *phydev;
 	struct device_node *np;
-	int ret, i;
+	int ret;
 
 	np = bp->pdev->dev.of_node;
 	ret = 0;
 
-	if (np) {
-		if (of_phy_is_fixed_link(np)) {
-			bp->phy_node = of_node_get(np);
-		} else {
-			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
-			/* fallback to standard phy registration if no
-			 * phy-handle was found nor any phy found during
-			 * dt phy registration
-			 */
-			if (!bp->phy_node && !phy_find_first(bp->mii_bus)) {
-				for (i = 0; i < PHY_MAX_ADDR; i++) {
-					phydev = mdiobus_scan(bp->mii_bus, i);
-					if (IS_ERR(phydev) &&
-					    PTR_ERR(phydev) != -ENODEV) {
-						ret = PTR_ERR(phydev);
-						break;
-					}
-				}
-
-				if (ret)
-					return -ENODEV;
-			}
-		}
+	bp->pl_config.dev = &dev->dev;
+	bp->pl_config.type = PHYLINK_NETDEV;
+	bp->pl = phylink_create(&bp->pl_config, of_fwnode_handle(np),
+				bp->phy_interface, &gem_phylink_ops);
+	if (IS_ERR(bp->pl)) {
+		netdev_err(dev,
+			   "error creating PHYLINK: %ld\n", PTR_ERR(bp->pl));
+		return PTR_ERR(bp->pl);
 	}
 
-	if (bp->phy_node) {
-		phydev = of_phy_connect(dev, bp->phy_node,
-					&macb_handle_link_change, 0,
-					bp->phy_interface);
-		if (!phydev)
-			return -ENODEV;
-	} else {
+	ret = phylink_of_phy_connect(bp->pl, np, 0);
+	if (ret == -ENODEV && bp->mii_bus) {
 		phydev = phy_find_first(bp->mii_bus);
 		if (!phydev) {
 			netdev_err(dev, "no PHY found\n");
@@ -554,29 +590,18 @@  static int macb_mii_probe(struct net_device *dev)
 		}
 
 		/* attach the mac to the phy */
-		ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
-					 bp->phy_interface);
+		ret = phylink_connect_phy(bp->pl, phydev);
 		if (ret) {
 			netdev_err(dev, "Could not attach to PHY\n");
 			return ret;
 		}
 	}
 
-	/* mask with MAC supported features */
-	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
-		phy_set_max_speed(phydev, SPEED_1000);
-	else
-		phy_set_max_speed(phydev, SPEED_100);
-
-	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
-		phy_remove_link_mode(phydev,
-				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
 	bp->link = 0;
 	bp->speed = 0;
 	bp->duplex = -1;
 
-	return 0;
+	return ret;
 }
 
 static int macb_mii_init(struct macb *bp)
@@ -604,17 +629,7 @@  static int macb_mii_init(struct macb *bp)
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
 
 	np = bp->pdev->dev.of_node;
-	if (np && of_phy_is_fixed_link(np)) {
-		if (of_phy_register_fixed_link(np) < 0) {
-			dev_err(&bp->pdev->dev,
-				"broken fixed-link specification %pOF\n", np);
-			goto err_out_free_mdiobus;
-		}
-
-		err = mdiobus_register(bp->mii_bus);
-	} else {
-		err = of_mdiobus_register(bp->mii_bus, np);
-	}
+	err = of_mdiobus_register(bp->mii_bus, np);
 
 	if (err)
 		goto err_out_free_fixed_link;
@@ -630,7 +645,6 @@  static int macb_mii_init(struct macb *bp)
 err_out_free_fixed_link:
 	if (np && of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
-err_out_free_mdiobus:
 	of_node_put(bp->phy_node);
 	mdiobus_free(bp->mii_bus);
 err_out:
@@ -2422,7 +2436,7 @@  static int macb_open(struct net_device *dev)
 	netif_carrier_off(dev);
 
 	/* if the phy is not yet register, retry later*/
-	if (!dev->phydev) {
+	if (!bp->pl) {
 		err = -EAGAIN;
 		goto pm_exit;
 	}
@@ -2444,7 +2458,7 @@  static int macb_open(struct net_device *dev)
 	macb_init_hw(bp);
 
 	/* schedule a link state check */
-	phy_start(dev->phydev);
+	phylink_start(bp->pl);
 
 	netif_tx_start_all_queues(dev);
 
@@ -2471,8 +2485,8 @@  static int macb_close(struct net_device *dev)
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
 		napi_disable(&queue->napi);
 
-	if (dev->phydev)
-		phy_stop(dev->phydev);
+	if (bp->pl)
+		phylink_stop(bp->pl);
 
 	spin_lock_irqsave(&bp->lock, flags);
 	macb_reset_hw(bp);
@@ -3161,6 +3175,29 @@  static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static int gem_ethtool_get_link_ksettings(struct net_device *netdev,
+					  struct ethtool_link_ksettings *cmd)
+{
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!bp->pl)
+		return -ENOTSUPP;
+
+	return phylink_ethtool_ksettings_get(bp->pl, cmd);
+}
+
+static int
+gem_ethtool_set_link_ksettings(struct net_device *netdev,
+			       const struct ethtool_link_ksettings *cmd)
+{
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!bp->pl)
+		return -ENOTSUPP;
+
+	return phylink_ethtool_ksettings_set(bp->pl, cmd);
+}
+
 static const struct ethtool_ops macb_ethtool_ops = {
 	.get_regs_len		= macb_get_regs_len,
 	.get_regs		= macb_get_regs,
@@ -3168,8 +3205,8 @@  static const struct ethtool_ops macb_ethtool_ops = {
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_wol		= macb_get_wol,
 	.set_wol		= macb_set_wol,
-	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings     = gem_ethtool_get_link_ksettings,
+	.set_link_ksettings     = gem_ethtool_set_link_ksettings,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
 };
@@ -3182,8 +3219,8 @@  static const struct ethtool_ops gem_ethtool_ops = {
 	.get_ethtool_stats	= gem_get_ethtool_stats,
 	.get_strings		= gem_get_ethtool_strings,
 	.get_sset_count		= gem_get_sset_count,
-	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings     = gem_ethtool_get_link_ksettings,
+	.set_link_ksettings     = gem_ethtool_set_link_ksettings,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
 	.get_rxnfc			= gem_get_rxnfc,
@@ -3192,17 +3229,16 @@  static const struct ethtool_ops gem_ethtool_ops = {
 
 static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
-	struct phy_device *phydev = dev->phydev;
 	struct macb *bp = netdev_priv(dev);
 
 	if (!netif_running(dev))
 		return -EINVAL;
 
-	if (!phydev)
+	if (!bp->pl)
 		return -ENODEV;
 
 	if (!bp->ptp_info)
-		return phy_mii_ioctl(phydev, rq, cmd);
+		return phylink_mii_ioctl(bp->pl, rq, cmd);
 
 	switch (cmd) {
 	case SIOCSHWTSTAMP:
@@ -3210,7 +3246,7 @@  static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	case SIOCGHWTSTAMP:
 		return bp->ptp_info->get_hwtst(dev, rq);
 	default:
-		return phy_mii_ioctl(phydev, rq, cmd);
+		return phylink_mii_ioctl(bp->pl, rq, cmd);
 	}
 }
 
@@ -3710,7 +3746,7 @@  static int at91ether_open(struct net_device *dev)
 			     MACB_BIT(HRESP));
 
 	/* schedule a link state check */
-	phy_start(dev->phydev);
+	phylink_start(lp->pl);
 
 	netif_start_queue(dev);
 
@@ -4183,13 +4219,12 @@  static int macb_probe(struct platform_device *pdev)
 	struct clk *tsu_clk = NULL;
 	unsigned int queue_mask, num_queues;
 	bool native_io;
-	struct phy_device *phydev;
 	struct net_device *dev;
 	struct resource *regs;
 	void __iomem *mem;
 	const char *mac;
 	struct macb *bp;
-	int err, val;
+	int err, val, phy_mode;
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mem = devm_ioremap_resource(&pdev->dev, regs);
@@ -4310,12 +4345,12 @@  static int macb_probe(struct platform_device *pdev)
 		macb_get_hwaddr(bp);
 	}
 
-	err = of_get_phy_mode(np);
-	if (err < 0)
+	phy_mode = of_get_phy_mode(np);
+	if (phy_mode < 0)
 		/* not found in DT, MII by default */
 		bp->phy_interface = PHY_INTERFACE_MODE_MII;
 	else
-		bp->phy_interface = err;
+		bp->phy_interface = phy_mode;
 
 	/* IP specific init */
 	err = init(pdev);
@@ -4326,8 +4361,6 @@  static int macb_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out_free_netdev;
 
-	phydev = dev->phydev;
-
 	netif_carrier_off(dev);
 
 	err = register_netdev(dev);
@@ -4339,7 +4372,8 @@  static int macb_probe(struct platform_device *pdev)
 	tasklet_init(&bp->hresp_err_tasklet, macb_hresp_error_task,
 		     (unsigned long)bp);
 
-	phy_attached_info(phydev);
+	if (dev->phydev)
+		phy_attached_info(dev->phydev);
 
 	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
@@ -4351,7 +4385,9 @@  static int macb_probe(struct platform_device *pdev)
 	return 0;
 
 err_out_unregister_mdio:
-	phy_disconnect(dev->phydev);
+	rtnl_lock();
+	phylink_disconnect_phy(bp->pl);
+	rtnl_unlock();
 	mdiobus_unregister(bp->mii_bus);
 	of_node_put(bp->phy_node);
 	if (np && of_phy_is_fixed_link(np))
@@ -4385,13 +4421,18 @@  static int macb_remove(struct platform_device *pdev)
 
 	if (dev) {
 		bp = netdev_priv(dev);
-		if (dev->phydev)
-			phy_disconnect(dev->phydev);
+		if (bp->pl) {
+			rtnl_lock();
+			phylink_disconnect_phy(bp->pl);
+			rtnl_unlock();
+		}
 		mdiobus_unregister(bp->mii_bus);
 		if (np && of_phy_is_fixed_link(np))
 			of_phy_deregister_fixed_link(np);
 		dev->phydev = NULL;
 		mdiobus_free(bp->mii_bus);
+		if (bp->pl)
+			phylink_destroy(bp->pl);
 
 		unregister_netdev(dev);
 		pm_runtime_disable(&pdev->dev);
@@ -4434,8 +4475,9 @@  static int __maybe_unused macb_suspend(struct device *dev)
 		for (q = 0, queue = bp->queues; q < bp->num_queues;
 		     ++q, ++queue)
 			napi_disable(&queue->napi);
-		phy_stop(netdev->phydev);
-		phy_suspend(netdev->phydev);
+		phylink_stop(bp->pl);
+		if (netdev->phydev)
+			phy_suspend(netdev->phydev);
 		spin_lock_irqsave(&bp->lock, flags);
 		macb_reset_hw(bp);
 		spin_unlock_irqrestore(&bp->lock, flags);
@@ -4483,9 +4525,11 @@  static int __maybe_unused macb_resume(struct device *dev)
 		for (q = 0, queue = bp->queues; q < bp->num_queues;
 		     ++q, ++queue)
 			napi_enable(&queue->napi);
-		phy_resume(netdev->phydev);
-		phy_init_hw(netdev->phydev);
-		phy_start(netdev->phydev);
+		if (netdev->phydev) {
+			phy_resume(netdev->phydev);
+			phy_init_hw(netdev->phydev);
+		}
+		phylink_start(bp->pl);
 	}
 
 	bp->macbgem_ops.mog_init_rings(bp);