Skip to contentSkip to conversation

More accurate PHC<->system clock synchronization

PATCH In review Miroslav Lichvar 6 participants 28 messages Nov 9, 2018 – Jan 8, 2019

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

cover More accurate PHC<->system clock synchronization

RFC->v1:
- added new patches
- separated PHC timestamp from ptp_system_timestamp
- fixed memory leak in PTP_SYS_OFFSET_EXTENDED
- changed PTP_SYS_OFFSET_EXTENDED to work with array of arrays
- fixed PTP_SYS_OFFSET_EXTENDED to break correctly from loop
- fixed timecounter updates in drivers
- split gettimex in igb driver
- fixed ptp_read_* functions to be available without CONFIG_PTP_1588_CLOCK

This series enables a more accurate synchronization between PTP hardware clocks and the system clock.

The first two patches are minor cleanup/bug fixes.

The third 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 get a smaller upper bound on the maximum error.

The fourth patch deprecates the original gettime function.

The remaining patches update the gettime function in order to support the new ioctl in the e1000e, igb, ixgbe, and tg3 drivers.

Tests with few different NICs in different machines show that:
- with an I219 (e1000e) the measured delay was reduced from 2500 to 1300 ns and the error in the measured offset, when compared to the cross timestamping supported by the driver, was reduced by a factor of 5
- with an I210 (igb) the delay was reduced from 5100 to 1700 ns
- with an I350 (igb) the delay was reduced from 2300 to 750 ns
- with an X550 (ixgbe) the delay was reduced from 1950 to 650 ns
- with a BCM5720 (tg3) the delay was reduced from 2400 to 1200 ns

Miroslav Lichvar (8):
ptp: reorder declarations in ptp_ioctl()
ptp: check gettime64 return code in PTP_SYS_OFFSET ioctl
ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
ptp: deprecate gettime64() in favor of gettimex64()
e1000e: extend PTP gettime function to read system clock
igb: extend PTP gettime function to read system clock
ixgbe: extend PTP gettime function to read system clock
tg3: extend PTP gettime function to read system clock

 drivers/net/ethernet/broadcom/tg3.c          | 19 ++++--
 drivers/net/ethernet/intel/e1000e/e1000.h    |  3 +
 drivers/net/ethernet/intel/e1000e/netdev.c   | 42 ++++++++++---
 drivers/net/ethernet/intel/e1000e/ptp.c      | 16 +++--
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 65 +++++++++++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 54 +++++++++++++---
 drivers/ptp/ptp_chardev.c                    | 55 ++++++++++++++---
 drivers/ptp/ptp_clock.c                      |  5 +-
 include/linux/ptp_clock_kernel.h             | 33 ++++++++++
 include/uapi/linux/ptp_clock.h               | 12 ++++
 10 files changed, 253 insertions(+), 51 deletions(-)
Signature · 1 line
2.17.2
#1 lore raw

PATCH 1/8 ptp: reorder declarations in ptp_ioctl()

Reorder declarations of variables as reversed Christmas tree.

---
 drivers/ptp/ptp_chardev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/ptp/ptp_chardev.c +7 -7
index 2012551d93e0..b54b8158ff8a 100644
@@ -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) {
 
Signature · 1 line
2.17.2
#2 lore raw

PATCH 2/8 ptp: check gettime64 return code in PTP_SYS_OFFSET ioctl

In reply to Miroslav Lichvar

If a gettime64 call fails, return the error and avoid copying data back to user.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
---
 drivers/ptp/ptp_chardev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/ptp/ptp_chardev.c +4 -1
index b54b8158ff8a..3c681bed5703 100644
@@ -228,7 +228,9 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;
 			pct++;
-			ptp->info->gettime64(ptp->info, &ts);
+			err = ptp->info->gettime64(ptp->info, &ts);
+			if (err)
+				goto out;
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;
 			pct++;
@@ -281,6 +283,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 	}
 
+out:
 	kfree(sysoff);
 	return err;
 }
Signature · 1 line
2.17.2
#3 lore raw

PATCH 3/8 ptp: add PTP_SYS_OFFSET_EXTENDED ioctl

In reply to 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 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 returns 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 <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Marcelo Tosatti <[email protected]>
---
 drivers/ptp/ptp_chardev.c        | 33 ++++++++++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h | 31 ++++++++++++++++++++++++++++++
 include/uapi/linux/ptp_clock.h   | 12 ++++++++++++
 3 files changed, 76 insertions(+)
drivers/ptp/ptp_chardev.c +33 -0
index 3c681bed5703..aad0d36cf5c0 100644
@@ -122,10 +122,12 @@ 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 *ptp = container_of(pc, struct ptp_clock, clock);
+	struct ptp_sys_offset_extended *extoff = NULL;
 	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_system_timestamp sts;
 	struct ptp_clock_request req;
 	struct ptp_clock_caps caps;
 	struct ptp_clock_time *pct;
