igb: ptp hardware clock

-----Original Message-----
From: Richard Cochran [mailto:[email protected]]
Sent: Saturday, January 07, 2012 11:38 AM
To: [email protected]
Cc: [email protected]; Keller, Jacob E; Kirsher, Jeffrey T; Ronciak, John; John Stultz; Thomas Gleixner
Subject: [PATCH net-next V3 1/2] igb: add PTP Hardware Clock code

This patch adds a source file implementing a PHC. Only the basic clock operations have been implemented, although the hardware would offer some ancillary functions. The code is fairly self contained and is not yet used in the main igb driver.

Every timestamp and clock read operation must consult the overflow counter to form a correct time value. Access to the counter is protected by a spin lock, and this would be sufficient, assuming that the time values are monotonic.

However, this assumption does not hold in general. Consider the following sequence.

1. Hardware latches a receive timestamp (just before counter
overflow).
2. User calls clock_gettime() (just after counter overflow).
3. User takes lock, detects 1-0 transition, and increments overflow
count.
4. Driver takes lock, incorrectly combines overflow count with
timestamp.

A very similar race condition exists between two nearly simultaneous hardware timestamps.

The implementation detects this race by checking the two most significant bits for a transition from 11b to 00b. When this pattern is detected, the code uses the previous value of the overflow count.

The two most significant bits divide the overflow period into four regions, and so the watchdog timeout is set to sample the counter at just over four times per period.

Signed-off-by Richard Cochran <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb.h     |    8 +
 drivers/net/ethernet/intel/igb/igb_ptp.c |  427
++++++++++++++++++++++++++++++
 2 files changed, 435 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c

drivers/net/ethernet/intel/igb/igb.h +8 -0

index c69feeb..f30458d 100644
@@ -37,6 +37,7 @@
 #include <linux/clocksource.h>
 #include <linux/timecompare.h>
 #include <linux/net_tstamp.h>
+#include <linux/ptp_clock_kernel.h>
 #include <linux/bitops.h>
 #include <linux/if_vlan.h>
 

@@ -364,6 +365,13 @@ struct igb_adapter {
 	u32 wvbr;
 	int node;
 	u32 *shadow_vfta;
+
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info caps;
+	struct delayed_work overflow_work;
+	spinlock_t tmreg_lock;
+	u32 overflow_counter;
+	unsigned int last_msb;
 };
 
 #define IGB_FLAG_HAS_MSI           (1 << 0)

drivers/net/ethernet/intel/igb/igb_ptp.c new file +6 -0

new file mode 100644
index 0000000..dd27be1
@@ -0,0 +1,427 @@
+/*
+ * PTP Hardware Clock (PHC) driver for the Intel 82576 and 82580
+ *
+ * Copyright (C) 2011 Richard Cochran <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */

drivers/net/ethernet/intel/igb/igb_ptp.c new file +57 -0

