Message-ID: <20191023090757.GA3355@kwain>
Date:   Wed, 23 Oct 2019 11:07:57 +0200
From:   Antoine Tenart <antoine.tenart@...tlin.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Antoine Tenart <antoine.tenart@...tlin.com>,
        davem@...emloft.net, alexandre.belloni@...tlin.com,
        nicolas.ferre@...rochip.com, netdev@...r.kernel.org,
        thomas.petazzoni@...tlin.com
Subject: Re: [PATCH net-next] net: macb: convert to phylink

Hi Russell,

Sorry for not including you at first on this patch, I'll make sure I do
for v2.

On Fri, Oct 18, 2019 at 09:08:23PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Oct 18, 2019 at 09:08:10PM +0200, Andrew Lunn wrote:
> > On Fri, Oct 18, 2019 at 04:39:24PM +0200, Antoine Tenart wrote:
> > > +
> > > +static int macb_mac_link_state(struct phylink_config *config,
> > > +			       struct phylink_link_state *state)
> > > +{
> > > +	return -EOPNOTSUPP;
> 
> Without this implemented, phylink can't support in-band link modes
> necessary for SFP support.  Since phylink is mostly about supporting
> SFP cages and similar, I'm not sure what the point is.

See the answer below.

> > > +static void macb_mac_config(struct phylink_config *config, unsigned int mode,
> > > +			    const struct phylink_link_state *state)
> > > +{
> > > +	struct net_device *ndev = to_net_dev(config->dev);
> > > +	struct macb *bp = netdev_priv(ndev);
> > >  	unsigned long flags;
> > > -	int status_change = 0;
> > > +	u32 old_ctrl, ctrl;
> > >  
> > >  	spin_lock_irqsave(&bp->lock, flags);
> > >  
> > > -	if (phydev->link) {
> > > -		if ((bp->speed != phydev->speed) ||
> > > -		    (bp->duplex != phydev->duplex)) {
> > > -			u32 reg;
> > > +	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
> > >  
> > > -			reg = macb_readl(bp, NCFGR);
> > > -			reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> > > -			if (macb_is_gem(bp))
> > > -				reg &= ~GEM_BIT(GBE);
> > > +	/* Clear all the bits we might set later */
> > > +	ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) |
> > > +		  GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
> > >  
> > > -			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);
> > > +	if (state->speed == SPEED_1000)
> > > +		ctrl |= GEM_BIT(GBE);
> > > +	else if (state->speed == SPEED_100)
> > > +		ctrl |= MACB_BIT(SPD);
> > >  
> > > -			macb_or_gem_writel(bp, NCFGR, reg);
> > > +	if (state->duplex)
> > > +		ctrl |= MACB_BIT(FD);
> > >  
> > > -			bp->speed = phydev->speed;
> > > -			bp->duplex = phydev->duplex;
> > > -			status_change = 1;
> > > -		}
> > > -	}
> > > +	/* We do not support MLO_PAUSE_RX yet */
> > > +	if (state->pause & MLO_PAUSE_TX)
> > > +		ctrl |= MACB_BIT(PAE);
> > >  
> > > -	if (phydev->link != bp->link) {
> > > -		if (!phydev->link) {
> > > -			bp->speed = 0;
> > > -			bp->duplex = -1;
> > > -		}
> > > -		bp->link = phydev->link;
> > > +	if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > > +		ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
> 
> Hmm, so you support SGMII but have no way to read the results of SGMII
> negotiation?

That seemed odd, I did not find a way to get the status (at least in the
datasheet I was using). I sent a first version without link_state being
implemented, but I'll double check now.

> > > +	/* Apply the new configuration, if any */
> > > +	if (old_ctrl ^ ctrl) {
> > > +		macb_or_gem_writel(bp, NCFGR, ctrl);
> > >  
> > > -		status_change = 1;
> > > +		if (state->link)
> > > +			macb_set_tx_clk(bp->tx_clk, state->speed, ndev);
> 
> Please see include/linux/phylink.h for documentation on the mac_config()
> method.  It exhaustively describes which members of the state are valid
> for each value of "mode", and describes which are not valid to be used.
> "state->link" is one such case of an invalid use.

I'll have a look at this. I may be done in link_up as well, where we
know the state of the link.

> > > +static int macb_phylink_connect(struct macb *bp)
> > > +{
> > > +	struct net_device *dev = bp->dev;
> > > +	struct phy_device *phydev;
> > > +	int ret;
> > > +
> > > +	if (bp->phy_node) {
> > > +		ret = phylink_of_phy_connect(bp->phylink, bp->phy_node, 0);
> 
> This looks quite odd.
> 
> phylink expects the second argument to always be the container device
> node of the "phy-handle", "phy" or "phy-device" property, never the
> node of the PHY itself.  Looking at how the driver sets up
> bp->phy_node, it is the PHY's node itself sometimes, and in that case
> will almost certainly fail.

That right, thanks for spotting this. I'll fix it.

> > > +		if (ret) {
> > > +			netdev_err(dev, "Could not attach PHY (%d)\n", ret);
> > > +			return ret;
> > > +		}
> > > +	} else {
> > > +		phydev = phy_find_first(bp->mii_bus);
> > > +		if (!phydev) {
> > > +			netdev_err(dev, "no PHY found\n");
> > > +			return -ENXIO;
> > > +		}
> > > +
> > > +		/* attach the mac to the phy */
> > > +		ret = phylink_connect_phy(bp->phylink, phydev);
> > > +		if (ret) {
> > > +			netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
> > > +			return ret;
> > >  		}
> 
> I'm not entirely sure what this is trying to achieve.

You may not have a "phy" property in the MAC node with this driver, so I
kept the logic of what was done previously, using phy_find_first() when
the phy isn't described in the dt. Do you suggest another approach?

> > >  	}
> > > +
> > > +	phylink_start(bp->phylink);
> 
> You're ready to handle a link-up event at this point in the driver?

Yes, the h/w is initialized, and we can handle link events.

> > >  static int macb_open(struct net_device *dev)
> > >  {
> > > -	struct macb *bp = netdev_priv(dev);
> > >  	size_t bufsz = dev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN;
> > > +	struct macb *bp = netdev_priv(dev);
> > >  	struct macb_queue *queue;
> > >  	unsigned int q;
> > >  	int err;
> > > @@ -2417,12 +2480,6 @@ static int macb_open(struct net_device *dev)
> > >  	/* carrier starts down */
> > >  	netif_carrier_off(dev);
> 
> Note, that's included in phylink_start().

That's right, I added it to phylink_start... :) I'll fix it.

> > > @@ -2467,8 +2524,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);
> > > +	phylink_stop(bp->phylink);
> > > +	phylink_disconnect_phy(bp->phylink);
> 
> This is fine if _this_ driver was the one to attach the PHY, otherwise
> it will disconnect someone else's PHY (eg, the copper SFP PHY if this
> driver supports SFPs.)

Do you suggest removing this? I'm seeing lots (all?) of phylink
converted drivers to still call this, what's the reasoning behind that?

> > > @@ -2703,8 +2760,9 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
> > >  	wol->wolopts = 0;
> > >  
> > >  	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
> > > -		wol->supported = WAKE_MAGIC;
> > > +		phylink_ethtool_get_wol(bp->phylink, wol);
> > >  
> > > +		wol->supported |= WAKE_MAGIC;
> > >  		if (bp->wol & MACB_WOL_ENABLED)
> > >  			wol->wolopts |= WAKE_MAGIC;
> 
> I'm not sure that the logic here is actually correct.  What would be
> the result if the PHY has WOL disabled but bp->wol has both these
> enabled?

That's a good question. We could check if there's a PHY, and if so make
sure it supports WOL before setting those bits.

> > >  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)
> > > -		return -ENODEV;
> > > -
> > >  	if (!bp->ptp_info)
> > > -		return phy_mii_ioctl(phydev, rq, cmd);
> > > +		return phylink_mii_ioctl(bp->phylink, rq, cmd);
> > >  
> > >  	switch (cmd) {
> > >  	case SIOCSHWTSTAMP:
> > > @@ -3206,7 +3281,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->phylink, rq, cmd);
> > >  	}
> 
> This can probably be cleaned up - move
> 
> 	return phylink_mii_ioctl(bp->phylink, rq, cmd);
> 
> to the bottom, and just make the switch() conditional on bp->ptp_info
> being non-NULL.

