[PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.

[PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.

[Richard Cochran]

This patch series adds three features: time stamping external events,
producing a periodic output signal, and an internal PPS event. The
i210 PCIe card has a 6 pin header with SDP0-3, making it easy to try
out this new functionality.

An earlier version of this series was posted way back in May, 2013,
but that version lacked a good way to assign functions to the pins. In
the mean time, the PHC core has a standard method to configure the
pins, and this series makes use of it.

Thanks,
Richard

Richard Cochran (4):
  igb: refactor time sync interrupt handling
  igb: do not clobber the TSAUXC bits on reset.
  igb: enable internal PPS for the i210.
  igb: enable auxiliary PHC functions for the i210.

 drivers/net/ethernet/intel/igb/igb.h      |    9 ++
 drivers/net/ethernet/intel/igb/igb_main.c |  100 ++++++++----
 drivers/net/ethernet/intel/igb/igb_ptp.c  |  251 ++++++++++++++++++++++++++++-
 3 files changed, 326 insertions(+), 34 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/4] igb: refactor time sync interrupt handling

[Richard Cochran]

The code that handles the time sync interrupt is repeated in three
different places. This patch refactors the identical code blocks into
a single helper function.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |   49 +++++++++++------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b0e12e7..7183a56 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5383,6 +5383,19 @@ void igb_update_stats(struct igb_adapter *adapter,
 	}
 }
 
+static void igb_tsync_interrupt(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 tsicr = rd32(E1000_TSICR);
+
+	if (tsicr & E1000_TSICR_TXTS) {
+		/* acknowledge the interrupt */
+		wr32(E1000_TSICR, E1000_TSICR_TXTS);
+		/* retrieve hardware timestamp */
+		schedule_work(&adapter->ptp_tx_work);
+	}
+}
+
 static irqreturn_t igb_msix_other(int irq, void *data)
 {
 	struct igb_adapter *adapter = data;
@@ -5414,16 +5427,8 @@ static irqreturn_t igb_msix_other(int irq, void *data)
 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
 	}
 
-	if (icr & E1000_ICR_TS) {
-		u32 tsicr = rd32(E1000_TSICR);
-
-		if (tsicr & E1000_TSICR_TXTS) {
-			/* acknowledge the interrupt */
-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
-			/* retrieve hardware timestamp */
-			schedule_work(&adapter->ptp_tx_work);
-		}
-	}
+	if (icr & E1000_ICR_TS)
+		igb_tsync_interrupt(adapter);
 
 	wr32(E1000_EIMS, adapter->eims_other);
 
@@ -6202,16 +6207,8 @@ static irqreturn_t igb_intr_msi(int irq, void *data)
 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
 	}
 
-	if (icr & E1000_ICR_TS) {
-		u32 tsicr = rd32(E1000_TSICR);
-
-		if (tsicr & E1000_TSICR_TXTS) {
-			/* acknowledge the interrupt */
-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
-			/* retrieve hardware timestamp */
-			schedule_work(&adapter->ptp_tx_work);
-		}
-	}
+	if (icr & E1000_ICR_TS)
+		igb_tsync_interrupt(adapter);
 
 	napi_schedule(&q_vector->napi);
 
@@ -6256,16 +6253,8 @@ static irqreturn_t igb_intr(int irq, void *data)
 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
 	}
 
-	if (icr & E1000_ICR_TS) {
-		u32 tsicr = rd32(E1000_TSICR);
-
-		if (tsicr & E1000_TSICR_TXTS) {
-			/* acknowledge the interrupt */
-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
-			/* retrieve hardware timestamp */
-			schedule_work(&adapter->ptp_tx_work);
-		}
-	}
+	if (icr & E1000_ICR_TS)
+		igb_tsync_interrupt(adapter);
 
 	napi_schedule(&q_vector->napi);
 
-- 
1.7.10.4

[PATCH net-next 2/4] igb: do not clobber the TSAUXC bits on reset.

[Richard Cochran]

