diff mbox

[RFC,v5,1/6] Timekeeping cross timestamp interface for device drivers

Message ID 1451911523-8534-2-git-send-email-christopher.s.hall@intel.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Christopher S. Hall Jan. 4, 2016, 12:45 p.m. UTC
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 precision with which events on multiple networked systems can be
synchronized using, as an example, PTP (IEEE 1588, 802.1AS) is limited
by the precision of the cross timestamps between the system clock and
the device (timestamp) clock. Precision here is the degree of
simultaneity when capturing the cross timestamp.

Currently the PTP cross timestamp is captured in software using the
PTP device driver ioctl PTP_SYS_OFFSET. Reads of the device clock are
interleaved with reads of the realtime clock. At best, the precision
of this cross timestamp is on the order of several microseconds due to
software latencies. Sub-microsecond precision is required for
industrial control and some media applications. To achieve this level
of precision hardware supported cross timestamping is needed.

Hardware cross timestamps are derived from simultaneously capturing
the device clock and the system clock. Applications use timestamps in
nanoseconds rather than clock ticks. The device driver can scale
device clock ticks to device time in nanoseconds, but cannot transform
system clock ticks. Only the kernel timekeeping code can do this. The
function get_device_system_crosststamp() allows device drivers to
return a cross timestamp with system time properly scaled to
nanoseconds.

The cross timestamps contain the realtime and monotonic raw clock
times. The realtime value is needed to discipline that clock using PTP
and the monotonic raw value is used for applications that don't
require a "real" time, but need an unadjusted clock time.

The get_device_system_crosststamp() code calls back into the driver
to ensure that the system counter is within the current timekeeping
update interval. Because of possible NTP/PTP frequency adjustments,
extrapolating a realtime clock time outside the current interval with
a potentially different scaling factor can result in a small amount of
error.

Modern Intel hardware provides an Always Running Timer (ART) which is
exactly related to TSC through a known frequency ratio. The ART is
routed to devices on the system and is used to precisely and
simultaneously capture the device clock with the ART. The kernel
timekeeping code requires a system clock value from the current clock
source to calculate the system time. The correlated clocksource adds a
means to transform a clock (in this case ART) exactly to the system
clock (TSC clocksource) that is used to calculate system time.

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

Comments

