[PATCH v3 0/4] Patchset enabling hardware based cross-timestamps for next gen Intel platforms

[PATCH v3 0/4] Patchset enabling hardware based cross-timestamps for next gen Intel platforms

[Christopher S. Hall]

6th generation Intel platforms will have an Always Running
Timer (ART) that always runs when the system is powered and
is available to both the CPU and various on-board devices.
Initially, those devices include audio and network.  The
ART will give these devices the capability of precisely
cross timestamping their local device clock with the system
clock.  The ART is precisely related to the TSC by a ratio
read from CPUID leaf 0x15.

A device (such as the network controller) produces cross timestamps in 
terms of the ART and the local device clock.  The ART value on its
own isn't useful.

The first two patches enable translation of ART to system time.
The first patch adds the correlated clocksource concept which is
an auxiliary clock directly relate-able to a clock registered as
a clocksource. The second patch adds the Intel specific ART 
correlated clocksource.

The last two patches modify the PTP character driver to call a
cross timestamp function (getsynctime()) in the driver when
available and perform the cross timestamp in the e1000e driver.

The patches taken together enable sub-microsecond cross timestamps
between the system clock and network device clock

Changelog since v2:

Split out x86 architecture specific code from common timekeeping code
	additions

Split ART initialization between early TSC initialization and TSC
	frequency refinement.  Now, cpu_has_art can be used in
	driver initialization code

Added e1000e PTP init code that detects presence of ART/spt disabling
	cross timestamp if they're not available

Added additional commenting in TSC/ART init code, minor renaming of
	functions and variables for greater clarity

Fixed a few formatting problems in e1000e driver patch


Christopher Hall (2):
  Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl
  Enabling hardware supported PTP system/device crosstimestamping

Christopher S. Hall (2):
  Add correlated clocksource deriving system time from an auxiliary
    clocksource
  Added ART correlated clocksource and ART CPU feature

 Documentation/ptp/testptp.c                 |  6 +-
 arch/x86/include/asm/cpufeature.h           |  3 +-
 arch/x86/include/asm/tsc.h                  |  2 +
 arch/x86/kernel/tsc.c                       | 54 ++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/defines.h |  5 ++
 drivers/net/ethernet/intel/e1000e/ptp.c     | 88 +++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/regs.h    |  4 ++
 drivers/ptp/ptp_chardev.c                   | 29 +++++++---
 include/linux/clocksource.h                 | 33 +++++++++++
 include/linux/ptp_clock_kernel.h            |  7 +++
 include/linux/timekeeping.h                 |  4 ++
 include/uapi/linux/ptp_clock.h              |  4 +-
 kernel/time/timekeeping.c                   | 65 +++++++++++++++++++++
 13 files changed, 292 insertions(+), 12 deletions(-)

-- 
2.1.4

[PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Christopher S. Hall]

Add struct correlated_cs with pointer to original clocksource and
	function pointer to convert correlated clocksource to the original

Add get_correlated_timestamp() function which given specific correlated_cs
	and correlated_ts convert correlated counter value to system time

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 include/linux/clocksource.h | 33 +++++++++++++++++++++++
 include/linux/timekeeping.h |  4 +++
 kernel/time/timekeeping.c   | 65 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 278dd27..4bedadb 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -258,4 +258,37 @@ void acpi_generic_timer_init(void);
 static inline void acpi_generic_timer_init(void) { }
 #endif
 
+/*
+ * struct correlated_cs - Descriptor for a clocksource correlated to another
+ *	clocksource
+ * @related_cs:		Pointer to the related timekeeping clocksource
+ * @convert:		Conversion function to convert a timestamp from
+ *			the correlated clocksource to cycles of the related
+ *			timekeeping clocksource
+ */
+struct correlated_cs {
+	struct clocksource	*related_cs;
+	u64			(*convert)(struct correlated_cs *cs,
+					   u64 cycles);
+};
+
+struct correlated_ts;
+
+/**
+ * struct correlated_ts - Descriptor for taking a correlated time stamp
+ * @get_ts:		Function to read out a synced system and device
+ *			timestamp
+ * @system_ts:		The raw system clock timestamp
+ * @device_ts:		The raw device timestamp
+ * @system_real:	@system_ts converted to CLOCK_REALTIME
+ * @system_raw:		@system_ts converted to CLOCK_MONOTONIC_RAW
+ */
+struct correlated_ts {
+	int			(*get_ts)(struct correlated_ts *ts);
+	u64			system_ts;
+	u64			device_ts;
+	ktime_t			system_real;
+	ktime_t			system_raw;
+	void			*private;
+};
 #endif /* _LINUX_CLOCKSOURCE_H */
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 6e191e4..a9e1a2d 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -258,6 +258,10 @@ extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
  */
 extern void getnstime_raw_and_real(struct timespec *ts_raw,
 				   struct timespec *ts_real);
