Re: [PATCH v10 08/11] clk: realtek: Add support for MMC-tuned PLL clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `clk_pll_mmc_determine_rate` and `clk_pll_mmc_set_rate` violate Common Clock Framework guarantees by computing mathematically inconsistent rates and completely ignoring the requested rate.
- [Medium] Integer overflow in `clk_pll_mmc_determine_rate()` causes severely miscalculated clock rates on 32-bit platforms.
--

--- Patch [8]: [PATCH v10 08/11] clk: realtek: Add support for MMC-tuned PLL clocks ---
commit 227763092de853595ae404163ed7dc1dfbbc5558
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add support for MMC-tuned PLL clocks

Add clk_pll_mmc_ops for enable/disable, prepare, rate control, and status
operations on MMC PLL clocks.

Also add clk_pll_mmc_phase_ops to support phase get/set operations.

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

[ ... ]

> +static int clk_pll_mmc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	u32 val;
> +
> +	if (!req->best_parent_rate)
> +		return -EINVAL;
> +
> +	val = DIV_ROUND_CLOSEST_ULL((u64)req->rate * 4, req->best_parent_rate);
> +	val = clamp_t(u32, val, PLL_MMC_VAL_MIN, PLL_MMC_VAL_MAX);
> +	req->rate = req->best_parent_rate * val / 4;

[Severity: High]
Could this formula contradict the one in clk_pll_mmc_recalc_rate()?

clk_pll_mmc_recalc_rate() uses:

	base = parent_rate / 4;
	rate = base * (val + 2);
	rate += div_u64(base * ext_f, 8192);

But here in clk_pll_mmc_determine_rate(), it predicts the rate as
(parent_rate * val / 4).

This mathematical inconsistency could cause the clock framework to record
incorrect achievable rates, confusing consumer drivers like the eMMC controller
that rely on predictable clk_set_rate() and clk_get_rate() behavior.

[Severity: Medium]
Can this multiplication overflow on 32-bit platforms?

req->best_parent_rate is an unsigned long, and val is a u32. On 32-bit
architectures, their product will be truncated to 32 bits before the division
by 4, which would result in a severely miscalculated clock rate if the parent
rate and requested rate are sufficiently high.

> +
> +	return 0;
> +}
> +
> +static int clk_pll_mmc_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> +{
> +	struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> +	u32 val = PLL_MMC_SSC_DIV_N_VAL;
> +	int ret;
> +
> +	/*
> +	 * The 'rate' and 'parent_rate' are intentionally unused here.
> +	 *
> +	 * Despite receiving various rate requests (e.g., 26MHz, 52MHz, 200MHz),
> +	 * this function consistently configures the hardware for 27MHz (0x1b).
> +	 * This is because these settings reflect the input reference clock
> +	 * frequency to the SSCPLL, not the final PLL output frequency.
> +	 *
> +	 * The actual frequency division to achieve the requested eMMC rate
> +	 * is handled internally by the downstream eMMC host controller.
> +	 */

[Severity: High]
Is it expected to completely ignore the requested rate and unconditionally
program hardcoded values here?

Since clk_pll_mmc_determine_rate() calculates a val, but
clk_pll_mmc_set_rate() ignores it and always writes val=27 and ext_f=1517, this
violates Common Clock Framework expectations where clk_set_rate() should
configure the hardware to match the rate negotiated during determine_rate().

> +
> +	ret = regmap_update_bits(clkm->clkr.regmap,

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