Re: [PATCH net-next v8 12/15] dpll: sit9531x: add support to get phase offset on the connected input pin

[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

- [Medium] The TDC sample width is described three mutually inconsistent
  ways by artifacts this patch adds.
- [Medium] The callback's own comment states a hardware precondition it
  neither creates nor checks: "the TDC compares against a signal the PLL…
- [Medium] A transient I2C/regmap failure is converted into a fabricated,
  successful phase-offset measurement of 0.
- [Medium] The callback decides which pin owns the TDC sample from the
  software cache `sitdev->chan[sitdpll->id].selected_ref`, but takes a…
- [Low] The new kernel-doc for sit9531x_dpll_input_pin_phase_offset_get()
  justifies its entire design decision (report the closed-loop residual,…
- [Low] struct sit9531x_dpll_pin::phase_offset is declared `s64
  phase_offset; /* picoseconds */` (drivers/dpll/sit9531x/dpll.h:35),…
- [Low] sit9531x_phase_offset_read() documents the conversion as
  `phase_diff (ps) = tdc_code * 1e12 / fvco` but implements `fvco_mhz =…

--

dpll: sit9531x: add support to get phase offset on the connected input pin

This adds sit9531x_phase_offset_read() to sample the on-chip
time-to-digital converter through the PLL debug window and convert the
code to picoseconds, and wires it up as the input pin
.phase_offset_get callback.  Pins that are not the PLL's selected
reference report zero.

A few questions on the readback and on what gets reported to userspace.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index c93578f3b750..f552a9c73796 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -1586,6 +1586,131 @@ int sit9531x_output_pulse_ctrl_set(struct sit9531x_dev *sitdev,
>  	return rc;
>  }
>  
> +/*
> + * sit9531x_phase_offset_read - read phase difference via TDC
> + * @phase_ps:	output phase difference in picoseconds
> + *
> + * Reads the Time-to-Digital Converter (TDC) 40-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.
> + */

[ ... ]

> +	tdc_raw = 0;
> +
> +	rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +				  SIT9531X_PLL_REG_DBG_DATA_4, &v);
> +	if (rc)
> +		return rc;
> +	sign = !!(v & BIT(SIT9531X_TDC_SIGN_BIT));
> +	tdc_raw = (u64)(v & 0x07) << 32;

[Severity: Medium]
How wide is the TDC sample meant to be?  Three places in this patch
disagree.

The kernel-doc above says "Reads the Time-to-Digital Converter (TDC)
40-bit code".

regs.h annotates the top byte as nine bits in an eight bit register:

	#define SIT9531X_PLL_REG_DBG_DATA_4		0xB9  /* [39:32] + sign */

And the code keeps three bits of that register and takes bit 3 as the
sign (SIT9531X_TDC_SIGN_BIT == 3):

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

That assembles code[34:0], not code[39:0].  If the register annotation is
the correct one, are magnitude bits [39:35] dropped here with no range
check, so a large sample is scaled and published as an arbitrary
DPLL_A_PIN_PHASE_OFFSET value instead of an error?

The 0x07 is also the only unnamed field mask in a header that names every
other field, which hides the width at the call site.  Could it get a
define next to SIT9531X_TDC_SIGN_BIT?

[ ... ]

> +	/*
> +	 * Get VCO frequency for conversion.  Fvco==0 means DIVN is not
> +	 * programmed (PLL unused on this board) -- skip silently rather
> +	 * than spamming the log on every poll cycle.
> +	 */
> +	fvco = sit9531x_get_fvco(sitdev, pll_idx);
> +	if (!fvco) {
> +		dev_dbg(sitdev->dev, "PLL%c: Fvco unknown, skip TDC\n",
> +			'A' + pll_idx);
> +		return -ENODEV;
> +	}

[Severity: Medium]
Is "Fvco==0 means DIVN is not programmed" the only case?  sit9531x_get_fvco()
also returns 0 on any register read failure:

	rc = sit9531x_divn_static(sitdev, pll_idx, &divn);
	if (rc || divn <= 0)
		return 0;

	doubler = sit9531x_is_xo_doubler_enabled(sitdev);
	if (doubler < 0)
		return 0;

So a single I2C NAK or timeout inside sit9531x_divn_static() ends up here
as -ENODEV, and the caller turns that into a successful measurement:

	if (rc == -ENODEV) {
		dpin->phase_offset = 0;
		*phase_offset = 0;
		return 0;
	}

Does that make a bus error indistinguishable from "the loop residual is
exactly 0 ps" on DPLL_A_PIN_PHASE_OFFSET?  The framework averages
successive samples, so every failing poll would pull the average toward
zero.

Note also the asymmetry within this same function: the identical bus
failure during the trigger and data reads is returned as an error, while
the one inside sit9531x_get_fvco() becomes a reported value.  Would
distinguishing "no DIVN programmed" from "read failed" (for example by
having sit9531x_get_fvco() return an errno separately from the value) keep
those two paths consistent?

> +
> +	/*
> +	 * phase_diff (seconds) = tdc_code / fvco
> +	 * phase_diff (ps) = tdc_code * 1e12 / fvco
> +	 *
> +	 * To avoid 64-bit overflow:
> +	 *   phase_ps = tdc_code * 1e6 / (fvco / 1e6)
> +	 */
> +	fvco_mhz = div64_u64(fvco, 1000000ULL);
> +	if (!fvco_mhz)
> +		return -EIO;
> +
> +	*phase_ps = div64_s64(tdc_signed * 1000000LL, (s64)fvco_mhz);

[Severity: Low]
The comment states the conversion as tdc_code * 1e12 / fvco, but the code
truncates the denominator to whole MHz first.

sit9531x_get_fvco() returns Hz derived from a fractional DIVN:

	return mul_u64_u64_div_u64(fref, (u64)divn, SIT9531X_DIVN_SCALE);

so a sub-MHz remainder is normal, and 2949120000 Hz becomes 2949 MHz here,
about 40 ppm of scale error on every reported magnitude.

Is the overflow concern real?  mul_u64_u64_div_u64() is already used twice
in this file (sit9531x_get_fvco() and sit9531x_pll_ffo_ppt()) and would
keep the exact Hz denominator:

	*phase_ps = mul_u64_u64_div_u64(abs(tdc_signed), 1000000000000ULL, fvco);

> +
> +	return 0;
> +}
> +
>  /*
>   * sit9531x_ref_state_fetch - read input reference status from hardware
>   * @index:	logical input index
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 76d7d7d21d3d..f813126077e1 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -528,6 +528,105 @@ 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

[ ... ]

> + * 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 the open-loop measurement lives behind a debugfs operation that owns
> + * the freeze and restores it; it is not this callback.

[Severity: Low]
Where is that debugfs operation?  Searching the driver directory at the tip
of the series, the only match for debugfs under drivers/dpll/sit9531x/ is
this comment line itself, and the directory contains only Kconfig, Makefile,
core.c/h, dpll.c/h, prop.c/h and regs.h.

Since this sentence is the justification for reporting the closed-loop
residual rather than the vendor's open-loop quantity, could it either point
at code that exists or be reworded so the rationale can be checked?

> + *
> + * 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 taken in that state "is simply not meaningful", what stops it
from being reported as a measurement?

The callback only checks selected_ref and then reads unconditionally, and
sit9531x_phase_offset_read() returns 0 regardless of whether the PLL drives
any output, so whatever the debug window hands back is scaled and published
as DPLL_A_PIN_PHASE_OFFSET with no extack warning.

regs.h already defines SIT9531X_PLL_REG_ZDB0/ZDB1 with
SIT9531X_PLL_ZDB_EN_BIT and SIT9531X_PLL_REG_OUT_MAP_HI/LO.  Could the
getter test those and report zero (as it does for non-selected pins) when
the precondition does not hold?

The commit message does not mention the limitation at all; it says the
small value "is the measurement, not an artefact of it".  Could the
restriction be described there as well, so it is visible to anyone reading
git history?

[ ... ]

> +	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.
> +	 */
> +	if (sitdev->chan[sitdpll->id].selected_ref != dpin->id) {
> +		mutex_unlock(&sitdev->multiop_lock);
> +		dpin->phase_offset = 0;
> +		*phase_offset = 0;
> +		return 0;
> +	}
> +
> +	rc = sit9531x_phase_offset_read(sitdev, sitdpll->id, &offset);
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: Medium]
Can the pin the sample is attributed to differ from the pin the TDC
actually measured?

The ownership test uses the software cache chan[].selected_ref, whose only
writer is sit9531x_chan_state_fetch() running from the periodic monitor
work / threaded IRQ:

	chan->selected_ref =
		sit9531x_hw_src_input(input_sel & SIT9531X_PRIO_NIBBLE_MASK);

The sample itself is live and always taken against whatever reference the
PLL has selected at read time.  multiop_lock serialises access to the cache
but does not stop the chip from switching reference on its own (LOS on the
active input, revertive switchback).

In the window between such a switch and the next cache refresh, would a
dpll pin get on the previously selected pin pass the
selected_ref == dpin->id test and publish the new reference's phase as the
old pin's DPLL_A_PIN_PHASE_OFFSET, while the pin that is genuinely selected
fails the test and reports exactly 0?

Would reading the active-select register inside this same multiop_lock
section, as sit9531x_chan_state_fetch() does, close that window?

> +
> +	/*
> +	 * -ENODEV 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.
> +	 */
> +	if (rc == -ENODEV) {
> +		dpin->phase_offset = 0;
> +		*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;
> +
> +	dpin->phase_offset = offset;

[Severity: Low]
The value cached here is in ABI units (1/1000 ps), but the field it is
stored in is documented as picoseconds:

	s32				phase_adjust;	/* picoseconds */
	s64				phase_offset;	/* picoseconds */

The sibling phase_adjust really does hold picoseconds, since
DPLL_A_PIN_PHASE_ADJUST has no divider, so the two identical unit comments
now differ by a factor of 1000.  Nothing reads dpin->phase_offset today,
but would it be worth either caching the unscaled value or fixing the
comment to say 1/DPLL_PHASE_OFFSET_DIVIDER picoseconds?

> +	*phase_offset = offset;
> +	return 0;
> +}
> +

[ ... ]

> diff --git a/drivers/dpll/sit9531x/regs.h b/drivers/dpll/sit9531x/regs.h
> index 251c98dc7cbe..aa60be243e6d 100644
> --- a/drivers/dpll/sit9531x/regs.h
> +++ b/drivers/dpll/sit9531x/regs.h
> @@ -256,6 +256,42 @@

[ ... ]

> +#define SIT9531X_PLL_REG_DBG_DATA_3		0xB8  /* [31:24] */
> +#define SIT9531X_PLL_REG_DBG_DATA_4		0xB9  /* [39:32] + sign */
> +#define SIT9531X_PLL_REG_DBG_DATA_5		0xBA  /* [47:40] */

[ ... ]

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

These are the defines referenced in the sample-width question above.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902214030.20955-1-arouhi%40sitime.com