+struct correlated_ts;
+struct correlated_cs;
+extern int get_correlated_timestamp(struct correlated_ts *crt,
+				    struct correlated_cs *crs);
 
 /*
  * Persistent clock related interfaces
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index bca3667..90a7c6f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -312,6 +312,19 @@ static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
 	return nsec + arch_gettimeoffset();
 }
 
+static inline s64 timekeeping_convert_to_ns(struct tk_read_base *tkr,
+					    cycle_t cycles)
+{
+	cycle_t delta;
+	s64 nsec;
+
+	/* calculate the delta since the last update_wall_time */
+	delta = clocksource_delta(cycles, tkr->cycle_last, tkr->mask);
+
+	nsec = delta * tkr->mult + tkr->xtime_nsec;
+	return nsec >> tkr->shift;
+}
+
 /**
  * update_fast_timekeeper - Update the fast and NMI safe monotonic timekeeper.
  * @tkr: Timekeeping readout base from which we take the update
@@ -885,6 +898,58 @@ EXPORT_SYMBOL(getnstime_raw_and_real);
 #endif /* CONFIG_NTP_PPS */
 
 /**
+ * get_correlated_timestamp - Get a correlated timestamp
+ *
+ * Reads a timestamp from a device and correlates it to system time
+ */
+int get_correlated_timestamp(struct correlated_ts *crt,
+			     struct correlated_cs *crs)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned long seq;
+	cycles_t cycles;
+	ktime_t base;
+	s64 nsecs;
+	int ret;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		/*
+		 * Verify that the correlated clocksoure is related to
+		 * the currently installed timekeeper clocksoure
+		 */
+		if (tk->tkr_mono.clock != crs->related_cs)
+			return -ENODEV;
+
+		/*
+		 * Try to get a timestamp from the device.
+		 */
+		ret = crt->get_ts(crt);
+		if (ret)
+			return ret;
+
+		/*
+		 * Convert the timestamp to timekeeper clock cycles
+		 */
+		cycles = crs->convert(crs, crt->system_ts);
+
+		/* Convert to clock realtime */
+		base = ktime_add(tk->tkr_mono.base,
+				 tk_core.timekeeper.offs_real);
+		nsecs = timekeeping_convert_to_ns(&tk->tkr_mono, cycles);
+		crt->system_real = ktime_add_ns(base, nsecs);
+
+		/* Convert to clock raw monotonic */
+		base = tk->tkr_raw.base;
+		nsecs = timekeeping_convert_to_ns(&tk->tkr_raw, cycles);
+		crt->system_raw = ktime_add_ns(base, nsecs);
+
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+	return 0;
+}
+EXPORT_SYMBOL(get_correlated_timestamp);
+
+/**
  * do_gettimeofday - Returns the time of day in a timeval
  * @tv:		pointer to the timeval to be set
  *
-- 
2.1.4

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Thomas Gleixner]

On Fri, 21 Aug 2015, Christopher S. Hall wrote:

> Add struct correlated_cs with pointer to original clocksource and
> 	function pointer to convert correlated clocksource to the original
> 
> Add get_correlated_timestamp() function which given specific correlated_cs
> 	and correlated_ts convert correlated counter value to system time

This is not a proper changelog.

1) The subject line lacks a subsystem prefix

   timekeeping:

   Is the proper choice here

2) The subject line should be short and precise

   timekeeping: Add mechanism to gather correlated timestamps

   Might be an informative one.

3) The changelog itself should describe the reason why we want this
   change, the purpose of the change etc.

   Add foo
   Add bar

   Is pointless because we can see that from the patch itself.

   What the patch cannot not explain is the WHY. That's what the
   changelog is for.

4) You dropped the authorship

   The proper way to do this is to add a 'FROM: author' at the top of
   the changelog body.

As I wrote the patch, so I give you a changelog along with it:

<---
Subject: timekeeping: Add mechanism to gather correlated timestamps

From: Thomas Gleixner <tglx@linutronix.de>

Modern Intel hardware provides the so called Always Running Timer
(ART). The TSC which is usually used for timekeeping is derived from
ART and runs with a fixed frequency ratio to it. ART is routed to
devices and allows to take atomic timestamp samples from the device
clock and the ART. One use case is PTP timestamps on network cards. We
want to utilize this feature as it allows us to better correlate the
PTP timestamp to the system time.

In order to gather precise timestamps we need to make sure that the
conversion from ART to TSC and the following conversion from TSC to
clock realtime happens synchronized with the ongoing timekeeping
updates. Otherwise we might convert an ART timestamp from point A in
time with the conversion factors of point B in time. These conversion
factors can differ due to NTP/PTP frequency adjustments and therefor
the resulting clock realtime timestamp would be slightly off, which is
contrary to the whole purpose of synchronized hardware timestamps.

Provide data structures which describe the correlation between two
clocksources and a function to gather correlated and convert
timestamps from a device. The function is as any other timekeeping
function protected against current timekeeper updates via the
timekeeper sequence lock. It calls the device function to gather the
hardware timestamps and converts them to clock real time and clock
monotonic raw.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---->

Can you see the difference?

> Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
> ---
>  include/linux/clocksource.h | 33 +++++++++++++++++++++++
>  include/linux/timekeeping.h |  4 +++
>  kernel/time/timekeeping.c   | 65 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
> 
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 278dd27..4bedadb 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -258,4 +258,37 @@ void acpi_generic_timer_init(void);
>  static inline void acpi_generic_timer_init(void) { }
>  #endif
>  
> +/*
> + * struct correlated_cs - Descriptor for a clocksource correlated to another
> + *	clocksource

Don't believe checkpatch here. KernelDoc requires that this is one
line, 80 char limit or not.

>  /**
> + * get_correlated_timestamp - Get a correlated timestamp
> + *

Lacks the parameter documentation:

* @crt: Pointer to a correlated timestamp structure which provides
*	the device specific timestamp function and is used to store
*	the raw and the correlated timestamps.
* @crs:	Pointer to a correlated clocksource structure which describes 
* 	the correlated clocksource and provides a conversion function
*	to the timekeeping clocksource

> +	return 0;
> +}
> +EXPORT_SYMBOL(get_correlated_timestamp);

EXPORT_SYMBOL_GPL please.

Thanks,

	tglx

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Hall, Christopher S]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, August 22, 2015 1:17 PM
> To: Hall, Christopher S
> Cc: Kirsher, Jeffrey T; hpa@zytor.com; mingo@redhat.com;
> john.stultz@linaro.org; richardcochran@gmail.com; x86@kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; peterz@infradead.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
>  
> > +/**
> > + * get_correlated_timestamp - Get a correlated timestamp
> > + *
> > + * Reads a timestamp from a device and correlates it to system time
> > + */
> > +int get_correlated_timestamp(struct correlated_ts *crt,
> > +			     struct correlated_cs *crs)
> > +{
> > +	struct timekeeper *tk = &tk_core.timekeeper;
> > +	unsigned long seq;
> > +	cycles_t cycles;
> > +	ktime_t base;
> > +	s64 nsecs;
> > +	int ret;
> > +
> > +	do {
> > +		seq = read_seqcount_begin(&tk_core.seq);
> > +		/*
> > +		 * Verify that the correlated clocksoure is related to
> > +		 * the currently installed timekeeper clocksoure
> > +		 */
> > +		if (tk->tkr_mono.clock != crs->related_cs)
> > +			return -ENODEV;
> > +
> > +		/*
> > +		 * Try to get a timestamp from the device.
> > +		 */
> > +		ret = crt->get_ts(crt);
> > +		if (ret)
> > +			return ret;
> > +
[Re-added code for context]

In addition to the network interface, ART will be used in the audio interface as well.
We need to support the case where an audio co-processor will control the audio device.
In this case, the get_ts() function supplied by the audio driver will be very slow
(several milliseconds) and the result will be out of date by some fraction of that 
amount.

This loop makes strict requirements on the latency and recency. Is it possible to relax
that requirement in some way?

For example, supply the ART value as an argument and, in the case of the realtime
clock, keep a short history of clock changes.  It would fail in cases where there
are a lot of calls to adjtimex(), but it will would work most of the time.

What can you suggest? Thanks

Chris

> > +	} while (read_seqcount_retry(&tk_core.seq, seq));
> > +	return 0;
> > +}

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Richard Cochran]

