Skip to contentSkip to conversation

ARM: imx: fix shared gate clock

PATCH New Shawn Guo 3 participants 4 messages Jul 7, 2014 – Jul 8, 2014

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape
lore
Share & tools
lore

Shawn Guo

author
#0 lore raw

PATCH 1/1 ARM: imx: fix shared gate clock

Let's say clock A and B are two gate clocks that share the same register bit in hardware. Therefore they are registered as shared gate clocks with imx_clk_gate2_shared().

In a scenario that only clock A is enabled by clk_enable(A) while B is not used, the shared gate will be unexpectedly disabled in hardware. It happens because clk_enable(A) increments the share_count from 0 to 1, while clock B is unused to clock core, and therefore the core function will just disable B by calling clk->ops->disable() directly. The consequence of that call is share_count is decremented to 0 and the gate is disabled in hardware, even though clock A is still in use.

The patch fixes the issue by initializing the share_count per hardware state and returns enable state per share_count from .is_enabled() hook, in case it's a shared gate.

While at it, add a check in clk_gate2_disable() to ensure it's never called with a zero share_count.

Reported-by Fabio Estevam <[email protected]>
Fixes f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by Shawn Guo <[email protected]>

---
Fabio,

Per my testing, it fixes the shared gate clock issue you reported. But I'd like to get your confirmation before I ask arm-soc folks to apply it for 3.16-rc.

Shawn

 arch/arm/mach-imx/clk-gate2.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)
arch/arm/mach-imx/clk-gate2.c +23 -8
index 4ba587da89d2..84acdfd1d715 100644
@@ -67,8 +67,12 @@ static void clk_gate2_disable(struct clk_hw *hw)
 
 	spin_lock_irqsave(gate->lock, flags);
 
-	if (gate->share_count && --(*gate->share_count) > 0)
-		goto out;
+	if (gate->share_count) {
+		if (WARN_ON(*gate->share_count == 0))
+			goto out;
+		else if (--(*gate->share_count) > 0)
+			goto out;
+	}
 
 	reg = readl(gate->reg);
 	reg &= ~(3 << gate->bit_idx);

@@ -78,19 +82,26 @@ out:
 	spin_unlock_irqrestore(gate->lock, flags);
 }
 
-static int clk_gate2_is_enabled(struct clk_hw *hw)
+static int clk_gate2_reg_is_enabled(void __iomem *reg, u8 bit_idx)
 {
-	u32 reg;
-	struct clk_gate2 *gate = to_clk_gate2(hw);
+	u32 val = readl(reg);
 
-	reg = readl(gate->reg);
-
-	if (((reg >> gate->bit_idx) & 1) == 1)
+	if (((val >> bit_idx) & 1) == 1)
 		return 1;
 
 	return 0;
 }
 
