While trying to get cpufreq working on msm8974 I've found an issue with
clock rates above 2.11GHz (e.g. 2.15GHz). When a rate above this
threshold gets requested the lowest possible frequency will be selected.
This is caused by an overflow of the "long" return type of .round_rate
which has a maximum value of 2147483647 (2.14GHz) on 32-bit systems,
which msm8974 is.
We can switch the drivers to determine_rate so we can use the full
"unsigned long" type which lets us go up to 4294967295 (4.29GHz) before
an overflow happens which is significantly below the maximum frequency
of any msm8974 which is around 2.45GHz.
Note, that while setting the main hfpll now works correctly, the code
setting the div2 is still sort of broken, since it's requesting
"req->rate * 2" which will still overflow the unsigned long maximum
value, but it seems this doesn't actually break anything since the div2
doesn't use the calculated value. That's my understanding at least.
Signed-off-by: Luca Weiss <[email protected]>
---
Luca Weiss (2):
clk: qcom: clk-krait: switch to .determine_rate
clk: qcom: clk-hfpll: switch to .determine_rate
drivers/clk/qcom/clk-hfpll.c | 14 +++++++-------
drivers/clk/qcom/clk-krait.c | 10 +++++-----
2 files changed, 12 insertions(+), 12 deletions(-)
---
base-commit: 6ba8a227fd19d19779005fb66ad7562608e1df83
change-id: 20230212-clk-qcom-determine_rate-c90c9ad0b337
Best regards,
--
Luca Weiss <[email protected]>
.determine_rate is meant to replace .round_rate. The former comes with a
benefit which is especially relevant on 32-bit systems: since
.determine_rate uses an "unsigned long" (compared to a "signed long"
which is used by .round_rate) the maximum value on 32-bit systems
increases from 2^31 (or approx. 2.14GHz) to 2^32 (or approx. 4.29GHz).
Signed-off-by: Luca Weiss <[email protected]>
---
drivers/clk/qcom/clk-hfpll.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
index 7dd17c184b69..86f728dc69e5 100644
--- a/drivers/clk/qcom/clk-hfpll.c
+++ b/drivers/clk/qcom/clk-hfpll.c
@@ -128,20 +128,20 @@ static void clk_hfpll_disable(struct clk_hw *hw)
spin_unlock_irqrestore(&h->lock, flags);
}
-static long clk_hfpll_round_rate(struct clk_hw *hw, unsigned long rate,
- unsigned long *parent_rate)
+static int clk_hfpll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
{
struct clk_hfpll *h = to_clk_hfpll(hw);
struct hfpll_data const *hd = h->d;
unsigned long rrate;
- rate = clamp(rate, hd->min_rate, hd->max_rate);
+ req->rate = clamp(req->rate, hd->min_rate, hd->max_rate);
- rrate = DIV_ROUND_UP(rate, *parent_rate) * *parent_rate;
+ rrate = DIV_ROUND_UP(req->rate, req->best_parent_rate) * req->best_parent_rate;
if (rrate > hd->max_rate)
- rrate -= *parent_rate;
+ rrate -= req->best_parent_rate;
- return rrate;
+ req->rate = rrate;
+ return 0;
}
/*
@@ -241,7 +241,7 @@ const struct clk_ops clk_ops_hfpll = {
.enable = clk_hfpll_enable,
.disable = clk_hfpll_disable,
.is_enabled = hfpll_is_enabled,
- .round_rate = clk_hfpll_round_rate,
+ .determine_rate = clk_hfpll_determine_rate,
.set_rate = clk_hfpll_set_rate,
.recalc_rate = clk_hfpll_recalc_rate,
.init = clk_hfpll_init,
--
2.39.1
On Sun, Feb 12, 2023 at 03:11:09PM +0100, Luca Weiss wrote:
> .determine_rate is meant to replace .round_rate. The former comes with a
> benefit which is especially relevant on 32-bit systems: since
> .determine_rate uses an "unsigned long" (compared to a "signed long"
> which is used by .round_rate) the maximum value on 32-bit systems
> increases from 2^31 (or approx. 2.14GHz) to 2^32 (or approx. 4.29GHz).
>
> Signed-off-by: Luca Weiss <[email protected]>
Tested-by: Christian Marangi <[email protected]>
> ---
> drivers/clk/qcom/clk-hfpll.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
> index 7dd17c184b69..86f728dc69e5 100644
> --- a/drivers/clk/qcom/clk-hfpll.c
> +++ b/drivers/clk/qcom/clk-hfpll.c
> @@ -128,20 +128,20 @@ static void clk_hfpll_disable(struct clk_hw *hw)
> spin_unlock_irqrestore(&h->lock, flags);
> }
>
> -static long clk_hfpll_round_rate(struct clk_hw *hw, unsigned long rate,
> - unsigned long *parent_rate)
> +static int clk_hfpll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> {
> struct clk_hfpll *h = to_clk_hfpll(hw);
> struct hfpll_data const *hd = h->d;
> unsigned long rrate;
>
> - rate = clamp(rate, hd->min_rate, hd->max_rate);
> + req->rate = clamp(req->rate, hd->min_rate, hd->max_rate);
>
> - rrate = DIV_ROUND_UP(rate, *parent_rate) * *parent_rate;
> + rrate = DIV_ROUND_UP(req->rate, req->best_parent_rate) * req->best_parent_rate;
> if (rrate > hd->max_rate)
> - rrate -= *parent_rate;
> + rrate -= req->best_parent_rate;
>
> - return rrate;
> + req->rate = rrate;
> + return 0;
> }
>
> /*
> @@ -241,7 +241,7 @@ const struct clk_ops clk_ops_hfpll = {
> .enable = clk_hfpll_enable,
> .disable = clk_hfpll_disable,
> .is_enabled = hfpll_is_enabled,
> - .round_rate = clk_hfpll_round_rate,
> + .determine_rate = clk_hfpll_determine_rate,
> .set_rate = clk_hfpll_set_rate,
> .recalc_rate = clk_hfpll_recalc_rate,
> .init = clk_hfpll_init,
>
> --
> 2.39.1
>
--
Ansuel
On Sun, 12 Feb 2023 15:11:07 +0100, Luca Weiss wrote:
> While trying to get cpufreq working on msm8974 I've found an issue with
> clock rates above 2.11GHz (e.g. 2.15GHz). When a rate above this
> threshold gets requested the lowest possible frequency will be selected.
>
> This is caused by an overflow of the "long" return type of .round_rate
> which has a maximum value of 2147483647 (2.14GHz) on 32-bit systems,
> which msm8974 is.
>
> [...]
Applied, thanks!
[1/2] clk: qcom: clk-krait: switch to .determine_rate
commit: a7074c3eb26e0193f2c6ed79987e633b7578024e
[2/2] clk: qcom: clk-hfpll: switch to .determine_rate
commit: 04648b8fad219599ccc9b103188a38e72d339a3d
Best regards,
--
Bjorn Andersson <[email protected]>