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 <[email protected]>
---
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, ®);
+ if (!(reg & clk))
+ return 0;
spin_lock_irqsave(gate->lock, flags);
--
1.8.3.1
Hi Eddie,
On Thu, Mar 8, 2018 at 3:06 AM, Eddie James <[email protected]> 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 <[email protected]>
> ---
> 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, ®);
> + 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, ®);
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 <[email protected]> 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
On Thu, Mar 8, 2018 at 2:12 PM, Joel Stanley <[email protected]> wrote:
> Hi Eddie,
>
> On Thu, Mar 8, 2018 at 3:06 AM, Eddie James <[email protected]> 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 <[email protected]>
>> ---
>> 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, ®);
>> + 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, ®);
> 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 <[email protected]> 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
On Thu, Mar 8, 2018 at 11:42 AM, Joel Stanley <[email protected]> wrote:
>> + /* Only reset/enable/unreset if clock is stopped */
>> + regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, ®);
>> + 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, ®);
> 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 <[email protected]> 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;
On Thu, Mar 8, 2018 at 4:08 PM, Lei YU <[email protected]> wrote:
> On Thu, Mar 8, 2018 at 11:42 AM, Joel Stanley <[email protected]> wrote:
>>> + /* Only reset/enable/unreset if clock is stopped */
>>> + regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, ®);
>>> + 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, ®);
>> 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 <[email protected]> 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