[v4,03/68] clk: Move no reparent case into a separate function

Commit Message

Maxime Ripard May 5, 2023, 11:25 a.m. UTC

From: Stephen Boyd <[e-mail]>

We'll need to turn the code in clk_mux_determine_rate_flags() to deal
with CLK_SET_RATE_NO_REPARENT into a helper clock drivers will be able
to use if they don't want to allow reparenting.

Cc: Abel Vesa <[e-mail]>
Cc: Alessandro Zummo <[e-mail]>
Cc: Alexandre Belloni <[e-mail]>
Cc: Alexandre Torgue <[e-mail]>
Cc: "Andreas Färber" <[e-mail]>
Cc: AngeloGioacchino Del Regno <[e-mail]>
Cc: Baolin Wang <[e-mail]>
Cc: Charles Keepax <[e-mail]>
Cc: Chen-Yu Tsai <[e-mail]>
Cc: Chen-Yu Tsai <[e-mail]>
Cc: Chunyan Zhang <[e-mail]>
Cc: Claudiu Beznea <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: David Lechner <[e-mail]>
Cc: Dinh Nguyen <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Jaroslav Kysela <[e-mail]>
Cc: Jernej Skrabec <[e-mail]>
Cc: Jonathan Hunter <[e-mail]>
Cc: Kishon Vijay Abraham I <[e-mail]>
Cc: Liam Girdwood <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Luca Ceresoli <[e-mail]>
Cc: Manivannan Sadhasivam <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Markus Schneider-Pargmann <[e-mail]>
Cc: Max Filippov <[e-mail]>
Cc: Maxime Coquelin <[e-mail]>
Cc: Mikko Perttunen <[e-mail]>
Cc: Miles Chen <[e-mail]>
Cc: Nicolas Ferre <[e-mail]>
Cc: Orson Zhai <[e-mail]>
Cc: Paul Cercueil <[e-mail]>
Cc: Peng Fan <[e-mail]>
Cc: Peter De Schrijver <[e-mail]>
Cc: Prashant Gaikwad <[e-mail]>
Cc: Richard Fitzgerald <[e-mail]>
Cc: Samuel Holland <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Sekhar Nori <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Takashi Iwai <[e-mail]>
Cc: Thierry Reding <[e-mail]>
Cc: Ulf Hansson <[e-mail]>
Cc: Vinod Koul <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: NXP Linux Team <[e-mail]>
Cc: [e-mail]
Cc: Pengutronix Kernel Team <[e-mail]>
Signed-off-by: Stephen Boyd <[e-mail]>
Signed-off-by: Maxime Ripard <[e-mail]>
---
 drivers/clk/clk.c | 75 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 32 deletions(-)

Comments

Marek Szyprowski June 13, 2023, 11:15 a.m. UTC | #1

On 05.05.2023 13:25, Maxime Ripard wrote:
> From: Stephen Boyd <[e-mail]>
>
> We'll need to turn the code in clk_mux_determine_rate_flags() to deal
> with CLK_SET_RATE_NO_REPARENT into a helper clock drivers will be able
> to use if they don't want to allow reparenting.
>
> Cc: Abel Vesa <[e-mail]>
> Cc: Alessandro Zummo <[e-mail]>
> Cc: Alexandre Belloni <[e-mail]>
> Cc: Alexandre Torgue <[e-mail]>
> Cc: "Andreas Färber" <[e-mail]>
> Cc: AngeloGioacchino Del Regno <[e-mail]>
> Cc: Baolin Wang <[e-mail]>
> Cc: Charles Keepax <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Chen-Yu Tsai <[e-mail]>
> Cc: Chunyan Zhang <[e-mail]>
> Cc: Claudiu Beznea <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: David Lechner <[e-mail]>
> Cc: Dinh Nguyen <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Jaroslav Kysela <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Jonathan Hunter <[e-mail]>
> Cc: Kishon Vijay Abraham I <[e-mail]>
> Cc: Liam Girdwood <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: Luca Ceresoli <[e-mail]>
> Cc: Manivannan Sadhasivam <[e-mail]>
> Cc: Mark Brown <[e-mail]>
> Cc: Markus Schneider-Pargmann <[e-mail]>
> Cc: Max Filippov <[e-mail]>
> Cc: Maxime Coquelin <[e-mail]>
> Cc: Mikko Perttunen <[e-mail]>
> Cc: Miles Chen <[e-mail]>
> Cc: Nicolas Ferre <[e-mail]>
> Cc: Orson Zhai <[e-mail]>
> Cc: Paul Cercueil <[e-mail]>
> Cc: Peng Fan <[e-mail]>
> Cc: Peter De Schrijver <[e-mail]>
> Cc: Prashant Gaikwad <[e-mail]>
> Cc: Richard Fitzgerald <[e-mail]>
> Cc: Samuel Holland <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Sekhar Nori <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Takashi Iwai <[e-mail]>
> Cc: Thierry Reding <[e-mail]>
> Cc: Ulf Hansson <[e-mail]>
> Cc: Vinod Koul <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: NXP Linux Team <[e-mail]>
> Cc: [e-mail]
> Cc: Pengutronix Kernel Team <[e-mail]>
> Signed-off-by: Stephen Boyd <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---

