[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Andrei Pistirica]

From: Harini Katakam <harinik@xilinx.com>

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 24 bit subns precision, the subns increment equals
  remainder of (NS_PER_SEC/TSU_CLK) * (2^24).
- 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: Harini Katakam <harinik@xilinx.com>
Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
---
This patch is based on original Harini's patch, implemented in a
separate file to ease the review/maintanance and integration with
other platforms (e.g. Zynq Ultrascale+ MPSoC).
Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
project and also with ptpd2 version 2.3.1. PTP was tested over
IPv4,IPv6 and 802.3 protocols.

Hariani, please let me know if you are ok with this patch, and if you
want to be stated as author/signed-off?

In case that macb is compiled as a module, it has been renamed to
cadence-macb.ko to avoid naming confusion in Makefile.

Patch is not completely ported to the very latest version of net-next,
and it will be after review.

 drivers/net/ethernet/cadence/Kconfig    |  10 +-
 drivers/net/ethernet/cadence/Makefile   |   8 +-
 drivers/net/ethernet/cadence/macb.h     |  82 ++++++++++++
 drivers/net/ethernet/cadence/macb_ptp.c | 224 ++++++++++++++++++++++++++++++++
 4 files changed, 322 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..8c3779d 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,12 @@
 #ifndef _MACB_H
 #define _MACB_H
 
+#include <linux/net_tstamp.h>
+#include <linux/ptp_clock.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/skbuff.h>
+#include <linux/timecounter.h>
+
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
 #define MACB_MAX_QUEUES 8
@@ -129,6 +135,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 +191,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 +333,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 +426,20 @@
 #define GEM_TX_PKT_BUFF_OFFSET			21
 #define GEM_TX_PKT_BUFF_SIZE			1
 
+/* Bitfields in 1588INCRSUBNS */
+#define GEM_SUBNSINCL_SHFT			24
+#define GEM_SUBNSINCL_MASK			0xFF
+#define GEM_SUBNSINCH_SHFT			8
+#define GEM_SUBNSINCH_MASK			0xFFFF00
+
+/* Bitfields in 1588INCRNS */
+#define GEM_NSINCR_OFFSET			0
+#define GEM_NSINCR_SIZE				8
+
+/* Bitfields in 1588ADJ */
+#define GEM_ADDSUB_OFFSET			31
+#define GEM_ADDSUB_SIZE				1
+
 /* Constants for CLK */
 #define MACB_CLK_DIV8				0
 #define MACB_CLK_DIV16				1
@@ -840,8 +905,25 @@ 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;
+	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..6d6a6ec
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,224 @@
+/*
+ * PTP 1588 clock for SAMA5D2 platform.
+ *
+ * Copyright (C) 2015 Xilinx Inc.
+ * Copyright (C) 2016 Microchip Technology
+ *
+ * Authors: Harini Katakam <harinik@xilinx.com>
+ *
+ * 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-ptp"
+
+static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
+{
+	u64 sech, secl;
+
+	/* get GEM internal time */
+	sech = gem_readl(bp, TSH);
+	secl = gem_readl(bp, TSL);
+
+	ts->tv_sec = (sech << 32) | secl;
+	ts->tv_nsec = gem_readl(bp, TN);
+}
+
+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);
+
+	/* set GEM internal time */
+	gem_writel(bp, TSH, sech);
+	gem_writel(bp, TSL, secl);
+	gem_writel(bp, TN, ns);
+}
+
+static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+	unsigned long rate = bp->tsu_rate;
+	u64 adjsub;
+	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
+	bool neg_adj = false;
+
+	if (ppb < 0) {
+		neg_adj = true;
+		ppb = -ppb;
+	}
+
+	addend = bp->ns_incr;
+	addendsub = bp->subns_incr;
+
+	diff = div_u64_rem(ppb, rate, &rem);
+	addend = neg_adj ? addend - diff : addend + diff;
+
+	if (rem) {
+		adjsub = rem;
+		/* Multiple by 2^24 as subns field is 24 bits */
+		adjsub = adjsub << 24;
+
+		diffsub = div_u64(adjsub, rate);
+	} else {
+		diffsub = 0;
+	}
+
+	if (neg_adj && (diffsub > addendsub)) {
+		addend -= 1;
+		rem = (NSEC_PER_SEC - rem);
+		neg_adj = false;
+
+		adjsub = rem;
+		adjsub = adjsub << 24;
+		diffsub = div_u64(adjsub, rate);
+	}
+
+	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
+	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
+	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
+		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
+
+	gem_writel(bp, TISUBN, subnsreg);
+	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
+
+	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 subnsreg, rem = 0;
+	u64 adj;
+
+	bp->ptp_caps = macb_ptp_caps;
+	bp->tsu_rate = clk_get_rate(bp->pclk);
+
+	gem_writel(bp, TSH, 0);
+	gem_writel(bp, TSL, 0);
+	gem_writel(bp, TN, 0);
+	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) {
+		adj = rem;
+		/* Multiply by 2^24 as subns register is 24 bits */
+		adj = adj << 24;
+
+		bp->subns_incr = div_u64(adj, bp->tsu_rate);
+	} else {
+		bp->subns_incr = 0;
+	}
+
+	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
+	subnsreg = ((bp->subns_incr & GEM_SUBNSINCL_MASK)
+		    << GEM_SUBNSINCL_SHFT) |
+		   ((bp->subns_incr & GEM_SUBNSINCH_MASK)
+		    >> GEM_SUBNSINCH_SHFT);
+	gem_writel(bp, TISUBN, subnsreg);
+	gem_writel(bp, TI, 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

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Andrei Pistirica]

From: Harini Katakam <harinik@xilinx.com>

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 24 bit subns precision, the subns increment equals
  remainder of (NS_PER_SEC/TSU_CLK) * (2^24).
- 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: Harini Katakam <harinik@xilinx.com>
Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
---
This patch is based on original Harini's patch, implemented in a
separate file to ease the review/maintanance and integration with
other platforms (e.g. Zynq Ultrascale+ MPSoC).
Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
project and also with ptpd2 version 2.3.1. PTP was tested over
IPv4,IPv6 and 802.3 protocols.

Hariani, please let me know if you are ok with this patch, and if you
want to be stated as author/signed-off?

In case that macb is compiled as a module, it has been renamed to
cadence-macb.ko to avoid naming confusion in Makefile.

