[Linuxptp-devel] [PATCH 1/7] phc2sys: Read maximum frequency adjustment.

[Miroslav Lichvar]

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

diff --git a/makefile b/makefile
index f30d836..058bdb2 100644
--- a/makefile
+++ b/makefile
@@ -52,8 +52,9 @@ ptp4l: $(OBJ)
pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
udp6.o uds.o util.o version.o

-phc2sys: clockadj.o msg.o phc2sys.o pmc_common.o print.o pi.o servo.o raw.o \
- sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o version.o
+phc2sys: clockadj.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
+ raw.o sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o \
+ version.o

hwstamp_ctl: hwstamp_ctl.o version.o

diff --git a/phc2sys.c b/phc2sys.c
index c4486c3..4c68f13 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -37,6 +37,7 @@
#include "ds.h"
#include "fsm.h"
#include "missing.h"
+#include "phc.h"
#include "pi.h"
#include "pmc_common.h"
#include "print.h"
@@ -52,8 +53,6 @@
#define KI 0.3
#define NS_PER_SEC 1000000000LL

-#define max_ppb 512000
-
#define PHC_PPS_OFFSET_LIMIT 10000000
#define PMC_UPDATE_INTERVAL (60 * NS_PER_SEC)

@@ -543,7 +542,7 @@ int main(int argc, char *argv[])
char *progname, *ethdev = NULL;
clockid_t src = CLOCK_INVALID;
int c, phc_readings = 5, phc_rate = 1, pps_fd = -1;
- int r, wait_sync = 0, forced_sync_offset = 0;
+ int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
double ppb;
struct clock dst_clock = {
@@ -701,6 +700,15 @@ int main(int argc, char *argv[])
make sure ppb is the actual frequency of the clock. */
clockadj_set_freq(dst_clock.clkid, ppb);
clockadj_set_leap(dst_clock.clkid, 0);
+ if (dst_clock.clkid == CLOCK_REALTIME) {
+ max_ppb = sysclk_max_freq();
+ } else {
+ max_ppb = phc_max_adj(dst_clock.clkid);
+ if (!max_ppb) {
+ pr_err("clock is not adjustable");
+ return -1;
+ }
+ }

dst_clock.servo = servo_create(CLOCK_SERVO_PI, -ppb, max_ppb, 0);

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 4 ++--
clockadj.c | 3 ++-
clockadj.h | 5 ++---
phc2sys.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/clock.c b/clock.c
index dedf8e0..cc692e9 100644
--- a/clock.c
+++ b/clock.c
@@ -508,7 +508,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
&leap, &utc_offset);
if (c->leap_set != clock_leap) {
if (c->kernel_leap)
- clockadj_set_leap(c->clkid, clock_leap);
+ sysclk_set_leap(clock_leap);
c->leap_set = clock_leap;
}
}
@@ -593,7 +593,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
c->clkid = CLOCK_REALTIME;
c->utc_timescale = 1;
max_adj = sysclk_max_freq();
- clockadj_set_leap(c->clkid, 0);
+ sysclk_set_leap(0);
}
c->leap_set = 0;
c->kernel_leap = dds->kernel_leap;
diff --git a/clockadj.c b/clockadj.c
index 870ee61..74963c8 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -71,8 +71,9 @@ void clockadj_step(clockid_t clkid, int64_t step)
pr_err("failed to step clock: %m");
}

