[PATCH v3 0/3] Fix unused clock disabling on LPC18xx

[PATCH v3 0/3] Fix unused clock disabling on LPC18xx

[Joachim Eastwood]

I have finally managed to track down the reason for the hang that would
occur when disabling clocks on lpc18xx under certain conditions. These
conditions depend on boot mode used, what the boot loader does and if
certain devices are enabled.

One of reasons why it took me a while was that my primary platform was
not affected by it and the data sheet has a rather misleading typo.

The hang will occur if the boot rom/loader doesn't setup USB0 clocks
and if certain AMBA devices are enabled (PL022 SSP and maybe others).

Cause of the hang is that the CCU registers can not be accessed if the
base (parent) clock is not enabled. To make sure the parent is running
a check for this has been added to the is_enabled clk_ops callback.
Since clocks in the CGU can be cascaded this check must also be added
here.

To better understand the clock system on lpc18xx please take the time
to look at the diagrams on:
https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks

This version address the comments from Stephen Boyd.

Changes since v3:
 - add clk_hw_is_enabled
 - fix typo on CGU gate enable function
 - use clk_hw_get_parent() insted of clk_get_parent()
 - rebase on v4.3-rc1

Changes since v2:
 - Add clk.h to cgu since it uses clk_get_parent().
 - Rebase on clk-next

Joachim Eastwood (3):
  clk: Add clk_hw_is_enabled() for use by clk providers
  clk: lpc18xx-ccu: fix potential system hang when disabling unused clocks
  clk: lpc18xx-cgu: fix potential system hang when disabling unused clocks

 drivers/clk/clk.c                 |  5 +++++
 drivers/clk/nxp/clk-lpc18xx-ccu.c | 17 ++++++++++++++--
 drivers/clk/nxp/clk-lpc18xx-cgu.c | 42 ++++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h      |  1 +
 4 files changed, 60 insertions(+), 5 deletions(-)

-- 
1.8.0

[PATCH v3 1/3] clk: Add clk_hw_is_enabled() for use by clk providers

[Joachim Eastwood]

Add clk_hw_is_enabled() to the provider APIs so clk providers can
use a struct clk_hw instead of a struct clk to check if a clk is
enabled or not.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/clk/clk.c            | 5 +++++
 include/linux/clk-provider.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 43e2c3ad6c31..44601851e79c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -427,6 +427,11 @@ bool clk_hw_is_prepared(const struct clk_hw *hw)
 	return clk_core_is_prepared(hw->core);
 }
 
+bool clk_hw_is_enabled(const struct clk_hw *hw)
+{
+	return clk_core_is_enabled(hw->core);
+}
+
 bool __clk_is_enabled(struct clk *clk)
 {
 	if (!clk)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 3ecc07d0da77..1a71436e4c4b 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -618,6 +618,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw);
 unsigned long __clk_get_flags(struct clk *clk);
 unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 bool clk_hw_is_prepared(const struct clk_hw *hw);
+bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
 struct clk *__clk_lookup(const char *name);
 int __clk_mux_determine_rate(struct clk_hw *hw,
-- 
1.8.0

[PATCH v3 2/3] clk: lpc18xx-ccu: fix potential system hang when disabling unused clocks

[Joachim Eastwood]

CCU branch clock register must only be accessed while the base
(parent) clock is running. Access with a disabled base clock
will cause the system to hang. Fix this issue by adding code
that check if the parent clock is running in the is_enabled
clk_ops callback.

This hang would occur when disabling unused clocks after AMBA
runtime pm had already disabled some of the clocks.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/clk/nxp/clk-lpc18xx-ccu.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-lpc18xx-ccu.c
index eeaee97da110..13aabbb3acbe 100644
--- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
+++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
@@ -179,9 +179,22 @@ static void lpc18xx_ccu_gate_disable(struct clk_hw *hw)
 
 static int lpc18xx_ccu_gate_is_enabled(struct clk_hw *hw)
 {
-	struct clk_gate *gate = to_clk_gate(hw);
+	const struct clk_hw *parent;
+
+	/*
+	 * The branch clock registers are only accessible
+	 * if the base (parent) clock is enabled. Register
+	 * access with a disabled base clock will hang the
+	 * system.
+	 */
+	parent = clk_hw_get_parent(hw);
+	if (!parent)
+		return 0;
+
+	if (!clk_hw_is_enabled(parent))
+		return 0;
 
-	return clk_readl(gate->reg) & LPC18XX_CCU_RUN;
+	return clk_gate_ops.is_enabled(hw);
 }
 
 static const struct clk_ops lpc18xx_ccu_gate_ops = {
-- 
1.8.0

[PATCH v3 3/3] clk: lpc18xx-cgu: fix potential system hang when disabling unused clocks

[Joachim Eastwood]

The clock consumer (CCU) of the CGU must be able to check if a CGU
base clock is really running since access to the CCU registers
requires a running base clock. Access with a disabled base clock will
cause the system to hang. Fix this issue by adding code that check if
the parent clock is running in the is_enabled clk_ops callback. Since
certain clocks can be cascaded this must be added to all clock gates.

The hang would occur if the boot ROM or boot loader didn't setup and
enable the USB0 clock. Then when the clk framework tried to access
the CCU register it would hang the system.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/clk/nxp/clk-lpc18xx-cgu.c | 42 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc18xx-cgu.c b/drivers/clk/nxp/clk-lpc18xx-cgu.c
index e0a3cb8970ab..c924572fc9bc 100644
--- a/drivers/clk/nxp/clk-lpc18xx-cgu.c
+++ b/drivers/clk/nxp/clk-lpc18xx-cgu.c
@@ -480,6 +480,42 @@ static const struct clk_ops lpc18xx_pll1_ops = {
 	.recalc_rate = lpc18xx_pll1_recalc_rate,
 };
 
+static int lpc18xx_cgu_gate_enable(struct clk_hw *hw)
+{
+	return clk_gate_ops.enable(hw);
+}
+
+static void lpc18xx_cgu_gate_disable(struct clk_hw *hw)
+{
+	clk_gate_ops.disable(hw);
+}
+
+static int lpc18xx_cgu_gate_is_enabled(struct clk_hw *hw)
+{
+	const struct clk_hw *parent;
+
+	/*
+	 * The consumer of base clocks needs know if the
+	 * base clock is really enabled before it can be
+	 * accessed. It is therefore necessary to verify
+	 * this all the way up.
+	 */
+	parent = clk_hw_get_parent(hw);
+	if (!parent)
+		return 0;
+
+	if (!clk_hw_is_enabled(parent))
+		return 0;
+
+	return clk_gate_ops.is_enabled(hw);
+}
+
+static const struct clk_ops lpc18xx_gate_ops = {
+	.enable = lpc18xx_cgu_gate_enable,
+	.disable = lpc18xx_cgu_gate_disable,
+	.is_enabled = lpc18xx_cgu_gate_is_enabled,
+};
+
 static struct lpc18xx_cgu_pll_clk lpc18xx_cgu_src_clk_plls[] = {
 	LPC1XX_CGU_CLK_PLL(PLL0USB,	pll0_src_ids, pll0_ops),
 	LPC1XX_CGU_CLK_PLL(PLL0AUDIO,	pll0_src_ids, pll0_ops),
@@ -510,7 +546,7 @@ static struct clk *lpc18xx_cgu_register_div(struct lpc18xx_cgu_src_clk_div *clk,
 	return clk_register_composite(NULL, name, parents, clk->n_parents,
 				      &clk->mux.hw, &clk_mux_ops,
 				      &clk->div.hw, &clk_divider_ops,
-				      &clk->gate.hw, &clk_gate_ops, 0);
+				      &clk->gate.hw, &lpc18xx_gate_ops, 0);
 }
 
 
@@ -538,7 +574,7 @@ static struct clk *lpc18xx_register_base_clk(struct lpc18xx_cgu_base_clk *clk,
 	return clk_register_composite(NULL, name, parents, clk->n_parents,
 				      &clk->mux.hw, &clk_mux_ops,
 				      NULL,  NULL,
-				      &clk->gate.hw, &clk_gate_ops, 0);
+				      &clk->gate.hw, &lpc18xx_gate_ops, 0);
 }
 
 
@@ -557,7 +593,7 @@ static struct clk *lpc18xx_cgu_register_pll(struct lpc18xx_cgu_pll_clk *clk,
 	return clk_register_composite(NULL, name, parents, clk->n_parents,
 				      &clk->mux.hw, &clk_mux_ops,
 				      &clk->pll.hw, clk->pll_ops,
-				      &clk->gate.hw, &clk_gate_ops, 0);
+				      &clk->gate.hw, &lpc18xx_gate_ops, 0);
 }
 
 static void __init lpc18xx_cgu_register_source_clks(struct device_node *np,
-- 
1.8.0

Re: [PATCH v3 1/3] clk: Add clk_hw_is_enabled() for use by clk providers

[Stephen Boyd]

On 10/24, Joachim Eastwood wrote:
> Add clk_hw_is_enabled() to the provider APIs so clk providers can
> use a struct clk_hw instead of a struct clk to check if a clk is
> enabled or not.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/3] clk: lpc18xx-ccu: fix potential system hang when disabling unused clocks

[Stephen Boyd]

On 10/24, Joachim Eastwood wrote:
> CCU branch clock register must only be accessed while the base
> (parent) clock is running. Access with a disabled base clock
> will cause the system to hang. Fix this issue by adding code
> that check if the parent clock is running in the is_enabled
> clk_ops callback.
> 
> This hang would occur when disabling unused clocks after AMBA
> runtime pm had already disabled some of the clocks.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 3/3] clk: lpc18xx-cgu: fix potential system hang when disabling unused clocks

[Stephen Boyd]

On 10/24, Joachim Eastwood wrote:
> The clock consumer (CCU) of the CGU must be able to check if a CGU
> base clock is really running since access to the CCU registers
> requires a running base clock. Access with a disabled base clock will
> cause the system to hang. Fix this issue by adding code that check if
> the parent clock is running in the is_enabled clk_ops callback. Since
> certain clocks can be cascaded this must be added to all clock gates.
> 
> The hang would occur if the boot ROM or boot loader didn't setup and
> enable the USB0 clock. Then when the clk framework tried to access
> the CCU register it would hang the system.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project