Patch is not completely ported to the very latest version of net-next,
and it will be after review.

 drivers/net/ethernet/cadence/Kconfig    |  10 +-
 drivers/net/ethernet/cadence/Makefile   |   8 +-
 drivers/net/ethernet/cadence/macb.h     |  82 ++++++++++++
 drivers/net/ethernet/cadence/macb_ptp.c | 224 ++++++++++++++++++++++++++++++++
 4 files changed, 322 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..8c3779d 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,12 @@
 #ifndef _MACB_H
 #define _MACB_H
 
+#include <linux/net_tstamp.h>
+#include <linux/ptp_clock.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/skbuff.h>
+#include <linux/timecounter.h>
+
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
 #define MACB_MAX_QUEUES 8
@@ -129,6 +135,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 +191,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 +333,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 +426,20 @@
 #define GEM_TX_PKT_BUFF_OFFSET			21
 #define GEM_TX_PKT_BUFF_SIZE			1
 
+/* Bitfields in 1588INCRSUBNS */
+#define GEM_SUBNSINCL_SHFT			24
+#define GEM_SUBNSINCL_MASK			0xFF
+#define GEM_SUBNSINCH_SHFT			8
+#define GEM_SUBNSINCH_MASK			0xFFFF00
+
+/* Bitfields in 1588INCRNS */
+#define GEM_NSINCR_OFFSET			0
+#define GEM_NSINCR_SIZE				8
+
+/* Bitfields in 1588ADJ */
+#define GEM_ADDSUB_OFFSET			31
+#define GEM_ADDSUB_SIZE				1
+
 /* Constants for CLK */
 #define MACB_CLK_DIV8				0
 #define MACB_CLK_DIV16				1
@@ -840,8 +905,25 @@ 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;
+	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..6d6a6ec
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -0,0 +1,224 @@
+/*
+ * PTP 1588 clock for SAMA5D2 platform.
+ *
+ * Copyright (C) 2015 Xilinx Inc.
+ * Copyright (C) 2016 Microchip Technology
+ *
+ * Authors: Harini Katakam <harinik@xilinx.com>
+ *
+ * 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-ptp"
+
+static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
+{
+	u64 sech, secl;
+
+	/* get GEM internal time */
+	sech = gem_readl(bp, TSH);
+	secl = gem_readl(bp, TSL);
+
+	ts->tv_sec = (sech << 32) | secl;
+	ts->tv_nsec = gem_readl(bp, TN);
+}
+
+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);
+
+	/* set GEM internal time */
+	gem_writel(bp, TSH, sech);
+	gem_writel(bp, TSL, secl);
+	gem_writel(bp, TN, ns);
+}
+
+static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
+	unsigned long rate = bp->tsu_rate;
+	u64 adjsub;
+	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
+	bool neg_adj = false;
+
+	if (ppb < 0) {
+		neg_adj = true;
+		ppb = -ppb;
+	}
+
+	addend = bp->ns_incr;
+	addendsub = bp->subns_incr;
+
+	diff = div_u64_rem(ppb, rate, &rem);
+	addend = neg_adj ? addend - diff : addend + diff;
+
+	if (rem) {
+		adjsub = rem;
+		/* Multiple by 2^24 as subns field is 24 bits */
+		adjsub = adjsub << 24;
+
+		diffsub = div_u64(adjsub, rate);
+	} else {
+		diffsub = 0;
+	}
+
+	if (neg_adj && (diffsub > addendsub)) {
+		addend -= 1;
+		rem = (NSEC_PER_SEC - rem);
+		neg_adj = false;
+
+		adjsub = rem;
+		adjsub = adjsub << 24;
+		diffsub = div_u64(adjsub, rate);
+	}
+
+	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
+	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
+	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
+		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
+
+	gem_writel(bp, TISUBN, subnsreg);
+	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
+
+	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 subnsreg, rem = 0;
+	u64 adj;
+
+	bp->ptp_caps = macb_ptp_caps;
+	bp->tsu_rate = clk_get_rate(bp->pclk);
+
+	gem_writel(bp, TSH, 0);
+	gem_writel(bp, TSL, 0);
+	gem_writel(bp, TN, 0);
+	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) {
+		adj = rem;
+		/* Multiply by 2^24 as subns register is 24 bits */
+		adj = adj << 24;
+
+		bp->subns_incr = div_u64(adj, bp->tsu_rate);
+	} else {
+		bp->subns_incr = 0;
+	}
+
+	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
+	subnsreg = ((bp->subns_incr & GEM_SUBNSINCL_MASK)
+		    << GEM_SUBNSINCL_SHFT) |
+		   ((bp->subns_incr & GEM_SUBNSINCH_MASK)
+		    >> GEM_SUBNSINCH_SHFT);
+	gem_writel(bp, TISUBN, subnsreg);
+	gem_writel(bp, TI, 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

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

[Andrei Pistirica]

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 <andrei.pistirica@microchip.com>
---
Integration with SAMA5D2 only. This feature wasn't tested on any
other platform that might use cadence/gem.

Patch is not completely ported to the very latest version of net-next,
and it will be after review.

 drivers/net/ethernet/cadence/macb.c     |  25 +++-
 drivers/net/ethernet/cadence/macb.h     |  13 ++
 drivers/net/ethernet/cadence/macb_ptp.c | 217 ++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+), 1 deletion(-)

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
 				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
 		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
 }
 
 /*
@@ -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);
+
 	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; }
 #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 <harinik@xilinx.com>
+ *	    Andrei Pistirica <andrei.pistirica@microchip.com>
  *
  * 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.
+ *
+ * Note: Inspired from drivers/net/ethernet/ti/cpts.c
+ */
+static inline int macb_get_ptp_peer(struct sk_buff *skb, int ptp_class)
+{
+	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)
+{
+	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)
+{
+	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);
+	}
+	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 !!! */
+	__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

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

[Andrei Pistirica]

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 <andrei.pistirica@microchip.com>
---
Integration with SAMA5D2 only. This feature wasn't tested on any
other platform that might use cadence/gem.

Patch is not completely ported to the very latest version of net-next,
and it will be after review.

 drivers/net/ethernet/cadence/macb.c     |  25 +++-
 drivers/net/ethernet/cadence/macb.h     |  13 ++
 drivers/net/ethernet/cadence/macb_ptp.c | 217 ++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+), 1 deletion(-)

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
 				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
 		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
 }
 
 /*
@@ -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);
+
 	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; }
 #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 <harinik@xilinx.com>
+ *	    Andrei Pistirica <andrei.pistirica@microchip.com>
  *
  * 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.
+ *
+ * Note: Inspired from drivers/net/ethernet/ti/cpts.c
+ */
+static inline int macb_get_ptp_peer(struct sk_buff *skb, int ptp_class)
+{
+	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)
+{
+	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)
+{
+	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);
+	}
+	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 !!! */
+	__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

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi Andrei,

