[PATCH net-next 0/6] dpll: zl3073x: add PTP periodic output support

[PATCH net-next 0/6] dpll: zl3073x: add PTP periodic output support

[Ivan Vecera]

This series extends the zl3073x output pin support and finally adds PTP
periodic output (perout) support on top of it.

Each zl3073x output exposes a P-pin and an N-pin that share a single HW
output and most of its configuration registers. Patch 1 makes a change
requested on one pin notify the sibling pin as well, so userspace on the
sibling is told when its effective frequency, esync or phase adjustment
changed.

Patches 2 and 3 add the plumbing to control an individual output pin:
GPO override registers and a clean stop/restart of an output (patch 2),
and then per-pin enable/disable through the state_on_dpll_get/set
callbacks - differential pins via the output stop condition, CMOS pins
via the GPO override bracketed by a glitch-free stop/restart (patch 3).

Patches 4 and 5 are preparatory: the per-pin 'esync_control' boolean is
replaced by an extensible 'caps' bitmap, and the divisor/N-division
computation is factored out of frequency_set() into a helper that
updates a struct zl3073x_out without committing it.

Patch 6 registers a PTP periodic output pin for each output pin that
supports step-time and declares 1 PPS support in firmware. Any perout
channel can be assigned to any such pin; only 1 PPS is supported.
Enabling a channel programs its pin for 1 Hz and connects it, disabling
disconnects it, reusing the helper and connect/disconnect primitive
introduced earlier in the series.

Ivan Vecera (6):
  dpll: zl3073x: notify sibling pin when shared output config changes
  dpll: zl3073x: add GPO support for output pins
  dpll: zl3073x: allow enabling/disabling output pins
  dpll: zl3073x: consolidate pin capabilities into bitmap
  dpll: zl3073x: factor out output pin frequency helper
  dpll: zl3073x: add PTP periodic output support

 drivers/dpll/zl3073x/core.c | 117 ++++++++
 drivers/dpll/zl3073x/core.h |  30 ++
 drivers/dpll/zl3073x/dpll.c | 539 +++++++++++++++++++++++++++++++-----
 drivers/dpll/zl3073x/dpll.h |   4 +
 drivers/dpll/zl3073x/out.c  |  50 +++-
 drivers/dpll/zl3073x/out.h  | 138 ++++++++-
 drivers/dpll/zl3073x/prop.c |   2 +
 drivers/dpll/zl3073x/prop.h |  21 ++
 drivers/dpll/zl3073x/regs.h |  20 ++
 9 files changed, 834 insertions(+), 87 deletions(-)


base-commit: 014d795c73837ea2339a4ea8e8f82c6e959b845d
-- 
2.55.0

[PATCH net-next 1/6] 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.

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.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 144 ++++++++++++++++++++++++++++--------
 1 file changed, 115 insertions(+), 29 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40ff..2c6de4dab8b4ad 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -132,6 +132,30 @@ 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;
+
+	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)
 {
@@ -910,11 +934,13 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
 	const struct zl3073x_synth *synth;
+	struct zl3073x_dpll_pin *sibling;
 	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);
@@ -923,8 +949,10 @@ 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;
+	}
 
 	/* Update clock type in output mode */
 	if (freq)
@@ -934,27 +962,44 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 		zl3073x_out_clock_type_set(&out,
 					   ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
 
-	/* If esync is being disabled just write mailbox and finish */
-	if (!freq)
-		return zl3073x_out_state_set(zldev, out_id, &out);
+	if (freq) {
+		/* Get attached synth frequency */
+		synth = zl3073x_synth_state_get(zldev,
+						zl3073x_out_synth_get(&out));
+		synth_freq = zl3073x_synth_freq_get(synth);
 
-	/* 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;
 
-	/* 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;
+	}
 
-	/* 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.
+	/* Commit output configuration */
+	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.
 	 */
-	out.esync_n_width = out.div / 2;
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
 
-	/* Commit output configuration */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	mutex_unlock(&zldpll->lock);
+
+	if (sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return 0;
+unlock:
+	mutex_unlock(&zldpll->lock);
+	return rc;
 }
 
 static int
@@ -984,12 +1029,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);
@@ -1002,7 +1049,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;
 
@@ -1010,7 +1058,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		out.width = new_div;
 
 		/* 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)) {
@@ -1022,8 +1079,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		 * 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)
-			return -EINVAL;
+		if (!out.esync_n_period) {
+			rc = -EINVAL;
+			goto unlock;
+		}
 
 		/* Update the output divisor */
 		out.div = new_div;
@@ -1039,15 +1098,24 @@ 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)
-			return -EINVAL;
+		if (!out.esync_n_period) {
+			rc = -EINVAL;
+			goto unlock;
+		}
 	}
 
 	/* 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 (sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -1086,10 +1154,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;
 	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);
@@ -1098,7 +1168,23 @@ 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) {
+		mutex_unlock(&zldpll->lock);
+		return rc;
+	}
+
+	/* 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);
+
+	mutex_unlock(&zldpll->lock);
+
+	if (sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return 0;
 }
 
 static int
-- 
2.55.0

[PATCH net-next 2/6] dpll: zl3073x: add GPO support for output pins

[Ivan Vecera]

Add the GPO override registers for CMOS output pins: the per-output
output_gpo_en and output_gpo_config_out_p/out_n mailbox fields and the
direct gpo_out_x bitmask registers selecting the static value of an
overridden pin. zl3073x_dev_gpo_set() sets that value for a given GPO
channel and zl3073x_out_pin_func_get()/_set() report and configure the
function (clock or GPO mode) of an individual output pin.

Add the output_ctrl_x stop bits and zl3073x_out_stop()/_start() to
request a clean, edge-aligned stop or restart of an output. Move ctrl
into the cfg struct_group and let zl3073x_out_state_set() write it
directly, since it is no longer invariant and is not part of the
output mailbox.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/core.c |  39 ++++++++++
 drivers/dpll/zl3073x/core.h |   1 +
 drivers/dpll/zl3073x/out.c  |  50 ++++++++++---
 drivers/dpll/zl3073x/out.h  | 138 +++++++++++++++++++++++++++++++++++-
 drivers/dpll/zl3073x/regs.h |  20 ++++++
 5 files changed, 236 insertions(+), 12 deletions(-)

diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 230df08e27cd19..7386932df0327f 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -628,6 +628,45 @@ int zl3073x_ref_phase_offsets_update(struct zl3073x_dev *zldev, int channel)
 				    ZL_POLL_PHASE_ERR_TIMEOUT_US);
 }
 
+/**
+ * zl3073x_dev_gpo_set - set the static value driven by a GPO channel
+ * @zldev: pointer to zl3073x_dev structure
+ * @gpo: GPO channel index (2 * output index for the P-pin, +1 for the
+ *	 N-pin)
+ * @value: value to drive when the channel is GPO-overridden
+ *
+ * The gpo_out_x registers are direct, multi-channel bitmask registers
+ * shared by all outputs, so the read-modify-write is serialized against
+ * concurrent updates to other channels via multiop_lock.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value)
+{
+	unsigned int reg;
+	u8 bit, val;
+	int rc;
+
+	if (gpo >= ZL3073X_NUM_OUTPUT_PINS)
+		return -EINVAL;
+
+	reg = ZL_REG_GPO_OUT(gpo / 8);
+	bit = gpo % 8;
+
+	guard(mutex)(&zldev->multiop_lock);
+
+	rc = zl3073x_read_u8(zldev, reg, &val);
+	if (rc)
+		return rc;
+
+	if (value)
+		val |= BIT(bit);
+	else
+		val &= ~BIT(bit);
+
+	return zl3073x_write_u8(zldev, reg, val);
+}
+
 /**
  * zl3073x_ref_freq_meas_latch - latch reference frequency measurements
  * @zldev: pointer to zl3073x_dev structure
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index 67c10e2595118c..e7064f2958fa5f 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -167,6 +167,7 @@ int zl3073x_write_hwreg_seq(struct zl3073x_dev *zldev,
  *****************/
 
 int zl3073x_ref_phase_offsets_update(struct zl3073x_dev *zldev, int channel);
+int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value);
 
 /**
  * zl3073x_dev_is_ref_phase_comp_32bit - check ref phase comp register size
diff --git a/drivers/dpll/zl3073x/out.c b/drivers/dpll/zl3073x/out.c
index 410d15b96d0bf0..82564045890db0 100644
--- a/drivers/dpll/zl3073x/out.c
+++ b/drivers/dpll/zl3073x/out.c
@@ -85,8 +85,22 @@ int zl3073x_out_state_fetch(struct zl3073x_dev *zldev, u8 index)
 	if (rc)
 		return rc;
 
-	return zl3073x_read_u32(zldev, ZL_REG_OUTPUT_PHASE_COMP,
-				&out->phase_comp);
+	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_PHASE_COMP,
+			      &out->phase_comp);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_u8(zldev, ZL_REG_OUTPUT_GPO_EN, &out->gpo_en);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_read_u8(zldev, ZL_REG_OUTPUT_GPO_CONFIG_OUT_P,
+			     &out->gpo_config_p);
+	if (rc)
+		return rc;
+
+	return zl3073x_read_u8(zldev, ZL_REG_OUTPUT_GPO_CONFIG_OUT_N,
+			       &out->gpo_config_n);
 }
 
 /**
@@ -108,11 +122,12 @@ const struct zl3073x_out *zl3073x_out_state_get(struct zl3073x_dev *zldev,
  * @index: output index to set state for
  * @out: desired output state
  *
- * Validates that invariant fields have not been modified, skips the HW
- * write if the mutable configuration is unchanged, and otherwise writes
- * only the changed cfg fields to hardware via the mailbox interface.
+ * Skips the HW write if the configuration is unchanged, writes ctrl
+ * directly to the output_ctrl_x register if it differs (it is not part
+ * of the output mailbox), and otherwise writes only the changed cfg
+ * fields to hardware via the mailbox interface.
  *
- * Return: 0 on success, -EINVAL if invariants changed, <0 on HW error
+ * Return: 0 on success, <0 on HW error
  */
 int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
 			  const struct zl3073x_out *out)
@@ -120,11 +135,17 @@ int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
 	struct zl3073x_out *dout = &zldev->out[index];
 	int rc;
 
-	/* Reject attempts to change invariant fields (set at fetch only) */
-	if (WARN_ON(memcmp(&dout->inv, &out->inv, sizeof(out->inv))))
-		return -EINVAL;
+	/* ctrl is a direct register, independent of the output mailbox */
+	if (dout->ctrl != out->ctrl) {
+		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
+				      out->ctrl);
+		if (rc)
+			return rc;
+
+		dout->ctrl = out->ctrl;
+	}
 
-	/* Skip HW write if configuration hasn't changed */
+	/* Skip the mailbox commit if nothing else has changed */
 	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
 		return 0;
 
@@ -152,6 +173,15 @@ int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
 	if (!rc && dout->phase_comp != out->phase_comp)
 		rc = zl3073x_write_u32(zldev, ZL_REG_OUTPUT_PHASE_COMP,
 				       out->phase_comp);
+	if (!rc && dout->gpo_en != out->gpo_en)
+		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_GPO_EN,
+				      out->gpo_en);
+	if (!rc && dout->gpo_config_p != out->gpo_config_p)
+		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_GPO_CONFIG_OUT_P,
+				      out->gpo_config_p);
+	if (!rc && dout->gpo_config_n != out->gpo_config_n)
+		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_GPO_CONFIG_OUT_N,
+				      out->gpo_config_n);
 	if (rc)
 		return rc;
 
diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
index 660889c57bffab..66a8432c00dfd0 100644
--- a/drivers/dpll/zl3073x/out.h
+++ b/drivers/dpll/zl3073x/out.h
@@ -19,6 +19,9 @@ struct zl3073x_dev;
  * @esync_n_width: embedded sync or n-pin pulse width
  * @phase_comp: phase compensation
  * @mode: output mode
+ * @gpo_en: GPO override enable for the P-pin and N-pin
+ * @gpo_config_p: GPO mode configuration for the P-pin
+ * @gpo_config_n: GPO mode configuration for the N-pin
  * @ctrl: output control
  */
 struct zl3073x_out {
@@ -29,8 +32,9 @@ struct zl3073x_out {
 		u32	esync_n_width;
 		s32	phase_comp;
 		u8	mode;
-	);
-	struct_group(inv, /* Invariants */
+		u8	gpo_en;
+		u8	gpo_config_p;
+		u8	gpo_config_n;
 		u8	ctrl;
 	);
 };
@@ -106,6 +110,136 @@ static inline bool zl3073x_out_is_enabled(const struct zl3073x_out *out)
 	return !!FIELD_GET(ZL_OUTPUT_CTRL_EN, out->ctrl);
 }
 
+/**
+ * zl3073x_out_is_stopped - check if the given output is stopped
+ * @out: pointer to out state
+ *
+ * Return: true if output clock is stopped, false if it is running
+ */
+static inline bool zl3073x_out_is_stopped(const struct zl3073x_out *out)
+{
+	return !!FIELD_GET(ZL_OUTPUT_CTRL_STOP, out->ctrl);
+}
+
+/**
+ * zl3073x_out_stop - request a clean stop of an output
+ * @out: pointer to out state to update
+ *
+ * Sets the stop and stop_hz bits together, so the output goes high-Z
+ * rather than holding a fixed level once stopped. The stop is
+ * edge-aligned (the device waits for the proper edge before actually
+ * stopping).
+ */
+static inline void zl3073x_out_stop(struct zl3073x_out *out)
+{
+	FIELD_MODIFY(ZL_OUTPUT_CTRL_STOP, &out->ctrl, 1);
+	FIELD_MODIFY(ZL_OUTPUT_CTRL_STOP_HZ, &out->ctrl, 1);
+}
+
+/**
+ * zl3073x_out_start - request a clean restart of a stopped output
+ * @out: pointer to out state to update
+ *
+ * Clears the stop and stop_hz bits together. See zl3073x_out_stop().
+ */
+static inline void zl3073x_out_start(struct zl3073x_out *out)
+{
+	FIELD_MODIFY(ZL_OUTPUT_CTRL_STOP, &out->ctrl, 0);
+	FIELD_MODIFY(ZL_OUTPUT_CTRL_STOP_HZ, &out->ctrl, 0);
+}
+
+#define ZL3073X_OUT_PIN_F_CLOCK		0
+#define ZL3073X_OUT_PIN_F_GPO_CONST	1
+#define ZL3073X_OUT_PIN_F_GPO_STATUS	2
+#define ZL3073X_OUT_PIN_F_GPO_IRQ	3
+#define ZL3073X_OUT_PIN_F_GPO_UNKNOWN	4
+
+/**
+ * zl3073x_out_pin_func_get - get the function of an output pin
+ * @out: pointer to out state
+ * @id: output pin ID (even for P pin, odd for N pin)
+ *
+ * Report the current function of the given output pin. If GPO override is
+ * disabled the pin acts as a clock, otherwise it acts as a GPO with the
+ * mode selected by its GPO config control field.
+ *
+ * Return: one of the ZL3073X_OUT_PIN_F_* function codes
+ */
+static inline u8
+zl3073x_out_pin_func_get(const struct zl3073x_out *out, u8 id)
+{
+	u8 gpo_config;
+	bool gpo_en;
+
+	if (id & 1) {
+		gpo_en = FIELD_GET(ZL_OUTPUT_GPO_EN_OUT_N, out->gpo_en);
+		gpo_config = out->gpo_config_n;
+	} else {
+		gpo_en = FIELD_GET(ZL_OUTPUT_GPO_EN_OUT_P, out->gpo_en);
+		gpo_config = out->gpo_config_p;
+	}
+
+	if (!gpo_en)
+		return ZL3073X_OUT_PIN_F_CLOCK;
+
+	switch (FIELD_GET(ZL_OUTPUT_GPO_CONFIG_CTRL, gpo_config)) {
+	case ZL_OUTPUT_GPO_CONFIG_CTRL_OUTPUT:
+		return ZL3073X_OUT_PIN_F_GPO_CONST;
+	case ZL_OUTPUT_GPO_CONFIG_CTRL_STATUS:
+		return ZL3073X_OUT_PIN_F_GPO_STATUS;
+	case ZL_OUTPUT_GPO_CONFIG_CTRL_IRQ:
+		return ZL3073X_OUT_PIN_F_GPO_IRQ;
+	}
+
+	return ZL3073X_OUT_PIN_F_GPO_UNKNOWN;
+}
+
+/**
+ * zl3073x_out_pin_func_set - set the function of an output pin
+ * @out: pointer to out state to update
+ * @id: output pin ID (even for P pin, odd for N pin)
+ * @func: requested function, one of the ZL3073X_OUT_PIN_F_* codes
+ *
+ * Configure the given output pin as a clock or as a GPO in the requested
+ * mode by updating its GPO enable and GPO config control fields. Unknown
+ * function codes are ignored.
+ */
+static inline void
+zl3073x_out_pin_func_set(struct zl3073x_out *out, u8 id, u8 func)
+{
+	bool gpo_en = true;
+	u8 *gpo_config;
+	int ctrl = -1;
+
+	switch (func) {
+	case ZL3073X_OUT_PIN_F_CLOCK:
+		gpo_en = false;
+		break;
+	case ZL3073X_OUT_PIN_F_GPO_CONST:
+		ctrl = ZL_OUTPUT_GPO_CONFIG_CTRL_OUTPUT;
+		break;
+	case ZL3073X_OUT_PIN_F_GPO_STATUS:
+		ctrl = ZL_OUTPUT_GPO_CONFIG_CTRL_STATUS;
+		break;
+	case ZL3073X_OUT_PIN_F_GPO_IRQ:
+		ctrl = ZL_OUTPUT_GPO_CONFIG_CTRL_IRQ;
+		break;
+	default:
+		return;
+	}
+
+	if (id & 1) {
+		FIELD_MODIFY(ZL_OUTPUT_GPO_EN_OUT_N, &out->gpo_en, gpo_en);
+		gpo_config = &out->gpo_config_n;
+	} else {
+		FIELD_MODIFY(ZL_OUTPUT_GPO_EN_OUT_P, &out->gpo_en, gpo_en);
+		gpo_config = &out->gpo_config_p;
+	}
+
+	if (ctrl != -1)
+		FIELD_MODIFY(ZL_OUTPUT_GPO_CONFIG_CTRL, gpo_config, ctrl);
+}
+
 /**
  * zl3073x_out_is_ndiv - check if the given output is in N-div mode
  * @out: pointer to out state
diff --git a/drivers/dpll/zl3073x/regs.h b/drivers/dpll/zl3073x/regs.h
index f3a5e1215aa36b..f17a2c78611827 100644
--- a/drivers/dpll/zl3073x/regs.h
+++ b/drivers/dpll/zl3073x/regs.h
@@ -94,6 +94,13 @@
 
 #define ZL_REG_DIE_TEMP_STATUS			ZL_REG(0, 0x44, 2)
 
+/*************************
+ * Register Page 1, GPIOs
+ *************************/
+
+#define ZL_REG_GPO_OUT(_idx)						\
+	ZL_REG_IDX(_idx, 1, 0x70, 1, 3, 1)
+
 /*************************
  * Register Page 2, Status
  *************************/