@@ -211,6 +213,36 @@ 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;
+		}
+		extoff = memdup_user((void __user *)arg, sizeof(*extoff));
+		if (IS_ERR(extoff)) {
+			err = PTR_ERR(extoff);
+			extoff = NULL;
+			break;
+		}
+		if (extoff->n_samples > PTP_MAX_SAMPLES) {
+			err = -EINVAL;
+			break;
+		}
+		for (i = 0; i < extoff->n_samples; i++) {
+			err = ptp->info->gettimex64(ptp->info, &ts, &sts);
+			if (err)
+				goto out;
+			extoff->ts[i][0].sec = sts.pre_ts.tv_sec;
+			extoff->ts[i][0].nsec = sts.pre_ts.tv_nsec;
+			extoff->ts[i][1].sec = ts.tv_sec;
+			extoff->ts[i][1].nsec = ts.tv_nsec;
+			extoff->ts[i][2].sec = sts.post_ts.tv_sec;
+			extoff->ts[i][2].nsec = sts.post_ts.tv_nsec;
+		}
+		if (copy_to_user((void __user *)arg, extoff, sizeof(*extoff)))
+			err = -EFAULT;
+		break;
+
 	case PTP_SYS_OFFSET:
 		sysoff = memdup_user((void __user *)arg, sizeof(*sysoff));
 		if (IS_ERR(sysoff)) {
@@ -284,6 +316,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	}
 
 out:
+	kfree(extoff);
 	kfree(sysoff);
 	return err;
 }
include/linux/ptp_clock_kernel.h +31 -0
index 51349d124ee5..a1ec0448e341 100644
@@ -39,6 +39,15 @@ struct ptp_clock_request {
 };
 
 struct system_device_crosststamp;
+
+/**
+ * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
+ */
+struct ptp_system_timestamp {
+	struct timespec64 pre_ts;
+	struct timespec64 post_ts;
+};
+
 /**
  * struct ptp_clock_info - decribes a PTP hardware clock
  *
@@ -75,6 +84,14 @@ 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 hardware clock and optionally
+ *               also the system clock.
+ *               parameter ts: Holds the PHC timestamp.
+ *               parameter sts: If not NULL, it holds a pair of timestamps from
+ *               the system clock. The first reading is made right before
+ *               reading the lowest bits of the PHC timestamp and the second
+ *               reading immediately follows that.
+ *
  * @getcrosststamp:  Reads the current time from the hardware clock and
  *                   system clock simultaneously.
  *                   parameter cts: Contains timestamp (device,system) pair,
@@ -124,6 +141,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 timespec64 *ts,
+			  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);
@@ -247,4 +266,16 @@ static inline int ptp_schedule_worker(struct ptp_clock *ptp,
 
 #endif
 
+static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
+{
+	if (sts)
+		ktime_get_real_ts64(&sts->pre_ts);
+}
+
+static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
+{
+	if (sts)
+		ktime_get_real_ts64(&sts->post_ts);
+}
+
 #endif
include/uapi/linux/ptp_clock.h +12 -0
index 3039bf6a742e..d73d83950265 100644
@@ -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 [system, phc, system] time stamps. The kernel will provide
+	 * 3*n_samples time stamps.
+	 */
+	struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
+};
+
 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. */
Signature · 1 line
2.17.2
#4 lore raw

PATCH 1/2 ptp: check that rsv field is zero in struct ptp_sys_offset_extended

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

---
 drivers/ptp/ptp_chardev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/ptp/ptp_chardev.c +2 -1
index 797fab3..7cbea79 100644
@@ -224,7 +224,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			extoff = NULL;
 			break;
 		}
