Re: [PATCH net-next v9 04/15] dpll: sit9531x: read DPLL types and pin properties from system firmware

On 9/17/26 11:42 AM, Ivan Vecera wrote:

quoted

+	/*
+	 * The fine step is 30 ps, but requests are accepted at 1 ps
+	 * resolution and rounded to the nearest achievable delay, so
+	 * advertise the request granularity, not the hardware step.
+	 */
+	props->dpll_props.phase_gran = 1;

What is the real HW granularity? If 30ps then you should announce 30 to 
inform the userspace that this is the step supported by the HW.

There is no single hardware step on this part, and phase-gran is not
only advisory: the core rejects any request that is not a multiple of
it, in dpll_pin_phase_adjust_set().

  if (pin->prop.phase_gran && phase_adj % (s32)pin->prop.phase_gran) {
          NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
                                  "phase adjust value not multiple of %u",
                                  pin->prop.phase_gran);
          return -EINVAL;
  }

So whatever the driver announces becomes a filter on what userspace is
allowed to ask for, and that is what makes 30 the wrong number here.

A requested delay is split between two fields that are added, not
selected between: a coarse delay counted in whole VCO cycles, and a
three-bit fine field in fixed 30 ps steps. The reachable delays are

  coarse * T_vco + fine * 30 ps,   fine in 0..7

where T_vco is the VCO period in force. For 30 to be the right modulus,
T_vco would have to be an exact multiple of 30 ps. It is not. The
feedback divider is fractional, so Fvco is XO * (DIVN_INT + NUM/DEN)
times the doubler, and the period that falls out of that does not land
on a 30 ps boundary. As soon as one coarse cycle is part of a delay,
the reachable values are offset by that remainder and stop being
multiples of 30 at all.

Take T_vco = 240.5 ps purely as arithmetic. The reachable delays are
0, 30 ... 210, then 240.5, 270.5, and so on. Announcing 30 against that
set would be wrong in both directions at once: the core would reject
240.5, which the device produces exactly, and accept 240, which it
cannot produce at all. A faster or slower VCO does not fix this, it
only moves where the mismatch starts, and Fvco differs per PLL and per
board.

Announcing T_vco instead would at least describe the coarse field, but
it discards the fine one: the core would then refuse every delay that
is not a whole number of VCO cycles, and the 30 ps resolution inside a
cycle becomes unreachable through the ABI. It is not exact either,
since phase-gran is an integer number of picoseconds and T_vco is not.

So the driver announces 1, accepts the request, rounds it to the
nearest delay the two fields can represent, and reports back what the
registers hold rather than what was asked for. A caller that needs the
exact applied value reads it back and gets the truth for its own VCO
rate.

I recognize this leaves phase-gran carrying less information than it
does for zl3073x, where one output period is a genuine step and the
readback scales by it. If you would rather the attribute describe the
coarse field even at the cost of the fine one, I will announce T_vco
instead. I did not want to close off the resolution inside a VCO cycle
in order to make a single number look tidy.

This is unchanged in v10, with the reasoning set out in the cover
letter:
https://lore.kernel.org/netdev/20260921201108.42676-1-arouhi@sitime.com/ (local)

Thanks,
Ali