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

Groups > linux.kernel > #1308764 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/9] Patchset enabling hardware based cross-timestamps for next gen Intel platforms "Christopher S. Hall" <christopher.s.hall@intel.com> - 2016-01-13 20:30 +0100
    [PATCH v6 3/9] Add correlated clocksource relating aliased auxiliary and system clocks "Christopher S. Hall" <christopher.s.hall@intel.com> - 2016-01-13 20:30 +0100
      Re: [PATCH v6 3/9] Add correlated clocksource relating aliased  auxiliary and system clocks John Stultz <john.stultz@linaro.org> - 2016-01-15 01:30 +0100
        Re: [PATCH v6 3/9] Add correlated clocksource relating aliased  auxiliary and system clocks Thomas Gleixner <tglx@linutronix.de> - 2016-01-15 11:40 +0100
    Re: [PATCH v6 0/9] Patchset enabling hardware based cross-timestamps  for next gen Intel platforms John Stultz <john.stultz@linaro.org> - 2016-01-15 01:50 +0100

#1308764 — [PATCH v6 0/9] Patchset enabling hardware based cross-timestamps for next gen Intel platforms

Modern Intel hardware adds an Always Running Timer (ART) that allows the
network and audio device clocks to precisely cross timestamp the device
clock with the system clock. This allows a precise correlation of the
device time and system time.

This patchset adds interfaces to the timekeeping code allowing drivers
to translate ART time to system time.

Changelog:

Changes from v5 to v6:

*	Pulled supporting code for snapshotting, correlated
	clocksource, and cycles to nanoseconds translation to separate
	patches. Added patches are marked as NEW below. There is,
	however, very little *actually* new code, just reorganized
	code
*	Renamed and moved clocksource change sequence to timekeeper
	struct (out of tk_read_base)
*	Renamed structs for system counter and synced device time
	callback to system_counterval_t and sync_device_time_cb,
	respectively
*	Changed PTP cross-timestamp callback name to getcrosststamp
	for consistency with the timekeeping code - corresponding
	function name changes in e1000e driver
*	Simplified PTP time calculations making use of ktime_to_* code

Changes from v4 to v5:

*	Changes the history mechanism to interpolate system time using
	a single historic system time pair (monotonic raw, realtime)
	rather than implementing a precise history using shadow
	timekeeper (see v4 changes). The advantage of this approach is
	that the history can be arbitrarily long. This approach may
	also be simpler in terms of coding. The major disadvantage is
	that the realtime clock can be adjusted.  When adjusted, the
	realtime clock time (when interpolating from history) is
	always approximate. In general, the longer the interpolation
	period the larger the potential error. There isn't any error
	interpolating the monotonic raw clock time.
*	This patchset also addresses objections to the previous
	patchsets overly complex correlated timestamp structure. This
	patchset splits that structure into several smaller
	structures.  The correlated timestamp interface is renamed
	cross timestamp to avoid any confusion with the correlated
	clocksource.
*	The correlated clocksource is separated from the cross
	timestamp mechanism.
*	Add monotonic raw to the PTP user interface
*	Add e1000e driver configuration option that wraps Intel PCH
	specific code

Changes v3 to v4: 

*	Adds a history mechanism to accomodate slower devices. In this
	case the response time for timestamp reads to the Intel DSP
	are too slow to be accomodated by the original correlated time
	mechanism. The history mechanism turns shadow timekeeper into
	an array where the history is stored.

Christopher S. Hall (9):
  Add cycles to nanoseconds translation (NEW)
  Add driver cross timestamp interface for higher precision time
    synchronization
  Add correlated clocksource relating aliased auxiliary and system
    clocks (NEW)
  Always Running Timer (ART) correlated clocksource
  Add timekeeping snapshot code capturing system time and counter (NEW)
  Add history to cross timestamp interface supporting slower devices
  Remove duplicated code in ktime_get_raw_and_real()
  Add PTP_SYS_OFFSET_PRECISE for driver crosstimestamping
  Adds hardware supported cross timestamp

 Documentation/ptp/testptp.c                 |   6 +-
 arch/x86/include/asm/cpufeature.h           |   2 +-
 arch/x86/include/asm/tsc.h                  |   2 +
 arch/x86/kernel/tsc.c                       |  46 ++++++
 drivers/net/ethernet/intel/Kconfig          |   9 +
 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                   |  27 +++
 include/linux/clocksource.h                 |  43 +++++
 include/linux/pps_kernel.h                  |  17 +-
 include/linux/ptp_clock_kernel.h            |   8 +
 include/linux/timekeeper_internal.h         |   2 +
 include/linux/timekeeping.h                 |  42 +++++
 include/uapi/linux/ptp_clock.h              |  13 +-
 kernel/time/timekeeping.c                   | 247 +++++++++++++++++++++++++---
 16 files changed, 521 insertions(+), 40 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1308765 — [PATCH v6 3/9] Add correlated clocksource relating aliased auxiliary and system clocks

ACKNOWLEDGMENT: The original correlated clock source and cross
timestamp code was developed by Thomas Gleixner
<tglx@linutronix.de>. It has changed considerably and any mistakes are
mine.

The timekeeping and clocksource code don't currently comprehend two
clocks that are aliases of one another. That is clocks that are
*exactly* related. The correlated_cs struct encapsulated this
relationship between a clocksource and its alias clock. Modern Intel
hardware provides an Always Running Timer (ART) which is exactly
related to TSC through a known frequency ratio. The ART is an example
of a correlated clocksource.

Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
---
 include/linux/clocksource.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 4542293..726ca68 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -267,4 +267,18 @@ struct system_counterval_t {
 	struct clocksource	*cs;
 };
 