I'll do this.

> > > @@ -4377,18 +4448,12 @@ static int macb_remove(struct platform_device *pdev)
> > >  {
> > >  	struct net_device *dev;
> > >  	struct macb *bp;
> > > -	struct device_node *np = pdev->dev.of_node;
> > >  
> > >  	dev = platform_get_drvdata(pdev);
> > >  
> > >  	if (dev) {
> > >  		bp = netdev_priv(dev);
> > > -		if (dev->phydev)
> > > -			phy_disconnect(dev->phydev);
> > >  		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);
> > >  
> > >  		unregister_netdev(dev);
> > > @@ -4403,7 +4468,6 @@ static int macb_remove(struct platform_device *pdev)
> > >  			clk_disable_unprepare(bp->tsu_clk);
> > >  			pm_runtime_set_suspended(&pdev->dev);
> > >  		}
> > > -		of_node_put(bp->phy_node);
> > >  		free_netdev(dev);
> > >  	}
> 
> Doesn't this need to cleanup phylink somewhere via a call to
> phylink_destroy() ?

You're right, I'll fix it.

> > > @@ -4421,7 +4485,6 @@ static int __maybe_unused macb_suspend(struct device *dev)
> > >  	if (!netif_running(netdev))
> > >  		return 0;
> > >  
> > > -
> > >  	if (bp->wol & MACB_WOL_ENABLED) {
> > >  		macb_writel(bp, IER, MACB_BIT(WOL));
> > >  		macb_writel(bp, WOL, MACB_BIT(MAG));
> > > @@ -4432,8 +4495,6 @@ 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);
> 
> So you don't need to shutdown the phylib state machine after all?
> 
> > >  		spin_lock_irqsave(&bp->lock, flags);
> > >  		macb_reset_hw(bp);
> > >  		spin_unlock_irqrestore(&bp->lock, flags);
> > > @@ -4481,12 +4542,8 @@ 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);
> 
> This looks like it was re-setting up the PHY, but nothing happens here
> after conversion?  Do we need to add something to phylink for this?
> Has this been tested?

I made some tests but after an internal discussion it seems like I can't
test the feature myself. Someone will, and we'll probably have to add a
suspend/resume logic to phylink.

Thanks for the review!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists