Re: [PATCH net-next v2 2/2] dpll: zl3073x: add NCO virtual input pin

[Ivan Vecera <ivecera@redhat.com>]

Sashiko findings clarification inline below...

On 5/16/26 7:57 PM, Ivan Vecera wrote:
> Add a virtual NCO (Numerically Controlled Oscillator) input pin that
> lets userspace switch a DPLL channel into NCO mode. A single NCO pin
> is shared across all DPLL channels — each channel has its own
> independent connection state. The NCO pin is registered with the new
> DPLL_PIN_TYPE_INT_NCO type and reports DPLL_PIN_STATE_CONNECTED /
> DPLL_PIN_OPERSTATE_ACTIVE when the channel is in NCO mode.
> ...
> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
> index 2fe3c3da84bb5..f5d6c7917e57a 100644
> --- a/drivers/dpll/zl3073x/chan.c
> +++ b/drivers/dpll/zl3073x/chan.c
> @@ -21,6 +21,11 @@ int zl3073x_chan_state_update(struct zl3073x_dev *zldev, u8 index)
>   	u64 val;
>   	int rc;
>   
> +	/* Serialize with zl3073x_chan_nco_mode_set() which also
> +	 * modifies chan->mode_refsel and chan->df_offset.
> +	 */
> +	guard(mutex)(&zldev->multiop_lock);
> +
>   	rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MON_STATUS(index),
>   			     &chan->mon_status);
>   	if (rc)
> @@ -31,7 +36,10 @@ int zl3073x_chan_state_update(struct zl3073x_dev *zldev, u8 index)
>   	if (rc)
>   		return rc;
>   
> -	/* Read df_offset vs tracked reference */
> +	/* Read df_offset only when locked to a reference */
> +	if (zl3073x_chan_lock_state_get(chan) != ZL_DPLL_MON_STATUS_STATE_LOCK)
> +		return 0;
> +

Sashiko commented:

> If the device boots already in NCO mode, can chan->df_offset ever be
> populated?
> Walking through zl3073x_chan_state_fetch() at probe time with the
> hardware already in NCO mode:
>   - state_fetch() reads ctrl and mode_refsel, then calls
>     zl3073x_chan_state_update()
>   - state_update() now early-returns above when lock_state is not
>     STATE_LOCK, and an NCO-mode channel is not LOCK, so df_offset
>     keeps its kzalloc default of 0
>   - zl3073x_dpll_nco_pin_register() updates ctrl bits via
>     zl3073x_chan_state_set() but does not touch mode_refsel, so the
>     edge-triggered nco_auto_read does not fire (it captures only on
>     the transition into NCO)
>   - zl3073x_dpll_nco_pin_ffo_get() then reports
>     -mul_s64_u64_shr(0, 244140625, 36) = 0
> The commit message states "Input reference pins are now always
> registered regardless of the initial DPLL mode", so boot-in-NCO is a
> supported configuration.
> Should zl3073x_dpll_nco_pin_register() (or state_fetch) read
> ZL_REG_DPLL_DF_OFFSET when the channel is already in NCO mode, similar
> to the read in the new zl3073x_chan_nco_mode_set()?

In boot-in-NCO configuration the df_offset is zero... so this behavior
is correct.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index cff85cdb9d0e5..2efefb1d7e224 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -605,6 +630,7 @@ zl3073x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *dpll_pin,
>   {
>   	struct zl3073x_dpll *zldpll = dpll_priv;
>   	struct zl3073x_dpll_pin *pin = pin_priv;
> +	struct zl3073x_dpll_pin *nco_pin = NULL;
>   	struct zl3073x_chan chan;
>   	u8 mode, ref;
>   	int rc;
> @@ -634,6 +660,10 @@ zl3073x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *dpll_pin,
>   			goto invalid_state;
>   		}
>   		break;
> +	case ZL_DPLL_MODE_REFSEL_MODE_NCO:
> +		if (state == DPLL_PIN_STATE_CONNECTED)
> +			nco_pin = zl3073x_dpll_nco_pin_get(zldpll);
> +		fallthrough;
>   	case ZL_DPLL_MODE_REFSEL_MODE_FREERUN:
>   	case ZL_DPLL_MODE_REFSEL_MODE_HOLDOVER:
>   		if (state == DPLL_PIN_STATE_CONNECTED) {

Sashiko commented:

> When a REF pin is connected while the channel is in REFLOCK and the
> NCO pin is then connected, the channel transitions REFLOCK -> NCO via
> zl3073x_chan_nco_mode_set(). The previously-locked REF pin now
> reports DPLL_PIN_STATE_DISCONNECTED via the default branch in
> zl3073x_dpll_input_pin_state_on_dpll_get(), but no immediate
> __dpll_pin_change_ntf() is sent for that REF pin.
> The periodic worker zl3073x_dpll_changes_check() will eventually pick
> up the operstate change and call dpll_pin_change_ntf() for the REF
> pin so userspace re-queries.
> Should the same explicit-immediate-notification pattern that this
> patch introduces for the NCO pin also apply to the previously-
> connected REF pin when its connection state changes due to a mode
> transition?

No, explicit notification for NCO pin is sent because NCO-type pin is
not monitored. For regulard HW input references the implicit
notification will be sent by peridic monitor function.

> ...
> +static int
> +zl3073x_dpll_nco_pin_ffo_get(const struct dpll_pin *dpll_pin, void *pin_priv,
> +			     const struct dpll_device *dpll, void *dpll_priv,
> +			     struct dpll_ffo_param *ffo,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct zl3073x_dpll *zldpll = dpll_priv;
> +	const struct zl3073x_chan *chan;
> +
> +	chan = zl3073x_chan_state_get(zldpll->dev, zldpll->id);
> +	if (!zl3073x_chan_mode_is_nco(chan))
> +		return -ENODATA;
> +
> +	/* NCO register has inverted sign: f_offset = -df_offset / 2^48
> +	 * Convert to PPT: ppt = -df * 5^12 / 2^36
> +	 */
> +	ffo->ffo = -mul_s64_u64_shr(zl3073x_chan_df_offset_get(chan),
> +				    244140625, 36);
> +
> +	return 0;
> +}

Sashiko commented:

> The new pin is registered with type DPLL_PIN_TYPE_INT_NCO. The UAPI
> header include/uapi/linux/dpll.h documents that type as: "When
> connected as a DPLL input, the DPLL enters NCO mode where the output
> frequency is adjusted by the host via the PTP clock interface."
> Does this driver satisfy that contract?
> There is no PTP clock device registered anywhere under
> drivers/dpll/zl3073x (no ptp_clock_register, no ptp_clock_info, no
> adjfine/adjphase). The new zl3073x_dpll_nco_pin_ops exposes only
> ffo_get, state_on_dpll_get/set, operstate_on_dpll_get and
> direction_get, with no frequency or phase control. The pin properties
> declare only DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE.
> The commit message also says "The value is static for the duration of
> NCO mode and serves as the FFO snapshot reported to userspace", so
> zl3073x_chan_nco_mode_set() captures df_offset once and never updates
> it.
> Generic userspace recognizing DPLL_PIN_TYPE_INT_NCO will reasonably
> expect a controllable NCO. Should this mode either expose a frequency
> adjustment path (PTP or otherwise) or document that this device
> exposes a read-only frozen-offset variant of the NCO type?

The PTP support for zl3073x will follow this series.

Thanks,
Ivan