Skip to contentSkip to conversation

igb: ptp hardware clock

PATCH In review Richard Cochran 5 participants 16 messages Jan 21, 2012 – Jan 28, 2012

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v4 cover igb: ptp hardware clock

* ChangeLog
** V4
- Use standard kernel cyclecounter/timecounter infrastructure instead of a home grown counter overflow implementation.
** 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. Only the base clock operations are implemented. The hardware does have some ancillary features, but these can be easily added later.

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 uses the overflow counter from the cyclecounter/timecounter code. Every read operation maintains the overflow counter, as does a "delayed work" watchdog.

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

Richard Cochran (2):
igb: add PTP Hardware Clock code
igb: offer a PTP Hardware Clock instead of the timecompare method

 drivers/net/ethernet/intel/igb/Makefile   |    2 +-
 drivers/net/ethernet/intel/igb/igb.h      |   20 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |  167 +------------
 drivers/net/ethernet/intel/igb/igb_ptp.c  |  388 +++++++++++++++++++++++++++++
 4 files changed, 407 insertions(+), 170 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c
Signature · 1 line
1.7.2.5
#1 lore raw

PATCH v4 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 the counter is implemented using the standard cyclecounter/timecounter code.

---
 drivers/net/ethernet/intel/igb/igb.h     |    8 +
 drivers/net/ethernet/intel/igb/igb_ptp.c |  329 ++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c
drivers/net/ethernet/intel/igb/igb.h +8 -0
index 3d12e67..79f354b 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;
+	struct cyclecounter cc;
+	struct timecounter tc;
 };
 
 #define IGB_FLAG_HAS_MSI           (1 << 0)
drivers/net/ethernet/intel/igb/igb_ptp.c new file +329 -0

329 lines not on this page · raw message

Signature · 14 lines
1.7.2.5


------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired
#2 lore raw

RE: [PATCH net V4 1/2] igb: add PTP Hardware Clock code

Quoted 15 lines

-----Original Message-----
From: Richard Cochran [mailto:[email protected]]
Sent: Saturday, January 21, 2012 8:04 AM
To: [email protected]
Cc: [email protected]; Keller, Jacob E; Kirsher, Jeffrey T; Ronciak, John; John Stultz; Thomas Gleixner
Subject: [PATCH net V4 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.

Quoted 5 lines

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 the counter is implemented using the standard cyclecounter/timecounter code.

Looks good overall other than the comments/questions inlined below.

Quoted 165 lines
---
 drivers/net/ethernet/intel/igb/igb.h     |    8 +
 drivers/net/ethernet/intel/igb/igb_ptp.c |  329
++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c
drivers/net/ethernet/intel/igb/igb.h +8 -0
index 3d12e67..79f354b 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;
+	struct cyclecounter cc;
+	struct timecounter tc;
 };
 
 #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..a8dc3e5