@@ -254,6 +261,8 @@
 #define ZL_REG_OUTPUT_CTRL(_idx)					\
 	ZL_REG_IDX(_idx, 9, 0x28, 1, ZL3073X_NUM_OUTS, 1)
 #define ZL_OUTPUT_CTRL_EN			BIT(0)
+#define ZL_OUTPUT_CTRL_STOP			BIT(1)
+#define ZL_OUTPUT_CTRL_STOP_HZ			BIT(3)
 #define ZL_OUTPUT_CTRL_SYNTH_SEL		GENMASK(6, 4)
 
 #define ZL_REG_OUTPUT_STEP_TIME_MASK		ZL_REG(9, 0x36, 2)
@@ -368,6 +377,17 @@
 #define ZL_REG_OUTPUT_ESYNC_WIDTH		ZL_REG(14, 0x18, 4)
 #define ZL_REG_OUTPUT_PHASE_COMP		ZL_REG(14, 0x20, 4)
 
+#define ZL_REG_OUTPUT_GPO_EN			ZL_REG(14, 0x24, 1)
+#define ZL_OUTPUT_GPO_EN_OUT_P			BIT(0)
+#define ZL_OUTPUT_GPO_EN_OUT_N			BIT(1)
+
+#define ZL_REG_OUTPUT_GPO_CONFIG_OUT_P		ZL_REG(14, 0x27, 1)
+#define ZL_REG_OUTPUT_GPO_CONFIG_OUT_N		ZL_REG(14, 0x2a, 1)
+#define ZL_OUTPUT_GPO_CONFIG_CTRL		GENMASK(2, 0)
+#define ZL_OUTPUT_GPO_CONFIG_CTRL_OUTPUT	1
+#define ZL_OUTPUT_GPO_CONFIG_CTRL_STATUS	3
+#define ZL_OUTPUT_GPO_CONFIG_CTRL_IRQ		4
+
 /*
  * Register Page 255 - HW registers access
  */
-- 
2.55.0

[PATCH net-next 3/6] dpll: zl3073x: allow enabling/disabling output pins

[Ivan Vecera]

Add support for enabling and disabling individual output pins through
the DPLL subsystem's state_on_dpll_get/set callbacks, at the
granularity of a single P-pin or N-pin rather than the whole output.
zl3073x_dev_output_pin_state_get() reports the pin connection state and
zl3073x_dev_output_pin_state_set() applies the requested one.

Differential pins are toggled directly through the output_ctrl_x::stop
condition. CMOS pins are muted/unmuted via the GPO override, bracketed
by a clean stop/restart of the whole output since the GPO toggle is not
glitch-free. Output pins advertise DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE
to allow this from userspace.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/core.c | 78 +++++++++++++++++++++++++++++++++++++
 drivers/dpll/zl3073x/core.h | 29 ++++++++++++++
 drivers/dpll/zl3073x/dpll.c | 68 +++++++++++++++++++++++++++++++-
 drivers/dpll/zl3073x/prop.c |  2 +
 4 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
index 7386932df0327f..89cf46111dcb13 100644
--- a/drivers/dpll/zl3073x/core.c
+++ b/drivers/dpll/zl3073x/core.c
@@ -3,6 +3,7 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/delay.h>
 #include <linux/dev_printk.h>
 #include <linux/device.h>
 #include <linux/export.h>
@@ -12,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/sprintf.h>
 #include <linux/string_choices.h>
+#include <linux/time64.h>
 #include <linux/unaligned.h>
 #include <net/devlink.h>
 
@@ -667,6 +669,82 @@ int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value)
 	return zl3073x_write_u8(zldev, reg, val);
 }
 
+/**
+ * zl3073x_dev_output_pin_state_set - enable or disable the given output pin
+ * @zldev: pointer to zl3073x_dev structure
+ * @id: output pin id
+ * @enable: true to enable the pin, false to disable it
+ *
+ * Differential output pins are enabled/disabled through the clean
+ * output_ctrl_x::stop condition, since they expose only a single logical
+ * pin.
+ *
+ * CMOS output pins are enabled/disabled by muting/unmuting the pin's
+ * driver via a GPO override. The GPO toggle is not glitch-free, so it is
+ * bracketed by a clean stop/restart of the whole output.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int zl3073x_dev_output_pin_state_set(struct zl3073x_dev *zldev, u8 id,
+				     bool enable)
+{
+	u8 out_id = zl3073x_output_pin_out_get(id);
+	struct zl3073x_out out;
+	u32 delay, freq;
+	int rc;
+
+	out = *zl3073x_out_state_get(zldev, out_id);
+
+	if (zl3073x_out_is_diff(&out)) {
+		if (enable)
+			zl3073x_out_start(&out);
+		else
+			zl3073x_out_stop(&out);
+
+		return zl3073x_out_state_set(zldev, out_id, &out);
+	}
+
+	/* Bracket the GPO override toggle below with a clean stop/restart,
+	 * since the toggle itself is not glitch-free.
+	 */
+	zl3073x_out_stop(&out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		return rc;
+
+	/* output_ctrl_x::stop is edge-aligned, so the device can take up
+	 * to half a period to actually reach the stopped state. Wait for
+	 * that long plus 25 ms, to make sure it is really stopped before
+	 * touching the GPO override below.
+	 */
+	delay = 25 * USEC_PER_MSEC;
+	freq = zl3073x_dev_output_pin_freq_get(zldev, id);
+	if (freq)
+		delay += USEC_PER_SEC / 2 / freq;
+	fsleep(delay);
+
+	if (enable) {
+		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_CLOCK);
+	} else {
+		rc = zl3073x_dev_gpo_set(zldev, id, false);
+		if (rc)
+			goto restart_output;
+		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_GPO_CONST);
+	}
+
+	/* Restart the output regardless of the result below: on failure,
+	 * don't leave the whole output, including the unrelated sibling
+	 * pin, stopped indefinitely.
+	 */
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+
+restart_output:
+	zl3073x_out_start(&out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;
+
+	return rc;
+}
+
 /**
  * zl3073x_ref_freq_meas_latch - latch reference frequency measurements
  * @zldev: pointer to zl3073x_dev structure
diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
index e7064f2958fa5f..2b4785ba18b237 100644
--- a/drivers/dpll/zl3073x/core.h
+++ b/drivers/dpll/zl3073x/core.h
@@ -168,6 +168,8 @@ int zl3073x_write_hwreg_seq(struct zl3073x_dev *zldev,
 
 int zl3073x_ref_phase_offsets_update(struct zl3073x_dev *zldev, int channel);
 int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value);
+int zl3073x_dev_output_pin_state_set(struct zl3073x_dev *zldev, u8 id,
+				     bool enable);
 
 /**
  * zl3073x_dev_is_ref_phase_comp_32bit - check ref phase comp register size
@@ -447,4 +449,31 @@ zl3073x_dev_output_pin_is_enabled(struct zl3073x_dev *zldev, u8 id)
 	return true;
 }
 
+/**
+ * zl3073x_dev_output_pin_state_get - get the given output pin connection state
+ * @zldev: pointer to zl3073x device
+ * @id: output pin id
+ *
+ * Differential outputs are connected when their clock is not stopped.
+ * CMOS ones are connected when they are not GPO-overridden - the pin would
+ * not have been registered at all if its P/N side was not enabled by the
+ * signal format in the first place.
+ *
+ * Return: true if the output pin is connected, false if disconnected
+ */
+static inline bool
+zl3073x_dev_output_pin_state_get(struct zl3073x_dev *zldev, u8 id)
+{
+	u8 out_id = zl3073x_output_pin_out_get(id);
+	const struct zl3073x_out *out;
+
+	out = zl3073x_out_state_get(zldev, out_id);
+
+	if (zl3073x_out_is_stopped(out))
+		return false;
+
+	return zl3073x_out_is_diff(out) ||
+	       zl3073x_out_pin_func_get(out, id) == ZL3073X_OUT_PIN_F_CLOCK;
+}
+
 #endif /* _ZL3073X_CORE_H */
diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 2c6de4dab8b4ad..426974b0b5dc5c 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -17,6 +17,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/slab.h>
 #include <linux/sprintf.h>
+#include <linux/time64.h>
 
 #include "core.h"
 #include "dpll.h"
@@ -1195,12 +1196,74 @@ zl3073x_dpll_output_pin_state_on_dpll_get(const struct dpll_pin *dpll_pin,
 					  enum dpll_pin_state *state,
 					  struct netlink_ext_ack *extack)
 {
-	/* If the output pin is registered then it is always connected */
-	*state = DPLL_PIN_STATE_CONNECTED;
+	struct zl3073x_dpll *zldpll = dpll_priv;
+	struct zl3073x_dev *zldev = zldpll->dev;
+	struct zl3073x_dpll_pin *pin = pin_priv;
+
+	guard(mutex)(&zldpll->lock);
+
+	if (zl3073x_dev_output_pin_state_get(zldev, pin->id))
+		*state = DPLL_PIN_STATE_CONNECTED;
+	else
+		*state = DPLL_PIN_STATE_DISCONNECTED;
 
 	return 0;
 }
 
+/**
+ * zl3073x_dpll_output_pin_state_on_dpll_set - enable or disable an output pin
+ * @dpll_pin: registered dpll_pin
+ * @pin_priv: pointer to zl3073x_dpll_pin structure
+ * @dpll: registered dpll_device
+ * @dpll_priv: pointer to zl3073x_dpll structure
+ * @state: requested pin state
+ * @extack: netlink extack pointer
+ *
+ * Differential output pins are enabled/disabled through the clean
+ * output_ctrl_x::stop condition, since they expose only a single
+ * logical pin.
+ *
+ * CMOS output pins are enabled/disabled by muting/unmuting the pin's
+ * driver via a GPO override rather than by changing the output's
+ * signal_format, since a signal_format change is not glitch-free on
+ * this hardware. The GPO toggle itself is not glitch-free either, so
+ * it is bracketed by a clean stop/restart of the whole output.
+ *
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *dpll_pin,
+					  void *pin_priv,
+					  const struct dpll_device *dpll,
+					  void *dpll_priv,
+					  enum dpll_pin_state state,
+					  struct netlink_ext_ack *extack)
+{
+	struct zl3073x_dpll *zldpll = dpll_priv;
+	struct zl3073x_dev *zldev = zldpll->dev;
+	struct zl3073x_dpll_pin *pin = pin_priv;
+	bool enable;
+	int rc = 0;
+
+	if (state != DPLL_PIN_STATE_CONNECTED &&
+	    state != DPLL_PIN_STATE_DISCONNECTED) {
+		NL_SET_ERR_MSG(extack, "Invalid pin state for output pin");
+		return -EINVAL;
+	}
+
+	guard(mutex)(&zldpll->lock);
+
+	enable = state == DPLL_PIN_STATE_CONNECTED;
+	if (zl3073x_dev_output_pin_state_get(zldev, pin->id) != enable) {
+		rc = zl3073x_dev_output_pin_state_set(zldev, pin->id, enable);
+		if (rc)
+			NL_SET_ERR_MSG(extack,
+				       "Failed to change output pin state");
+	}
+
+	return rc;
+}
+
 static int
 zl3073x_dpll_nco_pin_operstate_on_dpll_get(const struct dpll_pin *dpll_pin,
 					   void *pin_priv,
@@ -1685,6 +1748,7 @@ static const struct dpll_pin_ops zl3073x_dpll_output_pin_ops = {
 	.phase_adjust_get = zl3073x_dpll_output_pin_phase_adjust_get,
 	.phase_adjust_set = zl3073x_dpll_output_pin_phase_adjust_set,
 	.state_on_dpll_get = zl3073x_dpll_output_pin_state_on_dpll_get,
+	.state_on_dpll_set = zl3073x_dpll_output_pin_state_on_dpll_set,
 };
 
 static const struct dpll_pin_ops zl3073x_dpll_nco_pin_ops = {
diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
index ac9d41d0f978ef..806d1e6622da5a 100644
--- a/drivers/dpll/zl3073x/prop.c
+++ b/drivers/dpll/zl3073x/prop.c
@@ -214,6 +214,8 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
 		u32 f;
 
 		props->dpll_props.type = DPLL_PIN_TYPE_GNSS;
+		props->dpll_props.capabilities =
+			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
 
 		/* The output pin phase adjustment granularity equals half of
 		 * the synth frequency count.
-- 
2.55.0

[PATCH net-next 4/6] dpll: zl3073x: consolidate pin capabilities into bitmap

[Ivan Vecera]

Replace the per-pin 'esync_control' boolean with a 'caps' bitmap in
struct zl3073x_dpll_pin. The bitmap is built from the pin properties
during pin registration and is easily extensible with further per-pin
capabilities.

No functional change intended.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 426974b0b5dc5c..fcf91aba2988af 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -38,7 +38,7 @@
  * @dir: pin direction
  * @id: pin id
  * @prio: pin priority <0, 14>
- * @esync_control: embedded sync is controllable
+ * @caps: pin capabilities (ZL3073X_DPLL_PIN_CAP_*)
  * @phase_gran: phase adjustment granularity
  * @operstate: last saved operational state
  * @phase_offset: last saved pin phase offset
@@ -55,7 +55,7 @@ struct zl3073x_dpll_pin {
 	enum dpll_pin_direction	dir;
 	u8			id;
 	u8			prio;
-	bool			esync_control;
+	u8			caps;
 	s32			phase_gran;
 	enum dpll_pin_operstate	operstate;
 	s64			phase_offset;
@@ -63,6 +63,17 @@ struct zl3073x_dpll_pin {
 	u32			measured_freq;
 };
 
+/*
+ * DPLL pin capabilities
+ */
+enum zl3073x_dpll_pin_caps {
+	ZL3073X_DPLL_PIN_CAP_ESYNC_BIT,
+	ZL3073X_DPLL_PIN_CAPS_NBITS /* must be last */
+};
+
+#define __ZL3073X_DPLL_PIN_CAP(name)	BIT(ZL3073X_DPLL_PIN_CAP_##name##_BIT)
+#define ZL3073X_DPLL_PIN_CAP_ESYNC	__ZL3073X_DPLL_PIN_CAP(ESYNC)
+
 /*
  * Supported esync ranges for input and for output per output pair type
  */