Adding Richard Cochran for PTP.

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> wrote:
> From: Harini Katakam <harinik@xilinx.com>
>
> 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 24 bit subns precision, the subns increment equals
>   remainder of (NS_PER_SEC/TSU_CLK) * (2^24).
> - 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: Harini Katakam <harinik@xilinx.com>
> Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
> ---
> This patch is based on original Harini's patch, implemented in a
> separate file to ease the review/maintanance and integration with
> other platforms (e.g. Zynq Ultrascale+ MPSoC).
> Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
> project and also with ptpd2 version 2.3.1. PTP was tested over
> IPv4,IPv6 and 802.3 protocols.
>
> Hariani, please let me know if you are ok with this patch, and if you
> want to be stated as author/signed-off?
>

Thanks for the patch. It seems ok. I'm making some changes on
top of this, especially in your second patch for picking RX timestamp,
since that is handled from indication in the BD for ZynqMP.

Regards,
Harini

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi Andrei,

Adding Richard Cochran for PTP.

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> wrote:
> From: Harini Katakam <harinik@xilinx.com>
>
> 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 24 bit subns precision, the subns increment equals
>   remainder of (NS_PER_SEC/TSU_CLK) * (2^24).
> - 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: Harini Katakam <harinik@xilinx.com>
> Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
> ---
> This patch is based on original Harini's patch, implemented in a
> separate file to ease the review/maintanance and integration with
> other platforms (e.g. Zynq Ultrascale+ MPSoC).
> Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
> project and also with ptpd2 version 2.3.1. PTP was tested over
> IPv4,IPv6 and 802.3 protocols.
>
> Hariani, please let me know if you are ok with this patch, and if you
> want to be stated as author/signed-off?
>

Thanks for the patch. It seems ok. I'm making some changes on
top of this, especially in your second patch for picking RX timestamp,
since that is handled from indication in the BD for ZynqMP.

Regards,
Harini

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi Andrei,

Adding Richard Cochran for PTP.

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> wrote:
> From: Harini Katakam <harinik@xilinx.com>
>
> 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 24 bit subns precision, the subns increment equals
>   remainder of (NS_PER_SEC/TSU_CLK) * (2^24).
> - 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: Harini Katakam <harinik@xilinx.com>
> Signed-off-by: Andrei Pistirica <andrei.pistirica@microchip.com>
> ---
> This patch is based on original Harini's patch, implemented in a
> separate file to ease the review/maintanance and integration with
> other platforms (e.g. Zynq Ultrascale+ MPSoC).
> Feature was tested on SAMA5D2 platform using ptp4l v1.6 from linuxptp
> project and also with ptpd2 version 2.3.1. PTP was tested over
> IPv4,IPv6 and 802.3 protocols.
>
> Hariani, please let me know if you are ok with this patch, and if you
> want to be stated as author/signed-off?
>

Thanks for the patch. It seems ok. I'm making some changes on
top of this, especially in your second patch for picking RX timestamp,
since that is handled from indication in the BD for ZynqMP.

Regards,
Harini

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

[Harini Katakam]

Hi Andrei,

+Richard Cochran

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> 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 <andrei.pistirica@microchip.com>
> ---
> Integration with SAMA5D2 only. This feature wasn't tested on any
> other platform that might use cadence/gem.
>
> Patch is not completely ported to the very latest version of net-next,
> and it will be after review.
>
<snip>
> @@ -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

I'm just wondering if the same #ifdef can be used for timestamping
in all versions of this IP.
As you know, ZynqMP uses the timestamp from BD and older
versions of do not have an indication or extended BD support.
So, it might useful to add a dependency/check on the product family,
through config structure.
That way, both version can use their respective RX timestamp methods
based on the compatible string.
Please let me know if you have any other ideas.

Regards,
Harini

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

[Harini Katakam]

Hi Andrei,

+Richard Cochran

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> 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 <andrei.pistirica@microchip.com>
> ---
> Integration with SAMA5D2 only. This feature wasn't tested on any
> other platform that might use cadence/gem.
>
> Patch is not completely ported to the very latest version of net-next,
> and it will be after review.
>
<snip>
> @@ -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

I'm just wondering if the same #ifdef can be used for timestamping
in all versions of this IP.
As you know, ZynqMP uses the timestamp from BD and older
versions of do not have an indication or extended BD support.
So, it might useful to add a dependency/check on the product family,
through config structure.
That way, both version can use their respective RX timestamp methods
based on the compatible string.
Please let me know if you have any other ideas.

Regards,
Harini

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

[Harini Katakam]

Hi Andrei,

+Richard Cochran

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
<andrei.pistirica@microchip.com> 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 <andrei.pistirica@microchip.com>
> ---
> Integration with SAMA5D2 only. This feature wasn't tested on any
> other platform that might use cadence/gem.
>
> Patch is not completely ported to the very latest version of net-next,
> and it will be after review.
>
<snip>
> @@ -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

I'm just wondering if the same #ifdef can be used for timestamping
in all versions of this IP.
As you know, ZynqMP uses the timestamp from BD and older
versions of do not have an indication or extended BD support.
So, it might useful to add a dependency/check on the product family,
through config structure.
That way, both version can use their respective RX timestamp methods
based on the compatible string.
Please let me know if you have any other ideas.

Regards,
Harini

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

I have some issues with this patch...

On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:

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

This makes no sense.  If you cannot adjust the frequency, then you
must implement a timecounter/cyclecounter and do in software.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3f385ab..8c3779d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,12 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#include <linux/net_tstamp.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/skbuff.h>
> +#include <linux/timecounter.h>

Here in the header file, you only need ptp_clock_kernel.h.  You don't
need all the others here.  Move them to the .c file, but only if
really needed.  (timecounter.h isn't used, now is it?)

> @@ -129,6 +135,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 */

This regsiter does not exist.  Looking at
 
   Zynq-7000 AP SoC Technical Reference Manual
   UG585 (v1.10) February 23, 2015

starting on page 1273 we see:

udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment

There is no register at 0x1BC.

> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */

This one doesn't exist either.  What is going on here?

> +#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 */

BTW, it is really annoying that you invent new register names.  Why
can't you use the names from the TRM?

> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void macb_ptp_init(struct net_device *ndev);
> +#else
> +void macb_ptp_init(struct net_device *ndev) { }

This should be static inline.

> +#endif

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100644
> index 0000000..6d6a6ec
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,224 @@
> +/*
> + * PTP 1588 clock for SAMA5D2 platform.
> + *
> + * Copyright (C) 2015 Xilinx Inc.
> + * Copyright (C) 2016 Microchip Technology
> + *
> + * Authors: Harini Katakam <harinik@xilinx.com>
> + *
> + * 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-ptp"
> +
> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
> +{
> +	u64 sech, secl;
> +
> +	/* get GEM internal time */
> +	sech = gem_readl(bp, TSH);
> +	secl = gem_readl(bp, TSL);

Does reading TSH latch the time?  The TRM is silent about that, and
most other designs latch on reading the LSB.

> +	ts->tv_sec = (sech << 32) | secl;
> +	ts->tv_nsec = gem_readl(bp, TN);
> +}
> +
> +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);
> +
> +	/* set GEM internal time */

