Cadence GEM provides a 102 bit time counter with 48 bits for seconds,
30 bits for nsecs and 24 bits for sub-nsecs to control 1588 timestamping.
This patch does the following:
- Registers to ptp clock framework
- Timer initialization is done by writing time of day to the timer counter.
- ns increment register is programmed as NSEC_PER_SEC/TSU_CLK.
For a 16 bit subns precision, the subns increment equals
remainder of (NS_PER_SEC/TSU_CLK) * (2^16).
- HW time stamp capabilities are advertised via ethtool and macb ioctl is
updated accordingly.
- Timestamps are obtained from the TX/RX PTP event/PEER registers.
The timestamp obtained thus is updated in skb for upper layers to access.
- The drivers register functions with ptp to perform time and frequency
adjustment.
- Time adjustment is done by writing to the 1558_ADJUST register.
The controller will read the delta in this register and update the timer
counter register. Alternatively, for large time offset adjustments,
the driver reads the secs and nsecs counter values, adds/subtracts the
delta and updates the timer counter.
- Frequency adjustment is not directly supported by this IP.
addend is the initial value ns increment and similarly addendesub.
The ppb (parts per billion) provided is used as
ns_incr = addend +/- (ppb/rate).
Similarly the remainder of the above is used to populate subns increment.
In case the ppb requested is negative AND subns adjustment greater than
the addendsub, ns_incr is reduced by 1 and subns_incr is adjusted in
positive accordingly.
Signed-off-by: Andrei Pistirica <[email protected]>
Signed-off-by: Harini Katakam <[email protected]>
---
Version 2 patch for: https://patchwork.kernel.org/patch/9310989/.
Modifications:
- bitfields for TSU are named according to SAMA5D2 data sheet
- identify GEM-PTP support based on platform capability
- add spinlock for TSU access
- change macb_ptp_adjfreq and use fewer 64bit divisions
drivers/net/ethernet/cadence/Kconfig | 10 +-
drivers/net/ethernet/cadence/Makefile | 8 +-
drivers/net/ethernet/cadence/macb.h | 80 +++++++++++
drivers/net/ethernet/cadence/macb_ptp.c | 229 ++++++++++++++++++++++++++++++++
4 files changed, 325 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/ethernet/cadence/macb_ptp.c
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index f0bcb15..ebbc65f 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -29,6 +29,14 @@ config MACB
support for the MACB/GEM chip.
To compile this driver as a module, choose M here: the module
- will be called macb.
+ will be called cadence-macb.
+
+config MACB_USE_HWSTAMP
+ bool "Use IEEE 1588 hwstamp"
+ depends on MACB
+ default y
+ select PTP_1588_CLOCK
+ ---help---
+ Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
endif # NET_CADENCE
diff --git a/drivers/net/ethernet/cadence/Makefile b/drivers/net/ethernet/cadence/Makefile
index 91f79b1..4402d42 100644
--- a/drivers/net/ethernet/cadence/Makefile
+++ b/drivers/net/ethernet/cadence/Makefile
@@ -2,4 +2,10 @@
# Makefile for the Atmel network device drivers.
#
-obj-$(CONFIG_MACB) += macb.o
+cadence-macb-y := macb.o
+
+ifeq ($(CONFIG_MACB_USE_HWSTAMP),y)
+cadence-macb-y += macb_ptp.o
+endif
+
+obj-$(CONFIG_MACB) += cadence-macb.o
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 3f385ab..2ee9af8 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,10 @@
#ifndef _MACB_H
#define _MACB_H
+#include <linux/net_tstamp.h>
+#include <linux/ptp_clock.h>
+#include <linux/ptp_clock_kernel.h>
+
#define MACB_GREGS_NBR 16
#define MACB_GREGS_VERSION 2
#define MACB_MAX_QUEUES 8
@@ -129,6 +133,20 @@
#define GEM_RXIPCCNT 0x01a8 /* IP header Checksum Error Counter */
#define GEM_RXTCPCCNT 0x01ac /* TCP Checksum Error Counter */
#define GEM_RXUDPCCNT 0x01b0 /* UDP Checksum Error Counter */
+#define GEM_TISUBN 0x01bc /* 1588 Timer Increment Sub-ns */
+#define GEM_TSH 0x01c0 /* 1588 Timer Seconds High */
+#define GEM_TSL 0x01d0 /* 1588 Timer Seconds Low */
+#define GEM_TN 0x01d4 /* 1588 Timer Nanoseconds */
+#define GEM_TA 0x01d8 /* 1588 Timer Adjust */
+#define GEM_TI 0x01dc /* 1588 Timer Increment */
+#define GEM_EFTSL 0x01e0 /* PTP Event Frame Tx Seconds Low */
+#define GEM_EFTN 0x01e4 /* PTP Event Frame Tx Nanoseconds */
+#define GEM_EFRSL 0x01e8 /* PTP Event Frame Rx Seconds Low */
+#define GEM_EFRN 0x01ec /* PTP Event Frame Rx Nanoseconds */
+#define GEM_PEFTSL 0x01f0 /* PTP Peer Event Frame Tx Secs Low */
+#define GEM_PEFTN 0x01f4 /* PTP Peer Event Frame Tx Ns */
+#define GEM_PEFRSL 0x01f8 /* PTP Peer Event Frame Rx Sec Low */
+#define GEM_PEFRN 0x01fc /* PTP Peer Event Frame Rx Ns */
#define GEM_DCFG1 0x0280 /* Design Config 1 */
#define GEM_DCFG2 0x0284 /* Design Config 2 */
#define GEM_DCFG3 0x0288 /* Design Config 3 */
@@ -171,6 +189,7 @@
#define MACB_NCR_TPF_SIZE 1
#define MACB_TZQ_OFFSET 12 /* Transmit zero quantum pause frame */
#define MACB_TZQ_SIZE 1
+#define MACB_SRTSM_OFFSET 15
/* Bitfields in NCFGR */
#define MACB_SPD_OFFSET 0 /* Speed */
@@ -312,6 +331,36 @@
#define MACB_PFR_SIZE 1
#define MACB_PTZ_OFFSET 13 /* Enable pause time zero interrupt */
#define MACB_PTZ_SIZE 1
+#define MACB_PFTR_OFFSET 14 /* Pause Frame Transmitted */
+#define MACB_PFTR_SIZE 1
+#define MACB_DRQFR_OFFSET 18 /* PTP Delay Request Frame Received */
+#define MACB_DRQFR_SIZE 1
+#define MACB_SFR_OFFSET 19 /* PTP Sync Frame Received */
+#define MACB_SFR_SIZE 1
+#define MACB_DRQFT_OFFSET 20 /* PTP Delay Request Frame Transmitted */
+#define MACB_DRQFT_SIZE 1
+#define MACB_SFT_OFFSET 21 /* PTP Sync Frame Transmitted */
+#define MACB_SFT_SIZE 1
+#define MACB_PDRQFR_OFFSET 22 /* PDelay Request Frame Received */
+#define MACB_PDRQFR_SIZE 1
+#define MACB_PDRSFR_OFFSET 23 /* PDelay Response Frame Received */
+#define MACB_PDRSFR_SIZE 1
+#define MACB_PDRQFT_OFFSET 24 /* PDelay Request Frame Transmitted */
+#define MACB_PDRQFT_SIZE 1
+#define MACB_PDRSFT_OFFSET 25 /* PDelay Response Frame Transmitted */
+#define MACB_PDRSFT_SIZE 1
+#define MACB_SRI_OFFSET 26 /* TSU Seconds Register Increment */
+#define MACB_SRI_SIZE 1
+#define MACB_WOL_OFFSET 28 /* Wake On LAN */
+#define MACB_WOL_SIZE 1
+
+/* Timer increment fields */
+#define MACB_TI_CNS_OFFSET 0
+#define MACB_TI_CNS_SIZE 8
+#define MACB_TI_ACNS_OFFSET 8
+#define MACB_TI_ACNS_SIZE 8
+#define MACB_TI_NIT_OFFSET 16
+#define MACB_TI_NIT_SIZE 8
/* Bitfields in MAN */
#define MACB_DATA_OFFSET 0 /* data */
@@ -375,6 +424,18 @@
#define GEM_TX_PKT_BUFF_OFFSET 21
#define GEM_TX_PKT_BUFF_SIZE 1
+/* Bitfields in TISUBN */
+#define GEM_SUBNSINCR_OFFSET 0
+#define GEM_SUBNSINCR_SIZE 16
+
+/* Bitfields in TI */
+#define GEM_NSINCR_OFFSET 0
+#define GEM_NSINCR_SIZE 8
+
+/* Bitfields in ADJ */
+#define GEM_ADDSUB_OFFSET 31
+#define GEM_ADDSUB_SIZE 1
+
/* Constants for CLK */
#define MACB_CLK_DIV8 0
#define MACB_CLK_DIV16 1
@@ -405,6 +466,7 @@
#define MACB_CAPS_SG_DISABLED 0x40000000
#define MACB_CAPS_MACB_IS_GEM 0x80000000
#define MACB_CAPS_JUMBO 0x00000010
+#define MACB_CAPS_GEM_HAS_PTP 0x00000020
/* Bit manipulation macros */
#define MACB_BIT(name) \
@@ -840,8 +902,26 @@ struct macb {
unsigned int rx_frm_len_mask;
unsigned int jumbo_max_len;
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ unsigned int hwts_tx_en;
+ unsigned int hwts_rx_en;
+ spinlock_t tsu_clk_lock;
+ unsigned int tsu_rate;
+
+ struct ptp_clock *ptp_clock;
+ struct ptp_clock_info ptp_caps;
+ unsigned int ns_incr;
+ unsigned int subns_incr;
+#endif
};
+#ifdef CONFIG_MACB_USE_HWSTAMP
+void macb_ptp_init(struct net_device *ndev);
+#else
+void macb_ptp_init(struct net_device *ndev) { }
+#endif
+
static inline bool macb_is_gem(struct macb *bp)
{
return !!(bp->caps & MACB_CAPS_MACB_IS_GEM);
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
new file mode 100644
index 0000000..81ce3a9
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,229 @@
+/*
+ * PTP 1588 clock for SAMA5D2 platform.
+ *
+ * Copyright (C) 2015 Xilinx Inc.
+ * Copyright (C) 2016 Microchip Technology
+ *
+ * Authors: Harini Katakam <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/platform_device.h>
+#include <linux/time64.h>
+#include <linux/ptp_classify.h>
+#include <linux/if_ether.h>
+#include <linux/if_vlan.h>
+
+#include "macb.h"
+
+#define GMAC_TIMER_NAME "gmac-gem-ptp"
+
+static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
+{
+ u64 sec, sech, secl;
+
+ spin_lock(&bp->tsu_clk_lock);
+
+ /* get GEM internal time */
+ sech = gem_readl(bp, TSH);
+ secl = gem_readl(bp, TSL);
+ ts->tv_nsec = gem_readl(bp, TN);
+ ts->tv_sec = (sech << 32) | secl;
+
+ /* minimize the error */
+ sech = gem_readl(bp, TSH);
+ secl = gem_readl(bp, TSL);
+ sec = (sech << 32) | secl;
+ if (ts->tv_sec != sec) {
+ ts->tv_sec = sec;
+ ts->tv_nsec = gem_readl(bp, TN);
+ }
+
+ spin_unlock(&bp->tsu_clk_lock);
+}
+
+static inline void macb_tsu_set_time(struct macb *bp,
+ const struct timespec64 *ts)
+{
+ u32 ns, sech, secl;
+ s64 word_mask = 0xffffffff;
+
+ sech = (u32)ts->tv_sec;
+ secl = (u32)ts->tv_sec;
+ ns = ts->tv_nsec;
+ if (ts->tv_sec > word_mask)
+ sech = (ts->tv_sec >> 32);
+
+ spin_lock(&bp->tsu_clk_lock);
+
+ /* TSH doesn't latch the time and no atomicity! */
+ gem_writel(bp, TSH, sech);
+ gem_writel(bp, TSL, secl);
+ gem_writel(bp, TN, ns);
+
+ spin_unlock(&bp->tsu_clk_lock);
+}
+
+static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+ struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+ u32 addend, addend_frac, rem;
+ u64 drift_tmr, diff, diff_frac = 0;
+ int neg_adj = 0;
+
+ if (ppb < 0) {
+ neg_adj = 1;
+ ppb = -ppb;
+ }
+
+ /* drift/period */
+ drift_tmr = (bp->ns_incr * ppb) +
+ ((bp->subns_incr * ppb) >> 16);
+
+ /* drift/cycle */
+ diff = div_u64_rem(drift_tmr, 1000000000ULL, &rem);
+
+ /* drift fraction/cycle, if necessary */
+ if (rem) {
+ u64 fraction = rem;
+ fraction = fraction << 16;
+
+ diff_frac = div_u64(fraction, 1000000000ULL);
+ }
+
+ /* adjustmets */
+ addend = neg_adj ? (bp->ns_incr - diff) : (bp->ns_incr + diff);
+ addend_frac = neg_adj ? (bp->subns_incr - diff_frac) :
+ (bp->subns_incr + diff_frac);
+
+ spin_lock(&bp->tsu_clk_lock);
+
+ gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, addend_frac));
+ gem_writel(bp, TI, GEM_BF(NSINCR, addend));
+
+ spin_unlock(&bp->tsu_clk_lock);
+ return 0;
+}
+
+static int macb_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+ struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+ struct timespec64 now, then = ns_to_timespec64(delta);
+ u32 adj, sign = 0;
+
+ if (delta < 0) {
+ delta = -delta;
+ sign = 1;
+ }
+
+ if (delta > 0x3FFFFFFF) {
+ macb_tsu_get_time(bp, &now);
+
+ if (sign)
+ now = timespec64_sub(now, then);
+ else
+ now = timespec64_add(now, then);
+
+ macb_tsu_set_time(bp, (const struct timespec64 *)&now);
+ } else {
+ adj = delta;
+ if (sign)
+ adj |= GEM_BIT(ADDSUB);
+
+ gem_writel(bp, TA, adj);
+ }
+
+ return 0;
+}
+
+static int macb_ptp_gettime(struct ptp_clock_info *ptp,
+ struct timespec64 *ts)
+{
+ struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+
+ macb_tsu_get_time(bp, ts);
+
+ return 0;
+}
+
+static int macb_ptp_settime(struct ptp_clock_info *ptp,
+ const struct timespec64 *ts)
+{
+ struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+
+ macb_tsu_set_time(bp, ts);
+
+ return 0;
+}
+
+static int macb_ptp_enable(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info macb_ptp_caps = {
+ .owner = THIS_MODULE,
+ .name = GMAC_TIMER_NAME,
+ .max_adj = 250000000,
+ .n_alarm = 0,
+ .n_ext_ts = 0,
+ .n_per_out = 0,
+ .n_pins = 0,
+ .pps = 0,
+ .adjfreq = macb_ptp_adjfreq,
+ .adjtime = macb_ptp_adjtime,
+ .gettime64 = macb_ptp_gettime,
+ .settime64 = macb_ptp_settime,
+ .enable = macb_ptp_enable,
+};
+
+void macb_ptp_init(struct net_device *ndev)
+{
+ struct macb *bp = netdev_priv(ndev);
+ struct timespec64 now;
+ u32 rem = 0;
+
+ if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
+ netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
+ return;
+ }
+
+ spin_lock_init(&bp->tsu_clk_lock);
+
+ bp->ptp_caps = macb_ptp_caps;
+ bp->tsu_rate = clk_get_rate(bp->pclk);
+
+ getnstimeofday64(&now);
+ macb_tsu_set_time(bp, (const struct timespec64 *)&now);
+
+ bp->ns_incr = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
+ if (rem) {
+ u64 adj = rem;
+ /* Multiply by 2^16 as subns register is 16 bits */
+ adj <<= 16;
+ bp->subns_incr = div_u64(adj, bp->tsu_rate);
+ } else {
+ bp->subns_incr = 0;
+ }
+
+ gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, bp->subns_incr));
+ gem_writel(bp, TI, GEM_BF(NSINCR, bp->ns_incr));
+ gem_writel(bp, TA, 0);
+
+ bp->ptp_clock = ptp_clock_register(&bp->ptp_caps, NULL);
+ if (IS_ERR(&bp->ptp_clock)) {
+ bp->ptp_clock = NULL;
+ pr_err("ptp clock register failed\n");
+ return;
+ }
+
+ dev_info(&bp->pdev->dev, "%s ptp clock registered.\n", GMAC_TIMER_NAME);
+}
+
--
1.9.1
Hardware time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Where timers are obtained from the PTP event/peer
registers.
Signed-off-by: Andrei Pistirica <[email protected]>
---
Version 2 patch for: https://patchwork.kernel.org/patch/9310991/
Modificaions:
- add PTP caps for SAMA5D2/3/4 platforms
- and cosmetic changes
drivers/net/ethernet/cadence/macb.c | 24 +++-
drivers/net/ethernet/cadence/macb.h | 13 ++
drivers/net/ethernet/cadence/macb_ptp.c | 222 ++++++++++++++++++++++++++++++++
3 files changed, 254 insertions(+), 5 deletions(-)
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);
+
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);
+
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);
+
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,
.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);
+ default:
+ return phy_mii_ioctl(phydev, rq, cmd);
+ }
}
static int macb_set_features(struct net_device *netdev,
@@ -2812,7 +2825,7 @@ static const struct macb_config pc302gem_config = {
};
static const struct macb_config sama5d2_config = {
- .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+ .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
@@ -2820,14 +2833,15 @@ static const struct macb_config sama5d2_config = {
static const struct macb_config sama5d3_config = {
.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE
- | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+ | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII
+ | MACB_CAPS_GEM_HAS_PTP,
.dma_burst_length = 16,
.clk_init = macb_clk_init,
.init = macb_init,
};
static const struct macb_config sama5d4_config = {
- .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+ .caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP,
.dma_burst_length = 4,
.clk_init = macb_clk_init,
.init = macb_init,
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 2ee9af8..3ac824a 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -918,8 +918,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 -EOPNOTSUPP; }
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr)
+ { return -EOPNOTSUPP; }
#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 81ce3a9..f476b55 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -227,3 +227,225 @@ 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. UDP packets must be parsed to identify PTP packets.
+ *
+ * Note: Inspired from drivers/net/ethernet/ti/cpts.c
+ */
+static int macb_get_ptp_peer(struct sk_buff *skb, int ptp_class)
+{
+ unsigned int offset = 0;
+ u8 *msgtype, *data = skb->data;
+
+ /* PTP frames are rare! */
+ if (likely(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 void macb_ptp_tx_hwtstamp(struct macb *bp, struct sk_buff *skb,
+ int peer_ev)
+{
+ 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));
+}
+
+/* no static since it is called from macb driver! */
+void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb)
+{
+ if (!bp->hwts_tx_en)
+ return;
+
+ if (unlikely(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 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);
+ }
+ ns = timespec64_to_ns(&ts);
+
+ memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
+ shhwtstamps->hwtstamp = ns_to_ktime(ns);
+}
+
+/* no static since it is called from macb driver! */
+void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb)
+{
+ int class;
+ int peer;
+
+ if (!bp->hwts_rx_en)
+ return;
+
+ __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);
+}
+
+int macb_ptp_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
+{
+ struct macb *bp = netdev_priv(dev);
+
+ ethtool_op_get_ts_info(dev, info);
+ info->so_timestamping =
+ SOF_TIMESTAMPING_TX_SOFTWARE |
+ SOF_TIMESTAMPING_RX_SOFTWARE |
+ SOF_TIMESTAMPING_SOFTWARE |
+ SOF_TIMESTAMPING_TX_HARDWARE |
+ SOF_TIMESTAMPING_RX_HARDWARE |
+ SOF_TIMESTAMPING_RAW_HARDWARE;
+ info->phc_index = -1;
+
+ if (bp->ptp_clock)
+ info->phc_index = ptp_clock_index(bp->ptp_clock);
+
+ return 0;
+}
+
+int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+ struct hwtstamp_config config;
+ struct macb *priv = netdev_priv(netdev);
+ u32 regval;
+
+ netdev_vdbg(netdev, "macb_hwtstamp_ioctl\n");
+
+ if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+ return -EFAULT;
+
+ /* reserved for future extensions */
+ if (config.flags)
+ return -EINVAL;
+
+ switch (config.tx_type) {
+ case HWTSTAMP_TX_OFF:
+ priv->hwts_tx_en = 0;
+ break;
+ case HWTSTAMP_TX_ON:
+ priv->hwts_tx_en = 1;
+ break;
+ default:
+ return -ERANGE;
+ }
+
+ switch (config.rx_filter) {
+ case HWTSTAMP_FILTER_NONE:
+ if (priv->hwts_rx_en)
+ priv->hwts_rx_en = 0;
+ break;
+ case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+ case HWTSTAMP_FILTER_ALL:
+ case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+ case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+ case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+ case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+ case HWTSTAMP_FILTER_PTP_V2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+ config.rx_filter = HWTSTAMP_FILTER_ALL;
+ regval = macb_readl(priv, NCR);
+ macb_writel(priv, NCR, (regval | MACB_BIT(SRTSM)));
+
+ if (!priv->hwts_rx_en)
+ priv->hwts_rx_en = 1;
+ break;
+ default:
+ return -ERANGE;
+ }
+
+ return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
+ -EFAULT : 0;
+}
+
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr)
+{
+ struct hwtstamp_config config;
+ struct macb *priv = netdev_priv(netdev);
+
+ config.flags = 0;
+ config.tx_type = priv->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+ config.rx_filter = (priv->hwts_rx_en ?
+ HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE);
+
+ return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
+ -EFAULT : 0;
+}
+
--
1.9.1
On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
> - Frequency adjustment is not directly supported by this IP.
This statement still makes no sense. Doesn't the following text...
> addend is the initial value ns increment and similarly addendesub.
> The ppb (parts per billion) provided is used as
> ns_incr = addend +/- (ppb/rate).
> Similarly the remainder of the above is used to populate subns increment.
describe how frequency adjustment is in fact supported?
> +config MACB_USE_HWSTAMP
> + bool "Use IEEE 1588 hwstamp"
> + depends on MACB
> + default y
> + select PTP_1588_CLOCK
This "select" pattern is going to be replaced with "imply" soon.
http://www.mail-archive.com/[email protected]/msg1269181.html
You should use the new "imply" key word and reference that series in
your change log.
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3f385ab..2ee9af8 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,10 @@
> #ifndef _MACB_H
> #define _MACB_H
>
> +#include <linux/net_tstamp.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/ptp_clock_kernel.h>
Don't need net_tstamp.h here. Move it into the .c file please.
> @@ -840,8 +902,26 @@ struct macb {
>
> unsigned int rx_frm_len_mask;
> unsigned int jumbo_max_len;
> +
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + unsigned int hwts_tx_en;
> + unsigned int hwts_rx_en;
These two can be bool'eans.
> + spinlock_t tsu_clk_lock;
> + unsigned int tsu_rate;
> +
> + struct ptp_clock *ptp_clock;
> + struct ptp_clock_info ptp_caps;
> + unsigned int ns_incr;
> + unsigned int subns_incr;
These two are 32 bit register values, right? Then use the u32 type.
> +#endif
> };
> +static inline void macb_tsu_set_time(struct macb *bp,
> + const struct timespec64 *ts)
> +{
> + u32 ns, sech, secl;
> + s64 word_mask = 0xffffffff;
> +
> + sech = (u32)ts->tv_sec;
> + secl = (u32)ts->tv_sec;
> + ns = ts->tv_nsec;
> + if (ts->tv_sec > word_mask)
> + sech = (ts->tv_sec >> 32);
> +
> + spin_lock(&bp->tsu_clk_lock);
> +
> + /* TSH doesn't latch the time and no atomicity! */
> + gem_writel(bp, TSH, sech);
> + gem_writel(bp, TSL, secl);
If TN overflows here then the clock will be off by one whole second!
Why not clear TN first?
> + gem_writel(bp, TN, ns);
> +
> + spin_unlock(&bp->tsu_clk_lock);
> +}
> +
> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> + struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> + u32 addend, addend_frac, rem;
> + u64 drift_tmr, diff, diff_frac = 0;
> + int neg_adj = 0;
> +
> + if (ppb < 0) {
> + neg_adj = 1;
> + ppb = -ppb;
> + }
> +
> + /* drift/period */
> + drift_tmr = (bp->ns_incr * ppb) +
> + ((bp->subns_incr * ppb) >> 16);
What? Why the 16 bit shift? Last time your said it was 24 bits.
> + /* drift/cycle */
> + diff = div_u64_rem(drift_tmr, 1000000000ULL, &rem);
> +
> + /* drift fraction/cycle, if necessary */
> + if (rem) {
> + u64 fraction = rem;
> + fraction = fraction << 16;
> +
> + diff_frac = div_u64(fraction, 1000000000ULL);
If you use a combined tuning word like I explained last time, then you
will not need a second division.
Also, please use the new adjfine() PHC method, as adjfreq() is now deprecated.
> + }
> +
> + /* adjustmets */
> + addend = neg_adj ? (bp->ns_incr - diff) : (bp->ns_incr + diff);
> + addend_frac = neg_adj ? (bp->subns_incr - diff_frac) :
> + (bp->subns_incr + diff_frac);
> +
> + spin_lock(&bp->tsu_clk_lock);
> +
> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, addend_frac));
> + gem_writel(bp, TI, GEM_BF(NSINCR, addend));
> +
> + spin_unlock(&bp->tsu_clk_lock);
> + return 0;
> +}
> +void macb_ptp_init(struct net_device *ndev)
> +{
> + struct macb *bp = netdev_priv(ndev);
> + struct timespec64 now;
> + u32 rem = 0;
> +
> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
> + return;
> + }
> +
> + spin_lock_init(&bp->tsu_clk_lock);
> +
> + bp->ptp_caps = macb_ptp_caps;
> + bp->tsu_rate = clk_get_rate(bp->pclk);
> +
> + getnstimeofday64(&now);
> + macb_tsu_set_time(bp, (const struct timespec64 *)&now);
> +
> + bp->ns_incr = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
> + if (rem) {
> + u64 adj = rem;
> + /* Multiply by 2^16 as subns register is 16 bits */
Last time you said:
> + /* Multiple by 2^24 as subns field is 24 bits */
> + adj <<= 16;
> + bp->subns_incr = div_u64(adj, bp->tsu_rate);
> + } else {
> + bp->subns_incr = 0;
> + }
> +
> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, bp->subns_incr));
> + gem_writel(bp, TI, GEM_BF(NSINCR, bp->ns_incr));
> + gem_writel(bp, TA, 0);
> +
> + bp->ptp_clock = ptp_clock_register(&bp->ptp_caps, NULL);
You call regsiter, but you never call unregister!
> + if (IS_ERR(&bp->ptp_clock)) {
> + bp->ptp_clock = NULL;
> + pr_err("ptp clock register failed\n");
> + return;
> + }
> +
> + dev_info(&bp->pdev->dev, "%s ptp clock registered.\n", GMAC_TIMER_NAME);
> +}
> +
> --
> 1.9.1
>
Thanks,
Richard
On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void macb_ptp_init(struct net_device *ndev);
> +#else
> +void macb_ptp_init(struct net_device *ndev) { }
static inline ^^^
> +#endif
> +void macb_ptp_init(struct net_device *ndev)
> +{
> + struct macb *bp = netdev_priv(ndev);
> + struct timespec64 now;
> + u32 rem = 0;
> +
> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
> + return;
> + }
You would have needed '&' and not '|' here.
Also, using a flag limits the code to your platform. This works for
you, but it is short sighted. The other MACB PTP blocks have
different register layouts, and this patch does not lay the ground
work for the others.
The driver needs to be designed to support the other platforms.
Thanks,
Richard
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
On 20.11.2016 20:18, Richard Cochran wrote:
> On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
>> - Frequency adjustment is not directly supported by this IP.
>
> This statement still makes no sense. Doesn't the following text...
This statement is inherited from original patch, and it refers to the
fact that it doesn't change directly the frequency, it changes the
increment value.
I'll remove it to avoid any confusion.
>
>> addend is the initial value ns increment and similarly addendesub.
>> The ppb (parts per billion) provided is used as
>> ns_incr = addend +/- (ppb/rate).
>> Similarly the remainder of the above is used to populate subns increment.
>
> describe how frequency adjustment is in fact supported?
Ack, I will clarify.
>
>> +config MACB_USE_HWSTAMP
>> + bool "Use IEEE 1588 hwstamp"
>> + depends on MACB
>> + default y
>> + select PTP_1588_CLOCK
>
> This "select" pattern is going to be replaced with "imply" soon.
>
> http://www.mail-archive.com/[email protected]/msg1269181.html
>
> You should use the new "imply" key word and reference that series in
> your change log.
Ack.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 3f385ab..2ee9af8 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -10,6 +10,10 @@
>> #ifndef _MACB_H
>> #define _MACB_H
>>
>> +#include <linux/net_tstamp.h>
>> +#include <linux/ptp_clock.h>
>> +#include <linux/ptp_clock_kernel.h>
>
> Don't need net_tstamp.h here. Move it into the .c file please.
Ack.
>
>> @@ -840,8 +902,26 @@ struct macb {
>>
>> unsigned int rx_frm_len_mask;
>> unsigned int jumbo_max_len;
>> +
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> + unsigned int hwts_tx_en;
>> + unsigned int hwts_rx_en;
>
> These two can be bool'eans.
Ack.
>
>> + spinlock_t tsu_clk_lock;
>> + unsigned int tsu_rate;
>> +
>> + struct ptp_clock *ptp_clock;
>> + struct ptp_clock_info ptp_caps;
>> + unsigned int ns_incr;
>> + unsigned int subns_incr;
>
> These two are 32 bit register values, right? Then use the u32 type.
Yes. I will make the change.
>
>> +#endif
>> };
>
>> +static inline void macb_tsu_set_time(struct macb *bp,
>> + const struct timespec64 *ts)
>> +{
>> + u32 ns, sech, secl;
>> + s64 word_mask = 0xffffffff;
>> +
>> + sech = (u32)ts->tv_sec;
>> + secl = (u32)ts->tv_sec;
>> + ns = ts->tv_nsec;
>> + if (ts->tv_sec > word_mask)
>> + sech = (ts->tv_sec >> 32);
>> +
>> + spin_lock(&bp->tsu_clk_lock);
>> +
>> + /* TSH doesn't latch the time and no atomicity! */
>> + gem_writel(bp, TSH, sech);
>> + gem_writel(bp, TSL, secl);
>
> If TN overflows here then the clock will be off by one whole second!
> Why not clear TN first?
Ack.
>
>> + gem_writel(bp, TN, ns);
>> +
>> + spin_unlock(&bp->tsu_clk_lock);
>> +}
>> +
>> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>> +{
>> + struct macb *bp = container_of(ptp, struct macb, ptp_caps);
>> + u32 addend, addend_frac, rem;
>> + u64 drift_tmr, diff, diff_frac = 0;
>> + int neg_adj = 0;
>> +
>> + if (ppb < 0) {
>> + neg_adj = 1;
>> + ppb = -ppb;
>> + }
>> +
>> + /* drift/period */
>> + drift_tmr = (bp->ns_incr * ppb) +
>> + ((bp->subns_incr * ppb) >> 16);
>
> What? Why the 16 bit shift? Last time your said it was 24 bits.
SAMA5D2/3/4 uses GEM-PTP version (16bit), while Harini wrote a driver
for GEM-GXL (24bit). Probably will be a patch on top of this one for
GXL. This confusion was because I tried to keep the original patch
unchanged.
>
>> + /* drift/cycle */
>> + diff = div_u64_rem(drift_tmr, 1000000000ULL, &rem);
>> +
>> + /* drift fraction/cycle, if necessary */
>> + if (rem) {
>> + u64 fraction = rem;
>> + fraction = fraction << 16;
>> +
>> + diff_frac = div_u64(fraction, 1000000000ULL);
>
> If you use a combined tuning word like I explained last time, then you
> will not need a second division.
From what I understand, your suggestion is:
(ns | frac) * ppb = (total_ns | total_frac)
(total_ns | total_frac) / 10^9 = (adj_ns | adj_frac)
This is correct iff total_ns/10^9 >= 1, but the problem is that there
are missed fractions due to the following approximation:
frac*ppb =~
(ns*ppb+frac*ppb*2^16)*2^16-10^9*2^16*flor(ns*ppb+frac*ppb*2^16, 10^9).
An example which uses values from a real test:
let ppb=4891, ns=12 and frac=3158
- using suggested algorithm, yields: adj_ns = 0 and adj_frac = 0
- using in-place algorithm, yields: adj_ns = 0, adj_frac = 4
You can check the calculus.
Therefore, I would like to keep the in-place algorithm.
>
> Also, please use the new adjfine() PHC method, as adjfreq() is now deprecated.
Yes, I will port the patches on net-next and use adjfine.
>
>> + }
>> +
>> + /* adjustmets */
>> + addend = neg_adj ? (bp->ns_incr - diff) : (bp->ns_incr + diff);
>> + addend_frac = neg_adj ? (bp->subns_incr - diff_frac) :
>> + (bp->subns_incr + diff_frac);
>> +
>> + spin_lock(&bp->tsu_clk_lock);
>> +
>> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, addend_frac));
>> + gem_writel(bp, TI, GEM_BF(NSINCR, addend));
>> +
>> + spin_unlock(&bp->tsu_clk_lock);
>> + return 0;
>> +}
>
>> +void macb_ptp_init(struct net_device *ndev)
>> +{
>> + struct macb *bp = netdev_priv(ndev);
>> + struct timespec64 now;
>> + u32 rem = 0;
>> +
>> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
>> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
>> + return;
>> + }
>> +
>> + spin_lock_init(&bp->tsu_clk_lock);
>> +
>> + bp->ptp_caps = macb_ptp_caps;
>> + bp->tsu_rate = clk_get_rate(bp->pclk);
>> +
>> + getnstimeofday64(&now);
>> + macb_tsu_set_time(bp, (const struct timespec64 *)&now);
>> +
>> + bp->ns_incr = div_u64_rem(NSEC_PER_SEC, bp->tsu_rate, &rem);
>> + if (rem) {
>> + u64 adj = rem;
>> + /* Multiply by 2^16 as subns register is 16 bits */
>
> Last time you said:
>> + /* Multiple by 2^24 as subns field is 24 bits */
>
>> + adj <<= 16;
>> + bp->subns_incr = div_u64(adj, bp->tsu_rate);
>> + } else {
>> + bp->subns_incr = 0;
>> + }
>> +
>> + gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, bp->subns_incr));
>> + gem_writel(bp, TI, GEM_BF(NSINCR, bp->ns_incr));
>> + gem_writel(bp, TA, 0);
>> +
>> + bp->ptp_clock = ptp_clock_register(&bp->ptp_caps, NULL);
>
> You call regsiter, but you never call unregister!
Ack. I did a stupid mistake... sorry.
>
>> + if (IS_ERR(&bp->ptp_clock)) {
>> + bp->ptp_clock = NULL;
>> + pr_err("ptp clock register failed\n");
>> + return;
>> + }
>> +
>> + dev_info(&bp->pdev->dev, "%s ptp clock registered.\n", GMAC_TIMER_NAME);
>> +}
>> +
>> --
>> 1.9.1
>>
>
> Thanks,
> Richard
>
Regards,
Andrei
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
On 20.11.2016 20:37, Richard Cochran wrote:
> On Fri, Nov 18, 2016 at 03:21:51PM +0100, Andrei Pistirica wrote:
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void macb_ptp_init(struct net_device *ndev);
>> +#else
>> +void macb_ptp_init(struct net_device *ndev) { }
>
> static inline ^^^
I can do static inline only when PTP is not enabled (on else branch),
thus the empty function is defined in the header file (since the init
function is defined in macb_ptp and used in macb). To differentiate
between macb versions, I'll add a wrapper.
Would this be ok?
>
>> +#endif
>
>
>> +void macb_ptp_init(struct net_device *ndev)
>> +{
>> + struct macb *bp = netdev_priv(ndev);
>> + struct timespec64 now;
>> + u32 rem = 0;
>> +
>> + if (!(bp->caps | MACB_CAPS_GEM_HAS_PTP)){
>> + netdev_vdbg(bp->dev, "Platform does not support PTP!\n");
>> + return;
>> + }
>
> You would have needed '&' and not '|' here.
Yes. Another stupid mistake... sorry. I will be more careful next time.
>
> Also, using a flag limits the code to your platform. This works for
> you, but it is short sighted. The other MACB PTP blocks have
> different register layouts, and this patch does not lay the ground
> work for the others.
>
> The driver needs to be designed to support the other platforms.
It will support Xilinx.
>
> Thanks,
> Richard
>
Regards,
Andrei
On Wed, Nov 23, 2016 at 02:34:03PM +0100, Andrei Pistirica wrote:
> From what I understand, your suggestion is:
> (ns | frac) * ppb = (total_ns | total_frac)
> (total_ns | total_frac) / 10^9 = (adj_ns | adj_frac)
> This is correct iff total_ns/10^9 >= 1, but the problem is that there are
> missed fractions due to the following approximation:
> frac*ppb =~ (ns*ppb+frac*ppb*2^16)*2^16-10^9*2^16*flor(ns*ppb+frac*ppb*2^16,
> 10^9).
-ENOPARSE;
> An example which uses values from a real test:
> let ppb=4891, ns=12 and frac=3158
That is a very strange example for nominal frequency. The clock
period is 12.048187255859375 nanoseconds, and so the frequency is
83000037.99 Hz.
But hey, let's go with it...
> - using suggested algorithm, yields: adj_ns = 0 and adj_frac = 0
> - using in-place algorithm, yields: adj_ns = 0, adj_frac = 4
> You can check the calculus.
The test program, below, shows you what I meant. (Of course, you
should adjust this to fit the adjfine() method.)
Unfortunately, this device has a very coarse frequency resolution.
Using a nominal period of ns=12 as an example, the resolution is
2^-16 / 12 or 1.27 ppm. The 24 bit device is much better in this
repect.
The output using your example numbers is:
$ ./a.out 12 3158 4891
ns=12 frac=3158
ns=12 frac=3162
$ ./a.out 12 3158 -4891
ns=12 frac=3158
ns=12 frac=3154
See how you get a result of +/- 4 with just one division?
Thanks,
Richard
---
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
static void adjfreq(uint32_t ns, uint32_t frac, int32_t ppb)
{
uint64_t adj;
uint32_t diff, word;
int neg_adj = 0;
printf("ns=%u frac=%u\n", ns, frac);
if (ppb < 0) {
neg_adj = 1;
ppb = -ppb;
}
word = (ns << 16) + frac;
adj = word;
adj *= ppb;
adj += 500000000UL;
diff = adj / 1000000000UL;
word = neg_adj ? word - diff : word + diff;
printf("ns=%u frac=%u\n", word >> 16, word & 0xffff);
}
int main(int argc, char *argv[])
{
uint32_t ns, frac;
int32_t ppb;
if (argc != 4) {
puts("need ns, frac, and ppb");
return -1;
}
ns = atoi(argv[1]);
frac = atoi(argv[2]);
ppb = atoi(argv[3]);
adjfreq(ns, frac, ppb);
return 0;
}
> -----Original Message-----
> From: Richard Cochran [mailto:[email protected]]
> Sent: Wednesday, November 23, 2016 11:03 PM
> To: Andrei Pistirica - M16132
> Cc: [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC PATCH v2 1/2] macb: Add 1588 support in Cadence GEM.
>
> On Wed, Nov 23, 2016 at 02:34:03PM +0100, Andrei Pistirica wrote:
> > From what I understand, your suggestion is:
> > (ns | frac) * ppb = (total_ns | total_frac) (total_ns | total_frac) /
> > 10^9 = (adj_ns | adj_frac) This is correct iff total_ns/10^9 >= 1, but
> > the problem is that there are missed fractions due to the following
> > approximation:
> > frac*ppb =~
> > (ns*ppb+frac*ppb*2^16)*2^16-10^9*2^16*flor(ns*ppb+frac*ppb*2^16,
> > 10^9).
>
> -ENOPARSE;
>
> > An example which uses values from a real test:
> > let ppb=4891, ns=12 and frac=3158
>
> That is a very strange example for nominal frequency. The clock period is
> 12.048187255859375 nanoseconds, and so the frequency is
> 83000037.99 Hz.
>
> But hey, let's go with it...
>
> > - using suggested algorithm, yields: adj_ns = 0 and adj_frac = 0
> > - using in-place algorithm, yields: adj_ns = 0, adj_frac = 4 You can
> > check the calculus.
>
> The test program, below, shows you what I meant. (Of course, you should
> adjust this to fit the adjfine() method.)
>
> Unfortunately, this device has a very coarse frequency resolution.
> Using a nominal period of ns=12 as an example, the resolution is
> 2^-16 / 12 or 1.27 ppm. The 24 bit device is much better in this repect.
>
> The output using your example numbers is:
>
> $ ./a.out 12 3158 4891
> ns=12 frac=3158
> ns=12 frac=3162
>
> $ ./a.out 12 3158 -4891
> ns=12 frac=3158
> ns=12 frac=3154
>
> See how you get a result of +/- 4 with just one division?
>
> Thanks,
> Richard
>
> ---
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> static void adjfreq(uint32_t ns, uint32_t frac, int32_t ppb) {
> uint64_t adj;
> uint32_t diff, word;
> int neg_adj = 0;
>
> printf("ns=%u frac=%u\n", ns, frac);
>
> if (ppb < 0) {
> neg_adj = 1;
> ppb = -ppb;
> }
> word = (ns << 16) + frac;
> adj = word;
> adj *= ppb;
> adj += 500000000UL;
> diff = adj / 1000000000UL;
>
> word = neg_adj ? word - diff : word + diff;
> printf("ns=%u frac=%u\n", word >> 16, word & 0xffff); }
>
> int main(int argc, char *argv[])
> {
> uint32_t ns, frac;
> int32_t ppb;
>
> if (argc != 4) {
> puts("need ns, frac, and ppb");
> return -1;
> }
> ns = atoi(argv[1]);
> frac = atoi(argv[2]);
> ppb = atoi(argv[3]);
> adjfreq(ns, frac, ppb);
> return 0;
> }
Ok, thanks.
I will use this one then.
Regards,
Andrei