Re: [PATCH 1/3] clk: qcom: clk-alpha-pll: support a 28-bit ALPHA_VAL width

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The function `zonda_pll_adjust_l_val` incorrectly adds the unscaled target frequency `rate` instead of the divided `quotient` to the L-value multiplier.
--

commit 661ff85b8608f1b3893f18020783320b39a04c99
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: clk-alpha-pll: support a 28-bit ALPHA_VAL width
    
This commit introduces support for an optional 28-bit ALPHA_VAL width in the
clk-alpha-pll driver by adding an alpha_width field. This allows correct
scaling and calculation for PLLs like the Zonda instances that use a wider
register for finer fractional resolution.

> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 60173b076cc5f..981f973da02f3 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c

[ ... ]

> @@ -2290,7 +2291,7 @@ static int clk_zonda_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (a & PLL_ALPHA_MSB)
> +	if (a & BIT(alpha_width - 1))
>  		zonda_pll_adjust_l_val(rate, prate, &l);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this L-adjust
MSB check, does zonda_pll_adjust_l_val() incorrectly add the unscaled target
frequency to the L-value multiplier?

When this condition is met and zonda_pll_adjust_l_val() is invoked, it appears
to overwrite the previously calculated integer divider with the raw rate
instead of the local quotient:

drivers/clk/qcom/clk-alpha-pll.c:zonda_pll_adjust_l_val() {
	u64 remainder, quotient;

	quotient = rate;
	remainder = do_div(quotient, prate);

	*l = rate + (u32)(remainder * 2 >= prate);
}

Should this final assignment use the `quotient` variable instead of `rate`?
Writing an astronomically unscaled value (like the target rate in Hz) to the
L-value register would severely corrupt the PLL hardware configuration and
cause it to fail to lock.

>  
>  	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-nords_dprx-v1-0-d472af3f01b1@oss.qualcomm.com?part=1