Re: [Linuxptp-devel] [PATCH] servo: stop rounding initial frequency to nearest ppb

2022-11-16 Thread Keller, Jacob E



> -Original Message-
> From: Miroslav Lichvar 
> Sent: Wednesday, November 16, 2022 4:13 AM
> To: Keller, Jacob E 
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH] servo: stop rounding initial frequency 
> to
> nearest ppb
> 
> On Tue, Nov 15, 2022 at 04:41:09PM -0800, Jacob Keller wrote:
> > Refactor the servo_create API to take a double value instead of an integer
> > value. Fix the clockadj_get_freq and clockadj_set_freq initialization to
> > avoid casting to an integer and thus rounding the value.
> 
> Looks good to me. Thanks for submitting the patch.
> 
> I think this patch conflicts with the clockcheck patchset. I'll update
> it and resend if this one is accepted earlier.
> 
> --
> Miroslav Lichvar

Ah right, you refactored to drop a bunch of the code this modifies.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH] servo: stop rounding initial frequency to nearest ppb

2022-11-16 Thread Miroslav Lichvar
On Tue, Nov 15, 2022 at 04:41:09PM -0800, Jacob Keller wrote:
> Refactor the servo_create API to take a double value instead of an integer
> value. Fix the clockadj_get_freq and clockadj_set_freq initialization to
> avoid casting to an integer and thus rounding the value.

Looks good to me. Thanks for submitting the patch.

I think this patch conflicts with the clockcheck patchset. I'll update
it and resend if this one is accepted earlier.

-- 
Miroslav Lichvar



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


[Linuxptp-devel] [PATCH] servo: stop rounding initial frequency to nearest ppb

2022-11-15 Thread Jacob Keller
The interface to the Linux kernel for adjusting clock frequencies is
specified in scaled parts per million, where the frequency field is parts
per million with a 16 bit binary fractional field. (Equivalently it is
parts per ~65 billion).

The frequency adjustment is stored internally as a double, even though when
displaying we always display only a whole number adjustment. Thus ptp4l and
phc2sys already use the full range of frequency adjustments that can be
specified using a double type.

However, when initializing the servo we read the current clock frequency
and cast it to an integer, discarding the fractional part below a part per
billion.

Refactor the servo_create API to take a double value instead of an integer
value. Fix the clockadj_get_freq and clockadj_set_freq initialization to
avoid casting to an integer and thus rounding the value.

Signed-off-by: Jacob Keller 
---
Noticed this while investigating whether we supported the full range of the
frequency adjustment.


 clock.c  | 10 ++
 linreg.c |  2 +-
 linreg.h |  2 +-
 pi.c |  2 +-
 pi.h |  2 +-
 servo.c  |  2 +-
 servo.h  |  2 +-
 ts2phc.c |  5 +++--
 8 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/clock.c b/clock.c
index d37bb87b6c03..7decd836feda 100644
--- a/clock.c
+++ b/clock.c
@@ -897,10 +897,11 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
enum servo_type servo = config_get_int(config, NULL, "clock_servo");
char ts_label[IF_NAMESIZE], phc[32], *tmp;
enum timestamp_type timestamping;
-   int fadj = 0, max_adj = 0, sw_ts;
int phc_index, conf_phc_index, required_modes = 0;
struct clock *c = _clock;
+   int max_adj = 0, sw_ts;
const char *uds_ifname;
+   double fadj = 0.0;
struct port *p;
unsigned char oui[OUI_LEN];
struct interface *iface;
@@ -1143,7 +1144,7 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
c->time_flags = c->utc_timescale ? 0 : PTP_TIMESCALE;
 
if (c->clkid != CLOCK_INVALID) {
-   fadj = (int) clockadj_get_freq(c->clkid);
+   fadj = 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. */
@@ -1738,9 +1739,10 @@ struct tsproc *clock_get_tsproc(struct clock *c)
 int clock_switch_phc(struct clock *c, int phc_index)
 {
struct servo *servo;
-   int fadj, max_adj;
clockid_t clkid;
char phc[32];
+   double fadj;
+   int max_adj;
 
snprintf(phc, sizeof(phc), "/dev/ptp%d", phc_index);
clkid = phc_open(phc);
@@ -1754,7 +1756,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
phc_close(clkid);
return -1;
}
-   fadj = (int) clockadj_get_freq(clkid);
+   fadj = clockadj_get_freq(clkid);
clockadj_set_freq(clkid, fadj);
servo = servo_create(c->config, c->servo_type, -fadj, max_adj, 0);
if (!servo) {
diff --git a/linreg.c b/linreg.c
index 8f354f4b0d0b..363636e16937 100644
--- a/linreg.c
+++ b/linreg.c
@@ -330,7 +330,7 @@ static void linreg_leap(struct servo *servo, int leap)
s->leap = leap;
 }
 
-struct servo *linreg_servo_create(int fadj)
+struct servo *linreg_servo_create(double fadj)
 {
struct linreg_servo *s;
 
diff --git a/linreg.h b/linreg.h
index 5c86ea76ebf2..3c48ce5d465a 100644
--- a/linreg.h
+++ b/linreg.h
@@ -21,6 +21,6 @@
 
 #include "servo.h"
 
-struct servo *linreg_servo_create(int fadj);
+struct servo *linreg_servo_create(double fadj);
 
 #endif
diff --git a/pi.c b/pi.c
index bfe50223d96a..4a96a5b1913b 100644
--- a/pi.c
+++ b/pi.c
@@ -177,7 +177,7 @@ static void pi_reset(struct servo *servo)
s->count = 0;
 }
 
-struct servo *pi_servo_create(struct config *cfg, int fadj, int sw_ts)
+struct servo *pi_servo_create(struct config *cfg, double fadj, int sw_ts)
 {
struct pi_servo *s;
 
diff --git a/pi.h b/pi.h
index feb3ebeb25b9..1e7eb4f3fd51 100644
--- a/pi.h
+++ b/pi.h
@@ -21,6 +21,6 @@
 
 #include "servo.h"
 
-struct servo *pi_servo_create(struct config *cfg, int fadj, int sw_ts);
+struct servo *pi_servo_create(struct config *cfg, double fadj, int sw_ts);
 
 #endif
diff --git a/servo.c b/servo.c
index 46042aa176d9..6ba7cb42c71e 100644
--- a/servo.c
+++ b/servo.c
@@ -31,7 +31,7 @@
 #define NSEC_PER_SEC 10
 
 struct servo *servo_create(struct config *cfg, enum servo_type type,
-  int fadj, int max_ppb, int sw_ts)
+  double fadj, int max_ppb, int sw_ts)
 {
double servo_first_step_threshold;
double servo_step_threshold;
diff --git a/servo.h b/servo.h
index 6c30d3341fa6..6cedb66ada03 100644
--- a/servo.h
+++ b/servo.h
@@ -77,7 +77,7 @@ enum servo_state {
  * @return A pointer to a new servo on success, NULL