[Linuxptp-devel] [PATCH 00/10] Misc improvements.

[Miroslav Lichvar]

This is a collection of patches with minor improvements. Mostly it's
about frequency adjustment and phc2sys. There is also a patch for
the -O2 warnings.

Miroslav Lichvar (10):
ptp4l: Set clock frequency on start.
Limit maximum frequency adjustment to 90%.
ptp4l: Read system clock maximum frequency adjustment.
phc2sys: Read maximum frequency adjustment.
phc2sys: Don't try PTP_SYS_OFFSET with system clock as source.
phc2sys: Use phc_open().
Let kernel synchronize RTC to system clock.
phc2sys: Add option to set domain number.
phc2sys: Add option to wait for ptp4l to be slave.
Fix compiler warnings with -O2.

clock.c | 9 ++++++--
clockadj.c | 38 +++++++++++++++++++++++++++++++
clockadj.h | 15 +++++++++++-
config.c | 2 +-
makefile | 5 ++--
phc.c | 7 ++++++
phc2sys.8 | 14 +++++++++++-
phc2sys.c | 77 +++++++++++++++++++++++++++++++++++++++++++-------------------
port.c | 8 ++++---
sk.c | 4 ++--
10 files changed, 144 insertions(+), 35 deletions(-)

[Miroslav Lichvar]

Due to a bug in older kernels, frequency reading may silently fail and
return 0. Set the frequency to the read value to make sure the servo
has the actual frequency of the clock.

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

diff --git a/clock.c b/clock.c
index 1f55276..44ef81a 100644
--- a/clock.c
+++ b/clock.c
@@ -597,6 +597,10 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,

if (c->clkid != CLOCK_INVALID) {
fadj = (int) clockadj_get_freq(c->clkid);
+ /* Due to a bug in older kernels, the reading may silently fail
+ and return 0. Set the frequency back to make sure fadj is
+ the actual frequency of the clock. */
+ clockadj_set_freq(c->clkid, fadj);
}
c->servo = servo_create(servo, -fadj, max_adj, sw_ts);
if (!c->servo) {

[Richard Cochran]

Post by Miroslav Lichvar
Due to a bug in older kernels, frequency reading may silently fail and
return 0. Set the frequency to the read value to make sure the servo
has the actual frequency of the clock.

Applied.

Thanks,
Richard

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
clock.c | 2 +-
clockadj.c | 14 ++++++++++++++
clockadj.h | 7 +++++++
3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 44ef81a..e4af8f4 100644
--- a/clock.c
+++ b/clock.c
@@ -589,7 +589,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
} else {
c->clkid = CLOCK_REALTIME;
c->utc_timescale = 1;
- max_adj = 512000;
+ max_adj = clockadj_get_max_freq(c->clkid);
clockadj_set_leap(c->clkid, 0);
}
c->leap_set = 0;
diff --git a/clockadj.c b/clockadj.c
index 31fc765..7587dda 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -94,3 +94,17 @@ void clockadj_set_leap(clockid_t clkid, int leap)
else if (m)
pr_notice(m);
}
+
+int clockadj_get_max_freq(clockid_t clkid)
+{
+ int f = 0;
+ struct timex tx;
+ memset(&tx, 0, sizeof(tx));
+ if (clock_adjtime(clkid, &tx) < 0)
+ pr_err("failed to read out the clock maximum adjustment: %m");
+ else
+ f = tx.tolerance / 65.536;
+ if (!f)
+ f = 500000;
+ return f;
+}
diff --git a/clockadj.h b/clockadj.h
index 6e76e0f..dc94cfa 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -51,4 +51,11 @@ void clockadj_step(clockid_t clkid, int64_t step);
* 0 to reset the leap state.
*/
void clockadj_set_leap(clockid_t clkid, int leap);
+
+/*
+ * Read clock's maximum frequency adjustment.
+ * @param clkid CLOCK_REALTIME.
+ * @return The maximum frequency adjustment in parts per billion (ppb).
+ */
+int clockadj_get_max_freq(clockid_t clkid);
#endif

[Richard Cochran]

Post by Miroslav Lichvar
---
clock.c | 2 +-
clockadj.c | 14 ++++++++++++++
clockadj.h | 7 +++++++
3 files changed, 22 insertions(+), 1 deletion(-)

