[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[Peter Chen]

It can let the timeout value be more precise.

Suggested-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/mach-imx/clk-pllv3.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
index a9fad5f..e2c026f 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,7 @@ 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))
-- 
1.7.0.4

[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[Uwe Kleine-König]

On Sun, Jul 14, 2013 at 08:58:48AM +0800, Peter Chen wrote:
> It can let the timeout value be more precise.
> 
> Suggested-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Whoever applies this patch, can you make the ? an ? (or at least "oe")?

Peter, does this fix anything for you? Would be nice to mention that in
the commit log.

Best regards
Uwe

> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  arch/arm/mach-imx/clk-pllv3.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> index a9fad5f..e2c026f 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,7 @@ 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))
> -- 
> 1.7.0.4
> 
> 
> 

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

[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[Peter Chen]

On Sun, Jul 14, 2013 at 08:33:47PM +0200, Uwe Kleine-K?nig wrote:
> On Sun, Jul 14, 2013 at 08:58:48AM +0800, Peter Chen wrote:
> > It can let the timeout value be more precise.
> > 
> > Suggested-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Whoever applies this patch, can you make the ? an ? (or at least "oe")?

Oh, sorry.

> 
> Peter, does this fix anything for you? Would be nice to mention that in
> the commit log.

The root cause of this PLL lock timeout problem is:

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

If causes the jiffies is not precise sometimes. This commit is just an
improvement.

> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  arch/arm/mach-imx/clk-pllv3.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> > index a9fad5f..e2c026f 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,7 @@ 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))
> > -- 
> > 1.7.0.4
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 

Best Regards,
Peter Chen

[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[Fabio Estevam]

Hi Peter,

On Sun, Jul 14, 2013 at 10:47 PM, Peter Chen <peter.chen@freescale.com> wrote:

> The root cause of this PLL lock timeout problem is:
>
> http://marc.info/?l=linux-arm-kernel&m=137109340222931&w=2
>
> If causes the jiffies is not precise sometimes. This commit is just an
> improvement.

Your commit log does not explain what it improves, and why/how it
improves things.

Please provide a better description in the commit log.

[PATCH 1/1] ARM: imx: clk-pllv3: move the timeout just before the comparison

[Sascha Hauer]

On Sun, Jul 14, 2013 at 08:58:48AM +0800, Peter Chen wrote:
> It can let the timeout value be more precise.
> 
> Suggested-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  arch/arm/mach-imx/clk-pllv3.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c
> index a9fad5f..e2c026f 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,7 @@ 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))

For a proper timeout loop you have to check the condition once after the
timeout, like this:

	while (1) {
		if (condition)
			break;
		if (time_after())
			break;
	}

	if (condition)
		res = OK;
	else
		res = -ETIMEDOUT;

Otherwise you risk being scheduled away after checking the condition but
before checking the timeout.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |