[v7,1/4] clk: sunxi-ng: mux: fix determine helper rate propagation
Commit Message
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.
The right way to do it is to unapply the prediv from the requested rate,
which is the wanted rate at the input on the clock element, and pass this
to framework to do its thing.
Change the determine rate mux helper in this way.
Fixes: 1c8d7af61b37 ("clk: sunxi-ng: convert from divider_round_rate_parent() to divider_determine_rate()")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/clk/sunxi-ng/ccu_mux.c | 57 +++++++++++++++++++++---------------------
1 file changed, 28 insertions(+), 29 deletions(-)
Comments
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?
On jeu. 23 juil. 2026 at 09:58, sashiko-bot@kernel.org 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 <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?
@@ -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);
goto out;
}
for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
struct clk_rate_request tmp_req = *req;
- unsigned long parent_rate;
+ unsigned long rate;
struct clk_hw *parent;
parent = clk_hw_get_parent_by_index(hw, i);
if (!parent)
continue;
- parent_rate = ccu_mux_helper_apply_prediv(common, cm, i,
- clk_hw_get_rate(parent));
-
tmp_req.best_parent_hw = parent;
- tmp_req.best_parent_rate = parent_rate;
+ tmp_req.best_parent_rate = clk_hw_get_rate(parent);
+ tmp_req.rate = ccu_mux_helper_unapply_prediv(common, cm, i,
+ req->rate);
ret = round(cm, &tmp_req, data);
if (ret)
continue;
- /*
- * 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 = ccu_mux_helper_unapply_prediv(common, cm, i,
- tmp_req.best_parent_rate);
+ rate = ccu_mux_helper_apply_prediv(common, cm, i,
+ tmp_req.rate);
- if (tmp_req.rate == req->rate) {
+ if (rate == req->rate) {
best_parent = parent;
- best_parent_rate = parent_rate;
- best_rate = tmp_req.rate;
+ best_parent_rate = tmp_req.best_parent_rate;
+ best_rate = rate;
goto out;
}
- if (ccu_is_better_rate(common, req->rate, tmp_req.rate, best_rate)) {
- best_rate = tmp_req.rate;
- best_parent_rate = parent_rate;
+ if (ccu_is_better_rate(common, req->rate, rate, best_rate)) {
+ best_rate = rate;
+ best_parent_rate = tmp_req.best_parent_rate;
best_parent = parent;
}
}