Re: [net-next 03/15] net/mlx5: Add adjphase function to support hardware-only offset control

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Thu, 19 Jan, 2023 20:03:43 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 19 Jan 2023 19:56:24 -0800 Rahul Rameshbabu wrote:
>> >> Makes sense. Once you've verified that the delta is within the accepted
>> >> range you can just re-use the existing adjtime function.  
>> >
>> > Seems like we should add a "max_time_adj" to struct ptp_clock_info
>> > and let the core call adjphase if the offset is small enough to fit.
>> > Instead of having all drivers redirect the calls internally.  
>> 
>> With guidance from Saeed on this topic, I have a patch in the works for
>> advertising the max phase adjustment supported by a driver through the
>> use of the PTP_CLOCK_GETCAPS ioctl. This is how the ptp stack handles
>> advertising the max frequency supported by a driver today. In linuxptp,
>> this ioctl is wrapped in a function call for getting the max frequency
>> adjustment supported by a device before ptp is actually run. I believe a
>> similar logic should occur for phase (time) adjustments. This patch
>> would introduce a "max_phase_adj" in ptp_clock_info that would be
>> handled in ptp_clock_adjtime in the ptp core stack.
>
> Nice, can we make the core also call ->adjtime automatically if driver
> doesn't define ->adjphase and the abs(delta) < .max_phase_adj ?

One of my concerns with doing this is breaking userspace expectations.
In linuxptp, there is a configuration setting "write_phase_mode" and an
expectation that when adjphase is called, there will not be a fallback
to adjtime. This because adjphase is used in situations where small fine
tuning is explicitly needed, so the errors would indicate a logical or
situational error.

Quoting Vincent Cheng, the author of the adjphase functionality in the
ptp core stack.

-----BEGIN QUOTE-----
  adjtime modifies HW counter with a value to move the 1 PPS abruptly to new location.
  adjphase modifies the frequency to quickly nudge the 1 PPS to new location and also includes a HW filter to smooth out the adjustments and fine tune frequency.

  Continuous small offset adjustments using adjtime, likley see sudden shifts of the 1 PPS.  The 1 PPS probably disappears and re-appears.
  Continuous small offset adjustments using adjphase, should see continuous 1 PPS.

  adjtime is good for large offset corrections
  adjphase is good for small offset corrections to allow HW filter to control the frequency instead of relying on SW filter.
-----END QUOTE-----

https://lore.kernel.org/netdev/20220804132902.GA25315@renesas.com/

Using the mlx5 implementation as a reference, the mlx5 stack provides
the adjphase offset to the device. The device is then able to make the
sudden shift small offsets internally using the device's internal
functionality (offload). For the larger values that are handled using
adjtime, our devices perform just fine when we get the time from the
device, offset that time in the driver stack, and write back the new
time to the device.

>
> The other question is about the exact semantics of ->adjphase
> - do all timecounter based clock implementations support it
> by definition?

My understanding is no (though anyone is free to jump in to correct me
on this). Only implementations with support for precisely handling small
PPS corrections can support adjphase (being able to adjust small offsets
without causing same or worse drift).