@@ -189,7 +200,8 @@ zl3073x_dpll_input_pin_esync_get(const struct dpll_pin *dpll_pin,
 	ref_id = zl3073x_input_pin_ref_get(pin->id);
 	ref = zl3073x_ref_state_get(zldev, ref_id);
 
-	if (!pin->esync_control || zl3073x_ref_freq_get(ref) <= 1)
+	if (!(pin->caps & ZL3073X_DPLL_PIN_CAP_ESYNC) ||
+	    zl3073x_ref_freq_get(ref) <= 1)
 		return -EOPNOTSUPP;
 
 	esync->range = esync_freq_ranges;
@@ -897,7 +909,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
 	synth_freq = zl3073x_synth_freq_get(synth);
 	out_freq = synth_freq / out->div;
 
-	if (!pin->esync_control || out_freq <= 1)
+	if (!(pin->caps & ZL3073X_DPLL_PIN_CAP_ESYNC) || out_freq <= 1)
 		return -EOPNOTSUPP;
 
 	esync->range = esync_freq_ranges;
@@ -1837,14 +1849,17 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	if (IS_ERR(props))
 		return PTR_ERR(props);
 
-	/* Save package label, fwnode, esync capability and phase adjust
+	/* Save package label, fwnode, capabilities and phase adjust
 	 * granularity.
 	 */
 	strscpy(pin->label, props->package_label);
 	pin->fwnode = fwnode_handle_get(props->fwnode);
-	pin->esync_control = props->esync_control;
 	pin->phase_gran = props->dpll_props.phase_gran;
 
+	pin->caps = 0;
+	if (props->esync_control)
+		pin->caps |= ZL3073X_DPLL_PIN_CAP_ESYNC;
+
 	if (zl3073x_dpll_is_input_pin(pin)) {
 		const struct zl3073x_chan *chan;
 		u8 ref;
-- 
2.55.0

[PATCH net-next 5/6] dpll: zl3073x: factor out output pin frequency helper

[Ivan Vecera]

Extract the divisor and N-division computation from the output pin
frequency_set callback into zl3073x_dpll_output_pin_freq_set(), which
updates a struct zl3073x_out without committing it to hardware. This
lets the upcoming PTP periodic output support reuse the same P-pin,
N-pin and N-divided frequency handling.

No functional change intended.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 135 ++++++++++++++++++++----------------
 1 file changed, 74 insertions(+), 61 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index fcf91aba2988af..0a36a2acf15b9f 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -1032,96 +1032,109 @@ zl3073x_dpll_output_pin_frequency_get(const struct dpll_pin *dpll_pin,
 	return 0;
 }
 
+/**
+ * zl3073x_dpll_output_pin_freq_set - compute output config for pin frequency
+ * @pin: output pin to set the frequency for
+ * @out: output state to update, not committed to hardware
+ * @frequency: requested pin frequency in Hz
+ *
+ * Updates the divisor and N-division fields of @out so the given output
+ * pin runs at the requested frequency. For non N-divided formats the
+ * divisor is shared by both pins of the output pair. The caller is
+ * responsible for committing @out with zl3073x_out_state_set().
+ *
+ * Return: 0 on success, -EINVAL if the frequency cannot be represented
+ */
 static int
-zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
-				      void *pin_priv,
-				      const struct dpll_device *dpll,
-				      void *dpll_priv, u64 frequency,
-				      struct netlink_ext_ack *extack)
+zl3073x_dpll_output_pin_freq_set(struct zl3073x_dpll_pin *pin,
+				 struct zl3073x_out *out, u64 frequency)
 {
-	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_dev *zldev = pin->dpll->dev;
 	u32 new_div, synth_freq;
-	struct zl3073x_out out;
-	u8 out_id;
-	int rc;
+	u8 synth;
 
-	mutex_lock(&zldpll->lock);
-
-	out_id = zl3073x_output_pin_out_get(pin->id);
-	out = *zl3073x_out_state_get(zldev, out_id);
-
-	/* Get attached synth frequency and compute new divisor */
-	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(&out));
-	synth_freq = zl3073x_synth_freq_get(synth);
+	synth = zl3073x_out_synth_get(out);
+	synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
 	new_div = synth_freq / (u32)frequency;
 
-	/* Check signal format */
-	if (!zl3073x_out_is_ndiv(&out)) {
+	if (!zl3073x_out_is_ndiv(out)) {
 		/* For non N-divided signal formats the frequency is computed
 		 * as division of synth frequency and output divisor, which
 		 * is shared by both pins of the output pair.
 		 */
-		out.div = new_div;
+		out->div = new_div;
 
 		/* For 50/50 duty cycle the divisor is equal to width */
-		out.width = new_div;
-
-		/* Commit output configuration */
-		rc = zl3073x_out_state_set(zldev, out_id, &out);
-		if (rc)
-			goto unlock;
+		out->width = new_div;
 
-		/* 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;
+		return 0;
 	}
 
 	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
-		 * to compute N-pin divisor to keep its frequency unchanged.
-		 *
-		 * Update divisor for N-pin to keep N-pin frequency.
+		/* Changing the P-pin frequency, rescale the N-pin divisor to
+		 * keep the N-pin frequency unchanged. Fail if the requested
+		 * frequency is too low to represent the current N-pin one.
 		 */
-		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
-		if (!out.esync_n_period) {
-			rc = -EINVAL;
-			goto unlock;
-		}
+		out->esync_n_period = out->esync_n_period * out->div / new_div;
+		if (!out->esync_n_period)
+			return -EINVAL;
 
 		/* Update the output divisor */
-		out.div = new_div;
+		out->div = new_div;
 
 		/* For 50/50 duty cycle the divisor is equal to width */
-		out.width = out.div;
+		out->width = new_div;
 	} else {
-		/* We are going to change frequency of N-pin but if
-		 * the requested freq is greater or equal than freq of P-pin
-		 * in the output pair we cannot compute divisor for the N-pin.
-		 * In this case indicate a failure.
-		 *
-		 * Update divisor for N-pin
+		/* Changing the N-pin frequency. Fail if the requested
+		 * frequency is higher than or does not divide the P-pin one.
 		 */
-		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
-		if (!out.esync_n_period) {
-			rc = -EINVAL;
-			goto unlock;
-		}
+		out->esync_n_period = div64_u64(synth_freq,
+						frequency * out->div);
+		if (!out->esync_n_period)
+			return -EINVAL;
 	}
 
 	/* For 50/50 duty cycle the divisor is equal to width */
-	out.esync_n_width = out.esync_n_period;
+	out->esync_n_width = out->esync_n_period;
+
+	return 0;
+}
+
+static int
+zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
+				      void *pin_priv,
+				      const struct dpll_device *dpll,
+				      void *dpll_priv, u64 frequency,
+				      struct netlink_ext_ack *extack)
+{
+	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;
+
+	mutex_lock(&zldpll->lock);
+
+	out_id = zl3073x_output_pin_out_get(pin->id);
+	out = *zl3073x_out_state_get(zldev, out_id);
+
+	rc = zl3073x_dpll_output_pin_freq_set(pin, &out, frequency);
+	if (rc)
+		goto unlock;
 
 	/* Commit output configuration */
 	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		goto unlock;
+
+	/* For non N-divided formats the divisor is shared, so the other
+	 * pin's frequency changed too and has to be notified.
+	 */
+	if (!zl3073x_out_is_ndiv(&out))
+		sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+
 unlock:
 	mutex_unlock(&zldpll->lock);
 
-- 
2.55.0

[PATCH net-next 6/6] dpll: zl3073x: add PTP periodic output support

[Ivan Vecera]

Register a PTP periodic output pin for each DPLL output pin that
supports step-time and declares 1 PPS (1 Hz) support in firmware.
The pins are named after the output pin (e.g. OUT5, OUT5P, OUT5N) and
any perout channel can be assigned to any of them.

Only 1 PPS is supported. Enabling a channel programs the pin assigned
to it for 1 Hz and connects it; disabling disconnects it, reusing the
output pin frequency helper and the per-pin connect/disconnect
primitive. A 1PPS bit is added to the per-pin capabilities.

Tested-by: Chris du Quesnay <Chris.duQuesnay@microchip.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 211 ++++++++++++++++++++++++++++++++++++
 drivers/dpll/zl3073x/dpll.h |   4 +
 drivers/dpll/zl3073x/prop.h |  21 ++++
 3 files changed, 236 insertions(+)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 0a36a2acf15b9f..f406d1e72530ff 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -68,11 +68,13 @@ struct zl3073x_dpll_pin {
  */
 enum zl3073x_dpll_pin_caps {
 	ZL3073X_DPLL_PIN_CAP_ESYNC_BIT,
+	ZL3073X_DPLL_PIN_CAP_1PPS_BIT,
 	ZL3073X_DPLL_PIN_CAPS_NBITS /* must be last */
 };
 
 #define __ZL3073X_DPLL_PIN_CAP(name)	BIT(ZL3073X_DPLL_PIN_CAP_##name##_BIT)
 #define ZL3073X_DPLL_PIN_CAP_ESYNC	__ZL3073X_DPLL_PIN_CAP(ESYNC)
+#define ZL3073X_DPLL_PIN_CAP_1PPS	__ZL3073X_DPLL_PIN_CAP(1PPS)
 
 /*
  * Supported esync ranges for input and for output per output pair type
@@ -144,6 +146,19 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
 	return NULL;
 }
 
+static struct zl3073x_dpll_pin *
+zl3073x_dpll_output_pin_get_by_id(struct zl3073x_dpll *zldpll, u8 id)
+{
+	struct zl3073x_dpll_pin *pin;
+
+	list_for_each_entry(pin, &zldpll->pins, list) {
+		if (!zl3073x_dpll_is_input_pin(pin) && pin->id == id)
+			return pin;
+	}
+
+	return NULL;
+}
+
 /**
  * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
  * @pin: output pin whose sibling is sought
@@ -1872,6 +1887,8 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	pin->caps = 0;
 	if (props->esync_control)
 		pin->caps |= ZL3073X_DPLL_PIN_CAP_ESYNC;
+	if (zl3073x_props_is_freq_supported(props, 1))
+		pin->caps |= ZL3073X_DPLL_PIN_CAP_1PPS;
 
 	if (zl3073x_dpll_is_input_pin(pin)) {
 		const struct zl3073x_chan *chan;
@@ -2823,6 +2840,159 @@ zl3073x_dpll_ptp_getmaxphase(struct ptp_clock_info *info __always_unused)
 	return NSEC_PER_SEC - 1;
 }
 
+/**
+ * zl3073x_dpll_pin_is_perout_capable - check output pin perout eligibility
+ * @pin: output pin to check
+ *
+ * A registered output pin can be used for periodic output if its output
+ * supports step-time and the pin declares 1 PPS (1 Hz) support in firmware.
+ *
+ * Return: true if the pin can be used for periodic output.
+ */
+static bool
+zl3073x_dpll_pin_is_perout_capable(struct zl3073x_dpll_pin *pin)
+{
+	struct zl3073x_dev *zldev = pin->dpll->dev;
+	u8 out_id;
+
+	/* Periodic output is only available on output pins */
+	if (zl3073x_dpll_is_input_pin(pin) || zl3073x_dpll_is_nco_pin(pin))
+		return false;
+
+	out_id = zl3073x_output_pin_out_get(pin->id);
+
+	return zl3073x_dev_out_is_stepped(zldev, out_id) &&
+	       (pin->caps & ZL3073X_DPLL_PIN_CAP_1PPS);
+}
+
+/**
+ * zl3073x_dpll_perout_enable - enable 1 PPS periodic output on a pin
+ * @pin: output pin to enable periodic output on
+ * @perout: periodic output request
+ *
+ * Programs the pin for 1 PPS (1 Hz) output and connects it.
+ *
+ * Context: Caller must hold pin->dpll->lock.
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_dpll_perout_enable(struct zl3073x_dpll_pin *pin,
+			   struct ptp_perout_request *perout)
+{
+	u8 out_id = zl3073x_output_pin_out_get(pin->id);
+	struct zl3073x_dev *zldev = pin->dpll->dev;
+	struct zl3073x_out out;
+	int rc;
+
+	/* Only 1 PPS (1 Hz) periodic output is supported */
+	if (perout->period.sec != 1 || perout->period.nsec)
+		return -EINVAL;
+
+	out = *zl3073x_out_state_get(zldev, out_id);
+
+	rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);
+	if (rc)
+		return rc;
+
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		return rc;
+
+	if (zl3073x_dev_output_pin_state_get(zldev, pin->id))
+		return 0;
+
+	return zl3073x_dev_output_pin_state_set(zldev, pin->id, true);
+}
+
+/**
+ * zl3073x_dpll_perout_disable - disable periodic output on a pin
+ * @pin: output pin to disable periodic output on
+ *
+ * Context: Caller must hold pin->dpll->lock.
+ * Return: 0 on success, <0 on error
+ */
+static int
+zl3073x_dpll_perout_disable(struct zl3073x_dpll_pin *pin)
+{
+	struct zl3073x_dev *zldev = pin->dpll->dev;
+
+	if (!zl3073x_dev_output_pin_state_get(zldev, pin->id))
+		return 0;
+
+	return zl3073x_dev_output_pin_state_set(zldev, pin->id, false);
+}
+
+static int zl3073x_dpll_ptp_verify(struct ptp_clock_info *info,
+				   unsigned int pin_idx,
+				   enum ptp_pin_function func,
+				   unsigned int chan)
+{
+	/* Any perout pin can serve any perout channel, the channel range is
+	 * validated by the PTP core. The requested pin is resolved from the
+	 * channel via ptp_find_pin() in the enable callback.
+	 */
+	switch (func) {
+	case PTP_PF_NONE:
+	case PTP_PF_PEROUT:
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int zl3073x_dpll_ptp_enable(struct ptp_clock_info *info,
+				   struct ptp_clock_request *rq, int on)
+{
+	struct zl3073x_dpll *zldpll = container_of(info, struct zl3073x_dpll,
+						   ptp_info);
+	struct zl3073x_dpll_pin *pin = NULL;
+	struct zl3073x_dpll_pin *sibling;
+	int n, pin_idx, rc;
+	u8 id;
+
+	if (rq->type != PTP_CLK_REQ_PEROUT)
+		return -EOPNOTSUPP;
+
+	if (rq->perout.flags)
+		return -EOPNOTSUPP;
+
+	pin_idx = ptp_find_pin(zldpll->ptp_clock, PTP_PF_PEROUT,
+			       rq->perout.index);
+	if (pin_idx < 0)
+		return -EINVAL;
+
+	n = pin_idx;
+	for_each_set_bit(id, zldpll->perout_map, ZL3073X_NUM_OUTPUT_PINS) {
+		if (!n) {
+			pin = zl3073x_dpll_output_pin_get_by_id(zldpll, id);
+			break;
+		}
+		n--;
+	}
+	if (!pin)
+		return -EINVAL;
+
+	mutex_lock(&zldpll->lock);
+	if (on)
+		rc = zl3073x_dpll_perout_enable(pin, &rq->perout);
+	else
+		rc = zl3073x_dpll_perout_disable(pin);
+	mutex_unlock(&zldpll->lock);
+
+	if (rc)
+		return rc;
+
+	/* Notify the affected output pin and, for shared-divisor formats,
+	 * its sibling sharing the same HW output.
+	 */
+	dpll_pin_change_ntf(pin->dpll_pin);
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+	if (sibling)
+		dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return 0;
+}
+
 static const struct ptp_clock_info zl3073x_dpll_ptp_clock_info = {
 	.owner		= THIS_MODULE,
 	.max_adj	= ZL3073X_DPLL_PTP_MAX_ADJ,
@@ -2832,6 +3002,8 @@ static const struct ptp_clock_info zl3073x_dpll_ptp_clock_info = {
 	.adjfine	= zl3073x_dpll_ptp_adjfine,
 	.adjphase	= zl3073x_dpll_ptp_adjphase,
 	.getmaxphase	= zl3073x_dpll_ptp_getmaxphase,
+	.enable		= zl3073x_dpll_ptp_enable,
+	.verify		= zl3073x_dpll_ptp_verify,
 };
 
 /**
@@ -2843,16 +3015,53 @@ static const struct ptp_clock_info zl3073x_dpll_ptp_clock_info = {
 static int zl3073x_dpll_ptp_register(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dev *zldev = zldpll->dev;
+	struct ptp_pin_desc *pin_config;
+	struct zl3073x_dpll_pin *pin;
 	struct ptp_clock *ptp_clock;
+	unsigned int i;
+	u8 id;
 
 	zldpll->ptp_info = zl3073x_dpll_ptp_clock_info;
 	snprintf(zldpll->ptp_info.name, sizeof(zldpll->ptp_info.name),
 		 "%s-dpll%u", dev_name(zldev->dev), zldpll->id);
 
+	/* Count output pins eligible for periodic output */
+	bitmap_zero(zldpll->perout_map, ZL3073X_NUM_OUTPUT_PINS);
+	list_for_each_entry(pin, &zldpll->pins, list)
+		if (zl3073x_dpll_pin_is_perout_capable(pin))
+			set_bit(pin->id, zldpll->perout_map);
+
+	zldpll->ptp_info.n_pins = bitmap_weight(zldpll->perout_map,
+						ZL3073X_NUM_OUTPUT_PINS);
+	zldpll->ptp_info.n_per_out = zldpll->ptp_info.n_pins;
+	if (!zldpll->ptp_info.n_pins)
+		goto no_pins;
+
+	pin_config = kzalloc_objs(*pin_config, zldpll->ptp_info.n_pins);
+	if (!pin_config)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_set_bit(id, zldpll->perout_map, ZL3073X_NUM_OUTPUT_PINS) {
+		pin = zl3073x_dpll_output_pin_get_by_id(zldpll, id);
+		strscpy(pin_config[i].name, pin->label);
+		pin_config[i].index = i;
+		if (zl3073x_dev_output_pin_state_get(zldev, id)) {
+			pin_config[i].func = PTP_PF_PEROUT;
+			pin_config[i].chan = i;
+		}
+		i++;
+	}
+
+	zldpll->ptp_info.pin_config = pin_config;
+
+no_pins:
 	ptp_clock = ptp_clock_register(&zldpll->ptp_info, zldev->dev);
 	if (IS_ERR(ptp_clock)) {
 		dev_err(zldev->dev, "Failed to register PTP clock for DPLL%u\n",
 			zldpll->id);
+		kfree(zldpll->ptp_info.pin_config);
+		zldpll->ptp_info.pin_config = NULL;
 		return PTR_ERR(ptp_clock);
 	}
 
@@ -2871,6 +3080,8 @@ static void zl3073x_dpll_ptp_unregister(struct zl3073x_dpll *zldpll)
 		ptp_clock_unregister(zldpll->ptp_clock);
 		zldpll->ptp_clock = NULL;
 	}
+	kfree(zldpll->ptp_info.pin_config);
+	zldpll->ptp_info.pin_config = NULL;
 }
 
 /**
diff --git a/drivers/dpll/zl3073x/dpll.h b/drivers/dpll/zl3073x/dpll.h
index 993221dc63249d..b9ae6d8301031a 100644
--- a/drivers/dpll/zl3073x/dpll.h
+++ b/drivers/dpll/zl3073x/dpll.h
@@ -9,6 +9,8 @@
 
 #include "core.h"
 
+struct zl3073x_dpll_pin;
+
 /**
  * struct zl3073x_dpll - ZL3073x DPLL sub-device structure
  * @list: this DPLL list entry
@@ -25,6 +27,7 @@
  * @pins: list of pins
  * @ptp_info: PTP clock info
  * @ptp_clock: registered PTP clock (or NULL)
+ * @perout_map: bitmap of output pins eligible for periodic output
  */
 struct zl3073x_dpll {
 	struct list_head		list;
@@ -41,6 +44,7 @@ struct zl3073x_dpll {
 	struct list_head		pins;
 	struct ptp_clock_info		ptp_info;
 	struct ptp_clock		*ptp_clock;
+	DECLARE_BITMAP(perout_map, ZL3073X_NUM_OUTPUT_PINS);
 };
 
 struct zl3073x_dpll *zl3073x_dpll_alloc(struct zl3073x_dev *zldev, u8 ch);
diff --git a/drivers/dpll/zl3073x/prop.h b/drivers/dpll/zl3073x/prop.h
index 721a18f05938b8..c5f477b56d4ab1 100644
--- a/drivers/dpll/zl3073x/prop.h
+++ b/drivers/dpll/zl3073x/prop.h
@@ -31,4 +31,25 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
 
 void zl3073x_pin_props_put(struct zl3073x_pin_props *props);
 
+/**
+ * zl3073x_props_is_freq_supported - check if pin supports given frequency
+ * @props: pin properties
+ * @freq: frequency to check in Hz
+ *
+ * Return: true if the frequency is within the pin supported frequency ranges.
+ */
+static inline bool
+zl3073x_props_is_freq_supported(const struct zl3073x_pin_props *props, u64 freq)
+{
+	const struct dpll_pin_frequency *freqs;
+	int i;
+
+	freqs = props->dpll_props.freq_supported;
+	for (i = 0; i < props->dpll_props.freq_supported_num; i++)
+		if (freq >= freqs[i].min && freq <= freqs[i].max)
+			return true;
+
+	return false;
+}
+
 #endif /* _ZL3073X_PROP_H */
-- 
2.55.0

Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 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).

- [High] The patch adds zl3073x_dpll_output_pin_sibling_get()
  (drivers/dpll/zl3073x/dpll.c:146).
- [Medium] The commit says it closes the case where the sibling pin's
  frequency changes without a notification.
- [Low] The commit message presents the missing sibling notification as a
  user-visible defect: userspace listening on the sibling pin "is never…

--

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

This patch adds zl3073x_dpll_output_pin_sibling_get(), which finds the
other pin of a P/N output pair. The esync, frequency and phase adjust
setters use it to send a pin change notification for the sibling after
the shared output registers are written, and they now use explicit
mutex_lock()/mutex_unlock() so the notification goes out after
zldpll->lock is dropped.

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

[Severity: Low]
This describes a user-visible defect, but the trailers only have
Tested-by: and Signed-off-by:.

The dpll core only calls __dpll_pin_change_ntf() for the pin in the
request (for example in dpll_pin_freq_set()). The sibling has not been
notified since the output setters were added.

Should this carry Fixes: tags such as these?

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")

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40..2c6de4dab8b4 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -132,6 +132,30 @@ 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;
> +
> +	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;
> +}
> +