+#include <linux/device.h>
+#include <linux/pci.h>
+
+#include "igb.h"
+
+#define INCVALUE_MASK		0x7fffffff
+#define ISGN			0x80000000
+
+/*
+ * Neither the 82576 nor the 82580 offer registers wide enough to hold
+ * nanoseconds time values for very long. For the 82580, SYSTIM always
+ * counts nanoseconds, but the upper 24 bits are not availible. The
+ * frequency is adjusted by changing the 32 bit fractional nanoseconds
+ * register, TIMINCA.
+ *
+ * For the 82576, the SYSTIM register time unit is affect by the
+ * choice of the 24 bit TININCA:IV (incvalue) field. Five bits of this
+ * field are needed to provide the nominal 16 nanosecond period,
+ * leaving 19 bits for fractional nanoseconds.
+ *
+ *
+ *             SYSTIMH            SYSTIML
+ *        +--------------+   +---+---+------+
+ *  82576 |      32      |   | 8 | 5 |  19  |
+ *        +--------------+   +---+---+------+
+ *         \________ 45 bits _______/  fract
+ *
+ *        +----------+---+   +--------------+
+ *  82580 |    24    | 8 |   |      32      |
+ *        +----------+---+   +--------------+
+ *          reserved  \______ 40 bits _____/
+ *
+ *
+ * The 45 bit 82576 SYSTIM overflows every
+ *   2^45 * 10^-9 / 3600 = 9.77 hours.
+ *
+ * The 40 bit 82580 SYSTIM overflows every
+ *   2^40 * 10^-9 /  60  = 18.3 minutes.
+ *
+ * We implement a 32 bit overflow counter in software to hold the
+ * missing 19 or 24 bits, for a combined virtual nanosecond time
+ * register of 64 bits.
+ */
+
+#define IGB_OVERFLOW_PERIOD	(HZ * 60 * 4)
+#define INCPERIOD_82576		(1 << E1000_TIMINCA_16NS_SHIFT)
+#define INCVALUE_82576_MASK	((1 << E1000_TIMINCA_16NS_SHIFT) - 1)
+#define INCVALUE_82576		(16 << IGB_82576_TSYNC_SHIFT)
+#define OFL_SHIFT_82580		40
+#define OFL_SHIFT_82576		45
+#define NS_SHIFT_82576		IGB_82576_TSYNC_SHIFT
+
+/*
+ * Overflow counter for the SYSTIM register.
+ */
+
+static u64 igb_overflow_get(struct igb_adapter *igb, u64 low, unsigned

int nbit)

drivers/net/ethernet/intel/igb/igb_ptp.c new file +28 -0

+{
+	unsigned int msb_set = (low >> (nbit - 2)) & 0x3;
+
+	if (msb_set == 0x3 && igb->last_msb == 0x0) {
+		/*
+		 * This time stamp was taken before the last overflow,
+		 * but it lost the race to read the overflow counter.
+		 * Therefore, we just use the previous overflow value.
+		 */
+		low |= ((u64) igb->overflow_counter - 1ULL) << nbit;
+		return low;
+	}
+
+	/*
+	 * Test for a one to zero transition of the most significant
+	 * bit, and increment the overflow counter.
+	 */
+	if ((igb->last_msb & 0x2) && !(msb_set & 0x2))
+		igb->overflow_counter++;
+
+	igb->last_msb = msb_set;
+
+	low |= ((u64) igb->overflow_counter) << nbit;
+
+	return low;
+}
+
+static void igb_overflow_set(struct igb_adapter *igb, u64 ns, unsigned

int nbit)

drivers/net/ethernet/intel/igb/igb_ptp.c new file +121 -0

+{
+	igb->overflow_counter = ns >> nbit;
+	igb->last_msb = (ns >> (nbit - 2)) & 0x3;
+}
+
+/*
+ * SYSTIM / overflow access functions for the 82576
+ */
+
+static u64 igb_82576_systim_read(struct igb_adapter *igb)
+{
+	u64 ns;
+	u32 lo, hi;
+	struct e1000_hw *hw = &igb->hw;
+
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	ns = ((u64) hi) << 32;
+	ns |= lo;
+	ns >>= NS_SHIFT_82576;
+
+	ns = igb_overflow_get(igb, ns, OFL_SHIFT_82576);
+
+	return ns;
+}
+
+static void igb_82576_systim_write(struct igb_adapter *igb, u64 ns)
+{
+	u32 hi, lo;
+	struct e1000_hw *hw = &igb->hw;
+
+	hi = (ns >> 13) & 0xffffffff;
+	lo = (ns & 0x1fff) << NS_SHIFT_82576;
+
+	wr32(E1000_SYSTIML, lo);
+	wr32(E1000_SYSTIMH, hi);
+
+	igb_overflow_set(igb, ns, OFL_SHIFT_82576);
+}
+
+/*
+ * SYSTIM / overflow access functions for the 82580
+ */
+
+static u64 igb_82580_systim_read(struct igb_adapter *igb)
+{
+	u64 ns;
+	u32 lo, hi, jk;
+	struct e1000_hw *hw = &igb->hw;
+
+	jk = rd32(E1000_SYSTIMR);
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	ns = ((u64) hi) << 32;
+	ns |= lo;
+
+	ns = igb_overflow_get(igb, ns, OFL_SHIFT_82580);
+
+	return ns;
+}
+
+static void igb_82580_systim_write(struct igb_adapter *igb, u64 ns)
+{
+	u32 hi, lo;
+	struct e1000_hw *hw = &igb->hw;
+
+	hi = ns >> 32;
+	lo = ns & 0xffffffff;
+
+	wr32(E1000_SYSTIMR, 0);
+	wr32(E1000_SYSTIML, lo);
+	wr32(E1000_SYSTIMH, hi & 0xff);
+
+	igb_overflow_set(igb, ns, OFL_SHIFT_82580);
+}
+
+/*
+ * SYSTIM / overflow register access functions
+ * Callers must hold tmreg_lock.
+ */
+
+static u64 igb_systim_read(struct igb_adapter *igb)
+{
+	switch (igb->hw.mac.type) {
+	case e1000_i350:
+	case e1000_82580:
+		return igb_82580_systim_read(igb);
+	case e1000_82576:
+		return igb_82576_systim_read(igb);
+	default:
+		return 0;
+	}
+}
+
+static void igb_systim_write(struct igb_adapter *igb, u64 ns)
+{
+	switch (igb->hw.mac.type) {
+	case e1000_i350:
+	case e1000_82580:
+		igb_82580_systim_write(igb, ns);
+		break;
+	case e1000_82576:
+		igb_82576_systim_write(igb, ns);
+		break;
+	default:
+		break;
+	}
+}
+
+/*
+ * PTP clock operations
+ */
+
+static int ptp_82576_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	u64 rate;
+	u32 incvalue;
+	int neg_adj = 0;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,