The TSAUXC register has a number of different bits, one of which disables
the main clock function. Previously, the clock was re-enabled by clearing
the entire register. This patch changes the code to preserve the values
of the other bits in that register.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 794c139..ce57128 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -905,6 +905,8 @@ void igb_ptp_stop(struct igb_adapter *adapter)
 void igb_ptp_reset(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
+	unsigned long flags;
+	u32 tsauxc;
 
 	if (!(adapter->flags & IGB_FLAG_PTP))
 		return;
@@ -923,7 +925,11 @@ void igb_ptp_reset(struct igb_adapter *adapter)
 	case e1000_i210:
 	case e1000_i211:
 		/* Enable the timer functions and interrupts. */
-		wr32(E1000_TSAUXC, 0x0);
+		spin_lock_irqsave(&adapter->tmreg_lock, flags);
+		tsauxc = rd32(E1000_TSAUXC);
+		tsauxc &= ~TSAUXC_DISABLE;
+		wr32(E1000_TSAUXC, tsauxc);
+		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
 		wr32(E1000_TSIM, TSYNC_INTERRUPTS);
 		wr32(E1000_IMS, E1000_IMS_TS);
 		break;
-- 
1.7.10.4

[PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Richard Cochran]

The i210 device can produce an interrupt on the full second. This
patch allows using this interrupt to generate an internal PPS event
for adjusting the kernel system time.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |    6 ++++++
 drivers/net/ethernet/intel/igb/igb_ptp.c  |   32 +++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 7183a56..9412661 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
 static void igb_tsync_interrupt(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
+	struct ptp_clock_event event;
 	u32 tsicr = rd32(E1000_TSICR);
 
+	if (tsicr & TSINTR_SYS_WRAP) {
+		event.type = PTP_CLOCK_PPS;
+		ptp_clock_event(adapter->ptp_clock, &event);
+	}
+
 	if (tsicr & E1000_TSICR_TXTS) {
 		/* acknowledge the interrupt */
 		wr32(E1000_TSICR, E1000_TSICR_TXTS);
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index ce57128..70d3933 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -360,6 +360,34 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
+				       struct ptp_clock_request *rq, int on)
+{
+	struct igb_adapter *igb =
+		container_of(ptp, struct igb_adapter, ptp_caps);
+	struct e1000_hw *hw = &igb->hw;
+	unsigned long flags;
+	u32 tsim;
+
+	switch (rq->type) {
+	case PTP_CLK_REQ_PPS:
+		spin_lock_irqsave(&igb->tmreg_lock, flags);
+		tsim = rd32(E1000_TSIM);
+		if (on)
+			tsim |= TSINTR_SYS_WRAP;
+		else
+			tsim &= ~TSINTR_SYS_WRAP;
+		wr32(E1000_TSIM, tsim);
+		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+		return 0;
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
 				  struct ptp_clock_request *rq, int on)
 {
@@ -802,12 +830,12 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.owner = THIS_MODULE;
 		adapter->ptp_caps.max_adj = 62499999;
 		adapter->ptp_caps.n_ext_ts = 0;
-		adapter->ptp_caps.pps = 0;
+		adapter->ptp_caps.pps = 1;
 		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
 		adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
 		adapter->ptp_caps.settime = igb_ptp_settime_i210;
-		adapter->ptp_caps.enable = igb_ptp_feature_enable;
+		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
 		/* Enable the timer functions by clearing bit 31. */
 		wr32(E1000_TSAUXC, 0x0);
 		break;
-- 
1.7.10.4

[PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.

[Richard Cochran]

The i210 device offers a number of special PTP Hardware Clock features on
the Software Defined Pins (SDPs). This patch adds support for two of the
possible functions, namely time stamping external events, and periodic
output signals.

The assignment of PHC functions to the four SDP can be freely chosen by
the user.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb.h      |    9 ++
 drivers/net/ethernet/intel/igb/igb_main.c |   47 ++++++-
 drivers/net/ethernet/intel/igb/igb_ptp.c  |  219 ++++++++++++++++++++++++++++-
 3 files changed, 269 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 82d891e..f6aca7c 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -343,6 +343,9 @@ struct hwmon_buff {
 	};
 #endif
 
+#define IGB_N_EXTTS	2
+#define IGB_N_PEROUT	2
+#define IGB_N_SDP	4
 #define IGB_RETA_SIZE	128
 
 /* board specific private data structure */
@@ -439,6 +442,12 @@ struct igb_adapter {
 	u32 tx_hwtstamp_timeouts;
 	u32 rx_hwtstamp_cleared;
 
+	struct ptp_pin_desc sdp_config[IGB_N_SDP];
+	struct {
+		struct timespec start;
+		struct timespec period;
+	} perout[IGB_N_PEROUT];
+
 	char fw_version[32];
 #ifdef CONFIG_IGB_HWMON
 	struct hwmon_buff *igb_hwmon_buff;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 9412661..3a25661 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	struct ptp_clock_event event;
-	u32 tsicr = rd32(E1000_TSICR);
+	struct timespec ts;
+	u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
 
 	if (tsicr & TSINTR_SYS_WRAP) {
 		event.type = PTP_CLOCK_PPS;
@@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
 		/* retrieve hardware timestamp */
 		schedule_work(&adapter->ptp_tx_work);
 	}
+
+	if (tsicr & TSINTR_TT0) {
+		spin_lock(&adapter->tmreg_lock);
+		ts = timespec_add(adapter->perout[0].start,
+				  adapter->perout[0].period);
+		wr32(E1000_TRGTTIML0, ts.tv_nsec);
+		wr32(E1000_TRGTTIMH0, ts.tv_sec);
+		tsauxc = rd32(E1000_TSAUXC);
+		tsauxc |= TSAUXC_EN_TT0;
+		wr32(E1000_TSAUXC, tsauxc);
+		adapter->perout[0].start = ts;
+		spin_unlock(&adapter->tmreg_lock);
+	}
+
+	if (tsicr & TSINTR_TT1) {
+		spin_lock(&adapter->tmreg_lock);
+		ts = timespec_add(adapter->perout[1].start,
+				  adapter->perout[1].period);
+		wr32(E1000_TRGTTIML1, ts.tv_nsec);
+		wr32(E1000_TRGTTIMH1, ts.tv_sec);
+		tsauxc = rd32(E1000_TSAUXC);
+		tsauxc |= TSAUXC_EN_TT1;
+		wr32(E1000_TSAUXC, tsauxc);
+		adapter->perout[1].start = ts;
+		spin_unlock(&adapter->tmreg_lock);
+	}
+
+	if (tsicr & TSINTR_AUTT0) {
+		nsec = rd32(E1000_AUXSTMPL0);
+		sec  = rd32(E1000_AUXSTMPH0);
+		event.type = PTP_CLOCK_EXTTS;
+		event.index = 0;
+		event.timestamp = sec * 1000000000ULL + nsec;
+		ptp_clock_event(adapter->ptp_clock, &event);
+	}
+
+	if (tsicr & TSINTR_AUTT1) {
+		nsec = rd32(E1000_AUXSTMPL1);
+		sec  = rd32(E1000_AUXSTMPH1);
+		event.type = PTP_CLOCK_EXTTS;
+		event.index = 1;
+		event.timestamp = sec * 1000000000ULL + nsec;
+		ptp_clock_event(adapter->ptp_clock, &event);
+	}
 }
 
 static irqreturn_t igb_msix_other(int irq, void *data)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 70d3933..37b9fe6 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
+{
+	u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
+	u32 mask[IGB_N_SDP] = {
+		E1000_CTRL_SDP0_DIR,
+		E1000_CTRL_SDP1_DIR,
+		E1000_CTRL_EXT_SDP2_DIR,
+		E1000_CTRL_EXT_SDP3_DIR,
+	};
+
+	if (input)
+		*ptr &= ~mask[pin];
+	else
+		*ptr |= mask[pin];
+}
+
+static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
+{
+	struct e1000_hw *hw = &igb->hw;
+	u32 aux0_sel_sdp[IGB_N_SDP] = {
+		AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+	};
+	u32 aux1_sel_sdp[IGB_N_SDP] = {
+		AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+	};
+	u32 ts_sdp_en[IGB_N_SDP] = {
+		TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+	};
+	u32 ctrl, ctrl_ext, tssdp = 0;
+
+	ctrl = rd32(E1000_CTRL);
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	tssdp = rd32(E1000_TSSDP);
+
+	igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
+
+	/* Make sure this pin is not enabled as an ouput. */
+	tssdp &= ~ts_sdp_en[pin];
+
+	if (chan == 1) {
+		tssdp &= ~AUX1_SEL_SDP3;
+		tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
+	} else {
+		tssdp &= ~AUX0_SEL_SDP3;
+		tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
+	}
+
+	wr32(E1000_TSSDP, tssdp);
+	wr32(E1000_CTRL, ctrl);
+	wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
+static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
+{
+	struct e1000_hw *hw = &igb->hw;
+	u32 aux0_sel_sdp[IGB_N_SDP] = {
+		AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+	};
+	u32 aux1_sel_sdp[IGB_N_SDP] = {
+		AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+	};
+	u32 ts_sdp_en[IGB_N_SDP] = {
+		TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+	};
+	u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
+		TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
+		TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
+	};
+	u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
+		TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
+		TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
+	};
+	u32 ts_sdp_sel_clr[IGB_N_SDP] = {
+		TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
+		TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
+	};
+	u32 ctrl, ctrl_ext, tssdp = 0;
+
+	ctrl = rd32(E1000_CTRL);
+	ctrl_ext = rd32(E1000_CTRL_EXT);
+	tssdp = rd32(E1000_TSSDP);
+
+	igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
+
+	/* Make sure this pin is not enabled as an input. */
+	if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
+		tssdp &= ~AUX0_TS_SDP_EN;
+
+	if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
+		tssdp &= ~AUX1_TS_SDP_EN;
+
+	tssdp &= ~ts_sdp_sel_clr[pin];
+	if (chan == 1)
+		tssdp |= ts_sdp_sel_tt1[pin];
+	else
+		tssdp |= ts_sdp_sel_tt0[pin];
+
+	tssdp |= ts_sdp_en[pin];
+
+	wr32(E1000_TSSDP, tssdp);
+	wr32(E1000_CTRL, ctrl);
+	wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
 static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
 				       struct ptp_clock_request *rq, int on)
 {
 	struct igb_adapter *igb =
 		container_of(ptp, struct igb_adapter, ptp_caps);
 	struct e1000_hw *hw = &igb->hw;
+	u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
 	unsigned long flags;
-	u32 tsim;
+	struct timespec ts;
+	int pin;
+	s64 ns;
 
 	switch (rq->type) {
+	case PTP_CLK_REQ_EXTTS:
+		if (on) {
+			pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
+					   rq->extts.index);
+			if (pin < 0)
+				return -EBUSY;
+			igb_pin_extts(igb, rq->extts.index, pin);
+		}
+		if (rq->extts.index == 1) {
+			tsauxc_mask = TSAUXC_EN_TS1;
+			tsim_mask = TSINTR_AUTT1;
+		} else {
+			tsauxc_mask = TSAUXC_EN_TS0;
+			tsim_mask = TSINTR_AUTT0;
+		}
+		spin_lock_irqsave(&igb->tmreg_lock, flags);
+		tsauxc = rd32(E1000_TSAUXC);
+		tsim = rd32(E1000_TSIM);
+		if (on) {
+			tsauxc |= tsauxc_mask;
+			tsim |= tsim_mask;
+		} else {
+			tsauxc &= ~tsauxc_mask;
+			tsim &= ~tsim_mask;
+		}
+		wr32(E1000_TSAUXC, tsauxc);
+		wr32(E1000_TSIM, tsim);
+		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+		return 0;
+
+	case PTP_CLK_REQ_PEROUT:
+		if (on) {
+			pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
+					   rq->perout.index);
+			if (pin < 0)
+				return -EBUSY;
+			igb_pin_perout(igb, rq->perout.index, pin);
+		}
+		ts.tv_sec = rq->perout.period.sec;
+		ts.tv_nsec = rq->perout.period.nsec;
+		ns = timespec_to_ns(&ts);
+		ns = ns >> 1;
+		if (on && ns < 500000LL) {
+			/* 2k interrupts per second is an awful lot. */
+			return -EINVAL;
+		}
+		ts = ns_to_timespec(ns);
+		if (rq->perout.index == 1) {
+			tsauxc_mask = TSAUXC_EN_TT1;
+			tsim_mask = TSINTR_TT1;
+			trgttiml = E1000_TRGTTIML1;
+			trgttimh = E1000_TRGTTIMH1;
+		} else {
+			tsauxc_mask = TSAUXC_EN_TT0;
+			tsim_mask = TSINTR_TT0;
+			trgttiml = E1000_TRGTTIML0;
+			trgttimh = E1000_TRGTTIMH0;
+		}
+		spin_lock_irqsave(&igb->tmreg_lock, flags);
+		tsauxc = rd32(E1000_TSAUXC);
+		tsim = rd32(E1000_TSIM);
+		if (on) {
+			int i = rq->perout.index;
+			igb->perout[i].start.tv_sec = rq->perout.start.sec;
+			igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
+			igb->perout[i].period.tv_sec = ts.tv_sec;
+			igb->perout[i].period.tv_nsec = ts.tv_nsec;
+			wr32(trgttiml, rq->perout.start.sec);
+			wr32(trgttimh, rq->perout.start.nsec);
+			tsauxc |= tsauxc_mask;
+			tsim |= tsim_mask;
+		} else {
+			tsauxc &= ~tsauxc_mask;
+			tsim &= ~tsim_mask;
+		}
+		wr32(E1000_TSAUXC, tsauxc);
+		wr32(E1000_TSIM, tsim);
+		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+		return 0;
+
 	case PTP_CLK_REQ_PPS:
 		spin_lock_irqsave(&igb->tmreg_lock, flags);
 		tsim = rd32(E1000_TSIM);
@@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
 		wr32(E1000_TSIM, tsim);
 		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
 		return 0;
-
-	default:
-		break;
 	}
 
 	return -EOPNOTSUPP;
@@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
+			      enum ptp_pin_function func, unsigned int chan)
+{
+	switch (func) {
+	case PTP_PF_NONE:
+	case PTP_PF_EXTTS:
+	case PTP_PF_PEROUT:
+		break;
+	case PTP_PF_PHYSYNC:
+		return -1;
+	}
+	return 0;
+}
+
 /**
  * igb_ptp_tx_work
  * @work: pointer to work struct
@@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	struct net_device *netdev = adapter->netdev;
+	int i;
 
 	switch (hw->mac.type) {
 	case e1000_82576:
@@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		break;
 	case e1000_i210:
 	case e1000_i211:
+		for (i = 0; i < IGB_N_SDP; i++) {
+			struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
+			snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
+			ppd->index = i;
+			ppd->func = PTP_PF_NONE;
+		}
 		snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
 		adapter->ptp_caps.owner = THIS_MODULE;
 		adapter->ptp_caps.max_adj = 62499999;
-		adapter->ptp_caps.n_ext_ts = 0;
+		adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
+		adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
+		adapter->ptp_caps.n_pins = IGB_N_SDP;
 		adapter->ptp_caps.pps = 1;
+		adapter->ptp_caps.pin_config = adapter->sdp_config;
 		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
 		adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
 		adapter->ptp_caps.settime = igb_ptp_settime_i210;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
+		adapter->ptp_caps.verify = igb_ptp_verify_pin;
 		/* Enable the timer functions by clearing bit 31. */
 		wr32(E1000_TSAUXC, 0x0);
 		break;
-- 
1.7.10.4

Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> This patch series adds three features: time stamping external events,
> producing a periodic output signal, and an internal PPS event. The
> i210 PCIe card has a 6 pin header with SDP0-3, making it easy to try
> out this new functionality.
> 
> An earlier version of this series was posted way back in May, 2013,
> but that version lacked a good way to assign functions to the pins. In
> the mean time, the PHC core has a standard method to configure the
> pins, and this series makes use of it.
> 
> Thanks,
> Richard
> 
> Richard Cochran (4):
>   igb: refactor time sync interrupt handling
>   igb: do not clobber the TSAUXC bits on reset.
>   igb: enable internal PPS for the i210.
>   igb: enable auxiliary PHC functions for the i210.

Thanks Richard, I have added your series of igb patches to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next 2/4] igb: do not clobber the TSAUXC bits on reset.

[Keller, Jacob E]

On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The TSAUXC register has a number of different bits, one of which disables
> the main clock function. Previously, the clock was re-enabled by clearing
> the entire register. This patch changes the code to preserve the values
> of the other bits in that register.
> 

This is a step in the right direction, but won't fully work due to this
being called after a MAC reset.

> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_ptp.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 794c139..ce57128 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -905,6 +905,8 @@ void igb_ptp_stop(struct igb_adapter *adapter)
>  void igb_ptp_reset(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
> +	unsigned long flags;
> +	u32 tsauxc;
>  
>  	if (!(adapter->flags & IGB_FLAG_PTP))
>  		return;
> @@ -923,7 +925,11 @@ void igb_ptp_reset(struct igb_adapter *adapter)
>  	case e1000_i210:
>  	case e1000_i211:
>  		/* Enable the timer functions and interrupts. */
> -		wr32(E1000_TSAUXC, 0x0);
> +		spin_lock_irqsave(&adapter->tmreg_lock, flags);
> +		tsauxc = rd32(E1000_TSAUXC);
> +		tsauxc &= ~TSAUXC_DISABLE;
> +		wr32(E1000_TSAUXC, tsauxc);
> +		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);

This won't work. We call igb_ptp_reset directly after a HW reset (MAC
reset). TSAUXC data is not maintained. We probably need to store driver
state of the TSAUXC bits, and then re-enable them in this reset path.
This code today is fine, because there was no other state we cared about
in the path.

