ARM: imx: clk-pllv3: change wait method for PLL lock

arch/arm/mach-imx/clk-pllv3.c static int clk_pllv3_prepare(struct clk_hw *hw) · line 70

Quoted 11 lines · Peter Chen

arch/arm/mach-imx/clk-pllv3.c +4 -2

@@ -62,9 +63,11 @@ static int clk_pllv3_prepare(struct clk_hw *hw)
 	writel_relaxed(val, pll->base);
 
 	/* Wait for PLL to lock */
-	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK))
-		if (time_after(jiffies, timeout))
+	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) {
+		udelay(100);
+		if (--count == 0)
 			return -ETIMEDOUT;
+	}

This is still buggy in the ways you describe above.

	do {
		if (readl_relaxed(pll->base) & BM_PLL_LOCK)
			break;
		udelay(100);
	} while (--count);

	if (count == 0 && !(readl_relaxed(pll->base) & BM_PLL_LOCK))
		return -ETIMEDOUT;

Notice - we only return -ETIMEDOUT if the condition we're waiting for has not been satisfied _after_ the loop terminates, specifically, if this happens during the last 100us of our wait.

You can apply the same fix to your original; you don't need to move to using udelay() and a counter if you can tolerate some noise in the waiting time.

The lesson here is: if you're waiting for any kind of an event, then be very careful how you code the failure path so you don't miss a success coincident with the timeout condition becoming true.

For tickless system, the jiffies may be updated long time (>20ms).

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Signed-off-by Peter Chen <[email protected]>

---
 arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

arch/arm/mach-imx/clk-pllv3.c +2 -1

index 36aac94..eefc6c2 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 #include "clk.h"
 
 #define PLL_NUM_OFFSET		0x10

@@ -50,7 +51,7 @@ struct clk_pllv3 {
 static int clk_pllv3_prepare(struct clk_hw *hw)
 {
 	struct clk_pllv3 *pll = to_clk_pllv3(hw);
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	int count = 100;
 	u32 val;
 
 	val = readl_relaxed(pll->base);

Hello

[added jstultz to Cc:]

Quoted 1 line · Peter Chen

For tickless system, the jiffies may be updated long time (>20ms).

... may not be updated for a long time ... ?

Quoted 6 lines

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Hmm, I admit I didn't follow the tickless stuff, but still I wonder if the analysis is right. I thought on tickless jiffies are updated as before by the boot cpu that cannot run in tickless mode?

Anyhow, this only affects the commit log, not the problem.

Quoted 18 lines · Uwe Kleine-K?nig

Hello

[added jstultz to Cc:]

Quoted 1 line · Peter Chen

For tickless system, the jiffies may be updated long time (>20ms).

... may not be updated for a long time ... ?

Quoted 6 lines

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Hmm, I admit I didn't follow the tickless stuff, but still I wonder if the analysis is right. I thought on tickless jiffies are updated as before by the boot cpu that cannot run in tickless mode?

Quoted 2 lines

Anyhow, this only affects the commit log, not the problem.

I add jiffies print at irq_exit (kernel/softirq.c), and it is not updated every jiffies.

Meanwhile, I ran out this PLL Lock timeout issue with iperf test using usb ethernet gadget at current code (v3.5.7 kernel). After using udelay, this issue is disappeared.

Signature · 2 lines

Best Regards,
Peter Chen

Quoted 22 lines · Peter Chen

Quoted 20 lines · Uwe Kleine-K?nig

Hello

[added jstultz to Cc:]

Quoted 1 line · Peter Chen

For tickless system, the jiffies may be updated long time (>20ms).

... may not be updated for a long time ... ?

Quoted 6 lines

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Hmm, I admit I didn't follow the tickless stuff, but still I wonder if the analysis is right. I thought on tickless jiffies are updated as before by the boot cpu that cannot run in tickless mode?

Anyhow, this only affects the commit log, not the problem.

Quoted 2 lines

I add jiffies print at irq_exit (kernel/softirq.c), and it is not updated every jiffies.

Oh, I have not considered cpu idle condition. I think this problem may occur the condition that irq and softirq consume too much time.

Hello

[added jstultz to Cc:]

Quoted 1 line · Peter Chen

For tickless system, the jiffies may be updated long time (>20ms).

... may not be updated for a long time ... ?

Quoted 6 lines

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Hmm, I admit I didn't follow the tickless stuff, but still I wonder if the analysis is right. I thought on tickless jiffies are updated as before by the boot cpu that cannot run in tickless mode?

Anyhow, this only affects the commit log, not the problem.

Quoted 27 lines

Signed-off-by Peter Chen <[email protected]>

---
  arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
  1 files changed, 6 insertions(+), 3 deletions(-)

arch/arm/mach-imx/clk-pllv3.c +2 -1

index 36aac94..eefc6c2 100644
@@ -16,6 +16,7 @@
  #include <linux/slab.h>
  #include <linux/jiffies.h>
  #include <linux/err.h>
+#include <linux/delay.h>
  #include "clk.h"
 
  #define PLL_NUM_OFFSET		0x10

@@ -50,7 +51,7 @@ struct clk_pllv3 {
  static int clk_pllv3_prepare(struct clk_hw *hw)
  {
  	struct clk_pllv3 *pll = to_clk_pllv3(hw);
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	int count = 100;
  	u32 val;
 
  	val = readl_relaxed(pll->base);

arch/arm/mach-imx/clk-pllv3.c static int clk_pllv3_prepare(struct clk_hw *hw) · line 70

Quoted 10 lines

arch/arm/mach-imx/clk-pllv3.c +4 -2

@@ -62,9 +63,11 @@ static int clk_pllv3_prepare(struct clk_hw *hw)
  	writel_relaxed(val, pll->base);
 
  	/* Wait for PLL to lock */
-	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK))
-		if (time_after(jiffies, timeout))
+	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) {
+		udelay(100);
+		if (--count == 0)
  			return -ETIMEDOUT;
+	}

Maybe it's enough to do timeout = jiffies + msecs_to_jiffies(10); just after the pll is reprogrammed? i.e.

arch/arm/mach-imx/clk-pllv3.c +3 -1

index d09bc3d..37f734e 100644
@@ -48,7 +48,7 @@ struct clk_pllv3 {
  static int clk_pllv3_prepare(struct clk_hw *hw)
  {
  	struct clk_pllv3 *pll = to_clk_pllv3(hw);
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	unsigned long timeout;
  	u32 val;
 
  	val = readl_relaxed(pll->base);

@@ -59,6 +59,8 @@ static int clk_pllv3_prepare(struct clk_hw *hw)
  		val &= ~BM_PLL_POWER;
  	writel_relaxed(val, pll->base);
 
+	timeout = jiffies + msecs_to_jiffies(10);
+
  	/* Wait for PLL to lock */
  	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK))
  		if (time_after(jiffies, timeout))

Then at least the pll tries to look while the process is interrupted.

What is msecs_to_jiffies(10) for you? John, would you expect the problem here that Peter describes?

Quoted 77 lines · Uwe Kleine-K?nig

Hello

[added jstultz to Cc:]

Quoted 1 line · Peter Chen

For tickless system, the jiffies may be updated long time (>20ms).

... may not be updated for a long time ... ?

Quoted 6 lines

At high loading system, the current waiting method will cause waiting timeout, and cause a kernel dump at below case.
After timeout = jiffies + msecs_to_jiffies(10),
the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), then return back from interrupt, the time between above operations are tiny, the PLL is still not locked, but the timeout condition is satisfied.

