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 21:08:42 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 19 Jan 2023 20:26:04 -0800 Rahul Rameshbabu wrote:
>> 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.
>
> I don't mean fallback - just do what you do in mlx5 directly in 
> the core. The driver already does:
>
> if delta < MAX
> 	use precise method
> else
> 	use coarse method

Oh, I see. I was thinking we were discussing ADJ_OFFSET in the
ptp_clock_adjtime function in the ptp core stack. The suggestion you are
proposing would be for the ADJ_SETOFFSET operation, and I agree with
this. Thanks for the clarification.

>
>> 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.
>
> Hm, so are you saying that:
>
> adjtime(delta):
> 	clock += delta
>
> but:
>
> adjfreq(delta):

Did you mean adjphase here?

> 	on clock tick & while delta > 0:
> 		clock += small_value
> 		delta -= small_value
>
> because from looking at mlx5 driver code its unclear whether the
> implementation does a precise but one shot adjustment or gradual
> adjustments.

The pseudo code your drafted is accurate otherwise. The lack of clarity
in our driver comes from leaving the responsibility of that smooth
gradual transition (to keep in sync with the clock frequency while
running) up to the device.