Writing three registers is supposed to be one operation, and yet you
have no mutex or spinlock to protect against concurrent settime calls.

> +	gem_writel(bp, TSH, sech);
> +	gem_writel(bp, TSL, secl);
> +	gem_writel(bp, TN, ns);

Also, how does the HW behave here?  Latch on TN write?

> +}
> +
> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> +	unsigned long rate = bp->tsu_rate;
> +	u64 adjsub;
> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
> +	bool neg_adj = false;
> +
> +	if (ppb < 0) {
> +		neg_adj = true;
> +		ppb = -ppb;
> +	}
> +
> +	addend = bp->ns_incr;
> +	addendsub = bp->subns_incr;
> +
> +	diff = div_u64_rem(ppb, rate, &rem);

One ...

> +	addend = neg_adj ? addend - diff : addend + diff;
> +
> +	if (rem) {
> +		adjsub = rem;
> +		/* Multiple by 2^24 as subns field is 24 bits */
> +		adjsub = adjsub << 24;
> +
> +		diffsub = div_u64(adjsub, rate);

... Two ...

> +	} else {
> +		diffsub = 0;
> +	}
> +
> +	if (neg_adj && (diffsub > addendsub)) {
> +		addend -= 1;
> +		rem = (NSEC_PER_SEC - rem);
> +		neg_adj = false;
> +
> +		adjsub = rem;
> +		adjsub = adjsub << 24;
> +		diffsub = div_u64(adjsub, rate);

Three.  You need three 64 bit divisions?  There must be a better way.

If I guess correctly, the nsincr/subnsincr value is added to the
device's time on every clock.  So scaling these effectively adjusts
the frequency.  Here is an idea for you.

You have a 24 bit fractional field.  Combine that with 8 bits of whole
nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
bits in the setup code. (8 bits allows input clocks down to 4 MHz.)

Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
result.  Divide the result by 10^9 and that gives you the delta to add
to (or subtract from) the nominal tuning word.

See gianfar.c for an example.

> +	}
> +
> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
> +
> +	gem_writel(bp, TISUBN, subnsreg);
> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
> +
> +	return 0;
> +}

Thanks,
Richard

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

I have some issues with this patch...

On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:

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

This makes no sense.  If you cannot adjust the frequency, then you
must implement a timecounter/cyclecounter and do in software.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3f385ab..8c3779d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,12 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#include <linux/net_tstamp.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/skbuff.h>
> +#include <linux/timecounter.h>

Here in the header file, you only need ptp_clock_kernel.h.  You don't
need all the others here.  Move them to the .c file, but only if
really needed.  (timecounter.h isn't used, now is it?)

> @@ -129,6 +135,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 */

This regsiter does not exist.  Looking at
 
   Zynq-7000 AP SoC Technical Reference Manual
   UG585 (v1.10) February 23, 2015

starting on page 1273 we see:

udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment

There is no register at 0x1BC.

> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */

This one doesn't exist either.  What is going on here?

> +#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 */

BTW, it is really annoying that you invent new register names.  Why
can't you use the names from the TRM?

> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void macb_ptp_init(struct net_device *ndev);
> +#else
> +void macb_ptp_init(struct net_device *ndev) { }

This should be static inline.

> +#endif

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100644
> index 0000000..6d6a6ec
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,224 @@
> +/*
> + * PTP 1588 clock for SAMA5D2 platform.
> + *
> + * Copyright (C) 2015 Xilinx Inc.
> + * Copyright (C) 2016 Microchip Technology
> + *
> + * Authors: Harini Katakam <harinik@xilinx.com>
> + *
> + * 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-ptp"
> +
> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
> +{
> +	u64 sech, secl;
> +
> +	/* get GEM internal time */
> +	sech = gem_readl(bp, TSH);
> +	secl = gem_readl(bp, TSL);

Does reading TSH latch the time?  The TRM is silent about that, and
most other designs latch on reading the LSB.

> +	ts->tv_sec = (sech << 32) | secl;
> +	ts->tv_nsec = gem_readl(bp, TN);
> +}
> +
> +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);
> +
> +	/* set GEM internal time */

Writing three registers is supposed to be one operation, and yet you
have no mutex or spinlock to protect against concurrent settime calls.

> +	gem_writel(bp, TSH, sech);
> +	gem_writel(bp, TSL, secl);
> +	gem_writel(bp, TN, ns);

Also, how does the HW behave here?  Latch on TN write?

> +}
> +
> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> +	unsigned long rate = bp->tsu_rate;
> +	u64 adjsub;
> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
> +	bool neg_adj = false;
> +
> +	if (ppb < 0) {
> +		neg_adj = true;
> +		ppb = -ppb;
> +	}
> +
> +	addend = bp->ns_incr;
> +	addendsub = bp->subns_incr;
> +
> +	diff = div_u64_rem(ppb, rate, &rem);

One ...

> +	addend = neg_adj ? addend - diff : addend + diff;
> +
> +	if (rem) {
> +		adjsub = rem;
> +		/* Multiple by 2^24 as subns field is 24 bits */
> +		adjsub = adjsub << 24;
> +
> +		diffsub = div_u64(adjsub, rate);

... Two ...

> +	} else {
> +		diffsub = 0;
> +	}
> +
> +	if (neg_adj && (diffsub > addendsub)) {
> +		addend -= 1;
> +		rem = (NSEC_PER_SEC - rem);
> +		neg_adj = false;
> +
> +		adjsub = rem;
> +		adjsub = adjsub << 24;
> +		diffsub = div_u64(adjsub, rate);

Three.  You need three 64 bit divisions?  There must be a better way.

If I guess correctly, the nsincr/subnsincr value is added to the
device's time on every clock.  So scaling these effectively adjusts
the frequency.  Here is an idea for you.

You have a 24 bit fractional field.  Combine that with 8 bits of whole
nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
bits in the setup code. (8 bits allows input clocks down to 4 MHz.)

Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
result.  Divide the result by 10^9 and that gives you the delta to add
to (or subtract from) the nominal tuning word.

See gianfar.c for an example.

> +	}
> +
> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
> +
> +	gem_writel(bp, TISUBN, subnsreg);
> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
> +
> +	return 0;
> +}