This patch landed in today's linux-next as commit 1b4e99fda73f ("clk: 
Move no reparent case into a separate function"). Unfortunately it 
causes serious regression of some of my test boards. Namely Exynos3250 
based boards are so slow after it, that my test scripts fail with a 
timeout waiting for them to finish booting. I will try to debug this 
later in the evening to check what has happened that some clocks got 
very low rate.


> drivers/clk/clk.c | 75 
> +++++++++++++++++++++++++++++++------------------------
> 1 file changed, 43 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e495dd7a1eae..f57f821a5e5a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -594,6 +594,46 @@ clk_core_forward_rate_req(struct clk_core *core,
> req->max_rate = old_req->max_rate;
> }
> +static int
> +clk_core_determine_rate_no_reparent(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_core *core = hw->core;
> + struct clk_core *parent = core->parent;
> + unsigned long best;
> + int ret;
> +
> + if (core->flags & CLK_SET_RATE_PARENT) {
> + struct clk_rate_request parent_req;
> +
> + if (!parent) {
> + req->rate = 0;
> + return 0;
> + }
> +
> + clk_core_forward_rate_req(core, req, parent, &parent_req,
> + req->rate);
> +
> + trace_clk_rate_request_start(&parent_req);
> +
> + ret = clk_core_round_rate_nolock(parent, &parent_req);
> + if (ret)
> + return ret;
> +
> + trace_clk_rate_request_done(&parent_req);
> +
> + best = parent_req.rate;
> + } else if (parent) {
> + best = clk_core_get_rate_nolock(parent);
> + } else {
> + best = clk_core_get_rate_nolock(core);
> + }
> +
> + req->rate = best;
> +
> + return 0;
> +}
> +
> int clk_mux_determine_rate_flags(struct clk_hw *hw,
> struct clk_rate_request *req,
> unsigned long flags)
> @@ -603,35 +643,8 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
> unsigned long best = 0;
> /* if NO_REPARENT flag set, pass through to current parent */
> - if (core->flags & CLK_SET_RATE_NO_REPARENT) {
> - parent = core->parent;
> - if (core->flags & CLK_SET_RATE_PARENT) {
> - struct clk_rate_request parent_req;
> -
> - if (!parent) {
> - req->rate = 0;
> - return 0;
> - }
> -
> - clk_core_forward_rate_req(core, req, parent, &parent_req, req->rate);
> -
> - trace_clk_rate_request_start(&parent_req);
> -
> - ret = clk_core_round_rate_nolock(parent, &parent_req);
> - if (ret)
> - return ret;
> -
> - trace_clk_rate_request_done(&parent_req);
> -
> - best = parent_req.rate;
> - } else if (parent) {
> - best = clk_core_get_rate_nolock(parent);
> - } else {
> - best = clk_core_get_rate_nolock(core);
> - }
> -
> - goto out;
> - }
> + if (core->flags & CLK_SET_RATE_NO_REPARENT)
> + return clk_core_determine_rate_no_reparent(hw, req);
> /* find the parent that can provide the fastest rate <= rate */
> num_parents = core->num_parents;
> @@ -670,9 +683,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
> if (!best_parent)
> return -EINVAL;
> -out:
> - if (best_parent)
> - req->best_parent_hw = best_parent->hw;
> + req->best_parent_hw = best_parent->hw;
> req->best_parent_rate = best;
> req->rate = best;
>
Best regards