-void clockadj_set_leap(clockid_t clkid, int leap)
+void sysclk_set_leap(int leap)
{
+ clockid_t clkid = CLOCK_REALTIME;
struct timex tx;
const char *m = NULL;
memset(&tx, 0, sizeof(tx));
diff --git a/clockadj.h b/clockadj.h
index d9a8a65..3779f03 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -45,12 +45,11 @@ double clockadj_get_freq(clockid_t clkid);
void clockadj_step(clockid_t clkid, int64_t step);

/**
- * Insert/delete leap second at midnight.
- * @param clkid CLOCK_REALTIME.
+ * Set the system clock to insert/delete leap second at midnight.
* @param leap +1 to insert leap second, -1 to delete leap second,
* 0 to reset the leap state.
*/
-void clockadj_set_leap(clockid_t clkid, int leap);
+void sysclk_set_leap(int leap);

/**
* Read maximum frequency adjustment of the system clock (CLOCK_REALTIME).
diff --git a/phc2sys.c b/phc2sys.c
index 016a565..076840c 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -502,7 +502,7 @@ static int update_sync_offset(struct clock *clock, int64_t offset, uint64_t ts)
if (clock->leap_set != clock_leap) {
/* Only the system clock can leap. */
if (clock->clkid == CLOCK_REALTIME && clock->kernel_leap)
- clockadj_set_leap(clock->clkid, clock_leap);
+ sysclk_set_leap(clock_leap);
clock->leap_set = clock_leap;
}

@@ -702,8 +702,8 @@ int main(int argc, char *argv[])
/* The reading may silently fail and return 0, reset the frequency to
make sure ppb is the actual frequency of the clock. */
clockadj_set_freq(dst_clock.clkid, ppb);
- clockadj_set_leap(dst_clock.clkid, 0);
if (dst_clock.clkid == CLOCK_REALTIME) {
+ sysclk_set_leap(0);
max_ppb = sysclk_max_freq();
} else {
max_ppb = phc_max_adj(dst_clock.clkid);

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.8 | 5 +++++
phc2sys.c | 15 ++++++++++-----
2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index 67b2f27..f5f72b4 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -31,6 +31,8 @@ phc2sys \- synchronize two clocks
] [
.B \-w
] [
+.BI \-n " domain-number"
+] [
.B \-x
] [
.BI \-l " print-level"
@@ -129,6 +131,9 @@ option is not used, also keep the offset between the slave and master
times updated according to the currentUtcOffset value obtained from ptp4l and
the direction of the clock synchronization.
.TP
+.BI \-n " domain-number"
+Specify the domain number used by ptp4l. The default is 0.
+.TP
.B \-x
When a leap second is announced, don't apply it in the kernel by stepping the
clock, but let the servo correct the one-second offset slowly by changing the
diff --git a/phc2sys.c b/phc2sys.c
index ab35fb1..016a565 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -337,9 +337,10 @@ static void *get_mgt_data(struct ptp_message *msg)
return mgt->data;
}

