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

login
register
mail settings
Submitter Uwe Kleine-König
Date June 6, 2013, 12:16 p.m.
Message ID <20130606121647.GR23140@pengutronix.de>
Download mbox | patch
Permalink /patch/249422/
State New
Headers show

Comments

Uwe Kleine-König - June 6, 2013, 12:16 p.m.
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. 


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
Peter Chen - June 7, 2013, 2:53 a.m.
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.
Peter Chen - June 7, 2013, 3:33 a.m.
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
Uwe Kleine-König - June 7, 2013, 7:49 a.m.
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
John Stultz - June 7, 2013, 4:47 p.m.
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
Frédéric Weisbecker - June 13, 2013, 1:32 p.m.
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
>

Patch

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