-		if (extoff->n_samples > PTP_MAX_SAMPLES) {
+		if (extoff->n_samples > PTP_MAX_SAMPLES
+		    || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
 			err = -EINVAL;
 			break;
 		}
Signature · 1 line
2.1.4
maintainer
#5 lore raw

Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 7 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

I think the opportunity to enforce this has passed and you will break userspace by doing this.

#6 lore raw

Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

Quoted 6 lines · David Miller

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 3 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

Quoted 7 lines
Quoted 4 lines

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

I think the opportunity to enforce this has passed and you will break userspace by doing this.

FWIW, this ioctl wasn't present in a stable Linux release yet and the two applications that I know that support it both zero the rsv field, so I think at least this patch is very unlikely to break anything.

Anyway, thanks to Eugene for catching the issues.

Signature · 1 line
Miroslav Lichvar
maintainer
#7 lore raw

Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

From: Miroslav Lichvar <[email protected]>
Date: Mon, 7 Jan 2019 17:57:41 +0100

Quoted 12 lines · David Miller
Quoted 11 lines

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 7 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

Quoted 8 lines
Quoted 2 lines

I think the opportunity to enforce this has passed and you will break userspace by doing this.

FWIW, this ioctl wasn't present in a stable Linux release yet and the two applications that I know that support it both zero the rsv field, so I think at least this patch is very unlikely to break anything.

Anyway, thanks to Eugene for catching the issues.

Oh, this ioctl went into this merge window didn't it.

Ok. I'll keep reviewing this then.

maintainer
#8 lore raw

Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

In reply to David Miller
Quoted 6 lines · David Miller

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 3 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

Quoted 7 lines
Quoted 4 lines

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

I think the opportunity to enforce this has passed and you will break userspace by doing this.

Does this seriously mean that the 'rsv' field in

struct ptp_extts_request {
unsigned int index; /* Which channel to configure. */ unsigned int flags; /* Bit field for PTP_xxx flags. */ unsigned int rsv[2]; /* Reserved for future use. */
};

can never be extended with some semantics?

Thanks,
Richard

#9 lore raw

Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

Quoted 17 lines · Richard Cochran
Quoted 6 lines · David Miller

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 3 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

Quoted 7 lines
Quoted 4 lines

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

I think the opportunity to enforce this has passed and you will break userspace by doing this.

Does this seriously mean that the 'rsv' field in

Quoted 7 lines

struct ptp_extts_request {
unsigned int index; /* Which channel to configure. */ unsigned int flags; /* Bit field for PTP_xxx flags. */ unsigned int rsv[2]; /* Reserved for future use. */
};

can never be extended with some semantics?

Yes[*], since there's no check for garbage in both unused flags bits and rsv values. The same for ptp_perout_request, ptp_sys_offset, ptp_pin_desc in PTP_PIN_SETFUNC, and to some extent for ptp_sys_offset_precise, ptp_clock_caps, and ptp_pin_desc in PTP_PIN_GETFUNC (all newly added data has to be non-zero there). See also [1][2].

It can be worked around by adding new ioctl commands that operate on the same structures, but also perform proper checks, though.

[*] Well, it could be extended with some data that is written from kernel
to user space, but, again, it is not possible due to the fact that no new flags can be added there and it is an _IOW and not _IOWR command.
[1] https://www.kernel.org/doc/Documentation/process/adding-syscalls.rsti
"Designing the API: Planning for Extension"
[2] http://man7.org/conf/lcna2015/designing_linux_kernel_APIs-LCNA_2015-Kerrisk.pdf

Quoted 2 lines

Thanks,
Richard

#10 lore raw

RE: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

Quoted 37 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Eugene Syromiatnikov
Sent: Tuesday, January 08, 2019 6:38 AM
To: Richard Cochran <[email protected]>
Cc: David Miller <[email protected]>; [email protected]; [email protected]; Keller, Jacob E <[email protected]>; [email protected]
Subject: Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended

Quoted 17 lines · Richard Cochran
Quoted 6 lines · David Miller

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:29 +0100

Quoted 3 lines

Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there.

Quoted 7 lines
Quoted 4 lines

The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI.

I think the opportunity to enforce this has passed and you will break userspace by doing this.

Does this seriously mean that the 'rsv' field in

Quoted 7 lines

struct ptp_extts_request {
unsigned int index; /* Which channel to configure. */ unsigned int flags; /* Bit field for PTP_xxx flags. */ unsigned int rsv[2]; /* Reserved for future use. */
};

can never be extended with some semantics?

Quoted 9 lines

Yes[*], since there's no check for garbage in both unused flags bits and rsv values. The same for ptp_perout_request, ptp_sys_offset, ptp_pin_desc in PTP_PIN_SETFUNC, and to some extent for ptp_sys_offset_precise, ptp_clock_caps, and ptp_pin_desc in PTP_PIN_GETFUNC (all newly added data has to be non-zero there). See also [1][2].

It can be worked around by adding new ioctl commands that operate on the same structures, but also perform proper checks, though.

So if/when we want to use these fields, this is what we'd have to do...

Is it worth doing that sooner rather than later? i.e. so that the new ioctl command is available for userspace to update to?

Thanks,
Jake

#11 lore raw

PATCH 2/2 ptp: uapi: change _IOW to IOWR in PTP_SYS_OFFSET_EXTENDED definition

In reply to Miroslav Lichvar

The ioctl command is read/write (or just read, if the fact that user space writes n_samples field is ignored).

---
 include/uapi/linux/ptp_clock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/uapi/linux/ptp_clock.h +1 -1
index d73d839..1bc794a 100644
@@ -147,7 +147,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)
+	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
Signature · 1 line
2.1.4
maintainer
#12 lore raw

