Re: [PATCH 1/2] clk: qcom: clk-krait: switch to .determine_rate

Re: [PATCH 1/2] clk: qcom: clk-krait: switch to .determine_rate

[Christian Marangi]

On Sun, Feb 12, 2023 at 03:11:08PM +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 <luca@z3ntu.xyz>

Hi, found time to test this on ipq806x and seems to work correctly by
scaling to each freq and checking the hfpll reported freq.

Tested-by: Christian Marangi <ansuelsmth@gmail.com>

> ---
>  drivers/clk/qcom/clk-krait.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 293a9dfa7151..f5ce403e1e27 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -97,11 +97,11 @@ const struct clk_ops krait_mux_clk_ops = {
>  EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
>  
>  /* The divider can divide by 2, 4, 6 and 8. But we only really need div-2. */
> -static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
> -				  unsigned long *parent_rate)
> +static int krait_div2_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>  {
> -	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
> -	return DIV_ROUND_UP(*parent_rate, 2);
> +	req->best_parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), req->rate * 2);
> +	req->rate = DIV_ROUND_UP(req->best_parent_rate, 2);
> +	return 0;
>  }
>  
>  static int krait_div2_set_rate(struct clk_hw *hw, unsigned long rate,
> @@ -142,7 +142,7 @@ krait_div2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>  }
>  
>  const struct clk_ops krait_div2_clk_ops = {
> -	.round_rate = krait_div2_round_rate,
> +	.determine_rate = krait_div2_determine_rate,
>  	.set_rate = krait_div2_set_rate,
>  	.recalc_rate = krait_div2_recalc_rate,
>  };
> 
> -- 
> 2.39.1
> 

-- 
	Ansuel

Re: [PATCH 2/2] clk: qcom: clk-hfpll: switch to .determine_rate

[Christian Marangi]

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 <luca@z3ntu.xyz>

Tested-by: Christian Marangi <ansuelsmth@gmail.com>

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

[PATCH 0/2] Switch hfpll & krait clock drivers to .determine_rate

[Luca Weiss]

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 <luca@z3ntu.xyz>
---
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 <luca@z3ntu.xyz>

[PATCH 1/2] clk: qcom: clk-krait: switch to .determine_rate

[Luca Weiss]

.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 <luca@z3ntu.xyz>
---
 drivers/clk/qcom/clk-krait.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 293a9dfa7151..f5ce403e1e27 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -97,11 +97,11 @@ const struct clk_ops krait_mux_clk_ops = {
 EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
 
 /* The divider can divide by 2, 4, 6 and 8. But we only really need div-2. */
-static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
-				  unsigned long *parent_rate)
+static int krait_div2_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 {
-	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
-	return DIV_ROUND_UP(*parent_rate, 2);
+	req->best_parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), req->rate * 2);
+	req->rate = DIV_ROUND_UP(req->best_parent_rate, 2);
+	return 0;
 }
 
 static int krait_div2_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -142,7 +142,7 @@ krait_div2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 }
 
 const struct clk_ops krait_div2_clk_ops = {
-	.round_rate = krait_div2_round_rate,
+	.determine_rate = krait_div2_determine_rate,
 	.set_rate = krait_div2_set_rate,
 	.recalc_rate = krait_div2_recalc_rate,
 };

-- 
2.39.1

[PATCH 2/2] clk: qcom: clk-hfpll: switch to .determine_rate

[Luca Weiss]

.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 <luca@z3ntu.xyz>
---
 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

Re: (subset) [PATCH 0/2] Switch hfpll & krait clock drivers to .determine_rate

[Bjorn Andersson]

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 <andersson@kernel.org>