caps);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +18 -0

+	struct e1000_hw *hw = &igb->hw;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+	rate = ppb;
+	rate <<= 14;
+	rate = div_u64(rate, 1953125);
+
+	incvalue = 16 << IGB_82576_TSYNC_SHIFT;
+
+	if (neg_adj)
+		incvalue -= rate;
+	else
+		incvalue += rate;
+
+	wr32(E1000_TIMINCA, INCPERIOD_82576 | (incvalue &

INCVALUE_82576_MASK));

drivers/net/ethernet/intel/igb/igb_ptp.c new file +10 -0

+
+	return 0;
+}
+
+static int ptp_82580_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	u64 rate;
+	u32 inca;
+	int neg_adj = 0;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,

caps);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +24 -0

+	struct e1000_hw *hw = &igb->hw;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+	rate = ppb;
+	rate <<= 26;
+	rate = div_u64(rate, 1953125);
+
+	inca = rate & INCVALUE_MASK;
+	if (neg_adj)
+		inca |= ISGN;
+
+	wr32(E1000_TIMINCA, inca);
+
+	return 0;
+}
+
+static int igb_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	s64 now;
+	unsigned long flags;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,

caps);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +13 -0

+
+	spin_lock_irqsave(&igb->tmreg_lock, flags);
+
+	now = igb_systim_read(igb);
+	now += delta;
+	igb_systim_write(igb, now);
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	return 0;
+}
+
+static int igb_gettime(struct ptp_clock_info *ptp, struct timespec

*ts)

drivers/net/ethernet/intel/igb/igb_ptp.c new file +5 -0

+{
+	u64 ns;
+	u32 remainder;
+	unsigned long flags;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,

caps);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +14 -0

+
+	spin_lock_irqsave(&igb->tmreg_lock, flags);
+
+	ns = igb_systim_read(igb);
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	ts->tv_sec = div_u64_rem(ns, 1000000000, &remainder);
+	ts->tv_nsec = remainder;
+
+	return 0;
+}
+
+static int igb_settime(struct ptp_clock_info *ptp, const struct

timespec *ts)

drivers/net/ethernet/intel/igb/igb_ptp.c new file +4 -0

+{
+	u64 ns;
+	unsigned long flags;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,

caps);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +34 -0

