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

[Ivan Vecera <ivecera@redhat.com>]

On 10/8/26 17:31, netdev-bot+sashiko@kernel.org 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