Thanks,
Richard

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

I have some issues with this patch...

On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:

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

This makes no sense.  If you cannot adjust the frequency, then you
must implement a timecounter/cyclecounter and do in software.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3f385ab..8c3779d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,12 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#include <linux/net_tstamp.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/skbuff.h>
> +#include <linux/timecounter.h>

Here in the header file, you only need ptp_clock_kernel.h.  You don't
need all the others here.  Move them to the .c file, but only if
really needed.  (timecounter.h isn't used, now is it?)

> @@ -129,6 +135,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 */

This regsiter does not exist.  Looking at
 
   Zynq-7000 AP SoC Technical Reference Manual
   UG585 (v1.10) February 23, 2015

starting on page 1273 we see:

udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment

There is no register at 0x1BC.

> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */

This one doesn't exist either.  What is going on here?

> +#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 */

BTW, it is really annoying that you invent new register names.  Why
can't you use the names from the TRM?

> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +void macb_ptp_init(struct net_device *ndev);
> +#else
> +void macb_ptp_init(struct net_device *ndev) { }

This should be static inline.

> +#endif

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> new file mode 100644
> index 0000000..6d6a6ec
> --- /dev/null
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -0,0 +1,224 @@
> +/*
> + * PTP 1588 clock for SAMA5D2 platform.
> + *
> + * Copyright (C) 2015 Xilinx Inc.
> + * Copyright (C) 2016 Microchip Technology
> + *
> + * Authors: Harini Katakam <harinik@xilinx.com>
> + *
> + * 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-ptp"
> +
> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
> +{
> +	u64 sech, secl;
> +
> +	/* get GEM internal time */
> +	sech = gem_readl(bp, TSH);
> +	secl = gem_readl(bp, TSL);

Does reading TSH latch the time?  The TRM is silent about that, and
most other designs latch on reading the LSB.

> +	ts->tv_sec = (sech << 32) | secl;
> +	ts->tv_nsec = gem_readl(bp, TN);
> +}
> +
> +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);
> +
> +	/* set GEM internal time */

Writing three registers is supposed to be one operation, and yet you
have no mutex or spinlock to protect against concurrent settime calls.

> +	gem_writel(bp, TSH, sech);
> +	gem_writel(bp, TSL, secl);
> +	gem_writel(bp, TN, ns);

Also, how does the HW behave here?  Latch on TN write?

> +}
> +
> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
> +	unsigned long rate = bp->tsu_rate;
> +	u64 adjsub;
> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
> +	bool neg_adj = false;
> +
> +	if (ppb < 0) {
> +		neg_adj = true;
> +		ppb = -ppb;
> +	}
> +
> +	addend = bp->ns_incr;
> +	addendsub = bp->subns_incr;
> +
> +	diff = div_u64_rem(ppb, rate, &rem);

One ...

> +	addend = neg_adj ? addend - diff : addend + diff;
> +
> +	if (rem) {
> +		adjsub = rem;
> +		/* Multiple by 2^24 as subns field is 24 bits */
> +		adjsub = adjsub << 24;
> +
> +		diffsub = div_u64(adjsub, rate);

... Two ...

> +	} else {
> +		diffsub = 0;
> +	}
> +
> +	if (neg_adj && (diffsub > addendsub)) {
> +		addend -= 1;
> +		rem = (NSEC_PER_SEC - rem);
> +		neg_adj = false;
> +
> +		adjsub = rem;
> +		adjsub = adjsub << 24;
> +		diffsub = div_u64(adjsub, rate);

Three.  You need three 64 bit divisions?  There must be a better way.

If I guess correctly, the nsincr/subnsincr value is added to the
device's time on every clock.  So scaling these effectively adjusts
the frequency.  Here is an idea for you.

You have a 24 bit fractional field.  Combine that with 8 bits of whole
nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
bits in the setup code. (8 bits allows input clocks down to 4 MHz.)

Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
result.  Divide the result by 10^9 and that gives you the delta to add
to (or subtract from) the nominal tuning word.

See gianfar.c for an example.

> +	}
> +
> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
> +
> +	gem_writel(bp, TISUBN, subnsreg);
> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
> +
> +	return 0;
> +}

Thanks,
Richard

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

[Richard Cochran]

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 <andrei.pistirica@microchip.com>
> ---
> 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 <harinik@xilinx.com>
> + *	    Andrei Pistirica <andrei.pistirica@microchip.com>

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

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

[Richard Cochran]

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 <andrei.pistirica@microchip.com>
> ---
> 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 <harinik@xilinx.com>
> + *	    Andrei Pistirica <andrei.pistirica@microchip.com>

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

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

[Richard Cochran]

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 <andrei.pistirica@microchip.com>
> ---
> 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 <harinik@xilinx.com>
> + *	    Andrei Pistirica <andrei.pistirica@microchip.com>

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

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi,

On Tue, Sep 6, 2016 at 9:18 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
>
<snip>
>> +#define GEM_TISUBN           0x01bc /* 1588 Timer Increment Sub-ns */
>
> This regsiter does not exist.  Looking at
>
>    Zynq-7000 AP SoC Technical Reference Manual
>    UG585 (v1.10) February 23, 2015
>
> starting on page 1273 we see:
>
> udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
> timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
> timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
> timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
> timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
> timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
> timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment
>
> There is no register at 0x1BC.
>
>> +#define GEM_TSH                      0x01c0 /* 1588 Timer Seconds High */
>
> This one doesn't exist either.  What is going on here?

I cant be sure of the version of Cadence GEM used in SAMA5D2
but these registers (sub ns increments alteast) only exist in
the IP version used in Zynq Ultrascale+ MPSoC.

<snip>
>> +     /* get GEM internal time */
>> +     sech = gem_readl(bp, TSH);
>> +     secl = gem_readl(bp, TSL);
>
> Does reading TSH latch the time?  The TRM is silent about that, and
> most other designs latch on reading the LSB.

No, it does not latch the time.
When doing a read + adjust + write, this will
mean there's room for some error.

Although when writing, the write to MSB and LSB registers
was made atomic. This bug fix came only in the
most recent version of the IP, am afraid.

