Re: [PATCH net-next v2] dpll: zl3073x: Allow to use custom phase measure averaging factor

[Ivan Vecera <ivecera@redhat.com>]

On 17. 09. 25 3:10 odp., Kubalewski, Arkadiusz wrote:
>> From: Ivan Vecera <ivecera@redhat.com>
>> Sent: Wednesday, September 17, 2025 2:18 PM
>>
>> On 17. 09. 25 1:26 odp., Kubalewski, Arkadiusz wrote:
>>>> From: Jakub Kicinski <kuba@kernel.org>
>>>> Sent: Tuesday, September 16, 2025 1:47 AM
>>>>
>>>> cc: Arkadiusz
>>>>
>>>> On Thu, 11 Sep 2025 09:23:01 +0200 Ivan Vecera wrote:
>>>>> The DPLL phase measurement block uses an exponential moving average,
>>>>> calculated using the following equation:
>>>>>
>>>>>                          2^N - 1                1
>>>>> curr_avg = prev_avg * --------- + new_val * -----
>>>>>                            2^N                 2^N
>>>>>
>>>>> Where curr_avg is phase offset reported by the firmware to the driver,
>>>>> prev_avg is previous averaged value and new_val is currently measured
>>>>> value for particular reference.
>>>>>
>>>>> New measurements are taken approximately 40 Hz or at the frequency of
>>>>> the reference (whichever is lower).
>>>>>
>>>>> The driver currently uses the averaging factor N=2 which prioritizes
>>>>> a fast response time to track dynamic changes in the phase. But for
>>>>> applications requiring a very stable and precise reading of the average
>>>>> phase offset, and where rapid changes are not expected, a higher factor
>>>>> would be appropriate.
>>>>>
>>>>> Add devlink device parameter phase_offset_avg_factor to allow a user
>>>>> set tune the averaging factor via devlink interface.
>>>>
>>>> Is averaging phase offset normal for DPLL devices?
>>>> If it is we should probably add this to the official API.
>>>> If it isn't we should probably default to smallest possible history?
>>>>
>>>
>>> AFAIK, our phase offset measurement uses similar mechanics, but the
>>> algorithm
>>> is embedded in the DPLL device FW and currently not user controlled.
>>> Although it might happen that one day we would also provide such knob,
>>> if useful for users, no plans for it now.
>>>   From this perspective I would rather see it in dpll api, especially
>>> this relates to the phase measurement which is already there, the value
>>> being shared by multiple dpll devices seems HW related, but also seem not
>>> a
>>> problem, as long as a change would notify each device it relates with.
>>
>> What if the averaging is implemented in different HW differently? As I
>> mentioned the Microchip HW uses exponential moving average but
>> a different HW can do it differently.
>>
> 
> Yeah good point, in that case we would also need enumerate those, and the new
> HW would have to extend the uAPI to let the user know which method is used?
> Different methods could require different parameters?
> But for your current case only one attribute would be enough?
> Or maybe better to provide those together like:
> DPLL_A_PHASE_MEASUREMENT_EMA_N
> Then different method would have own attributes/params?
> 
> Next question if a HW could have multiple of those methods available and
> controlled, in which case we shall also have in mind a plan for a
> "turned-off" value for further extensions?
> 
> Thank you!
> Arkadiusz

Jiri, Vadim,
any comments / opinions?

Thanks,
Ivan