Re: [PATCH 2/4] clk: siflower: Add SF19A2890 clock manager driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: siflower: fractional term scaled by fbdiv in sf19a28_pll_recalc_rate()
- [High] clk: siflower: divide by zero on rate 0 in sf19a28_muxdiv_determine_rate()
--

commit d358931350f6daafe39bfeb8c6193c67ccee78b9
Author: Chuanhong Guo <gch981213@gmail.com>

clk: siflower: Add SF19A2890 clock manager driver

This commit adds an early clock driver for the Siflower SF19A2890 SoC,
supporting its CPU, DDR, and common PLLs as well as the downstream mux-divider
clocks.

> diff --git a/drivers/clk/siflower/clk-sf19a2890.c b/drivers/clk/siflower/clk-sf19a2890.c
> new file mode 100644
> index 0000000000000..f7553ac49d66f
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf19a2890.c

[ ... ]

> +static unsigned long sf19a28_pll_recalc_rate(struct clk_hw *hw,
> +					     unsigned long parent_rate)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf19a2890_clk *priv = cmn_to_clk(cmn_priv);
> +	u32 refdiv = sf_pll_param_get(priv, PLL_REFDIV_HI, PLL_REFDIV_LO);
> +	u32 fbdiv = sf_pll_param_get(priv, PLL_FBDIV_HI, PLL_FBDIV_LO);
> +	u32 postdiv1 = sf_pll_param_get(priv, PLL_POSTDIV1_HI, PLL_POSTDIV1_LO);
> +	u32 postdiv2 = sf_pll_param_get(priv, PLL_POSTDIV2_HI, PLL_POSTDIV2_LO);
> +	u32 pll_pd = sf_readl(cmn_priv, priv->offset + REG_PLL_PD);
> +	u64 rate;
> +	u32 frac;
> +
> +	if (!refdiv || !postdiv1 || !postdiv2)
> +		return 0;
> +
> +	rate = (u64)(parent_rate / refdiv) * fbdiv;
> +
> +	if (!(pll_pd & PLL_PD_DSM)) {
> +		frac = sf_pll_param_get(priv, PLL_FRAC_HI, PLL_FRAC_LO);
> +		rate += (rate * frac) >> PLL_FRAC_BITS;
> +	}

[Severity: High]
In sf19a28_pll_recalc_rate() in drivers/clk/siflower/clk-sf19a2890.c, when the
delta-sigma modulator is enabled (!(pll_pd & PLL_PD_DSM)), does multiplying
rate by frac scale the fractional component by an extra factor of fbdiv?

Because rate is already multiplied by fbdiv before the DSM check, computing
(rate * frac) >> PLL_FRAC_BITS evaluates to:

    ((parent_rate / refdiv) * fbdiv * frac) >> PLL_FRAC_BITS

For a fractional-N PLL, shouldn't the fractional term be scaled only by the
reference frequency (parent_rate / refdiv) rather than by the integer VCO
rate?

[ ... ]

> +static int sf19a28_muxdiv_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	unsigned int div;
> +
> +	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
> +	if (!div)
> +		div = 1;
> +	else if (div > MUXDIV_RATIO_MAX + 1)
> +		div = MUXDIV_RATIO_MAX + 1;
> +
> +	req->rate = req->best_parent_rate / div;
> +	return 0;
> +}
> +
> +static int sf19a28_muxdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +				   unsigned long parent_rate)
> +{
> +	struct sf_clk_common *cmn_priv = hw_to_sf_clk_common(hw);
> +	struct sf19a2890_clk *priv = cmn_to_clk(cmn_priv);
> +	unsigned int div;
> +
> +	div = DIV_ROUND_CLOSEST(parent_rate, rate);

[Severity: High]
Can sf19a28_muxdiv_determine_rate() and sf19a28_muxdiv_set_rate() in
drivers/clk/siflower/clk-sf19a2890.c divide by zero if a consumer calls
clk_round_rate(clk, 0) or clk_set_rate(clk, 0)?

In the common clock framework, clk_core_init_rate_req() initializes
req->min_rate to 0 by default, so a requested rate of 0 is passed directly
into sf19a28_muxdiv_determine_rate() as req->rate == 0 without being clamped.
Similarly, if req->rate evaluates to 0, clk_change_rate() can pass rate == 0
to sf19a28_muxdiv_set_rate().

Should both functions guard against a zero target rate before calling
DIV_ROUND_CLOSEST()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-sf19a2890-sysm-v1-0-a4fac07df823@telaak.dev?part=2