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

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

[Peter Chen]

For some unknown reasons, the jiffies will be updated more
than one tick at every short time. Eg, at this code:
After timeout = jiffies + msecs_to_jiffies(10),
the interrupt occurs, and the softirq 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>
---
Changes for v2:
- According to Russell King's suggestion, change code for more reasonable
for timeout condition.
- Change commit log.

 arch/arm/mach-imx/clk-pllv3.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
index d09bc3d..52e54df 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
@@ -48,7 +49,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);
@@ -60,9 +61,14 @@ 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))
-			return -ETIMEDOUT;
+	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;
 
 	return 0;
 }
-- 
1.7.0.4

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

[Uwe Kleine-König]

On Fri, Jun 07, 2013 at 04:19:06PM +0800, Peter Chen wrote:
> For some unknown reasons, the jiffies will be updated more
> than one tick at every short time. Eg, at this code:
> After timeout = jiffies + msecs_to_jiffies(10),
> the interrupt occurs, and the softirq 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>
Does this mean my patch doesn't solve the issue for you?

Best regards
Uwe


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

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

[Peter Chen]

On Fri, Jun 07, 2013 at 10:24:57AM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Jun 07, 2013 at 04:19:06PM +0800, Peter Chen wrote:
> > For some unknown reasons, the jiffies will be updated more
> > than one tick at every short time. Eg, at this code:
> > After timeout = jiffies + msecs_to_jiffies(10),
> > the interrupt occurs, and the softirq 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>
> Does this mean my patch doesn't solve the issue for you?

Not try, but your patch just delay timeout assignment, if there
is jiffies update after that but before timeout check, it still
has problem.

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

-- 

Best Regards,
Peter Chen

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

[Uwe Kleine-König]

On Fri, Jun 07, 2013 at 04:31:35PM +0800, Peter Chen wrote:
> On Fri, Jun 07, 2013 at 10:24:57AM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Jun 07, 2013 at 04:19:06PM +0800, Peter Chen wrote:
> > > For some unknown reasons, the jiffies will be updated more
> > > than one tick at every short time. Eg, at this code:
> > > After timeout = jiffies + msecs_to_jiffies(10),
> > > the interrupt occurs, and the softirq 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>
> > Does this mean my patch doesn't solve the issue for you?
> 
> Not try, but your patch just delay timeout assignment, if there
> is jiffies update after that but before timeout check, it still
> has problem.
But if that large update happens because there was a long preemption the
pll should be locked because I only start counting when the pll is
programmed.

IMHO you should try my patch as it is the better fix (assuming it fixes
it for you).

Best regards
Uwe

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

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

[Peter Chen]

On Fri, Jun 07, 2013 at 10:35:28AM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Jun 07, 2013 at 04:31:35PM +0800, Peter Chen wrote:
> > On Fri, Jun 07, 2013 at 10:24:57AM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, Jun 07, 2013 at 04:19:06PM +0800, Peter Chen wrote:
> > > > For some unknown reasons, the jiffies will be updated more
> > > > than one tick at every short time. Eg, at this code:
> > > > After timeout = jiffies + msecs_to_jiffies(10),
> > > > the interrupt occurs, and the softirq 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>
> > > Does this mean my patch doesn't solve the issue for you?
> > 
> > Not try, but your patch just delay timeout assignment, if there
> > is jiffies update after that but before timeout check, it still
> > has problem.
> But if that large update happens because there was a long preemption the
> pll should be locked because I only start counting when the pll is
> programmed.
> 
> IMHO you should try my patch as it is the better fix (assuming it fixes
> it for you).

I will try your fix, but still it just reduces the possibilities.
The problem is not the preemption takes too long, it is the jiffies
updates more than one tick at one short preemption.

-- 

Best Regards,
Peter Chen

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

[Uwe Kleine-König]

Hello Peter,

