clk: aspeed: Fix is_enabled and prevent reset if clock enabled

Some of the Aspeed clocks are disabled by setting the relevant bit in the "clock stop control" register to one, while others are disabled by setting their bit to zero. The driver already uses a flag per gate to identify this behavior, but doesn't apply it in the clock is_enabled function.

Use the existing gate flag to correctly return whether or not a clock is enabled in the aspeed_clk_is_enabled function.

Signed-off-by Eddie James <[email protected]>

---
 drivers/clk/clk-aspeed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index 9f7f931..1687771 100644
@@ -259,11 +259,12 @@ 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, &reg);
 
-       return (reg & clk) ? 0 : 1;
+       return ((reg & clk) == enval) ? 1 : 0;
 }
 
 static const struct clk_ops aspeed_clk_gate_ops = {
--

1.8.3.1

Some of the Aspeed clocks are disabled by setting the relevant bit in the "clock stop control" register to one, while others are disabled by setting their bit to zero. The driver already uses a flag per gate to identify this behavior, but doesn't apply it in the clock is_enabled function.
=

Use the existing gate flag to correctly return whether or not a clock is enabled in the aspeed_clk_is_enabled function.
=

Signed-off-by Eddie James <[email protected]>

---

From testing on the AST2500, the LPC Controller has problems if the

Quoted 10 lines

static int aspeed_clk_enable(struct clk_hw *hw)

 {
        struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);

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

@@ -215,6 +227,11 @@ static int aspeed_clk_enable(struct clk_hw *hw)
 
        spin_lock_irqsave(gate->lock, flags);
 

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

Quoted 5 lines

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

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

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

Signed-off-by Eddie James <[email protected]>

---
 drivers/clk/clk-aspeed.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/clk/clk-aspeed.c +17 -13

index 1687771..5eb50c3 100644
@@ -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, &reg);
+
+       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, &reg);
-
-       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

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

Signed-off-by Eddie James <[email protected]>

---

Here are two fixes for the Aspeed clock driver. The first fixes the is_en=

clock function to account for different clock gates getting disabled with either 0s or 1s. The second patch addresses some issue found with the LPC controller clock if it gets reset while the clock is enabled, which it is=

default. Thanks to Lei Yu for tracking down the LPC issue. =

Quoting Eddie James (2018-03-08 12:57:18)

Quoted 6 lines

Here are two fixes for the Aspeed clock driver. The first fixes the is_enabled clock function to account for different clock gates getting disabled with either 0s or 1s. The second patch addresses some issue found with the LPC controller clock if it gets reset while the clock is enabled, which it is by default. Thanks to Lei Yu for tracking down the LPC issue.

Can you please add some "Fixes:" tags to these patches? Or just indicate the tags and I can pick them up. If it doesn't happen, I'll just apply these early next week.