| Message ID | 1353349867-28494-11-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 19, 2012 at 7:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > These fields duplicate e.g. struct clock_event_device's suspend and > resume fields, so remove them now that nothing is using them. The aim > is to remove all fields from struct sys_timer except .init, then replace > the ARM machine descriptor's .timer field with a .init_time function > instead, and delete struct sys_timer. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Oh and there was this comment/TODO: On Mon, Nov 19, 2012 at 7:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > @@ -17,15 +17,6 @@ > * Initialise the kernels jiffy timer source, claim interrupt > * using setup_irq. This is called early on during initialisation > * while interrupts are still disabled on the local CPU. > - * - suspend > - * Suspend the kernel jiffy timer source, if necessary. This > - * is called with interrupts disabled, after all normal devices > - * have been suspended. If no action is required, set this to > - * NULL. > - * - resume > - * Resume the kernel jiffy timer source, if necessary. This > - * is called with interrupts disabled before any normal devices > - * are resumed. If no action is required, set this to NULL. > * - offset > * Return the timer offset in microseconds since the last timer > * interrupt. Note: this must take account of any unprocessed > @@ -33,8 +24,6 @@ > */ > struct sys_timer { > void (*init)(void); > - void (*suspend)(void); > - void (*resume)(void); > }; So from the above it is quite clear that the sys_timer is breaking the suspend_noirq/resume_noirq naming convention from runtime PM as IRQs are disabled on these paths. The same goes for struct clock_event_device ... So while this looks just as bad after as before the patch, we should take a mental notice to rename the .suspend and .resume hooks in the clock_event_device to .suspend_noirq and .resume_noirq at some point. I was thinking that if your patch set is introducing a plethora of new users of these hooks we should maybe stick a patch at the beginning of the series renaming the hooks to *_noirq, but if it's a major obstacle it can surely wait. Yours, Linus Walleij
On 11/21/2012 01:28 AM, Linus Walleij wrote: > Oh and there was this comment/TODO: > > On Mon, Nov 19, 2012 at 7:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> @@ -17,15 +17,6 @@ >> * Initialise the kernels jiffy timer source, claim interrupt >> * using setup_irq. This is called early on during initialisation >> * while interrupts are still disabled on the local CPU. >> - * - suspend >> - * Suspend the kernel jiffy timer source, if necessary. This >> - * is called with interrupts disabled, after all normal devices >> - * have been suspended. If no action is required, set this to >> - * NULL. >> - * - resume >> - * Resume the kernel jiffy timer source, if necessary. This >> - * is called with interrupts disabled before any normal devices >> - * are resumed. If no action is required, set this to NULL. >> * - offset >> * Return the timer offset in microseconds since the last timer >> * interrupt. Note: this must take account of any unprocessed >> @@ -33,8 +24,6 @@ >> */ >> struct sys_timer { >> void (*init)(void); >> - void (*suspend)(void); >> - void (*resume)(void); >> }; > > So from the above it is quite clear that the sys_timer is breaking > the suspend_noirq/resume_noirq naming convention from > runtime PM as IRQs are disabled on these paths. > > The same goes for struct clock_event_device ... > > So while this looks just as bad after as before the patch, > we should take a mental notice to rename the .suspend > and .resume hooks in the clock_event_device to > .suspend_noirq and .resume_noirq at some point. > > I was thinking that if your patch set is introducing a > plethora of new users of these hooks we should maybe > stick a patch at the beginning of the series renaming the > hooks to *_noirq, but if it's a major obstacle it can surely wait. I think I'd prefer to keep that rename separate and do it later; while this series does introduce a few new users of the type, there are many more pre-existing users. Renaming all the users would end up making this series potentially conflict with addition of new users in tip.git or elsewhere, whereas any possible current conflicts from this series should be resolvable in arm-soc pretty easily I hope, so I'd rather create a separate series that does the rename, and probably apply it to tip.git, probably for 3.10?
On Mon, Nov 26, 2012 at 8:25 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 11/21/2012 01:28 AM, Linus Walleij wrote: >> Oh and there was this comment/TODO: >> I was thinking that if your patch set is introducing a >> plethora of new users of these hooks we should maybe >> stick a patch at the beginning of the series renaming the >> hooks to *_noirq, but if it's a major obstacle it can surely wait. > > I think I'd prefer to keep that rename separate and do it later; while > this series does introduce a few new users of the type, there are many > more pre-existing users. Renaming all the users would end up making this > series potentially conflict with addition of new users in tip.git or > elsewhere, whereas any possible current conflicts from this series > should be resolvable in arm-soc pretty easily I hope, so I'd rather > create a separate series that does the rename, and probably apply it to > tip.git, probably for 3.10? Sure no big deal. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/arch/arm/include/asm/mach/time.h b/arch/arm/include/asm/mach/time.h index cac8d9c..d316d76 100644 --- a/arch/arm/include/asm/mach/time.h +++ b/arch/arm/include/asm/mach/time.h @@ -17,15 +17,6 @@ * Initialise the kernels jiffy timer source, claim interrupt * using setup_irq. This is called early on during initialisation * while interrupts are still disabled on the local CPU. - * - suspend - * Suspend the kernel jiffy timer source, if necessary. This - * is called with interrupts disabled, after all normal devices - * have been suspended. If no action is required, set this to - * NULL. - * - resume - * Resume the kernel jiffy timer source, if necessary. This - * is called with interrupts disabled before any normal devices - * are resumed. If no action is required, set this to NULL. * - offset * Return the timer offset in microseconds since the last timer * interrupt. Note: this must take account of any unprocessed @@ -33,8 +24,6 @@ */ struct sys_timer { void (*init)(void); - void (*suspend)(void); - void (*resume)(void); }; extern void timer_tick(void); diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c index ea36bfa..0b51a7c 100644 --- a/arch/arm/kernel/time.c +++ b/arch/arm/kernel/time.c @@ -21,7 +21,6 @@ #include <linux/timex.h> #include <linux/errno.h> #include <linux/profile.h> -#include <linux/syscore_ops.h> #include <linux/timer.h> #include <linux/irq.h> @@ -119,39 +118,6 @@ int __init register_persistent_clock(clock_access_fn read_boot, return -EINVAL; } -#if defined(CONFIG_PM) && !defined(CONFIG_GENERIC_CLOCKEVENTS) -static int timer_suspend(void) -{ - if (system_timer->suspend) - system_timer->suspend(); - - return 0; -} - -static void timer_resume(void) -{ - if (system_timer->resume) - system_timer->resume(); -} -#else -#define timer_suspend NULL -#define timer_resume NULL -#endif - -static struct syscore_ops timer_syscore_ops = { - .suspend = timer_suspend, - .resume = timer_resume, -}; - -static int __init timer_init_syscore_ops(void) -{ - register_syscore_ops(&timer_syscore_ops); - - return 0; -} - -device_initcall(timer_init_syscore_ops); - void __init time_init(void) { system_timer = machine_desc->timer;