Re: [PATCH net-next v11 11/13] dpll: sit9531x: add support to adjust output phase

[Ali Rouhi <arouhi@sitime.com>]

On Mon, 5 Oct 2026, netdev-bot+sashiko@kernel.org wrote:

Replies inline, in the order of the summary list. Nine of the ten are
fixed; the third is answered and its documentation fixed.

> [Severity: Medium]
> How many times does one rate change disturb the PLL here?
> sit9531x_output_freq_set() has already called sit9531x_output_phase_flush()
> after the DIVO commit.
>
> [...]
>
> Should the commit message mention the full PLL restart? And could the rate
> change avoid the second programming cycle and flush?

Fixed: a rate change on an output with a programmed delay re-times the
delay inside the rate change's own programming window, so there is one
sequence and one flush instead of two of each.

The commit message now also says what the flush is on a PLL the loaded
profile builds without the phase-flush feature: realigning its outputs
restarts the whole PLL, so a phase adjust or a rate change on such a PLL
is a loss of lock as well as an edge jump on its other outputs.

> [Severity: Medium]
> Can this seed a clamped value that a later rate change then writes to the
> device?
>
> [...]
>
> The getter also reports +1 ms while the hardware holds 10 ms. A request
> for exactly +1 ms is therefore dropped by dpll_pin_phase_adj_set() and
> never reaches the device.

Fixed: a delay the loaded configuration left beyond the advertised window
is reported clamped and is not armed, so a later rate change does not
write the clamp into the device. The setter arms only a request that
quantized to something. Both comments are corrected.

> [Severity: Medium]
> Is this the inverse of the encoding in
> sit9531x_output_phase_adjust_set()? After the fold, ps < t_out_ps. When
> t_out_ps <= SIT9531X_OUT_PHASE_ADJ_MAX_PS, which is every output faster
> than 1 kHz, the two conditions can never both hold, so the function never
> returns a negative value.
>
> [...]
>
> The kernel-doc above says such an advance "reads back as that advance".
> That does not seem to hold for these outputs.

The register is unsigned, so a delay D and an advance of T - D are the
same edge, and the sign the driver reports for it is a convention. The
reader reports the advance form only where the delay form lies beyond the
advertised window, which for an output faster than 1 kHz never happens;
so a -30000 ps request on 10 MHz reads back as +70000 ps. Both describe
the same edge, and a later rate change re-times the same edge either way.

What was wrong is the kernel-doc, which promised the advance form
unconditionally. It now says when it applies, and the commit message says
the same.

> [Severity: Medium]
> Does this drop register access errors from sit9531x_output_divo_read()?
> An I2C or regmap error is handled the same way as -ENODATA, and the
> function falls through to:
>
> [...]
>
> Should only -ENODATA skip the fold, with other errors returned?

Fixed: a bus error from the divider read fails the read-back; only the
"no divider programmed" case skips the fold.

> [Severity: Medium]
> Is a failed flush here ever retried? The delay bytes can be committed and
> then sit9531x_output_phase_flush() can fail, for example on its first
> CONFIG47 read. When that happens, phase_stale is the only record of the
> failure.
>
> [...]
>
> Either way the realignment never happens, although phase_stale exists to
> force a rewrite after this kind of failure. Should a pending flush be
> tracked separately from an unconfirmed cache?

Fixed: a flush that failed after the delay was committed is remembered in
its own flag, which the getter leaves alone and the setter honors by
flushing even when the requested bytes match the cache. The cache keeps
the value the output actually realizes.

> [Severity: Medium]
> Does truncating the period to whole picoseconds give the wrong fold for
> requests longer than one period? The dropped fraction accumulates once for
> every period that is folded away.
>
> [...]
>
> The read-back fold in sit9531x_output_phase_read() uses the same truncated
> period.

Fixed: the fold is counted in VCO cycles modulo the divider, which is
exactly the period, and the sub-cycle picoseconds are carried across.
1e8 ps on a 128 MHz output of a 5.12 GHz VCO folds to zero, as it should,
and the read-back folds the same way.

> [Severity: Medium]
> Is this block reached on every path where the divider changed? Earlier in
> sit9531x_output_freq_set(), the function returns before getting here:
>
> [...]
>
> Should the error path set phase_stale when phase_armed is set?

Fixed: when the commit fails after the divider was written, an armed
delay is marked stale so the next read goes to the device rather than to
a cache that describes the old period.

> [Severity: Medium]
> Can this conversion overflow and raise a divide error? Here coarse is a
> raw 34-bit value from the device. sit9531x_get_fvco() only rejects
> fvco == 0; an out-of-band fvco gets a dev_warn_once() and is returned
> anyway.
>
> [...]
>
> sit9531x_divn_static() already guards against this kind of #DE. Should
> these conversions also bound fvco or the result?

Fixed through the VCO floor described on 09/13: a rate below the low band
is reported as no data before any conversion runs, so neither the phase
code nor the time-to-digital converter path can divide by it.

> [Severity: Low]
> Is "well beyond the DCO dynamic range" accurate? The PRG_RST_DELAY coarse
> field is 34 bits of VCO cycles, about 3.4 s at 5 GHz, and requests are
> folded modulo one output period. On a 1 PPS output, the hardware can
> therefore realize delays anywhere in the 1 s period.
>
> [...]
>
> On slow outputs, the +/-1 ms window is narrower than what the hardware can
> do. The real limit is the s32 picosecond uAPI (about +/-2.147 ms), not the
> device.

Fixed: the device holds a delay anywhere within the output period, so on
a slow output the bound is the signed 32-bit picosecond attribute rather
than the hardware. The comment in prop.c and the commit message say that
instead.

> [Severity: Low]
> Is the previous coarse cycle ever considered here? The fine field goes up
> to 7 * 30 = 210 ps, which is longer than one VCO period in every
> supported band. So coarse - 1 with a large fine code can be closer, or
> even exact.
>
> [...]
>
> The quantizer is also not idempotent for such encodings. A profile holding
> coarse 0 / fine 7 seeds phase_adj = 210 and phase_armed = true. The next
> frequency_set re-encodes it as 200 ps, which rewrites the registers and
> flushes the PLL.

Fixed: the candidates are one cycle fewer, the floor and one cycle more,
each with the fine steps capped, and the nearest wins; 210 ps at 5 GHz
encodes as seven fine steps exactly rather than one cycle short. The
comment that said the fine field tops out below one VCO period was wrong
too -- 210 ps exceeds every in-band cycle -- and is gone.