On Wed, 14 Oct 2026 03:31:45 +0530 Soumyadeep Hore <[email protected]> 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 <[email protected]> > --- > 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.
