[PATCH v10 11/14] dpll: sit9531x: add support to get phase offset on the connected input pin

[Ali Rouhi]

From: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@xxxxxxxxxxxxxxxxx>

Report the phase difference between a PLL's reference and the PLL itself,
which is the loop's own residual error and therefore trends small on a
locked device -- that is the measurement, not an artefact of it.

The value comes from the on-chip time-to-digital converter, read through
the debug window: unlock the window, point it at the converter, then read
the trigger register, which latches a fresh sample and returns the
previous one.  It is read three times per sample for that reason; a single
read hands back the sample from the last call, so a repeated measurement
would look perfectly steady while saying nothing.

Only the input a PLL has actually selected has a phase offset against it.
For any other pin there is nothing to measure and zero is reported,
because the core abandons an entire pin dump on an error from any one pin.

Signed-off-by: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@xxxxxxxxxxxxxxxxx>
Assisted-by: Claude:claude-4-opus [chat]
Signed-off-by: Ali Rouhi <arouhi@xxxxxxxxxx>
---

Notes:
    Changes in v10:
    Gave the dormant-PLL case its own errno, so a bus error is not reported
    as a valid zero reading.
    
    Read the selected reference from the device for the measurement rather
    than from a cache up to a poll period old.
    
    Restored the debug tap selection the read changes, and dropped the
    cached per-pin phase offset, which was written on every path of the
    getter and read on none.

 drivers/dpll/sit9531x/core.c | 215 +++++++++++++++++++++++++++++++++++
 drivers/dpll/sit9531x/core.h |   4 +
 drivers/dpll/sit9531x/dpll.c | 118 +++++++++++++++++++
 drivers/dpll/sit9531x/regs.h |  36 ++++++
 4 files changed, 373 insertions(+)

diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
index c8c3cd6a64ba..8f6f8ffc8dc1 100644
--- a/drivers/dpll/sit9531x/core.c
+++ b/drivers/dpll/sit9531x/core.c
@@ -2314,6 +2314,221 @@ int sit9531x_clear_notifications(struct sit9531x_dev *sitdev)
 	return 0;
 }
 
