Re: [PATCH net-next v9 08/15] dpll: sit9531x: add support to get and set frequency on pins

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 4 · Low: 3

- [High] sit9531x_output_divo_calc() computes the output divider with
  truncating division (divo = div64_u64(fvco, frequency)) and accepts…
- [High] sit9531x_divn_fixed() computes frac =
  mul_u64_u64_div_u64(abs(fracn), SIT9531X_DIVN_SCALE, fracd) directly…
- [Medium] sit9531x_prg_enter() unlocks the Page-3 debug block and then
  issues PRG_CMD; if the second write fails it returns the error with…
- [Medium] A phase-flush failure is reported after the new divider has
  already been committed: sit9531x_output_freq_set() returns the error…
- [Medium] When the Fvco derived from the chip's DIVN registers falls
  outside the per-PLL band assumed from the PLL index parity,…
- [Medium] The phase-flush sequence issues the PLL small-change command
  with regmap_update_bits() (sit9531x_update_pll_u8) against the PLL-…
- [Low] Output-pin frequency ops added by this patch are not reachable in
  this snapshot because sit9531x_dpll_pin_is_registrable() rejects every…
- [Low] Claim that the new frequency callbacks index sitdev->ref[] /
  sitdev->out[] with the wider DPLL pin-id namespace, reading past the…
- [Low] The new frequency code branches on and extensively documents the
  'sitime,pll-fvco' / 'sitime,output-pll-map' DT overrides (including a…

--

dpll: sit9531x: add support to get and set frequency on pins

This adds frequency_get for input and XO pins (reported from the cached
firmware description) plus frequency_get/frequency_set for output pins,
where the rate is derived from the VCO and the per-output DIVO divider.
The set path enters the output-system programming state, rewrites the
five DIVO bytes, commits, and then flushes the PLL output phase.

No Link: tags in the commit header.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 484c94def578c..a83bf8ac83c1d 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -396,6 +396,84 @@ static int sit9531x_output_forced_hiz(struct sit9531x_dev *sitdev,
>  	return 0;
>  }
>  
> +/*
> + * Enter the output-system programming state: unlock the debug
> + * registers on Page 3 and issue the PRG_CMD state command.  Register
> + * writes that reconfigure the output system only take effect when
> + * they are made inside this state.
> + */
> +static int sit9531x_prg_enter(struct sit9531x_dev *sitdev)
> +{
> +	int rc;
> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> +			       SIT9531X_DEBUG_UNLOCK_VAL);
> +	if (rc)
> +		return rc;
> +
> +	return sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +				 SIT9531X_PRG_CMD_STATE);
> +}

[Severity: Medium]
If the second write in sit9531x_prg_enter() fails, is the Page 3 debug
unlock left in place?

The only code that writes SIT9531X_DEBUG_LOCK_VAL and re-issues
SIT9531X_LOOP_LOCK is sit9531x_prg_commit(), and the caller skips it:

	rc = sit9531x_prg_enter(sitdev);
	if (rc)
		return rc;

So on a bus error for the PRG_CMD write the debug block stays writable
indefinitely, and if the failed transfer's data did reach the part the chip
is left in PRG_CMD with the output loops open, which the comment in
sit9531x_prg_commit() calls "worse than any other failure this function can
report".

Would a goto to the commit/relock path be preferable here, in the same shape
sit9531x_phase_offset_read() already uses with its "goto relock" for its own
debug window?

[ ... ]

> @@ -921,6 +1004,510 @@ int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,
>  /* The output divider is a 34-bit field */
>  #define SIT9531X_DIVO_MAX			GENMASK_ULL(33, 0)
>  
> +/*

[ ... ]

> +static s64 sit9531x_divn_fixed(u32 int_part, s64 fracn, u64 fracd)
> +{
> +	s64 whole = (s64)int_part * SIT9531X_DIVN_SCALE;
> +	u64 frac;
> +
> +	if (!fracd)
> +		return whole;
> +
> +	frac = mul_u64_u64_div_u64(abs(fracn), SIT9531X_DIVN_SCALE, fracd);

[Severity: High]
Can this division trap when the register contents are not what the driver
expects?

The numerator and denominator come straight from the chip with no
plausibility check, and the denominator is biased by one in the caller:

	*divn = sit9531x_divn_fixed(int_part, (s32)fracn_raw,
				    (u64)fracd_raw + 1);

which also makes the "if (!fracd)" guard above unreachable.

abs(fracn) can be up to 2^31, so |fracn| * 1e12 / fracd exceeds U64_MAX
whenever |fracn| / fracd is above roughly 1.8e7 -- for example DEN reading
all zeroes (fracd == 1) with a NUM above about 18,446,744, or any fracd
below about 116 with a large numerator.

On x86_64 the helper is a mulq followed by a divq:

arch/x86/include/asm/div64.h:
	/*
	 * Will generate an #DE when the result doesn't fit u64, could fix with an
	 * __ex_table[] entry when it becomes an issue.
	 */