Marek Szyprowski June 13, 2023, 12:15 p.m. UTC | #2

On 13.06.2023 13:15, Marek Szyprowski wrote:
> On 05.05.2023 13:25, Maxime Ripard wrote:
>> From: Stephen Boyd <[e-mail]>
>>
>> We'll need to turn the code in clk_mux_determine_rate_flags() to deal
>> with CLK_SET_RATE_NO_REPARENT into a helper clock drivers will be able
>> to use if they don't want to allow reparenting.
>>
>> Cc: Abel Vesa <[e-mail]>
>> Cc: Alessandro Zummo <[e-mail]>
>> Cc: Alexandre Belloni <[e-mail]>
>> Cc: Alexandre Torgue <[e-mail]>
>> Cc: "Andreas Färber" <[e-mail]>
>> Cc: AngeloGioacchino Del Regno <[e-mail]>
>> Cc: Baolin Wang <[e-mail]>
>> Cc: Charles Keepax <[e-mail]>
>> Cc: Chen-Yu Tsai <[e-mail]>
>> Cc: Chen-Yu Tsai <[e-mail]>
>> Cc: Chunyan Zhang <[e-mail]>
>> Cc: Claudiu Beznea <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: David Airlie <[e-mail]>
>> Cc: David Lechner <[e-mail]>
>> Cc: Dinh Nguyen <[e-mail]>
>> Cc: Fabio Estevam <[e-mail]>
>> Cc: Geert Uytterhoeven <[e-mail]>
>> Cc: Jaroslav Kysela <[e-mail]>
>> Cc: Jernej Skrabec <[e-mail]>
>> Cc: Jonathan Hunter <[e-mail]>
>> Cc: Kishon Vijay Abraham I <[e-mail]>
>> Cc: Liam Girdwood <[e-mail]>
>> Cc: Linus Walleij <[e-mail]>
>> Cc: Luca Ceresoli <[e-mail]>
>> Cc: Manivannan Sadhasivam <[e-mail]>
>> Cc: Mark Brown <[e-mail]>
>> Cc: Markus Schneider-Pargmann <[e-mail]>
>> Cc: Max Filippov <[e-mail]>
>> Cc: Maxime Coquelin <[e-mail]>
>> Cc: Mikko Perttunen <[e-mail]>
>> Cc: Miles Chen <[e-mail]>
>> Cc: Nicolas Ferre <[e-mail]>
>> Cc: Orson Zhai <[e-mail]>
>> Cc: Paul Cercueil <[e-mail]>
>> Cc: Peng Fan <[e-mail]>
>> Cc: Peter De Schrijver <[e-mail]>
>> Cc: Prashant Gaikwad <[e-mail]>
>> Cc: Richard Fitzgerald <[e-mail]>
>> Cc: Samuel Holland <[e-mail]>
>> Cc: Sascha Hauer <[e-mail]>
>> Cc: Sekhar Nori <[e-mail]>
>> Cc: Shawn Guo <[e-mail]>
>> Cc: Takashi Iwai <[e-mail]>
>> Cc: Thierry Reding <[e-mail]>
>> Cc: Ulf Hansson <[e-mail]>
>> Cc: Vinod Koul <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: NXP Linux Team <[e-mail]>
>> Cc: [e-mail]
>> Cc: Pengutronix Kernel Team <[e-mail]>
>> Signed-off-by: Stephen Boyd <[e-mail]>
>> Signed-off-by: Maxime Ripard <[e-mail]>
>> ---
>
> This patch landed in today's linux-next as commit 1b4e99fda73f ("clk: 
> Move no reparent case into a separate function"). Unfortunately it 
> causes serious regression of some of my test boards. Namely Exynos3250 
> based boards are so slow after it, that my test scripts fail with a 
> timeout waiting for them to finish booting. I will try to debug this 
> later in the evening to check what has happened that some clocks got 
> very low rate.
>
I just got a few spare minutes, so I decided to take a look into this 
issue. The following change fixed my problem:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ffc9f03840b7..7ac9f7a8cb84 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -629,6 +629,7 @@ clk_core_determine_rate_no_reparent(struct clk_hw *hw,
                 best = clk_core_get_rate_nolock(core);
         }