+/**
+ * sit9531x_chan_selected_ref_read - read a PLL's active reference now
+ * @sitdev:	device pointer
+ * @pll_idx:	PLL index (0-3)
+ * @ref:	result, logical input index of the selected reference
+ *
+ * chan->selected_ref is refreshed by the monitor twice a second, which is
+ * close enough for reporting pin state but not for attributing a
+ * measurement: the device picks its own reference, so a sample taken now
+ * can belong to a pin the cache has not caught up with.
+ *
+ * Caller must hold sitdev->multiop_lock.
+ *
+ * Return: 0 on success, <0 on error
+ */
+int sit9531x_chan_selected_ref_read(struct sit9531x_dev *sitdev, u8 pll_idx,
+				    u8 *ref)
+{
+	u8 activesel_reg, input_sel;
+	int rc;
+
+	lockdep_assert_held(&sitdev->multiop_lock);
+
+	if (pll_idx >= SIT9531X_NUM_PLLS)
+		return -EINVAL;
+
+	activesel_reg = SIT9531X_PRIO_BASE_REG +
+			SIT9531X_PRIO_REGS_PER_PLL * pll_idx +
+			SIT9531X_PRIO_ACTIVESEL_OFF;
+	rc = sit9531x_read_u8(sitdev,
+			      SIT9531X_REG(SIT9531X_PAGE_PRIOSYS,
+					   activesel_reg),
+			      &input_sel);
+	if (rc)
+		return rc;
+
+	*ref = sit9531x_hw_src_input(input_sel & SIT9531X_PRIO_NIBBLE_MASK);
+
+	return 0;
+}
+
+/*
+ * sit9531x_phase_offset_read - read phase difference via TDC
+ * @phase_ps:	output phase difference in picoseconds
+ *
+ * Reads the Time-to-Digital Converter (TDC) signed 35-bit code from the
+ * PLL page registers, then converts to picoseconds using the VCO
+ * frequency: phase_diff = tdc_code / fvco.
+ *
+ * Caller must hold sitdev->multiop_lock.
+ */
+int sit9531x_phase_offset_read(struct sit9531x_dev *sitdev, u8 pll_idx,
+			       s64 *phase_ps)
+{
+	u8 v, old_write_code, old_read_code;
+	bool have_old = false;
+	int rc, lock_rc, i;
+	u64 fvco, mag_ps;
+	s64 tdc_signed;
+	u64 tdc_raw;
+	bool sign;
+
+	lockdep_assert_held(&sitdev->multiop_lock);
+
+	if (pll_idx >= SIT9531X_NUM_PLLS)
+		return -EINVAL;
+
+	/* Unlock the debug page so the TDC registers are accessible. */
+	rc = sit9531x_write_pll_u8(sitdev, pll_idx,
+				   SIT9531X_PLL_REG_DEBUG,
+				   SIT9531X_PLL_DEBUG_UNLOCK);
+	if (rc)
+		goto relock;
+
+	/*
+	 * Remember the tap selection so it can be put back.  The key
+	 * register is re-locked below, but the mux is not part of the key:
+	 * leaving it parked on the TDC with a slow sampling clock selected
+	 * is a state change the caller did not ask for, and the next reader
+	 * of a different tap would have to know to undo it.
+	 */
+	if (!sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_WRITE_CODE,
+				  &old_write_code) &&
+	    !sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_READ_CODE,
+				  &old_read_code))
+		have_old = true;
+
+	/*
+	 * Select the debug clock for taps below 200 kHz, then point the
+	 * readback at the TDC.  Only the one bit is touched: writing the
+	 * modifier register whole would clear the fields belonging to
+	 * other taps.
+	 */
+	rc = sit9531x_update_pll_u8(sitdev, pll_idx,
+				    SIT9531X_PLL_REG_DBG_WRITE_CODE,
+				    SIT9531X_DBG_LOW_FREQ_CLK_BIT,
+				    SIT9531X_DBG_LOW_FREQ_CLK_BIT);
+	if (rc)
+		goto relock;
+	rc = sit9531x_write_pll_u8(sitdev, pll_idx,
+				   SIT9531X_PLL_REG_DBG_READ_CODE,
+				   SIT9531X_DBG_READ_CODE_TDC);
+	if (rc)
+		goto relock;
+
+	/*
+	 * Latch a sample by reading the trigger register.  A single
+	 * read returns the previous latch, so read it three times as
+	 * the documented phase-difference procedure does.
+	 */
+	for (i = 0; i < SIT9531X_DBG_LATCH_READS; i++) {
+		rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+					  SIT9531X_PLL_REG_DBG_TRIGGER, &v);
+		if (rc)
+			goto relock;
+	}
+
+	tdc_raw = 0;
+
+	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_DATA_4, &v);
+	if (rc)
+		goto relock;
+	sign = !!(v & BIT(SIT9531X_TDC_SIGN_BIT));
+	tdc_raw = (u64)(v & SIT9531X_TDC_MAG_HI_MASK) << 32;
+
+	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_DATA_3, &v);
+	if (rc)
+		goto relock;
+	tdc_raw |= (u64)v << 24;
+
+	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_DATA_2, &v);
+	if (rc)
+		goto relock;
+	tdc_raw |= (u64)v << 16;
+
+	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_DATA_1, &v);
+	if (rc)
+		goto relock;
+	tdc_raw |= (u64)v << 8;
+
+	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
+				  SIT9531X_PLL_REG_DBG_DATA_0, &v);
+	if (rc)
+		goto relock;
+	tdc_raw |= v;
+
+	/*
+	 * Apply sign.  Per the register map the sign bit is active-high
+	 * for a positive offset: bit set -> +code, bit clear -> -code.
+	 */
+	tdc_signed = sign ? (s64)tdc_raw : -(s64)tdc_raw;
+
+	/*
+	 * Get VCO frequency for conversion.  -ENODATA means DIVN is not
+	 * programmed (PLL unused on this board) -- skip silently rather
+	 * than spamming the log on every poll cycle.  It is passed up as
+	 * itself rather than as -ENODEV, which the I2C layer produces for
+	 * an adapter that has gone away: the caller turns the dormant-PLL
+	 * case into a zero reading, and a bus failure must not take that
+	 * path.
+	 */
+	rc = sit9531x_get_fvco(sitdev, pll_idx, &fvco);
+	if (rc) {
+		if (rc == -ENODATA)
+			dev_dbg(sitdev->dev,
+				"PLL%c: Fvco unknown, skip TDC\n",
+				'A' + pll_idx);
+		goto relock;
+	}
+
+	/*
+	 * phase_diff (seconds) = tdc_code / fvco
+	 * phase_diff (ps) = tdc_code * 1e12 / fvco
+	 *
+	 * mul_u64_u64_div_u64() keeps the exact Hz denominator; dividing
+	 * by whole MHz instead would lose up to ~40 ppm of scale on a
+	 * fractional-DIVN Fvco.
+	 */
+	mag_ps = mul_u64_u64_div_u64(tdc_signed < 0 ? -tdc_signed : tdc_signed,
+				     1000000000000ULL, fvco);
+	*phase_ps = tdc_signed < 0 ? -(s64)mag_ps : (s64)mag_ps;
+
+	rc = 0;
+
+relock:
+	if (have_old) {
+		sit9531x_write_pll_u8(sitdev, pll_idx,
+				      SIT9531X_PLL_REG_DBG_READ_CODE,
+				      old_read_code);
+		sit9531x_write_pll_u8(sitdev, pll_idx,
+				      SIT9531X_PLL_REG_DBG_WRITE_CODE,
+				      old_write_code);
+	}
+
+	/*
+	 * Close the debug window again.  The key register opens every debug
+	 * register on this PLL while it holds the unlock value, and this read
+	 * runs on every pin-get of a connected input, so leaving it open
+	 * would mean normal monitoring permanently unlocks the block.
+	 */
+	lock_rc = sit9531x_write_pll_u8(sitdev, pll_idx,
+					SIT9531X_PLL_REG_DEBUG,
+					SIT9531X_PLL_DEBUG_LOCK);
+	if (lock_rc && !rc)
+		rc = lock_rc;
+
+	return rc;
+}
+
 /*
  * sit9531x_ref_state_fetch - read input reference status from hardware
  * @index:	logical input index
diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
index 7848ac9bd6ca..f2750c319ff3 100644
--- a/drivers/dpll/sit9531x/core.h
+++ b/drivers/dpll/sit9531x/core.h
@@ -289,6 +289,10 @@ int sit9531x_clear_notifications(struct sit9531x_dev *sitdev);
 /* ---- INTSYNC (inter-PLL synchronization) ---- */
 
 /* ---- Phase offset (TDC readback) ---- */
