Re: [PATCH v10 02/14] dt-bindings: dpll: add SiTime SiT95316 clock generator

[Ali Rouhi <arouhi@sitime.com>]

On Fri, 25 Sep 2026, Jakub Kicinski wrote:
> [Severity: Medium]
> Do clock-frequency, sitime,pll-fvco and sitime,output-pll-map describe
> the hardware, or what the Linux driver and clock framework do today?
>
> Documentation/devicetree/bindings/writing-bindings.rst says:
>
>   DON'T refer to Linux or "device driver" in bindings. Bindings should be
>   based on what the hardware has, not what an OS and driver currently
>   support.
>
> Both vendor properties copy state that the chip already holds in its
> programmed configuration, and they would become permanent DT ABI.

Both properties are gone in v11, along with the patch that added
them. That is part of why the series is 13 patches rather than 14.

sitime,output-pll-map described routing the device can be asked for.
Each PLL page holds OUT_MAP_LO/OUT_MAP_HI naming the slots that PLL
drives, the driver already reads them, and it never writes them, so the
routing is fixed by the configuration loaded from NVM at boot and no
DPLL call changes it. The property was kept on the strength of a claim
that those bitmaps can be ambiguous; they are not, and device tree is
for what firmware cannot discover.

sitime,pll-fvco is gone for the same reason. The driver derives the VCO
frequency from the registers, and that derivation reproduces every rate
these parts are configured for. Keeping a property to override a value
the device already answers for is the same mistake in a different
place.

Dropping the map exposed a bug it had been masking, since the register
scan is now the only path to the association: on PLLC and PLLD the
OUT_MAP_LO/OUT_MAP_HI bit order is reversed relative to PLLA and PLLB.
That is fixed in v11 and described in the cover letter.

clock-frequency stays, and the description no longer refers to Linux or
to the driver. It is not an alternative to the clock framework but to
firmware that has no clock provider to describe the oscillator with,
which is the ACPI case; there is no fixed-clock node to point at.

> Can a static sitime,pll-fvco value stay correct at runtime? Later in the
> series, "dpll: sit9531x: model the inter-PLL sync net as a pair of pins"
> lets userspace switch INTSYNC through netlink.

Moot with the property gone, but the answer was no.

> The reasons given also don't match. This binding cites INTSYNC mode. The
> later commit "dpll: sit9531x: allow the device tree to override two board
> facts" cites "free-run with a divider the configuration never programmed".
> Which case is the override meant for?

Neither, in the end. That the two justifications disagreed was the
clearest sign the property was describing our uncertainty rather than
the hardware.

> [Severity: Low]
> Is a node that has both clocks and clock-frequency supposed to pass this
> oneOf?

No, and it did. v11 adds:

    dependencies:
      clocks: [clock-names]

so clock-names is required whenever clocks is present. A node carrying
both clocks and clock-frequency then matches both oneOf branches and
fails validation, which makes the two mutually exclusive as intended.

> [Severity: Low]
> This isn't a bug introduced by this patch, but the new binding inherits a
> limitation from the shared schema. In dpll-device.yaml, input-pins
> children match:
>
>     "^pin@[0-9a-f]+$":
>
> output-pins children match:
>
>     "^pin@[0-9]+$":
>
> Can outputs 10 and 11 be described with the usual hex unit-address names?
> The only way to pass validation seems to be pin@10 with reg = <10>, which
> breaks the hex convention. microchip,zl30731.yaml (20 single-ended outputs)
> has the same gap. Should the output-pins pattern in dpll-device.yaml be
> changed to match the input-pins one?

Yes. That is patch 1 of v11, "dt-bindings: dpll: allow hex unit
addresses on output pins", carrying

Fixes: 0afcee10dda1 ("dt-bindings: dpll: Add DPLL device and pin")

It comes first so that no commit in the series leaves a binding that
cannot validate. The example in this patch now includes pin@a, so the
series exercises the pattern it changes rather than only depending on
it.

> [Severity: Medium]
> What do the reg values of input-pins and output-pins children mean on
> these parts, and which values are valid for each variant?
>
> Could the binding document the reg numbering and add reg limits for each
> variant?

Both are in v11.

The numbering is stated in the description. An input pin's reg is its
lane: 2k is CLKINkP and 2k+1 is CLKINkN. An output pin's reg is its
physical OUTn, and the SiT95317 bonds out a subset of the slots rather
than renumbering them, which is the mismatch you found between the pin
reg space and the dense logical index the dropped map used.

reg is now bounded per compatible, so dt_binding_check rejects the
cases you listed at review time instead of the driver discarding the
node silently:

    input-pins   reg maximum 7, the same on both parts
    output-pins  reg maximum 11 on the SiT95316
                 reg enum [0, 3, 4, 5, 7, 8, 9, 11] on the SiT95317

pin@1, pin@2, pin@6 and pin@a no longer validate on a SiT95317.

This patch is patch 3 of v11.

Ali