[PATCH net-next v3 0/2] dpll: use pin owner's dpll ref for pin-level set callbacks

[PATCH net-next v3 0/2] dpll: use pin owner's dpll ref for pin-level set callbacks

[Ivan Vecera]

Pin-level attributes (frequency, phase adjust, embedded sync, reference
sync) are properties of the pin itself. The get callbacks already use
only the pin owner's DPLL reference, but the set callbacks iterate over
all registered DPLL devices, resulting in redundant HW writes for
drivers that share a pin across multiple DPLLs.

This series simplifies the set side to match the get side: call the set
callback only through the owner's reference.

Patch 1 prepares the zl3073x driver whose ref_sync_set callback had
per-channel behavior (setting priority on a single DPLL channel). It now
iterates all channels internally so it remains correct when invoked only
once.

Patch 2 drops the xa_for_each loops from dpll_pin_freq_set(),
dpll_pin_esync_set(), dpll_pin_ref_sync_state_set() and
dpll_pin_phase_adj_set(), along with the rollback logic and the per-ref
-EOPNOTSUPP validation scan. The dpll.rst documentation is updated to
reflect the new behavior.

v3:
  - Fix missing mutex_unlock on the normal loop path (was only on
    the early-continue branch) - folded into patch 1
  - Skip channels where sync ref priority is already NONE
  - Send __dpll_pin_change_ntf for the sync pin when priority changed
  - Reword dpll.rst to describe observable effect, restore rationale
  - Align changelog: "recommended" instead of "must"
  - Use 7-bit ASCII throughout
  - Rebase onto current origin/main
v2:
  - Split zl3073x ref_sync_set fix into a separate preparation patch
  - Update dpll.rst documentation to match the new behavior
  - Expand commit message to describe the -EOPNOTSUPP check change

Ivan Vecera (2):
  dpll: zl3073x: update all DPLL channels on ref_sync_set
  dpll: use pin owner's dpll ref for pin-level attribute setting

 Documentation/driver-api/dpll.rst |  11 +-
 drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
 drivers/dpll/zl3073x/dpll.c       |  58 ++++++--
 3 files changed, 104 insertions(+), 178 deletions(-)


base-commit: 4fa4977a0d900f936bcae5cd2c510be5554e8dd6
-- 
2.54.0