-static int init_pmc(struct clock *clock)
+static int init_pmc(struct clock *clock, int domain_number)
{
- clock->pmc = pmc_create(TRANS_UDS, "/var/run/phc2sys", 0, 0, 0);
+ clock->pmc = pmc_create(TRANS_UDS, "/var/run/phc2sys", 0,
+ domain_number, 0);
if (!clock->pmc) {
pr_err("failed to create pmc");
return -1;
@@ -525,6 +526,7 @@ static void usage(char *progname)
" -O [offset] slave-master time offset (0)\n"
" -u [num] number of clock updates in summary stats (0)\n"
" -w wait for ptp4l\n"
+ " -n [num] domain number (0)\n"
" -x apply leap seconds by servo instead of kernel\n"
" -l [num] set the logging level to 'num' (6)\n"
" -m print messages to stdout\n"
@@ -539,7 +541,7 @@ int main(int argc, char *argv[])
{
char *progname, *ethdev = NULL;
clockid_t src = CLOCK_INVALID;
- int c, phc_readings = 5, phc_rate = 1, pps_fd = -1;
+ int c, domain_number = 0, phc_readings = 5, phc_rate = 1, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
double ppb;
@@ -556,7 +558,7 @@ int main(int argc, char *argv[])
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
while (EOF != (c = getopt(argc, argv,
- "c:d:hs:P:I:S:R:N:O:i:u:wxl:mqv"))) {
+ "c:d:hs:P:I:S:R:N:O:i:u:wn:xl:mqv"))) {
switch (c) {
case 'c':
dst_clock.clkid = clock_open(optarg);
@@ -601,6 +603,9 @@ int main(int argc, char *argv[])
case 'w':
wait_sync = 1;
break;
+ case 'n':
+ domain_number = atoi(optarg);
+ break;
case 'x':
dst_clock.kernel_leap = 0;
break;
@@ -664,7 +669,7 @@ int main(int argc, char *argv[])
print_set_level(print_level);

if (wait_sync) {
- if (init_pmc(&dst_clock))
+ if (init_pmc(&dst_clock, domain_number))
return -1;

while (1) {

[Miroslav Lichvar]

PHC drivers may allow frequency offset of -1.0, which stops the
clock. Limit the frequency adjustment to 90% to be sure that two
consecutive clock readings will make different time stamps.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/phc.c b/phc.c
index de5eeb8..b52253c 100644
--- a/phc.c
+++ b/phc.c
@@ -36,6 +36,10 @@
#define BITS_PER_LONG (sizeof(long)*8)
#define MAX_PPB_32 32767999 /* 2^31 - 1 / 65.536 */

+/* Limit the maximum frequency adjustment to 90% to avoid stopping
+ * the clock with drivers which allow extremely large adjustments. */
+#define MAX_PPB 900000000
+
clockid_t phc_open(char *phc)
{
int fd = open(phc, O_RDWR);
@@ -66,5 +70,8 @@ int phc_max_adj(clockid_t clkid)
if (BITS_PER_LONG == 32 && max > MAX_PPB_32)
max = MAX_PPB_32;

+ if (max > MAX_PPB)
+ max = MAX_PPB;
+
return max;
}

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index cab5c9a..ab35fb1 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -61,7 +61,7 @@ static int update_sync_offset(struct clock *clock, int64_t offset, uint64_t ts);

static clockid_t clock_open(char *device)
{
- int fd;
+ int clkid;

if (device[0] != '/') {
if (!strcasecmp(device, "CLOCK_REALTIME"))
@@ -71,12 +71,10 @@ static clockid_t clock_open(char *device)
return CLOCK_INVALID;
}

- fd = open(device, O_RDWR);
- if (fd < 0) {
+ clkid = phc_open(device);
+ if (clkid == CLOCK_INVALID)
fprintf(stderr, "cannot open %s: %m\n", device);
- return CLOCK_INVALID;
- }
- return FD_TO_CLOCKID(fd);
+ return clkid;
}

static int read_phc(clockid_t clkid, clockid_t sysclk, int readings,

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/phc2sys.c b/phc2sys.c
index 4c68f13..cab5c9a 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -715,7 +715,7 @@ int main(int argc, char *argv[])
if (pps_fd >= 0)
return do_pps_loop(&dst_clock, pps_fd, src, phc_readings);

- if (dst_clock.clkid == CLOCK_REALTIME &&
+ if (dst_clock.clkid == CLOCK_REALTIME && src != CLOCK_REALTIME &&
SYSOFF_SUPPORTED == sysoff_probe(CLOCKID_TO_FD(src), phc_readings))
return do_sysoff_loop(&dst_clock, src, phc_rate, phc_readings);

[Miroslav Lichvar]

Reset the STA_UNSYNC flag and maxerror value with every frequency update
to let the kernel synchronize the RTC to the system clock (11 minute mode).

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 2 ++
clockadj.c | 17 +++++++++++++++++
clockadj.h | 5 +++++
phc2sys.c | 2 ++
4 files changed, 26 insertions(+)

diff --git a/clock.c b/clock.c
index cc692e9..eceedc5 100644
--- a/clock.c
+++ b/clock.c
@@ -1070,6 +1070,8 @@ enum servo_state clock_synchronize(struct clock *c,
break;
case SERVO_LOCKED:
clockadj_set_freq(c->clkid, -adj);
+ if (c->clkid == CLOCK_REALTIME)
+ sysclk_set_sync();
break;
}
return state;
diff --git a/clockadj.c b/clockadj.c
index 74963c8..b0505b7 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -25,6 +25,8 @@

#define NS_PER_SEC 1000000000LL

+static int realtime_leap_bit;
+
void clockadj_set_freq(clockid_t clkid, double freq)
{
struct timex tx;
@@ -94,6 +96,7 @@ void sysclk_set_leap(int leap)
pr_err("failed to set the clock status: %m");
else if (m)
pr_notice(m);
+ realtime_leap_bit = tx.status;
}

int sysclk_max_freq(void)
@@ -110,3 +113,17 @@ int sysclk_max_freq(void)
f = 500000;
return f;
}
+
+void sysclk_set_sync(void)
+{
+ clockid_t clkid = CLOCK_REALTIME;
+ struct timex tx;
+ memset(&tx, 0, sizeof(tx));
+ /* Clear the STA_UNSYNC flag from the status and keep the maxerror
+ value (which is increased automatically by 500 ppm) below 16 seconds
+ to avoid getting the STA_UNSYNC flag back. */
+ tx.modes = ADJ_STATUS | ADJ_MAXERROR;
+ tx.status = realtime_leap_bit;
+ if (clock_adjtime(clkid, &tx) < 0)
+ pr_err("failed to set clock status and maximum error: %m");
+}
diff --git a/clockadj.h b/clockadj.h
index 3779f03..79ae80d 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -57,4 +57,9 @@ void sysclk_set_leap(int leap);
*/
int sysclk_max_freq(void);

+/**
+ * Mark the system clock as synchronized to let the kernel synchronize
+ * the real-time clock (RTC) to it.
+ */
+void sysclk_set_sync(void);
#endif
diff --git a/phc2sys.c b/phc2sys.c
index 076840c..93495a4 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -187,6 +187,8 @@ static void update_clock(struct clock *clock,
/* Fall through. */
case SERVO_LOCKED:
clockadj_set_freq(clock->clkid, -ppb);
+ if (clock->clkid == CLOCK_REALTIME)
+ sysclk_set_sync();
break;
}

[Richard Cochran]

Here is the rest of the patches from the previous set reworked for the
clkid parameter removal. It doesn't include the phc2sys -W patch. It
should be applied on top of the "clockadj: make Doxygen comment by
using two stars." and "clockadj: remove useless clockid parameter."
patches posted by Richard.
phc2sys: Read maximum frequency adjustment.
phc2sys: Don't try PTP_SYS_OFFSET with system clock as source.
phc2sys: Use phc_open().
phc2sys: Add option to set domain number.
clockadj: Remove clkid parameter from set_leap function.
Let kernel synchronize RTC to system clock.
Limit maximum frequency adjustment to 90%.

All applied, except for the last one.

I gave it some more thought, and setting an adjustment limit in order
to work around driver flaws simply cannot work. I compiled a list of
the current max_adj values.

1953124 drivers/net/phy/dp83640.c
50000000 drivers/ptp/ptp_pch.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
66666655 drivers/ptp/ptp_ixp46x.c
249999999 p2020ds.dtsi
249999999 p2020rdb-pc.dtsi
249999999 p2020rdb.dts
250000000 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
499999999 mpc8572ds.dtsi
659999998 mpc8313erdb.dts
1000000000 drivers/net/ethernet/intel/igb/igb_ptp.c *

* The value for igb is a bug, and it has been fixed already.

Any one of these current (or future) drivers might have an off-by-one
bug that could cause the clock to stop or to freak out. We have no
possible way of choosing a "safe" maximum of 90% , 75%, 50%, or 25%,
because some buggy driver with a lower maximum will still cause a hang.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran
I gave it some more thought, and setting an adjustment limit in order
to work around driver flaws simply cannot work. I compiled a list of
the current max_adj values.

The patch was not supposed to work around driver bugs. It sets a
practical limit on the clock adjustment.

Post by Richard Cochran
1953124 drivers/net/phy/dp83640.c
50000000 drivers/ptp/ptp_pch.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
66666655 drivers/ptp/ptp_ixp46x.c
249999999 p2020ds.dtsi
249999999 p2020rdb-pc.dtsi
249999999 p2020rdb.dts
250000000 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
499999999 mpc8572ds.dtsi
659999998 mpc8313erdb.dts
1000000000 drivers/net/ethernet/intel/igb/igb_ptp.c *
* The value for igb is a bug, and it has been fixed already.

The new igb value is 999999881, which I think is still way too much to
be used by ptp4l. If one reading of the clock takes 10 microseconds,
the clock will advance only by ~1 picosecond in that time. It will
take ~8.4 milliseconds to see a difference in struct timespec.

Also, what if there will be a new driver implemented which will
correctly support 1e9 ppm or more? I think we need some limit in
ptp4l.

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran
1953124 drivers/net/phy/dp83640.c
50000000 drivers/ptp/ptp_pch.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
66666655 drivers/ptp/ptp_ixp46x.c
249999999 p2020ds.dtsi
249999999 p2020rdb-pc.dtsi
249999999 p2020rdb.dts
250000000 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
499999999 mpc8572ds.dtsi
659999998 mpc8313erdb.dts
1000000000 drivers/net/ethernet/intel/igb/igb_ptp.c *
* The value for igb is a bug, and it has been fixed already.

The new igb value is 999999881, which I think is still way too much to
be used by ptp4l. If one reading of the clock takes 10 microseconds,
the clock will advance only by ~1 picosecond in that time. It will
take ~8.4 milliseconds to see a difference in struct timespec.

True, but allowing 90% is not much better, is it?

Post by Miroslav Lichvar
Also, what if there will be a new driver implemented which will
correctly support 1e9 ppm or more? I think we need some limit in
ptp4l.

(How can you have more than -999999999 ppm offset?)

With the exception of the Intel 82576, the HW listed drivers all are
limiting the servo far below your 90% limit.

Are you concerned that some people don't want their clocks to run at
more that X ppm offset? In that case, a config option would be a
better solution.

Thoughts?

Richard

[Jeroen Van den Keybus]

Post by Richard Cochran
With the exception of the Intel 82576, the HW listed drivers all are
limiting the servo far below your 90% limit.
Are you concerned that some people don't want their clocks to run at
more that X ppm offset? In that case, a config option would be a
better solution.
Thoughts?

I think the main problem is that ptp_clock_info *p->max_adj may be filled
in by clock designers as the max. range the clock actually supports. That
limit may be far beyond a sensible limit during operation, which
essentially should protect against overly large deviations in clock speed.
As such it's part of the PLL design and should be chosen in accordance with
Ki and Kp. I therefore would expect the definition of the limit near
HWTS_KI and friends in pi.c.

As asked, just my thought,

Jeroen.

Post by Richard Cochran
Richard
------------------------------------------------------------------------------
Precog is a next-generation analytics platform capable of advanced
analytics on semi-structured data. The platform includes APIs for building
apps and a phenomenal toolset for data science. Developers can use
our toolset for easy data analysis & visualization. Get a free account!
http://www2.precog.com/precogplatform/slashdotnewsletter
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Keller, Jacob E]

-----Original Message-----
Sent: Tuesday, April 09, 2013 12:20 PM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [PATCHv2 0/7] Misc improvements.

Post by Miroslav Lichvar

Post by Richard Cochran
1953124 drivers/net/phy/dp83640.c
50000000 drivers/ptp/ptp_pch.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
62499999 drivers/net/ethernet/intel/igb/igb_ptp.c
66666655 drivers/ptp/ptp_ixp46x.c
249999999 p2020ds.dtsi
249999999 p2020rdb-pc.dtsi
249999999 p2020rdb.dts
250000000 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
499999999 mpc8572ds.dtsi
659999998 mpc8313erdb.dts
1000000000 drivers/net/ethernet/intel/igb/igb_ptp.c *
* The value for igb is a bug, and it has been fixed already.

The new igb value is 999999881, which I think is still way too much to
be used by ptp4l. If one reading of the clock takes 10 microseconds,
the clock will advance only by ~1 picosecond in that time. It will
take ~8.4 milliseconds to see a difference in struct timespec.

True, but allowing 90% is not much better, is it?

Post by Miroslav Lichvar
Also, what if there will be a new driver implemented which will
correctly support 1e9 ppm or more? I think we need some limit in
ptp4l.

(How can you have more than -999999999 ppm offset?)
With the exception of the Intel 82576, the HW listed drivers all are
limiting the servo far below your 90% limit.
Are you concerned that some people don't want their clocks to run at
more that X ppm offset? In that case, a config option would be a
better solution.
Thoughts?
Richard

I think there is no sensible limit because even if we could freeze the clock, that would be a valid action to take when adjusting the clock (freeze it for the duration of the delay), although we shouldn't ever make the clock go "backwards". (but I don't believe any hardware CAN even support that.)

We could config this but what value is being added?

And being able to increase ppb above 100% would be useful too, if it is supported.

The driver for igb had a bug where the max offset instead of doubling, it did freeze, which is wrong. But going the other way (negative) freezing would be the correct solution. We fixed the igb bug. But I don't think ptp4l can have a useful limit.

- Jake

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
The new igb value is 999999881, which I think is still way too much to
be used by ptp4l. If one reading of the clock takes 10 microseconds,
the clock will advance only by ~1 picosecond in that time. It will
take ~8.4 milliseconds to see a difference in struct timespec.

True, but allowing 90% is not much better, is it?

At -90% the clock is almost million times closer to its nominal speed
than at -99.9999881% and the time needed to correct an offset at the
minimum frequency is only 11% longer. I think that is a good tradeoff.

Post by Richard Cochran
Are you concerned that some people don't want their clocks to run at
more that X ppm offset? In that case, a config option would be a
better solution.

Yes, I think some users have expectations on the clock speed, it
shouldn't run much faster or slower than real time. Running the clock
10 times slower might be acceptable, but running it 10 million times
slower (or stopped) almost certainly is not. BTW, the maximum
adjustment for the system clock is 10% and it's not a technical
limitation.

I like the idea proposed by Jeroen, to move the limit to the servo.
There could be an option setting the limit as multiplication and
division instead of the frequency offset. If set to 10 (that could be
the default value) the servo will use -90% and +900% as the minimum
and maximum offset. Zero could be used to disable the limit. What do
you think?

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
I like the idea proposed by Jeroen, to move the limit to the servo.
There could be an option setting the limit as multiplication and
division instead of the frequency offset. If set to 10 (that could be
the default value) the servo will use -90% and +900% as the minimum
and maximum offset. Zero could be used to disable the limit. What do
you think?

Yes, this should be a servo option just like KI and KP. Maybe the
limit should be expressed in ppb or PPM, since that is typical when
talking about clocks, but I don't have a strong opinion on that.

Would you care to code this up?

Thanks,
Richard

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
default.cfg | 10 +++++-----
gPTP.cfg | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/default.cfg b/default.cfg
index 7cb7566..f9007b1 100644
--- a/default.cfg
+++ b/default.cfg
@@ -40,9 +40,9 @@ kernel_leap 1
#
pi_proportional_const 0.0
pi_integral_const 0.0
-pi_offset_const 0.0
+pi_offset_const 0.0
pi_max_frequency 900000000
-clock_servo pi
+clock_servo pi
#
# Transport options
#
@@ -59,6 +59,6 @@ time_stamping hardware
#
# Clock description
#
-productDescription ;;
-revisionData ;;
-manufacturerIdentity 00:00:00
+productDescription ;;
+revisionData ;;
+manufacturerIdentity 00:00:00
diff --git a/gPTP.cfg b/gPTP.cfg
index 7f27911..ed134ff 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -39,9 +39,9 @@ kernel_leap 1
#
pi_proportional_const 0.0
pi_integral_const 0.0
-pi_offset_const 0.0
+pi_offset_const 0.0
pi_max_frequency 900000000
-clock_servo pi
+clock_servo pi
#
# Transport options
#

[Richard Cochran]

Post by Miroslav Lichvar
---
default.cfg | 10 +++++-----
gPTP.cfg | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)

Applied.

Thanks,
Richard

[Miroslav Lichvar]

The option sets an additional limit to the hardware limit. It's disabled
if set to zero. The default is 900000000 ppb.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 5 +++++
config.h | 1 +
default.cfg | 1 +
gPTP.cfg | 1 +
pi.c | 7 ++++++-
pi.h | 6 ++++++
ptp4l.8 | 6 ++++++
ptp4l.c | 1 +
8 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 08a134f..e056652 100644
--- a/config.c
+++ b/config.c
@@ -290,6 +290,11 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->pi_offset_const = df;

+ } else if (!strcmp(option, "pi_max_frequency")) {
+ if (1 != sscanf(value, "%d", &val) || !(val >= 0))
+ return BAD_VALUE;
+ *cfg->pi_max_frequency = val;
+
} else if (!strcmp(option, "ptp_dst_mac")) {
if (MAC_LEN != sscanf(value, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5]))
diff --git a/config.h b/config.h
index e7f1f50..901b50a 100644
--- a/config.h
+++ b/config.h
@@ -68,6 +68,7 @@ struct config {
double *pi_proportional_const;
double *pi_integral_const;
double *pi_offset_const;
+ int *pi_max_frequency;

unsigned char *ptp_dst_mac;
unsigned char *p2p_dst_mac;
diff --git a/default.cfg b/default.cfg
index 4694e32..7cb7566 100644
--- a/default.cfg
+++ b/default.cfg
@@ -41,6 +41,7 @@ kernel_leap 1
pi_proportional_const 0.0
pi_integral_const 0.0
pi_offset_const 0.0
+pi_max_frequency 900000000
clock_servo pi
#
# Transport options
diff --git a/gPTP.cfg b/gPTP.cfg
index 83a44bf..7f27911 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -40,6 +40,7 @@ kernel_leap 1
pi_proportional_const 0.0
pi_integral_const 0.0
pi_offset_const 0.0
+pi_max_frequency 900000000
clock_servo pi
#
# Transport options
diff --git a/pi.c b/pi.c
index ec7e456..427cb3e 100644
--- a/pi.c
+++ b/pi.c
@@ -31,10 +31,11 @@

#define NSEC_PER_SEC 1000000000

-/* These two take their values from the configuration file. (see ptp4l.c) */
+/* These take their values from the configuration file. (see ptp4l.c) */
double configured_pi_kp = 0.0;
double configured_pi_ki = 0.0;
double configured_pi_offset = 0.0;
+int configured_pi_max_freq = 900000000;

struct pi_servo {
struct servo servo;
@@ -150,5 +151,9 @@ struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts)
s->max_offset = 0.0;
}

+ if (configured_pi_max_freq && s->maxppb > configured_pi_max_freq) {
+ s->maxppb = configured_pi_max_freq;
+ }
+
return &s->servo;
}
diff --git a/pi.h b/pi.h
index eff90aa..954f495 100644
--- a/pi.h
+++ b/pi.h
@@ -42,6 +42,12 @@ extern double configured_pi_ki;
*/
extern double configured_pi_offset;

+/**
+ * When set to a non-zero value, this variable sets an additional limit for
+ * the frequency adjustment of the clock. It's in ppb.
+ */
+extern int configured_pi_max_freq;
+
struct servo *pi_servo_create(int fadj, int max_ppb, int sw_ts);

#endif
diff --git a/ptp4l.8 b/ptp4l.8
index ee9cccd..11b1b6f 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -292,6 +292,12 @@ frequency instead of stepping the clock. When set to 0.0, the controller will
never step the clock.
The default is 0.0.
.TP
+.B pi_max_frequency
+The maximum allowed frequency adjustment of the clock in parts per billion
+(ppb). This is an additional limit to the maximum allowed by the hardware. When
+set to 0, the hardware limit will be used.
+The default is 900000000 (90%).
+.TP
.B ptp_dst_mac
The MAC address where should be PTP messages sent.
Relevant only with L2 transport. The default is 01:1B:19:00:00:00.
diff --git a/ptp4l.c b/ptp4l.c
index 3bf1f86..3792375 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -96,6 +96,7 @@ static struct config cfg_settings = {
.pi_proportional_const = &configured_pi_kp,
.pi_integral_const = &configured_pi_ki,
.pi_offset_const = &configured_pi_offset,
+ .pi_max_frequency = &configured_pi_max_freq,

.ptp_dst_mac = ptp_dst_mac,
.p2p_dst_mac = p2p_dst_mac,

[Richard Cochran]

Post by Miroslav Lichvar
diff --git a/config.c b/config.c
index 08a134f..e056652 100644
--- a/config.c
+++ b/config.c
@@ -290,6 +290,11 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->pi_offset_const = df;
+ } else if (!strcmp(option, "pi_max_frequency")) {
+ if (1 != sscanf(value, "%d", &val) || !(val >= 0))
+ return BAD_VALUE;

Didn't we want to allow zero?

Post by Miroslav Lichvar
+ *cfg->pi_max_frequency = val;
+
} else if (!strcmp(option, "ptp_dst_mac")) {
if (MAC_LEN != sscanf(value, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&mac[0], &mac[1], &mac[2], &mac[3], &mac[4], &mac[5]))

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
diff --git a/config.c b/config.c
index 08a134f..e056652 100644
--- a/config.c
+++ b/config.c
@@ -290,6 +290,11 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->pi_offset_const = df;
+ } else if (!strcmp(option, "pi_max_frequency")) {
+ if (1 != sscanf(value, "%d", &val) || !(val >= 0))
+ return BAD_VALUE;

Didn't we want to allow zero?

We do. I think the code allows it, or not?

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar

Post by Richard Cochran

Post by Miroslav Lichvar
diff --git a/config.c b/config.c
index 08a134f..e056652 100644
--- a/config.c
+++ b/config.c
@@ -290,6 +290,11 @@ static enum parser_result parse_global_setting(const char *option,
return BAD_VALUE;
*cfg->pi_offset_const = df;
+ } else if (!strcmp(option, "pi_max_frequency")) {
+ if (1 != sscanf(value, "%d", &val) || !(val >= 0))
+ return BAD_VALUE;

Didn't we want to allow zero?

We do. I think the code allows it, or not?

Uh, yeah, it does.

Sorry,
Richard

[Richard Cochran]

Post by Richard Cochran

Post by Miroslav Lichvar
We do. I think the code allows it, or not?

Uh, yeah, it does.

But still, the expression could be expressed more simply as:

if (1 != sscanf(value, "%d", &val) || val < 0) {
...
}

Mind if I fix it up like that?

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
Mind if I fix it up like that?

Never mind. I see we have a bunch of similar tests in config.c. I will
fix them in another patch later.

Thanks,
Richard

[Richard Cochran]

Post by Miroslav Lichvar
The option sets an additional limit to the hardware limit. It's disabled
if set to zero. The default is 900000000 ppb.

Applied.

Thanks,
Richard

[Keller, Jacob E]

-----Original Message-----
Sent: Wednesday, April 10, 2013 8:39 AM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [PATCHv2 0/7] Misc improvements.

Post by Miroslav Lichvar
I like the idea proposed by Jeroen, to move the limit to the servo.
There could be an option setting the limit as multiplication and
division instead of the frequency offset. If set to 10 (that could be
the default value) the servo will use -90% and +900% as the minimum
and maximum offset. Zero could be used to disable the limit. What do
you think?

Yes, this should be a servo option just like KI and KP. Maybe the
limit should be expressed in ppb or PPM, since that is typical when
talking about clocks, but I don't have a strong opinion on that.
Would you care to code this up?
Thanks,
Richard

I would prefer ppb/ppm as well as that is what the drivers use, but anything would work. Definitely prefer this as a servo option :)

- Jake