+       req->best_parent_rate = best;
         req->rate = best;

         return 0;

best_parent_rate is still being used somewhere in the code and needs to 
be updated regardless of the CLK_SET_RATE_NO_REPARENT flag.

 > ...

Best regards

Maxime Ripard June 13, 2023, 12:29 p.m. UTC | #3

Hi,

On Tue, Jun 13, 2023 at 02:15:10PM +0200, Marek Szyprowski wrote:
> On 13.06.2023 13:15, Marek Szyprowski wrote:
> > On 05.05.2023 13:25, Maxime Ripard wrote:
> >> From: Stephen Boyd <[e-mail]>
> >>
> >> We'll need to turn the code in clk_mux_determine_rate_flags() to deal
> >> with CLK_SET_RATE_NO_REPARENT into a helper clock drivers will be able
> >> to use if they don't want to allow reparenting.
> >>
> >> Cc: Abel Vesa <[e-mail]>
> >> Cc: Alessandro Zummo <[e-mail]>
> >> Cc: Alexandre Belloni <[e-mail]>
> >> Cc: Alexandre Torgue <[e-mail]>
> >> Cc: "Andreas Färber" <[e-mail]>
> >> Cc: AngeloGioacchino Del Regno <[e-mail]>
> >> Cc: Baolin Wang <[e-mail]>
> >> Cc: Charles Keepax <[e-mail]>
> >> Cc: Chen-Yu Tsai <[e-mail]>
> >> Cc: Chen-Yu Tsai <[e-mail]>
> >> Cc: Chunyan Zhang <[e-mail]>
> >> Cc: Claudiu Beznea <[e-mail]>
> >> Cc: Daniel Vetter <[e-mail]>
> >> Cc: David Airlie <[e-mail]>
> >> Cc: David Lechner <[e-mail]>
> >> Cc: Dinh Nguyen <[e-mail]>
> >> Cc: Fabio Estevam <[e-mail]>
> >> Cc: Geert Uytterhoeven <[e-mail]>
> >> Cc: Jaroslav Kysela <[e-mail]>
> >> Cc: Jernej Skrabec <[e-mail]>
> >> Cc: Jonathan Hunter <[e-mail]>
> >> Cc: Kishon Vijay Abraham I <[e-mail]>
> >> Cc: Liam Girdwood <[e-mail]>
> >> Cc: Linus Walleij <[e-mail]>
> >> Cc: Luca Ceresoli <[e-mail]>
> >> Cc: Manivannan Sadhasivam <[e-mail]>
> >> Cc: Mark Brown <[e-mail]>
> >> Cc: Markus Schneider-Pargmann <[e-mail]>
> >> Cc: Max Filippov <[e-mail]>
> >> Cc: Maxime Coquelin <[e-mail]>
> >> Cc: Mikko Perttunen <[e-mail]>
> >> Cc: Miles Chen <[e-mail]>
> >> Cc: Nicolas Ferre <[e-mail]>
> >> Cc: Orson Zhai <[e-mail]>
> >> Cc: Paul Cercueil <[e-mail]>
> >> Cc: Peng Fan <[e-mail]>
> >> Cc: Peter De Schrijver <[e-mail]>
> >> Cc: Prashant Gaikwad <[e-mail]>
> >> Cc: Richard Fitzgerald <[e-mail]>
> >> Cc: Samuel Holland <[e-mail]>
> >> Cc: Sascha Hauer <[e-mail]>
> >> Cc: Sekhar Nori <[e-mail]>
> >> Cc: Shawn Guo <[e-mail]>
> >> Cc: Takashi Iwai <[e-mail]>
> >> Cc: Thierry Reding <[e-mail]>
> >> Cc: Ulf Hansson <[e-mail]>
> >> Cc: Vinod Koul <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Cc: NXP Linux Team <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: Pengutronix Kernel Team <[e-mail]>
> >> Signed-off-by: Stephen Boyd <[e-mail]>
> >> Signed-off-by: Maxime Ripard <[e-mail]>
> >> ---
> >
> > This patch landed in today's linux-next as commit 1b4e99fda73f ("clk: 
> > Move no reparent case into a separate function"). Unfortunately it 
> > causes serious regression of some of my test boards. Namely Exynos3250 
> > based boards are so slow after it, that my test scripts fail with a 
> > timeout waiting for them to finish booting. I will try to debug this 
> > later in the evening to check what has happened that some clocks got 
> > very low rate.
> >
> I just got a few spare minutes, so I decided to take a look into this 
> issue. The following change fixed my problem:
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index ffc9f03840b7..7ac9f7a8cb84 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -629,6 +629,7 @@ clk_core_determine_rate_no_reparent(struct clk_hw *hw,
>                  best = clk_core_get_rate_nolock(core);
>          }
> 
> +       req->best_parent_rate = best;
>          req->rate = best;
> 
>          return 0;
> 
> best_parent_rate is still being used somewhere in the code and needs to 
> be updated regardless of the CLK_SET_RATE_NO_REPARENT flag.

