igb: auxiliary PHC functions for the i210.
Subsystems (3)
cover igb: auxiliary PHC functions for the i210.
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 and SDP1, making it easy to try out this new functionality.
Thanks,
Richard
Richard Cochran (4):
igb: refactor and simplify time sync interrupt handling
igb: add more register definitions for time sync functions.
igb: do not clobber the TSAUXC bits on reset.
igb: enable auxiliary PHC functions for the i210.
drivers/net/ethernet/intel/igb/e1000_defines.h | 68 +++++++++- drivers/net/ethernet/intel/igb/e1000_regs.h | 9 ++ drivers/net/ethernet/intel/igb/igb.h | 2 + drivers/net/ethernet/intel/igb/igb_main.c | 77 ++++++----- drivers/net/ethernet/intel/igb/igb_ptp.c | 169 +++++++++++++++++++++++- 5 files changed, 288 insertions(+), 37 deletions(-)
Signature · 1 line
1.7.10.4
PATCH 1/4 igb: refactor and simplify time sync interrupt handling
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. Also, reading the TSICR register already acknowledges the time sync interrupts, and so there is no need to write that register at all.
--- drivers/net/ethernet/intel/igb/igb_main.c | 47 +++++++++++------------------ 1 file changed, 17 insertions(+), 30 deletions(-)
drivers/net/ethernet/intel/igb/igb_main.c +17 -30
@@ -5035,6 +5035,17 @@ 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 ) { +/* retrieve hardware timestamp */ +schedule_work (&adapter->ptp_tx_work); + } +} +static irqreturn_t igb_msix_other (int irq,void *data) {struct igb_adapter *adapter = data;
@@ -5066,16 +5077,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);
@@ -5850,16 +5853,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);
@@ -5904,16 +5899,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);
Signature · 1 line
1.7.10.4
Re: [PATCH net-next 1/4] igb: refactor and simplify time sync interrupt handling
Quoted 11 lines · 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. Also, reading the TSICR register already acknowledges the time sync interrupts, and so there is no need to write that register at all.
Signed-off-by Richard Cochran <[email protected]>--- drivers/net/ethernet/intel/igb/igb_main.c | 47 +++++++++++------------------ 1 file changed, 17 insertions(+), 30 deletions(-)
Thanks Richard, I will add this to my queue.
------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Re: [PATCH net-next 1/4] igb: refactor and simplify time sync interrupt handling
In reply to Richard CochranQuoted 96 lines · 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. Also, reading the TSICR register already acknowledges the time sync interrupts, and so there is no need to write that register at all.
Signed-off-by Richard Cochran <[email protected]>---
drivers/net/ethernet/intel/igb/igb_main.c| 47
+++++++++++------------------1 file changed, 17 insertions(+), 30 deletions(-)drivers/net/ethernet/intel/igb/igb_main.c +17
-30@@ -5035,6 +5035,17 @@ 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 ) { +/* retrieve hardware timestamp */ +schedule_work (&adapter->ptp_tx_work); + } +} +static irqreturn_t igb_msix_other (int irq,void *data) {struct igb_adapter *adapter = data;@@ -5066,16 +5077,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);@@ -5850,16 +5853,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);@@ -5904,16 +5899,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);Signature · 1 line
1.7.10.4
NAK due to the question you raised in the other thread--this will break 1588 on the 82580, but there was really no way for you to know that. As I mentioned before, I'm getting the internal discussion going to confirm that.
Matthew
PATCH 2/4 igb: add more register definitions for time sync functions.
In reply to Richard CochranThis patch adds missing definitions needed for implementing the auxiliary time sync functions.
--- drivers/net/ethernet/intel/igb/e1000_defines.h | 68 ++++++++++++++++++++++-- drivers/net/ethernet/intel/igb/e1000_regs.h | 9 ++++ drivers/net/ethernet/intel/igb/igb_ptp.c | 4 +- 3 files changed, 76 insertions(+), 5 deletions(-)
drivers/net/ethernet/intel/igb/e1000_defines.h +65 -3
@@ -44,7 +44,10 @@#define E1000_WUFC_BC 0x00000010/* Broadcast Wakeup Enable */ /* Extended Device Control */ +#define E1000_CTRL_EXT_SDP2_DATA 0x00000040/* Value of SW Defineable Pin 2 */ #define E1000_CTRL_EXT_SDP3_DATA 0x00000080/* Value of SW Defineable Pin 3 */ +#define E1000_CTRL_EXT_SDP2_DIR 0x00000400/* SDP2 data direction */ +#define E1000_CTRL_EXT_SDP3_DIR 0x00000800/* SDP3 data direction */ /* Physical Func Reset Done Indication */ #define E1000_CTRL_EXT_PFRSTD 0x00004000#define E1000_CTRL_EXT_LINK_MODE_MASK 0x00C00000
@@ -191,7 +194,8 @@/* enable link status from external LINK_0 and LINK_1 pins */ #define E1000_CTRL_SWDPIN0 0x00040000/* SWDPIN 0 value */ #define E1000_CTRL_SWDPIN1 0x00080000/* SWDPIN 1 value */ -#define E1000_CTRL_SWDPIO0 0x00400000/* SWDPIN 0 Input or output */ +#define E1000_CTRL_SDP0_DIR 0x00400000/* SDP0 data direction */ +#define E1000_CTRL_SDP1_DIR 0x00800000/* SDP1 data direction */ #define E1000_CTRL_RST 0x04000000/* Global reset */ #define E1000_CTRL_RFCE 0x08000000/* Receive Flow Control enable */ #define E1000_CTRL_TFCE 0x10000000/* Transmit flow control enable */
@@ -524,8 +528,66 @@#define E1000_TIMINCA_16NS_SHIFT 24 -#define E1000_TSICR_TXTS 0x00000002 -#define E1000_TSIM_TXTS 0x00000002 +/* Time Sync Interrupt Cause/Mask Regsiter Bits */ +#define TSINTR_SYS_WRAP (1 << 0)/* SYSTIM Wrap around. */ +#define TSINTR_TXTS (1 << 1)/* Transmit Timestamp. */ +#define TSINTR_RXTS (1 << 2)/* Receive Timestamp. */ +#define TSINTR_TT0 (1 << 3)/* Target Time 0 Trigger. */ +#define TSINTR_TT1 (1 << 4)/* Target Time 1 Trigger. */ +#define TSINTR_AUTT0 (1 << 5)/* Auxiliary Timestamp 0 Taken. */ +#define TSINTR_AUTT1 (1 << 6)/* Auxiliary Timestamp 1 Taken. */ +#define TSINTR_TADJ (1 << 7)/* Time Adjust Done. */ + +#define TSYNC_INTERRUPTS TSINTR_TXTS +#define E1000_TSICR_TXTS TSINTR_TXTS + +/* TSAUXC Configuration Bits */ +#define TSAUXC_EN_TT0 (1 << 0)/* Enable target time 0. */ +#define TSAUXC_EN_TT1 (1 << 1)/* Enable target time 1. */ +#define TSAUXC_EN_CLK0 (1 << 2)/* Enable Configurable Frequency Clock 0. */ +#define TSAUXC_SAMP_AUT0 (1 << 3)/* Latch SYSTIML/H into AUXSTMPL/0. */ +#define TSAUXC_ST0 (1 << 4)/* Start Clock 0 Toggle on Target Time 0. */ +#define TSAUXC_EN_CLK1 (1 << 5)/* Enable Configurable Frequency Clock 1. */ +#define TSAUXC_SAMP_AUT1 (1 << 6)/* Latch SYSTIML/H into AUXSTMPL/1. */ +#define TSAUXC_ST1 (1 << 7)/* Start Clock 1 Toggle on Target Time 1. */ +#define TSAUXC_EN_TS0 (1 << 8)/* Enable hardware timestamp 0. */ +#define TSAUXC_AUTT0 (1 << 9)/* Auxiliary Timestamp Taken. */ +#define TSAUXC_EN_TS1 (1 << 10)/* Enable hardware timestamp 0. */ +#define TSAUXC_AUTT1 (1 << 11)/* Auxiliary Timestamp Taken. */ +#define TSAUXC_PLSG (1 << 17)/* Generate a pulse. */ +#define TSAUXC_DISABLE (1 << 31)/* Disable SYSTIM Count Operation. */ + +/* SDP Configuration Bits */ +#define AUX0_SEL_SDP0 (0 << 0)/* Assign SDP0 to auxiliary time stamp 0. */ +#define AUX0_SEL_SDP1 (1 << 0)/* Assign SDP1 to auxiliary time stamp 0. */ +#define AUX0_SEL_SDP2 (2 << 0)/* Assign SDP2 to auxiliary time stamp 0. */ +#define AUX0_SEL_SDP3 (3 << 0)/* Assign SDP3 to auxiliary time stamp 0. */ +#define AUX0_TS_SDP_EN (1 << 2)/* Enable auxiliary time stamp trigger 0. */ +#define AUX1_SEL_SDP0 (0 << 3)/* Assign SDP0 to auxiliary time stamp 1. */ +#define AUX1_SEL_SDP1 (1 << 3)/* Assign SDP1 to auxiliary time stamp 1. */ +#define AUX1_SEL_SDP2 (2 << 3)/* Assign SDP2 to auxiliary time stamp 1. */ +#define AUX1_SEL_SDP3 (3 << 3)/* Assign SDP3 to auxiliary time stamp 1. */ +#define AUX1_TS_SDP_EN (1 << 5)/* Enable auxiliary time stamp trigger 1. */ +#define TS_SDP0_SEL_TT0 (0 << 6)/* Target time 0 is output on SDP0. */ +#define TS_SDP0_SEL_TT1 (1 << 6)/* Target time 1 is output on SDP0. */ +#define TS_SDP0_SEL_FC0 (2 << 6)/* Freq clock 0 is output on SDP0. */ +#define TS_SDP0_SEL_FC1 (3 << 6)/* Freq clock 1 is output on SDP0. */ +#define TS_SDP0_EN (1 << 8)/* SDP0 is assigned to Tsync. */ +#define TS_SDP1_SEL_TT0 (0 << 9)/* Target time 0 is output on SDP1. */ +#define TS_SDP1_SEL_TT1 (1 << 9)/* Target time 1 is output on SDP1. */ +#define TS_SDP1_SEL_FC0 (2 << 9)/* Freq clock 0 is output on SDP1. */ +#define TS_SDP1_SEL_FC1 (3 << 9)/* Freq clock 1 is output on SDP1. */ +#define TS_SDP1_EN (1 << 11)/* SDP1 is assigned to Tsync. */ +#define TS_SDP2_SEL_TT0 (0 << 12)/* Target time 0 is output on SDP2. */ +#define TS_SDP2_SEL_TT1 (1 << 12)/* Target time 1 is output on SDP2. */ +#define TS_SDP2_SEL_FC0 (2 << 12)/* Freq clock 0 is output on SDP2. */ +#define TS_SDP2_SEL_FC1 (3 << 12)/* Freq clock 1 is output on SDP2. */ +#define TS_SDP2_EN (1 << 14)/* SDP2 is assigned to Tsync. */ +#define TS_SDP3_SEL_TT0 (0 << 15)/* Target time 0 is output on SDP3. */ +#define TS_SDP3_SEL_TT1 (1 << 15)/* Target time 1 is output on SDP3. */ +#define TS_SDP3_SEL_FC0 (2 << 15)/* Freq clock 0 is output on SDP3. */ +#define TS_SDP3_SEL_FC1 (3 << 15)/* Freq clock 1 is output on SDP3. */ +#define TS_SDP3_EN (1 << 17)/* SDP3 is assigned to Tsync. */ #define E1000_MDICNFG_EXT_MDIO 0x80000000/* MDI ext/int destination */ #define E1000_MDICNFG_COM_MDIO 0x40000000/* MDI shared w/ lan 0 */
drivers/net/ethernet/intel/igb/e1000_regs.h +9 -0
@@ -41,6 +41,7 @@#define E1000_FCT 0x00030/* Flow Control Type - RW */ #define E1000_CONNSW 0x00034/* Copper/Fiber switch control - RW */ #define E1000_VET 0x00038/* VLAN Ether Type - RW */ +#define E1000_TSSDP 0x0003C/* Time Sync SDP Configuration Register - RW */ #define E1000_ICR 0x000C0/* Interrupt Cause Read - R/clr */ #define E1000_ITR 0x000C4/* Interrupt Throttling Rate - RW */ #define E1000_ICS 0x000C8/* Interrupt Cause Set - WO */
@@ -102,6 +103,14 @@#define E1000_SYSTIMH 0x0B604/* System time register High - RO */ #define E1000_TIMINCA 0x0B608/* Increment attributes register - RW */ #define E1000_TSAUXC 0x0B640/* Timesync Auxiliary Control register */ +#define E1000_TRGTTIML0 0x0B644/* Target Time Register 0 Low - RW */ +#define E1000_TRGTTIMH0 0x0B648/* Target Time Register 0 High - RW */ +#define E1000_TRGTTIML1 0x0B64C/* Target Time Register 1 Low - RW */ +#define E1000_TRGTTIMH1 0x0B650/* Target Time Register 1 High - RW */ +#define E1000_AUXSTMPL0 0x0B65C/* Auxiliary Time Stamp 0 Register Low - RO */ +#define E1000_AUXSTMPH0 0x0B660/* Auxiliary Time Stamp 0 Register High - RO */ +#define E1000_AUXSTMPL1 0x0B664/* Auxiliary Time Stamp 1 Register Low - RO */ +#define E1000_AUXSTMPH1 0x0B668/* Auxiliary Time Stamp 1 Register High - RO */ #define E1000_SYSTIMR 0x0B6F8/* System time register Residue */ #define E1000_TSICR 0x0B66C/* Interrupt Cause Register */ #define E1000_TSIM 0x0B674/* Interrupt Mask Register */
drivers/net/ethernet/intel/igb/igb_ptp.c +2 -2
@@ -798,7 +798,7 @@ void igb_ptp_init(struct igb_adapter *adapter)/* Initialize the time sync interrupts for devices that support it. */ if (hw->mac.type >= e1000_82580) { -wr32 (E1000_TSIM ,E1000_TSIM_TXTS ); +wr32 (E1000_TSIM ,TSYNC_INTERRUPTS );wr32 (E1000_IMS ,E1000_IMS_TS ); }
@@ -876,7 +876,7 @@ void igb_ptp_reset(struct igb_adapter *adapter)case e1000_i211:/* Enable the timer functions and interrupts. */ wr32 (E1000_TSAUXC ,0x0 ); -wr32 (E1000_TSIM ,E1000_TSIM_TXTS ); +wr32 (E1000_TSIM ,TSYNC_INTERRUPTS );wr32 (E1000_IMS ,E1000_IMS_TS );break ;default :
Signature · 14 lines
1.7.10.4 ------------------------------------------------------------------------------ Try New Relic Now & We'll Send You this Cool Shirt New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Re: [PATCH net-next 2/4] igb: add more register definitions for time sync functions.
Quoted 10 lines · Richard Cochran
This patch adds missing definitions needed for implementing the auxiliary time sync functions.
Signed-off-by Richard Cochran <[email protected]>--- drivers/net/ethernet/intel/igb/e1000_defines.h | 68 ++++++++++++++++++++++-- drivers/net/ethernet/intel/igb/e1000_regs.h | 9 ++++ drivers/net/ethernet/intel/igb/igb_ptp.c | 4 +- 3 files changed, 76 insertions(+), 5 deletions(-)
Thanks Richard, I will add this to my queue.
PATCH 3/4 igb: do not clobber the TSAUXC bits on reset.
In reply to Richard CochranThe 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.
--- drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/net/ethernet/intel/igb/igb_ptp.c +7 -1
@@ -860,6 +860,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 ;
@@ -875,7 +877,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 ;
Signature · 14 lines
1.7.10.4 ------------------------------------------------------------------------------ Try New Relic Now & We'll Send You this Cool Shirt New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Re: [PATCH net-next 3/4] igb: do not clobber the TSAUXC bits on reset.
Quoted 8 lines · 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.
Quoted 4 lines
Signed-off-by Richard Cochran <[email protected]>--- drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Thanks Richard, I will add this to my queue.
PATCH 4/4 igb: enable auxiliary PHC functions for the i210.
In reply to Richard CochranThe 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.
--- 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(-)
drivers/net/ethernet/intel/igb/igb.h +2 -0
@@ -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
drivers/net/ethernet/intel/igb/igb_main.c +30 -0
@@ -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); + } }static irqreturn_t igb_msix_other (int irq,void *data)
drivers/net/ethernet/intel/igb/igb_ptp.c +156 -1
187 lines not on this page · raw message
Signature · 14 lines
1.7.10.4 ------------------------------------------------------------------------------ Try New Relic Now & We'll Send You this Cool Shirt New Relic is the only SaaS-based application performance monitoring service that delivers powerful full stack analytics. Optimize and monitor your browser, app, & servers with just a few lines of code. Try New Relic and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
Quoted 13 lines · 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 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.
Quoted 7 lines
Signed-off-by Richard Cochran <[email protected]>--- 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(-)
Thanks Richard, I will add this to my queue.
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
In reply to Richard CochranQuoted 67 lines · 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 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 <[email protected]>---
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(-)drivers/net/ethernet/intel/igb/igb.h +2
-0@@ -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 drivers/net/ethernet/intel/igb/igb_main.c +21
-0@@ -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); + } +
drivers/net/ethernet/intel/igb/igb_main.c void igb_update_stats(struct igb_adapter *adapter, · line 5077
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_main.c +9
-0+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.
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_main.c +0
-0static irqreturn_t igb_msix_other (int irq,void *data)drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -23,6 +23,15 @@#include "igb.h"
drivers/net/ethernet/intel/igb/igb_ptp.c line 34
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +9
-0+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.
Quoted 93 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +80
-0#define INCVALUE_MASK 0x7fffffff#define ISGN 0x80000000@@ -359,6 +368,86 @@ static int igb_ptp_settime_i210(structptp_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){
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -711,6 +800,68 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, -EFAULT : 0;
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, · line 810
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +8
-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?
Quoted 14 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +14
-0+ } + + 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 ;
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, · line 834
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +10
-0+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?
Quoted 11 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +11
-0+ } + +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 ;
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, · line 855
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +10
-0+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?
Quoted 22 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +10
-1+ } + +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 ;
drivers/net/ethernet/intel/igb/igb_ptp.c void igb_ptp_init(struct igb_adapter *adapter) · line 939
Quoted 4 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +1
-0@@ -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?
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +3
-0+ 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 ()));Signature · 1 line
1.7.10.4
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
Quoted 6 lines · Matthew
On 5/27/13 2:21 AM, "Richard Cochran" <[email protected]> wrote:
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.
I can respin with a comment that the additional bits are i210 only. I think this is better than adding more checks into ISR. Since we only enable these bits for the i210, the checks would be redundant.
Quoted 9 lines
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -23,6 +23,15 @@#include "igb.h"
Quoted 12 lines
drivers/net/ethernet/intel/igb/igb_ptp.c line 34
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +9
-0+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.
This the way I handled it for the PHYTER, and I think it is the best way from our three choices:
1. kconfig option (inflexible)
2. module param
3. ethtool (can o'worms)
Quoted 4 lines
Quoted 4 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -711,6 +800,68 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, -EFAULT : 0; }
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, · line 810
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +8
-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?
Maybe we don't need the return value at all. Just the message is important, I think.
Quoted 7 lines
drivers/net/ethernet/intel/igb/igb_ptp.c void igb_ptp_init(struct igb_adapter *adapter) · line 939
Quoted 5 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +1
-0@@ -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_initfails?
Hmm, maybe we should then disable the extra features... will consider for V2.
Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c void igb_ptp_init(struct igb_adapter *adapter) · line 942
Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +3
-0+ adapter->ptp_caps.n_ext_ts =1 ; + adapter->ptp_caps.n_per_out =1 ; + adapter->ptp_caps.pps =1 ;
Thanks,
Richard
Re: [E1000-devel] [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
Quoted 31 lines · Richard Cochran
Quoted 6 lines · Matthew
On 5/27/13 2:21 AM, "Richard Cochran" <[email protected]> wrote:
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.
I can respin with a comment that the additional bits are i210 only. I think this is better than adding more checks into ISR. Since we only enable these bits for the i210, the checks would be redundant.
Quoted 9 lines
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -23,6 +23,15 @@#include "igb.h" Quoted 11 lines
drivers/net/ethernet/intel/igb/igb_ptp.c line 34
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +9
-0+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.
Quoted 6 lines
This the way I handled it for the PHYTER, and I think it is the best way from our three choices:
1. kconfig option (inflexible)
2. module param
3. ethtool (can o'worms)
I really think the ethtool approach would be preferred since it is the most flexible. The problem with module parameters is that it only really works with one port.
Couldn't you use the ethtool private flags interface or something similar to achieve the same result?
Thanks,
Alex
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
In reply to Richard CochranQuoted 11 lines · Richard Cochran
Quoted 6 lines · Matthew
On 5/27/13 2:21 AM, "Richard Cochran" <[email protected]> wrote:
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.
I can respin with a comment that the additional bits are i210 only. I think this is better than adding more checks into ISR. Since we only enable these bits for the i210, the checks would be redundant.
Personal preference would dictate a MAC check, but I'm alright with a comment. :)
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -23,6 +23,15 @@#include "igb.h" Quoted 13 lines
drivers/net/ethernet/intel/igb/igb_ptp.c line 34
Quoted 9 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +9
-0+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.
Quoted 6 lines
This the way I handled it for the PHYTER, and I think it is the best way from our three choices:
1. kconfig option (inflexible)
2. module param
3. ethtool (can o'worms)
Ah, I didn't realize we had a precedent. Module param is fine with me, then. ethtool does seem like the right long-term solution, but I don't think that should gate your patchset if we already have a precedent for this functionality.
Quoted 16 lines
Quoted 5 lines
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device
Quoted 1 line
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -711,6 +800,68 @@ int igb_ptp_hwtstamp_ioctl(struct net_device*netdev,
Quoted 3 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0-EFAULT : 0; }Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ptp.c int igb_ptp_hwtstamp_ioctl(struct net_device *netdev, · line 810
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +8
-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?
Quoted 2 lines
Maybe we don't need the return value at all. Just the message is important, I think.
I think an error is the right way to go, but if it's not then we should print a warning instead of an error.
Also, on second thought, please use the dev_err or dev_warn macros for consistency with the rest of the codebase.
Quoted 11 lines
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c void igb_ptp_init(struct igb_adapter *adapter) · line 939
Quoted 6 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +1
-0@@ -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_initfails?Hmm, maybe we should then disable the extra features... will consider for V2.
That would be my vote, since we would have failed to initialize that extra feature.
RE: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
Quoted 57 lines
-----Original Message-----
From: Vick, Matthew
Sent: Tuesday, May 28, 2013 10:49 AM
To: Richard Cochran
Cc: [email protected]; David Miller; e1000- [email protected]; Keller, Jacob E; Kirsher, Jeffrey T
Subject: Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.Quoted 2 lines · Richard Cochran
Quoted 1 line · Matthew
On 5/27/13 2:21 AM, "Richard Cochran" <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
I would prefer it if we did a MAC check before these two TSICR
checks,
Quoted 7 lines
Quoted 3 lines
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.
I can respin with a comment that the additional bits are i210 only. I think this is better than adding more checks into ISR. Since we only enable these bits for the i210, the checks would be redundant.
Personal preference would dictate a MAC check, but I'm alright with a comment. :)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +0
-0@@ -23,6 +23,15 @@Quoted 8 lines
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c
Quoted 8 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +6
-0#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 externalevents");
Quoted 2 lines
Quoted 2 lines
drivers/net/ethernet/intel/igb/igb_ptp.c line 33
Quoted 2 lines
drivers/net/ethernet/intel/igb/igb_ptp.c +2
-0+MODULE_PARM_DESC(igb_output_sdp, + "The SDP used to output the programmable periodicsignal");
Quoted 9 lines
Quoted 5 lines
drivers/net/ethernet/intel/igb/igb_ptp.c line 34
Quoted 1 line
drivers/net/ethernet/intel/igb/igb_ptp.c +1
-0+Is there any other mechanism we could use to control this? I would imagine
not, but I know module parameters are generally frowned upon.This the way I handled it for the PHYTER, and I think it is the best way from our three choices:
Quoted 8 lines
Quoted 3 lines
1. kconfig option (inflexible)
2. module param
3. ethtool (can o'worms)Ah, I didn't realize we had a precedent. Module param is fine with me, then. ethtool does seem like the right long-term solution, but I don't think that should gate your patchset if we already have a precedent for this functionality.
I think ethtool would be good, but what about using sysfs?
- Jake
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
Quoted 2 lines · Jacob E
I think ethtool would be good, but what about using sysfs?
The problem I see with ethtool is that configuring GPIOs (and these SDPs are really just GPIOs by a different name) is really the job of the device tree. But of course, most x86 users of the i210 don't have this option.
Also, using ethtool or sysfs is not really attractive for embedded designs (let's not forget the poor embedded guys). I think we need to have a way to specify the pin functions at (or before) kernel boot time. Barring DT and kconfig, using a module parameter is the only way to accomplish this.
Thanks,
Richard
David Miller
maintainerRe: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
From: Richard Cochran <[email protected]>
Date: Wed, 29 May 2013 09:24:45 +0200
Quoted 5 lines
Also, using ethtool or sysfs is not really attractive for embedded designs (let's not forget the poor embedded guys). I think we need to have a way to specify the pin functions at (or before) kernel boot time. Barring DT and kconfig, using a module parameter is the only way to accomplish this.
No module parameters.
The embedded guys can run an ethtool command early in boot right after the driver loads.
Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
In reply to Richard CochranQuoted 9 lines · Richard Cochran
Quoted 2 lines · Jacob E
I think ethtool would be good, but what about using sysfs?
The problem I see with ethtool is that configuring GPIOs (and these SDPs are really just GPIOs by a different name) is really the job of the device tree. But of course, most x86 users of the i210 don't have this option.
Quoted 5 lines
Also, using ethtool or sysfs is not really attractive for embedded designs (let's not forget the poor embedded guys). I think we need to have a way to specify the pin functions at (or before) kernel boot time. Barring DT and kconfig, using a module parameter is the only way to accomplish this.
On x86 the correct place to put this information would presumably be an ACPI table. And if the BIOS doesn't provide it, there is CONFIG_ACPI_CUSTOM_DSDT, though that may be a bit of a sledgehammer...
Ben.
Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
Quoted 7 lines · 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 and SDP1, making it easy to try out this new functionality.
Thanks,
Richard
I have some questions about this series, but thanks for doing this work, Richard!
Cheers,
Matthew
Matthew Vick
Linux Development
Networking Division
Intel Corporation