Messages in this thread | | | Subject | Re: [RFC PATCH v2 2/2] macb: Enable 1588 support in SAMA5D2 platform. | From | Andrei Pistirica <> | Date | Wed, 23 Nov 2016 14:35:14 +0100 |
| |
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
| |