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.
Christopher Hall (1):
Added additional callback to ptp_clock_info:
drivers/ptp/ptp_chardev.c | 29 +++++++++++++++++++++--------
include/linux/ptp_clock_kernel.h | 8 ++++++++
include/uapi/linux/ptp_clock.h | 4 +++-
3 files changed, 32 insertions(+), 9 deletions(-)
--
1.9.1
* 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
precise timestamping
Signed-off-by: Christopher Hall <[email protected]>
---
drivers/ptp/ptp_chardev.c | 29 +++++++++++++++++++++--------
include/linux/ptp_clock_kernel.h | 8 ++++++++
include/uapi/linux/ptp_clock.h | 4 +++-
3 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index da7bae9..2a83aea 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.precise_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);
+ 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..344f129 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,9 @@ 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..421b637 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 precise_timestamping;
+ int rsv[13]; /* Reserved for future use. */
};
struct ptp_extts_request {
--
1.9.1
On Thu, Jul 02, 2015 at 06:14:47PM -0700, Christopher Hall wrote:
> 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.
This discription is now much more clear to me, thanks. Since there is
only one patch, please put this text into the commit message and drop
the cover letter.
Thanks,
Richard
I have three nits to pick...
On Thu, Jul 02, 2015 at 06:14:48PM -0700, Christopher Hall wrote:
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index b8b7306..344f129 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,9 @@ 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);
This indentation looks funny, how about:
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..421b637 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 precise_timestamping;
I prefer another name, like "cross_timestamping" or similar. I get
lots and lots of nube questions about PTP, and people will start
asking whether this means their packet time stamps are bad.
Also, could you update Documentation/ptp/testptp.c with the new field?
Thanks,
Richard
On Thu, Jul 02, 2015 at 06:14:48PM -0700, Christopher Hall wrote:
> * getsynctime64()
Hello Christopher-
A couple comments below.
>
> 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
> precise timestamping
>
> Signed-off-by: Christopher Hall <[email protected]>
> ---
[..]
> @@ -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);
> + pct->sec = systs.tv_sec;
> + pct->nsec = systs.tv_nsec;
It's difficult to make too many judgements without seeing how a driver
might implement this; is there another patchset that shows how a driver
implements this?
[..]
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index f0b7bfe..421b637 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 precise_timestamping;
Perhaps now is a good time to add an unsigned int 'flags' member instead,
and start allocating bits.
Josh
On Mon, Jul 06, 2015 at 03:44:58PM -0500, Josh Cartwright wrote:
> It's difficult to make too many judgements without seeing how a driver
> might implement this; is there another patchset that shows how a driver
> implements this?
The interface is certainly clear enough to me. The details of
obtaining a cross time stamp from the hardware will remain hidden
behind the interface in any case.
> > - int rsv[14]; /* Reserved for future use. */
> > + /* Whether the clock supports precise system-device cross timestamps */
> > + int precise_timestamping;
>
> Perhaps now is a good time to add an unsigned int 'flags' member instead,
> and start allocating bits.
Considering the rate of growth for the PHC stuff, I am not worried
about spending a whole 'int' on this.
Thanks,
Richard
On Wed, Jul 08, 2015 at 01:54:34PM +0200, Richard Cochran wrote:
> On Mon, Jul 06, 2015 at 03:44:58PM -0500, Josh Cartwright wrote:
> > It's difficult to make too many judgements without seeing how a driver
> > might implement this; is there another patchset that shows how a driver
> > implements this?
>
> The interface is certainly clear enough to me. The details of
> obtaining a cross time stamp from the hardware will remain hidden
> behind the interface in any case.
It's unusual to see interfaces added like this without also seeing users
at the same time to see how the pieces fit together. Should I have
assumed this was a PATCH RFC for just the API?
I'm afraid this is the tip of a much larger iceberg. If a device is
doing hardware crosstimestamping, what is the hardwares view of this
"system clock"? How is it tied into the kernels' timekeeping? How is
it ensured that same clock the hardware sees is the kernel's actively
selected clocksource?
You get this for free in software with getnstimeofday(), by pushing it
to hardware, the boundaries of responsibilities are all
unclear...without seeing the whole picture.
> > > - int rsv[14]; /* Reserved for future use. */
> > > + /* Whether the clock supports precise system-device cross timestamps */
> > > + int precise_timestamping;
> >
> > Perhaps now is a good time to add an unsigned int 'flags' member instead,
> > and start allocating bits.
>
> Considering the rate of growth for the PHC stuff, I am not worried
> about spending a whole 'int' on this.
Fair enough!
Thanks,
Josh
On Wed, Jul 08, 2015 at 07:17:37AM -0500, Josh Cartwright wrote:
> It's unusual to see interfaces added like this without also seeing users
> at the same time to see how the pieces fit together. Should I have
> assumed this was a PATCH RFC for just the API?
Chris, any idea when we might see a driver using the interface?
Thanks,
Richard