John Stultz Jan. 6, 2016, 6:55 p.m. UTC | #1
On Mon, Jan 4, 2016 at 4:45 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 precision with which events on multiple networked systems can be
> synchronized using, as an example, PTP (IEEE 1588, 802.1AS) is limited
> by the precision of the cross timestamps between the system clock and
> the device (timestamp) clock. Precision here is the degree of
> simultaneity when capturing the cross timestamp.
>
> Currently the PTP cross timestamp is captured in software using the
> PTP device driver ioctl PTP_SYS_OFFSET. Reads of the device clock are
> interleaved with reads of the realtime clock. At best, the precision
> of this cross timestamp is on the order of several microseconds due to
> software latencies. Sub-microsecond precision is required for
> industrial control and some media applications. To achieve this level
> of precision hardware supported cross timestamping is needed.
>
> Hardware cross timestamps are derived from simultaneously capturing
> the device clock and the system clock. Applications use timestamps in
> nanoseconds rather than clock ticks. The device driver can scale
> device clock ticks to device time in nanoseconds, but cannot transform
> system clock ticks. Only the kernel timekeeping code can do this. The
> function get_device_system_crosststamp() allows device drivers to
> return a cross timestamp with system time properly scaled to
> nanoseconds.
>
> The cross timestamps contain the realtime and monotonic raw clock
> times. The realtime value is needed to discipline that clock using PTP
> and the monotonic raw value is used for applications that don't
> require a "real" time, but need an unadjusted clock time.
>
> The get_device_system_crosststamp() code calls back into the driver
> to ensure that the system counter is within the current timekeeping
> update interval. Because of possible NTP/PTP frequency adjustments,
> extrapolating a realtime clock time outside the current interval with
> a potentially different scaling factor can result in a small amount of
> error.
>
> Modern Intel hardware provides an Always Running Timer (ART) which is
> exactly related to TSC through a known frequency ratio. The ART is
> routed to devices on the system and is used to precisely and
> simultaneously capture the device clock with the ART. The kernel
> timekeeping code requires a system clock value from the current clock
> source to calculate the system time. The correlated clocksource adds a
> means to transform a clock (in this case ART) exactly to the system
> clock (TSC clocksource) that is used to calculate system time.
>
> Signed-off-by: Christopher S. Hall <christopher.s.hall@intel.com>
> ---
>  include/linux/clocksource.h | 27 ++++++++++++++++
>  include/linux/timekeeping.h | 35 +++++++++++++++++++++
>  kernel/time/timekeeping.c   | 77 ++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 135 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 7784b59..4b7973d 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -255,4 +255,31 @@ static inline void clocksource_probe(void) {}
>  #define CLOCKSOURCE_ACPI_DECLARE(name, table_id, fn)           \
>         ACPI_DECLARE_PROBE_ENTRY(clksrc, name, table_id, 0, NULL, 0, fn)
>
> +/*
> + * 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);
> +};
> +
> +/*
> + * struct raw_system_counterval - system counter value captured in device
> + *     driver used to produce system/device cross-timestamp
> + * @system:    System counter value
> + * @cs:                Clocksource related to system counter value. This is used by
> + *             timekeeping code to verify validity of counter for system time
> + *             conversion
> + */
> +struct raw_system_counterval {
> +       cycle_t                 cycles;
> +       struct clocksource      *cs;
> +};
> +
>  #endif /* _LINUX_CLOCKSOURCE_H */
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index ec89d84..2209943 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -266,6 +266,41 @@ extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
>  extern void ktime_get_raw_and_real_ts64(struct timespec64 *ts_raw,
>                                         struct timespec64 *ts_real);
>
> +
> +/*
> + * struct system_device_crosststamp - system/device cross-timestamp
> + *     (syncronized capture)
> + * @device:    Device time
> + * @realtime:  Realtime simultaneous with device time
> + * @monoraw:   Monotonic raw simultaneous with device time
> + */
> +struct system_device_crosststamp {
> +       ktime_t device;
> +       ktime_t sys_realtime;
> +       ktime_t sys_monoraw;
> +};
> +
> +struct raw_system_counterval;
> +/*
> + * struct get_sync_device_time - Provides method to capture device time
> + *     synchronized with raw system counter value
> + * @get_time:  Callback providing synchronized capture of device time
> + *             and system counter. Returns 0 on success, < 0 on failure
> + * @ctx:       Context provided to callback function
> + */
> +struct get_sync_device_time {
> +       int     (*get_time)(ktime_t *device,
> +                           struct raw_system_counterval *system,
> +                           void *ctx);
> +       void     *ctx;
> +};
> +
> +/*
> + * Get cross timestamp between system clock and device clock
> + */
> +extern int get_device_system_crosststamp(struct system_device_crosststamp *ct,
> +                                        struct get_sync_device_time *dt);


I feel like this is introducing a lot of very small structures, which
to the casual reviewer aren't immediately obvious how they interlink
and are used.  It also adds to the number of types we have to keep in
our head. I dunno, maybe its just the correlated_cs is added but isn't
used in this patch, but I keep feeling like these should be more
obvious somehow.

That's terribly vague feedback, so I'm sorry.  Maybe some concrete suggestions?

* Maybe try renaming get_sync_device_time as just  crosststamp_device/struct ?

* raw_system_counterval feels like its of limited value. Maybe just
pass the clocksource and cycle value to the functions separately?




