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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: sit9531x: add support to get phase offset on the connected input pin

This adds a phase_offset_get callback for sit9531x input pins. It reads the
on-chip TDC through the per-PLL debug window and converts the code to
picoseconds using the VCO frequency. Only the input the PLL has selected
gets a measurement; other pins and dormant PLLs report zero.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index c8c3cd6a64baf..8f6f8ffc8dc13 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)

[ ... ]

> +/*
> + * 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)
> +{

[Severity: Low]
This isn't a bug, but is "signed 35-bit code" accurate? The decode below
builds a 35-bit magnitude from DBG_DATA_4[2:0] through DBG_DATA_0. The sign
comes from a separate bit in DATA_4, so the field is 36-bit sign-magnitude:

	sign = !!(v & BIT(SIT9531X_TDC_SIGN_BIT));
	tdc_raw = (u64)(v & SIT9531X_TDC_MAG_HI_MASK) << 32;

regs.h also describes DATA_4 as "[34:32] + sign".

This header also opens with /* rather than /**. It doesn't document
@sitdev, @pll_idx or a Return: section, while
sit9531x_chan_selected_ref_read() just above it does.

[ ... ]

> +	/*
> +	 * 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;

[Severity: Medium]
What happens if either snapshot read fails? have_old stays false, but the
sequence continues. SIT9531X_DBG_LOW_FREQ_CLK_BIT is still set in WRITE_CODE,
and SIT9531X_DBG_READ_CODE_TDC is still written to READ_CODE.

The restore at the relock label below is then skipped because have_old is
false. The PLL is left with the mux parked on the TDC and the slow sampling
clock selected, which is the state the comment above says must not be left
behind.

The next successful call then snapshots this modified state and restores to
it, so the change is never undone.

Separately, the two restore sit9531x_write_pll_u8() calls at relock ignore
their return values. A failed restore still lets the function return 0.

The v10 notes say "Restored the debug tap selection the read changes".
Should a failed snapshot abort before the tap is modified? Should restore
errors be folded into rc?

In this series, sit9531x_dbg_sample() reprograms both registers itself, so
no in-driver reader currently sees the wrong tap.

[ ... ]

> +	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;
> +	}

[ ... ]

> +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);
> +	}
> +

[ ... ]

> +	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;
> +}

[Severity: Medium]
On a dormant PLL, can a bus error still reach user space as a valid zero
reading?

sit9531x_get_fvco() runs only after the debug window has been unlocked, the
tap reprogrammed, the trigger read and DATA_4..DATA_0 read. When it returns
-ENODATA, the code jumps to relock with rc == -ENODATA.

The restore writes there ignore their errors. A failed lock write is also
dropped, because rc is already nonzero when this runs:

	if (lock_rc && !rc)
		rc = lock_rc;

The function returns -ENODATA. sit9531x_dpll_input_pin_phase_offset_get()
then turns that into success:

	if (rc == -ENODATA) {
		*phase_offset = 0;
		return 0;
	}

The PLL debug key can stay unlocked, and the tap may stay parked on the TDC,
while user space sees a successful 0.

sit9531x_dpll_selection_state_get() doesn't check chan->active. A dormant PLL
with a populated priority table can therefore pass the gate in the caller.

The v10 notes say "Gave the dormant-PLL case its own errno, so a bus error is
not reported as a valid zero reading."

Would it close this gap to check Fvco before opening the debug window, and
to fold lock and restore errors into rc whatever its previous value?

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index af1089f192b6f..5f7c2d01562c9 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
> + *

[ ... ]

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

[Severity: Medium]
If a reading in this state is not meaningful, what stops it from being
published?

Before sit9531x_phase_offset_read() runs, the only checks are the cached
CONNECTED state, the live ACTIVESEL match and the -ENODATA dormant-DIVN
case.

Take a PLL with a programmed DIVN and a selected reference but no routed or
enabled output. The TDC is still sampled. The value is scaled by
DPLL_PHASE_OFFSET_DIVIDER and returned as a valid DPLL_A_PIN_PHASE_OFFSET.
User space can't tell it apart from a real measurement.

The driver already tracks output routing in struct sit9531x_out (routed,
pll_idx, enabled). sit9531x_out_state_fetch() fills these from the OUT_MAP
registers and the Hi-Z force bits. From the code it isn't clear whether a
Hi-Z output counts as driving for the TDC, and ZDB state isn't tracked. The
no-routed-output case does look detectable, though.

Could the callback report 0 when the PLL drives no output, as it does for
the dormant-DIVN case?

The commit message doesn't mention this limitation, and it describes the
value as "the measurement".

[ ... ]

> +	/*
> +	 * 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);

[ ... ]

> +	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;
> +	}

[Severity: Low]
Does this match the comment above it? The gate still needs
sit9531x_dpll_selection_state_get() to return CONNECTED, and that result
comes from the cached chan->selected_ref and chan->mode:

	active_input = !chan->mode && chan->selected_ref == pin_id;

So the live ACTIVESEL read can only veto a sample. It doesn't decide which
pin gets it.

Suppose the device switches from IN0 to IN1 on its own. IN0 reports 0
because the live value no longer matches. IN1 also reports 0 because the
cache still says SELECTABLE. This lasts until the next
sit9531x_dev_periodic_work() poll refreshes the cache.

No wrong value is published. However, this comment and the
sit9531x_chan_selected_ref_read() kernel-doc ("close enough for reporting
pin state but not for attributing a measurement") both describe live-only
attribution.

Could the comments be updated to say that both the cached state and the live
read are required?

> +
> +	rc = sit9531x_phase_offset_read(sitdev, sitdpll->id, &offset);
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: Low]
Can the device fail over between the ACTIVESEL read above and the TDC latch?

Before the three SIT9531X_PLL_REG_DBG_TRIGGER reads latch the sample,
sit9531x_phase_offset_read() does several I2C transactions: the unlock, two
snapshot reads, the write-code update and the read-code write.
multiop_lock serialises the driver's accesses, but not the device's own
reference selection.

If the device switches reference in that window, the TDC samples against
the new reference. The result is then published as the old pin's phase
offset. This is the case the v10 change targets, now limited to a few
milliseconds.

Would re-reading ACTIVESEL after the latch, and reporting 0 on a mismatch,
close this?

[ ... ]