[RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Miroslav Lichvar]

This series adds support for a more accurate synchronization between a
PTP hardware clock and the system clock.

The first patch adds an extended version of the PTP_SYS_OFFSET ioctl,
which returns three timestamps for each measurement. The idea is to
shorten the interval between the system timestamps to contain just the
reading of the lowest register of the PHC in order to reduce the error
in the measured offset and give a better bound on the maximum error.

The other patches add support for the new ioctl to the e1000e, igb,
and ixgbe driver. Tests with few different NICs in different machines
(and PCIe slots) show that:
- with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
  and the error in the measured offset, when compared to cross
  timestamping, was reduced by a factor of 5
- with an I210 (igb) the delay improved from 5100 to 1700 ns
- with an I350 (igb) the delay improved from 2300 to 750 ns
- with an X550 (ixgbe) the delay improved from 1950 to 650 ns

There is some duplication of code in the igb and ixgbe drivers, which I
don't like very much, but I thought it's better than extending and
wrapping the existing functions like in the e1000e driver. Also, mixing
SYSTIM and "system time" in the code will probably be confusing.

I wasn't able to find a better name for the ioctl, the structures, and
the driver function. If anyone has suggestions, please let me know.

Miroslav Lichvar (4):
  ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
  e1000e: add support for extended PHC gettime
  igb: add support for extended PHC gettime
  ixgbe: add support for extended PHC gettime

 drivers/net/ethernet/intel/e1000e/e1000.h    |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c   | 48 +++++++++++++----
 drivers/net/ethernet/intel/e1000e/ptp.c      | 21 ++++++++
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 43 +++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
 drivers/ptp/ptp_chardev.c                    | 39 ++++++++++++++
 include/linux/ptp_clock_kernel.h             | 26 +++++++++
 include/uapi/linux/ptp_clock.h               | 12 +++++
 8 files changed, 239 insertions(+), 10 deletions(-)

-- 
2.17.2

[Intel-wired-lan] [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Miroslav Lichvar]

This series adds support for a more accurate synchronization between a
PTP hardware clock and the system clock.

The first patch adds an extended version of the PTP_SYS_OFFSET ioctl,
which returns three timestamps for each measurement. The idea is to
shorten the interval between the system timestamps to contain just the
reading of the lowest register of the PHC in order to reduce the error
in the measured offset and give a better bound on the maximum error.

The other patches add support for the new ioctl to the e1000e, igb,
and ixgbe driver. Tests with few different NICs in different machines
(and PCIe slots) show that:
- with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
  and the error in the measured offset, when compared to cross
  timestamping, was reduced by a factor of 5
- with an I210 (igb) the delay improved from 5100 to 1700 ns
- with an I350 (igb) the delay improved from 2300 to 750 ns
- with an X550 (ixgbe) the delay improved from 1950 to 650 ns

There is some duplication of code in the igb and ixgbe drivers, which I
don't like very much, but I thought it's better than extending and
wrapping the existing functions like in the e1000e driver. Also, mixing
SYSTIM and "system time" in the code will probably be confusing.

I wasn't able to find a better name for the ioctl, the structures, and
the driver function. If anyone has suggestions, please let me know.

Miroslav Lichvar (4):
  ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
  e1000e: add support for extended PHC gettime
  igb: add support for extended PHC gettime
  ixgbe: add support for extended PHC gettime

 drivers/net/ethernet/intel/e1000e/e1000.h    |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c   | 48 +++++++++++++----
 drivers/net/ethernet/intel/e1000e/ptp.c      | 21 ++++++++
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 43 +++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
 drivers/ptp/ptp_chardev.c                    | 39 ++++++++++++++
 include/linux/ptp_clock_kernel.h             | 26 +++++++++
 include/uapi/linux/ptp_clock.h               | 12 +++++
 8 files changed, 239 insertions(+), 10 deletions(-)

-- 
2.17.2

[RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Miroslav Lichvar]

The PTP_SYS_OFFSET ioctl, which can be used to measure the offset
between a PHC and the system clock, includes the total time that the
gettime64 function of a driver needs to read the PHC timestamp.

This typically involves reading of multiple PCI registers (sometimes in
multiple iterations) and the register that contains the lowest bits of
the timestamp is not read in the middle between the two readings of the
system clock. This asymmetry causes the measured offset to have a
significant error.

Introduce a new ioctl, driver function, and helper functions, which
allow the reading of the lowest register to be isolated from the other
readings in order to reduce the asymmetry. The ioctl and driver function
return three timestamps for each measurement:
- system time right before reading the lowest bits of the PHC timestamp
- PHC time
- system time immediately after reading the lowest bits of the PHC
  timestamp

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/ptp/ptp_chardev.c        | 39 ++++++++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h | 26 +++++++++++++++++++++
 include/uapi/linux/ptp_clock.h   | 12 ++++++++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 2012551d93e0..1a04c437fd4f 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock_caps caps;
 	struct ptp_clock_request req;
 	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_sys_offset_extended *sysoff_extended = NULL;
 	struct ptp_sys_offset_precise precise_offset;
 	struct ptp_pin_desc pd;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_clock_time *pct;
+	struct ptp_system_timestamp sts;
 	struct timespec64 ts;
 	struct system_device_crosststamp xtstamp;
 	int enable, err = 0;
@@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 		break;
 