Applied.

Thanks,
Richard

[Richard Cochran]

Miroslav,

I applied this, but on second thought I don't like it.

In this series, there are a bunch of functions that take a clkid, but
you can only pass CLOCK_REALTIME. That is silly. Please remove the
argument and let the callers check for clkid==CLOCK_REALTIME (in some
cases you are doing this already).

[ If you want to keep the argument because you expect that the
operation may one day work on all clkids, then please add a comment
to this effect. ]

Patches 6, 8, and 9 all look okay to me as is, but they don't apply
when skipping the others.

Thanks,
Richard

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
clockadj.h | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/clockadj.h b/clockadj.h
index dc94cfa..5b91d40 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -23,28 +23,28 @@
#include <inttypes.h>
#include <time.h>

-/*
+/**
* Set clock's frequency offset.
* @param clkid A clock ID obtained using phc_open() or CLOCK_REALTIME.
* @param freq The frequency offset in parts per billion (ppb).
*/
void clockadj_set_freq(clockid_t clkid, double freq);

-/*
+/**
* Read clock's frequency offset.
* @param clkid A clock ID obtained using phc_open() or CLOCK_REALTIME.
* @return The frequency offset in parts per billion (ppb).
*/
double clockadj_get_freq(clockid_t clkid);

-/*
+/**
* Step clock's time.
* @param clkid A clock ID obtained using phc_open() or CLOCK_REALTIME.
* @param step The time step in nanoseconds.
*/
void clockadj_step(clockid_t clkid, int64_t step);

-/*
+/**
* Insert/delete leap second at midnight.
* @param clkid CLOCK_REALTIME.
* @param leap +1 to insert leap second, -1 to delete leap second,
@@ -52,7 +52,7 @@ void clockadj_step(clockid_t clkid, int64_t step);
*/
void clockadj_set_leap(clockid_t clkid, int leap);

-/*
+/**
* Read clock's maximum frequency adjustment.
* @param clkid CLOCK_REALTIME.
* @return The maximum frequency adjustment in parts per billion (ppb).

[Richard Cochran]

The clockid parameter to the function to get the system clock's maximum
adjustment is redundant, so let us just remove it.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 2 +-
clockadj.c | 3 ++-
clockadj.h | 8 ++++----
3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index bd22de5..c83ec76 100644
--- a/clock.c
+++ b/clock.c
@@ -589,7 +589,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
} else {
c->clkid = CLOCK_REALTIME;
c->utc_timescale = 1;
- max_adj = clockadj_get_max_freq(c->clkid);
+ max_adj = sysclk_max_freq();
clockadj_set_leap(c->clkid, 0);
}
c->leap_set = 0;
diff --git a/clockadj.c b/clockadj.c
index 7587dda..870ee61 100644
--- a/clockadj.c
+++ b/clockadj.c
@@ -95,8 +95,9 @@ void clockadj_set_leap(clockid_t clkid, int leap)
pr_notice(m);
}

-int clockadj_get_max_freq(clockid_t clkid)
+int sysclk_max_freq(void)
{
+ clockid_t clkid = CLOCK_REALTIME;
int f = 0;
struct timex tx;
memset(&tx, 0, sizeof(tx));
diff --git a/clockadj.h b/clockadj.h
index 5b91d40..d9a8a65 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -53,9 +53,9 @@ void clockadj_step(clockid_t clkid, int64_t step);
void clockadj_set_leap(clockid_t clkid, int leap);

/**
- * Read clock's maximum frequency adjustment.
- * @param clkid CLOCK_REALTIME.
- * @return The maximum frequency adjustment in parts per billion (ppb).
+ * Read maximum frequency adjustment of the system clock (CLOCK_REALTIME).
+ * @return The maximum frequency adjustment in parts per billion (ppb).
*/
-int clockadj_get_max_freq(clockid_t clkid);
+int sysclk_max_freq(void);
+
#endif

[Miroslav Lichvar]

Post by Richard Cochran
The clockid parameter to the function to get the system clock's maximum
adjustment is redundant, so let us just remove it.

I'm ok with both patches. I'll rework the rest of my patchset.

Thanks,

[Miroslav Lichvar]