Re: [PATCH net 2/2] ptp: uapi: change _IOW to IOWR in PTP_SYS_OFFSET_EXTENDED definition

From: Eugene Syromiatnikov <[email protected]>
Date: Mon, 7 Jan 2019 16:22:38 +0100

include/uapi/linux/ptp_clock.h struct ptp_pin_desc { · line 150

Quoted 8 lines
include/uapi/linux/ptp_clock.h +1 -1
@@ -147,7 +147,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)
+	_IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)

Again, this changes the ioctl cmd value and will break userspace.

#13 lore raw

PATCH 4/8 ptp: deprecate gettime64() in favor of gettimex64()

In reply to Miroslav Lichvar

When a driver provides gettimex64(), use it in the PTP_SYS_OFFSET ioctl and POSIX clock's gettime() instead of gettime64(). Drivers should provide only one of the functions.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
---
 drivers/ptp/ptp_chardev.c        | 5 ++++-
 drivers/ptp/ptp_clock.c          | 5 ++++-
 include/linux/ptp_clock_kernel.h | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)
drivers/ptp/ptp_chardev.c +4 -1
index aad0d36cf5c0..797fab33bb98 100644
@@ -260,7 +260,10 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;
 			pct++;
-			err = ptp->info->gettime64(ptp->info, &ts);
+			if (ops->gettimex64)
+				err = ops->gettimex64(ops, &ts, NULL);
+			else
+				err = ops->gettime64(ops, &ts);
 			if (err)
 				goto out;
 			pct->sec = ts.tv_sec;
drivers/ptp/ptp_clock.c +4 -1
index 5419a89d300e..40fda23e4b05 100644
@@ -117,7 +117,10 @@ static int ptp_clock_gettime(struct posix_clock *pc, struct timespec64 *tp)
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	int err;
 
-	err = ptp->info->gettime64(ptp->info, tp);
+	if (ptp->info->gettimex64)
+		err = ptp->info->gettimex64(ptp->info, tp, NULL);
+	else
+		err = ptp->info->gettime64(ptp->info, tp);
 	return err;
 }
 
include/linux/ptp_clock_kernel.h +2 -0
index a1ec0448e341..7121bbe76979 100644
@@ -82,6 +82,8 @@ struct ptp_system_timestamp {
  *            parameter delta: Desired change in nanoseconds.
  *
  * @gettime64:  Reads the current time from the hardware clock.
+ *              This method is deprecated.  New drivers should implement
+ *              the @gettimex64 method instead.
  *              parameter ts: Holds the result.
  *
  * @gettimex64:  Reads the current time from the hardware clock and optionally
Signature · 1 line
2.17.2
#14 lore raw

PATCH 5/8 e1000e: extend PTP gettime function to read system clock

In reply to Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/e1000.h  |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c | 42 ++++++++++++++++------
 drivers/net/ethernet/intel/e1000e/ptp.c    | 16 +++++----
 3 files changed, 45 insertions(+), 16 deletions(-)
drivers/net/ethernet/intel/e1000e/e1000.h +3 -0
index c760dc72c520..be13227f1697 100644
@@ -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);
drivers/net/ethernet/intel/e1000e/netdev.c +32 -10

89 lines not on this page · raw message