+	case PTP_SYS_OFFSET_EXTENDED:
+		if (!ptp->info->gettimex64) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+		sysoff_extended = memdup_user((void __user *)arg,
+					      sizeof(*sysoff_extended));
+		if (IS_ERR(sysoff_extended)) {
+			err = PTR_ERR(sysoff_extended);
+			sysoff = NULL;
+			break;
+		}
+		if (sysoff_extended->n_samples > PTP_MAX_SAMPLES) {
+			err = -EINVAL;
+			break;
+		}
+
+		pct = &sysoff_extended->ts[0];
+		for (i = 0; i < sysoff_extended->n_samples; i++) {
+			err = ptp->info->gettimex64(ptp->info, &sts);
+			if (err)
+				break;
+			pct->sec = sts.sys_ts1.tv_sec;
+			pct->nsec = sts.sys_ts1.tv_nsec;
+			pct++;
+			pct->sec = sts.phc_ts.tv_sec;
+			pct->nsec = sts.phc_ts.tv_nsec;
+			pct++;
+			pct->sec = sts.sys_ts2.tv_sec;
+			pct->nsec = sts.sys_ts2.tv_nsec;
+			pct++;
+		}
+		if (copy_to_user((void __user *)arg, sysoff_extended,
+				 sizeof(*sysoff_extended)))
+			err = -EFAULT;
+		break;
+
 	case PTP_SYS_OFFSET:
 		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
 		if (IS_ERR(sysoff)) {
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 51349d124ee5..79321d929925 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -39,6 +39,13 @@ struct ptp_clock_request {
 };
 
 struct system_device_crosststamp;
+
+struct ptp_system_timestamp {
+	struct timespec64 sys_ts1;
+	struct timespec64 phc_ts;
+	struct timespec64 sys_ts2;
+};
+
 /**
  * struct ptp_clock_info - decribes a PTP hardware clock
  *
@@ -75,6 +82,13 @@ struct system_device_crosststamp;
  * @gettime64:  Reads the current time from the hardware clock.
  *              parameter ts: Holds the result.
  *
+ * @gettimex64:  Reads the current time from the system clock, hardware clock,
+ *               and system clock again.
+ *               parameter sts:  The structure contains system time right
+ *               before reading the lowest bits of the PHC timestamp, the PHC
+ *               timestamp itself, and system time immediately after reading
+ *               the lowest bits of the PHC timestamp.
+ *
  * @getcrosststamp:  Reads the current time from the hardware clock and
  *                   system clock simultaneously.
  *                   parameter cts: Contains timestamp (device,system) pair,
@@ -124,6 +138,8 @@ 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 (*gettimex64)(struct ptp_clock_info *ptp,
+			  struct ptp_system_timestamp *sts);
 	int (*getcrosststamp)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
@@ -227,6 +243,16 @@ int ptp_find_pin(struct ptp_clock *ptp,
 
 int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay);
 
+static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
+{
+	ktime_get_real_ts64(&sts->sys_ts1);
+}
+
+static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
+{
+	ktime_get_real_ts64(&sts->sys_ts2);
+}
+
 #else
 static inline struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 						   struct device *parent)
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 3039bf6a742e..0cb61aed9077 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -84,6 +84,16 @@ struct ptp_sys_offset {
 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
 };
 
+struct ptp_sys_offset_extended {
+	unsigned int n_samples; /* Desired number of measurements. */
+	unsigned int rsv[3];    /* Reserved for future use. */
+	/*
+	 * Array of sys, phc, sys, sys, phc, sys, ... time stamps. The kernel
+	 * will provide 3*n_samples time stamps.
+	 */
+	struct ptp_clock_time ts[3 * PTP_MAX_SAMPLES];
+};
+
 struct ptp_sys_offset_precise {
 	struct ptp_clock_time device;
 	struct ptp_clock_time sys_realtime;
@@ -136,6 +146,8 @@ struct ptp_pin_desc {
 #define PTP_PIN_SETFUNC    _IOW(PTP_CLK_MAGIC, 7, struct ptp_pin_desc)
 #define PTP_SYS_OFFSET_PRECISE \
 	_IOWR(PTP_CLK_MAGIC, 8, struct ptp_sys_offset_precise)
+#define PTP_SYS_OFFSET_EXTENDED \
+	_IOW(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
-- 
2.17.2

[Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Miroslav Lichvar]

The PTP_SYS_OFFSET ioctl, which can be used to measure the offset
between a PHC and the system clock, includes the total time that the
gettime64 function of a driver needs to read the PHC timestamp.

This typically involves reading of multiple PCI registers (sometimes in
multiple iterations) and the register that contains the lowest bits of
the timestamp is not read in the middle between the two readings of the
system clock. This asymmetry causes the measured offset to have a
significant error.

Introduce a new ioctl, driver function, and helper functions, which
allow the reading of the lowest register to be isolated from the other
readings in order to reduce the asymmetry. The ioctl and driver function
return three timestamps for each measurement:
- system time right before reading the lowest bits of the PHC timestamp
- PHC time
- system time immediately after reading the lowest bits of the PHC
  timestamp

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/ptp/ptp_chardev.c        | 39 ++++++++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h | 26 +++++++++++++++++++++
 include/uapi/linux/ptp_clock.h   | 12 ++++++++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 2012551d93e0..1a04c437fd4f 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock_caps caps;
 	struct ptp_clock_request req;
 	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_sys_offset_extended *sysoff_extended = NULL;
 	struct ptp_sys_offset_precise precise_offset;
 	struct ptp_pin_desc pd;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_clock_time *pct;
+	struct ptp_system_timestamp sts;
 	struct timespec64 ts;
 	struct system_device_crosststamp xtstamp;
 	int enable, err = 0;
@@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			err = -EFAULT;
 		break;
 
+	case PTP_SYS_OFFSET_EXTENDED:
+		if (!ptp->info->gettimex64) {
+			err = -EOPNOTSUPP;
+			break;
+		}
+		sysoff_extended = memdup_user((void __user *)arg,
+					      sizeof(*sysoff_extended));
+		if (IS_ERR(sysoff_extended)) {
+			err = PTR_ERR(sysoff_extended);
+			sysoff = NULL;
+			break;
+		}
+		if (sysoff_extended->n_samples > PTP_MAX_SAMPLES) {
+			err = -EINVAL;
+			break;
+		}
+
+		pct = &sysoff_extended->ts[0];
+		for (i = 0; i < sysoff_extended->n_samples; i++) {
+			err = ptp->info->gettimex64(ptp->info, &sts);
+			if (err)
+				break;
+			pct->sec = sts.sys_ts1.tv_sec;
+			pct->nsec = sts.sys_ts1.tv_nsec;
+			pct++;
+			pct->sec = sts.phc_ts.tv_sec;
+			pct->nsec = sts.phc_ts.tv_nsec;
+			pct++;
+			pct->sec = sts.sys_ts2.tv_sec;
+			pct->nsec = sts.sys_ts2.tv_nsec;
+			pct++;
+		}
+		if (copy_to_user((void __user *)arg, sysoff_extended,
+				 sizeof(*sysoff_extended)))
+			err = -EFAULT;
+		break;
+
 	case PTP_SYS_OFFSET:
 		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
 		if (IS_ERR(sysoff)) {
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 51349d124ee5..79321d929925 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -39,6 +39,13 @@ struct ptp_clock_request {
 };
 
 struct system_device_crosststamp;
+
+struct ptp_system_timestamp {
+	struct timespec64 sys_ts1;
+	struct timespec64 phc_ts;
+	struct timespec64 sys_ts2;
+};
+
 /**
  * struct ptp_clock_info - decribes a PTP hardware clock
  *
@@ -75,6 +82,13 @@ struct system_device_crosststamp;
  * @gettime64:  Reads the current time from the hardware clock.
  *              parameter ts: Holds the result.
  *
+ * @gettimex64:  Reads the current time from the system clock, hardware clock,
+ *               and system clock again.
+ *               parameter sts:  The structure contains system time right
+ *               before reading the lowest bits of the PHC timestamp, the PHC
+ *               timestamp itself, and system time immediately after reading
+ *               the lowest bits of the PHC timestamp.
+ *
  * @getcrosststamp:  Reads the current time from the hardware clock and
  *                   system clock simultaneously.
  *                   parameter cts: Contains timestamp (device,system) pair,
@@ -124,6 +138,8 @@ 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 (*gettimex64)(struct ptp_clock_info *ptp,
+			  struct ptp_system_timestamp *sts);
 	int (*getcrosststamp)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
@@ -227,6 +243,16 @@ int ptp_find_pin(struct ptp_clock *ptp,
 
 int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay);
 
+static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
+{
+	ktime_get_real_ts64(&sts->sys_ts1);
+}
+
+static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
+{
+	ktime_get_real_ts64(&sts->sys_ts2);
+}
+
 #else
 static inline struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 						   struct device *parent)
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 3039bf6a742e..0cb61aed9077 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -84,6 +84,16 @@ struct ptp_sys_offset {
 	struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
 };
 
+struct ptp_sys_offset_extended {
+	unsigned int n_samples; /* Desired number of measurements. */
+	unsigned int rsv[3];    /* Reserved for future use. */
+	/*
+	 * Array of sys, phc, sys, sys, phc, sys, ... time stamps. The kernel
+	 * will provide 3*n_samples time stamps.
+	 */
+	struct ptp_clock_time ts[3 * PTP_MAX_SAMPLES];
+};
+
 struct ptp_sys_offset_precise {
 	struct ptp_clock_time device;
 	struct ptp_clock_time sys_realtime;
@@ -136,6 +146,8 @@ struct ptp_pin_desc {
 #define PTP_PIN_SETFUNC    _IOW(PTP_CLK_MAGIC, 7, struct ptp_pin_desc)
 #define PTP_SYS_OFFSET_PRECISE \
 	_IOWR(PTP_CLK_MAGIC, 8, struct ptp_sys_offset_precise)
+#define PTP_SYS_OFFSET_EXTENDED \
+	_IOW(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
-- 
2.17.2

Re: [Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Vinicius Costa Gomes]

Hi Miroslav,

Miroslav Lichvar <mlichvar@redhat.com> writes:

> The PTP_SYS_OFFSET ioctl, which can be used to measure the offset
> between a PHC and the system clock, includes the total time that the
> gettime64 function of a driver needs to read the PHC timestamp.
>
> This typically involves reading of multiple PCI registers (sometimes in
> multiple iterations) and the register that contains the lowest bits of
> the timestamp is not read in the middle between the two readings of the
> system clock. This asymmetry causes the measured offset to have a
> significant error.
>
> Introduce a new ioctl, driver function, and helper functions, which
> allow the reading of the lowest register to be isolated from the other
> readings in order to reduce the asymmetry. The ioctl and driver function
> return three timestamps for each measurement:
> - system time right before reading the lowest bits of the PHC timestamp
> - PHC time
> - system time immediately after reading the lowest bits of the PHC
>   timestamp

Cool stuff!

Just one little thing below. Feel free to add my ack to the series.

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
>  drivers/ptp/ptp_chardev.c        | 39 ++++++++++++++++++++++++++++++++
>  include/linux/ptp_clock_kernel.h | 26 +++++++++++++++++++++
>  include/uapi/linux/ptp_clock.h   | 12 ++++++++++
>  3 files changed, 77 insertions(+)
>
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 2012551d93e0..1a04c437fd4f 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	struct ptp_clock_caps caps;
>  	struct ptp_clock_request req;
>  	struct ptp_sys_offset *sysoff = NULL;
> +	struct ptp_sys_offset_extended *sysoff_extended = NULL;
>  	struct ptp_sys_offset_precise precise_offset;
>  	struct ptp_pin_desc pd;
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
>  	struct ptp_clock_info *ops = ptp->info;
>  	struct ptp_clock_time *pct;
> +	struct ptp_system_timestamp sts;
>  	struct timespec64 ts;
>  	struct system_device_crosststamp xtstamp;
>  	int enable, err = 0;
> @@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  		break;
>  
> +	case PTP_SYS_OFFSET_EXTENDED:
> +		if (!ptp->info->gettimex64) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +		sysoff_extended = memdup_user((void __user *)arg,
> +					      sizeof(*sysoff_extended));

Looks like you forgot to free 'sysoff_extended', no? 

[Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Vinicius Costa Gomes]

Hi Miroslav,

Miroslav Lichvar <mlichvar@redhat.com> writes:

> The PTP_SYS_OFFSET ioctl, which can be used to measure the offset
> between a PHC and the system clock, includes the total time that the
> gettime64 function of a driver needs to read the PHC timestamp.
>
> This typically involves reading of multiple PCI registers (sometimes in
> multiple iterations) and the register that contains the lowest bits of
> the timestamp is not read in the middle between the two readings of the
> system clock. This asymmetry causes the measured offset to have a
> significant error.
>
> Introduce a new ioctl, driver function, and helper functions, which
> allow the reading of the lowest register to be isolated from the other
> readings in order to reduce the asymmetry. The ioctl and driver function
> return three timestamps for each measurement:
> - system time right before reading the lowest bits of the PHC timestamp
> - PHC time
> - system time immediately after reading the lowest bits of the PHC
>   timestamp

Cool stuff!

Just one little thing below. Feel free to add my ack to the series.

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
>  drivers/ptp/ptp_chardev.c        | 39 ++++++++++++++++++++++++++++++++
>  include/linux/ptp_clock_kernel.h | 26 +++++++++++++++++++++
>  include/uapi/linux/ptp_clock.h   | 12 ++++++++++
>  3 files changed, 77 insertions(+)
>
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 2012551d93e0..1a04c437fd4f 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	struct ptp_clock_caps caps;
>  	struct ptp_clock_request req;
>  	struct ptp_sys_offset *sysoff = NULL;
> +	struct ptp_sys_offset_extended *sysoff_extended = NULL;
>  	struct ptp_sys_offset_precise precise_offset;
>  	struct ptp_pin_desc pd;
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
>  	struct ptp_clock_info *ops = ptp->info;
>  	struct ptp_clock_time *pct;
> +	struct ptp_system_timestamp sts;
>  	struct timespec64 ts;
>  	struct system_device_crosststamp xtstamp;
>  	int enable, err = 0;
> @@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  		break;
>  
> +	case PTP_SYS_OFFSET_EXTENDED:
> +		if (!ptp->info->gettimex64) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +		sysoff_extended = memdup_user((void __user *)arg,
> +					      sizeof(*sysoff_extended));

Looks like you forgot to free 'sysoff_extended', no? 


--
Vinicius

Re: [Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Miroslav Lichvar]

On Fri, Oct 26, 2018 at 03:16:47PM -0700, Vinicius Costa Gomes wrote:
> > +	case PTP_SYS_OFFSET_EXTENDED:
> > +		if (!ptp->info->gettimex64) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> > +		sysoff_extended = memdup_user((void __user *)arg,
> > +					      sizeof(*sysoff_extended));
> 
> Looks like you forgot to free 'sysoff_extended', no? 

Oh, I did. Thanks for catching that. I'll fix it in the next version.

-- 
Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Miroslav Lichvar]

On Fri, Oct 26, 2018 at 03:16:47PM -0700, Vinicius Costa Gomes wrote:
> > +	case PTP_SYS_OFFSET_EXTENDED:
> > +		if (!ptp->info->gettimex64) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> > +		sysoff_extended = memdup_user((void __user *)arg,
> > +					      sizeof(*sysoff_extended));
> 
> Looks like you forgot to free 'sysoff_extended', no? 

Oh, I did. Thanks for catching that. I'll fix it in the next version.

-- 
Miroslav Lichvar

Re: [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:39PM +0200, Miroslav Lichvar wrote:
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 2012551d93e0..1a04c437fd4f 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	struct ptp_clock_caps caps;
>  	struct ptp_clock_request req;
>  	struct ptp_sys_offset *sysoff = NULL;
> +	struct ptp_sys_offset_extended *sysoff_extended = NULL;

How about a more succinct name, like 'extoff' ?

>  	struct ptp_sys_offset_precise precise_offset;
>  	struct ptp_pin_desc pd;
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
>  	struct ptp_clock_info *ops = ptp->info;
>  	struct ptp_clock_time *pct;
> +	struct ptp_system_timestamp sts;
>  	struct timespec64 ts;
>  	struct system_device_crosststamp xtstamp;
>  	int enable, err = 0;

This collection of automatic variables is getting ugly.  May I ask you
to prefix a patch that puts them into reverse Christmas tree before
your changes?  (Patch below)

> @@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  		break;
>  
> +	case PTP_SYS_OFFSET_EXTENDED:
> +		if (!ptp->info->gettimex64) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +		sysoff_extended = memdup_user((void __user *)arg,
> +					      sizeof(*sysoff_extended));

As pointed out before, this needs to be freed, and

> +		if (IS_ERR(sysoff_extended)) {
> +			err = PTR_ERR(sysoff_extended);
> +			sysoff = NULL;

here you meant, sysoff_extended = NULL;

> +			break;
> +		}
> +		if (sysoff_extended->n_samples > PTP_MAX_SAMPLES) {
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		pct = &sysoff_extended->ts[0];
> +		for (i = 0; i < sysoff_extended->n_samples; i++) {
> +			err = ptp->info->gettimex64(ptp->info, &sts);
> +			if (err)
> +				break;
> +			pct->sec = sts.sys_ts1.tv_sec;
> +			pct->nsec = sts.sys_ts1.tv_nsec;
> +			pct++;
> +			pct->sec = sts.phc_ts.tv_sec;
> +			pct->nsec = sts.phc_ts.tv_nsec;
> +			pct++;
> +			pct->sec = sts.sys_ts2.tv_sec;
> +			pct->nsec = sts.sys_ts2.tv_nsec;
> +			pct++;
> +		}
> +		if (copy_to_user((void __user *)arg, sysoff_extended,
> +				 sizeof(*sysoff_extended)))
> +			err = -EFAULT;
> +		break;
> +
>  	case PTP_SYS_OFFSET:
>  		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
>  		if (IS_ERR(sysoff)) {
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 51349d124ee5..79321d929925 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -39,6 +39,13 @@ struct ptp_clock_request {
>  };
>  
>  struct system_device_crosststamp;
> +

KernelDoc please for this:

> +struct ptp_system_timestamp {
> +	struct timespec64 sys_ts1;
> +	struct timespec64 phc_ts;
> +	struct timespec64 sys_ts2;
> +};
> +

Thanks,
Richard

[Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:39PM +0200, Miroslav Lichvar wrote:
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 2012551d93e0..1a04c437fd4f 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -124,11 +124,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  	struct ptp_clock_caps caps;
>  	struct ptp_clock_request req;
>  	struct ptp_sys_offset *sysoff = NULL;
> +	struct ptp_sys_offset_extended *sysoff_extended = NULL;

How about a more succinct name, like 'extoff' ?

>  	struct ptp_sys_offset_precise precise_offset;
>  	struct ptp_pin_desc pd;
>  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
>  	struct ptp_clock_info *ops = ptp->info;
>  	struct ptp_clock_time *pct;
> +	struct ptp_system_timestamp sts;
>  	struct timespec64 ts;
>  	struct system_device_crosststamp xtstamp;
>  	int enable, err = 0;

This collection of automatic variables is getting ugly.  May I ask you
to prefix a patch that puts them into reverse Christmas tree before
your changes?  (Patch below)

> @@ -211,6 +213,43 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>  			err = -EFAULT;
>  		break;
>  
> +	case PTP_SYS_OFFSET_EXTENDED:
> +		if (!ptp->info->gettimex64) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}
> +		sysoff_extended = memdup_user((void __user *)arg,
> +					      sizeof(*sysoff_extended));

As pointed out before, this needs to be freed, and

> +		if (IS_ERR(sysoff_extended)) {
> +			err = PTR_ERR(sysoff_extended);
> +			sysoff = NULL;

here you meant, sysoff_extended = NULL;

> +			break;
> +		}
> +		if (sysoff_extended->n_samples > PTP_MAX_SAMPLES) {
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		pct = &sysoff_extended->ts[0];
> +		for (i = 0; i < sysoff_extended->n_samples; i++) {
> +			err = ptp->info->gettimex64(ptp->info, &sts);
> +			if (err)
> +				break;
> +			pct->sec = sts.sys_ts1.tv_sec;
> +			pct->nsec = sts.sys_ts1.tv_nsec;
> +			pct++;
> +			pct->sec = sts.phc_ts.tv_sec;
> +			pct->nsec = sts.phc_ts.tv_nsec;
> +			pct++;
> +			pct->sec = sts.sys_ts2.tv_sec;
> +			pct->nsec = sts.sys_ts2.tv_nsec;
> +			pct++;
> +		}
> +		if (copy_to_user((void __user *)arg, sysoff_extended,
> +				 sizeof(*sysoff_extended)))
> +			err = -EFAULT;
> +		break;
> +
>  	case PTP_SYS_OFFSET:
>  		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
>  		if (IS_ERR(sysoff)) {
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 51349d124ee5..79321d929925 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -39,6 +39,13 @@ struct ptp_clock_request {
>  };
>  
>  struct system_device_crosststamp;
> +

KernelDoc please for this:

> +struct ptp_system_timestamp {
> +	struct timespec64 sys_ts1;
> +	struct timespec64 phc_ts;
> +	struct timespec64 sys_ts2;
> +};
> +

Thanks,
Richard

Re: [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Richard Cochran]

On Tue, Oct 30, 2018 at 07:23:51PM -0700, Richard Cochran wrote:
> This collection of automatic variables is getting ugly.  May I ask you
> to prefix a patch that puts them into reverse Christmas tree before
> your changes?  (Patch below)

Forgot the diff. Here it is...

---
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 2012551d93e0..b54b8158ff8a 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -121,18 +121,18 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
 
 long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
-	struct ptp_clock_caps caps;
-	struct ptp_clock_request req;
-	struct ptp_sys_offset *sysoff = NULL;
-	struct ptp_sys_offset_precise precise_offset;
-	struct ptp_pin_desc pd;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct ptp_sys_offset_precise precise_offset;
+	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
+	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_clock_request req;
+	struct ptp_clock_caps caps;
 	struct ptp_clock_time *pct;
+	unsigned int i, pin_index;
+	struct ptp_pin_desc pd;
 	struct timespec64 ts;
-	struct system_device_crosststamp xtstamp;
 	int enable, err = 0;
-	unsigned int i, pin_index;
 
 	switch (cmd) {
 

[Intel-wired-lan] [RFC PATCH 1/4] ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

[Richard Cochran]

On Tue, Oct 30, 2018 at 07:23:51PM -0700, Richard Cochran wrote:
> This collection of automatic variables is getting ugly.  May I ask you
> to prefix a patch that puts them into reverse Christmas tree before
> your changes?  (Patch below)

Forgot the diff. Here it is...

---
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 2012551d93e0..b54b8158ff8a 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -121,18 +121,18 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode)
 
 long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
-	struct ptp_clock_caps caps;
-	struct ptp_clock_request req;
-	struct ptp_sys_offset *sysoff = NULL;
-	struct ptp_sys_offset_precise precise_offset;
-	struct ptp_pin_desc pd;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+	struct ptp_sys_offset_precise precise_offset;
+	struct system_device_crosststamp xtstamp;
 	struct ptp_clock_info *ops = ptp->info;
+	struct ptp_sys_offset *sysoff = NULL;
+	struct ptp_clock_request req;
+	struct ptp_clock_caps caps;
 	struct ptp_clock_time *pct;
+	unsigned int i, pin_index;
+	struct ptp_pin_desc pd;
 	struct timespec64 ts;
-	struct system_device_crosststamp xtstamp;
 	int enable, err = 0;
-	unsigned int i, pin_index;
 
 	switch (cmd) {
 

[RFC PATCH 2/4] e1000e: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h  |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c | 48 +++++++++++++++++-----
 drivers/net/ethernet/intel/e1000e/ptp.c    | 21 ++++++++++
 3 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index c760dc72c520..be13227f1697 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -505,6 +505,9 @@ extern const struct e1000_info e1000_es2_info;
 void e1000e_ptp_init(struct e1000_adapter *adapter);
 void e1000e_ptp_remove(struct e1000_adapter *adapter);
 
+u64 e1000e_read_systim(struct e1000_adapter *adapter,
+		       struct ptp_system_timestamp *sts);
+
 static inline s32 e1000_phy_hw_reset(struct e1000_hw *hw)
 {
 	return hw->phy.ops.reset(hw);
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 3ba0c90e7055..3bad1a1f36c3 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4319,13 +4319,16 @@ void e1000e_reinit_locked(struct e1000_adapter *adapter)
 /**
  * e1000e_sanitize_systim - sanitize raw cycle counter reads
  * @hw: pointer to the HW structure
- * @systim: time value read, sanitized and returned
+ * @systim: PHC time value read, sanitized and returned
+ * @sts: structure which will contain system time before and after reading
+ * SYSTIML, may be NULL
  *
  * Errata for 82574/82583 possible bad bits read from SYSTIMH/L:
  * check to see that the time is incrementing at a reasonable
  * rate and is a multiple of incvalue.
  **/
-static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
+static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim,
+				  struct ptp_system_timestamp *sts)
 {
 	u64 time_delta, rem, temp;
 	u64 systim_next;
@@ -4335,7 +4338,11 @@ static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
 	incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
 	for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
 		/* latch SYSTIMH on read of SYSTIML */
+		if (sts)
+			ptp_read_system_prets(sts);
 		systim_next = (u64)er32(SYSTIML);
+		if (sts)
+			ptp_read_system_postts(sts);
 		systim_next |= (u64)er32(SYSTIMH) << 32;
 
 		time_delta = systim_next - systim;
@@ -4353,15 +4360,16 @@ static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
 }
 
 /**
- * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
- * @cc: cyclecounter structure
+ * e1000e_read_systim - read SYSTIM register
+ * @adapter: board private structure
+ * @sts: structure which will contain system time before and after reading
+ * SYSTIML, may be NULL
  **/
-static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
+u64 e1000e_read_systim(struct e1000_adapter *adapter,
+		       struct ptp_system_timestamp *sts)
 {
-	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
-						     cc);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 systimel, systimeh;
+	u32 systimel, systimel_2, systimeh;
 	u64 systim;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
@@ -4369,11 +4377,19 @@ static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	 * will experience a huge non linear increment in the systime value
 	 * to fix that we test for overflow and if true, we re-read systime.
 	 */
+	if (sts)
+		ptp_read_system_prets(sts);
 	systimel = er32(SYSTIML);
+	if (sts)
+		ptp_read_system_postts(sts);
 	systimeh = er32(SYSTIMH);
 	/* Is systimel is so large that overflow is possible? */
 	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {
-		u32 systimel_2 = er32(SYSTIML);
+		if (sts)
+			ptp_read_system_prets(sts);
+		systimel_2 = er32(SYSTIML);
+		if (sts)
+			ptp_read_system_postts(sts);
 		if (systimel > systimel_2) {
 			/* There was an overflow, read again SYSTIMH, and use
 			 * systimel_2
@@ -4386,11 +4402,23 @@ static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim |= (u64)systimeh << 32;
 
 	if (adapter->flags2 & FLAG2_CHECK_SYSTIM_OVERFLOW)
-		systim = e1000e_sanitize_systim(hw, systim);
+		systim = e1000e_sanitize_systim(hw, systim, sts);
 
 	return systim;
 }
 
+/**
+ * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
+ * @cc: cyclecounter structure
+ **/
+static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
+{
+	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
+						     cc);
+
+	return e1000e_read_systim(adapter, NULL);
+}
+
 /**
  * e1000_sw_init - Initialize general software structures (struct e1000_adapter)
  * @adapter: board private structure to initialize
diff --git a/drivers/net/ethernet/intel/e1000e/ptp.c b/drivers/net/ethernet/intel/e1000e/ptp.c
index e1f821edbc21..bf1ca7ba8c37 100644
--- a/drivers/net/ethernet/intel/e1000e/ptp.c
+++ b/drivers/net/ethernet/intel/e1000e/ptp.c
@@ -188,6 +188,26 @@ static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return 0;
 }
 
+static int e1000e_phc_gettimex(struct ptp_clock_info *ptp,
+			       struct ptp_system_timestamp *sts)
+{
+	struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter,
+						     ptp_clock_info);
+	unsigned long flags;
+	u64 cycles, ns;
+
+	spin_lock_irqsave(&adapter->systim_lock, flags);
+
+	cycles = e1000e_read_systim(adapter, sts);
+	ns = timecounter_cyc2time(&adapter->tc, cycles);
+
+	spin_unlock_irqrestore(&adapter->systim_lock, flags);
+
+	sts->phc_ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
 /**
  * e1000e_phc_settime - Set the current time on the hardware clock
  * @ptp: ptp clock structure
@@ -259,6 +279,7 @@ static const struct ptp_clock_info e1000e_ptp_clock_info = {
 	.adjfreq	= e1000e_phc_adjfreq,
 	.adjtime	= e1000e_phc_adjtime,
 	.gettime64	= e1000e_phc_gettime,
+	.gettimex64	= e1000e_phc_gettimex,
 	.settime64	= e1000e_phc_settime,
 	.enable		= e1000e_phc_enable,
 };
-- 
2.17.2

[Intel-wired-lan] [RFC PATCH 2/4] e1000e: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h  |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c | 48 +++++++++++++++++-----
 drivers/net/ethernet/intel/e1000e/ptp.c    | 21 ++++++++++
 3 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index c760dc72c520..be13227f1697 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -505,6 +505,9 @@ extern const struct e1000_info e1000_es2_info;
 void e1000e_ptp_init(struct e1000_adapter *adapter);
 void e1000e_ptp_remove(struct e1000_adapter *adapter);
 
+u64 e1000e_read_systim(struct e1000_adapter *adapter,
+		       struct ptp_system_timestamp *sts);
+
 static inline s32 e1000_phy_hw_reset(struct e1000_hw *hw)
 {
 	return hw->phy.ops.reset(hw);
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 3ba0c90e7055..3bad1a1f36c3 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4319,13 +4319,16 @@ void e1000e_reinit_locked(struct e1000_adapter *adapter)
 /**
  * e1000e_sanitize_systim - sanitize raw cycle counter reads
  * @hw: pointer to the HW structure
- * @systim: time value read, sanitized and returned
+ * @systim: PHC time value read, sanitized and returned
+ * @sts: structure which will contain system time before and after reading
+ * SYSTIML, may be NULL
  *
  * Errata for 82574/82583 possible bad bits read from SYSTIMH/L:
  * check to see that the time is incrementing at a reasonable
  * rate and is a multiple of incvalue.
  **/
-static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
+static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim,
+				  struct ptp_system_timestamp *sts)
 {
 	u64 time_delta, rem, temp;
 	u64 systim_next;
@@ -4335,7 +4338,11 @@ static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
 	incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
 	for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
 		/* latch SYSTIMH on read of SYSTIML */
+		if (sts)
+			ptp_read_system_prets(sts);
 		systim_next = (u64)er32(SYSTIML);
+		if (sts)
+			ptp_read_system_postts(sts);
 		systim_next |= (u64)er32(SYSTIMH) << 32;
 
 		time_delta = systim_next - systim;
@@ -4353,15 +4360,16 @@ static u64 e1000e_sanitize_systim(struct e1000_hw *hw, u64 systim)
 }
 
 /**
- * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
- * @cc: cyclecounter structure
+ * e1000e_read_systim - read SYSTIM register
+ * @adapter: board private structure
+ * @sts: structure which will contain system time before and after reading
+ * SYSTIML, may be NULL
  **/
-static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
+u64 e1000e_read_systim(struct e1000_adapter *adapter,
+		       struct ptp_system_timestamp *sts)
 {
-	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
-						     cc);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 systimel, systimeh;
+	u32 systimel, systimel_2, systimeh;
 	u64 systim;
 	/* SYSTIMH latching upon SYSTIML read does not work well.
 	 * This means that if SYSTIML overflows after we read it but before
@@ -4369,11 +4377,19 @@ static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	 * will experience a huge non linear increment in the systime value
 	 * to fix that we test for overflow and if true, we re-read systime.
 	 */
+	if (sts)
+		ptp_read_system_prets(sts);
 	systimel = er32(SYSTIML);
+	if (sts)
+		ptp_read_system_postts(sts);
 	systimeh = er32(SYSTIMH);
 	/* Is systimel is so large that overflow is possible? */
 	if (systimel >= (u32)0xffffffff - E1000_TIMINCA_INCVALUE_MASK) {
-		u32 systimel_2 = er32(SYSTIML);
+		if (sts)
+			ptp_read_system_prets(sts);
+		systimel_2 = er32(SYSTIML);
+		if (sts)
+			ptp_read_system_postts(sts);
 		if (systimel > systimel_2) {
 			/* There was an overflow, read again SYSTIMH, and use
 			 * systimel_2
@@ -4386,11 +4402,23 @@ static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
 	systim |= (u64)systimeh << 32;
 
 	if (adapter->flags2 & FLAG2_CHECK_SYSTIM_OVERFLOW)
-		systim = e1000e_sanitize_systim(hw, systim);
+		systim = e1000e_sanitize_systim(hw, systim, sts);
 
 	return systim;
 }
 
+/**
+ * e1000e_cyclecounter_read - read raw cycle counter (used by time counter)
+ * @cc: cyclecounter structure
+ **/
+static u64 e1000e_cyclecounter_read(const struct cyclecounter *cc)
+{
+	struct e1000_adapter *adapter = container_of(cc, struct e1000_adapter,
+						     cc);
+
+	return e1000e_read_systim(adapter, NULL);
+}
+
 /**
  * e1000_sw_init - Initialize general software structures (struct e1000_adapter)
  * @adapter: board private structure to initialize
diff --git a/drivers/net/ethernet/intel/e1000e/ptp.c b/drivers/net/ethernet/intel/e1000e/ptp.c
index e1f821edbc21..bf1ca7ba8c37 100644
--- a/drivers/net/ethernet/intel/e1000e/ptp.c
+++ b/drivers/net/ethernet/intel/e1000e/ptp.c
@@ -188,6 +188,26 @@ static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return 0;
 }
 
+static int e1000e_phc_gettimex(struct ptp_clock_info *ptp,
+			       struct ptp_system_timestamp *sts)
+{
+	struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter,
+						     ptp_clock_info);
+	unsigned long flags;
+	u64 cycles, ns;
+
+	spin_lock_irqsave(&adapter->systim_lock, flags);
+
+	cycles = e1000e_read_systim(adapter, sts);
+	ns = timecounter_cyc2time(&adapter->tc, cycles);
+
+	spin_unlock_irqrestore(&adapter->systim_lock, flags);
+
+	sts->phc_ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
 /**
  * e1000e_phc_settime - Set the current time on the hardware clock
  * @ptp: ptp clock structure
@@ -259,6 +279,7 @@ static const struct ptp_clock_info e1000e_ptp_clock_info = {
 	.adjfreq	= e1000e_phc_adjfreq,
 	.adjtime	= e1000e_phc_adjtime,
 	.gettime64	= e1000e_phc_gettime,
+	.gettimex64	= e1000e_phc_gettimex,
 	.settime64	= e1000e_phc_settime,
 	.enable		= e1000e_phc_enable,
 };
-- 
2.17.2

[RFC PATCH 3/4] igb: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 43 ++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 29ced6b74d36..6294d18b5a60 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -310,6 +310,46 @@ static int igb_ptp_gettime_i210(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
+			    struct ptp_system_timestamp *sts)
+{
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
+	struct e1000_hw *hw = &igb->hw;
+	unsigned long flags;
+	u32 lo, hi;
+	u64 ns;
+
+	spin_lock_irqsave(&igb->tmreg_lock, flags);
+
+	/* 82576 doesn't have SYSTIMR */
+	if (igb->hw.mac.type == e1000_82576) {
+		ptp_read_system_prets(sts);
+		lo = rd32(E1000_SYSTIML);
+		ptp_read_system_postts(sts);
+		hi = rd32(E1000_SYSTIMH);
+	} else {
+		ptp_read_system_prets(sts);
+		rd32(E1000_SYSTIMR);
+		ptp_read_system_postts(sts);
+		lo = rd32(E1000_SYSTIML);
+		hi = rd32(E1000_SYSTIMH);
+	}
+
+	/* SYSTIM on I210/I211 counts time in seconds and nanoseconds */
+	if (igb->hw.mac.type == e1000_i210 || igb->hw.mac.type == e1000_i211) {
+		sts->phc_ts.tv_sec = hi;
+		sts->phc_ts.tv_nsec = lo;
+	} else {
+		ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
+		sts->phc_ts = ns_to_timespec64(ns);
+	}
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	return 0;
+}
+
 static int igb_ptp_settime_82576(struct ptp_clock_info *ptp,
 				 const struct timespec64 *ts)
 {
@@ -1125,6 +1165,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82576;
@@ -1144,6 +1185,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82580;
@@ -1172,6 +1214,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_i210;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_i210;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
 		adapter->ptp_caps.verify = igb_ptp_verify_pin;
-- 
2.17.2

[Intel-wired-lan] [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 43 ++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 29ced6b74d36..6294d18b5a60 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -310,6 +310,46 @@ static int igb_ptp_gettime_i210(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
+			    struct ptp_system_timestamp *sts)
+{
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
+	struct e1000_hw *hw = &igb->hw;
+	unsigned long flags;
+	u32 lo, hi;
+	u64 ns;
+
+	spin_lock_irqsave(&igb->tmreg_lock, flags);
+
+	/* 82576 doesn't have SYSTIMR */
+	if (igb->hw.mac.type == e1000_82576) {
+		ptp_read_system_prets(sts);
+		lo = rd32(E1000_SYSTIML);
+		ptp_read_system_postts(sts);
+		hi = rd32(E1000_SYSTIMH);
+	} else {
+		ptp_read_system_prets(sts);
+		rd32(E1000_SYSTIMR);
+		ptp_read_system_postts(sts);
+		lo = rd32(E1000_SYSTIML);
+		hi = rd32(E1000_SYSTIMH);
+	}
+
+	/* SYSTIM on I210/I211 counts time in seconds and nanoseconds */
+	if (igb->hw.mac.type == e1000_i210 || igb->hw.mac.type == e1000_i211) {
+		sts->phc_ts.tv_sec = hi;
+		sts->phc_ts.tv_nsec = lo;
+	} else {
+		ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
+		sts->phc_ts = ns_to_timespec64(ns);
+	}
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	return 0;
+}
+
 static int igb_ptp_settime_82576(struct ptp_clock_info *ptp,
 				 const struct timespec64 *ts)
 {
@@ -1125,6 +1165,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82576;
@@ -1144,6 +1185,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82580;
@@ -1172,6 +1214,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
 		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
 		adapter->ptp_caps.gettime64 = igb_ptp_gettime_i210;
+		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_i210;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
 		adapter->ptp_caps.verify = igb_ptp_verify_pin;
-- 
2.17.2

Re: [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> +			    struct ptp_system_timestamp *sts)
> +{
> +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> +					       ptp_caps);
> +	struct e1000_hw *hw = &igb->hw;
> +	unsigned long flags;
> +	u32 lo, hi;
> +	u64 ns;
> +
> +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> +
> +	/* 82576 doesn't have SYSTIMR */
> +	if (igb->hw.mac.type == e1000_82576) {

Instead of if/then/else, can't you follow the pattern of providing
different function flavors ...

> +		ptp_read_system_prets(sts);
> +		lo = rd32(E1000_SYSTIML);
> +		ptp_read_system_postts(sts);
> +		hi = rd32(E1000_SYSTIMH);
> +	} else {
> +		ptp_read_system_prets(sts);
> +		rd32(E1000_SYSTIMR);
> +		ptp_read_system_postts(sts);
> +		lo = rd32(E1000_SYSTIML);
> +		hi = rd32(E1000_SYSTIMH);
> +	}
> +
> +	/* SYSTIM on I210/I211 counts time in seconds and nanoseconds */
> +	if (igb->hw.mac.type == e1000_i210 || igb->hw.mac.type == e1000_i211) {
> +		sts->phc_ts.tv_sec = hi;
> +		sts->phc_ts.tv_nsec = lo;
> +	} else {
> +		ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
> +		sts->phc_ts = ns_to_timespec64(ns);
> +	}
> +
> +	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> +
> +	return 0;
> +}
> +
>  static int igb_ptp_settime_82576(struct ptp_clock_info *ptp,
>  				 const struct timespec64 *ts)
>  {
> @@ -1125,6 +1165,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;

... here?

Thanks,
Richard

>  		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable;
>  		adapter->cc.read = igb_ptp_read_82576;
> @@ -1144,6 +1185,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable;
>  		adapter->cc.read = igb_ptp_read_82580;
> @@ -1172,6 +1214,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_i210;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = igb_ptp_settime_i210;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
>  		adapter->ptp_caps.verify = igb_ptp_verify_pin;

[Intel-wired-lan] [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> +			    struct ptp_system_timestamp *sts)
> +{
> +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> +					       ptp_caps);
> +	struct e1000_hw *hw = &igb->hw;
> +	unsigned long flags;
> +	u32 lo, hi;
> +	u64 ns;
> +
> +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> +
> +	/* 82576 doesn't have SYSTIMR */
> +	if (igb->hw.mac.type == e1000_82576) {

Instead of if/then/else, can't you follow the pattern of providing
different function flavors ...

> +		ptp_read_system_prets(sts);
> +		lo = rd32(E1000_SYSTIML);
> +		ptp_read_system_postts(sts);
> +		hi = rd32(E1000_SYSTIMH);
> +	} else {
> +		ptp_read_system_prets(sts);
> +		rd32(E1000_SYSTIMR);
> +		ptp_read_system_postts(sts);
> +		lo = rd32(E1000_SYSTIML);
> +		hi = rd32(E1000_SYSTIMH);
> +	}
> +
> +	/* SYSTIM on I210/I211 counts time in seconds and nanoseconds */
> +	if (igb->hw.mac.type == e1000_i210 || igb->hw.mac.type == e1000_i211) {
> +		sts->phc_ts.tv_sec = hi;
> +		sts->phc_ts.tv_nsec = lo;
> +	} else {
> +		ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
> +		sts->phc_ts = ns_to_timespec64(ns);
> +	}
> +
> +	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> +
> +	return 0;
> +}
> +
>  static int igb_ptp_settime_82576(struct ptp_clock_info *ptp,
>  				 const struct timespec64 *ts)
>  {
> @@ -1125,6 +1165,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;

... here?

Thanks,
Richard

>  		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable;
>  		adapter->cc.read = igb_ptp_read_82576;
> @@ -1144,6 +1185,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_82576;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_82576;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable;
>  		adapter->cc.read = igb_ptp_read_82580;
> @@ -1172,6 +1214,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
>  		adapter->ptp_caps.adjfine = igb_ptp_adjfine_82580;
>  		adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
>  		adapter->ptp_caps.gettime64 = igb_ptp_gettime_i210;
> +		adapter->ptp_caps.gettimex64 = igb_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = igb_ptp_settime_i210;
>  		adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
>  		adapter->ptp_caps.verify = igb_ptp_verify_pin;

Re: [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Miroslav Lichvar]

On Tue, Oct 30, 2018 at 07:29:16PM -0700, Richard Cochran wrote:
> On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> > +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> > +			    struct ptp_system_timestamp *sts)
> > +{
> > +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> > +					       ptp_caps);
> > +	struct e1000_hw *hw = &igb->hw;
> > +	unsigned long flags;
> > +	u32 lo, hi;
> > +	u64 ns;
> > +
> > +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> > +
> > +	/* 82576 doesn't have SYSTIMR */
> > +	if (igb->hw.mac.type == e1000_82576) {
> 
> Instead of if/then/else, can't you follow the pattern of providing
> different function flavors ...

I can. I was just trying to minimize the amount of triplicated code.
In the next version I'll add a patch to deprecate the old gettime
functions, as Jacob suggested, and replace them with the extended
versions, so the amount of code will not change that much.

Thanks,

-- 
Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Miroslav Lichvar]

On Tue, Oct 30, 2018 at 07:29:16PM -0700, Richard Cochran wrote:
> On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> > +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> > +			    struct ptp_system_timestamp *sts)
> > +{
> > +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> > +					       ptp_caps);
> > +	struct e1000_hw *hw = &igb->hw;
> > +	unsigned long flags;
> > +	u32 lo, hi;
> > +	u64 ns;
> > +
> > +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> > +
> > +	/* 82576 doesn't have SYSTIMR */
> > +	if (igb->hw.mac.type == e1000_82576) {
> 
> Instead of if/then/else, can't you follow the pattern of providing
> different function flavors ...

I can. I was just trying to minimize the amount of triplicated code.
In the next version I'll add a patch to deprecate the old gettime
functions, as Jacob suggested, and replace them with the extended
versions, so the amount of code will not change that much.

Thanks,

-- 
Miroslav Lichvar

RE: [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Wednesday, October 31, 2018 2:40 AM
> To: Richard Cochran <richardcochran@gmail.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH 3/4] igb: add support for extended PHC gettime
> 
> On Tue, Oct 30, 2018 at 07:29:16PM -0700, Richard Cochran wrote:
> > On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> > > +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> > > +			    struct ptp_system_timestamp *sts)
> > > +{
> > > +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> > > +					       ptp_caps);
> > > +	struct e1000_hw *hw = &igb->hw;
> > > +	unsigned long flags;
> > > +	u32 lo, hi;
> > > +	u64 ns;
> > > +
> > > +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> > > +
> > > +	/* 82576 doesn't have SYSTIMR */
> > > +	if (igb->hw.mac.type == e1000_82576) {
> >
> > Instead of if/then/else, can't you follow the pattern of providing
> > different function flavors ...
> 
> I can. I was just trying to minimize the amount of triplicated code.
> In the next version I'll add a patch to deprecate the old gettime
> functions, as Jacob suggested, and replace them with the extended
> versions, so the amount of code will not change that much.
> 

Excellent.

-Jake

> Thanks,
> 
> --
> Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 3/4] igb: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> Sent: Wednesday, October 31, 2018 2:40 AM
> To: Richard Cochran <richardcochran@gmail.com>
> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: Re: [RFC PATCH 3/4] igb: add support for extended PHC gettime
> 
> On Tue, Oct 30, 2018 at 07:29:16PM -0700, Richard Cochran wrote:
> > On Fri, Oct 26, 2018 at 06:27:41PM +0200, Miroslav Lichvar wrote:
> > > +static int igb_ptp_gettimex(struct ptp_clock_info *ptp,
> > > +			    struct ptp_system_timestamp *sts)
> > > +{
> > > +	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
> > > +					       ptp_caps);
> > > +	struct e1000_hw *hw = &igb->hw;
> > > +	unsigned long flags;
> > > +	u32 lo, hi;
> > > +	u64 ns;
> > > +
> > > +	spin_lock_irqsave(&igb->tmreg_lock, flags);
> > > +
> > > +	/* 82576 doesn't have SYSTIMR */
> > > +	if (igb->hw.mac.type == e1000_82576) {
> >
> > Instead of if/then/else, can't you follow the pattern of providing
> > different function flavors ...
> 
> I can. I was just trying to minimize the amount of triplicated code.
> In the next version I'll add a patch to deprecate the old gettime
> functions, as Jacob suggested, and replace them with the extended
> versions, so the amount of code will not change that much.
> 

Excellent.

-Jake

> Thanks,
> 
> --
> Miroslav Lichvar

[RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index b3e0d8bb5cbd..d31e8d3effc7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return 0;
 }
 
+/**
+ * ixgbe_ptp_gettimex
+ * @ptp: the ptp clock structure
+ * @sts: structure to hold the system time before reading the PHC,
+ * the PHC timestamp, and system time after reading the PHC
+ *
+ * read the timecounter and return the correct value on ns,
+ * after converting it into a struct timespec.
+ */
+static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
+			      struct ptp_system_timestamp *sts)
+{
+	struct ixgbe_adapter *adapter =
+		container_of(ptp, struct ixgbe_adapter, ptp_caps);
+	struct ixgbe_hw *hw = &adapter->hw;
+	unsigned long flags;
+	struct timespec64 ts;
+	u64 ns, stamp;
+
+	spin_lock_irqsave(&adapter->tmreg_lock, flags);
+
+	switch (adapter->hw.mac.type) {
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+	case ixgbe_mac_x550em_a:
+		/* Upper 32 bits represent billions of cycles, lower 32 bits
+		 * represent cycles. However, we use timespec64_to_ns for the
+		 * correct math even though the units haven't been corrected
+		 * yet.
+		 */
+		ptp_read_system_prets(sts);
+		IXGBE_READ_REG(hw, IXGBE_SYSTIMR);
+		ptp_read_system_postts(sts);
+		ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
+		ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH);
+		stamp = timespec64_to_ns(&ts);
+		break;
+	default:
+		ptp_read_system_prets(sts);
+		stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
+		ptp_read_system_postts(sts);
+		stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32;
+		break;
+	}
+
+	ns = timecounter_cyc2time(&adapter->hw_tc, stamp);
+
+	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+
+	sts->phc_ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
 /**
  * ixgbe_ptp_settime
  * @ptp: the ptp clock structure
@@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540;
@@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		break;
@@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		adapter->ptp_setup_sdp = NULL;
-- 
2.17.2

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index b3e0d8bb5cbd..d31e8d3effc7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return 0;
 }
 
+/**
+ * ixgbe_ptp_gettimex
+ * @ptp: the ptp clock structure
+ * @sts: structure to hold the system time before reading the PHC,
+ * the PHC timestamp, and system time after reading the PHC
+ *
+ * read the timecounter and return the correct value on ns,
+ * after converting it into a struct timespec.
+ */
+static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
+			      struct ptp_system_timestamp *sts)
+{
+	struct ixgbe_adapter *adapter =
+		container_of(ptp, struct ixgbe_adapter, ptp_caps);
+	struct ixgbe_hw *hw = &adapter->hw;
+	unsigned long flags;
+	struct timespec64 ts;
+	u64 ns, stamp;
+
+	spin_lock_irqsave(&adapter->tmreg_lock, flags);
+
+	switch (adapter->hw.mac.type) {
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+	case ixgbe_mac_x550em_a:
+		/* Upper 32 bits represent billions of cycles, lower 32 bits
+		 * represent cycles. However, we use timespec64_to_ns for the
+		 * correct math even though the units haven't been corrected
+		 * yet.
+		 */
+		ptp_read_system_prets(sts);
+		IXGBE_READ_REG(hw, IXGBE_SYSTIMR);
+		ptp_read_system_postts(sts);
+		ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
+		ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH);
+		stamp = timespec64_to_ns(&ts);
+		break;
+	default:
+		ptp_read_system_prets(sts);
+		stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
+		ptp_read_system_postts(sts);
+		stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32;
+		break;
+	}
+
+	ns = timecounter_cyc2time(&adapter->hw_tc, stamp);
+
+	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+
+	sts->phc_ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
 /**
  * ixgbe_ptp_settime
  * @ptp: the ptp clock structure
@@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540;
@@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		break;
@@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550;
 		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
 		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
+		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
 		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
 		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
 		adapter->ptp_setup_sdp = NULL;
-- 
2.17.2

RE: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> index b3e0d8bb5cbd..d31e8d3effc7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> @@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp,
> struct timespec64 *ts)
>  	return 0;
>  }
> 
> +/**
> + * ixgbe_ptp_gettimex
> + * @ptp: the ptp clock structure
> + * @sts: structure to hold the system time before reading the PHC,
> + * the PHC timestamp, and system time after reading the PHC
> + *
> + * read the timecounter and return the correct value on ns,
> + * after converting it into a struct timespec.
> + */
> +static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
> +			      struct ptp_system_timestamp *sts)
> +{
> +	struct ixgbe_adapter *adapter =
> +		container_of(ptp, struct ixgbe_adapter, ptp_caps);
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	unsigned long flags;
> +	struct timespec64 ts;
> +	u64 ns, stamp;
> +
> +	spin_lock_irqsave(&adapter->tmreg_lock, flags);
> +
> +	switch (adapter->hw.mac.type) {
> +	case ixgbe_mac_X550:
> +	case ixgbe_mac_X550EM_x:
> +	case ixgbe_mac_x550em_a:
> +		/* Upper 32 bits represent billions of cycles, lower 32 bits
> +		 * represent cycles. However, we use timespec64_to_ns for the
> +		 * correct math even though the units haven't been corrected
> +		 * yet.
> +		 */
> +		ptp_read_system_prets(sts);
> +		IXGBE_READ_REG(hw, IXGBE_SYSTIMR);
> +		ptp_read_system_postts(sts);
> +		ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
> +		ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH);
> +		stamp = timespec64_to_ns(&ts);
> +		break;
> +	default:
> +		ptp_read_system_prets(sts);
> +		stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
> +		ptp_read_system_postts(sts);
> +		stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32;
> +		break;
> +	}
> +
> +	ns = timecounter_cyc2time(&adapter->hw_tc, stamp);
> +
> +	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
> +
> +	sts->phc_ts = ns_to_timespec64(ns);
> +
> +	return 0;
> +}
> +


What about replacing gettime64 with:

static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
{
    struct ptp_system_timestamp sts
    
    ixgbe_ptp_gettimex(ptp, &tst);
    *ts = sts.phc_ts
}

Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call?

Thanks,
Jake

>  /**
>   * ixgbe_ptp_settime
>   * @ptp: the ptp clock structure
> @@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540;
> @@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		break;
> @@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		adapter->ptp_setup_sdp = NULL;
> --
> 2.17.2

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev at vger.kernel.org
> Cc: intel-wired-lan at lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> index b3e0d8bb5cbd..d31e8d3effc7 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> @@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp,
> struct timespec64 *ts)
>  	return 0;
>  }
> 
> +/**
> + * ixgbe_ptp_gettimex
> + * @ptp: the ptp clock structure
> + * @sts: structure to hold the system time before reading the PHC,
> + * the PHC timestamp, and system time after reading the PHC
> + *
> + * read the timecounter and return the correct value on ns,
> + * after converting it into a struct timespec.
> + */
> +static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
> +			      struct ptp_system_timestamp *sts)
> +{
> +	struct ixgbe_adapter *adapter =
> +		container_of(ptp, struct ixgbe_adapter, ptp_caps);
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	unsigned long flags;
> +	struct timespec64 ts;
> +	u64 ns, stamp;
> +
> +	spin_lock_irqsave(&adapter->tmreg_lock, flags);
> +
> +	switch (adapter->hw.mac.type) {
> +	case ixgbe_mac_X550:
> +	case ixgbe_mac_X550EM_x:
> +	case ixgbe_mac_x550em_a:
> +		/* Upper 32 bits represent billions of cycles, lower 32 bits
> +		 * represent cycles. However, we use timespec64_to_ns for the
> +		 * correct math even though the units haven't been corrected
> +		 * yet.
> +		 */
> +		ptp_read_system_prets(sts);
> +		IXGBE_READ_REG(hw, IXGBE_SYSTIMR);
> +		ptp_read_system_postts(sts);
> +		ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
> +		ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH);
> +		stamp = timespec64_to_ns(&ts);
> +		break;
> +	default:
> +		ptp_read_system_prets(sts);
> +		stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML);
> +		ptp_read_system_postts(sts);
> +		stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32;
> +		break;
> +	}
> +
> +	ns = timecounter_cyc2time(&adapter->hw_tc, stamp);
> +
> +	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
> +
> +	sts->phc_ts = ns_to_timespec64(ns);
> +
> +	return 0;
> +}
> +


What about replacing gettime64 with:

static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
{
    struct ptp_system_timestamp sts
    
    ixgbe_ptp_gettimex(ptp, &tst);
    *ts = sts.phc_ts
}

Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call?

Thanks,
Jake

>  /**
>   * ixgbe_ptp_settime
>   * @ptp: the ptp clock structure
> @@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540;
> @@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		break;
> @@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter
> *adapter)
>  		adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550;
>  		adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime;
>  		adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime;
> +		adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex;
>  		adapter->ptp_caps.settime64 = ixgbe_ptp_settime;
>  		adapter->ptp_caps.enable = ixgbe_ptp_feature_enable;
>  		adapter->ptp_setup_sdp = NULL;
> --
> 2.17.2

Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> > Sent: Friday, October 26, 2018 9:28 AM
> > To: netdev@vger.kernel.org
> > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> > 
> > Cc: Richard Cochran <richardcochran@gmail.com>
> > Cc: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>

> What about replacing gettime64 with:
> 
> static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
> {
>     struct ptp_system_timestamp sts
>     
>     ixgbe_ptp_gettimex(ptp, &tst);
>     *ts = sts.phc_ts
> }

That will work, but it will be slower. With HPET as a clocksource
there would be few microseconds of an extra (symmetric) delay and the
applications would have to assume a larger maximum error.

I think there could be a flag in ptp_system_timestamp, or a parameter
of gettimex64(), which would enable/disable reading of the system
clock.

> Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call?

Good idea.

Thanks,

-- 
Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> > Sent: Friday, October 26, 2018 9:28 AM
> > To: netdev at vger.kernel.org
> > Cc: intel-wired-lan at lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> > 
> > Cc: Richard Cochran <richardcochran@gmail.com>
> > Cc: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>

> What about replacing gettime64 with:
> 
> static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
> {
>     struct ptp_system_timestamp sts
>     
>     ixgbe_ptp_gettimex(ptp, &tst);
>     *ts = sts.phc_ts
> }

That will work, but it will be slower. With HPET as a clocksource
there would be few microseconds of an extra (symmetric) delay and the
applications would have to assume a larger maximum error.

I think there could be a flag in ptp_system_timestamp, or a parameter
of gettimex64(), which would enable/disable reading of the system
clock.

> Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call?

Good idea.

Thanks,

-- 
Miroslav Lichvar

RE: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Miroslav Lichvar
> Sent: Monday, October 29, 2018 6:31 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> > > Sent: Friday, October 26, 2018 9:28 AM
> > > To: netdev@vger.kernel.org
> > > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>;
> > > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar
> <mlichvar@redhat.com>
> > > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> > >
> > > Cc: Richard Cochran <richardcochran@gmail.com>
> > > Cc: Jacob Keller <jacob.e.keller@intel.com>
> > > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> 
> > What about replacing gettime64 with:
> >
> > static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
> > {
> >     struct ptp_system_timestamp sts
> >
> >     ixgbe_ptp_gettimex(ptp, &tst);
> >     *ts = sts.phc_ts
> > }
> 
> That will work, but it will be slower. With HPET as a clocksource
> there would be few microseconds of an extra (symmetric) delay and the
> applications would have to assume a larger maximum error.
> 

Right. My intention for this would be that we'd switch from gettime64 to gettime64x going forward, and provide this as a way to avoid having to duplicate logic in drivers while we're transitioning? Thus, new applications should be using the new call if it's available in the driver.

Hmm.
 
> I think there could be a flag in ptp_system_timestamp, or a parameter
> of gettimex64(), which would enable/disable reading of the system
> clock.
> 
> > Actually, could that even just be provided by the PTP core if gettime64 isn't
> implemented? This way new drivers only have to implement the new interface, and
> userspace will just get the old behavior if they use the old call?
> 
> Good idea.

Ideally we can find a way that minimizes the overhead for the old call.

Thanks,
Jake

> 
> Thanks,
> 
> --
> Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On
> Behalf Of Miroslav Lichvar
> Sent: Monday, October 29, 2018 6:31 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> > > Sent: Friday, October 26, 2018 9:28 AM
> > > To: netdev at vger.kernel.org
> > > Cc: intel-wired-lan at lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>;
> > > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar
> <mlichvar@redhat.com>
> > > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> > >
> > > Cc: Richard Cochran <richardcochran@gmail.com>
> > > Cc: Jacob Keller <jacob.e.keller@intel.com>
> > > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> 
> > What about replacing gettime64 with:
> >
> > static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts)
> > {
> >     struct ptp_system_timestamp sts
> >
> >     ixgbe_ptp_gettimex(ptp, &tst);
> >     *ts = sts.phc_ts
> > }
> 
> That will work, but it will be slower. With HPET as a clocksource
> there would be few microseconds of an extra (symmetric) delay and the
> applications would have to assume a larger maximum error.
> 

Right. My intention for this would be that we'd switch from gettime64 to gettime64x going forward, and provide this as a way to avoid having to duplicate logic in drivers while we're transitioning? Thus, new applications should be using the new call if it's available in the driver.

Hmm.
 
> I think there could be a flag in ptp_system_timestamp, or a parameter
> of gettimex64(), which would enable/disable reading of the system
> clock.
> 
> > Actually, could that even just be provided by the PTP core if gettime64 isn't
> implemented? This way new drivers only have to implement the new interface, and
> userspace will just get the old behavior if they use the old call?
> 
> Good idea.

Ideally we can find a way that minimizes the overhead for the old call.

Thanks,
Jake

> 
> Thanks,
> 
> --
> Miroslav Lichvar

Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Richard Cochran]

On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> I think there could be a flag in ptp_system_timestamp, or a parameter
> of gettimex64(), which would enable/disable reading of the system
> clock.

I'm not a fan of functions that change their behavior based on flags
in their input parameters.

Thanks,
Richard

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Richard Cochran]

On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> I think there could be a flag in ptp_system_timestamp, or a parameter
> of gettimex64(), which would enable/disable reading of the system
> clock.

I'm not a fan of functions that change their behavior based on flags
in their input parameters.

Thanks,
Richard

Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

On Wed, Oct 31, 2018 at 07:40:03AM -0700, Richard Cochran wrote:
> On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> > I think there could be a flag in ptp_system_timestamp, or a parameter
> > of gettimex64(), which would enable/disable reading of the system
> > clock.
> 
> I'm not a fan of functions that change their behavior based on flags
> in their input parameters.

How about separating the PHC timestamp from the ptp_system_timestamp
structure and use NULL to indicate we don't want to read the system
clock? A gettimex64(ptp, ts, NULL) call would be equal to
gettime64(ptp, ts).

struct ptp_system_timestamp {
	struct timespec64 pre_ts;
	struct timespec64 post_ts;
};

int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
		  struct ptp_system_timestamp *sts);

-- 
Miroslav Lichvar

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Miroslav Lichvar]

On Wed, Oct 31, 2018 at 07:40:03AM -0700, Richard Cochran wrote:
> On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> > I think there could be a flag in ptp_system_timestamp, or a parameter
> > of gettimex64(), which would enable/disable reading of the system
> > clock.
> 
> I'm not a fan of functions that change their behavior based on flags
> in their input parameters.

How about separating the PHC timestamp from the ptp_system_timestamp
structure and use NULL to indicate we don't want to read the system
clock? A gettimex64(ptp, ts, NULL) call would be equal to
gettime64(ptp, ts).

struct ptp_system_timestamp {
	struct timespec64 pre_ts;
	struct timespec64 post_ts;
};

int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
		  struct ptp_system_timestamp *sts);

-- 
Miroslav Lichvar

Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Richard Cochran]

On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote:
> 
> How about separating the PHC timestamp from the ptp_system_timestamp
> structure and use NULL to indicate we don't want to read the system
> clock? A gettimex64(ptp, ts, NULL) call would be equal to
> gettime64(ptp, ts).

Doesn't sound too bad to me.

Thanks,
Richard

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Richard Cochran]

On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote:
> 
> How about separating the PHC timestamp from the ptp_system_timestamp
> structure and use NULL to indicate we don't want to read the system
> clock? A gettimex64(ptp, ts, NULL) call would be equal to
> gettime64(ptp, ts).

Doesn't sound too bad to me.

Thanks,
Richard

RE: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, October 31, 2018 2:17 PM
> To: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote:
> >
> > How about separating the PHC timestamp from the ptp_system_timestamp
> > structure and use NULL to indicate we don't want to read the system
> > clock? A gettimex64(ptp, ts, NULL) call would be equal to
> > gettime64(ptp, ts).
> 
> Doesn't sound too bad to me.
> 
> Thanks,
> Richard

Yep, this seems fine to me as well.

Regards,
Jake

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran at gmail.com]
> Sent: Wednesday, October 31, 2018 2:17 PM
> To: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev at vger.kernel.org; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote:
> >
> > How about separating the PHC timestamp from the ptp_system_timestamp
> > structure and use NULL to indicate we don't want to read the system
> > clock? A gettimex64(ptp, ts, NULL) call would be equal to
> > gettime64(ptp, ts).
> 
> Doesn't sound too bad to me.
> 
> Thanks,
> Richard

Yep, this seems fine to me as well.

Regards,
Jake

RE: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, October 31, 2018 7:40 AM
> To: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> > I think there could be a flag in ptp_system_timestamp, or a parameter
> > of gettimex64(), which would enable/disable reading of the system
> > clock.
> 
> I'm not a fan of functions that change their behavior based on flags
> in their input parameters.
> 
> Thanks,
> Richard

Neither am I. I do however want to find a solution that avoids having drivers needlessly duplicate almost the same functionality.

Thanks,
Jake

[Intel-wired-lan] [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime

[Keller, Jacob E]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran at gmail.com]
> Sent: Wednesday, October 31, 2018 7:40 AM
> To: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev at vger.kernel.org; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime
> 
> On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote:
> > I think there could be a flag in ptp_system_timestamp, or a parameter
> > of gettimex64(), which would enable/disable reading of the system
> > clock.
> 
> I'm not a fan of functions that change their behavior based on flags
> in their input parameters.
> 
> Thanks,
> Richard

Neither am I. I do however want to find a solution that avoids having drivers needlessly duplicate almost the same functionality.

Thanks,
Jake

RE: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Keller, Jacob E]

Hi Miroslav,

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization
> 
> This series adds support for a more accurate synchronization between a
> PTP hardware clock and the system clock.
> 
> The first patch adds an extended version of the PTP_SYS_OFFSET ioctl,
> which returns three timestamps for each measurement. The idea is to
> shorten the interval between the system timestamps to contain just the
> reading of the lowest register of the PHC in order to reduce the error
> in the measured offset and give a better bound on the maximum error.
> 
> The other patches add support for the new ioctl to the e1000e, igb,
> and ixgbe driver. Tests with few different NICs in different machines
> (and PCIe slots) show that:
> - with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
>   and the error in the measured offset, when compared to cross
>   timestamping, was reduced by a factor of 5
> - with an I210 (igb) the delay improved from 5100 to 1700 ns
> - with an I350 (igb) the delay improved from 2300 to 750 ns
> - with an X550 (ixgbe) the delay improved from 1950 to 650 ns
> 

That is some very significant improvements! Excellent find.

> There is some duplication of code in the igb and ixgbe drivers, which I
> don't like very much, but I thought it's better than extending and
> wrapping the existing functions like in the e1000e driver. Also, mixing
> SYSTIM and "system time" in the code will probably be confusing.
> 

Yea...

> I wasn't able to find a better name for the ioctl, the structures, and
> the driver function. If anyone has suggestions, please let me know.
> 

I don't have any good suggestions yet. I'll reply after reviewing if I think of any.

Thanks,
Jake

[Intel-wired-lan] [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Keller, Jacob E]

Hi Miroslav,

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev at vger.kernel.org
> Cc: intel-wired-lan at lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization
> 
> This series adds support for a more accurate synchronization between a
> PTP hardware clock and the system clock.
> 
> The first patch adds an extended version of the PTP_SYS_OFFSET ioctl,
> which returns three timestamps for each measurement. The idea is to
> shorten the interval between the system timestamps to contain just the
> reading of the lowest register of the PHC in order to reduce the error
> in the measured offset and give a better bound on the maximum error.
> 
> The other patches add support for the new ioctl to the e1000e, igb,
> and ixgbe driver. Tests with few different NICs in different machines
> (and PCIe slots) show that:
> - with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
>   and the error in the measured offset, when compared to cross
>   timestamping, was reduced by a factor of 5
> - with an I210 (igb) the delay improved from 5100 to 1700 ns
> - with an I350 (igb) the delay improved from 2300 to 750 ns
> - with an X550 (ixgbe) the delay improved from 1950 to 650 ns
> 

That is some very significant improvements! Excellent find.

> There is some duplication of code in the igb and ixgbe drivers, which I
> don't like very much, but I thought it's better than extending and
> wrapping the existing functions like in the e1000e driver. Also, mixing
> SYSTIM and "system time" in the code will probably be confusing.
> 

Yea...

> I wasn't able to find a better name for the ioctl, the structures, and
> the driver function. If anyone has suggestions, please let me know.
> 

I don't have any good suggestions yet. I'll reply after reviewing if I think of any.

Thanks,
Jake

RE: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization
> 

I read the whole series, and it looks good to me.

Acked-by: Jacob Keller <jacob.e.keller@intel.com>

> There is some duplication of code in the igb and ixgbe drivers, which I
> don't like very much, but I thought it's better than extending and
> wrapping the existing functions like in the e1000e driver. Also, mixing
> SYSTIM and "system time" in the code will probably be confusing.
> 
> I wasn't able to find a better name for the ioctl, the structures, and
> the driver function. If anyone has suggestions, please let me know.
> 

Hmm.. Yea, I don't really have better names either.

> Miroslav Lichvar (4):
>   ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
>   e1000e: add support for extended PHC gettime
>   igb: add support for extended PHC gettime
>   ixgbe: add support for extended PHC gettime
> 
>  drivers/net/ethernet/intel/e1000e/e1000.h    |  3 ++
>  drivers/net/ethernet/intel/e1000e/netdev.c   | 48 +++++++++++++----
>  drivers/net/ethernet/intel/e1000e/ptp.c      | 21 ++++++++
>  drivers/net/ethernet/intel/igb/igb_ptp.c     | 43 +++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
>  drivers/ptp/ptp_chardev.c                    | 39 ++++++++++++++
>  include/linux/ptp_clock_kernel.h             | 26 +++++++++
>  include/uapi/linux/ptp_clock.h               | 12 +++++
>  8 files changed, 239 insertions(+), 10 deletions(-)
> 
> --
> 2.17.2

[Intel-wired-lan] [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar at redhat.com]
> Sent: Friday, October 26, 2018 9:28 AM
> To: netdev at vger.kernel.org
> Cc: intel-wired-lan at lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com>
> Subject: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization
> 

I read the whole series, and it looks good to me.

Acked-by: Jacob Keller <jacob.e.keller@intel.com>

> There is some duplication of code in the igb and ixgbe drivers, which I
> don't like very much, but I thought it's better than extending and
> wrapping the existing functions like in the e1000e driver. Also, mixing
> SYSTIM and "system time" in the code will probably be confusing.
> 
> I wasn't able to find a better name for the ioctl, the structures, and
> the driver function. If anyone has suggestions, please let me know.
> 

Hmm.. Yea, I don't really have better names either.

> Miroslav Lichvar (4):
>   ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
>   e1000e: add support for extended PHC gettime
>   igb: add support for extended PHC gettime
>   ixgbe: add support for extended PHC gettime
> 
>  drivers/net/ethernet/intel/e1000e/e1000.h    |  3 ++
>  drivers/net/ethernet/intel/e1000e/netdev.c   | 48 +++++++++++++----
>  drivers/net/ethernet/intel/e1000e/ptp.c      | 21 ++++++++
>  drivers/net/ethernet/intel/igb/igb_ptp.c     | 43 +++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++
>  drivers/ptp/ptp_chardev.c                    | 39 ++++++++++++++
>  include/linux/ptp_clock_kernel.h             | 26 +++++++++
>  include/uapi/linux/ptp_clock.h               | 12 +++++
>  8 files changed, 239 insertions(+), 10 deletions(-)
> 
> --
> 2.17.2

Re: [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:38PM +0200, Miroslav Lichvar wrote:
> The other patches add support for the new ioctl to the e1000e, igb,
> and ixgbe driver. Tests with few different NICs in different machines
> (and PCIe slots) show that:
> - with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
>   and the error in the measured offset, when compared to cross
>   timestamping, was reduced by a factor of 5
> - with an I210 (igb) the delay improved from 5100 to 1700 ns
> - with an I350 (igb) the delay improved from 2300 to 750 ns
> - with an X550 (ixgbe) the delay improved from 1950 to 650 ns

Nice work!  This is a welcome improvement.

Thanks,
Richard

[Intel-wired-lan] [RFC PATCH 0/4] More accurate PHC<->system clock synchronization

[Richard Cochran]

On Fri, Oct 26, 2018 at 06:27:38PM +0200, Miroslav Lichvar wrote:
> The other patches add support for the new ioctl to the e1000e, igb,
> and ixgbe driver. Tests with few different NICs in different machines
> (and PCIe slots) show that:
> - with an I219 (e1000e) the measured delay improved from 2500 to 1300 ns
>   and the error in the measured offset, when compared to cross
>   timestamping, was reduced by a factor of 5
> - with an I210 (igb) the delay improved from 5100 to 1700 ns
> - with an I350 (igb) the delay improved from 2300 to 750 ns
> - with an X550 (ixgbe) the delay improved from 1950 to 650 ns

Nice work!  This is a welcome improvement.

Thanks,
Richard