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

[PATCH] 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.

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

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 9f7f931..a13054d 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -212,6 +212,12 @@ static int aspeed_clk_enable(struct clk_hw *hw)
 	u32 clk = BIT(gate->clock_idx);
 	u32 rst = BIT(gate->reset_idx);
 	u32 enval;
+	u32 reg;
+
+	/* Only reset/enable/unreset if clock is stopped */
+	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
+	if (!(reg & clk))
+		return 0;
 
 	spin_lock_irqsave(gate->lock, flags);
 
-- 
1.8.3.1

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

[Joel Stanley]

Hi Eddie,

On Thu, Mar 8, 2018 at 3:06 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.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/clk/clk-aspeed.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 9f7f931..a13054d 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -212,6 +212,12 @@ static int aspeed_clk_enable(struct clk_hw *hw)
>         u32 clk = BIT(gate->clock_idx);
>         u32 rst = BIT(gate->reset_idx);
>         u32 enval;
> +       u32 reg;
> +
> +       /* Only reset/enable/unreset if clock is stopped */
> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
> +       if (!(reg & clk))
> +               return 0;

This doesn't generalise to all of the clocks, as some clocks use set
to disable. Perhaps we could do something like this:

       /* Only reset/enable/unreset if clock is stopped. The LPC clock
on ast2500 has issues otherwise */
       enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
       if ((reg & clk) == enval) {
               spin_unlock_irqrestore(gate->lock, flags);
               return 0;
       }

I think we should also do this operation under the lock.

Please cc Ryan Chen <ryan_chen@aspeedtech.com> so he can confirm that
this workaround is valid, and credit Lei who spent a lot of time
investigating this issue. Perhaps "Root-caused-by".

Cheers,

Joel

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

[Joel Stanley]

On Thu, Mar 8, 2018 at 2:12 PM, Joel Stanley <joel@jms.id.au> wrote:
> Hi Eddie,
>
> On Thu, Mar 8, 2018 at 3:06 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.
>>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
>> ---
>>  drivers/clk/clk-aspeed.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
>> index 9f7f931..a13054d 100644
>> --- a/drivers/clk/clk-aspeed.c
>> +++ b/drivers/clk/clk-aspeed.c
>> @@ -212,6 +212,12 @@ static int aspeed_clk_enable(struct clk_hw *hw)
>>         u32 clk = BIT(gate->clock_idx);
>>         u32 rst = BIT(gate->reset_idx);
>>         u32 enval;
>> +       u32 reg;
>> +
>> +       /* Only reset/enable/unreset if clock is stopped */
>> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>> +       if (!(reg & clk))
>> +               return 0;
>
> This doesn't generalise to all of the clocks, as some clocks use set
> to disable. Perhaps we could do something like this:
>
>        /* Only reset/enable/unreset if clock is stopped. The LPC clock
> on ast2500 has issues otherwise */

We've seen this on both the ast2400 and ast2500, so if you do take my
suggestion perhaps just say "The LPC clock has issues otherwise".

>        enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>        regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>        if ((reg & clk) == enval) {
>                spin_unlock_irqrestore(gate->lock, flags);
>                return 0;
>        }
>
> I think we should also do this operation under the lock.
>
> Please cc Ryan Chen <ryan_chen@aspeedtech.com> so he can confirm that
> this workaround is valid, and credit Lei who spent a lot of time
> investigating this issue. Perhaps "Root-caused-by".
>
> Cheers,
>
> Joel

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

[Lei YU]

On Thu, Mar 8, 2018 at 11:42 AM, Joel Stanley <joel@jms.id.au> wrote:
>> +       /* Only reset/enable/unreset if clock is stopped */
>> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>> +       if (!(reg & clk))
>> +               return 0;
>
> This doesn't generalise to all of the clocks, as some clocks use set
> to disable. Perhaps we could do something like this:
>
>        /* Only reset/enable/unreset if clock is stopped. The LPC clock
> on ast2500 has issues otherwise */
>        enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>        regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>        if ((reg & clk) == enval) {
>                spin_unlock_irqrestore(gate->lock, flags);
>                return 0;
>        }
>
> I think we should also do this operation under the lock.
>
> Please cc Ryan Chen <ryan_chen@aspeedtech.com> so he can confirm that
> this workaround is valid, and credit Lei who spent a lot of time
> investigating this issue. Perhaps "Root-caused-by".
>

The code has aspeed_clk_is_enabled() already, why not just:

    if (aspeed_clk_is_enabled(hw))
        return 0;

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

[Joel Stanley]

On Thu, Mar 8, 2018 at 4:08 PM, Lei YU <mine260309@gmail.com> wrote:
> On Thu, Mar 8, 2018 at 11:42 AM, Joel Stanley <joel@jms.id.au> wrote:
>>> +       /* Only reset/enable/unreset if clock is stopped */
>>> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>>> +       if (!(reg & clk))
>>> +               return 0;
>>
>> This doesn't generalise to all of the clocks, as some clocks use set
>> to disable. Perhaps we could do something like this:
>>
>>        /* Only reset/enable/unreset if clock is stopped. The LPC clock
>> on ast2500 has issues otherwise */
>>        enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>>        regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>>        if ((reg & clk) == enval) {
>>                spin_unlock_irqrestore(gate->lock, flags);
>>                return 0;
>>        }
>>
>> I think we should also do this operation under the lock.
>>
>> Please cc Ryan Chen <ryan_chen@aspeedtech.com> so he can confirm that
>> this workaround is valid, and credit Lei who spent a lot of time
>> investigating this issue. Perhaps "Root-caused-by".
>>
>
> The code has aspeed_clk_is_enabled() already, why not just:
>
>     if (aspeed_clk_is_enabled(hw))
>         return 0;

Good suggestion.

We should also fix up aspeed_clk_is_enabled() for clocks that have
CLK_GATE_SET_TO_DISABLE set.

Cheers,

Joel