On Thu, Sep 03, 2015 at 11:20:37PM +0000, Hall, Christopher S wrote:
> In addition to the network interface, ART will be used in the audio interface as well.
> We need to support the case where an audio co-processor will control the audio device.
> In this case, the get_ts() function supplied by the audio driver will be very slow
> (several milliseconds) and the result will be out of date by some fraction of that 
> amount.

Why does it take milliseconds to read one audio time stamp?

Thanks,
Richard

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Peter Zijlstra]

On Fri, Sep 04, 2015 at 10:11:22AM +0200, Richard Cochran wrote:
> On Thu, Sep 03, 2015 at 11:20:37PM +0000, Hall, Christopher S wrote:
> > In addition to the network interface, ART will be used in the audio interface as well.
> > We need to support the case where an audio co-processor will control the audio device.
> > In this case, the get_ts() function supplied by the audio driver will be very slow
> > (several milliseconds) and the result will be out of date by some fraction of that 
> > amount.
> 
> Why does it take milliseconds to read one audio time stamp?

So what I suspect, but please correct me if I'm wrong Chris, is that a
DSP will buffer and process audio signals, and only later wake up the
main CPU.

So by the time the CPU is made aware of the data, it's 'old'.

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Hall, Christopher S]

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Friday, September 04, 2015 7:28 AM
> To: Richard Cochran
> Cc: Hall, Christopher S; Thomas Gleixner; Kirsher, Jeffrey T;
> hpa@zytor.com; mingo@redhat.com; john.stultz@linaro.org; x86@kernel.org;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
> 
> > > (several milliseconds) and the result will be out of date by some
> fraction of that
> > > amount.
> >
> > Why does it take milliseconds to read one audio time stamp?
> 
> So what I suspect, but please correct me if I'm wrong Chris, is that a
> DSP will buffer and process audio signals, and only later wake up the
> main CPU.
> 
> So by the time the CPU is made aware of the data, it's 'old'.

That's about right.  The DSP runs on a 1 ms cadence.  Any access to registers controlled by the DSP will take 1-2 DSP ticks to access.

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Thomas Gleixner]

On Thu, 3 Sep 2015, Hall, Christopher S wrote:

Can you please teach your mail client to add proper line breaks around
80? Your mail renders horrible in a text based mail client.

> In addition to the network interface, ART will be used in the audio
> interface as well.  We need to support the case where an audio
> co-processor will control the audio device.  In this case, the
> get_ts() function supplied by the audio driver will be very slow
> (several milliseconds) and the result will be out of date by some
> fraction of that amount.

You are not telling at all, what this driver is supposed to do, what
this get_ts() function is for and how that co-processor thing works.

You just make claims, that you need this without explaining WHY. And
that WHY is the most interesting part.

> This loop makes strict requirements on the latency and recency. Is
> it possible to relax that requirement in some way?

No. This function is explicitely for the precise timestamp usecase,
which is required by PTP and other sane use cases.

> For example, supply the ART value as an argument and, in the case of
> the realtime clock, keep a short history of clock changes.  It would

It's not only clock realtime which is affected by those.

> fail in cases where there are a lot of calls to adjtimex(),

That has nothing to do with lots of adjtimex calls. The kernel does a
slow correction of the conversion values itself to avoid time jumping
around.

> but it will would work most of the time.

Will, would, most? - Could, perhaps, sometimes?

Looks like a design from the trainwreck engineering departement. We
want to have it very precise, but we don't care if it behaves like a
random number generator.

Can you folks please get your act together and provide coherent
explanations about the usecase and the constraints instead of
proposing random functions with obscure semantics?

Thanks,

	tglx

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Peter Zijlstra]

On Fri, Sep 04, 2015 at 03:02:19PM +0200, Thomas Gleixner wrote:
> > For example, supply the ART value as an argument and, in the case of
> > the realtime clock, keep a short history of clock changes.  It would
> 
> It's not only clock realtime which is affected by those.
> 
> > fail in cases where there are a lot of calls to adjtimex(),
> 
> That has nothing to do with lots of adjtimex calls. The kernel does a
> slow correction of the conversion values itself to avoid time jumping
> around.

I think what they're getting at is asking if there's a rate limit to
time adjustments, without that, saving the last n transition points will
still not cover any given length of history.

So what I think they're looking for; is given an upper bound on the DSP
delaying its data, come up with a fixed minimal amount of transitions
points we must store to cover the history.

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Richard Cochran]

On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> I think what they're getting at is asking if there's a rate limit to
> time adjustments, without that, saving the last n transition points will
> still not cover any given length of history.

As if the ntp code isn't complex enough already - now we're adding
sample histories and adjustment rating limiting?

And all for some unknown DSP in a mythical sound card??


Thanks,
Richard

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Peter Zijlstra]

On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > I think what they're getting at is asking if there's a rate limit to
> > time adjustments, without that, saving the last n transition points will
> > still not cover any given length of history.
> 
> As if the ntp code isn't complex enough already - now we're adding
> sample histories and adjustment rating limiting?
> 
> And all for some unknown DSP in a mythical sound card??

Hehe, I'm just a 'translator' here. But going by you answer I'm taking
it there isn't in fact a rate-limit to adjustments. Which, even if you
were not opposed to that direction, makes it an unfeasible proposition.

Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
audio DSP stuff, I think a newer version will just gain ART support.

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Thomas Gleixner]

On Fri, 4 Sep 2015, Peter Zijlstra wrote:
> On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> > On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > > I think what they're getting at is asking if there's a rate limit to
> > > time adjustments, without that, saving the last n transition points will
> > > still not cover any given length of history.
> > 
> > As if the ntp code isn't complex enough already - now we're adding
> > sample histories and adjustment rating limiting?
> > 
> > And all for some unknown DSP in a mythical sound card??
> 
> Hehe, I'm just a 'translator' here. But going by you answer I'm taking
> it there isn't in fact a rate-limit to adjustments. Which, even if you
> were not opposed to that direction, makes it an unfeasible proposition.
> 
> Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
> audio DSP stuff, I think a newer version will just gain ART support.

Right, but we still do not know how that is going to be used. And
that's the key question. As long as that is not answered all can do is
wild guessing.

