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

[netdev-bot+sashiko]

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

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

- [Medium] In `sit9531x_pin_props_get()` (drivers/dpll/sit9531x/prop.c,
  output branch) every output pin is advertised to the DPLL core with…
- [Medium] `sit9531x_pin_props_get()` (drivers/dpll/sit9531x/prop.c)
  reinterprets the first entry of the DT property `supported-…
- [Medium] `sit9531x_prop_pin_package_label_set()` builds the DPLL
  package label for outputs as `snprintf(..., "OUT%u", id)` from the…
- [Low] In `sit9531x_pin_props_get()` the firmware frequency is held in
  `u64 curr_freq` (assigned `curr_freq = freqs[0]` straight from…

--

dpll: sit9531x: read DPLL types and pin properties from system firmware

This patch adds prop.c, which builds the per-pin dpll_pin_properties
(package label, type, capabilities, supported frequencies) and the per-channel
DPLL type from the firmware node, with defaults when the node is absent.  It
also adds the per-input and per-output runtime state arrays to struct
sit9531x_dev.

A few questions about the property parsing below.

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> index 76a2632f0ce4a..230b21b9e2381 100644
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h

[ ... ]

> @@ -41,6 +51,39 @@ struct sit9531x_chip_info {
>  	const u8	*clkout_map;
>  };
>  
> +/*
> + * struct sit9531x_ref - input reference state
> + * @freq:		configured frequency in Hz
> + * @label:		board label from DT or default
> + * @sig_mode:		signal mode of the pair this lane belongs to
> + *			(detected from CLKINx_INPUT_MODE at probe)
> + */
> +struct sit9531x_ref {
> +	u32				freq;
> +	const char			*label;
> +	enum sit9531x_signal_mode	sig_mode;
> +};
> +
> +/*
> + * struct sit9531x_out - output state
> + * @freq:		configured frequency in Hz
> + * @label:		board label from DT or default
> + */
> +struct sit9531x_out {
> +	u32		freq;
> +	const char	*label;
> +};
> +

[ ... ]

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 0000000000000..c9642b702c4ab
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c
> @@ -0,0 +1,437 @@

[ ... ]

> +	if (dir == DPLL_PIN_DIRECTION_INPUT)
> +		sit9531x_input_pin_label(sitdev, props, id);
> +	else
> +		snprintf(props->package_label, sizeof(props->package_label),
> +			 "OUT%u", id);

[Severity: Medium]
Should the output label go through sitdev->info->clkout_map[] rather than the
logical index?

drivers/dpll/sit9531x/core.c has a non-identity map for one variant:

    static const u8 clkout_map_95317[] = {0, 3, 4, 5, 7, 8, 9, 11};

and core.h documents it as "@clkout_map: per-output slot mapping (output
index -> physical slot)", with sit9531x_output_divo_write() using it for the
actual register access.  On SiT95317 that makes logical index 1 program
physical slot 3, but userspace is shown DPLL_A_PIN_PACKAGE_LABEL "OUT1", and
"OUT10"/"OUT11" are never emitted even though those chip pins exist.

The same logical index is used as the firmware lookup key in
sit9531x_prop_pin_fwnode_get():

    if (fwnode_property_read_u32(pin_node, "reg", &reg))
            continue;

    if (id == reg)
            break;

while Documentation/devicetree/bindings/dpll/dpll-pin.yaml describes reg as
"Hardware index of the DPLL pin".  A board DT written against the binding
(pin@3, pin@7, pin@b) then matches nothing, and pin@1 has its label,
connection-type, esync-control and frequency list applied to physical slot 3.

Should prop.c translate through clkout_map, or should the sitime binding
state that reg is a logical, variant-dependent index?

[ ... ]

> +	} else {
> +		/*
> +		 * A synthesized clock output is an external connection with
> +		 * no more specific meaning; a board that knows better says
> +		 * so through the pin's connection-type property below.
> +		 */
> +		props->dpll_props.type = DPLL_PIN_TYPE_EXT;
> +		props->dpll_props.capabilities =
> +			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
> +		curr_freq = sitdev->out[index].freq;
> +
> +		/*
> +		 * Allow phase-adjust over a +/-1 ms window.  The subsystem
> +		 * rejects pin_set(phase-adjust, X) when X falls outside
> +		 * [min, max], so leaving these at 0 silently blocks every
> +		 * netlink call.  1 ms is well beyond the DCO dynamic range
> +		 * but costs nothing.  Only outputs get a range: input pins
> +		 * have no .phase_adjust_set, and advertising one there would
> +		 * promise userspace something every set would refuse.
> +		 */
> +		props->dpll_props.phase_range.min = -1000000000; /* -1 ms in ps */
> +		props->dpll_props.phase_range.max =  1000000000; /* +1 ms in ps */
> +		/*
> +		 * 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;
> +	}

[Severity: Medium]
Can these advertised values mislead userspace, given they are uAPI once
released?

dpll_pin_phase_adj_set() in drivers/dpll/dpll_netlink.c validates a request
only against the driver-supplied range and granularity:

    if (phase_adj > pin->prop.phase_range.max ||
        phase_adj < pin->prop.phase_range.min) {

So every integer-picosecond request up to 1 ms is accepted, and
sit9531x_output_phase_adjust_set() then folds an offset larger than one
output period back into a single period and rounds to the 30 ps step.  A
request of 500000 ps on a 1 GHz output is applied as a few hundred ps and
phase_adjust_get() reports that different value back.

Documentation/netlink/specs/dpll.yaml describes
DPLL_A_PIN_PHASE_ADJUST_GRAN as "The value of phase adjustment must be a
multiple of this granularity", so is phase_gran = 1 the right thing to
publish when the real step is 30 ps?

Related, further down in the same function:

    if (dir == DPLL_PIN_DIRECTION_OUTPUT && num_freqs == 0) {
            ranges[j].min = 1;
            ranges[j].max = 1000000000ULL; /* 1 GHz */

For an output whose firmware node lists no supported-frequencies-hz this
claims 1 Hz to 1 GHz and effectively disables the core-side frequency_set
validation, so the advertised capability follows DT completeness rather than
the silicon.  For comparison, zl3073x derives phase_gran from the synth rate
and filters DT frequencies through zl3073x_pin_check_freq().  The commit
message does not mention either the phase-adjust window or this fallback
range; could it describe them?

[ ... ]

> +	/*
> +	 * Seed the runtime ref->freq / out->freq with the first DT-listed
> +	 * supported frequency so the netlink frequency_get callback reports
> +	 * a sane initial value before any pin_set occurs.  DT lists the
> +	 * physically-wired reference frequency for each input pin and the
> +	 * default output frequency for each output pin.
> +	 */
> +	if (num_freqs > 0) {
> +		if (dir != DPLL_PIN_DIRECTION_INPUT ||
> +		    index != SIT9531X_MAX_INPUTS)
> +			curr_freq = freqs[0];
> +	}

[Severity: Medium]
Is element 0 of supported-frequencies-hz allowed to mean "current
frequency"?

Documentation/devicetree/bindings/dpll/dpll-pin.yaml defines the property as:

    supported-frequencies-hz:
        description: List of supported frequencies for this pin, expressed in Hz.

with no special meaning for the first entry.  The other consumer of the same
binding, drivers/dpll/zl3073x/prop.c, reads curr_freq from the hardware and
uses the DT list only to build freq_supported:

    /* Start with current frequency at index 0 */
    ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);

For a board that legitimately lists several supported frequencies, this
invents a current rate.  The cached value is then reported verbatim by
sit9531x_dpll_input_pin_frequency_get() and used as the output period in
sit9531x_output_phase_adjust_set() ("freq = sitdev->out[out_idx].freq;"), so
a seeded rate that differs from what the chip is generating yields a wrong
programmed delay.

> +skip_fwnode_props:

[ ... ]

> +	/*
> +	 * Publish the seeded rate only once the pin is certain to be built.
> +	 * The allocation above is the last thing that can fail, and a call
> +	 * that reports failure must not leave the device's cached rate
> +	 * changed behind it.
> +	 */
> +	if (curr_freq) {
> +		if (dir == DPLL_PIN_DIRECTION_INPUT)
> +			sitdev->ref[index].freq = curr_freq;
> +		else
> +			sitdev->out[index].freq = curr_freq;
> +	}

[Severity: Low]
Does this truncate curr_freq?

curr_freq is a u64 taken straight from fwnode_property_read_u64_array() and
published at full width through DPLL_PIN_FREQUENCY(), but sit9531x_ref.freq
and sit9531x_out.freq are u32 in core.h.  A firmware entry of 2^32 Hz or more
is stored truncated while the advertised freq_supported entry keeps the full
value, so the cached rate matches nothing in the advertised set; an exact
multiple of 2^32 caches as 0, which also affects the period used by
sit9531x_output_phase_adjust_set().

zl3073x_pin_props_get() runs every DT entry through
zl3073x_pin_check_freq(); would a similar bound check fit here?

> +
> +	j = 0;

[ ... ]

> +	if (dir == DPLL_PIN_DIRECTION_OUTPUT && num_freqs == 0) {
> +		ranges[j].min = 1;
> +		ranges[j].max = 1000000000ULL; /* 1 GHz */
> +		j++;
> +	}

[ ... ]

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