[PATCH] Add PTP cross-timestamp to the PTP driver interface

[PATCH] 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 timestamping is performed in the PTP_SYS_OFFSET 
ioctl.  It reads gettimeofday() and the gettime64() callback provided by the 
driver. The cross-timestamp is best effort ignoring the latency between the 
capture of system time (getnstimeofday()) and the device time 
(driver callback).

This patch adds an additional callback getsynctime64(). Which will be called 
when the driver is able to perform a more accurate, implementation specific 
cross-timestamp.  The ioctl code uses the callback when available.

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


Christopher Hall (1):
  Added additional callback to ptp_clock_info:

 drivers/ptp/ptp_chardev.c        | 30 +++++++++++++++++++++---------
 include/linux/ptp_clock_kernel.h |  3 +++
 2 files changed, 24 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH] 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

Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
---
 drivers/ptp/ptp_chardev.c        | 30 +++++++++++++++++++++---------
 include/linux/ptp_clock_kernel.h |  3 +++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index da7bae9..e91f98e 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;
 
@@ -196,19 +196,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;
+			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..edff9a5 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -105,6 +105,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);
-- 
1.9.1

Re: [PATCH] Added additional callback to ptp_clock_info:

[Richard Cochran]

On Thu, Jun 25, 2015 at 04:42:57PM -0700, Christopher Hall wrote:
> @@ -196,19 +196,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;

Please use pct++ for the sake of consistency.

>  			pct->sec = ts.tv_sec;
>  			pct->nsec = ts.tv_nsec;
> -			pct++;
> -			ptp->info->gettime64(ptp->info, &ts);
> +			++pct;
> +			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..edff9a5 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -105,6 +105,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);

The struct's KernelDoc also needs updating.

>  	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);
> -- 
> 1.9.1

Thanks,
Richard

Re: [PATCH] Add PTP cross-timestamp to the PTP driver interface

[Richard Cochran]

Chris,

Basically this patch looks okay to me.  Could you please add LKML,
John Stultz and tglx (the time guys) onto CC?  I would like to get
their Acks or at least let them have a chance to review it.

On Thu, Jun 25, 2015 at 04:42:56PM -0700, Christopher Hall wrote:
> This patch allows system and device time ("cross-timestamp") to be performed 
> by the driver. Currently, the timestamping is performed in the PTP_SYS_OFFSET 
> ioctl.  It reads gettimeofday() and the gettime64() callback provided by the 
> driver. The cross-timestamp is best effort ignoring the latency between the 
> capture of system time (getnstimeofday()) and the device time 
> (driver callback).

You can make the motivation more clear by mentioning how the newer
PCIe spec foresees "perfect" timestamps.  If I didn't already know the
background, I would wonder who would ever want "best effort" single
cross timestamps.

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

There should be a way for user space to find out whether a particular
device offers the cross timestamp capability.  There are reserved
fields in 'struct ptp_clock_caps' that could be used.

Thanks,
Richard

Re: [PATCH] Add PTP cross-timestamp to the PTP driver interface

[Keller, Jacob E]

On Fri, 2015-06-26 at 08:55 +0200, Richard Cochran wrote:
> Chris,
> 
> Basically this patch looks okay to me.  Could you please add LKML,
> John Stultz and tglx (the time guys) onto CC?  I would like to get
> their Acks or at least let them have a chance to review it.
> 
> On Thu, Jun 25, 2015 at 04:42:56PM -0700, Christopher Hall wrote:
> > This patch allows system and device time ("cross-timestamp") to be 
> > performed 
> > by the driver. Currently, the timestamping is performed in the 
> > PTP_SYS_OFFSET 
> > ioctl.  It reads gettimeofday() and the gettime64() callback 
> > provided by the 
> > driver. The cross-timestamp is best effort ignoring the latency 
> > between the 
> > capture of system time (getnstimeofday()) and the device time 
> > (driver callback).
> 
> You can make the motivation more clear by mentioning how the newer
> PCIe spec foresees "perfect" timestamps.  If I didn't already know 
> the
> background, I would wonder who would ever want "best effort" single
> cross timestamps.
> 

FYI, I read "best effort" as a comment on how it's implemented today,
not what the implementation could provide.

> > Additionally, the callback, getsynctime64(), will only be called 
> > when 
> > n_samples == 1 because the driver returns only 1 cross-timestamp 
> > where 
> > multiple samples cannot be chained together.
> 
> There should be a way for user space to find out whether a particular
> device offers the cross timestamp capability.  There are reserved
> fields in 'struct ptp_clock_caps' that could be used.
> 

Yes, I agree. Otherwise we'd have to use only one sample in ptp4l in
order to benefit, but we'd be unable to tell it was worth it.

Regards,
Jake

> Thanks,
> Richard
> --
> 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