Thanks,

	tglx

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Hall, Christopher S]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Friday, September 04, 2015 9:35 AM
> To: Peter Zijlstra
> Cc: Richard Cochran; Hall, Christopher S; Kirsher, Jeffrey T;
> hpa@zytor.com; mingo@redhat.com; john.stultz@linaro.org; x86@kernel.org;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [PATCH v3 1/4] Add correlated clocksource deriving system time
> from an auxiliary clocksource
> 
> On Fri, 4 Sep 2015, Peter Zijlstra wrote:
> > On Fri, Sep 04, 2015 at 05:17:43PM +0200, Richard Cochran wrote:
> > > On Fri, Sep 04, 2015 at 05:10:21PM +0200, Peter Zijlstra wrote:
> > > > I think what they're getting at is asking if there's a rate limit to
> > > > time adjustments, without that, saving the last n transition points
> will
> > > > still not cover any given length of history.
> > >
> > > As if the ntp code isn't complex enough already - now we're adding
> > > sample histories and adjustment rating limiting?
> > >
> > > And all for some unknown DSP in a mythical sound card??
> >
> > Hehe, I'm just a 'translator' here. But going by you answer I'm taking
> > it there isn't in fact a rate-limit to adjustments. Which, even if you
> > were not opposed to that direction, makes it an unfeasible proposition.
> >
> > Also, I'm not thinking its too mythical, sound/soc/intel/ is full of
> > audio DSP stuff, I think a newer version will just gain ART support.
> 
> Right, but we still do not know how that is going to be used. And
> that's the key question. As long as that is not answered all can do is
> wild guessing.

It's not wild guessing.  We do have it working on other OSs and have a pretty good 
idea of how it will work.  The DSP firmware will be largely identical for Linux.  I 
think now, we have a chicken and egg problem.

We can't post audio drivers that break, or are broken by, the current ART interface.  
How do I move this forward?  Should I minimally (I don't know exactly what that means 
just yet) rewrite the ART interface so that the audio driver is mostly not broken and 
post that along with the audio driver code?  Is this an acceptable approach?

> 
> Thanks,
> 
> 	tglx

RE: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Thomas Gleixner]

On Fri, 4 Sep 2015, Hall, Christopher S wrote:
> > Right, but we still do not know how that is going to be used. And
> > that's the key question. As long as that is not answered all can do is
> > wild guessing.
> 
> It's not wild guessing.  We do have it working on other OSs and have
> a pretty good idea of how it will work.  The DSP firmware will be
> largely identical for Linux.  I think now, we have a chicken and egg
> problem.

You have a totally different problem. You are just refusing to explain
how all that stuff is supposed to work and what kind of functionality
you need exactly.

Is it that hard to describe the technical requirements and the
presumably assbackwards restrictions of the firmware?

Thanks,

	tglx

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 4 Sep 2015, Hall, Christopher S wrote:
> > > Right, but we still do not know how that is going to be used. And
> > > that's the key question. As long as that is not answered all can do is
> > > wild guessing.
> > 
> > It's not wild guessing.  We do have it working on other OSs and have a pretty 
> > good idea of how it will work.  The DSP firmware will be largely identical for 
> > Linux.  I think now, we have a chicken and egg problem.
> 
> You have a totally different problem. You are just refusing to explain how all 
> that stuff is supposed to work and what kind of functionality you need exactly.

Yeah, so I'm just going to NAK this until things are improved:

   NAKed-by: Ingo Molnar <mingo@kernel.org>

it's not like we are overly bored in timekeeping and need the extra complexity as 
much as possible.

Proper, comprehensive, proactive technical description is needed, with proper 
changelogs, not just half-baked notes. If all that is fixed I'll lift my NAK.

Thanks,

	Ingo

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[Richard Cochran]

On Fri, Sep 04, 2015 at 03:02:19PM +0200, Thomas Gleixner wrote:
> No. This function is explicitely for the precise timestamp usecase,
> which is required by PTP and other sane use cases.

Right.  The audio department only needs to know the (ART, ptp) offset.
The kernel and user space never need the (ART, mediaclock) offset.
That is private information for the DSP.

As long as user space reads (ART, ptp) and provides this regulary to
the audio DSP, then the DSP will have all the information it needs to
figure out (ptp, mediaclock).

Thanks,
Richard

Re: [PATCH v3 1/4] Add correlated clocksource deriving system time from an auxiliary clocksource

[John Stultz]

On Thu, Sep 3, 2015 at 4:20 PM, Hall, Christopher S
<christopher.s.hall@intel.com> wrote:
> For example, supply the ART value as an argument and, in the case of the realtime
> clock, keep a short history of clock changes.  It would fail in cases where there
> are a lot of calls to adjtimex(), but it will would work most of the time.

So, I really don't think something like this would be reasonable. For
one, keeping track of the adjtimex adjustments would be difficult
enough to do sanely, but the real issue is that the clock has its own
long-term error correction adjustments that it does in order to keep
long term frequency accuracy with coarsely adjusted clocksources.
Trying to track those small oscillation intervals would be even more
complicated.

I still think that being able to calculate the CLOCK_MONOTONIC_RAW
value for a given ART counter value is reasonable, and then one can
use the getnstime_raw_and_real() to get a current raw/real sync point,
which you can then calculate the raw delta, and subtract that from the
sycned real timestamp.

You're error there would be bound by the maxium clocksource adjustment
rate * the raw-delta interval length.

To clarify on the need to understand if this error would be
reasonable, can you provide a sense of what the delay from an ART read
to trying to calculate a REALTIME value might be?

thanks
-john

[PATCH v3 2/4] Added ART correlated clocksource and ART CPU feature

[Christopher S. Hall]

Add detect_art() call to early TSC initialization which reads ART->TSC
	numerator/denominator and sets CPU feature if present

Add convert_art_to_tsc() function performing conversion ART to TSC

Add art_timestamp referencing art_to_tsc() and clocksource_tsc enabling
	driver conversion of ART to TSC

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 arch/x86/include/asm/cpufeature.h |  3 ++-
 arch/x86/include/asm/tsc.h        |  2 ++
 arch/x86/kernel/tsc.c             | 54 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 3d6606f..a9322e5 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -85,7 +85,7 @@
 #define X86_FEATURE_P4		( 3*32+ 7) /* "" P4 */
 #define X86_FEATURE_CONSTANT_TSC ( 3*32+ 8) /* TSC ticks at a constant rate */
 #define X86_FEATURE_UP		( 3*32+ 9) /* smp kernel running on up */
-/* free, was #define X86_FEATURE_FXSAVE_LEAK ( 3*32+10) * "" FXSAVE leaks FOP/FIP/FOP */
+#define X86_FEATURE_ART		(3*32+10) /* Platform has always running timer (ART) */
 #define X86_FEATURE_ARCH_PERFMON ( 3*32+11) /* Intel Architectural PerfMon */
 #define X86_FEATURE_PEBS	( 3*32+12) /* Precise-Event Based Sampling */
 #define X86_FEATURE_BTS		( 3*32+13) /* Branch Trace Store */
@@ -352,6 +352,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define cpu_has_de		boot_cpu_has(X86_FEATURE_DE)
 #define cpu_has_pse		boot_cpu_has(X86_FEATURE_PSE)
 #define cpu_has_tsc		boot_cpu_has(X86_FEATURE_TSC)