Regards,
Harini

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi,

On Tue, Sep 6, 2016 at 9:18 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
>
<snip>
>> +#define GEM_TISUBN           0x01bc /* 1588 Timer Increment Sub-ns */
>
> This regsiter does not exist.  Looking at
>
>    Zynq-7000 AP SoC Technical Reference Manual
>    UG585 (v1.10) February 23, 2015
>
> starting on page 1273 we see:
>
> udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
> timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
> timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
> timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
> timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
> timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
> timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment
>
> There is no register at 0x1BC.
>
>> +#define GEM_TSH                      0x01c0 /* 1588 Timer Seconds High */
>
> This one doesn't exist either.  What is going on here?

I cant be sure of the version of Cadence GEM used in SAMA5D2
but these registers (sub ns increments alteast) only exist in
the IP version used in Zynq Ultrascale+ MPSoC.

<snip>
>> +     /* get GEM internal time */
>> +     sech = gem_readl(bp, TSH);
>> +     secl = gem_readl(bp, TSL);
>
> Does reading TSH latch the time?  The TRM is silent about that, and
> most other designs latch on reading the LSB.

No, it does not latch the time.
When doing a read + adjust + write, this will
mean there's room for some error.

Although when writing, the write to MSB and LSB registers
was made atomic. This bug fix came only in the
most recent version of the IP, am afraid.

Regards,
Harini

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Harini Katakam]

Hi,

On Tue, Sep 6, 2016 at 9:18 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
>
<snip>
>> +#define GEM_TISUBN           0x01bc /* 1588 Timer Increment Sub-ns */
>
> This regsiter does not exist.  Looking at
>
>    Zynq-7000 AP SoC Technical Reference Manual
>    UG585 (v1.10) February 23, 2015
>
> starting on page 1273 we see:
>
> udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
> timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
> timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
> timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
> timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
> timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
> timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment
>
> There is no register at 0x1BC.
>
>> +#define GEM_TSH                      0x01c0 /* 1588 Timer Seconds High */
>
> This one doesn't exist either.  What is going on here?

I cant be sure of the version of Cadence GEM used in SAMA5D2
but these registers (sub ns increments alteast) only exist in
the IP version used in Zynq Ultrascale+ MPSoC.

<snip>
>> +     /* get GEM internal time */
>> +     sech = gem_readl(bp, TSH);
>> +     secl = gem_readl(bp, TSL);
>
> Does reading TSH latch the time?  The TRM is silent about that, and
> most other designs latch on reading the LSB.

No, it does not latch the time.
When doing a read + adjust + write, this will
mean there's room for some error.

Although when writing, the write to MSB and LSB registers
was made atomic. This bug fix came only in the
most recent version of the IP, am afraid.

Regards,
Harini

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> I cant be sure of the version of Cadence GEM used in SAMA5D2
> but these registers (sub ns increments alteast) only exist in
> the IP version used in Zynq Ultrascale+ MPSoC.

Then you need to find a way to make sure the driver only binds to the
correct silicon.

Thanks,
Richard

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> I cant be sure of the version of Cadence GEM used in SAMA5D2
> but these registers (sub ns increments alteast) only exist in
> the IP version used in Zynq Ultrascale+ MPSoC.

Then you need to find a way to make sure the driver only binds to the
correct silicon.

Thanks,
Richard

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> I cant be sure of the version of Cadence GEM used in SAMA5D2
> but these registers (sub ns increments alteast) only exist in
> the IP version used in Zynq Ultrascale+ MPSoC.

Then you need to find a way to make sure the driver only binds to the
correct silicon.

Thanks,
Richard

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> >> +     /* get GEM internal time */
> >> +     sech = gem_readl(bp, TSH);
> >> +     secl = gem_readl(bp, TSL);
> >
> > Does reading TSH latch the time?  The TRM is silent about that, and
> > most other designs latch on reading the LSB.
> 
> No, it does not latch the time.
> When doing a read + adjust + write, this will
> mean there's room for some error.

It also means that you will have to handle when the TSL value
overflows into TSH.  That means reading TSH twice, once before and
once after reading TSL and retrying if needed.

The code as written above will produce apparent jumps backwards in
time, whenever the overflow occurs between the two read operations.

Thanks,
Richard

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> >> +     /* get GEM internal time */
> >> +     sech = gem_readl(bp, TSH);
> >> +     secl = gem_readl(bp, TSL);
> >
> > Does reading TSH latch the time?  The TRM is silent about that, and
> > most other designs latch on reading the LSB.
> 
> No, it does not latch the time.
> When doing a read + adjust + write, this will
> mean there's room for some error.

It also means that you will have to handle when the TSL value
overflows into TSH.  That means reading TSH twice, once before and
once after reading TSL and retrying if needed.

The code as written above will produce apparent jumps backwards in
time, whenever the overflow occurs between the two read operations.

Thanks,
Richard

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Richard Cochran]

On Thu, Sep 08, 2016 at 10:22:43AM +0530, Harini Katakam wrote:
> >> +     /* get GEM internal time */
> >> +     sech = gem_readl(bp, TSH);
> >> +     secl = gem_readl(bp, TSL);
> >
> > Does reading TSH latch the time?  The TRM is silent about that, and
> > most other designs latch on reading the LSB.
> 
> No, it does not latch the time.
> When doing a read + adjust + write, this will
> mean there's room for some error.

It also means that you will have to handle when the TSL value
overflows into TSH.  That means reading TSH twice, once before and
once after reading TSL and retrying if needed.

The code as written above will produce apparent jumps backwards in
time, whenever the overflow occurs between the two read operations.

Thanks,
Richard

Re: [RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Andrei Pistirica]

