Re: [PATCH v7 1/4] clk: sunxi-ng: mux: fix determine helper rate propagation

[sashiko-bot@kernel.org]

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

New issues:
- [High] Treating the pre-divider as a post-divider changes the rounding order, causing a mathematical mismatch between `determine_rate` and `recalc_rate`.

Pre-existing issues:
- [High] `ccu_mux_get_prediv` fails to translate hardware indices to logical indices for the active parent, breaking pre-dividers in `recalc_rate`.
--

--- Patch [1]: [PATCH v7 1/4] clk: sunxi-ng: mux: fix determine helper rate propagation ---
commit 3ac5416eb08e665a33aeec5afae741b08d4fdb57
Author: Jerome Brunet <jbrunet@baylibre.com>

clk: sunxi-ng: mux: fix determine helper rate propagation

Applying the pre divider on the parent rate is wrong because, while
handling rate propagation through determine_rate(), the framework will
likely round the parent rate again while cycling through the possibilities,
throwing away the prediv applied. This means, the parent rate will then
be wrong when the prediv is unapplied from a parent rate on which it
was never applied to begin with.

> diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/ccu_mux.c
> index 09230728c4009..4503c9780c39f 100644
> --- a/drivers/clk/sunxi-ng/ccu_mux.c
> +++ b/drivers/clk/sunxi-ng/ccu_mux.c
> @@ -92,66 +92,65 @@ int ccu_mux_helper_determine_rate(struct ccu_common *common,
>  		struct clk_rate_request adj_req = *req;
>  
>  		best_parent = clk_hw_get_parent(hw);
> -		best_parent_rate = clk_hw_get_rate(best_parent);
> -
> +		adj_req.best_parent_rate = clk_hw_get_rate(best_parent);
>  		adj_req.best_parent_hw = best_parent;
> -		adj_req.best_parent_rate = ccu_mux_helper_apply_prediv(common, cm, -1,
> -								       best_parent_rate);
> +
> +		/*
> +		 * This effectively treats the predivider as a postdivider.
> +		 * It stays mathematically correct and ensures whatever
> +		 * round() will do stays correct while walking the tree.
> +		 * It may query the parent rate too while handling rate
> +		 * propagation.
> +		 */
> +		adj_req.rate = ccu_mux_helper_unapply_prediv(common, cm, -1,
> +							     req->rate);
>  
>  		ret = round(cm, &adj_req, data);
>  		if (ret)
>  			return ret;
>  
> -		best_rate = adj_req.rate;
> -
>  		/*
> -		 * best_parent_rate might have been modified by our clock.
> -		 * Unapply the pre-divider if there's one, and give
> -		 * the actual frequency the parent needs to run at.
> +		 * parent_rate might have been modified by our clock as part
> +		 * of the rate propagation mechanism. Same goes below.
>  		 */
> -		best_parent_rate = ccu_mux_helper_unapply_prediv(common, cm, -1,
> -								 adj_req.best_parent_rate);
> +		best_parent_rate = adj_req.best_parent_rate;
> +		best_rate = ccu_mux_helper_apply_prediv(common, cm, -1,
> +							adj_req.rate);

[Severity: High]
Does treating the pre-divider as a post-divider here cause a mathematical
mismatch between determine_rate() and recalc_rate()?

Because integer division with round-up does not commute, this inverted
calculation order yields different results compared to recalculating the rate.

For example, given a parent rate of 100 Hz, pre-divider 3, and main divider 8
for a target rate of 4 Hz:

- determine_rate() evaluates DIV_ROUND_UP(100, 8) / 3 = 13 / 3 = 4 Hz.
- recalc_rate() evaluates DIV_ROUND_UP(100 / 3, 8) = DIV_ROUND_UP(33, 8) = 5 Hz.

Could this violate the clock framework's requirement that determine_rate()
accurately predicts recalc_rate(), potentially causing rate deviation warnings
or infinite loops during rate setting?


[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the pre-divider
logic, I noticed a potential issue in how hardware indices are handled when
the parent_index is less than 0.

drivers/clk/sunxi-ng/ccu_mux.c:ccu_mux_get_prediv() {
    ...
    if (parent_index < 0) {
        parent_index = reg >> cm->shift;
        parent_index &= (1 << cm->width) - 1;
    }
    ...
    for (i = 0; i < cm->n_fixed_predivs; i++)
        if (parent_index == cm->fixed_predivs[i].index)
            return cm->fixed_predivs[i].div;
    ...
}

Does this fail to translate hardware indices to logical indices for the active
parent?

When ccu_mux_get_prediv() is called with parent_index < 0 (e.g. from
recalc_rate), it reads the raw hardware index from the register. It then
compares this directly against fixed_predivs[i].index, which is populated with
logical indices.

Could this cause recalc_rate() to report an incorrect clock rate by falling back
to a pre-divider of 1 instead of the configured pre-divider for clocks with both
a hardware mapping .table and fixed/variable predividers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-a733-rtc-v7-0-8fd68aab94ae@baylibre.com?part=1