+#define cpu_has_art		boot_cpu_has(X86_FEATURE_ART)
 #define cpu_has_pge		boot_cpu_has(X86_FEATURE_PGE)
 #define cpu_has_apic		boot_cpu_has(X86_FEATURE_APIC)
 #define cpu_has_sep		boot_cpu_has(X86_FEATURE_SEP)
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 94605c0..8d52d91 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -45,6 +45,8 @@ static __always_inline cycles_t vget_cycles(void)
 	return (cycles_t)__native_read_tsc();
 }
 
+extern struct correlated_cs art_timestamper;
+
 extern void tsc_init(void);
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 7437b41..13f12e0 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -939,10 +939,36 @@ static struct notifier_block time_cpufreq_notifier_block = {
 	.notifier_call  = time_cpufreq_notifier
 };
 
+#define ART_CPUID_LEAF (0x15)
+#define ART_MIN_DENOMINATOR (2)
+
+static u32 art_to_tsc_numerator;
+static u32 art_to_tsc_denominator;
+
+/*
+ * If ART is present detect the numberator:denominator to convert to TSC
+ */
+void detect_art(void)
+{
+	unsigned int unused[2];
+
+	if (boot_cpu_data.cpuid_level >= ART_CPUID_LEAF) {
+		cpuid(ART_CPUID_LEAF, &art_to_tsc_denominator,
+		      &art_to_tsc_numerator, unused, unused+1);
+
+		if (art_to_tsc_denominator >= ART_MIN_DENOMINATOR) {
+			set_cpu_cap(&boot_cpu_data, X86_FEATURE_ART);
+		}
+	}
+}
+
 static int __init cpufreq_tsc(void)
 {
 	if (!cpu_has_tsc)
 		return 0;
+
+	detect_art();
+
 	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
 		return 0;
 	cpufreq_register_notifier(&time_cpufreq_notifier_block,
@@ -1059,6 +1085,32 @@ int unsynchronized_tsc(void)
 	return 0;
 }
 
+/*
+ * Convert ART to TSC given numerator/denominator found in detect_art()
+ */
+static u64 convert_art_to_tsc(struct correlated_cs *cs, u64 cycles)
+{
+	u64 tmp, res;
+
+	switch (art_to_tsc_denominator) {
+	default:
+		res = (cycles / art_to_tsc_denominator) * art_to_tsc_numerator;
+		tmp = (cycles % art_to_tsc_denominator) * art_to_tsc_numerator;
+		res += tmp / art_to_tsc_denominator;
+		break;
+	case 2:
+	       res = (cycles >> 1) * art_to_tsc_numerator;
+	       tmp = (cycles & 0x1) * art_to_tsc_numerator;
+	       res += tmp >> 1;
+	       break;
+	}
+	return res;
+}
+
+struct correlated_cs art_timestamper = {
+	.convert	= convert_art_to_tsc,
+};
+EXPORT_SYMBOL(art_timestamper);
 
 static void tsc_refine_calibration_work(struct work_struct *work);
 static DECLARE_DELAYED_WORK(tsc_irqwork, tsc_refine_calibration_work);
@@ -1130,6 +1182,8 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		(unsigned long)tsc_khz % 1000);
 
 out:
+	if (cpu_has_art)
+		art_timestamper.related_cs = &clocksource_tsc;
 	clocksource_register_khz(&clocksource_tsc, tsc_khz);
 }
 
-- 
2.1.4

Re: [PATCH v3 2/4] Added ART correlated clocksource and ART CPU feature

[Thomas Gleixner]

On Fri, 21 Aug 2015, Christopher S. Hall wrote:

> Add detect_art() call to early TSC initialization which reads ART->TSC
> 	numerator/denominator and sets CPU feature if present
> 
> Add convert_art_to_tsc() function performing conversion ART to TSC
> 
> Add art_timestamp referencing art_to_tsc() and clocksource_tsc enabling
> 	driver conversion of ART to TSC

That changelog needs a rewrite. See patch 1/4

> @@ -352,6 +352,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>  #define cpu_has_de		boot_cpu_has(X86_FEATURE_DE)
>  #define cpu_has_pse		boot_cpu_has(X86_FEATURE_PSE)
>  #define cpu_has_tsc		boot_cpu_has(X86_FEATURE_TSC)
> +#define cpu_has_art		boot_cpu_has(X86_FEATURE_ART)

Please do not add more cpu_has macros. There is nothing wrong to write
boot_cpu_has(X86_FEATURE_ART) in the code.

> +#define ART_CPUID_LEAF (0x15)
> +#define ART_MIN_DENOMINATOR (2)

#define ART_CPUID_LEAF	       0x15
#define ART_MIN_DENOMINATOR    2

Why is the minimum denominator 2? That wants a comment.

> +static u32 art_to_tsc_numerator;
> +static u32 art_to_tsc_denominator;

Both want to be read_mostly

> +/*
> + * If ART is present detect the numberator:denominator to convert to TSC
> + */
> +void detect_art(void)
> +{
> +	unsigned int unused[2];
> +
> +	if (boot_cpu_data.cpuid_level >= ART_CPUID_LEAF) {
> +		cpuid(ART_CPUID_LEAF, &art_to_tsc_denominator,
> +		      &art_to_tsc_numerator, unused, unused+1);
> +
> +		if (art_to_tsc_denominator >= ART_MIN_DENOMINATOR) {
> +			set_cpu_cap(&boot_cpu_data, X86_FEATURE_ART);
> +		}

No parentheses around one liners please.

> +	}
> +}
> +
>  static int __init cpufreq_tsc(void)
>  {
>  	if (!cpu_has_tsc)
>  		return 0;
> +
> +	detect_art();
> +
>  	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>  		return 0;
>  	cpufreq_register_notifier(&time_cpufreq_notifier_block,
> @@ -1059,6 +1085,32 @@ int unsynchronized_tsc(void)
>  	return 0;
>  }
>  
> +/*
> + * Convert ART to TSC given numerator/denominator found in detect_art()
> + */
> +static u64 convert_art_to_tsc(struct correlated_cs *cs, u64 cycles)
> +{
> +	u64 tmp, res;
> +
> +	switch (art_to_tsc_denominator) {
> +	default:
> +		res = (cycles / art_to_tsc_denominator) * art_to_tsc_numerator;
> +		tmp = (cycles % art_to_tsc_denominator) * art_to_tsc_numerator;
> +		res += tmp / art_to_tsc_denominator;
> +		break;
> +	case 2:
> +	       res = (cycles >> 1) * art_to_tsc_numerator;
> +	       tmp = (cycles & 0x1) * art_to_tsc_numerator;
> +	       res += tmp >> 1;
> +	       break;

Is it really worth do do this optimization? And if we do it we
shouldn't special case it for 2. You can check at ART detection time
whether the denominator is a power of two and have a flag which
selects a div/mod base or a shift based conversion.

Thanks,

	tglx

[PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Christopher S. Hall]

From: Christopher Hall <christopher.s.hall@intel.com>

This patch allows system and device time ("cross-timestamp") to be
performed by the driver. Currently, the cross-timestamping is performed
in the PTP_SYS_OFFSET ioctl.  The PTP clock driver reads gettimeofday()
and the gettime64() callback provided by the driver. The cross-timestamp
is best effort where the latency between the capture of system time
(getnstimeofday()) and the device time (driver callback) may be
significant.

This patch adds an additional callback getsynctime64(). Which will be
called when the driver is able to perform a more accurate, implementation
specific cross-timestamping.  For example, future network devices that
implement PCIE PTM will be able to precisely correlate the device clock
with the system clock with virtually zero latency between captures.
This added callback can be used by the driver to expose this functionality.

The callback, getsynctime64(), will only be called when defined and
n_samples == 1 because the driver returns only 1 cross-timestamp where
multiple samples cannot be chained together.

This patch also adds to the capabilities ioctl (PTP_CLOCK_GETCAPS),
allowing applications to query whether or not drivers implement the
getsynctime callback, providing more precise cross timestamping.

Commit Details:

Added additional callback to ptp_clock_info:

* getsynctime64()

This takes 2 arguments referring to system and device time

With this callback drivers may provide both system time and device time
to ensure precise correlation

Modified PTP_SYS_OFFSET ioctl in PTP clock driver to use the above
callback if it's available

Added capability (PTP_CLOCK_GETCAPS) for checking whether driver supports
cross timestamping

Added check for cross timestamping flag to testptp.c

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 Documentation/ptp/testptp.c      |  6 ++++--
 drivers/ptp/ptp_chardev.c        | 29 +++++++++++++++++++++--------
 include/linux/ptp_clock_kernel.h |  7 +++++++
 include/uapi/linux/ptp_clock.h   |  4 +++-
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/Documentation/ptp/testptp.c b/Documentation/ptp/testptp.c
index 2bc8abc..8004efd 100644
--- a/Documentation/ptp/testptp.c
+++ b/Documentation/ptp/testptp.c
@@ -276,13 +276,15 @@ int main(int argc, char *argv[])
 			       "  %d external time stamp channels\n"
 			       "  %d programmable periodic signals\n"
 			       "  %d pulse per second\n"
-			       "  %d programmable pins\n",
+			       "  %d programmable pins\n"
+			       "  %d cross timestamping\n",
 			       caps.max_adj,
 			       caps.n_alarm,
 			       caps.n_ext_ts,
 			       caps.n_per_out,
 			       caps.pps,
-			       caps.n_pins);
+			       caps.n_pins,
+			       caps.cross_timestamping);
 		}
 	}
 
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index da7bae9..392ccfa 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -124,7 +124,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_clock_time *pct;
-	struct timespec64 ts;
+	struct timespec64 ts, systs;
 	int enable, err = 0;
 	unsigned int i, pin_index;
 
@@ -138,6 +138,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		caps.n_per_out = ptp->info->n_per_out;
 		caps.pps = ptp->info->pps;
 		caps.n_pins = ptp->info->n_pins;
+		caps.cross_timestamping = ptp->info->getsynctime64 != NULL;
 		if (copy_to_user((void __user *)arg, &caps, sizeof(caps)))
 			err = -EFAULT;
 		break;
@@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			break;
 		}
 		pct = &sysoff->ts[0];
-		for (i = 0; i < sysoff->n_samples; i++) {
-			getnstimeofday64(&ts);
+		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&
+		    ptp->info->getsynctime64(ptp->info, &ts, &systs) == 0) {
+			pct->sec = systs.tv_sec;
+			pct->nsec = systs.tv_nsec;
+			pct++;
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;
 			pct++;
-			ptp->info->gettime64(ptp->info, &ts);
+			pct->sec = systs.tv_sec;
+			pct->nsec = systs.tv_nsec;
+		} else {
+			for (i = 0; i < sysoff->n_samples; i++) {
+				getnstimeofday64(&ts);
+				pct->sec = ts.tv_sec;
+				pct->nsec = ts.tv_nsec;
+				pct++;
+				ptp->info->gettime64(ptp->info, &ts);
+				pct->sec = ts.tv_sec;
+				pct->nsec = ts.tv_nsec;
+				pct++;
+			}
+			getnstimeofday64(&ts);
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;
-			pct++;
 		}