so an out-of-range quotient is a divide-error oops rather than a value the
later "if (divn <= 0) return -ENODATA;" in sit9531x_get_fvco() could reject.
This is reachable from the new output frequency get/set as well as from
sit9531x_phase_offset_read() on an ordinary pin get.

Would rejecting fracn magnitudes at or above fracd (or clamping the
fraction) before the division be a better fit for values read off a bus?

> +
> +	return fracn < 0 ? whole - (s64)frac : whole + (s64)frac;
> +}

[ ... ]

> +static int sit9531x_output_phase_flush(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
> +	u8 ctrl, orig;
> +	int rc, ret;
> +
> +	/* Arm the on-demand phase-flush on the PLL page. */
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx,
> +				    SIT9531X_PLL_REG_PHFL_CTRL,
> +				    SIT9531X_PLL_PHFL_ON_DEMAND_EN,
> +				    SIT9531X_PLL_PHFL_ON_DEMAND_EN);
> +	if (rc)
> +		return rc;
> +
> +	/* Latch it with the PLL small-change update. */
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx,
> +				    SIT9531X_PLL_REG_SMALL_UPDATE,
> +				    SIT9531X_SMALL_UPDATE_CMD,
> +				    SIT9531X_SMALL_UPDATE_CMD);

[Severity: Medium]
Is read-modify-write the right access for the small-change command?

SIT9531X_PLL_REG_SMALL_UPDATE is PLL-page offset 0x0F, which regs.h
describes as the per-page programming directive register (loop lock, NVM
bank, efuse program, small change update, PRG_CMD escape). The regmap has
.cache_type = REGCACHE_NONE, so sit9531x_update_pll_u8() reads the hardware
and performs no write at all when the masked bit already reads back set,
and it writes back whatever other directive bits the read returned.

If the command bit does not self-clear, does the latch here silently not
happen, and likewise the disarm latch below:

	if (!ret)
		ret = sit9531x_update_pll_u8(sitdev, pll_idx,
					     SIT9531X_PLL_REG_SMALL_UPDATE,
					     SIT9531X_SMALL_UPDATE_CMD,
					     SIT9531X_SMALL_UPDATE_CMD);

leaving the on-demand flush armed, which the comment there says the one-shot
sequence must not do?

The rest of the driver issues this command with a plain write, in
sit9531x_prio_prg_commit() and in dpll.c via sit9531x_write_pll_u8(...,
SIT9531X_PLL_REG_SMALL_UPDATE, SIT9531X_SMALL_UPDATE_CMD). Should this site
follow the same convention?

> +	if (rc)
> +		goto disarm;

[ ... ]