Post by Richard Cochran
Miroslav,
I applied this, but on second thought I don't like it.
In this series, there are a bunch of functions that take a clkid, but
you can only pass CLOCK_REALTIME. That is silly. Please remove the
argument and let the callers check for clkid==CLOCK_REALTIME (in some
cases you are doing this already).
[ If you want to keep the argument because you expect that the
operation may one day work on all clkids, then please add a comment
to this effect. ]

Yes, I wanted to keep the clock argument in case PHC will support it.
I'm not sure how likely that is and if we would ever use it.

In the leap second case, the caller checks if the clock is
CLOCK_REALTIME because we want to insert/delete leap second only in
the system clock, not because the PHCs don't support it.

In the case with reading max frequency, would it make sense to merge
the function with phc_max_adj() and not check for CLOCK_REALTIME in
the caller?

Thanks,

[Richard Cochran]

Post by Miroslav Lichvar
In the leap second case, the caller checks if the clock is
CLOCK_REALTIME because we want to insert/delete leap second only in
the system clock, not because the PHCs don't support it.

If the caller checks the ID, then the function should not have to as
well. Leap seconds do not have any meaning for a PHC, so I think the
function should not have a clockid parameter at all.

Post by Miroslav Lichvar
In the case with reading max frequency, would it make sense to merge
the function with phc_max_adj() and not check for CLOCK_REALTIME in
the caller?

The phc_ functions are only about PHC devices, so it doesn't make
sense to me. We already need to treat the system clock as a special
case, so it is not so bad to have two different ways to get max_adj.

Thanks,
Richard

[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 d210df8..0628da1 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)

@@ -541,7 +540,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 = {
@@ -699,6 +698,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 = clockadj_get_max_freq(dst_clock.clkid);
+ } 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>
---
phc2sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/phc2sys.c b/phc2sys.c
index 0628da1..d63ef50 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -713,7 +713,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]

Apparently, drivers may allow frequency offset of -1.0, which stops the
clock. Limit the frequency offset to 90% so the clock can run at most
ten times slower.

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;
}

[Keller, Jacob E]

-----Original Message-----
Sent: Thursday, April 04, 2013 8:28 AM
Subject: [Linuxptp-devel] [PATCH 02/10] Limit maximum frequency
adjustment to 90%.
Apparently, drivers may allow frequency offset of -1.0, which stops the
clock. Limit the frequency offset to 90% so the clock can run at most
ten times slower.
---
phc.c | 7 +++++++
1 file changed, 7 insertions(+)

What does this serve? If the driver *actually* supports the higher PPB, then it should support it correctly. If it does not, that is a bug against the driver, not against ptp4l. PPB should always be adjustment against the base..

I would make it higher actually, as high as you can go without stopping, as this would make it slow the clock faster which is good...

What devices even support that large of a negative offset? (and btw, it should only do this in the negative direction)

[Richard Cochran]

Post by Keller, Jacob E
What devices even support that large of a negative offset?

Miroslav, are you trying to fix the driver for the Intel 82576?
A fix for this has already been accepted.

http://patchwork.ozlabs.org/patch/229087/

I agree that the drivers should be fixed. The application cannot
second-guess what the driver really supports.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Keller, Jacob E
What devices even support that large of a negative offset?

Miroslav, are you trying to fix the driver for the Intel 82576?
A fix for this has already been accepted.
http://patchwork.ozlabs.org/patch/229087/

That patch fixes clock stopping (incorrectly) with +1.0 offset. The
patch I'm proposing here is to prevent applications seeing the clock
as stopped with offsets close to -1.0.

The minimum frequency offset which will always result in two different
consecutive readings is probably specific to the driver/hw and it
depends on the time it takes to read the clock and resolution of the
clock. To me, 90% seemed like a safe value large enough that the
difference between the times it takes to correct an offset at -1.0 and
-0.9 (or +0.9 and +1.0) frequency offset is insignificant.

I'm not really familiar with the hw, if you think 99% or 99.9% is
still safe, I have no problem with changing it.

If there will be a driver which allows even larger adjustments (up to
~400% allowed by the int value), we can change the code to work with
both minimum and maximum frequency offset to allow the clock to run
faster.

Post by Richard Cochran
I agree that the drivers should be fixed. The application cannot
second-guess what the driver really supports.

Agreed. The driver should give a value which it can correctly handle,
even if it means stopped or backward running clock.