-		getnstimeofday64(&ts);
-		pct->sec = ts.tv_sec;
-		pct->nsec = ts.tv_nsec;
 		if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
 			err = -EFAULT;
 		break;
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index b8b7306..8c43345 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -67,6 +67,11 @@ struct ptp_clock_request {
  * @gettime64:  Reads the current time from the hardware clock.
  *              parameter ts: Holds the result.
  *
+ * @getsynctime64:  Reads the current time from the hardware clock and system
+ *                  clock simultaneously.
+ *                  parameter dev: Holds the device time
+ *                  parameter sys: Holds the system time
+ *
  * @settime64:  Set the current time on the hardware clock.
  *              parameter ts: Time value to set.
  *
@@ -105,6 +110,8 @@ struct ptp_clock_info {
 	int (*adjfreq)(struct ptp_clock_info *ptp, s32 delta);
 	int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
 	int (*gettime64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
+	int (*getsynctime64)(struct ptp_clock_info *ptp, struct timespec64 *dev,
+			struct timespec64 *sys);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index f0b7bfe..ffb2635 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -51,7 +51,9 @@ struct ptp_clock_caps {
 	int n_per_out; /* Number of programmable periodic signals. */
 	int pps;       /* Whether the clock supports a PPS callback. */
 	int n_pins;    /* Number of input/output pins. */
-	int rsv[14];   /* Reserved for future use. */
+	/* Whether the clock supports precise system-device cross timestamps */
+	int cross_timestamping;
+	int rsv[13];   /* Reserved for future use. */
 };
 
 struct ptp_extts_request {
-- 
2.1.4

Re: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Thomas Gleixner]

On Fri, 21 Aug 2015, Christopher S. Hall wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> This patch allows system and device time ("cross-timestamp") to be
> performed by the driver. Currently, the cross-timestamping is performed
> in the PTP_SYS_OFFSET ioctl.  The PTP clock driver reads gettimeofday()
> and the gettime64() callback provided by the driver. The cross-timestamp
> is best effort where the latency between the capture of system time
> (getnstimeofday()) and the device time (driver callback) may be
> significant.
> 
> This patch adds an additional callback getsynctime64(). Which will be
> called when the driver is able to perform a more accurate, implementation
> specific cross-timestamping.  For example, future network devices that
> implement PCIE PTM will be able to precisely correlate the device clock
> with the system clock with virtually zero latency between captures.
> This added callback can be used by the driver to expose this functionality.
> 
> The callback, getsynctime64(), will only be called when defined and
> n_samples == 1 because the driver returns only 1 cross-timestamp where
> multiple samples cannot be chained together.
> 
> This patch also adds to the capabilities ioctl (PTP_CLOCK_GETCAPS),
> allowing applications to query whether or not drivers implement the
> getsynctime callback, providing more precise cross timestamping.

That looks close to a proper changelog. A few nitpicks though.

Please avoid 'This patch does ...' phrases. We already know that this
is a patch.


> Commit Details:

Please get rid of this. It's useless noise.
 
> Added additional callback to ptp_clock_info:
> 
> * getsynctime64()

> @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			break;
>  		}
>  		pct = &sysoff->ts[0];
> -		for (i = 0; i < sysoff->n_samples; i++) {
> -			getnstimeofday64(&ts);
> +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&

The number of samples should be irrelevant for this sampling method.

> +		    ptp->info->getsynctime64(ptp->info, &ts, &systs) == 0) {

Why is this function taking struct timespec64 pointers? Just so every
driver which implements the callback needs to convert from u64 to
struct timespec64? That's simply wrong. Use u64 for both and do the
conversion in the ioctl.

Thanks,

	tglx

Re: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Richard Cochran]

On Sat, Aug 22, 2015 at 10:33:48PM +0200, Thomas Gleixner wrote:
> > @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> >  			break;
> >  		}
> >  		pct = &sysoff->ts[0];
> > -		for (i = 0; i < sysoff->n_samples; i++) {
> > -			getnstimeofday64(&ts);
> > +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&
> 
> The number of samples should be irrelevant for this sampling method.

Chris had send me a preview of this before he posted, so I can explain
that test for one sample.

User space requests N (1 to 25) samples of the two clocks.  The kernel
is supposed to deliver that many samples.  This has always been the
documented behavior.  From ptp_clock.h:

  struct ptp_sys_offset {
	unsigned int n_samples; /* Desired number of measurements. */
	unsigned int rsv[3];    /* Reserved for future use. */
	/*
	 * Array of interleaved system/phc time stamps. The kernel
	 * will provide 2*n_samples + 1 time stamps, with the last
	 * one as a system time stamp.
	 */
	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
  };

So the kernel cannot simply change n_samples to 1.

I would prefer to have a new system call that compares any two posix
clock_t, but that is of course more work.

Allowing n_samples=1 as a special case is a kind of overloading of the
ioctl to support the new capability.  At least it preserves the
behavior of the interface from the user's perspective.

Thanks,
Richard

Re: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Thomas Gleixner]

On Sat, 22 Aug 2015, Richard Cochran wrote:
> On Sat, Aug 22, 2015 at 10:33:48PM +0200, Thomas Gleixner wrote:
> > > @@ -196,19 +197,31 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> > >  			break;
> > >  		}
> > >  		pct = &sysoff->ts[0];
> > > -		for (i = 0; i < sysoff->n_samples; i++) {
> > > -			getnstimeofday64(&ts);
> > > +		if (ptp->info->getsynctime64 && sysoff->n_samples == 1 &&
> > 
> > The number of samples should be irrelevant for this sampling method.
> 
> Chris had send me a preview of this before he posted, so I can explain
> that test for one sample.
> 
> User space requests N (1 to 25) samples of the two clocks.  The kernel
> is supposed to deliver that many samples.  This has always been the
> documented behavior.  From ptp_clock.h:
> 
>   struct ptp_sys_offset {
> 	unsigned int n_samples; /* Desired number of measurements. */
> 	unsigned int rsv[3];    /* Reserved for future use. */
> 	/*
> 	 * Array of interleaved system/phc time stamps. The kernel
> 	 * will provide 2*n_samples + 1 time stamps, with the last
> 	 * one as a system time stamp.
> 	 */
> 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
>   };
> 
> So the kernel cannot simply change n_samples to 1.
> 
> I would prefer to have a new system call that compares any two posix
> clock_t, but that is of course more work.
> 
> Allowing n_samples=1 as a special case is a kind of overloading of the
> ioctl to support the new capability.  At least it preserves the
> behavior of the interface from the user's perspective.

So why can't you take N samples from the synced hardware? It does not
make any sense to me to switch to the imprecise mode if nsamples > 1.

You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
-ENOSYS if hardware timestamping is not available and avoid the whole
nsamples dance for the case where we can get precise timestamps.

Thanks,

	tglx

Re: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Richard Cochran]

On Sun, Aug 23, 2015 at 10:15:00AM +0200, Thomas Gleixner wrote:
> So why can't you take N samples from the synced hardware? It does not
> make any sense to me to switch to the imprecise mode if nsamples > 1.

Ok, then I prefer to leave this "imprecise" method in place and ...
 
> You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
> -ENOSYS if hardware timestamping is not available and avoid the whole
> nsamples dance for the case where we can get precise timestamps.

have this for the new way.

By keeping the imprecise method, we will be able to run both methods
on the new hardware.  That will help to quantify how imprecise the old
method is.

Thanks,
Richard

RE: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Hall, Christopher S]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Sunday, August 23, 2015 4:26 AM
> To: Thomas Gleixner
> Cc: Hall, Christopher S; Kirsher, Jeffrey T; hpa@zytor.com;
> mingo@redhat.com; john.stultz@linaro.org; x86@kernel.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; peterz@infradead.org
> Subject: Re: [PATCH v3 3/4] Add support for driver cross-timestamp to
> PTP_SYS_OFFSET ioctl
> 
> On Sun, Aug 23, 2015 at 10:15:00AM +0200, Thomas Gleixner wrote:
> > So why can't you take N samples from the synced hardware? It does not
> > make any sense to me to switch to the imprecise mode if nsamples > 1.
> 
> Ok, then I prefer to leave this "imprecise" method in place and ...
> 
> > You can also provide a new IOCTL PTP_SYS_OFFSET_PRECISE which returns
> > -ENOSYS if hardware timestamping is not available and avoid the whole
> > nsamples dance for the case where we can get precise timestamps.
> 
> have this for the new way.
> 
> By keeping the imprecise method, we will be able to run both methods
> on the new hardware.  That will help to quantify how imprecise the old
> method is.