[PATCH net-next v3 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

[Ivan Vecera]

zl3073x_dpll_input_pin_ref_sync_set() excludes the sync source from
automatic reference selection by setting its priority to NONE, but
currently only does this on the single DPLL channel whose pin_priv
was passed to the callback.

Since input pins are registered with every DPLL channel, the datasheet
recommends covering all channels to prevent the sync source from
remaining a selectable candidate on the other channels. This is
a preparation for the following patch which changes the DPLL core to
invoke pin-level set callbacks only through the pin owner's reference
instead of iterating over all registered DPLL devices.

Replace the single-channel priority write with a list_for_each_entry()
loop over all DPLL channels. Each channel's lock is acquired
individually for its read-modify-write sequence. The guard(mutex) is
replaced with explicit mutex_lock/mutex_unlock to allow releasing the
owner's lock before iterating, avoiding nested locking of the same
mutex class. A change notification is sent for the sync pin if any
channel's priority was actually modified.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 58 ++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 0488ae6ac486c8..83bd3027dbaa1e 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -263,9 +263,10 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 	u8 mode, ref_id, sync_ref_id;
 	struct zl3073x_chan chan;
 	struct zl3073x_ref ref;
+	bool sync_ntf = false;
 	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	ref_id = zl3073x_input_pin_ref_get(pin->id);
 	sync_ref_id = zl3073x_input_pin_ref_get(sync_pin->id);
@@ -285,17 +286,20 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 		if (sync_freq > 8000) {
 			NL_SET_ERR_MSG(extack,
 				       "sync frequency must be 8 kHz or less");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 		if (ref_freq < 1000) {
 			NL_SET_ERR_MSG(extack,
 				       "clock frequency must be 1 kHz or more");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 		if (ref_freq <= sync_freq) {
 			NL_SET_ERR_MSG(extack,
 				       "clock frequency must be higher than sync frequency");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 
 		zl3073x_ref_sync_pair_set(&ref, sync_ref_id);
@@ -308,20 +312,54 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 
 	rc = zl3073x_ref_state_set(zldev, ref_id, &ref);
 	if (rc)
-		return rc;
+		goto unlock;
 
-	/* Exclude sync source from automatic reference selection by setting
-	 * its priority to NONE. On disconnect the priority is left as NONE
-	 * and the user must explicitly make the pin selectable again.
+	/* All code paths accessing per-channel reference priorities are
+	 * serialized by the subsystem dpll_lock, so it is safe to release
+	 * our lock here before iterating over the other channels.
 	 */
-	if (state == DPLL_PIN_STATE_CONNECTED) {
+	mutex_unlock(&zldpll->lock);
+
+	if (state != DPLL_PIN_STATE_CONNECTED)
+		return 0;
+
+	/* The datasheet recommends excluding the sync source from automatic
+	 * reference selection by setting its priority to NONE on all DPLL
+	 * channels. This is advisory - the ref sync pair is already
+	 * configured, so a failure here is not fatal. On disconnect the
+	 * priority is left as NONE and the user must explicitly make the
+	 * pin selectable again.
+	 */
+	list_for_each_entry(zldpll, &zldev->dplls, list) {
+		u8 prio;
+
+		mutex_lock(&zldpll->lock);
+
 		chan = *zl3073x_chan_state_get(zldev, zldpll->id);
+		prio = zl3073x_chan_ref_prio_get(&chan, sync_ref_id);
+		if (prio == ZL_DPLL_REF_PRIO_NONE) {
+			mutex_unlock(&zldpll->lock);
+			continue; /* Ref is already non-selectable */
+		}
+
 		zl3073x_chan_ref_prio_set(&chan, sync_ref_id,
 					  ZL_DPLL_REF_PRIO_NONE);
-		return zl3073x_chan_state_set(zldev, zldpll->id, &chan);
+		if (zl3073x_chan_state_set(zldev, zldpll->id, &chan))
+			dev_warn(zldev->dev,
+				 "Failed to set ref prio on DPLL%u\n",
+				 zldpll->id);
+		else
+			sync_ntf = true;
+
+		mutex_unlock(&zldpll->lock);
 	}
+	if (sync_ntf)
+		__dpll_pin_change_ntf(sync_pin->dpll_pin);
 
 	return 0;
+unlock:
+	mutex_unlock(&zldpll->lock);
+	return rc;
 }
 
 static int
-- 
2.54.0

Re: [PATCH net-next v3 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

[Ivan Vecera]

Sahisko findings and my replies...

 > Is the loss of the error return here intentional?
 > ...
 > isn't the pair being already armed exactly the reason the sync input
 > must not stay selectable?

Yes, this is intentional. The ref sync pair is configured by the
preceding zl3073x_ref_state_set() call and is fully functional at
that point - the priority exclusion is a separate, advisory step
recommended by the datasheet. It prevents the automatic selection
algorithm from picking the sync source as a clock reference, which
would be counterproductive but does not affect the sync pair itself.

A failure in zl3073x_chan_state_set() here means a mailbox or bus
error, which is highly unlikely during normal operation and would
indicate a serious hardware problem that surfaces through other
error paths as well. Propagating it to userspace would falsely
suggest that the ref sync pair was not configured, when in fact it
was. The dev_warn is sufficient to flag the condition for debugging.

The old code propagated the error only because it was a single
return statement at the end of the function - not because the
priority update was considered essential for the operation to
succeed.

Ivan

Re: [PATCH net-next v3 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

[Petr Oros]

On 8/7/26 11:59, Ivan Vecera wrote:
> zl3073x_dpll_input_pin_ref_sync_set() excludes the sync source from
> automatic reference selection by setting its priority to NONE, but
> currently only does this on the single DPLL channel whose pin_priv
> was passed to the callback.
>
> Since input pins are registered with every DPLL channel, the datasheet
> recommends covering all channels to prevent the sync source from
> remaining a selectable candidate on the other channels. This is
> a preparation for the following patch which changes the DPLL core to
> invoke pin-level set callbacks only through the pin owner's reference
> instead of iterating over all registered DPLL devices.
>
> Replace the single-channel priority write with a list_for_each_entry()
> loop over all DPLL channels. Each channel's lock is acquired
> individually for its read-modify-write sequence. The guard(mutex) is
> replaced with explicit mutex_lock/mutex_unlock to allow releasing the
> owner's lock before iterating, avoiding nested locking of the same
> mutex class. A change notification is sent for the sync pin if any
> channel's priority was actually modified.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/dpll.c | 58 ++++++++++++++++++++++++++++++-------
>   1 file changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 0488ae6ac486c8..83bd3027dbaa1e 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -263,9 +263,10 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
>   	u8 mode, ref_id, sync_ref_id;
>   	struct zl3073x_chan chan;
>   	struct zl3073x_ref ref;
> +	bool sync_ntf = false;
>   	int rc;
>   
> -	guard(mutex)(&zldpll->lock);
> +	mutex_lock(&zldpll->lock);
>   
>   	ref_id = zl3073x_input_pin_ref_get(pin->id);
>   	sync_ref_id = zl3073x_input_pin_ref_get(sync_pin->id);
> @@ -285,17 +286,20 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
>   		if (sync_freq > 8000) {
>   			NL_SET_ERR_MSG(extack,
>   				       "sync frequency must be 8 kHz or less");
> -			return -EINVAL;
> +			rc = -EINVAL;
> +			goto unlock;
>   		}
>   		if (ref_freq < 1000) {
>   			NL_SET_ERR_MSG(extack,
>   				       "clock frequency must be 1 kHz or more");
> -			return -EINVAL;
> +			rc = -EINVAL;
> +			goto unlock;
>   		}
>   		if (ref_freq <= sync_freq) {
>   			NL_SET_ERR_MSG(extack,
>   				       "clock frequency must be higher than sync frequency");
> -			return -EINVAL;
> +			rc = -EINVAL;
> +			goto unlock;
>   		}
>   
>   		zl3073x_ref_sync_pair_set(&ref, sync_ref_id);
> @@ -308,20 +312,54 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
>   
>   	rc = zl3073x_ref_state_set(zldev, ref_id, &ref);
>   	if (rc)
> -		return rc;
> +		goto unlock;
>   
> -	/* Exclude sync source from automatic reference selection by setting
> -	 * its priority to NONE. On disconnect the priority is left as NONE
> -	 * and the user must explicitly make the pin selectable again.
> +	/* All code paths accessing per-channel reference priorities are
> +	 * serialized by the subsystem dpll_lock, so it is safe to release
> +	 * our lock here before iterating over the other channels.
>   	 */
> -	if (state == DPLL_PIN_STATE_CONNECTED) {
> +	mutex_unlock(&zldpll->lock);
> +
> +	if (state != DPLL_PIN_STATE_CONNECTED)
> +		return 0;
> +
> +	/* The datasheet recommends excluding the sync source from automatic
> +	 * reference selection by setting its priority to NONE on all DPLL
> +	 * channels. This is advisory - the ref sync pair is already
> +	 * configured, so a failure here is not fatal. On disconnect the
> +	 * priority is left as NONE and the user must explicitly make the
> +	 * pin selectable again.
> +	 */
> +	list_for_each_entry(zldpll, &zldev->dplls, list) {
> +		u8 prio;
> +
> +		mutex_lock(&zldpll->lock);
> +
>   		chan = *zl3073x_chan_state_get(zldev, zldpll->id);
> +		prio = zl3073x_chan_ref_prio_get(&chan, sync_ref_id);
> +		if (prio == ZL_DPLL_REF_PRIO_NONE) {
> +			mutex_unlock(&zldpll->lock);
> +			continue; /* Ref is already non-selectable */
> +		}
> +
>   		zl3073x_chan_ref_prio_set(&chan, sync_ref_id,
>   					  ZL_DPLL_REF_PRIO_NONE);
> -		return zl3073x_chan_state_set(zldev, zldpll->id, &chan);
> +		if (zl3073x_chan_state_set(zldev, zldpll->id, &chan))
> +			dev_warn(zldev->dev,
> +				 "Failed to set ref prio on DPLL%u\n",
> +				 zldpll->id);
> +		else
> +			sync_ntf = true;
> +
> +		mutex_unlock(&zldpll->lock);
>   	}
> +	if (sync_ntf)
> +		__dpll_pin_change_ntf(sync_pin->dpll_pin);
>   
>   	return 0;
> +unlock:
> +	mutex_unlock(&zldpll->lock);
> +	return rc;
>   }
>   
>   static int

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

[PATCH net-next v3 2/2] dpll: use pin owner's dpll ref for pin-level attribute setting

[Ivan Vecera]

Pin-level attributes (frequency, phase adjust, embedded sync, reference
sync) are properties of the pin itself, not of a particular DPLL device.
The get callbacks already use only the pin owner's DPLL reference
(via dpll_pin_own_dpll_ref_first()), but the set callbacks iterate over
all registered DPLL references and invoke the set operation on each one.

This is redundant because a pin is a single physical entity - setting
its frequency or phase adjust once through the owner's ops is sufficient.
Calling set on every registered DPLL just results in duplicate HW writes
for drivers that share a pin across multiple DPLL devices (e.g. ice
registers each input pin with both the EEC and PPS DPLL, zl3073x
registers input pins with every DPLL channel).

Simplify dpll_pin_freq_set(), dpll_pin_esync_set(),
dpll_pin_ref_sync_state_set() and dpll_pin_phase_adj_set() to call the
set callback only through the owner's DPLL reference, matching the
existing get-side behavior. This removes the xa_for_each iteration
loops, the now-unnecessary rollback logic, and several local variables.

The -EOPNOTSUPP validation loop, which checked ops support across all
owner-matching references, is replaced with a direct check on the
single owner reference returned by dpll_pin_own_dpll_ref_first().

The documentation in dpll.rst is updated to reflect that pin-level
attributes are set through the pin owner's dpll reference only.

No existing driver is affected:
  - ptp_ocp and mlx5 register each pin with a single DPLL.
  - ice registers input pins with two DPLLs (EEC and PPS) using
    identical ops and pin_priv; the set callbacks address the HW by
    pin index, not by DPLL, so the second call was a no-op.
  - zl3073x registers input pins with every DPLL channel; the set
    callbacks address HW by pin/ref ID regardless of DPLL. The
    ref_sync_set callback was the only one with per-channel behavior,
    addressed by the preceding patch.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 Documentation/driver-api/dpll.rst |  11 +-
 drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
 2 files changed, 56 insertions(+), 168 deletions(-)

diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
index f83150917814e2..7c117ae37cc1bc 100644
--- a/Documentation/driver-api/dpll.rst
+++ b/Documentation/driver-api/dpll.rst
@@ -116,8 +116,9 @@ Shared pins
 A single pin object can be attached to multiple dpll devices.
 Then there are two groups of configuration knobs:
 
-1) Set on a pin - the configuration affects all dpll devices pin is
-   registered to (i.e., ``DPLL_A_PIN_FREQUENCY``),
+1) Set on a pin - the configuration is a property of the pin itself and
+   applies to all dpll devices the pin is registered with
+   (i.e., ``DPLL_A_PIN_FREQUENCY``),
 2) Set on a pin-dpll tuple - the configuration affects only selected
    dpll device (i.e., ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE``,
    ``DPLL_A_PIN_DIRECTION``).
@@ -507,9 +508,9 @@ as well as parameter being configured (``DPLL_A_MODE``).
 ``DPLL_CMD_PIN_SET`` - to target a pin user must provide a
 ``DPLL_A_PIN_ID``, which is unique identifier of a pin in the system.
 Also configured pin parameters must be added.
-If ``DPLL_A_PIN_FREQUENCY`` is configured, this affects all the dpll
-devices that are connected with the pin, that is why frequency attribute
-shall not be enclosed in ``DPLL_A_PIN_PARENT_DEVICE``.
+If ``DPLL_A_PIN_FREQUENCY`` is configured, it is a property of the pin
+itself and applies to all dpll devices the pin is registered with, so the
+frequency attribute shall not be enclosed in ``DPLL_A_PIN_PARENT_DEVICE``.
 Other attributes: ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE`` or
 ``DPLL_A_PIN_DIRECTION`` must be enclosed in
 ``DPLL_A_PIN_PARENT_DEVICE`` as their configuration relates to only one
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index afb31c0040382c..a909cd4451b008 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -1079,10 +1079,9 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		  struct netlink_ext_ack *extack)
 {
 	u64 freq = nla_get_u64(a), old_freq;
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	if (!dpll_pin_is_freq_supported(pin, freq)) {
@@ -1090,22 +1089,17 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->frequency_set || !ops->frequency_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack,
-				       "frequency set not supported by the device");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->frequency_set || !ops->frequency_get) {
+		NL_SET_ERR_MSG(extack,
+			       "frequency set not supported by the device");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
 				 dpll_priv(dpll), &old_freq, extack);
@@ -1116,68 +1110,42 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 	if (freq == old_freq)
 		return 0;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->frequency_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					 dpll, dpll_priv(dpll), freq, extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack, "frequency set failed for dpll_id:%u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				 dpll, dpll_priv(dpll), freq, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "frequency set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
-
-rollback:
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		if (ref == failed)
-			break;
-		ops = dpll_pin_ops(ref);
-		if (!ops->frequency_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				       dpll, dpll_priv(dpll), old_freq, extack))
-			NL_SET_ERR_MSG(extack, "set frequency rollback failed");
-	}
-	return ret;
 }
 
 static int
 dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 		   struct netlink_ext_ack *extack)
 {
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
 	struct dpll_pin_esync esync;
 	u64 freq = nla_get_u64(a);
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
 	bool supported = false;
-	unsigned long i;
-	int ret;
+	int ret, i;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->esync_set || !ops->esync_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack,
-				       "embedded sync feature is not supported by this device");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->esync_set || !ops->esync_get) {
+		NL_SET_ERR_MSG(extack,
+			       "embedded sync feature is not supported by this device");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
 			     dpll_priv(dpll), &esync, extack);
