From mboxrd@z Thu Jan 1 00:00:00 1970 From: andrei.pistirica@microchip.com (Andrei Pistirica) Date: Fri, 9 Sep 2016 16:08:24 +0200 Subject: [RFC PATCH 2/2] macb: Enable 1588 support in SAMA5D2 platform. In-Reply-To: <20160906163718.GB7012@localhost.localdomain> References: <1472820817-21874-1-git-send-email-andrei.pistirica@microchip.com> <1472820817-21874-2-git-send-email-andrei.pistirica@microchip.com> <20160906163718.GB7012@localhost.localdomain> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Richard, I will take your indications into account in next version of the patch. Regards, Andrei On 06.09.2016 18:37, Richard Cochran wrote: > On Fri, Sep 02, 2016 at 02:53:37PM +0200, Andrei Pistirica wrote: >> Hardware time stamp on the PTP Ethernet packets are received using the >> SO_TIMESTAMPING API. Timers are obtained from the PTP event/peer >> gem registers. >> >> Signed-off-by: Andrei Pistirica >> --- >> Integration with SAMA5D2 only. This feature wasn't tested on any >> other platform that might use cadence/gem. > > What does that mean? I didn't see any references to SAMA5D2 anywhere > in your patch. > > The driver needs to positively identify the HW that supports this > feature. > >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >> index 8d54e7b..18f0715 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -697,6 +697,11 @@ static void macb_tx_interrupt(struct macb_queue *queue) >> >> /* First, update TX stats if needed */ >> if (skb) { >> +/* guard the hot-path */ >> +#ifdef CONFIG_MACB_USE_HWSTAMP >> + if (bp->hwts_tx_en) >> + macb_ptp_do_txstamp(bp, skb); >> +#endif > > Pull the test into the helper function, and then you can drop the > ifdef and the funny comment. > >> netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n", >> macb_tx_ring_wrap(tail), skb->data); >> bp->stats.tx_packets++; >> @@ -853,6 +858,11 @@ static int gem_rx(struct macb *bp, int budget) >> GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK) >> skb->ip_summed = CHECKSUM_UNNECESSARY; >> >> +/* guard the hot-path */ >> +#ifdef CONFIG_MACB_USE_HWSTAMP >> + if (bp->hwts_rx_en) >> + macb_ptp_do_rxstamp(bp, skb); >> +#endif > > Same here. > >> bp->stats.rx_packets++; >> bp->stats.rx_bytes += skb->len; >> >> @@ -1723,6 +1733,11 @@ static void macb_init_hw(struct macb *bp) >> >> /* Enable TX and RX */ >> macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE)); >> + >> +#ifdef CONFIG_MACB_USE_HWSTAMP >> + bp->hwts_tx_en = 0; >> + bp->hwts_rx_en = 0; >> +#endif > > We don't initialize to zero unless we have to. > >> } >> >> /* >> @@ -1885,6 +1900,8 @@ static int macb_open(struct net_device *dev) >> >> netif_tx_start_all_queues(dev); >> >> + macb_ptp_init(dev); >> + >> return 0; >> } >> >> @@ -2143,7 +2160,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, >> .get_ethtool_stats = gem_get_ethtool_stats, >> .get_strings = gem_get_ethtool_strings, >> .get_sset_count = gem_get_sset_count, >> @@ -2157,6 +2174,12 @@ static int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) >> if (!netif_running(dev)) >> return -EINVAL; >> >> + if (cmd == SIOCSHWTSTAMP) >> + return macb_hwtst_set(dev, rq, cmd); >> + >> + if (cmd == SIOCGHWTSTAMP) >> + return macb_hwtst_get(dev, rq); > > switch/case? > >> + >> if (!phydev) >> return -ENODEV; >> >> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h >> index 8c3779d..555316a 100644 >> --- a/drivers/net/ethernet/cadence/macb.h >> +++ b/drivers/net/ethernet/cadence/macb.h >> @@ -920,8 +920,21 @@ struct macb { >> >> #ifdef CONFIG_MACB_USE_HWSTAMP >> void macb_ptp_init(struct net_device *ndev); >> +void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb); >> +void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb); >> +int macb_ptp_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info); >> +#define macb_get_ts_info macb_ptp_get_ts_info >> +int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd); >> +int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr); >> #else >> void macb_ptp_init(struct net_device *ndev) { } >> +void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) { } >> +void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) { } >> +#define macb_get_ts_info ethtool_op_get_ts_info >> +int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd) >> + { return -1; } >> +int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr) >> + { return -1; } > > Use a proper return code please. > >> #endif >> >> static inline bool macb_is_gem(struct macb *bp) >> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c >> index 6d6a6ec..e3f784a 100644 >> --- a/drivers/net/ethernet/cadence/macb_ptp.c >> +++ b/drivers/net/ethernet/cadence/macb_ptp.c >> @@ -5,6 +5,7 @@ >> * Copyright (C) 2016 Microchip Technology >> * >> * Authors: Harini Katakam >> + * Andrei Pistirica > > We don't add additional authors any more, because git tells us that info. > >> * >> * This file is licensed under the terms of the GNU General Public >> * License version 2. This program is licensed "as is" without any >> @@ -222,3 +223,219 @@ void macb_ptp_init(struct net_device *ndev) >> >> dev_info(&bp->pdev->dev, "%s ptp clock registered.\n", GMAC_TIMER_NAME); >> } >> + >> +/* While the GEM can timestamp PTP packets, it does not mark the RX descriptor >> + * to identify them. This is entirely the wrong place to be parsing UDP >> + * headers, but some minimal effort must be made. > > If you must parse, then it isn't the wrong place. > >> + * >> + * Note: Inspired from drivers/net/ethernet/ti/cpts.c >> + */ >> +static inline int macb_get_ptp_peer(struct sk_buff *skb, int ptp_class) > > Leave off inline, let the compiler decide. > >> +{ >> + unsigned int offset = 0; >> + u8 *msgtype, *data = skb->data; >> + >> + if (ptp_class == PTP_CLASS_NONE) >> + return -1; >> + >> + if (ptp_class & PTP_CLASS_VLAN) >> + offset += VLAN_HLEN; >> + >> + switch (ptp_class & PTP_CLASS_PMASK) { >> + case PTP_CLASS_IPV4: >> + offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN; >> + break; >> + case PTP_CLASS_IPV6: >> + offset += ETH_HLEN + IP6_HLEN + UDP_HLEN; >> + break; >> + case PTP_CLASS_L2: >> + offset += ETH_HLEN; >> + break; >> + >> + /* something went wrong! */ >> + default: >> + return -1; >> + } >> + >> + if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID) >> + return -1; >> + >> + if (unlikely(ptp_class & PTP_CLASS_V1)) >> + msgtype = data + offset + OFF_PTP_CONTROL; >> + else >> + msgtype = data + offset; >> + >> + return (*msgtype) & 0x2; >> +} >> + >> +static inline void macb_ptp_tx_hwtstamp(struct macb *bp, struct sk_buff *skb, >> + int peer_ev) > > no 'inline' please > >> +{ >> + struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb); >> + struct timespec64 ts; >> + u64 ns; >> + >> + /* PTP Peer Event Frame packets */ >> + if (peer_ev) { >> + ts.tv_sec = gem_readl(bp, PEFTSL); >> + ts.tv_nsec = gem_readl(bp, PEFTN); >> + >> + /* PTP Event Frame packets */ >> + } else { >> + ts.tv_sec = gem_readl(bp, EFTSL); >> + ts.tv_nsec = gem_readl(bp, EFTN); >> + } >> + ns = timespec64_to_ns(&ts); >> + >> + memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps)); >> + shhwtstamps->hwtstamp = ns_to_ktime(ns); >> + skb_tstamp_tx(skb, skb_hwtstamps(skb)); >> +} >> + >> +inline void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) > > s/inline/static/ > >> +{ >> + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) { >> + int class = ptp_classify_raw(skb); >> + int peer; >> + >> + peer = macb_get_ptp_peer(skb, class); >> + if (peer < 0) >> + return; >> + >> + /* Timestamp this packet */ >> + macb_ptp_tx_hwtstamp(bp, skb, peer); >> + } >> +} >> + >> +static inline void macb_ptp_rx_hwtstamp(struct macb *bp, struct sk_buff *skb, >> + int peer_ev) >> +{ >> + struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb); >> + struct timespec64 ts; >> + u64 ns; >> + >> + if (peer_ev) { >> + /* PTP Peer Event Frame packets */ >> + ts.tv_sec = gem_readl(bp, PEFRSL); >> + ts.tv_nsec = gem_readl(bp, PEFRN); >> + } else { >> + /* PTP Event Frame packets */ >> + ts.tv_sec = gem_readl(bp, EFRSL); >> + ts.tv_nsec = gem_readl(bp, EFRN); >> + } > > So you say the HW provides no matching information? Then it is really > poor. I surely don't want to let this out into the wild. I'll get > questions on the linuxptp list, like "PTP on mainline Linux is > mysteriously broken!" > >> + ns = timespec64_to_ns(&ts); >> + >> + memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps)); >> + shhwtstamps->hwtstamp = ns_to_ktime(ns); >> +} >> + >> +inline void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) >> +{ >> + int class; >> + int peer; >> + >> + /* ffs !!! */ > > What is this comment for? > >> + __skb_push(skb, ETH_HLEN); >> + class = ptp_classify_raw(skb); >> + __skb_pull(skb, ETH_HLEN); >> + >> + peer = macb_get_ptp_peer(skb, class); >> + if (peer < 0) >> + return; >> + >> + macb_ptp_rx_hwtstamp(bp, skb, peer); >> +} > > Thanks, > Richard >