Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203073 > unrolled thread

[PATCH v2 4/4] Added getsynctime64() callback

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 4/4] Added getsynctime64() callback Christopher Hall <christopher.s.hall@intel.com> - 2015-08-08 01:10 +0200
    Re: [PATCH v2 4/4] Added getsynctime64() callback Richard Cochran <richardcochran@gmail.com> - 2015-08-10 11:00 +0200
      RE: [PATCH v2 4/4] Added getsynctime64() callback "Hall, Christopher S" <christopher.s.hall@intel.com> - 2015-08-13 23:20 +0200
        Re: [PATCH v2 4/4] Added getsynctime64() callback Richard Cochran <richardcochran@gmail.com> - 2015-08-14 08:40 +0200

#1203073 — [PATCH v2 4/4] Added getsynctime64() callback

Reads ART (TSC correlated clocksource), converts to realtime clock, and
	reports	cross timestamp to PTP driver
---
 drivers/net/ethernet/intel/e1000e/defines.h |  7 +++
 drivers/net/ethernet/intel/e1000e/ptp.c     | 88 +++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/regs.h    |  4 ++
 3 files changed, 99 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
index 133d407..9f16269 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -527,6 +527,13 @@
 #define E1000_RXCW_C          0x20000000        /* Receive config */
 #define E1000_RXCW_SYNCH      0x40000000        /* Receive config synch */
 
+/* HH Time Sync */
+#define E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK   0x0000F000      /* max delay */
+#define E1000_TSYNCTXCTL_SYNC_COMP              0x40000000      /* sync complete
+ */
+#define E1000_TSYNCTXCTL_START_SYNC             0x80000000      /* initiate sync
+ */
+
 #define E1000_TSYNCTXCTL_VALID		0x00000001 /* Tx timestamp valid */
 #define E1000_TSYNCTXCTL_ENABLED	0x00000010 /* enable Tx timestamping */
 
diff --git a/drivers/net/ethernet/intel/e1000e/ptp.c b/drivers/net/ethernet/intel/e1000e/ptp.c
index 25a0ad5..c3d80c4 100644
--- a/drivers/net/ethernet/intel/e1000e/ptp.c
+++ b/drivers/net/ethernet/intel/e1000e/ptp.c
@@ -25,6 +25,8 @@
  */
 
 #include "e1000.h"
+#include <asm/tsc.h>
+#include <linux/timekeeping.h>
 
 /**
  * e1000e_phc_adjfreq - adjust the frequency of the hardware clock
@@ -98,6 +100,91 @@ static int e1000e_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
+#define HW_WAIT_COUNT (2)
+#define HW_RETRY_COUNT (2)
+
+static int e1000e_phc_get_ts(struct correlated_ts *cts)
+{
+	struct e1000_adapter *adapter = (struct e1000_adapter *) cts->private;
+	struct e1000_hw *hw = &adapter->hw;
+	int i, j;
+	u32 tsync_ctrl;
+	int ret;
+
+	if (hw->mac.type < e1000_pch_spt)
+		return -EOPNOTSUPP;
+
+	for (j = 0; j < HW_RETRY_COUNT; ++j) {
+		tsync_ctrl = er32(TSYNCTXCTL);
+		tsync_ctrl |= E1000_TSYNCTXCTL_START_SYNC |
+			E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK;
+		ew32(TSYNCTXCTL, tsync_ctrl);
+		ret = 0;
+		for (i = 0; i < HW_WAIT_COUNT; ++i) {
+			udelay(2);
+			tsync_ctrl = er32(TSYNCTXCTL);
+			if (tsync_ctrl & E1000_TSYNCTXCTL_SYNC_COMP)
+				break;
+		}
+
+		if (i == HW_WAIT_COUNT) {
+			ret = -ETIMEDOUT;
+		} else if (ret == 0) {
+			cts->system_ts = er32(PLTSTMPH);
+			cts->system_ts <<= 32;
+			cts->system_ts |= er32(PLTSTMPL);
+			cts->device_ts = er32(SYSSTMPH);
+			cts->device_ts <<= 32;
+			cts->device_ts |= er32(SYSSTMPL);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+#define SYNCTIME_RETRY_COUNT (2)
+
+static int e1000e_phc_getsynctime(struct ptp_clock_info *ptp,
+				  struct timespec64 *devts,
+				  struct timespec64 *systs)
+{
+	struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter,
+						     ptp_clock_info);
+	unsigned long flags;
+	u32 remainder;
+	struct correlated_ts art_correlated_ts;
+	u64 device_time;
+	int i, ret;
+
+	if (!cpu_has_art)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < SYNCTIME_RETRY_COUNT; ++i) {
+		art_correlated_ts.get_ts = e1000e_phc_get_ts;
+		art_correlated_ts.private = adapter;
+		ret = get_correlated_timestamp(&art_correlated_ts,
+					&art_timestamper);
+		if (ret != 0)
+			continue;
+
+		systs->tv_sec =
+			div_u64_rem(art_correlated_ts.system_real.tv64,
+				NSEC_PER_SEC, &remainder);
+		systs->tv_nsec = remainder;
+		spin_lock_irqsave(&adapter->systim_lock, flags);
+		device_time = timecounter_cyc2time(&adapter->tc,
+					art_correlated_ts.device_ts);
+		spin_unlock_irqrestore(&adapter->systim_lock, flags);
+		devts->tv_sec =
+			div_u64_rem(device_time, NSEC_PER_SEC, &remainder);
+		devts->tv_nsec = remainder;
+		break;
+	}
+
+	return ret;
+}
+
 /**
  * e1000e_phc_gettime - Reads the current time from the hardware clock
  * @ptp: ptp clock structure
@@ -190,6 +277,7 @@ static const struct ptp_clock_info e1000e_ptp_clock_info = {
 	.adjfreq	= e1000e_phc_adjfreq,
 	.adjtime	= e1000e_phc_adjtime,
 	.gettime64	= e1000e_phc_gettime,
+	.getsynctime64	= e1000e_phc_getsynctime,
 	.settime64	= e1000e_phc_settime,
 	.enable		= e1000e_phc_enable,
 };
diff --git a/drivers/net/ethernet/intel/e1000e/regs.h b/drivers/net/ethernet/intel/e1000e/regs.h
index b24e5fe..4dd5b54 100644
--- a/drivers/net/ethernet/intel/e1000e/regs.h
+++ b/drivers/net/ethernet/intel/e1000e/regs.h
@@ -246,6 +246,10 @@
 #define E1000_SYSTIML	0x0B600	/* System time register Low - RO */
 #define E1000_SYSTIMH	0x0B604	/* System time register High - RO */
 #define E1000_TIMINCA	0x0B608	/* Increment attributes register - RW */