> +static int sit9531x_output_divo_calc(struct sit9531x_dev *sitdev, u8 out_idx,
> +				     u8 pll_idx, u64 frequency, u64 *fvco_out,
> +				     u64 *divo_out)
> +{

[ ... ]

> +	/* Determine VCO frequency band limits */
> +	if (pll_idx == 1 || pll_idx == 3) {
> +		/* PLLB, PLLD: high band */
> +		fvco_min = SIT9531X_FVCO_HIGHBAND_MIN;
> +		fvco_max = SIT9531X_FVCO_HIGHBAND_MAX;
> +	} else {
> +		/* PLLA, PLLC: low band */
> +		fvco_min = SIT9531X_FVCO_LOWBAND_MIN;
> +		fvco_max = SIT9531X_FVCO_LOWBAND_MAX;
> +	}
> +

[ ... ]

> +	rc = sit9531x_get_fvco(sitdev, pll_idx, &fvco);
> +	if (rc)
> +		return rc == -ENODATA ? -ENODEV : rc;
> +	if (!sitdev->pll_fvco[pll_idx]) {
> +		if (fvco < fvco_min)
> +			fvco = fvco_min;
> +		else if (fvco > fvco_max)
> +			fvco = fvco_max;
> +	}

[Severity: Medium]
Do the set and get paths disagree once this clamp fires?

sitdev->pll_fvco[] is zero on every board today, so any Fvco derived from
DIVN that falls outside the band picked from the PLL index parity is
replaced by the band edge, and both the divider and the cached rate come
from the fabricated value:

	sitdev->out[out_idx].freq = div64_u64(fvco, divo);

sit9531x_output_freq_get() applies no clamp:

	rc = sit9531x_get_fvco(sitdev, pll_idx, &fvco);
	...
	*frequency = div64_u64(fvco, divo);

So a 6 GHz derived VCO on a low-band PLL is treated as 5.89824 GHz on set
and 6 GHz on get, and the set returns success with nothing logged. The
comment a few lines above states "A VCO that cannot be read fails the
request: programming a divider from a guessed rate would put the output far
from what was asked for while reporting success" -- is clamping to a band
edge not also programming from a guessed rate?

> +
> +	divo = div64_u64(fvco, frequency);
> +	if (!divo)
> +		return -EINVAL;

[Severity: High]
Should a request that the divider cannot represent be rejected or rounded?

This is a truncating division, and the only checks are that the divider is
non-zero and fits the 34-bit field, so the realized rate fvco/floor(fvco/f)
is always above the request with no bound on the error.

With Fvco 4.9152 GHz, a 156250000 Hz request gives DIVO 31, i.e. 158.548
MHz, about 1.5% high. A 1000000000 Hz request gives DIVO 4, i.e. 1.2288
GHz, 23% high and above the 1 GHz maximum the pin itself advertises. With
Fvco 5.89824 GHz, a 100000000 Hz request gives DIVO 58 (101.69 MHz) even
though DIVO 59 would yield 99.97 MHz, so flooring picks the worse of the two
adjacent dividers.

These values reach the driver because prop.c advertises a continuous range
for outputs that list no supported frequencies:

drivers/dpll/sit9531x/prop.c:sit9531x_pin_props_get() {
	if (dir == DPLL_PIN_DIRECTION_OUTPUT && num_freqs == 0) {
		ranges[j].min = 1;
		ranges[j].max = 1000000000ULL; /* 1 GHz */
	...
}

and dpll_pin_freq_set() accepts any value inside an advertised range.

Would rounding to the nearest achievable divider plus a representability or
tolerance check (or refusing the request) be preferable to reporting success
for a rate the part is not running?

> +
> +	/*
> +	 * DIVO is a 34-bit field.  With a band-clamped Fvco this cannot
> +	 * overflow, but a DT Fvco override is taken verbatim, so guard the
> +	 * field width rather than silently truncating the divider.
> +	 */
> +	if (divo > SIT9531X_DIVO_MAX)
> +		return -EINVAL;

[ ... ]

> +int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
> +			     u8 pll_idx, u64 frequency)
> +{

[ ... ]

> +	ret = sit9531x_prg_commit(sitdev);
> +	if (ret && !rc)
> +		rc = ret;
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Step 5: flush the PLL's output phase so the new DIVO starts
> +	 * aligned instead of keeping the arbitrary phase the divider
> +	 * happened to be at.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
> +	if (rc)
> +		return rc;
> +
> +	sitdev->out[out_idx].freq = div64_u64(fvco, divo);

[Severity: Medium]
When the flush fails, the divider has already been committed, so the part
runs at the new rate while userspace is told the set failed, and
sitdev->out[out_idx].freq still holds the old rate. Any of the six register
accesses in sit9531x_output_phase_flush() can produce that error.

Can a retry recover the skipped phase realignment?

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	ret = ops->frequency_get(... &old_freq, extack);
	...
	if (freq == old_freq)
		return 0;
	...
}

The readback now returns the committed divider, so an identical retry never
re-enters frequency_set and the flush never runs -- which per the comment in
sit9531x_output_phase_flush() leaves the output with an offset that only a
power cycle used to clear.

Should the previous DIVO be restored on a flush failure, or the cache be
updated before the flush is attempted so the reported rate matches the
hardware?

> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index e2ff21adfdb23..88122a368053c 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -644,8 +668,78 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,
>  	return 0;
>  }
>  

[ ... ]

>  static const struct dpll_pin_ops sit9531x_dpll_output_pin_ops = {
>  	.direction_get		= sit9531x_dpll_output_pin_direction_get,
> +	.frequency_get		= sit9531x_dpll_output_pin_frequency_get,
> +	.frequency_set		= sit9531x_dpll_output_pin_frequency_set,
>  };

[Severity: Low]
This isn't a bug, but at this commit no output pin is registered:
sit9531x_dpll_pin_is_registrable() still starts with

	if (dir != DPLL_PIN_DIRECTION_INPUT)
		return false;

so these two callbacks are not dispatchable until the following patch in the
series rewrites that helper. The commit message describes the output path,
including "a frequency set on this device delays netlink traffic for every
DPLL in the system for that long", as if it were already live. Could the
message note that output pins become reachable later in the series?

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