[PATCH net v3 0/4] dpll: zl3073x: fix output pin frequency, esync and sibling notifications

[PATCH net v3 0/4] dpll: zl3073x: fix output pin frequency, esync and sibling notifications

[Ivan Vecera]

This series fixes several issues in the zl3073x output pin handling.

Patch 1 rejects output frequencies whose divisor is too small for the
hardware (less than 2, or less than 4 for the N-pin of an N-divided
output) as well as a zero frequency; such frequencies were accepted and
offered to userspace before.

Patch 2 makes the embedded sync (eSync) configuration follow the output
carrier frequency. Changing an output pin's frequency did not update its
eSync period/width, so eSync ran at the wrong rate and, for a 1 Hz
carrier, could get stuck on because esync_get() hides it. This was hit
during development and confirmed on an oscilloscope.

Patch 3 rejects N-divided output frequencies that cannot be represented
exactly. Such requests used to be silently rounded, which could also
change the sibling N-pin frequency.

Patch 4 notifies the sibling pin of an output pair when a shared HW
setting (divisor, clock type, eSync period/width, phase compensation) is
changed through one pin, so userspace is told the change happened. It
also ties the zldpll->pins list membership to the DPLL registration
state so the sibling lookup cannot miss a registered pin during probe or
teardown.

Changes since v2:
- Added patch 1 ("reject output frequencies with too small divisor").
- Added patch 3 ("reject inexact frequencies for N-divided outputs"),
  ordered before the notification patch so that a successful P-pin
  frequency change is guaranteed to keep the N-pin frequency unchanged.
- Patch 2: simplify zl3073x_out_esync_is_enabled(); reword the
  esync_get() readback note to only claim what it does - it reports the
  driver's nominal 1 Hz / 25% eSync model and does not faithfully report
  a configuration the driver did not program.
- Patch 4: add/remove pins from zldpll->pins around dpll_pin_register()/
  dpll_pin_unregister() so a registered sibling is always findable by the
  notification lookup (fixes a lost notification during probe/teardown
  pointed out in the v2 review).

Changes since v1:
- Patch 2: pass the actual output carrier (synth_freq / out.div) to
  zl3073x_out_esync_enable() in the frequency change path, matching
  esync_set() (no functional change, output frequencies always divide
  the synth frequency); drop the redundant "? true : false" reported by
  coccinelle (boolconv); note in the commit message that esync_get() now
  reports the fixed 1 Hz / 25% values the driver programs; fix a comment
  typo.
- Patch 4: narrow the commit message wording about zldpll->lock to the
  new sibling lookup helper (no code change).
- Both original commit messages now describe how the issues were found
  and whether they were triggered, as requested on the v1 thread.

v2: https://lore.kernel.org/netdev/20261002074520.30048-1-ivecera@redhat.com/
v1: https://lore.kernel.org/netdev/20261001080648.1424172-1-ivecera@redhat.com/

Ivan Vecera (4):
  dpll: zl3073x: reject output frequencies with too small divisor
  dpll: zl3073x: make embedded sync follow the output carrier frequency
  dpll: zl3073x: reject inexact frequencies for N-divided outputs
  dpll: zl3073x: notify sibling pin when shared output config changes

 drivers/dpll/zl3073x/dpll.c | 244 +++++++++++++++++++++++++++---------
 drivers/dpll/zl3073x/out.h  |  36 ++++++
 drivers/dpll/zl3073x/prop.c |  29 +++--
 3 files changed, 241 insertions(+), 68 deletions(-)

-- 
2.55.0

[PATCH net v3 1/4] dpll: zl3073x: reject output frequencies with too small divisor

[Ivan Vecera]

The output divider has to be at least 2. In N-pin divide mode the
per-output divider must be 2 or more and the N-pin is divided once more
(also by 2 or more), so the N-pin needs an effective divisor of at least
4.

zl3073x_pin_check_freq() only checked that an output frequency divides the
synth frequency, so it accepted frequencies that would require a smaller
divisor. Reject them: require a divisor of at least 2 for normal outputs
and the P-pin of N-divided outputs, and 4 for the N-pin. Also reject a
zero frequency, which would otherwise divide by zero.

Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/prop.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
index ac9d41d0f978..a4bdc3878590 100644
--- a/drivers/dpll/zl3073x/prop.c
+++ b/drivers/dpll/zl3073x/prop.c
@@ -22,7 +22,8 @@
  * The function checks the given frequency is valid for the device. For input
  * pins it checks that the frequency can be factorized using supported base
  * frequencies. For output pins it checks that the frequency divides connected
- * synth frequency without remainder.
+ * synth frequency without remainder and that the resulting divisor is within
+ * the range supported by the hardware.
  *
  * Return: true if the frequency is valid, false if not.
  */
@@ -30,7 +31,7 @@ static bool
 zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
 		       u8 id, u64 freq)
 {
-	if (freq > U32_MAX)
+	if (!freq || freq > U32_MAX)
 		goto err_inv_freq;
 
 	if (dir == DPLL_PIN_DIRECTION_INPUT) {
@@ -41,18 +42,32 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
 		if (rc)
 			goto err_inv_freq;
 	} else {
-		u32 synth_freq;
-		u8 out, synth;
+		const struct zl3073x_out *out;
+		u32 synth_freq, div, min_div, rem;
+		u8 out_id, synth;
 
 		/* Get output pin synthesizer */
-		out = zl3073x_output_pin_out_get(id);
-		synth = zl3073x_dev_out_synth_get(zldev, out);
+		out_id = zl3073x_output_pin_out_get(id);
+		synth = zl3073x_dev_out_synth_get(zldev, out_id);
 
 		/* Get synth frequency */
 		synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
 
 		/* Check the frequency divides synth frequency */
-		if (synth_freq % (u32)freq)
+		div = div_u64_rem(synth_freq, freq, &rem);
+		if (rem)
+			goto err_inv_freq;
+
+		/* The output divisor has to be at least 2. For the N-pin of an
+		 * N-divided output the effective divisor also includes the N
+		 * divider (>= 2), so the minimum is 4.
+		 */
+		out = zl3073x_out_state_get(zldev, out_id);
+		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
+			min_div = 4;
+		else
+			min_div = 2;
+		if (div < min_div)
 			goto err_inv_freq;
 	}
 
-- 
2.55.0

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

[Ivan Vecera]

Changing an output pin's frequency did not update its embedded sync
(eSync) configuration. The eSync period and width are computed relative
to the carrier:

    esync_n_period = out_freq / esync_freq
    esync_n_width  = div / 2

Once zl3073x_dpll_output_pin_frequency_set() changes div, both values no
longer match the new carrier, so the embedded sync runs at the wrong
frequency and duty cycle. Worse, if the new carrier is <= 1 Hz then
esync_get() returns -EOPNOTSUPP (out_freq <= 1), hiding the stale ESYNC
mode so the user can no longer turn it off - it is stuck on.

This was hit during development: after changing an output pin's
frequency the embedded sync output stopped working correctly, as
confirmed on an oscilloscope - it ran at the wrong frequency and duty
cycle because the eSync period and width still matched the old carrier.
For a new 1 Hz carrier esync_get() returned -EOPNOTSUPP, leaving eSync
enabled with no way to disable it.

Factor the eSync handling into zl3073x_out_esync_{is_enabled,enable,
disable}() helpers and reuse them from esync_get()/esync_set(), then
adjust the eSync parameters in the output frequency change path for
non-N-divided outputs after updating div/width:

- If the new frequency is 1 Hz, disable eSync. It cannot work at a 1 Hz
  carrier and esync_get() hides it, so it must be cleared rather than
  left stuck on.
- Otherwise, if eSync is active, recompute its period and width for the
  new carrier keeping the 1 Hz eSync frequency.

As a side effect of reusing the helpers, esync_get() now reports the
driver's eSync model - a 1 Hz frequency and a 25% pulse - from the clock
type when eSync is enabled, instead of deriving the values from the
cached esync_n_period/esync_n_width registers. The driver only ever
programs a 1 Hz / 25% eSync, so this matches what esync_set() configures
and keeps get and set symmetric. A configuration the driver did not
establish - a non-1 Hz eSync left in flash or by an older kernel, or the
truncated duty cycle of an odd divider - is therefore reported as the
nominal 1 Hz / 25% rather than its exact register values.

N-divided outputs are left untouched: there esync_n_period is the N
divider and eSync is not supported anyway.

Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 68 ++++++++++++++++---------------------
 drivers/dpll/zl3073x/out.h  | 36 ++++++++++++++++++++
 2 files changed, 66 insertions(+), 38 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40..7c997966c3c3 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -852,7 +852,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll_pin *pin = pin_priv;
 	const struct zl3073x_synth *synth;
 	const struct zl3073x_out *out;
-	u32 synth_freq, out_freq;
+	u32 synth_freq;
 	u8 out_id;
 
 	guard(mutex)(&zldpll->lock);
@@ -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;
 
 	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;
+	} else {
 		esync->freq = 0;
 		esync->pulse = 0;
-
-		return 0;
 	}
 
-	/* Compute esync frequency */
-	esync->freq = out_freq / out->esync_n_period;
-
-	/* By comparing the esync_pulse_width to the half of the pulse width
-	 * the esync pulse percentage can be determined.
-	 * Note that half pulse width is in units of half synth cycles, which
-	 * is why it reduces down to be output_div.
-	 */
-	esync->pulse = (50 * out->esync_n_width) / out->div;
-
 	return 0;
 }
 
@@ -926,32 +916,17 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	if (zl3073x_out_is_ndiv(&out))
 		return -EOPNOTSUPP;
 
