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

[Ivan Vecera]

On 17. 09. 25 3:10 odp., Kubalewski, Arkadiusz wrote:
> > From: Ivan Vecera <ivecera@xxxxxxxxxx>
> > Sent: Wednesday, September 17, 2025 2:18 PM
> >
> > On 17. 09. 25 1:26 odp., Kubalewski, Arkadiusz wrote:
> > > > From: Jakub Kicinski <kuba@xxxxxxxxxx>
> > > > 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