drivers/net/ethernet/intel/e1000e/ptp.c +10 -6
index e1f821edbc21..1a4c65d9feb4 100644
@@ -161,14 +161,18 @@ static int e1000e_phc_getcrosststamp(struct ptp_clock_info *ptp,
 #endif/*CONFIG_E1000E_HWTS*/
 
 /**
- * e1000e_phc_gettime - Reads the current time from the hardware clock
+ * e1000e_phc_gettimex - Reads the current time from the hardware clock and
+ *                       system clock
  * @ptp: ptp clock structure
- * @ts: timespec structure to hold the current time value
+ * @ts: timespec structure to hold the current PHC time
+ * @sts: structure to hold the current system time
  *
  * Read the timecounter and return the correct value in ns after converting
  * it into a struct timespec.
  **/
-static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+static int e1000e_phc_gettimex(struct ptp_clock_info *ptp,
+			       struct timespec64 *ts,
+			       struct ptp_system_timestamp *sts)
 {
 	struct e1000_adapter *adapter = container_of(ptp, struct e1000_adapter,
 						     ptp_clock_info);
@@ -177,8 +181,8 @@ static int e1000e_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 
 	spin_lock_irqsave(&adapter->systim_lock, flags);
 
-	/* Use timecounter_cyc2time() to allow non-monotonic SYSTIM readings */
-	cycles = adapter->cc.read(&adapter->cc);
+	/* NOTE: Non-monotonic SYSTIM readings may be returned */
+	cycles = e1000e_read_systim(adapter, sts);
 	ns = timecounter_cyc2time(&adapter->tc, cycles);
 
 	spin_unlock_irqrestore(&adapter->systim_lock, flags);
@@ -258,7 +262,7 @@ static const struct ptp_clock_info e1000e_ptp_clock_info = {
 	.pps		= 0,
 	.adjfreq	= e1000e_phc_adjfreq,
 	.adjtime	= e1000e_phc_adjtime,
-	.gettime64	= e1000e_phc_gettime,
+	.gettimex64	= e1000e_phc_gettimex,
 	.settime64	= e1000e_phc_settime,
 	.enable		= e1000e_phc_enable,
 };
Signature · 1 line
2.17.2
Acked-by on v1 5/8
#15 lore raw

Re: [PATCH net-next 5/8] e1000e: extend PTP gettime function to read system clock

Quoted 11 lines · Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/e1000e/e1000.h  |  3 ++
 drivers/net/ethernet/intel/e1000e/netdev.c | 42 ++++++++++++++++------
 drivers/net/ethernet/intel/e1000e/ptp.c    | 16 +++++----
 3 files changed, 45 insertions(+), 16 deletions(-)
#16 lore raw

PATCH 6/8 igb: extend PTP gettime function to read system clock

In reply to Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 65 ++++++++++++++++++++----
 1 file changed, 55 insertions(+), 10 deletions(-)
drivers/net/ethernet/intel/igb/igb_ptp.c +55 -10
index 29ced6b74d36..8c1833a157d3 100644
@@ -275,17 +275,53 @@ static int igb_ptp_adjtime_i210(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
-static int igb_ptp_gettime_82576(struct ptp_clock_info *ptp,
-				 struct timespec64 *ts)
+static int igb_ptp_gettimex_82576(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  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);
 
-	ns = timecounter_read(&igb->tc);
+	ptp_read_system_prets(sts);
+	lo = rd32(E1000_SYSTIML);
+	ptp_read_system_postts(sts);
+	hi = rd32(E1000_SYSTIMH);
+
+	ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
+
+	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
+static int igb_ptp_gettimex_82580(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  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);
+
+	ptp_read_system_prets(sts);
+	rd32(E1000_SYSTIMR);
+	ptp_read_system_postts(sts);
+	lo = rd32(E1000_SYSTIML);
+	hi = rd32(E1000_SYSTIMH);
+
+	ns = timecounter_cyc2time(&igb->tc, ((u64)hi << 32) | lo);
 
 	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
 
@@ -294,16 +330,22 @@ static int igb_ptp_gettime_82576(struct ptp_clock_info *ptp,
 	return 0;
 }
 
-static int igb_ptp_gettime_i210(struct ptp_clock_info *ptp,
-				struct timespec64 *ts)
+static int igb_ptp_gettimex_i210(struct ptp_clock_info *ptp,
+				 struct timespec64 *ts,
+				 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;
 
 	spin_lock_irqsave(&igb->tmreg_lock, flags);
 
-	igb_ptp_read_i210(igb, ts);
+	ptp_read_system_prets(sts);
+	rd32(E1000_SYSTIMR);
+	ptp_read_system_postts(sts);
+	ts->tv_nsec = rd32(E1000_SYSTIML);
+	ts->tv_sec = rd32(E1000_SYSTIMH);
 
 	spin_unlock_irqrestore(&igb->tmreg_lock, flags);
 
@@ -656,9 +698,12 @@ static void igb_ptp_overflow_check(struct work_struct *work)
 	struct igb_adapter *igb =
 		container_of(work, struct igb_adapter, ptp_overflow_work.work);
 	struct timespec64 ts;
+	u64 ns;
 
-	igb->ptp_caps.gettime64(&igb->ptp_caps, &ts);
+	/* Update the timecounter */
+	ns = timecounter_read(&igb->tc);
 
+	ts = ns_to_timespec64(ns);
 	pr_debug("igb overflow check at %lld.%09lu\n",
 		 (long long) ts.tv_sec, ts.tv_nsec);
 
@@ -1124,7 +1169,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.pps = 0;
 		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_82576;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82576;
@@ -1143,7 +1188,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.pps = 0;
 		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_82580;
 		adapter->ptp_caps.settime64 = igb_ptp_settime_82576;
 		adapter->ptp_caps.enable = igb_ptp_feature_enable;
 		adapter->cc.read = igb_ptp_read_82580;
@@ -1171,7 +1216,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->ptp_caps.pin_config = adapter->sdp_config;
 		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_i210;
 		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;
Signature · 1 line
2.17.2
Acked-by on v1 6/8
#17 lore raw

Re: [PATCH net-next 6/8] igb: extend PTP gettime function to read system clock

Quoted 9 lines · Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 65 ++++++++++++++++++++----
 1 file changed, 55 insertions(+), 10 deletions(-)
#18 lore raw

PATCH 7/8 ixgbe: extend PTP gettime function to read system clock

In reply to Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 54 ++++++++++++++++----
 1 file changed, 44 insertions(+), 10 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +44 -10
index b3e0d8bb5cbd..d81a50dc9535 100644
@@ -443,22 +443,52 @@ static int ixgbe_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 }
 
 /**
- * ixgbe_ptp_gettime
+ * ixgbe_ptp_gettimex
  * @ptp: the ptp clock structure
- * @ts: timespec structure to hold the current time value
+ * @ts: timespec to hold the PHC timestamp
+ * @sts: structure to hold the system time before and 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_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
+			      struct timespec64 *ts,
+			      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;
-	u64 ns;
+	u64 ns, stamp;
 
 	spin_lock_irqsave(&adapter->tmreg_lock, flags);
-	ns = timecounter_read(&adapter->hw_tc);
+
+	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);
 
 	*ts = ns_to_timespec64(ns);
@@ -567,10 +597,14 @@ void ixgbe_ptp_overflow_check(struct ixgbe_adapter *adapter)
 {
 	bool timeout = time_is_before_jiffies(adapter->last_overflow_check +
 					     IXGBE_OVERFLOW_PERIOD);
-	struct timespec64 ts;
+	unsigned long flags;
 
 	if (timeout) {
-		ixgbe_ptp_gettime(&adapter->ptp_caps, &ts);
+		/* Update the timecounter */
+		spin_lock_irqsave(&adapter->tmreg_lock, flags);
+		timecounter_read(&adapter->hw_tc);
+		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+
 		adapter->last_overflow_check = jiffies;
 	}
 }
@@ -1216,7 +1250,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.pps = 1;
 		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;
@@ -1233,7 +1267,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.pps = 0;
 		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;
@@ -1249,7 +1283,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter)
 		adapter->ptp_caps.pps = 0;
 		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;
