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 
> Signed-off-by: Eddie James 
> ---

Applied to clk-fixes

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 
> Signed-off-by: Eddie James 
> ---

Applied to clk-fixes

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  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 
> Signed-off-by: Eddie James 

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

Reviewed-by: Joel Stanley 

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, );
> +
> +   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, );
> -
> -   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

[Joel Stanley]

On Fri, Mar 9, 2018 at 7:27 AM, Eddie James  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 
> Signed-off-by: Eddie James 

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

Reviewed-by: Joel Stanley 

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, );
> +
> +   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, );
> -
> -   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

[Joel Stanley]

Hi Lei,

On Fri, Mar 9, 2018 at 1:49 PM, Lei YU  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]

Hi Lei,

On Fri, Mar 9, 2018 at 1:49 PM, Lei YU  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

[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

[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()?