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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
Acked-by: Stephen Boyd <[email protected]>
On Thu 26 Dec 2019 at 20:12, Martin Blumenstingl <[email protected]> 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(-)