On 11 June 2014 04:47, Kevin Hilman khilman@linaro.org wrote:
Viresh Kumar viresh.kumar@linaro.org writes:
A clockevent device is used to service timers/hrtimers requests and the next event (when it should fire) is decided by the timer/hrtimer expiring next.
Just drop this intro sentence. It adds more confusion than value, IMO.
When no timers/hrtimers are pending to be serviced, the expiry time is set to a
drop "to be serviced"
special value: KTIME_MAX. This means that no events are required for indefinite amount of time.
s/required for indefinite amount of time/pending/
Thanks for all above suggestions, will fix those.
Ideally, as the clock event device is programmed in ONESHOT mode it should just fire one more time and that's it.
Is this really the "ideal" case?
I thought so as core has asked driver to program device in ONESHOT mode and it should stop by itself after firing once. Many drivers have taken care of this even when they have emulated ONESHOT over PERIODIC mode by disabling clockevent device from irq handler.
Even if it fires one more time, it's still a spurious interrupt that causes an unncessary wakeup, right?
Yes, still a problem but not as worst as getting these spurious interrupts at tick-rate. And so thought maybe we should mention this.
But many implementations (like arm_arch_timer, etc) only have PERIODIC mode available and their drivers emulate ONESHOT over that. Which means that on these platforms we will get spurious interrupts at tick rate and that will hurt our tickless-ness badly.
At this time the clockevent device should be stopped, or its interrupts may be
At which time? The copy/paste from the cover letter here adds confusion.
Hmm..
masked in order to get these issues fixed.
See comment in cover letter.
Ok.
Two new APIs tick_stop_event() and tick_restart_event() are also implemented.
Uh, Why?
They are introduced here without explanation and without any users. This part should be a separate patch, introduced along with their users.
Correct.
IMO, you should be following the sequence as discussed earlier
- introduce new ->set_dev_mode()
- convert some clockevent devices to use it
- introduce ONESHOT_STOPPED (and any dependencies)
- convert a platform to use it.
The ordering of this is all mixed up in this series and rather confusing to review.
Okay. Will combine both patchsets for next version and make it as light-weight as possible ..
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index eaf5b0d..9348da1 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -107,8 +107,18 @@ void clockevents_set_mode(struct clock_event_device *dev, if (dev->mode != mode) { int ret = dev->set_dev_mode(mode, dev);
/* Currently available modes shouldn't fail */WARN_ONCE(ret, "Requested mode: %d, error: %d\n", mode, ret);this code is not in mainline (or -next), so I'm not sure what this series applies to. Normally, it's good practice to state in the cover letter what a series applies to.
I mentioned this in cover-letter: "This is the next planned step of "add ->set_dev_mode" patchset.."
and didn't mention explicitly that it is based of "set_dev_mode" patchset. Should have mentioned that clearly ..
However, the above removed code has never been upstream AFAICT, so it looks to me like this series applies on top of some earlier version of your patchset, which means this series has never actually been applied to mainline and compile tested. Ugh.
As this was based over set_dev_mode patchset, these problems weren't around. It was actually tested on my exynos-dual A15 board and we could get rid of these spurious interrupts.
Combined with the fact that I don't think you followed my previous advice about breaking up the series into the right sequence, I'm quickly arriving at burnout on this series.
Did I get the sequence wrong?
Yes, i haven't sent a light-weight (i.e. modifying few drivers only) V6 of "set_dev_mode" patchset as I was worried that people may want to review V5 (though I should have sent sent a light-weight version during V5 only).
Also I did made this patchset light-weight by just updating "arm_arch_timer" driver and not touching anything else.
Sorry for the confusion.