On 06.09.2016 17:48, Richard Cochran wrote:
>
> I have some issues with this patch...
>
> On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:
>
>> - 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.
>
> This makes no sense.  If you cannot adjust the frequency, then you
> must implement a timecounter/cyclecounter and do in software.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 3f385ab..8c3779d 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -10,6 +10,12 @@
>>  #ifndef _MACB_H
>>  #define _MACB_H
>>
>> +#include <linux/net_tstamp.h>
>> +#include <linux/ptp_clock.h>
>> +#include <linux/ptp_clock_kernel.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/timecounter.h>
>
> Here in the header file, you only need ptp_clock_kernel.h.  You don't
> need all the others here.  Move them to the .c file, but only if
> really needed.  (timecounter.h isn't used, now is it?)

Ack.
Except timecounter.h, I need these headers for
ptp_clock and ptp_clock_info parameters from struct macb.

>
>> @@ -129,6 +135,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 */
>
> This regsiter does not exist.  Looking at
>
>    Zynq-7000 AP SoC Technical Reference Manual
>    UG585 (v1.10) February 23, 2015
>
> starting on page 1273 we see:
>
> udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
> timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
> timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
> timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
> timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
> timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
> timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment
>
> There is no register at 0x1BC.
>

For this feature I've used the following reference manual: 
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf

In section 37.8 (page 980) there is: "588 Timer Increment 
Sub-nanoseconds Register GMAC_TISUBN Read/Write", detailed in section 
37.8.92.
I kept the naming convention used in the entire header.

>> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */
>
> This one doesn't exist either.  What is going on here?
>
>> +#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 */
>
> BTW, it is really annoying that you invent new register names.  Why
> can't you use the names from the TRM?

All these registers are found in the document mentioned above.

>
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void macb_ptp_init(struct net_device *ndev);
>> +#else
>> +void macb_ptp_init(struct net_device *ndev) { }
>
> This should be static inline.

Ack. I will update this properly.

>
>> +#endif
>
>> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
>> new file mode 100644
>> index 0000000..6d6a6ec
>> --- /dev/null
>> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
>> @@ -0,0 +1,224 @@
>> +/*
>> + * PTP 1588 clock for SAMA5D2 platform.
>> + *
>> + * Copyright (C) 2015 Xilinx Inc.
>> + * Copyright (C) 2016 Microchip Technology
>> + *
>> + * Authors: Harini Katakam <harinik@xilinx.com>
>> + *
>> + * 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-ptp"
>> +
>> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
>> +{
>> +	u64 sech, secl;
>> +
>> +	/* get GEM internal time */
>> +	sech = gem_readl(bp, TSH);
>> +	secl = gem_readl(bp, TSL);
>
> Does reading TSH latch the time?  The TRM is silent about that, and
> most other designs latch on reading the LSB.
>
>> +	ts->tv_sec = (sech << 32) | secl;
>> +	ts->tv_nsec = gem_readl(bp, TN);
>> +}
>> +
>> +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);
>> +
>> +	/* set GEM internal time */
>
> Writing three registers is supposed to be one operation, and yet you
> have no mutex or spinlock to protect against concurrent settime calls.

Ack. I will add a spinlock.

>
>> +	gem_writel(bp, TSH, sech);
>> +	gem_writel(bp, TSL, secl);
>> +	gem_writel(bp, TN, ns);
>
> Also, how does the HW behave here?  Latch on TN write?
>
>> +}
>> +
>> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>> +{
>> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
>> +	unsigned long rate = bp->tsu_rate;
>> +	u64 adjsub;
>> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
>> +	bool neg_adj = false;
>> +
>> +	if (ppb < 0) {
>> +		neg_adj = true;
>> +		ppb = -ppb;
>> +	}
>> +
>> +	addend = bp->ns_incr;
>> +	addendsub = bp->subns_incr;
>> +
>> +	diff = div_u64_rem(ppb, rate, &rem);
>
> One ...
>
>> +	addend = neg_adj ? addend - diff : addend + diff;
>> +
>> +	if (rem) {
>> +		adjsub = rem;
>> +		/* Multiple by 2^24 as subns field is 24 bits */
>> +		adjsub = adjsub << 24;
>> +
>> +		diffsub = div_u64(adjsub, rate);
>
> ... Two ...
>
>> +	} else {
>> +		diffsub = 0;
>> +	}
>> +
>> +	if (neg_adj && (diffsub > addendsub)) {
>> +		addend -= 1;
>> +		rem = (NSEC_PER_SEC - rem);
>> +		neg_adj = false;
>> +
>> +		adjsub = rem;
>> +		adjsub = adjsub << 24;
>> +		diffsub = div_u64(adjsub, rate);
>
> Three.  You need three 64 bit divisions?  There must be a better way.
>
> If I guess correctly, the nsincr/subnsincr value is added to the
> device's time on every clock.  So scaling these effectively adjusts
> the frequency.  Here is an idea for you.
>
> You have a 24 bit fractional field.  Combine that with 8 bits of whole
> nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
> bits in the setup code. (8 bits allows input clocks down to 4 MHz.)
>
> Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
> result.  Divide the result by 10^9 and that gives you the delta to add
> to (or subtract from) the nominal tuning word.
>
> See gianfar.c for an example.

Ack.
This code was taken with respect from Harini's first patch: 
https://lkml.org/lkml/2015/9/11/92.

I can add  an additional patch to take care of this on top of Harini's 
patch.

>
>> +	}
>> +
>> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
>> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
>> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
>> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
>> +
>> +	gem_writel(bp, TISUBN, subnsreg);
>> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
>> +
>> +	return 0;
>> +}
>
> Thanks,
> Richard
>

Thanks,
Andrei

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Andrei Pistirica]