@@ -0,0 +1,329 @@
+/*
+ * 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 +55 -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.
+ */
+
+#define IGB_OVERFLOW_PERIOD	(HZ * 60 * 9)
+#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 IGB_NBITS_82580		40
+
+/*
+ * SYSTIM read access for the 82576
+ */
+
+static cycle_t igb_82576_systim_read(const struct cyclecounter *cc)
+{
+	u64 val;
+	u32 lo, hi;
+	struct igb_adapter *igb = container_of(cc, struct igb_adapter,

cc);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +20 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	val = ((u64) hi) << 32;
+	val |= lo;
+
+	return val;
+}
+
+/*
+ * SYSTIM read access for the 82580
+ */
+
+static cycle_t igb_82580_systim_read(const struct cyclecounter *cc)
+{
+	u64 val;
+	u32 lo, hi, jk;
+	struct igb_adapter *igb = container_of(cc, struct igb_adapter,

cc);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +22 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	jk = rd32(E1000_SYSTIMR);
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	val = ((u64) hi) << 32;
+	val |= lo;
+
+	return val;
+}
+
+/*
+ * 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 line 126

Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +10 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+	rate = ppb;
+	rate <<= 14;
+	rate = div_u64(rate, 1953125);
+

So is the rate ppb accumulating? I was under the impression that it calculated from the current clock frequency so it would need to be applied to the current incvalue, not the base... Is this not the case? What was the intention of the ppb?

Quoted 158 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +8 -0
+	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 = timecounter_read(&igb->tc);
+	now += delta;
+	timecounter_init(&igb->tc, &igb->cc, 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 = timecounter_read(&igb->tc);
+
+	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);
+
+	timecounter_init(&igb->tc, &igb->cc, 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 +38 -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;
+		adapter->cc.read	= igb_82580_systim_read;
+		adapter->cc.mask	= CLOCKSOURCE_MASK(IGB_NBITS_82580);
+		adapter->cc.mult	= 1;
+		adapter->cc.shift	= 0;
+		/* 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;

drivers/net/ethernet/intel/igb/igb_ptp.c line 286

Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +10 -0
+		adapter->caps.settime	= igb_settime;
+		adapter->caps.enable	= ptp_82576_enable;
+		adapter->cc.read	= igb_82576_systim_read;
+		adapter->cc.mask	= CLOCKSOURCE_MASK(64);
+		adapter->cc.mult	= 1;
+		adapter->cc.shift	= IGB_82576_TSYNC_SHIFT;
+		/* Dial the nominal frequency. */
+		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		break;
+

It would be good to check whether the link speed has an impact on the SYSTIME register rate. I know on the 10 Gb parts the systime registers are driven by the DMA clock which is partitioned differently at slower speeds so the values have to be updated whenever the link speed changes..

Quoted 47 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +15 -0
+	default:
+		adapter->ptp_clock = NULL;
+		return;
+	}
+
+	wrfl();
+
+	timecounter_init(&adapter->tc, &adapter->cc,
+			 ktime_to_ns(ktime_get_real()));
+
+	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

#3 lore raw

Re: [PATCH net V4 1/2] igb: add PTP Hardware Clock code

Quoted 13 lines · Jacob E
Quoted 13 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +13 -0
+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);
+	struct e1000_hw *hw = &igb->hw;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+	rate = ppb;
Quoted 8 lines

drivers/net/ethernet/intel/igb/igb_ptp.c line 126

Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +3 -0
+	rate <<= 14;
+	rate = div_u64(rate, 1953125);
+

So is the rate ppb accumulating? I was under the impression that it calculated from the current clock frequency so it would need to be applied to the current incvalue, not the base... Is this not the case? What was the intention of the ppb?

The ppb is simply the desired rate offset in parts per billion. It is not a delta from the current offset, but rather fixed from the clock's nominal frequency. This comes from the NTP timex.freq field (but the unit here is ppb, timex.freq is ppm with a 16 bit fraction.)

If the clock servo is a typical PI controller for example, then the servo output already represents the accumulated offset.

Quoted 12 lines
Quoted 11 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +11 -0
+	incvalue = 16 << IGB_82576_TSYNC_SHIFT;
+
+	if (neg_adj)
+		incvalue -= rate;
+	else
+		incvalue += rate;
+
+	wr32(E1000_TIMINCA, INCPERIOD_82576 | (incvalue & INCVALUE_82576_MASK));
+
+	return 0;
+}
Quoted 10 lines

drivers/net/ethernet/intel/igb/igb_ptp.c line 286

Quoted 4 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +4 -0
+		/* Dial the nominal frequency. */
+		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		break;
+

It would be good to check whether the link speed has an impact on the SYSTIME register rate. I know on the 10 Gb parts the systime registers are driven by the DMA clock which is partitioned differently at slower speeds so the values have to be updated whenever the link speed changes..

If this is true for 82576, then it certainly is not documented. I don't have that card, so maybe someone from Intel can answer this?

I think the documentation for the 82580 is clear about the basic rate always being the same.

Thanks,
Richard

#4 lore raw

RE: [PATCH net V4 1/2] igb: add PTP Hardware Clock code

Quoted 67 lines

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

Quoted 6 lines · Jacob E

drivers/net/ethernet/intel/igb/igb_ptp.c

