Re: [PATCH v10 14/14] dpll: sit9531x: allow the device tree to override two board facts

[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: allow the device tree to override two board facts

This patch adds two optional device tree overrides to the sit9531x driver.
"sitime,pll-fvco" replaces the VCO frequency derived from DIVN for each
PLL. "sitime,output-pll-map" replaces the output-to-PLL routing that is
normally read from the OUT_MAP registers.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index e8d47999f1a97..5316e6bcb338c 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -3238,6 +3241,27 @@ static int sit9531x_out_state_fetch(struct sit9531x_dev *sitdev, u8 index)
>  
>  	sitdev->out[index].state_stale = false;
>  
> +	/*
> +	 * DT board-config override: the per-PLL OUTPUT_ENABLE bitmaps
> +	 * (0x27/0x28) do not unambiguously express output->PLL routing on
> +	 * every config (overlaps, and some outputs routed outside that
> +	 * path).  When the board supplies an explicit map, trust it.
> +	 */
> +	if (sitdev->out_pll_map_valid) {
> +		u8 m = sitdev->out_pll_map[index];

[Severity: Medium]
Which index namespace is out_pll_map[] meant to use?

Here the map is indexed by the driver's packed logical output index. The
register path further down in the same function uses the physical slot:

	slot = sitdev->info->clkout_map[index];

The other firmware-facing output code also uses the physical slot.
sit9531x_prop_pin_fwnode_get() in prop.c translates before it matches
output-pins/pin@reg:

	id = sitdev->info->clkout_map[id];

sit9531x_prop_pin_label_init() also names package labels by slot:

	"OUT%u", sitdev->info->clkout_map[id]);

On SiT95317 the mapping is not contiguous:

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

So map entry 1 describes OUT3 and entry 4 describes OUT7. Seven of the
eight positions don't match OUTn numbering.

The binding text for sitime,output-pll-map only says "for each output
0..11". It doesn't say that SiT95317 entries are packed logical indices.

Suppose a board author numbers the map by OUTn, as pin@reg and the labels
suggest. Could the routing then be applied to the wrong outputs without
any warning?

An 8- or 12-entry map like that passes the value checks in
sit9531x_parse_board_config(). It also passes the length check in
sit9531x_dev_probe(), because the count is not less than num_outputs.

The misrouted out->pll_idx then picks the wrong PLL's Fvco in
sit9531x_get_fvco(). That affects the frequency get, frequency set and
phase adjust paths. The low and high VCO bands are disjoint, so the error
would be large. A 255 entry in the wrong position would also unregister
the wrong pin.

This binding is new in this series, so should the indexing be settled
before DTs ship with it? The map could be indexed by physical slot like
pin@reg, or the binding could document the SiT95317 packing explicitly.

> +
> +		if (m < SIT9531X_NUM_PLLS) {
> +			out->pll_idx = m;
> +			out->routed = true;
> +			out->enabled = !muted;
> +		} else {
> +			out->pll_idx = 0;
> +			out->routed = false;
> +			out->enabled = false;
> +		}
> +		return 0;
> +	}

[Severity: Low]
This isn't a bug, but the header comment of sit9531x_out_state_fetch() is
now stale. It still says:

 * Reads the output PLL association from the PLL page output map
 * registers into out->routed / out->pll_idx, ...

With out_pll_map_valid set, pll_idx and routed come from the DT map and
the OUT_MAP_LO/HI registers are never read. The comment for
sit9531x_get_fvco() was updated for its override path in this patch.
Could this one be updated too?

> +
>  	/*
>  	 * The OUT_MAP_LO/HI bitmaps are indexed by the physical slot the
>  	 * output occupies on the chip, not by the driver's logical output

[ ... ]

> +		/*
> +		 * The override is used verbatim by the divider math, so an
> +		 * implausible value (units typo, wrong cell count worked
> +		 * around with zeros) must not silently misprogram DIVO.
> +		 * Anything outside both VCO bands is dropped with a warning
> +		 * rather than trusted.
> +		 */
> +		for (i = 0; i < SIT9531X_NUM_PLLS; i++) {
> +			u64 f = sitdev->pll_fvco[i];
> +
> +			if (f && !sit9531x_fvco_in_band(i, f)) {
> +				dev_warn(sitdev->dev,
> +					 "PLL%c Fvco override %llu Hz is outside the band that PLL runs in, ignoring\n",
> +					 'A' + i, f);
> +				sitdev->pll_fvco[i] = 0;
> +			}
> +		}

[Severity: Low]
Should the binding describe the per-PLL range enforced here?

The binding declares sitime,pll-fvco as a 4-item uint64-array. It has no
per-item minimum or maximum and no documented range, and says only that 0
keeps the register-derived value.

This loop zeroes any non-zero entry that fails sit9531x_fvco_in_band().
That check requires 6.875 to 7.8125 GHz for PLLB/PLLD and 4.9152 to
5.89824 GHz for PLLA/PLLC.

A DT can pass dt_binding_check and still have its override dropped at
probe. Examples are a PLLA value in the high band, or a value in the gap
between the bands. The driver then falls back to the DIVN derivation that
the override was meant to replace.

The binding example of 6875000000 for PLLB sits exactly on
SIT9531X_FVCO_HIGHBAND_MIN. It is accepted only because the check is
inclusive.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h
> @@ -224,6 +224,7 @@ struct sit9531x_dev {
>  	u64			pll_fvco[SIT9531X_NUM_PLLS];
>  	u8			out_pll_map[SIT9531X_MAX_OUTPUTS];
>  	bool			out_pll_map_valid;
> +	u8			out_pll_map_count;
>  
>  	/* Inter-PLL synchronization state */
>  	s8			intsync_src;

[Severity: Low]
This isn't a bug, but the kernel-doc block above struct sit9531x_dev
wasn't updated for the new out_pll_map_count member. Running
scripts/kernel-doc over core.h will warn about the missing
@out_pll_map_count description.

A short description would also help the reader. The name alone doesn't
say what the member counts.

sit9531x_parse_board_config() fills it with the raw length of the
sitime,output-pll-map property. That happens before the variant is
known. The value only means something while out_pll_map_valid is set.
sit9531x_dev_probe() then compares it against info->num_outputs to
decide whether to drop the map.

Could a one-line @out_pll_map_count entry be added that says this?