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

[Andrei Pistirica]

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