Signature · 1 line
2.17.2
#19 lore raw

RE: [PATCH net-next 7/8] ixgbe: extend PTP gettime function to read system clock

Quoted 41 lines

-----Original Message-----
From: Miroslav Lichvar [mailto:[email protected]]
Sent: Friday, November 09, 2018 2:15 AM
To: [email protected]
Cc: Richard Cochran <[email protected]>; Keller, Jacob E <[email protected]>; Miroslav Lichvar <[email protected]>; Kirsher, Jeffrey T <[email protected]>
Subject: [PATCH net-next 7/8] ixgbe: extend PTP gettime function to read system clock

drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +22 -3
-static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp,
+			      struct timespec64 *ts,
+			      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;
-	u64 ns;
+	u64 ns, stamp;
 
 	spin_lock_irqsave(&adapter->tmreg_lock, flags);
-	ns = timecounter_read(&adapter->hw_tc);
+
+	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;

drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c static int ixgbe_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) · line 491

Quoted 10 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +10 -0
+	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);
+

At first, I was confused by this entire block of code, but then realized that we can't update the timecounter_read method, so we instead have to break this out so that our calls to ptp_read_system_prets() and ptp_read_system_postts() can be added between the register reads.

Ok, that makes sense.

Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +0 -0
 	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
 
 	*ts = ns_to_timespec64(ns);
