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

[Ivan Vecera]

On 10/8/26 17:31, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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?

eSync on a 1 Hz carrier needs output_esync_period = 1, which the
datasheet defines as undefined. Such a state in flash is an invalid
configuration and the driver does not try to recover from it. The
patch only makes sure the driver itself never creates it.

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

Agreed. I will drop the fixed 1 Hz / 25% reporting and derive the
frequency and pulse from the registers again in V4. The pulse should
be also computed in 64 bits, as 50 * esync_n_width can overflow u32.

Thanks,
Ivan

pw-bot: cr