[ ... ]

> @@ -934,27 +962,44 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,

[ ... ]

> +	/* Commit output configuration */
> +	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.
>  	 */
> -	out.esync_n_width = out.div / 2;
> +	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
>  
> -	/* Commit output configuration */
> -	return zl3073x_out_state_set(zldev, out_id, &out);
> +	mutex_unlock(&zldpll->lock);
> +
> +	if (sibling)
> +		__dpll_pin_change_ntf(sibling->dpll_pin);

[Severity: High]
Can sibling->dpll_pin be NULL, or the sibling already freed, by the time
it is dereferenced here?

zl3073x_dpll_output_pin_sibling_get() returns any output pin on
zldpll->pins with the matching id. It does not check that
sibling->dpll_pin is still set. The lookup runs under zldpll->lock, but
the dereference happens after mutex_unlock().

The teardown path does not take zldpll->lock:

drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_pins_unregister() {
	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
		zl3073x_dpll_pin_unregister(pin);
		list_del(&pin->list);
		zl3073x_dpll_pin_free(pin);
	}
}

zl3073x_dpll_pin_unregister() ends with:

	dpll_pin_put(pin->dpll_pin, &pin->tracker);
	pin->dpll_pin = NULL;

and dpll_pin_put() has already dropped dpll_lock at that point.

zl3073x_dpll_pins_register() uses list_add(), so the N pin (2k+1) sits
before its P pin (2k) on the list. That means N is torn down while P is
still registered and reachable over netlink.

One possible interleaving:

CPU1 (devlink reload, flash_prepare or unbind)
zl3073x_dev_stop()
  zl3073x_dpll_unregister()
    zl3073x_dpll_pins_unregister()
      dpll_pin_put(N)          /* dpll_lock released */

CPU0 (DPLL_CMD_PIN_SET on P, dpll_lock held)
dpll_pin_freq_set()
  zl3073x_dpll_output_pin_frequency_set(P)
    mutex_lock(&zldpll->lock)
    sibling = zl3073x_dpll_output_pin_sibling_get(P)  /* finds N */
    mutex_unlock(&zldpll->lock)

CPU1
      N->dpll_pin = NULL
      list_del(&N->list)
      zl3073x_dpll_pin_free(N)

CPU0
    __dpll_pin_change_ntf(sibling->dpll_pin)

In that case, would __dpll_pin_change_ntf() get NULL and oops on
pin->id and pin->clock_id in dpll_pin_notify()? Or would CPU0 read the
freed zl3073x_dpll_pin?

The list walk in zl3073x_dpll_output_pin_sibling_get() can also run
while list_del() is in progress, because the writer never takes
zldpll->lock.

zl3073x_dpll_output_pin_frequency_set() and
zl3073x_dpll_output_pin_phase_adjust_set() have the same pattern.

The later patch "dpll: zl3073x: add PTP periodic output support" also
calls zl3073x_dpll_output_pin_sibling_get() from
zl3073x_dpll_ptp_enable() without holding zldpll->lock at all.

[ ... ]

> @@ -1010,7 +1058,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>  		out.width = new_div;
>  
>  		/* 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)) {
> @@ -1022,8 +1079,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>  		 * Update divisor for N-pin to keep N-pin frequency.
>  		 */
>  		out.esync_n_period = (out.esync_n_period * out.div) / new_div;

[Severity: Medium]
Does the N pin also need a notification on this path?

In the N-div formats OUTPUT_DIV is still shared. The N-pin frequency is
computed from both values:

drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
	...
	freq = zl3073x_synth_freq_get(synth) / out->div;

	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;
	...
}

The rescale above truncates and only rejects 0. zl3073x_pin_check_freq()
only requires the new P frequency to divide the synth frequency.

For example, take a 500 MHz synth with P = 10 MHz (div 50) and
N = 2 MHz (period 5). Setting P to 25 MHz gives new_div 20 and period
250 / 20 = 12. N then becomes 500M / (20 * 12), about 2.083 MHz.

This path reaches the unlock label with sibling still NULL, so the N
pin gets no DPLL_CMD_PIN_CHANGE_NTF. The commit message says the sibling
is notified "whenever the shared HW state actually changes". Should this
case be covered too?

The later commit "dpll: zl3073x: factor out output pin frequency helper"
does not change this. The truncation moves into
zl3073x_dpll_output_pin_freq_set(), and frequency_set still calls
zl3073x_dpll_output_pin_sibling_get() only when
!zl3073x_out_is_ndiv(&out).

> -		if (!out.esync_n_period)
> -			return -EINVAL;
> +		if (!out.esync_n_period) {
> +			rc = -EINVAL;
> +			goto unlock;
> +		}

[ ... ]

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

Re: [PATCH net-next 2/6] dpll: zl3073x: add GPO support for output pins

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] Error-recovery path applies the output restart before the
  pending GPO change.
- [Low] The struct zl3073x_dev kernel-doc in drivers/dpll/zl3073x/core.h
  (line 65, '@out: array of outs' invariants') and the inline '/*…

--

dpll: zl3073x: add GPO support for output pins

This commit adds the GPO override mailbox fields and the direct gpo_out_x
registers for CMOS output pins, along with helpers to read and set the pin
function. It also adds the output_ctrl_x stop bits and stop/start helpers.
ctrl moves into the cfg struct_group, and zl3073x_out_state_set() now
writes it directly.

> diff --git a/drivers/dpll/zl3073x/out.c b/drivers/dpll/zl3073x/out.c
> index 410d15b96d0b..82564045890d 100644
> --- a/drivers/dpll/zl3073x/out.c
> +++ b/drivers/dpll/zl3073x/out.c

[ ... ]

> @@ -120,11 +135,17 @@ int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
>  	struct zl3073x_out *dout = &zldev->out[index];
>  	int rc;
>  
> -	/* Reject attempts to change invariant fields (set at fetch only) */
> -	if (WARN_ON(memcmp(&dout->inv, &out->inv, sizeof(out->inv))))
> -		return -EINVAL;
> +	/* ctrl is a direct register, independent of the output mailbox */
> +	if (dout->ctrl != out->ctrl) {
> +		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
> +				      out->ctrl);
> +		if (rc)
> +			return rc;
> +
> +		dout->ctrl = out->ctrl;
> +	}

[Severity: Medium]

zl3073x_out_state_set() now always writes output_ctrl_x before any mailbox
field. It does not roll dout->ctrl back if the later zl3073x_mb_op() or a
mailbox field write fails.

Can this order apply a restart before a pending GPO change on an error
recovery path?

The follow-up patch "dpll: zl3073x: allow enabling/disabling output pins"
adds zl3073x_dev_output_pin_state_set(). It puts a stop before the GPO
change and a start after it. Its recovery path looks like this:

	rc = zl3073x_out_state_set(zldev, out_id, &out);

restart_output:
	zl3073x_out_start(&out);
	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;

The second zl3073x_out_state_set() call only changes gpo_en/gpo_config.
Suppose it fails in zl3073x_mb_op() or in one of the mailbox field writes.
Then dout->cfg keeps the old GPO values, while out still holds the new
ones.

The recovery call then carries two changes: the cleared STOP/STOP_HZ bits
and the uncommitted GPO change. Because ctrl is written first, would the
clock restart, with the GPO override then changed on a running output?

That looks like the toggle the stop/start pair is meant to avoid. The
driver's own comment says the GPO toggle is not glitch-free. For
enable=true, the override is released onto a clock that is already
running, at an arbitrary phase. For enable=false, the override is engaged
in the middle of a clock cycle.

Also, if the retried commit succeeds, the "? : rc" still returns the
earlier error. zl3073x_dpll_output_pin_state_on_dpll_set() would then
report a failure even though the pin state changed.

>  
> -	/* Skip HW write if configuration hasn't changed */
> +	/* Skip the mailbox commit if nothing else has changed */
>  	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
>  		return 0;

[ ... ]

> diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
> index 660889c57bff..66a8432c00df 100644
> --- a/drivers/dpll/zl3073x/out.h
> +++ b/drivers/dpll/zl3073x/out.h

[ ... ]

> @@ -29,8 +32,9 @@ struct zl3073x_out {
>  		u32	esync_n_width;
>  		s32	phase_comp;
>  		u8	mode;
> -	);
> -	struct_group(inv, /* Invariants */
> +		u8	gpo_en;
> +		u8	gpo_config_p;
> +		u8	gpo_config_n;
>  		u8	ctrl;
>  	);
>  };

[Severity: Low]

This isn't a bug, but with the inv group removed, struct zl3073x_out has
no invariant fields left. The struct zl3073x_dev comments in core.h still
describe out[] as invariants:

 * @out: array of outs' invariants
...
	/* Invariants */
	struct zl3073x_ref	ref[ZL3073X_NUM_REFS];
	struct zl3073x_out	out[ZL3073X_NUM_OUTS];

These comments are still unchanged at the end of the series. Should they
be updated along with this change?

[ ... ]

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

Re: [PATCH net-next 3/6] dpll: zl3073x: allow enabling/disabling output pins

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · 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] On CMOS outputs, the output-wide STOP bit is mixed up with the
  per-pin GPO state.
- [Medium] CMOS output pins that firmware or board configuration set up
  as GPO status, IRQ or unknown-mode outputs…
- [Medium] The error path of zl3073x_dev_output_pin_state_set() does not
  close the stop/restart bracket symmetrically, for either CMOS pin.
- [Medium] The settle delay before the non-glitch-free GPO toggle in
  zl3073x_dev_output_pin_state_set() may be too short.

--

dpll: zl3073x: allow enabling/disabling output pins