+
+	ns = ts->tv_sec * 1000000000ULL;
+	ns += ts->tv_nsec;
+
+	spin_lock_irqsave(&igb->tmreg_lock, flags);
+
+	igb_systim_write(igb, ns);
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	return 0;
+}
+
+static int ptp_82576_enable(struct ptp_clock_info *ptp,
+			    struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ptp_82580_enable(struct ptp_clock_info *ptp,
+			    struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static void igb_overflow_check(struct work_struct *work)
+{
+	struct timespec ts;
+	struct igb_adapter *igb =
+		container_of(work, struct igb_adapter, overflow_work.work);
+
+	igb_gettime(&igb->caps, &ts);
+
+	pr_debug("igb overflow check at %ld.%09lu\n", ts.tv_sec,

ts.tv_nsec);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +52 -0

+
+	schedule_delayed_work(&igb->overflow_work, IGB_OVERFLOW_PERIOD);
+}
+
+void igb_ptp_init(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+
+	switch (hw->mac.type) {
+	case e1000_i350:
+	case e1000_82580:
+		adapter->caps.owner	= THIS_MODULE;
+		strcpy(adapter->caps.name, "igb-82580");
+		adapter->caps.max_adj	= 62499999;
+		adapter->caps.n_ext_ts	= 0;
+		adapter->caps.pps	= 0;
+		adapter->caps.adjfreq	= ptp_82580_adjfreq;
+		adapter->caps.adjtime	= igb_adjtime;
+		adapter->caps.gettime	= igb_gettime;
+		adapter->caps.settime	= igb_settime;
+		adapter->caps.enable	= ptp_82580_enable;
+		/* Enable the timer functions by clearing bit 31. */
+		wr32(E1000_TSAUXC, 0x0);
+		break;
+
+	case e1000_82576:
+		adapter->caps.owner	= THIS_MODULE;
+		strcpy(adapter->caps.name, "igb-82576");
+		adapter->caps.max_adj	= 1000000000;
+		adapter->caps.n_ext_ts	= 0;
+		adapter->caps.pps	= 0;
+		adapter->caps.adjfreq	= ptp_82576_adjfreq;
+		adapter->caps.adjtime	= igb_adjtime;
+		adapter->caps.gettime	= igb_gettime;
+		adapter->caps.settime	= igb_settime;
+		adapter->caps.enable	= ptp_82576_enable;
+		/* Dial the nominal frequency. */
+		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		break;
+
+	default:
+		adapter->ptp_clock = NULL;
+		return;
+	}
+
+	wrfl();
+
+	INIT_DELAYED_WORK(&adapter->overflow_work, igb_overflow_check);
+
+	spin_lock_init(&adapter->tmreg_lock);
+
+	schedule_delayed_work(&adapter->overflow_work,

IGB_OVERFLOW_PERIOD);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +7 -0

+
+#ifdef CONFIG_PTP_1588_CLOCK
+
+	adapter->ptp_clock = ptp_clock_register(&adapter->caps);
+	if (IS_ERR(adapter->ptp_clock)) {
+		adapter->ptp_clock = NULL;
+		dev_err(&adapter->pdev->dev, "ptp_clock_register

failed\n");

drivers/net/ethernet/intel/igb/igb_ptp.c new file +21 -1

+	} else
+		dev_info(&adapter->pdev->dev, "added PHC on %s\n",
+			 adapter->netdev->name);
+
+#endif /*CONFIG_PTP_1588_CLOCK*/
+}
+
+void igb_ptp_remove(struct igb_adapter *adapter)
+{
+	cancel_delayed_work_sync(&adapter->overflow_work);
+
+#ifdef CONFIG_PTP_1588_CLOCK
+
+	if (adapter->ptp_clock) {
+		ptp_clock_unregister(adapter->ptp_clock);
+		dev_info(&adapter->pdev->dev, "removed PHC on %s\n",
+			 adapter->netdev->name);
+	}
+
+#endif /*CONFIG_PTP_1588_CLOCK*/
+}
--

1.7.2.5

Is there a reason for not using the timecounter structure from the kernel? It is a layer beneath the timecompare code which is meant to handle this condition. As far as I can tell this issue is solved in the timecounter code. If it is not, then that should be a bug in the timecounter cyclecounter code. I don't know if this issue occurs in the timecounter structure because it handles the ns conversion differently.

Quoted 7 lines · Jacob E

Is there a reason for not using the timecounter structure from the kernel? It is a layer beneath the timecompare code which is meant to handle this condition. As far as I can tell this issue is solved in the timecounter code. If it is not, then that should be a bug in the timecounter cyclecounter code. I don't know if this issue occurs in the timecounter structure because it handles the ns conversion differently.

My only reason is that I am not sure that the timecounter code really does what we need. It might well work. Consider, though, that the 82580 register does not overflow in the usual way. The upper 24 bits are always zero.

What I wrote does the right thing, I think. However, duplicated effort is always bad, so can you show me how to change it?

-----Original Message-----
From: Richard Cochran [mailto:[email protected]]
Sent: Tuesday, January 10, 2012 12:45 AM
To: Keller, Jacob E
Cc: [email protected]; [email protected]; Kirsher, Jeffrey T; Ronciak, John; John Stultz; Thomas Gleixner
Subject: Re: [PATCH net-next V3 1/2] igb: add PTP Hardware Clock code

Quoted 7 lines · Jacob E

Is there a reason for not using the timecounter structure from the kernel? It is a layer beneath the timecompare code which is meant to handle this condition. As far as I can tell this issue is solved in the timecounter code. If it is not, then that should be a bug in the timecounter cyclecounter code. I don't know if this issue occurs in the timecounter structure because it handles the ns conversion differently.

My only reason is that I am not sure that the timecounter code really does what we need. It might well work. Consider, though, that the 82580 register does not overflow in the usual way. The upper 24 bits are always zero.

What I wrote does the right thing, I think. However, duplicated effort is always bad, so can you show me how to change it?

Thanks,
Richard

* ChangeLog
** V3
- Driver compiles even without CONFIG_PTP_1588_CLOCK.
- Timestamping always works, even when PTP code missing or fails.
** V2
- Fixed wrong bit shifting in the 82576 code.
- Explained the timestamp locking with a comment in the code.
- Preserved the comments from the original timecompare
implementation.
- Added an additional test within the overflow counter code to fix a race condition. Details of the problem are given in the commit message.

This patch series implements a PHC driver for the Intel 82576 and 82580 devices, as part of the igb driver.

The first patch adds the PHC driver code as a new source module but does not link it into the main igb driver. Because the system time counter is not so very wide, the code implements an overflow counter in software. Every read operation maintains the overflow counter, as does a "delayed work" watchdog. Only the base clock operations are implemented. The hardware does have some ancillary features, but these can be easily added later.

The second patch removes the timecompare code and links in the new functions.

I have tested the 82580 with good results. However, I don't have the 82576 and so would appreciate testing and feedback.

Thanks,
Richard

Quoted 12 lines · Richard Cochran

* ChangeLog
** V3
- Driver compiles even without CONFIG_PTP_1588_CLOCK.
- Timestamping always works, even when PTP code missing or fails.
** V2
- Fixed wrong bit shifting in the 82576 code.
- Explained the timestamp locking with a comment in the code.
- Preserved the comments from the original timecompare
implementation.
- Added an additional test within the overflow counter code to fix a race condition. Details of the problem are given in the commit message.

Thanks Richard, I will add this series to my queue.

Quoted 13 lines · Jeff Kirsher

Quoted 12 lines · Richard Cochran

* ChangeLog
** V3
- Driver compiles even without CONFIG_PTP_1588_CLOCK.
- Timestamping always works, even when PTP code missing or fails.
** V2
- Fixed wrong bit shifting in the 82576 code.
- Explained the timestamp locking with a comment in the code.
- Preserved the comments from the original timecompare
implementation.
- Added an additional test within the overflow counter code to fix a race condition. Details of the problem are given in the commit message.

...

Quoted 1 line

Thanks Richard, I will add this series to my queue.

Jeff,

Please hold off with these. I adapted the driver to use timecounter/cyclecounter as Jacob suggested, and the result is surely nicer. I will post a V4 after I finish testing it.

Thanks,
Richard