Re: [PATCH net v3 2/4] dpll: zl3073x: make embedded sync follow the output carrier frequency

[Petr Oros <poros@redhat.com>]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> Changing an output pin's frequency did not update its embedded sync
> (eSync) configuration. The eSync period and width are computed relative
> to the carrier:
> 
>      esync_n_period = out_freq / esync_freq
>      esync_n_width  = div / 2
> 
> Once zl3073x_dpll_output_pin_frequency_set() changes div, both values no
> longer match the new carrier, so the embedded sync runs at the wrong
> frequency and duty cycle. Worse, if the new carrier is <= 1 Hz then
> esync_get() returns -EOPNOTSUPP (out_freq <= 1), hiding the stale ESYNC
> mode so the user can no longer turn it off - it is stuck on.
> 
> This was hit during development: after changing an output pin's
> frequency the embedded sync output stopped working correctly, as
> confirmed on an oscilloscope - it ran at the wrong frequency and duty
> cycle because the eSync period and width still matched the old carrier.
> For a new 1 Hz carrier esync_get() returned -EOPNOTSUPP, leaving eSync
> enabled with no way to disable it.
> 
> Factor the eSync handling into zl3073x_out_esync_{is_enabled,enable,
> disable}() helpers and reuse them from esync_get()/esync_set(), then
> adjust the eSync parameters in the output frequency change path for
> non-N-divided outputs after updating div/width:
> 
> - If the new frequency is 1 Hz, disable eSync. It cannot work at a 1 Hz
>    carrier and esync_get() hides it, so it must be cleared rather than
>    left stuck on.
> - Otherwise, if eSync is active, recompute its period and width for the
>    new carrier keeping the 1 Hz eSync frequency.
> 
> As a side effect of reusing the helpers, esync_get() now reports the
> driver's eSync model - a 1 Hz frequency and a 25% pulse - from the clock
> type when eSync is enabled, instead of deriving the values from the
> cached esync_n_period/esync_n_width registers. The driver only ever
> programs a 1 Hz / 25% eSync, so this matches what esync_set() configures
> and keeps get and set symmetric. A configuration the driver did not
> establish - a non-1 Hz eSync left in flash or by an older kernel, or the
> truncated duty cycle of an odd divider - is therefore reported as the
> nominal 1 Hz / 25% rather than its exact register values.
> 
> N-divided outputs are left untouched: there esync_n_period is the N
> divider and eSync is not supported anyway.
> 
> Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
> Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/dpll.c | 68 ++++++++++++++++---------------------
>   drivers/dpll/zl3073x/out.h  | 36 ++++++++++++++++++++
>   2 files changed, 66 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40..7c997966c3c3 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -852,7 +852,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
>   	struct zl3073x_dpll_pin *pin = pin_priv;
>   	const struct zl3073x_synth *synth;
>   	const struct zl3073x_out *out;
> -	u32 synth_freq, out_freq;
> +	u32 synth_freq;
>   	u8 out_id;
>   
>   	guard(mutex)(&zldpll->lock);
> @@ -864,38 +864,28 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
>   	 * for N-division is also used for the esync divider so both cannot
>   	 * be used.
>   	 */
> -	if (zl3073x_out_is_ndiv(out))
> +	if (zl3073x_out_is_ndiv(out) || !pin->esync_control)
>   		return -EOPNOTSUPP;
>   
>   	/* Get attached synth frequency */
>   	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(out));
>   	synth_freq = zl3073x_synth_freq_get(synth);
> -	out_freq = synth_freq / out->div;
>   
> -	if (!pin->esync_control || out_freq <= 1)
> +	/* The esync is not supported for 1 Hz base frequency */
> +	if (synth_freq / out->div <= 1)
>   		return -EOPNOTSUPP;
>   
>   	esync->range = esync_freq_ranges;
>   	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
>   
> -	if (zl3073x_out_clock_type_get(out) != ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC) {
> -		/* No need to read esync data if it is not enabled */
> +	if (zl3073x_out_esync_is_enabled(out)) {
> +		esync->freq = 1;
> +		esync->pulse = 25;
> +	} else {
>   		esync->freq = 0;
>   		esync->pulse = 0;
> -
> -		return 0;
>   	}
>   
> -	/* Compute esync frequency */
> -	esync->freq = out_freq / out->esync_n_period;
> -
> -	/* By comparing the esync_pulse_width to the half of the pulse width
> -	 * the esync pulse percentage can be determined.
> -	 * Note that half pulse width is in units of half synth cycles, which
> -	 * is why it reduces down to be output_div.
> -	 */
> -	esync->pulse = (50 * out->esync_n_width) / out->div;
> -
>   	return 0;
>   }
>   
> @@ -926,32 +916,17 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
>   	if (zl3073x_out_is_ndiv(&out))
>   		return -EOPNOTSUPP;
>   
> -	/* Update clock type in output mode */
> -	if (freq)
> -		zl3073x_out_clock_type_set(&out,
> -					   ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
> -	else
> -		zl3073x_out_clock_type_set(&out,
> -					   ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
> -
> -	/* If esync is being disabled just write mailbox and finish */
> -	if (!freq)
> +	if (!freq) {
> +		zl3073x_out_esync_disable(&out);
>   		return zl3073x_out_state_set(zldev, out_id, &out);
> +	}
>   
>   	/* Get attached synth frequency */
>   	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(&out));
>   	synth_freq = zl3073x_synth_freq_get(synth);
>   
> -	/* Compute and update esync period */
> -	out.esync_n_period = synth_freq / (u32)freq / out.div;
> -
> -	/* Half of the period in units of 1/2 synth cycle can be represented by
> -	 * the output_div. To get the supported esync pulse width of 25% of the
> -	 * period the output_div can just be divided by 2. Note that this
> -	 * assumes that output_div is even, otherwise some resolution will be
> -	 * lost.
> -	 */
> -	out.esync_n_width = out.div / 2;
> +	/* Enable 1PPS eSync for this pin frequency */
> +	zl3073x_out_esync_enable(&out, synth_freq / out.div);
>   
>   	/* Commit output configuration */
>   	return zl3073x_out_state_set(zldev, out_id, &out);
> @@ -1009,6 +984,23 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		/* For 50/50 duty cycle the divisor is equal to width */
>   		out.width = new_div;
>   
> +		/* The embedded sync period and width are computed relative to
> +		 * the output carrier so they have to be adjusted to follow the
> +		 * new frequency.
> +		 */
> +		if (zl3073x_out_esync_is_enabled(&out)) {
> +			if (frequency == 1) {
> +				/* Disable eSync if the new frequency is 1 Hz */
> +				zl3073x_out_esync_disable(&out);
> +			} else {
> +				/* Update esync period and width according to
> +				 * the new frequency.
> +				 */
> +				zl3073x_out_esync_enable(&out,
> +							 synth_freq / out.div);
> +			}
> +		}
> +
>   		/* Commit output configuration */
>   		return zl3073x_out_state_set(zldev, out_id, &out);
>   	}
> diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
> index 660889c57bff..d2b8b4eb5a76 100644
> --- a/drivers/dpll/zl3073x/out.h
> +++ b/drivers/dpll/zl3073x/out.h
> @@ -134,4 +134,40 @@ static inline u8 zl3073x_out_synth_get(const struct zl3073x_out *out)
>   	return FIELD_GET(ZL_OUTPUT_CTRL_SYNTH_SEL, out->ctrl);
>   }
>   
> +static inline bool zl3073x_out_esync_is_enabled(const struct zl3073x_out *out)
> +{
> +	u8 clock_type = zl3073x_out_clock_type_get(out);
> +
> +	return clock_type == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC;
> +}
> +
> +static inline
> +void zl3073x_out_esync_enable(struct zl3073x_out *out, u32 base_freq)
> +{
> +	/* Period is expressed as the number of output divider clock cycles.
> +	 * So for 1 Hz the period is equal to base frequency.
> +	 */
> +	out->esync_n_period = base_freq;
> +
> +	/* Half of the period in units of 1/2 synth cycle can be represented by
> +	 * the output_div. To get the supported esync pulse width of 25% of the
> +	 * period the output_div can just be divided by 2. Note that this
> +	 * assumes that output_div is even, otherwise some resolution will be
> +	 * lost.
> +	 */
> +	out->esync_n_width = out->div / 2;
> +
> +	/* Update the clock type */
> +	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
> +}
> +
> +static inline
> +void zl3073x_out_esync_disable(struct zl3073x_out *out)
> +{
> +	/* Just update the clock type. The eSync period and width are
> +	 * ignored for this clock type.
> +	 */
> +	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
> +}
> +
>   #endif /* _ZL3073X_OUT_H */

Reviewed-by: Petr Oros <poros@redhat.com>