Re: [PATCH net-next v9 10/15] dpll: sit9531x: add support to adjust output phase

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

Critical: 0 · High: 0 · Medium: 5 · Low: 1

- [Medium] `struct sit9531x_out.phase_adj` is a pure software cache that
  is never seeded from the hardware.
- [Medium] A per-pin phase-adjust request ends with
  `sit9531x_output_phase_flush(sitdev, pll_idx)`, which arms…
- [Medium] `sit9531x_output_phase_adjust_set()` commits the new
  PRG_RST_DELAY bytes to the device and only afterwards updates its…
- [Medium] `sit9531x_output_freq_set()` now returns the result of the
  added phase-adjust re-encode.
- [Medium] `sit9531x_output_phase_adjust_set()` derives the output period
  from the cached `sitdev->out[out_idx].freq` and only falls back to a…
- [Low] Concern that the block comment above
  `sit9531x_output_phase_adjust_set()` in core.c documents page/base…

--

dpll: sit9531x: add support to adjust output phase

This adds phase_adjust_get/phase_adjust_set to the sit9531x output pin ops,
programming the per-output PRG_RST_DELAY block (34-bit coarse in VCO cycles
plus a 3-bit fine field in 30 ps steps) from inside the PRG_CMD programming
state, and re-encoding a cached request when the output rate changes.

A few questions about the new code below.

[Severity: Medium]
Can this turn a completed frequency change into a reported failure?  By the
time this runs, sit9531x_output_divo_write(), sit9531x_prg_commit() and the
phase flush have all succeeded and sitdev->out[out_idx].freq already holds
the new rate.

Any error inside sit9531x_output_phase_adjust_set() (bus error in
sit9531x_prg_enter(), the read/write loops, prg_commit or the second flush,
or -ENODEV from sit9531x_get_fvco()) becomes the return value here, with no
rollback of DIVO, and sit9531x_dpll_output_pin_frequency_set() reports it as
"Output frequency set failed".

An identical retry does not repair it either, because dpll_pin_freq_set()
asks the driver for the current rate first:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	...
	if (freq == old_freq)
		return 0;
	...
}

and sit9531x_dpll_output_pin_frequency_set()'s companion get reads DIVO back
from hardware, so the second request never reaches the driver and the phase
re-encode that failed is never performed.  For a negative cached adjust the
delay registers then stay encoded against the old period, since the
rendering T_out - |phase| depends on the period.

Would it be better to log the re-encode failure and still return success for
the frequency set?

[ ... ]

+	freq = sitdev->out[out_idx].freq;
+	if (!freq) {
+		/*
+		 * The cache is only seeded by a DT frequency list or an
+		 * earlier get/set; a board without supported-frequencies-hz
+		 * would otherwise get -EINVAL on every phase request forever.
+		 * Read the effective rate back from the divider chain.
+		 */
+		rc = sit9531x_output_freq_get(sitdev, out_idx, &freq);
+		if (rc)
+			return rc;
+		if (!freq)
+			return -EINVAL;
+	}

[Severity: Medium]
The live divider is consulted only when the cache is zero.  Can a non-zero
cached value be a rate the hardware never produced?

sit9531x_pin_props_get() in prop.c seeds it from the DT list rather than
from the device:

	if (curr_freq) {
		if (dir == DPLL_PIN_DIRECTION_INPUT)
			sitdev->ref[index].freq = curr_freq;
		else
			sitdev->out[index].freq = curr_freq;
	}

where curr_freq is the first entry of supported-frequencies-hz, and the
binding only enumerates the rates the output supports.  The driver also
documents the cache as unreliable in sit9531x_output_freq_get():

	 * correct even when the divider was reprogrammed behind the driver's
	 * back (e.g. by a direct-I2C userspace tool), where the cached value
	 * would be stale.

A wrong period lands directly in the encoding, because the negative path
computes abs_ps = t_out_ps - advance.  With a true period of 40000 ps but
6400 ps assumed (156.25 MHz listed first), a request of -1000 ps programs a
5400 ps delay instead of 39000 ps, and success is returned.  Positive
requests are mis-folded the same way once the request exceeds the assumed
period.

Nothing in dpll_pin_phase_adj_set() calls frequency_get() first, so a phase
request can be the very first operation on the pin.  Should the period be
read back from DIVO on every phase update?

[ ... ]

+	slot = info->clkout_map[out_idx];
+	page = (slot > SIT9531X_PAGE_OUTSYS0_SLOT_MAX) ?
+	       SIT9531X_PAGE_OUTSYS1 : SIT9531X_PAGE_OUTSYS0;
+	base = SIT9531X_OUT_PRG_DELAY_BASE +
+	       SIT9531X_OUT_PRG_SLOT_STRIDE * (slot % 6);

