Re: [PATCH v8 04/10] clk: realtek: Add support for phase locked loops (PLLs)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `clk_pll_determine_rate()` ignores consumer constraints (`req->min_rate` and `req->max_rate`), leading to invalid frequency selection and rejected clock rate changes.
- [High] `is_power_on()` incorrectly returns `true` when register reads fail, masking hardware errors and causing silent enablement failures.
- [Low] Unnecessary semicolon at the end of the `ftbl_find_by_val_with_mask` function definition.
--

commit 4d3f4fa427421504b7a5b08f5c9946c2979abab1
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add support for phase locked loops (PLLs)

Provide a full set of PLL operations for programmable PLLs and a read-only
variant for fixed or hardware-managed PLLs.

> diff --git a/drivers/clk/realtek/clk-pll.c b/drivers/clk/realtek/clk-pll.c
> new file mode 100644
> index 0000000000000..6c4343d71dbe4
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c

[ ... ]

> +static bool is_power_on(struct clk_pll *clkp)
> +{
> +	u32 val;
> +
> +	/* reg == 0 means not configured (assume always on).
> +	 * Register offset 0 is never a valid address on Realtek SoCs.
> +	 */
> +	if (!clkp->power_reg)
> +		return true;
> +
> +	if (regmap_read(clkp->clkr.regmap, clkp->power_reg, &val))
> +		return true;

[Severity: High]
Will this incorrectly mask hardware errors and cause silent enablement failures?

If regmap_read() fails, the function returns true. This false positive
causes clk_pll_enable() to assume the PLL is already powered on.

This skips the seq_power_on sequence and immediately returns success,
so the clock core framework considers the clock successfully enabled while
the physical hardware remains disabled.

> +
> +	return (val & clkp->power_mask) == clkp->power_val_on;
> +}

[ ... ]

> +static int clk_pll_determine_rate(struct clk_hw *hw,
> +				  struct clk_rate_request *req)
> +{
> +	struct clk_pll *clkp = to_clk_pll(hw);
> +	const struct freq_table *ftblv = NULL;
> +
> +	ftblv = ftbl_find_by_rate(clkp->freq_tbl, req->rate);

[Severity: High]
Does this ignore the req->min_rate and req->max_rate constraints?

The ftbl_find_by_rate() helper solely searches for the largest available
frequency less than or equal to req->rate.

If req->rate was clamped to min_rate by the core framework, the search may
select an entry strictly less than min_rate. The framework will subsequently
reject this out-of-bounds rate, even though a valid higher frequency
within the bounds might exist in the table.

> +	if (!ftblv)
> +		return -EINVAL;
> +
> +	req->rate = ftblv->rate;
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/clk/realtek/freq_table.c b/drivers/clk/realtek/freq_table.c
> new file mode 100644
> index 0000000000000..0ff1e5e79102b
> --- /dev/null
> +++ b/drivers/clk/realtek/freq_table.c

[ ... ]

> +const struct freq_table *
> +ftbl_find_by_val_with_mask(const struct freq_table *ftbl, u32 mask, u32 value)
> +{
> +	for (; !IS_FREQ_TABLE_END(ftbl); ftbl++) {
> +		if ((ftbl->val & mask) == (value & mask))
> +			return ftbl;
> +	}
> +	return NULL;
> +};

[Severity: Low]
this isn't a bug, but there is an unnecessary semicolon at the end of the
ftbl_find_by_val_with_mask() definition.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610080824.255063-1-eleanor.lin@realtek.com?part=4