[PATCH linux dev-4.13] clk: aspeed: Prevent reset and enable if clock is already enabled

[Joel Stanley]

Hi Eddie,

On Wed, Mar 7, 2018 at 1:03 PM, Lei YU <mine260309 at gmail.com> wrote:
> With the patch, the hang issue in ISTEPS (openbmc/openbmc#2948) is gone, in both
> cold and warm boot.
>
> Tested-by: Lei YU <mine260309 at gmail.com>

Please send this upstream after addressing Lei's concerns. Can you
also add a Fixes tag?

Cheers,

Joel
>
> On Wed, Mar 7, 2018 at 4:24 AM, Eddie James <eajames at linux.vnet.ibm.com> wrote:
>> Systems are seeing some issues where the LPC clock gets into a bad state
>> when the ASPEED clock driver resets and enables the clock during
>> startup. The LPC clock (according to AST2500 spec) is enabled by default,
>> so resetting and re-enabling is not desired. Add a simple check before the
>> enable procedure to see if the clock is already enabled.
>>
>> Signed-off-by: Eddie James <eajames at linux.vnet.ibm.com>
>> ---
>>  drivers/clk/clk-aspeed.c | 42 ++++++++++++++++++++++++------------------
>>  1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
>> index b67ba14..3d12f7a 100644
>> --- a/drivers/clk/clk-aspeed.c
>> +++ b/drivers/clk/clk-aspeed.c
>> @@ -221,27 +221,33 @@ 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;
>>
>
> I would prefer to call aspeed_clk_is_enabled() to check if it's
> enabled and return
> before spin_lock_irqsave().
>
>>         spin_lock_irqsave(gate->lock, flags);
>>
>> -       if (gate->reset_idx >= 0) {
>> -               /* Put IP in reset */
>> -               regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
>> -
>> -               /* Delay 100us */
>> -               udelay(100);
>> -       }
>> -
>> -       /* Enable clock */
>> -       enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>> -       regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, enval);
>> -
>> -       if (gate->reset_idx >= 0) {
>> -               /* A delay of 10ms is specified by the ASPEED docs */
>> -               mdelay(10);
>> -
>> -               /* Take IP out of reset */
>> -               regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
>> +       /* Only reset/enable/unreset if clock isn't already enabled */
>> +       regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
>> +       if (reg & clk) {
>> +               if (gate->reset_idx >= 0) {
>> +                       /* Put IP in reset */
>> +                       regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst,
>> +                                          rst);
>> +
>> +                       /* Delay 100us */
>> +                       udelay(100);
>> +               }
>> +
>> +               /* Enable clock */
>> +               enval = (gate->flags & CLK_GATE_SET_TO_DISABLE) ? 0 : clk;
>> +               regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, enval);
>> +
>> +               if (gate->reset_idx >= 0) {
>> +                       /* A delay of 10ms is specified by the ASPEED docs */
>> +                       mdelay(10);
>> +
>> +                       /* Take IP out of reset */
>> +                       regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst,                                              0);
>
> Remove the extra blanks or put it in next line.
>
>> +               }
>>         }
>>
>>         spin_unlock_irqrestore(gate->lock, flags);
>> --
>> 1.8.3.1
>>