-	/* Update clock type in output mode */
-	if (freq)
-		zl3073x_out_clock_type_set(&out,
-					   ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
-	else
-		zl3073x_out_clock_type_set(&out,
-					   ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
-
-	/* If esync is being disabled just write mailbox and finish */
-	if (!freq)
+	if (!freq) {
+		zl3073x_out_esync_disable(&out);
 		return zl3073x_out_state_set(zldev, out_id, &out);
+	}
 
 	/* Get attached synth frequency */
 	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(&out));
 	synth_freq = zl3073x_synth_freq_get(synth);
 
-	/* Compute and update esync period */
-	out.esync_n_period = synth_freq / (u32)freq / out.div;
-
-	/* Half of the period in units of 1/2 synth cycle can be represented by
-	 * the output_div. To get the supported esync pulse width of 25% of the
-	 * period the output_div can just be divided by 2. Note that this
-	 * assumes that output_div is even, otherwise some resolution will be
-	 * lost.
-	 */
-	out.esync_n_width = out.div / 2;
+	/* Enable 1PPS eSync for this pin frequency */
+	zl3073x_out_esync_enable(&out, synth_freq / out.div);
 
 	/* Commit output configuration */
 	return zl3073x_out_state_set(zldev, out_id, &out);
@@ -1009,6 +984,23 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		/* For 50/50 duty cycle the divisor is equal to width */
 		out.width = new_div;
 
+		/* The embedded sync period and width are computed relative to
+		 * the output carrier so they have to be adjusted to follow the
+		 * new frequency.
+		 */
+		if (zl3073x_out_esync_is_enabled(&out)) {
+			if (frequency == 1) {
+				/* Disable eSync if the new frequency is 1 Hz */
+				zl3073x_out_esync_disable(&out);
+			} else {
+				/* Update esync period and width according to
+				 * the new frequency.
+				 */
+				zl3073x_out_esync_enable(&out,
+							 synth_freq / out.div);
+			}
+		}
+
 		/* Commit output configuration */
 		return zl3073x_out_state_set(zldev, out_id, &out);
 	}
diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
index 660889c57bff..d2b8b4eb5a76 100644
--- a/drivers/dpll/zl3073x/out.h
+++ b/drivers/dpll/zl3073x/out.h
@@ -134,4 +134,40 @@ static inline u8 zl3073x_out_synth_get(const struct zl3073x_out *out)
 	return FIELD_GET(ZL_OUTPUT_CTRL_SYNTH_SEL, out->ctrl);
 }
 
+static inline bool zl3073x_out_esync_is_enabled(const struct zl3073x_out *out)
+{
+	u8 clock_type = zl3073x_out_clock_type_get(out);
+
+	return clock_type == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC;
+}
+
+static inline
+void zl3073x_out_esync_enable(struct zl3073x_out *out, u32 base_freq)
+{
+	/* Period is expressed as the number of output divider clock cycles.
+	 * So for 1 Hz the period is equal to base frequency.
+	 */
+	out->esync_n_period = base_freq;
+
+	/* Half of the period in units of 1/2 synth cycle can be represented by
+	 * the output_div. To get the supported esync pulse width of 25% of the
+	 * period the output_div can just be divided by 2. Note that this
+	 * assumes that output_div is even, otherwise some resolution will be
+	 * lost.
+	 */
+	out->esync_n_width = out->div / 2;
+
+	/* Update the clock type */
+	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
+}
+
+static inline
+void zl3073x_out_esync_disable(struct zl3073x_out *out)
+{
+	/* Just update the clock type. The eSync period and width are
+	 * ignored for this clock type.
+	 */
+	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
+}
+
 #endif /* _ZL3073X_OUT_H */
-- 
2.55.0

[PATCH net v3 3/4] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Ivan Vecera]

For N-divided outputs the P-pin and N-pin share the output divisor and
the N-pin frequency is synth_freq / (div * esync_n_period). The frequency
set helper computed esync_n_period with a truncating division and only
rejected a zero result, so a frequency that does not divide evenly was
silently rounded. Setting the P-pin could then shift the N-pin frequency
too (e.g. 600 MHz synth, div=60, period=10 gives P=10 MHz, N=1 MHz;
setting P to 2.5 MHz moves N to 1.25 MHz).

Check the division remainder and reject the request if it is not exact,
and require esync_n_period >= 2 so the N-pin frequency stays below the
P-pin one.

Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 7c997966c3c3..65107b4cc4f8 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -1007,15 +1007,28 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 
 	if (zl3073x_dpll_is_p_pin(pin)) {
 		/* We are going to change output frequency for P-pin but
-		 * if the requested frequency is less than current N-pin
-		 * frequency then indicate a failure as we are not able
+		 * if the requested frequency is less or equal than current
+		 * N-pin frequency then indicate a failure as we are not able
 		 * to compute N-pin divisor to keep its frequency unchanged.
 		 *
 		 * Update divisor for N-pin to keep N-pin frequency.
 		 */
-		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
-		if (!out.esync_n_period)
+		u64 prod = mul_u32_u32(out.esync_n_period, out.div);
+		u32 rem;
+
+		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
+		if (rem != 0) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "OUT%uN freq must divide OUT%uP freq",
+					   out_id, out_id);
 			return -EINVAL;
+		}
+		if (out.esync_n_period < 2) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "OUT%uN freq must be less than OUT%uP freq",
+					   out_id, out_id);
+			return -EINVAL;
+		}
 
 		/* Update the output divisor */
 		out.div = new_div;
@@ -1030,9 +1043,21 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		 *
 		 * Update divisor for N-pin
 		 */
-		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
-		if (!out.esync_n_period)
+		u64 rem, prod = frequency * out.div;
+
+		out.esync_n_period = div64_u64_rem(synth_freq, prod, &rem);
+		if (rem != 0) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "OUT%uN freq must divide OUT%uP freq",
+					   out_id, out_id);
 			return -EINVAL;
+		}
+		if (out.esync_n_period < 2) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "OUT%uN freq must be less than OUT%uP freq",
+					   out_id, out_id);
+			return -EINVAL;
+		}
 	}
 
 	/* For 50/50 duty cycle the divisor is equal to width */
-- 
2.55.0

[PATCH net v3 4/4] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

Each zl3073x output has a P-pin and an N-pin that share a single HW
output and, outside N-pin divide mode, share the output's divisor,
clock type, esync period/width and phase compensation registers.
Changing one of these settings through one pin's dpll_pin therefore
also changes the other (sibling) pin's effective configuration, but
only the pin the change was requested on gets a dpll_pin_change_ntf()
notification - userspace listening on the sibling pin is never told
its frequency, esync configuration or phase adjustment changed.

This was found by code inspection rather than triggered at runtime: a
dpll pin-get on the sibling pin returns the correct current values, but
because no change notification is emitted for it, userspace is never
told that its configuration changed asynchronously through the other
pin.

Add zl3073x_dpll_output_pin_sibling_get() to look up the other pin of
an output pair, and use it in frequency_set() (for the non-N-divided
signal formats, where the output divisor is shared), esync_set() and
phase_adjust_set() to notify the sibling pin, if it is registered,
whenever the shared HW state actually changes.

The three callbacks are switched from guard(mutex) to explicit
mutex_lock()/mutex_unlock() with goto-based unwinding, because the
notification must run after zldpll->lock is released: the notification
re-enters the pin get callbacks, which take zldpll->lock again, so
calling it under the lock would deadlock. The DPLL subsystem's own
dpll_lock is still held across the callback, so the __ (lock-held)
notification variant is used.