That being said, I don't think this change breaks anything. I just think
it will not solve the problem.

Thanks,
Jake

>  		wr32(E1000_TSIM, TSYNC_INTERRUPTS);
>  		wr32(E1000_IMS, E1000_IMS_TS);
>  		break;

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Keller, Jacob E]

On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device can produce an interrupt on the full second. This
> patch allows using this interrupt to generate an internal PPS event
> for adjusting the kernel system time.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |    6 ++++++
>  drivers/net/ethernet/intel/igb/igb_ptp.c  |   32 +++++++++++++++++++++++++++--
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 7183a56..9412661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
>  static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
> +	struct ptp_clock_event event;
>  	u32 tsicr = rd32(E1000_TSICR);
>  
> +	if (tsicr & TSINTR_SYS_WRAP) {
> +		event.type = PTP_CLOCK_PPS;
> +		ptp_clock_event(adapter->ptp_clock, &event);
> +	}
> +

Nice! I hadn't thought of using the SYS_WRAP around interrupt for pps.
Much simpler than having to setup a SDP pin trigger.

Regards,
Jake

Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.

[Keller, Jacob E]

On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device offers a number of special PTP Hardware Clock features on
> the Software Defined Pins (SDPs). This patch adds support for two of the
> possible functions, namely time stamping external events, and periodic
> output signals.
> 
> The assignment of PHC functions to the four SDP can be freely chosen by
> the user.
>  

