Re: [PATCH v2] examples/ptpclient: compensate for clock frequency drift

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 14 Oct 2026 03:31:45 +0530
Soumyadeep Hore <soumyadeep.hore@intel.com> wrote:

> The ptpclient example corrected only the instantaneous phase offset by
> calling rte_eth_timesync_adjust_time() once per DELAY_RESP. It never
> compensated for oscillator frequency drift, so the transmitter/receiver
> offset settled at a non-zero steady-state value (proportional to the
> drift rate times the sync interval) instead of converging towards zero.
> 
> Replace the one-shot phase step with a PI clock servo, using the same
> model as linuxptp. The servo output is a frequency correction that is
> applied through the best mechanism the PMD provides:
> 
> - When the driver supports hardware frequency adjustment, the correction
>   is applied with rte_eth_timesync_adjust_freq() (for example the E810
>   INCVAL register).
> - Otherwise the same correction is realized in software as an
>   incremental phase step over the elapsed interval using
>   rte_eth_timesync_adjust_time(), which every timesync-capable PMD
>   provides.
> 
> Hardware support is probed once at runtime, so any PMD that implements
> the adjust_freq operation in the future uses the hardware path without
> further changes. The servo state is kept per port and the selected path
> is logged once per port.
> 
> Bugzilla ID: 1474
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
> v2:
>  - Use transmitter/receiver instead of master/slave terminology

More detail AI review.

Review: [PATCH] examples/ptpclient: compensate for clock frequency drift

Builds clean with -Werror. Sign convention checked: delta_eval()
returns master - slave, so feeding it to the servo with a positive
output applied as +ppb is correct (linuxptp negates because its
offset is slave - master).

Warnings
--------

1. Integral term has no limit.

   drift_ppb accumulates KI * offset forever. linuxptp clamps the
   drift and the output to max_frequency (900000 ppb) because the
   hardware range is finite; ice turns an unbounded ppm into an
   incval that can run off the end. Clamp after the update:

	#define PTP_SERVO_MAX_PPB	900000.0
	...
	if (servo->drift_ppb > PTP_SERVO_MAX_PPB)
		servo->drift_ppb = PTP_SERVO_MAX_PPB;
	else if (servo->drift_ppb < -PTP_SERVO_MAX_PPB)
		servo->drift_ppb = -PTP_SERVO_MAX_PPB;

   and the same for ppb before applying it.

2. Gains assume a 1 s sync interval.

   KP = 0.7 and KI = 0.3 are linuxptp's values for sync interval
   1 s; linuxptp scales them as kp * interval^-0.3 and
   ki * interval^0.4 for other rates. Here both the integral update
   and the slope seed run once per DELAY_RESP with no time scaling,
   so at -3 or -4 log sync interval the integral runs 8-16x too
   fast and the loop is under-damped. The SYNC header carries
   logMessageInterval; use it:

	int8_t lmi = ptp_msg->sync.hdr.log_message_interval;
	double interval = ldexp(1.0, lmi);
	servo->kp = PTP_SERVO_KP_SCALE * pow(interval, -0.3);
	servo->ki = PTP_SERVO_KI_SCALE * pow(interval, 0.4);

   Or at minimum document that the constants are only correct for
   1 s sync.

3. Step branch leaves last_local pointing before the step.

   local is read before rte_eth_timesync_adjust_time(), so the next
   interval is measured as elapsed + step. On the first sync the
   NIC clock is usually at 0, so the step is ~56 years and the
   slope seed in case 1 comes out as 0. For a negative step larger
   than the elapsed time, local <= last_local and the software
   correction is silently skipped that sample. Account for the
   step:

	rte_eth_timesync_adjust_time(ptp->portid, offset);
	servo->last_offset = 0;
	servo->last_local = local + offset;

4. Software path does not remove the drift.

   In software mode the correction is a phase step applied at the
   end of each interval, so the clock free-runs for the whole
   interval and is pulled back afterwards. This moves the sawtooth
   so that the offset measured at sample time converges to 0, but
   the peak error is still drift * interval, same as before the
   patch. The commit message claims the frequency correction is
   "realized" in software; say what it actually does, otherwise
   users on PMDs without adjust_freq will expect a flat offset and
   not get one.

5. Documentation not updated.

   doc/guides/sample_app_ug/ptpclient.rst still describes the
   adjustment as a one-shot adj = -[(T2-T1)-(T4-T3)]/2 and "we
   adjust the PTP time receiver clock". Add a short paragraph on
   the PI servo, the step threshold and the hardware/software
   selection.

Info
----

6. The "\n" added to the print_clock_info() header line is an
   unrelated change; either mention it in the commit message or
   drop it.

7. Probing adjust_freq inside the servo and initialising
   freq_adj_supported = -1 over all RTE_MAX_ETHPORTS in lcore_main()
   is roundabout. Probe in port_init() where timesync is enabled,
   which also removes the tri-state int:

	ret = rte_eth_timesync_adjust_freq(port, 0);
	ptp_servo[port].freq_adj_hw = (ret == 0);

8. Double blank line after the ptp_servo[] definition.