+#define E1000_SYSSTMPL  0x0B648 /* HH Timesync system stamp low register */
+#define E1000_SYSSTMPH  0x0B64C /* HH Timesync system stamp hi register */
+#define E1000_PLTSTMPL  0x0B640 /* HH Timesync platform stamp low register */
+#define E1000_PLTSTMPH  0x0B644 /* HH Timesync platform stamp hi register */
 #define E1000_RXMTRL	0x0B634	/* Time sync Rx EtherType and Msg Type - RW */
 #define E1000_RXUDP	0x0B638	/* Time Sync Rx UDP Port - RW */
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1203905

On Fri, Aug 07, 2015 at 04:01:35PM -0700, Christopher Hall wrote:
> --- a/drivers/net/ethernet/intel/e1000e/defines.h
> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> @@ -527,6 +527,13 @@
>  #define E1000_RXCW_C          0x20000000        /* Receive config */
>  #define E1000_RXCW_SYNCH      0x40000000        /* Receive config synch */
>  
> +/* HH Time Sync */
> +#define E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK   0x0000F000      /* max delay */
> +#define E1000_TSYNCTXCTL_SYNC_COMP              0x40000000      /* sync complete
> + */
> +#define E1000_TSYNCTXCTL_START_SYNC             0x80000000      /* initiate sync
> + */

Split comment looks bad.  Trim this leading space instead.   ^^^^^^

> @@ -98,6 +100,91 @@ static int e1000e_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	return 0;
>  }
>  
> +#define HW_WAIT_COUNT (2)
> +#define HW_RETRY_COUNT (2)

A busy wait, plus a retry, ...

> +#define SYNCTIME_RETRY_COUNT (2)

plus another retry!

Seems a bit heavy handed to me.  Is the HW really that flakey?

I would expect that a reasonably long polling loop should be
sufficient.  If not, then the HW ignores certain requests, and that is
worth a comment.

In any case, I don't understand why you have two nested retry loops.

> +static int e1000e_phc_getsynctime(struct ptp_clock_info *ptp,
> +				  struct timespec64 *devts,
> +				  struct timespec64 *systs)
> +{
> +	struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter,
> +						     ptp_clock_info);
> +	unsigned long flags;
> +	u32 remainder;
> +	struct correlated_ts art_correlated_ts;
> +	u64 device_time;
> +	int i, ret;
> +
> +	if (!cpu_has_art)
> +		return -EOPNOTSUPP;