The sibling lookup walks the zldpll->pins list, so the list membership
has to match the DPLL registration state. dpll_pin_register() publishes
a pin (and P is registered before N) before it used to be added to the
list, and teardown detached the whole list before unregistering any
pin, so a sibling that is registered - and thus reachable by a PIN_SET
on the other pin - could be missing from the list and never notified.
Add the pin to the list before dpll_pin_register() and remove it only
after dpll_pin_unregister(), both under zldpll->lock, and assert the
lock in the lookup helper. A pin that is transiently on the list while
not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
pin that is not available, and the sibling cannot be freed under the
lookup because the freeing goes through dpll_pin_unregister(), which
takes dpll_lock that the notification path holds.

Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 6287262f761e ("dpll: zl3073x: Add support to adjust phase")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 187 ++++++++++++++++++++++++++++--------
 1 file changed, 146 insertions(+), 41 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 65107b4cc4f8..9c678acc3e77 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -123,6 +123,8 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
 {
 	struct zl3073x_dpll_pin *pin;
 
+	lockdep_assert_held(&zldpll->lock);
+
 	list_for_each_entry(pin, &zldpll->pins, list) {
 		if (zl3073x_dpll_is_input_pin(pin) &&
 		    zl3073x_input_pin_ref_get(pin->id) == ref_id)
@@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
 	return NULL;
 }
 
+/**
+ * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
+ * @pin: output pin whose sibling is sought
+ *
+ * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
+ * single HW output. Looks up the other pin of the pair, if it is
+ * registered as a dpll_pin on this DPLL.
+ *
+ * Return: pointer to sibling pin, or NULL if it is not registered
+ */
+static struct zl3073x_dpll_pin *
+zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
+{
+	struct zl3073x_dpll_pin *sibling;
+
+	lockdep_assert_held(&pin->dpll->lock);
+
+	list_for_each_entry(sibling, &pin->dpll->pins, list) {
+		if (!zl3073x_dpll_is_input_pin(sibling) &&
+		    sibling->id == (pin->id ^ 1))
+			return sibling;
+	}
+
+	return NULL;
+}
+
 static struct zl3073x_dpll_pin *
 zl3073x_dpll_nco_pin_get(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dpll_pin *pin;
 
+	lockdep_assert_held(&zldpll->lock);
+
 	list_for_each_entry(pin, &zldpll->pins, list) {
 		if (zl3073x_dpll_is_nco_pin(pin))
 			return pin;
@@ -899,12 +929,14 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	const struct zl3073x_synth *synth;
 	struct zl3073x_out out;
 	u32 synth_freq;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -913,12 +945,14 @@ zl3073x_dpll_output_pin_esync_set(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))
-		return -EOPNOTSUPP;
+	if (zl3073x_out_is_ndiv(&out)) {
+		rc = -EOPNOTSUPP;
+		goto unlock;
+	}
 
 	if (!freq) {
 		zl3073x_out_esync_disable(&out);
-		return zl3073x_out_state_set(zldev, out_id, &out);
+		goto commit;
 	}
 
 	/* Get attached synth frequency */
@@ -927,9 +961,24 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 
 	/* Enable 1PPS eSync for this pin frequency */
 	zl3073x_out_esync_enable(&out, synth_freq / out.div);
-
+commit:
 	/* Commit output configuration */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		goto unlock;
+
+	/* The clock type, esync period and esync width are all shared by
+	 * both pins of the output pair, so the sibling pin's esync
+	 * configuration changes too and userspace has to be notified.
+	 */
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -959,12 +1008,14 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	const struct zl3073x_synth *synth;
 	u32 new_div, synth_freq;
 	struct zl3073x_out out;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -977,7 +1028,8 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	/* Check signal format */
 	if (!zl3073x_out_is_ndiv(&out)) {
 		/* For non N-divided signal formats the frequency is computed
-		 * as division of synth frequency and output divisor.
+		 * as division of synth frequency and output divisor, which
+		 * is shared by both pins of the output pair.
 		 */
 		out.div = new_div;
 
@@ -1002,7 +1054,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		}
 
 		/* Commit output configuration */
-		return zl3073x_out_state_set(zldev, out_id, &out);
+		rc = zl3073x_out_state_set(zldev, out_id, &out);
+		if (rc)
+			goto unlock;
+
+		/* The other pin's frequency changed too - it has to be
+		 * notified about the change.
+		 */
+		sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+
+		goto unlock;
 	}
 
 	if (zl3073x_dpll_is_p_pin(pin)) {
@@ -1017,18 +1078,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		u32 rem;
 
 		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
-		if (rem != 0) {
-			NL_SET_ERR_MSG_FMT(extack,
-					   "OUT%uN freq must divide OUT%uP freq",
-					   out_id, out_id);
-			return -EINVAL;
-		}
-		if (out.esync_n_period < 2) {
-			NL_SET_ERR_MSG_FMT(extack,
-					   "OUT%uN freq must be less than OUT%uP freq",
-					   out_id, out_id);
-			return -EINVAL;
-		}
+		if (rem != 0)
+			goto err_n_nondiv;
+		if (out.esync_n_period < 2)
+			goto err_n_toohigh;
 
 		/* Update the output divisor */
 		out.div = new_div;
@@ -1046,25 +1099,36 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		u64 rem, prod = frequency * out.div;
 
 		out.esync_n_period = div64_u64_rem(synth_freq, prod, &rem);
-		if (rem != 0) {
-			NL_SET_ERR_MSG_FMT(extack,
-					   "OUT%uN freq must divide OUT%uP freq",
-					   out_id, out_id);
-			return -EINVAL;
-		}
-		if (out.esync_n_period < 2) {
-			NL_SET_ERR_MSG_FMT(extack,
-					   "OUT%uN freq must be less than OUT%uP freq",
-					   out_id, out_id);
-			return -EINVAL;
-		}
+		if (rem != 0)
+			goto err_n_nondiv;
+		if (out.esync_n_period < 2)
+			goto err_n_toohigh;
 	}
 
 	/* For 50/50 duty cycle the divisor is equal to width */
 	out.esync_n_width = out.esync_n_period;
 
 	/* Commit output configuration */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
+err_n_nondiv:
+	NL_SET_ERR_MSG_FMT(extack,
+			   "OUT%uN freq must divide OUT%uP freq",
+			   out_id, out_id);
+	rc = -EINVAL;
+	goto unlock;
+err_n_toohigh:
+	NL_SET_ERR_MSG_FMT(extack,
+			   "OUT%uN freq must be less than OUT%uP freq",
+			   out_id, out_id);
+	rc = -EINVAL;
+	goto unlock;
 }
 
 static int
@@ -1103,10 +1167,12 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	struct zl3073x_out out;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -1115,7 +1181,21 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
 	out.phase_comp = phase_adjust / pin->phase_gran;
 
 	/* Update output configuration from mailbox */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		goto unlock;
+
+	/* The phase compensation register is shared by both pins of the
+	 * output pair, so the sibling pin's phase adjustment changes too.
+	 */
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -1739,6 +1819,13 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	else
 		ops = &zl3073x_dpll_output_pin_ops;
 
+	/* Add the pin to the list before registering it with the DPLL core so
+	 * that it is findable as a sibling as soon as the core publishes it.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_add(&pin->list, &zldpll->pins);
+	mutex_unlock(&zldpll->lock);
+
 	/* Register the pin */
 	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
 	if (rc)
@@ -1750,6 +1837,9 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	return 0;
 
 err_register:
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 err_pin_get:
 	pin->dpll_pin = NULL;
@@ -1784,6 +1874,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
 	/* Unregister the pin */
 	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
 
+	/* Remove the pin from the list only after it has been unregistered so
+	 * that a still-registered pin is always findable as a sibling.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
+
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 	pin->dpll_pin = NULL;
 
@@ -1803,9 +1900,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dpll_pin *pin, *next;
 
+	/* Unregister each pin before removing it from the list so that a
+	 * still-registered pin is always findable as a sibling.
+	 */
 	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
 		zl3073x_dpll_pin_unregister(pin);
-		list_del(&pin->list);
 		zl3073x_dpll_pin_free(pin);
 	}
 }
@@ -1918,16 +2017,24 @@ zl3073x_dpll_nco_pin_register(struct zl3073x_dpll *zldpll)
 		goto err_pin_get;
 	}
 
+	/* Add the pin to the list before registering it with the DPLL core so
+	 * that the list reflects the DPLL registration state.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_add(&pin->list, &zldpll->pins);
+	mutex_unlock(&zldpll->lock);
+
 	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin,
 			       &zl3073x_dpll_nco_pin_ops, pin);
 	if (rc)
 		goto err_register;
 
-	list_add(&pin->list, &zldpll->pins);
-
 	return 0;
 
 err_register:
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 err_pin_get:
 	pin->dpll_pin = NULL;
@@ -1979,8 +2086,6 @@ zl3073x_dpll_pins_register(struct zl3073x_dpll *zldpll)
 			zl3073x_dpll_pin_free(pin);
 			goto error;
 		}
-
-		list_add(&pin->list, &zldpll->pins);
 	}
 
 	/* Register NCO virtual input pin */
-- 
2.55.0

Re: [PATCH net v3 1/4] dpll: zl3073x: reject output frequencies with too small divisor