>  /*
>   * Persistent clock related interfaces
>   */
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d563c19..9c1ddc3 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -298,13 +298,11 @@ u32 (*arch_gettimeoffset)(void) = default_arch_gettimeoffset;
>  static inline u32 arch_gettimeoffset(void) { return 0; }
>  #endif
>
> -static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
> +static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
> +                                         cycle_t delta)
>  {
> -       cycle_t delta;
>         s64 nsec;
>
> -       delta = timekeeping_get_delta(tkr);
> -
>         nsec = delta * tkr->mult + tkr->xtime_nsec;
>         nsec >>= tkr->shift;
>
> @@ -312,6 +310,24 @@ static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
>         return nsec + arch_gettimeoffset();
>  }
>
> +static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
> +{
> +       cycle_t delta;
> +
> +       delta = timekeeping_get_delta(tkr);
> +       return timekeeping_delta_to_ns(tkr, delta);
> +}
> +
> +static inline s64 timekeeping_cycles_to_ns(struct tk_read_base *tkr,
> +                                           cycle_t cycles)
> +{
> +       cycle_t delta;
> +
> +       /* calculate the delta since the last update_wall_time */
> +       delta = clocksource_delta(cycles, tkr->cycle_last, tkr->mask);
> +       return timekeeping_delta_to_ns(tkr, delta);
> +}
>

Mind splitting the above out into its own small patch?



>  /**
>   * update_fast_timekeeper - Update the fast and NMI safe monotonic timekeeper.
>   * @tkr: Timekeeping readout base from which we take the update
> @@ -885,6 +901,59 @@ EXPORT_SYMBOL(ktime_get_raw_and_real_ts64);
>  #endif /* CONFIG_NTP_PPS */
>
>  /**
> + * get_device_system_crosststamp - Synchronously capture system/device timestamp
> + * @xtstamp:           Receives simultaneously captured system and device time
> + * @sync_devicetime:   Callback to get simultaneous device time and
> + *     system counter from the device driver
> + *
> + * Reads a timestamp from a device and correlates it to system time
> + */
> +int get_device_system_crosststamp(struct system_device_crosststamp *xtstamp,
> +                                 struct get_sync_device_time *sync_devicetime)

Another nit, but to me something like:
int get_device_system_crosststamp(struct get_sync_device_time *sync_devicetime,
                                                          struct
system_device_crosststamp *ret)

Reads better to me. ie: use this device_time -> return that crosststamp.


Otherwise this is looking pretty good.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher S. Hall Jan. 8, 2016, 12:42 a.m. UTC | #2
Hi John,

On Wed, 06 Jan 2016 10:55:22 -0800, John Stultz <john.stultz@linaro.org>  
wrote:
> On Mon, Jan 4, 2016 at 4:45 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);
>> +};

How about making this another patch? It's not directly related to the  
cross timestamp. I would lean toward making it its own patch leaving the  
ART correlated clocksource patch touching arch/ only.

