Hi Thomas/Daniel,
I have incorporated all the improvements Daniel suggested on V1 and so here is V2.
The first patch allows set-state callbacks to be optional, otherwise it leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, these noop-callbacks result in full function calls for nothing really useful.
Rest of the series converts few clockevent drivers to the new set-state interface. This would enable these drivers to use new states (like: ONESHOT_STOPPED, etc.) of a clockevent device (if required), as the set-mode interface is marked obsolete now and wouldn't be expanded to handle new states.
Once all the drivers are migrated to the new interface in future, we can remove the code supporting '->mode' in clockevents core.
Drivers converted in this series are selected based on the diff they generate. These are different diffs we shall have for most of the drivers and any suggestions/improvements for these patches will be applied to other drivers as well.
This is based of latest tip/master from few days back due to dependency on clockevent_state_*() helpers.
Only the first patch is tested on hardware, others are ONLY compile tested.
V1->V2: - New commit: 1/7 (Daniel) - Added all Acks from Daniel - Updated 2/7 to return 0 directly from timer_shutdown() - Naming fixes suggested for kona driver (Ray Jui) - Minor changes in commit logs - Not adding noop-callbacks
Cc: Andres Salomon dilinger@queued.net Cc: bcm-kernel-feedback-list@broadcom.com Cc: Florian Fainelli f.fainelli@gmail.com Cc: Lee Jones lee@kernel.org Cc: Magnus Damm magnus.damm@gmail.com Cc: Marc Zyngier marc.zyngier@arm.com Cc: Maxime Coquelin maxime.coquelin@st.com Cc: Patrice Chotard patrice.chotard@st.com Cc: Ray Jui rjui@broadcom.com Cc: Scott Branden sbranden@broadcom.com Cc: Srinivas Kandagatla srinivas.kandagatla@gmail.com Cc: Stephen Warren swarren@wwwdotorg.org
Viresh Kumar (7): clockevents: Allow set-state callbacks to be optional clocksource: arm_arch_timer: Migrate to new 'set-state' interface clocksource: arm_global_timer: Migrate to new 'set-state' interface clocksource: bcm2835: Migrate to new 'set-state' interface clocksource: bcm_kona: Migrate to new 'set-state' interface clocksource: cs5535: Migrate to new 'set-state' interface clocksource: em_sti: Migrate to new 'set-state' interface
drivers/clocksource/arm_arch_timer.c | 52 ++++++++++++++-------------------- drivers/clocksource/arm_global_timer.c | 37 +++++++++++------------- drivers/clocksource/bcm2835_timer.c | 16 ----------- drivers/clocksource/bcm_kona_timer.c | 17 ++++------- drivers/clocksource/cs5535-clockevt.c | 24 +++++++++------- drivers/clocksource/em_sti.c | 39 +++++++++---------------- kernel/time/clockevents.c | 24 ++++++---------- 7 files changed, 81 insertions(+), 128 deletions(-)
Its mandatory for the drivers to provide set_state_{oneshot|periodic}() (only if related modes are supported) and set_state_shutdown() callbacks today, if they are implementing the new set-state interface.
But this leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, it will lead to a full function call for nothing really useful.
Lets make all set-state callbacks optional.
Suggested-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/clockevents.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 2397b97320d8..37ba06f01818 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -120,19 +120,25 @@ static int __clockevents_switch_state(struct clock_event_device *dev, /* The clockevent device is getting replaced. Shut it down. */
case CLOCK_EVT_STATE_SHUTDOWN: - return dev->set_state_shutdown(dev); + if (dev->set_state_shutdown) + return dev->set_state_shutdown(dev); + return 0;
case CLOCK_EVT_STATE_PERIODIC: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC)) return -ENOSYS; - return dev->set_state_periodic(dev); + if (dev->set_state_periodic) + return dev->set_state_periodic(dev); + return 0;
case CLOCK_EVT_STATE_ONESHOT: /* Core internal bug */ if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return -ENOSYS; - return dev->set_state_oneshot(dev); + if (dev->set_state_oneshot) + return dev->set_state_oneshot(dev); + return 0;
case CLOCK_EVT_STATE_ONESHOT_STOPPED: /* Core internal bug */ @@ -471,18 +477,6 @@ static int clockevents_sanity_check(struct clock_event_device *dev) if (dev->features & CLOCK_EVT_FEAT_DUMMY) return 0;
- /* New state-specific callbacks */ - if (!dev->set_state_shutdown) - return -EINVAL; - - if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && - !dev->set_state_periodic) - return -EINVAL; - - if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) && - !dev->set_state_oneshot) - return -EINVAL; - return 0; }
On 06/12/2015 10:00 AM, Viresh Kumar wrote:
Its mandatory for the drivers to provide set_state_{oneshot|periodic}() (only if related modes are supported) and set_state_shutdown() callbacks today, if they are implementing the new set-state interface.
But this leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, it will lead to a full function call for nothing really useful.
Lets make all set-state callbacks optional.
Suggested-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
As this patch is needed for the other changes in this patchset, is it possible to take it through my tree Thomas ?
On Mon, 29 Jun 2015, Daniel Lezcano wrote:
On 06/12/2015 10:00 AM, Viresh Kumar wrote:
Its mandatory for the drivers to provide set_state_{oneshot|periodic}() (only if related modes are supported) and set_state_shutdown() callbacks today, if they are implementing the new set-state interface.
But this leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, it will lead to a full function call for nothing really useful.
Lets make all set-state callbacks optional.
Suggested-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
As this patch is needed for the other changes in this patchset, is it possible to take it through my tree Thomas ?
I have not other dependencies in that area ATM, so it can bake in yours and I'll pull it over then.
Thanks,
tglx
Migrate arm_arch_timer driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Cc: Marc Zyngier marc.zyngier@arm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/arm_arch_timer.c | 52 +++++++++++++++--------------------- 1 file changed, 22 insertions(+), 30 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135ddbf80..d6e3e49399dd 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -181,44 +181,36 @@ 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, - struct clock_event_device *clk) +static __always_inline int timer_shutdown(const int access, + struct clock_event_device *clk) { unsigned long ctrl; - switch (mode) { - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk); - ctrl &= ~ARCH_TIMER_CTRL_ENABLE; - arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); - break; - default: - break; - } + + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk); + ctrl &= ~ARCH_TIMER_CTRL_ENABLE; + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk); + + return 0; }
-static void arch_timer_set_mode_virt(enum clock_event_mode mode, - struct clock_event_device *clk) +static int arch_timer_shutdown_virt(struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk); + return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); }
-static void arch_timer_set_mode_phys(enum clock_event_mode mode, - struct clock_event_device *clk) +static int arch_timer_shutdown_phys(struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk); + return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); }
-static void arch_timer_set_mode_virt_mem(enum clock_event_mode mode, - struct clock_event_device *clk) +static int arch_timer_shutdown_virt_mem(struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk); + return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); }
-static void arch_timer_set_mode_phys_mem(enum clock_event_mode mode, - struct clock_event_device *clk) +static int arch_timer_shutdown_phys_mem(struct clock_event_device *clk) { - timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk); + return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); }
static __always_inline void set_next_event(const int access, unsigned long evt, @@ -273,11 +265,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_state_shutdown = arch_timer_shutdown_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_state_shutdown = arch_timer_shutdown_phys; clk->set_next_event = arch_timer_set_next_event_phys; } } else { @@ -286,17 +278,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_state_shutdown = arch_timer_shutdown_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_state_shutdown = arch_timer_shutdown_phys_mem; clk->set_next_event = arch_timer_set_next_event_phys_mem; } }
- clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk); + clk->set_state_shutdown(clk);
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); } @@ -506,7 +498,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_state_shutdown(clk); }
static int arch_timer_cpu_notify(struct notifier_block *self,
Migrate arm_global_timer driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Srinivas Kandagatla srinivas.kandagatla@gmail.com Cc: Maxime Coquelin maxime.coquelin@st.com Cc: Patrice Chotard patrice.chotard@st.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/arm_global_timer.c | 37 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index e6833771a716..29ea50ac366a 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -107,26 +107,21 @@ 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, - struct clock_event_device *clk) +static int gt_clockevent_shutdown(struct clock_event_device *evt) { unsigned long ctrl;
- switch (mode) { - case CLOCK_EVT_MODE_PERIODIC: - gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1); - break; - case CLOCK_EVT_MODE_ONESHOT: - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - ctrl = readl(gt_base + GT_CONTROL); - ctrl &= ~(GT_CONTROL_COMP_ENABLE | - GT_CONTROL_IRQ_ENABLE | GT_CONTROL_AUTO_INC); - writel(ctrl, gt_base + GT_CONTROL); - break; - default: - break; - } + ctrl = readl(gt_base + GT_CONTROL); + ctrl &= ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE | + GT_CONTROL_AUTO_INC); + writel(ctrl, gt_base + GT_CONTROL); + return 0; +} + +static int gt_clockevent_set_periodic(struct clock_event_device *evt) +{ + gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1); + return 0; }
static int gt_clockevent_set_next_event(unsigned long evt, @@ -155,7 +150,7 @@ static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id) * the Global Timer flag _after_ having incremented * the Comparator register value to a higher value. */ - if (evt->mode == CLOCK_EVT_MODE_ONESHOT) + if (clockevent_state_oneshot(evt)) gt_compare_set(ULONG_MAX, 0);
writel_relaxed(GT_INT_STATUS_EVENT_FLAG, gt_base + GT_INT_STATUS); @@ -171,7 +166,9 @@ 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_state_shutdown = gt_clockevent_shutdown; + clk->set_state_periodic = gt_clockevent_set_periodic; + clk->set_state_oneshot = gt_clockevent_shutdown; clk->set_next_event = gt_clockevent_set_next_event; clk->cpumask = cpumask_of(cpu); clk->rating = 300; @@ -184,7 +181,7 @@ static int gt_clockevents_init(struct clock_event_device *clk)
static void gt_clockevents_stop(struct clock_event_device *clk) { - gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk); + gt_clockevent_shutdown(clk); disable_percpu_irq(clk->irq); }
Hi Viresh,
On 06/12/2015 10:00 AM, Viresh Kumar wrote:
Migrate arm_global_timer driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Srinivas Kandagatla srinivas.kandagatla@gmail.com Cc: Maxime Coquelin maxime.coquelin@st.com Cc: Patrice Chotard patrice.chotard@st.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/clocksource/arm_global_timer.c | 37 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-)
You can add:
Acked-by: Maxime Coquelin maxime.coquelin@st.com
Thanks! Maxime
On 12-06-15, 10:22, Maxime Coquelin wrote:
You can add:
Acked-by: Maxime Coquelin maxime.coquelin@st.com
Thanks Maxime.
On 12/06/15 09:00, Viresh Kumar wrote:
Migrate arm_global_timer driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Srinivas Kandagatla srinivas.kandagatla@gmail.com Cc: Maxime Coquelin maxime.coquelin@st.com Cc: Patrice Chotard patrice.chotard@st.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/clocksource/arm_global_timer.c | 37 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index e6833771a716..29ea50ac366a 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c
Hi Viresh, Thanks for the patch, Should have been careful with the my last Ack :-)
Acked-by: Srinivas Kandagatla<srinivas.kandagatla@linaro.org mailto:srinivas.kandagatla@linaro.org>
--srini
Migrate bcm2835 driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
We weren't doing anything in the ->set_mode() callback. So, this patch doesn't provide any set-state callbacks.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Stephen Warren swarren@wwwdotorg.org Cc: Lee Jones lee@kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/bcm2835_timer.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/drivers/clocksource/bcm2835_timer.c b/drivers/clocksource/bcm2835_timer.c index 26ed331b1aad..6f2822928963 100644 --- a/drivers/clocksource/bcm2835_timer.c +++ b/drivers/clocksource/bcm2835_timer.c @@ -54,21 +54,6 @@ static u64 notrace bcm2835_sched_read(void) return readl_relaxed(system_clock); }
-static void bcm2835_time_set_mode(enum clock_event_mode mode, - struct clock_event_device *evt_dev) -{ - switch (mode) { - case CLOCK_EVT_MODE_ONESHOT: - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - case CLOCK_EVT_MODE_RESUME: - break; - default: - WARN(1, "%s: unhandled event mode %d\n", __func__, mode); - break; - } -} - static int bcm2835_time_set_next_event(unsigned long event, struct clock_event_device *evt_dev) { @@ -129,7 +114,6 @@ 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_next_event = bcm2835_time_set_next_event; timer->evt.cpumask = cpumask_of(0); timer->act.name = node->name;
On 06/12/2015 02:00 AM, Viresh Kumar wrote:
Migrate bcm2835 driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
We weren't doing anything in the ->set_mode() callback. So, this patch doesn't provide any set-state callbacks.
This generates a panic at boot (on top of 4.1.0-rc8+, which certainly at least booted fine):
[ 0.008586] clocksource timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns [ 0.018080] ------------[ cut here ]------------ [ 0.022843] kernel BUG at kernel/time/clockevents.c:480! [ 0.028299] Internal error: Oops - BUG: 0 [#1] ARM [ 0.033237] CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.0-rc8+ #46 [ 0.039567] Hardware name: BCM2835 [ 0.043092] task: c06fb648 ti: c06f6000 task.ti: c06f6000 [ 0.048668] PC is at clockevents_register_device+0x15c/0x174 [ 0.054481] LR is at clockevents_config_and_register+0x2c/0x30 [ 0.060467] pc : [<c00665e0>] lr : [<c00666b4>] psr: 600001d3 [ 0.060467] sp : c06f7f18 ip : c0066498 fp : c06f7f34 [ 0.072243] r10: cdffc660 r9 : 410fb767 r8 : c06db128 [ 0.077611] r7 : 0000001b r6 : f0003018 r5 : cde38ed4 r4 : cd422020 [ 0.084294] r3 : 00000002 r2 : 00000000 r1 : 000003e7 r0 : cd422020 [ 0.090979] Flags: nZCv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel [ 0.098627] Control: 00c5387d Table: 00004008 DAC: 00000015 [ 0.104521] Process swapper (pid: 0, stack limit = 0xc06f6188) [ 0.110502] Stack: (0xc06f7f18 to 0xc06f8000) [ 0.114993] 7f00: 000003e7 cd422020 [ 0.123354] 7f20: cde38ed4 f0003018 c06f7f4c c06f7f38 c00666b4 c0066490 ffffffff cd422000 [ 0.131715] 7f40: c06f7f7c c06f7f50 c06c3358 c0066694 00000020 c03429ac c06f7f7c 000f4240 [ 0.140076] 7f60: cde38ed4 00000001 c06f7f84 ffffffff c06f7fa4 c06f7f80 c06c3124 c06c321c [ 0.148436] 7f80: 00000001 c06f4ea0 ffffffff 00000000 00000001 c075aea0 c06f7fb4 c06f7fa8 [ 0.156798] 7fa0: c06988e4 c06c30dc c06f7ff4 c06f7fb8 c0695b90 c06988c0 ffffffff ffffffff [ 0.165159] 7fc0: c06956e4 00000000 00000000 c06db128 c075b014 c06f8024 c06db124 c06fc910 [ 0.173518] 7fe0: 00004008 006d9fcc 00000000 c06f7ff8 00008074 c069597c 00000000 00000000 [ 0.181910] [<c00665e0>] (clockevents_register_device) from [<c00666b4>] (clockevents_config_and_register+0x2c/0x30) [ 0.192661] [<c00666b4>] (clockevents_config_and_register) from [<c06c3358>] (bcm2835_timer_init+0x148/0x19c) [ 0.202785] [<c06c3358>] (bcm2835_timer_init) from [<c06c3124>] (clocksource_of_init+0x54/0x98) [ 0.211693] [<c06c3124>] (clocksource_of_init) from [<c06988e4>] (time_init+0x30/0x38) [ 0.219798] [<c06988e4>] (time_init) from [<c0695b90>] (start_kernel+0x220/0x3a4) [ 0.227459] [<c0695b90>] (start_kernel) from [<00008074>] (0x8074) [ 0.233803] Code: e5943078 e3530000 1affffd5 eaffffd2 (e7f001f2) [ 0.240103] ---[ end trace cb88537fdc8fa200 ]--- [ 0.244866] Kernel panic - not syncing: Attempted to kill the idle task! [ 0.251730] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
On 15-06-15, 20:57, Stephen Warren wrote:
On 06/12/2015 02:00 AM, Viresh Kumar wrote:
Migrate bcm2835 driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
We weren't doing anything in the ->set_mode() callback. So, this patch doesn't provide any set-state callbacks.
This generates a panic at boot (on top of 4.1.0-rc8+, which certainly at least booted fine):
[ 0.008586] clocksource timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns [ 0.018080] ------------[ cut here ]------------ [ 0.022843] kernel BUG at kernel/time/clockevents.c:480! [ 0.028299] Internal error: Oops - BUG: 0 [#1] ARM [ 0.033237] CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.0-rc8+ #46 [ 0.039567] Hardware name: BCM2835 [ 0.043092] task: c06fb648 ti: c06f6000 task.ti: c06f6000 [ 0.048668] PC is at clockevents_register_device+0x15c/0x174
This failed the sanity checks of clockevents core. Did you apply the first patch as well? Yes, its very much required.
Also, there were dependencies on the latest tip, prepared for 4.2 merge window and would have been better if you tested on top of that.
But those dependencies are for some helpers which aren't used in this patch. So, it might work over rc8 + the first patch from this series..
In case it doesn't, please test it over tip/master once.
On 06/15/2015 09:17 PM, Viresh Kumar wrote:
On 15-06-15, 20:57, Stephen Warren wrote:
On 06/12/2015 02:00 AM, Viresh Kumar wrote:
Migrate bcm2835 driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
We weren't doing anything in the ->set_mode() callback. So, this patch doesn't provide any set-state callbacks.
This generates a panic at boot (on top of 4.1.0-rc8+, which certainly at least booted fine):
[ 0.008586] clocksource timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns [ 0.018080] ------------[ cut here ]------------ [ 0.022843] kernel BUG at kernel/time/clockevents.c:480! [ 0.028299] Internal error: Oops - BUG: 0 [#1] ARM [ 0.033237] CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.0-rc8+ #46 [ 0.039567] Hardware name: BCM2835 [ 0.043092] task: c06fb648 ti: c06f6000 task.ti: c06f6000 [ 0.048668] PC is at clockevents_register_device+0x15c/0x174
This failed the sanity checks of clockevents core. Did you apply the first patch as well? Yes, its very much required.
Also, there were dependencies on the latest tip, prepared for 4.2 merge window and would have been better if you tested on top of that.
But those dependencies are for some helpers which aren't used in this patch. So, it might work over rc8 + the first patch from this series..
In case it doesn't, please test it over tip/master once.
I see. You didn't Cc me on patch 1, and didn't mention the dependency in this patch. That usually means they're all independent, e.g. the same change in n different drivers.
Anyway, I tracked down the whole series and applied it on top of next-20150615 and everything seems OK (kernel boots, and UART, USB kbd & SD card work), so this patch, Tested-by: Stephen Warren swarren@wwwdotorg.org
On 15-06-15, 22:16, Stephen Warren wrote:
I see. You didn't Cc me on patch 1, and didn't mention the dependency in this patch. That usually means they're all independent, e.g. the same change in n different drivers.
Yeah, I cc'd you on the cover-letter and missed it on the first patch. My fault. Actually during V1 the first patch wasn't there and so there was no dependency, but in V2 this patch came in and I completely forgot you and other guys for that patch.
Anyway, I tracked down the whole series and applied it on top of next-20150615 and everything seems OK (kernel boots, and UART, USB kbd & SD card work), so this patch, Tested-by: Stephen Warren swarren@wwwdotorg.org
Thanks a lot Stephen.
Migrate bcm_kona driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Oneshot callback isn't required as it was empty.
Acked-by: Ray Jui rjui@broadcom.com Cc: Florian Fainelli f.fainelli@gmail.com Cc: Ray Jui rjui@broadcom.com Cc: Scott Branden sbranden@broadcom.com Cc: bcm-kernel-feedback-list@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/bcm_kona_timer.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..e717e87df9bc 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,18 @@ static int kona_timer_set_next_event(unsigned long clc, return 0; }
-static void kona_timer_set_mode(enum clock_event_mode mode, - struct clock_event_device *unused) +static int kona_timer_shutdown(struct clock_event_device *evt) { - switch (mode) { - case CLOCK_EVT_MODE_ONESHOT: - /* by default mode is one shot don't do any thing */ - break; - case CLOCK_EVT_MODE_UNUSED: - case CLOCK_EVT_MODE_SHUTDOWN: - default: - kona_timer_disable_and_clear(timers.tmr_regs); - } + kona_timer_disable_and_clear(timers.tmr_regs); + 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_state_shutdown = kona_timer_shutdown, + .tick_resume = kona_timer_shutdown, };
static void __init kona_timer_clockevents_init(void)
Migrate cs5535 driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Andres Salomon dilinger@queued.net Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/cs5535-clockevt.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/clocksource/cs5535-clockevt.c b/drivers/clocksource/cs5535-clockevt.c index db2105290898..9a7e37cf56b0 100644 --- a/drivers/clocksource/cs5535-clockevt.c +++ b/drivers/clocksource/cs5535-clockevt.c @@ -42,7 +42,6 @@ MODULE_PARM_DESC(irq, "Which IRQ to use for the clock source MFGPT ticks."); * 256 128 .125 512.000 */
-static unsigned int cs5535_tick_mode = CLOCK_EVT_MODE_SHUTDOWN; static struct cs5535_mfgpt_timer *cs5535_event_clock;
/* Selected from the table above */ @@ -77,15 +76,17 @@ 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, - struct clock_event_device *evt) +static int mfgpt_shutdown(struct clock_event_device *evt) { disable_timer(cs5535_event_clock); + return 0; +}
- if (mode == CLOCK_EVT_MODE_PERIODIC) - start_timer(cs5535_event_clock, MFGPT_PERIODIC); - - cs5535_tick_mode = mode; +static int mfgpt_set_periodic(struct clock_event_device *evt) +{ + disable_timer(cs5535_event_clock); + start_timer(cs5535_event_clock, MFGPT_PERIODIC); + return 0; }
static int mfgpt_next_event(unsigned long delta, struct clock_event_device *evt) @@ -97,7 +98,10 @@ 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_state_shutdown = mfgpt_shutdown, + .set_state_periodic = mfgpt_set_periodic, + .set_state_oneshot = mfgpt_shutdown, + .tick_resume = mfgpt_shutdown, .set_next_event = mfgpt_next_event, .rating = 250, }; @@ -113,7 +117,7 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id) /* Turn off the clock (and clear the event) */ disable_timer(cs5535_event_clock);
- if (cs5535_tick_mode == CLOCK_EVT_MODE_SHUTDOWN) + if (clockevent_state_shutdown(&cs5535_clockevent)) return IRQ_HANDLED;
/* Clear the counter */ @@ -121,7 +125,7 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
/* Restart the clock in periodic mode */
- if (cs5535_tick_mode == CLOCK_EVT_MODE_PERIODIC) + if (clockevent_state_periodic(&cs5535_clockevent)) cs5535_mfgpt_write(cs5535_event_clock, MFGPT_REG_SETUP, MFGPT_SETUP_CNTEN | MFGPT_SETUP_CMP2);
Migrate em_sti driver to the new 'set-state' interface provided by the clockevents core, the earlier 'set-mode' interface is marked obsolete now.
This also enables us to implement callbacks for new states of clockevent devices, for example: ONESHOT_STOPPED.
NOTE: This also drops a special check:
if (old_mode == CLOCK_EVT_MODE_ONESHOT) em_sti_stop(p, USER_CLOCKEVENT);
as it doesn't look like that important. This driver only supports ONESHOT and we can only move only to SHUTDOWN from ONESHOT and. Also on second call (on shutdown), em_sti_stop() would return without disabling the device again.
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org Cc: Magnus Damm magnus.damm@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/clocksource/em_sti.c | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-)
diff --git a/drivers/clocksource/em_sti.c b/drivers/clocksource/em_sti.c index dc3c6ee04aaa..7a97a34dba70 100644 --- a/drivers/clocksource/em_sti.c +++ b/drivers/clocksource/em_sti.c @@ -251,33 +251,21 @@ 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, - struct clock_event_device *ced) +static int em_sti_clock_event_shutdown(struct clock_event_device *ced) { struct em_sti_priv *p = ced_to_em_sti(ced); + em_sti_stop(p, USER_CLOCKEVENT); + return 0; +}
- /* deal with old setting first */ - switch (ced->mode) { - case CLOCK_EVT_MODE_ONESHOT: - em_sti_stop(p, USER_CLOCKEVENT); - break; - default: - break; - } +static int em_sti_clock_event_set_oneshot(struct clock_event_device *ced) +{ + struct em_sti_priv *p = ced_to_em_sti(ced);
- switch (mode) { - case CLOCK_EVT_MODE_ONESHOT: - dev_info(&p->pdev->dev, "used for oneshot clock events\n"); - em_sti_start(p, USER_CLOCKEVENT); - clockevents_config(&p->ced, p->rate); - break; - case CLOCK_EVT_MODE_SHUTDOWN: - case CLOCK_EVT_MODE_UNUSED: - em_sti_stop(p, USER_CLOCKEVENT); - break; - default: - break; - } + dev_info(&p->pdev->dev, "used for oneshot clock events\n"); + em_sti_start(p, USER_CLOCKEVENT); + clockevents_config(&p->ced, p->rate); + return 0; }
static int em_sti_clock_event_next(unsigned long delta, @@ -303,11 +291,12 @@ 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_state_shutdown = em_sti_clock_event_shutdown; + ced->set_state_oneshot = em_sti_clock_event_set_oneshot;
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_state_oneshot() */ clockevents_config_and_register(ced, 1, 2, 0xffffffff); }
On 06/12/2015 10:00 AM, Viresh Kumar wrote:
Hi Thomas/Daniel,
I have incorporated all the improvements Daniel suggested on V1 and so here is V2.
The first patch allows set-state callbacks to be optional, otherwise it leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, these noop-callbacks result in full function calls for nothing really useful.
Rest of the series converts few clockevent drivers to the new set-state interface. This would enable these drivers to use new states (like: ONESHOT_STOPPED, etc.) of a clockevent device (if required), as the set-mode interface is marked obsolete now and wouldn't be expanded to handle new states.
Once all the drivers are migrated to the new interface in future, we can remove the code supporting '->mode' in clockevents core.
Drivers converted in this series are selected based on the diff they generate. These are different diffs we shall have for most of the drivers and any suggestions/improvements for these patches will be applied to other drivers as well.
This is based of latest tip/master from few days back due to dependency on clockevent_state_*() helpers.
Applied to my tree for 3.5
Thanks !
-- Daniel
On 06/18/2015 11:10 AM, Daniel Lezcano wrote:
On 06/12/2015 10:00 AM, Viresh Kumar wrote:
Hi Thomas/Daniel,
I have incorporated all the improvements Daniel suggested on V1 and so here is V2.
The first patch allows set-state callbacks to be optional, otherwise it leads to unnecessary noop callbacks for drivers which don't want to implement them. Over that, these noop-callbacks result in full function calls for nothing really useful.
Rest of the series converts few clockevent drivers to the new set-state interface. This would enable these drivers to use new states (like: ONESHOT_STOPPED, etc.) of a clockevent device (if required), as the set-mode interface is marked obsolete now and wouldn't be expanded to handle new states.
Once all the drivers are migrated to the new interface in future, we can remove the code supporting '->mode' in clockevents core.
Drivers converted in this series are selected based on the diff they generate. These are different diffs we shall have for most of the drivers and any suggestions/improvements for these patches will be applied to other drivers as well.
This is based of latest tip/master from few days back due to dependency on clockevent_state_*() helpers.
Applied to my tree for 3.5
Pfff, 4.3
linaro-kernel@lists.linaro.org
-
Daniel Lezcano -
Maxime Coquelin -
srini -
Stephen Warren -
Thomas Gleixner -
Viresh Kumar