Thanks,

[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 | 1 +
clockadj.c | 24 ++++++++++++++++++++++++
clockadj.h | 8 +++++++-
phc2sys.c | 1 +
4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index e4af8f4..89d59b7 100644
--- a/clock.c
+++ b/clock.c
@@ -1067,6 +1067,7 @@ enum servo_state clock_synchronize(struct clock *c,
break;
case SERVO_LOCKED:
clockadj_set_freq(c->clkid, -adj);
+ clockadj_set_sync(c->clkid);
break;
}
return state;
diff --git a/clockadj.c b/clockadj.c
index 7587dda..a286b01 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;
@@ -75,6 +77,10 @@ void clockadj_set_leap(clockid_t clkid, int leap)
{
struct timex tx;
const char *m = NULL;
+
+ if (clkid != CLOCK_REALTIME)
+ return;
+
memset(&tx, 0, sizeof(tx));
tx.modes = ADJ_STATUS;
switch (leap) {
@@ -93,6 +99,7 @@ void clockadj_set_leap(clockid_t clkid, int leap)
pr_err("failed to set the clock status: %m");
else if (m)
pr_notice(m);
+ realtime_leap_bit = tx.status;
}

int clockadj_get_max_freq(clockid_t clkid)
@@ -108,3 +115,20 @@ int clockadj_get_max_freq(clockid_t clkid)
f = 500000;
return f;
}
+
+void clockadj_set_sync(clockid_t clkid)
+{
+ struct timex tx;
+
+ if (clkid != CLOCK_REALTIME)
+ return;
+
+ 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 dc94cfa..ab5f6bb 100644
--- a/clockadj.h
+++ b/clockadj.h
@@ -46,7 +46,7 @@ void clockadj_step(clockid_t clkid, int64_t step);

/*
* Insert/delete leap second at midnight.
- * @param clkid CLOCK_REALTIME.
+ * @param clkid A clock ID. If not CLOCK_REALTIME, the call does nothing.
* @param leap +1 to insert leap second, -1 to delete leap second,
* 0 to reset the leap state.
*/
@@ -58,4 +58,10 @@ void clockadj_set_leap(clockid_t clkid, int leap);
* @return The maximum frequency adjustment in parts per billion (ppb).
*/
int clockadj_get_max_freq(clockid_t clkid);
+
+/*
+ * Mark clock as synchronized to let the kernel synchronize the RTC to it.
+ * @param clkid A clock ID. If not CLOCK_REALTIME, the call does nothing.
+ */
+void clockadj_set_sync(clockid_t clkid);
#endif
diff --git a/phc2sys.c b/phc2sys.c
index 129df73..01ad98c 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -187,6 +187,7 @@ static void update_clock(struct clock *clock,
/* Fall through. */
case SERVO_LOCKED:
clockadj_set_freq(clock->clkid, -ppb);
+ clockadj_set_sync(clock->clkid);
break;
}

[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 01ad98c..dae5999 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -336,9 +336,10 @@ static void *get_mgt_data(struct ptp_message *msg)
return ((struct management_tlv *) msg->management.suffix)->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;
@@ -524,6 +525,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"
@@ -538,7 +540,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;
@@ -555,7 +557,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);
@@ -600,6 +602,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;
@@ -663,7 +668,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]

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 d63ef50..129df73 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]

This post might be inappropriate. Click to display it.

[Richard Cochran]

Post by Miroslav Lichvar
---
clock.c | 2 +-
config.c | 2 +-
phc2sys.c | 6 ++++--
port.c | 8 +++++---
sk.c | 4 ++--
5 files changed, 13 insertions(+), 9 deletions(-)

Applied.

Thanks,
Richard

[Miroslav Lichvar]

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

