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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] zl3073x_dpll_output_pin_esync_get()
  (drivers/dpll/zl3073x/dpll.c) now returns esync->freq = 1 and…
- [Medium] The fix for the 'stuck-on' eSync only works when the driver's
  own frequency_set() moves the carrier to 1 Hz.

--

dpll: zl3073x: make embedded sync follow the output carrier frequency

This patch moves the eSync handling into zl3073x_out_esync_{is_enabled,
enable,disable}() helpers and uses them from the esync get/set callbacks.
On non-N-divided outputs, a frequency change now recomputes the eSync
period and width, or disables eSync when the new carrier is 1 Hz.
esync_get() now reports a nominal 1 Hz / 25% whenever the clock type is
ESYNC.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40f..7c997966c3c33 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -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;

[Severity: Medium]
Can eSync still get stuck on if the output already has the ESYNC clock
type on a 1 Hz carrier when the driver probes?

The commit message mentions configurations "left in flash or by an older
kernel". zl3073x_out_state_fetch() keeps the mode register exactly as it
reads it from the device. So this state can exist without the driver's
own frequency_set() ever having run.

In that case this check returns -EOPNOTSUPP even though eSync is enabled.
dpll_pin_esync_set() then returns before it reaches the driver's disable
path:

drivers/dpll/dpll_netlink.c:dpll_pin_esync_set() {
    ...
	ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
			     dpll_priv(dpll), &esync, extack);
	if (ret) {
		NL_SET_ERR_MSG(extack, "unable to get current embedded sync frequency value");
		return ret;
	}
    ...
}

The new disable branch in zl3073x_dpll_output_pin_frequency_set() can't
be reached either. The core short-circuits a request for 1 Hz on a pin
that is already at 1 Hz:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
    ...
	if (freq == old_freq)
		return 0;
    ...
}

The only escape seems to be: move the pin to another supported frequency,
disable eSync, then go back to 1 Hz. That doesn't work if 1 Hz is the
only frequency the pin supports.

Could the driver clear the ESYNC clock type at fetch time when the carrier
is 1 Hz or less? Alternatively, esync_get() could report an enabled eSync
so that it can be turned off.

>  
>  	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;

[Severity: Medium]
Does reporting a fixed 1 Hz here stop the core from correcting a
mismatched eSync?

dpll_pin_esync_set() treats the esync_get() result as the current
hardware state, and it skips the driver callback when the values match:

drivers/dpll/dpll_netlink.c:dpll_pin_esync_set() {
    ...
	if (freq == esync.freq)
		return 0;
    ...
}

zl3073x_out_state_fetch() imports whatever eSync period and width the
device holds and doesn't normalise them. For example, an older kernel
might have moved the carrier from 10 MHz to 20 MHz and left the period at
10000000. The real eSync is then 2 Hz.

Before this patch the getter reported 2 in that case. A request for 1 Hz
therefore reached zl3073x_dpll_output_pin_esync_set(), which reprogrammed
the period and width.

With this patch the getter reports 1. The same request returns success,
but the hardware keeps the wrong period. The only workaround is to set 0
and then 1, and nothing in the reported state tells the user that this
is needed.

The reported pulse can also differ from the programmed width.
zl3073x_out_esync_enable() does:

	out->esync_n_width = out->div / 2;

With an odd divider the real duty cycle is below 25%, for example about
16.7% for div=3. frequency_set() now writes that width on every carrier
change while eSync is on, but the getter still reports 25.

Documentation/driver-api/dpll.rst describes DPLL_A_PIN_ESYNC_FREQUENCY as
the "current Embedded SYNC frequency". In the same file,
zl3073x_dpll_input_pin_esync_get() reports 1 Hz only when
ref->esync_n_div really holds ZL_REF_ESYNC_DIV_1HZ.

The commit message mentions the nominal reporting. Should it also say
that a 1 Hz set can no longer fix a mismatched eSync? Or could the getter
keep deriving freq and pulse from esync_n_period and esync_n_width?

>  	} else {
>  		esync->freq = 0;
>  		esync->pulse = 0;
> -
> -		return 0;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006153116.347497-1-ivecera%40redhat.com