Re: [PATCH v2 06/10] clk: ambarella: add CV75 CCU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] `do_div()` is incorrectly used on `unsigned long` variables, causing stack memory corruption on 32-bit architectures.
- [High] The generic divider implementation contains logic bugs that cause it to check the wrong bit for status, fail to disable clocks, and corrupt divider values.
- [High] A zero or invalid parent rate causes a fatal divide-by-zero panic during PLL parameter calculation.
- [High] Infinite loop in `amb_pll_calc_params` when `parent_rate` is smaller than the maximum post-divider.
- [Medium] `amb_pll_calc_params` omits static `fsdiv` and `fsout` dividers from its rational target approximation, resulting in incorrect rates.
- [Medium] `amb_pll_set_rate` fails to apply `pre_scaler` and `post_scaler` to target calculations, breaking rate configurations.
- [Medium] Stale hardware register read causes the incorrect VCO range to be selected for fractional PLL modes.
- [Medium] Integer PLL rates are unnecessarily restricted to coarse multiples of `parent_rate / 2`.
--

--- Patch [6]: [PATCH v2 06/10] clk: ambarella: add CV75 CCU driver ---
commit 3c34aa8f15f802b4bcbd9606eec16b27a37dd8d9
Author: Long Zhao <longzhao@ambarella.com>

clk: ambarella: add CV75 CCU driver

This commit adds a minimal Ambarella clock controller driver for the CV75 SoC,
providing core PLL, bus, and UART clocks required for early console bring-up.