Hmm, I admit I didn't follow the tickless stuff, but still I wonder if the analysis is right. I thought on tickless jiffies are updated as before by the boot cpu that cannot run in tickless mode?

Anyhow, this only affects the commit log, not the problem.

Quoted 25 lines

Signed-off-by Peter Chen <[email protected]>

---
 arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

arch/arm/mach-imx/clk-pllv3.c +2 -1

index 36aac94..eefc6c2 100644
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 #include "clk.h"
 #define PLL_NUM_OFFSET		0x10

@@ -50,7 +51,7 @@ struct clk_pllv3 {
 static int clk_pllv3_prepare(struct clk_hw *hw)
 {
 	struct clk_pllv3 *pll = to_clk_pllv3(hw);
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	int count = 100;
 	u32 val;
 	val = readl_relaxed(pll->base);

arch/arm/mach-imx/clk-pllv3.c static int clk_pllv3_prepare(struct clk_hw *hw) · line 69

Quoted 9 lines

arch/arm/mach-imx/clk-pllv3.c +4 -2

@@ -62,9 +63,11 @@ static int clk_pllv3_prepare(struct clk_hw *hw)
 	writel_relaxed(val, pll->base);
 	/* Wait for PLL to lock */
-	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK))
-		if (time_after(jiffies, timeout))
+	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) {
+		udelay(100);
+		if (--count == 0)
 			return -ETIMEDOUT;
+	}

Maybe it's enough to do timeout = jiffies + msecs_to_jiffies(10); just after the pll is reprogrammed? i.e.

arch/arm/mach-imx/clk-pllv3.c +3 -1

index d09bc3d..37f734e 100644
@@ -48,7 +48,7 @@ struct clk_pllv3 {
 static int clk_pllv3_prepare(struct clk_hw *hw)
 {
 	struct clk_pllv3 *pll = to_clk_pllv3(hw);
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	unsigned long timeout;
 	u32 val;
 	val = readl_relaxed(pll->base);

@@ -59,6 +59,8 @@ static int clk_pllv3_prepare(struct clk_hw *hw)
 		val &= ~BM_PLL_POWER;
 	writel_relaxed(val, pll->base);
+	timeout = jiffies + msecs_to_jiffies(10);
+
 	/* Wait for PLL to lock */
 	while (!(readl_relaxed(pll->base) & BM_PLL_LOCK))
 		if (time_after(jiffies, timeout))
 

Quoted 4 lines

Then at least the pll tries to look while the process is interrupted.

What is msecs_to_jiffies(10) for you? John, would you expect the problem here that Peter describes?

No, I'm not sure I see how being tickless would cause such a problem (Adding Frederic here in case he spots something).

The only issues that pop up for me right away is:
1) The one Uwe noted, where jiffies may be incremented from the early timeout assignment to before the wait loop begins.

2) That at HZ=100, msecs_to_jiffies(10) is just 1, so if you were to catch jiffies right before it was updated, you would end up waiting only 10ms before timing out. That's still seems like plenty of time, but I don't know the hardware details here.

thanks
-john