Documentation/networking/devlink/zl3073x.rst | 4 ++ drivers/dpll/zl3073x/core.c | 6 +- drivers/dpll/zl3073x/core.h | 8 ++- drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++ 4 files changed, 82 insertions(+), 3 deletions(-)
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.
Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
Documentation/networking/devlink/zl3073x.rst | 4 ++
drivers/dpll/zl3073x/core.c | 6 +-
drivers/dpll/zl3073x/core.h | 8 ++-
drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++
4 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
index 4b6cfaf386433..ddd159e39e616 100644
--- a/Documentation/networking/devlink/zl3073x.rst
+++ b/Documentation/networking/devlink/zl3073x.rst
@@ -20,6 +20,10 @@ Parameters
- driverinit
- Set the clock ID that is used by the driver for registering DPLL devices
and pins.
+ * - ``phase_offset_avg_factor``
+ - runtime
+ - Set the factor for the exponential moving average used by DPLL phase
+ measurement block. The value has to be in range <0, 15>.
Info versions
=============
diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 7ebcfc5ec1f09..4f6395372f0eb 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -915,7 +915,8 @@ zl3073x_dev_phase_meas_setup(struct zl3073x_dev *zldev, int num_channels)
/* Setup phase measurement averaging factor */
dpll_meas_ctrl &= ~ZL_DPLL_MEAS_CTRL_AVG_FACTOR;
- dpll_meas_ctrl |= FIELD_PREP(ZL_DPLL_MEAS_CTRL_AVG_FACTOR, 3);
+ dpll_meas_ctrl |= FIELD_PREP(ZL_DPLL_MEAS_CTRL_AVG_FACTOR,
+ zldev->phase_avg_factor);
/* Enable DPLL measurement block */
dpll_meas_ctrl |= ZL_DPLL_MEAS_CTRL_EN;
@@ -991,6 +992,9 @@ int zl3073x_dev_probe(struct zl3073x_dev *zldev,
*/
zldev->clock_id = get_random_u64();
+ /* Default phase offset averaging factor */
+ zldev->phase_avg_factor = 3;
+
/* Initialize mutex for operations where multiple reads, writes
* and/or polls are required to be done atomically.
*/
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 71af2c8001109..289d09fcc5c5a 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -67,19 +67,19 @@ struct zl3073x_synth {
* @dev: pointer to device
* @regmap: regmap to access device registers
* @multiop_lock: to serialize multiple register operations
- * @clock_id: clock id of the device
* @ref: array of input references' invariants
* @out: array of outs' invariants
* @synth: array of synths' invariants
* @dplls: list of DPLLs
* @kworker: thread for periodic work
* @work: periodic work
+ * @clock_id: clock id of the device
+ * @phase_avg_factor: phase offset measurement averaging factor
*/
struct zl3073x_dev {
struct device *dev;
struct regmap *regmap;
struct mutex multiop_lock;
- u64 clock_id;
/* Invariants */
struct zl3073x_ref ref[ZL3073X_NUM_REFS];
@@ -92,6 +92,10 @@ struct zl3073x_dev {
/* Monitor */
struct kthread_worker *kworker;
struct kthread_delayed_work work;
+
+ /* Devlink parameters */
+ u64 clock_id;
+ u8 phase_avg_factor;
};
struct zl3073x_chip_info {
diff --git a/drivers/dpll/zl3073x/devlink.c b/drivers/dpll/zl3073x/devlink.c
index 7e7fe726ee37a..83491a99264db 100644
--- a/drivers/dpll/zl3073x/devlink.c
+++ b/drivers/dpll/zl3073x/devlink.c
@@ -195,10 +195,77 @@ zl3073x_devlink_param_clock_id_validate(struct devlink *devlink, u32 id,
return 0;
}
+static int
+zl3073x_devlink_param_phase_avg_factor_get(struct devlink *devlink, u32 id,
+ struct devlink_param_gset_ctx *ctx)
+{
+ struct zl3073x_dev *zldev = devlink_priv(devlink);
+
+ /* Convert the value to actual factor value */
+ if (zldev->phase_avg_factor > 0)
+ ctx->val.vu8 = zldev->phase_avg_factor - 1;
+ else
+ ctx->val.vu8 = 15;
+
+ return 0;
+}
+
+static int
+zl3073x_devlink_param_phase_avg_factor_set(struct devlink *devlink, u32 id,
+ struct devlink_param_gset_ctx *ctx,
+ struct netlink_ext_ack *extack)
+{
+ struct zl3073x_dev *zldev = devlink_priv(devlink);
+ u8 avg_factor, dpll_meas_ctrl;
+ int rc;
+
+ /* Read DPLL phase measurement control register */
+ rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, &dpll_meas_ctrl);
+ if (rc)
+ return rc;
+
+ /* Convert requested factor to register value */
+ if (ctx->val.vu8 < 15)
+ avg_factor = ctx->val.vu8 + 1;
+ else
+ avg_factor = 0;
+
+ /* Update phase measurement control register */
+ dpll_meas_ctrl &= ~ZL_DPLL_MEAS_CTRL_AVG_FACTOR;
+ dpll_meas_ctrl |= FIELD_PREP(ZL_DPLL_MEAS_CTRL_AVG_FACTOR, avg_factor);
+ rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, dpll_meas_ctrl);
+ if (rc)
+ return rc;
+
+ /* Save the new factor */
+ zldev->phase_avg_factor = avg_factor;
+
+ return 0;
+}
+
+static int
+zl3073x_devlink_param_phase_avg_factor_validate(struct devlink *devlink, u32 id,
+ union devlink_param_value val,
+ struct netlink_ext_ack *extack)
+{
+ return (val.vu8 < 16) ? 0 : -EINVAL;
+}
+
+enum zl3073x_dl_param_id {
+ ZL3073X_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+ ZL3073X_DEVLINK_PARAM_ID_PHASE_OFFSET_AVG_FACTOR,
+};
+
static const struct devlink_param zl3073x_devlink_params[] = {
DEVLINK_PARAM_GENERIC(CLOCK_ID, BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
NULL, NULL,
zl3073x_devlink_param_clock_id_validate),
+ DEVLINK_PARAM_DRIVER(ZL3073X_DEVLINK_PARAM_ID_PHASE_OFFSET_AVG_FACTOR,
+ "phase_offset_avg_factor", DEVLINK_PARAM_TYPE_U8,
+ BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+ zl3073x_devlink_param_phase_avg_factor_get,
+ zl3073x_devlink_param_phase_avg_factor_set,
+ zl3073x_devlink_param_phase_avg_factor_validate),
};
static void
--
2.49.1On Wed, Sep 10, 2025 at 12:32:21PM +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.
>
> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> Documentation/networking/devlink/zl3073x.rst | 4 ++
> drivers/dpll/zl3073x/core.c | 6 +-
> drivers/dpll/zl3073x/core.h | 8 ++-
> drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++
> 4 files changed, 82 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
> index 4b6cfaf386433..ddd159e39e616 100644
> --- a/Documentation/networking/devlink/zl3073x.rst
> +++ b/Documentation/networking/devlink/zl3073x.rst
> @@ -20,6 +20,10 @@ Parameters
> - driverinit
> - Set the clock ID that is used by the driver for registering DPLL devices
> and pins.
> + * - ``phase_offset_avg_factor``
> + - runtime
> + - Set the factor for the exponential moving average used by DPLL phase
> + measurement block. The value has to be in range <0, 15>.
Maybe put the text in the commit message here as well?
Andrew
On 10. 09. 25 6:13 odp., Andrew Lunn wrote: > On Wed, Sep 10, 2025 at 12:32:21PM +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. >> >> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com> >> Signed-off-by: Ivan Vecera <ivecera@redhat.com> >> --- >> Documentation/networking/devlink/zl3073x.rst | 4 ++ >> drivers/dpll/zl3073x/core.c | 6 +- >> drivers/dpll/zl3073x/core.h | 8 ++- >> drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++ >> 4 files changed, 82 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst >> index 4b6cfaf386433..ddd159e39e616 100644 >> --- a/Documentation/networking/devlink/zl3073x.rst >> +++ b/Documentation/networking/devlink/zl3073x.rst >> @@ -20,6 +20,10 @@ Parameters >> - driverinit >> - Set the clock ID that is used by the driver for registering DPLL devices >> and pins. >> + * - ``phase_offset_avg_factor`` >> + - runtime >> + - Set the factor for the exponential moving average used by DPLL phase >> + measurement block. The value has to be in range <0, 15>. > > Maybe put the text in the commit message here as well? Do you mean to put the equation and details from commit message here? This is pretty long. Ivan
On Wed, Sep 10, 2025 at 06:50:47PM +0200, Ivan Vecera wrote: > On 10. 09. 25 6:13 odp., Andrew Lunn wrote: > > On Wed, Sep 10, 2025 at 12:32:21PM +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. > > > > > > Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com> > > > Signed-off-by: Ivan Vecera <ivecera@redhat.com> > > > --- > > > Documentation/networking/devlink/zl3073x.rst | 4 ++ > > > drivers/dpll/zl3073x/core.c | 6 +- > > > drivers/dpll/zl3073x/core.h | 8 ++- > > > drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++ > > > 4 files changed, 82 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst > > > index 4b6cfaf386433..ddd159e39e616 100644 > > > --- a/Documentation/networking/devlink/zl3073x.rst > > > +++ b/Documentation/networking/devlink/zl3073x.rst > > > @@ -20,6 +20,10 @@ Parameters > > > - driverinit > > > - Set the clock ID that is used by the driver for registering DPLL devices > > > and pins. > > > + * - ``phase_offset_avg_factor`` > > > + - runtime > > > + - Set the factor for the exponential moving average used by DPLL phase > > > + measurement block. The value has to be in range <0, 15>. > > > > Maybe put the text in the commit message here as well? > > Do you mean to put the equation and details from commit message here? > This is pretty long. So what if it is long? At the moment, it is hiding in the commit message. It is not easy to find, you effectively need to be a kernel developer to find it. If it is in the documentation of the device, it will be much easier to find and understand what this knob actually does. Andrew
On 10. 09. 25 7:06 odp., Andrew Lunn wrote: > On Wed, Sep 10, 2025 at 06:50:47PM +0200, Ivan Vecera wrote: >> On 10. 09. 25 6:13 odp., Andrew Lunn wrote: >>> On Wed, Sep 10, 2025 at 12:32:21PM +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. >>>> >>>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com> >>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com> >>>> --- >>>> Documentation/networking/devlink/zl3073x.rst | 4 ++ >>>> drivers/dpll/zl3073x/core.c | 6 +- >>>> drivers/dpll/zl3073x/core.h | 8 ++- >>>> drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++ >>>> 4 files changed, 82 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst >>>> index 4b6cfaf386433..ddd159e39e616 100644 >>>> --- a/Documentation/networking/devlink/zl3073x.rst >>>> +++ b/Documentation/networking/devlink/zl3073x.rst >>>> @@ -20,6 +20,10 @@ Parameters >>>> - driverinit >>>> - Set the clock ID that is used by the driver for registering DPLL devices >>>> and pins. >>>> + * - ``phase_offset_avg_factor`` >>>> + - runtime >>>> + - Set the factor for the exponential moving average used by DPLL phase >>>> + measurement block. The value has to be in range <0, 15>. >>> >>> Maybe put the text in the commit message here as well? >> >> Do you mean to put the equation and details from commit message here? >> This is pretty long. > > So what if it is long? At the moment, it is hiding in the commit > message. It is not easy to find, you effectively need to be a kernel > developer to find it. If it is in the documentation of the device, it > will be much easier to find and understand what this knob actually > does. > Agree. I will describe it in more detail in the documentation. + optimization proposed by Vadim. Thanks for the review. Ivan
On 9/10/25 12:32, 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.
>
> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> Documentation/networking/devlink/zl3073x.rst | 4 ++
> drivers/dpll/zl3073x/core.c | 6 +-
> drivers/dpll/zl3073x/core.h | 8 ++-
> drivers/dpll/zl3073x/devlink.c | 67 ++++++++++++++++++++
> 4 files changed, 82 insertions(+), 3 deletions(-)
>
looks good,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> +static int
> +zl3073x_devlink_param_phase_avg_factor_validate(struct devlink *devlink, u32 id,
> + union devlink_param_value val,
> + struct netlink_ext_ack *extack)
> +{
> + return (val.vu8 < 16) ? 0 : -EINVAL;
nit: redundant params, not worth respining
On 10.09.2025 11:32, 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.
>
> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
[...]
> +static int
> +zl3073x_devlink_param_phase_avg_factor_set(struct devlink *devlink, u32 id,
> + struct devlink_param_gset_ctx *ctx,
> + struct netlink_ext_ack *extack)
> +{
> + struct zl3073x_dev *zldev = devlink_priv(devlink);
> + u8 avg_factor, dpll_meas_ctrl;
> + int rc;
> +
> + /* Read DPLL phase measurement control register */
> + rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, &dpll_meas_ctrl);
> + if (rc)
> + return rc;
> +
> + /* Convert requested factor to register value */
> + if (ctx->val.vu8 < 15)
> + avg_factor = ctx->val.vu8 + 1;
> + else
> + avg_factor = 0;
> +
This looks like avg_factor = (ctx->val.vu8 + 1) & 0x0f;
The same logic can be applied for get() function assuming we are aware of
unsigned roll-over...
On 10. 09. 25 2:34 odp., Vadim Fedorenko wrote:
> On 10.09.2025 11:32, 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.
>>
>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>
> [...]
>
>> +static int
>> +zl3073x_devlink_param_phase_avg_factor_set(struct devlink *devlink,
>> u32 id,
>> + struct devlink_param_gset_ctx *ctx,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct zl3073x_dev *zldev = devlink_priv(devlink);
>> + u8 avg_factor, dpll_meas_ctrl;
>> + int rc;
>> +
>> + /* Read DPLL phase measurement control register */
>> + rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MEAS_CTRL, &dpll_meas_ctrl);
>> + if (rc)
>> + return rc;
>> +
>> + /* Convert requested factor to register value */
>> + if (ctx->val.vu8 < 15)
>> + avg_factor = ctx->val.vu8 + 1;
>> + else
>> + avg_factor = 0;
>> +
>
> This looks like avg_factor = (ctx->val.vu8 + 1) & 0x0f;
> The same logic can be applied for get() function assuming we are aware of
> unsigned roll-over...
Yes, I know about this trick but I wanted to use more readable code and
leave potential optimization to the compiler.
Ivan
© 2016 - 2025 Red Hat, Inc.