[PATCH v2 0/1] Add PTP cross-timestamp to the PTP driver interface

[PATCH v2 0/1] Add PTP cross-timestamp to the PTP driver interface

[Christopher Hall]

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

[PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Christopher Hall]

* 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 <christopher.s.hall@intel.com>
---
 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

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Richard Cochran]

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

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

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 <christopher.s.hall@intel.com>
> ---
[..]
> @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Richard Cochran]

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

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Josh Cartwright]

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

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Richard Cochran]

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

Re: [PATCH v2 0/1] Add PTP cross-timestamp to the PTP driver interface

[Richard Cochran]

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