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

On jeu. 23 juil. 2026 at 09:58, [email protected] wrote:

> 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`.

I don't get what sashiko is trying to say here. determine_rate() and
recalc_rate() do the same thing.

Once determine rate is done, it gets the parent rate and apply the
prediv, which simulate what the signal actually does.

recalc_rate() does the same thing.

So there is confusion here ... from sashiko or from me :/


>
> 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 <[email protected]>
>
> 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?

-- 
Jerome