Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags
@ 2019-12-26 19:12 Martin Blumenstingl
  2019-12-26 19:12 ` [PATCH v2 1/2] clk: meson: meson8b: make the CCF use the glitch-free "mali" mux Martin Blumenstingl
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Martin Blumenstingl @ 2019-12-26 19:12 UTC (permalink / raw)
  To: linux-amlogic, jbrunet, sboyd
  Cc: narmstrong, mturquette, linux-clk, linux-arm-kernel,
	linux-kernel, Martin Blumenstingl

While playing with devfreq support for the lima driver I experienced
sporadic (random) system lockups. It turned out that this was in
certain cases when changing the mali clock.

The Amlogic vendor GPU platform driver (which is responsible for
changing the clock frequency) uses the following pattern when updating
the mali clock rate:
- at initialization: initialize the two mali_0 and mali_1 clock trees
  with a default setting and enable both clocks
- when changing the clock frequency:
-- set HHI_MALI_CLK_CNTL[31] to temporarily use the mali_1 clock output
-- update the mali_0 clock tree (set the mux, divider, etc.)
-- clear HHI_MALI_CLK_CNTL[31] to temporarily use the mali_0 clock
   output again

With the common clock framework we can even do better:
by setting CLK_SET_RATE_PARENT for the mali_0 and mali_1 output gates
we can force the common clock framework to update the "inactive" clock
and then switch to it's output.

I only tested this patch for a limited time only (approx. 2 hours).
So far I couldn't reproduce the sporadic system lockups with it.
However, broader testing would be great so I would like this to be
applied for -next.

Changes since v1 at [0]:
- extend the existing comment in patch #1 to describe how the glitch-
  free mux works with the CCF
- slightly updated the patch description of patch #1 to clarify that
  the "mali_0" or "mali_1" trees must not be changed while running
- add patch #2 to update the clk_set_rate() kerneldoc because we agreed
  that clk_set_rate() should do a root-to-leaf update (it does already,
  it's just not documented)


[0] https://patchwork.kernel.org/cover/11293177/


Martin Blumenstingl (2):
  clk: meson: meson8b: make the CCF use the glitch-free "mali" mux
  clk: clarify that clk_set_rate() does updates from top to bottom

 drivers/clk/meson/meson8b.c | 11 +++++++----
 include/linux/clk.h         |  3 +++
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] clk: meson: meson8b: make the CCF use the glitch-free "mali" mux
  2019-12-26 19:12 [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Martin Blumenstingl
@ 2019-12-26 19:12 ` Martin Blumenstingl
  2019-12-26 19:12 ` [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom Martin Blumenstingl
  2020-01-07 11:03 ` [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Jerome Brunet
  2 siblings, 0 replies; 5+ messages in thread
From: Martin Blumenstingl @ 2019-12-26 19:12 UTC (permalink / raw)
  To: linux-amlogic, jbrunet, sboyd
  Cc: narmstrong, mturquette, linux-clk, linux-arm-kernel,
	linux-kernel, Martin Blumenstingl

The "mali_0" or "mali_1" clock trees should not be updated while the
clock is running. Enforce this by setting CLK_SET_RATE_GATE on the
"mali_0" and "mali_1" gates. This makes the CCF switch to the "mali_1"
tree when "mali_0" is currently active and vice versa, which is exactly
what the vendor driver does when updating the frequency of the mali
clock.

This fixes a potential hang when changing the GPU frequency at runtime.

Fixes: 74e1f2521f16ff ("clk: meson: meson8b: add the GPU clock tree")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 3408297bff65..9fd31f23b2a9 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -1772,8 +1772,11 @@ static struct clk_regmap meson8b_hdmi_sys = {
 
 /*
  * The MALI IP is clocked by two identical clocks (mali_0 and mali_1)
- * muxed by a glitch-free switch on Meson8b and Meson8m2. Meson8 only
- * has mali_0 and no glitch-free mux.
+ * muxed by a glitch-free switch on Meson8b and Meson8m2. The CCF can
+ * actually manage this glitch-free mux because it does top-to-bottom
+ * updates the each clock tree and switches to the "inactive" one when
+ * CLK_SET_RATE_GATE is set.
+ * Meson8 only has mali_0 and no glitch-free mux.
  */
 static const struct clk_parent_data meson8b_mali_0_1_parent_data[] = {
 	{ .fw_name = "xtal", .name = "xtal", .index = -1, },
@@ -1838,7 +1841,7 @@ static struct clk_regmap meson8b_mali_0 = {
 			&meson8b_mali_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -1893,7 +1896,7 @@ static struct clk_regmap meson8b_mali_1 = {
 			&meson8b_mali_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
 	},
 };
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom
  2019-12-26 19:12 [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Martin Blumenstingl
  2019-12-26 19:12 ` [PATCH v2 1/2] clk: meson: meson8b: make the CCF use the glitch-free "mali" mux Martin Blumenstingl
@ 2019-12-26 19:12 ` Martin Blumenstingl
  2020-01-06  3:03   ` Stephen Boyd
  2020-01-07 11:03 ` [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Jerome Brunet
  2 siblings, 1 reply; 5+ messages in thread
From: Martin Blumenstingl @ 2019-12-26 19:12 UTC (permalink / raw)
  To: linux-amlogic, jbrunet, sboyd
  Cc: narmstrong, mturquette, linux-clk, linux-arm-kernel,
	linux-kernel, Martin Blumenstingl

clk_set_rate() currently starts updating the rate for a clock at the
top-most affected clock and then walks down the tree to update the
bottom-most affected clock last.
This behavior is important for protected clocks where we can switch
between multiple parents to achieve the same output.

An example for this is the mali clock tree on Amlogic SoCs:
  mali_0_mux (must not change when enabled)
    mali_0_div (must not change when enabled)
     mali_0 (gate)
  mali_1_mux (must not change when enabled)
    mali_1_div (must not change when enabled)
      mali_1 (gate)
The final output can either use mali_0_gate or mali_1. To change the
final output we must switch to the "inactive" tree. Assuming mali_0 is
active, then we need to prepare mali_1 with the new desired rate and
finally switch the output to the mali_1 tree. This process will then
protect the mali_1 tree and at the same time unprotect the mali_0 tree.
The next call to clk_set_rate() will then switch from the mali_1 tree
back to mali_0.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 include/linux/clk.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 18b7b95a8253..7fd6a1febcf4 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -627,6 +627,9 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
  * @clk: clock source
  * @rate: desired clock rate in Hz
  *
+ * Updating the rate starts at the top-most affected clock and then
+ * walks the tree down to the bottom-most clock that needs updating.
+ *
  * Returns success (0) or negative errno.
  */
 int clk_set_rate(struct clk *clk, unsigned long rate);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom
  2019-12-26 19:12 ` [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom Martin Blumenstingl
@ 2020-01-06  3:03   ` Stephen Boyd
  0 siblings, 0 replies; 5+ messages in thread
From: Stephen Boyd @ 2020-01-06  3:03 UTC (permalink / raw)
  To: Martin Blumenstingl, jbrunet, linux-amlogic
  Cc: narmstrong, mturquette, linux-clk, linux-arm-kernel,
	linux-kernel, Martin Blumenstingl

Quoting Martin Blumenstingl (2019-12-26 11:12:24)
> clk_set_rate() currently starts updating the rate for a clock at the
> top-most affected clock and then walks down the tree to update the
> bottom-most affected clock last.
> This behavior is important for protected clocks where we can switch
> between multiple parents to achieve the same output.
> 
> An example for this is the mali clock tree on Amlogic SoCs:
>   mali_0_mux (must not change when enabled)
>     mali_0_div (must not change when enabled)
>      mali_0 (gate)
>   mali_1_mux (must not change when enabled)
>     mali_1_div (must not change when enabled)
>       mali_1 (gate)
> The final output can either use mali_0_gate or mali_1. To change the
> final output we must switch to the "inactive" tree. Assuming mali_0 is
> active, then we need to prepare mali_1 with the new desired rate and
> finally switch the output to the mali_1 tree. This process will then
> protect the mali_1 tree and at the same time unprotect the mali_0 tree.
> The next call to clk_set_rate() will then switch from the mali_1 tree
> back to mali_0.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags
  2019-12-26 19:12 [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Martin Blumenstingl
  2019-12-26 19:12 ` [PATCH v2 1/2] clk: meson: meson8b: make the CCF use the glitch-free "mali" mux Martin Blumenstingl
  2019-12-26 19:12 ` [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom Martin Blumenstingl
@ 2020-01-07 11:03 ` Jerome Brunet
  2 siblings, 0 replies; 5+ messages in thread
From: Jerome Brunet @ 2020-01-07 11:03 UTC (permalink / raw)
  To: Martin Blumenstingl, linux-amlogic, sboyd
  Cc: narmstrong, mturquette, linux-clk, linux-arm-kernel, linux-kernel


On Thu 26 Dec 2019 at 20:12, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> While playing with devfreq support for the lima driver I experienced
> sporadic (random) system lockups. It turned out that this was in
> certain cases when changing the mali clock.
>
> The Amlogic vendor GPU platform driver (which is responsible for
> changing the clock frequency) uses the following pattern when updating
> the mali clock rate:
> - at initialization: initialize the two mali_0 and mali_1 clock trees
>   with a default setting and enable both clocks
> - when changing the clock frequency:
> -- set HHI_MALI_CLK_CNTL[31] to temporarily use the mali_1 clock output
> -- update the mali_0 clock tree (set the mux, divider, etc.)
> -- clear HHI_MALI_CLK_CNTL[31] to temporarily use the mali_0 clock
>    output again
>
> With the common clock framework we can even do better:
> by setting CLK_SET_RATE_PARENT for the mali_0 and mali_1 output gates
> we can force the common clock framework to update the "inactive" clock
> and then switch to it's output.
>
> I only tested this patch for a limited time only (approx. 2 hours).
> So far I couldn't reproduce the sporadic system lockups with it.
> However, broader testing would be great so I would like this to be
> applied for -next.
>
> Changes since v1 at [0]:
> - extend the existing comment in patch #1 to describe how the glitch-
>   free mux works with the CCF
> - slightly updated the patch description of patch #1 to clarify that
>   the "mali_0" or "mali_1" trees must not be changed while running
> - add patch #2 to update the clk_set_rate() kerneldoc because we agreed
>   that clk_set_rate() should do a root-to-leaf update (it does already,
>   it's just not documented)
>
>
> [0] https://patchwork.kernel.org/cover/11293177/
>
>
> Martin Blumenstingl (2):
>   clk: meson: meson8b: make the CCF use the glitch-free "mali" mux
>   clk: clarify that clk_set_rate() does updates from top to bottom
>

Applied with Stephen's Ack

>  drivers/clk/meson/meson8b.c | 11 +++++++----
>  include/linux/clk.h         |  3 +++
>  2 files changed, 10 insertions(+), 4 deletions(-)


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-26 19:12 [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Martin Blumenstingl
2019-12-26 19:12 ` [PATCH v2 1/2] clk: meson: meson8b: make the CCF use the glitch-free "mali" mux Martin Blumenstingl
2019-12-26 19:12 ` [PATCH v2 2/2] clk: clarify that clk_set_rate() does updates from top to bottom Martin Blumenstingl
2020-01-06  3:03   ` Stephen Boyd
2020-01-07 11:03 ` [PATCH v2 0/2] clk: Meson8/8b/8m2: fix the mali clock flags Jerome Brunet

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git