On 06.09.2016 17:48, Richard Cochran wrote:
>
> I have some issues with this patch...
>
> On Fri, Sep 02, 2016 at 02:53:36PM +0200, Andrei Pistirica wrote:
>
>> - 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.
>
> This makes no sense.  If you cannot adjust the frequency, then you
> must implement a timecounter/cyclecounter and do in software.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 3f385ab..8c3779d 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -10,6 +10,12 @@
>>  #ifndef _MACB_H
>>  #define _MACB_H
>>
>> +#include <linux/net_tstamp.h>
>> +#include <linux/ptp_clock.h>
>> +#include <linux/ptp_clock_kernel.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/timecounter.h>
>
> Here in the header file, you only need ptp_clock_kernel.h.  You don't
> need all the others here.  Move them to the .c file, but only if
> really needed.  (timecounter.h isn't used, now is it?)

Ack.
Except timecounter.h, I need these headers for
ptp_clock and ptp_clock_info parameters from struct macb.

>
>> @@ -129,6 +135,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 */
>
> This regsiter does not exist.  Looking at
>
>    Zynq-7000 AP SoC Technical Reference Manual
>    UG585 (v1.10) February 23, 2015
>
> starting on page 1273 we see:
>
> udp_csum_errors 0x000001B0 32 ro    0x00000000 UDP checksum error
> timer_strobe_s  0x000001C8 32 rw    0x00000000 1588 timer sync strobe seconds
> timer_strobe_ns 0x000001CC 32 mixed 0x00000000 1588 timer sync strobe nanoseconds
> timer_s         0x000001D0 32 rw    0x00000000 1588 timer seconds
> timer_ns        0x000001D4 32 mixed 0x00000000 1588 timer nanoseconds
> timer_adjust    0x000001D8 32 mixed 0x00000000 1588 timer adjust
> timer_incr      0x000001DC 32 mixed 0x00000000 1588 timer increment
>
> There is no register at 0x1BC.
>

For this feature I've used the following reference manual: 
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf

In section 37.8 (page 980) there is: "588 Timer Increment 
Sub-nanoseconds Register GMAC_TISUBN Read/Write", detailed in section 
37.8.92.
I kept the naming convention used in the entire header.

>> +#define GEM_TSH			0x01c0 /* 1588 Timer Seconds High */
>
> This one doesn't exist either.  What is going on here?
>
>> +#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 */
>
> BTW, it is really annoying that you invent new register names.  Why
> can't you use the names from the TRM?

All these registers are found in the document mentioned above.

>
>> +#ifdef CONFIG_MACB_USE_HWSTAMP
>> +void macb_ptp_init(struct net_device *ndev);
>> +#else
>> +void macb_ptp_init(struct net_device *ndev) { }
>
> This should be static inline.

Ack. I will update this properly.

>
>> +#endif
>
>> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
>> new file mode 100644
>> index 0000000..6d6a6ec
>> --- /dev/null
>> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
>> @@ -0,0 +1,224 @@
>> +/*
>> + * PTP 1588 clock for SAMA5D2 platform.
>> + *
>> + * Copyright (C) 2015 Xilinx Inc.
>> + * Copyright (C) 2016 Microchip Technology
>> + *
>> + * Authors: Harini Katakam <harinik@xilinx.com>
>> + *
>> + * 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-ptp"
>> +
>> +static inline void macb_tsu_get_time(struct macb *bp, struct timespec64 *ts)
>> +{
>> +	u64 sech, secl;
>> +
>> +	/* get GEM internal time */
>> +	sech = gem_readl(bp, TSH);
>> +	secl = gem_readl(bp, TSL);
>
> Does reading TSH latch the time?  The TRM is silent about that, and
> most other designs latch on reading the LSB.
>
>> +	ts->tv_sec = (sech << 32) | secl;
>> +	ts->tv_nsec = gem_readl(bp, TN);
>> +}
>> +
>> +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);
>> +
>> +	/* set GEM internal time */
>
> Writing three registers is supposed to be one operation, and yet you
> have no mutex or spinlock to protect against concurrent settime calls.

Ack. I will add a spinlock.

>
>> +	gem_writel(bp, TSH, sech);
>> +	gem_writel(bp, TSL, secl);
>> +	gem_writel(bp, TN, ns);
>
> Also, how does the HW behave here?  Latch on TN write?
>
>> +}
>> +
>> +static int macb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
>> +{
>> +	struct macb *bp = container_of(ptp, struct macb, ptp_caps);
>> +	unsigned long rate = bp->tsu_rate;
>> +	u64 adjsub;
>> +	u32 addend, addendsub, diff, rem, diffsub, subnsreg;
>> +	bool neg_adj = false;
>> +
>> +	if (ppb < 0) {
>> +		neg_adj = true;
>> +		ppb = -ppb;
>> +	}
>> +
>> +	addend = bp->ns_incr;
>> +	addendsub = bp->subns_incr;
>> +
>> +	diff = div_u64_rem(ppb, rate, &rem);
>
> One ...
>
>> +	addend = neg_adj ? addend - diff : addend + diff;
>> +
>> +	if (rem) {
>> +		adjsub = rem;
>> +		/* Multiple by 2^24 as subns field is 24 bits */
>> +		adjsub = adjsub << 24;
>> +
>> +		diffsub = div_u64(adjsub, rate);
>
> ... Two ...
>
>> +	} else {
>> +		diffsub = 0;
>> +	}
>> +
>> +	if (neg_adj && (diffsub > addendsub)) {
>> +		addend -= 1;
>> +		rem = (NSEC_PER_SEC - rem);
>> +		neg_adj = false;
>> +
>> +		adjsub = rem;
>> +		adjsub = adjsub << 24;
>> +		diffsub = div_u64(adjsub, rate);
>
> Three.  You need three 64 bit divisions?  There must be a better way.
>
> If I guess correctly, the nsincr/subnsincr value is added to the
> device's time on every clock.  So scaling these effectively adjusts
> the frequency.  Here is an idea for you.
>
> You have a 24 bit fractional field.  Combine that with 8 bits of whole
> nanoseconds to form one 32 tuning word.  Restrict the nanoseconds to 8
> bits in the setup code. (8 bits allows input clocks down to 4 MHz.)
>
> Multiply the tuning word by the 32 ppb variable, keeping the 64 bit
> result.  Divide the result by 10^9 and that gives you the delta to add
> to (or subtract from) the nominal tuning word.
>
> See gianfar.c for an example.

Ack.
This code was taken with respect from Harini's first patch: 
https://lkml.org/lkml/2015/9/11/92.

I can add  an additional patch to take care of this on top of Harini's 
patch.

>
>> +	}
>> +
>> +	addendsub = neg_adj ? addendsub - diffsub : addendsub + diffsub;
>> +	/* RegBit[15:0] = Subns[23:8]; RegBit[31:24] = Subns[7:0] */
>> +	subnsreg = ((addendsub & GEM_SUBNSINCL_MASK) << GEM_SUBNSINCL_SHFT) |
>> +		   ((addendsub & GEM_SUBNSINCH_MASK) >> GEM_SUBNSINCH_SHFT);
>> +
>> +	gem_writel(bp, TISUBN, subnsreg);
>> +	gem_writel(bp, TI, GEM_BF(NSINCR, addend));
>> +
>> +	return 0;
>> +}
>
> Thanks,
> Richard
>

Thanks,
Andrei

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

[Andrei Pistirica]

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 <andrei.pistirica@microchip.com>
>> ---
>> 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 <harinik@xilinx.com>
>> + *	    Andrei Pistirica <andrei.pistirica@microchip.com>
>
> 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
>

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

[Andrei Pistirica]

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 <andrei.pistirica@microchip.com>
>> ---
>> 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 <harinik@xilinx.com>
>> + *	    Andrei Pistirica <andrei.pistirica@microchip.com>
>
> 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
>

[RFC PATCH 1/2] macb: Add 1588 support in Cadence GEM.

[Rafal Ozieblo]

Hello Andrei,

>+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,
>+};

I'm wondering, how did you count max_adj ?

Best regards,

Rafal Ozieblo   |   Firmware System Engineer, 
phone nbr.: +48 32 5085469 


www.cadence.com