Documentation/networking/devlink/zl3073x.rst | 17 ++++++ drivers/dpll/zl3073x/core.c | 6 +- drivers/dpll/zl3073x/core.h | 8 ++- drivers/dpll/zl3073x/devlink.c | 61 ++++++++++++++++++++ 4 files changed, 89 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.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
v2:
* optimized conversion between factor value and register value
* more detailed parameter documentation
---
Documentation/networking/devlink/zl3073x.rst | 17 ++++++
drivers/dpll/zl3073x/core.c | 6 +-
drivers/dpll/zl3073x/core.h | 8 ++-
drivers/dpll/zl3073x/devlink.c | 61 ++++++++++++++++++++
4 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
index 4b6cfaf386433..1988721bdfa8b 100644
--- a/Documentation/networking/devlink/zl3073x.rst
+++ b/Documentation/networking/devlink/zl3073x.rst
@@ -20,6 +20,23 @@ 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 for phase offset
+ reporting. The DPLL phase measurement block applies this value in the
+ following formula:
+
+ .. math::
+ curr\_avg = prev\_avg * \frac{2^N-1}{2^N} + new\_val * \frac{1}{2^N}
+
+ where `curr_avg` is the current phase offset, `prev_avg` is the previous
+ phase offset, and `new_val` is currently measured phase offset.
+
+ New measurements are taken approximately 40 Hz or at the frequency of
+ the reference, whichever is lower.
+
+ The default value of this parameter is 2, and the supported range of
+ values is <0, 15>, where a value 0 effectively disables averaging.
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..fe8333a2ea1ee 100644
--- a/drivers/dpll/zl3073x/devlink.c
+++ b/drivers/dpll/zl3073x/devlink.c
@@ -195,10 +195,71 @@ 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 */
+ ctx->val.vu8 = (zldev->phase_avg_factor - 1) & 0x0f;
+
+ 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 */
+ avg_factor = (ctx->val.vu8 + 1) & 0x0f;
+
+ /* 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.1cc: 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?
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> v2:
> * optimized conversion between factor value and register value
> * more detailed parameter documentation
> ---
> Documentation/networking/devlink/zl3073x.rst | 17 ++++++
> drivers/dpll/zl3073x/core.c | 6 +-
> drivers/dpll/zl3073x/core.h | 8 ++-
> drivers/dpll/zl3073x/devlink.c | 61 ++++++++++++++++++++
> 4 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
> index 4b6cfaf386433..1988721bdfa8b 100644
> --- a/Documentation/networking/devlink/zl3073x.rst
> +++ b/Documentation/networking/devlink/zl3073x.rst
> @@ -20,6 +20,23 @@ 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 for phase offset
> + reporting. The DPLL phase measurement block applies this value in the
> + following formula:
> +
> + .. math::
> + curr\_avg = prev\_avg * \frac{2^N-1}{2^N} + new\_val * \frac{1}{2^N}
> +
> + where `curr_avg` is the current phase offset, `prev_avg` is the previous
> + phase offset, and `new_val` is currently measured phase offset.
> +
> + New measurements are taken approximately 40 Hz or at the frequency of
> + the reference, whichever is lower.
> +
> + The default value of this parameter is 2, and the supported range of
> + values is <0, 15>, where a value 0 effectively disables averaging.
>
> 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..fe8333a2ea1ee 100644
> --- a/drivers/dpll/zl3073x/devlink.c
> +++ b/drivers/dpll/zl3073x/devlink.c
> @@ -195,10 +195,71 @@ 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 */
> + ctx->val.vu8 = (zldev->phase_avg_factor - 1) & 0x0f;
> +
> + 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 */
> + avg_factor = (ctx->val.vu8 + 1) & 0x0f;
> +
> + /* 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
>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.
Does frequency offset measurement for EEC DPLL would also use the same value?
Thank you!
Arkadiusz
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>> v2:
>> * optimized conversion between factor value and register value
>> * more detailed parameter documentation
>> ---
>> Documentation/networking/devlink/zl3073x.rst | 17 ++++++
>> drivers/dpll/zl3073x/core.c | 6 +-
>> drivers/dpll/zl3073x/core.h | 8 ++-
>> drivers/dpll/zl3073x/devlink.c | 61 ++++++++++++++++++++
>> 4 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/networking/devlink/zl3073x.rst
>>b/Documentation/networking/devlink/zl3073x.rst
>> index 4b6cfaf386433..1988721bdfa8b 100644
>> --- a/Documentation/networking/devlink/zl3073x.rst
>> +++ b/Documentation/networking/devlink/zl3073x.rst
>> @@ -20,6 +20,23 @@ 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 for phase
>>offset
>> + reporting. The DPLL phase measurement block applies this value in
>>the
>> + following formula:
>> +
>> + .. math::
>> + curr\_avg = prev\_avg * \frac{2^N-1}{2^N} + new\_val *
>>\frac{1}{2^N}
>> +
>> + where `curr_avg` is the current phase offset, `prev_avg` is the
>>previous
>> + phase offset, and `new_val` is currently measured phase offset.
>> +
>> + New measurements are taken approximately 40 Hz or at the
>>frequency of
>> + the reference, whichever is lower.
>> +
>> + The default value of this parameter is 2, and the supported range
>>of
>> + values is <0, 15>, where a value 0 effectively disables
>averaging.
>>
>> 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..fe8333a2ea1ee 100644
>> --- a/drivers/dpll/zl3073x/devlink.c
>> +++ b/drivers/dpll/zl3073x/devlink.c
>> @@ -195,10 +195,71 @@ 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 */
>> + ctx->val.vu8 = (zldev->phase_avg_factor - 1) & 0x0f;
>> +
>> + 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 */
>> + avg_factor = (ctx->val.vu8 + 1) & 0x0f;
>> +
>> + /* 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
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. > Does frequency offset measurement for EEC DPLL would also use the same value? Nope, this only affects phase offset measurement. AFAIK there is no such tuning knob for FFO or frequency measurement in general... Is it correct Prathosh? Thanks, Ivan
>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 >> Does frequency offset measurement for EEC DPLL would also use the same >>value? > >Nope, this only affects phase offset measurement. AFAIK there is no such >tuning knob for FFO or frequency measurement in general... >Is it correct Prathosh? > >Thanks, >Ivan
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
On 17/09/2025 15:55, Ivan Vecera wrote: > 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? Our HW doesn't have such options, and it doesn't looks like we will be able to extend it this way. But I'm fine with providing UAPI if there will other users.
On September 16, 2025 1:46:41 AM GMT+02:00, Jakub Kicinski <kuba@kernel.org> wrote: >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? I don't know... I expect that DPLL chips support phase offset measurement but probably implementation specific. >If it is we should probably add this to the official API. The problem in case of this Microchip DPLL devices is that this parameter is per ASIC. It is common for all DPLL channels (devices) inside the chip. That's why I chose devlink. >If it isn't we should probably default to smallest possible history? Do you mean the default value? Ivan
On Tue, 16 Sep 2025 20:16:57 +0200 Ivan Vecera wrote: > >> 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? > > I don't know... I expect that DPLL chips support phase offset > measurement but probably implementation specific. Ack, I was hoping Arkadiusz and Vadim could comment what their DPLL devices do. > >If it is we should probably add this to the official API. > > The problem in case of this Microchip DPLL devices is that this > parameter is per ASIC. It is common for all DPLL channels > (devices) inside the chip. That's why I chose devlink. > > >If it isn't we should probably default to smallest possible history? > > Do you mean the default value? Yes, but let's wait for others to chime in.
© 2016 - 2025 Red Hat, Inc.