>> +/*
>> + * struct raw_system_counterval - system counter value captured in  
>> device
>> + *     driver used to produce system/device cross-timestamp
>> + * @system:    System counter value
>> + * @cs:                Clocksource related to system counter value.  
>> This is used by
>> + *             timekeeping code to verify validity of counter for  
>> system time
>> + *             conversion
>> + */
>> +struct raw_system_counterval {
>> +       cycle_t                 cycles;
>> +       struct clocksource      *cs;
>> +};
>> +
>>  #endif /* _LINUX_CLOCKSOURCE_H */
>> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
>> index ec89d84..2209943 100644
>> --- a/include/linux/timekeeping.h
>> +++ b/include/linux/timekeeping.h
>> @@ -266,6 +266,41 @@ extern void timekeeping_inject_sleeptime64(struct  
>> timespec64 *delta);
>>  extern void ktime_get_raw_and_real_ts64(struct timespec64 *ts_raw,
>>                                         struct timespec64 *ts_real);
>>
>> +
>> +/*
>> + * struct system_device_crosststamp - system/device cross-timestamp
>> + *     (syncronized capture)
>> + * @device:    Device time
>> + * @realtime:  Realtime simultaneous with device time
>> + * @monoraw:   Monotonic raw simultaneous with device time
>> + */
>> +struct system_device_crosststamp {
>> +       ktime_t device;
>> +       ktime_t sys_realtime;
>> +       ktime_t sys_monoraw;
>> +};
>> +
>> +struct raw_system_counterval;
>> +/*
>> + * struct get_sync_device_time - Provides method to capture device time
>> + *     synchronized with raw system counter value
>> + * @get_time:  Callback providing synchronized capture of device time
>> + *             and system counter. Returns 0 on success, < 0 on failure
>> + * @ctx:       Context provided to callback function
>> + */
>> +struct get_sync_device_time {
>> +       int     (*get_time)(ktime_t *device,
>> +                           struct raw_system_counterval *system,
>> +                           void *ctx);
>> +       void     *ctx;
>> +};
>> +
>> +/*
>> + * Get cross timestamp between system clock and device clock
>> + */
>> +extern int get_device_system_crosststamp(struct  
>> system_device_crosststamp *ct,
>> +                                        struct get_sync_device_time  
>> *dt);
>
> I feel like this is introducing a lot of very small structures, which
> to the casual reviewer aren't immediately obvious how they interlink
> and are used.  It also adds to the number of types we have to keep in
> our head. I dunno, maybe its just the correlated_cs is added but isn't
> used in this patch, but I keep feeling like these should be more
> obvious somehow.
>
> That's terribly vague feedback, so I'm sorry.  Maybe some concrete  
> suggestions?
>
> * Maybe try renaming get_sync_device_time as just   
> crosststamp_device/struct ?

My goal here is to differentiate between the cross timestamp (final  
result) and the synchronized capture (intermediate value). Maybe it isn't  
particularly useful (or worse additionally confusing), but when reading  
the code I found it confusing to call everything a cross timestamp. I  
think it's the units - cycles and nanoseconds - that are the source of my  
confusion in this case. The units are obvious from the struct member  
types, but it seemed more straightforward to differentiate in the struct  
name.  Does this make sense?  If I missed the mark maybe it should be  
changed.

> * raw_system_counterval feels like its of limited value. Maybe just
> pass the clocksource and cycle value to the functions separately?

I agree it is of somewhat limited value, but I think it makes the cross  
timestamp code more intuitive (the header file, as you pointed out, not as  
much). Maybe better commenting of the struct declaration would be useful  
here? To your comment above, I can keep more structs in my head than  
arguments. Also, to my thinking there isn't any intuitive ordering if  
these struct members are changed to individual arguments which might make  
the driver callback confusing.

>>
>> -static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
>> +static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
>> +                                         cycle_t delta)
>>  {
>> -       cycle_t delta;
>>         s64 nsec;
>>
>> -       delta = timekeeping_get_delta(tkr);
>> -
>>         nsec = delta * tkr->mult + tkr->xtime_nsec;
>>         nsec >>= tkr->shift;
>>
>> @@ -312,6 +310,24 @@ static inline s64 timekeeping_get_ns(struct  
>> tk_read_base *tkr)
>>         return nsec + arch_gettimeoffset();
>>  }

> Mind splitting the above out into its own small patch?

Makes sense. To be clear your suggestion is to make this patch 1 of X and  
then 2 of X will be the cross timestamp patch.  Is that right?

>>  /**
>>   * update_fast_timekeeper - Update the fast and NMI safe monotonic  
>> timekeeper.
>>   * @tkr: Timekeeping readout base from which we take the update
>> @@ -885,6 +901,59 @@ EXPORT_SYMBOL(ktime_get_raw_and_real_ts64);
>>  #endif /* CONFIG_NTP_PPS */
>>
>>  /**
>> + * get_device_system_crosststamp - Synchronously capture system/device  
>> timestamp
>> + * @xtstamp:           Receives simultaneously captured system and  
>> device time
>> + * @sync_devicetime:   Callback to get simultaneous device time and
>> + *     system counter from the device driver
>> + *
>> + * Reads a timestamp from a device and correlates it to system time
>> + */
>> +int get_device_system_crosststamp(struct system_device_crosststamp  
>> *xtstamp,
>> +                                 struct get_sync_device_time  
>> *sync_devicetime)
>
> Another nit, but to me something like:
> int get_device_system_crosststamp(struct get_sync_device_time  
> *sync_devicetime,
>                                                           struct
> system_device_crosststamp *ret)
>
> Reads better to me. ie: use this device_time -> return that crosststamp.