+static int clk_gate2_is_enabled(struct clk_hw *hw)
+{
+	struct clk_gate2 *gate = to_clk_gate2(hw);
+
+	if (gate->share_count)
+		return !!(*gate->share_count);
+	else
+		return clk_gate2_reg_is_enabled(gate->reg, gate->bit_idx);
+}
+
 static struct clk_ops clk_gate2_ops = {
 	.enable = clk_gate2_enable,
 	.disable = clk_gate2_disable,

@@ -116,6 +127,10 @@ struct clk *clk_register_gate2(struct device *dev, const char *name,
 	gate->bit_idx = bit_idx;
 	gate->flags = clk_gate2_flags;
 	gate->lock = lock;
+
+	/* Initialize share_count per hardware state */
+	if (share_count)
+		*share_count = clk_gate2_reg_is_enabled(reg, bit_idx) ? 1 : 0;
 	gate->share_count = share_count;
 
 	init.name = name;
Signature · 1 line
1.8.3.2
reviewer
#1 lore raw

PATCH 1/1 ARM: imx: fix shared gate clock

Quoted 22 lines · Shawn Guo

Let's say clock A and B are two gate clocks that share the same register bit in hardware. Therefore they are registered as shared gate clocks with imx_clk_gate2_shared().

In a scenario that only clock A is enabled by clk_enable(A) while B is not used, the shared gate will be unexpectedly disabled in hardware. It happens because clk_enable(A) increments the share_count from 0 to 1, while clock B is unused to clock core, and therefore the core function will just disable B by calling clk->ops->disable() directly. The consequence of that call is share_count is decremented to 0 and the gate is disabled in hardware, even though clock A is still in use.

The patch fixes the issue by initializing the share_count per hardware state and returns enable state per share_count from .is_enabled() hook, in case it's a shared gate.

While at it, add a check in clk_gate2_disable() to ensure it's never called with a zero share_count.

Reported-by Fabio Estevam <[email protected]>
Fixes f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by Shawn Guo <[email protected]>
Quoted 6 lines

---
Fabio,

Per my testing, it fixes the shared gate clock issue you reported. But I'd like to get your confirmation before I ask arm-soc folks to apply it for 3.16-rc.

I can get audio working now and I have also inspected CCGR5 register and it looks correct now:

Tested-by Fabio Estevam <[email protected]>
author
#2 lore raw

PATCH 1/1 ARM: imx: fix shared gate clock

Quoted 26 lines · Fabio Estevam
Quoted 25 lines · Shawn Guo

Let's say clock A and B are two gate clocks that share the same register bit in hardware. Therefore they are registered as shared gate clocks with imx_clk_gate2_shared().

In a scenario that only clock A is enabled by clk_enable(A) while B is not used, the shared gate will be unexpectedly disabled in hardware. It happens because clk_enable(A) increments the share_count from 0 to 1, while clock B is unused to clock core, and therefore the core function will just disable B by calling clk->ops->disable() directly. The consequence of that call is share_count is decremented to 0 and the gate is disabled in hardware, even though clock A is still in use.

The patch fixes the issue by initializing the share_count per hardware state and returns enable state per share_count from .is_enabled() hook, in case it's a shared gate.

While at it, add a check in clk_gate2_disable() to ensure it's never called with a zero share_count.

Reported-by Fabio Estevam <[email protected]>
Fixes f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by Shawn Guo <[email protected]>

---
Fabio,

Quoted 8 lines
Quoted 3 lines

Per my testing, it fixes the shared gate clock issue you reported. But I'd like to get your confirmation before I ask arm-soc folks to apply it for 3.16-rc.

I can get audio working now and I have also inspected CCGR5 register and it looks correct now:

Tested-by Fabio Estevam <[email protected]>

Thanks, Fabio.

Arnd, Olof,

Can you please apply this patch for 3.16?

Shawn

#3 lore raw

PATCH 1/1 ARM: imx: fix shared gate clock

Quoted 34 lines · Shawn Guo
Quoted 33 lines · Fabio Estevam
Quoted 22 lines · Shawn Guo

Let's say clock A and B are two gate clocks that share the same register bit in hardware. Therefore they are registered as shared gate clocks with imx_clk_gate2_shared().

In a scenario that only clock A is enabled by clk_enable(A) while B is not used, the shared gate will be unexpectedly disabled in hardware. It happens because clk_enable(A) increments the share_count from 0 to 1, while clock B is unused to clock core, and therefore the core function will just disable B by calling clk->ops->disable() directly. The consequence of that call is share_count is decremented to 0 and the gate is disabled in hardware, even though clock A is still in use.

The patch fixes the issue by initializing the share_count per hardware state and returns enable state per share_count from .is_enabled() hook, in case it's a shared gate.

While at it, add a check in clk_gate2_disable() to ensure it's never called with a zero share_count.

Reported-by Fabio Estevam <[email protected]>
Fixes f9f28cdf2167 ("ARM: imx: add shared gate clock support")
Signed-off-by Shawn Guo <[email protected]>
Quoted 6 lines

---
Fabio,

Per my testing, it fixes the shared gate clock issue you reported. But I'd like to get your confirmation before I ask arm-soc folks to apply it for 3.16-rc.

I can get audio working now and I have also inspected CCGR5 register and it looks correct now:

Quoted 7 lines
Quoted 1 line
Tested-by Fabio Estevam <[email protected]>

Thanks, Fabio.

Arnd, Olof,

Can you please apply this patch for 3.16?

Done, thanks.

-Olof