[Linuxptp-devel] [PATCH 1/2] Add non-PPS mode to phc2sys.

[Miroslav Lichvar]

As some PHC hardware/drivers don't provide PPS, it may be useful to keep the
system clock synchronized to PHC via clock_gettime(). While the precision
of the clock is only in microsecond range, the error seems to be quite stable.

The -d parameter now can be omitted if -s is provided.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
makefile | 2 +
phc2sys.c | 113 +++++++++++++++++++++++++++++++++++--------------------------
2 files changed, 67 insertions(+), 48 deletions(-)

diff --git a/makefile b/makefile
index fe0dcbd..c09b644 100644
--- a/makefile
+++ b/makefile
@@ -16,6 +16,8 @@
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

KBUILD_OUTPUT ?= /lib/modules/$(shell uname -r)/build
+KBUILD_OUTPUT=.
+EXTRA_CFLAGS=-I/root/vanilla/include/

DEBUG =
CC = $(CROSS_COMPILE)gcc
diff --git a/phc2sys.c b/phc2sys.c
index c9e051c..24cc8d6 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -85,33 +85,36 @@ static void clock_step(clockid_t clkid, int64_t ns)
fprintf(stderr, "failed to step clock: %m\n");
}

-static int64_t read_phc(clockid_t clkid, clockid_t sysclk, int rdelay)
+static int read_phc(clockid_t clkid, clockid_t sysclk, int rdelay, int64_t *offset, uint64_t *ts)
{
- int64_t offset = 0;
+ struct timespec tsrc, tdst;

if (clkid == CLOCK_INVALID) {
return 0;
}
- if (rdelay) {
- struct timespec tsrc, tdst;

- if (clock_gettime(clkid, &tsrc))
- perror("clock_gettime");
-
- if (clock_gettime(sysclk, &tdst))
- perror("clock_gettime");
+ if (clock_gettime(clkid, &tsrc)) {
+ perror("clock_gettime");
+ return 0;
+ }

- offset = tdst.tv_sec * NS_PER_SEC - tsrc.tv_sec * NS_PER_SEC +
- tdst.tv_nsec - tsrc.tv_nsec - rdelay;
+ if (clock_gettime(sysclk, &tdst)) {
+ perror("clock_gettime");
+ return 0;
}
- return offset;
+
+ *offset = tdst.tv_sec * NS_PER_SEC - tsrc.tv_sec * NS_PER_SEC +
+ tdst.tv_nsec - tsrc.tv_nsec - rdelay;
+ *ts = tdst.tv_sec * NS_PER_SEC + tdst.tv_nsec;
+
+ return 1;
}

struct servo {
uint64_t last_ts;
double drift;
enum {
- PPS_0, PPS_1, PPS_2, PPS_3, PPS_N
+ SAMPLE_0, SAMPLE_1, SAMPLE_2, SAMPLE_3, SAMPLE_N
} state;
};

@@ -119,42 +122,34 @@ static struct servo servo;

static void do_servo(struct servo *srv,
clockid_t src, clockid_t dst,
- uint64_t ts, double kp, double ki, int rdelay)
+ int64_t offset, uint64_t ts, double kp, double ki)
{
double ki_term, ppb;
- int64_t delta, offset, phc;
-
- offset = ts % NS_PER_SEC;
- if (offset > NS_PER_SEC / 2) {
- offset -= NS_PER_SEC;
- }
-
- phc = read_phc(src, dst, rdelay);
+ int64_t delta;

- printf("s%d %lld.%09llu offset %9lld phc %9lld drift %.2f\n",
- srv->state, ts / NS_PER_SEC, ts % NS_PER_SEC,
- offset, phc, srv->drift);
+ printf("s%d %lld.%09llu drift %.2f\n",
+ srv->state, ts / NS_PER_SEC, ts % NS_PER_SEC, srv->drift);

switch (srv->state) {
- case PPS_0:
+ case SAMPLE_0:
clock_ppb(dst, 0.0);
- srv->state = PPS_1;
+ srv->state = SAMPLE_1;
break;
- case PPS_1:
- srv->state = PPS_2;
+ case SAMPLE_1:
+ srv->state = SAMPLE_2;
break;
- case PPS_2:
+ case SAMPLE_2:
delta = ts - srv->last_ts;
offset = delta - NS_PER_SEC;
srv->drift = offset;
clock_ppb(dst, -offset);
- srv->state = PPS_3;
+ srv->state = SAMPLE_3;
break;
- case PPS_3:
+ case SAMPLE_3:
clock_step(dst, -offset);
- srv->state = PPS_N;
+ srv->state = SAMPLE_N;
break;
- case PPS_N:
+ case SAMPLE_N:
ki_term = ki * offset;
ppb = kp * offset + srv->drift + ki_term;
if (ppb < min_ppb) {
@@ -171,10 +166,10 @@ static void do_servo(struct servo *srv,
srv->last_ts = ts;
}

-static uint64_t read_pps(int fd)
+static int read_pps(int fd, int64_t *offset, uint64_t *ts)
{
struct pps_fdata pfd;
- uint64_t ts;
+
pfd.timeout.sec = 10;
pfd.timeout.nsec = 0;
pfd.timeout.flags = ~PPS_TIME_INVALID;
@@ -182,9 +177,15 @@ static uint64_t read_pps(int fd)
perror("ioctl PPS_FETCH");
return 0;
}
- ts = pfd.info.assert_tu.sec * NS_PER_SEC;
- ts += pfd.info.assert_tu.nsec;
- return ts;
+
+ *ts = pfd.info.assert_tu.sec * NS_PER_SEC;
+ *ts += pfd.info.assert_tu.nsec;
+
+ *offset = *ts % NS_PER_SEC;
+ if (*offset > NS_PER_SEC / 2)
+ *offset -= NS_PER_SEC;
+
+ return 1;
}

static void usage(char *progname)
@@ -208,8 +209,9 @@ int main(int argc, char *argv[])
double kp = KP, ki = KI;
char *device = NULL, *progname;
clockid_t src = CLOCK_INVALID, dst = CLOCK_REALTIME;
- uint64_t ts;
- int c, fd, rdelay = 0;
+ uint64_t pps_ts, phc_ts;
+ int64_t pps_offset, phc_offset;
+ int c, fd = 0, rdelay = 0;

/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
@@ -243,14 +245,16 @@ int main(int argc, char *argv[])
}
}

