Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 6/7/2023 9:19 AM, Su, Simei wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Saturday, June 3, 2023 3:44 AM
>> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
>> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
>> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
>> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync
>>
>> On 5/22/2023 2:23 PM, Simei Su wrote:
>>> This patchset cover below parts:
>>> (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq"
>> that
>>>    enables frequency adjustment during PTP timesync. This new API aligns
>> with
>>>    the kernel PTP which already supports frequency adjustment. It brings
>> DPDK
>>>    closer in alignment with the kernel's best practice.
>>>
>>
>> Hi Simei,
>>
>> I am not expert on PTP, so please help me understand,
>>
>> I can see there is a new API to update device frequency by updating device
>> clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
>> (Although I have some reservation with the API name, we can visit it later.)
> 
> Yes, this API is to adjust the frequency in a finer granularity, so naming it as 'rte_eth_timesync_adjust_fine()'.
> 

Can existing 'rte_eth_timesync_adjust_time()' API also used to adjust
the device clock frequency?

If so, since 'fine' is subjective, how user can decide to use one or
other, and why we can't use existing API?

>>
>> PTP doesn't enforce any specific sync method, right?
>> Like adjusting by adding/subtracting or adjusting by updating clock frequency.
>> As far as I understand updating device clock frequency is HW capability to
>> adjust time, perhaps in a finer grain, is this correct?
> 
> Yes, it's correct.
> 
>>
>> If so, is this HW capability should be exposed to user with a new API?
>> If this is a generic HW capability, I think it is OK to have single responsibility
>> API, but if this is a vendor specific feature it should be behind a generic API.
> 
> It's a generic HW capability instead of vendor specific feature.
> 
>>
>>
>> Or should we hide this capability behind existing
>> 'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
>> method for synchronization, so this can be transparent the user.
>> What is the benefit to make user selected explicitly one option or other?
>>
> 
> In general, 'rte_eth_timesync_adjust_fine()' is used in conjunction with
> 'rte_eth_timesync_adjust_time()' at different moment to do more precise time control.
> 

If I understand this is not defined by PTP control, so how user can do
distinction between two?

Or same argument again, can user call same existing API and this detail
can be hidden behind implementation? Meaning driver/HW selects that
moment to call one or other.

>>
>>
>>> (2)Refine the ptpclient application by applying a PI algorithm that leverages
>>>    the new API to further improve timesync accuracy. These changes
>> doesn't break
>>>    original solution and creates a more accurate solution for DPDK-based
>> high
>>>    accuracy PTP. We have provided significant improvements for timesync
>> accuracy
>>>    on e810 and we believe these improvements will also benefit other
>> devices.
>>>
>>
>> What is "PI algorithm"? and what PI stands for?
> 
> Proportional-integral-derivative control is called PID controller. It is widely used in industrial control.
> The control deviation is constituted based on the given value and the actual output value.
> The deviation produces control quantity based on proportional, integrated and differentiated linear combination, thereby controlling the controlled object.
> 
> 'P' means proportional controller. It changes the magnitude only.
> 'I' means integral controller. It lags the output phase.
> 'PI' means proportional Integral controller. It changes the magnitude as well as laging the output.
> 
> Proportional-Integral(PI) controller is used to produce a fractional tick-rate adjustment that coordinates the local clock with 
> master clock time. The PI controller corrects both the time and rate of the local clock. The proportional term tracks and 
> corrects the direct input, which is the time difference between two clocks. The integral term tracks and corrects steady-state 
> error, which is the rate difference between two clocks.
> 

Thank you.

> Thanks,
> Simei
> 
>>
>>> The original command for starting ptpclient is:
>>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
>>> 0x1
>>>
>>> The command with PI algorithm is:
>>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
>>> 0x1 -- controller=pi
>>>
>>> [RFC v3 1/3] ethdev: add frequency adjustment API.
>>> [RFC v3 2/3] examples/ptpclient: refine application.
>>> [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
>>>
>>> v3:
>>> * Refine commit log in patch.
>>> * Add more description for the new API.
>>>
>>> v2:
>>> * Remove the ice PMD part from the RFC.
>>> * Add description in cover letter.
>>> * Refine commit log in patch.
>>>
>>> Simei Su (3):
>>>   ethdev: add frequency adjustment API
>>>   examples/ptpclient: refine application
>>>   examples/ptpclient: add frequency adjustment support
>>>
>>>  examples/ptpclient/ptpclient.c   | 222
>> +++++++++++++++++++++++++++++++++------
>>>  lib/ethdev/ethdev_driver.h       |   5 +
>>>  lib/ethdev/ethdev_trace.h        |   9 ++
>>>  lib/ethdev/ethdev_trace_points.c |   3 +
>>>  lib/ethdev/rte_ethdev.c          |  19 ++++
>>>  lib/ethdev/rte_ethdev.h          |  38 +++++++
>>>  6 files changed, 265 insertions(+), 31 deletions(-)
>>>
>