[Petr Oros]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> The output divider has to be at least 2. In N-pin divide mode the
> per-output divider must be 2 or more and the N-pin is divided once more
> (also by 2 or more), so the N-pin needs an effective divisor of at least
> 4.
> 
> zl3073x_pin_check_freq() only checked that an output frequency divides the
> synth frequency, so it accepted frequencies that would require a smaller
> divisor. Reject them: require a divisor of at least 2 for normal outputs
> and the P-pin of N-divided outputs, and 4 for the N-pin. Also reject a
> zero frequency, which would otherwise divide by zero.
> 
> Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/prop.c | 29 ++++++++++++++++++++++-------
>   1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978..a4bdc3878590 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -22,7 +22,8 @@
>    * The function checks the given frequency is valid for the device. For input
>    * pins it checks that the frequency can be factorized using supported base
>    * frequencies. For output pins it checks that the frequency divides connected
> - * synth frequency without remainder.
> + * synth frequency without remainder and that the resulting divisor is within
> + * the range supported by the hardware.
>    *
>    * Return: true if the frequency is valid, false if not.
>    */
> @@ -30,7 +31,7 @@ static bool
>   zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>   		       u8 id, u64 freq)
>   {
> -	if (freq > U32_MAX)
> +	if (!freq || freq > U32_MAX)
>   		goto err_inv_freq;
>   
>   	if (dir == DPLL_PIN_DIRECTION_INPUT) {
> @@ -41,18 +42,32 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>   		if (rc)
>   			goto err_inv_freq;
>   	} else {
> -		u32 synth_freq;
> -		u8 out, synth;
> +		const struct zl3073x_out *out;
> +		u32 synth_freq, div, min_div, rem;
> +		u8 out_id, synth;
>   
>   		/* Get output pin synthesizer */
> -		out = zl3073x_output_pin_out_get(id);
> -		synth = zl3073x_dev_out_synth_get(zldev, out);
> +		out_id = zl3073x_output_pin_out_get(id);
> +		synth = zl3073x_dev_out_synth_get(zldev, out_id);
>   
>   		/* Get synth frequency */
>   		synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
>   
>   		/* Check the frequency divides synth frequency */
> -		if (synth_freq % (u32)freq)
> +		div = div_u64_rem(synth_freq, freq, &rem);
> +		if (rem)
> +			goto err_inv_freq;
> +
> +		/* The output divisor has to be at least 2. For the N-pin of an
> +		 * N-divided output the effective divisor also includes the N
> +		 * divider (>= 2), so the minimum is 4.
> +		 */
> +		out = zl3073x_out_state_get(zldev, out_id);
> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> +			min_div = 4;
> +		else
> +			min_div = 2;
> +		if (div < min_div)
>   			goto err_inv_freq;
>   	}
>   

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

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

[Petr Oros]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> Changing an output pin's frequency did not update its embedded sync
> (eSync) configuration. The eSync period and width are computed relative
> to the carrier:
> 
>      esync_n_period = out_freq / esync_freq
>      esync_n_width  = div / 2
> 
> Once zl3073x_dpll_output_pin_frequency_set() changes div, both values no
> longer match the new carrier, so the embedded sync runs at the wrong
> frequency and duty cycle. Worse, if the new carrier is <= 1 Hz then
> esync_get() returns -EOPNOTSUPP (out_freq <= 1), hiding the stale ESYNC
> mode so the user can no longer turn it off - it is stuck on.
> 
> This was hit during development: after changing an output pin's
> frequency the embedded sync output stopped working correctly, as
> confirmed on an oscilloscope - it ran at the wrong frequency and duty
> cycle because the eSync period and width still matched the old carrier.
> For a new 1 Hz carrier esync_get() returned -EOPNOTSUPP, leaving eSync
> enabled with no way to disable it.
> 
> Factor the eSync handling into zl3073x_out_esync_{is_enabled,enable,
> disable}() helpers and reuse them from esync_get()/esync_set(), then
> adjust the eSync parameters in the output frequency change path for
> non-N-divided outputs after updating div/width:
> 
> - If the new frequency is 1 Hz, disable eSync. It cannot work at a 1 Hz
>    carrier and esync_get() hides it, so it must be cleared rather than
>    left stuck on.
> - Otherwise, if eSync is active, recompute its period and width for the
>    new carrier keeping the 1 Hz eSync frequency.
> 
> As a side effect of reusing the helpers, esync_get() now reports the
> driver's eSync model - a 1 Hz frequency and a 25% pulse - from the clock
> type when eSync is enabled, instead of deriving the values from the
> cached esync_n_period/esync_n_width registers. The driver only ever
> programs a 1 Hz / 25% eSync, so this matches what esync_set() configures
> and keeps get and set symmetric. A configuration the driver did not
> establish - a non-1 Hz eSync left in flash or by an older kernel, or the
> truncated duty cycle of an odd divider - is therefore reported as the
> nominal 1 Hz / 25% rather than its exact register values.
> 
> N-divided outputs are left untouched: there esync_n_period is the N
> divider and eSync is not supported anyway.
> 
> Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
> Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/dpll.c | 68 ++++++++++++++++---------------------
>   drivers/dpll/zl3073x/out.h  | 36 ++++++++++++++++++++
>   2 files changed, 66 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40..7c997966c3c3 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -852,7 +852,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
>   	struct zl3073x_dpll_pin *pin = pin_priv;
>   	const struct zl3073x_synth *synth;
>   	const struct zl3073x_out *out;
> -	u32 synth_freq, out_freq;
> +	u32 synth_freq;
>   	u8 out_id;
>   
>   	guard(mutex)(&zldpll->lock);
> @@ -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;
>   
>   	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;
> +	} else {
>   		esync->freq = 0;
>   		esync->pulse = 0;
> -
> -		return 0;
>   	}
>   
> -	/* Compute esync frequency */
> -	esync->freq = out_freq / out->esync_n_period;
> -
> -	/* By comparing the esync_pulse_width to the half of the pulse width
> -	 * the esync pulse percentage can be determined.
> -	 * Note that half pulse width is in units of half synth cycles, which
> -	 * is why it reduces down to be output_div.
> -	 */
> -	esync->pulse = (50 * out->esync_n_width) / out->div;
> -
>   	return 0;
>   }
>   
> @@ -926,32 +916,17 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
>   	if (zl3073x_out_is_ndiv(&out))
>   		return -EOPNOTSUPP;
>   
> -	/* Update clock type in output mode */
> -	if (freq)
> -		zl3073x_out_clock_type_set(&out,
> -					   ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
> -	else
> -		zl3073x_out_clock_type_set(&out,
> -					   ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
> -
> -	/* If esync is being disabled just write mailbox and finish */
> -	if (!freq)
> +	if (!freq) {
> +		zl3073x_out_esync_disable(&out);
>   		return zl3073x_out_state_set(zldev, out_id, &out);
> +	}
>   
>   	/* Get attached synth frequency */
>   	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(&out));
>   	synth_freq = zl3073x_synth_freq_get(synth);
>   
> -	/* Compute and update esync period */
> -	out.esync_n_period = synth_freq / (u32)freq / out.div;
> -
> -	/* Half of the period in units of 1/2 synth cycle can be represented by
> -	 * the output_div. To get the supported esync pulse width of 25% of the
> -	 * period the output_div can just be divided by 2. Note that this
> -	 * assumes that output_div is even, otherwise some resolution will be
> -	 * lost.
> -	 */
> -	out.esync_n_width = out.div / 2;
> +	/* Enable 1PPS eSync for this pin frequency */
> +	zl3073x_out_esync_enable(&out, synth_freq / out.div);
>   
>   	/* Commit output configuration */
>   	return zl3073x_out_state_set(zldev, out_id, &out);
> @@ -1009,6 +984,23 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		/* For 50/50 duty cycle the divisor is equal to width */
>   		out.width = new_div;
>   
> +		/* The embedded sync period and width are computed relative to
> +		 * the output carrier so they have to be adjusted to follow the
> +		 * new frequency.
> +		 */
> +		if (zl3073x_out_esync_is_enabled(&out)) {
> +			if (frequency == 1) {
> +				/* Disable eSync if the new frequency is 1 Hz */
> +				zl3073x_out_esync_disable(&out);
> +			} else {
> +				/* Update esync period and width according to
> +				 * the new frequency.
> +				 */
> +				zl3073x_out_esync_enable(&out,
> +							 synth_freq / out.div);
> +			}
> +		}
> +
>   		/* Commit output configuration */
>   		return zl3073x_out_state_set(zldev, out_id, &out);
>   	}
> diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
> index 660889c57bff..d2b8b4eb5a76 100644
> --- a/drivers/dpll/zl3073x/out.h
> +++ b/drivers/dpll/zl3073x/out.h
> @@ -134,4 +134,40 @@ static inline u8 zl3073x_out_synth_get(const struct zl3073x_out *out)
>   	return FIELD_GET(ZL_OUTPUT_CTRL_SYNTH_SEL, out->ctrl);
>   }
>   
> +static inline bool zl3073x_out_esync_is_enabled(const struct zl3073x_out *out)
> +{
> +	u8 clock_type = zl3073x_out_clock_type_get(out);
> +
> +	return clock_type == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC;
> +}
> +
> +static inline
> +void zl3073x_out_esync_enable(struct zl3073x_out *out, u32 base_freq)
> +{
> +	/* Period is expressed as the number of output divider clock cycles.
> +	 * So for 1 Hz the period is equal to base frequency.
> +	 */
> +	out->esync_n_period = base_freq;
> +
> +	/* Half of the period in units of 1/2 synth cycle can be represented by
> +	 * the output_div. To get the supported esync pulse width of 25% of the
> +	 * period the output_div can just be divided by 2. Note that this
> +	 * assumes that output_div is even, otherwise some resolution will be
> +	 * lost.
> +	 */
> +	out->esync_n_width = out->div / 2;
> +
> +	/* Update the clock type */
> +	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
> +}
> +
> +static inline
> +void zl3073x_out_esync_disable(struct zl3073x_out *out)
> +{
> +	/* Just update the clock type. The eSync period and width are
> +	 * ignored for this clock type.
> +	 */
> +	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
> +}
> +
>   #endif /* _ZL3073X_OUT_H */

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

Re: [PATCH net v3 3/4] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Petr Oros]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> For N-divided outputs the P-pin and N-pin share the output divisor and
> the N-pin frequency is synth_freq / (div * esync_n_period). The frequency
> set helper computed esync_n_period with a truncating division and only
> rejected a zero result, so a frequency that does not divide evenly was
> silently rounded. Setting the P-pin could then shift the N-pin frequency
> too (e.g. 600 MHz synth, div=60, period=10 gives P=10 MHz, N=1 MHz;
> setting P to 2.5 MHz moves N to 1.25 MHz).
> 
> Check the division remainder and reject the request if it is not exact,
> and require esync_n_period >= 2 so the N-pin frequency stays below the
> P-pin one.
> 
> Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/dpll.c | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 7c997966c3c3..65107b4cc4f8 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -1007,15 +1007,28 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   
>   	if (zl3073x_dpll_is_p_pin(pin)) {
>   		/* We are going to change output frequency for P-pin but
> -		 * if the requested frequency is less than current N-pin
> -		 * frequency then indicate a failure as we are not able
> +		 * if the requested frequency is less or equal than current
> +		 * N-pin frequency then indicate a failure as we are not able
>   		 * to compute N-pin divisor to keep its frequency unchanged.
>   		 *
>   		 * Update divisor for N-pin to keep N-pin frequency.
>   		 */
> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> -		if (!out.esync_n_period)
> +		u64 prod = mul_u32_u32(out.esync_n_period, out.div);
> +		u32 rem;
> +
> +		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
> +		if (rem != 0) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "OUT%uN freq must divide OUT%uP freq",
> +					   out_id, out_id);
>   			return -EINVAL;
> +		}
> +		if (out.esync_n_period < 2) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "OUT%uN freq must be less than OUT%uP freq",
> +					   out_id, out_id);
> +			return -EINVAL;
> +		}
>   
>   		/* Update the output divisor */
>   		out.div = new_div;
> @@ -1030,9 +1043,21 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		 *
>   		 * Update divisor for N-pin
>   		 */
> -		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> -		if (!out.esync_n_period)
> +		u64 rem, prod = frequency * out.div;
> +
> +		out.esync_n_period = div64_u64_rem(synth_freq, prod, &rem);
> +		if (rem != 0) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "OUT%uN freq must divide OUT%uP freq",
> +					   out_id, out_id);
>   			return -EINVAL;
> +		}
> +		if (out.esync_n_period < 2) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "OUT%uN freq must be less than OUT%uP freq",
> +					   out_id, out_id);
> +			return -EINVAL;
> +		}
>   	}
>   
>   	/* For 50/50 duty cycle the divisor is equal to width */

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

Re: [PATCH net v3 4/4] dpll: zl3073x: notify sibling pin when shared output config changes