Yeah, that makes sense, could you send a patch?

Thanks for figuring it out!
Maxime

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e495dd7a1eae..f57f821a5e5a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -594,6 +594,46 @@  clk_core_forward_rate_req(struct clk_core *core,
 		req->max_rate = old_req->max_rate;
 }
 
+static int
+clk_core_determine_rate_no_reparent(struct clk_hw *hw,
+				    struct clk_rate_request *req)
+{
+	struct clk_core *core = hw->core;
+	struct clk_core *parent = core->parent;
+	unsigned long best;
+	int ret;
+
+	if (core->flags & CLK_SET_RATE_PARENT) {
+		struct clk_rate_request parent_req;
+
+		if (!parent) {
+			req->rate = 0;
+			return 0;
+		}
+
+		clk_core_forward_rate_req(core, req, parent, &parent_req,
+					  req->rate);
+
+		trace_clk_rate_request_start(&parent_req);
+
+		ret = clk_core_round_rate_nolock(parent, &parent_req);
+		if (ret)
+			return ret;
+
+		trace_clk_rate_request_done(&parent_req);
+
+		best = parent_req.rate;
+	} else if (parent) {
+		best = clk_core_get_rate_nolock(parent);
+	} else {
+		best = clk_core_get_rate_nolock(core);
+	}
+
+	req->rate = best;
+
+	return 0;
+}
+
 int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags)
@@ -603,35 +643,8 @@  int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	unsigned long best = 0;
 
 	/* if NO_REPARENT flag set, pass through to current parent */
-	if (core->flags & CLK_SET_RATE_NO_REPARENT) {
-		parent = core->parent;
-		if (core->flags & CLK_SET_RATE_PARENT) {
-			struct clk_rate_request parent_req;
-
-			if (!parent) {
-				req->rate = 0;
-				return 0;
-			}
-
-			clk_core_forward_rate_req(core, req, parent, &parent_req, req->rate);
-
-			trace_clk_rate_request_start(&parent_req);
-
-			ret = clk_core_round_rate_nolock(parent, &parent_req);
-			if (ret)
-				return ret;
-
-			trace_clk_rate_request_done(&parent_req);
-
-			best = parent_req.rate;
-		} else if (parent) {
-			best = clk_core_get_rate_nolock(parent);
-		} else {
-			best = clk_core_get_rate_nolock(core);
-		}
-
-		goto out;
-	}
+	if (core->flags & CLK_SET_RATE_NO_REPARENT)
+		return clk_core_determine_rate_no_reparent(hw, req);
 
 	/* find the parent that can provide the fastest rate <= rate */
 	num_parents = core->num_parents;
@@ -670,9 +683,7 @@  int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	if (!best_parent)
 		return -EINVAL;
 
-out:
-	if (best_parent)
-		req->best_parent_hw = best_parent->hw;
+	req->best_parent_hw = best_parent->hw;
 	req->best_parent_rate = best;
 	req->rate = best;