This commit adds state_on_dpll_set support for zl3073x output pins.
Differential pins are toggled through the output_ctrl_x::stop bit. CMOS
pins are muted/unmuted through a GPO override, bracketed by a stop/restart
of the whole output. Output pins now advertise
DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE.

> diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
> index 7386932df032..89cf46111dcb 100644
> --- a/drivers/dpll/zl3073x/core.c
> +++ b/drivers/dpll/zl3073x/core.c

[ ... ]

> @@ -667,6 +669,82 @@ int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value)

[ ... ]

> +int zl3073x_dev_output_pin_state_set(struct zl3073x_dev *zldev, u8 id,
> +				     bool enable)
> +{

[ ... ]

> +	/* Bracket the GPO override toggle below with a clean stop/restart,
> +	 * since the toggle itself is not glitch-free.
> +	 */
> +	zl3073x_out_stop(&out);
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +	if (rc)
> +		return rc;
> +
> +	/* output_ctrl_x::stop is edge-aligned, so the device can take up
> +	 * to half a period to actually reach the stopped state. Wait for
> +	 * that long plus 25 ms, to make sure it is really stopped before
> +	 * touching the GPO override below.
> +	 */
> +	delay = 25 * USEC_PER_MSEC;
> +	freq = zl3073x_dev_output_pin_freq_get(zldev, id);
> +	if (freq)
> +		delay += USEC_PER_SEC / 2 / freq;
> +	fsleep(delay);

[Severity: Medium]
Is this delay always long enough? zl3073x_dev_output_pin_freq_get() returns
an integer Hz value:

drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
    ...
	freq = zl3073x_synth_freq_get(synth) / out->div;

	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;
    ...
}

Sub-Hz rates truncate to 0. Take an N-div N pin with P at 1 Hz and
esync_n_period = 2, which runs at 0.5 Hz. freq becomes 0, so the half-period
term is skipped. The code then waits only 25 ms, while the real half period
is about 1 s.

Also, only the requested pin's frequency is used. On an N-div output, the
P pin's delay comes from the fast P rate. If the device aligns the stop of
the whole output to the slower N edge, could the P pin still be running
when its GPO override is toggled? This second case depends on how the
hardware aligns the stop, so it is less certain.

In either case, can the GPO mux switch while the clock is still active and
produce the runt pulse that the stop/restart bracket is meant to prevent?
This code is still the same at the end of the series.

> +
> +	if (enable) {
> +		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_CLOCK);

[Severity: Medium]
What happens here to a CMOS pin that firmware or the board set up as a GPO
status, IRQ or unknown-mode output (ZL3073X_OUT_PIN_F_GPO_STATUS,
ZL3073X_OUT_PIN_F_GPO_IRQ or ZL3073X_OUT_PIN_F_GPO_UNKNOWN)?

zl3073x_dpll_pin_is_registrable() only checks synth/output enablement and
signal format, so these pins are still registered.

Before this patch, every registered output pin reported connected. Now
zl3073x_dev_output_pin_state_get() reports these pins as disconnected,
because their function is not CLOCK:

	return zl3073x_out_is_diff(out) ||
	       zl3073x_out_pin_func_get(out, id) == ZL3073X_OUT_PIN_F_CLOCK;

zl3073x_pin_props_get() also now sets DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE
on every output pin.

A connect request then reaches this zl3073x_out_pin_func_set() call. It
clears gpo_en and throws away the status/IRQ function. A clock is then
driven onto a line the board may have wired as a lock-status or interrupt
signal.

A later disconnect writes GPO_CONST low, so the DPLL API can never restore
the original function.

Should pins with a GPO status/IRQ function be excluded from state changes,
or at least not advertise STATE_CAN_CHANGE? Neither the commit message nor
the rest of the series covers these pins.

> +	} else {
> +		rc = zl3073x_dev_gpo_set(zldev, id, false);
> +		if (rc)
> +			goto restart_output;
> +		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_GPO_CONST);
> +	}
> +
> +	/* Restart the output regardless of the result below: on failure,
> +	 * don't leave the whole output, including the unrelated sibling
> +	 * pin, stopped indefinitely.
> +	 */
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +
> +restart_output:
> +	zl3073x_out_start(&out);
> +	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;

[Severity: Medium]
What does this restart do if the zl3073x_out_state_set() call just above
fails?

zl3073x_out_state_set() writes ctrl first. It stores dout->cfg only after
a successful mailbox commit:

drivers/dpll/zl3073x/out.c:zl3073x_out_state_set() {
    ...
	if (dout->ctrl != out->ctrl) {
		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
				      out->ctrl);
		...
		dout->ctrl = out->ctrl;
	}

	/* Skip the mailbox commit if nothing else has changed */
	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
		return 0;
    ...
	/* After successful commit store new state */
	dout->cfg = out->cfg;
    ...
}

After a failed GPO commit, the local out holds the new gpo_en/gpo_config
values and the cache still holds the old ones.

The restart call writes ctrl first, which clears STOP and starts the output
again. It then sees the cfg difference and retries the same GPO mailbox
commit while the output is running. Isn't that the non-glitch-free toggle
the bracket is supposed to prevent?

If the retry succeeds, the restart call returns 0 and "? : rc" keeps the
earlier error.

zl3073x_dpll_output_pin_state_on_dpll_set() then reports "Failed to change
output pin state". dpll_pin_state_set() also skips __dpll_pin_change_ntf(),
even though the hardware and zldev->out[] now hold the new pin state.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 2c6de4dab8b4..426974b0b5dc 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> +static int
> +zl3073x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *dpll_pin,

[ ... ]