This patch probably needs some code added to igb_ptp_reset which will
re-enable the SDP pins after a MAC reset to the same current settings
stored by the driver. It looks like you maintain all this data in
internal structures, so it should be trivial to add the correct function
calls to the reset path.

If you don't have this code, then any time there is a hardware reset,
these will be disabled. Normal operation can sometimes have resets due
to TX hang. I think even one of the ethtool operations can cause a reset
to occur sometimes. See my comments on one of the other patch in the
series about this, also.

Thanks,
Jake

> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      |    9 ++
>  drivers/net/ethernet/intel/igb/igb_main.c |   47 ++++++-
>  drivers/net/ethernet/intel/igb/igb_ptp.c  |  219 ++++++++++++++++++++++++++++-
>  3 files changed, 269 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 82d891e..f6aca7c 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -343,6 +343,9 @@ struct hwmon_buff {
>  	};
>  #endif
>  
> +#define IGB_N_EXTTS	2
> +#define IGB_N_PEROUT	2
> +#define IGB_N_SDP	4
>  #define IGB_RETA_SIZE	128
>  
>  /* board specific private data structure */
> @@ -439,6 +442,12 @@ struct igb_adapter {
>  	u32 tx_hwtstamp_timeouts;
>  	u32 rx_hwtstamp_cleared;
>  
> +	struct ptp_pin_desc sdp_config[IGB_N_SDP];
> +	struct {
> +		struct timespec start;
> +		struct timespec period;
> +	} perout[IGB_N_PEROUT];
> +
>  	char fw_version[32];
>  #ifdef CONFIG_IGB_HWMON
>  	struct hwmon_buff *igb_hwmon_buff;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 9412661..3a25661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
>  	struct ptp_clock_event event;
> -	u32 tsicr = rd32(E1000_TSICR);
> +	struct timespec ts;
> +	u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
>  
>  	if (tsicr & TSINTR_SYS_WRAP) {
>  		event.type = PTP_CLOCK_PPS;
> @@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  		/* retrieve hardware timestamp */
>  		schedule_work(&adapter->ptp_tx_work);
>  	}
> +
> +	if (tsicr & TSINTR_TT0) {
> +		spin_lock(&adapter->tmreg_lock);
> +		ts = timespec_add(adapter->perout[0].start,
> +				  adapter->perout[0].period);
> +		wr32(E1000_TRGTTIML0, ts.tv_nsec);
> +		wr32(E1000_TRGTTIMH0, ts.tv_sec);
> +		tsauxc = rd32(E1000_TSAUXC);
> +		tsauxc |= TSAUXC_EN_TT0;
> +		wr32(E1000_TSAUXC, tsauxc);
> +		adapter->perout[0].start = ts;
> +		spin_unlock(&adapter->tmreg_lock);
> +	}
> +
> +	if (tsicr & TSINTR_TT1) {
> +		spin_lock(&adapter->tmreg_lock);
> +		ts = timespec_add(adapter->perout[1].start,
> +				  adapter->perout[1].period);
> +		wr32(E1000_TRGTTIML1, ts.tv_nsec);
> +		wr32(E1000_TRGTTIMH1, ts.tv_sec);
> +		tsauxc = rd32(E1000_TSAUXC);
> +		tsauxc |= TSAUXC_EN_TT1;
> +		wr32(E1000_TSAUXC, tsauxc);
> +		adapter->perout[1].start = ts;
> +		spin_unlock(&adapter->tmreg_lock);
> +	}
> +
> +	if (tsicr & TSINTR_AUTT0) {
> +		nsec = rd32(E1000_AUXSTMPL0);
> +		sec  = rd32(E1000_AUXSTMPH0);
> +		event.type = PTP_CLOCK_EXTTS;
> +		event.index = 0;
> +		event.timestamp = sec * 1000000000ULL + nsec;
> +		ptp_clock_event(adapter->ptp_clock, &event);
> +	}
> +
> +	if (tsicr & TSINTR_AUTT1) {
> +		nsec = rd32(E1000_AUXSTMPL1);
> +		sec  = rd32(E1000_AUXSTMPH1);
> +		event.type = PTP_CLOCK_EXTTS;
> +		event.index = 1;
> +		event.timestamp = sec * 1000000000ULL + nsec;
> +		ptp_clock_event(adapter->ptp_clock, &event);
> +	}
>  }
>  
>  static irqreturn_t igb_msix_other(int irq, void *data)
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 70d3933..37b9fe6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
>  	return 0;
>  }
>  
> +static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
> +{
> +	u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
> +	u32 mask[IGB_N_SDP] = {
> +		E1000_CTRL_SDP0_DIR,
> +		E1000_CTRL_SDP1_DIR,
> +		E1000_CTRL_EXT_SDP2_DIR,
> +		E1000_CTRL_EXT_SDP3_DIR,
> +	};
> +
> +	if (input)
> +		*ptr &= ~mask[pin];
> +	else
> +		*ptr |= mask[pin];
> +}
> +
> +static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
> +{
> +	struct e1000_hw *hw = &igb->hw;
> +	u32 aux0_sel_sdp[IGB_N_SDP] = {
> +		AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> +	};
> +	u32 aux1_sel_sdp[IGB_N_SDP] = {
> +		AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> +	};
> +	u32 ts_sdp_en[IGB_N_SDP] = {
> +		TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> +	};
> +	u32 ctrl, ctrl_ext, tssdp = 0;
> +
> +	ctrl = rd32(E1000_CTRL);
> +	ctrl_ext = rd32(E1000_CTRL_EXT);
> +	tssdp = rd32(E1000_TSSDP);
> +
> +	igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
> +
> +	/* Make sure this pin is not enabled as an ouput. */
> +	tssdp &= ~ts_sdp_en[pin];
> +
> +	if (chan == 1) {
> +		tssdp &= ~AUX1_SEL_SDP3;
> +		tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
> +	} else {
> +		tssdp &= ~AUX0_SEL_SDP3;
> +		tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
> +	}
> +
> +	wr32(E1000_TSSDP, tssdp);
> +	wr32(E1000_CTRL, ctrl);
> +	wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
> +static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
> +{
> +	struct e1000_hw *hw = &igb->hw;
> +	u32 aux0_sel_sdp[IGB_N_SDP] = {
> +		AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> +	};
> +	u32 aux1_sel_sdp[IGB_N_SDP] = {
> +		AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> +	};
> +	u32 ts_sdp_en[IGB_N_SDP] = {
> +		TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> +	};
> +	u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
> +		TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
> +		TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
> +	};
> +	u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
> +		TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
> +		TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
> +	};
> +	u32 ts_sdp_sel_clr[IGB_N_SDP] = {
> +		TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
> +		TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
> +	};
> +	u32 ctrl, ctrl_ext, tssdp = 0;
> +
> +	ctrl = rd32(E1000_CTRL);
> +	ctrl_ext = rd32(E1000_CTRL_EXT);
> +	tssdp = rd32(E1000_TSSDP);
> +
> +	igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
> +
> +	/* Make sure this pin is not enabled as an input. */
> +	if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
> +		tssdp &= ~AUX0_TS_SDP_EN;
> +
> +	if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
> +		tssdp &= ~AUX1_TS_SDP_EN;
> +
> +	tssdp &= ~ts_sdp_sel_clr[pin];
> +	if (chan == 1)
> +		tssdp |= ts_sdp_sel_tt1[pin];
> +	else
> +		tssdp |= ts_sdp_sel_tt0[pin];
> +
> +	tssdp |= ts_sdp_en[pin];
> +
> +	wr32(E1000_TSSDP, tssdp);
> +	wr32(E1000_CTRL, ctrl);
> +	wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
>  static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
>  				       struct ptp_clock_request *rq, int on)
>  {
>  	struct igb_adapter *igb =
>  		container_of(ptp, struct igb_adapter, ptp_caps);
>  	struct e1000_hw *hw = &igb->hw;
> +	u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
>  	unsigned long flags;
> -	u32 tsim;
> +	struct timespec ts;
> +	int pin;
> +	s64 ns;
>  
>  	switch (rq->type) {
> +	case PTP_CLK_REQ_EXTTS:
> +		if (on) {
> +			pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
> +					   rq->extts.index);
> +			if (pin < 0)
> +				return -EBUSY;
> +			igb_pin_extts(igb, rq->extts.index, pin);
> +		}
> +		if (rq->extts.index == 1) {
> +			tsauxc_mask = TSAUXC_EN_TS1;
> +			tsim_mask = TSINTR_AUTT1;
> +		} else {
> +			tsauxc_mask = TSAUXC_EN_TS0;
> +			tsim_mask = TSINTR_AUTT0;
> +		}
> +		spin_lock_irqsave(&igb->tmreg_lock, flags);
> +		tsauxc = rd32(E1000_TSAUXC);
> +		tsim = rd32(E1000_TSIM);
> +		if (on) {
> +			tsauxc |= tsauxc_mask;
> +			tsim |= tsim_mask;
> +		} else {
> +			tsauxc &= ~tsauxc_mask;
> +			tsim &= ~tsim_mask;
> +		}
> +		wr32(E1000_TSAUXC, tsauxc);
> +		wr32(E1000_TSIM, tsim);
> +		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> +		return 0;
> +
> +	case PTP_CLK_REQ_PEROUT:
> +		if (on) {
> +			pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
> +					   rq->perout.index);
> +			if (pin < 0)
> +				return -EBUSY;
> +			igb_pin_perout(igb, rq->perout.index, pin);
> +		}
> +		ts.tv_sec = rq->perout.period.sec;
> +		ts.tv_nsec = rq->perout.period.nsec;
> +		ns = timespec_to_ns(&ts);
> +		ns = ns >> 1;
> +		if (on && ns < 500000LL) {
> +			/* 2k interrupts per second is an awful lot. */
> +			return -EINVAL;
> +		}
> +		ts = ns_to_timespec(ns);
> +		if (rq->perout.index == 1) {
> +			tsauxc_mask = TSAUXC_EN_TT1;
> +			tsim_mask = TSINTR_TT1;
> +			trgttiml = E1000_TRGTTIML1;
> +			trgttimh = E1000_TRGTTIMH1;
> +		} else {
> +			tsauxc_mask = TSAUXC_EN_TT0;
> +			tsim_mask = TSINTR_TT0;
> +			trgttiml = E1000_TRGTTIML0;
> +			trgttimh = E1000_TRGTTIMH0;
> +		}
> +		spin_lock_irqsave(&igb->tmreg_lock, flags);
> +		tsauxc = rd32(E1000_TSAUXC);
> +		tsim = rd32(E1000_TSIM);
> +		if (on) {
> +			int i = rq->perout.index;
> +			igb->perout[i].start.tv_sec = rq->perout.start.sec;
> +			igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
> +			igb->perout[i].period.tv_sec = ts.tv_sec;
> +			igb->perout[i].period.tv_nsec = ts.tv_nsec;
> +			wr32(trgttiml, rq->perout.start.sec);
> +			wr32(trgttimh, rq->perout.start.nsec);
> +			tsauxc |= tsauxc_mask;
> +			tsim |= tsim_mask;
> +		} else {
> +			tsauxc &= ~tsauxc_mask;
> +			tsim &= ~tsim_mask;
> +		}
> +		wr32(E1000_TSAUXC, tsauxc);
> +		wr32(E1000_TSIM, tsim);
> +		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> +		return 0;
> +
>  	case PTP_CLK_REQ_PPS:
>  		spin_lock_irqsave(&igb->tmreg_lock, flags);
>  		tsim = rd32(E1000_TSIM);
> @@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
>  		wr32(E1000_TSIM, tsim);
>  		spin_unlock_irqrestore(&igb->tmreg_lock, flags);
>  		return 0;
> -
> -	default:
> -		break;
>  	}
>  
>  	return -EOPNOTSUPP;
> @@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
> +			      enum ptp_pin_function func, unsigned int chan)
> +{
> +	switch (func) {
> +	case PTP_PF_NONE:
> +	case PTP_PF_EXTTS:
> +	case PTP_PF_PEROUT:
> +		break;
> +	case PTP_PF_PHYSYNC:
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * igb_ptp_tx_work
>   * @work: pointer to work struct
> @@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
>  	struct net_device *netdev = adapter->netdev;
> +	int i;
>  
>  	switch (hw->mac.type) {
>  	case e1000_82576:
> @@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		break;
>  	case e1000_i210:
>  	case e1000_i211:
> +		for (i = 0; i < IGB_N_SDP; i++) {
> +			struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
> +			snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
> +			ppd->index = i;
> +			ppd->func = PTP_PF_NONE;
> +		}
>  		snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
>  		adapter->ptp_caps.owner = THIS_MODULE;
>  		adapter->ptp_caps.max_adj = 62499999;
> -		adapter->ptp_caps.n_ext_ts = 0;
> +		adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
> +		adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
> +		adapter->ptp_caps.n_pins = IGB_N_SDP;
>  		adapter->ptp_caps.pps = 1;
> +		adapter->ptp_caps.pin_config = adapter->sdp_config;
>  		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
>  		adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
>  		adapter->ptp_caps.settime = igb_ptp_settime_i210;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
> +		adapter->ptp_caps.verify = igb_ptp_verify_pin;
>  		/* Enable the timer functions by clearing bit 31. */
>  		wr32(E1000_TSAUXC, 0x0);
>  		break;

Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.

[Richard Cochran]

On Mon, Nov 17, 2014 at 03:15:18PM -0800, Jeff Kirsher wrote:
> Thanks Richard, I have added your series of igb patches to my queue.

Jeff, please hold off on these. I found a bug in the PPS code. I will
re-submit with a fix.

Thanks,
Richard

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Richard Cochran]

On Tue, Nov 18, 2014 at 12:06:24AM +0100, Richard Cochran wrote:
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
>  static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
> +	struct ptp_clock_event event;
>  	u32 tsicr = rd32(E1000_TSICR);
>  
> +	if (tsicr & TSINTR_SYS_WRAP) {
> +		event.type = PTP_CLOCK_PPS;
> +		ptp_clock_event(adapter->ptp_clock, &event);

This causes a BUG for the 82580. When you enable E1000_TSICR_TXTS, it
seems that TSINTR_SYS_WRAP is also automatically enabled. Because the
82580 variant has no pps device enabled, the driver then dereferences
a null pointer.

So we need to make the call to ptp_clock_event() conditional based on
whether pps is enabled. I'll fix this in V2.

> +	}
> +

Thanks,
Richard

Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

On Wed, 2014-11-19 at 07:31 +0100, Richard Cochran wrote:
> On Mon, Nov 17, 2014 at 03:15:18PM -0800, Jeff Kirsher wrote:
> > Thanks Richard, I have added your series of igb patches to my queue.
> 
> Jeff, please hold off on these. I found a bug in the PPS code. I will
> re-submit with a fix.

Ok, I will drop the series that I currently have in my queue and will
await your v2 series.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Keller, Jacob E]

On Wed, 2014-11-19 at 07:37 +0100, Richard Cochran wrote:
> On Tue, Nov 18, 2014 at 12:06:24AM +0100, Richard Cochran wrote:
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
> >  static void igb_tsync_interrupt(struct igb_adapter *adapter)
> >  {
> >  	struct e1000_hw *hw = &adapter->hw;
> > +	struct ptp_clock_event event;
> >  	u32 tsicr = rd32(E1000_TSICR);
> >  
> > +	if (tsicr & TSINTR_SYS_WRAP) {
> > +		event.type = PTP_CLOCK_PPS;
> > +		ptp_clock_event(adapter->ptp_clock, &event);
> 
> This causes a BUG for the 82580. When you enable E1000_TSICR_TXTS, it
> seems that TSINTR_SYS_WRAP is also automatically enabled. Because the
> 82580 variant has no pps device enabled, the driver then dereferences
> a null pointer.
> 
> So we need to make the call to ptp_clock_event() conditional based on
> whether pps is enabled. I'll fix this in V2.
> 
> > +	}
> > +
> 
> Thanks,
> Richard

Good catch :)

Did you see my concern about the reset path needing to fully restore the
state since it is called after a hardware MAC reset which has cleared
all these registers?

Regards,
Jake

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Richard Cochran]