On Fri, Jun 07, 2013 at 04:43:55PM +0800, Peter Chen wrote:
> On Fri, Jun 07, 2013 at 10:35:28AM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Jun 07, 2013 at 04:31:35PM +0800, Peter Chen wrote:
> > > On Fri, Jun 07, 2013 at 10:24:57AM +0200, Uwe Kleine-K?nig wrote:
> > > > On Fri, Jun 07, 2013 at 04:19:06PM +0800, Peter Chen wrote:
> > > > > For some unknown reasons, the jiffies will be updated more
> > > > > than one tick at every short time. Eg, at this code:
> > > > > After timeout = jiffies + msecs_to_jiffies(10),
> > > > > the interrupt occurs, and the softirq 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>
> > > > Does this mean my patch doesn't solve the issue for you?
> > > 
> > > Not try, but your patch just delay timeout assignment, if there
> > > is jiffies update after that but before timeout check, it still
> > > has problem.
> > But if that large update happens because there was a long preemption the
> > pll should be locked because I only start counting when the pll is
> > programmed.
> > 
> > IMHO you should try my patch as it is the better fix (assuming it fixes
> > it for you).
> 
> I will try your fix, but still it just reduces the possibilities.
> The problem is not the preemption takes too long, it is the jiffies
> updates more than one tick at one short preemption.
If that is really the problem that many more instances that use the same
incarnation need the same fix. I would be surprised if that was the
case.

Best regards
Uwe

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

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

[Shawn Guo]

On Fri, Jun 07, 2013 at 10:49:38AM +0200, Uwe Kleine-K?nig wrote:
> > I will try your fix, but still it just reduces the possibilities.
> > The problem is not the preemption takes too long, it is the jiffies
> > updates more than one tick at one short preemption.
> If that is really the problem that many more instances that use the same
> incarnation need the same fix. I would be surprised if that was the
> case.

+1

We have plenty of timeout code written like that in the kernel.

Shawn

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

[Peter Chen]

On Fri, Jun 07, 2013 at 07:07:43PM +0800, Shawn Guo wrote:
> On Fri, Jun 07, 2013 at 10:49:38AM +0200, Uwe Kleine-K?nig wrote:
> > > I will try your fix, but still it just reduces the possibilities.
> > > The problem is not the preemption takes too long, it is the jiffies
> > > updates more than one tick at one short preemption.
> > If that is really the problem that many more instances that use the same
> > incarnation need the same fix. I would be surprised if that was the
> > case.
> 
> +1
> 

Using uwe's patch, the pll lock timeout hasn't appeared during the
overtime test, usually, it will occur 4 or 5 times during overnight
test. The reason why I suspect jiffies update problem that is we
meet the similiar issue at other drivers which timeout is 2 jiffies,
but it is satisfied within 1ms.

I will do more test, if it is passed, I will send patch with uwe's suggestion.
Thanks.

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

On Sat, Jun 08, 2013 at 09:09:23AM +0800, Peter Chen wrote:
> On Fri, Jun 07, 2013 at 07:07:43PM +0800, Shawn Guo wrote:
> > On Fri, Jun 07, 2013 at 10:49:38AM +0200, Uwe Kleine-K?nig wrote:
> > > > I will try your fix, but still it just reduces the possibilities.
> > > > The problem is not the preemption takes too long, it is the jiffies
> > > > updates more than one tick at one short preemption.
> > > If that is really the problem that many more instances that use the same
> > > incarnation need the same fix. I would be surprised if that was the
> > > case.
> > 
> > +1
> > 
> 
> Using uwe's patch, the pll lock timeout hasn't appeared during the
> overtime test, usually, it will occur 4 or 5 times during overnight
> test. The reason why I suspect jiffies update problem that is we
> meet the similiar issue at other drivers which timeout is 2 jiffies,
> but it is satisfied within 1ms.
> 
> I will do more test, if it is passed, I will send patch with uwe's suggestion.
> Thanks.

The root cause of this problem is timer problem, Jason has already submitted a patch
to fix this problem.

http://marc.info/?l=linux-arm-kernel&m=137109340222931&w=2

I will send a improvement patch with uwe's suggestion.

-- 

Best Regards,
Peter Chen