Re: [PATCH v10 04/14] dpll: sit9531x: read DPLL types and pin properties from system firmware

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: sit9531x: read DPLL types and pin properties from system firmware

This commit adds prop.c, which reads the DPLL type for each channel and
the properties for each pin from the firmware node. The pin properties
are the package label, board label, type, capabilities and supported
frequencies, with defaults when the node does not describe them. It also
adds the input and output state structures to core.h, plus helpers that
map a logical input index to a pair and a lane.

> Two of the advertised properties are worth naming.  Outputs get a
> phase-adjust window of one millisecond either way, which is wider than
> the dynamic range but costs nothing and is what keeps the subsystem from
> refusing every request; the granularity is one picosecond, because the
> achievable delays are whole VCO cycles plus thirty-picosecond steps and
> so form no uniform lattice for the core to check against.

[Severity: Low]
Is this paragraph describing code that is in this patch? At this commit,
nothing under drivers/dpll/sit9531x/ sets dpll_props.phase_range or
dpll_props.phase_gran. The output branch of sit9531x_pin_props_get()
sets only type, capabilities and curr_freq.

The phase window, and a comment giving the same reasons, arrive later in
"dpll: sit9531x: add support to adjust output phase". The v10 notes say
the window was moved to that patch, but this paragraph was left here.
Could it move to that patch's commit message?

A smaller mismatch of the same kind is the INTSYNC "re-prioritised"
comment in sit9531x_pin_props_get(), noted further down.

> 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;

[Severity: Low]
Can this u32 truncate the u64 curr_freq that sit9531x_pin_props_get()
stores here and in sit9531x_out::freq? A rate of 2^32 Hz or more would
be cached truncated, while the full value is advertised in
freq_supported.

This can't be reached in any tree state. The next patch, "dpll:
sit9531x: register DPLL devices and pins", adds the first caller and
widens both fields to u64 in the same change. Could they simply be u64
from this patch?

> +	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;
> +};

[Severity: Low]
Is anything meant to write sit9531x_ref::label and sit9531x_out::label?
Both are documented as "board label from DT or default", but nothing in
the driver assigns them, either here or at the end of the series.

The DT label goes into props->dpll_props.board_label in
sit9531x_pin_props_get(). The package label goes into
props->package_label. Could these fields be dropped, or their
documentation corrected?

sig_mode is also never set at this commit, so the differential "IN%u"
branch of sit9531x_input_pin_label() cannot be reached yet. That part is
fixed by sit9531x_input_mode_fetch() in "dpll: sit9531x: register DPLL
devices and pins", which runs before any pin reaches this code.

[ ... ]

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

[ ... ]

> +static int
> +sit9531x_prop_pin_fwnode_get(struct sit9531x_dev *sitdev,
> +			     struct sit9531x_pin_props *props,
> +			     enum dpll_pin_direction dir, u8 id)
> +{
> +	struct fwnode_handle *pins_node, *pin_node;
> +	const char *node_name;
> +
> +	if (dir == DPLL_PIN_DIRECTION_INPUT) {
> +		node_name = "input-pins";
> +	} else {
> +		node_name = "output-pins";
> +		if (id < sitdev->info->num_outputs)
> +			id = sitdev->info->clkout_map[id];
> +	}

[ ... ]

> +	/* Enumerate child pin nodes and find the requested one */
> +	fwnode_for_each_child_node(pins_node, pin_node) {
> +		u32 reg;
> +
> +		if (fwnode_property_read_u32(pin_node, "reg", &reg))
> +			continue;
> +
> +		if (id == reg)
> +			break;
> +	}

[Severity: Medium]
How is a DT author expected to number reg in these nodes? Two different
numberings are used here. Neither dpll-pin.yaml nor sitime,sit95316.yaml
defines either one.

For inputs, reg is compared directly with the interleaved lane index:
2k is INkP and 2k+1 is INkN. The binding only says there are "4
differential input pairs whose lanes can also be driven independently as
single-ended references, so 8 inputs are individually selectable".

A differential pair k is registered only as index 2k, because
sit9531x_input_pin_is_registrable() skips the N lane. Say a board
describes differential IN1 with reg = <1>. Would its label,
connection-type and supported-frequencies-hz be applied to IN0N when
pair 0 is single-ended? And would the node be silently ignored when
pair 0 is differential?

The input pin@1 in the binding example ("clkin1", synce) only works if
pair 0 is strapped single-ended.

For outputs, the logical index goes through clkout_map[] first. On
SiT95317 that means reg has to be one of 0, 3, 4, 5, 7, 8, 9 or 11.

In the same device node, sitime,output-pll-map is indexed by logical
output number. The binding describes it as "for each output 0..11" and
caps it at 8 items for SiT95317. sit9531x_parse_board_config() stores it
by logical index:

drivers/dpll/sit9531x/core.c:sit9531x_parse_board_config() {
    ...
	for (i = 0; i < n; i++)
		sitdev->out_pll_map[i] = map[i];
    ...
}

So on SiT95317, output-pll-map entry 1 refers to the pad labelled OUT3,
but output-pins reg = <1> refers to a slot that is not bonded out. A
node written like the output pin@1 in the binding example would match
nothing. That output would then silently get the 1 Hz to 1 GHz fallback
range instead of the board's list.

Could the binding document both numberings? Could the driver also warn
when a pin node's reg matches no registered pin?

[ ... ]

> +	if (dir == DPLL_PIN_DIRECTION_INPUT &&
> +	    index == SIT9531X_INTSYNC_PIN_ID) {
> +		/*
> +		 * INTSYNC destination pin: a PLL locks to the INTSYNC net as a
> +		 * reference, so it can be connected and re-prioritised.
> +		 */
> +		props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR;
> +		props->dpll_props.capabilities =
> +			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;

[Severity: Low]
The comment says this pin can be re-prioritised, but the mask only has
DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE. The physical input branch below
uses the same mask. With this mask, wouldn't the DPLL core reject every
priority change on these pins?

Nothing is registered at this commit and there are no prio ops yet.
"dpll: sit9531x: add support to get and set priority on input pins" adds
DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE to both the INTSYNC
destination pin and the physical inputs, so the series fixes this
later. Could the comment and the capability land in the same patch?

[ ... ]

> +	/*
> +	 * 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: Low]
For output pins, this replaces curr_freq, taken from
sitdev->out[index].freq, with the first supported-frequencies-hz entry.
That value is then cached back into sitdev->out[index].freq further
down. Doesn't that contradict the v10 note "Took an output's current
rate from its divider instead of the first entry of the list of rates
the board supports"?

No output pin ever reaches this code while it behaves this way:

- sit9531x_pin_props_get() has no caller at this commit.
- The next patch registers only inputs and XO.
- "dpll: sit9531x: add support to get and set frequency on pins" limits
  the seeding to inputs and reads outputs back through
  sit9531x_output_freq_get(), before any output pin is registered.

For inputs, taking the first entry as the wired rate is intentional,
according to that later patch. The convention is not written down in
dpll-pin.yaml, though. Should the binding mention it?

[ ... ]