Re: [PATCH v8 07/10] clk: realtek: Add support for MMC-tuned PLL clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `clk_pll_mmc_set_rate` entirely ignores dynamic rate parameters and hardcodes the PLL frequency.
- [Medium] `clk_pll_mmc_determine_rate` lacks bounds clamping and allows out-of-bounds rates that map to unsupported hardware configurations.
- [Low] The tristate `RTK_CLK_PLL_MMC` config causes `clk-pll-mmc.o` to be silently dropped from the build when `CONFIG_RTK_CLK_COMMON=y` and `CONFIG_RTK_CLK_PLL_MMC=m`.
--

commit 021382a87679f7b201df250be3c9547b24d2131a
Author: Cheng-Yu Lee <cylee12@realtek.com>

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

This commit adds clk_pll_mmc_ops for enable/disable, prepare, rate control,
and status operations on MMC PLL clocks. It also adds clk_pll_mmc_phase_ops
to support phase get/set operations.

> diff --git a/drivers/clk/realtek/Kconfig b/drivers/clk/realtek/Kconfig
> --- a/drivers/clk/realtek/Kconfig
> +++ b/drivers/clk/realtek/Kconfig
[ ... ]
> @@ -27,4 +27,7 @@ config RTK_CLK_COMMON
>  	  multiple Realtek clock implementations, and include integration
>  	  with reset controllers where required.
>  
> +config RTK_CLK_PLL_MMC
> +	tristate
> +
>  endif

[ ... ]

> diff --git a/drivers/clk/realtek/Makefile b/drivers/clk/realtek/Makefile
> --- a/drivers/clk/realtek/Makefile
> +++ b/drivers/clk/realtek/Makefile
[ ... ]
> @@ -7,3 +7,5 @@ clk-rtk-y += clk-pll.o
>  clk-rtk-y += clk-regmap-gate.o
>  clk-rtk-y += clk-regmap-mux.o
>  clk-rtk-y += freq_table.o
> +
> +clk-rtk-$(CONFIG_RTK_CLK_PLL_MMC) += clk-pll-mmc.o

[Severity: Low]
When CONFIG_RTK_CLK_COMMON is set to 'y' and CONFIG_RTK_CLK_PLL_MMC is set to
'm', does Kbuild drop clk-pll-mmc.o from the build entirely?

Since clk-rtk is a built-in object (obj-y) when CONFIG_RTK_CLK_COMMON=y,
conditionally appending to it via clk-rtk-m += clk-pll-mmc.o will cause
Kbuild to ignore the module object. This can lead to undefined reference
build errors when attempting to link against rtk_clk_pll_mmc_ops.

[ ... ]

> diff --git a/drivers/clk/realtek/clk-pll-mmc.c b/drivers/clk/realtek/clk-pll-mmc.c
> new file mode 100644
> --- /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(req->rate * 4, req->best_parent_rate);
> +	req->rate = req->best_parent_rate * val / 4;
> +
> +	return 0;
> +}

[Severity: Medium]
Is there a risk of integer overflow here on 32-bit platforms if req->rate
exceeds ~1.07 GHz? The calculation req->rate * 4 operates on an unsigned long.

Also, does val need to be bounds-checked against the hardware's supported
tuning limits? If an extreme rate is requested, the framework might approve it
but it could map to unsupported hardware configurations later on, since val
is never clamped here.

[ ... ]

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

[Severity: High]
Does this function intentionally ignore the rate and parent_rate parameters?

Because val is initialized to a hardcoded constant (PLL_MMC_SSC_DIV_N_VAL,
which is 27), the hardware is strictly locked to a single static configuration
and the dynamic rate arguments are bypassed.

Additionally, this hardcoded value causes the subsequent switch statement
to unconditionally take the `case 20 ... 30:` branch, making the rest of
the frequency ranges dead code.

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