Hi,
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
---------
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at-least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patchset first adds another callback with return capability, set_dev_mode(), then it migrates all drivers one by one and finally removes earlier callback set_mode() when nobody is using it.
NOTE: Following issue is still unresolved as this patchset isn't making it worse. But it should be fixed before addition of ONESHOT_STOPPED mode.
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
V2->V3: - s/WARN_ON/WARN_ON_ONCE - improved logs - V2 had code belonging to problem mentioned in "NOTE" above, its dropped now and would be addressed separately.
Viresh Kumar (8): clockevents: add ->set_dev_mode() to struct clock_event_device clockevents: arm: migrate to ->set_dev_mode() clockevents: mips: migrate to ->set_dev_mode() clockevents: sparc: migrate to ->set_dev_mode() clockevents: x86: migrate to ->set_dev_mode() clockevents: drivers/: migrate to ->set_dev_mode() clockevents: misc: migrate to ->set_dev_mode() clockevents: remove ->set_mode() from struct clock_event_device
arch/alpha/kernel/time.c | 32 ++++++++++++++++++---- arch/arc/kernel/time.c | 11 +++++--- arch/arm/common/timer-sp.c | 9 ++++--- arch/arm/kernel/smp_twd.c | 10 ++++--- arch/arm/mach-at91/at91rm9200_time.c | 7 +++-- arch/arm/mach-at91/at91sam926x_time.c | 11 ++++---- arch/arm/mach-clps711x/common.c | 9 ++++--- arch/arm/mach-cns3xxx/core.c | 10 ++++--- arch/arm/mach-davinci/time.c | 7 +++-- arch/arm/mach-footbridge/dc21285-timer.c | 7 +++-- arch/arm/mach-gemini/time.c | 7 ++--- arch/arm/mach-imx/epit.c | 11 ++++---- arch/arm/mach-imx/time.c | 11 ++++---- arch/arm/mach-integrator/integrator_ap.c | 8 +++--- arch/arm/mach-ixp4xx/common.c | 8 +++--- arch/arm/mach-ks8695/time.c | 22 ++++++++++----- arch/arm/mach-lpc32xx/timer.c | 11 ++++---- arch/arm/mach-mmp/time.c | 12 +++++---- arch/arm/mach-netx/time.c | 11 ++++---- arch/arm/mach-omap1/time.c | 7 +++-- arch/arm/mach-omap1/timer32k.c | 7 +++-- arch/arm/mach-omap2/timer.c | 7 +++-- arch/arm/mach-pxa/time.c | 8 +++--- arch/arm/mach-sa1100/time.c | 8 +++--- arch/arm/mach-spear/time.c | 10 +++---- arch/arm/mach-w90x900/time.c | 7 +++-- arch/arm/plat-iop/time.c | 8 +++--- arch/arm/plat-orion/time.c | 20 ++++++++++---- arch/avr32/kernel/time.c | 7 ++--- arch/blackfin/kernel/time-ts.c | 14 +++++++--- arch/c6x/platforms/timer64.c | 7 +++-- arch/hexagon/kernel/time.c | 11 +++++--- arch/m68k/platform/coldfire/pit.c | 7 +++-- arch/microblaze/kernel/timer.c | 7 +++-- arch/mips/alchemy/common/time.c | 14 ++++++++-- arch/mips/include/asm/cevt-r4k.h | 2 +- arch/mips/jazz/irq.c | 14 ++++++++-- arch/mips/jz4740/time.c | 9 ++++--- arch/mips/kernel/cevt-bcm1480.c | 11 +++++--- arch/mips/kernel/cevt-ds1287.c | 12 ++++++--- arch/mips/kernel/cevt-gic.c | 14 ++++++++-- arch/mips/kernel/cevt-gt641xx.c | 12 ++++++--- arch/mips/kernel/cevt-r4k.c | 14 ++++++++-- arch/mips/kernel/cevt-sb1250.c | 11 +++++--- arch/mips/kernel/cevt-smtc.c | 2 +- arch/mips/kernel/cevt-txx9.c | 7 +++-- arch/mips/loongson/common/cs5536/cs5536_mfgpt.c | 12 ++++----- arch/mips/ralink/cevt-rt3352.c | 13 +++++---- arch/mips/sgi-ip27/ip27-timer.c | 14 ++++++++-- arch/mips/sni/time.c | 9 ++++--- arch/mn10300/kernel/cevt-mn10300.c | 14 ++++++++-- arch/openrisc/kernel/time.c | 11 ++++---- arch/powerpc/kernel/time.c | 16 ++++++++--- arch/s390/kernel/time.c | 14 ++++++++-- arch/score/kernel/time.c | 8 +++--- arch/sh/kernel/localtimer.c | 15 +++++++++-- arch/sparc/kernel/time_32.c | 18 ++++++++----- arch/sparc/kernel/time_64.c | 11 ++++---- arch/tile/kernel/time.c | 16 ++++++++--- arch/um/kernel/time.c | 7 +++-- arch/unicore32/kernel/time.c | 8 +++--- arch/x86/kernel/apic/apic.c | 10 ++++--- arch/x86/kernel/hpet.c | 19 +++++++------ arch/x86/lguest/boot.c | 9 ++++--- arch/x86/platform/uv/uv_time.c | 10 ++++--- arch/x86/xen/time.c | 23 +++++++--------- arch/xtensa/kernel/time.c | 9 ++++--- drivers/clocksource/arm_arch_timer.c | 36 ++++++++++++++----------- drivers/clocksource/arm_global_timer.c | 9 ++++--- drivers/clocksource/bcm2835_timer.c | 8 +++--- drivers/clocksource/bcm_kona_timer.c | 10 ++++--- drivers/clocksource/cadence_ttc_timer.c | 7 +++-- drivers/clocksource/cs5535-clockevt.c | 17 +++++++++--- drivers/clocksource/dummy_timer.c | 15 +++++++++-- drivers/clocksource/dw_apb_timer.c | 7 +++-- drivers/clocksource/em_sti.c | 11 +++++--- drivers/clocksource/exynos_mct.c | 16 +++++++---- drivers/clocksource/i8253.c | 8 ++++-- drivers/clocksource/metag_generic.c | 11 ++++---- drivers/clocksource/moxart_timer.c | 8 +++--- drivers/clocksource/mxs_timer.c | 10 +++---- drivers/clocksource/nomadik-mtu.c | 7 +++-- drivers/clocksource/qcom-timer.c | 11 ++++---- drivers/clocksource/samsung_pwm_timer.c | 7 +++-- drivers/clocksource/sh_cmt.c | 9 ++++--- drivers/clocksource/sh_mtu2.c | 9 ++++--- drivers/clocksource/sh_tmu.c | 9 ++++--- drivers/clocksource/sun4i_timer.c | 9 ++++--- drivers/clocksource/tcb_clksrc.c | 11 +++++--- drivers/clocksource/tegra20_timer.c | 7 +++-- drivers/clocksource/time-armada-370-xp.c | 20 +++++++++----- drivers/clocksource/time-efm32.c | 7 +++-- drivers/clocksource/time-orion.c | 17 +++++++++--- drivers/clocksource/timer-keystone.c | 9 ++++--- drivers/clocksource/timer-marco.c | 10 ++++--- drivers/clocksource/timer-prima2.c | 10 +++---- drivers/clocksource/timer-sun5i.c | 10 ++++--- drivers/clocksource/timer-u300.c | 7 +++-- drivers/clocksource/vf_pit_timer.c | 12 ++++++--- drivers/clocksource/vt8500_timer.c | 8 +++--- drivers/clocksource/zevio-timer.c | 9 +++---- include/linux/clockchips.h | 4 +-- kernel/time/clockevents.c | 7 +++-- kernel/time/tick-broadcast-hrtimer.c | 11 +++++--- kernel/time/timer_list.c | 2 +- 105 files changed, 756 insertions(+), 376 deletions(-)
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patch adds support for ->set_dev_mode() in clockevents core.
For now, just add a WARN_ON_ONCE() to callers of ->set_dev_mode() as none of currently implemented modes are expected/allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/clockchips.h | 5 ++++- kernel/time/clockevents.c | 18 +++++++++++++++--- kernel/time/timer_list.c | 5 ++++- 3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..8ab1a86 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -81,7 +81,8 @@ enum clock_event_mode { * @mode: operating mode assigned by the management code * @features: features * @retries: number of forced programming retries - * @set_mode: set mode function + * @set_dev_mode: set dev mode function + * @set_mode: set mode function (deprecated, use set_dev_mode instead) * @broadcast: function to broadcast events * @min_delta_ticks: minimum delta value in ticks stored for reconfiguration * @max_delta_ticks: maximum delta value in ticks stored for reconfiguration @@ -109,6 +110,8 @@ struct clock_event_device { unsigned long retries;
void (*broadcast)(const struct cpumask *mask); + int (*set_dev_mode)(enum clock_event_mode mode, + struct clock_event_device *); void (*set_mode)(enum clock_event_mode mode, struct clock_event_device *); void (*suspend)(struct clock_event_device *); diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..f9bed16 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,14 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) { - dev->set_mode(mode, dev); + if (dev->set_dev_mode) { + /* WARN: Currently available modes shouldn't fail */ + if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev))) + return; + } else { + dev->set_mode(mode, dev); + } + dev->mode = mode;
/* @@ -446,8 +453,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); + if (dev->mode == CLOCK_EVT_MODE_PERIODIC) { + if (dev->set_dev_mode) + WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, + dev)); + else + dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); + }
return 0; } diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c index 61ed862..957a049 100644 --- a/kernel/time/timer_list.c +++ b/kernel/time/timer_list.c @@ -229,7 +229,10 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu) SEQ_printf(m, "\n");
SEQ_printf(m, " set_mode: "); - print_name_offset(m, dev->set_mode); + if (dev->set_dev_mode) + print_name_offset(m, dev->set_dev_mode); + else + print_name_offset(m, dev->set_mode); SEQ_printf(m, "\n");
SEQ_printf(m, " event_handler: ");
Hi Viresh,
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patch adds support for ->set_dev_mode() in clockevents core.
For now, just add a WARN_ON_ONCE() to callers of ->set_dev_mode() as none of currently implemented modes are expected/allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
include/linux/clockchips.h | 5 ++++- kernel/time/clockevents.c | 18 +++++++++++++++--- kernel/time/timer_list.c | 5 ++++- 3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..8ab1a86 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -81,7 +81,8 @@ enum clock_event_mode {
- @mode: operating mode assigned by the management code
- @features: features
- @retries: number of forced programming retries
- @set_mode: set mode function
- @set_dev_mode: set dev mode function
- @set_mode: set mode function (deprecated, use set_dev_mode instead)
- @broadcast: function to broadcast events
- @min_delta_ticks: minimum delta value in ticks stored for reconfiguration
- @max_delta_ticks: maximum delta value in ticks stored for reconfiguration
@@ -109,6 +110,8 @@ struct clock_event_device { unsigned long retries;
void (*broadcast)(const struct cpumask *mask);
- int (*set_dev_mode)(enum clock_event_mode mode,
struct clock_event_device *);diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..f9bed16 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,14 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) {
dev->set_mode(mode, dev);
if (dev->set_dev_mode) {/* WARN: Currently available modes shouldn't fail */if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev)))return;} else {dev->set_mode(mode, dev);}dev->mode = mode;
/*
@@ -446,8 +453,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
if (dev->set_dev_mode)WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC,dev));elsedev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);}
return 0;
}
When dev->set_dev_mode fails in the above calls, we must add error handling for each of the modes. A mere 'WARN_ON()' will not do,
I understand that the reason you have not implemented error handling for each mode is because these calls will not fail today. But without error handling, the patchset appears incomplete to me. IMO, we need to give a thought to what should be done if any of the modes fail, else it will remain a half baked idea.
Regards Preeti U Murthy
On 25 May 2014 15:16, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
When dev->set_dev_mode fails in the above calls, we must add error handling for each of the modes. A mere 'WARN_ON()' will not do,
I understand that the reason you have not implemented error handling for each mode is because these calls will not fail today. But without error handling, the patchset appears incomplete to me. IMO, we need to give a thought to what should be done if any of the modes fail, else it will remain a half baked idea.
This is what tglx wrote about this:
On 12 May 2014 15:49, Thomas Gleixner tglx@linutronix.de wrote:
if (dev->set_dev_mode) { ret = dev->set_dev_mode(dev, mode); handle_return_value(ret);
Does that handle_return_value() mean that you sprinkle WARN_ONs all over the place? Does it mean, that you change the return value semantics of functions which happen to call clock_events_set_mode() just because it now has a return value?
Hell no.
handle_return_value() means handle the return value right there. We know what we want to set and we know what may fail and what not, so we can explicitely WARN right there. And for those things which might fail, we return the failure and handle it at the call site.
I believe I have done exactly what he had asked for. And I dare not play more with him anymore, its like entering Lion's Den :)
On 05/26/2014 10:55 AM, Viresh Kumar wrote:
On 25 May 2014 15:16, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
When dev->set_dev_mode fails in the above calls, we must add error handling for each of the modes. A mere 'WARN_ON()' will not do,
I understand that the reason you have not implemented error handling for each mode is because these calls will not fail today. But without error handling, the patchset appears incomplete to me. IMO, we need to give a thought to what should be done if any of the modes fail, else it will remain a half baked idea.
This is what tglx wrote about this:
On 12 May 2014 15:49, Thomas Gleixner tglx@linutronix.de wrote:
if (dev->set_dev_mode) { ret = dev->set_dev_mode(dev, mode); handle_return_value(ret);
Does that handle_return_value() mean that you sprinkle WARN_ONs all over the place? Does it mean, that you change the return value semantics of functions which happen to call clock_events_set_mode() just because it now has a return value?
Hell no.
handle_return_value() means handle the return value right there. We know what we want to set and we know what may fail and what not, so we can explicitely WARN right there. And for those things which might fail, we return the failure and handle it at the call site.
I believe I have done exactly what he had asked for. And I dare not play more with him anymore, its like entering Lion's Den :)
Ok in that case you must be returning an error in clockevent_set_mode() right? You currently just return. clockevent_set_mode() should be made to return an integer type to signify that setting clock modes will fail. Handling of these errors at the call site can be done in the next patchset when ONESHOT_STOPPED mode is introduced.
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
Regards Preeti U Murthy
On 26 May 2014 11:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ok in that case you must be returning an error in clockevent_set_mode() right? You currently just return. clockevent_set_mode() should be made to return an integer type to signify that setting clock modes will fail. Handling of these errors at the call site can be done in the next patchset when ONESHOT_STOPPED mode is introduced.
I had that earlier in my patches earlier and it looked irrelevant there and I decided to move that to the ONESHOT_STOPPED patchset. As returning an error wouldn't be useful with this patchset alone (Callers aren't handling it yet)...
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
In update_freq() we must never fail as all we are doing is setting a valid mode. So, we don't need to return an error even (to make it complex for call sites..)
I wasn't sure if WARN_ON() is the right thing to do here or we should rather have a BUG_ON() :) ..
On 05/26/2014 12:08 PM, Viresh Kumar wrote:
On 26 May 2014 11:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ok in that case you must be returning an error in clockevent_set_mode() right? You currently just return. clockevent_set_mode() should be made to return an integer type to signify that setting clock modes will fail. Handling of these errors at the call site can be done in the next patchset when ONESHOT_STOPPED mode is introduced.
I had that earlier in my patches earlier and it looked irrelevant there and I decided to move that to the ONESHOT_STOPPED patchset. As returning an error wouldn't be useful with this patchset alone (Callers aren't handling it yet)...
In this case, just have a WARN_ON() and let the code fall through for now. The reason I am pushing on this front is when we look at the code, it should make sense one way or the other.
Either return the error code signifying that the call sites have to take care of this or don't return at this point. The early 'return' there is not of any significance. You may say that you will eventually replace that 'return' with 'return error'. But the code is not conveying this.
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
In update_freq() we must never fail as all we are doing is setting a valid mode. So, we don't need to return an error even (to make it complex for
Then why have a WARN_ON() here at all?
Regards Preeti U Murthy
call sites..)
I wasn't sure if WARN_ON() is the right thing to do here or we should rather have a BUG_ON() :) ..
On 26 May 2014 12:21, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
In this case, just have a WARN_ON() and let the code fall through for now. The reason I am pushing on this front is when we look at the code, it should make sense one way or the other.
Either return the error code signifying that the call sites have to take care of this or don't return at this point. The early 'return' there is not of any significance. You may say that you will eventually replace that 'return' with 'return error'. But the code is not conveying this.
Hmmm... Continuing here doesn't make any sense, when we know the mode hasn't changed at all.
Maybe we can leave it as is and see what tglx thinks ?
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
In update_freq() we must never fail as all we are doing is setting a valid mode. So, we don't need to return an error even (to make it complex for
Then why have a WARN_ON() here at all?
Actually can be removed but is there just for protection that we aren't failing. It is very much similar to the WARN_ON in above routine. If we buy your argument, then we don't even need a WARN() in clocevents_set_mode(), we can just check for failure in some special case and return error. Why a WARN ? And so I added it here as well..
On 05/26/2014 12:34 PM, Viresh Kumar wrote:
On 26 May 2014 12:21, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
In this case, just have a WARN_ON() and let the code fall through for now. The reason I am pushing on this front is when we look at the code, it should make sense one way or the other.
Either return the error code signifying that the call sites have to take care of this or don't return at this point. The early 'return' there is not of any significance. You may say that you will eventually replace that 'return' with 'return error'. But the code is not conveying this.
Hmmm... Continuing here doesn't make any sense, when we know the mode hasn't changed at all.
Maybe we can leave it as is and see what tglx thinks ?
I would suggest add a WARN_ON() on failure and fall through.
The same goes for clockevents_update_freq(). On warning, we need to return an error code. This will ensure that this patchset is complete in itself. As of now since none of the modes are expected to fail, the call sites needn't handle the return value of clockevents_set_mode() and clockevents_update_freq().
In update_freq() we must never fail as all we are doing is setting a valid mode. So, we don't need to return an error even (to make it complex for
Then why have a WARN_ON() here at all?
Actually can be removed but is there just for protection that we aren't failing. It is very much similar to the WARN_ON in above routine. If we buy your argument, then we don't even need a WARN() in clocevents_set_mode(), we can just check for failure in some special case and return error. Why a WARN ? And so I added it here as well..
In the first case of clockevents_set_mode() we are unsure of the mode we are setting. There can be more modes added, going forward and we cannot special case each for a failed return. In the second case of update_freq() you *know* you are setting PERIODIC_MODE which is rest assured will not fail.
Regards Preeti U Murthy
On 26 May 2014 12:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
I would suggest add a WARN_ON() on failure and fall through.
Okay, lets do a voting here :)
@Frederic/Kevin/Daniel: What do you guys think ?
In the first case of clockevents_set_mode() we are unsure of the mode we are setting.
True.
There can be more modes added, going forward and we cannot special case each for a failed return.
True, but we will do WARN_ON() only for modes like PERIODIC which can't fail. Isn't it ?
In the second case of update_freq() you *know* you are setting PERIODIC_MODE which is rest assured will not fail.
Yeah, and WARN_ON() assures us that it doesn't fail. :)
On Mon, May 26, 2014 at 01:16:10PM +0530, Viresh Kumar wrote:
On 26 May 2014 12:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
I would suggest add a WARN_ON() on failure and fall through.
Okay, lets do a voting here :)
@Frederic/Kevin/Daniel: What do you guys think ?
It think as long as it's expected _not_ to fail, it should WARN_ON_ONCE() (really avoid BUG_ON(), it should be used only on stuff like fs where more harm to hardware or data integrity is possible after the error).
Then once you add more modes that might not be supported by hardware, you should add real error handling.
And for now in this patchset, things shouldn't fail, right?
On 26-May-2014, at 10:18 pm, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, May 26, 2014 at 01:16:10PM +0530, Viresh Kumar wrote:
On 26 May 2014 12:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote: I would suggest add a WARN_ON() on failure and fall through.
Okay, lets do a voting here :)
@Frederic/Kevin/Daniel: What do you guys think ?
It think as long as it's expected _not_ to fail, it should WARN_ON_ONCE() (really avoid BUG_ON(), it should be used only on stuff like fs where more harm to hardware or data integrity is possible after the error).
Then once you add more modes that might not be supported by hardware, you should add real error handling.
And for now in this patchset, things shouldn't fail, right?
Right.
On Mon, May 26, 2014 at 10:30:23PM +0530, Viresh Kumar wrote:
On 26-May-2014, at 10:18 pm, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, May 26, 2014 at 01:16:10PM +0530, Viresh Kumar wrote:
On 26 May 2014 12:57, Preeti U Murthy preeti@linux.vnet.ibm.com wrote: I would suggest add a WARN_ON() on failure and fall through.
Okay, lets do a voting here :)
@Frederic/Kevin/Daniel: What do you guys think ?
It think as long as it's expected _not_ to fail, it should WARN_ON_ONCE() (really avoid BUG_ON(), it should be used only on stuff like fs where more harm to hardware or data integrity is possible after the error).
Then once you add more modes that might not be supported by hardware, you should add real error handling.
And for now in this patchset, things shouldn't fail, right?
Right.
I mean, I don't want to shortcut what Preeti said. He's right that we should do error handling. But given what Thomas said and the fact that the patchset only does the conversion for now, the error handling can be done when we add a new mode.
Just make sure you told about it in the changelog somewhere, to reduce the amount of frozen sharks.
On 28 May 2014 19:28, Frederic Weisbecker fweisbec@gmail.com wrote:
I mean, I don't want to shortcut what Preeti said. He's right that we should
s/He/She :)
do error handling. But given what Thomas said and the fact that the patchset only does the conversion for now, the error handling can be done when we add a new mode.
Just make sure you told about it in the changelog somewhere, to reduce the amount of frozen sharks.
Yeah, I have mentioned we are just doing a WARN() for now.
On Wed, May 28, 2014 at 10:01:10PM +0530, Viresh Kumar wrote:
On 28 May 2014 19:28, Frederic Weisbecker fweisbec@gmail.com wrote:
I mean, I don't want to shortcut what Preeti said. He's right that we should
s/He/She :)
Oops, sorry!
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patch adds support for ->set_dev_mode() in clockevents core.
For now, just add a WARN_ON_ONCE() to callers of ->set_dev_mode() as none of currently implemented modes are expected/allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
include/linux/clockchips.h | 5 ++++- kernel/time/clockevents.c | 18 +++++++++++++++--- kernel/time/timer_list.c | 5 ++++- 3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..8ab1a86 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -81,7 +81,8 @@ enum clock_event_mode {
- @mode: operating mode assigned by the management code
- @features: features
- @retries: number of forced programming retries
- @set_mode: set mode function
- @set_dev_mode: set dev mode function
- @set_mode: set mode function (deprecated, use set_dev_mode instead)
- @broadcast: function to broadcast events
- @min_delta_ticks: minimum delta value in ticks stored for reconfiguration
- @max_delta_ticks: maximum delta value in ticks stored for reconfiguration
@@ -109,6 +110,8 @@ struct clock_event_device { unsigned long retries;
void (*broadcast)(const struct cpumask *mask);
- int (*set_dev_mode)(enum clock_event_mode mode,
struct clock_event_device *);diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..f9bed16 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,14 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) {
dev->set_mode(mode, dev);
if (dev->set_dev_mode) {/* WARN: Currently available modes shouldn't fail */if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev)))return;
Would you be doing just a WARN_ON_ONCE() here?
if (dev->set_dev_mode) WARN_ON_ONCE(dev->set_dev_mode()) else dev->set_mode()
?
In that case please add my Reviewed-by to this patch.
Thanks
Regards Preeti U Murthy
On 29 May 2014 14:12, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev)))return;Would you be doing just a WARN_ON_ONCE() here?
if (dev->set_dev_mode) WARN_ON_ONCE(dev->set_dev_mode()) else dev->set_mode()?
In that case please add my Reviewed-by to this patch.
Okay, I will do it :)
Viresh Kumar viresh.kumar@linaro.org writes:
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
This paragraph is a copy/paste from cover letter, so has the same readability problems I mentioned there.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patch adds support for ->set_dev_mode() in clockevents core.
For now, just add a WARN_ON_ONCE() to callers of ->set_dev_mode() as none of currently implemented modes are expected/allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
[...]
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..f9bed16 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,7 +105,14 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) {
dev->set_mode(mode, dev);
if (dev->set_dev_mode) {/* WARN: Currently available modes shouldn't fail */if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev)))return;} else {dev->set_mode(mode, dev);}- dev->mode = mode;
/* @@ -446,8 +453,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
if (dev->set_dev_mode)
As discussed already, PERIODIC should never fail, so maybe a comment here stating that we don't ever expect periodic mode to fail.
Kevin
On 29 May 2014 22:14, Kevin Hilman khilman@linaro.org wrote:
Viresh Kumar viresh.kumar@linaro.org writes:
@@ -446,8 +453,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
if (dev->mode == CLOCK_EVT_MODE_PERIODIC)dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {if (dev->set_dev_mode)As discussed already, PERIODIC should never fail, so maybe a comment here stating that we don't ever expect periodic mode to fail.
Okay.
On 29 May 2014 22:14, Kevin Hilman khilman@linaro.org wrote:
As discussed already, PERIODIC should never fail, so maybe a comment here stating that we don't ever expect periodic mode to fail.
Done.
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all ARM specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/common/timer-sp.c | 9 ++++++--- arch/arm/kernel/smp_twd.c | 10 +++++++--- arch/arm/mach-at91/at91rm9200_time.c | 7 +++++-- arch/arm/mach-at91/at91sam926x_time.c | 11 ++++++----- arch/arm/mach-clps711x/common.c | 9 +++++---- arch/arm/mach-cns3xxx/core.c | 10 +++++++--- arch/arm/mach-davinci/time.c | 7 +++++-- arch/arm/mach-footbridge/dc21285-timer.c | 7 +++++-- arch/arm/mach-gemini/time.c | 7 ++++--- arch/arm/mach-imx/epit.c | 11 +++++------ arch/arm/mach-imx/time.c | 11 +++++------ arch/arm/mach-integrator/integrator_ap.c | 8 ++++---- arch/arm/mach-ixp4xx/common.c | 8 +++++--- arch/arm/mach-ks8695/time.c | 22 ++++++++++++++++------ arch/arm/mach-lpc32xx/timer.c | 11 +++++------ arch/arm/mach-mmp/time.c | 12 +++++++----- arch/arm/mach-netx/time.c | 11 +++++------ arch/arm/mach-omap1/time.c | 7 +++++-- arch/arm/mach-omap1/timer32k.c | 7 +++++-- arch/arm/mach-omap2/timer.c | 7 +++++-- arch/arm/mach-pxa/time.c | 8 +++++--- arch/arm/mach-sa1100/time.c | 8 +++++--- arch/arm/mach-spear/time.c | 10 +++++----- arch/arm/mach-w90x900/time.c | 7 +++++-- arch/arm/plat-iop/time.c | 8 +++++--- arch/arm/plat-orion/time.c | 20 +++++++++++++++----- 26 files changed, 157 insertions(+), 96 deletions(-)
diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..9ed3845 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -127,7 +127,7 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; }
-static void sp804_set_mode(enum clock_event_mode mode, +static int sp804_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE; @@ -147,11 +147,14 @@ static void sp804_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; }
writel(ctrl, clkevt_base + TIMER_CTRL); + return 0; }
static int sp804_set_next_event(unsigned long next, @@ -168,7 +171,7 @@ static int sp804_set_next_event(unsigned long next, static struct clock_event_device sp804_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ, - .set_mode = sp804_set_mode, + .set_dev_mode = sp804_set_mode, .set_next_event = sp804_set_next_event, .rating = 300, }; diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index dfc3213..f45420d 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -36,7 +36,7 @@ static DEFINE_PER_CPU(bool, percpu_setup_called); static struct clock_event_device __percpu *twd_evt; static int twd_ppi;
-static void twd_set_mode(enum clock_event_mode mode, +static int twd_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned long ctrl; @@ -54,11 +54,15 @@ static void twd_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: ctrl = 0; + break; + default: + return -ENOSYS; }
writel_relaxed(ctrl, twd_base + TWD_TIMER_CONTROL); + return 0; }
static int twd_set_next_event(unsigned long evt, @@ -296,7 +300,7 @@ static void twd_timer_setup(void) clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP; clk->rating = 350; - clk->set_mode = twd_set_mode; + clk->set_dev_mode = twd_set_mode; clk->set_next_event = twd_set_next_event; clk->irq = twd_ppi; clk->cpumask = cpumask_of(cpu); diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c index 7fd13ae..aefece8 100644 --- a/arch/arm/mach-at91/at91rm9200_time.c +++ b/arch/arm/mach-at91/at91rm9200_time.c @@ -112,7 +112,7 @@ static struct clocksource clk32k = { .flags = CLOCK_SOURCE_IS_CONTINUOUS, };
-static void +static int clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev) { /* Disable and flush pending timer interrupts */ @@ -138,8 +138,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev) case CLOCK_EVT_MODE_RESUME: irqmask = 0; break; + default: + return -ENOSYS; } at91_st_write(AT91_ST_IER, irqmask); + return 0; }
static int @@ -177,7 +180,7 @@ static struct clock_event_device clkevt = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 150, .set_next_event = clkevt32k_next_event, - .set_mode = clkevt32k_mode, + .set_dev_mode = clkevt32k_mode, };
void __iomem *at91_st_base; diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c index 0a9e2fc..9698eb6 100644 --- a/arch/arm/mach-at91/at91sam926x_time.c +++ b/arch/arm/mach-at91/at91sam926x_time.c @@ -83,7 +83,7 @@ static struct clocksource pit_clk = { /* * Clockevent device: interrupts every 1/HZ (== pit_cycles * MCK/16) */ -static void +static int pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -93,9 +93,6 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN | AT91_PIT_PITIEN); break; - case CLOCK_EVT_MODE_ONESHOT: - BUG(); - /* FALLTHROUGH */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: /* disable irq, leaving the clocksource active */ @@ -103,7 +100,11 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + + return 0; }
static void at91sam926x_pit_suspend(struct clock_event_device *cedev) @@ -135,7 +136,7 @@ static struct clock_event_device pit_clkevt = { .features = CLOCK_EVT_FEAT_PERIODIC, .shift = 32, .rating = 100, - .set_mode = pit_clkevt_mode, + .set_dev_mode = pit_clkevt_mode, .suspend = at91sam926x_pit_suspend, .resume = at91sam926x_pit_resume, }; diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c index aee81fa..69ada1e 100644 --- a/arch/arm/mach-clps711x/common.c +++ b/arch/arm/mach-clps711x/common.c @@ -69,7 +69,7 @@ static u64 notrace clps711x_sched_clock_read(void) return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D); }
-static void clps711x_clockevent_set_mode(enum clock_event_mode mode, +static int clps711x_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { disable_irq(IRQ_TC2OI); @@ -78,21 +78,22 @@ static void clps711x_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: enable_irq(IRQ_TC2OI); break; - case CLOCK_EVT_MODE_ONESHOT: - /* Not supported */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_clps711x = { .name = "clps711x-clockevent", .rating = 300, .features = CLOCK_EVT_FEAT_PERIODIC, - .set_mode = clps711x_clockevent_set_mode, + .set_dev_mode = clps711x_clockevent_set_mode, };
static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c index 2ae28a6..9746501 100644 --- a/arch/arm/mach-cns3xxx/core.c +++ b/arch/arm/mach-cns3xxx/core.c @@ -113,7 +113,7 @@ void cns3xxx_power_off(void) */ static void __iomem *cns3xxx_tmr1;
-static void cns3xxx_timer_set_mode(enum clock_event_mode mode, +static int cns3xxx_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned long ctrl = readl(cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET); @@ -132,11 +132,15 @@ static void cns3xxx_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: ctrl = 0; + break; + default: + return -ENOSYS; }
writel(ctrl, cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET); + return 0; }
static int cns3xxx_timer_set_next_event(unsigned long evt, @@ -153,7 +157,7 @@ static int cns3xxx_timer_set_next_event(unsigned long evt, static struct clock_event_device cns3xxx_tmr1_clockevent = { .name = "cns3xxx timer1", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = cns3xxx_timer_set_mode, + .set_dev_mode = cns3xxx_timer_set_mode, .set_next_event = cns3xxx_timer_set_next_event, .rating = 350, .cpumask = cpu_all_mask, diff --git a/arch/arm/mach-davinci/time.c b/arch/arm/mach-davinci/time.c index 24ad30f..7ab6a6c 100644 --- a/arch/arm/mach-davinci/time.c +++ b/arch/arm/mach-davinci/time.c @@ -303,7 +303,7 @@ static int davinci_set_next_event(unsigned long cycles, return 0; }
-static void davinci_set_mode(enum clock_event_mode mode, +static int davinci_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct timer_s *t = &timers[TID_CLOCKEVENT]; @@ -326,13 +326,16 @@ static void davinci_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_davinci = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = davinci_set_next_event, - .set_mode = davinci_set_mode, + .set_dev_mode = davinci_set_mode, };
diff --git a/arch/arm/mach-footbridge/dc21285-timer.c b/arch/arm/mach-footbridge/dc21285-timer.c index bf7aa7d..c949daf 100644 --- a/arch/arm/mach-footbridge/dc21285-timer.c +++ b/arch/arm/mach-footbridge/dc21285-timer.c @@ -57,7 +57,7 @@ static int ckevt_dc21285_set_next_event(unsigned long delta, return 0; }
-static void ckevt_dc21285_set_mode(enum clock_event_mode mode, +static int ckevt_dc21285_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -74,7 +74,10 @@ static void ckevt_dc21285_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: *CSR_TIMER1_CNTL = 0; break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device ckevt_dc21285 = { @@ -84,7 +87,7 @@ static struct clock_event_device ckevt_dc21285 = { .rating = 200, .irq = IRQ_TIMER1, .set_next_event = ckevt_dc21285_set_next_event, - .set_mode = ckevt_dc21285_set_mode, + .set_dev_mode = ckevt_dc21285_set_mode, };
static irqreturn_t timer1_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-gemini/time.c b/arch/arm/mach-gemini/time.c index 0a63c4d..8fea6f3 100644 --- a/arch/arm/mach-gemini/time.c +++ b/arch/arm/mach-gemini/time.c @@ -59,7 +59,7 @@ static int gemini_timer_set_next_event(unsigned long cycles, return 0; }
-static void gemini_timer_set_mode(enum clock_event_mode mode, +static int gemini_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 period = DIV_ROUND_CLOSEST(tick_rate, HZ); @@ -91,8 +91,9 @@ static void gemini_timer_set_mode(enum clock_event_mode mode, writel(cr, TIMER_CR(IO_ADDRESS(GEMINI_TIMER_BASE))); break; default: - break; + return -ENOSYS; } + return 0; }
/* Use TIMER2 as clock event */ @@ -101,7 +102,7 @@ static struct clock_event_device gemini_clockevent = { .rating = 300, /* Reasonably fast and accurate clock event */ .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = gemini_timer_set_next_event, - .set_mode = gemini_timer_set_mode, + .set_dev_mode = gemini_timer_set_mode, };
/* diff --git a/arch/arm/mach-imx/epit.c b/arch/arm/mach-imx/epit.c index 074b1a8..d621494 100644 --- a/arch/arm/mach-imx/epit.c +++ b/arch/arm/mach-imx/epit.c @@ -106,7 +106,7 @@ static int epit_set_next_event(unsigned long evt, return 0; }
-static void epit_set_mode(enum clock_event_mode mode, +static int epit_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long flags; @@ -132,10 +132,6 @@ static void epit_set_mode(enum clock_event_mode mode, local_irq_restore(flags);
switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - printk(KERN_ERR "epit_set_mode: Periodic mode is not " - "supported for i.MX EPIT\n"); - break; case CLOCK_EVT_MODE_ONESHOT: /* * Do not put overhead of interrupt enable/disable into @@ -152,7 +148,10 @@ static void epit_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + return -ENOSYS; } + return 0; }
/* @@ -178,7 +177,7 @@ static struct irqaction epit_timer_irq = { static struct clock_event_device clockevent_epit = { .name = "epit", .features = CLOCK_EVT_FEAT_ONESHOT, - .set_mode = epit_set_mode, + .set_dev_mode = epit_set_mode, .set_next_event = epit_set_next_event, .rating = 200, }; diff --git a/arch/arm/mach-imx/time.c b/arch/arm/mach-imx/time.c index 65222ea..aef1944 100644 --- a/arch/arm/mach-imx/time.c +++ b/arch/arm/mach-imx/time.c @@ -179,7 +179,7 @@ static const char *clock_event_mode_label[] = { }; #endif /* DEBUG */
-static void mxc_set_mode(enum clock_event_mode mode, +static int mxc_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long flags; @@ -217,10 +217,6 @@ static void mxc_set_mode(enum clock_event_mode mode, local_irq_restore(flags);
switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - printk(KERN_ERR"mxc_set_mode: Periodic mode is not " - "supported for i.MX\n"); - break; case CLOCK_EVT_MODE_ONESHOT: /* * Do not put overhead of interrupt enable/disable into @@ -237,7 +233,10 @@ static void mxc_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + return -ENOSYS; } + return 0; }
/* @@ -269,7 +268,7 @@ static struct irqaction mxc_timer_irq = { static struct clock_event_device clockevent_mxc = { .name = "mxc_timer1", .features = CLOCK_EVT_FEAT_ONESHOT, - .set_mode = mxc_set_mode, + .set_dev_mode = mxc_set_mode, .set_next_event = mx1_2_set_next_event, .rating = 200, }; diff --git a/arch/arm/mach-integrator/integrator_ap.c b/arch/arm/mach-integrator/integrator_ap.c index dd0cc67..4805d60 100644 --- a/arch/arm/mach-integrator/integrator_ap.c +++ b/arch/arm/mach-integrator/integrator_ap.c @@ -307,7 +307,7 @@ static irqreturn_t integrator_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; }
-static void clkevt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) +static int clkevt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl = readl(clkevt_base + TIMER_CTRL) & ~TIMER_CTRL_ENABLE;
@@ -331,9 +331,9 @@ static void clkevt_set_mode(enum clock_event_mode mode, struct clock_event_devic case CLOCK_EVT_MODE_RESUME: default: /* Just leave in disabled state */ - break; + return -ENOSYS; } - + return 0; }
static int clkevt_set_next_event(unsigned long next, struct clock_event_device *evt) @@ -350,7 +350,7 @@ static int clkevt_set_next_event(unsigned long next, struct clock_event_device * static struct clock_event_device integrator_clockevent = { .name = "timer1", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = clkevt_set_mode, + .set_dev_mode = clkevt_set_mode, .set_next_event = clkevt_set_next_event, .rating = 300, }; diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c index fc4b7b2..05c050a 100644 --- a/arch/arm/mach-ixp4xx/common.c +++ b/arch/arm/mach-ixp4xx/common.c @@ -521,7 +521,7 @@ static int ixp4xx_set_next_event(unsigned long evt, return 0; }
-static void ixp4xx_set_mode(enum clock_event_mode mode, +static int ixp4xx_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long opts = *IXP4XX_OSRT1 & IXP4XX_OST_RELOAD_MASK; @@ -544,19 +544,21 @@ static void ixp4xx_set_mode(enum clock_event_mode mode, opts |= IXP4XX_OST_ENABLE; break; case CLOCK_EVT_MODE_UNUSED: - default: osrt = opts = 0; break; + default: + return -ENOSYS; }
*IXP4XX_OSRT1 = osrt | opts; + return 0; }
static struct clock_event_device clockevent_ixp4xx = { .name = "ixp4xx timer1", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 200, - .set_mode = ixp4xx_set_mode, + .set_dev_mode = ixp4xx_set_mode, .set_next_event = ixp4xx_set_next_event, };
diff --git a/arch/arm/mach-ks8695/time.c b/arch/arm/mach-ks8695/time.c index a197874..8d2149c 100644 --- a/arch/arm/mach-ks8695/time.c +++ b/arch/arm/mach-ks8695/time.c @@ -54,14 +54,15 @@ /* Timer0 Timeout Counter Register */ #define T0TC_WATCHDOG (0xff) /* Enable watchdog mode */
-static void ks8695_set_mode(enum clock_event_mode mode, +static int ks8695_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { - u32 tmcon; + u32 tmcon, rate, half;
- if (mode == CLOCK_EVT_FEAT_PERIODIC) { - u32 rate = DIV_ROUND_CLOSEST(KS8695_CLOCK_RATE, HZ); - u32 half = DIV_ROUND_CLOSEST(rate, 2); + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + rate = DIV_ROUND_CLOSEST(KS8695_CLOCK_RATE, HZ); + half = DIV_ROUND_CLOSEST(rate, 2);
/* Disable timer 1 */ tmcon = readl_relaxed(KS8695_TMR_VA + KS8695_TMCON); @@ -75,7 +76,16 @@ static void ks8695_set_mode(enum clock_event_mode mode, /* Re-enable timer1 */ tmcon |= TMCON_T1EN; writel_relaxed(tmcon, KS8695_TMR_VA + KS8695_TMCON); + break; + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + case CLOCK_EVT_MODE_UNUSED: + break; + default: + return -ENOSYS; } + return 0; }
static int ks8695_set_next_event(unsigned long cycles, @@ -106,7 +116,7 @@ static struct clock_event_device clockevent_ks8695 = { .rating = 300, /* Reasonably fast and accurate clock event */ .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC, .set_next_event = ks8695_set_next_event, - .set_mode = ks8695_set_mode, + .set_dev_mode = ks8695_set_mode, };
/* diff --git a/arch/arm/mach-lpc32xx/timer.c b/arch/arm/mach-lpc32xx/timer.c index 4e58372..d57a84d 100644 --- a/arch/arm/mach-lpc32xx/timer.c +++ b/arch/arm/mach-lpc32xx/timer.c @@ -43,14 +43,10 @@ static int lpc32xx_clkevt_next_event(unsigned long delta, return 0; }
-static void lpc32xx_clkevt_mode(enum clock_event_mode mode, +static int lpc32xx_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - WARN_ON(1); - break; - case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_SHUTDOWN: /* @@ -64,7 +60,10 @@ static void lpc32xx_clkevt_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device lpc32xx_clkevt = { @@ -72,7 +71,7 @@ static struct clock_event_device lpc32xx_clkevt = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 300, .set_next_event = lpc32xx_clkevt_next_event, - .set_mode = lpc32xx_clkevt_mode, + .set_dev_mode = lpc32xx_clkevt_mode, };
static irqreturn_t lpc32xx_timer_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index 2756351..8491da9 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -124,24 +124,26 @@ static int timer_set_next_event(unsigned long delta, return 0; }
-static void timer_set_mode(enum clock_event_mode mode, +static int timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags;
- local_irq_save(flags); switch (mode) { case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: /* disable the matching interrupt */ + local_irq_save(flags); __raw_writel(0x00, mmp_timer_base + TMR_IER(0)); + local_irq_restore(flags); break; case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: break; + default: + return -ENOSYS; } - local_irq_restore(flags); + return 0; }
static struct clock_event_device ckevt = { @@ -149,7 +151,7 @@ static struct clock_event_device ckevt = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = timer_set_next_event, - .set_mode = timer_set_mode, + .set_dev_mode = timer_set_mode, };
static cycle_t clksrc_read(struct clocksource *cs) diff --git a/arch/arm/mach-netx/time.c b/arch/arm/mach-netx/time.c index 5fb2a59..4f0f9db 100644 --- a/arch/arm/mach-netx/time.c +++ b/arch/arm/mach-netx/time.c @@ -34,7 +34,7 @@ #define TIMER_CLOCKEVENT 0 #define TIMER_CLOCKSOURCE 1
-static void netx_set_mode(enum clock_event_mode mode, +static int netx_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { u32 tmode; @@ -56,18 +56,17 @@ static void netx_set_mode(enum clock_event_mode mode, NETX_GPIO_COUNTER_CTRL_RUN; break;
- default: - WARN(1, "%s: unhandled mode %d\n", __func__, mode); - /* fall through */ - case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: tmode = 0; break; + default: + return -ENOSYS; }
writel(tmode, NETX_GPIO_COUNTER_CTRL(TIMER_CLOCKEVENT)); + return 0; }
static int netx_set_next_event(unsigned long evt, @@ -81,7 +80,7 @@ static struct clock_event_device netx_clockevent = { .name = "netx-timer" __stringify(TIMER_CLOCKEVENT), .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = netx_set_next_event, - .set_mode = netx_set_mode, + .set_dev_mode = netx_set_mode, };
/* diff --git a/arch/arm/mach-omap1/time.c b/arch/arm/mach-omap1/time.c index a7588cf..b2bcd25 100644 --- a/arch/arm/mach-omap1/time.c +++ b/arch/arm/mach-omap1/time.c @@ -124,7 +124,7 @@ static int omap_mpu_set_next_event(unsigned long cycles, return 0; }
-static void omap_mpu_set_mode(enum clock_event_mode mode, +static int omap_mpu_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -139,14 +139,17 @@ static void omap_mpu_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_mpu_timer1 = { .name = "mpu_timer1", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = omap_mpu_set_next_event, - .set_mode = omap_mpu_set_mode, + .set_dev_mode = omap_mpu_set_mode, };
static irqreturn_t omap_mpu_timer1_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-omap1/timer32k.c b/arch/arm/mach-omap1/timer32k.c index 107e7ab..6cf8962 100644 --- a/arch/arm/mach-omap1/timer32k.c +++ b/arch/arm/mach-omap1/timer32k.c @@ -119,7 +119,7 @@ static int omap_32k_timer_set_next_event(unsigned long delta, return 0; }
-static void omap_32k_timer_set_mode(enum clock_event_mode mode, +static int omap_32k_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { omap_32k_timer_stop(); @@ -134,14 +134,17 @@ static void omap_32k_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_32k_timer = { .name = "32k-timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = omap_32k_timer_set_next_event, - .set_mode = omap_32k_timer_set_mode, + .set_dev_mode = omap_32k_timer_set_mode, };
static irqreturn_t omap_32k_timer_interrupt(int irq, void *dev_id) diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index b62de9f..749774f 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -101,7 +101,7 @@ static int omap2_gp_timer_set_next_event(unsigned long cycles, return 0; }
-static void omap2_gp_timer_set_mode(enum clock_event_mode mode, +static int omap2_gp_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 period; @@ -125,14 +125,17 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_gpt = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 300, .set_next_event = omap2_gp_timer_set_next_event, - .set_mode = omap2_gp_timer_set_mode, + .set_dev_mode = omap2_gp_timer_set_mode, };
static struct property device_disabled = { diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c index fca174e..a4a8789 100644 --- a/arch/arm/mach-pxa/time.c +++ b/arch/arm/mach-pxa/time.c @@ -67,7 +67,7 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -84,9 +84,11 @@ pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) break;
case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: break; + default: + return -ENOSYS; } + return 0; }
#ifdef CONFIG_PM @@ -130,7 +132,7 @@ static struct clock_event_device ckevt_pxa_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = pxa_osmr0_set_next_event, - .set_mode = pxa_osmr0_set_mode, + .set_dev_mode = pxa_osmr0_set_mode, .suspend = pxa_timer_suspend, .resume = pxa_timer_resume, }; diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index 1dea6cf..2282858 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -56,7 +56,7 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -68,9 +68,11 @@ sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break;
case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: break; + default: + return -ENOSYS; } + return 0; }
#ifdef CONFIG_PM @@ -109,7 +111,7 @@ static struct clock_event_device ckevt_sa1100_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = sa1100_osmr0_set_next_event, - .set_mode = sa1100_osmr0_set_mode, + .set_dev_mode = sa1100_osmr0_set_mode, .suspend = sa1100_timer_suspend, .resume = sa1100_timer_resume, }; diff --git a/arch/arm/mach-spear/time.c b/arch/arm/mach-spear/time.c index 26fda4e..6bfc536 100644 --- a/arch/arm/mach-spear/time.c +++ b/arch/arm/mach-spear/time.c @@ -66,7 +66,7 @@ static __iomem void *gpt_base; static struct clk *gpt_clk;
-static void clockevent_set_mode(enum clock_event_mode mode, +static int clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *clk_event_dev); static int clockevent_next_event(unsigned long evt, struct clock_event_device *clk_event_dev); @@ -98,12 +98,12 @@ static void __init spear_clocksource_init(void) static struct clock_event_device clkevt = { .name = "tmr0", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = clockevent_set_mode, + .set_dev_mode = clockevent_set_mode, .set_next_event = clockevent_next_event, .shift = 0, /* to be computed */ };
-static void clockevent_set_mode(enum clock_event_mode mode, +static int clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *clk_event_dev) { u32 period; @@ -138,9 +138,9 @@ static void clockevent_set_mode(enum clock_event_mode mode,
break; default: - pr_err("Invalid mode requested\n"); - break; + return -ENOSYS; } + return 0; }
static int clockevent_next_event(unsigned long cycles, diff --git a/arch/arm/mach-w90x900/time.c b/arch/arm/mach-w90x900/time.c index 9230d37..e4a8067 100644 --- a/arch/arm/mach-w90x900/time.c +++ b/arch/arm/mach-w90x900/time.c @@ -48,7 +48,7 @@
static unsigned int timer0_load;
-static void nuc900_clockevent_setmode(enum clock_event_mode mode, +static int nuc900_clockevent_setmode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned int val; @@ -70,9 +70,12 @@ static void nuc900_clockevent_setmode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; }
__raw_writel(val, REG_TCSR0); + return 0; }
static int nuc900_clockevent_setnextevent(unsigned long evt, @@ -92,7 +95,7 @@ static int nuc900_clockevent_setnextevent(unsigned long evt, static struct clock_event_device nuc900_clockevent_device = { .name = "nuc900-timer0", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = nuc900_clockevent_setmode, + .set_dev_mode = nuc900_clockevent_setmode, .set_next_event = nuc900_clockevent_setnextevent, .rating = 300, }; diff --git a/arch/arm/plat-iop/time.c b/arch/arm/plat-iop/time.c index 6ad65d8..a21a18c 100644 --- a/arch/arm/plat-iop/time.c +++ b/arch/arm/plat-iop/time.c @@ -77,7 +77,7 @@ static int iop_set_next_event(unsigned long delta,
static unsigned long ticks_per_jiffy;
-static void iop_set_mode(enum clock_event_mode mode, +static int iop_set_mode(enum clock_event_mode mode, struct clock_event_device *unused) { u32 tmr = read_tmr0(); @@ -98,12 +98,14 @@ static void iop_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: - default: tmr &= ~IOP_TMR_EN; break; + default: + return -ENOSYS; }
write_tmr0(tmr); + return 0; }
static struct clock_event_device iop_clockevent = { @@ -111,7 +113,7 @@ static struct clock_event_device iop_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 300, .set_next_event = iop_set_next_event, - .set_mode = iop_set_mode, + .set_dev_mode = iop_set_mode, };
static irqreturn_t diff --git a/arch/arm/plat-orion/time.c b/arch/arm/plat-orion/time.c index 261258f..7dc5cc8 100644 --- a/arch/arm/plat-orion/time.c +++ b/arch/arm/plat-orion/time.c @@ -106,14 +106,15 @@ orion_clkevt_next_event(unsigned long delta, struct clock_event_device *dev) return 0; }
-static void +static int orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags; u32 u;
local_irq_save(flags); - if (mode == CLOCK_EVT_MODE_PERIODIC) { + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: /* * Setup timer to fire at 1/HZ intervals. */ @@ -132,7 +133,12 @@ orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) u = readl(timer_base + TIMER_CTRL_OFF); writel(u | TIMER1_EN | TIMER1_RELOAD_EN, timer_base + TIMER_CTRL_OFF); - } else { + break; + + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* * Disable timer. */ @@ -149,9 +155,13 @@ orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) * ACK pending timer interrupt. */ writel(bridge_timer1_clr_mask, bridge_base + BRIDGE_CAUSE_OFF); - + break; + default: + local_irq_restore(flags); + return -ENOSYS; } local_irq_restore(flags); + return 0; }
static struct clock_event_device orion_clkevt = { @@ -159,7 +169,7 @@ static struct clock_event_device orion_clkevt = { .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC, .rating = 300, .set_next_event = orion_clkevt_next_event, - .set_mode = orion_clkevt_mode, + .set_dev_mode = orion_clkevt_mode, };
static irqreturn_t orion_timer_interrupt(int irq, void *dev_id)
On Fri, May 23, 2014 at 08:55:04PM +0530, Viresh Kumar wrote:
diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..9ed3845 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -127,7 +127,7 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } -static void sp804_set_mode(enum clock_event_mode mode, +static int sp804_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE; @@ -147,11 +147,14 @@ static void sp804_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
- default:
- case CLOCK_EVT_MODE_RESUME: break;
So your coccinelle script makes sure that all current modes are supported in the case enumeration, right?
- default:
return -ENOSYS;writel(ctrl, clkevt_base + TIMER_CTRL);
Otherwise we would have a functional change here as unsupported mode won't do the writel() anymore.
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c index 0a9e2fc..9698eb6 100644 --- a/arch/arm/mach-at91/at91sam926x_time.c +++ b/arch/arm/mach-at91/at91sam926x_time.c @@ -83,7 +83,7 @@ static struct clocksource pit_clk = { /*
- Clockevent device: interrupts every 1/HZ (== pit_cycles * MCK/16)
*/ -static void +static int pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -93,9 +93,6 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN | AT91_PIT_PITIEN); break;
- case CLOCK_EVT_MODE_ONESHOT:
BUG();/* FALLTHROUGH */@@ -103,7 +100,11 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME: break;
- default:
return -ENOSYS;- return 0;
And here you replace the implementation's BUG() with the generic WARN_ON(), right?
diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c index aee81fa..69ada1e 100644 --- a/arch/arm/mach-clps711x/common.c +++ b/arch/arm/mach-clps711x/common.c @@ -69,7 +69,7 @@ static u64 notrace clps711x_sched_clock_read(void) return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D); } -static void clps711x_clockevent_set_mode(enum clock_event_mode mode, +static int clps711x_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { disable_irq(IRQ_TC2OI); @@ -78,21 +78,22 @@ static void clps711x_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: enable_irq(IRQ_TC2OI); break;
- case CLOCK_EVT_MODE_ONESHOT:
/* Not supported */
- default:
return -ENOSYS;- return 0;
If oneshot is not supported, the mode is never called to the driver?
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index 2756351..8491da9 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -124,24 +124,26 @@ static int timer_set_next_event(unsigned long delta, return 0; } -static void timer_set_mode(enum clock_event_mode mode, +static int timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags;
- local_irq_save(flags); switch (mode) { case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: /* disable the matching interrupt */
local_irq_save(flags);local_irq_restore(flags);
- case CLOCK_EVT_MODE_PERIODIC:
Why periodic is removed here?
break;
- default:
return -ENOSYS;
- local_irq_restore(flags);
- return 0;
} static struct clock_event_device ckevt = { @@ -149,7 +151,7 @@ static struct clock_event_device ckevt = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = timer_set_next_event,
- .set_mode = timer_set_mode,
- .set_dev_mode = timer_set_mode,
};
[...]
static cycle_t clksrc_read(struct clocksource *cs) diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index 1dea6cf..2282858 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -56,7 +56,7 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; } -static void +static int sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -68,9 +68,11 @@ sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break; case CLOCK_EVT_MODE_RESUME:
- case CLOCK_EVT_MODE_PERIODIC:
Same here?
break;
- default:
return -ENOSYS;- return 0;
} #ifdef CONFIG_PM @@ -109,7 +111,7 @@ static struct clock_event_device ckevt_sa1100_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = sa1100_osmr0_set_next_event,
- .set_mode = sa1100_osmr0_set_mode,
- .set_dev_mode = sa1100_osmr0_set_mode, .suspend = sa1100_timer_suspend, .resume = sa1100_timer_resume,
};
On 28 May 2014 19:55, Frederic Weisbecker fweisbec@gmail.com wrote:
On Fri, May 23, 2014 at 08:55:04PM +0530, Viresh Kumar wrote:
diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..9ed3845 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -127,7 +127,7 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; }
-static void sp804_set_mode(enum clock_event_mode mode, +static int sp804_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE; @@ -147,11 +147,14 @@ static void sp804_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
default:
case CLOCK_EVT_MODE_RESUME: break;So your coccinelle script makes sure that all current modes are supported in the case enumeration, right?
Coccinelle script doesn't as it doesn't have very good support for switch blocks. I had a chat with Julia Lawall about it.
This was made sure manually though that all supported modes are present in the switch block and we should never hit the default case with these patches.
default:return -ENOSYS; } writel(ctrl, clkevt_base + TIMER_CTRL);Otherwise we would have a functional change here as unsupported mode won't do the writel() anymore.
Correct.
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c index 0a9e2fc..9698eb6 100644 --- a/arch/arm/mach-at91/at91sam926x_time.c +++ b/arch/arm/mach-at91/at91sam926x_time.c @@ -83,7 +83,7 @@ static struct clocksource pit_clk = { /*
- Clockevent device: interrupts every 1/HZ (== pit_cycles * MCK/16)
*/ -static void +static int pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -93,9 +93,6 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN | AT91_PIT_PITIEN); break;
case CLOCK_EVT_MODE_ONESHOT:BUG();/* FALLTHROUGH */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: /* disable irq, leaving the clocksource active */@@ -103,7 +100,11 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME: break;
default:return -ENOSYS; }return 0;And here you replace the implementation's BUG() with the generic WARN_ON(), right?
Correct and this is mentioned in changelog..
diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c index aee81fa..69ada1e 100644 --- a/arch/arm/mach-clps711x/common.c +++ b/arch/arm/mach-clps711x/common.c @@ -69,7 +69,7 @@ static u64 notrace clps711x_sched_clock_read(void) return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D); }
-static void clps711x_clockevent_set_mode(enum clock_event_mode mode, +static int clps711x_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { disable_irq(IRQ_TC2OI); @@ -78,21 +78,22 @@ static void clps711x_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: enable_irq(IRQ_TC2OI); break;
case CLOCK_EVT_MODE_ONESHOT:/* Not supported */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break;
default:return -ENOSYS; }return 0;If oneshot is not supported, the mode is never called to the driver?
Yes.
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index 2756351..8491da9 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -124,24 +124,26 @@ static int timer_set_next_event(unsigned long delta, return 0; }
-static void timer_set_mode(enum clock_event_mode mode, +static int timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags;
local_irq_save(flags); switch (mode) { case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: /* disable the matching interrupt */
local_irq_save(flags); __raw_writel(0x00, mmp_timer_base + TMR_IER(0));local_irq_restore(flags); break; case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:Why periodic is removed here?
See below..
break;
default:return -ENOSYS; }
local_irq_restore(flags);
return 0;}
static struct clock_event_device ckevt = { @@ -149,7 +151,7 @@ static struct clock_event_device ckevt = { .features = CLOCK_EVT_FEAT_ONESHOT,
PERIODIC isn't supported and it would never be called.
.rating = 200, .set_next_event = timer_set_next_event,
.set_mode = timer_set_mode,
.set_dev_mode = timer_set_mode,};
[...]
static cycle_t clksrc_read(struct clocksource *cs) diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index 1dea6cf..2282858 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -56,7 +56,7 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -68,9 +68,11 @@ sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break;
case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:Same here?
See below..
break;
default:return -ENOSYS; }return 0;}
#ifdef CONFIG_PM @@ -109,7 +111,7 @@ static struct clock_event_device ckevt_sa1100_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT,
Same here.
On Wed, May 28, 2014 at 10:05:54PM +0530, Viresh Kumar wrote:
On 28 May 2014 19:55, Frederic Weisbecker fweisbec@gmail.com wrote:
On Fri, May 23, 2014 at 08:55:04PM +0530, Viresh Kumar wrote:
diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..9ed3845 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -127,7 +127,7 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; }
-static void sp804_set_mode(enum clock_event_mode mode, +static int sp804_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE; @@ -147,11 +147,14 @@ static void sp804_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
default:
case CLOCK_EVT_MODE_RESUME: break;So your coccinelle script makes sure that all current modes are supported in the case enumeration, right?
Coccinelle script doesn't as it doesn't have very good support for switch blocks. I had a chat with Julia Lawall about it.
This was made sure manually though that all supported modes are present in the switch block and we should never hit the default case with these patches.
Ok looks all good then!
default:return -ENOSYS; } writel(ctrl, clkevt_base + TIMER_CTRL);Otherwise we would have a functional change here as unsupported mode won't do the writel() anymore.
Correct.
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c index 0a9e2fc..9698eb6 100644 --- a/arch/arm/mach-at91/at91sam926x_time.c +++ b/arch/arm/mach-at91/at91sam926x_time.c @@ -83,7 +83,7 @@ static struct clocksource pit_clk = { /*
- Clockevent device: interrupts every 1/HZ (== pit_cycles * MCK/16)
*/ -static void +static int pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -93,9 +93,6 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN | AT91_PIT_PITIEN); break;
case CLOCK_EVT_MODE_ONESHOT:BUG();/* FALLTHROUGH */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: /* disable irq, leaving the clocksource active */@@ -103,7 +100,11 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) break; case CLOCK_EVT_MODE_RESUME: break;
default:return -ENOSYS; }return 0;And here you replace the implementation's BUG() with the generic WARN_ON(), right?
Correct and this is mentioned in changelog..
Yeah I'm growing with bad habits :)
Thanks.
Viresh Kumar viresh.kumar@linaro.org writes:
On 28 May 2014 19:55, Frederic Weisbecker fweisbec@gmail.com wrote:
On Fri, May 23, 2014 at 08:55:04PM +0530, Viresh Kumar wrote:
[...]
@@ -78,21 +78,22 @@ static void clps711x_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: enable_irq(IRQ_TC2OI); break;
case CLOCK_EVT_MODE_ONESHOT:/* Not supported */ case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break;
default:return -ENOSYS; }return 0;If oneshot is not supported, the mode is never called to the driver?
Yes.
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c index 2756351..8491da9 100644 --- a/arch/arm/mach-mmp/time.c +++ b/arch/arm/mach-mmp/time.c @@ -124,24 +124,26 @@ static int timer_set_next_event(unsigned long delta, return 0; }
-static void timer_set_mode(enum clock_event_mode mode, +static int timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { unsigned long flags;
local_irq_save(flags); switch (mode) { case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: /* disable the matching interrupt */
local_irq_save(flags); __raw_writel(0x00, mmp_timer_base + TMR_IER(0));local_irq_restore(flags); break; case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:Why periodic is removed here?
See below..
break;
default:return -ENOSYS; }
local_irq_restore(flags);
return 0;}
static struct clock_event_device ckevt = { @@ -149,7 +151,7 @@ static struct clock_event_device ckevt = { .features = CLOCK_EVT_FEAT_ONESHOT,
PERIODIC isn't supported and it would never be called.
.rating = 200, .set_next_event = timer_set_next_event,
.set_mode = timer_set_mode,
.set_dev_mode = timer_set_mode,};
[...]
static cycle_t clksrc_read(struct clocksource *cs) diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c index 1dea6cf..2282858 100644 --- a/arch/arm/mach-sa1100/time.c +++ b/arch/arm/mach-sa1100/time.c @@ -56,7 +56,7 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -68,9 +68,11 @@ sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break;
case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:Same here?
See below..
break;
default:return -ENOSYS; }return 0;}
#ifdef CONFIG_PM @@ -109,7 +111,7 @@ static struct clock_event_device ckevt_sa1100_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT,
Same here.
OK, but strictly speaking, removing the ONESHOT and PERIODIC cases like this has nothing to do with this patch. The fact that they are there even though they will never be called by the core is independent of this series, and having those changes in this patch is distracting and not really related to your goal.
If you want to remove modes that are never called because of feature flags, that should probably be a preliminary cleanup patch and separated out from this change.
Mixing them together is distracting.
Kevin
On 29 May 2014 22:41, Kevin Hilman khilman@linaro.org wrote:
OK, but strictly speaking, removing the ONESHOT and PERIODIC cases like this has nothing to do with this patch. The fact that they are there even though they will never be called by the core is independent of this series, and having those changes in this patch is distracting and not really related to your goal.
If you want to remove modes that are never called because of feature flags, that should probably be a preliminary cleanup patch and separated out from this change.
Mixing them together is distracting.
Hmm, I have seen that enough in yours and preeti's comments. Will fix it :)
Hi Viresh,
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all ARM specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is: diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index fd6bff0..9ed3845 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -127,7 +127,7 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; }
-static void sp804_set_mode(enum clock_event_mode mode, +static int sp804_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE; @@ -147,11 +147,14 @@ static void sp804_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
- default:
case CLOCK_EVT_MODE_RESUME: break;
default:
return -ENOSYS;}
writel(ctrl, clkevt_base + TIMER_CTRL);
return 0;
How about cases like above where just before entering the switch block, where some code(in this case writel) is executed, but for default cases they return early and do not execute the code(writel() in this case) after the switch block. Is that ok?
[snip]
void __iomem *at91_st_base; diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c index 0a9e2fc..9698eb6 100644 --- a/arch/arm/mach-at91/at91sam926x_time.c +++ b/arch/arm/mach-at91/at91sam926x_time.c @@ -83,7 +83,7 @@ static struct clocksource pit_clk = { /*
- Clockevent device: interrupts every 1/HZ (== pit_cycles * MCK/16)
*/ -static void +static int pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -93,9 +93,6 @@ pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN | AT91_PIT_PITIEN); break;
- case CLOCK_EVT_MODE_ONESHOT:
BUG();/* FALLTHROUGH */
Don't you think you should keep the BUG_ON() as is? This becomes a WARN_ON() instead. Would that be ok? While the original code thinks it unwise to continue executing after this, this patch allows that to happen.
Regards Preeti U Murthy
On 29 May 2014 15:04, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
How about cases like above where just before entering the switch block, where some code(in this case writel) is executed, but for default cases they return early and do not execute the code(writel() in this case) after the switch block. Is that ok?
Yeah, that's a problem and this is what I mentioned in the logs that we discussed earlier. We shouldn't disable timer for default modes.
Though it was already there and so isn't fixed with this patchset. Would be fixed later on..
And as I replied to Kevin, I would not be removing any unsupported modes anymore in this patchset.
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/mips/alchemy/common/time.c | 14 ++++++++++++-- arch/mips/include/asm/cevt-r4k.h | 2 +- arch/mips/jazz/irq.c | 14 ++++++++++++-- arch/mips/jz4740/time.c | 9 ++++++--- arch/mips/kernel/cevt-bcm1480.c | 11 +++++++---- arch/mips/kernel/cevt-ds1287.c | 12 +++++++++--- arch/mips/kernel/cevt-gic.c | 14 ++++++++++++-- arch/mips/kernel/cevt-gt641xx.c | 12 +++++++++--- arch/mips/kernel/cevt-r4k.c | 14 ++++++++++++-- arch/mips/kernel/cevt-sb1250.c | 11 +++++++---- arch/mips/kernel/cevt-smtc.c | 2 +- arch/mips/kernel/cevt-txx9.c | 7 +++++-- arch/mips/loongson/common/cs5536/cs5536_mfgpt.c | 12 ++++++------ arch/mips/ralink/cevt-rt3352.c | 13 ++++++++----- arch/mips/sgi-ip27/ip27-timer.c | 14 ++++++++++++-- arch/mips/sni/time.c | 9 +++++---- 16 files changed, 124 insertions(+), 46 deletions(-)
diff --git a/arch/mips/alchemy/common/time.c b/arch/mips/alchemy/common/time.c index 93fa586..4af4ed2 100644 --- a/arch/mips/alchemy/common/time.c +++ b/arch/mips/alchemy/common/time.c @@ -70,9 +70,19 @@ static int au1x_rtcmatch2_set_next_event(unsigned long delta, return 0; }
-static void au1x_rtcmatch2_set_mode(enum clock_event_mode mode, +static int au1x_rtcmatch2_set_mode(enum clock_event_mode mode, struct clock_event_device *cd) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + break; + default: + return -ENOSYS; + } + return 0; }
static irqreturn_t au1x_rtcmatch2_irq(int irq, void *dev_id) @@ -87,7 +97,7 @@ static struct clock_event_device au1x_rtcmatch2_clockdev = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 1500, .set_next_event = au1x_rtcmatch2_set_next_event, - .set_mode = au1x_rtcmatch2_set_mode, + .set_dev_mode = au1x_rtcmatch2_set_mode, .cpumask = cpu_all_mask, };
diff --git a/arch/mips/include/asm/cevt-r4k.h b/arch/mips/include/asm/cevt-r4k.h index 65f9bdd..e4872df 100644 --- a/arch/mips/include/asm/cevt-r4k.h +++ b/arch/mips/include/asm/cevt-r4k.h @@ -21,7 +21,7 @@ DECLARE_PER_CPU(struct clock_event_device, mips_clockevent_device);
void mips_event_handler(struct clock_event_device *dev); int c0_compare_int_usable(void); -void mips_set_clock_mode(enum clock_event_mode, struct clock_event_device *); +int mips_set_clock_mode(enum clock_event_mode, struct clock_event_device *); irqreturn_t c0_compare_interrupt(int, void *);
extern struct irqaction c0_compare_irqaction; diff --git a/arch/mips/jazz/irq.c b/arch/mips/jazz/irq.c index e1ea4f6..1ceee92 100644 --- a/arch/mips/jazz/irq.c +++ b/arch/mips/jazz/irq.c @@ -110,10 +110,20 @@ asmlinkage void plat_irq_dispatch(void) } }
-static void r4030_set_mode(enum clock_event_mode mode, +static int r4030_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */ + break; + default: + return -ENOSYS; + } + return 0; }
struct clock_event_device r4030_clockevent = { @@ -121,7 +131,7 @@ struct clock_event_device r4030_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC, .rating = 300, .irq = JAZZ_TIMER_IRQ, - .set_mode = r4030_set_mode, + .set_dev_mode = r4030_set_mode, };
static irqreturn_t r4030_timer_interrupt(int irq, void *dev_id) diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c index 5e430ce..2eb3fe1 100644 --- a/arch/mips/jz4740/time.c +++ b/arch/mips/jz4740/time.c @@ -57,7 +57,7 @@ static irqreturn_t jz4740_clockevent_irq(int irq, void *devid) return IRQ_HANDLED; }
-static void jz4740_clockevent_set_mode(enum clock_event_mode mode, +static int jz4740_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *cd) { switch (mode) { @@ -72,9 +72,12 @@ static void jz4740_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: jz4740_timer_disable(TIMER_CLOCKEVENT); break; - default: + case CLOCK_EVT_MODE_UNUSED: break; + default: + return -ENOSYS; } + return 0; }
static int jz4740_clockevent_set_next(unsigned long evt, @@ -91,7 +94,7 @@ static struct clock_event_device jz4740_clockevent = { .name = "jz4740-timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = jz4740_clockevent_set_next, - .set_mode = jz4740_clockevent_set_mode, + .set_dev_mode = jz4740_clockevent_set_mode, .rating = 200, .irq = JZ4740_IRQ_TCU0, }; diff --git a/arch/mips/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457..007fdb1 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -40,7 +40,7 @@ * The general purpose timer ticks at 1MHz independent if * the rest of the system */ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -63,10 +63,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
- case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */ + case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: - ; + break; + default: + return -ENOSYS; } + return 0; }
static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -130,7 +133,7 @@ void sb1480_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event; - cd->set_mode = sibyte_set_mode; + cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd);
bcm1480_mask_irq(cpu, irq); diff --git a/arch/mips/kernel/cevt-ds1287.c b/arch/mips/kernel/cevt-ds1287.c index ff1f01b..f86642a 100644 --- a/arch/mips/kernel/cevt-ds1287.c +++ b/arch/mips/kernel/cevt-ds1287.c @@ -59,7 +59,7 @@ static int ds1287_set_next_event(unsigned long delta, return -EINVAL; }
-static void ds1287_set_mode(enum clock_event_mode mode, +static int ds1287_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u8 val; @@ -72,14 +72,20 @@ static void ds1287_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: val |= RTC_PIE; break; - default: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: val &= ~RTC_PIE; break; + default: + spin_unlock(&rtc_lock); + return -ENOSYS; }
CMOS_WRITE(val, RTC_REG_B);
spin_unlock(&rtc_lock); + return 0; }
static void ds1287_event_handler(struct clock_event_device *dev) @@ -90,7 +96,7 @@ static struct clock_event_device ds1287_clockevent = { .name = "ds1287", .features = CLOCK_EVT_FEAT_PERIODIC, .set_next_event = ds1287_set_next_event, - .set_mode = ds1287_set_mode, + .set_dev_mode = ds1287_set_mode, .event_handler = ds1287_event_handler, };
diff --git a/arch/mips/kernel/cevt-gic.c b/arch/mips/kernel/cevt-gic.c index 594cbbf..ae4b1ee 100644 --- a/arch/mips/kernel/cevt-gic.c +++ b/arch/mips/kernel/cevt-gic.c @@ -31,10 +31,20 @@ static int gic_next_event(unsigned long delta, struct clock_event_device *evt) return res; }
-void gic_set_clock_mode(enum clock_event_mode mode, +int gic_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */ + break; + default: + return -ENOSYS; + } + return 0; }
irqreturn_t gic_compare_interrupt(int irq, void *dev_id) @@ -85,7 +95,7 @@ int gic_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = gic_next_event; - cd->set_mode = gic_set_clock_mode; + cd->set_dev_mode = gic_set_clock_mode; cd->event_handler = gic_event_handler;
clockevents_register_device(cd); diff --git a/arch/mips/kernel/cevt-gt641xx.c b/arch/mips/kernel/cevt-gt641xx.c index f069460..7c152ba 100644 --- a/arch/mips/kernel/cevt-gt641xx.c +++ b/arch/mips/kernel/cevt-gt641xx.c @@ -64,7 +64,7 @@ static int gt641xx_timer0_set_next_event(unsigned long delta, return 0; }
-static void gt641xx_timer0_set_mode(enum clock_event_mode mode, +static int gt641xx_timer0_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -81,13 +81,19 @@ static void gt641xx_timer0_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: ctrl |= GT_TC_CONTROL_ENTC0_MSK; break; - default: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: break; + default: + raw_spin_unlock(>641xx_timer_lock); + return -ENOSYS; }
GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
raw_spin_unlock(>641xx_timer_lock); + return 0; }
static void gt641xx_timer0_event_handler(struct clock_event_device *dev) @@ -99,7 +105,7 @@ static struct clock_event_device gt641xx_timer0_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .irq = GT641XX_TIMER0_IRQ, .set_next_event = gt641xx_timer0_set_next_event, - .set_mode = gt641xx_timer0_set_mode, + .set_dev_mode = gt641xx_timer0_set_mode, .event_handler = gt641xx_timer0_event_handler, };
diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c index 50d3f5a..3ad0f42 100644 --- a/arch/mips/kernel/cevt-r4k.c +++ b/arch/mips/kernel/cevt-r4k.c @@ -38,10 +38,20 @@ static int mips_next_event(unsigned long delta,
#endif /* CONFIG_MIPS_MT_SMTC */
-void mips_set_clock_mode(enum clock_event_mode mode, +int mips_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */ + break; + default: + return -ENOSYS; + } + return 0; }
DEFINE_PER_CPU(struct clock_event_device, mips_clockevent_device); @@ -207,7 +217,7 @@ int r4k_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = mips_next_event; - cd->set_mode = mips_set_clock_mode; + cd->set_dev_mode = mips_set_clock_mode; cd->event_handler = mips_event_handler;
#ifdef CONFIG_CEVT_GIC diff --git a/arch/mips/kernel/cevt-sb1250.c b/arch/mips/kernel/cevt-sb1250.c index 5ea6d6b..f7cf430 100644 --- a/arch/mips/kernel/cevt-sb1250.c +++ b/arch/mips/kernel/cevt-sb1250.c @@ -38,7 +38,7 @@ * The general purpose timer ticks at 1MHz independent if * the rest of the system */ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -61,10 +61,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
- case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */ + case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: - ; + break; + default: + return -ENOSYS; } + return 0; }
static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -129,7 +132,7 @@ void sb1250_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event; - cd->set_mode = sibyte_set_mode; + cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd);
sb1250_mask_irq(cpu, irq); diff --git a/arch/mips/kernel/cevt-smtc.c b/arch/mips/kernel/cevt-smtc.c index b6cf0a6..0d41366 100644 --- a/arch/mips/kernel/cevt-smtc.c +++ b/arch/mips/kernel/cevt-smtc.c @@ -297,7 +297,7 @@ int smtc_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = mips_next_event; - cd->set_mode = mips_set_clock_mode; + cd->set_dev_mode = mips_set_clock_mode; cd->event_handler = mips_event_handler;
clockevents_register_device(cd); diff --git a/arch/mips/kernel/cevt-txx9.c b/arch/mips/kernel/cevt-txx9.c index 2ae0846..05bd5ce 100644 --- a/arch/mips/kernel/cevt-txx9.c +++ b/arch/mips/kernel/cevt-txx9.c @@ -76,7 +76,7 @@ static void txx9tmr_stop_and_clear(struct txx9_tmr_reg __iomem *tmrptr) __raw_writel(0, &tmrptr->tisr); }
-static void txx9tmr_set_mode(enum clock_event_mode mode, +static int txx9tmr_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct txx9_clock_event_device *txx9_cd = @@ -105,7 +105,10 @@ static void txx9tmr_set_mode(enum clock_event_mode mode, __raw_writel(TIMER_CCD, &tmrptr->ccdr); __raw_writel(0, &tmrptr->itmr); break; + default: + return -ENOSYS; } + return 0; }
static int txx9tmr_set_next_event(unsigned long delta, @@ -128,7 +131,7 @@ static struct txx9_clock_event_device txx9_clock_event_device = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 200, - .set_mode = txx9tmr_set_mode, + .set_dev_mode = txx9tmr_set_mode, .set_next_event = txx9tmr_set_next_event, }, }; diff --git a/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c b/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c index c639b9d..846f4ab 100644 --- a/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c +++ b/arch/mips/loongson/common/cs5536/cs5536_mfgpt.c @@ -52,7 +52,7 @@ void enable_mfgpt0_counter(void) } EXPORT_SYMBOL(enable_mfgpt0_counter);
-static void init_mfgpt_timer(enum clock_event_mode mode, +static int init_mfgpt_timer(enum clock_event_mode mode, struct clock_event_device *evt) { spin_lock(&mfgpt_lock); @@ -71,21 +71,21 @@ static void init_mfgpt_timer(enum clock_event_mode mode, disable_mfgpt0_counter(); break;
- case CLOCK_EVT_MODE_ONESHOT: - /* The oneshot mode have very high deviation, Not use it! */ - break; - case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + spin_unlock(&mfgpt_lock); + return -ENOSYS; } spin_unlock(&mfgpt_lock); + return 0; }
static struct clock_event_device mfgpt_clockevent = { .name = "mfgpt", .features = CLOCK_EVT_FEAT_PERIODIC, - .set_mode = init_mfgpt_timer, + .set_dev_mode = init_mfgpt_timer, .irq = CS5536_MFGPT_INTR, };
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event, - .set_mode = systick_set_clock_mode, + .set_dev_mode = systick_set_clock_mode, .event_handler = systick_event_handler, }, }; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
- default: - pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void __init ralink_systick_init(struct device_node *np) diff --git a/arch/mips/sgi-ip27/ip27-timer.c b/arch/mips/sgi-ip27/ip27-timer.c index 1d97eab..5d5e90c 100644 --- a/arch/mips/sgi-ip27/ip27-timer.c +++ b/arch/mips/sgi-ip27/ip27-timer.c @@ -63,10 +63,20 @@ static int rt_next_event(unsigned long delta, struct clock_event_device *evt) return LOCAL_HUB_L(PI_RT_COUNT) >= cnt ? -ETIME : 0; }
-static void rt_set_mode(enum clock_event_mode mode, +static int rt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */ + break; + default: + return -ENOSYS; + } + return 0; }
unsigned int rt_timer_irq; @@ -123,7 +133,7 @@ void hub_rt_clock_event_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = rt_next_event; - cd->set_mode = rt_set_mode; + cd->set_dev_mode = rt_set_mode; clockevents_register_device(cd); }
diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec56..ac666e5 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -14,7 +14,7 @@ #define SNI_COUNTER2_DIV 64 #define SNI_COUNTER0_DIV ((SNI_CLOCK_TICK_RATE / SNI_COUNTER2_DIV) / HZ)
-static void a20r_set_mode(enum clock_event_mode mode, +static int a20r_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -34,13 +34,14 @@ static void a20r_set_mode(enum clock_event_mode mode, wmb();
break; - case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device a20r_clockevent_device = { @@ -51,7 +52,7 @@ static struct clock_event_device a20r_clockevent_device = {
.rating = 300, .irq = SNI_A20R_IRQ_TIMER, - .set_mode = a20r_set_mode, + .set_dev_mode = a20r_set_mode, };
static irqreturn_t a20r_interrupt(int irq, void *dev_id)
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@
[snip]
diff --git a/arch/mips/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457..007fdb1 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -40,7 +40,7 @@
- The general purpose timer ticks at 1MHz independent if
- the rest of the system
*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -63,10 +63,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
- case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */
Why is this comment removed?
- case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:
;
break;- default:
return -ENOSYS;- return 0;
}
static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -130,7 +133,7 @@ void sb1480_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event;
- cd->set_mode = sibyte_set_mode;
cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd);
bcm1480_mask_irq(cpu, irq);
diff --git a/arch/mips/kernel/cevt-ds1287.c b/arch/mips/kernel/cevt-ds1287.c index ff1f01b..f86642a 100644 --- a/arch/mips/kernel/cevt-ds1287.c +++ b/arch/mips/kernel/cevt-ds1287.c @@ -59,7 +59,7 @@ static int ds1287_set_next_event(unsigned long delta, return -EINVAL; }
-static void ds1287_set_mode(enum clock_event_mode mode, +static int ds1287_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u8 val; @@ -72,14 +72,20 @@ static void ds1287_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: val |= RTC_PIE; break;
- default:
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
case CLOCK_EVT_MODE_RESUME: val &= ~RTC_PIE; break;
default:
spin_unlock(&rtc_lock);return -ENOSYS;}
CMOS_WRITE(val, RTC_REG_B);
What about this ^^ in default case ? In default case, they would fall through and execute this instruction. After this patch, they won't. Would this be ok?
spin_unlock(&rtc_lock);
- return 0;
}
[snip]
diff --git a/arch/mips/kernel/cevt-sb1250.c b/arch/mips/kernel/cevt-sb1250.c index 5ea6d6b..f7cf430 100644 --- a/arch/mips/kernel/cevt-sb1250.c +++ b/arch/mips/kernel/cevt-sb1250.c @@ -38,7 +38,7 @@
- The general purpose timer ticks at 1MHz independent if
- the rest of the system
*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -61,10 +61,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
- case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */
Same here.
- case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:
;
break;- default:
return -ENOSYS;- return 0;
}
[snip]
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event,
.set_mode = systick_set_clock_mode,
.set_dev_mode = systick_set_clock_mode,}; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
- default:
pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_RESUME: break;
- default:
return -ENOSYS;
This is another scenario I am confused about. We have a pr_err() already in the default case. Why do we skip adding the pr_err() in the case of MODE_UNUSED and MODE_RESUME and in the default case ? Only then would it be consistent with the earlier scenario right? I am saying something like the below:
- default: - pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_RESUME: + pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); break; + default: + pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); + return -ENOSYS; }
- return 0;
}
static void __init ralink_systick_init(struct device_node *np) diff --git a/arch/mips/sgi-ip27/ip27-timer.c b/arch/mips/sgi-ip27/ip27-timer.c index 1d97eab..5d5e90c 100644 --- a/arch/mips/sgi-ip27/ip27-timer.c +++ b/arch/mips/sgi-ip27/ip27-timer.c @@ -63,10 +63,20 @@ static int rt_next_event(unsigned long delta, struct clock_event_device *evt) return LOCAL_HUB_L(PI_RT_COUNT) >= cnt ? -ETIME : 0; }
-static void rt_set_mode(enum clock_event_mode mode, +static int rt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
- switch (mode) {
- case CLOCK_EVT_MODE_ONESHOT:
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */
break;- default:
return -ENOSYS;- }
- return 0;
}
unsigned int rt_timer_irq; @@ -123,7 +133,7 @@ void hub_rt_clock_event_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = rt_next_event;
- cd->set_mode = rt_set_mode;
- cd->set_dev_mode = rt_set_mode; clockevents_register_device(cd);
}
diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec56..ac666e5 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -14,7 +14,7 @@ #define SNI_COUNTER2_DIV 64 #define SNI_COUNTER0_DIV ((SNI_CLOCK_TICK_RATE / SNI_COUNTER2_DIV) / HZ)
-static void a20r_set_mode(enum clock_event_mode mode, +static int a20r_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -34,13 +34,14 @@ static void a20r_set_mode(enum clock_event_mode mode, wmb();
break;
- case CLOCK_EVT_MODE_ONESHOT:
Why is this line deleted?
On 29 May 2014 15:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@
[snip]
diff --git a/arch/mips/kernel/cevt-bcm1480.c b/arch/mips/kernel/cevt-bcm1480.c index 7976457..007fdb1 100644 --- a/arch/mips/kernel/cevt-bcm1480.c +++ b/arch/mips/kernel/cevt-bcm1480.c @@ -40,7 +40,7 @@
- The general purpose timer ticks at 1MHz independent if
- the rest of the system
*/ -static void sibyte_set_mode(enum clock_event_mode mode, +static int sibyte_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); @@ -63,10 +63,13 @@ static void sibyte_set_mode(enum clock_event_mode mode, __raw_writeq(0, cfg); break;
case CLOCK_EVT_MODE_UNUSED: /* shuddup gcc */Why is this comment removed?
I thought it was added there because it didn't had a default case and these empty 'cases' are there to make sure compiler doesn't throw warnings at them.
Because we have a default case now, these prints should go.
case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME:
;
break;default:return -ENOSYS; }return 0;}
static int sibyte_next_event(unsigned long delta, struct clock_event_device *cd) @@ -130,7 +133,7 @@ void sb1480_clockevent_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = sibyte_next_event;
cd->set_mode = sibyte_set_mode;
cd->set_dev_mode = sibyte_set_mode; clockevents_register_device(cd); bcm1480_mask_irq(cpu, irq);diff --git a/arch/mips/kernel/cevt-ds1287.c b/arch/mips/kernel/cevt-ds1287.c index ff1f01b..f86642a 100644 --- a/arch/mips/kernel/cevt-ds1287.c +++ b/arch/mips/kernel/cevt-ds1287.c @@ -59,7 +59,7 @@ static int ds1287_set_next_event(unsigned long delta, return -EINVAL; }
-static void ds1287_set_mode(enum clock_event_mode mode, +static int ds1287_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u8 val; @@ -72,14 +72,20 @@ static void ds1287_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_PERIODIC: val |= RTC_PIE; break;
default:
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: val &= ~RTC_PIE; break;default:spin_unlock(&rtc_lock);return -ENOSYS; } CMOS_WRITE(val, RTC_REG_B);What about this ^^ in default case ? In default case, they would fall through and execute this instruction. After this patch, they won't. Would this be ok?
They shouldn't execute this for any unsupported modes, like ONESHOT_STOPPED as we are handling all available modes in the switch block now.
Earlier default meant: UNUSED/SHUTDOWN/RESUME probably.
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event,
.set_mode = systick_set_clock_mode,
.set_dev_mode = systick_set_clock_mode, .event_handler = systick_event_handler, },}; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME: break;default:return -ENOSYS; }This is another scenario I am confused about. We have a pr_err() already in the default case. Why do we skip adding the pr_err() in the case of MODE_UNUSED and MODE_RESUME and in the default case ? Only then would it be consistent with the earlier scenario right? I am saying something like the below:
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); break;default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);return -ENOSYS; }
These modes aren't optional and so we don't return any error from them. And so doing a pr_err wouldn't be right anymore.
return 0;}
static void __init ralink_systick_init(struct device_node *np) diff --git a/arch/mips/sgi-ip27/ip27-timer.c b/arch/mips/sgi-ip27/ip27-timer.c index 1d97eab..5d5e90c 100644 --- a/arch/mips/sgi-ip27/ip27-timer.c +++ b/arch/mips/sgi-ip27/ip27-timer.c @@ -63,10 +63,20 @@ static int rt_next_event(unsigned long delta, struct clock_event_device *evt) return LOCAL_HUB_L(PI_RT_COUNT) >= cnt ? -ETIME : 0; }
-static void rt_set_mode(enum clock_event_mode mode, +static int rt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
switch (mode) {case CLOCK_EVT_MODE_ONESHOT:case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */break;default:return -ENOSYS;}return 0;}
unsigned int rt_timer_irq; @@ -123,7 +133,7 @@ void hub_rt_clock_event_init(void) cd->irq = irq; cd->cpumask = cpumask_of(cpu); cd->set_next_event = rt_next_event;
cd->set_mode = rt_set_mode;
cd->set_dev_mode = rt_set_mode; clockevents_register_device(cd);}
diff --git a/arch/mips/sni/time.c b/arch/mips/sni/time.c index cf8ec56..ac666e5 100644 --- a/arch/mips/sni/time.c +++ b/arch/mips/sni/time.c @@ -14,7 +14,7 @@ #define SNI_COUNTER2_DIV 64 #define SNI_COUNTER0_DIV ((SNI_CLOCK_TICK_RATE / SNI_COUNTER2_DIV) / HZ)
-static void a20r_set_mode(enum clock_event_mode mode, +static int a20r_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -34,13 +34,14 @@ static void a20r_set_mode(enum clock_event_mode mode, wmb();
break;
case CLOCK_EVT_MODE_ONESHOT:Why is this line deleted?
ONESHOT isn't supported by this driver, they just had a empty case here.
On 05/30/2014 07:06 AM, Viresh Kumar wrote:
On 29 May 2014 15:28, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all MIPS specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
[snip]
diff --git a/arch/mips/ralink/cevt-rt3352.c b/arch/mips/ralink/cevt-rt3352.c index 24bf057..c92b211 100644 --- a/arch/mips/ralink/cevt-rt3352.c +++ b/arch/mips/ralink/cevt-rt3352.c @@ -36,7 +36,7 @@ struct systick_device { int freq_scale; };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt);
static int systick_next_event(unsigned long delta, @@ -76,7 +76,7 @@ static struct systick_device systick = { .rating = 310, .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = systick_next_event,
.set_mode = systick_set_clock_mode,
.set_dev_mode = systick_set_clock_mode, .event_handler = systick_event_handler, },}; @@ -87,7 +87,7 @@ static struct irqaction systick_irqaction = { .dev_id = &systick.dev, };
-static void systick_set_clock_mode(enum clock_event_mode mode, +static int systick_set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct systick_device *sdev; @@ -110,10 +110,13 @@ static void systick_set_clock_mode(enum clock_event_mode mode, iowrite32(0, systick.membase + SYSTICK_CONFIG); break;
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME: break;default:return -ENOSYS; }This is another scenario I am confused about. We have a pr_err() already in the default case. Why do we skip adding the pr_err() in the case of MODE_UNUSED and MODE_RESUME and in the default case ? Only then would it be consistent with the earlier scenario right? I am saying something like the below:
default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);
case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_RESUME:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name); break;default:pr_err("%s: Unhandeled mips clock_mode\n", systick.dev.name);return -ENOSYS; }These modes aren't optional and so we don't return any error from them. And so doing a pr_err wouldn't be right anymore.
In which case why don't you club them with the 'default' case ? default case is meant for unhandled clock modes anyway.
I am fine with the rest of this patch.
Regards Preeti U Murthy
On 30 May 2014 11:54, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
In which case why don't you club them with the 'default' case ? default case is meant for unhandled clock modes anyway.
We are returning error from default and shouldn't return errors from these modes.. And so are they separate.
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all SPARC specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/sparc/kernel/time_32.c | 18 ++++++++++++------ arch/sparc/kernel/time_64.c | 11 +++++------ 2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/sparc/kernel/time_32.c b/arch/sparc/kernel/time_32.c index c4c27b0..4861c53 100644 --- a/arch/sparc/kernel/time_32.c +++ b/arch/sparc/kernel/time_32.c @@ -107,7 +107,7 @@ irqreturn_t notrace timer_interrupt(int dummy, void *dev_id) return IRQ_HANDLED; }
-static void timer_ce_set_mode(enum clock_event_mode mode, +static int timer_ce_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -118,10 +118,13 @@ static void timer_ce_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: timer_ce_enabled = 0; break; - default: + case CLOCK_EVT_MODE_UNUSED: break; + default: + return -ENOSYS; } smp_mb(); + return 0; }
static __init void setup_timer_ce(void) @@ -133,7 +136,7 @@ static __init void setup_timer_ce(void) ce->name = "timer_ce"; ce->rating = 100; ce->features = CLOCK_EVT_FEAT_PERIODIC; - ce->set_mode = timer_ce_set_mode; + ce->set_dev_mode = timer_ce_set_mode; ce->cpumask = cpu_possible_mask; ce->shift = 32; ce->mult = div_sc(sparc_config.clock_rate, NSEC_PER_SEC, @@ -193,7 +196,7 @@ static __init int setup_timer_cs(void) }
#ifdef CONFIG_SMP -static void percpu_ce_setup(enum clock_event_mode mode, +static int percpu_ce_setup(enum clock_event_mode mode, struct clock_event_device *evt) { int cpu = __first_cpu(evt->cpumask); @@ -208,9 +211,12 @@ static void percpu_ce_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: sparc_config.load_profile_irq(cpu, 0); break; - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int percpu_ce_set_next_event(unsigned long delta, @@ -234,7 +240,7 @@ void register_percpu_ce(int cpu) ce->name = "percpu_ce"; ce->rating = 200; ce->features = features; - ce->set_mode = percpu_ce_setup; + ce->set_dev_mode = percpu_ce_setup; ce->set_next_event = percpu_ce_set_next_event; ce->cpumask = cpumask_of(cpu); ce->shift = 32; diff --git a/arch/sparc/kernel/time_64.c b/arch/sparc/kernel/time_64.c index 3fddf64..1708edc 100644 --- a/arch/sparc/kernel/time_64.c +++ b/arch/sparc/kernel/time_64.c @@ -691,7 +691,7 @@ static int sparc64_next_event(unsigned long delta, return tick_ops->add_compare(delta) ? -ETIME : 0; }
-static void sparc64_timer_setup(enum clock_event_mode mode, +static int sparc64_timer_setup(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -703,16 +703,15 @@ static void sparc64_timer_setup(enum clock_event_mode mode, tick_ops->disable_irq(); break;
- case CLOCK_EVT_MODE_PERIODIC: - case CLOCK_EVT_MODE_UNUSED: - WARN_ON(1); - break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device sparc64_clockevent = { .features = CLOCK_EVT_FEAT_ONESHOT, - .set_mode = sparc64_timer_setup, + .set_dev_mode = sparc64_timer_setup, .set_next_event = sparc64_next_event, .rating = 100, .shift = 30,
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all x86 specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/x86/kernel/apic/apic.c | 10 +++++++--- arch/x86/kernel/hpet.c | 19 +++++++++++-------- arch/x86/lguest/boot.c | 9 +++++---- arch/x86/platform/uv/uv_time.c | 10 ++++++---- arch/x86/xen/time.c | 23 ++++++++++------------- 5 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index ad28db7..f89084e 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -495,7 +495,7 @@ static int lapic_next_deadline(unsigned long delta, /* * Setup the lapic timer in periodic or oneshot mode */ -static void lapic_timer_setup(enum clock_event_mode mode, +static int lapic_timer_setup(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long flags; @@ -503,7 +503,7 @@ static void lapic_timer_setup(enum clock_event_mode mode,
/* Lapic used as dummy for broadcast ? */ if (evt->features & CLOCK_EVT_FEAT_DUMMY) - return; + return 0;
local_irq_save(flags);
@@ -523,9 +523,13 @@ static void lapic_timer_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + local_irq_restore(flags); + return -ENOSYS; }
local_irq_restore(flags); + return 0; }
/* @@ -547,7 +551,7 @@ static struct clock_event_device lapic_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP | CLOCK_EVT_FEAT_DUMMY, .shift = 32, - .set_mode = lapic_timer_setup, + .set_dev_mode = lapic_timer_setup, .set_next_event = lapic_next_event, .broadcast = lapic_timer_broadcast, .rating = 100, diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c index 4177bfb..8cab472 100644 --- a/arch/x86/kernel/hpet.c +++ b/arch/x86/kernel/hpet.c @@ -228,7 +228,7 @@ static void hpet_reserve_platform_timers(unsigned int id) { } */ static unsigned long hpet_freq;
-static void hpet_legacy_set_mode(enum clock_event_mode mode, +static int hpet_legacy_set_mode(enum clock_event_mode mode, struct clock_event_device *evt); static int hpet_legacy_next_event(unsigned long delta, struct clock_event_device *evt); @@ -239,7 +239,7 @@ static int hpet_legacy_next_event(unsigned long delta, static struct clock_event_device hpet_clockevent = { .name = "hpet", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = hpet_legacy_set_mode, + .set_dev_mode = hpet_legacy_set_mode, .set_next_event = hpet_legacy_next_event, .irq = 0, .rating = 50, @@ -310,7 +310,7 @@ static void hpet_legacy_clockevent_register(void)
static int hpet_setup_msi_irq(unsigned int irq);
-static void hpet_set_mode(enum clock_event_mode mode, +static int hpet_set_mode(enum clock_event_mode mode, struct clock_event_device *evt, int timer) { unsigned int cfg, cmp, now; @@ -367,7 +367,10 @@ static void hpet_set_mode(enum clock_event_mode mode, } hpet_print_config(); break; + default: + return -ENOSYS; } + return 0; }
static int hpet_next_event(unsigned long delta, @@ -407,10 +410,10 @@ static int hpet_next_event(unsigned long delta, return res < HPET_MIN_CYCLES ? -ETIME : 0; }
-static void hpet_legacy_set_mode(enum clock_event_mode mode, +static int hpet_legacy_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { - hpet_set_mode(mode, evt, 0); + return hpet_set_mode(mode, evt, 0); }
static int hpet_legacy_next_event(unsigned long delta, @@ -462,11 +465,11 @@ void hpet_msi_read(struct hpet_dev *hdev, struct msi_msg *msg) msg->address_hi = 0; }
-static void hpet_msi_set_mode(enum clock_event_mode mode, +static int hpet_msi_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt); - hpet_set_mode(mode, evt, hdev->num); + return hpet_set_mode(mode, evt, hdev->num); }
static int hpet_msi_next_event(unsigned long delta, @@ -558,7 +561,7 @@ static void init_one_hpet_msi_clockevent(struct hpet_dev *hdev, int cpu) if (hdev->flags & HPET_DEV_PERI_CAP) evt->features |= CLOCK_EVT_FEAT_PERIODIC;
- evt->set_mode = hpet_msi_set_mode; + evt->set_dev_mode = hpet_msi_set_mode; evt->set_next_event = hpet_msi_next_event; evt->cpumask = cpumask_of(hdev->cpu);
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index aae9413..54aa944 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -962,7 +962,7 @@ static int lguest_clockevent_set_next_event(unsigned long delta, return 0; }
-static void lguest_clockevent_set_mode(enum clock_event_mode mode, +static int lguest_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -974,11 +974,12 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: /* This is what we expect. */ break; - case CLOCK_EVT_MODE_PERIODIC: - BUG(); case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
/* This describes our primitive timer chip. */ @@ -986,7 +987,7 @@ static struct clock_event_device lguest_clockevent = { .name = "lguest", .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = lguest_clockevent_set_next_event, - .set_mode = lguest_clockevent_set_mode, + .set_dev_mode = lguest_clockevent_set_mode, .rating = INT_MAX, .mult = 1, .shift = 0, diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c index 5c86786..b9c822d 100644 --- a/arch/x86/platform/uv/uv_time.c +++ b/arch/x86/platform/uv/uv_time.c @@ -32,7 +32,7 @@
static cycle_t uv_read_rtc(struct clocksource *cs); static int uv_rtc_next_event(unsigned long, struct clock_event_device *); -static void uv_rtc_timer_setup(enum clock_event_mode, +static int uv_rtc_timer_setup(enum clock_event_mode, struct clock_event_device *);
static struct clocksource clocksource_uv = { @@ -50,7 +50,7 @@ static struct clock_event_device clock_event_device_uv = { .rating = 400, .irq = -1, .set_next_event = uv_rtc_next_event, - .set_mode = uv_rtc_timer_setup, + .set_dev_mode = uv_rtc_timer_setup, .event_handler = NULL, };
@@ -323,13 +323,12 @@ static int uv_rtc_next_event(unsigned long delta, /* * Setup the RTC timer in oneshot mode */ -static void uv_rtc_timer_setup(enum clock_event_mode mode, +static int uv_rtc_timer_setup(enum clock_event_mode mode, struct clock_event_device *evt) { int ced_cpu = cpumask_first(evt->cpumask);
switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_RESUME: /* Nothing to do here yet */ @@ -338,7 +337,10 @@ static void uv_rtc_timer_setup(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: uv_rtc_unset_timer(ced_cpu, 1); break; + default: + return -ENOSYS; } + return 0; }
static void uv_rtc_interrupt(void) diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c index 7b78f88..08952ed 100644 --- a/arch/x86/xen/time.c +++ b/arch/x86/xen/time.c @@ -274,15 +274,10 @@ static s64 get_abs_timeout(unsigned long delta) return xen_clocksource_read() + delta; }
-static void xen_timerop_set_mode(enum clock_event_mode mode, +static int xen_timerop_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - /* unsupported */ - WARN_ON(1); - break; - case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_RESUME: break; @@ -291,7 +286,10 @@ static void xen_timerop_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: HYPERVISOR_set_timer_op(0); /* cancel timeout */ break; + default: + return -ENOSYS; } + return 0; }
static int xen_timerop_set_next_event(unsigned long delta, @@ -320,22 +318,18 @@ static const struct clock_event_device xen_timerop_clockevent = { .shift = 0, .rating = 500,
- .set_mode = xen_timerop_set_mode, + .set_dev_mode = xen_timerop_set_mode, .set_next_event = xen_timerop_set_next_event, };
-static void xen_vcpuop_set_mode(enum clock_event_mode mode, +static int xen_vcpuop_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { int cpu = smp_processor_id();
switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - WARN_ON(1); /* unsupported */ - break; - case CLOCK_EVT_MODE_ONESHOT: if (HYPERVISOR_vcpu_op(VCPUOP_stop_periodic_timer, cpu, NULL)) BUG(); @@ -349,7 +343,10 @@ static void xen_vcpuop_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int xen_vcpuop_set_next_event(unsigned long delta, @@ -382,7 +379,7 @@ static const struct clock_event_device xen_vcpuop_clockevent = { .shift = 0, .rating = 500,
- .set_mode = xen_vcpuop_set_mode, + .set_dev_mode = xen_vcpuop_set_mode, .set_next_event = xen_vcpuop_set_next_event, };
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all x86 specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index aae9413..54aa944 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -962,7 +962,7 @@ static int lguest_clockevent_set_next_event(unsigned long delta, return 0; }
-static void lguest_clockevent_set_mode(enum clock_event_mode mode, +static int lguest_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -974,11 +974,12 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: /* This is what we expect. */ break;
- case CLOCK_EVT_MODE_PERIODIC:
BUG();
Will not demoting a BUG_ON() to a WARN_ON() be a problem?
On 29 May 2014 15:41, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all x86 specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index aae9413..54aa944 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -962,7 +962,7 @@ static int lguest_clockevent_set_next_event(unsigned long delta, return 0; }
-static void lguest_clockevent_set_mode(enum clock_event_mode mode, +static int lguest_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -974,11 +974,12 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: /* This is what we expect. */ break;
case CLOCK_EVT_MODE_PERIODIC:BUG();Will not demoting a BUG_ON() to a WARN_ON() be a problem?
Don't know. The important issue is "what should be done when we get called for a unsupported mode"? And probably a WARN() is enough. What drivers are doing already isn't something "written on stone" and isn't perfect :)
On 05/30/2014 07:08 AM, Viresh Kumar wrote:
On 29 May 2014 15:41, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all x86 specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index aae9413..54aa944 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -962,7 +962,7 @@ static int lguest_clockevent_set_next_event(unsigned long delta, return 0; }
-static void lguest_clockevent_set_mode(enum clock_event_mode mode, +static int lguest_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -974,11 +974,12 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: /* This is what we expect. */ break;
case CLOCK_EVT_MODE_PERIODIC:BUG();Will not demoting a BUG_ON() to a WARN_ON() be a problem?
Don't know. The important issue is "what should be done when we get called for a unsupported mode"? And probably a WARN() is enough. What drivers are doing already isn't something "written on stone" and isn't perfect :)
Hmm.. I would suggest leaving the BUG() as is for this mode to be safe.But lets see what tglx has to say.
Regards Preeti U Murthy
On 05/30/2014 07:08 AM, Viresh Kumar wrote:
On 29 May 2014 15:41, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all x86 specific clockevent drivers to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index aae9413..54aa944 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -962,7 +962,7 @@ static int lguest_clockevent_set_next_event(unsigned long delta, return 0; }
-static void lguest_clockevent_set_mode(enum clock_event_mode mode, +static int lguest_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -974,11 +974,12 @@ static void lguest_clockevent_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: /* This is what we expect. */ break;
case CLOCK_EVT_MODE_PERIODIC:BUG();Will not demoting a BUG_ON() to a WARN_ON() be a problem?
Don't know. The important issue is "what should be done when we get called for a unsupported mode"? And probably a WARN() is enough. What drivers are doing already isn't something "written on stone" and isn't perfect :)
Actually I think this is fine. This patch is creating a uniform way of handling unsupported modes through a WARN_ON(). Its best to not have these BUG_ON()s and printks sprinkled all over for specific modes like the above. You can add my reviewed-by.
Thanks
Regards Preeti U Murthy
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all drivers in 'drivers/clocksource' to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/arm_arch_timer.c | 36 ++++++++++++++++++-------------- drivers/clocksource/arm_global_timer.c | 9 +++++--- drivers/clocksource/bcm2835_timer.c | 8 +++---- drivers/clocksource/bcm_kona_timer.c | 10 ++++++--- drivers/clocksource/cadence_ttc_timer.c | 7 +++++-- drivers/clocksource/cs5535-clockevt.c | 17 ++++++++++++--- drivers/clocksource/dummy_timer.c | 15 +++++++++++-- drivers/clocksource/dw_apb_timer.c | 7 +++++-- drivers/clocksource/em_sti.c | 11 ++++++---- drivers/clocksource/exynos_mct.c | 16 +++++++++----- drivers/clocksource/i8253.c | 8 +++++-- drivers/clocksource/metag_generic.c | 11 +++++----- drivers/clocksource/moxart_timer.c | 8 ++++--- drivers/clocksource/mxs_timer.c | 10 ++++----- drivers/clocksource/nomadik-mtu.c | 7 +++++-- drivers/clocksource/qcom-timer.c | 11 +++++----- drivers/clocksource/samsung_pwm_timer.c | 7 +++++-- drivers/clocksource/sh_cmt.c | 9 +++++--- drivers/clocksource/sh_mtu2.c | 9 +++++--- drivers/clocksource/sh_tmu.c | 9 +++++--- drivers/clocksource/sun4i_timer.c | 9 +++++--- drivers/clocksource/tcb_clksrc.c | 11 +++++++--- drivers/clocksource/tegra20_timer.c | 7 +++++-- drivers/clocksource/time-armada-370-xp.c | 20 ++++++++++++------ drivers/clocksource/time-efm32.c | 7 +++++-- drivers/clocksource/time-orion.c | 17 +++++++++++---- drivers/clocksource/timer-keystone.c | 9 +++++--- drivers/clocksource/timer-marco.c | 10 ++++++--- drivers/clocksource/timer-prima2.c | 10 ++++----- drivers/clocksource/timer-sun5i.c | 10 ++++++--- drivers/clocksource/timer-u300.c | 7 +++++-- drivers/clocksource/vf_pit_timer.c | 12 ++++++++--- drivers/clocksource/vt8500_timer.c | 8 ++++--- drivers/clocksource/zevio-timer.c | 9 ++++---- 34 files changed, 246 insertions(+), 125 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 5163ec1..44f1eaa 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -179,7 +179,7 @@ static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id) return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt); }
-static __always_inline void timer_set_mode(const int access, int mode, +static __always_inline int timer_set_mode(const int access, int mode, struct clock_event_device *clk) { unsigned long ctrl; @@ -190,33 +190,37 @@ static __always_inline void timer_set_mode(const int access, int mode, ctrl &= ~ARCH_TIMER_CTRL_ENABLE; arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); break; - default: + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
-static void arch_timer_set_mode_virt(enum clock_event_mode mode, +static int arch_timer_set_mode_virt(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk); + return timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk); }
-static void arch_timer_set_mode_phys(enum clock_event_mode mode, +static int arch_timer_set_mode_phys(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk); + return timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk); }
-static void arch_timer_set_mode_virt_mem(enum clock_event_mode mode, +static int arch_timer_set_mode_virt_mem(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk); + return timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk); }
-static void arch_timer_set_mode_phys_mem(enum clock_event_mode mode, +static int arch_timer_set_mode_phys_mem(enum clock_event_mode mode, struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk); + return timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk); }
static __always_inline void set_next_event(const int access, unsigned long evt, @@ -271,11 +275,11 @@ static void __arch_timer_setup(unsigned type, clk->cpumask = cpumask_of(smp_processor_id()); if (arch_timer_use_virtual) { clk->irq = arch_timer_ppi[VIRT_PPI]; - clk->set_mode = arch_timer_set_mode_virt; + clk->set_dev_mode = arch_timer_set_mode_virt; clk->set_next_event = arch_timer_set_next_event_virt; } else { clk->irq = arch_timer_ppi[PHYS_SECURE_PPI]; - clk->set_mode = arch_timer_set_mode_phys; + clk->set_dev_mode = arch_timer_set_mode_phys; clk->set_next_event = arch_timer_set_next_event_phys; } } else { @@ -284,17 +288,17 @@ static void __arch_timer_setup(unsigned type, clk->rating = 400; clk->cpumask = cpu_all_mask; if (arch_timer_mem_use_virtual) { - clk->set_mode = arch_timer_set_mode_virt_mem; + clk->set_dev_mode = arch_timer_set_mode_virt_mem; clk->set_next_event = arch_timer_set_next_event_virt_mem; } else { - clk->set_mode = arch_timer_set_mode_phys_mem; + clk->set_dev_mode = arch_timer_set_mode_phys_mem; clk->set_next_event = arch_timer_set_next_event_phys_mem; } }
- clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk); + clk->set_dev_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); } @@ -457,7 +461,7 @@ static void arch_timer_stop(struct clock_event_device *clk) disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]); }
- clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk); + clk->set_dev_mode(CLOCK_EVT_MODE_UNUSED, clk); }
static int arch_timer_cpu_notify(struct notifier_block *self, diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index 0fc31d0..937e039 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -107,7 +107,7 @@ static void gt_compare_set(unsigned long delta, int periodic) writel(ctrl, gt_base + GT_CONTROL); }
-static void gt_clockevent_set_mode(enum clock_event_mode mode, +static int gt_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned long ctrl; @@ -124,9 +124,12 @@ static void gt_clockevent_set_mode(enum clock_event_mode mode, GT_CONTROL_IRQ_ENABLE | GT_CONTROL_AUTO_INC); writel(ctrl, gt_base + GT_CONTROL); break; - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int gt_clockevent_set_next_event(unsigned long evt, @@ -171,7 +174,7 @@ static int gt_clockevents_init(struct clock_event_device *clk) clk->name = "arm_global_timer"; clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERCPU; - clk->set_mode = gt_clockevent_set_mode; + clk->set_dev_mode = gt_clockevent_set_mode; clk->set_next_event = gt_clockevent_set_next_event; clk->cpumask = cpumask_of(cpu); clk->rating = 300; diff --git a/drivers/clocksource/bcm2835_timer.c b/drivers/clocksource/bcm2835_timer.c index 26ed331..455b5dd 100644 --- a/drivers/clocksource/bcm2835_timer.c +++ b/drivers/clocksource/bcm2835_timer.c @@ -54,7 +54,7 @@ static u64 notrace bcm2835_sched_read(void) return readl_relaxed(system_clock); }
-static void bcm2835_time_set_mode(enum clock_event_mode mode, +static int bcm2835_time_set_mode(enum clock_event_mode mode, struct clock_event_device *evt_dev) { switch (mode) { @@ -64,9 +64,9 @@ static void bcm2835_time_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: break; default: - WARN(1, "%s: unhandled event mode %d\n", __func__, mode); - break; + return -ENOSYS; } + return 0; }
static int bcm2835_time_set_next_event(unsigned long event, @@ -129,7 +129,7 @@ static void __init bcm2835_timer_init(struct device_node *node) timer->evt.name = node->name; timer->evt.rating = 300; timer->evt.features = CLOCK_EVT_FEAT_ONESHOT; - timer->evt.set_mode = bcm2835_time_set_mode; + timer->evt.set_dev_mode = bcm2835_time_set_mode; timer->evt.set_next_event = bcm2835_time_set_next_event; timer->evt.cpumask = cpumask_of(0); timer->act.name = node->name; diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index 0595dc6..a3899f4 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -128,7 +128,7 @@ static int kona_timer_set_next_event(unsigned long clc, return 0; }
-static void kona_timer_set_mode(enum clock_event_mode mode, +static int kona_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *unused) { switch (mode) { @@ -137,16 +137,20 @@ static void kona_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: kona_timer_disable_and_clear(timers.tmr_regs); + break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device kona_clockevent_timer = { .name = "timer 1", .features = CLOCK_EVT_FEAT_ONESHOT, .set_next_event = kona_timer_set_next_event, - .set_mode = kona_timer_set_mode + .set_dev_mode = kona_timer_set_mode };
static void __init kona_timer_clockevents_init(void) diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c index 49fbe28..3d355ec 100644 --- a/drivers/clocksource/cadence_ttc_timer.c +++ b/drivers/clocksource/cadence_ttc_timer.c @@ -196,7 +196,7 @@ static int ttc_set_next_event(unsigned long cycles, * @mode: Mode to be set * @evt: Address of clock event instance **/ -static void ttc_set_mode(enum clock_event_mode mode, +static int ttc_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct ttc_timer_clockevent *ttce = to_ttc_timer_clkevent(evt); @@ -224,7 +224,10 @@ static void ttc_set_mode(enum clock_event_mode mode, __raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET); break; + default: + return -ENOSYS; } + return 0; }
static int ttc_rate_change_clocksource_cb(struct notifier_block *nb, @@ -428,7 +431,7 @@ static void __init ttc_setup_clockevent(struct clk *clk, ttcce->ce.name = "ttc_clockevent"; ttcce->ce.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; ttcce->ce.set_next_event = ttc_set_next_event; - ttcce->ce.set_mode = ttc_set_mode; + ttcce->ce.set_dev_mode = ttc_set_mode; ttcce->ce.rating = 200; ttcce->ce.irq = irq; ttcce->ce.cpumask = cpu_possible_mask; diff --git a/drivers/clocksource/cs5535-clockevt.c b/drivers/clocksource/cs5535-clockevt.c index db21052..c1b1f7f 100644 --- a/drivers/clocksource/cs5535-clockevt.c +++ b/drivers/clocksource/cs5535-clockevt.c @@ -77,15 +77,26 @@ static void start_timer(struct cs5535_mfgpt_timer *timer, uint16_t delta) MFGPT_SETUP_CNTEN | MFGPT_SETUP_CMP2); }
-static void mfgpt_set_mode(enum clock_event_mode mode, +static int mfgpt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { disable_timer(cs5535_event_clock);
- if (mode == CLOCK_EVT_MODE_PERIODIC) + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: start_timer(cs5535_event_clock, MFGPT_PERIODIC); + break; + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + break; + default: + return -ENOSYS; + }
cs5535_tick_mode = mode; + return 0; }
static int mfgpt_next_event(unsigned long delta, struct clock_event_device *evt) @@ -97,7 +108,7 @@ static int mfgpt_next_event(unsigned long delta, struct clock_event_device *evt) static struct clock_event_device cs5535_clockevent = { .name = DRV_NAME, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = mfgpt_set_mode, + .set_dev_mode = mfgpt_set_mode, .set_next_event = mfgpt_next_event, .rating = 250, }; diff --git a/drivers/clocksource/dummy_timer.c b/drivers/clocksource/dummy_timer.c index ad35725..2d0cc09 100644 --- a/drivers/clocksource/dummy_timer.c +++ b/drivers/clocksource/dummy_timer.c @@ -16,13 +16,24 @@
static DEFINE_PER_CPU(struct clock_event_device, dummy_timer_evt);
-static void dummy_timer_set_mode(enum clock_event_mode mode, +static int dummy_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* * Core clockevents code will call this when exchanging timer devices. * We don't need to do anything here. */ + break; + default: + return -ENOSYS; + } + return 0; }
static void dummy_timer_setup(void) @@ -35,7 +46,7 @@ static void dummy_timer_setup(void) CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DUMMY; evt->rating = 100; - evt->set_mode = dummy_timer_set_mode; + evt->set_dev_mode = dummy_timer_set_mode; evt->cpumask = cpumask_of(cpu);
clockevents_register_device(evt); diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c index f3656a6b..15ac39c 100644 --- a/drivers/clocksource/dw_apb_timer.c +++ b/drivers/clocksource/dw_apb_timer.c @@ -110,7 +110,7 @@ static void apbt_enable_int(struct dw_apb_timer *timer) apbt_writel(timer, ctrl, APBTMR_N_CONTROL); }
-static void apbt_set_mode(enum clock_event_mode mode, +static int apbt_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long ctrl; @@ -173,7 +173,10 @@ static void apbt_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: apbt_enable_int(&dw_ced->timer); break; + default: + return -ENOSYS; } + return 0; }
static int apbt_next_event(unsigned long delta, @@ -232,7 +235,7 @@ dw_apb_clockevent_init(int cpu, const char *name, unsigned rating, dw_ced->ced.min_delta_ns = clockevent_delta2ns(5000, &dw_ced->ced); dw_ced->ced.cpumask = cpumask_of(cpu); dw_ced->ced.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; - dw_ced->ced.set_mode = apbt_set_mode; + dw_ced->ced.set_dev_mode = apbt_set_mode; dw_ced->ced.set_next_event = apbt_next_event; dw_ced->ced.irq = dw_ced->timer.irq; dw_ced->ced.rating = rating; diff --git a/drivers/clocksource/em_sti.c b/drivers/clocksource/em_sti.c index 9d17083..34b9a01 100644 --- a/drivers/clocksource/em_sti.c +++ b/drivers/clocksource/em_sti.c @@ -251,7 +251,7 @@ static struct em_sti_priv *ced_to_em_sti(struct clock_event_device *ced) return container_of(ced, struct em_sti_priv, ced); }
-static void em_sti_clock_event_mode(enum clock_event_mode mode, +static int em_sti_clock_event_mode(enum clock_event_mode mode, struct clock_event_device *ced) { struct em_sti_priv *p = ced_to_em_sti(ced); @@ -275,9 +275,12 @@ static void em_sti_clock_event_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: em_sti_stop(p, USER_CLOCKEVENT); break; - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int em_sti_clock_event_next(unsigned long delta, @@ -303,11 +306,11 @@ static void em_sti_register_clockevent(struct em_sti_priv *p) ced->rating = 200; ced->cpumask = cpu_possible_mask; ced->set_next_event = em_sti_clock_event_next; - ced->set_mode = em_sti_clock_event_mode; + ced->set_dev_mode = em_sti_clock_event_mode;
dev_info(&p->pdev->dev, "used for clock events\n");
- /* Register with dummy 1 Hz value, gets updated in ->set_mode() */ + /* Register with dummy 1 Hz value, gets updated in ->set_dev_mode() */ clockevents_config_and_register(ced, 1, 2, 0xffffffff); }
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index acf5a32..f4c70d3 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -242,7 +242,7 @@ static int exynos4_comp_set_next_event(unsigned long cycles, return 0; }
-static void exynos4_comp_set_mode(enum clock_event_mode mode, +static int exynos4_comp_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { unsigned long cycles_per_jiffy; @@ -260,7 +260,10 @@ static void exynos4_comp_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device mct_comp_device = { @@ -268,7 +271,7 @@ static struct clock_event_device mct_comp_device = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 250, .set_next_event = exynos4_comp_set_next_event, - .set_mode = exynos4_comp_set_mode, + .set_dev_mode = exynos4_comp_set_mode, };
static irqreturn_t exynos4_mct_comp_isr(int irq, void *dev_id) @@ -344,7 +347,7 @@ static int exynos4_tick_set_next_event(unsigned long cycles, return 0; }
-static inline void exynos4_tick_set_mode(enum clock_event_mode mode, +static inline int exynos4_tick_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct mct_clock_event_device *mevt = this_cpu_ptr(&percpu_mct_tick); @@ -364,7 +367,10 @@ static inline void exynos4_tick_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int exynos4_mct_tick_clear(struct mct_clock_event_device *mevt) @@ -413,7 +419,7 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt) evt->name = mevt->name; evt->cpumask = cpumask_of(cpu); evt->set_next_event = exynos4_tick_set_next_event; - evt->set_mode = exynos4_tick_set_mode; + evt->set_dev_mode = exynos4_tick_set_mode; evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; evt->rating = 450;
@@ -440,7 +446,7 @@ static int exynos4_local_timer_setup(struct clock_event_device *evt)
static void exynos4_local_timer_stop(struct clock_event_device *evt) { - evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); + evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt); if (mct_int_type == MCT_INT_SPI) free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick)); else diff --git a/drivers/clocksource/i8253.c b/drivers/clocksource/i8253.c index 14ee3ef..936da9c 100644 --- a/drivers/clocksource/i8253.c +++ b/drivers/clocksource/i8253.c @@ -105,7 +105,7 @@ int __init clocksource_i8253_init(void) * * This is also called after resume to bring the PIT into operation again. */ -static void init_pit_timer(enum clock_event_mode mode, +static int init_pit_timer(enum clock_event_mode mode, struct clock_event_device *evt) { raw_spin_lock(&i8253_lock); @@ -136,8 +136,12 @@ static void init_pit_timer(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + raw_spin_unlock(&i8253_lock); + return -ENOSYS; } raw_spin_unlock(&i8253_lock); + return 0; }
/* @@ -162,7 +166,7 @@ static int pit_next_event(unsigned long delta, struct clock_event_device *evt) struct clock_event_device i8253_clockevent = { .name = "pit", .features = CLOCK_EVT_FEAT_PERIODIC, - .set_mode = init_pit_timer, + .set_dev_mode = init_pit_timer, .set_next_event = pit_next_event, };
diff --git a/drivers/clocksource/metag_generic.c b/drivers/clocksource/metag_generic.c index 9e4db41..501fff4 100644 --- a/drivers/clocksource/metag_generic.c +++ b/drivers/clocksource/metag_generic.c @@ -56,7 +56,7 @@ static int metag_timer_set_next_event(unsigned long delta, return 0; }
-static void metag_timer_set_mode(enum clock_event_mode mode, +static int metag_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -68,11 +68,10 @@ static void metag_timer_set_mode(enum clock_event_mode mode, /* We should disable the IRQ here */ break;
- case CLOCK_EVT_MODE_PERIODIC: - case CLOCK_EVT_MODE_UNUSED: - WARN_ON(1); - break; + default: + return -ENOSYS; }; + return 0; }
static cycle_t metag_clocksource_read(struct clocksource *cs) @@ -129,7 +128,7 @@ static void arch_timer_setup(unsigned int cpu) clk->rating = 200, clk->shift = 12, clk->irq = tbisig_map(TBID_SIGNUM_TRT), - clk->set_mode = metag_timer_set_mode, + clk->set_dev_mode = metag_timer_set_mode, clk->set_next_event = metag_timer_set_next_event,
clk->mult = div_sc(hwtimer_freq, NSEC_PER_SEC, clk->shift); diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c index 5eb2c35..edae259 100644 --- a/drivers/clocksource/moxart_timer.c +++ b/drivers/clocksource/moxart_timer.c @@ -58,7 +58,7 @@ static void __iomem *base; static unsigned int clock_count_per_tick;
-static void moxart_clkevt_mode(enum clock_event_mode mode, +static int moxart_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *clk) { switch (mode) { @@ -73,10 +73,12 @@ static void moxart_clkevt_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: writel(TIMER1_DISABLE, base + TIMER_CR); break; + default: + return -ENOSYS; } + return 0; }
static int moxart_clkevt_next_event(unsigned long cycles, @@ -98,7 +100,7 @@ static struct clock_event_device moxart_clockevent = { .name = "moxart_timer", .rating = 200, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = moxart_clkevt_mode, + .set_dev_mode = moxart_clkevt_mode, .set_next_event = moxart_clkevt_next_event, };
diff --git a/drivers/clocksource/mxs_timer.c b/drivers/clocksource/mxs_timer.c index 445b68a..6de37bf 100644 --- a/drivers/clocksource/mxs_timer.c +++ b/drivers/clocksource/mxs_timer.c @@ -150,7 +150,7 @@ static const char *clock_event_mode_label[] const = { }; #endif /* DEBUG */
-static void mxs_set_mode(enum clock_event_mode mode, +static int mxs_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { /* Disable interrupt in timer module */ @@ -179,9 +179,6 @@ static void mxs_set_mode(enum clock_event_mode mode, mxs_clockevent_mode = mode;
switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - pr_err("%s: Periodic mode is not implemented\n", __func__); - break; case CLOCK_EVT_MODE_ONESHOT: timrot_irq_enable(); break; @@ -190,13 +187,16 @@ static void mxs_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Left event sources disabled, no more interrupts appear */ break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device mxs_clockevent_device = { .name = "mxs_timrot", .features = CLOCK_EVT_FEAT_ONESHOT, - .set_mode = mxs_set_mode, + .set_dev_mode = mxs_set_mode, .set_next_event = timrotv2_set_next_event, .rating = 200, }; diff --git a/drivers/clocksource/nomadik-mtu.c b/drivers/clocksource/nomadik-mtu.c index a709cfa..8c3d355 100644 --- a/drivers/clocksource/nomadik-mtu.c +++ b/drivers/clocksource/nomadik-mtu.c @@ -119,7 +119,7 @@ static void nmdk_clkevt_reset(void) } }
-static void nmdk_clkevt_mode(enum clock_event_mode mode, +static int nmdk_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -140,7 +140,10 @@ static void nmdk_clkevt_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void nmdk_clksrc_reset(void) @@ -167,7 +170,7 @@ static struct clock_event_device nmdk_clkevt = { .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_DYNIRQ, .rating = 200, - .set_mode = nmdk_clkevt_mode, + .set_dev_mode = nmdk_clkevt_mode, .set_next_event = nmdk_clkevt_next, .resume = nmdk_clkevt_resume, }; diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c index e807acf..f72ef19 100644 --- a/drivers/clocksource/qcom-timer.c +++ b/drivers/clocksource/qcom-timer.c @@ -75,7 +75,7 @@ static int msm_timer_set_next_event(unsigned long cycles, return 0; }
-static void msm_timer_set_mode(enum clock_event_mode mode, +static int msm_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -85,16 +85,17 @@ static void msm_timer_set_mode(enum clock_event_mode mode,
switch (mode) { case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: - break; case CLOCK_EVT_MODE_ONESHOT: /* Timer is enabled in set_next_event */ break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break; + default: + return -ENOSYS; } writel_relaxed(ctrl, event_base + TIMER_ENABLE); + return 0; }
static struct clock_event_device __percpu *msm_evt; @@ -126,7 +127,7 @@ static int msm_local_timer_setup(struct clock_event_device *evt) evt->name = "msm_timer"; evt->features = CLOCK_EVT_FEAT_ONESHOT; evt->rating = 200; - evt->set_mode = msm_timer_set_mode; + evt->set_dev_mode = msm_timer_set_mode; evt->set_next_event = msm_timer_set_next_event; evt->cpumask = cpumask_of(cpu);
@@ -147,7 +148,7 @@ static int msm_local_timer_setup(struct clock_event_device *evt)
static void msm_local_timer_stop(struct clock_event_device *evt) { - evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); + evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt); disable_percpu_irq(evt->irq); }
diff --git a/drivers/clocksource/samsung_pwm_timer.c b/drivers/clocksource/samsung_pwm_timer.c index 5645cfc..4ce11bf 100644 --- a/drivers/clocksource/samsung_pwm_timer.c +++ b/drivers/clocksource/samsung_pwm_timer.c @@ -207,7 +207,7 @@ static int samsung_set_next_event(unsigned long cycles, return 0; }
-static void samsung_set_mode(enum clock_event_mode mode, +static int samsung_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { samsung_time_stop(pwm.event_id); @@ -225,7 +225,10 @@ static void samsung_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void samsung_clockevent_resume(struct clock_event_device *cev) @@ -244,7 +247,7 @@ static struct clock_event_device time_event_device = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = samsung_set_next_event, - .set_mode = samsung_set_mode, + .set_dev_mode = samsung_set_mode, .resume = samsung_clockevent_resume, };
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c index 0b1836a..ed4e5d0 100644 --- a/drivers/clocksource/sh_cmt.c +++ b/drivers/clocksource/sh_cmt.c @@ -585,7 +585,7 @@ static void sh_cmt_clock_event_start(struct sh_cmt_priv *p, int periodic) sh_cmt_set_next(p, p->max_match_value); }
-static void sh_cmt_clock_event_mode(enum clock_event_mode mode, +static int sh_cmt_clock_event_mode(enum clock_event_mode mode, struct clock_event_device *ced) { struct sh_cmt_priv *p = ced_to_sh_cmt(ced); @@ -613,9 +613,12 @@ static void sh_cmt_clock_event_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: sh_cmt_stop(p, FLAG_CLOCKEVENT); break; - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int sh_cmt_clock_event_next(unsigned long delta, @@ -661,7 +664,7 @@ static void sh_cmt_register_clockevent(struct sh_cmt_priv *p, ced->rating = rating; ced->cpumask = cpumask_of(0); ced->set_next_event = sh_cmt_clock_event_next; - ced->set_mode = sh_cmt_clock_event_mode; + ced->set_dev_mode = sh_cmt_clock_event_mode; ced->suspend = sh_cmt_clock_event_suspend; ced->resume = sh_cmt_clock_event_resume;
diff --git a/drivers/clocksource/sh_mtu2.c b/drivers/clocksource/sh_mtu2.c index e30d76e..3d51350 100644 --- a/drivers/clocksource/sh_mtu2.c +++ b/drivers/clocksource/sh_mtu2.c @@ -184,7 +184,7 @@ static struct sh_mtu2_priv *ced_to_sh_mtu2(struct clock_event_device *ced) return container_of(ced, struct sh_mtu2_priv, ced); }
-static void sh_mtu2_clock_event_mode(enum clock_event_mode mode, +static int sh_mtu2_clock_event_mode(enum clock_event_mode mode, struct clock_event_device *ced) { struct sh_mtu2_priv *p = ced_to_sh_mtu2(ced); @@ -210,9 +210,12 @@ static void sh_mtu2_clock_event_mode(enum clock_event_mode mode, sh_mtu2_disable(p); break; case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void sh_mtu2_clock_event_suspend(struct clock_event_device *ced) @@ -237,7 +240,7 @@ static void sh_mtu2_register_clockevent(struct sh_mtu2_priv *p, ced->features = CLOCK_EVT_FEAT_PERIODIC; ced->rating = rating; ced->cpumask = cpumask_of(0); - ced->set_mode = sh_mtu2_clock_event_mode; + ced->set_dev_mode = sh_mtu2_clock_event_mode; ced->suspend = sh_mtu2_clock_event_suspend; ced->resume = sh_mtu2_clock_event_resume;
diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c index ecd7b60..e8f9573 100644 --- a/drivers/clocksource/sh_tmu.c +++ b/drivers/clocksource/sh_tmu.c @@ -321,7 +321,7 @@ static void sh_tmu_clock_event_start(struct sh_tmu_priv *p, int periodic) } }
-static void sh_tmu_clock_event_mode(enum clock_event_mode mode, +static int sh_tmu_clock_event_mode(enum clock_event_mode mode, struct clock_event_device *ced) { struct sh_tmu_priv *p = ced_to_sh_tmu(ced); @@ -352,9 +352,12 @@ static void sh_tmu_clock_event_mode(enum clock_event_mode mode, sh_tmu_disable(p); break; case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int sh_tmu_clock_event_next(unsigned long delta, @@ -393,7 +396,7 @@ static void sh_tmu_register_clockevent(struct sh_tmu_priv *p, ced->rating = rating; ced->cpumask = cpumask_of(0); ced->set_next_event = sh_tmu_clock_event_next; - ced->set_mode = sh_tmu_clock_event_mode; + ced->set_dev_mode = sh_tmu_clock_event_mode; ced->suspend = sh_tmu_clock_event_suspend; ced->resume = sh_tmu_clock_event_resume;
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c index efb17c3..b755fcb 100644 --- a/drivers/clocksource/sun4i_timer.c +++ b/drivers/clocksource/sun4i_timer.c @@ -81,7 +81,7 @@ static void sun4i_clkevt_time_start(u8 timer, bool periodic) timer_base + TIMER_CTL_REG(timer)); }
-static void sun4i_clkevt_mode(enum clock_event_mode mode, +static int sun4i_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *clk) { switch (mode) { @@ -96,10 +96,13 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: + case CLOCK_EVT_MODE_RESUME: sun4i_clkevt_time_stop(0); break; + default: + return -ENOSYS; } + return 0; }
static int sun4i_clkevt_next_event(unsigned long evt, @@ -116,7 +119,7 @@ static struct clock_event_device sun4i_clockevent = { .name = "sun4i_tick", .rating = 350, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = sun4i_clkevt_mode, + .set_dev_mode = sun4i_clkevt_mode, .set_next_event = sun4i_clkevt_next_event, };
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c index 00fdd11..b6b23f1 100644 --- a/drivers/clocksource/tcb_clksrc.c +++ b/drivers/clocksource/tcb_clksrc.c @@ -91,7 +91,7 @@ static struct tc_clkevt_device *to_tc_clkevt(struct clock_event_device *clkevt) */ static u32 timer_clock;
-static void tc_mode(enum clock_event_mode m, struct clock_event_device *d) +static int tc_mode(enum clock_event_mode m, struct clock_event_device *d) { struct tc_clkevt_device *tcd = to_tc_clkevt(d); void __iomem *regs = tcd->regs; @@ -137,9 +137,14 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d) /* set_next_event() configures and starts the timer */ break;
- default: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int tc_next_event(unsigned long delta, struct clock_event_device *d) @@ -160,7 +165,7 @@ static struct tc_clkevt_device clkevt = { /* Should be lower than at91rm9200's system timer */ .rating = 125, .set_next_event = tc_next_event, - .set_mode = tc_mode, + .set_dev_mode = tc_mode, }, };
diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c index d1869f0..aa0d3e9 100644 --- a/drivers/clocksource/tegra20_timer.c +++ b/drivers/clocksource/tegra20_timer.c @@ -69,7 +69,7 @@ static int tegra_timer_set_next_event(unsigned long cycles, return 0; }
-static void tegra_timer_set_mode(enum clock_event_mode mode, +static int tegra_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 reg; @@ -87,7 +87,10 @@ static void tegra_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device tegra_clockevent = { @@ -95,7 +98,7 @@ static struct clock_event_device tegra_clockevent = { .rating = 300, .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC, .set_next_event = tegra_timer_set_next_event, - .set_mode = tegra_timer_set_mode, + .set_dev_mode = tegra_timer_set_mode, };
static u64 notrace tegra_read_sched_clock(void) diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c index 0451e62..8aa35c1 100644 --- a/drivers/clocksource/time-armada-370-xp.c +++ b/drivers/clocksource/time-armada-370-xp.c @@ -120,12 +120,12 @@ armada_370_xp_clkevt_next_event(unsigned long delta, return 0; }
-static void +static int armada_370_xp_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { - if (mode == CLOCK_EVT_MODE_PERIODIC) { - + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: /* * Setup timer to fire at 1/HZ intervals. */ @@ -136,7 +136,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * Enable timer. */ local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask); - } else { + break; + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* * Disable timer. */ @@ -146,7 +150,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * ACK pending timer interrupt. */ writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS); + break; + default: + return -ENOSYS; } + return 0; }
static int armada_370_xp_clkevt_irq; @@ -184,7 +192,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt) evt->shift = 32, evt->rating = 300, evt->set_next_event = armada_370_xp_clkevt_next_event, - evt->set_mode = armada_370_xp_clkevt_mode, + evt->set_dev_mode = armada_370_xp_clkevt_mode, evt->irq = armada_370_xp_clkevt_irq; evt->cpumask = cpumask_of(cpu);
@@ -196,7 +204,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt)
static void armada_370_xp_timer_stop(struct clock_event_device *evt) { - evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); + evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt); disable_percpu_irq(evt->irq); }
diff --git a/drivers/clocksource/time-efm32.c b/drivers/clocksource/time-efm32.c index 1a6205b..6e5af98 100644 --- a/drivers/clocksource/time-efm32.c +++ b/drivers/clocksource/time-efm32.c @@ -48,7 +48,7 @@ struct efm32_clock_event_ddata { unsigned periodic_top; };
-static void efm32_clock_event_set_mode(enum clock_event_mode mode, +static int efm32_clock_event_set_mode(enum clock_event_mode mode, struct clock_event_device *evtdev) { struct efm32_clock_event_ddata *ddata = @@ -81,7 +81,10 @@ static void efm32_clock_event_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int efm32_clock_event_set_next_event(unsigned long evt, @@ -112,7 +115,7 @@ static struct efm32_clock_event_ddata clock_event_ddata = { .evtdev = { .name = "efm32 clockevent", .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_MODE_PERIODIC, - .set_mode = efm32_clock_event_set_mode, + .set_dev_mode = efm32_clock_event_set_mode, .set_next_event = efm32_clock_event_set_next_event, .rating = 200, }, diff --git a/drivers/clocksource/time-orion.c b/drivers/clocksource/time-orion.c index 0b3ce03..3a26ce9 100644 --- a/drivers/clocksource/time-orion.c +++ b/drivers/clocksource/time-orion.c @@ -60,21 +60,30 @@ static int orion_clkevt_next_event(unsigned long delta, return 0; }
-static void orion_clkevt_mode(enum clock_event_mode mode, +static int orion_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) { - if (mode == CLOCK_EVT_MODE_PERIODIC) { + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: /* setup and enable periodic timer at 1/HZ intervals */ writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD); writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL); atomic_io_modify(timer_base + TIMER_CTRL, TIMER1_RELOAD_EN | TIMER1_EN, TIMER1_RELOAD_EN | TIMER1_EN); - } else { + break; + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* disable timer */ atomic_io_modify(timer_base + TIMER_CTRL, TIMER1_RELOAD_EN | TIMER1_EN, 0); + break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device orion_clkevt = { @@ -83,7 +92,7 @@ static struct clock_event_device orion_clkevt = { .shift = 32, .rating = 300, .set_next_event = orion_clkevt_next_event, - .set_mode = orion_clkevt_mode, + .set_dev_mode = orion_clkevt_mode, };
static irqreturn_t orion_clkevt_irq_handler(int irq, void *dev_id) diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c index 0250354..722c7e3 100644 --- a/drivers/clocksource/timer-keystone.c +++ b/drivers/clocksource/timer-keystone.c @@ -141,7 +141,7 @@ static int keystone_set_next_event(unsigned long cycles, return keystone_timer_config(cycles, evt->mode); }
-static void keystone_set_mode(enum clock_event_mode mode, +static int keystone_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -153,9 +153,12 @@ static void keystone_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_ONESHOT: keystone_timer_disable(); break; - default: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void __init keystone_timer_init(struct device_node *np) @@ -222,7 +225,7 @@ static void __init keystone_timer_init(struct device_node *np) /* setup clockevent */ event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; event_dev->set_next_event = keystone_set_next_event; - event_dev->set_mode = keystone_set_mode; + event_dev->set_dev_mode = keystone_set_mode; event_dev->cpumask = cpu_all_mask; event_dev->owner = THIS_MODULE; event_dev->name = TIMER_NAME; diff --git a/drivers/clocksource/timer-marco.c b/drivers/clocksource/timer-marco.c index b52e1c0..4f202eb 100644 --- a/drivers/clocksource/timer-marco.c +++ b/drivers/clocksource/timer-marco.c @@ -114,18 +114,22 @@ static int sirfsoc_timer_set_next_event(unsigned long delta, return 0; }
-static void sirfsoc_timer_set_mode(enum clock_event_mode mode, +static int sirfsoc_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *ce) { switch (mode) { case CLOCK_EVT_MODE_ONESHOT: /* enable in set_next_event */ + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: break; default: - break; + return -ENOSYS; }
sirfsoc_timer_count_disable(smp_processor_id()); + return 0; }
static void sirfsoc_clocksource_suspend(struct clocksource *cs) @@ -190,7 +194,7 @@ static int sirfsoc_local_timer_setup(struct clock_event_device *ce) ce->name = "local_timer"; ce->features = CLOCK_EVT_FEAT_ONESHOT; ce->rating = 200; - ce->set_mode = sirfsoc_timer_set_mode; + ce->set_dev_mode = sirfsoc_timer_set_mode; ce->set_next_event = sirfsoc_timer_set_next_event; clockevents_calc_mult_shift(ce, MARCO_CLOCK_FREQ, 60); ce->max_delta_ns = clockevent_delta2ns(-2, ce); diff --git a/drivers/clocksource/timer-prima2.c b/drivers/clocksource/timer-prima2.c index 1a6b2d6..0b49c10 100644 --- a/drivers/clocksource/timer-prima2.c +++ b/drivers/clocksource/timer-prima2.c @@ -99,14 +99,11 @@ static int sirfsoc_timer_set_next_event(unsigned long delta, return next - now > delta ? -ETIME : 0; }
-static void sirfsoc_timer_set_mode(enum clock_event_mode mode, +static int sirfsoc_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *ce) { u32 val = readl_relaxed(sirfsoc_timer_base + SIRFSOC_TIMER_INT_EN); switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - WARN_ON(1); - break; case CLOCK_EVT_MODE_ONESHOT: writel_relaxed(val | BIT(0), sirfsoc_timer_base + SIRFSOC_TIMER_INT_EN); break; @@ -116,7 +113,10 @@ static void sirfsoc_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void sirfsoc_clocksource_suspend(struct clocksource *cs) @@ -144,7 +144,7 @@ static struct clock_event_device sirfsoc_clockevent = { .name = "sirfsoc_clockevent", .rating = 200, .features = CLOCK_EVT_FEAT_ONESHOT, - .set_mode = sirfsoc_timer_set_mode, + .set_dev_mode = sirfsoc_timer_set_mode, .set_next_event = sirfsoc_timer_set_next_event, };
diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c index deebcd6..ce690f4 100644 --- a/drivers/clocksource/timer-sun5i.c +++ b/drivers/clocksource/timer-sun5i.c @@ -79,7 +79,7 @@ static void sun5i_clkevt_time_start(u8 timer, bool periodic) timer_base + TIMER_CTL_REG(timer)); }
-static void sun5i_clkevt_mode(enum clock_event_mode mode, +static int sun5i_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *clk) { switch (mode) { @@ -94,10 +94,14 @@ static void sun5i_clkevt_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: - default: sun5i_clkevt_time_stop(0); break; + case CLOCK_EVT_MODE_RESUME: + break; + default: + return -ENOSYS; } + return 0; }
static int sun5i_clkevt_next_event(unsigned long evt, @@ -114,7 +118,7 @@ static struct clock_event_device sun5i_clockevent = { .name = "sun5i_tick", .rating = 340, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = sun5i_clkevt_mode, + .set_dev_mode = sun5i_clkevt_mode, .set_next_event = sun5i_clkevt_next_event, };
diff --git a/drivers/clocksource/timer-u300.c b/drivers/clocksource/timer-u300.c index 5dcf756..000a865 100644 --- a/drivers/clocksource/timer-u300.c +++ b/drivers/clocksource/timer-u300.c @@ -192,7 +192,7 @@ struct u300_clockevent_data { * have oneshot timer active, the oneshot scheduling function * u300_set_next_event() is called immediately after. */ -static void u300_set_mode(enum clock_event_mode mode, +static int u300_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct u300_clockevent_data *cevdata = @@ -265,7 +265,10 @@ static void u300_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Ignore this call */ break; + default: + return -ENOSYS; } + return 0; }
/* @@ -315,7 +318,7 @@ static struct u300_clockevent_data u300_clockevent_data = { .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = u300_set_next_event, - .set_mode = u300_set_mode, + .set_dev_mode = u300_set_mode, }, };
diff --git a/drivers/clocksource/vf_pit_timer.c b/drivers/clocksource/vf_pit_timer.c index a918bc4..a34e5c8 100644 --- a/drivers/clocksource/vf_pit_timer.c +++ b/drivers/clocksource/vf_pit_timer.c @@ -86,16 +86,22 @@ static int pit_set_next_event(unsigned long delta, return 0; }
-static void pit_set_mode(enum clock_event_mode mode, +static int pit_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_PERIODIC: pit_set_next_event(cycle_per_jiffy, evt); break; - default: + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static irqreturn_t pit_timer_interrupt(int irq, void *dev_id) @@ -121,7 +127,7 @@ static irqreturn_t pit_timer_interrupt(int irq, void *dev_id) static struct clock_event_device clockevent_pit = { .name = "VF pit timer", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = pit_set_mode, + .set_dev_mode = pit_set_mode, .set_next_event = pit_set_next_event, .rating = 300, }; diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c index 1098ed3..17492ed 100644 --- a/drivers/clocksource/vt8500_timer.c +++ b/drivers/clocksource/vt8500_timer.c @@ -88,12 +88,11 @@ static int vt8500_timer_set_next_event(unsigned long cycles, return 0; }
-static void vt8500_timer_set_mode(enum clock_event_mode mode, +static int vt8500_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: break; case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: @@ -102,7 +101,10 @@ static void vt8500_timer_set_mode(enum clock_event_mode mode, regbase + TIMER_CTRL_VAL); writel(0, regbase + TIMER_IER_VAL); break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent = { @@ -110,7 +112,7 @@ static struct clock_event_device clockevent = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = vt8500_timer_set_next_event, - .set_mode = vt8500_timer_set_mode, + .set_dev_mode = vt8500_timer_set_mode, };
static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id) diff --git a/drivers/clocksource/zevio-timer.c b/drivers/clocksource/zevio-timer.c index 7ce4421..ce7246d 100644 --- a/drivers/clocksource/zevio-timer.c +++ b/drivers/clocksource/zevio-timer.c @@ -76,7 +76,7 @@ static int zevio_timer_set_event(unsigned long delta, return 0; }
-static void zevio_timer_set_mode(enum clock_event_mode mode, +static int zevio_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { struct zevio_timer *timer = container_of(dev, struct zevio_timer, @@ -97,11 +97,10 @@ static void zevio_timer_set_mode(enum clock_event_mode mode, /* Stop timer */ writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL); break; - case CLOCK_EVT_MODE_PERIODIC: default: - /* Unsupported */ - break; + return -ENOSYS; } + return 0; }
static irqreturn_t zevio_timer_interrupt(int irq, void *dev_id) @@ -162,7 +161,7 @@ static int __init zevio_timer_add(struct device_node *node) if (timer->interrupt_regs && irqnr) { timer->clkevt.name = timer->clockevent_name; timer->clkevt.set_next_event = zevio_timer_set_event; - timer->clkevt.set_mode = zevio_timer_set_mode; + timer->clkevt.set_dev_mode = zevio_timer_set_mode; timer->clkevt.rating = 200; timer->clkevt.cpumask = cpu_all_mask; timer->clkevt.features = CLOCK_EVT_FEAT_ONESHOT;
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
Migrate all drivers in 'drivers/clocksource' to implement this new callback.
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
[snip]
diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c index e807acf..f72ef19 100644 --- a/drivers/clocksource/qcom-timer.c +++ b/drivers/clocksource/qcom-timer.c @@ -75,7 +75,7 @@ static int msm_timer_set_next_event(unsigned long cycles, return 0; }
-static void msm_timer_set_mode(enum clock_event_mode mode, +static int msm_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -85,16 +85,17 @@ static void msm_timer_set_mode(enum clock_event_mode mode,
switch (mode) { case CLOCK_EVT_MODE_RESUME:
- case CLOCK_EVT_MODE_PERIODIC:
break;
Sorry about repeatedly pointing this. I may be missing something. But why are the above two lines removed?
case CLOCK_EVT_MODE_ONESHOT: /* Timer is enabled in set_next_event */ break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break;
- default:
return -ENOSYS;
Would it be fine to skip this ^^ in the default case?
- return 0;
}
[snip]
diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c index 0451e62..8aa35c1 100644 --- a/drivers/clocksource/time-armada-370-xp.c +++ b/drivers/clocksource/time-armada-370-xp.c @@ -120,12 +120,12 @@ armada_370_xp_clkevt_next_event(unsigned long delta, return 0; }
-static void +static int armada_370_xp_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) {
- if (mode == CLOCK_EVT_MODE_PERIODIC) {
- switch (mode) {
- case CLOCK_EVT_MODE_PERIODIC: /*
*/
- Setup timer to fire at 1/HZ intervals.
@@ -136,7 +136,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * Enable timer. */ local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask);
- } else {
break;- case CLOCK_EVT_MODE_ONESHOT:
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME: /*
*/
- Disable timer.
@@ -146,7 +150,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * ACK pending timer interrupt. */ writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
This line in the right place right? I mean do you need it in default case also? I am unable to make out.
break;- default:
return -ENOSYS;- return 0;
}
static int armada_370_xp_clkevt_irq; @@ -184,7 +192,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt) evt->shift = 32, evt->rating = 300, evt->set_next_event = armada_370_xp_clkevt_next_event,
- evt->set_mode = armada_370_xp_clkevt_mode,
- evt->set_dev_mode = armada_370_xp_clkevt_mode, evt->irq = armada_370_xp_clkevt_irq; evt->cpumask = cpumask_of(cpu);
@@ -196,7 +204,7 @@ static int armada_370_xp_timer_setup(struct clock_event_device *evt)
static void armada_370_xp_timer_stop(struct clock_event_device *evt) {
- evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
- evt->set_dev_mode(CLOCK_EVT_MODE_UNUSED, evt); disable_percpu_irq(evt->irq);
[snip]
diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c index 1098ed3..17492ed 100644 --- a/drivers/clocksource/vt8500_timer.c +++ b/drivers/clocksource/vt8500_timer.c @@ -88,12 +88,11 @@ static int vt8500_timer_set_next_event(unsigned long cycles, return 0; }
-static void vt8500_timer_set_mode(enum clock_event_mode mode, +static int vt8500_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_RESUME:
- case CLOCK_EVT_MODE_PERIODIC: break;
Is this deletion right?
case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_UNUSED: @@ -102,7 +101,10 @@ static void vt8500_timer_set_mode(enum clock_event_mode mode, regbase + TIMER_CTRL_VAL); writel(0, regbase + TIMER_IER_VAL); break;
- default:
return -ENOSYS;- return 0;
}
static struct clock_event_device clockevent = { @@ -110,7 +112,7 @@ static struct clock_event_device clockevent = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = vt8500_timer_set_next_event,
- .set_mode = vt8500_timer_set_mode,
- .set_dev_mode = vt8500_timer_set_mode,
};
static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
Regards Preeti U Murthy
On 29 May 2014 16:50, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c
-static void msm_timer_set_mode(enum clock_event_mode mode, +static int msm_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -85,16 +85,17 @@ static void msm_timer_set_mode(enum clock_event_mode mode,
switch (mode) { case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:break;Sorry about repeatedly pointing this. I may be missing something. But why are the above two lines removed?
mode isn't supported..
case CLOCK_EVT_MODE_ONESHOT: /* Timer is enabled in set_next_event */ break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break;
default:return -ENOSYS; } writel_relaxed(ctrl, event_base + TIMER_ENABLE);Would it be fine to skip this ^^ in the default case?
We shouldn't be executing this for unsupported modes, right?
return 0;}
[snip]
diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c index 0451e62..8aa35c1 100644 --- a/drivers/clocksource/time-armada-370-xp.c +++ b/drivers/clocksource/time-armada-370-xp.c @@ -120,12 +120,12 @@ armada_370_xp_clkevt_next_event(unsigned long delta, return 0; }
-static void +static int armada_370_xp_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) {
if (mode == CLOCK_EVT_MODE_PERIODIC) {
switch (mode) {case CLOCK_EVT_MODE_PERIODIC: /* * Setup timer to fire at 1/HZ intervals. */@@ -136,7 +136,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * Enable timer. */ local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask);
} else {
break;case CLOCK_EVT_MODE_ONESHOT:case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: /* * Disable timer. */@@ -146,7 +150,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * ACK pending timer interrupt. */ writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
This line in the right place right? I mean do you need it in default case also? I am unable to make out.
default cases now are only for unsupported modes and we shouldn't call anything in that..
+++ b/drivers/clocksource/vt8500_timer.c @@ -88,12 +88,11 @@ static int vt8500_timer_set_next_event(unsigned long cycles, return 0; }
-static void vt8500_timer_set_mode(enum clock_event_mode mode, +static int vt8500_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC: break;Is this deletion right?
mode isn't supported..
On 05/30/2014 07:14 AM, Viresh Kumar wrote:
On 29 May 2014 16:50, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
diff --git a/drivers/clocksource/qcom-timer.c b/drivers/clocksource/qcom-timer.c
-static void msm_timer_set_mode(enum clock_event_mode mode, +static int msm_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { u32 ctrl; @@ -85,16 +85,17 @@ static void msm_timer_set_mode(enum clock_event_mode mode,
switch (mode) { case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:break;Sorry about repeatedly pointing this. I may be missing something. But why are the above two lines removed?
mode isn't supported..
case CLOCK_EVT_MODE_ONESHOT: /* Timer is enabled in set_next_event */ break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN: break;
default:return -ENOSYS; } writel_relaxed(ctrl, event_base + TIMER_ENABLE);Would it be fine to skip this ^^ in the default case?
We shouldn't be executing this for unsupported modes, right?
Right. Sorry I overlooked this.
return 0;}
[snip]
diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c index 0451e62..8aa35c1 100644 --- a/drivers/clocksource/time-armada-370-xp.c +++ b/drivers/clocksource/time-armada-370-xp.c @@ -120,12 +120,12 @@ armada_370_xp_clkevt_next_event(unsigned long delta, return 0; }
-static void +static int armada_370_xp_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev) {
if (mode == CLOCK_EVT_MODE_PERIODIC) {
switch (mode) {case CLOCK_EVT_MODE_PERIODIC: /* * Setup timer to fire at 1/HZ intervals. */@@ -136,7 +136,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * Enable timer. */ local_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | enable_mask);
} else {
break;case CLOCK_EVT_MODE_ONESHOT:case CLOCK_EVT_MODE_UNUSED:case CLOCK_EVT_MODE_SHUTDOWN:case CLOCK_EVT_MODE_RESUME: /* * Disable timer. */@@ -146,7 +150,11 @@ armada_370_xp_clkevt_mode(enum clock_event_mode mode, * ACK pending timer interrupt. */ writel(TIMER0_CLR_MASK, local_base + LCL_TIMER_EVENTS_STATUS);
This line in the right place right? I mean do you need it in default case also? I am unable to make out.
default cases now are only for unsupported modes and we shouldn't call anything in that..
+++ b/drivers/clocksource/vt8500_timer.c @@ -88,12 +88,11 @@ static int vt8500_timer_set_next_event(unsigned long cycles, return 0; }
-static void vt8500_timer_set_mode(enum clock_event_mode mode, +static int vt8500_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC: break;Is this deletion right?
mode isn't supported..
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
This patch migrates clockevent drivers for all architectures that had a single clockevent driver (in order to limit patch count).
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
A simplified version of the semantic patch is:
@@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c);
@@ identifier setmode; @@ -void +int setmode(enum clock_event_mode, struct clock_event_device *);
@fixret@ identifier m,c,setmode; @@ -void +int setmode(enum clock_event_mode m, struct clock_event_device *c) { ... + return 0; }
@depends on fixret@ identifier ced; identifier fixret.setmode; @@ ... struct clock_event_device ced = { ..., -.set_mode +.set_dev_mode = setmode, };
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced->set_mode + ced->set_dev_mode = setmode
@depends on fixret@ identifier fixret.setmode; @@ { . -set_mode +set_dev_mode = setmode }
@depends on fixret@ expression ced; identifier fixret.setmode; @@ - ced.set_mode + ced.set_dev_mode = setmode
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/alpha/kernel/time.c | 32 +++++++++++++++++++++++++++----- arch/arc/kernel/time.c | 11 +++++++---- arch/avr32/kernel/time.c | 7 ++++--- arch/blackfin/kernel/time-ts.c | 14 ++++++++++---- arch/c6x/platforms/timer64.c | 7 +++++-- arch/hexagon/kernel/time.c | 11 ++++++++--- arch/m68k/platform/coldfire/pit.c | 7 +++++-- arch/microblaze/kernel/timer.c | 7 +++++-- arch/mn10300/kernel/cevt-mn10300.c | 14 ++++++++++++-- arch/openrisc/kernel/time.c | 11 +++++------ arch/powerpc/kernel/time.c | 16 ++++++++++++---- arch/s390/kernel/time.c | 14 ++++++++++++-- arch/score/kernel/time.c | 8 ++++---- arch/sh/kernel/localtimer.c | 15 +++++++++++++-- arch/tile/kernel/time.c | 16 +++++++++++++--- arch/um/kernel/time.c | 7 +++++-- arch/unicore32/kernel/time.c | 8 +++++--- arch/xtensa/kernel/time.c | 9 +++++---- kernel/time/tick-broadcast-hrtimer.c | 11 ++++++++--- 19 files changed, 165 insertions(+), 60 deletions(-)
diff --git a/arch/alpha/kernel/time.c b/arch/alpha/kernel/time.c index ee39cee..97b8bac 100644 --- a/arch/alpha/kernel/time.c +++ b/arch/alpha/kernel/time.c @@ -104,11 +104,22 @@ rtc_timer_interrupt(int irq, void *dev) return IRQ_HANDLED; }
-static void +static int rtc_ce_set_mode(enum clock_event_mode mode, struct clock_event_device *ce) { /* The mode member of CE is updated in generic code. Since we only support periodic events, nothing to do. */ + switch (mode) { + case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + break; + + default: + return -ENOSYS; + } + return 0; }
static int @@ -129,7 +140,7 @@ init_rtc_clockevent(void) .features = CLOCK_EVT_FEAT_PERIODIC, .rating = 100, .cpumask = cpumask_of(cpu), - .set_mode = rtc_ce_set_mode, + .set_dev_mode = rtc_ce_set_mode, .set_next_event = rtc_ce_set_next_event, };
@@ -161,12 +172,23 @@ static struct clocksource qemu_cs = { * The QEMU alarm as a clock_event_device primitive. */
-static void +static int qemu_ce_set_mode(enum clock_event_mode mode, struct clock_event_device *ce) { /* The mode member of CE is updated for us in generic code. Just make sure that the event is disabled. */ - qemu_set_alarm_abs(0); + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + qemu_set_alarm_abs(0); + break; + + default: + return -ENOSYS; + } + return 0; }
static int @@ -197,7 +219,7 @@ init_qemu_clockevent(void) .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 400, .cpumask = cpumask_of(cpu), - .set_mode = qemu_ce_set_mode, + .set_dev_mode = qemu_ce_set_mode, .set_next_event = qemu_ce_set_next_event, };
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c index 71c4252..5a9d348 100644 --- a/arch/arc/kernel/time.c +++ b/arch/arc/kernel/time.c @@ -163,7 +163,7 @@ static int arc_clkevent_set_next_event(unsigned long delta, return 0; }
-static void arc_clkevent_set_mode(enum clock_event_mode mode, +static int arc_clkevent_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -171,12 +171,15 @@ static void arc_clkevent_set_mode(enum clock_event_mode mode, arc_timer_event_setup(arc_get_core_freq() / HZ); break; case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: break; default: - break; + return -ENOSYS; }
- return; + return 0; }
static DEFINE_PER_CPU(struct clock_event_device, arc_clockevent_device) = { @@ -186,7 +189,7 @@ static DEFINE_PER_CPU(struct clock_event_device, arc_clockevent_device) = { .rating = 300, .irq = TIMER0_IRQ, /* hardwired, no need for resources */ .set_next_event = arc_clkevent_set_next_event, - .set_mode = arc_clkevent_set_mode, + .set_dev_mode = arc_clkevent_set_mode, };
static irqreturn_t timer_irq_handler(int irq, void *dev_id) diff --git a/arch/avr32/kernel/time.c b/arch/avr32/kernel/time.c index d0f771b..3f82011 100644 --- a/arch/avr32/kernel/time.c +++ b/arch/avr32/kernel/time.c @@ -80,7 +80,7 @@ static int comparator_next_event(unsigned long delta, return 0; }
-static void comparator_mode(enum clock_event_mode mode, +static int comparator_mode(enum clock_event_mode mode, struct clock_event_device *evdev) { switch (mode) { @@ -108,8 +108,9 @@ static void comparator_mode(enum clock_event_mode mode, } break; default: - BUG(); + return -ENOSYS; } + return 0; }
static struct clock_event_device comparator = { @@ -118,7 +119,7 @@ static struct clock_event_device comparator = { .shift = 16, .rating = 50, .set_next_event = comparator_next_event, - .set_mode = comparator_mode, + .set_dev_mode = comparator_mode, };
void read_persistent_clock(struct timespec *ts) diff --git a/arch/blackfin/kernel/time-ts.c b/arch/blackfin/kernel/time-ts.c index cb0a484..b1592a1 100644 --- a/arch/blackfin/kernel/time-ts.c +++ b/arch/blackfin/kernel/time-ts.c @@ -136,7 +136,7 @@ static int bfin_gptmr0_set_next_event(unsigned long cycles, return 0; }
-static void bfin_gptmr0_set_mode(enum clock_event_mode mode, +static int bfin_gptmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -173,7 +173,10 @@ static void bfin_gptmr0_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static void bfin_gptmr0_ack(void) @@ -217,7 +220,7 @@ static struct clock_event_device clockevent_gptmr0 = { .shift = 32, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, .set_next_event = bfin_gptmr0_set_next_event, - .set_mode = bfin_gptmr0_set_mode, + .set_dev_mode = bfin_gptmr0_set_mode, };
static void __init bfin_gptmr0_clockevent_init(struct clock_event_device *evt) @@ -250,7 +253,7 @@ static int bfin_coretmr_set_next_event(unsigned long cycles, return 0; }
-static void bfin_coretmr_set_mode(enum clock_event_mode mode, +static int bfin_coretmr_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -279,7 +282,10 @@ static void bfin_coretmr_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
void bfin_coretmr_init(void) @@ -335,7 +341,7 @@ void bfin_coretmr_clockevent_init(void) evt->shift = 32; evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; evt->set_next_event = bfin_coretmr_set_next_event; - evt->set_mode = bfin_coretmr_set_mode; + evt->set_dev_mode = bfin_coretmr_set_mode;
clock_tick = get_cclk() / TIME_SCALE; evt->mult = div_sc(clock_tick, NSEC_PER_SEC, evt->shift); diff --git a/arch/c6x/platforms/timer64.c b/arch/c6x/platforms/timer64.c index 3c73d74..3eef8fb 100644 --- a/arch/c6x/platforms/timer64.c +++ b/arch/c6x/platforms/timer64.c @@ -126,7 +126,7 @@ static int next_event(unsigned long delta, return 0; }
-static void set_clock_mode(enum clock_event_mode mode, +static int set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -146,14 +146,17 @@ static void set_clock_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device t64_clockevent_device = { .name = "TIMER64_EVT32_TIMER", .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC, .rating = 200, - .set_mode = set_clock_mode, + .set_dev_mode = set_clock_mode, .set_next_event = next_event, };
diff --git a/arch/hexagon/kernel/time.c b/arch/hexagon/kernel/time.c index 17fbf45..540b138 100644 --- a/arch/hexagon/kernel/time.c +++ b/arch/hexagon/kernel/time.c @@ -100,15 +100,20 @@ static int set_next_event(unsigned long delta, struct clock_event_device *evt) /* * Sets the mode (periodic, shutdown, oneshot, etc) of a timer. */ -static void set_mode(enum clock_event_mode mode, +static int set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { case CLOCK_EVT_MODE_SHUTDOWN: /* XXX implement me */ - default: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
#ifdef CONFIG_SMP @@ -125,7 +130,7 @@ static struct clock_event_device hexagon_clockevent_dev = { .rating = 400, .irq = RTOS_TIMER_INT, .set_next_event = set_next_event, - .set_mode = set_mode, + .set_dev_mode = set_mode, #ifdef CONFIG_SMP .broadcast = broadcast, #endif diff --git a/arch/m68k/platform/coldfire/pit.c b/arch/m68k/platform/coldfire/pit.c index 493b311..993561a 100644 --- a/arch/m68k/platform/coldfire/pit.c +++ b/arch/m68k/platform/coldfire/pit.c @@ -42,7 +42,7 @@ static u32 pit_cnt; * This is also called after resume to bring the PIT into operation again. */
-static void init_cf_pit_timer(enum clock_event_mode mode, +static int init_cf_pit_timer(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -72,7 +72,10 @@ static void init_cf_pit_timer(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: /* Nothing to do here */ break; + default: + return -ENOSYS; } + return 0; }
/* @@ -90,7 +93,7 @@ static int cf_pit_next_event(unsigned long delta, struct clock_event_device cf_pit_clockevent = { .name = "pit", .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = init_cf_pit_timer, + .set_dev_mode = init_cf_pit_timer, .set_next_event = cf_pit_next_event, .shift = 32, .irq = MCF_IRQ_PIT1, diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c index dd96f0e..21c17db 100644 --- a/arch/microblaze/kernel/timer.c +++ b/arch/microblaze/kernel/timer.c @@ -121,7 +121,7 @@ static int xilinx_timer_set_next_event(unsigned long delta, return 0; }
-static void xilinx_timer_set_mode(enum clock_event_mode mode, +static int xilinx_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -142,7 +142,10 @@ static void xilinx_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: pr_info("%s: resume\n", __func__); break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device clockevent_xilinx_timer = { @@ -151,7 +154,7 @@ static struct clock_event_device clockevent_xilinx_timer = { .shift = 8, .rating = 300, .set_next_event = xilinx_timer_set_next_event, - .set_mode = xilinx_timer_set_mode, + .set_dev_mode = xilinx_timer_set_mode, };
static inline void timer_ack(void) diff --git a/arch/mn10300/kernel/cevt-mn10300.c b/arch/mn10300/kernel/cevt-mn10300.c index 60f64ca..668906a 100644 --- a/arch/mn10300/kernel/cevt-mn10300.c +++ b/arch/mn10300/kernel/cevt-mn10300.c @@ -41,10 +41,20 @@ static int next_event(unsigned long delta, return 0; }
-static void set_clock_mode(enum clock_event_mode mode, +static int set_clock_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: /* Nothing to do ... */ + break; + default: + return -ENOSYS; + } + return 0; }
static DEFINE_PER_CPU(struct clock_event_device, mn10300_clockevent_device); @@ -108,7 +118,7 @@ int __init init_clockevents(void)
cd->rating = 200; cd->cpumask = cpumask_of(smp_processor_id()); - cd->set_mode = set_clock_mode; + cd->set_dev_mode = set_clock_mode; cd->event_handler = event_handler; cd->set_next_event = next_event;
diff --git a/arch/openrisc/kernel/time.c b/arch/openrisc/kernel/time.c index 7c52e94..6844084 100644 --- a/arch/openrisc/kernel/time.c +++ b/arch/openrisc/kernel/time.c @@ -48,14 +48,10 @@ static int openrisc_timer_set_next_event(unsigned long delta, return 0; }
-static void openrisc_timer_set_mode(enum clock_event_mode mode, +static int openrisc_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - pr_debug(KERN_INFO "%s: periodic\n", __func__); - BUG(); - break; case CLOCK_EVT_MODE_ONESHOT: pr_debug(KERN_INFO "%s: oneshot\n", __func__); break; @@ -68,7 +64,10 @@ static void openrisc_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: pr_debug(KERN_INFO "%s: resume\n", __func__); break; + default: + return -ENOSYS; } + return 0; }
/* This is the clock event device based on the OR1K tick timer. @@ -82,7 +81,7 @@ static struct clock_event_device clockevent_openrisc_timer = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 300, .set_next_event = openrisc_timer_set_next_event, - .set_mode = openrisc_timer_set_mode, + .set_dev_mode = openrisc_timer_set_mode, };
static inline void timer_ack(void) diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 122a580..f552041 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -98,7 +98,7 @@ static struct clocksource clocksource_timebase = {
static int decrementer_set_next_event(unsigned long evt, struct clock_event_device *dev); -static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev);
struct clock_event_device decrementer_clockevent = { @@ -106,7 +106,7 @@ struct clock_event_device decrementer_clockevent = { .rating = 200, .irq = 0, .set_next_event = decrementer_set_next_event, - .set_mode = decrementer_set_mode, + .set_dev_mode = decrementer_set_mode, .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP, }; EXPORT_SYMBOL(decrementer_clockevent); @@ -826,11 +826,19 @@ static int decrementer_set_next_event(unsigned long evt, return 0; }
-static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { - if (mode != CLOCK_EVT_MODE_ONESHOT) + switch (mode) { + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: decrementer_set_next_event(DECREMENTER_MAX, dev); + break; + default: + return -ENOSYS; + } + return 0; }
/* Interrupt handler for the timer broadcast IPI */ diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c index 386d37a..49e3812 100644 --- a/arch/s390/kernel/time.c +++ b/arch/s390/kernel/time.c @@ -116,9 +116,19 @@ static int s390_next_event(unsigned long delta, return 0; }
-static void s390_set_mode(enum clock_event_mode mode, +static int s390_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + break; + default: + return -ENOSYS; + } + return 0; }
/* @@ -144,7 +154,7 @@ void init_cpu_timer(void) cd->rating = 400; cd->cpumask = cpumask_of(cpu); cd->set_next_event = s390_next_event; - cd->set_mode = s390_set_mode; + cd->set_dev_mode = s390_set_mode;
clockevents_register_device(cd);
diff --git a/arch/score/kernel/time.c b/arch/score/kernel/time.c index f0a43aff..886bfdd 100644 --- a/arch/score/kernel/time.c +++ b/arch/score/kernel/time.c @@ -55,7 +55,7 @@ static int score_timer_set_next_event(unsigned long delta, return 0; }
-static void score_timer_set_mode(enum clock_event_mode mode, +static int score_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evdev) { switch (mode) { @@ -64,14 +64,14 @@ static void score_timer_set_mode(enum clock_event_mode mode, outl(SYSTEM_CLOCK/HZ, P_TIMER0_PRELOAD); outl(inl(P_TIMER0_CTRL) | TMR_ENABLE, P_TIMER0_CTRL); break; - case CLOCK_EVT_MODE_ONESHOT: case CLOCK_EVT_MODE_SHUTDOWN: case CLOCK_EVT_MODE_RESUME: case CLOCK_EVT_MODE_UNUSED: break; default: - BUG(); + return -ENOSYS; } + return 0; }
static struct clock_event_device score_clockevent = { @@ -79,7 +79,7 @@ static struct clock_event_device score_clockevent = { .features = CLOCK_EVT_FEAT_PERIODIC, .shift = 16, .set_next_event = score_timer_set_next_event, - .set_mode = score_timer_set_mode, + .set_dev_mode = score_timer_set_mode, };
void __init time_init(void) diff --git a/arch/sh/kernel/localtimer.c b/arch/sh/kernel/localtimer.c index 8bfc6df..7d48435 100644 --- a/arch/sh/kernel/localtimer.c +++ b/arch/sh/kernel/localtimer.c @@ -39,9 +39,20 @@ void local_timer_interrupt(void) irq_exit(); }
-static void dummy_timer_set_mode(enum clock_event_mode mode, +static int dummy_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_PERIODIC: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + break; + default: + return -ENOSYS; + } + return 0; }
void local_timer_setup(unsigned int cpu) @@ -54,7 +65,7 @@ void local_timer_setup(unsigned int cpu) CLOCK_EVT_FEAT_DUMMY; clk->rating = 400; clk->mult = 1; - clk->set_mode = dummy_timer_set_mode; + clk->set_dev_mode = dummy_timer_set_mode; clk->broadcast = smp_timer_broadcast; clk->cpumask = cpumask_of(cpu);
diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c index 462dcd0..40d532b 100644 --- a/arch/tile/kernel/time.c +++ b/arch/tile/kernel/time.c @@ -140,10 +140,20 @@ static int tile_timer_set_next_event(unsigned long ticks, * Whenever anyone tries to change modes, we just mask interrupts * and wait for the next event to get set. */ -static void tile_timer_set_mode(enum clock_event_mode mode, +static int tile_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { - arch_local_irq_mask_now(INT_TILE_TIMER); + switch (mode) { + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_SHUTDOWN: + case CLOCK_EVT_MODE_RESUME: + arch_local_irq_mask_now(INT_TILE_TIMER); + break; + default: + return -ENOSYS; + } + return 0; }
/* @@ -157,7 +167,7 @@ static DEFINE_PER_CPU(struct clock_event_device, tile_timer) = { .rating = 100, .irq = -1, .set_next_event = tile_timer_set_next_event, - .set_mode = tile_timer_set_mode, + .set_dev_mode = tile_timer_set_mode, };
void setup_tile_timer(void) diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c index 117568d..fdcd4eb 100644 --- a/arch/um/kernel/time.c +++ b/arch/um/kernel/time.c @@ -22,7 +22,7 @@ void timer_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) local_irq_restore(flags); }
-static void itimer_set_mode(enum clock_event_mode mode, +static int itimer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) { @@ -38,7 +38,10 @@ static void itimer_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
static int itimer_next_event(unsigned long delta, @@ -52,7 +55,7 @@ static struct clock_event_device itimer_clockevent = { .rating = 250, .cpumask = cpu_all_mask, .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, - .set_mode = itimer_set_mode, + .set_dev_mode = itimer_set_mode, .set_next_event = itimer_next_event, .shift = 32, .irq = 0, diff --git a/arch/unicore32/kernel/time.c b/arch/unicore32/kernel/time.c index d3824b2..c19f5a5 100644 --- a/arch/unicore32/kernel/time.c +++ b/arch/unicore32/kernel/time.c @@ -46,7 +46,7 @@ puv3_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int puv3_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -58,9 +58,11 @@ puv3_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break;
case CLOCK_EVT_MODE_RESUME: - case CLOCK_EVT_MODE_PERIODIC: break; + default: + return -ENOSYS; } + return 0; }
static struct clock_event_device ckevt_puv3_osmr0 = { @@ -68,7 +70,7 @@ static struct clock_event_device ckevt_puv3_osmr0 = { .features = CLOCK_EVT_FEAT_ONESHOT, .rating = 200, .set_next_event = puv3_osmr0_set_next_event, - .set_mode = puv3_osmr0_set_mode, + .set_dev_mode = puv3_osmr0_set_mode, };
static cycle_t puv3_read_oscr(struct clocksource *cs) diff --git a/arch/xtensa/kernel/time.c b/arch/xtensa/kernel/time.c index 2a1823d..529f060 100644 --- a/arch/xtensa/kernel/time.c +++ b/arch/xtensa/kernel/time.c @@ -52,7 +52,7 @@ static struct clocksource ccount_clocksource = {
static int ccount_timer_set_next_event(unsigned long delta, struct clock_event_device *dev); -static void ccount_timer_set_mode(enum clock_event_mode mode, +static int ccount_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt); struct ccount_timer { struct clock_event_device evt; @@ -77,7 +77,7 @@ static int ccount_timer_set_next_event(unsigned long delta, return ret; }
-static void ccount_timer_set_mode(enum clock_event_mode mode, +static int ccount_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { struct ccount_timer *timer = @@ -104,8 +104,9 @@ static void ccount_timer_set_mode(enum clock_event_mode mode, timer->irq_enabled = 1; } default: - break; + return -ENOSYS; } + return 0; }
static irqreturn_t timer_interrupt(int irq, void *dev_id); @@ -126,7 +127,7 @@ void local_timer_setup(unsigned cpu) clockevent->features = CLOCK_EVT_FEAT_ONESHOT; clockevent->rating = 300; clockevent->set_next_event = ccount_timer_set_next_event; - clockevent->set_mode = ccount_timer_set_mode; + clockevent->set_dev_mode = ccount_timer_set_mode; clockevent->cpumask = cpumask_of(cpu); clockevent->irq = irq_create_mapping(NULL, LINUX_TIMER_INT); if (WARN(!clockevent->irq, "error: can't map timer irq")) diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c index eb682d5..de2e5e2 100644 --- a/kernel/time/tick-broadcast-hrtimer.c +++ b/kernel/time/tick-broadcast-hrtimer.c @@ -18,7 +18,7 @@
static struct hrtimer bctimer;
-static void bc_set_mode(enum clock_event_mode mode, +static int bc_set_mode(enum clock_event_mode mode, struct clock_event_device *bc) { switch (mode) { @@ -38,9 +38,14 @@ static void bc_set_mode(enum clock_event_mode mode, */ hrtimer_try_to_cancel(&bctimer); break; - default: + case CLOCK_EVT_MODE_ONESHOT: + case CLOCK_EVT_MODE_UNUSED: + case CLOCK_EVT_MODE_RESUME: break; + default: + return -ENOSYS; } + return 0; }
/* @@ -72,7 +77,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc) }
static struct clock_event_device ce_broadcast_hrtimer = { - .set_mode = bc_set_mode, + .set_dev_mode = bc_set_mode, .set_next_ktime = bc_set_next, .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_KTIME |
Hi Viresh,
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 122a580..f552041 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -98,7 +98,7 @@ static struct clocksource clocksource_timebase = {
static int decrementer_set_next_event(unsigned long evt, struct clock_event_device *dev); -static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev);
struct clock_event_device decrementer_clockevent = { @@ -106,7 +106,7 @@ struct clock_event_device decrementer_clockevent = { .rating = 200, .irq = 0, .set_next_event = decrementer_set_next_event,
- .set_mode = decrementer_set_mode,
- .set_dev_mode = decrementer_set_mode, .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP,
}; EXPORT_SYMBOL(decrementer_clockevent); @@ -826,11 +826,19 @@ static int decrementer_set_next_event(unsigned long evt, return 0; }
-static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) {
- if (mode != CLOCK_EVT_MODE_ONESHOT)
- switch (mode) {
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME: decrementer_set_next_event(DECREMENTER_MAX, dev);
break;- default:
return -ENOSYS;- }
- return 0;
}
You have left out CLOCK_EVT_MODE_ONESHOT here. We support this mode, in that, we do not need to do anything specific. In the above code it falls to default and returns "unsupported".
So you should have the below too.
case CLOCK_EVT_MODE_ONESHOT: break;
Regards Preeti U Murthy
On 25 May 2014 14:58, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
You have left out CLOCK_EVT_MODE_ONESHOT here. We support this mode, in that, we do not need to do anything specific. In the above code it falls to default and returns "unsupported".
So you should have the below too.
case CLOCK_EVT_MODE_ONESHOT: break;
Well, I tried to take care of this and somehow got skipped for you. Will fix it.
On 26 May 2014 09:47, Viresh Kumar viresh.kumar@linaro.org wrote:
On 25 May 2014 14:58, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
You have left out CLOCK_EVT_MODE_ONESHOT here. We support this mode, in that, we do not need to do anything specific. In the above code it falls to default and returns "unsupported".
So you should have the below too.
case CLOCK_EVT_MODE_ONESHOT: break;
Well, I tried to take care of this and somehow got skipped for you. Will fix it.
Okay I did another review of the all 102 files manually and found only this fix:
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index f552041..a6be92c 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -835,6 +835,8 @@ static int decrementer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: decrementer_set_next_event(DECREMENTER_MAX, dev); break; + case CLOCK_EVT_MODE_ONESHOT: + break; default: return -ENOSYS; }
On 05/26/2014 10:13 AM, Viresh Kumar wrote:
On 26 May 2014 09:47, Viresh Kumar viresh.kumar@linaro.org wrote:
On 25 May 2014 14:58, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
You have left out CLOCK_EVT_MODE_ONESHOT here. We support this mode, in that, we do not need to do anything specific. In the above code it falls to default and returns "unsupported".
So you should have the below too.
case CLOCK_EVT_MODE_ONESHOT: break;
Well, I tried to take care of this and somehow got skipped for you. Will fix it.
Okay I did another review of the all 102 files manually and found only this fix:
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index f552041..a6be92c 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -835,6 +835,8 @@ static int decrementer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: decrementer_set_next_event(DECREMENTER_MAX, dev); break;
case CLOCK_EVT_MODE_ONESHOT:break; default: return -ENOSYS; }
Yes this looks good.
Regards Preeti U Murthy
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
This patch migrates clockevent drivers for all architectures that had a single clockevent driver (in order to limit patch count).
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
arch/alpha/kernel/time.c | 32 +++++++++++++++++++++++++++----- arch/arc/kernel/time.c | 11 +++++++---- arch/avr32/kernel/time.c | 7 ++++--- arch/blackfin/kernel/time-ts.c | 14 ++++++++++---- arch/c6x/platforms/timer64.c | 7 +++++-- arch/hexagon/kernel/time.c | 11 ++++++++--- arch/m68k/platform/coldfire/pit.c | 7 +++++-- arch/microblaze/kernel/timer.c | 7 +++++-- arch/mn10300/kernel/cevt-mn10300.c | 14 ++++++++++++-- arch/openrisc/kernel/time.c | 11 +++++------ arch/powerpc/kernel/time.c | 16 ++++++++++++---- arch/s390/kernel/time.c | 14 ++++++++++++-- arch/score/kernel/time.c | 8 ++++---- arch/sh/kernel/localtimer.c | 15 +++++++++++++-- arch/tile/kernel/time.c | 16 +++++++++++++--- arch/um/kernel/time.c | 7 +++++-- arch/unicore32/kernel/time.c | 8 +++++--- arch/xtensa/kernel/time.c | 9 +++++---- kernel/time/tick-broadcast-hrtimer.c | 11 ++++++++--- 19 files changed, 165 insertions(+), 60 deletions(-)
static inline void timer_ack(void) diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 122a580..f552041 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -98,7 +98,7 @@ static struct clocksource clocksource_timebase = {
static int decrementer_set_next_event(unsigned long evt, struct clock_event_device *dev); -static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev);
struct clock_event_device decrementer_clockevent = { @@ -106,7 +106,7 @@ struct clock_event_device decrementer_clockevent = { .rating = 200, .irq = 0, .set_next_event = decrementer_set_next_event,
- .set_mode = decrementer_set_mode,
- .set_dev_mode = decrementer_set_mode, .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP,
}; EXPORT_SYMBOL(decrementer_clockevent); @@ -826,11 +826,19 @@ static int decrementer_set_next_event(unsigned long evt, return 0; }
-static void decrementer_set_mode(enum clock_event_mode mode, +static int decrementer_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) {
- if (mode != CLOCK_EVT_MODE_ONESHOT)
- switch (mode) {
- case CLOCK_EVT_MODE_UNUSED:
- case CLOCK_EVT_MODE_SHUTDOWN:
- case CLOCK_EVT_MODE_RESUME: decrementer_set_next_event(DECREMENTER_MAX, dev);
break;- default:
return -ENOSYS;- }
- return 0;
}
For the powerpc part,the above patch + the one you posted ontop of it.
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
This patch migrates clockevent drivers for all architectures that had a single clockevent driver (in order to limit patch count).
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
[snip]
diff --git a/arch/openrisc/kernel/time.c b/arch/openrisc/kernel/time.c index 7c52e94..6844084 100644 --- a/arch/openrisc/kernel/time.c +++ b/arch/openrisc/kernel/time.c @@ -48,14 +48,10 @@ static int openrisc_timer_set_next_event(unsigned long delta, return 0; }
-static void openrisc_timer_set_mode(enum clock_event_mode mode, +static int openrisc_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) {
- case CLOCK_EVT_MODE_PERIODIC:
pr_debug(KERN_INFO "%s: periodic\n", __func__);BUG();break;
Why is this removed?
case CLOCK_EVT_MODE_ONESHOT: pr_debug(KERN_INFO "%s: oneshot\n", __func__); break; @@ -68,7 +64,10 @@ static void openrisc_timer_set_mode(enum clock_event_mode mode, case CLOCK_EVT_MODE_RESUME: pr_debug(KERN_INFO "%s: resume\n", __func__); break;
- default:
return -ENOSYS;- return 0;
}
[snip]
diff --git a/arch/score/kernel/time.c b/arch/score/kernel/time.c index f0a43aff..886bfdd 100644 --- a/arch/score/kernel/time.c +++ b/arch/score/kernel/time.c @@ -55,7 +55,7 @@ static int score_timer_set_next_event(unsigned long delta, return 0; }
-static void score_timer_set_mode(enum clock_event_mode mode, +static int score_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evdev) { switch (mode) { @@ -64,14 +64,14 @@ static void score_timer_set_mode(enum clock_event_mode mode, outl(SYSTEM_CLOCK/HZ, P_TIMER0_PRELOAD); outl(inl(P_TIMER0_CTRL) | TMR_ENABLE, P_TIMER0_CTRL); break;
- case CLOCK_EVT_MODE_ONESHOT:
Here too, why is this removed? [snip]
diff --git a/arch/unicore32/kernel/time.c b/arch/unicore32/kernel/time.c index d3824b2..c19f5a5 100644 --- a/arch/unicore32/kernel/time.c +++ b/arch/unicore32/kernel/time.c @@ -46,7 +46,7 @@ puv3_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c) return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0; }
-static void +static int puv3_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) { switch (mode) { @@ -58,9 +58,11 @@ puv3_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c) break;
case CLOCK_EVT_MODE_RESUME:
- case CLOCK_EVT_MODE_PERIODIC: break;
Same here.
Regards Preeti U Murthy
On 29 May 2014 15:53, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Clockevents core now supports ->set_dev_mode() (as a replacement to ->set_mode()), with capability to return error codes.
This patch migrates clockevent drivers for all architectures that had a single clockevent driver (in order to limit patch count).
Drivers now return -ENOSYS when a unsupported mode is passed to their ->set_dev_mode() callbacks and return 0 on success.
Most of the changes are automated with help of Coccinelle (http://coccinelle.lip6.fr/) and the ones left are around the switch block which are handled manually.
Some drivers had a WARN()/BUG()/pr_err()/empty-implementation for unsupported modes. These statements and unsupported modes are removed now as proper error handling with a WARN_ON() is done at clockevents core.
[snip]
diff --git a/arch/openrisc/kernel/time.c b/arch/openrisc/kernel/time.c index 7c52e94..6844084 100644 --- a/arch/openrisc/kernel/time.c +++ b/arch/openrisc/kernel/time.c @@ -48,14 +48,10 @@ static int openrisc_timer_set_next_event(unsigned long delta, return 0; }
-static void openrisc_timer_set_mode(enum clock_event_mode mode, +static int openrisc_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) { switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:pr_debug(KERN_INFO "%s: periodic\n", __func__);BUG();break;Why is this removed?
Modes aren't supported by these drivers and so should return error for them. And let core handle these..
Same for all other comments too.
All clockevent drivers are migrated to use the new callback ->set_dev_mode(). Get rid of the deprecated ->dev_mode() callback now.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/clockchips.h | 3 --- kernel/time/clockevents.c | 19 +++++-------------- kernel/time/timer_list.c | 5 +---- 3 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 8ab1a86..08b203e 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -82,7 +82,6 @@ enum clock_event_mode { * @features: features * @retries: number of forced programming retries * @set_dev_mode: set dev mode function - * @set_mode: set mode function (deprecated, use set_dev_mode instead) * @broadcast: function to broadcast events * @min_delta_ticks: minimum delta value in ticks stored for reconfiguration * @max_delta_ticks: maximum delta value in ticks stored for reconfiguration @@ -112,8 +111,6 @@ struct clock_event_device { void (*broadcast)(const struct cpumask *mask); int (*set_dev_mode)(enum clock_event_mode mode, struct clock_event_device *); - void (*set_mode)(enum clock_event_mode mode, - struct clock_event_device *); void (*suspend)(struct clock_event_device *); void (*resume)(struct clock_event_device *); unsigned long min_delta_ticks; diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index f9bed16..9ff94d4 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -105,13 +105,9 @@ void clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) { - if (dev->set_dev_mode) { - /* WARN: Currently available modes shouldn't fail */ - if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev))) - return; - } else { - dev->set_mode(mode, dev); - } + /* WARN: Currently available modes shouldn't fail */ + if (WARN_ON_ONCE(dev->set_dev_mode(mode, dev))) + return;
dev->mode = mode;
@@ -453,13 +449,8 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) if (dev->mode == CLOCK_EVT_MODE_ONESHOT) return clockevents_program_event(dev, dev->next_event, false);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) { - if (dev->set_dev_mode) - WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, - dev)); - else - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); - } + if (dev->mode == CLOCK_EVT_MODE_PERIODIC) + WARN_ON_ONCE(dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev));
return 0; } diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c index 957a049..8e43c9f 100644 --- a/kernel/time/timer_list.c +++ b/kernel/time/timer_list.c @@ -229,10 +229,7 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu) SEQ_printf(m, "\n");
SEQ_printf(m, " set_mode: "); - if (dev->set_dev_mode) - print_name_offset(m, dev->set_dev_mode); - else - print_name_offset(m, dev->set_mode); + print_name_offset(m, dev->set_dev_mode); SEQ_printf(m, "\n");
SEQ_printf(m, " event_handler: ");
Hi Viresh,
On 05/23/2014 08:55 PM, Viresh Kumar wrote:
Hi,
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at-least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't. This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patchset first adds another callback with return capability, set_dev_mode(), then it migrates all drivers one by one and finally removes earlier callback set_mode() when nobody is using it.
NOTE: Following issue is still unresolved as this patchset isn't making it worse. But it should be fixed before addition of ONESHOT_STOPPED mode.
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
So I would suggest the below for this problem:
Add an additional case CLOCK_EVT_MODE_ONESHOT_STOPPED, which returns an error code. Let the 'default' case stay as it is and return success just like it is implemented today.
We should not be tampering with the 'default' case in the above scenario and make it return an error code when clearly the arch has taken care of the remaining modes that exist *today*. But when we add newer modes we should explicitly add these cases so that we do not break the arch's existing implementation from our end and let the arch decide for itself whether it wants a special case for the new mode or would default be good for the new mode also. What do you say?
Regards Preeti U Murthy
On 25 May 2014 15:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
So I would suggest the below for this problem:
Add an additional case CLOCK_EVT_MODE_ONESHOT_STOPPED, which returns an error code. Let the 'default' case stay as it is and return success just like it is implemented today.
Its not a good idea to bug all drivers for every new mode that comes in. Let drivers only take care of modes they are concerned with and for rest they should return -ENOSYS.
Returning success from 'default' look like a poor design to me.
On 05/26/2014 10:49 AM, Viresh Kumar wrote:
On 25 May 2014 15:08, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
So I would suggest the below for this problem:
Add an additional case CLOCK_EVT_MODE_ONESHOT_STOPPED, which returns an error code. Let the 'default' case stay as it is and return success just like it is implemented today.
Its not a good idea to bug all drivers for every new mode that comes in. Let drivers only take care of modes they are concerned with and for rest they should return -ENOSYS.
Returning success from 'default' look like a poor design to me.
Ah ok! I see now. I mistook the changelog. I assumed that these drivers are returning success in default case although they are disabling the clock device and that we now intend to change that to return an error code. Sorry about this!
Now I see that it is an issue. Now we are going to hit the default case and get returned an error code, but with disabled clock device which is unexpected.Hmmm..this needs some thought.
Regards Preeti U Murthy
On 26 May 2014 11:43, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ah ok! I see now. I mistook the changelog. I assumed that these drivers are returning success in default case although they are disabling the clock device and that we now intend to change that to return an error code. Sorry about this!
Now I see that it is an issue. Now we are going to hit the default case and get returned an error code, but with disabled clock device which is unexpected.Hmmm..this needs some thought.
Gud that we are on the same page now :)
I have already fixed it locally, not sure if that's the best approach though.
clockevent: drivers: don't disable events for unsupported modes
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
This patch tries to make sure such drivers don't left events disabled for unsupported modes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/common/timer-sp.c | 4 ++-- arch/arm/mach-at91/at91rm9200_time.c | 7 +++++-- arch/arm/mach-clps711x/common.c | 3 +-- arch/arm/mach-imx/epit.c | 9 +++++++-- arch/arm/mach-imx/time.c | 9 +++++++-- arch/arm/mach-integrator/integrator_ap.c | 12 ++++++------ arch/arm/mach-netx/time.c | 5 ++--- arch/arm/mach-omap1/timer32k.c | 5 ++--- arch/arm/mach-omap2/timer.c | 5 ++--- arch/arm/mach-spear/time.c | 10 ++++++---- arch/mips/kernel/cevt-txx9.c | 5 ++++- drivers/clocksource/cs5535-clockevt.c | 4 ++-- drivers/clocksource/exynos_mct.c | 7 ++++--- drivers/clocksource/mxs_timer.c | 7 ++++--- drivers/clocksource/samsung_pwm_timer.c | 6 ++---- drivers/clocksource/tegra20_timer.c | 5 ++--- drivers/clocksource/timer-marco.c | 2 +- 17 files changed, 59 insertions(+), 46 deletions(-)
diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c index 9ed3845..8831e9e 100644 --- a/arch/arm/common/timer-sp.c +++ b/arch/arm/common/timer-sp.c @@ -132,16 +132,16 @@ static int sp804_set_mode(enum clock_event_mode mode, { unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE;
- writel(ctrl, clkevt_base + TIMER_CTRL); - switch (mode) { case CLOCK_EVT_MODE_PERIODIC: + writel(ctrl, clkevt_base + TIMER_CTRL); writel(clkevt_reload, clkevt_base + TIMER_LOAD); ctrl |= TIMER_CTRL_PERIODIC | TIMER_CTRL_ENABLE; break;
case CLOCK_EVT_MODE_ONESHOT: /* period set, and timer enabled in 'next_event' hook */ + writel(ctrl, clkevt_base + TIMER_CTRL); ctrl |= TIMER_CTRL_ONESHOT; break;
... .. .
And this goes on for the drivers mentioned above.
Anyway, it was broken before this patch as well and so I will try getting that fixed once this set is in.
Have we reached a state to start getting reivewed-by's ? :)
On 23 May 2014 20:55, Viresh Kumar viresh.kumar@linaro.org wrote:
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
Hi Guys,
Does this still look *ugly* and can be improved?
Also please let me know how should I send it to LKML now? I think all 8 patches should be sent together as that's a complete solution, over that what time would be best for this considering merge window is around?
-- viresh
On Tue, May 27, 2014 at 05:37:33AM +0530, Viresh Kumar wrote:
On 23 May 2014 20:55, Viresh Kumar viresh.kumar@linaro.org wrote:
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
Hi Guys,
Does this still look *ugly* and can be improved?
It looks good to me.
Also please let me know how should I send it to LKML now? I think all 8 patches should be sent together as that's a complete solution, over that what time would be best for this considering merge window is around?
Yeah the 8 patches should be sent together. Feel free to send them when you want. Even during the merge window it's fine, there is just an increased risk that it gets ignored at that period. But we aren't yet there.
Thanks.
On 29 May 2014 06:41, Frederic Weisbecker fweisbec@gmail.com wrote:
Yeah the 8 patches should be sent together. Feel free to send them when you want. Even during the merge window it's fine, there is just an increased risk that it gets ignored at that period. But we aren't yet there.
Thanks..
HI Viresh,
Viresh Kumar viresh.kumar@linaro.org writes:
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
I'm going to nitpick some more on the changelog here, as well has a few comments. Sorry for the lag...
A clockevent device should be stopped, or its events should be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
This would normally happen with NO_HZ (both NO_HZ_IDLE and NO_HZ_FULL) when tick-sched timer is removed and we don't have any more timers scheduled for long.
If we don't STOP clockevent device, we are guaranteed to receive at least one spurious interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device (on expires == KTIME_MAX). Depending on particular implementation of clockevent device, this can be a spurious interrupt at tick-rate.. (When driver is emulating ONESHOT over PERIODIC mode. This was observed on at-least one implementation: arm_arch_timer.c).
A simple (yet hacky) solution to get this fixed could be: update hrtimer_force_reprogram() to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
However, Thomas suggested to add an optional mode: ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to solve this problem.
Up to here, the summary and background looks pretty good.
clockevents core would need to reconfigure mode later, when event is required again, for platforms that implement ONESHOT_STOPPED mode and so it must know which platforms implement it and which don't.
This is a long sentence that doesn't read well, and honestly, I'm not sure exactly what it says. I think you are trying to summarize Thomas' proposal? If so, it should maybe say: "With this proposal, introducing a new ONESHOT_STOPPED mode would require the core to know whether the platform implements this mode so it could be reprogrammed later."
This requires ->set_mode() callback to have capability to return 'success' or 'failure'. Currently its return type is 'void'.
"In order for the core to tell if the mode is implmented, the ->set_mode() callback needs to be able to return success or failure..."
To change return type of set_mode(), Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patchset first adds another callback with return capability, set_dev_mode(), then it migrates all drivers one by one and finally removes earlier callback set_mode() when nobody is using it.
NOTE: Following issue is still unresolved as this patchset isn't making it worse. But it should be fixed before addition of ONESHOT_STOPPED mode.
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
Not sure what you're saying heare about the default case. Do you mean currently, or after this series? A quick glance suggests that at least under arch/arm, there are several switch statments today that fall back to the default case, so I'm guessing you mean after this series? Please clarify.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
Why would core expect events to be anbled upon ->set_dev_mode() failure?
You're introducing this feature, so you get to decide what the core expects.
IMO, since current behavior of most clockevent drivers is to leave clockevent device disabled for unsupported modes, it's perfectly reasonable for the core to assume that the clockevent driver is/remains disabled when ->set_dev_mode() fails.
Since this series (or probably the followup which adds ONESHOT_STOPPED) will be handling the error modes. It should just be clear that when ->set_dev_mode() fails, part of the error handling should be that the core must put the clockevent device into a working mode.
Kevin
On 29 May 2014 22:12, Kevin Hilman khilman@linaro.org wrote:
I'm going to nitpick some more on the changelog here, as well has a few comments. Sorry for the lag...
No worries, I was out on a week-long holiday, just back today.
All accepted.
NOTE: Following issue is still unresolved as this patchset isn't making it worse. But it should be fixed before addition of ONESHOT_STOPPED mode.
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
Not sure what you're saying heare about the default case. Do you mean currently, or after this series? A quick glance suggests that at least under arch/arm, there are several switch statments today that fall back to the default case, so I'm guessing you mean after this series? Please clarify.
Yeah, I meant after this series is applied.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
Why would core expect events to be anbled upon ->set_dev_mode() failure?
I wasn't sure and so just mentioned that I can see this problem being there. Don't know how Thomas wants to see this :)
You're introducing this feature, so you get to decide what the core expects.
I wasn't talking about this feature but the default case in general, if something isn't supported we fail and core might/must believe nothing has changed..
IMO, since current behavior of most clockevent drivers is to leave clockevent device disabled for unsupported modes, it's perfectly reasonable for the core to assume that the clockevent driver is/remains disabled when ->set_dev_mode() fails.
Since this series (or probably the followup which adds ONESHOT_STOPPED) will be handling the error modes. It should just be clear that when ->set_dev_mode() fails, part of the error handling should be that the core must put the clockevent device into a working mode.
Probably yes. But would it be fine to have this NOTE in cover-letter for this series and ask for what's the right thing to do? And maybe give the options you gave and what I had ?
Viresh Kumar viresh.kumar@linaro.org writes:
This is my third attempt to migrate clockevent drivers to use set_dev_mode(). I have tried to fix all issues raised by Frederic/Kevin on V2.
Please let me know if I missed something. Though patches 2-7 look big, it should be fairly easy to review them now, so do try that :)
Speaking of ease of review...
In this series, I don't think you should do a wholesale conversion in this series and remove the existing ->set_mode().
I think the goal of this series should be simply the introduction of the new ->set_dev_mode() with its error handling, and the conversion of a handful clockevent devices to use it (IMO, arch/arm/* is a good representation of a variety of clockevent devices.)
Then, pick a clockevent device and implement ONESHOT_STOPPED for it.
You don't need to convert the world and remove the old way quite yet. Let's use this series to demonstrate the idea, showing a few examples, and the new ONESHOT_STOPPED implementation.
That will allow folks to see the whole idea, get a handle on it and allow us to come to agreement on the general approach.
Once there's agreement on the general approach, we can then cleanup the rest of the clockevent devices, convert them to ->set_dev_mode() and remove ->set_mode().
Kevin
On 29 May 2014 23:47, Kevin Hilman khilman@linaro.org wrote:
Speaking of ease of review...
In this series, I don't think you should do a wholesale conversion in this series and remove the existing ->set_mode().
I think the goal of this series should be simply the introduction of the new ->set_dev_mode() with its error handling, and the conversion of a handful clockevent devices to use it (IMO, arch/arm/* is a good representation of a variety of clockevent devices.)
Then, pick a clockevent device and implement ONESHOT_STOPPED for it.
You don't need to convert the world and remove the old way quite yet. Let's use this series to demonstrate the idea, showing a few examples, and the new ONESHOT_STOPPED implementation.
That will allow folks to see the whole idea, get a handle on it and allow us to come to agreement on the general approach.
Once there's agreement on the general approach, we can then cleanup the rest of the clockevent devices, convert them to ->set_dev_mode() and remove ->set_mode().
That's opposite of what Frederic said :)
I thought as I already have the series ready (with fixes suggested), we can send it as is. We just have 8 patches here.
If we try to send the ONESHOT series before this goes in, it will make the whole cycle much longer I feel.. People will keep jumping between these series and none would be merged easily..
So, to get these in I though probably first get this series merged and then try the next one..
Don't know really confused now after your suggestion :)
Viresh Kumar viresh.kumar@linaro.org writes:
On 29 May 2014 23:47, Kevin Hilman khilman@linaro.org wrote:
Speaking of ease of review...
In this series, I don't think you should do a wholesale conversion in this series and remove the existing ->set_mode().
I think the goal of this series should be simply the introduction of the new ->set_dev_mode() with its error handling, and the conversion of a handful clockevent devices to use it (IMO, arch/arm/* is a good representation of a variety of clockevent devices.)
Then, pick a clockevent device and implement ONESHOT_STOPPED for it.
You don't need to convert the world and remove the old way quite yet. Let's use this series to demonstrate the idea, showing a few examples, and the new ONESHOT_STOPPED implementation.
That will allow folks to see the whole idea, get a handle on it and allow us to come to agreement on the general approach.
Once there's agreement on the general approach, we can then cleanup the rest of the clockevent devices, convert them to ->set_dev_mode() and remove ->set_mode().
That's opposite of what Frederic said :)
I thought as I already have the series ready (with fixes suggested), we can send it as is. We just have 8 patches here.
Sure, it's "just 8 patches" but they all will have to change if we decide do do something differently with default cases, or error handling etc. We're still not in complete agreement about what to do there, so dont' try to fix the world until there's agreement about the approach.
If we try to send the ONESHOT series before this goes in, it will make the whole cycle much longer I feel.. People will keep jumping between these series and none would be merged easily..
So, to get these in I though probably first get this series merged and then try the next one..
If we were in agreeement on the general approach (and were confident Thomas was as well), this might be the right approach. But we're not.
Don't know really confused now after your suggestion :)
First let's get a small series in order to get agreement on the approach, and get agreement from Thomas.
FWIW, I've chatted offline with Thomas and he agrees that we should not try to fix the whole world before we agree on the approach.
Kevin
linaro-kernel@lists.linaro.org
-
Frederic Weisbecker -
Kevin Hilman -
Preeti U Murthy -
Viresh Kumar