Re: [PATCH v3 4/6] clk: ambarella: add CV75 CCU driver

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] VCO range iteration underflows and corrupts the PLL CTRL3 register if `fvco_mhz` evaluates to 0.
- [High] `amb_pll_calc_params` omits the feedback and output dividers (`fsdiv` and `fsout`) when determining parameters, resulting in drastically incorrect clock frequencies.
- [High] Division by zero kernel panic in `amb_pll_calc_params()` when the parent clock rate is zero.
- [Medium] Setting the rate of a non-ONE_BASED divider to 0 permanently disables the clock due to an incorrect mask.
- [Medium] `amb_pll_determine_rate` unnecessarily restricts integer PLL rates to multiples of `parent_rate / 2`.
- [Low] Passing a 64-bit divisor to `DIV_ROUND_CLOSEST_ULL` causes a build failure on 32-bit architectures.
--

--- Patch [4]: [PATCH v3 4/6] clk: ambarella: add CV75 CCU driver ---
commit 434160130974a0a15aae2d546aa85876cbc1fe59
Author: Long Zhao <[email protected]>

clk: ambarella: add CV75 CCU driver

Add the Ambarella CV75 clock driver covering the core PLL and the
UART0 / AHB / APB clocks needed 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
> --- /dev/null
> +++ b/drivers/clk/ambarella/ccu_mux_div.c
[ ... ]
> +static int amb_div_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{
[ ... ]
> +	if (!rate) {
> +		/* Assert the reset/disable bit above the divider field. */
> +		val = BIT(div->width);
> +		mask = clk_div_mask(div->width + 1);
> +	} 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);

[Severity: Medium]
When setting the rate to 0, the driver asserts the reset/disable bit above
the divider field. When a non-zero rate is subsequently requested, this logic
uses a mask that strictly covers only the divider field.

Will this exclude the higher disable bit during the update, leaving the clock
permanently disabled?

> +	}
> +
> +	regmap_update_bits(div->map, div->offset, mask << div->shift,
> +			   val << div->shift);
[ ... ]
> diff --git a/drivers/clk/ambarella/ccu_pll.c b/drivers/clk/ambarella/ccu_pll.c
> new file mode 100644
> --- /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 -EINVAL;
> +
> +	req->rate = roundup(req->rate, half_refclk);

[Severity: Medium]
Does clamping the requested rate to a multiple of half_refclk artificially
restrict the hardware's capabilities?

Because the hardware sout divider supports ranges up to 16, it looks like it
could natively support a wide and fine-grained selection of exact integer
frequencies instead of being clamped this way.

> +
> +	return 0;
> +}
> +
> +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 = soc_data->vco_max_mhz;
> +	max_numerator = div_u64(max_numerator * 1000000ULL, parent_rate);

[Severity: High]
In amb_pll_determine_rate(), if the PLL is in frac_mode, it returns 0
immediately without validating if half_refclk is zero:

static int amb_pll_determine_rate(...) {
        ...
        if (pll->frac_mode)
                return 0;

        if (!half_refclk)
                return -EINVAL;
        ...
}

If a zero-rate parent clock reaches amb_pll_calc_params(), will this
division by zero trigger a kernel panic?

> +	max_numerator = div_u64(max_numerator, *vcodiv * *fsdiv);
> +	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: High]
The hardware frequency depends on fsdiv and fsout, but this call requests an
approximation using just rate and parent_rate. 

If the hardware is left in a state where fsdiv and fsout are not identical,
will the resulting frequency be scaled incorrectly and bypass the refinement
loop?

> +
> +	while (parent_rate * *fsdiv * intp_ul * *sdiv / *fsout / sout_ul >
[ ... ]
> +static int amb_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			    unsigned long parent_rate)
> +{
[ ... ]
> +	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);

[Severity: Low]
The divider variable evaluates to a 64-bit integer, but
DIV_ROUND_CLOSEST_ULL() invokes do_div() which expects a 32-bit divisor on
32-bit architectures.

Will this trigger a __div64_32 link failure under COMPILE_TEST?

> +		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;
> +		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;
> +
> +		regmap_read(pll->map, reg[CTRL3_OFFSET], &ctrl3_val);
> +		ctrl3_val &= ~CTRL3_VCO_RANGE_MASK;
> +		ctrl3_val |= range << 1;

[Severity: High]
If the PLL parent rate is exceptionally low (e.g., from a dummy clock),
fvco_mhz can evaluate to 0. Since 0 is not strictly greater than any range
minimum, the loop finishes at 4.

Will the subsequent calculation 4 - 4 - 1 underflow to 0xFFFFFFFF and
corrupt the CTRL3 register bits?

> +		regmap_write(pll->map, reg[CTRL3_OFFSET], ctrl3_val);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4