@@ -567,10 +597,14 @@ void ixgbe_ptp_overflow_check(struct ixgbe_adapter
 *adapter)
 {
 	bool timeout = time_is_before_jiffies(adapter->last_overflow_check +
 					     IXGBE_OVERFLOW_PERIOD);

drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c void ixgbe_ptp_overflow_check(struct ixgbe_adapter · line 608

Quoted 10 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +6 -2
-	struct timespec64 ts;
+	unsigned long flags;
 
 	if (timeout) {
-		ixgbe_ptp_gettime(&adapter->ptp_caps, &ts);
+		/* Update the timecounter */
+		spin_lock_irqsave(&adapter->tmreg_lock, flags);
+		timecounter_read(&adapter->hw_tc);
+		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+

This also explains this change where we now have to update the timecounter during the overflow check.

Ok, this makes sense to me.

Thanks,
Jake

Acked-by on v1 7/8
#20 lore raw

Re: [PATCH net-next 7/8] ixgbe: extend PTP gettime function to read system clock

In reply to Miroslav Lichvar
Quoted 9 lines · Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Jacob Keller <[email protected]>
Cc Jeff Kirsher <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 54 ++++++++++++++++----
 1 file changed, 44 insertions(+), 10 deletions(-)
#21 lore raw

PATCH 8/8 tg3: extend PTP gettime function to read system clock

In reply to Miroslav Lichvar

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Cc Richard Cochran <[email protected]>
Cc Michael Chan <[email protected]>
---
 drivers/net/ethernet/broadcom/tg3.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/tg3.c +13 -6
index 89295306f161..ce44d208e137 100644
@@ -6135,10 +6135,16 @@ static int tg3_setup_phy(struct tg3 *tp, bool force_reset)
 }
 
 /* tp->lock must be held */
-static u64 tg3_refclk_read(struct tg3 *tp)
+static u64 tg3_refclk_read(struct tg3 *tp, struct ptp_system_timestamp *sts)
 {
-	u64 stamp = tr32(TG3_EAV_REF_CLCK_LSB);
-	return stamp | (u64)tr32(TG3_EAV_REF_CLCK_MSB) << 32;
+	u64 stamp;
+
+	ptp_read_system_prets(sts);
+	stamp = tr32(TG3_EAV_REF_CLCK_LSB);
+	ptp_read_system_postts(sts);
+	stamp |= (u64)tr32(TG3_EAV_REF_CLCK_MSB) << 32;
+
+	return stamp;
 }
 
 /* tp->lock must be held */
@@ -6229,13 +6235,14 @@ static int tg3_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
-static int tg3_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+static int tg3_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts,
+			    struct ptp_system_timestamp *sts)
 {
 	u64 ns;
 	struct tg3 *tp = container_of(ptp, struct tg3, ptp_info);
 
 	tg3_full_lock(tp, 0);
-	ns = tg3_refclk_read(tp);
+	ns = tg3_refclk_read(tp, sts);
 	ns += tp->ptp_adjust;
 	tg3_full_unlock(tp);
 
@@ -6330,7 +6337,7 @@ static const struct ptp_clock_info tg3_ptp_caps = {
 	.pps		= 0,
 	.adjfreq	= tg3_ptp_adjfreq,
 	.adjtime	= tg3_ptp_adjtime,
-	.gettime64	= tg3_ptp_gettime,
+	.gettimex64	= tg3_ptp_gettimex,
 	.settime64	= tg3_ptp_settime,
 	.enable		= tg3_ptp_enable,
 };
Signature · 1 line
2.17.2
Reviewed-by on the v1 series
#22 lore raw
In reply to Miroslav Lichvar
Quoted 21 lines

-----Original Message-----
From: Miroslav Lichvar [mailto:[email protected]]
Sent: Friday, November 09, 2018 2:15 AM
To: [email protected]
Cc: Richard Cochran <[email protected]>; Keller, Jacob E <[email protected]>; Miroslav Lichvar <[email protected]>; Marcelo Tosatti <[email protected]>; Kirsher, Jeffrey T <[email protected]>; Michael Chan <[email protected]>
Subject: [PATCH net-next 0/8] More accurate PHC<->system clock synchronization

RFC->v1:
- added new patches
- separated PHC timestamp from ptp_system_timestamp
- fixed memory leak in PTP_SYS_OFFSET_EXTENDED
- changed PTP_SYS_OFFSET_EXTENDED to work with array of arrays
- fixed PTP_SYS_OFFSET_EXTENDED to break correctly from loop
- fixed timecounter updates in drivers
- split gettimex in igb driver
- fixed ptp_read_* functions to be available without CONFIG_PTP_1588_CLOCK

Quoted 2 lines

This series enables a more accurate synchronization between PTP hardware clocks and the system clock.

Thanks for doing this, Miroslav!

Quoted 15 lines

The first two patches are minor cleanup/bug fixes.

The third 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 get a smaller upper bound on the maximum error.

The fourth patch deprecates the original gettime function.

The remaining patches update the gettime function in order to support the new ioctl in the e1000e, igb, ixgbe, and tg3 drivers.

Quoted 9 lines

Tests with few different NICs in different machines show that:
- with an I219 (e1000e) the measured delay was reduced from 2500 to 1300 ns and the error in the measured offset, when compared to the cross timestamping supported by the driver, was reduced by a factor of 5
- with an I210 (igb) the delay was reduced from 5100 to 1700 ns
- with an I350 (igb) the delay was reduced from 2300 to 750 ns
- with an X550 (ixgbe) the delay was reduced from 1950 to 650 ns
- with a BCM5720 (tg3) the delay was reduced from 2400 to 1200 ns

Impressive results!

For the main portions and the Intel driver changes this is

Regards,
Jake

Quoted 25 lines

Miroslav Lichvar (8):
ptp: reorder declarations in ptp_ioctl()
ptp: check gettime64 return code in PTP_SYS_OFFSET ioctl
ptp: add PTP_SYS_OFFSET_EXTENDED ioctl
ptp: deprecate gettime64() in favor of gettimex64()
e1000e: extend PTP gettime function to read system clock
igb: extend PTP gettime function to read system clock
ixgbe: extend PTP gettime function to read system clock
tg3: extend PTP gettime function to read system clock

 drivers/net/ethernet/broadcom/tg3.c          | 19 ++++--
 drivers/net/ethernet/intel/e1000e/e1000.h    |  3 +
 drivers/net/ethernet/intel/e1000e/netdev.c   | 42 ++++++++++---
 drivers/net/ethernet/intel/e1000e/ptp.c      | 16 +++--
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 65 +++++++++++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 54 +++++++++++++---
 drivers/ptp/ptp_chardev.c                    | 55 ++++++++++++++---
 drivers/ptp/ptp_clock.c                      |  5 +-
 include/linux/ptp_clock_kernel.h             | 33 ++++++++++
 include/uapi/linux/ptp_clock.h               | 12 ++++
 10 files changed, 253 insertions(+), 51 deletions(-)

--
2.17.2

maintainer
#23 lore raw
In reply to Miroslav Lichvar

From: Miroslav Lichvar <[email protected]>
Date: Fri, 9 Nov 2018 11:14:41 +0100

Quoted 11 lines

RFC->v1:
- added new patches
- separated PHC timestamp from ptp_system_timestamp
- fixed memory leak in PTP_SYS_OFFSET_EXTENDED
- changed PTP_SYS_OFFSET_EXTENDED to work with array of arrays
- fixed PTP_SYS_OFFSET_EXTENDED to break correctly from loop
- fixed timecounter updates in drivers
- split gettimex in igb driver
- fixed ptp_read_* functions to be available without CONFIG_PTP_1588_CLOCK

Quoted 2 lines

This series enables a more accurate synchronization between PTP hardware clocks and the system clock.

...

This series looks good to me but I want to give Richard an opportunity to review it first.

#24 lore raw
Quoted 14 lines · David Miller

From: Miroslav Lichvar <[email protected]>
Date: Fri, 9 Nov 2018 11:14:41 +0100

Quoted 11 lines

RFC->v1:
- added new patches
- separated PHC timestamp from ptp_system_timestamp
- fixed memory leak in PTP_SYS_OFFSET_EXTENDED
- changed PTP_SYS_OFFSET_EXTENDED to work with array of arrays
- fixed PTP_SYS_OFFSET_EXTENDED to break correctly from loop
- fixed timecounter updates in drivers
- split gettimex in igb driver
- fixed ptp_read_* functions to be available without CONFIG_PTP_1588_CLOCK

Quoted 7 lines
Quoted 3 lines

This series enables a more accurate synchronization between PTP hardware
clocks and the system clock.

...

This series looks good to me but I want to give Richard an opportunity to review it first.

Dave, I also do not want to hold this series up by picking up patches 5, 6 and 7 (Intel drivers) so please apply the entire series after Richard provides his review.

maintainer
#25 lore raw

From: Jeff Kirsher <[email protected]>
Date: Fri, 09 Nov 2018 15:33:10 -0800

Quoted 18 lines · David Miller
Quoted 17 lines

From: Miroslav Lichvar <[email protected]>
Date: Fri, 9 Nov 2018 11:14:41 +0100

Quoted 14 lines

RFC->v1:
- added new patches
- separated PHC timestamp from ptp_system_timestamp
- fixed memory leak in PTP_SYS_OFFSET_EXTENDED
- changed PTP_SYS_OFFSET_EXTENDED to work with array of arrays
- fixed PTP_SYS_OFFSET_EXTENDED to break correctly from loop
- fixed timecounter updates in drivers
- split gettimex in igb driver
- fixed ptp_read_* functions to be available without CONFIG_PTP_1588_CLOCK

This series enables a more accurate synchronization between PTP hardware
clocks and the system clock.

Quoted 8 lines
Quoted 4 lines

...

This series looks good to me but I want to give Richard an opportunity to review it first.

Dave, I also do not want to hold this series up by picking up patches 5, 6 and 7 (Intel drivers) so please apply the entire series after Richard provides his review.

Ok, will do.

maintainer
#27 lore raw

From: Richard Cochran <[email protected]>
Date: Fri, 9 Nov 2018 17:44:31 -0800

Quoted 7 lines · David Miller
Quoted 2 lines

This series looks good to me but I want to give Richard an opportunity to review it first.

The series is good to go.

Great, series applied to net-next, thanks everyone.