> +	guard(mutex)(&zldpll->lock);
> +
> +	enable = state == DPLL_PIN_STATE_CONNECTED;
> +	if (zl3073x_dev_output_pin_state_get(zldev, pin->id) != enable) {
> +		rc = zl3073x_dev_output_pin_state_set(zldev, pin->id, enable);

[Severity: Medium]
Can a disconnect request on a CMOS pin be silently dropped here?
zl3073x_dev_output_pin_state_get() returns false for both P and N whenever
the output-wide STOP bit is set, whatever each pin's GPO function is:

	if (zl3073x_out_is_stopped(out))
		return false;

A CMOS output can be stopped in two ways:

- At probe. zl3073x_dpll_pin_is_registrable() does not reject stopped
  outputs, so an output that firmware left stopped is still registered.
- After the final restart write in zl3073x_dev_output_pin_state_set()
  fails.

Take a stopped CMOS output where N is still in CLOCK function:

  disconnect N
    zl3073x_dev_output_pin_state_get(N) returns false, same as enable
    -> returns 0, N stays in CLOCK function

  connect P
    zl3073x_dev_output_pin_state_set(P, true)
      zl3073x_out_start(&out)    <- restarts the whole output

Wouldn't N then drive its clock again, even though userspace was told N is
disconnected?

Changing one pin can also change the sibling's state:

- Restarting a stopped output moves the sibling from disconnected to
  connected.
- A failed final restart moves the sibling from connected to disconnected.

dpll_pin_state_set() only calls __dpll_pin_change_ntf() for the pin that
was requested.

Should this follow the pattern that the earlier patch in this series, "dpll:
zl3073x: notify sibling pin when shared output config changes", added to
zl3073x_dpll_output_pin_phase_adjust_set()?

	sibling = zl3073x_dpll_output_pin_sibling_get(pin);

	mutex_unlock(&zldpll->lock);

	if (sibling)
		__dpll_pin_change_ntf(sibling->dpll_pin);

This is still present at the end of the series. The PTP perout
enable/disable helpers added in "dpll: zl3073x: add PTP periodic output
support" call the same zl3073x_dev_output_pin_state_get() and
zl3073x_dev_output_pin_state_set() pair.

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

Re: [PATCH net-next 5/6] dpll: zl3073x: factor out output pin frequency helper

[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 kernel-doc of the new helper
  zl3073x_dpll_output_pin_freq_set() in drivers/dpll/zl3073x/dpll.c says…

Pre-existing issues:
- [High] A user with CAP_NET_ADMIN can trigger a divide-by-zero oops in
  zl3073x_dpll_output_pin_freq_set() (drivers/dpll/zl3073x/dpll.c:1058,…
- [Medium] (Baseline: preexisting, medium; not reported as a finding of
  this patch.) In the N-div P-pin branch of…
- [Medium] (Baseline: preexisting, medium; not reported as a finding of
  this patch.) In zl3073x_dpll_output_pin_freq_set(), the P-pin rescale…
- [Low] (Baseline: not confirmed as a new issue.) The concern says
  zl3073x_dpll_output_pin_freq_set() does not validate its input, which…

--

dpll: zl3073x: factor out output pin frequency helper

This commit moves the divisor and N-division computation out of
zl3073x_dpll_output_pin_frequency_set() into a new helper,
zl3073x_dpll_output_pin_freq_set(). The helper updates a struct
zl3073x_out but does not write it to hardware, so the upcoming PTP
periodic output code can reuse it. No functional change is intended.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index fcf91aba2988..0a36a2acf15b 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -1032,96 +1032,109 @@ zl3073x_dpll_output_pin_frequency_get(const struct dpll_pin *dpll_pin,

[ ... ]

> +zl3073x_dpll_output_pin_freq_set(struct zl3073x_dpll_pin *pin,
> +				 struct zl3073x_out *out, u64 frequency)
>  {

[ ... ]

> +	synth = zl3073x_out_synth_get(out);
> +	synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
>  	new_div = synth_freq / (u32)frequency;

[Severity: High]
This isn't a bug introduced by this patch, but can frequency be 0 here?
The old zl3073x_dpll_output_pin_frequency_set() had the same division.

zl3073x_pin_props_get() always puts the current frequency in slot 0 of
the supported list:

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

curr_freq comes from zl3073x_dev_output_pin_freq_get(), which returns 0
for an output running below 1 Hz. Two examples:

  - an N-div N-pin with synth 1 GHz, div 10 and esync_n_period 2e8
  - a P-pin with div > synth_freq

zl3073x_out_state_fetch() only rejects a zero div and a zero
esync_n_period, so both setups are accepted.

Suppose the pin is first moved to another advertised frequency (for
example 1 Hz, or through a rescale caused by the sibling P-pin). A later
DPLL_CMD_PIN_SET with frequency 0 then passes
dpll_pin_is_freq_supported() through the {0, 0} range.

It also skips the "freq == old_freq" shortcut in dpll_pin_freq_set(),
because old_freq is no longer 0:

dpll_nl_pin_set_doit()
  dpll_pin_freq_set()
    zl3073x_dpll_output_pin_frequency_set()
      zl3073x_dpll_output_pin_freq_set()
        new_div = synth_freq / (u32)frequency;

On x86 this looks like a divide error oops, raised while zldpll->lock
and the dpll core lock are held.

Would it make sense to return -EINVAL from the helper for !frequency
(and for new_div == 0)? That would also match the new kernel-doc.
Another option is for zl3073x_pin_props_get() to skip a zero curr_freq.

[Severity: Low]
This is a pre-existing issue, but what happens here if the synth reports
a frequency of 0?

zl3073x_synth_state_fetch() only validates freq_n, so a synth_freq of 0
read from the device gives new_div == 0. In that case div = 0 is
committed, and zl3073x_dev_output_pin_freq_get() later divides by
out->div. This case looks speculative, and the netlink path could
already reach it before this patch.

The upcoming PTP caller does not seem to add a new path into this
division. zl3073x_dpll_perout_enable(), added later in the series by
"dpll: zl3073x: add PTP periodic output support", rejects everything
except a 1 second period and passes a constant:

    rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);

So the only remaining gap between the helper's "-EINVAL if the
frequency cannot be represented" contract and its behaviour is the
frequency 0 case above and this synth_freq 0 case.

[ ... ]

>  	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
> -		 * to compute N-pin divisor to keep its frequency unchanged.
> -		 *
> -		 * Update divisor for N-pin to keep N-pin frequency.
> +		/* Changing the P-pin frequency, rescale the N-pin divisor to
> +		 * keep the N-pin frequency unchanged. Fail if the requested
> +		 * frequency is too low to represent the current N-pin one.
>  		 */
> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> -		if (!out.esync_n_period) {
> -			rc = -EINVAL;
> -			goto unlock;
> -		}
> +		out->esync_n_period = out->esync_n_period * out->div / new_div;
> +		if (!out->esync_n_period)
> +			return -EINVAL;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this rescale really
keep the N-pin frequency unchanged? The division truncates, and only a
zero result is rejected.

For example, synth 1 GHz, div 100 and esync_n_period 2 give P = 10 MHz
and N = 5 MHz. Setting P to 8 MHz passes zl3073x_pin_check_freq(). That
gives new_div = 125 and esync_n_period = 200 / 125 = 1, so N becomes
8 MHz while the helper returns 0.

zl3073x_dpll_output_pin_frequency_set() only looks up the sibling for
non N-divided formats:

    if (!zl3073x_out_is_ndiv(&out))
        sibling = zl3073x_dpll_output_pin_sibling_get(pin);

So no change notification is sent for the N-pin even though its
frequency changed.

The old code had the same (out.esync_n_period * out.div) / new_div
rescale. The earlier commit "dpll: zl3073x: notify sibling pin when
shared output config changes" limited the notification to non N-divided
formats.

Could the helper return -EINVAL when the rescale is not exact?

[Severity: Medium]
This is also a pre-existing issue, but can out->esync_n_period *
out->div overflow here? Both are u32, so the product is computed in 32
bits before the division by new_div.

If div * period exceeds 2^32 (an N-pin below about 0.23 Hz on a 1 GHz
synth), the product wraps:

  - div 8, period 536870912 and a new P of 62.5 MHz (new_div 16): the
    product wraps to exactly 0, so a representable setup is rejected
    with -EINVAL.
  - period 600000000: the wrapped product is 505032704, which gives
    period 31564544. The N-pin then runs at about 1.98 Hz instead of
    about 0.208 Hz.

Would something like div_u64((u64)out->esync_n_period * out->div,
new_div), with a range check on the result, be safer?

[ ... ]

>  	} else {
> -		/* We are going to change frequency of N-pin but if
> -		 * the requested freq is greater or equal than freq of P-pin
> -		 * in the output pair we cannot compute divisor for the N-pin.
> -		 * In this case indicate a failure.
> -		 *
> -		 * Update divisor for N-pin
> +		/* Changing the N-pin frequency. Fail if the requested
> +		 * frequency is higher than or does not divide the P-pin one.
>  		 */
> -		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> -		if (!out.esync_n_period) {
> -			rc = -EINVAL;
> -			goto unlock;
> -		}
> +		out->esync_n_period = div64_u64(synth_freq,
> +						frequency * out->div);
> +		if (!out->esync_n_period)
> +			return -EINVAL;
>  	}

[Severity: Low]
Is the new comment accurate? It says the helper fails when the requested
frequency "does not divide the P-pin one". However, div64_u64() truncates
and only a zero quotient is rejected.

The new kernel-doc makes a similar promise:

    Return: 0 on success, -EINVAL if the frequency cannot be represented

For example, take synth 1 GHz and div 100 (P = 10 MHz):

  - A request for N = 4 MHz passes zl3073x_pin_check_freq(), because
    1e9 % 4e6 == 0.
  - esync_n_period becomes 1e9 / 4e8 = 2 (2.5 truncated), and the
    helper returns 0.
  - zl3073x_dev_output_pin_freq_get() then reports N = 5 MHz.

The non N-divided branch also returns 0 without checking new_div.

The truncation itself was already there. What this patch adds is the
comment and kernel-doc that don't match it.

Should there be a remainder check here, such as returning -EINVAL when
synth_freq % (frequency * out->div) is nonzero? Or should the comment
and kernel-doc be reworded to match the current behaviour?

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

Re: [PATCH net-next 6/6] dpll: zl3073x: add PTP periodic output support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 3

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 zl3073x_dpll_ptp_register()
  (drivers/dpll/zl3073x/dpll.c:3045-3054), any perout-capable output pin…
- [Medium] zl3073x_dpll_perout_enable()
  (drivers/dpll/zl3073x/dpll.c:2887-2904) checks only perout->period ==…
- [Medium] For single-ended outputs that are not N-divided, both pins of
  the pair (OUTxP/OUTxN) share the div and width registers.
- [Medium] zl3073x_dpll_ptp_enable() passes zldpll->ptp_clock to
  ptp_find_pin() (dpll.c:2959).
- [Low] zl3073x_dpll_perout_enable() writes the new 1 Hz divider to
  hardware first (zl3073x_out_state_set() at dpll.c:2897, which also…
- [Low] zl3073x_dpll_ptp_register() reads the zldev->out[] cache through
  zl3073x_dev_output_pin_state_get() (dpll.c:3049) without zldpll->lock,…
- [Low] On a non-N-divided output, zl3073x_dpll_perout_enable() relies on
  zl3073x_dpll_output_pin_freq_set(), which only rewrites div and width…

--

dpll: zl3073x: add PTP periodic output support

This commit registers a PTP periodic output pin for each zl3073x DPLL
output pin that supports step-time and declares 1 Hz support in firmware.
Enabling a perout channel programs the assigned pin for 1 PPS and
connects it. Disabling the channel disconnects the pin.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 0a36a2acf15b..f406d1e72530 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -2823,6 +2840,159 @@ zl3073x_dpll_ptp_getmaxphase(struct ptp_clock_info *info __always_unused)

[ ... ]

> +static int
> +zl3073x_dpll_perout_enable(struct zl3073x_dpll_pin *pin,
> +			   struct ptp_perout_request *perout)
> +{
> +	u8 out_id = zl3073x_output_pin_out_get(pin->id);
> +	struct zl3073x_dev *zldev = pin->dpll->dev;
> +	struct zl3073x_out out;
> +	int rc;
> +
> +	/* Only 1 PPS (1 Hz) periodic output is supported */
> +	if (perout->period.sec != 1 || perout->period.nsec)
> +		return -EINVAL;

[Severity: Medium]
Should perout->start be checked here too?

supported_perout_flags is left at 0, so ptp_perout_request() rejects
PTP_PEROUT_PHASE. That means every request reaching this driver uses the
absolute start time semantics from include/uapi/linux/ptp_clock.h:

	/*
	 * Absolute start time.
	 * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
	 */
	struct ptp_clock_time start;

start is never read. The divider is programmed, the pin is connected
right away, and success is returned. A request with a future start time
or a non-zero start.nsec is accepted, but the edge is not aligned to it.
If the pin is already connected, the only hardware change is a divider
rewrite on the running output.

Could requests whose start time cannot be honoured be rejected? At a
minimum, could the commit message mention this limitation?

> +
> +	out = *zl3073x_out_state_get(zldev, out_id);
> +
> +	rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);

[Severity: Medium]
On single-ended outputs that are not N-divided, both pins of the pair
share div and width. zl3073x_dpll_output_pin_freq_set() just overwrites
them in that case:

	if (!zl3073x_out_is_ndiv(out)) {
		...
		out->div = new_div;
		...
		out->width = new_div;

		return 0;
	}

OUTxP and OUTxN can both be advertised as independent PTP perout pins.
Enabling a channel on OUT5P would therefore also retune OUT5N to 1 Hz,
even if OUT5N is connected and driving something like 10 MHz. The
sibling dpll_pin_change_ntf() only reports this after the fact.

Should this path refuse when the sibling is connected at a different
frequency?

zl3073x_dpll_perout_disable() also only disconnects the pin. It does not
restore the previous divider, so a sibling that is still connected stays
at 1 Hz after the PTP channel is turned off. Is that intended?

[Severity: Low]
On a non-N-divided output, the same helper leaves the output mode clock
type and esync_n_period/esync_n_width unchanged. If the output was in
ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC, does it stay in ESYNC mode after the
switch to 1 Hz? esync_n_period and esync_n_width would still be the
values computed for the old carrier.

Once out_freq is 1, zl3073x_dpll_output_pin_esync_get() hides this
state:

	if (!(pin->caps & ZL3073X_DPLL_PIN_CAP_ESYNC) || out_freq <= 1)
		return -EOPNOTSUPP;

Userspace can then no longer see or clear the ESYNC mode that is still
set in hardware.

Should the perout path force CLOCK_TYPE_NORMAL, or reject outputs that
are in ESYNC mode? The existing DPLL frequency_set path appears to have
the same gap.

> +	if (rc)
> +		return rc;
> +
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +	if (rc)
> +		return rc;
> +
> +	if (zl3073x_dev_output_pin_state_get(zldev, pin->id))
> +		return 0;
> +
> +	return zl3073x_dev_output_pin_state_set(zldev, pin->id, true);

[Severity: Low]
What happens if zl3073x_dev_output_pin_state_set() fails here? By then
the new 1 Hz divider has already been committed by the
zl3073x_out_state_set() call above, and the cache has been updated.

The connect sequence can fail early. For example, the first stop write
in zl3073x_dev_output_pin_state_set() returns immediately:

	zl3073x_out_stop(&out);
	rc = zl3073x_out_state_set(zldev, out_id, &out);
	if (rc)
		return rc;

The old div/width and esync_n_period/width are not restored. The output
is left at 1 Hz and disconnected, and so is the sibling for non-N-div
formats.

zl3073x_dpll_ptp_enable() then returns before either notification:

	if (rc)
		return rc;

zl3073x_dpll_changes_check() only sends notifications for input pins.
Would DPLL userspace ever learn that the frequency of this pin or its
sibling changed?

> +}

[ ... ]

> +static int zl3073x_dpll_ptp_enable(struct ptp_clock_info *info,
> +				   struct ptp_clock_request *rq, int on)
> +{

[ ... ]

> +	pin_idx = ptp_find_pin(zldpll->ptp_clock, PTP_PF_PEROUT,
> +			       rq->perout.index);

[Severity: Medium]
Can zldpll->ptp_clock still be NULL here? zl3073x_dpll_ptp_register()
only assigns it after ptp_clock_register() returns:

	ptp_clock = ptp_clock_register(&zldpll->ptp_info, zldev->dev);
	...
	zldpll->ptp_clock = ptp_clock;

Inside ptp_clock_register(), posix_clock_register() has already made
/dev/ptpN and the sysfs pin and period attributes live. Any of these,
issued in that window, would reach this call with a NULL clock:

  - PTP_PEROUT_REQUEST
  - PTP_PIN_SETFUNC, via ptp_set_pinfunc()->ptp_disable_pinfunc()->
    enable(on=0), which is reachable because pins can be seeded as
    PEROUT at probe
  - a write to the sysfs period attribute

ptp_find_pin() dereferences ptp->info->n_pins without a NULL check.

The callback already receives info. Could it scan info->pin_config
directly rather than depend on the pointer that is published later?

> +	if (pin_idx < 0)
> +		return -EINVAL;

[ ... ]

> @@ -2843,16 +3015,53 @@ static const struct ptp_clock_info zl3073x_dpll_ptp_clock_info = {
>  static int zl3073x_dpll_ptp_register(struct zl3073x_dpll *zldpll)
>  {

[ ... ]

> +	i = 0;
> +	for_each_set_bit(id, zldpll->perout_map, ZL3073X_NUM_OUTPUT_PINS) {
> +		pin = zl3073x_dpll_output_pin_get_by_id(zldpll, id);
> +		strscpy(pin_config[i].name, pin->label);
> +		pin_config[i].index = i;
> +		if (zl3073x_dev_output_pin_state_get(zldev, id)) {
> +			pin_config[i].func = PTP_PF_PEROUT;
> +			pin_config[i].chan = i;
> +		}

[Severity: Medium]
Should this also check that the output runs at 1 Hz? Any perout-capable
pin that is connected at probe is reported as holding PEROUT channel i.
That includes a pin that firmware or DPLL netlink set up as, say, a
10 MHz clock.

Normal PTP pin management would then turn that output off. Each of
these triggers it:

  - PTP_PIN_SETFUNC moving channel i to another pin
  - setting the pin to PTP_PF_NONE
  - PTP_PEROUT_REQUEST with period 0 on channel i

They all end up in:

ptp_set_pinfunc() or ptp_perout_request()
  zl3073x_dpll_ptp_enable(on = 0)
    zl3073x_dpll_perout_disable()
      zl3073x_dev_output_pin_state_set(zldev, pin->id, false)

This disconnects an output that was never enabled through PTP. The
commit message does not mention that existing outputs are adopted this
way.

Would it be better to seed only pins that are connected and running at
1 Hz as PTP_PF_PEROUT, and leave the rest as PTP_PF_NONE?

[Severity: Low]
This reads the zldev->out[] cache through
zl3073x_dev_output_pin_state_get() without holding zldpll->lock.

zl3073x_dpll_register() has already registered the DPLL device and pins
before zl3073x_dpll_ptp_register() runs. DPLL netlink ops such as
zl3073x_dpll_output_pin_state_on_dpll_set() can therefore update the
same cache concurrently under zldpll->lock.

zl3073x_dev_output_pin_state_set() also briefly stops the whole output
pair around an fsleep(). An unlocked read in that window could see a
connected pin as stopped and seed it as PTP_PF_NONE.

Would holding zldpll->lock around this seeding loop fix it?

> +		i++;
> +	}

[ ... ]

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

Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

On 9/30/26 8:57 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 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).
> 
> - [High] The patch adds zl3073x_dpll_output_pin_sibling_get()
>    (drivers/dpll/zl3073x/dpll.c:146).
> - [Medium] The commit says it closes the case where the sibling pin's
>    frequency changes without a notification.
> - [Low] The commit message presents the missing sibling notification as a
>    user-visible defect: userspace listening on the sibling pin "is never…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> This patch adds zl3073x_dpll_output_pin_sibling_get(), which finds the
> other pin of a P/N output pair. The esync, frequency and phase adjust
> setters use it to send a pin change notification for the sibling after
> the shared output registers are written, and they now use explicit
> mutex_lock()/mutex_unlock() so the notification goes out after
> zldpll->lock is dropped.
> 
>> 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.
> 
> [Severity: Low]
> This describes a user-visible defect, but the trailers only have
> Tested-by: and Signed-off-by:.
> 
> The dpll core only calls __dpll_pin_change_ntf() for the pin in the
> request (for example in dpll_pin_freq_set()). The sibling has not been
> notified since the output setters were added.
> 
> Should this carry Fixes: tags such as these?
> 
> 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")

Agreed, this is a pre-existing user-visible defect. I will add the three
Fixes: tags in v2:

   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")

Jakub, a process question: this patch is both the fix and a prerequisite
for the rest of the series - it introduces
zl3073x_dpll_output_pin_sibling_get() and the notify-outside-lock
handling that the output enable/disable and PTP perout patches build on.
The defect itself is only a missing change notification (no crash, no
data corruption). Is it OK to keep it as patch 1 of this net-next series,
or would you prefer it split out and sent to net first?

>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index f2e137475b40..2c6de4dab8b4 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
>> @@ -132,6 +132,30 @@ 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;
>> +
>> +	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;
>> +}
>> +
> 
> [ ... ]
> 
>> @@ -934,27 +962,44 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
> 
> [ ... ]
> 
>> +	/* Commit output configuration */
>> +	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.
>>   	 */
>> -	out.esync_n_width = out.div / 2;
>> +	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
>>   
>> -	/* Commit output configuration */
>> -	return zl3073x_out_state_set(zldev, out_id, &out);
>> +	mutex_unlock(&zldpll->lock);
>> +
>> +	if (sibling)
>> +		__dpll_pin_change_ntf(sibling->dpll_pin);
> 
> [Severity: High]
> Can sibling->dpll_pin be NULL, or the sibling already freed, by the time
> it is dereferenced here?
> 
> zl3073x_dpll_output_pin_sibling_get() returns any output pin on
> zldpll->pins with the matching id. It does not check that
> sibling->dpll_pin is still set. The lookup runs under zldpll->lock, but
> the dereference happens after mutex_unlock().
> 
> The teardown path does not take zldpll->lock:
> 
> drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_pins_unregister() {
> 	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
> 		zl3073x_dpll_pin_unregister(pin);
> 		list_del(&pin->list);
> 		zl3073x_dpll_pin_free(pin);
> 	}
> }
> 
> zl3073x_dpll_pin_unregister() ends with:
> 
> 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
> 	pin->dpll_pin = NULL;
> 
> and dpll_pin_put() has already dropped dpll_lock at that point.
> 
> zl3073x_dpll_pins_register() uses list_add(), so the N pin (2k+1) sits
> before its P pin (2k) on the list. That means N is torn down while P is
> still registered and reachable over netlink.
> 
> One possible interleaving:
> 
> CPU1 (devlink reload, flash_prepare or unbind)
> zl3073x_dev_stop()
>    zl3073x_dpll_unregister()
>      zl3073x_dpll_pins_unregister()
>        dpll_pin_put(N)          /* dpll_lock released */
> 
> CPU0 (DPLL_CMD_PIN_SET on P, dpll_lock held)
> dpll_pin_freq_set()
>    zl3073x_dpll_output_pin_frequency_set(P)
>      mutex_lock(&zldpll->lock)
>      sibling = zl3073x_dpll_output_pin_sibling_get(P)  /* finds N */
>      mutex_unlock(&zldpll->lock)
> 
> CPU1
>        N->dpll_pin = NULL
>        list_del(&N->list)
>        zl3073x_dpll_pin_free(N)
> 
> CPU0
>      __dpll_pin_change_ntf(sibling->dpll_pin)
> 
> In that case, would __dpll_pin_change_ntf() get NULL and oops on
> pin->id and pin->clock_id in dpll_pin_notify()? Or would CPU0 read the
> freed zl3073x_dpll_pin?
> 
> The list walk in zl3073x_dpll_output_pin_sibling_get() can also run
> while list_del() is in progress, because the writer never takes
> zldpll->lock.
> 
> zl3073x_dpll_output_pin_frequency_set() and
> zl3073x_dpll_output_pin_phase_adjust_set() have the same pattern.
> 
> The later patch "dpll: zl3073x: add PTP periodic output support" also
> calls zl3073x_dpll_output_pin_sibling_get() from
> zl3073x_dpll_ptp_enable() without holding zldpll->lock at all.

This cannot happen; the notification is already serialized against pin
teardown.

The output setters (esync_set / frequency_set / phase_adjust_set) run
with dpll_lock held: the dpll core invokes the pin ops under dpll_lock,
and they use __dpll_pin_change_ntf(), whose contract is exactly "caller
must hold dpll_lock" (dpll_netlink.c: lockdep_assert_held(&dpll_lock),
"suitable for use inside pin callbacks which are already invoked under
dpll_lock"). So sibling_get() and the __dpll_pin_change_ntf() call - even
though it runs after mutex_unlock(&zldpll->lock) - are all covered by
dpll_lock.

Pin teardown frees the sibling via zl3073x_dpll_pin_unregister() ->
dpll_pin_unregister(), which takes dpll_lock. While a setter holds
dpll_lock for the whole callback, teardown cannot even reach
dpll_pin_unregister(N), let alone the following list_del()/kfree(). The
proposed interleaving (CPU1 freeing N while CPU0 notifies) is therefore
impossible: CPU0 holds dpll_lock throughout.

The PTP enable() path (zl3073x_dpll_ptp_enable(), added in patch 6) does
call sibling_get() outside dpll_lock, but it is serialized differently:
zl3073x_dpll_unregister() unregisters the PTP clock *before* the pins
(zl3073x_dpll_ptp_unregister() then zl3073x_dpll_pins_unregister()), and
ptp_clock_unregister() quiesces in-flight enable() callbacks. No
enable() can run while the pins are being freed.

> 
> [ ... ]
> 
>> @@ -1010,7 +1058,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>>   		out.width = new_div;
>>   
>>   		/* 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)) {
>> @@ -1022,8 +1079,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>>   		 * Update divisor for N-pin to keep N-pin frequency.
>>   		 */
>>   		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> 
> [Severity: Medium]
> Does the N pin also need a notification on this path?
> 
> In the N-div formats OUTPUT_DIV is still shared. The N-pin frequency is
> computed from both values:
> 
> drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
> 	...
> 	freq = zl3073x_synth_freq_get(synth) / out->div;
> 
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
> 	...
> }
> 
> The rescale above truncates and only rejects 0. zl3073x_pin_check_freq()
> only requires the new P frequency to divide the synth frequency.
> 
> For example, take a 500 MHz synth with P = 10 MHz (div 50) and
> N = 2 MHz (period 5). Setting P to 25 MHz gives new_div 20 and period
> 250 / 20 = 12. N then becomes 500M / (20 * 12), about 2.083 MHz.
> 
> This path reaches the unlock label with sibling still NULL, so the N
> pin gets no DPLL_CMD_PIN_CHANGE_NTF. The commit message says the sibling
> is notified "whenever the shared HW state actually changes". Should this
> case be covered too?
> 
> The later commit "dpll: zl3073x: factor out output pin frequency helper"
> does not change this. The truncation moves into
> zl3073x_dpll_output_pin_freq_set(), and frequency_set still calls
> zl3073x_dpll_output_pin_sibling_get() only when
> !zl3073x_out_is_ndiv(&out).

By design the N-pin frequency is meant to be preserved when the P-pin
frequency changes - that is the purpose of rescaling esync_n_period. With
an exact rescale, div_new * period_new == div_old * period_old, so
N = synth / (div * period) is unchanged and no sibling notification is
needed.

The only thing that can actually perturb N here is the truncation in that
rescale, which is the separate issue Sashiko raised on patch 5 ("does this
rescale really keep the N-pin frequency unchanged?"). I will fix it there
by rejecting an inexact rescale with -EINVAL (i.e. when div_old * period_old
is not divisible by div_new), so N is guaranteed to stay unchanged. With that
fix there is nothing to notify on this path, so no sibling notification is added
here.

Thanks,
Ivan

Re: [PATCH net-next 2/6] dpll: zl3073x: add GPO support for output pins

[Ivan Vecera]

On 9/30/26 8:57 AM, 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: 1 · Low: 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] Error-recovery path applies the output restart before the
>    pending GPO change.
> - [Low] The struct zl3073x_dev kernel-doc in drivers/dpll/zl3073x/core.h
>    (line 65, '@out: array of outs' invariants') and the inline '/*…
> 
> --
> 
> dpll: zl3073x: add GPO support for output pins
> 
> This commit adds the GPO override mailbox fields and the direct gpo_out_x
> registers for CMOS output pins, along with helpers to read and set the pin
> function. It also adds the output_ctrl_x stop bits and stop/start helpers.
> ctrl moves into the cfg struct_group, and zl3073x_out_state_set() now
> writes it directly.
> 
>> diff --git a/drivers/dpll/zl3073x/out.c b/drivers/dpll/zl3073x/out.c
>> index 410d15b96d0b..82564045890d 100644
>> --- a/drivers/dpll/zl3073x/out.c
>> +++ b/drivers/dpll/zl3073x/out.c
> 
> [ ... ]
> 
>> @@ -120,11 +135,17 @@ int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
>>   	struct zl3073x_out *dout = &zldev->out[index];
>>   	int rc;
>>   
>> -	/* Reject attempts to change invariant fields (set at fetch only) */
>> -	if (WARN_ON(memcmp(&dout->inv, &out->inv, sizeof(out->inv))))
>> -		return -EINVAL;
>> +	/* ctrl is a direct register, independent of the output mailbox */
>> +	if (dout->ctrl != out->ctrl) {
>> +		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
>> +				      out->ctrl);
>> +		if (rc)
>> +			return rc;
>> +
>> +		dout->ctrl = out->ctrl;
>> +	}
> 
> [Severity: Medium]
> 
> zl3073x_out_state_set() now always writes output_ctrl_x before any mailbox
> field. It does not roll dout->ctrl back if the later zl3073x_mb_op() or a
> mailbox field write fails.
> 
> Can this order apply a restart before a pending GPO change on an error
> recovery path?
> 
> The follow-up patch "dpll: zl3073x: allow enabling/disabling output pins"
> adds zl3073x_dev_output_pin_state_set(). It puts a stop before the GPO
> change and a start after it. Its recovery path looks like this:
> 
> 	rc = zl3073x_out_state_set(zldev, out_id, &out);
> 
> restart_output:
> 	zl3073x_out_start(&out);
> 	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;
> 
> The second zl3073x_out_state_set() call only changes gpo_en/gpo_config.
> Suppose it fails in zl3073x_mb_op() or in one of the mailbox field writes.
> Then dout->cfg keeps the old GPO values, while out still holds the new
> ones.
> 
> The recovery call then carries two changes: the cleared STOP/STOP_HZ bits
> and the uncommitted GPO change. Because ctrl is written first, would the
> clock restart, with the GPO override then changed on a running output?
> 
> That looks like the toggle the stop/start pair is meant to avoid. The
> driver's own comment says the GPO toggle is not glitch-free. For
> enable=true, the override is released onto a clock that is already
> running, at an arbitrary phase. For enable=false, the override is engaged
> in the middle of a clock cycle.
> 
> Also, if the retried commit succeeds, the "? : rc" still returns the
> earlier error. zl3073x_dpll_output_pin_state_on_dpll_set() would then
> report a failure even though the pin state changed.

Good catch. I will reorder zl3073x_out_state_set() to commit the mailbox
fields first and write the direct output_ctrl_x register last.

That fixes the recovery path in zl3073x_dev_output_pin_state_set():

   - stop phase: only ctrl changes (STOP/STOP_HZ), there is no cfg diff, so
     the mailbox commit is skipped and only ctrl is written - unchanged.
   - restart phase: state_set() will write the GPO change through the
     mailbox *while the output is still stopped* (ctrl not written yet),
     and only then clears STOP via ctrl - a clean restart, no glitch on a
     running output.
   - on a mailbox failure ctrl is left untouched, so there is no premature
     restart and dout->ctrl stays consistent with the hardware.

The other callers (frequency_set / esync_set / phase_adjust_set) only
change cfg fields with ctrl unchanged, so the ordering does not affect
them.

> 
>>   
>> -	/* Skip HW write if configuration hasn't changed */
>> +	/* Skip the mailbox commit if nothing else has changed */
>>   	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
>>   		return 0;
> 
> [ ... ]
> 
>> diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
>> index 660889c57bff..66a8432c00df 100644
>> --- a/drivers/dpll/zl3073x/out.h
>> +++ b/drivers/dpll/zl3073x/out.h
> 
> [ ... ]
> 
>> @@ -29,8 +32,9 @@ struct zl3073x_out {
>>   		u32	esync_n_width;
>>   		s32	phase_comp;
>>   		u8	mode;
>> -	);
>> -	struct_group(inv, /* Invariants */
>> +		u8	gpo_en;
>> +		u8	gpo_config_p;
>> +		u8	gpo_config_n;
>>   		u8	ctrl;
>>   	);
>>   };
> 
> [Severity: Low]
> 
> This isn't a bug, but with the inv group removed, struct zl3073x_out has
> no invariant fields left. The struct zl3073x_dev comments in core.h still
> describe out[] as invariants:
> 
>   * @out: array of outs' invariants
> ...
> 	/* Invariants */
> 	struct zl3073x_ref	ref[ZL3073X_NUM_REFS];
> 	struct zl3073x_out	out[ZL3073X_NUM_OUTS];
> 
> These comments are still unchanged at the end of the series. Should they
> be updated along with this change?

Yes, I will fix them in v2.

Thanks,
Ivan

pw-bot: cr

Re: [PATCH net-next 3/6] dpll: zl3073x: allow enabling/disabling output pins

[Ivan Vecera]

On 9/30/26 8:57 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 4 · 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] On CMOS outputs, the output-wide STOP bit is mixed up with the
>    per-pin GPO state.
> - [Medium] CMOS output pins that firmware or board configuration set up
>    as GPO status, IRQ or unknown-mode outputs…
> - [Medium] The error path of zl3073x_dev_output_pin_state_set() does not
>    close the stop/restart bracket symmetrically, for either CMOS pin.
> - [Medium] The settle delay before the non-glitch-free GPO toggle in
>    zl3073x_dev_output_pin_state_set() may be too short.
> 
> --
> 
> dpll: zl3073x: allow enabling/disabling output pins
> 
> This commit adds state_on_dpll_set support for zl3073x output pins.
> Differential pins are toggled through the output_ctrl_x::stop bit. CMOS
> pins are muted/unmuted through a GPO override, bracketed by a stop/restart
> of the whole output. Output pins now advertise
> DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE.
> 
>> diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
>> index 7386932df032..89cf46111dcb 100644
>> --- a/drivers/dpll/zl3073x/core.c
>> +++ b/drivers/dpll/zl3073x/core.c
> 
> [ ... ]
> 
>> @@ -667,6 +669,82 @@ int zl3073x_dev_gpo_set(struct zl3073x_dev *zldev, u8 gpo, bool value)
> 
> [ ... ]
> 
>> +int zl3073x_dev_output_pin_state_set(struct zl3073x_dev *zldev, u8 id,
>> +				     bool enable)
>> +{
> 
> [ ... ]
> 
>> +	/* Bracket the GPO override toggle below with a clean stop/restart,
>> +	 * since the toggle itself is not glitch-free.
>> +	 */
>> +	zl3073x_out_stop(&out);
>> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* output_ctrl_x::stop is edge-aligned, so the device can take up
>> +	 * to half a period to actually reach the stopped state. Wait for
>> +	 * that long plus 25 ms, to make sure it is really stopped before
>> +	 * touching the GPO override below.
>> +	 */
>> +	delay = 25 * USEC_PER_MSEC;
>> +	freq = zl3073x_dev_output_pin_freq_get(zldev, id);
>> +	if (freq)
>> +		delay += USEC_PER_SEC / 2 / freq;
>> +	fsleep(delay);
> 
> [Severity: Medium]
> Is this delay always long enough? zl3073x_dev_output_pin_freq_get() returns
> an integer Hz value:
> 
> drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
>      ...
> 	freq = zl3073x_synth_freq_get(synth) / out->div;
> 
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
>      ...
> }
> 
> Sub-Hz rates truncate to 0. Take an N-div N pin with P at 1 Hz and
> esync_n_period = 2, which runs at 0.5 Hz. freq becomes 0, so the half-period
> term is skipped. The code then waits only 25 ms, while the real half period
> is about 1 s.
> 
> Also, only the requested pin's frequency is used. On an N-div output, the
> P pin's delay comes from the fast P rate. If the device aligns the stop of
> the whole output to the slower N edge, could the P pin still be running
> when its GPO override is toggled? This second case depends on how the
> hardware aligns the stop, so it is less certain.
> 
> In either case, can the GPO mux switch while the clock is still active and
> produce the runt pulse that the stop/restart bracket is meant to prevent?
> This code is still the same at the end of the series.

