[PATCH v2 0/2] Fix unused clock disabling on LPC18xx

[PATCH v2 0/2] 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.

Maybe this check to see if the parent clock is running clock be added
to the clk core and enabled with a flag(?) For now this patch set fixes
the issue seen on lpc18xx.

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

Joachim Eastwood (2):
  clk: lpc18xx-ccu: fix potential system hang when disabling unused clocks
  clk: lpc18xx-cgu: fix potential system hang when disabling unused clocks

 drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 +++++++++++++
 drivers/clk/nxp/clk-lpc18xx-cgu.c | 43 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 3 deletions(-)

-- 
1.8.0

[PATCH v2 1/2] 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>
---

No changes from v1.

 drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-lpc18xx-ccu.c
index eeaee97da110..1845476e635e 100644
--- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
+++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
@@ -180,6 +180,20 @@ 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);
+	struct clk *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_get_parent(hw->clk);
+	if (IS_ERR(parent))
+		return 0;
+
+	if (!__clk_is_enabled(parent))
+		return 0;
 
 	return clk_readl(gate->reg) & LPC18XX_CCU_RUN;
 }
-- 
1.8.0

[PATCH v2 2/2] 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>
---

Changes: add clk.h to get clk_get_parent().

 drivers/clk/nxp/clk-lpc18xx-cgu.c | 43 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc18xx-cgu.c b/drivers/clk/nxp/clk-lpc18xx-cgu.c
index e0a3cb8970ab..3d8d0ac38f96 100644
--- a/drivers/clk/nxp/clk-lpc18xx-cgu.c
+++ b/drivers/clk/nxp/clk-lpc18xx-cgu.c
@@ -8,6 +8,7 @@
  * warranty of any kind, whether express or implied.
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
@@ -480,6 +481,42 @@ static const struct clk_ops lpc18xx_pll1_ops = {
 	.recalc_rate = lpc18xx_pll1_recalc_rate,
 };
 
+static int lpc18xx_cgu_gate_enabled(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)
+{
+	struct clk *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_get_parent(hw->clk);
+	if (IS_ERR(parent))
+		return 0;
+
+	if (!__clk_is_enabled(parent))
+		return 0;
+
+	return clk_gate_ops.is_enabled(hw);
+}
+
+static const struct clk_ops lpc18xx_gate_ops = {
+	.enable = lpc18xx_cgu_gate_enabled,
+	.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 +547,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 +575,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 +594,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 v2 0/2] Fix unused clock disabling on LPC18xx

[Joachim Eastwood]

On 25 August 2015 at 20:34, Joachim Eastwood <manabian@gmail.com> wrote:
> 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.
>
> Maybe this check to see if the parent clock is running clock be added
> to the clk core and enabled with a flag(?) For now this patch set fixes
> the issue seen on lpc18xx.
>
> Changes since v2:
>  - Add clk.h to cgu since it uses clk_get_parent().
>  - Rebase on clk-next
>
> Joachim Eastwood (2):
>   clk: lpc18xx-ccu: fix potential system hang when disabling unused clocks
>   clk: lpc18xx-cgu: fix potential system hang when disabling unused clocks
>
>  drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 +++++++++++++
>  drivers/clk/nxp/clk-lpc18xx-cgu.c | 43 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 54 insertions(+), 3 deletions(-)

Ping?


regards,
Joachim Eastwood

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

[Stephen Boyd]

On 08/25, 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>
> ---
> 
> No changes from v1.
> 
>  drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> index eeaee97da110..1845476e635e 100644
> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> @@ -180,6 +180,20 @@ 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);
> +	struct clk *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_get_parent(hw->clk);

Why not use provider APIs (clk_hw_get_parent and we could add a
clk_hw_is_enabled)? I also wonder why we don't just read the
register directly here instead of going through the framework to
find out if the parent is enabled?

It may also make sense to "optimize" the disable unused code to
do a depth first search for a disabled clock and then disable
clocks from the leaves to the root. That would avoid this problem
right?

> +	if (IS_ERR(parent))
> +		return 0;
> +
> +	if (!__clk_is_enabled(parent))
> +		return 0;
>  
>  	return clk_readl(gate->reg) & LPC18XX_CCU_RUN;
>  }
> -- 
> 1.8.0
> 

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

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

[Stephen Boyd]

On 08/25, Joachim Eastwood wrote:
> diff --git a/drivers/clk/nxp/clk-lpc18xx-cgu.c b/drivers/clk/nxp/clk-lpc18xx-cgu.c
> index e0a3cb8970ab..3d8d0ac38f96 100644
> --- a/drivers/clk/nxp/clk-lpc18xx-cgu.c
> +++ b/drivers/clk/nxp/clk-lpc18xx-cgu.c
> @@ -8,6 +8,7 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
> @@ -480,6 +481,42 @@ static const struct clk_ops lpc18xx_pll1_ops = {
>  	.recalc_rate = lpc18xx_pll1_recalc_rate,
>  };
>  
> +static int lpc18xx_cgu_gate_enabled(struct clk_hw *hw)

