[Linuxptp-devel] [PATCH] igb: add timer (alarm) functionality

[Kieran Tyrrell]

implement PTP hardware clock timers (alarms) in the igb ethernet driver

Signed-off-by: Kieran Tyrrell <***@sienda.com>
---
drivers/net/ethernet/intel/igb/igb.h | 4 ++
drivers/net/ethernet/intel/igb/igb_main.c | 34 ++++++++----
drivers/net/ethernet/intel/igb/igb_ptp.c | 92 +++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 5387b3a..ea4023d 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -352,6 +352,7 @@ struct hwmon_buff {

#define IGB_N_EXTTS 2
#define IGB_N_PEROUT 2
+#define IGB_N_ALARM 1
#define IGB_N_SDP 4
#define IGB_RETA_SIZE 128

@@ -473,6 +474,8 @@ struct igb_adapter {
int copper_tries;
struct e1000_info ei;
u16 eee_advert;
+
+ bool timer_enabled;
};

/* flags controlling PTP/1588 function */
@@ -558,6 +561,7 @@ void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, unsigned char *va,
int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
int igb_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
+void igb_tt0_timer_enable(struct igb_adapter *adapter, bool enable);
#ifdef CONFIG_IGB_HWMON
void igb_sysfs_exit(struct igb_adapter *adapter);
int igb_sysfs_init(struct igb_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 942a89f..c55577e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5638,17 +5638,29 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
}

if (tsicr & TSINTR_TT0) {
- spin_lock(&adapter->tmreg_lock);
- ts = timespec64_add(adapter->perout[0].start,
- adapter->perout[0].period);
- /* u32 conversion of tv_sec is safe until y2106 */
- wr32(E1000_TRGTTIML0, ts.tv_nsec);
- wr32(E1000_TRGTTIMH0, (u32)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 (adapter->timer_enabled) {
+ /* disable timer */
+ spin_lock(&adapter->tmreg_lock);
+ igb_tt0_timer_enable(adapter, false);
+ spin_unlock(&adapter->tmreg_lock);
+ event.type = PTP_CLOCK_ALARM;
+ event.index = 0;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
+ else {
+ /* this is a periodic output interrupt */
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec64_add(adapter->perout[0].start,
+ adapter->perout[0].period);
+ /* u32 conversion of tv_sec is safe until y2106 */
+ wr32(E1000_TRGTTIML0, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH0, (u32)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);
+ }
ack |= TSINTR_TT0;
}

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 336c103..2285ad8 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -19,6 +19,7 @@
#include <linux/device.h>
#include <linux/pci.h>
#include <linux/ptp_classify.h>
+#include <linux/posix-timers.h>

#include "igb.h"

@@ -1063,6 +1064,93 @@ int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr)
-EFAULT : 0;
}

+/* tmreg_lock should be held for this call */
+void igb_tt0_timer_enable(struct igb_adapter *adapter, bool enable)
+{
+ u32 tsauxc, tsim;
+ struct e1000_hw *hw = &adapter->hw;
+ tsauxc = rd32(E1000_TSAUXC);
+ tsim = rd32(E1000_TSIM);
+
+ if (enable) {
+ tsauxc |= TSAUXC_EN_TT0;
+ tsim |= TSINTR_TT0;
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ }
+ else {
+ tsauxc &= ~TSAUXC_EN_TT0;
+ tsim &= ~TSINTR_TT0;
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ }
+}
+
+static int igb_ptp_timer_settime_i210(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+ struct igb_adapter *igb =
+ container_of(ptp, struct igb_adapter, ptp_caps);
+ struct e1000_hw *hw = &igb->hw;
+ unsigned long irqsaveflags;
+
+ spin_lock_irqsave(&igb->tmreg_lock, irqsaveflags);
+
+ if (timespec64_to_ns(ts) == 0) {
+ /* disable timer */
+ igb_tt0_timer_enable(igb, false);
+ }
+ else {
+ /* set trigger time and then enable timer */
+ wr32(E1000_TRGTTIML0, ts->tv_nsec);
+ wr32(E1000_TRGTTIMH0, (u32)ts->tv_sec);
+
+ igb_tt0_timer_enable(igb, true);
+ }
+
+ spin_unlock_irqrestore(&igb->tmreg_lock, irqsaveflags);
+
+ return 0;
+}
+
+int igb_ptp_timer_enable_i210(struct ptp_clock_info *ptp, bool enable)
+{
+ struct igb_adapter *igb =
+ container_of(ptp, struct igb_adapter, ptp_caps);
+ struct e1000_hw *hw = &igb->hw;
+ u32 tsauxc;
+ unsigned long flags;
+ bool periodic_output_enabled;
+
+ if (enable) {
+ if(igb->timer_enabled == false) {
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ periodic_output_enabled = tsauxc & (TSAUXC_EN_TT0 | TSAUXC_EN_CLK0 | TSAUXC_ST0);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+ if (periodic_output_enabled) {
+ /* we share the TT0 with the periodic output generator, and that is already enabled */
+ return -EBUSY;
+ }
+
+ igb->timer_enabled = true;
+ return 0;
+ }
+ /* timer is already enabled */
+ return -EINVAL;
+ }
+ else {
+ if(!igb->timer_enabled)
+ return -EINVAL;
+
+ /* disable the timer */
+ igb_tt0_timer_enable(igb, false);
+ igb->timer_enabled = false;
+ }
+
+ return 0;
+}
+
/**
* igb_ptp_init - Initialize PTP functionality
* @adapter: Board private structure
@@ -1125,6 +1213,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
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_alarm = IGB_N_ALARM;
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;
@@ -1136,6 +1225,9 @@ void igb_ptp_init(struct igb_adapter *adapter)
adapter->ptp_caps.settime64 = igb_ptp_settime_i210;
adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
adapter->ptp_caps.verify = igb_ptp_verify_pin;
+
+ adapter->ptp_caps.timerenable = igb_ptp_timer_enable_i210;
+ adapter->ptp_caps.timersettime = igb_ptp_timer_settime_i210;
break;
default:
adapter->ptp_clock = NULL;

[Richard Cochran]

Post by Kieran Tyrrell
@@ -5638,17 +5638,29 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
}
if (tsicr & TSINTR_TT0) {
- spin_lock(&adapter->tmreg_lock);
- ts = timespec64_add(adapter->perout[0].start,
- adapter->perout[0].period);
- /* u32 conversion of tv_sec is safe until y2106 */
- wr32(E1000_TRGTTIML0, ts.tv_nsec);
- wr32(E1000_TRGTTIMH0, (u32)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 (adapter->timer_enabled) {
+ /* disable timer */
+ spin_lock(&adapter->tmreg_lock);
+ igb_tt0_timer_enable(adapter, false);
+ spin_unlock(&adapter->tmreg_lock);
+ event.type = PTP_CLOCK_ALARM;
+ event.index = 0;
+ ptp_clock_event(adapter->ptp_clock, &event);

The functional interface between the PHC layer and the driver should
allow an immediate reprogramming of the HW's interrupt control
registers, don't you think?

Thanks,
Richard

Post by Kieran Tyrrell
+ }
+ else {
+ /* this is a periodic output interrupt */
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec64_add(adapter->perout[0].start,
+ adapter->perout[0].period);
+ /* u32 conversion of tv_sec is safe until y2106 */
+ wr32(E1000_TRGTTIML0, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH0, (u32)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);
+ }
ack |= TSINTR_TT0;
}

------------------------------------------------------------------------------

[Kieran Tyrrell]

Post by Richard Cochran

Post by Kieran Tyrrell
@@ -5638,17 +5638,29 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
}
if (tsicr & TSINTR_TT0) {
- spin_lock(&adapter->tmreg_lock);
- ts = timespec64_add(adapter->perout[0].start,
- adapter->perout[0].period);
- /* u32 conversion of tv_sec is safe until y2106 */
- wr32(E1000_TRGTTIML0, ts.tv_nsec);
- wr32(E1000_TRGTTIMH0, (u32)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 (adapter->timer_enabled) {
+ /* disable timer */
+ spin_lock(&adapter->tmreg_lock);
+ igb_tt0_timer_enable(adapter, false);
+ spin_unlock(&adapter->tmreg_lock);
+ event.type = PTP_CLOCK_ALARM;
+ event.index = 0;
+ ptp_clock_event(adapter->ptp_clock, &event);

The functional interface between the PHC layer and the driver should
allow an immediate reprogramming of the HW's interrupt control
registers, don't you think?

I’m not sure I understand your point here. I would say that the PHC layer should be abstracted from the driver, as we can’t make any assumptions about the hardware. For example on the i210 we must disable the interrupts and/or TT0 before the interrupt handler acknowledges the interrupt, but on other hardware (STM32F4 iirc) the driver could actually leave interrupts enabled as the timer will only trigger when the compare value is EQUAL to the system time (not <=). On these systems it would be very useful for the driver to be able to enable/disable interrupts in the timer_settime() function, so the compare register(s) can be set, interrupts enabled, the current time and interrupt status read, to check if the compare time was set in time (and if it was set too late, fire the timer immediately).

Also on hardware with only a 32bit timer register (where the ‘seconds’ or high nanosecond bits are maintained in the driver) the driver must enable and disable interrupts depending on counter rollovers etc.

But maybe I have misunderstood your point…

Thanks, Kieran.




------------------------------------------------------------------------------

[Richard Cochran]

Post by Kieran Tyrrell
But maybe I have misunderstood your point…

The existing code immediately re-configures TRGTTIML/H and re-enables
TSAUXC_EN_TT for periodic outputs. This is a time critical operation
that should not be delayed.

The ptp_clock_event() method (or some new function) could return the
next deadline immediately, depending on the state of the timer queue.

That is all I meant.

Thanks,
Richard

------------------------------------------------------------------------------

[Kieran Tyrrell]

Post by Richard Cochran

Post by Kieran Tyrrell
But maybe I have misunderstood your point…

The existing code immediately re-configures TRGTTIML/H and re-enables
TSAUXC_EN_TT for periodic outputs. This is a time critical operation
that should not be delayed.
The ptp_clock_event() method (or some new function) could return the
next deadline immediately, depending on the state of the timer queue.
That is all I meant.

Ah ok, yes that makes sense. I have modified the code so that in the case of a PTP_CLOCK_ALARM event ptp_clock_event() will return the next fire time in the ptp_clock_event structure. That’s much more direct than how I had it before.

Thanks, Kieran.


------------------------------------------------------------------------------