This means: remove code changes from the PTP_SYS_OFFSET ioctl and call getsynctime64() from a new ioctl PTP_SYS_OFFSET_PRECISE.  Right?

And use the same type (struct ptp_sys_offset) for the new ioctl?  Or should a new simplified struct be used? Such as:

struct precise_ptp_sys_offset {
	struct ptp_clock_time device;
	struct ptp_clock_time system;
};

Does it make sense to keep the "cross-timestamp" capabilities flag as-is?

> 
> Thanks,
> Richard

Re: [PATCH v3 3/4] Add support for driver cross-timestamp to PTP_SYS_OFFSET ioctl

[Richard Cochran]

On Mon, Aug 24, 2015 at 08:16:51PM +0000, Hall, Christopher S wrote:
> 
> This means: remove code changes from the PTP_SYS_OFFSET ioctl and call getsynctime64() from a new ioctl PTP_SYS_OFFSET_PRECISE.  Right?

Yes.
 
> And use the same type (struct ptp_sys_offset) for the new ioctl?  Or should a new simplified struct be used? Such as:
> 
> struct precise_ptp_sys_offset {
> 	struct ptp_clock_time device;
> 	struct ptp_clock_time system;
> };

I don't have a strong preference either way.  I would not mind reusing
the existing struct.

> Does it make sense to keep the "cross-timestamp" capabilities flag as-is?

Yes, indeed.
 

Thanks,
Richard

[PATCH v3 4/4] Enabling hardware supported PTP system/device crosstimestamping

[Christopher S. Hall]

From: Christopher Hall <christopher.s.hall@intel.com>

Add getsynctime() PTP device callback to cross timestamp system device
	clock using ART	translation depends on platform being >= SPT
	and having ART

getsynctime() reads ART (TSC-derived)/device cross timestamp and
	converts to realtime/device time reporting cross timestamp to
	PTP driver

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 drivers/net/ethernet/intel/e1000e/defines.h |  5 ++
 drivers/net/ethernet/intel/e1000e/ptp.c     | 88 +++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/e1000e/regs.h    |  4 ++
 3 files changed, 97 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
index 133d407..13cff75 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -527,6 +527,11 @@
 #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..228f3f3 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,87 @@ static int e1000e_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
+#define MAX_HW_WAIT_COUNT (3)
+
+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;
+	u32 tsync_ctrl;
+	int ret;
+
+	tsync_ctrl = er32(TSYNCTXCTL);
+	tsync_ctrl |= E1000_TSYNCTXCTL_START_SYNC |
+		E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK;
+	ew32(TSYNCTXCTL, tsync_ctrl);
+	for (i = 0; i < MAX_HW_WAIT_COUNT; ++i) {
+		udelay(1);
+		tsync_ctrl = er32(TSYNCTXCTL);
+		if (tsync_ctrl & E1000_TSYNCTXCTL_SYNC_COMP)
+			break;
+	}
+
+	if (i == MAX_HW_WAIT_COUNT) {
+		ret = -ETIMEDOUT;
+	} else {
+		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);
+	}
+
+	return ret;
+}
+
+/**
+ * e1000e_phc_getsynctime - Reads the current time from the hardware clock and
+ * correlated system time
+ * @ptp: ptp clock structure
+ * @devts: timespec structure to hold the current device time value
+ * @systs: timespec structure to hold the current system time value
+ *
+ * Read device and system (ART) clock simultaneously and return the correct
+ * clock values in ns after converting into a struct timespec.
+ **/
+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 ret;
+
+	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)
+		goto bail;
+
+	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;
+
+bail:
+	return ret;
+}
+
 /**
  * e1000e_phc_gettime - Reads the current time from the hardware clock
  * @ptp: ptp clock structure
@@ -190,6 +273,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,
 };
@@ -236,6 +320,10 @@ void e1000e_ptp_init(struct e1000_adapter *adapter)
 		break;
 	}
 
+	/* CPU must have ART and GBe must be from Sunrise Point or greater */
+	if (hw->mac.type < e1000_pch_spt || !cpu_has_art)
+		adapter->ptp_clock_info.getsynctime64 = NULL;
+
 	INIT_DELAYED_WORK(&adapter->systim_overflow_work,
 			  e1000e_systim_overflow_work);
 
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 */
 
-- 
2.1.4

Re: [PATCH v3 4/4] Enabling hardware supported PTP system/device crosstimestamping

[Thomas Gleixner]

On Fri, 21 Aug 2015, Christopher S. Hall wrote:
> From: Christopher Hall <christopher.s.hall@intel.com>
> 
> Add getsynctime() PTP device callback to cross timestamp system device
> 	clock using ART	translation depends on platform being >= SPT
> 	and having ART
> 
> getsynctime() reads ART (TSC-derived)/device cross timestamp and
> 	converts to realtime/device time reporting cross timestamp to
> 	PTP driver

See patch 1/4
 
> index 25a0ad5..228f3f3 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>

The usual way to order includes is:

#include <linux/timekeeping.h>

#include <asm/tsc.h>

#include "e1000.h"

> +/**
> + * e1000e_phc_getsynctime - Reads the current time from the hardware clock and
> + * correlated system time
> + * @ptp: ptp clock structure
> + * @devts: timespec structure to hold the current device time value
> + * @systs: timespec structure to hold the current system time value
> + *
> + * Read device and system (ART) clock simultaneously and return the correct
> + * clock values in ns after converting into a struct timespec.
> + **/
> +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 ret;
> +
> +	art_correlated_ts.get_ts = e1000e_phc_get_ts;
> +	art_correlated_ts.private = adapter;
> +	ret = get_correlated_timestamp(&art_correlated_ts,
> +				       &art_timestamper);

Pointless line break

> +	if (ret != 0)
> +		goto bail;

What's the purpose of this goto?

       if (ret)
		return ret;

is completely sufficient.

> +
> +	systs->tv_sec =
> +		div_u64_rem(art_correlated_ts.system_real.tv64,
> +			    NSEC_PER_SEC, &remainder);
> +	systs->tv_nsec = remainder;

ktime_to_timespec64() perhaps?

And please move that conversion to the ptp ioctl

> +	spin_lock_irqsave(&adapter->systim_lock, flags);
> +	device_time = timecounter_cyc2time(&adapter->tc,
> +					   art_correlated_ts.device_ts);

....

> +	/* CPU must have ART and GBe must be from Sunrise Point or greater */
> +	if (hw->mac.type < e1000_pch_spt || !cpu_has_art)
> +		adapter->ptp_clock_info.getsynctime64 = NULL;

We do it the other way round. We leave the default NULL and update it
if we detect the feature.

Thanks,

	tglx