Quoted 6 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +6 -0
+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);

Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +7 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+	rate = ppb;
Quoted 8 lines

drivers/net/ethernet/intel/igb/igb_ptp.c line 126

Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +3 -0
+	rate <<= 14;
+	rate = div_u64(rate, 1953125);
+

So is the rate ppb accumulating? I was under the impression that it calculated from the current clock frequency so it would need to be applied to the current incvalue, not the base... Is this not the case? What was the intention of the ppb?

The ppb is simply the desired rate offset in parts per billion. It is not a delta from the current offset, but rather fixed from the clock's nominal frequency. This comes from the NTP timex.freq field (but the unit here is ppb, timex.freq is ppm with a 16 bit fraction.)

If the clock servo is a typical PI controller for example, then the servo output already represents the accumulated offset.

Quoted 8 lines

drivers/net/ethernet/intel/igb/igb_ptp.c

Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +8 -0
+	incvalue = 16 << IGB_82576_TSYNC_SHIFT;
+
+	if (neg_adj)
+		incvalue -= rate;
+	else
+		incvalue += rate;
+
+	wr32(E1000_TIMINCA, INCPERIOD_82576 | (incvalue &

INCVALUE_82576_MASK));

Quoted 4 lines
Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +3 -0
+
+	return 0;
+}
Quoted 10 lines

drivers/net/ethernet/intel/igb/igb_ptp.c line 286

Quoted 4 lines
drivers/net/ethernet/intel/igb/igb_ptp.c new file +4 -0
+		/* Dial the nominal frequency. */
+		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		break;
+

It would be good to check whether the link speed has an impact on the SYSTIME register rate. I know on the 10 Gb parts the systime registers are driven by the DMA clock which is partitioned differently at slower speeds so the values have to be updated whenever the link speed changes..

Quoted 3 lines

If this is true for 82576, then it certainly is not documented. I don't have that card, so maybe someone from Intel can answer this?

I'll look into this.

Quoted 2 lines

I think the documentation for the 82580 is clear about the basic rate always being the same.

Yeah, I believe the 82580 stays the same regardless of the device link speed.

Quoted 3 lines

Thanks,
Richard

#5 lore raw

RE: [PATCH net V4 1/2] igb: add PTP Hardware Clock code

In reply to Richard Cochran

Have you tested this with ptp support configured as a module instead of directly compiled into the kernel? My experience in the past has been that the CONFIG_PTP_1588_CLOCK flag is not defined when support is not directly compiled into the kernel. (A different flag, which signifies module vs compiled) is defined. I would like to know the results, Thanks.

Quoted 408 lines

-----Original Message-----
From: Richard Cochran [mailto:[email protected]]
Sent: Saturday, January 21, 2012 8:04 AM
To: [email protected]
Cc: [email protected]; Keller, Jacob E; Kirsher, Jeffrey T; Ronciak, John; John Stultz; Thomas Gleixner
Subject: [PATCH net V4 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 the counter is implemented using the standard cyclecounter/timecounter code.

---
 drivers/net/ethernet/intel/igb/igb.h     |    8 +
 drivers/net/ethernet/intel/igb/igb_ptp.c |  329
++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c
drivers/net/ethernet/intel/igb/igb.h +8 -0
index 3d12e67..79f354b 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;
+	struct cyclecounter cc;
+	struct timecounter tc;
 };
 
 #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..a8dc3e5
