| Message ID | 1370593146-14025-1-git-send-email-peter.chen@freescale.com |
|---|---|
| State | New |
| Headers | show |
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
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/ | >
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
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.
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
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
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.
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.
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; }
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(-)