| Message ID | 1354297568-26366-7-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/30/12 09:46, Mark Rutland wrote: > Currently, the arch_timer driver is tied to the arm port, as it relies > on code in arch/arm/smp.c to setup and teardown timers as cores are > hotplugged on and off. The timer is registered through an arm-specific > registration mechanism, preventing sharing the driver with the arm64 > port. > > This patch moves the driver to using a cpu notifier instead, making it > easier to port. How does ipi_timer() work after this change? Don't we need it because of FEAT_C3_STOP? > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > --- [snip] > @@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk) > clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk); > } > > -static struct local_timer_ops arch_timer_ops __cpuinitdata = { > - .setup = arch_timer_setup, > - .stop = arch_timer_stop, > -}; > +static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self, > + unsigned long action, void *hcpu) > +{ > + struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt); > + > + switch (action) { > + case CPU_STARTING: > + case CPU_STARTING_FROZEN: > + arch_timer_setup(evt); > + break; > + case CPU_DYING: > + case CPU_DYING_FROZEN: > + arch_timer_stop(evt); > + break; > + } You can save 2 lines with a switch (action & ~CPU_TASKS_FROZEN) here. > + > + return NOTIFY_OK; > +} > > -static struct clock_event_device arch_timer_global_evt; > +static struct notifier_block __cpuinitdata arch_timer_cpu_nb = { __cpuinitdata goes last before the equals sign. > + .notifier_call = arch_timer_cpu_notify, > +}; > > static int __init arch_timer_register(void) > { >
On Fri, Dec 07, 2012 at 10:16:42PM +0000, Stephen Boyd wrote: > On 11/30/12 09:46, Mark Rutland wrote: > > Currently, the arch_timer driver is tied to the arm port, as it relies > > on code in arch/arm/smp.c to setup and teardown timers as cores are > > hotplugged on and off. The timer is registered through an arm-specific > > registration mechanism, preventing sharing the driver with the arm64 > > port. > > > > This patch moves the driver to using a cpu notifier instead, making it > > easier to port. > > How does ipi_timer() work after this change? Don't we need it because of > FEAT_C3_STOP? The unfortunate answer is it doesn't, and we'll need broadcast for any systems where the timers turn off in low power states. I'll take a look at decoupling the broadcast mechanism from the drivers, it's not really a property of the clock hardware and it would be nice to split it. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > [snip] > > @@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk) > > clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk); > > } > > > > -static struct local_timer_ops arch_timer_ops __cpuinitdata = { > > - .setup = arch_timer_setup, > > - .stop = arch_timer_stop, > > -}; > > +static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self, > > + unsigned long action, void *hcpu) > > +{ > > + struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt); > > + > > + switch (action) { > > + case CPU_STARTING: > > + case CPU_STARTING_FROZEN: > > + arch_timer_setup(evt); > > + break; > > + case CPU_DYING: > > + case CPU_DYING_FROZEN: > > + arch_timer_stop(evt); > > + break; > > + } > > You can save 2 lines with a switch (action & ~CPU_TASKS_FROZEN) here. Sure. I'll fold that in. > > + > > + return NOTIFY_OK; > > +} > > > > -static struct clock_event_device arch_timer_global_evt; > > +static struct notifier_block __cpuinitdata arch_timer_cpu_nb = { > > __cpuinitdata goes last before the equals sign. I'll fix that up. > > + .notifier_call = arch_timer_cpu_notify, > > +}; > > > > static int __init arch_timer_register(void) > > { > > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation > > Thanks, Mark.
Stephen, On Mon, Dec 10, 2012 at 11:09:41AM +0000, Mark Rutland wrote: > On Fri, Dec 07, 2012 at 10:16:42PM +0000, Stephen Boyd wrote: > > On 11/30/12 09:46, Mark Rutland wrote: > > > Currently, the arch_timer driver is tied to the arm port, as it relies > > > on code in arch/arm/smp.c to setup and teardown timers as cores are > > > hotplugged on and off. The timer is registered through an arm-specific > > > registration mechanism, preventing sharing the driver with the arm64 > > > port. > > > > > > This patch moves the driver to using a cpu notifier instead, making it > > > easier to port. > > > > How does ipi_timer() work after this change? Don't we need it because of > > FEAT_C3_STOP? > > The unfortunate answer is it doesn't, and we'll need broadcast for any systems > where the timers turn off in low power states. > > I'll take a look at decoupling the broadcast mechanism from the drivers, it's > not really a property of the clock hardware and it would be nice to split it. I've had a go at splitting the broadcast mechanism in another series: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137929.html I'm evidently not proficient with git send-email -- I'd intended for you to be on Cc. Thanks, Mark
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c index fda8382..10e1657 100644 --- a/arch/arm/kernel/arch_timer.c +++ b/arch/arm/kernel/arch_timer.c @@ -21,7 +21,6 @@ #include <linux/io.h> #include <asm/delay.h> -#include <asm/localtimer.h> #include <asm/arch_timer.h> #include <asm/sched_clock.h> @@ -37,7 +36,7 @@ enum ppi_nr { static int arch_timer_ppi[MAX_TIMER_PPI]; -static struct clock_event_device __percpu **arch_timer_evt; +static struct clock_event_device __percpu *arch_timer_evt; static struct delay_timer arch_delay_timer; static bool arch_timer_use_virtual = true; @@ -63,14 +62,14 @@ static irqreturn_t inline timer_handler(const int access, static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id) { - struct clock_event_device *evt = *(struct clock_event_device **)dev_id; + struct clock_event_device *evt = dev_id; return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt); } static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id) { - struct clock_event_device *evt = *(struct clock_event_device **)dev_id; + struct clock_event_device *evt = dev_id; return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt); } @@ -141,13 +140,13 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk) clk->set_next_event = arch_timer_set_next_event_phys; } + clk->cpumask = cpumask_of(smp_processor_id()); + clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, NULL); clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff); - *__this_cpu_ptr(arch_timer_evt) = clk; - if (arch_timer_use_virtual) enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0); else { @@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk) clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk); } -static struct local_timer_ops arch_timer_ops __cpuinitdata = { - .setup = arch_timer_setup, - .stop = arch_timer_stop, -}; +static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self, + unsigned long action, void *hcpu) +{ + struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt); + + switch (action) { + case CPU_STARTING: + case CPU_STARTING_FROZEN: + arch_timer_setup(evt); + break; + case CPU_DYING: + case CPU_DYING_FROZEN: + arch_timer_stop(evt); + break; + } + + return NOTIFY_OK; +} -static struct clock_event_device arch_timer_global_evt; +static struct notifier_block __cpuinitdata arch_timer_cpu_nb = { + .notifier_call = arch_timer_cpu_notify, +}; static int __init arch_timer_register(void) { @@ -261,7 +276,7 @@ static int __init arch_timer_register(void) if (err) goto out; - arch_timer_evt = alloc_percpu(struct clock_event_device *); + arch_timer_evt = alloc_percpu(struct clock_event_device); if (!arch_timer_evt) { err = -ENOMEM; goto out; @@ -297,20 +312,13 @@ static int __init arch_timer_register(void) goto out_free; } - err = local_timer_register(&arch_timer_ops); - if (err) { - /* - * We couldn't register as a local timer (could be - * because we're on a UP platform, or because some - * other local timer is already present...). Try as a - * global timer instead. - */ - arch_timer_global_evt.cpumask = cpumask_of(0); - err = arch_timer_setup(&arch_timer_global_evt); - } + err = register_cpu_notifier(&arch_timer_cpu_nb); if (err) goto out_free_irq; + /* Immediately configure the timer on the boot CPU */ + arch_timer_setup(this_cpu_ptr(arch_timer_evt)); + /* Use the architected timer for the delay loop. */ arch_delay_timer.read_current_timer = &arch_timer_read_current_timer; arch_delay_timer.freq = arch_timer_rate;