[Petr Oros]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> Each zl3073x output has a P-pin and an N-pin that share a single HW
> output and, outside N-pin divide mode, share the output's divisor,
> clock type, esync period/width and phase compensation registers.
> Changing one of these settings through one pin's dpll_pin therefore
> also changes the other (sibling) pin's effective configuration, but
> only the pin the change was requested on gets a dpll_pin_change_ntf()
> notification - userspace listening on the sibling pin is never told
> its frequency, esync configuration or phase adjustment changed.
> 
> This was found by code inspection rather than triggered at runtime: a
> dpll pin-get on the sibling pin returns the correct current values, but
> because no change notification is emitted for it, userspace is never
> told that its configuration changed asynchronously through the other
> pin.
> 
> Add zl3073x_dpll_output_pin_sibling_get() to look up the other pin of
> an output pair, and use it in frequency_set() (for the non-N-divided
> signal formats, where the output divisor is shared), esync_set() and
> phase_adjust_set() to notify the sibling pin, if it is registered,
> whenever the shared HW state actually changes.
> 
> The three callbacks are switched from guard(mutex) to explicit
> mutex_lock()/mutex_unlock() with goto-based unwinding, because the
> notification must run after zldpll->lock is released: the notification
> re-enters the pin get callbacks, which take zldpll->lock again, so
> calling it under the lock would deadlock. The DPLL subsystem's own
> dpll_lock is still held across the callback, so the __ (lock-held)
> notification variant is used.
> 
> The sibling lookup walks the zldpll->pins list, so the list membership
> has to match the DPLL registration state. dpll_pin_register() publishes
> a pin (and P is registered before N) before it used to be added to the
> list, and teardown detached the whole list before unregistering any
> pin, so a sibling that is registered - and thus reachable by a PIN_SET
> on the other pin - could be missing from the list and never notified.
> Add the pin to the list before dpll_pin_register() and remove it only
> after dpll_pin_unregister(), both under zldpll->lock, and assert the
> lock in the lookup helper. A pin that is transiently on the list while
> not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
> pin that is not available, and the sibling cannot be freed under the
> lookup because the freeing goes through dpll_pin_unregister(), which
> takes dpll_lock that the notification path holds.
> 
> Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
> Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
> Fixes: 6287262f761e ("dpll: zl3073x: Add support to adjust phase")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/zl3073x/dpll.c | 187 ++++++++++++++++++++++++++++--------
>   1 file changed, 146 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 65107b4cc4f8..9c678acc3e77 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -123,6 +123,8 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
>   {
>   	struct zl3073x_dpll_pin *pin;
>   
> +	lockdep_assert_held(&zldpll->lock);
> +
>   	list_for_each_entry(pin, &zldpll->pins, list) {
>   		if (zl3073x_dpll_is_input_pin(pin) &&
>   		    zl3073x_input_pin_ref_get(pin->id) == ref_id)
> @@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
>   	return NULL;
>   }
>   
> +/**
> + * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
> + * @pin: output pin whose sibling is sought
> + *
> + * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
> + * single HW output. Looks up the other pin of the pair, if it is
> + * registered as a dpll_pin on this DPLL.
> + *
> + * Return: pointer to sibling pin, or NULL if it is not registered
> + */
> +static struct zl3073x_dpll_pin *
> +zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
> +{
> +	struct zl3073x_dpll_pin *sibling;
> +
> +	lockdep_assert_held(&pin->dpll->lock);
> +
> +	list_for_each_entry(sibling, &pin->dpll->pins, list) {
> +		if (!zl3073x_dpll_is_input_pin(sibling) &&
> +		    sibling->id == (pin->id ^ 1))
> +			return sibling;
> +	}
> +
> +	return NULL;
> +}
> +
>   static struct zl3073x_dpll_pin *
>   zl3073x_dpll_nco_pin_get(struct zl3073x_dpll *zldpll)
>   {
>   	struct zl3073x_dpll_pin *pin;
>   
> +	lockdep_assert_held(&zldpll->lock);
> +
>   	list_for_each_entry(pin, &zldpll->pins, list) {
>   		if (zl3073x_dpll_is_nco_pin(pin))
>   			return pin;
> @@ -899,12 +929,14 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
>   	struct zl3073x_dpll *zldpll = dpll_priv;
>   	struct zl3073x_dev *zldev = zldpll->dev;
>   	struct zl3073x_dpll_pin *pin = pin_priv;
> +	struct zl3073x_dpll_pin *sibling = NULL;
>   	const struct zl3073x_synth *synth;
>   	struct zl3073x_out out;
>   	u32 synth_freq;
>   	u8 out_id;
> +	int rc;
>   
> -	guard(mutex)(&zldpll->lock);
> +	mutex_lock(&zldpll->lock);
>   
>   	out_id = zl3073x_output_pin_out_get(pin->id);
>   	out = *zl3073x_out_state_get(zldev, out_id);
> @@ -913,12 +945,14 @@ zl3073x_dpll_output_pin_esync_set(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))
> -		return -EOPNOTSUPP;
> +	if (zl3073x_out_is_ndiv(&out)) {
> +		rc = -EOPNOTSUPP;
> +		goto unlock;
> +	}
>   
>   	if (!freq) {
>   		zl3073x_out_esync_disable(&out);
> -		return zl3073x_out_state_set(zldev, out_id, &out);
> +		goto commit;
>   	}
>   
>   	/* Get attached synth frequency */
> @@ -927,9 +961,24 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
>   
>   	/* Enable 1PPS eSync for this pin frequency */
>   	zl3073x_out_esync_enable(&out, synth_freq / out.div);
> -
> +commit:
>   	/* Commit output configuration */
> -	return zl3073x_out_state_set(zldev, out_id, &out);
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +	if (rc)
> +		goto unlock;
> +
> +	/* The clock type, esync period and esync width are all shared by
> +	 * both pins of the output pair, so the sibling pin's esync
> +	 * configuration changes too and userspace has to be notified.
> +	 */
> +	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> +unlock:
> +	mutex_unlock(&zldpll->lock);
> +
> +	if (!rc && sibling)
> +		__dpll_pin_change_ntf(sibling->dpll_pin);
> +
> +	return rc;
>   }
>   
>   static int
> @@ -959,12 +1008,14 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   	struct zl3073x_dpll *zldpll = dpll_priv;
>   	struct zl3073x_dev *zldev = zldpll->dev;
>   	struct zl3073x_dpll_pin *pin = pin_priv;
> +	struct zl3073x_dpll_pin *sibling = NULL;
>   	const struct zl3073x_synth *synth;
>   	u32 new_div, synth_freq;
>   	struct zl3073x_out out;
>   	u8 out_id;
> +	int rc;
>   
> -	guard(mutex)(&zldpll->lock);
> +	mutex_lock(&zldpll->lock);
>   
>   	out_id = zl3073x_output_pin_out_get(pin->id);
>   	out = *zl3073x_out_state_get(zldev, out_id);
> @@ -977,7 +1028,8 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   	/* Check signal format */
>   	if (!zl3073x_out_is_ndiv(&out)) {
>   		/* For non N-divided signal formats the frequency is computed
> -		 * as division of synth frequency and output divisor.
> +		 * as division of synth frequency and output divisor, which
> +		 * is shared by both pins of the output pair.
>   		 */
>   		out.div = new_div;
>   
> @@ -1002,7 +1054,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		}
>   
>   		/* Commit output configuration */
> -		return zl3073x_out_state_set(zldev, out_id, &out);
> +		rc = zl3073x_out_state_set(zldev, out_id, &out);
> +		if (rc)
> +			goto unlock;
> +
> +		/* The other pin's frequency changed too - it has to be
> +		 * notified about the change.
> +		 */
> +		sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> +
> +		goto unlock;
>   	}
>   
>   	if (zl3073x_dpll_is_p_pin(pin)) {
> @@ -1017,18 +1078,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		u32 rem;
>   
>   		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
> -		if (rem != 0) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must divide OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> -		if (out.esync_n_period < 2) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must be less than OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> +		if (rem != 0)
> +			goto err_n_nondiv;
> +		if (out.esync_n_period < 2)
> +			goto err_n_toohigh;
>   
>   		/* Update the output divisor */
>   		out.div = new_div;
> @@ -1046,25 +1099,36 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		u64 rem, prod = frequency * out.div;
>   
>   		out.esync_n_period = div64_u64_rem(synth_freq, prod, &rem);
> -		if (rem != 0) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must divide OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> -		if (out.esync_n_period < 2) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must be less than OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> +		if (rem != 0)
> +			goto err_n_nondiv;
> +		if (out.esync_n_period < 2)
> +			goto err_n_toohigh;
>   	}
>   
>   	/* For 50/50 duty cycle the divisor is equal to width */
>   	out.esync_n_width = out.esync_n_period;
>   
>   	/* Commit output configuration */
> -	return zl3073x_out_state_set(zldev, out_id, &out);
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +unlock:
> +	mutex_unlock(&zldpll->lock);
> +
> +	if (!rc && sibling)
> +		__dpll_pin_change_ntf(sibling->dpll_pin);
> +
> +	return rc;
> +err_n_nondiv:
> +	NL_SET_ERR_MSG_FMT(extack,
> +			   "OUT%uN freq must divide OUT%uP freq",
> +			   out_id, out_id);
> +	rc = -EINVAL;
> +	goto unlock;
> +err_n_toohigh:
> +	NL_SET_ERR_MSG_FMT(extack,
> +			   "OUT%uN freq must be less than OUT%uP freq",
> +			   out_id, out_id);
> +	rc = -EINVAL;
> +	goto unlock;
>   }
>   
>   static int
> @@ -1103,10 +1167,12 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
>   	struct zl3073x_dpll *zldpll = dpll_priv;
>   	struct zl3073x_dev *zldev = zldpll->dev;
>   	struct zl3073x_dpll_pin *pin = pin_priv;
> +	struct zl3073x_dpll_pin *sibling = NULL;
>   	struct zl3073x_out out;
>   	u8 out_id;
> +	int rc;
>   
> -	guard(mutex)(&zldpll->lock);
> +	mutex_lock(&zldpll->lock);
>   
>   	out_id = zl3073x_output_pin_out_get(pin->id);
>   	out = *zl3073x_out_state_get(zldev, out_id);
> @@ -1115,7 +1181,21 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
>   	out.phase_comp = phase_adjust / pin->phase_gran;
>   
>   	/* Update output configuration from mailbox */
> -	return zl3073x_out_state_set(zldev, out_id, &out);
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +	if (rc)
> +		goto unlock;
> +
> +	/* The phase compensation register is shared by both pins of the
> +	 * output pair, so the sibling pin's phase adjustment changes too.
> +	 */
> +	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> +unlock:
> +	mutex_unlock(&zldpll->lock);
> +
> +	if (!rc && sibling)
> +		__dpll_pin_change_ntf(sibling->dpll_pin);
> +
> +	return rc;
>   }
>   
>   static int
> @@ -1739,6 +1819,13 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
>   	else
>   		ops = &zl3073x_dpll_output_pin_ops;
>   
> +	/* Add the pin to the list before registering it with the DPLL core so
> +	 * that it is findable as a sibling as soon as the core publishes it.
> +	 */
> +	mutex_lock(&zldpll->lock);
> +	list_add(&pin->list, &zldpll->pins);
> +	mutex_unlock(&zldpll->lock);
> +
>   	/* Register the pin */
>   	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
>   	if (rc)
> @@ -1750,6 +1837,9 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
>   	return 0;
>   
>   err_register:
> +	mutex_lock(&zldpll->lock);
> +	list_del(&pin->list);
> +	mutex_unlock(&zldpll->lock);
>   	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>   err_pin_get:
>   	pin->dpll_pin = NULL;
> @@ -1784,6 +1874,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
>   	/* Unregister the pin */
>   	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
>   
> +	/* Remove the pin from the list only after it has been unregistered so
> +	 * that a still-registered pin is always findable as a sibling.
> +	 */
> +	mutex_lock(&zldpll->lock);
> +	list_del(&pin->list);
> +	mutex_unlock(&zldpll->lock);
> +
>   	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>   	pin->dpll_pin = NULL;
>   
> @@ -1803,9 +1900,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
>   {
>   	struct zl3073x_dpll_pin *pin, *next;
>   
> +	/* Unregister each pin before removing it from the list so that a
> +	 * still-registered pin is always findable as a sibling.
> +	 */
>   	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
>   		zl3073x_dpll_pin_unregister(pin);
> -		list_del(&pin->list);
>   		zl3073x_dpll_pin_free(pin);
>   	}
>   }
> @@ -1918,16 +2017,24 @@ zl3073x_dpll_nco_pin_register(struct zl3073x_dpll *zldpll)
>   		goto err_pin_get;
>   	}
>   
> +	/* Add the pin to the list before registering it with the DPLL core so
> +	 * that the list reflects the DPLL registration state.
> +	 */
> +	mutex_lock(&zldpll->lock);
> +	list_add(&pin->list, &zldpll->pins);
> +	mutex_unlock(&zldpll->lock);
> +
>   	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin,
>   			       &zl3073x_dpll_nco_pin_ops, pin);
>   	if (rc)
>   		goto err_register;
>   
> -	list_add(&pin->list, &zldpll->pins);
> -
>   	return 0;
>   
>   err_register:
> +	mutex_lock(&zldpll->lock);
> +	list_del(&pin->list);
> +	mutex_unlock(&zldpll->lock);
>   	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>   err_pin_get:
>   	pin->dpll_pin = NULL;
> @@ -1979,8 +2086,6 @@ zl3073x_dpll_pins_register(struct zl3073x_dpll *zldpll)
>   			zl3073x_dpll_pin_free(pin);
>   			goto error;
>   		}
> -
> -		list_add(&pin->list, &zldpll->pins);
>   	}
>   
>   	/* Register NCO virtual input pin */

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