[Severity: Low]
This isn't a bug, but the block comment immediately above this function
describes the same addressing in terms of the logical output index:

 * Outputs 0-5 live on Page 3, outputs 6-11 on Page 4, with each
 * output's block at base = 0x15 + 16 * (out_idx % 6).

while the code here and the new regs.h comment use the physical slot from
clkout_map[].  The code is the correct one of the two.

The wording is already corrected later in the same series, in "dpll:
sit9531x: model the inter-PLL sync net as a pair of pins", where it becomes
"Slots 0-5 live on Page 3, slots 6-11 on Page 4 ... not the logical output
index", so this only concerns this commit.

[ ... ]

+	for (i = 0; i < ARRAY_SIZE(new_bytes); i++) {
+		rc = sit9531x_write_u8(sitdev,
+				       SIT9531X_REG(page, base + i),
+				       new_bytes[i]);
+		if (rc)
+			goto rollback;
+	}
+
+	goto commit;

[ ... ]

+commit:
+	/*
+	 * Always leave the PRG_CMD state via prg_commit(), even on a
+	 * mid-sequence write failure, so the output loops are re-locked rather
+	 * than stranded unlocked; keep the first error.
+	 */
+	ret = sit9531x_prg_commit(sitdev);
+	if (ret && !rc)
+		rc = ret;
+	if (rc)
+		return rc;

[Severity: Medium]
When the five writes all succeeded and sit9531x_prg_commit() then fails,
this returns before the phase_adj assignment at the end of the function, and
nothing restores old_bytes.  The same applies to the
sit9531x_output_phase_flush() failure below.

So the delay registers of that slot hold the new coarse/fine value while
sitdev->out[out_idx].phase_adj still holds the previous one, and only a
mid-write failure takes the rollback path.

Getting back to the reported value is then not possible through the
attribute, because the core drops a request equal to what the getter
reports:

drivers/dpll/dpll_netlink.c:dpll_pin_phase_adj_set() {
	...
	if (phase_adj == old_phase_adj)
		return 0;
	...
}

The unrecorded delay is also applied at the next output-divider phase flush
of that PLL, for instance during a frequency set on a sibling output, so the
phase step appears at a point the driver never recorded.

Should these two paths roll the delay bytes back, or update the cache to
what the registers now hold?

+	/*
+	 * Restart the output divider phase so the freshly programmed delay is
+	 * applied against a known edge instead of the divider's arbitrary
+	 * running phase.
+	 */
+	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
+	if (rc)
+		return rc;

[Severity: Medium]
Does a single-pin phase-adjust request disturb the sibling outputs of the
same PLL?  sit9531x_output_phase_flush() arms
SIT9531X_PLL_PHFL_ON_DEMAND_EN on the PLL page and then pulses the global
trigger:

	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_GPIO_FUNC_CTRL1,
			       ctrl | SIT9531X_DIVO_PHASE_TRIG);

regs.h documents SIT9531X_DIVO_PHASE_TRIG as flushing the output phase with
no per-output selection, so the output-divider phase of every output fed by
that PLL looks like it gets restarted, i.e. an edge jump on the outputs that
were not adjusted.

On top of that, sit9531x_prg_enter() puts the whole output system into the
PRG_CMD state with the loops open, and sit9531x_prg_commit() re-locks them
after msleep(100), so each phase-adjust request also touches outputs of the
other PLLs.

The DPLL ABI models phase adjust as a per-pin property.  Could the scope of
this operation be narrowed, or at least described in the commit message and
in a comment?

[Severity: Medium]
Is the value returned here ever reconciled with the device?  phase_adj is
zeroed by the devm_kzalloc() in sit9531x_i2c_probe(),
sit9531x_out_state_fetch() reads output mode, forced Hi-Z and routing but
never the five PROG6..PROG2 delay bytes, and the only assignment is at the
tail of sit9531x_output_phase_adjust_set().

Since that setter commits with SIT9531X_UPDATE_NVM through
sit9531x_prg_commit(), and the delay fields are part of the profile the chip
loads before probe, the device can hold a non-zero delay while this reports
0 - after a module unload/reload, after a warm reboot, or on a board whose
profile programmed a delay.

A request of 0 ps meant to clear such a delay then never reaches the driver:

drivers/dpll/dpll_netlink.c:dpll_pin_phase_adj_set() {
	...
	if (phase_adj == old_phase_adj)
		return 0;
	...
}

and the re-encode added to sit9531x_output_freq_set() is gated on
if (sitdev->out[out_idx].phase_adj), so an unrecorded hardware delay is not
re-timed on a rate change either.

This also does not match the commit message:

    the two and what the core reads back is what the registers hold rather
    than what was asked for

nor the new kernel-doc "phase adjust the delay registers actually realize",
since the registers are never read for this purpose.  This looks unchanged
at the end of the series.

Could phase_adjust_get() decode the PRG_RST_DELAY block, or the startup
state fetch seed phase_adj from it?

[ ... ]

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