diff --git a/phc2sys.8 b/phc2sys.8
index f5f72b4..ad8855a 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -30,6 +30,8 @@ phc2sys \- synchronize two clocks
.BI \-u " summary-updates"
] [
.B \-w
+|
+.B \-W
] [
.BI \-n " domain-number"
] [
@@ -125,12 +127,17 @@ statistics. The messages are printed at the LOG_INFO level.
The default is 0 (disabled).
.TP
.B \-w
-Wait until ptp4l is in a synchronized state. If the
+Wait until ptp4l is in a synchronized state (master or slave). If the
.B \-O
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
+.B \-W
+Similar to
+.BR \-w ,
+but wait until ptp4l is slave to another clock.
+.TP
.BI \-n " domain-number"
Specify the domain number used by ptp4l. The default is 0.
.TP
diff --git a/phc2sys.c b/phc2sys.c
index dae5999..8942d5a 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -56,6 +56,12 @@
#define PHC_PPS_OFFSET_LIMIT 10000000
#define PMC_UPDATE_INTERVAL (60 * NS_PER_SEC)

+enum wait_state {
+ WS_ANY = 0,
+ WS_SLAVE_OR_MASTER,
+ WS_SLAVE
+};
+
struct clock;
static int update_sync_offset(struct clock *clock, int64_t offset, uint64_t ts);

@@ -349,7 +355,7 @@ static int init_pmc(struct clock *clock, int domain_number)
}

static int run_pmc(struct clock *clock, int timeout,
- int wait_sync, int get_utc_offset)
+ enum wait_state wait_state, int get_utc_offset)
{
struct ptp_message *msg;
void *data;
@@ -365,7 +371,7 @@ static int run_pmc(struct clock *clock, int timeout,
while (clock->pmc_ds_idx < N_ID) {
/* Check if the data set is really needed. */
if ((ds_ids[clock->pmc_ds_idx] == PORT_DATA_SET &&
- !wait_sync) ||
+ !wait_state) ||
(ds_ids[clock->pmc_ds_idx] == TIME_PROPERTIES_DATA_SET &&
!get_utc_offset)) {
clock->pmc_ds_idx++;
@@ -417,8 +423,13 @@ static int run_pmc(struct clock *clock, int timeout,
case PORT_DATA_SET:
switch (((struct portDS *)data)->portState) {
case PS_MASTER:
+ if (wait_state == WS_SLAVE_OR_MASTER)
+ ds_done = 1;
+ break;
case PS_SLAVE:
- ds_done = 1;
+ if (wait_state == WS_SLAVE_OR_MASTER ||
+ wait_state == WS_SLAVE)
+ ds_done = 1;
break;
}

@@ -524,7 +535,8 @@ static void usage(char *progname)
" -N [num] number of master clock readings per update (5)\n"
" -O [offset] slave-master time offset (0)\n"
" -u [num] number of clock updates in summary stats (0)\n"
- " -w wait for ptp4l\n"
+ " -w wait for ptp4l to be slave or master\n"
+ " -W wait for ptp4l to be slave\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"
@@ -540,8 +552,9 @@ int main(int argc, char *argv[])
{
char *progname, *ethdev = NULL;
clockid_t src = CLOCK_INVALID;
+ enum wait_state wait_state = WS_ANY;
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 max_ppb, r, forced_sync_offset = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
double ppb;
struct clock dst_clock = {
@@ -557,7 +570,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:wn:xl:mqv"))) {
+ "c:d:hs:P:I:S:R:N:O:i:u:wWn:xl:mqv"))) {
switch (c) {
case 'c':
dst_clock.clkid = clock_open(optarg);
@@ -600,7 +613,10 @@ int main(int argc, char *argv[])
dst_clock.stats_max_count = atoi(optarg);
break;
case 'w':
- wait_sync = 1;
+ wait_state = WS_SLAVE_OR_MASTER;
+ break;
+ case 'W':
+ wait_state = WS_SLAVE;
break;
case 'n':
domain_number = atoi(optarg);
@@ -667,13 +683,13 @@ int main(int argc, char *argv[])
print_set_syslog(use_syslog);
print_set_level(print_level);

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

while (1) {
r = run_pmc(&dst_clock, 1000,
- wait_sync, !forced_sync_offset);
+ wait_state, !forced_sync_offset);
if (r < 0)
return -1;
else if (r > 0)

[Miroslav Lichvar]

Post by Miroslav Lichvar
---
phc2sys.8 | 9 ++++++++-
phc2sys.c | 34 +++++++++++++++++++++++++---------
2 files changed, 33 insertions(+), 10 deletions(-)

This patch may not be as useful as I originally thought. With ordinary
clock, using the slaveOnly option will be probably preferred. With
boundary clock, I guess it could be useful, but I'm not sure if it's
worth it.