On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> Good catch :)

(Well, my X session suddenly disappeared, and a kernel oops appeared in
the console... hard to overlook ;^)
 
> Did you see my concern about the reset path needing to fully restore the
> state since it is called after a hardware MAC reset which has cleared
> all these registers?

Yes, and that bit I copied from the first series a year ago. I don't
remember why, but IIRC that was necessary to let the SDP stuff work at
all. Maybe the reset function was called under different circumstances
back then. I'll take another look.

I find it a bit weird that the auxiliary functions don't work when the
interface or the link is down.

Thanks,
Richard

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Keller, Jacob E]

On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > Good catch :)
> 
> (Well, my X session suddenly disappeared, and a kernel oops appeared in
> the console... hard to overlook ;^)
>  
> > Did you see my concern about the reset path needing to fully restore the
> > state since it is called after a hardware MAC reset which has cleared
> > all these registers?
> 
> Yes, and that bit I copied from the first series a year ago. I don't
> remember why, but IIRC that was necessary to let the SDP stuff work at
> all. Maybe the reset function was called under different circumstances
> back then. I'll take another look.
> 
> I find it a bit weird that the auxiliary functions don't work when the
> interface or the link is down.
> 
> Thanks,
> Richard

I think you need something here, but it should be clearing that register
after a MAC reset, so it needs to be re-initialized. I'm not sure if
that reset path was used in the same place in the past.