+int sit9531x_chan_selected_ref_read(struct sit9531x_dev *sitdev, u8 pll_idx,
+				    u8 *ref);
+int sit9531x_phase_offset_read(struct sit9531x_dev *sitdev, u8 pll_idx,
+			       s64 *phase_ps);
 
 /* ---- State helpers ---- */
 
diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
index af1089f192b6..5f7c2d01562c 100644
--- a/drivers/dpll/sit9531x/dpll.c
+++ b/drivers/dpll/sit9531x/dpll.c
@@ -638,6 +638,123 @@ sit9531x_dpll_input_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
 	return 0;
 }
 
+/*
+ * sit9531x_dpll_input_pin_phase_offset_get - phase offset of a reference
+ *
+ * What this reports, and what it deliberately does not:
+ *
+ * The ABI defines the attribute as the phase difference between the signal
+ * on a pin and its parent DPLL device, so this is the loop's own residual
+ * error, sampled with the loop closed.  On a locked DPLL it therefore
+ * trends small -- that is the measurement, not an artefact of it.  The
+ * documentation describes the reported value as one that may be averaged
+ * over prior measurements, which suits a closed-loop residual and not a
+ * one-shot open-loop capture; the core publishes whatever this callback
+ * returns, so the averaging, if any, would be this driver's to do.
+ *
+ * The chip can also measure the reference against the local oscillator
+ * with the outer loop's correction frozen, which is a different quantity
+ * and the one the documented phase-difference procedure produces.  That
+ * needs the digital loop filter held (and, on the 1PPS PLL, the automatic
+ * phase- and frequency-lock helpers held off), which leaves the PLL
+ * undisciplined until it is released.  A netlink read must not do that,
+ * so that measurement is not offered here at all; it belongs to a caller
+ * that can own the freeze and restore it.
+ *
+ * Precondition, which this callback cannot create: the TDC compares
+ * against a signal the PLL drives, so a PLL driving no output with its
+ * zero-delay buffer off has nothing to measure.  SiTime confirms this is
+ * a property of the hardware rather than of their measurement script.
+ * The script satisfies it by mapping a spare output and restarting the
+ * PLL -- side effects that do not belong in a getter, so a reading taken
+ * in that state is simply not meaningful.
+ *
+ * Non-selected pins and a PLL with no programmed divider report zero
+ * rather than an error: the DPLL core propagates any error from this
+ * callback and fails the whole pin dump with it, unlike the frequency
+ * offset getter, where -ENODATA makes the core omit the attribute.  There
+ * is no per-pin "no data" for phase offset, so it is a value or no
+ * callback at all.
+ */
+static int
+sit9531x_dpll_input_pin_phase_offset_get(const struct dpll_pin *pin,
+					 void *pin_priv,
+					 const struct dpll_device *dpll,
+					 void *dpll_priv, s64 *phase_offset,
+					 struct netlink_ext_ack *extack)
+{
+	struct sit9531x_dpll_pin *dpin = pin_priv;
+	struct sit9531x_dpll *sitdpll = dpll_priv;
+	struct sit9531x_dev *sitdev = sitdpll->dev;
+	enum dpll_pin_state state;
+	s64 offset;
+	u8 selected;
+	int rc;
+
+	mutex_lock(&sitdev->multiop_lock);
+
+	/*
+	 * The on-chip TDC is a per-PLL resource that always measures the
+	 * phase difference between the VCO and the PLL's currently
+	 * selected reference; it cannot be pointed at an arbitrary input.
+	 * For any input that is not the active reference there is no
+	 * meaningful per-pin phase offset, so report 0 instead of the
+	 * active reference's value.
+	 */
+	/*
+	 * Which pin the sample belongs to is read from the device rather
+	 * than taken from the monitor's cache: the device selects its own
+	 * reference, so a cache up to a poll period old could attribute a
+	 * live measurement to the pin that used to be selected.
+	 */
+	rc = sit9531x_chan_selected_ref_read(sitdev, sitdpll->id,
+					     &selected);
+	if (rc) {
+		mutex_unlock(&sitdev->multiop_lock);
+		NL_SET_ERR_MSG(extack,
+			       "Selected reference could not be read back");
+		return rc;
+	}
+
+	sit9531x_dpll_selection_state_get(sitdev, sitdpll, dpin->id, &state);
+	if (state != DPLL_PIN_STATE_CONNECTED || selected != dpin->id) {
+		mutex_unlock(&sitdev->multiop_lock);
+		*phase_offset = 0;
+		return 0;
+	}
+
+	rc = sit9531x_phase_offset_read(sitdev, sitdpll->id, &offset);
+	mutex_unlock(&sitdev->multiop_lock);
+
+	/*
+	 * -ENODATA means the PLL has no programmed DIVN (unused on this
+	 * board); report phase_offset = 0 so a full pin-get dump does not
+	 * fail just because one DPLL is dormant.  Every other errno,
+	 * -ENODEV from a vanished adapter included, is a failure.
+	 */
+	if (rc == -ENODATA) {
+		*phase_offset = 0;
+		return 0;
+	}
+	if (rc) {
+		NL_SET_ERR_MSG(extack, "TDC phase readback failed");
+		return rc;
+	}
+
+	/*
+	 * The ABI reports phase offset in units of 1/DPLL_PHASE_OFFSET_DIVIDER
+	 * picoseconds: the integer part of the attribute is the value divided
+	 * by the divider, the remainder is the fraction.  The TDC resolves one
+	 * VCO period (hundreds of picoseconds), so the fractional digits are
+	 * always zero here, but the magnitude still has to be scaled or every
+	 * reading would be reported a thousand times too small.
+	 */
+	offset *= DPLL_PHASE_OFFSET_DIVIDER;
+
+	*phase_offset = offset;
+	return 0;
+}
+
 static const struct dpll_pin_ops sit9531x_dpll_input_pin_ops = {
 	.direction_get		= sit9531x_dpll_input_pin_direction_get,
 	.frequency_get		= sit9531x_dpll_input_pin_frequency_get,
@@ -645,6 +762,7 @@ static const struct dpll_pin_ops sit9531x_dpll_input_pin_ops = {
 	.state_on_dpll_set	= sit9531x_dpll_input_pin_state_on_dpll_set,
 	.prio_get		= sit9531x_dpll_input_pin_prio_get,
 	.prio_set		= sit9531x_dpll_input_pin_prio_set,
+	.phase_offset_get	= sit9531x_dpll_input_pin_phase_offset_get,
 };
 
 /*
diff --git a/drivers/dpll/sit9531x/regs.h b/drivers/dpll/sit9531x/regs.h
index cbce62404c97..a9731c360bc0 100644
--- a/drivers/dpll/sit9531x/regs.h
+++ b/drivers/dpll/sit9531x/regs.h
@@ -255,6 +255,42 @@
 #define SIT9531X_PLL_REG_DIVN_NUM		0x32  /* 4 bytes (0x32-0x35) */
 #define SIT9531X_PLL_REG_DIVN_DEN		0x38  /* 4 bytes (0x38-0x3B) */
 
+/* Debug register unlock */
+#define SIT9531X_PLL_REG_DEBUG		0xBD
+#define SIT9531X_PLL_DEBUG_UNLOCK		0xC3
+#define SIT9531X_PLL_DEBUG_LOCK			0x00
+
+/*
+ * Signal pathway debug readback -- PLL page.  Dig_Sys_ReadCode selects
+ * which point of the pathway is tapped, Dig_Sys_WriteCode carries the
+ * modifiers for that read, Dig_Sys_read7..read0 hold the sampled bytes
+ * and the trigger register latches a sample.  The TDC phase
+ * measurement is one tap among several, reached through read code 69.
+ */
+#define SIT9531X_PLL_REG_DBG_READ_CODE	0xB3
+#define SIT9531X_PLL_REG_DBG_WRITE_CODE	0xB4
+#define SIT9531X_DBG_LOW_FREQ_CLK_BIT	BIT(7)
+#define SIT9531X_PLL_REG_DBG_DATA_0		0xB5  /* [7:0] */
+#define SIT9531X_PLL_REG_DBG_DATA_1		0xB6  /* [15:8] */
+#define SIT9531X_PLL_REG_DBG_DATA_2		0xB7  /* [23:16] */
+#define SIT9531X_PLL_REG_DBG_DATA_3		0xB8  /* [31:24] */
+#define SIT9531X_PLL_REG_DBG_DATA_4		0xB9  /* [34:32] + sign */
+/* read to latch a sample */
+#define SIT9531X_PLL_REG_DBG_TRIGGER		0xD0
+
+/*
+ * Reads of the trigger needed to latch a fresh sample.  One returns the
+ * previous latch, which the documented procedures work around by reading it
+ * three times.
+ */
+#define SIT9531X_DBG_LATCH_READS		3
+#define SIT9531X_DBG_DATA_BYTES		8
+
+/* Read code of the TDC phase tap, and the sign bit of its sample */
+#define SIT9531X_DBG_READ_CODE_TDC		69
+#define SIT9531X_TDC_SIGN_BIT		3
+#define SIT9531X_TDC_MAG_HI_MASK	GENMASK(2, 0)
+
 /*
  * DIVN carried as fixed point, and the unit the DPLL ABI wants the
  * fractional frequency offset in.  Equal in value, distinct in meaning.
-- 
2.43.0