clk: aspeed: Prevent reset and enable if clock is already enabled

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.

Quoted 4 lines · Eddie James

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.

Well, maybe. Why do we have to avoid this sequence?

Quoted 3 lines

Add a simple check before the
enable procedure to see if the clock is already enabled.

I don't think this actually solves the problem, just papers over it. Can we solve it properly?

Andrew

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 <[email protected]>

---
 drivers/clk/clk-aspeed.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

drivers/clk/clk-aspeed.c +0 -0

index b67ba14..3d12f7a 100644

drivers/clk/clk-aspeed.c +12 -18

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

drivers/clk/clk-aspeed.c +1 -1

+               }
        }
 
        spin_unlock_irqrestore(gate->lock, flags);
--

1.8.3.1

With the patch, the hang issue in ISTEPS (openbmc/openbmc#2948) is gone, in both cold and warm boot.

Tested-by Lei YU <[email protected]>

Quoted 16 lines

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 <[email protected]>

---
 drivers/clk/clk-aspeed.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

drivers/clk/clk-aspeed.c +0 -0

index b67ba14..3d12f7a 100644

drivers/clk/clk-aspeed.c static int aspeed_clk_enable(struct clk_hw *hw) · line 225

Quoted 5 lines

drivers/clk/clk-aspeed.c +1 -0

@@ -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().

Quoted 32 lines

drivers/clk/clk-aspeed.c +12 -18

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

drivers/clk/clk-aspeed.c static int aspeed_clk_enable(struct clk_hw *hw) · line 249

Quoted 10 lines

drivers/clk/clk-aspeed.c +10 -0

+               /* 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.

Quoted 7 lines

drivers/clk/clk-aspeed.c +1 -1

+               }
        }
 
        spin_unlock_irqrestore(gate->lock, flags);
--

1.8.3.1