Re: [PATCH net v3 1/4] dpll: zl3073x: reject output frequencies with too small divisor

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In N-div output mode, the new probe-time check in
  zl3073x_pin_check_freq() (drivers/dpll/zl3073x/prop.c) only requires…

--

dpll: zl3073x: reject output frequencies with too small divisor

This patch tightens zl3073x_pin_check_freq() so output pin frequencies
from firmware must divide the synth frequency with a divisor of at
least 2. For the N-pin of an N-divided output the minimum is 4. It also
rejects a zero frequency so the division cannot be by zero.

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978e..a4bdc3878590f 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c

[ ... ]

> @@ -41,18 +42,32 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,

[ ... ]

>  		/* Check the frequency divides synth frequency */
> -		if (synth_freq % (u32)freq)
> +		div = div_u64_rem(synth_freq, freq, &rem);
> +		if (rem)
> +			goto err_inv_freq;
> +
> +		/* The output divisor has to be at least 2. For the N-pin of an
> +		 * N-divided output the effective divisor also includes the N
> +		 * divider (>= 2), so the minimum is 4.
> +		 */
> +		out = zl3073x_out_state_get(zldev, out_id);
> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> +			min_div = 4;
> +		else
> +			min_div = 2;
> +		if (div < min_div)
>  			goto err_inv_freq;

[Severity: Low]
For N-divided outputs, the new check compares the effective divisor
only against the synth. It does not look at the shared output divider
(out->div). Does that let an N-pin frequency pass this filter even
though no valid N divider exists for it?

In this patch, zl3073x_dpll_output_pin_frequency_set() in
drivers/dpll/zl3073x/dpll.c computes the N divider with a truncating
division and only rejects a result of 0:

drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_output_pin_frequency_set() {
    ...
	} else {
		...
		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
		if (!out.esync_n_period)
			return -EINVAL;
	}
    ...
}

Take a 1 GHz synth with P-pin divider 10:

  N-pin request of 100 MHz: div = 10 >= 4, so it passes here.
  esync_n_period then works out to 1, and that is committed to
  hardware.

  N-pin request of 40 MHz: div = 25 >= 4, so it also passes.
  esync_n_period is truncated from 2.5 to 2. The output runs at
  50 MHz, but the call reports success.

The P-pin branch has the same truncation and zero-only check:

		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
		if (!out.esync_n_period)
			return -EINVAL;

The later commit in this series, "dpll: zl3073x: reject inexact
frequencies for N-divided outputs", fixes this. It adds remainder
checks and esync_n_period >= 2 checks to both branches, and returns
-EINVAL with an extack message.

After that commit, one gap remains. Some frequencies have an effective
divisor that cannot be split into out_div >= 2 times n_div >= 2, such
as synth/5. They still appear in freq_supported, but setting them is
rejected. Since this check is necessary but not sufficient, should the
commit message or the comment say so?

>  	}
>

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

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

[netdev-bot+sashiko]

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

Re: [PATCH net v3 3/4] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (1)

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] In the P-pin branch of zl3073x_dpll_output_pin_frequency_set()
  (drivers/dpll/zl3073x/dpll.c:1016-1031), the patch widens the product…

Pre-existing issues:
- [High] A privileged netlink request for frequency 0 can reach the
  unchecked division `new_div = synth_freq / (u32)frequency;` in…
- [Low] The four NL_SET_ERR_MSG_FMT() calls this patch adds to
  zl3073x_dpll_output_pin_frequency_set() (drivers/dpll/zl3073x/dpll.c,…

--

dpll: zl3073x: reject inexact frequencies for N-divided outputs

This patch changes zl3073x_dpll_output_pin_frequency_set() to check the
division remainder when it computes esync_n_period for N-divided outputs.
A request is rejected if it does not divide evenly or if the N-pin
frequency would not stay below the P-pin frequency. Both failure cases
get new extack messages.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 7c997966c3c33..65107b4cc4f82 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -1007,15 +1007,28 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,

[Severity: High]
This is a pre-existing issue, but can a frequency of 0 reach the division
at the top of zl3073x_dpll_output_pin_frequency_set()?

	new_div = synth_freq / (u32)frequency;

zl3073x_dev_output_pin_freq_get() uses integer division, so a sub-Hz
output reads back as 0 Hz. For example, with a 600 MHz synth, an N-divided
output, div=60 and esync_n_period=20000000, N is 0.5 Hz and is reported
as 0. zl3073x_out_state_fetch() only rejects zero divisors.

zl3073x_pin_props_get() in drivers/dpll/zl3073x/prop.c then always
registers the current frequency as supported. This bypasses the !freq
check in zl3073x_pin_check_freq():

	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);

Suppose firmware also lists 1 Hz for that N-pin. A CAP_NET_ADMIN user can
set the N-pin to 1 Hz and then request 0 Hz:

dpll_pin_freq_set()
  dpll_pin_is_freq_supported(0)   -> true, ranges[0] is {0, 0}
  freq != old_freq (1)
  ops->frequency_set(..., 0, ...)
    zl3073x_dpll_output_pin_frequency_set()
      synth_freq / (u32)0

The division happens before any of the new remainder checks run.

There may be a similar path through a non N-divided output with
div > synth_freq, where P reads as 0 Hz. The DPLL_A_PIN_FREQUENCY policy
is a plain NLA_U64 with no range. This code is unchanged at the end of
the series.

Would rejecting a zero frequency here, or in dpll_pin_freq_set(), make
sense?

>  
>  	if (zl3073x_dpll_is_p_pin(pin)) {

[ ... ]

> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> -		if (!out.esync_n_period)
> +		u64 prod = mul_u32_u32(out.esync_n_period, out.div);
> +		u32 rem;
> +
> +		out.esync_n_period = div_u64_rem(prod, new_div, &rem);

[Severity: Medium]
Is the 64-bit quotient silently truncated when it is stored in the u32
out.esync_n_period?

mul_u32_u32() widens prod, but the result of div_u64_rem() is narrowed
before the rem and < 2 checks run. Those checks never see the upper bits.

For example, take a 600 MHz synth with div=12 and
esync_n_period=0x80000001 read from hardware (P=50 MHz, N about 0.023 Hz).
Requesting P=100 MHz gives new_div=6:

    prod     = 0x80000001 * 12 = 0x60000000C
    quotient = 0x60000000C / 6 = 0x100000002, rem = 0
    (u32)    = 2

Both checks pass, and zl3073x_out_state_set() commits div=6 and
esync_n_period=2. The N-pin then jumps from about 0.023 Hz to 50 MHz.
The commit message says this patch prevents that kind of N-pin shift.

zl3073x_out_state_fetch() only rejects a zero esync_n_period, so it
accepts this state from hardware. The driver's own N-pin path cannot
create it, but EEPROM or firmware can.

Could the quotient be kept in a u64 temporary, with values above U32_MAX
rejected before the assignment? This code is unchanged at the end of the
series.

> +		if (rem != 0) {
> +			NL_SET_ERR_MSG_FMT(extack,
> +					   "OUT%uN freq must divide OUT%uP freq",
> +					   out_id, out_id);
>  			return -EINVAL;
> +		}

[Severity: Low]
This isn't a bug introduced by this patch, but do the new extack messages
(here and in the N-pin branch) ever reach userspace?

The only caller, dpll_pin_freq_set(), sets its own message whenever
frequency_set() fails:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
    ...
	if (ret) {
		NL_SET_ERR_MSG_FMT(extack,
				   "frequency set failed for dpll_id:%u",
				   dpll->id);
		return ret;
	}
    ...
}

