Re: [RFC PATCH v2 2/2] macb: Enable 1588 support in SAMA5D2 platform.

[Richard Cochran]

On Fri, Nov 18, 2016 at 03:21:52PM +0100, Andrei Pistirica wrote:
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index d975882..eb66b76 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -697,6 +697,8 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>  
>  			/* First, update TX stats if needed */
>  			if (skb) {
> +				macb_ptp_do_txstamp(bp, skb);

This is in the hot path, and so you should have an inline wrapper that
tests (bp->hwts_tx_en) and THEN calls into macb_ptp.c

In case PTP isn't configured, then the inline wrapper should be empty.

>  				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
>  					    macb_tx_ring_wrap(tail), skb->data);
>  				bp->stats.tx_packets++;
> @@ -853,6 +855,8 @@ static int gem_rx(struct macb *bp, int budget)
>  		    GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
>  			skb->ip_summed = CHECKSUM_UNNECESSARY;
>  
> +		macb_ptp_do_rxstamp(bp, skb);

Same here.

>  		bp->stats.rx_packets++;
>  		bp->stats.rx_bytes += skb->len;
>  
> @@ -1946,6 +1950,8 @@ static int macb_open(struct net_device *dev)
>  
>  	netif_tx_start_all_queues(dev);
>  
> +	macb_ptp_init(dev);

This leaks PHC instances starting the second time that the interface goes up!

>  	return 0;
>  }
>  
> @@ -2204,7 +2210,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
>  	.get_regs_len		= macb_get_regs_len,
>  	.get_regs		= macb_get_regs,
>  	.get_link		= ethtool_op_get_link,
> -	.get_ts_info		= ethtool_op_get_ts_info,
> +	.get_ts_info		= macb_get_ts_info,

You enable the time stamping logic unconditionally here ...

>  	.get_ethtool_stats	= gem_get_ethtool_stats,
>  	.get_strings		= gem_get_ethtool_strings,
>  	.get_sset_count		= gem_get_sset_count,
> @@ -2221,7 +2227,14 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	if (!phydev)
>  		return -ENODEV;
>  
> -	return phy_mii_ioctl(phydev, rq, cmd);
> +	switch (cmd) {
> +	case SIOCSHWTSTAMP:
> +		return macb_hwtst_set(dev, rq, cmd);
> +	case SIOCGHWTSTAMP:
> +		return macb_hwtst_get(dev, rq);

and here.

Is that logic always available on every MACB device?  If so, is the
implementation also identical?

Thanks,
Richard