OK.  My brain "works" the opposite way, but I think (after a small amount  
of research) I'm in the minority:)

Thanks,
Chris
John Stultz Jan. 8, 2016, 1:05 a.m. UTC | #3
On Thu, Jan 7, 2016 at 4:42 PM, Christopher Hall
<christopher.s.hall@intel.com> wrote:
> Hi John,
>
> On Wed, 06 Jan 2016 10:55:22 -0800, John Stultz <john.stultz@linaro.org>
> wrote:
>>
>> On Mon, Jan 4, 2016 at 4:45 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);
>>> +};
>
>
> How about making this another patch? It's not directly related to the cross
> timestamp. I would lean toward making it its own patch leaving the ART
> correlated clocksource patch touching arch/ only.

Sure. Tying it more closely (via explicit commit message) to the
change that uses it would be better then kind of slipping it in here.


>> I feel like this is introducing a lot of very small structures, which
>> to the casual reviewer aren't immediately obvious how they interlink
>> and are used.  It also adds to the number of types we have to keep in
>> our head. I dunno, maybe its just the correlated_cs is added but isn't
>> used in this patch, but I keep feeling like these should be more
>> obvious somehow.
>>
>> That's terribly vague feedback, so I'm sorry.  Maybe some concrete
>> suggestions?
>>
>> * Maybe try renaming get_sync_device_time as just
>> crosststamp_device/struct ?
>
>
> My goal here is to differentiate between the cross timestamp (final result)
> and the synchronized capture (intermediate value). Maybe it isn't
> particularly useful (or worse additionally confusing), but when reading the
> code I found it confusing to call everything a cross timestamp. I think it's
> the units - cycles and nanoseconds - that are the source of my confusion in
> this case. The units are obvious from the struct member types, but it seemed
> more straightforward to differentiate in the struct name.  Does this make
> sense?  If I missed the mark maybe it should be changed.

Sure. I think having a different type then just cycles here is good,
and counterval is fine for an alternate name, but get_sync_device_time
looks like a function, not a structure name to me. The fact that the
structure is mostly an accessor to the hardware is fine, but maybe
counterval_device or something would be better? counterval_source? I
dunno.


>> * raw_system_counterval feels like its of limited value. Maybe just
>> pass the clocksource and cycle value to the functions separately?
>
>
> I agree it is of somewhat limited value, but I think it makes the cross
> timestamp code more intuitive (the header file, as you pointed out, not as
> much). Maybe better commenting of the struct declaration would be useful
> here? To your comment above, I can keep more structs in my head than
> arguments. Also, to my thinking there isn't any intuitive ordering if these
> struct members are changed to individual arguments which might make the
> driver callback confusing.

Yea. I just feel like lots of structures add extra abstraction that
makes the code harder to learn or re-learn. Not only is one trying to
remember the base types that are being passed around, but you also
have to remember what all the meta-structures are for. Especially for
these two-value structures.

And I apologize. I know this sort of nit-picking is sort of the worst.
"Its great you named your child that, but I'd prefer to call them..."
;)  But for long-term maintenance, under the fog of time, obviousness
is really important.


>>> -static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
>>> +static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
>>> +                                         cycle_t delta)
>>>  {
>>> -       cycle_t delta;
>>>         s64 nsec;
>>>
>>> -       delta = timekeeping_get_delta(tkr);
>>> -
>>>         nsec = delta * tkr->mult + tkr->xtime_nsec;
>>>         nsec >>= tkr->shift;
>>>
>>> @@ -312,6 +310,24 @@ static inline s64 timekeeping_get_ns(struct
>>> tk_read_base *tkr)
>>>         return nsec + arch_gettimeoffset();
>>>  }
>
>
>> Mind splitting the above out into its own small patch?
>
>
> Makes sense. To be clear your suggestion is to make this patch 1 of X and
> then 2 of X will be the cross timestamp patch.  Is that right?