- if (!device || dst == CLOCK_INVALID) {
+ if (!(device || src != CLOCK_INVALID) || dst == CLOCK_INVALID) {
usage(progname);
return -1;
}
- fd = open(device, O_RDONLY);
- if (fd < 0) {
- fprintf(stderr, "cannot open %s: %m\n", device);
- return -1;
+ if (device) {
+ fd = open(device, O_RDONLY);
+ if (fd < 0) {
+ fprintf(stderr, "cannot open %s: %m\n", device);
+ return -1;
+ }
}
if (src != CLOCK_INVALID) {
struct timespec now;
@@ -260,8 +264,21 @@ int main(int argc, char *argv[])
perror("clock_settime");
}
while (1) {
- ts = read_pps(fd);
- do_servo(&servo, src, dst, ts, kp, ki, rdelay);
+ if (fd > 0) {
+ if (!read_pps(fd, &pps_offset, &pps_ts))
+ continue;
+ printf("pps %9lld ", pps_offset);
+ } else
+ usleep(1000000);
+
+ if (!read_phc(src, dst, rdelay, &phc_offset, &phc_ts))
+ continue;
+ printf("phc %9lld ", phc_offset);
+
+ if (fd > 0)
+ do_servo(&servo, src, dst, pps_offset, pps_ts, kp, ki);
+ else
+ do_servo(&servo, src, dst, phc_offset, phc_ts, kp, ki);
}
return 0;
}

[Miroslav Lichvar]

Make five PHC readings in sequence and pick the quickest one.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 35 +++++++++++++++++++++++------------
1 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 24cc8d6..21cb7c2 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -41,6 +41,8 @@
#define max_ppb 512000
#define min_ppb -512000

+#define PHC_READINGS 5
+
static clockid_t clock_open(char *device)
{
int fd = open(device, O_RDWR);
@@ -87,26 +89,35 @@ static void clock_step(clockid_t clkid, int64_t ns)

static int read_phc(clockid_t clkid, clockid_t sysclk, int rdelay, int64_t *offset, uint64_t *ts)
{
- struct timespec tsrc, tdst;
+ struct timespec tdst1, tdst2, tsrc;
+ int i;
+ int64_t interval, best_interval = INT64_MAX;

if (clkid == CLOCK_INVALID) {
return 0;
}

- if (clock_gettime(clkid, &tsrc)) {
- perror("clock_gettime");
- return 0;
- }
+ /* Pick the quickest clkid reading. */
+ for (i = 0; i < PHC_READINGS; i++) {
+ if (clock_gettime(sysclk, &tdst1) ||
+ clock_gettime(clkid, &tsrc) ||
+ clock_gettime(sysclk, &tdst2)) {
+ perror("clock_gettime");
+ return 0;
+ }
+
+ interval = (tdst2.tv_sec - tdst1.tv_sec) * NS_PER_SEC +
+ tdst2.tv_nsec - tdst1.tv_nsec;

- if (clock_gettime(sysclk, &tdst)) {
- perror("clock_gettime");
- return 0;
+ if (best_interval > interval) {
+ best_interval = interval;
+ *offset = (tdst1.tv_sec - tsrc.tv_sec) * NS_PER_SEC +
+ tdst1.tv_nsec - tsrc.tv_nsec +
+ interval / 2 - rdelay;
+ *ts = tdst2.tv_sec * NS_PER_SEC + tdst2.tv_nsec;
+ }
}

- *offset = tdst.tv_sec * NS_PER_SEC - tsrc.tv_sec * NS_PER_SEC +
- tdst.tv_nsec - tsrc.tv_nsec - rdelay;
- *ts = tdst.tv_sec * NS_PER_SEC + tdst.tv_nsec;
-
return 1;
}

[Richard Cochran]

Post by Miroslav Lichvar
Make five PHC readings in sequence and pick the quickest one.

...

Post by Miroslav Lichvar
@@ -87,26 +89,35 @@ static void clock_step(clockid_t clkid, int64_t ns)
static int read_phc(clockid_t clkid, clockid_t sysclk, int rdelay, int64_t *offset, uint64_t *ts)
{
- struct timespec tsrc, tdst;
+ struct timespec tdst1, tdst2, tsrc;
+ int i;
+ int64_t interval, best_interval = INT64_MAX;
if (clkid == CLOCK_INVALID) {
return 0;
}
- if (clock_gettime(clkid, &tsrc)) {
- perror("clock_gettime");
- return 0;
- }
+ /* Pick the quickest clkid reading. */
+ for (i = 0; i < PHC_READINGS; i++) {
+ if (clock_gettime(sysclk, &tdst1) ||
+ clock_gettime(clkid, &tsrc) ||
+ clock_gettime(sysclk, &tdst2)) {
+ perror("clock_gettime");

Wouldn't it be better to swap 'sysclk' and 'clkid' here, and negate
the resulting offset? In the normal case, clkid is CLOCK_REALTIME,
and reading it is probably quicker.

Quoting from linux/include/linux/timecompare.h which implements the
same basic idea:

* The target time corresponding to a source time is determined by
* reading target time, reading source time, reading target time
* again, then assuming that average target time corresponds to source
* time. In other words, the assumption is that reading the source
* time is slow and involves equal time for sending the request and
* receiving the reply, whereas reading target time is assumed to be
* fast.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
Make five PHC readings in sequence and pick the quickest one.

...

Post by Miroslav Lichvar
@@ -87,26 +89,35 @@ static void clock_step(clockid_t clkid, int64_t ns)
static int read_phc(clockid_t clkid, clockid_t sysclk, int rdelay, int64_t *offset, uint64_t *ts)
{
+ /* Pick the quickest clkid reading. */
+ for (i = 0; i < PHC_READINGS; i++) {
+ if (clock_gettime(sysclk, &tdst1) ||
+ clock_gettime(clkid, &tsrc) ||
+ clock_gettime(sysclk, &tdst2)) {
+ perror("clock_gettime");

Wouldn't it be better to swap 'sysclk' and 'clkid' here, and negate
the resulting offset? In the normal case, clkid is CLOCK_REALTIME,
and reading it is probably quicker.

I think the order is corrent here, sysclk is the fast target
(CLOCK_REALTIME) clock and clkid is the slow source (PHC) clock.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Miroslav Lichvar
Make five PHC readings in sequence and pick the quickest one.

...

Post by Miroslav Lichvar
@@ -87,26 +89,35 @@ static void clock_step(clockid_t clkid, int64_t ns)
static int read_phc(clockid_t clkid, clockid_t sysclk, int rdelay, int64_t *offset, uint64_t *ts)
{
+ /* Pick the quickest clkid reading. */
+ for (i = 0; i < PHC_READINGS; i++) {
+ if (clock_gettime(sysclk, &tdst1) ||
+ clock_gettime(clkid, &tsrc) ||
+ clock_gettime(sysclk, &tdst2)) {
+ perror("clock_gettime");

Wouldn't it be better to swap 'sysclk' and 'clkid' here, and negate
the resulting offset? In the normal case, clkid is CLOCK_REALTIME,
and reading it is probably quicker.

I think the order is corrent here, sysclk is the fast target
(CLOCK_REALTIME) clock and clkid is the slow source (PHC) clock.

Okay, never mind, I am confused.

BTW, I am working on exporting the in-kernel timecompare method as a
standard ioctl for all PHC devices. If you want me to, I'll put you on
CC for the patches to review.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran
BTW, I am working on exporting the in-kernel timecompare method as a
standard ioctl for all PHC devices. If you want me to, I'll put you on
CC for the patches to review.

Yes, please CC me.

What advantage will it have? No context switch? In the userspace we
can make the number of loops adaptive, keep a minimum delay and as
soon as we get a reading with that or a very close delay, we don't
need to make any more readings.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran
BTW, I am working on exporting the in-kernel timecompare method as a
standard ioctl for all PHC devices. If you want me to, I'll put you on
CC for the patches to review.

Yes, please CC me.
What advantage will it have? No context switch?

- avoids user/kernel mode switching (avoids preemption points)
- shorter code path

But the main reason was just to make use of that code. It won't cost
much to add it as an option, and it does not preclude doing the same
in user space.

Take a look at the implementation (I didn't write it). It does what
your phc2sys code does, but in a different way. It might be that it
works better on some machines than a user space solution.

Only blackfin still uses that code, and if the timecompare stuff is no
good, then we should remove it (after converting blackfin to PHC).

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
What advantage will it have? No context switch?

- avoids user/kernel mode switching (avoids preemption points)
- shorter code path
But the main reason was just to make use of that code. It won't cost
much to add it as an option, and it does not preclude doing the same
in user space.
Take a look at the implementation (I didn't write it). It does what
your phc2sys code does, but in a different way. It might be that it
works better on some machines than a user space solution.

It seems it's a more general approach to what I did in phc2sys,
instead of picking the fastest sample it calculates an average from 75%
fastest readings. I suspect the delay has much larger effect on the
accuracy of the result than the jitter in the timestamp itself, but I don't
really have any numbers to back it up. I think the kernel code could
be improved by using a dynamic cut-off (e.g. specified by maximum
ratio of the samples' duration to the minimum sample duration) instead
of the static 75%.

Perhaps it would make sense to reduce the kernel code to just fill a
buffer provided by user space with samples consisting of PHC
timestamp, PHC-SYS offset and the duration, so we can filter the
samples in phc2sys as we see fit?

Post by Richard Cochran
Only blackfin still uses that code, and if the timecompare stuff is no
good, then we should remove it (after converting blackfin to PHC).

It seems the timecompare_update() function can be done easily
in user space too, without any drawbacks, is that correct?

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
It seems it's a more general approach to what I did in phc2sys,
instead of picking the fastest sample it calculates an average from 75%
fastest readings. I suspect the delay has much larger effect on the
accuracy of the result than the jitter in the timestamp itself, but I don't
really have any numbers to back it up. I think the kernel code could
be improved by using a dynamic cut-off (e.g. specified by maximum
ratio of the samples' duration to the minimum sample duration) instead
of the static 75%.

FWIW, I don't think the timecompare thing is really well tested,
except on one particular flavor of Intel hardware that Patrick Ohly
was working with.

Post by Miroslav Lichvar
Perhaps it would make sense to reduce the kernel code to just fill a
buffer provided by user space with samples consisting of PHC
timestamp, PHC-SYS offset and the duration, so we can filter the
samples in phc2sys as we see fit?

I like this idea. Maybe just deliver an array of <sys,phc,sys> triples
and let user space do the rest.

Post by Miroslav Lichvar
It seems the timecompare_update() function can be done easily
in user space too, without any drawbacks, is that correct?

Yep, that is how I see it, too. I think I will just avoid the
timecompare code altogether.

Thanks,
Richard

[Jacob Keller]

Post by Richard Cochran

Post by Miroslav Lichvar
It seems it's a more general approach to what I did in phc2sys,
instead of picking the fastest sample it calculates an average from 75%
fastest readings. I suspect the delay has much larger effect on the
accuracy of the result than the jitter in the timestamp itself, but I don't
really have any numbers to back it up. I think the kernel code could
be improved by using a dynamic cut-off (e.g. specified by maximum
ratio of the samples' duration to the minimum sample duration) instead
of the static 75%.

FWIW, I don't think the timecompare thing is really well tested,
except on one particular flavor of Intel hardware that Patrick Ohly
was working with.

Post by Miroslav Lichvar
Perhaps it would make sense to reduce the kernel code to just fill a
buffer provided by user space with samples consisting of PHC
timestamp, PHC-SYS offset and the duration, so we can filter the
samples in phc2sys as we see fit?

I like this idea. Maybe just deliver an array of <sys,phc,sys> triples
and let user space do the rest.

Post by Miroslav Lichvar
It seems the timecompare_update() function can be done easily
in user space too, without any drawbacks, is that correct?

Yep, that is how I see it, too. I think I will just avoid the
timecompare code altogether.
Thanks,
Richard
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

I'd suggest going ahead and removing timecompare after updating
blackfin. Also, please CC me as well on those patches, I am interested.
I definitely agree, return sys,phc,sys triples is a lot more useful.
Please leave the cyclecounter/timecounter code though, as that is what
we use for detecting clock-wraparounds on intel parts.

- Jake

[Richard Cochran]

Post by Jacob Keller
I'd suggest going ahead and removing timecompare after updating
blackfin. Also, please CC me as well on those patches, I am interested.
I definitely agree, return sys,phc,sys triples is a lot more useful.
Please leave the cyclecounter/timecounter code though, as that is what
we use for detecting clock-wraparounds on intel parts.

Anyone got a blackfin kit out there for testing?

Thanks,
Richard

[Miroslav Lichvar]

Post by Miroslav Lichvar
--- a/makefile
+++ b/makefile
@@ -16,6 +16,8 @@
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
KBUILD_OUTPUT ?= /lib/modules/$(shell uname -r)/build
+KBUILD_OUTPUT=.
+EXTRA_CFLAGS=-I/root/vanilla/include/

Please ignore this hunk.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Miroslav Lichvar
--- a/makefile
+++ b/makefile
@@ -16,6 +16,8 @@
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
KBUILD_OUTPUT ?= /lib/modules/$(shell uname -r)/build
+KBUILD_OUTPUT=.
+EXTRA_CFLAGS=-I/root/vanilla/include/

Please ignore this hunk.

Applied both patches, without the makefile hunk and fixing some stray
whitespace.

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
Applied both patches, without the makefile hunk and fixing some stray
whitespace.

FYI, I reverted both patches and then reapplied them. When I rebased
with the fixups, git lost the original committer info.

Thanks,
Richard