Well, I think igb and ixgbe destroy the ptp device when the interface
goes down, and restore it when it comes up. Probably we should instead
handle some things in reset path and allow the ptp device to remain.

It's partly due to the clock speed changing based on link speed.

Regards,
Jake

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Richard Cochran]

On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote:
> On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > > Good catch :)

I have not been able to reproduce the crash, and so the cause is not
what I thought it was. Maybe it was my patch that preserved the
enabled interrupts in igb_ptp_reset(). I didn't notice that the driver
frees and reallocates the ptp_clock. I would never do that, myself.

> I think you need something here, but it should be clearing that register
> after a MAC reset, so it needs to be re-initialized. I'm not sure if
> that reset path was used in the same place in the past.

Okay, lets figure this out. Why is there a PTP reset function at all?
I don't know, lets see who calls it...

  Finding functions calling: igb_ptp_reset
  ----------------------------------------
  *** drivers/net/ethernet/intel/igb/igb_main.c:
  igb_reset[2033]                igb_ptp_reset(adapter);

Easy enough to understand. But who is calling igb_reset?

  Finding functions calling: igb_reset
  ------------------------------------
  *** drivers/net/ethernet/intel/igb/igb_ethtool.c:
  igb_set_settings[345]          igb_reset(adapter);
  igb_set_pauseparam[409]        igb_reset(adapter);
  igb_diag_test[2016]            igb_reset(adapter);
  igb_set_eee[2729]              igb_reset(adapter);
  *** drivers/net/ethernet/intel/igb/igb_main.c:
  igb_down[1814]                 igb_reset(adapter);
  igb_set_features[2069]         igb_reset(adapter);
  igb_probe[2526]                igb_reset(adapter);
  __igb_open[3110]               igb_reset(adapter);
  igb_watchdog_task[4231]        igb_reset(adapter);
  igb_change_mtu[5189]           igb_reset(adapter);
  igb_resume[7460]               igb_reset(adapter);
  igb_sriov_reinit[7545]         igb_reset(adapter);
  igb_io_slot_reset[7678]        igb_reset(adapter);