Two separate points here.

1) Sub-Hz truncation to 0: this cannot happen for a pin managed here. The
    DPLL core does not support sub-Hz frequencies, so an output pin's
    frequency is always >= 1 Hz and zl3073x_dev_output_pin_freq_get() never
    returns 0 for the half-period term.

2) Using only the requested pin's frequency: good point. The stop applies
    to the whole output, which settles on its slowest edge, so basing the
    wait on a fast P pin can be too short on an N-div output. I will compute
    the delay from the lower frequency of the two pins in the pair:

	freq = min_not_zero(zl3073x_dev_output_pin_freq_get(zldev, id),
                             zl3073x_dev_output_pin_freq_get(zldev, id ^ 1));

>> +
>> +	if (enable) {
>> +		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_CLOCK);
> 
> [Severity: Medium]
> What happens here to a CMOS pin that firmware or the board set up as a GPO
> status, IRQ or unknown-mode output (ZL3073X_OUT_PIN_F_GPO_STATUS,
> ZL3073X_OUT_PIN_F_GPO_IRQ or ZL3073X_OUT_PIN_F_GPO_UNKNOWN)?
> 
> zl3073x_dpll_pin_is_registrable() only checks synth/output enablement and
> signal format, so these pins are still registered.
> 
> Before this patch, every registered output pin reported connected. Now
> zl3073x_dev_output_pin_state_get() reports these pins as disconnected,
> because their function is not CLOCK:
> 
> 	return zl3073x_out_is_diff(out) ||
> 	       zl3073x_out_pin_func_get(out, id) == ZL3073X_OUT_PIN_F_CLOCK;
> 
> zl3073x_pin_props_get() also now sets DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE
> on every output pin.
> 
> A connect request then reaches this zl3073x_out_pin_func_set() call. It
> clears gpo_en and throws away the status/IRQ function. A clock is then
> driven onto a line the board may have wired as a lock-status or interrupt
> signal.
> 
> A later disconnect writes GPO_CONST low, so the DPLL API can never restore
> the original function.
> 
> Should pins with a GPO status/IRQ function be excluded from state changes,
> or at least not advertise STATE_CAN_CHANGE? Neither the commit message nor
> the rest of the series covers these pins.

Agreed - such pins should not be registered as clock outputs at all. In v2
I will skip a single-ended output pin unless it is a manageable clock output,
i.e. its function is either CLOCK, or GPO_CONST driving 0 (the driver's own
disconnected state). A pin in GPO_CONST driving 1, or in the status/IRQ/unknown
function, is reserved by firmware/board and is skipped, so it is never
registered.

>> +	} else {
>> +		rc = zl3073x_dev_gpo_set(zldev, id, false);
>> +		if (rc)
>> +			goto restart_output;
>> +		zl3073x_out_pin_func_set(&out, id, ZL3073X_OUT_PIN_F_GPO_CONST);
>> +	}
>> +
>> +	/* Restart the output regardless of the result below: on failure,
>> +	 * don't leave the whole output, including the unrelated sibling
>> +	 * pin, stopped indefinitely.
>> +	 */
>> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
>> +
>> +restart_output:
>> +	zl3073x_out_start(&out);
>> +	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;
> 
> [Severity: Medium]
> What does this restart do if the zl3073x_out_state_set() call just above
> fails?
> 
> zl3073x_out_state_set() writes ctrl first. It stores dout->cfg only after
> a successful mailbox commit:
> 
> drivers/dpll/zl3073x/out.c:zl3073x_out_state_set() {
>      ...
> 	if (dout->ctrl != out->ctrl) {
> 		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
> 				      out->ctrl);
> 		...
> 		dout->ctrl = out->ctrl;
> 	}
> 
> 	/* Skip the mailbox commit if nothing else has changed */
> 	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
> 		return 0;
>      ...
> 	/* After successful commit store new state */
> 	dout->cfg = out->cfg;
>      ...
> }
> 
> After a failed GPO commit, the local out holds the new gpo_en/gpo_config
> values and the cache still holds the old ones.
> 
> The restart call writes ctrl first, which clears STOP and starts the output
> again. It then sees the cfg difference and retries the same GPO mailbox
> commit while the output is running. Isn't that the non-glitch-free toggle
> the bracket is supposed to prevent?
> 
> If the retry succeeds, the restart call returns 0 and "? : rc" keeps the
> earlier error.
> 
> zl3073x_dpll_output_pin_state_on_dpll_set() then reports "Failed to change
> output pin state". dpll_pin_state_set() also skips __dpll_pin_change_ntf(),
> even though the hardware and zldev->out[] now hold the new pin state.

Two parts.

The glitch will be fixed by the write-ordering change in patch 2:
zl3073x_out_state_set() will commit the mailbox fields (the GPO change)
before the direct output_ctrl_x register, so on a restart the GPO write
lands while the output is still stopped and STOP is cleared last - no
toggle on a running output.

For the "? : rc" masking, next version will fold the separate "apply GPO while
stopped" and "restart" writes into a single zl3073x_out_state_set(). With the
mailbox-before-ctrl ordering that one commit applies the GPO change and
clears STOP in the right order.