@@ -0,0 +1,329 @@
+/*
+ * 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 +55 -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.
+ */
+
+#define IGB_OVERFLOW_PERIOD	(HZ * 60 * 9)
+#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 IGB_NBITS_82580		40
+
+/*
+ * SYSTIM read access for the 82576
+ */
+
+static cycle_t igb_82576_systim_read(const struct cyclecounter *cc)
+{
+	u64 val;
+	u32 lo, hi;
+	struct igb_adapter *igb = container_of(cc, struct igb_adapter,

cc);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +20 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	val = ((u64) hi) << 32;
+	val |= lo;
+
+	return val;
+}
+
+/*
+ * SYSTIM read access for the 82580
+ */
+
+static cycle_t igb_82580_systim_read(const struct cyclecounter *cc)
+{
+	u64 val;
+	u32 lo, hi, jk;
+	struct igb_adapter *igb = container_of(cc, struct igb_adapter,

cc);

drivers/net/ethernet/intel/igb/igb_ptp.c new file +22 -0
+	struct e1000_hw *hw = &igb->hw;
+
+	jk = rd32(E1000_SYSTIMR);
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	val = ((u64) hi) << 32;
+	val |= lo;
+
+	return val;
+}
+
+/*
+ * 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 = timecounter_read(&igb->tc);
+	now += delta;
+	timecounter_init(&igb->tc, &igb->cc, 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 = timecounter_read(&igb->tc);
+
+	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);
+
+	timecounter_init(&igb->tc, &igb->cc, 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 +63 -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;
+		adapter->cc.read	= igb_82580_systim_read;
+		adapter->cc.mask	= CLOCKSOURCE_MASK(IGB_NBITS_82580);
+		adapter->cc.mult	= 1;
+		adapter->cc.shift	= 0;
+		/* 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;
+		adapter->cc.read	= igb_82576_systim_read;
+		adapter->cc.mask	= CLOCKSOURCE_MASK(64);
+		adapter->cc.mult	= 1;
+		adapter->cc.shift	= IGB_82576_TSYNC_SHIFT;
+		/* Dial the nominal frequency. */
+		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		break;
+
+	default:
+		adapter->ptp_clock = NULL;
+		return;
+	}
+
+	wrfl();
+
+	timecounter_init(&adapter->tc, &adapter->cc,
+			 ktime_to_ns(ktime_get_real()));
+
+	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

#6 lore raw

PATCH v4 2/2 igb: offer a PTP Hardware Clock instead of the timecompare method

In reply to Richard Cochran

This commit removes the legacy timecompare code from the igb driver and offers a tunable PHC instead.

---
 drivers/net/ethernet/intel/igb/Makefile   |    2 +-
 drivers/net/ethernet/intel/igb/igb.h      |   12 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |  167 +----------------------------
 drivers/net/ethernet/intel/igb/igb_ptp.c  |   59 ++++++++++
 4 files changed, 70 insertions(+), 170 deletions(-)
drivers/net/ethernet/intel/igb/Makefile +1 -1
index c6e4621..42f0868 100644
@@ -32,6 +32,6 @@
 
 obj-$(CONFIG_IGB) += igb.o
 
-igb-objs := igb_main.o igb_ethtool.o e1000_82575.o \
+igb-objs := igb_main.o igb_ethtool.o igb_ptp.o e1000_82575.o \
 	    e1000_mac.o e1000_nvm.o e1000_phy.o e1000_mbx.o
 
drivers/net/ethernet/intel/igb/igb.h +7 -5
index 79f354b..cd08f73 100644
@@ -35,7 +35,6 @@
 #include "e1000_82575.h"
 
 #include <linux/clocksource.h>
-#include <linux/timecompare.h>
 #include <linux/net_tstamp.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/bitops.h>