Wow, that is quite much. So, whenever any random parameter is changed,
we reset the PTP clock. Great.

Really, wouldn't better to reset the clock functions only when
absolutely necessary?

Thanks,
Richard

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Keller, Jacob E]

On Thu, 2014-11-20 at 10:18 +0100, Richard Cochran wrote:
> On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote:
> > On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> > > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > > > Good catch :)
> 
> I have not been able to reproduce the crash, and so the cause is not
> what I thought it was. Maybe it was my patch that preserved the
> enabled interrupts in igb_ptp_reset(). I didn't notice that the driver
> frees and reallocates the ptp_clock. I would never do that, myself.
> 

Yea, I think that was a design I tried in the ixgbe driver, but it
really isn't good.

> > I think you need something here, but it should be clearing that register
> > after a MAC reset, so it needs to be re-initialized. I'm not sure if
> > that reset path was used in the same place in the past.
> 
> Okay, lets figure this out. Why is there a PTP reset function at all?
> I don't know, lets see who calls it...
> 
>   Finding functions calling: igb_ptp_reset
>   ----------------------------------------
>   *** drivers/net/ethernet/intel/igb/igb_main.c:
>   igb_reset[2033]                igb_ptp_reset(adapter);
> 
> Easy enough to understand. But who is calling igb_reset?
> 
>   Finding functions calling: igb_reset
>   ------------------------------------
>   *** drivers/net/ethernet/intel/igb/igb_ethtool.c:
>   igb_set_settings[345]          igb_reset(adapter);
>   igb_set_pauseparam[409]        igb_reset(adapter);
>   igb_diag_test[2016]            igb_reset(adapter);
>   igb_set_eee[2729]              igb_reset(adapter);
>   *** drivers/net/ethernet/intel/igb/igb_main.c:
>   igb_down[1814]                 igb_reset(adapter);
>   igb_set_features[2069]         igb_reset(adapter);
>   igb_probe[2526]                igb_reset(adapter);
>   __igb_open[3110]               igb_reset(adapter);
>   igb_watchdog_task[4231]        igb_reset(adapter);
>   igb_change_mtu[5189]           igb_reset(adapter);
>   igb_resume[7460]               igb_reset(adapter);
>   igb_sriov_reinit[7545]         igb_reset(adapter);
>   igb_io_slot_reset[7678]        igb_reset(adapter);
> 
> Wow, that is quite much. So, whenever any random parameter is changed,
> we reset the PTP clock. Great.