s/enabled/enable/?

> +{
> +	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)
> +{
> +	struct clk *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_get_parent(hw->clk);
> +	if (IS_ERR(parent))
> +		return 0;
> +
> +	if (!__clk_is_enabled(parent))

Same comment as in patch 1.

> +		return 0;
> +
> +	return clk_gate_ops.is_enabled(hw);
> +}
> +
> +static const struct clk_ops lpc18xx_gate_ops = {
> +	.enable = lpc18xx_cgu_gate_enabled,
> +	.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),

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

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

[Joachim Eastwood]

On 20 October 2015 at 00:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/25, 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>
>> ---
>>
>> No changes from v1.
>>
>>  drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> index eeaee97da110..1845476e635e 100644
>> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> @@ -180,6 +180,20 @@ 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);
>> +     struct clk *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_get_parent(hw->clk);
>
> Why not use provider APIs (clk_hw_get_parent and we could add a
> clk_hw_is_enabled)?

That is simply because I didn't know it existed.
Adding a clk_hw_is_enabled() seems like a good idea to me.


>I also wonder why we don't just read the
> register directly here instead of going through the framework to
> find out if the parent is enabled?

Reading the register when the parent clock is off will hang the hardware.


> It may also make sense to "optimize" the disable unused code to
> do a depth first search for a disabled clock and then disable
> clocks from the leaves to the root. That would avoid this problem
> right?

As long the leaf clock registers that has a disabled parent clock is
not touched it will solve the problem I have.


regards,
Joachim Eastwood

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

[Michael Turquette]

Quoting Joachim Eastwood (2015-10-20 03:42:14)
> On 20 October 2015 at 00:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 08/25, 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>
> >> ---
> >>
> >> No changes from v1.
> >>
> >>  drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-l=
pc18xx-ccu.c
> >> index eeaee97da110..1845476e635e 100644
> >> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> >> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> >> @@ -180,6 +180,20 @@ static void lpc18xx_ccu_gate_disable(struct clk_h=
w *hw)
> >>  static int lpc18xx_ccu_gate_is_enabled(struct clk_hw *hw)
> >>  {
> >>       struct clk_gate *gate =3D to_clk_gate(hw);
> >> +     struct clk *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 =3D clk_get_parent(hw->clk);
> >
> > Why not use provider APIs (clk_hw_get_parent and we could add a
> > clk_hw_is_enabled)?
> =

> That is simply because I didn't know it existed.
> Adding a clk_hw_is_enabled() seems like a good idea to me.
> =

> =

> >I also wonder why we don't just read the
> > register directly here instead of going through the framework to
> > find out if the parent is enabled?
> =

> Reading the register when the parent clock is off will hang the hardware.

I think Stephen meant, why not simply read the parent clk register?

Regards,
Mike

> =

> =

> > It may also make sense to "optimize" the disable unused code to
> > do a depth first search for a disabled clock and then disable
> > clocks from the leaves to the root. That would avoid this problem
> > right?
> =

> As long the leaf clock registers that has a disabled parent clock is
> not touched it will solve the problem I have.
> =

> =

> regards,
> Joachim Eastwood

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

[Joachim Eastwood]

On 21 October 2015 at 11:34, Michael Turquette <mturquette@baylibre.com> wrote:
> Quoting Joachim Eastwood (2015-10-20 03:42:14)
>> On 20 October 2015 at 00:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 08/25, 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>
>> >> ---
>> >>
>> >> No changes from v1.
>> >>
>> >>  drivers/clk/nxp/clk-lpc18xx-ccu.c | 14 ++++++++++++++
>> >>  1 file changed, 14 insertions(+)
>> >>
>> >> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c b/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> >> index eeaee97da110..1845476e635e 100644
>> >> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> >> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
>> >> @@ -180,6 +180,20 @@ 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);
>> >> +     struct clk *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_get_parent(hw->clk);
>> >
>> > Why not use provider APIs (clk_hw_get_parent and we could add a
>> > clk_hw_is_enabled)?
>>
>> That is simply because I didn't know it existed.
>> Adding a clk_hw_is_enabled() seems like a good idea to me.
>>
>>
>> >I also wonder why we don't just read the
>> > register directly here instead of going through the framework to
>> > find out if the parent is enabled?
>>
>> Reading the register when the parent clock is off will hang the hardware.
>
> I think Stephen meant, why not simply read the parent clk register?

The parent clk register is in another hardware block.


regards,
Joachim Eastwood