+/*
+ * 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;
+	cycle_t			(*convert)(struct correlated_cs *cs,
+					   cycle_t cycles);
+};
+
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
2.1.4

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

#1309782 — Re: [PATCH v6 3/9] Add correlated clocksource relating aliased auxiliary and system clocks

On Wed, Jan 13, 2016 at 4:12 AM, Christopher S. Hall
<christopher.s.hall@intel.com> wrote:
> ACKNOWLEDGMENT: The original correlated clock source and cross
> timestamp code was developed by Thomas Gleixner
> <tglx@linutronix.de>. It has changed considerably and any mistakes are
> mine.
>
> The timekeeping and clocksource code don't currently comprehend two
> clocks that are aliases of one another. That is clocks that are
> *exactly* related. The correlated_cs struct encapsulated this
> relationship between a clocksource and its alias clock. Modern Intel
> hardware provides an Always Running Timer (ART) which is exactly
> related to TSC through a known frequency ratio. The ART is an example
> of a correlated clocksource.
>
> Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
> ---
>  include/linux/clocksource.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 4542293..726ca68 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -267,4 +267,18 @@ struct system_counterval_t {
>         struct clocksource      *cs;
>  };
>
> +/*
> + * 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;
> +       cycle_t                 (*convert)(struct correlated_cs *cs,
> +                                          cycle_t cycles);
> +};
> +

So.. In reworking your patch set, I've preserved this, but I'm still
not totally convinced. Its a generic structure, but not used by any
generic code and its only used by hardware specific implementations
(ie: the tsc and e1000e_hwts logic). It seems like this could be a
tsc.h specific structure w/o a real issue.

And really this doesn't seem to be a generic thing. The e1000e hwts is
always the ART based. Its not likely these sorts of cross hardware
timestamps are going to be completely abstract and interchangeable,
is it?

thanks
-john

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

#1310039 — Re: [PATCH v6 3/9] Add correlated clocksource relating aliased auxiliary and system clocks

On Thu, 14 Jan 2016, John Stultz wrote:
> On Wed, Jan 13, 2016 at 4:12 AM, Christopher S. Hall
> <christopher.s.hall@intel.com> wrote:
> > +/*
> > + * 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;
> > +       cycle_t                 (*convert)(struct correlated_cs *cs,
> > +                                          cycle_t cycles);
> > +};
> > +
> 
> So.. In reworking your patch set, I've preserved this, but I'm still
> not totally convinced. Its a generic structure, but not used by any
> generic code and its only used by hardware specific implementations
> (ie: the tsc and e1000e_hwts logic). It seems like this could be a
> tsc.h specific structure w/o a real issue.

That correlated_cs is my fault. I invented that when I had that first stab on
the cross time stamp thing.
 
> And really this doesn't seem to be a generic thing. The e1000e hwts is
> always the ART based. Its not likely these sorts of cross hardware
> timestamps are going to be completely abstract and interchangeable,
> is it?

They might be in future. I guess other archs will provide similar means to
distribute an always on timer to PCIe for timestamping purposes.

So the question is whether we should expose that timestamp reference via a
generic mechanism, e.g. store it somewhere in the pci root complex or wherever
the appropriate point for it is.

Though for now, we certainly can make that x86 private and deal with it when
others come along.

Thanks,

	tglx

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

#1309786 — Re: [PATCH v6 0/9] Patchset enabling hardware based cross-timestamps for next gen Intel platforms

On Wed, Jan 13, 2016 at 4:12 AM, Christopher S. Hall
<christopher.s.hall@intel.com> wrote:
> Modern Intel hardware adds an Always Running Timer (ART) that allows the
> network and audio device clocks to precisely cross timestamp the device
> clock with the system clock. This allows a precise correlation of the
> device time and system time.
>
> This patchset adds interfaces to the timekeeping code allowing drivers
> to translate ART time to system time.

So thanks again for sending this out. Instead of sending you a bunch
of nitpicky changes as I've done in the past, I figured I'd try to
more directly help out and try to rework some of the patches to be
more to my own liking.

I've pushed them here:
   https://git.linaro.org/people/john.stultz/linux.git dev/xtimestamp

My main changes have been:
* Reordered the patches, putting all the timekeeping core changes
first, then the usage of those interfaces last.

* Implemented Richard's suggestion to drop one of the helper
structures that didn't provide much value

* Moved structures defined in clocksource.h but only used in
timekeeping.h to timekeeping.h

* Fixed a few build issues I caught (as well as some of the ones
kbuild bot found)

* Reworked the tsc logic to avoid 64bit divisions (which don't build
on 32bit systems)

I still have not:
* Done *any* testing at all with this. Please verify I didn't break anything. :)

* Fixed the 64bit div on 32bit systems build issue in
get_device_system_crosststamp()/adjust_historical_crosststamp()

* Done another review/edit pass on the commit messages, as they've
gotten a bit long (I know, I know.. "be verbose" I said!), but they
can probably be tweaked to be better and more contextual to the
patches.

Still on the fence:
* Probably should zap the correlated_cs structure or move it to tsc.h,
as mentioned in my other email

Anyway, I'll let you take a look at this and feel free to integrate
and adapt these ideas as you please into your patchset. Look forward
to your next revision!

thanks
-john

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web