From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vick, Matthew" Subject: Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210. Date: Tue, 28 May 2013 15:58:07 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: David Miller , "e1000-devel@lists.sourceforge.net" , "Keller, Jacob E" , "Kirsher, Jeffrey T" To: Richard Cochran , "netdev@vger.kernel.org" Return-path: Received: from mga09.intel.com ([134.134.136.24]:26596 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934550Ab3E1P6J convert rfc822-to-8bit (ORCPT ); Tue, 28 May 2013 11:58:09 -0400 In-Reply-To: Content-Language: en-US Content-ID: Sender: netdev-owner@vger.kernel.org List-ID: On 5/27/13 2:21 AM, "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 three of the >possible functions, namely time stamping external events, a periodic >output signal, and an internal PPS event for adjusting the kernel system >time. > >A pair of module parameters lets the user choose which of the four SDPs >will be used as the input signal and which as the output. > >Signed-off-by: Richard Cochran >--- > drivers/net/ethernet/intel/igb/igb.h | 2 + > drivers/net/ethernet/intel/igb/igb_main.c | 30 ++++++ > drivers/net/ethernet/intel/igb/igb_ptp.c | 157 >++++++++++++++++++++++++++++- > 3 files changed, 188 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/ethernet/intel/igb/igb.h >b/drivers/net/ethernet/intel/igb/igb.h >index 15ea8dc..802b79c 100644 >--- a/drivers/net/ethernet/intel/igb/igb.h >+++ b/drivers/net/ethernet/intel/igb/igb.h >@@ -435,6 +435,8 @@ struct igb_adapter { > struct timecounter tc; > u32 tx_hwtstamp_timeouts; > u32 rx_hwtstamp_cleared; >+ struct timespec start; >+ struct timespec period; > > char fw_version[32]; > #ifdef CONFIG_IGB_HWMON >diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >b/drivers/net/ethernet/intel/igb/igb_main.c >index d25a965..58120cd 100644 >--- a/drivers/net/ethernet/intel/igb/igb_main.c >+++ b/drivers/net/ethernet/intel/igb/igb_main.c >@@ -5038,12 +5038,42 @@ 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) { > /* retrieve hardware timestamp */ > schedule_work(&adapter->ptp_tx_work); > } >+ >+ if (tsicr & TSINTR_TT0) { >+ struct timespec ts; >+ u32 tsauxc; >+ spin_lock(&adapter->tmreg_lock); >+ ts = timespec_add(adapter->start, adapter->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->start = ts; >+ spin_unlock(&adapter->tmreg_lock); >+ } >+ >+ if (tsicr & TSINTR_AUTT0) { >+ u32 sec, nsec; >+ 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); >+ } > } I would prefer it if we did a MAC check before these two TSICR checks, since we're making some assumptions about the hardware within the interrupt cases. At the very least, a comment that these are only applicable to I210/I211 would be nice. > > 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 5944de0..8cf4b8a 100644 >--- a/drivers/net/ethernet/intel/igb/igb_ptp.c >+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c >@@ -23,6 +23,15 @@ > > #include "igb.h" > >+static int igb_input_sdp = 0; >+static int igb_output_sdp = 1; >+module_param(igb_input_sdp, int, 0444); >+module_param(igb_output_sdp, int, 0444); >+MODULE_PARM_DESC(igb_input_sdp, >+ "The SDP used as an input, to time stamp external events"); >+MODULE_PARM_DESC(igb_output_sdp, >+ "The SDP used to output the programmable periodic signal"); >+ Is there any other mechanism we could use to control this? I would imagine not, but I know module parameters are generally frowned upon. > #define INCVALUE_MASK 0x7fffffff > #define ISGN 0x80000000 > >@@ -359,6 +368,86 @@ static int igb_ptp_settime_i210(struct >ptp_clock_info *ptp, > return 0; > } > >+static int igb_ptp_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; >+ struct timespec ts; >+ u32 tsauxc, tsim; >+ s64 ns; >+ >+ switch (rq->type) { >+ case PTP_CLK_REQ_EXTTS: >+ if (rq->extts.index != 0) >+ return -EINVAL; >+ spin_lock_irqsave(&igb->tmreg_lock, flags); >+ tsauxc = rd32(E1000_TSAUXC); >+ tsim = rd32(E1000_TSIM); >+ if (on) { >+ tsauxc |= TSAUXC_EN_TS0; >+ tsim |= TSINTR_AUTT0; >+ } else { >+ tsauxc &= ~TSAUXC_EN_TS0; >+ tsim &= ~TSINTR_AUTT0; >+ } >+ wr32(E1000_TSAUXC, tsauxc); >+ wr32(E1000_TSIM, tsim); >+ spin_unlock_irqrestore(&igb->tmreg_lock, flags); >+ return 0; >+ >+ case PTP_CLK_REQ_PEROUT: >+ if (rq->perout.index != 0) >+ return -EINVAL; >+ >+ 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); >+ >+ spin_lock_irqsave(&igb->tmreg_lock, flags); >+ tsauxc = rd32(E1000_TSAUXC); >+ tsim = rd32(E1000_TSIM); >+ if (on) { >+ igb->start.tv_sec = rq->perout.start.sec; >+ igb->start.tv_nsec = rq->perout.start.nsec; >+ igb->period.tv_sec = ts.tv_sec; >+ igb->period.tv_nsec = ts.tv_nsec; >+ wr32(E1000_TRGTTIML0, rq->perout.start.sec); >+ wr32(E1000_TRGTTIMH0, rq->perout.start.nsec); >+ tsauxc |= TSAUXC_EN_TT0; >+ tsim |= TSINTR_TT0; >+ } else { >+ tsauxc &= ~TSAUXC_EN_TT0; >+ tsim &= ~TSINTR_TT0; >+ } >+ 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); >+ if (on) >+ tsim |= TSINTR_SYS_WRAP; >+ else >+ tsim &= ~TSINTR_SYS_WRAP; >+ wr32(E1000_TSIM, tsim); >+ spin_unlock_irqrestore(&igb->tmreg_lock, flags); >+ return 0; >+ } >+ >+ return -EOPNOTSUPP; >+} >+ > static int igb_ptp_enable(struct ptp_clock_info *ptp, > struct ptp_clock_request *rq, int on) > { >@@ -711,6 +800,68 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, > -EFAULT : 0; > } > >+static int igb_sdp_init(struct igb_adapter *adapter) >+{ >+ struct e1000_hw *hw = &adapter->hw; >+ u32 ctrl, ctrl_ext, tssdp = 0; >+ >+ if (igb_input_sdp == igb_output_sdp) { >+ pr_err("SDP %d set as both input and output\n", igb_input_sdp); >+ return -1; Shouldn't this return -EINVAL? >+ } >+ >+ ctrl = rd32(E1000_CTRL); >+ ctrl_ext = rd32(E1000_CTRL_EXT); >+ >+ switch (igb_input_sdp) { >+ case 0: >+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP0; >+ ctrl &= ~E1000_CTRL_SDP0_DIR; >+ break; >+ case 1: >+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP1; >+ ctrl &= ~E1000_CTRL_SDP1_DIR; >+ break; >+ case 2: >+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP2; >+ ctrl_ext &= ~E1000_CTRL_EXT_SDP2_DIR; >+ break; >+ case 3: >+ tssdp |= AUX0_TS_SDP_EN | AUX0_SEL_SDP3; >+ ctrl_ext &= ~E1000_CTRL_EXT_SDP3_DIR; >+ break; >+ default: >+ pr_err("Input SDP %d out of range\n", igb_input_sdp); Shouldn't this return -EINVAL as well? >+ } >+ >+ switch (igb_output_sdp) { >+ case 0: >+ tssdp |= TS_SDP0_SEL_TT0 | TS_SDP0_EN; >+ ctrl |= E1000_CTRL_SDP0_DIR; >+ break; >+ case 1: >+ tssdp |= TS_SDP1_SEL_TT0 | TS_SDP1_EN; >+ ctrl |= E1000_CTRL_SDP1_DIR; >+ break; >+ case 2: >+ tssdp |= TS_SDP2_SEL_TT0 | TS_SDP2_EN; >+ ctrl_ext |= E1000_CTRL_EXT_SDP2_DIR; >+ break; >+ case 3: >+ tssdp |= TS_SDP3_SEL_TT0 | TS_SDP3_EN; >+ ctrl_ext |= E1000_CTRL_EXT_SDP3_DIR; >+ break; >+ default: >+ pr_err("Output SDP %d out of range\n", igb_output_sdp); Same here? >+ } >+ >+ wr32(E1000_TSSDP, tssdp); >+ wr32(E1000_CTRL, ctrl); >+ wr32(E1000_CTRL_EXT, ctrl_ext); >+ >+ return 0; >+} >+ > void igb_ptp_init(struct igb_adapter *adapter) > { > struct e1000_hw *hw = &adapter->hw; >@@ -766,7 +917,7 @@ void igb_ptp_init(struct igb_adapter *adapter) > 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_enable; >+ adapter->ptp_caps.enable = igb_ptp_enable_i210; > /* Enable the timer functions by clearing bit 31. */ > wr32(E1000_TSAUXC, 0x0); > break; >@@ -785,6 +936,10 @@ void igb_ptp_init(struct igb_adapter *adapter) > struct timespec ts = ktime_to_timespec(ktime_get_real()); > > igb_ptp_settime_i210(&adapter->ptp_caps, &ts); >+ igb_sdp_init(adapter); What if igb_sdp_init fails? >+ adapter->ptp_caps.n_ext_ts = 1; >+ adapter->ptp_caps.n_per_out = 1; >+ adapter->ptp_caps.pps = 1; > } else { > timecounter_init(&adapter->tc, &adapter->cc, > ktime_to_ns(ktime_get_real())); >-- >1.7.10.4 >