NL_SET_ERR_MSG_FMT() writes into the same extack->_msg_buf and points
extack->_msg at it again. Userspace therefore gets only the generic
message. The driver text shows up only in the netlink_extack tracepoint.
The core still overwrites the message at the end of the series.

A smaller point: in the P-pin branch the remainder check runs before the
< 2 check. If the requested P frequency is below the current N frequency
(new_div > esync_n_period * div), the quotient is 0 and rem equals prod.
That case gets "must divide" instead of "must be less than". Would
checking esync_n_period < 2 first give the more accurate message?

[ ... ]

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

Re: [PATCH net v3 4/4] dpll: zl3073x: notify sibling pin when shared output config changes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message describes the old teardown incorrectly.
- [Low] The documentation for zl3073x_dpll_output_pin_sibling_get() does
  not match its code, and the commit message's safety claim is only…

--

dpll: zl3073x: notify sibling pin when shared output config changes

The P and N pins of a zl3073x output share the divisor, clock type,
esync and phase compensation registers. This patch adds a sibling
lookup helper and sends a change notification to the sibling pin from
frequency_set(), esync_set() and phase_adjust_set(). It also changes
the order of list_add()/list_del() on zldpll->pins relative to DPLL
pin registration, and does both under zldpll->lock.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 65107b4cc4f82..9c678acc3e777 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
>  	return NULL;
>  }
>  
> +/**
> + * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
> + * @pin: output pin whose sibling is sought
> + *
> + * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
> + * single HW output. Looks up the other pin of the pair, if it is
> + * registered as a dpll_pin on this DPLL.
> + *
> + * Return: pointer to sibling pin, or NULL if it is not registered
> + */
> +static struct zl3073x_dpll_pin *
> +zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
> +{
> +	struct zl3073x_dpll_pin *sibling;
> +
> +	lockdep_assert_held(&pin->dpll->lock);
> +
> +	list_for_each_entry(sibling, &pin->dpll->pins, list) {
> +		if (!zl3073x_dpll_is_input_pin(sibling) &&
> +		    sibling->id == (pin->id ^ 1))
> +			return sibling;
> +	}
> +
> +	return NULL;
> +}

[Severity: Low]
Does this kernel-doc match what the helper does? It says NULL is
returned when the sibling is not registered. The loop only checks list
membership, direction and id.

This patch makes zl3073x_dpll_pin_register() and
zl3073x_dpll_nco_pin_register() add the pin to zldpll->pins before
dpll_pin_register(). It also makes zl3073x_dpll_pin_unregister()
remove the pin only after dpll_pin_unregister(). During those windows
the helper can return a pin that is not registered.

esync_set(), frequency_set() and phase_adjust_set() then pass that pin
directly to __dpll_pin_change_ntf(), whose kernel-doc says:

 * @pin: registered pin pointer

The window looks reachable. zl3073x_dpll_register() registers the DPLL
device before its pins, so a PIN_SET on the P pin can run while the N
pin's dpll_pin_register() waits on dpll_lock. On teardown, the N pin is
unregistered while the P pin is still available.

The commit message also says:

  A pin that is transiently on the list while
  not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
  pin that is not available

Is that accurate? Only dpll_pin_event_send() checks
dpll_pin_available(). dpll_pin_notify() runs first and calls the
in-kernel notifier chain without any check:

__dpll_pin_change_ntf()
    dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
        call_dpll_notifiers(action, &info);
    return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);

So an in-kernel DPLL_PIN_CHANGED event can be sent for a pin that is
not registered. Nothing in the tree seems to be affected today: the
only in-tree notifier, ice_dpll_pin_notify(), ignores actions other
than CREATED and DELETED.

Should the kernel-doc and the commit message be corrected? The other
option is to check that the sibling is actually registered before it
is notified.

[ ... ]

> @@ -1784,6 +1874,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
>  	/* Unregister the pin */
>  	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
>  
> +	/* Remove the pin from the list only after it has been unregistered so
> +	 * that a still-registered pin is always findable as a sibling.
> +	 */
> +	mutex_lock(&zldpll->lock);
> +	list_del(&pin->list);
> +	mutex_unlock(&zldpll->lock);
> +
>  	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>  	pin->dpll_pin = NULL;
>  
> @@ -1803,9 +1900,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
>  {
>  	struct zl3073x_dpll_pin *pin, *next;
>  
> +	/* Unregister each pin before removing it from the list so that a
> +	 * still-registered pin is always findable as a sibling.
> +	 */
>  	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
>  		zl3073x_dpll_pin_unregister(pin);
> -		list_del(&pin->list);
>  		zl3073x_dpll_pin_free(pin);
>  	}
>  }

[Severity: Low]
The commit message says:

  and teardown detached the whole list before unregistering any
  pin, so a sibling that is registered - and thus reachable by a PIN_SET
  on the other pin - could be missing from the list and never notified.

Does this describe the old teardown correctly? Before this patch,
zl3073x_dpll_pins_unregister() already unregistered each pin before
removing it from the list:

	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
		zl3073x_dpll_pin_unregister(pin);
		list_del(&pin->list);
		zl3073x_dpll_pin_free(pin);
	}

drivers/dpll/zl3073x/ has no list_splice() or similar call on this
list. A pin that was still registered therefore stayed on zldpll->pins
during teardown.

The list and the registration state only got out of step at
registration time, because dpll_pin_register() ran before list_add().
On the teardown side, this hunk mainly moves list_del() under
zldpll->lock, which the new lockdep-asserted lookup needs.

Could the commit message describe what the teardown change actually
does?

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

Re: [PATCH net v3 1/4] dpll: zl3073x: reject output frequencies with too small divisor

[Ivan Vecera]

On 10/8/26 17:31, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] In N-div output mode, the new probe-time check in
>    zl3073x_pin_check_freq() (drivers/dpll/zl3073x/prop.c) only requires…
> 
> --
> 
> dpll: zl3073x: reject output frequencies with too small divisor
> 
> This patch tightens zl3073x_pin_check_freq() so output pin frequencies
> from firmware must divide the synth frequency with a divisor of at
> least 2. For the N-pin of an N-divided output the minimum is 4. It also
> rejects a zero frequency so the division cannot be by zero.
> 
>> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
>> index ac9d41d0f978e..a4bdc3878590f 100644
>> --- a/drivers/dpll/zl3073x/prop.c
>> +++ b/drivers/dpll/zl3073x/prop.c
> 
> [ ... ]
> 
>> @@ -41,18 +42,32 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
> 
> [ ... ]
> 
>>   		/* Check the frequency divides synth frequency */
>> -		if (synth_freq % (u32)freq)
>> +		div = div_u64_rem(synth_freq, freq, &rem);
>> +		if (rem)
>> +			goto err_inv_freq;
>> +
>> +		/* The output divisor has to be at least 2. For the N-pin of an
>> +		 * N-divided output the effective divisor also includes the N
>> +		 * divider (>= 2), so the minimum is 4.
>> +		 */
>> +		out = zl3073x_out_state_get(zldev, out_id);
>> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
>> +			min_div = 4;
>> +		else
>> +			min_div = 2;
>> +		if (div < min_div)
>>   			goto err_inv_freq;
> 
> [Severity: Low]
> For N-divided outputs, the new check compares the effective divisor
> only against the synth. It does not look at the shared output divider
> (out->div). Does that let an N-pin frequency pass this filter even
> though no valid N divider exists for it?
> 
> In this patch, zl3073x_dpll_output_pin_frequency_set() in
> drivers/dpll/zl3073x/dpll.c computes the N divider with a truncating
> division and only rejects a result of 0:
> 
> drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_output_pin_frequency_set() {
>      ...
> 	} else {
> 		...
> 		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 	}
>      ...
> }
> 
> Take a 1 GHz synth with P-pin divider 10:
> 
>    N-pin request of 100 MHz: div = 10 >= 4, so it passes here.
>    esync_n_period then works out to 1, and that is committed to
>    hardware.
> 
>    N-pin request of 40 MHz: div = 25 >= 4, so it also passes.
>    esync_n_period is truncated from 2.5 to 2. The output runs at
>    50 MHz, but the call reports success.
> 
> The P-pin branch has the same truncation and zero-only check:
> 
> 		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 
> The later commit in this series, "dpll: zl3073x: reject inexact
> frequencies for N-divided outputs", fixes this. It adds remainder
> checks and esync_n_period >= 2 checks to both branches, and returns
> -EINVAL with an extack message.
> 
> After that commit, one gap remains. Some frequencies have an effective
> divisor that cannot be split into out_div >= 2 times n_div >= 2, such
> as synth/5. They still appear in freq_supported, but setting them is
> rejected. Since this check is necessary but not sufficient, should the
> commit message or the comment say so?
> 

Yes, next version will say so in the commit message. The remaining cases are
rejected by the frequency set callback.

Thanks,
Ivan

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

Re: [PATCH net v3 3/4] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Ivan Vecera]