Yep.


>>>  /**
>>>   * update_fast_timekeeper - Update the fast and NMI safe monotonic
>>> timekeeper.
>>>   * @tkr: Timekeeping readout base from which we take the update
>>> @@ -885,6 +901,59 @@ EXPORT_SYMBOL(ktime_get_raw_and_real_ts64);
>>>  #endif /* CONFIG_NTP_PPS */
>>>
>>>  /**
>>> + * get_device_system_crosststamp - Synchronously capture system/device
>>> timestamp
>>> + * @xtstamp:           Receives simultaneously captured system and
>>> device time
>>> + * @sync_devicetime:   Callback to get simultaneous device time and
>>> + *     system counter from the device driver
>>> + *
>>> + * Reads a timestamp from a device and correlates it to system time
>>> + */
>>> +int get_device_system_crosststamp(struct system_device_crosststamp
>>> *xtstamp,
>>> +                                 struct get_sync_device_time
>>> *sync_devicetime)
>>
>>
>> Another nit, but to me something like:
>> int get_device_system_crosststamp(struct get_sync_device_time
>> *sync_devicetime,
>>                                                           struct
>> system_device_crosststamp *ret)
>>
>> Reads better to me. ie: use this device_time -> return that crosststamp.
>
>
> OK.  My brain "works" the opposite way, but I think (after a small amount of
> research) I'm in the minority:)

Yea. memcpy and others do work the other way, but my left-to-right
bias is pretty strong. :)

thanks
-john
Richard Cochran Jan. 8, 2016, 9:13 a.m. UTC | #4
On Thu, Jan 07, 2016 at 05:05:24PM -0800, John Stultz wrote:
> Yea. I just feel like lots of structures add extra abstraction that
> makes the code harder to learn or re-learn. Not only is one trying to
> remember the base types that are being passed around, but you also
> have to remember what all the meta-structures are for. Especially for
> these two-value structures.

FWIW, I had exactly this trouble when reading this series.

Thanks,
Richard
diff mbox

Patch

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 7784b59..4b7973d 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -255,4 +255,31 @@  static inline void clocksource_probe(void) {}
 #define CLOCKSOURCE_ACPI_DECLARE(name, table_id, fn)		\
 	ACPI_DECLARE_PROBE_ENTRY(clksrc, name, table_id, 0, NULL, 0, fn)
 
+/*
+ * 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);
+};
+
+/*
+ * struct raw_system_counterval - system counter value captured in device
+ *	driver used to produce system/device cross-timestamp
+ * @system:	System counter value
+ * @cs:		Clocksource related to system counter value. This is used by
+ *		timekeeping code to verify validity of counter for system time
+ *		conversion
+ */
+struct raw_system_counterval {
+	cycle_t			cycles;
+	struct clocksource	*cs;
+};
+
 #endif /* _LINUX_CLOCKSOURCE_H */
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index ec89d84..2209943 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -266,6 +266,41 @@  extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
 extern void ktime_get_raw_and_real_ts64(struct timespec64 *ts_raw,
 				        struct timespec64 *ts_real);
 
