| Message ID | 1370501726-7421-1-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. >
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; }
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(-)