@@ -1196,44 +1164,17 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		void *pin_dpll_priv;
-
-		ops = dpll_pin_ops(ref);
-		if (!ops->esync_set)
-			continue;
-		dpll = ref->dpll;
-		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
-		ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
-				      freq, extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack,
-					   "embedded sync frequency set failed for dpll_id: %u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->esync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+			     dpll_priv(dpll), freq, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "embedded sync frequency set failed for dpll_id: %u",
+				   dpll->id);
+		return ret;
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
-
-rollback:
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		void *pin_dpll_priv;
-
-		if (ref == failed)
-			break;
-		ops = dpll_pin_ops(ref);
-		if (!ops->esync_set)
-			continue;
-		dpll = ref->dpll;
-		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
-		if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
-				   esync.freq, extack))
-			NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed");
-	}
-	return ret;
 }
 
 static int
@@ -1241,14 +1182,12 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 			    unsigned long ref_sync_pin_idx,
 			    const enum dpll_pin_state state,
 			    struct netlink_ext_ack *extack)
-
 {
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
 	enum dpll_pin_state old_state;
 	struct dpll_pin *ref_sync_pin;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	ref_sync_pin = xa_find(&pin->ref_sync_pins, &ref_sync_pin_idx,
@@ -1282,42 +1221,20 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 	}
 	if (state == old_state)
 		return 0;
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->ref_sync_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					ref_sync_pin,
-					dpll_pin_on_dpll_priv(dpll,
-							      ref_sync_pin),
-					state, extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack, "reference sync set failed for dpll_id:%u",
-					   dpll->id);
-			goto rollback;
-		}
+
+	ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				ref_sync_pin,
+				dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
+				state, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "reference sync set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
-
-rollback:
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		if (ref == failed)
-			break;
-		ops = dpll_pin_ops(ref);
-		if (!ops->ref_sync_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				      ref_sync_pin,
-				      dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
-				      old_state, extack))
-			NL_SET_ERR_MSG(extack, "set reference sync rollback failed");
-	}
-	return ret;
 }
 
 static int
@@ -1478,11 +1395,10 @@ static int
 dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		       struct netlink_ext_ack *extack)
 {
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
 	s32 phase_adj, old_phase_adj;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	phase_adj = nla_get_s32(phase_adj_attr);
@@ -1499,21 +1415,16 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->phase_adjust_set || !ops->phase_adjust_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack, "phase adjust not supported");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->phase_adjust_set || !ops->phase_adjust_get) {
+		NL_SET_ERR_MSG(extack, "phase adjust not supported");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
 				    dpll, dpll_priv(dpll), &old_phase_adj,
@@ -1525,41 +1436,17 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 	if (phase_adj == old_phase_adj)
 		return 0;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->phase_adjust_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->phase_adjust_set(pin,
-					    dpll_pin_on_dpll_priv(dpll, pin),
-					    dpll, dpll_priv(dpll), phase_adj,
-					    extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack,
-					   "phase adjust set failed for dpll_id:%u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll), phase_adj, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "phase adjust set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
-
-rollback:
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		if (ref == failed)
-			break;
-		ops = dpll_pin_ops(ref);
-		if (!ops->phase_adjust_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					  dpll, dpll_priv(dpll), old_phase_adj,
-					  extack))
-			NL_SET_ERR_MSG(extack, "set phase adjust rollback failed");
-	}
-	return ret;
 }
 
 static int
-- 
2.54.0

Re: [PATCH net-next v3 2/2] dpll: use pin owner's dpll ref for pin-level attribute setting

[Jiri Pirko]

Fri, Aug 07, 2026 at 11:59:26AM +0200, ivecera@redhat.com wrote:
>Pin-level attributes (frequency, phase adjust, embedded sync, reference
>sync) are properties of the pin itself, not of a particular DPLL device.
>The get callbacks already use only the pin owner's DPLL reference
>(via dpll_pin_own_dpll_ref_first()), but the set callbacks iterate over
>all registered DPLL references and invoke the set operation on each one.
>
>This is redundant because a pin is a single physical entity - setting
>its frequency or phase adjust once through the owner's ops is sufficient.
>Calling set on every registered DPLL just results in duplicate HW writes
>for drivers that share a pin across multiple DPLL devices (e.g. ice
>registers each input pin with both the EEC and PPS DPLL, zl3073x
>registers input pins with every DPLL channel).
>
>Simplify dpll_pin_freq_set(), dpll_pin_esync_set(),
>dpll_pin_ref_sync_state_set() and dpll_pin_phase_adj_set() to call the
>set callback only through the owner's DPLL reference, matching the
>existing get-side behavior. This removes the xa_for_each iteration
>loops, the now-unnecessary rollback logic, and several local variables.
>
>The -EOPNOTSUPP validation loop, which checked ops support across all
>owner-matching references, is replaced with a direct check on the
>single owner reference returned by dpll_pin_own_dpll_ref_first().
>
>The documentation in dpll.rst is updated to reflect that pin-level
>attributes are set through the pin owner's dpll reference only.
>
>No existing driver is affected:
>  - ptp_ocp and mlx5 register each pin with a single DPLL.
>  - ice registers input pins with two DPLLs (EEC and PPS) using
>    identical ops and pin_priv; the set callbacks address the HW by
>    pin index, not by DPLL, so the second call was a no-op.
>  - zl3073x registers input pins with every DPLL channel; the set
>    callbacks address HW by pin/ref ID regardless of DPLL. The
>    ref_sync_set callback was the only one with per-channel behavior,
>    addressed by the preceding patch.
>
>Signed-off-by: Ivan Vecera <ivecera@redhat.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next v3 0/2] dpll: use pin owner's dpll ref for pin-level set callbacks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  7 Aug 2026 11:59:24 +0200 you wrote:
> Pin-level attributes (frequency, phase adjust, embedded sync, reference
> sync) are properties of the pin itself. The get callbacks already use
> only the pin owner's DPLL reference, but the set callbacks iterate over
> all registered DPLL devices, resulting in redundant HW writes for
> drivers that share a pin across multiple DPLLs.
> 
> This series simplifies the set side to match the get side: call the set
> callback only through the owner's reference.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set
    https://git.kernel.org/netdev/net-next/c/346630e46b38
  - [net-next,v3,2/2] dpll: use pin owner's dpll ref for pin-level attribute setting
    https://git.kernel.org/netdev/net-next/c/84e85c325e5e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html