| Message ID | 1354632915-27134-2-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 04, 2012 at 10:55:12PM +0800, Shawn Guo wrote: > The timer is configured in free-run mode. The counter should be > allowed to roll over to 0 when reaching 0xffffffff. Let's do that > by always returning 0 in set_next_event. Are you sure this is correct? It looks wrong to me. If the time set by the next event has passed, you must return -ETIME from set_next_event() so that the generic timer code can compensate for the missed event and recalculate it, otherwise you will end up having to wait a full count cycle before receiving the next event. If you find that you're being passed such a small increment that you're constantly hitting that condition, you need to increase your minimum waited interval. > diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c > index f017302..858098c 100644 > --- a/arch/arm/mach-imx/time.c > +++ b/arch/arm/mach-imx/time.c > @@ -139,8 +139,7 @@ static int mx1_2_set_next_event(unsigned long evt, > > __raw_writel(tcmp, timer_base + MX1_2_TCMP); > > - return (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? > - -ETIME : 0; > + return 0; So what this code was doing is: tcmp is the counter value we expect with the expected event time added. This may wrap 32-bits. We subtract the current timer value after we've set the compare register. If the current timer value was larger than the expected event time, we return -ETIME. That to me sounds 100% correct. Your replacement code of always returning zero looks wrong.
On Wed, Dec 05, 2012 at 08:14:51PM +0800, Shawn Guo wrote: > On Tue, Dec 04, 2012 at 03:44:59PM +0000, Russell King - ARM Linux wrote: > > On Tue, Dec 04, 2012 at 10:55:12PM +0800, Shawn Guo wrote: > > > The timer is configured in free-run mode. The counter should be > > > allowed to roll over to 0 when reaching 0xffffffff. Let's do that > > > by always returning 0 in set_next_event. > > > > Are you sure this is correct? It looks wrong to me. > > > > If the time set by the next event has passed, you must return -ETIME > > from set_next_event() so that the generic timer code can compensate > > for the missed event and recalculate it, otherwise you will end up > > having to wait a full count cycle before receiving the next event. > > > > If you find that you're being passed such a small increment that you're > > constantly hitting that condition, you need to increase your minimum > > waited interval. > > I think we already have a proper min_delta_tick setting to ensure we > will not run into the situation that the event will get expired in > v2_set_next_event(). I guess that's the reason why most of the > set_next_event() implementations under arch/arm can always return 0? There are two cases here: there are those where there is a free-running counter which continually increments/decrements, and it is compared to a match value to trigger the next interrupt. There are also those cases where you load the timer with the count value and it counts towards the interrupt trigger value (either up or down). With the former, you should check that the event has not passed before returning, and if it has, you must return -ETIME, particularly if the wrap takes several seconds. With the latter, you're going to time out reasonably close to the desired time anyway, so there's no need to do any checking (you can't in any case.) So in this case, you would always return zero. > > > diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c > > > index f017302..858098c 100644 > > > --- a/arch/arm/mach-imx/time.c > > > +++ b/arch/arm/mach-imx/time.c > > > @@ -139,8 +139,7 @@ static int mx1_2_set_next_event(unsigned long evt, > > > > > > __raw_writel(tcmp, timer_base + MX1_2_TCMP); > > > > > > - return (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? > > > - -ETIME : 0; > > > + return 0; > > > > So what this code was doing is: tcmp is the counter value we expect with > > the expected event time added. This may wrap 32-bits. We subtract the > > current timer value after we've set the compare register. If the current > > timer value was larger than the expected event time, we return -ETIME. > > >From what I have seen, it never happens in event expiring case but > always in counter wrapping. Since the timer is configured in free-run > mode and can keep running after wrapping, it should be safe to always > return zero. Well, let's look at the maths here. Let's say that the counter value is at 0xfffffff0, and the desired timeout is 32. This makes tcmp 0x00000010. Let's say that the counter has only incremented a small amount to 0xfffffff8: (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN) So this becomes 0x10 - 0xfffffff8, which will be 0x18. So the return value in this case will be zero. Now lets consider what happens if the timer has wrapped to 0x00000008. The calculation becomes 0x10 - 0x08, which is 0x08. Again, this is still positive, so the resulting return value will still be zero. Now for the boundary condition. Lets say the timer has incremented to 0x10. 0x10 - 0x10 is 0x00, which is still positive, so the return value will be zero. But the next count, 0x11 produces a difference. 0x10 - 0x11 is negative, so the return value will be -ETIME. It appears that the original code here is doing the right thing: it's returning -ETIME if the event which has been programmed has already passed. > > That to me sounds 100% correct. Your replacement code of always returning > > zero looks wrong. > > The code change is fixing a problem seen with WAIT mode support, without > introducing any regression from what I've seen. > > Enabling WAIT mode support with the original code, we will easily run > into an infinite loop in tick_handle_oneshot_broadcast() - "goto again;" > This happens because when global event did not expire any CPU local > events, the broadcast device will be rearmed to a CPU local next_event, > which could be far away from now and result in a max_delta_tick > programming in set_next_event(). Ah, so it's not a wrap of the counter caused by too small an increment, but a wrap of the timeout value causing the timeout value to potentially appear wrapped. That's a slightly more complex case to deal with; how sa11x0 and PXA deals with this by restricting the max_delta_tick to avoid the problem: ckevt_sa1100_osmr0.max_delta_ns = clockevent_delta2ns(0x7fffffff, &ckevt_sa1100_osmr0); ckevt_pxa_osmr0.max_delta_ns = clockevent_delta2ns(0x7fffffff, &ckevt_pxa_osmr0); Another solution which avoids restricting max_delta_tick would be to detect the large increment and avoid the check in that case, so: return evt < ((u32)~0) / 2 && (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? -ETIME : 0; > One thing I do not understand is why tick_broadcast_set_event() is being > called with parameter "force" being 0 in tick_handle_oneshot_broadcast(). Presumably because the design of the code is to detect whenever the requested event has passed, and recalculate the expiry time, rather than just trying to set a timeout that will occur immediately.
On Tue, Dec 04, 2012 at 03:44:59PM +0000, Russell King - ARM Linux wrote: > On Tue, Dec 04, 2012 at 10:55:12PM +0800, Shawn Guo wrote: > > The timer is configured in free-run mode. The counter should be > > allowed to roll over to 0 when reaching 0xffffffff. Let's do that > > by always returning 0 in set_next_event. > > Are you sure this is correct? It looks wrong to me. > > If the time set by the next event has passed, you must return -ETIME > from set_next_event() so that the generic timer code can compensate > for the missed event and recalculate it, otherwise you will end up > having to wait a full count cycle before receiving the next event. > > If you find that you're being passed such a small increment that you're > constantly hitting that condition, you need to increase your minimum > waited interval. I think we already have a proper min_delta_tick setting to ensure we will not run into the situation that the event will get expired in v2_set_next_event(). I guess that's the reason why most of the set_next_event() implementations under arch/arm can always return 0? > > diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c > > index f017302..858098c 100644 > > --- a/arch/arm/mach-imx/time.c > > +++ b/arch/arm/mach-imx/time.c > > @@ -139,8 +139,7 @@ static int mx1_2_set_next_event(unsigned long evt, > > > > __raw_writel(tcmp, timer_base + MX1_2_TCMP); > > > > - return (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? > > - -ETIME : 0; > > + return 0; > > So what this code was doing is: tcmp is the counter value we expect with > the expected event time added. This may wrap 32-bits. We subtract the > current timer value after we've set the compare register. If the current > timer value was larger than the expected event time, we return -ETIME. >From what I have seen, it never happens in event expiring case but always in counter wrapping. Since the timer is configured in free-run mode and can keep running after wrapping, it should be safe to always return zero. > That to me sounds 100% correct. Your replacement code of always returning > zero looks wrong. The code change is fixing a problem seen with WAIT mode support, without introducing any regression from what I've seen. Enabling WAIT mode support with the original code, we will easily run into an infinite loop in tick_handle_oneshot_broadcast() - "goto again;" This happens because when global event did not expire any CPU local events, the broadcast device will be rearmed to a CPU local next_event, which could be far away from now and result in a max_delta_tick programming in set_next_event(). This will certainly cause a counter wrapping and then a -ETIME return, which in turn makes the execution fall into the "goto again" loop. One thing I do not understand is why tick_broadcast_set_event() is being called with parameter "force" being 0 in tick_handle_oneshot_broadcast(). This will make clockevents_program_event() skip clockevents_program_min_delta() call in that case. Changing "force" to 1 also fixes my problem, but I wouldn't be so aggressive to change the clockevents core code when there is a fix that could be applied at platform level. Shawn
On Wed, Dec 05, 2012 at 12:09:00PM +0000, Russell King - ARM Linux wrote: > Well, let's look at the maths here. > > Let's say that the counter value is at 0xfffffff0, and the desired timeout > is 32. This makes tcmp 0x00000010. Let's say that the counter has only > incremented a small amount to 0xfffffff8: > > (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN) > > So this becomes 0x10 - 0xfffffff8, which will be 0x18. So the return value > in this case will be zero. > > Now lets consider what happens if the timer has wrapped to 0x00000008. The > calculation becomes 0x10 - 0x08, which is 0x08. Again, this is still > positive, so the resulting return value will still be zero. > > Now for the boundary condition. Lets say the timer has incremented to 0x10. > 0x10 - 0x10 is 0x00, which is still positive, so the return value will be > zero. But the next count, 0x11 produces a difference. 0x10 - 0x11 is > negative, so the return value will be -ETIME. > > It appears that the original code here is doing the right thing: it's > returning -ETIME if the event which has been programmed has already passed. This is a helpful explanation. I was indeed fix my problem in a wrong way. > > > That to me sounds 100% correct. Your replacement code of always returning > > > zero looks wrong. > > > > The code change is fixing a problem seen with WAIT mode support, without > > introducing any regression from what I've seen. > > > > Enabling WAIT mode support with the original code, we will easily run > > into an infinite loop in tick_handle_oneshot_broadcast() - "goto again;" > > This happens because when global event did not expire any CPU local > > events, the broadcast device will be rearmed to a CPU local next_event, > > which could be far away from now and result in a max_delta_tick > > programming in set_next_event(). > > Ah, so it's not a wrap of the counter caused by too small an increment, > but a wrap of the timeout value causing the timeout value to potentially > appear wrapped. That's a slightly more complex case to deal with; how > sa11x0 and PXA deals with this by restricting the max_delta_tick to avoid > the problem: > > ckevt_sa1100_osmr0.max_delta_ns = > clockevent_delta2ns(0x7fffffff, &ckevt_sa1100_osmr0); > ckevt_pxa_osmr0.max_delta_ns = > clockevent_delta2ns(0x7fffffff, &ckevt_pxa_osmr0); > > Another solution which avoids restricting max_delta_tick would be to > detect the large increment and avoid the check in that case, so: > > return evt < ((u32)~0) / 2 && > (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? > -ETIME : 0; > I will go for this one, since it keeps max_delta_tick stay as big as possible. > > One thing I do not understand is why tick_broadcast_set_event() is being > > called with parameter "force" being 0 in tick_handle_oneshot_broadcast(). > > Presumably because the design of the code is to detect whenever the > requested event has passed, and recalculate the expiry time, rather > than just trying to set a timeout that will occur immediately. Thanks for help understand all these, Russell. Shawn
diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c index f017302..858098c 100644 --- a/arch/arm/mach-imx/time.c +++ b/arch/arm/mach-imx/time.c @@ -139,8 +139,7 @@ static int mx1_2_set_next_event(unsigned long evt, __raw_writel(tcmp, timer_base + MX1_2_TCMP); - return (int)(tcmp - __raw_readl(timer_base + MX1_2_TCN)) < 0 ? - -ETIME : 0; + return 0; } static int v2_set_next_event(unsigned long evt, @@ -152,8 +151,7 @@ static int v2_set_next_event(unsigned long evt, __raw_writel(tcmp, timer_base + V2_TCMP); - return (int)(tcmp - __raw_readl(timer_base + V2_TCN)) < 0 ? - -ETIME : 0; + return 0; } #ifdef DEBUG
The timer is configured in free-run mode. The counter should be allowed to roll over to 0 when reaching 0xffffffff. Let's do that by always returning 0 in set_next_event. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- arch/arm/mach-imx/time.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)