@@ -329,9 +328,6 @@ struct igb_adapter {
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
-	struct cyclecounter cycles;
-	struct timecounter clock;
-	struct timecompare compare;
 	struct hwtstamp_config hwtstamp_config;
 
 	spinlock_t stats64_lock;
@@ -386,7 +382,6 @@ struct igb_adapter {
 #define IGB_DMCTLX_DCFLUSH_DIS     0x80000000  /* Disable DMA Coal Flush */
 
 #define IGB_82576_TSYNC_SHIFT 19
-#define IGB_82580_TSYNC_SHIFT 24
 #define IGB_TS_HDR_LEN        16
 enum e1000_state_t {
 	__IGB_TESTING,
@@ -423,6 +418,13 @@ extern bool igb_has_link(struct igb_adapter *adapter);
 extern void igb_set_ethtool_ops(struct net_device *);
 extern void igb_power_up_link(struct igb_adapter *);
 
+extern void igb_ptp_init(struct igb_adapter *adapter);
+extern void igb_ptp_remove(struct igb_adapter *adapter);
+
+extern void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
+				   struct skb_shared_hwtstamps *hwtstamps,
+				   u64 systim);
+
 static inline s32 igb_reset_phy(struct e1000_hw *hw)
 {
 	if (hw->phy.ops.reset)
drivers/net/ethernet/intel/igb/igb_main.c +3 -164

203 lines not on this page · raw message

drivers/net/ethernet/intel/igb/igb_ptp.c +59 -0

80 lines not on this page · raw message

Signature · 14 lines
1.7.2.5


------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired
#7 lore raw
In reply to Richard Cochran
Quoted 40 lines · Richard Cochran

* ChangeLog
** V4
- Use standard kernel cyclecounter/timecounter infrastructure instead of a home grown counter overflow implementation.
** 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. Only the base clock operations are implemented. The hardware does have some ancillary features, but these can be easily added later.

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 uses the overflow counter from the cyclecounter/timecounter code. Every read operation maintains the overflow counter, as does a "delayed work" watchdog.

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

Richard Cochran (2):
igb: add PTP Hardware Clock code
igb: offer a PTP Hardware Clock instead of the timecompare method

Quoted 7 lines
 drivers/net/ethernet/intel/igb/Makefile   |    2 +-
 drivers/net/ethernet/intel/igb/igb.h      |   20 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |  167 +------------
 drivers/net/ethernet/intel/igb/igb_ptp.c  |  388 +++++++++++++++++++++++++++++
 4 files changed, 407 insertions(+), 170 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c

Thanks Richard, I have added the patches to my queue.

#8 lore raw
In reply to Richard Cochran
Quoted 40 lines · Richard Cochran

* ChangeLog
** V4
- Use standard kernel cyclecounter/timecounter infrastructure instead of a home grown counter overflow implementation.
** 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. Only the base clock operations are implemented. The hardware does have some ancillary features, but these can be easily added later.

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 uses the overflow counter from the cyclecounter/timecounter code. Every read operation maintains the overflow counter, as does a "delayed work" watchdog.

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

Richard Cochran (2):
igb: add PTP Hardware Clock code
igb: offer a PTP Hardware Clock instead of the timecompare method

Quoted 7 lines
 drivers/net/ethernet/intel/igb/Makefile   |    2 +-
 drivers/net/ethernet/intel/igb/igb.h      |   20 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |  167 +------------
 drivers/net/ethernet/intel/igb/igb_ptp.c  |  388 +++++++++++++++++++++++++++++
 4 files changed, 407 insertions(+), 170 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_ptp.c

Added Aaron Brown to the thread

Richard-

Did you compile test? Why I ask is our validation team is getting the following errors when trying to compile in PTP support:

---------------------------
drivers/net/ethernet/intel/igb/igb_ptp.c: In function âigb_ptp_initâ:
drivers/net/ethernet/intel/igb/igb_ptp.c:266: error: âTHIS_MODULEâ undeclared (first use in this function) drivers/net/ethernet/intel/igb/igb_ptp.c:266: error: (Each undeclared identifier is reported only once drivers/net/ethernet/intel/igb/igb_ptp.c:266: error: for each function it appears in.) make[5]: *** [drivers/net/ethernet/intel/igb/igb_ptp.o] Error 1
make[4]: *** [drivers/net/ethernet/intel/igb] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [drivers/net/ethernet/intel] Error 2
make[2]: *** [drivers/net/ethernet] Error 2
make[1]: *** [drivers/net] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2
-------------------------------

Cheers,
Jeff

#9 lore raw
Quoted 4 lines · Jeff Kirsher

Richard-

Did you compile test? Why I ask is our validation team is getting the following errors when trying to compile in PTP support:

I did compile and also tested the driver.

What .config are you using? What kernel version?

Thanks,
Richard

#10 lore raw
Quoted 6 lines · Richard Cochran
Quoted 4 lines · Jeff Kirsher

Richard-

Did you compile test? Why I ask is our validation team is getting the following errors when trying to compile in PTP support:

Quoted 6 lines

I did compile and also tested the driver.

What .config are you using? What kernel version?

Thanks,
Richard

I will have Aaron post his .config's that he tried. I know that he tried multiple configs with no success. As far as kernel version, he was using the latest net-next tree from David Miller.

#11 lore raw
Quoted 9 lines · Jeff Kirsher
Quoted 8 lines · Richard Cochran
Quoted 4 lines · Jeff Kirsher

Richard-

Did you compile test? Why I ask is our validation team is getting the following errors when trying to compile in PTP support:

I did compile and also tested the driver.

Quoted 8 lines
Quoted 4 lines

What .config are you using? What kernel version?

Thanks,
Richard

I will have Aaron post his .config's that he tried. I know that he tried multiple configs with no success. As far as kernel version, he was using the latest net-next tree from David Miller.

This might coming from

commit 36a1211970193ce215de50ed1e4e1272bc814df1

Author: Paul Gortmaker <[email protected]>
Date:   Tue Jan 24 11:33:19 2012 +0000

netprio_cgroup.h: dont include module.h from other includes

A considerable effort was invested in wiping out module.h from being present in all the other standard includes. This one leaked back in, but once again isn't strictly necessary, so remove it.

#12 lore raw
Quoted 25 lines · Eric Dumazet
Quoted 9 lines · Jeff Kirsher
Quoted 8 lines · Richard Cochran
Quoted 4 lines · Jeff Kirsher

Richard-

Did you compile test? Why I ask is our validation team is getting the following errors when trying to compile in PTP support:

I did compile and also tested the driver.

Quoted 8 lines
Quoted 4 lines

What .config are you using? What kernel version?

Thanks,
Richard

I will have Aaron post his .config's that he tried. I know that he tried multiple configs with no success. As far as kernel version, he was using the latest net-next tree from David Miller.

This might coming from

commit 36a1211970193ce215de50ed1e4e1272bc814df1

Author: Paul Gortmaker <[email protected]>
Date:   Tue Jan 24 11:33:19 2012 +0000
Quoted 9 lines

netprio_cgroup.h: dont include module.h from other includes

A considerable effort was invested in wiping out module.h from being present in all the other standard includes. This one leaked back in, but once again isn't strictly necessary, so remove it.

Eric was correct. I added #include <linux/mdoule.h> to the igb_ptp.c file and the error goes away.

Richard are you fine with me adding that change to your first patch of the series? That way I can have the validation team finish up testing your patches.

#13 lore raw
Quoted 6 lines · Jeff Kirsher

Eric was correct. I added #include <linux/mdoule.h> to the igb_ptp.c file and the error goes away.

Richard are you fine with me adding that change to your first patch of the series? That way I can have the validation team finish up testing your patches.

By all means, please go right ahead.

Thanks,
Richard

#14 lore raw
Quoted 12 lines

From: Richard Cochran [mailto:[email protected]]
Sent: Friday, January 27, 2012 4:12 AM
To: Kirsher, Jeffrey T
Cc: Eric Dumazet; Brown, Aaron F; [email protected]; e1000- [email protected]; Keller, Jacob E; Ronciak, John; John Stultz; Thomas Gleixner
Subject: Re: [PATCH net V4 0/2] igb: ptp hardware clock

Quoted 3 lines · Jeff Kirsher

Eric was correct. I added #include <linux/mdoule.h> to the igb_ptp.c file and the error goes away.

Quoted 9 lines
Quoted 3 lines

Richard are you fine with me adding that change to your first patch of the series? That way I can have the validation team finish up testing your patches.

By all means, please go right ahead.

Thanks,
Richard

Yes, that does indeed resolve the compilation issue I was seeing in the lab, thanks... I'm assuming a set of .config files that were failing is no longer necessary.

#15 lore raw
Quoted 3 lines · Aaron F

Yes, that does indeed resolve the compilation issue I was seeing in the lab, thanks... I'm assuming a set of .config files that were failing is no longer necessary.

Right, don't need them anymore.

Thanks,
Richard