>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 2c6de4dab8b4..426974b0b5dc 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> +static int
>> +zl3073x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *dpll_pin,
> 
> [ ... ]
> 
>> +	guard(mutex)(&zldpll->lock);
>> +
>> +	enable = state == DPLL_PIN_STATE_CONNECTED;
>> +	if (zl3073x_dev_output_pin_state_get(zldev, pin->id) != enable) {
>> +		rc = zl3073x_dev_output_pin_state_set(zldev, pin->id, enable);
> 
> [Severity: Medium]
> Can a disconnect request on a CMOS pin be silently dropped here?
> zl3073x_dev_output_pin_state_get() returns false for both P and N whenever
> the output-wide STOP bit is set, whatever each pin's GPO function is:
> 
> 	if (zl3073x_out_is_stopped(out))
> 		return false;
> 
> A CMOS output can be stopped in two ways:
> 
> - At probe. zl3073x_dpll_pin_is_registrable() does not reject stopped
>    outputs, so an output that firmware left stopped is still registered.
> - After the final restart write in zl3073x_dev_output_pin_state_set()
>    fails.
> 
> Take a stopped CMOS output where N is still in CLOCK function:
> 
>    disconnect N
>      zl3073x_dev_output_pin_state_get(N) returns false, same as enable
>      -> returns 0, N stays in CLOCK function
> 
>    connect P
>      zl3073x_dev_output_pin_state_set(P, true)
>        zl3073x_out_start(&out)    <- restarts the whole output
> 
> Wouldn't N then drive its clock again, even though userspace was told N is
> disconnected?
> 
> Changing one pin can also change the sibling's state:
> 
> - Restarting a stopped output moves the sibling from disconnected to
>    connected.
> - A failed final restart moves the sibling from connected to disconnected.
> 
> dpll_pin_state_set() only calls __dpll_pin_change_ntf() for the pin that
> was requested.
> 
> Should this follow the pattern that the earlier patch in this series, "dpll:
> zl3073x: notify sibling pin when shared output config changes", added to
> zl3073x_dpll_output_pin_phase_adjust_set()?
> 
> 	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> 
> 	mutex_unlock(&zldpll->lock);
> 
> 	if (sibling)
> 		__dpll_pin_change_ntf(sibling->dpll_pin);
> 
> This is still present at the end of the series. The PTP perout
> enable/disable helpers added in "dpll: zl3073x: add PTP periodic output
> support" call the same zl3073x_dev_output_pin_state_get() and
> zl3073x_dev_output_pin_state_set() pair.

Both symptoms come from a single-ended output being stopped in steady
state. For CMOS the whole-output STOP is only a transient mechanism inside
the connect/disconnect bracket (stop -> toggle GPO -> restart, always
restored); in steady state a CMOS output runs and the per-pin GPO function
alone determines connection. So the only way state_get() returns false for
a CLOCK-function CMOS pin is an output that firmware left stopped at probe.

In that stopped case connecting one pin restarts the whole output and
revives the sibling too, which is exactly what breaks here. In v2
I will therefore skip a single-ended output pin whose output is stopped.
Such an output cannot be managed per-pin. (Differential outputs keep stopped
as their valid disconnected state and stay registrable.)

Thanks,
Ivan

pw-bot: cr

Re: [PATCH net-next 5/6] dpll: zl3073x: factor out output pin frequency helper

[Ivan Vecera]

On 9/30/26 8:57 AM, 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 kernel-doc of the new helper
>    zl3073x_dpll_output_pin_freq_set() in drivers/dpll/zl3073x/dpll.c says…
> 
> Pre-existing issues:
> - [High] A user with CAP_NET_ADMIN can trigger a divide-by-zero oops in
>    zl3073x_dpll_output_pin_freq_set() (drivers/dpll/zl3073x/dpll.c:1058,…
> - [Medium] (Baseline: preexisting, medium; not reported as a finding of
>    this patch.) In the N-div P-pin branch of…
> - [Medium] (Baseline: preexisting, medium; not reported as a finding of
>    this patch.) In zl3073x_dpll_output_pin_freq_set(), the P-pin rescale…
> - [Low] (Baseline: not confirmed as a new issue.) The concern says
>    zl3073x_dpll_output_pin_freq_set() does not validate its input, which…
> 
> --
> 
> dpll: zl3073x: factor out output pin frequency helper
> 
> This commit moves the divisor and N-division computation out of
> zl3073x_dpll_output_pin_frequency_set() into a new helper,
> zl3073x_dpll_output_pin_freq_set(). The helper updates a struct
> zl3073x_out but does not write it to hardware, so the upcoming PTP
> periodic output code can reuse it. No functional change is intended.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index fcf91aba2988..0a36a2acf15b 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
>> @@ -1032,96 +1032,109 @@ zl3073x_dpll_output_pin_frequency_get(const struct dpll_pin *dpll_pin,
> 
> [ ... ]
> 
>> +zl3073x_dpll_output_pin_freq_set(struct zl3073x_dpll_pin *pin,
>> +				 struct zl3073x_out *out, u64 frequency)
>>   {
> 
> [ ... ]
> 
>> +	synth = zl3073x_out_synth_get(out);
>> +	synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
>>   	new_div = synth_freq / (u32)frequency;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can frequency be 0 here?
> The old zl3073x_dpll_output_pin_frequency_set() had the same division.
> 
> zl3073x_pin_props_get() always puts the current frequency in slot 0 of
> the supported list:
> 
>      ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> 
> curr_freq comes from zl3073x_dev_output_pin_freq_get(), which returns 0
> for an output running below 1 Hz. Two examples:
> 
>    - an N-div N-pin with synth 1 GHz, div 10 and esync_n_period 2e8
>    - a P-pin with div > synth_freq
> 
> zl3073x_out_state_fetch() only rejects a zero div and a zero
> esync_n_period, so both setups are accepted.
> 
> Suppose the pin is first moved to another advertised frequency (for
> example 1 Hz, or through a rescale caused by the sibling P-pin). A later
> DPLL_CMD_PIN_SET with frequency 0 then passes
> dpll_pin_is_freq_supported() through the {0, 0} range.
> 
> It also skips the "freq == old_freq" shortcut in dpll_pin_freq_set(),
> because old_freq is no longer 0:
> 
> dpll_nl_pin_set_doit()
>    dpll_pin_freq_set()
>      zl3073x_dpll_output_pin_frequency_set()
>        zl3073x_dpll_output_pin_freq_set()
>          new_div = synth_freq / (u32)frequency;
> 
> On x86 this looks like a divide error oops, raised while zldpll->lock
> and the dpll core lock are held.
> 
> Would it make sense to return -EINVAL from the helper for !frequency
> (and for new_div == 0)? That would also match the new kernel-doc.
> Another option is for zl3073x_pin_props_get() to skip a zero curr_freq.
> 
> [Severity: Low]
> This is a pre-existing issue, but what happens here if the synth reports
> a frequency of 0?
> 
> zl3073x_synth_state_fetch() only validates freq_n, so a synth_freq of 0
> read from the device gives new_div == 0. In that case div = 0 is
> committed, and zl3073x_dev_output_pin_freq_get() later divides by
> out->div. This case looks speculative, and the netlink path could
> already reach it before this patch.
> 
> The upcoming PTP caller does not seem to add a new path into this
> division. zl3073x_dpll_perout_enable(), added later in the series by
> "dpll: zl3073x: add PTP periodic output support", rejects everything
> except a 1 second period and passes a constant:
> 
>      rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);
> 
> So the only remaining gap between the helper's "-EINVAL if the
> frequency cannot be represented" contract and its behaviour is the
> frequency 0 case above and this synth_freq 0 case.

I will guard both by:

	if (!frequency)
		return -EINVAL;
	...
	new_div = synth_freq / (u32)frequency;
	if (!new_div)
		return -EINVAL;

!new_div also covers a synth_freq of 0 (and any request above synth_freq).

> 
> [ ... ]
> 
>>   	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
>> -		 * to compute N-pin divisor to keep its frequency unchanged.
>> -		 *
>> -		 * Update divisor for N-pin to keep N-pin frequency.
>> +		/* Changing the P-pin frequency, rescale the N-pin divisor to
>> +		 * keep the N-pin frequency unchanged. Fail if the requested
>> +		 * frequency is too low to represent the current N-pin one.
>>   		 */
>> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
>> -		if (!out.esync_n_period) {
>> -			rc = -EINVAL;
>> -			goto unlock;
>> -		}
>> +		out->esync_n_period = out->esync_n_period * out->div / new_div;
>> +		if (!out->esync_n_period)
>> +			return -EINVAL;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does this rescale really
> keep the N-pin frequency unchanged? The division truncates, and only a
> zero result is rejected.
> 
> For example, synth 1 GHz, div 100 and esync_n_period 2 give P = 10 MHz
> and N = 5 MHz. Setting P to 8 MHz passes zl3073x_pin_check_freq(). That
> gives new_div = 125 and esync_n_period = 200 / 125 = 1, so N becomes
> 8 MHz while the helper returns 0.
> 
> zl3073x_dpll_output_pin_frequency_set() only looks up the sibling for
> non N-divided formats:
> 
>      if (!zl3073x_out_is_ndiv(&out))
>          sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> 
> So no change notification is sent for the N-pin even though its
> frequency changed.
> 
> The old code had the same (out.esync_n_period * out.div) / new_div
> rescale. The earlier commit "dpll: zl3073x: notify sibling pin when
> shared output config changes" limited the notification to non N-divided
> formats.
> 
> Could the helper return -EINVAL when the rescale is not exact?
> 
> [Severity: Medium]
> This is also a pre-existing issue, but can out->esync_n_period *
> out->div overflow here? Both are u32, so the product is computed in 32
> bits before the division by new_div.
> 
> If div * period exceeds 2^32 (an N-pin below about 0.23 Hz on a 1 GHz
> synth), the product wraps:
> 
>    - div 8, period 536870912 and a new P of 62.5 MHz (new_div 16): the
>      product wraps to exactly 0, so a representable setup is rejected
>      with -EINVAL.
>    - period 600000000: the wrapped product is 505032704, which gives
>      period 31564544. The N-pin then runs at about 1.98 Hz instead of
>      about 0.208 Hz.
> 
> Would something like div_u64((u64)out->esync_n_period * out->div,
> new_div), with a range check on the result, be safer?

I will fix together. The product will be computed in 64 bits and
the rescale is rejected unless it is exact:

	u64 product = (u64)out->esync_n_period * out->div;
	u32 rem;

	out->esync_n_period = div_u64_rem(product, new_div, &rem);
	if (rem || !out->esync_n_period)
		return -EINVAL;

So there is no u32 overflow, and an inexact rescale returns -EINVAL instead
of silently changing the N-pin frequency. That also settles the patch 1
question about notifying the N-pin on this path: with an exact rescale the
N-pin frequency is guaranteed unchanged, so no sibling notification is
needed.

> [ ... ]
> 
>>   	} else {
>> -		/* We are going to change frequency of N-pin but if
>> -		 * the requested freq is greater or equal than freq of P-pin
>> -		 * in the output pair we cannot compute divisor for the N-pin.
>> -		 * In this case indicate a failure.
>> -		 *
>> -		 * Update divisor for N-pin
>> +		/* Changing the N-pin frequency. Fail if the requested
>> +		 * frequency is higher than or does not divide the P-pin one.
>>   		 */
>> -		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
>> -		if (!out.esync_n_period) {
>> -			rc = -EINVAL;
>> -			goto unlock;
>> -		}
>> +		out->esync_n_period = div64_u64(synth_freq,
>> +						frequency * out->div);
>> +		if (!out->esync_n_period)
>> +			return -EINVAL;
>>   	}
> 
> [Severity: Low]
> Is the new comment accurate? It says the helper fails when the requested
> frequency "does not divide the P-pin one". However, div64_u64() truncates
> and only a zero quotient is rejected.
> 
> The new kernel-doc makes a similar promise:
> 
>      Return: 0 on success, -EINVAL if the frequency cannot be represented
> 
> For example, take synth 1 GHz and div 100 (P = 10 MHz):
> 
>    - A request for N = 4 MHz passes zl3073x_pin_check_freq(), because
>      1e9 % 4e6 == 0.
>    - esync_n_period becomes 1e9 / 4e8 = 2 (2.5 truncated), and the
>      helper returns 0.
>    - zl3073x_dev_output_pin_freq_get() then reports N = 5 MHz.
> 
> The non N-divided branch also returns 0 without checking new_div.
> 
> The truncation itself was already there. What this patch adds is the
> comment and kernel-doc that don't match it.
> 
> Should there be a remainder check here, such as returning -EINVAL when
> synth_freq % (frequency * out->div) is nonzero? Or should the comment
> and kernel-doc be reworded to match the current behaviour?

With above the comment matches the code:

	u64 rem, divisor = frequency * out->div;

	out->esync_n_period = div64_u64_rem(synth_freq, divisor, &rem);
	if (rem || !out->esync_n_period)
		return -EINVAL;

A requested N-pin frequency that does not divide the P-pin one exactly now
returns -EINVAL.

Thanks,
Ivan

pw-bot: cr

Re: [PATCH net-next 1/6] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

On 9/30/26 9:49 AM, Ivan Vecera wrote:
>> [Severity: High]
>> Can sibling->dpll_pin be NULL, or the sibling already freed, by the time
>> it is dereferenced here?
>>
>> zl3073x_dpll_output_pin_sibling_get() returns any output pin on
>> zldpll->pins with the matching id. It does not check that
>> sibling->dpll_pin is still set. The lookup runs under zldpll->lock, but
>> the dereference happens after mutex_unlock().
>>
>> The teardown path does not take zldpll->lock:
>>
>> drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_pins_unregister() {
>>     list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
>>         zl3073x_dpll_pin_unregister(pin);
>>         list_del(&pin->list);
>>         zl3073x_dpll_pin_free(pin);
>>     }
>> }
>>
>> zl3073x_dpll_pin_unregister() ends with:
>>
>>     dpll_pin_put(pin->dpll_pin, &pin->tracker);
>>     pin->dpll_pin = NULL;
>>
>> and dpll_pin_put() has already dropped dpll_lock at that point.
>>
>> zl3073x_dpll_pins_register() uses list_add(), so the N pin (2k+1) sits
>> before its P pin (2k) on the list. That means N is torn down while P is
>> still registered and reachable over netlink.
>>
>> One possible interleaving:
>>
>> CPU1 (devlink reload, flash_prepare or unbind)
>> zl3073x_dev_stop()
>>    zl3073x_dpll_unregister()
>>      zl3073x_dpll_pins_unregister()
>>        dpll_pin_put(N)          /* dpll_lock released */
>>
>> CPU0 (DPLL_CMD_PIN_SET on P, dpll_lock held)
>> dpll_pin_freq_set()
>>    zl3073x_dpll_output_pin_frequency_set(P)
>>      mutex_lock(&zldpll->lock)
>>      sibling = zl3073x_dpll_output_pin_sibling_get(P)  /* finds N */
>>      mutex_unlock(&zldpll->lock)
>>
>> CPU1
>>        N->dpll_pin = NULL
>>        list_del(&N->list)
>>        zl3073x_dpll_pin_free(N)
>>
>> CPU0
>>      __dpll_pin_change_ntf(sibling->dpll_pin)
>>
>> In that case, would __dpll_pin_change_ntf() get NULL and oops on
>> pin->id and pin->clock_id in dpll_pin_notify()? Or would CPU0 read the
>> freed zl3073x_dpll_pin?
>>
>> The list walk in zl3073x_dpll_output_pin_sibling_get() can also run
>> while list_del() is in progress, because the writer never takes
>> zldpll->lock.
>>
>> zl3073x_dpll_output_pin_frequency_set() and
>> zl3073x_dpll_output_pin_phase_adjust_set() have the same pattern.
>>
>> The later patch "dpll: zl3073x: add PTP periodic output support" also
>> calls zl3073x_dpll_output_pin_sibling_get() from
>> zl3073x_dpll_ptp_enable() without holding zldpll->lock at all.
> 
> This cannot happen; the notification is already serialized against pin
> teardown.
> 
> The output setters (esync_set / frequency_set / phase_adjust_set) run
> with dpll_lock held: the dpll core invokes the pin ops under dpll_lock,
> and they use __dpll_pin_change_ntf(), whose contract is exactly "caller
> must hold dpll_lock" (dpll_netlink.c: lockdep_assert_held(&dpll_lock),
> "suitable for use inside pin callbacks which are already invoked under
> dpll_lock"). So sibling_get() and the __dpll_pin_change_ntf() call - even
> though it runs after mutex_unlock(&zldpll->lock) - are all covered by
> dpll_lock.
> 
> Pin teardown frees the sibling via zl3073x_dpll_pin_unregister() ->
> dpll_pin_unregister(), which takes dpll_lock. While a setter holds
> dpll_lock for the whole callback, teardown cannot even reach
> dpll_pin_unregister(N), let alone the following list_del()/kfree(). The
> proposed interleaving (CPU1 freeing N while CPU0 notifies) is therefore
> impossible: CPU0 holds dpll_lock throughout.
> 
> The PTP enable() path (zl3073x_dpll_ptp_enable(), added in patch 6) does
> call sibling_get() outside dpll_lock, but it is serialized differently:
> zl3073x_dpll_unregister() unregisters the PTP clock *before* the pins
> (zl3073x_dpll_ptp_unregister() then zl3073x_dpll_pins_unregister()), and
> ptp_clock_unregister() quiesces in-flight enable() callbacks. No
> enable() can run while the pins are being freed.
> 

More thinking about it... and yes this can happen :-(
The pin removal from the list must be performed prior its unregistration
and also the list management has to be protected by zldpll->lock.

Will fix and send as bugfix to net branch with proper Fixes: tags.

Thanks,
Ivan