Subject: Re: [RFC PATCH v2 2/2] macb: Enable 1588 support in SAMA5D2 platform.
To: Richard Cochran <richardcochran@gmail.com>
References: <1479478912-14067-1-git-send-email-andrei.pistirica@microchip.com>
 <1479478912-14067-2-git-send-email-andrei.pistirica@microchip.com>
 <20161120195413.GB7874@localhost.localdomain>
CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <davem@davemloft.net>,
        <nicolas.ferre@atmel.com>, <harinikatakamlinux@gmail.com>,
        <harini.katakam@xilinx.com>, <punnaia@xilinx.com>,
        <michals@xilinx.com>, <anirudh@xilinx.com>,
        <boris.brezillon@free-electrons.com>,
        <alexandre.belloni@free-electrons.com>, <tbultel@pixelsurmer.com>
From: Andrei Pistirica <andrei.pistirica@microchip.com>
Message-ID: <0717c12e-fd91-db48-eb7b-997695fa39dc@microchip.com>
Date: Wed, 23 Nov 2016 14:35:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20161120195413.GB7874@localhost.localdomain>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2621
Lines: 91


On 20.11.2016 20:54, Richard Cochran wrote:
> 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

Ack.

>
> 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!

Yes, I will call unregister at interface down.

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

I will add a wrapper to test if macb is gem and if it has PTP 
capability, otherwise call ethtool_op_get_ts_info.

>
>>  	.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?

As before, I will add a wrapper and the related tests.

>
> Thanks,
> Richard
>

Regards,
Andrei