Here is the problem. In igb_reset, we call e1000_reset_hw() which will
eventually perform a hardware MAC reset. Yes maybe we shouldn't be
calling igb_reset everywhere.. I cannot answer that, however...

> 
> Really, wouldn't better to reset the clock functions only when
> absolutely necessary?
> 

We *are*. If e1000_reset_hw(hw) is called, the MAC registers will be
reset to their initial values, which includes not having SYSTIME setup,
and not having the outputs configured. There is not much that can be
done to avoid that besides major refactoring of the igb driver to avoid
resetting as much as it does. In most of those cases, I think the reset
is fine, and we actually aren't going to reset that many times during
nominal operation.

Some of the ethtool settings maybe could avoid resets, but I am not
certain during exactly which path they end up calling resets.

Regards,
Jake

> Thanks,
> Richard

Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

[Richard Cochran]

On Thu, Nov 20, 2014 at 09:28:26PM +0000, Keller, Jacob E wrote:
> Here is the problem. In igb_reset, we call e1000_reset_hw() which will
> eventually perform a hardware MAC reset. Yes maybe we shouldn't be
> calling igb_reset everywhere.. I cannot answer that, however...

It is a typical device driver "code smell". The developer thinks, 
"Hm, something isn't working in this function. I know - lets reset the
device. That _always_ works."

> Some of the ethtool settings maybe could avoid resets, but I am not
> certain during exactly which path they end up calling resets.

Jup. Surely changing the MTU is no reason to reset the PTP clock. But
as you said, fixing the reset stuff is a huge amount of work. Instead
of trying to do that, I'll just go with the flow and reset the
auxiliary functions, too.

Thanks,
Richard