Hi Thomas/Daniel,
This 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 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.
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 (6): 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 | 61 +++++++++++++++++----------------- drivers/clocksource/arm_global_timer.c | 37 ++++++++++----------- drivers/clocksource/bcm2835_timer.c | 17 +++------- drivers/clocksource/bcm_kona_timer.c | 24 ++++++------- drivers/clocksource/cs5535-clockevt.c | 24 +++++++------ drivers/clocksource/em_sti.c | 39 ++++++++-------------- 6 files changed, 92 insertions(+), 110 deletions(-)
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 | 61 ++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135ddbf80..d0b0cf43b981 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -181,44 +181,44 @@ 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 void 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); }
-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); + timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); + return 0; }
-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); + timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); + return 0; }
-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); + timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); + return 0; }
-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); + timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); + return 0; +} + +static int arch_timer_set_oneshot(struct clock_event_device *clk) +{ + /* Nothing to do here */ + return 0; }
static __always_inline void set_next_event(const int access, unsigned long evt, @@ -273,11 +273,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 +286,18 @@ 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_oneshot = arch_timer_set_oneshot; + clk->set_state_shutdown(clk);
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); } @@ -506,7 +507,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,
On 06/08/2015 03:40 PM, Viresh Kumar wrote:
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 | 61 ++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135ddbf80..d0b0cf43b981 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -181,44 +181,44 @@ 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 void timer_shutdown(const int access,
struct clock_event_device *clk)
[ ... ]
Instead of returning zero in the functions 'timer_shutdown_virt|phys|virt_mem|phys_mem', do it here directly.
}
-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);
- timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
- return 0;
As suggested above:
return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
}
[ ... ]
@@ -286,17 +286,18 @@ 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;
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_oneshot = arch_timer_set_oneshot;
- clk->set_state_shutdown(clk);
Why don't you call clockevent_shutdown(clk) ?
There is some initialization there, no ?
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); } @@ -506,7 +507,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); }
Ditto.
On 11-06-15, 14:23, Daniel Lezcano wrote:
[ ... ]
Instead of returning zero in the functions 'timer_shutdown_virt|phys|virt_mem|phys_mem', do it here directly.
As suggested above:
return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk);
}
Sure.
@@ -286,17 +286,18 @@ static void __arch_timer_setup(unsigned type,
- clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
- clk->set_state_oneshot = arch_timer_set_oneshot;
- clk->set_state_shutdown(clk);
Why don't you call clockevent_shutdown(clk) ?
I haven't changed the behavior as the current code doesn't do it that way. If we want to change it, then it should be a separate patch.
Having said that, in this particular instance the device is not yet registered with the core and we probably didn't wanted 'mode' to be set to SHUTDOWN. That would have also hit a WARN/BUG as clockevents core expected the device to be in UNUSED mode at registration.
There is some initialization there, no ?
We don't want it at this point of time.
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); } @@ -506,7 +507,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);
}
Ditto.
This is called from the CPU_DYING notifier, not sure what the intention was behind not letting the clockevents core about mode change.
But if it has to change, then it should happen in a separate patch.
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.
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); }
On 06/08/2015 03:40 PM, 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.
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 ++++++++++++++++------------------
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
- 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;
nit: it sounds weird to use the same function as the purpose of the patch is use the new API which is to ventilate those functions (anyway ...)
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); }
On 11-06-15, 16:49, Daniel Lezcano wrote:
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
Thanks :)
- 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;
nit: it sounds weird to use the same function as the purpose of the patch is use the new API which is to ventilate those functions (anyway ...)
Hmm, but there is no point creating two routines to do exactly the same thing. And this is making it evident that we actually shutdown the device in oneshot-state request. And enable it only when the next event is programmed.
On 06/11/2015 04:56 PM, Viresh Kumar wrote:
On 11-06-15, 16:49, Daniel Lezcano wrote:
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
Thanks :)
- 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;
nit: it sounds weird to use the same function as the purpose of the patch is use the new API which is to ventilate those functions (anyway ...)
Hmm, but there is no point creating two routines to do exactly the same thing. And this is making it evident that we actually shutdown the device in oneshot-state request. And enable it only when the next event is programmed.
Yes, this is really a detail.
On 08/06/15 14:40, 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.
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,
Acked-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
--srini
On 12-06-15, 09:39, Srinivas Kandagatla wrote:
Acked-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
Thanks Srinivas, but you gave this for the V1 version. Can you please do that again for V2 ?
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.
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 | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/clocksource/bcm2835_timer.c b/drivers/clocksource/bcm2835_timer.c index 26ed331b1aad..b59b8e1ba503 100644 --- a/drivers/clocksource/bcm2835_timer.c +++ b/drivers/clocksource/bcm2835_timer.c @@ -54,19 +54,9 @@ 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) +static int bcm2835_time_noop(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: - WARN(1, "%s: unhandled event mode %d\n", __func__, mode); - break; - } + return 0; }
static int bcm2835_time_set_next_event(unsigned long event, @@ -129,7 +119,8 @@ 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_state_shutdown = bcm2835_time_noop; + timer->evt.set_state_oneshot = bcm2835_time_noop; timer->evt.set_next_event = bcm2835_time_set_next_event; timer->evt.cpumask = cpumask_of(0); timer->act.name = node->name;
On 06/08/2015 03:40 PM, 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.
Please mention in the log you removed the RESUME because it does nothing and the core code will just ignore it.
Cc: Stephen Warren swarren@wwwdotorg.org Cc: Lee Jones lee@kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/clocksource/bcm2835_timer.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/clocksource/bcm2835_timer.c b/drivers/clocksource/bcm2835_timer.c index 26ed331b1aad..b59b8e1ba503 100644 --- a/drivers/clocksource/bcm2835_timer.c +++ b/drivers/clocksource/bcm2835_timer.c @@ -54,19 +54,9 @@ 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)
+static int bcm2835_time_noop(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:
WARN(1, "%s: unhandled event mode %d\n", __func__, mode);break;- }
return 0; }
static int bcm2835_time_set_next_event(unsigned long event,
@@ -129,7 +119,8 @@ 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_state_shutdown = bcm2835_time_noop;
- timer->evt.set_state_oneshot = bcm2835_time_noop;
The patch is ok but there is an inconsistency on how are handled the different callbacks. tick_resume can be omitted but not state_shutdown and state_oneshot forcing the drivers to define empty functions.
timer->evt.set_next_event = bcm2835_time_set_next_event; timer->evt.cpumask = cpumask_of(0); timer->act.name = node->name;
On 11-06-15, 17:25, Daniel Lezcano wrote:
Please mention in the log you removed the RESUME because it does nothing and the core code will just ignore it.
Okay.
The patch is ok but there is an inconsistency on how are handled the different callbacks. tick_resume can be omitted but not state_shutdown and state_oneshot forcing the drivers to define empty functions.
It was done because they are thought to be compulsory, but even I am not sure if we should make all callbacks as optional.
On 06/11/2015 06:00 PM, Viresh Kumar wrote:
On 11-06-15, 17:25, Daniel Lezcano wrote:
Please mention in the log you removed the RESUME because it does nothing and the core code will just ignore it.
Okay.
The patch is ok but there is an inconsistency on how are handled the different callbacks. tick_resume can be omitted but not state_shutdown and state_oneshot forcing the drivers to define empty functions.
It was done because they are thought to be compulsory, but even I am not sure if we should make all callbacks as optional.
Yes, I think it would make sense to ignore NULL callback in __clockevents_set_state, otherwise we will end up to fill the different drivers with duplicated noop functions.
Hi Thomas,
On 11-06-15, 18:13, Daniel Lezcano wrote:
Yes, I think it would make sense to ignore NULL callback in __clockevents_set_state, otherwise we will end up to fill the different drivers with duplicated noop functions.
Do you suggest the same ?
Thanks.
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.
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 | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..4efca5f7d443 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,25 @@ 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 cona_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 int cona_timer_set_oneshot(struct clock_event_device *evt) +{ + /* By default mode is one shot, don't do any thing */ + 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 = cona_timer_shutdown, + .set_state_oneshot = cona_timer_set_oneshot, + .tick_resume = cona_timer_shutdown, };
static void __init kona_timer_clockevents_init(void)
Hi Viresh,
On 6/8/2015 6:40 AM, Viresh Kumar wrote:
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.
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 | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..4efca5f7d443 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,25 @@ 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 cona_timer_shutdown(struct clock_event_device *evt)
There's a typo here. It's "kona" not "cona"
{
- 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 int cona_timer_set_oneshot(struct clock_event_device *evt)
Same typo.
+{
- /* By default mode is one shot, don't do any thing */
- 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 = cona_timer_shutdown,
- .set_state_oneshot = cona_timer_set_oneshot,
- .tick_resume = cona_timer_shutdown,
}; static void __init kona_timer_clockevents_init(void)
Thanks,
Ray
On 08-06-15, 09:09, Ray Jui wrote:
There's a typo here. It's "kona" not "cona"
Crap, how does this look:
From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 27 Feb 2015 13:39:52 +0530 Subject: [PATCH] clocksource: bcm_kona: Migrate to new 'set-state' interface
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.
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 | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..9df3f1e3fd0e 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,25 @@ 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 int kona_timer_set_oneshot(struct clock_event_device *evt) +{ + /* By default mode is one shot, don't do any thing */ + 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, + .set_state_oneshot = kona_timer_set_oneshot, + .tick_resume = kona_timer_shutdown, };
static void __init kona_timer_clockevents_init(void)
This patch looks fine to me.
Thanks,
Ray
On 6/8/2015 9:22 AM, Viresh Kumar wrote:
On 08-06-15, 09:09, Ray Jui wrote:
There's a typo here. It's "kona" not "cona"
Crap, how does this look:
From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 27 Feb 2015 13:39:52 +0530 Subject: [PATCH] clocksource: bcm_kona: Migrate to new 'set-state' interface
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.
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 | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..9df3f1e3fd0e 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,25 @@ 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 int kona_timer_set_oneshot(struct clock_event_device *evt) +{
- /* By default mode is one shot, don't do any thing */
- 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,
- .set_state_oneshot = kona_timer_set_oneshot,
- .tick_resume = kona_timer_shutdown,
}; static void __init kona_timer_clockevents_init(void)
Forgot to ACK the patch...
On 6/8/2015 9:30 AM, Ray Jui wrote:
This patch looks fine to me.
Thanks,
Ray
On 6/8/2015 9:22 AM, Viresh Kumar wrote:
On 08-06-15, 09:09, Ray Jui wrote:
There's a typo here. It's "kona" not "cona"
Crap, how does this look:
From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 27 Feb 2015 13:39:52 +0530 Subject: [PATCH] clocksource: bcm_kona: Migrate to new 'set-state' interface
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.
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 | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c index f1e33d08dd83..9df3f1e3fd0e 100644 --- a/drivers/clocksource/bcm_kona_timer.c +++ b/drivers/clocksource/bcm_kona_timer.c @@ -127,25 +127,25 @@ 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 int kona_timer_set_oneshot(struct clock_event_device *evt) +{
- /* By default mode is one shot, don't do any thing */
- 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,
- .set_state_oneshot = kona_timer_set_oneshot,
- .tick_resume = kona_timer_shutdown,
}; static void __init kona_timer_clockevents_init(void)
Acked-by: Ray Jui rjui@broadcom.com
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.
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);
On 06/08/2015 03:40 PM, Viresh Kumar wrote:
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.
Cc: Andres Salomon dilinger@queued.net Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
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.
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/08/2015 03:40 PM, Viresh Kumar wrote:
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.
Cc: Magnus Damm magnus.damm@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
[ ... ]
On 06/08/2015 03:40 PM, Viresh Kumar wrote:
Hi Thomas/Daniel,
This 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 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.
I suggest you use the kernel continuous integration [1] to test the patches, it is probable some of the hardware is there.
-- Daniel
linaro-kernel@lists.linaro.org
-
Daniel Lezcano -
Ray Jui -
Srinivas Kandagatla -
Viresh Kumar