On 10/8/26 17:31, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (1)
> 
> 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] In the P-pin branch of zl3073x_dpll_output_pin_frequency_set()
>    (drivers/dpll/zl3073x/dpll.c:1016-1031), the patch widens the product…
> 
> Pre-existing issues:
> - [High] A privileged netlink request for frequency 0 can reach the
>    unchecked division `new_div = synth_freq / (u32)frequency;` in…
> - [Low] The four NL_SET_ERR_MSG_FMT() calls this patch adds to
>    zl3073x_dpll_output_pin_frequency_set() (drivers/dpll/zl3073x/dpll.c,…
> 
> --
> 
> dpll: zl3073x: reject inexact frequencies for N-divided outputs
> 
> This patch changes zl3073x_dpll_output_pin_frequency_set() to check the
> division remainder when it computes esync_n_period for N-divided outputs.
> A request is rejected if it does not divide evenly or if the N-pin
> frequency would not stay below the P-pin frequency. Both failure cases
> get new extack messages.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 7c997966c3c33..65107b4cc4f82 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
>> @@ -1007,15 +1007,28 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
> 
> [Severity: High]
> This is a pre-existing issue, but can a frequency of 0 reach the division
> at the top of zl3073x_dpll_output_pin_frequency_set()?

Yes. In V4 I will add a separate patch that will stops offering a 0 Hz current
frequency as supported, so the DPLL core rejects a 0 Hz request before it
reaches the driver.

> 	new_div = synth_freq / (u32)frequency;
> 
> zl3073x_dev_output_pin_freq_get() uses integer division, so a sub-Hz
> output reads back as 0 Hz. For example, with a 600 MHz synth, an N-divided
> output, div=60 and esync_n_period=20000000, N is 0.5 Hz and is reported
> as 0. zl3073x_out_state_fetch() only rejects zero divisors.
> 
> zl3073x_pin_props_get() in drivers/dpll/zl3073x/prop.c then always
> registers the current frequency as supported. This bypasses the !freq
> check in zl3073x_pin_check_freq():
> 
> 	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> 
> Suppose firmware also lists 1 Hz for that N-pin. A CAP_NET_ADMIN user can
> set the N-pin to 1 Hz and then request 0 Hz:
> 
> dpll_pin_freq_set()
>    dpll_pin_is_freq_supported(0)   -> true, ranges[0] is {0, 0}
>    freq != old_freq (1)
>    ops->frequency_set(..., 0, ...)
>      zl3073x_dpll_output_pin_frequency_set()
>        synth_freq / (u32)0
> 
> The division happens before any of the new remainder checks run.
> 
> There may be a similar path through a non N-divided output with
> div > synth_freq, where P reads as 0 Hz. The DPLL_A_PIN_FREQUENCY policy
> is a plain NLA_U64 with no range. This code is unchanged at the end of
> the series.
> 
> Would rejecting a zero frequency here, or in dpll_pin_freq_set(), make
> sense?
> 
>>   
>>   	if (zl3073x_dpll_is_p_pin(pin)) {
> 
> [ ... ]
> 
>> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
>> -		if (!out.esync_n_period)
>> +		u64 prod = mul_u32_u32(out.esync_n_period, out.div);
>> +		u32 rem;
>> +
>> +		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
> 
> [Severity: Medium]
> Is the 64-bit quotient silently truncated when it is stored in the u32
> out.esync_n_period?

Right. Will fix this in v4 so the quotient will be kept in u64 and the request
is rejected if it does not fit into u32.
> mul_u32_u32() widens prod, but the result of div_u64_rem() is narrowed
> before the rem and < 2 checks run. Those checks never see the upper bits.
> 
> For example, take a 600 MHz synth with div=12 and
> esync_n_period=0x80000001 read from hardware (P=50 MHz, N about 0.023 Hz).
> Requesting P=100 MHz gives new_div=6:
> 
>      prod     = 0x80000001 * 12 = 0x60000000C
>      quotient = 0x60000000C / 6 = 0x100000002, rem = 0
>      (u32)    = 2
> 
> Both checks pass, and zl3073x_out_state_set() commits div=6 and
> esync_n_period=2. The N-pin then jumps from about 0.023 Hz to 50 MHz.
> The commit message says this patch prevents that kind of N-pin shift.
> 
> zl3073x_out_state_fetch() only rejects a zero esync_n_period, so it
> accepts this state from hardware. The driver's own N-pin path cannot
> create it, but EEPROM or firmware can.
> 
> Could the quotient be kept in a u64 temporary, with values above U32_MAX
> rejected before the assignment? This code is unchanged at the end of the
> series.
> 
>> +		if (rem != 0) {
>> +			NL_SET_ERR_MSG_FMT(extack,
>> +					   "OUT%uN freq must divide OUT%uP freq",
>> +					   out_id, out_id);
>>   			return -EINVAL;
>> +		}
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but do the new extack messages
> (here and in the N-pin branch) ever reach userspace?

No, dpll_pin_freq_set() overwrites them. That is a DPLL core issue and
out of scope for this series.

^^^
Jiri, Arek, Vadim? There are many places that simply overwrite extack messages
from a driver's callbacks.

> The only caller, dpll_pin_freq_set(), sets its own message whenever
> frequency_set() fails:
> 
> drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
>      ...
> 	if (ret) {
> 		NL_SET_ERR_MSG_FMT(extack,
> 				   "frequency set failed for dpll_id:%u",
> 				   dpll->id);
> 		return ret;
> 	}
>      ...
> }
> 
> NL_SET_ERR_MSG_FMT() writes into the same extack->_msg_buf and points
> extack->_msg at it again. Userspace therefore gets only the generic
> message. The driver text shows up only in the netlink_extack tracepoint.
> The core still overwrites the message at the end of the series.
> 
> A smaller point: in the P-pin branch the remainder check runs before the
> < 2 check. If the requested P frequency is below the current N frequency
> (new_div > esync_n_period * div), the quotient is 0 and rem equals prod.
> That case gets "must divide" instead of "must be less than". Would
> checking esync_n_period < 2 first give the more accurate message?
Yes, will fix in v4.

Thanks,
Ivan

pw-bot: cr

Re: [PATCH net v3 4/4] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

On 10/8/26 17:31, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message describes the old teardown incorrectly.
> - [Low] The documentation for zl3073x_dpll_output_pin_sibling_get() does
>    not match its code, and the commit message's safety claim is only…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> The P and N pins of a zl3073x output share the divisor, clock type,
> esync and phase compensation registers. This patch adds a sibling
> lookup helper and sends a change notification to the sibling pin from
> frequency_set(), esync_set() and phase_adjust_set(). It also changes
> the order of list_add()/list_del() on zldpll->pins relative to DPLL
> pin registration, and does both under zldpll->lock.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 65107b4cc4f82..9c678acc3e777 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
>>   	return NULL;
>>   }
>>   
>> +/**
>> + * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
>> + * @pin: output pin whose sibling is sought
>> + *
>> + * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
>> + * single HW output. Looks up the other pin of the pair, if it is
>> + * registered as a dpll_pin on this DPLL.
>> + *
>> + * Return: pointer to sibling pin, or NULL if it is not registered
>> + */
>> +static struct zl3073x_dpll_pin *
>> +zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
>> +{
>> +	struct zl3073x_dpll_pin *sibling;
>> +
>> +	lockdep_assert_held(&pin->dpll->lock);
>> +
>> +	list_for_each_entry(sibling, &pin->dpll->pins, list) {
>> +		if (!zl3073x_dpll_is_input_pin(sibling) &&
>> +		    sibling->id == (pin->id ^ 1))
>> +			return sibling;
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> [Severity: Low]
> Does this kernel-doc match what the helper does? It says NULL is
> returned when the sibling is not registered. The loop only checks list
> membership, direction and id.
> 
> This patch makes zl3073x_dpll_pin_register() and
> zl3073x_dpll_nco_pin_register() add the pin to zldpll->pins before
> dpll_pin_register(). It also makes zl3073x_dpll_pin_unregister()
> remove the pin only after dpll_pin_unregister(). During those windows
> the helper can return a pin that is not registered.
> 
> esync_set(), frequency_set() and phase_adjust_set() then pass that pin
> directly to __dpll_pin_change_ntf(), whose kernel-doc says:
> 
>   * @pin: registered pin pointer
> 
> The window looks reachable. zl3073x_dpll_register() registers the DPLL
> device before its pins, so a PIN_SET on the P pin can run while the N
> pin's dpll_pin_register() waits on dpll_lock. On teardown, the N pin is
> unregistered while the P pin is still available.
> 
> The commit message also says:
> 
>    A pin that is transiently on the list while
>    not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
>    pin that is not available
> 
> Is that accurate? Only dpll_pin_event_send() checks
> dpll_pin_available(). dpll_pin_notify() runs first and calls the
> in-kernel notifier chain without any check:
> 
> __dpll_pin_change_ntf()
>      dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
>          call_dpll_notifiers(action, &info);
>      return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
> 
> So an in-kernel DPLL_PIN_CHANGED event can be sent for a pin that is
> not registered. Nothing in the tree seems to be affected today: the
> only in-tree notifier, ice_dpll_pin_notify(), ignores actions other
> than CREATED and DELETED.
> 
> Should the kernel-doc and the commit message be corrected? The other
> option is to check that the sibling is actually registered before it
> is notified.

Will correct.
> [ ... ]
> 
>> @@ -1784,6 +1874,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
>>   	/* Unregister the pin */
>>   	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
>>   
>> +	/* Remove the pin from the list only after it has been unregistered so
>> +	 * that a still-registered pin is always findable as a sibling.
>> +	 */
>> +	mutex_lock(&zldpll->lock);
>> +	list_del(&pin->list);
>> +	mutex_unlock(&zldpll->lock);
>> +
>>   	dpll_pin_put(pin->dpll_pin, &pin->tracker);
>>   	pin->dpll_pin = NULL;
>>   
>> @@ -1803,9 +1900,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
>>   {
>>   	struct zl3073x_dpll_pin *pin, *next;
>>   
>> +	/* Unregister each pin before removing it from the list so that a
>> +	 * still-registered pin is always findable as a sibling.
>> +	 */
>>   	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
>>   		zl3073x_dpll_pin_unregister(pin);
>> -		list_del(&pin->list);
>>   		zl3073x_dpll_pin_free(pin);
>>   	}
>>   }
> 
> [Severity: Low]
> The commit message says:
> 
>    and teardown detached the whole list before unregistering any
>    pin, so a sibling that is registered - and thus reachable by a PIN_SET
>    on the other pin - could be missing from the list and never notified.
> 
> Does this describe the old teardown correctly? Before this patch,
> zl3073x_dpll_pins_unregister() already unregistered each pin before
> removing it from the list:
> 
> 	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
> 		zl3073x_dpll_pin_unregister(pin);
> 		list_del(&pin->list);
> 		zl3073x_dpll_pin_free(pin);
> 	}
> 
> drivers/dpll/zl3073x/ has no list_splice() or similar call on this
> list. A pin that was still registered therefore stayed on zldpll->pins
> during teardown.
> 
> The list and the registration state only got out of step at
> registration time, because dpll_pin_register() ran before list_add().
> On the teardown side, this hunk mainly moves list_del() under
> zldpll->lock, which the new lockdep-asserted lookup needs.
> 
> Could the commit message describe what the teardown change actually
> does?
Yes, will rewrite it in v4.

Thanks,
Ivan