+
+/*
+ * struct system_device_crosststamp - system/device cross-timestamp
+ *	(syncronized capture)
+ * @device:	Device time
+ * @realtime:	Realtime simultaneous with device time
+ * @monoraw:	Monotonic raw simultaneous with device time
+ */
+struct system_device_crosststamp {
+	ktime_t device;
+	ktime_t sys_realtime;
+	ktime_t sys_monoraw;
+};
+
+struct raw_system_counterval;
+/*
+ * struct get_sync_device_time - Provides method to capture device time
+ *	synchronized with raw system counter value
+ * @get_time:	Callback providing synchronized capture of device time
+ *		and system counter. Returns 0 on success, < 0 on failure
+ * @ctx:	Context provided to callback function
+ */
+struct get_sync_device_time {
+	int	(*get_time)(ktime_t *device,
+			    struct raw_system_counterval *system,
+			    void *ctx);
+	void	 *ctx;
+};
+
+/*
+ * Get cross timestamp between system clock and device clock
+ */
+extern int get_device_system_crosststamp(struct system_device_crosststamp *ct,
+					 struct get_sync_device_time *dt);
+
 /*
  * Persistent clock related interfaces
  */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d563c19..9c1ddc3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -298,13 +298,11 @@  u32 (*arch_gettimeoffset)(void) = default_arch_gettimeoffset;
 static inline u32 arch_gettimeoffset(void) { return 0; }
 #endif
 
-static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
+static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
+					  cycle_t delta)
 {
-	cycle_t delta;
 	s64 nsec;
 
-	delta = timekeeping_get_delta(tkr);
-
 	nsec = delta * tkr->mult + tkr->xtime_nsec;
 	nsec >>= tkr->shift;
 
@@ -312,6 +310,24 @@  static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
 	return nsec + arch_gettimeoffset();
 }
 
+static inline s64 timekeeping_get_ns(struct tk_read_base *tkr)
+{
+	cycle_t delta;
+
+	delta = timekeeping_get_delta(tkr);
+	return timekeeping_delta_to_ns(tkr, delta);
+}
+
+static inline s64 timekeeping_cycles_to_ns(struct tk_read_base *tkr,
+					    cycle_t cycles)
+{
+	cycle_t delta;
+
+	/* calculate the delta since the last update_wall_time */
+	delta = clocksource_delta(cycles, tkr->cycle_last, tkr->mask);
+	return timekeeping_delta_to_ns(tkr, delta);
+}
+
 /**
  * update_fast_timekeeper - Update the fast and NMI safe monotonic timekeeper.
  * @tkr: Timekeeping readout base from which we take the update
@@ -885,6 +901,59 @@  EXPORT_SYMBOL(ktime_get_raw_and_real_ts64);
 #endif /* CONFIG_NTP_PPS */
 
 /**
+ * get_device_system_crosststamp - Synchronously capture system/device timestamp
+ * @xtstamp:		Receives simultaneously captured system and device time
+ * @sync_devicetime:	Callback to get simultaneous device time and
+ *	system counter from the device driver
+ *
+ * Reads a timestamp from a device and correlates it to system time
+ */
+int get_device_system_crosststamp(struct system_device_crosststamp *xtstamp,
+				  struct get_sync_device_time *sync_devicetime)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned long seq;
+	struct raw_system_counterval raw_sys;
+	ktime_t base_raw;
+	ktime_t base_real;
+	s64 nsec_raw;
+	s64 nsec_real;
+	int ret;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		/*
+		 * Try to get a timestamp from the device driver.
+		 */
+		ret = sync_devicetime->get_time(&xtstamp->device, &raw_sys,
+						sync_devicetime->ctx);
+		if (ret)
+			return ret;
+
+		/*
+		 * Verify that the correlated clocksource is related to
+		 * the currently installed timekeeper clocksource
+		 */
+		if (tk->tkr_mono.clock != raw_sys.cs)
+			return -ENODEV;
+
+		base_real = ktime_add(tk->tkr_mono.base,
+				      tk_core.timekeeper.offs_real);
+		base_raw = tk->tkr_raw.base;
+
+		nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono,
+						     raw_sys.cycles);
+		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw,
+						    raw_sys.cycles);
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	xtstamp->sys_realtime = ktime_add_ns(base_real, nsec_real);
+	xtstamp->sys_monoraw = ktime_add_ns(base_raw, nsec_raw);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
+
+/**
  * do_gettimeofday - Returns the time of day in a timeval
  * @tv:		pointer to the timeval to be set
  *