> diff --git a/drivers/clk/ambarella/ccu_mux_div.c b/drivers/clk/ambarella/ccu_mux_div.c
> new file mode 100644
> index 0000000000000..33430506f8ed1
> --- /dev/null
> +++ b/drivers/clk/ambarella/ccu_mux_div.c
[ ... ]
> +static unsigned long amb_div_recalc_rate(struct clk_hw *hw,
> +					 unsigned long parent_rate)
> +{
> +	struct amb_div *div = to_amb_div(hw);
> +	unsigned long rate;
> +	u32 val;
> +
> +	regmap_read(div->map, div->offset, &val);
> +
> +	if (val & BIT(div->width))
> +		return 0;

[Severity: High]
Does this check the correct bit? The code checks BIT(div->width) directly
against the raw register value before shifting it by div->shift. If
div->shift is non-zero, this evaluates the wrong hardware bit.

> +
> +	val >>= div->shift;
> +	val &= clk_div_mask(div->width);
> +
> +	rate = divider_recalc_rate(hw, parent_rate, val, NULL,
> +				   div->flags, div->width);
> +	if (div->fix_divider)
> +		do_div(rate, div->fix_divider);

[Severity: High]
Can this cause a stack buffer overflow on 32-bit architectures? The do_div
macro expects a 64-bit u64 pointer. Because this driver can be built for
32-bit targets via COMPILE_TEST, unsigned long is 32 bits on those systems.
Passing a 32-bit variable causes the macro to write 64 bits, overwriting
adjacent stack memory.

> +
> +	return rate;
> +}
> +
> +static int amb_div_determine_rate(struct clk_hw *hw,
> +				  struct clk_rate_request *req)
> +{
[ ... ]
> +	req->rate = scaled.rate;
> +	req->best_parent_rate = scaled.best_parent_rate;
> +	req->best_parent_hw = scaled.best_parent_hw;
> +	if (div->fix_divider)
> +		do_div(req->rate, div->fix_divider);

[Severity: High]
Could this corrupt the adjacent fields in the clk_rate_request struct? The
do_div macro writes 64 bits to req->rate, which is an unsigned long and
thus only 32 bits on 32-bit builds.

> +
> +	return 0;
> +}
> +
> +static int amb_div_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{
> +	struct amb_div *div = to_amb_div(hw);
> +	int val, mask;
> +
> +	if (div->fix_divider)
> +		rate *= div->fix_divider;
> +
> +	if (!rate) {
> +		val = BIT(div->width);
> +	} else {
> +		val = divider_get_val(rate, parent_rate, NULL,
> +				      div->width, div->flags);
> +		if (val < 0)
> +			return val;
> +	}
> +
> +	mask = (div->flags & CLK_DIVIDER_ONE_BASED) ?
> +		clk_div_mask(div->width + 1) : clk_div_mask(div->width);
> +
> +	regmap_update_bits(div->map, div->offset, mask << div->shift,
> +			   val << div->shift);

[Severity: High]
Can this prevent the clock from being disabled? The update mask for
zero-based dividers uses clk_div_mask(div->width), which excludes
BIT(div->width). When setting a rate of 0, val evaluates to BIT(div->width),
but it gets masked out here, silently leaving the clock enabled with a zero
divider.

> +
> +	if (!(div->flags & CLK_DIVIDER_ONE_BASED)) {
> +		regmap_update_bits(div->map, div->offset, BIT(0), BIT(0));
> +		regmap_update_bits(div->map, div->offset, BIT(0), 0);
> +	}

[Severity: High]
Will toggling BIT(0) directly on the register corrupt the actual divider
field if div->shift is 0?

> diff --git a/drivers/clk/ambarella/ccu_pll.c b/drivers/clk/ambarella/ccu_pll.c
> new file mode 100644
> index 0000000000000..1293836e222e5
> --- /dev/null
> +++ b/drivers/clk/ambarella/ccu_pll.c
[ ... ]
> +static int amb_pll_determine_rate(struct clk_hw *hw,
> +				  struct clk_rate_request *req)
> +{
> +	struct amb_pll *pll = to_amb_pll(hw);
> +	unsigned long half_refclk = req->best_parent_rate / 2;
> +
> +	if (pll->frac_mode)
> +		return 0;
> +
> +	if (!half_refclk)
> +		return 0;

[Severity: High]
If the best parent rate is 0 or 1, half_refclk evaluates to 0, returning 0
(success). Doesn't this allow a 0 parent rate to reach amb_pll_set_rate,
which then calls amb_pll_calc_params and triggers a divide-by-zero panic?

> +
> +	req->rate = roundup(req->rate, half_refclk);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this artificially restrict the integer PLL solver? By forcibly rounding
up the requested frequency to multiples of parent_rate / 2, this prevents
the framework from utilizing the fine-grained post-divider (sout) inside
amb_pll_calc_params to generate precise integer ratios.

> +static int amb_pll_calc_params(struct amb_pll *pll, unsigned long rate,
> +			       unsigned long parent_rate, u32 ctrl2_val,
> +			       u32 *intp, u32 *sdiv, u32 *sout,
> +			       u32 *vcodiv, u32 *fsdiv, u32 *fsout)
> +{
[ ... ]
> +	max_numerator = min(128UL, max_numerator);
> +	if (!max_numerator)
> +		return -EINVAL;
> +
> +	max_denominator = AMB_PLL_MAX_SOUT;
> +	rate_tmp = rate;
> +	rational_best_approximation(rate_tmp, parent_rate, max_numerator,
> +				    max_denominator, &intp_ul, &sout_ul);

[Severity: Medium]
Should the rate approximation factor in the static fsdiv and fsout dividers?
The rational_best_approximation is called assuming the output rate
relationship is solely parent_rate * intp / sout. Because the true hardware
equation includes fsdiv and fsout, omitting them from the target ratio
approximation results in incorrectly calculated parameters.

> +
> +	while (parent_rate * *fsdiv * intp_ul * *sdiv / *fsout / sout_ul >
> +	       rate) {
> +		unsigned long resolution = parent_rate / AMB_PLL_MAX_SOUT;
> +
> +		if (rate_tmp <= resolution)
> +			return -EINVAL;
> +
> +		rate_tmp -= resolution;

[Severity: High]
Is there a risk of an infinite loop here if parent_rate is very low? If
parent_rate is less than AMB_PLL_MAX_SOUT (16), resolution truncates to 0.
Subtracting 0 leaves rate_tmp unchanged, causing the loop condition to
evaluate as permanently true.

> +		rational_best_approximation(rate_tmp, parent_rate,
> +					    max_numerator, max_denominator,
> +					    &intp_ul, &sout_ul);
> +	}
[ ... ]
> +static int amb_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{
> +	struct amb_pll *pll = to_amb_pll(hw);
[ ... ]
> +	ret = amb_pll_calc_params(pll, rate, parent_rate, ctrl2_val,
> +				  &intp, &sdiv, &sout, &vcodiv, &fsdiv,
> +				  &fsout);

[Severity: Medium]
Should this adjust the requested rate and parent_rate using the hardware
pre_scaler and post_scaler? Unlike amb_pll_recalc_rate which reads these
scalers from hardware registers, amb_pll_set_rate passes the raw rate and
parent_rate directly into amb_pll_calc_params, which will configure the PLL
clocks to incorrect frequencies.

> +	if (ret)
> +		return ret;
> +
> +	if (soc_data->ctrl2_val)
> +		regmap_write(pll->map, reg[CTRL2_OFFSET], soc_data->ctrl2_val);
> +
> +	ctrl_val = ((intp - 1) & 0x7f) << 24;
> +	ctrl_val |= ((sdiv - 1) & 0xf) << 12;
> +	ctrl_val |= ((sout - 1) & 0xf) << 16;
> +	if (soc_data->pll_version >= 2) {
> +		ctrl_val |= vcodiv == 2 ? soc_data->vcodiv_val : 0;
> +		ctrl_val |= fsdiv == 2 ? soc_data->fsdiv_val : 0;
> +		ctrl_val |= fsout == 2 ? soc_data->fsout_val : 0;
> +	}
> +
> +	regmap_write(pll->map, reg[CTRL_OFFSET], ctrl_val);
> +	regmap_write(pll->map, reg[FRAC_OFFSET], 0);
> +
> +	old_rate = amb_pll_recalc_rate(hw, parent_rate) * pll->fix_divider;
> +	rate_tmp = old_rate > rate ? 0 : rate - old_rate;
> +	if (rate_tmp && pll->frac_mode) {
> +		u64 dividend, divider;
> +
> +		dividend = (u64)rate_tmp * sout * fsout;
> +		dividend <<= 32;
> +		divider = (u64)sdiv * fsdiv * parent_rate;
> +		frac_val = DIV_ROUND_CLOSEST_ULL(dividend, divider);
> +		regmap_write(pll->map, reg[FRAC_OFFSET], frac_val);
> +		ctrl_val |= CTRL_FRAC_MODE;
> +	}
> +
> +	if (soc_data->pll_version >= 2) {
[ ... ]
> +	} else {
> +		u32 fvco_mhz, range;
> +
> +		fvco_mhz = amb_pll_calc_vco(pll, parent_rate) / 1000000UL;

[Severity: Medium]
Does amb_pll_calc_vco() use a stale hardware register read to determine the
VCO frequency? The CTRL_FRAC_MODE bit is enabled in the local ctrl_val
variable above, but not yet written to the hardware register. When
amb_pll_calc_vco() reads the register directly, it misses the uncommitted
fractional change, which can lead to the driver selecting the wrong
VCO_RANGE boundary.

> +		for (range = 0; range < ARRAY_SIZE(soc_data->vco_range);
> +		     range++) {
> +			if (fvco_mhz > soc_data->vco_range[range])
> +				break;
> +		}
> +		range = ARRAY_SIZE(soc_data->vco_range) - range - 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-longzhao-upstream-cv75-v2-v2-0-6b09707c5fe9@ambarella.com?part=6