[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Peter Chen]

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 <peter.chen@freescale.com>
---
 arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
index 36aac94..eefc6c2 100644
--- a/arch/arm/mach-imx/clk-pllv3.c
+++ b/arch/arm/mach-imx/clk-pllv3.c
@@ -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);
@@ -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;
+	}
 
 	return 0;
 }
-- 
1.7.0.4

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Russell King - ARM Linux]

On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> @@ -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.

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Peter Chen]

On Thu, Jun 06, 2013 at 10:21:56AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> > @@ -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.

Thanks for your comments, it can make code be more reasonable.

> 
> 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.
> 

-- 

Best Regards,
Peter Chen

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Uwe Kleine-König]

Hello

[added jstultz to Cc:]

On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> For tickless system, the jiffies may be updated long time (>20ms).
... may not be updated for a long time ... ?

> 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.
 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> index 36aac94..eefc6c2 100644
> --- a/arch/arm/mach-imx/clk-pllv3.c
> +++ b/arch/arm/mach-imx/clk-pllv3.c
> @@ -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);
> @@ -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. 

diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
index d09bc3d..37f734e 100644
--- a/arch/arm/mach-imx/clk-pllv3.c
+++ b/arch/arm/mach-imx/clk-pllv3.c
@@ -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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Peter Chen]

On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-K?nig wrote:
> Hello
> 
> [added jstultz to Cc:]
> 
> On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> > For tickless system, the jiffies may be updated long time (>20ms).
> ... may not be updated for a long time ... ?
> 
> > 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.
>  

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.

-- 

Best Regards,
Peter Chen

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Peter Chen]

On Fri, Jun 07, 2013 at 10:53:20AM +0800, Peter Chen wrote:
> On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-K?nig wrote:
> > Hello
> > 
> > [added jstultz to Cc:]
> > 
> > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> > > For tickless system, the jiffies may be updated long time (>20ms).
> > ... may not be updated for a long time ... ?
> > 
> > > 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.
> >  
> 
> 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.

> 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.
> 
> -- 
> 
> Best Regards,
> Peter Chen

-- 

Best Regards,
Peter Chen

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Uwe Kleine-König]

On Fri, Jun 07, 2013 at 11:33:07AM +0800, Peter Chen wrote:
> On Fri, Jun 07, 2013 at 10:53:20AM +0800, Peter Chen wrote:
> > On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-K?nig wrote:
> > > Hello
> > > 
> > > [added jstultz to Cc:]
> > > 
> > > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> > > > For tickless system, the jiffies may be updated long time (>20ms).
> > > ... may not be updated for a long time ... ?
> > > 
> > > > 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.
> > >  
> > 
> > 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.
That was my thought, too. With my patch this time at least only happens
after the pll is programmed and the issue might be solved, too?! (Maybe
apart from Russell's comment.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[John Stultz]

On 06/06/2013 05:16 AM, Uwe Kleine-K?nig wrote:
> Hello
>
> [added jstultz to Cc:]
>
> On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
>> For tickless system, the jiffies may be updated long time (>20ms).
> ... may not be updated for a long time ... ?
>
>> 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.
>   
>> Signed-off-by: Peter Chen <peter.chen@freescale.com>
>> ---
>>   arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
>>   1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
>> index 36aac94..eefc6c2 100644
>> --- a/arch/arm/mach-imx/clk-pllv3.c
>> +++ b/arch/arm/mach-imx/clk-pllv3.c
>> @@ -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);
>> @@ -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.
>
> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> index d09bc3d..37f734e 100644
> --- a/arch/arm/mach-imx/clk-pllv3.c
> +++ b/arch/arm/mach-imx/clk-pllv3.c
> @@ -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?

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

[PATCH 1/1] ARM: imx: clk-pllv3: change wait method for PLL lock

[Frederic Weisbecker]

On Fri, Jun 07, 2013 at 09:47:26AM -0700, John Stultz wrote:
> On 06/06/2013 05:16 AM, Uwe Kleine-K?nig wrote:
> >Hello
> >
> >[added jstultz to Cc:]
> >
> >On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote:
> >>For tickless system, the jiffies may be updated long time (>20ms).
> >... may not be updated for a long time ... ?
> >
> >>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.
> >>Signed-off-by: Peter Chen <peter.chen@freescale.com>
> >>---
> >>  arch/arm/mach-imx/clk-pllv3.c |    9 ++++++---
> >>  1 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> >>index 36aac94..eefc6c2 100644
> >>--- a/arch/arm/mach-imx/clk-pllv3.c
> >>+++ b/arch/arm/mach-imx/clk-pllv3.c
> >>@@ -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);
> >>@@ -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.
> >
> >diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> >index d09bc3d..37f734e 100644
> >--- a/arch/arm/mach-imx/clk-pllv3.c
> >+++ b/arch/arm/mach-imx/clk-pllv3.c
> >@@ -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?
> 
> No, I'm not sure I see how being tickless would cause such a problem
> (Adding Frederic here in case he spots something).

I must confess I don't understand very well the problem description :-s

> 
> 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
>