Perform this check before registration, setting .getsynctime64
accordingly.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207171

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Monday, August 10, 2015 1:50 AM
> To: Hall, Christopher S
> Cc: john.stultz@linaro.org; tglx@linutronix.de; mingo@redhat.com; Kirsher,
> Jeffrey T; Ronciak, John; hpa@zytor.com; x86@kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] Added getsynctime64() callback
> 
> On Fri, Aug 07, 2015 at 04:01:35PM -0700, Christopher Hall wrote:
> > --- a/drivers/net/ethernet/intel/e1000e/defines.h
> > +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> > @@ -527,6 +527,13 @@
> >  #define E1000_RXCW_C          0x20000000        /* Receive config */
> >  #define E1000_RXCW_SYNCH      0x40000000        /* Receive config synch
> */
> >
> > +/* HH Time Sync */
> > +#define E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK   0x0000F000      /* max
> delay */
> > +#define E1000_TSYNCTXCTL_SYNC_COMP              0x40000000      /* sync
> complete
> > + */
> > +#define E1000_TSYNCTXCTL_START_SYNC             0x80000000      /*
> initiate sync
> > + */
> 
> Split comment looks bad.  Trim this leading space instead.   ^^^^^^

OK.

> 
> > @@ -98,6 +100,91 @@ static int e1000e_phc_adjtime(struct ptp_clock_info
> *ptp, s64 delta)
> >  	return 0;
> >  }
> >
> > +#define HW_WAIT_COUNT (2)
> > +#define HW_RETRY_COUNT (2)
> 
> A busy wait, plus a retry, ...
> 
> > +#define SYNCTIME_RETRY_COUNT (2)
> 
> plus another retry!
> 
> Seems a bit heavy handed to me.  Is the HW really that flakey?
> 
> I would expect that a reasonably long polling loop should be
> sufficient.  If not, then the HW ignores certain requests, and that is
> worth a comment.
> 
> In any case, I don't understand why you have two nested retry loops.

The retry in get_synctime() is a left over from the previous patch.  It's not necessary,
the current timekeeping code won't fail in a way necessitating a retry.  It's removed.

The inner retry loop is due to huge inaccuracies in udelay().  I've done some testing
and it appears udelay(2) actually results in about an 8 microsecond delay.  On
Skylake the time for completion of the cross timestamp should be about 2 microseconds.
If we eliminate the inner most loop we either spin for too long or possibly risk not
waiting long enough.  Are there any guarantees for udelay()?

As for HW_RETRY_LOOP, I will confirm whether this is necessary.  It was in reference
code I was given, but, I agree, it seems odd.

> 
> > +static int e1000e_phc_getsynctime(struct ptp_clock_info *ptp,
> > +				  struct timespec64 *devts,
> > +				  struct timespec64 *systs)
> > +{
> > +	struct e1000_adapter *adapter = container_of(ptp, struct
> e1000_adapter,
> > +						     ptp_clock_info);
> > +	unsigned long flags;
> > +	u32 remainder;
> > +	struct correlated_ts art_correlated_ts;
> > +	u64 device_time;
> > +	int i, ret;
> > +
> > +	if (!cpu_has_art)
> > +		return -EOPNOTSUPP;
> 
> Perform this check before registration, setting .getsynctime64
> accordingly.

The problem here is that ART initialization doesn't happen until we install TSC as a clocksource.  This design is per Thomas' suggestion.  That occurs after the driver is loaded (as a module).

In my somewhat limited testing, it's about 400 ms later.  The problem is the several seconds of TSC frequency refinement.  I, in principle, agree, but we either need to move the ART initialization earlier (probably split it) or defer PTP clock initialization in the driver.

> 
> Thanks,
> Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207349

On Thu, Aug 13, 2015 at 09:10:36PM +0000, Hall, Christopher S wrote:
> > > +	if (!cpu_has_art)
> > > +		return -EOPNOTSUPP;
> > 
> > Perform this check before registration, setting .getsynctime64
> > accordingly.
> 
> The problem here is that ART initialization doesn't happen until we
> install TSC as a clocksource.  This design is per Thomas'
> suggestion.  That occurs after the driver is loaded (as a module).

So that 'cpu_has_art' actually means 'cpu_has_art_and_has_been_initialized'?

In any case, returning EOPNOTSUPP early on, but OK later seems mean to
me.  If the clocks aren't ready yet, the error should be EBUSY so that
user space knows it can try again.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web