[PATCH v2 0/2] clk: aspeed: Fix is_enabled and prevent reset if clock enabled

[PATCH v2 0/2] clk: aspeed: Fix is_enabled and prevent reset if clock enabled

[Eddie James]

Here are two fixes for the Aspeed clock driver. The first fixes the is_enabled
clock function to account for different clock gates getting disabled with
either 0s or 1s. The second patch addresses some issue found with the LPC
controller clock if it gets reset while the clock is enabled, which it is by
default. Thanks to Lei Yu for tracking down the LPC issue.

Changes since v1:
 - Fix is_enabled.
 - Check for enabled in the spinlock in the enable function.
 - Use is_enabled instead of more code to read the register.

Eddie James (2):
  clk: aspeed: Fix is_enabled for certain clocks
  clk: aspeed: Prevent reset if clock is enabled

 drivers/clk/clk-aspeed.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] clk: aspeed: Fix is_enabled for certain clocks

[Eddie James]

Some of the Aspeed clocks are disabled by setting the relevant bit in
the "clock stop control" register to one, while others are disabled by
setting their bit to zero. The driver already uses a flag per gate  to
identify this behavior, but doesn't apply it in the clock is_enabled
function.

Use the existing gate flag to correctly return whether or not a clock
is enabled in the aspeed_clk_is_enabled function.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/clk/clk-aspeed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 9f7f931..1687771 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -259,11 +259,12 @@ static int aspeed_clk_is_enabled(struct clk_hw *hw)
 {
 	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
 	u32 clk = BIT(gate->clock_idx);
+	u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
 	u32 reg;
 
 	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
 
-	return (reg & clk) ? 0 : 1;
+	return ((reg & clk) == enval) ? 1 : 0;
 }
 
 static const struct clk_ops aspeed_clk_gate_ops = {
-- 
1.8.3.1

Re: [PATCH v2 1/2] clk: aspeed: Fix is_enabled for certain clocks

[Joel Stanley]

On Fri, Mar 9, 2018 at 7:27 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> Some of the Aspeed clocks are disabled by setting the relevant bit in
> the "clock stop control" register to one, while others are disabled by
> setting their bit to zero. The driver already uses a flag per gate  to
> identify this behavior, but doesn't apply it in the clock is_enabled
> function.
>
> Use the existing gate flag to correctly return whether or not a clock
> is enabled in the aspeed_clk_is_enabled function.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>

Fixes: 6671507f0fbd ("clk: aspeed: Handle inverse polarity of USB port
1 clock gate")

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel

> ---
>  drivers/clk/clk-aspeed.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 9f7f931..1687771 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -259,11 +259,12 @@ static int aspeed_clk_is_enabled(struct clk_hw *hw)
>  {
>         struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
>         u32 clk = BIT(gate->clock_idx);
> +       u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>         u32 reg;
>
>         regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>
> -       return (reg & clk) ? 0 : 1;
> +       return ((reg & clk) == enval) ? 1 : 0;
>  }
>
>  static const struct clk_ops aspeed_clk_gate_ops = {
> --
> 1.8.3.1
>

Re: [PATCH v2 1/2] clk: aspeed: Fix is_enabled for certain clocks

[Stephen Boyd]

Quoting Eddie James (2018-03-08 12:57:19)
> Some of the Aspeed clocks are disabled by setting the relevant bit in
> the "clock stop control" register to one, while others are disabled by
> setting their bit to zero. The driver already uses a flag per gate  to
> identify this behavior, but doesn't apply it in the clock is_enabled
> function.
> 
> Use the existing gate flag to correctly return whether or not a clock
> is enabled in the aspeed_clk_is_enabled function.
> 
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---

Applied to clk-fixes

[PATCH v2 2/2] clk: aspeed: Prevent reset if clock is enabled

[Eddie James]

According to the Aspeed specification, the reset and enable sequence
should be done when the clock is stopped. The specification doesn't
define behavior if the reset is done while the clock is enabled.

>From testing on the AST2500, the LPC Controller has problems if the
clock is reset while enabled.

Therefore, check whether the clock is enabled or not before performing
the reset and enable sequence in the Aspeed clock driver.

Root-caused-by: Lei Yu <mine260309@gmail.com>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/clk/clk-aspeed.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 1687771..5eb50c3 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -205,6 +205,18 @@ struct aspeed_clk_soc_data {
 	.calc_pll = aspeed_ast2400_calc_pll,
 };
 
+static int aspeed_clk_is_enabled(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	u32 clk = BIT(gate->clock_idx);
+	u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
+	u32 reg;
+
+	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
+
+	return ((reg & clk) == enval) ? 1 : 0;
+}
+
 static int aspeed_clk_enable(struct clk_hw *hw)
 {
 	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
@@ -215,6 +227,11 @@ static int aspeed_clk_enable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
+	if (aspeed_clk_is_enabled(hw)) {
+		spin_unlock_irqrestore(gate->lock, flags);
+		return 0;
+	}
+
 	if (gate->reset_idx >= 0) {
 		/* Put IP in reset */
 		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
@@ -255,18 +272,6 @@ static void aspeed_clk_disable(struct clk_hw *hw)
 	spin_unlock_irqrestore(gate->lock, flags);
 }
 
-static int aspeed_clk_is_enabled(struct clk_hw *hw)
-{
-	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
-	u32 clk = BIT(gate->clock_idx);
-	u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
-	u32 reg;
-
-	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
-
-	return ((reg & clk) == enval) ? 1 : 0;
-}
-
 static const struct clk_ops aspeed_clk_gate_ops = {
 	.enable = aspeed_clk_enable,
 	.disable = aspeed_clk_disable,
-- 
1.8.3.1

Re: [PATCH v2 2/2] clk: aspeed: Prevent reset if clock is enabled

[Lei YU]

>  static int aspeed_clk_enable(struct clk_hw *hw)
>  {
>         struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> @@ -215,6 +227,11 @@ static int aspeed_clk_enable(struct clk_hw *hw)
>
>         spin_lock_irqsave(gate->lock, flags);
>
> +       if (aspeed_clk_is_enabled(hw)) {
> +               spin_unlock_irqrestore(gate->lock, flags);
> +               return 0;
> +       }
> +

I think this piece of code can be run before spin_lock_irqsave(), so it is
able to just return without spin_unlock_irqrestore()?

Re: [PATCH v2 2/2] clk: aspeed: Prevent reset if clock is enabled

[Joel Stanley]

Hi Lei,

On Fri, Mar 9, 2018 at 1:49 PM, Lei YU <mine260309@gmail.com> wrote:
>
>
>
>>  static int aspeed_clk_enable(struct clk_hw *hw)
>>  {
>>         struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
>> @@ -215,6 +227,11 @@ static int aspeed_clk_enable(struct clk_hw *hw)
>>
>>         spin_lock_irqsave(gate->lock, flags);
>>
>> +       if (aspeed_clk_is_enabled(hw)) {
>> +               spin_unlock_irqrestore(gate->lock, flags);
>> +               return 0;
>> +       }
>> +
>
> I think this piece of code can be run before spin_lock_irqsave(), so it is
> able to just return without spin_unlock_irqrestore()?

As far as I understand, we are not running under any kind of lock when
calling into the clock framework.

Consider two clock consumer (any old driver) attempting an operation
to change the state of a clock. The first consumer calls
aspeed_clk_enable, and run the aspeed_clk_is_enabled function. This
consumer is then preempted, and the second consume calls
aspeed_clk_disable, taking the lock, they then disable the clock. They
return out of aspeed_clk_disable and the first consumer can run again.

The first consumer has already checked that the clock was disabled, so
they execute the 'return 0', without enabling it. However, their
information is out of date, so they are now in a state where the clock
hardware is disabled, but the clock framework and the consumer think
it is enabled.

By doing the check under a lock, the second consumer won't be able to
perform it's operation until after the first has finished (and as we
disable IRQs, the first bit of code will run to completion without
being preempted).

I might have missed something, so if you're confident we don't need to
check the value under a lock then please let me know :)

Cheers,

Joel

Re: [PATCH v2 2/2] clk: aspeed: Prevent reset if clock is enabled

[Joel Stanley]

On Fri, Mar 9, 2018 at 7:27 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> According to the Aspeed specification, the reset and enable sequence
> should be done when the clock is stopped. The specification doesn't
> define behavior if the reset is done while the clock is enabled.
>
> From testing on the AST2500, the LPC Controller has problems if the
> clock is reset while enabled.
>
> Therefore, check whether the clock is enabled or not before performing
> the reset and enable sequence in the Aspeed clock driver.
>
> Root-caused-by: Lei Yu <mine260309@gmail.com>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>

Fixes: 15ed8ce5f84e ("clk: aspeed: Register gated clocks")

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel


> ---
>  drivers/clk/clk-aspeed.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 1687771..5eb50c3 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -205,6 +205,18 @@ struct aspeed_clk_soc_data {
>         .calc_pll = aspeed_ast2400_calc_pll,
>  };
>
> +static int aspeed_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +       u32 clk = BIT(gate->clock_idx);
> +       u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
> +       u32 reg;
> +
> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
> +
> +       return ((reg & clk) == enval) ? 1 : 0;
> +}
> +
>  static int aspeed_clk_enable(struct clk_hw *hw)
>  {
>         struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> @@ -215,6 +227,11 @@ static int aspeed_clk_enable(struct clk_hw *hw)
>
>         spin_lock_irqsave(gate->lock, flags);
>
> +       if (aspeed_clk_is_enabled(hw)) {
> +               spin_unlock_irqrestore(gate->lock, flags);
> +               return 0;
> +       }
> +
>         if (gate->reset_idx >= 0) {
>                 /* Put IP in reset */
>                 regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
> @@ -255,18 +272,6 @@ static void aspeed_clk_disable(struct clk_hw *hw)
>         spin_unlock_irqrestore(gate->lock, flags);
>  }
>
> -static int aspeed_clk_is_enabled(struct clk_hw *hw)
> -{
> -       struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> -       u32 clk = BIT(gate->clock_idx);
> -       u32 enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
> -       u32 reg;
> -
> -       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
> -
> -       return ((reg & clk) == enval) ? 1 : 0;
> -}
> -
>  static const struct clk_ops aspeed_clk_gate_ops = {
>         .enable = aspeed_clk_enable,
>         .disable = aspeed_clk_disable,
> --
> 1.8.3.1
>

Re: [PATCH v2 2/2] clk: aspeed: Prevent reset if clock is enabled

[Stephen Boyd]

Quoting Eddie James (2018-03-08 12:57:20)
> According to the Aspeed specification, the reset and enable sequence
> should be done when the clock is stopped. The specification doesn't
> define behavior if the reset is done while the clock is enabled.
> 
> From testing on the AST2500, the LPC Controller has problems if the
> clock is reset while enabled.
> 
> Therefore, check whether the clock is enabled or not before performing
> the reset and enable sequence in the Aspeed clock driver.
> 
> Root-caused-by: Lei Yu <mine260309@gmail.com>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---

Applied to clk-fixes

Re: [PATCH v2 0/2] clk: aspeed: Fix is_enabled and prevent reset if clock enabled

[Stephen Boyd]

Quoting Eddie James (2018-03-08 12:57:18)
> Here are two fixes for the Aspeed clock driver. The first fixes the is_enabled
> clock function to account for different clock gates getting disabled with
> either 0s or 1s. The second patch addresses some issue found with the LPC
> controller clock if it gets reset while the clock is enabled, which it is by
> default. Thanks to Lei Yu for tracking down the LPC issue.
> 

Can you please add some "Fixes:" tags to these patches? Or just indicate
the tags and I can pick them up. If it doesn't happen, I'll just apply
these early next week.

Re: [PATCH v2 0/2] clk: aspeed: Fix is_enabled and prevent reset if clock enabled

[Eddie James]

On 03/09/2018 06:41 PM, Stephen Boyd wrote:
> Quoting Eddie James (2018-03-08 12:57:18)
>> Here are two fixes for the Aspeed clock driver. The first fixes the is_enabled
>> clock function to account for different clock gates getting disabled with
>> either 0s or 1s. The second patch addresses some issue found with the LPC
>> controller clock if it gets reset while the clock is enabled, which it is by
>> default. Thanks to Lei Yu for tracking down the LPC issue.
>>
> Can you please add some "Fixes:" tags to these patches? Or just indicate
> the tags and I can pick them up. If it doesn't happen, I'll just apply
> these early next week.

Sure, sorry for the late response...

Patch 1 fixes 6671507f0fbd582b4003f837ab791d03ade8e0f4
Patch 2 fixes 15ed8ce5f84e2b3718690915dbee12ebd497dc0f

Thanks!
Eddie

>