* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug [not found] <AANLkTi=fJeOUhH0xUJF8qx-fTJ5popE7ZuVxDZXpTH-i@mail.gmail.com> @ 2010-11-29 9:54 ` Vincent Guittot 2010-11-29 10:41 ` Russell King - ARM Linux 0 siblings, 1 reply; 10+ messages in thread From: Vincent Guittot @ 2010-11-29 9:54 UTC (permalink / raw) To: linux-arm-kernel This patch extracts the common code of the cpu hotplug feature across arm platforms. The goal is to only keep the specific stuff of the platform in the sub-architecture. I have created a hotplug.c file in the ?arm/common directory after studying the cpu hotplug code of omap2, realview, s5pv310, ux500 and tegra. I have extracted 3 main platform dependent functions: ?-platform_enter_lowpower which prepares the platform for low power. ?-platform_do_lowpower on which the cpu will loop until it becomes really plugged (spurious wake up). This function must returned the cpu Id in order to leave the unplug state. ?-platform_leave_lowpower which restore the platform context. ?An ux500 patch is available which uses the common/hotplug.c code. This patch is quite short because the idle / power down functions are not yet upstreamed Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> --- ?arch/arm/common/Kconfig ? ? ? ?| ? ?4 ++ ?arch/arm/common/Makefile ? ? ? | ? ?1 + ?arch/arm/common/hotplug.c ? ? ?| ? 85 ++++++++++++++++++++++++++++++++++++++++ ?arch/arm/include/asm/hotplug.h | ? ?8 ++++ ?4 files changed, 98 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/common/hotplug.c ?create mode 100644 arch/arm/include/asm/hotplug.h diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig index 0a34c81..b1c5f6b 100644 --- a/arch/arm/common/Kconfig +++ b/arch/arm/common/Kconfig @@ -41,3 +41,7 @@ config SHARP_SCOOP ?config COMMON_CLKDEV ? ? ? ?bool ? ? ? ?select HAVE_CLK + +config USE_COMMON_ARM_HOTPLUG + ? ? ? bool + ? ? ? depends on HOTPLUG_CPU diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile index e6e8664..d8a66d7 100644 --- a/arch/arm/common/Makefile +++ b/arch/arm/common/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_ARCH_IXP2000) ? ?+= uengine.o ?obj-$(CONFIG_ARCH_IXP23XX) ? ? += uengine.o ?obj-$(CONFIG_PCI_HOST_ITE8152) ?+= it8152.o ?obj-$(CONFIG_COMMON_CLKDEV) ? ?+= clkdev.o +obj-$(CONFIG_USE_COMMON_ARM_HOTPLUG) ? ? ? ? ? += hotplug.o diff --git a/arch/arm/common/hotplug.c b/arch/arm/common/hotplug.c new file mode 100644 index 0000000..05f39f9 --- /dev/null +++ b/arch/arm/common/hotplug.c @@ -0,0 +1,85 @@ +/* + * Copyright (C) STMicroelectronics 2009 + * Copyright (C) ST-Ericsson SA 2010 + * + * License Terms: GNU General Public License v2 + * ? ? Based on ARM realview platform + * + * Author: Sundar Iyer <sundar.iyer@stericsson.com> + * Author: Vincent Guittot <vincent.guittot@stericsson.com> + * + */ +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/smp.h> +#include <linux/completion.h> + +#include <asm/cacheflush.h> +#include <asm/hotplug.h> + +static DECLARE_COMPLETION(cpu_killed); + +int platform_cpu_kill(unsigned int cpu) +{ + ? ? ? int err; + ? ? ? err = wait_for_completion_timeout(&cpu_killed, 5000); +#ifdef DEBUG + ? ? ? printk(KERN_INFO "platform_cpu_kill %d err %d\n", cpu, err); +#endif + ? ? ? return err; +} + +/* + * architecture-specific code to shutdown a CPU + * + * Called with IRQs disabled + */ +void platform_cpu_die(unsigned int cpu) +{ + ? ? ? unsigned long spurious = 0; +#ifdef DEBUG + ? ? ? unsigned int this_cpu = hard_smp_processor_id(); + + ? ? ? if (cpu != this_cpu) { + ? ? ? ? ? ? ? printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n", + ? ? ? ? ? ? ? ? ? ? ? ? ?this_cpu, cpu); + ? ? ? ? ? ? ? BUG(); + ? ? ? } + + ? ? ? printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); +#endif + ? ? ? complete(&cpu_killed); + + ? ? ? flush_cache_all(); + ? ? ? wmb(); + + ? ? ? platform_enter_lowpower(cpu); + + ? ? ? for (;;) { + + ? ? ? ? ? ? ? if (platform_do_lowpower(cpu) == cpu) { + ? ? ? ? ? ? ? ? ? ? ? /* + ? ? ? ? ? ? ? ? ? ? ? ?* OK, proper wakeup, we're done + ? ? ? ? ? ? ? ? ? ? ? ?*/ + ? ? ? ? ? ? ? ? ? ? ? break; + ? ? ? ? ? ? ? } + ? ? ? ? ? ? ? spurious++; + ? ? ? } + + ? ? ? platform_leave_lowpower(cpu); + +#ifdef DEBUG + ? ? ? printk(KERN_NOTICE "CPU%u: wake up (%lu spurious wake up)\n", + ? ? ? ? ? ? ? ? ? ? ? cpu, spurious); +#endif + +} + +int platform_cpu_disable(unsigned int cpu) +{ + ? ? ? /* + ? ? ? ?* we don't allow CPU 0 to be shutdown (it is still too special + ? ? ? ?* e.g. clock tick interrupts) + ? ? ? ?*/ + ? ? ? return cpu == 0 ? -EPERM : 0; +} diff --git a/arch/arm/include/asm/hotplug.h b/arch/arm/include/asm/hotplug.h new file mode 100644 index 0000000..b38e737 --- /dev/null +++ b/arch/arm/include/asm/hotplug.h @@ -0,0 +1,8 @@ +#ifndef __ASM_MACH_HOTPLUG_H +#define __ASM_MACH_HOTPLUG_H + +extern void platform_enter_lowpower(unsigned int cpu); +extern int platform_do_lowpower(unsigned int cpu); +extern void platform_leave_lowpower(unsigned int cpu); + +#endif -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-29 9:54 ` [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug Vincent Guittot @ 2010-11-29 10:41 ` Russell King - ARM Linux 2010-11-29 17:27 ` Vincent Guittot 0 siblings, 1 reply; 10+ messages in thread From: Russell King - ARM Linux @ 2010-11-29 10:41 UTC (permalink / raw) To: linux-arm-kernel On Mon, Nov 29, 2010 at 10:54:35AM +0100, Vincent Guittot wrote: > This patch extracts the common code of the cpu hotplug feature across > arm platforms. The goal is to only keep the specific stuff of the > platform in the sub-architecture. I have created a hotplug.c file in > the ?arm/common directory after studying the cpu hotplug code of > omap2, realview, s5pv310, ux500 and tegra. I have extracted 3 main > platform dependent functions: > ?-platform_enter_lowpower which prepares the platform for low power. > ?-platform_do_lowpower on which the cpu will loop until it becomes > really plugged (spurious wake up). This function must returned the cpu > Id in order to leave the unplug state. > ?-platform_leave_lowpower which restore the platform context. I still do not like this patch. The only thing that is worth doing is this. This leaves less than 256 bytes of object code in the Realview hotplug.c, most of which is the stuff to handle the low power mode which you haven't dealt with in your patch either. I see no point in adding another API on top of the already existing and simple API. diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 8c19595..e8856aa 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -24,6 +24,7 @@ #include <linux/irq.h> #include <linux/percpu.h> #include <linux/clockchips.h> +#include <linux/completion.h> #include <asm/atomic.h> #include <asm/cacheflush.h> @@ -252,12 +253,19 @@ int __cpu_disable(void) return 0; } +static DECLARE_COMPLETION(cpu_died); + /* * called on the thread which is asking for a CPU to be shutdown - * waits until shutdown has completed, or it is timed out. */ void __cpu_die(unsigned int cpu) { + if (wait_for_completion_timeout(&cpu_died, 5000)) { + printk("CPU%u: cpu didn't die\n"); + return; + } + if (!platform_cpu_kill(cpu)) printk("CPU%u: unable to kill\n", cpu); } @@ -277,6 +285,9 @@ void __ref cpu_die(void) local_irq_disable(); idle_task_exit(); + printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); + complete(&cpu_died); + /* * actual CPU shutdown procedure is at least platform (if not * CPU) specific diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c index f95521a..7d58c16 100644 --- a/arch/arm/mach-realview/hotplug.c +++ b/arch/arm/mach-realview/hotplug.c @@ -11,14 +11,11 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); - static inline void cpu_enter_lowpower(void) { unsigned int v; @@ -95,7 +92,7 @@ static inline void platform_do_lowpower(unsigned int cpu) int platform_cpu_kill(unsigned int cpu) { - return wait_for_completion_timeout(&cpu_killed, 5000); + return 1; } /* @@ -115,9 +112,6 @@ void platform_cpu_die(unsigned int cpu) } #endif - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* * we're ready for shutdown now, so do it */ diff --git a/arch/arm/mach-s5pv310/hotplug.c b/arch/arm/mach-s5pv310/hotplug.c index 03652c3..d7be70a 100644 --- a/arch/arm/mach-s5pv310/hotplug.c +++ b/arch/arm/mach-s5pv310/hotplug.c @@ -13,14 +13,11 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); - static inline void cpu_enter_lowpower(void) { unsigned int v; @@ -98,7 +95,7 @@ static inline void platform_do_lowpower(unsigned int cpu) int platform_cpu_kill(unsigned int cpu) { - return wait_for_completion_timeout(&cpu_killed, 5000); + return 1; } /* @@ -118,9 +115,6 @@ void platform_cpu_die(unsigned int cpu) } #endif - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* * we're ready for shutdown now, so do it */ diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c index 8e7f115..ecaa41c 100644 --- a/arch/arm/mach-tegra/hotplug.c +++ b/arch/arm/mach-tegra/hotplug.c @@ -11,12 +11,9 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> -static DECLARE_COMPLETION(cpu_killed); - static inline void cpu_enter_lowpower(void) { unsigned int v; @@ -94,7 +91,7 @@ static inline void platform_do_lowpower(unsigned int cpu) int platform_cpu_kill(unsigned int cpu) { - return wait_for_completion_timeout(&cpu_killed, 5000); + return 1; } /* @@ -114,9 +111,6 @@ void platform_cpu_die(unsigned int cpu) } #endif - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* * we're ready for shutdown now, so do it */ diff --git a/arch/arm/mach-ux500/hotplug.c b/arch/arm/mach-ux500/hotplug.c index b782a03..7a4890b 100644 --- a/arch/arm/mach-ux500/hotplug.c +++ b/arch/arm/mach-ux500/hotplug.c @@ -11,14 +11,11 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); - static inline void platform_do_lowpower(unsigned int cpu) { flush_cache_all(); @@ -38,7 +35,7 @@ static inline void platform_do_lowpower(unsigned int cpu) int platform_cpu_kill(unsigned int cpu) { - return wait_for_completion_timeout(&cpu_killed, 5000); + return 1; } /* @@ -58,9 +55,6 @@ void platform_cpu_die(unsigned int cpu) } #endif - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* directly enter low power state, skipping secure registers */ platform_do_lowpower(cpu); } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-29 10:41 ` Russell King - ARM Linux @ 2010-11-29 17:27 ` Vincent Guittot 2010-11-29 19:24 ` Russell King - ARM Linux 0 siblings, 1 reply; 10+ messages in thread From: Vincent Guittot @ 2010-11-29 17:27 UTC (permalink / raw) To: linux-arm-kernel The goal of this patch is to remove as much duplicated code as possible in each platform hotplug file. I have also tried to keep in mind that current platform upstreamed code make nearly no power management in their current implementation. I have added a new interface and a new file in order to -Keep the current interface as it is. So each platform could move to common code when they want -Have a dedicated file for arm hotplug function in which we can add all code that must be executed on an arm core whatever the platform (flushing cache, SCU disabling and handling spurious wake up as a staring point). I agree that the current code is quite small for now and we can wonder if a dedicated file is useful, code might be put in kernel/smp.c file. The next step is also to add some hotplug tracepoints and I would prefer to add the tracepoints only in a platform independent code. With the common code, a new arm platform can have the hotplug feature with less than 20 lines (power management not included) and can be sure that minimal actions will be handled by the common Arm code. We can also keep the platform_cpu_die has the platform entry point and move the common part into kernel/smp.c file. We still have few duplicated code (spurious wake-up) but this seems to be acceptable. I have taken your example into account and have updated the patch accordingly Vincent On 29 November 2010 11:41, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Nov 29, 2010 at 10:54:35AM +0100, Vincent Guittot wrote: >> This patch extracts the common code of the cpu hotplug feature across >> arm platforms. The goal is to only keep the specific stuff of the >> platform in the sub-architecture. I have created a hotplug.c file in >> the ?arm/common directory after studying the cpu hotplug code of >> omap2, realview, s5pv310, ux500 and tegra. I have extracted 3 main >> platform dependent functions: >> ?-platform_enter_lowpower which prepares the platform for low power. >> ?-platform_do_lowpower on which the cpu will loop until it becomes >> really plugged (spurious wake up). This function must returned the cpu >> Id in order to leave the unplug state. >> ?-platform_leave_lowpower which restore the platform context. > > I still do not like this patch. ?The only thing that is worth doing is > this. ?This leaves less than 256 bytes of object code in the Realview > hotplug.c, most of which is the stuff to handle the low power mode > which you haven't dealt with in your patch either. > > I see no point in adding another API on top of the already existing > and simple API. > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> --- arch/arm/kernel/smp.c | 35 ++++++++++++++++++++---- arch/arm/mach-omap2/omap-hotplug.c | 28 ------------------- arch/arm/mach-realview/hotplug.c | 29 -------------------- arch/arm/mach-s5pv310/hotplug.c | 29 -------------------- arch/arm/mach-tegra/hotplug.c | 30 --------------------- arch/arm/mach-ux500/hotplug.c | 51 +++++------------------------------- 6 files changed, 36 insertions(+), 166 deletions(-) diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 8c19595..03042db 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -24,6 +24,7 @@ #include <linux/irq.h> #include <linux/percpu.h> #include <linux/clockchips.h> +#include <linux/completion.h> #include <asm/atomic.h> #include <asm/cacheflush.h> @@ -213,11 +214,13 @@ int __cpu_disable(void) { unsigned int cpu = smp_processor_id(); struct task_struct *p; - int ret; - ret = platform_cpu_disable(cpu); - if (ret) - return ret; + /* + * we don't allow CPU 0 to be shutdown (it is still too special + * e.g. clock tick interrupts) + */ + if (cpu == 0) + return -EPERM; /* * Take this CPU offline. Once we clear this, we can't return, @@ -252,14 +255,16 @@ int __cpu_disable(void) return 0; } +static DECLARE_COMPLETION(cpu_killed); + /* * called on the thread which is asking for a CPU to be shutdown - * waits until shutdown has completed, or it is timed out. */ void __cpu_die(unsigned int cpu) { - if (!platform_cpu_kill(cpu)) - printk("CPU%u: unable to kill\n", cpu); + if (!wait_for_completion_timeout(&cpu_killed, 5000)) + printk(KERN_NOTICE "CPU%u: cpu didn't die\n", cpu); } /* @@ -273,14 +278,32 @@ void __cpu_die(unsigned int cpu) void __ref cpu_die(void) { unsigned int cpu = smp_processor_id(); +#ifdef DEBUG + unsigned int this_cpu = hard_smp_processor_id(); +#endif local_irq_disable(); idle_task_exit(); + printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); + complete(&cpu_killed); + +#ifdef DEBUG + if (cpu != this_cpu) { + printk(KERN_CRIT "Eek! platform_cpu_die is going to run on %u, should be %u\n", + this_cpu, cpu); + BUG(); + } +#endif + + flush_cache_all(); + wmb(); + /* * actual CPU shutdown procedure is at least platform (if not * CPU) specific */ + platform_cpu_die(cpu); /* diff --git a/arch/arm/mach-omap2/omap-hotplug.c b/arch/arm/mach-omap2/omap-hotplug.c index 6cee456..00f4190 100644 --- a/arch/arm/mach-omap2/omap-hotplug.c +++ b/arch/arm/mach-omap2/omap-hotplug.c @@ -17,35 +17,15 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> -#include <asm/cacheflush.h> #include <mach/omap4-common.h> -static DECLARE_COMPLETION(cpu_killed); - -int platform_cpu_kill(unsigned int cpu) -{ - return wait_for_completion_timeout(&cpu_killed, 5000); -} - /* * platform-specific code to shutdown a CPU * Called with IRQs disabled */ void platform_cpu_die(unsigned int cpu) { - unsigned int this_cpu = hard_smp_processor_id(); - - if (cpu != this_cpu) { - pr_crit("platform_cpu_die running on %u, should be %u\n", - this_cpu, cpu); - BUG(); - } - pr_notice("CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - flush_cache_all(); - dsb(); /* * we're ready for shutdown now, so do it @@ -69,11 +49,3 @@ void platform_cpu_die(unsigned int cpu) } } -int platform_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c index f95521a..580cf7c 100644 --- a/arch/arm/mach-realview/hotplug.c +++ b/arch/arm/mach-realview/hotplug.c @@ -11,19 +11,16 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" @@ -93,11 +90,6 @@ static inline void platform_do_lowpower(unsigned int cpu) } } -int platform_cpu_kill(unsigned int cpu) -{ - return wait_for_completion_timeout(&cpu_killed, 5000); -} - /* * platform-specific code to shutdown a CPU * @@ -105,18 +97,6 @@ int platform_cpu_kill(unsigned int cpu) */ void platform_cpu_die(unsigned int cpu) { -#ifdef DEBUG - unsigned int this_cpu = hard_smp_processor_id(); - - if (cpu != this_cpu) { - printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n", - this_cpu, cpu); - BUG(); - } -#endif - - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); /* * we're ready for shutdown now, so do it @@ -130,12 +110,3 @@ void platform_cpu_die(unsigned int cpu) */ cpu_leave_lowpower(); } - -int platform_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} diff --git a/arch/arm/mach-s5pv310/hotplug.c b/arch/arm/mach-s5pv310/hotplug.c index 03652c3..59eafad 100644 --- a/arch/arm/mach-s5pv310/hotplug.c +++ b/arch/arm/mach-s5pv310/hotplug.c @@ -13,19 +13,16 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" @@ -96,11 +93,6 @@ static inline void platform_do_lowpower(unsigned int cpu) } } -int platform_cpu_kill(unsigned int cpu) -{ - return wait_for_completion_timeout(&cpu_killed, 5000); -} - /* * platform-specific code to shutdown a CPU * @@ -108,19 +100,6 @@ int platform_cpu_kill(unsigned int cpu) */ void platform_cpu_die(unsigned int cpu) { -#ifdef DEBUG - unsigned int this_cpu = hard_smp_processor_id(); - - if (cpu != this_cpu) { - printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n", - this_cpu, cpu); - BUG(); - } -#endif - - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* * we're ready for shutdown now, so do it */ @@ -134,11 +113,3 @@ void platform_cpu_die(unsigned int cpu) cpu_leave_lowpower(); } -int platform_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c index 8e7f115..7e5eb3d 100644 --- a/arch/arm/mach-tegra/hotplug.c +++ b/arch/arm/mach-tegra/hotplug.c @@ -11,17 +11,14 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> #include <asm/cacheflush.h> -static DECLARE_COMPLETION(cpu_killed); static inline void cpu_enter_lowpower(void) { unsigned int v; - flush_cache_all(); asm volatile( " mcr p15, 0, %1, c7, c5, 0\n" " mcr p15, 0, %1, c7, c10, 4\n" @@ -92,11 +89,6 @@ static inline void platform_do_lowpower(unsigned int cpu) } } -int platform_cpu_kill(unsigned int cpu) -{ - return wait_for_completion_timeout(&cpu_killed, 5000); -} - /* * platform-specific code to shutdown a CPU * @@ -104,19 +96,6 @@ int platform_cpu_kill(unsigned int cpu) */ void platform_cpu_die(unsigned int cpu) { -#ifdef DEBUG - unsigned int this_cpu = hard_smp_processor_id(); - - if (cpu != this_cpu) { - printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n", - this_cpu, cpu); - BUG(); - } -#endif - - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - /* * we're ready for shutdown now, so do it */ @@ -129,12 +108,3 @@ void platform_cpu_die(unsigned int cpu) */ cpu_leave_lowpower(); } - -int platform_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} diff --git a/arch/arm/mach-ux500/hotplug.c b/arch/arm/mach-ux500/hotplug.c index b782a03..37b443b 100644 --- a/arch/arm/mach-ux500/hotplug.c +++ b/arch/arm/mach-ux500/hotplug.c @@ -6,23 +6,22 @@ * Based on ARM realview platform * * Author: Sundar Iyer <sundar.iyer@stericsson.com> + * Author: vincent.guittot <vincent.guittot@stericsson.com> * */ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/smp.h> -#include <linux/completion.h> - -#include <asm/cacheflush.h> extern volatile int pen_release; -static DECLARE_COMPLETION(cpu_killed); - -static inline void platform_do_lowpower(unsigned int cpu) +/* + * platform-specific code to shutdown a CPU + * + * Called with IRQs disabled + */ +void platform_cpu_die(unsigned int cpu) { - flush_cache_all(); - /* we put the platform to just WFI */ for (;;) { __asm__ __volatile__("dsb\n\t" "wfi\n\t" @@ -36,40 +35,4 @@ static inline void platform_do_lowpower(unsigned int cpu) } } -int platform_cpu_kill(unsigned int cpu) -{ - return wait_for_completion_timeout(&cpu_killed, 5000); -} - -/* - * platform-specific code to shutdown a CPU - * - * Called with IRQs disabled - */ -void platform_cpu_die(unsigned int cpu) -{ -#ifdef DEBUG - unsigned int this_cpu = hard_smp_processor_id(); - - if (cpu != this_cpu) { - printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n", - this_cpu, cpu); - BUG(); - } -#endif - - printk(KERN_NOTICE "CPU%u: shutdown\n", cpu); - complete(&cpu_killed); - - /* directly enter low power state, skipping secure registers */ - platform_do_lowpower(cpu); -} -int platform_cpu_disable(unsigned int cpu) -{ - /* - * we don't allow CPU 0 to be shutdown (it is still too special - * e.g. clock tick interrupts) - */ - return cpu == 0 ? -EPERM : 0; -} -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-29 17:27 ` Vincent Guittot @ 2010-11-29 19:24 ` Russell King - ARM Linux 2010-11-30 10:47 ` Amit Kucheria 0 siblings, 1 reply; 10+ messages in thread From: Russell King - ARM Linux @ 2010-11-29 19:24 UTC (permalink / raw) To: linux-arm-kernel On Mon, Nov 29, 2010 at 06:27:02PM +0100, Vincent Guittot wrote: > The goal of this patch is to remove as much duplicated code as > possible in each platform hotplug file. I have also tried to keep in > mind that current platform upstreamed code make nearly no power > management in their current implementation. I have added a new > interface and a new file in order to > -Keep the current interface as it is. So each platform could move to > common code when they want > -Have a dedicated file for arm hotplug function in which we can add > all code that must be executed on an arm core whatever the platform > (flushing cache, SCU disabling and handling spurious wake up as a > staring point). I agree that the current code is quite small for now > and we can wonder if a dedicated file is useful, code might be put in > kernel/smp.c file. > The next step is also to add some hotplug tracepoints and I would > prefer to add the tracepoints only in a platform independent code. > > With the common code, a new arm platform can have the hotplug feature > with less than 20 lines (power management not included) and can be > sure that minimal actions will be handled by the common Arm code. > > We can also keep the platform_cpu_die has the platform entry point and > move the common part into kernel/smp.c file. We still have few > duplicated code (spurious wake-up) but this seems to be acceptable. > > I have taken your example into account and have updated the patch accordingly No you haven't because you don't understand the point of keeping the existing interfaces. I'm not going to cripple the flexibility of the current interface just in the name of merging them all together. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-29 19:24 ` Russell King - ARM Linux @ 2010-11-30 10:47 ` Amit Kucheria 2010-11-30 11:03 ` Russell King - ARM Linux 0 siblings, 1 reply; 10+ messages in thread From: Amit Kucheria @ 2010-11-30 10:47 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 30, 2010 at 12:54 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > On Mon, Nov 29, 2010 at 06:27:02PM +0100, Vincent Guittot wrote: > > The goal of this patch is to remove as much duplicated code as > > possible in each platform hotplug file. I have also tried to keep in > > mind that current platform upstreamed code make nearly no power > > management in their current implementation. I have added a new > > interface and a new file in order to > > ?-Keep the current interface as it is. So each platform could move to > > common code when they want > > ?-Have a dedicated file for arm hotplug function in which we can add > > all code that must be executed on an arm core whatever the platform > > (flushing cache, SCU disabling and handling spurious wake up as a > > staring ?point). I agree that the current code is quite small for now > > and we can wonder if a dedicated file is useful, code might be put in > > kernel/smp.c file. > > The next step is also to add some hotplug tracepoints and I would > > prefer to add the tracepoints only in a platform independent code. > > > > With the common code, a new arm platform can have the hotplug feature > > with less than 20 lines (power management not included) and can be > > sure that minimal actions will be handled by the common Arm code. > > > > We can also keep the platform_cpu_die has the platform entry point and > > move the common part into kernel/smp.c file. We still have few > > duplicated code (spurious wake-up) but this seems to be acceptable. > > > > I have taken your example into account and have updated the patch accordingly > > No you haven't because you don't understand the point of keeping the > existing interfaces. ?I'm not going to cripple the flexibility of the > current interface just in the name of merging them all together. > To bring into context your reply to an earlier submission of a similar patch [1], you stated "So what if a platform wants to completely power down a CPU? You'd want to implement it such that platform_cpu_die() does all the shutdown of the CPU except for power-off, and platform_cpu_kill() waits for that to complete before turning power off. With platform_cpu_kill() and platform_cpu_die() moved into generic code, you can no longer do this." Since the main aim here is to consolidate as much code here as possible while still allowing platforms to override the defaults, would you have an objection to the introduction of a struct smp_ops that'll allow a platform to override the defaults? This seems to be done on other platforms I've briefly looked at. $ git grep "\.cpu_die" arch/mips/cavium-octeon/smp.c: .cpu_die = octeon_cpu_die, arch/mips/kernel/smp-up.c: .cpu_die = up_cpu_die, arch/powerpc/kernel/smp.c: if (ppc_md.cpu_die) arch/powerpc/kernel/smp.c: ppc_md.cpu_die(); arch/powerpc/kernel/sysfs.c: if (ppc_md.cpu_die) arch/powerpc/platforms/powermac/setup.c: .cpu_die = pmac64_cpu_die, arch/powerpc/platforms/powermac/setup.c: .cpu_die = pmac32_cpu_die, arch/powerpc/platforms/powermac/setup.c: .cpu_die = generic_mach_cpu_die, arch/powerpc/platforms/powermac/smp.c: .cpu_die = smp_core99_cpu_die, arch/powerpc/platforms/powermac/smp.c: .cpu_die = generic_cpu_die, arch/powerpc/platforms/pseries/hotplug-cpu.c: ppc_md.cpu_die = pseries_mach_cpu_die; arch/sh/kernel/cpu/sh4a/smp-shx3.c: .cpu_die = native_cpu_die, arch/x86/include/asm/smp.h: smp_ops.cpu_die(cpu); arch/x86/kernel/smp.c: .cpu_die = native_cpu_die, arch/x86/xen/smp.c: .cpu_die = xen_cpu_die, [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/89542 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-30 10:47 ` Amit Kucheria @ 2010-11-30 11:03 ` Russell King - ARM Linux 2010-11-30 11:51 ` Russell King - ARM Linux 2010-11-30 16:24 ` Rob Herring 0 siblings, 2 replies; 10+ messages in thread From: Russell King - ARM Linux @ 2010-11-30 11:03 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 30, 2010 at 04:17:32PM +0530, Amit Kucheria wrote: > Since the main aim here is to consolidate as much code here as > possible while still allowing platforms to override the defaults, > would you have an objection to the introduction of a struct smp_ops > that'll allow a platform to override the defaults? This seems to be > done on other platforms I've briefly looked at. I see no point to what is being proposed in this thread. It's _soo_ little code that the platforms have to implement that it really is not worth the effort. How do you know whether separating out the cache flushes from the wait-for-interrupt is an acceptable thing to do? On the Realview platforms, I suspect it's not acceptable. That means your attempts to move the cache flusing into a separate function from the wait-for- interrupt will cause problems - as entering a function creates a stack frame, and therefore writes to memory which can hit the cache. Leave it as is. The generic interface for platforms to implement is: platform_do_lowpower() - does whatever's necessary to idle etc the CPU platform_cpu_kill() - returns 1 if there's nothing to be done platform_cpu_disable() - returns 0 if the CPU can be taken offline Trying to get rid of platform_cpu_kill and platform_cpu_disable, and then splitting platform_do_lowpower into three new smaller functions is NOT an improvement. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-30 11:03 ` Russell King - ARM Linux @ 2010-11-30 11:51 ` Russell King - ARM Linux 2010-11-30 14:05 ` Will Deacon 2010-11-30 16:24 ` Rob Herring 1 sibling, 1 reply; 10+ messages in thread From: Russell King - ARM Linux @ 2010-11-30 11:51 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 30, 2010 at 11:03:31AM +0000, Russell King - ARM Linux wrote: > I see no point to what is being proposed in this thread. It's _soo_ > little code that the platforms have to implement that it really is > not worth the effort. > > How do you know whether separating out the cache flushes from the > wait-for-interrupt is an acceptable thing to do? On the Realview > platforms, I suspect it's not acceptable. That means your attempts > to move the cache flusing into a separate function from the wait-for- > interrupt will cause problems - as entering a function creates a > stack frame, and therefore writes to memory which can hit the cache. > > Leave it as is. The generic interface for platforms to implement is: > > platform_do_lowpower() - does whatever's necessary to idle etc the CPU > platform_cpu_kill() - returns 1 if there's nothing to be done > platform_cpu_disable() - returns 0 if the CPU can be taken offline > > Trying to get rid of platform_cpu_kill and platform_cpu_disable, and > then splitting platform_do_lowpower into three new smaller functions > is NOT an improvement. Note also that most implementations of this get it wrong. Realview assumes ARMv6 MPcore layout of the auxcontrol register, which is cut'n'pasted into s5pv310 (ARMv7) and Tegra (ARMv7) without thinking. The only implementation which has put some thought into it is the UX500 one, which is extremely simple. So, out of the four existing implementations, at least two need fixing to be different, and I'll bet that they do have some way to power down the offlined CPUs for greater power savings. On that basis, there is absolutely no way to start thinking about collecting these up into common code, as you don't know what the proper implementation for these platforms should look like. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-30 11:51 ` Russell King - ARM Linux @ 2010-11-30 14:05 ` Will Deacon 0 siblings, 0 replies; 10+ messages in thread From: Will Deacon @ 2010-11-30 14:05 UTC (permalink / raw) To: linux-arm-kernel Russell, > On Tue, Nov 30, 2010 at 11:03:31AM +0000, Russell King - ARM Linux wrote: > > I see no point to what is being proposed in this thread. It's _soo_ > > little code that the platforms have to implement that it really is > > not worth the effort. [...] > Note also that most implementations of this get it wrong. Realview > assumes ARMv6 MPcore layout of the auxcontrol register, which is > cut'n'pasted into s5pv310 (ARMv7) and Tegra (ARMv7) without thinking. > The only implementation which has put some thought into it is the > UX500 one, which is extremely simple. > > So, out of the four existing implementations, at least two need fixing > to be different, and I'll bet that they do have some way to power down > the offlined CPUs for greater power savings. I have patches that fix this for RealView and Versatile Express, I posted them to the list in the past (note that these are now out of date): http://lists.infradead.org/pipermail/linux-arm-kernel/2010-September/026160.html I have a newer version here: git://linux-arm.org/linux-2.6-wd.git cpu-hotplug but it relies on the Versatile Express multi-tile patches that I'm posting at the moment. If the hotplug APIs aren't about to change fundamentally, then I'll repost these patches once the multi-tile stuff is accepted. Will ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-30 11:03 ` Russell King - ARM Linux 2010-11-30 11:51 ` Russell King - ARM Linux @ 2010-11-30 16:24 ` Rob Herring 2010-11-30 16:58 ` Russell King - ARM Linux 1 sibling, 1 reply; 10+ messages in thread From: Rob Herring @ 2010-11-30 16:24 UTC (permalink / raw) To: linux-arm-kernel On 11/30/2010 05:03 AM, Russell King - ARM Linux wrote: > On Tue, Nov 30, 2010 at 04:17:32PM +0530, Amit Kucheria wrote: >> Since the main aim here is to consolidate as much code here as >> possible while still allowing platforms to override the defaults, >> would you have an objection to the introduction of a struct smp_ops >> that'll allow a platform to override the defaults? This seems to be >> done on other platforms I've briefly looked at. > > I see no point to what is being proposed in this thread. It's _soo_ > little code that the platforms have to implement that it really is > not worth the effort. > Whether the code can be consolidated or not, the current API does not allow a single kernel binary (ignoring the list of other issues). The introduction of struct smp_ops would help enable both single kernel and default versions of the functions. Rob ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug 2010-11-30 16:24 ` Rob Herring @ 2010-11-30 16:58 ` Russell King - ARM Linux 0 siblings, 0 replies; 10+ messages in thread From: Russell King - ARM Linux @ 2010-11-30 16:58 UTC (permalink / raw) To: linux-arm-kernel On Tue, Nov 30, 2010 at 10:24:11AM -0600, Rob Herring wrote: > On 11/30/2010 05:03 AM, Russell King - ARM Linux wrote: >> On Tue, Nov 30, 2010 at 04:17:32PM +0530, Amit Kucheria wrote: >>> Since the main aim here is to consolidate as much code here as >>> possible while still allowing platforms to override the defaults, >>> would you have an objection to the introduction of a struct smp_ops >>> that'll allow a platform to override the defaults? This seems to be >>> done on other platforms I've briefly looked at. >> >> I see no point to what is being proposed in this thread. It's _soo_ >> little code that the platforms have to implement that it really is >> not worth the effort. >> > Whether the code can be consolidated or not, the current API does not > allow a single kernel binary (ignoring the list of other issues). The > introduction of struct smp_ops would help enable both single kernel and > default versions of the functions. True. However, the "single kernel binary" is a long way off, with a fair number of fundamental issues. Currently, the toolchain people seem to be going in a completely different direction, adding blocks to being able to build a single binary for several different CPUs. See the recent thread on the SWP emulation build failure. However, that's a different issue to the subject of these patches, which is to add another _optional_ layer of APIs between the generic ARM SMP code, and the platform code. We currently have one API, which is: - platform_cpu_kill, platform_cpu_die, platform_cpu_disable What is being proposed in these patches so far is that we introduce another optional layer which introduces a new set of APIs: - platform_enter_lowpower, platform_do_lowpower, platform_leave_lowpower to support the noddy 'spin in a low power loop waiting for wakeup' method. In order to support platforms which don't want to use the noddy method, instead of having just three interfaces to deal with being different between platforms, we instead have _six_ interfaces. This is not the way to go. We really don't need to increase the complexity by adding a 'consolidation' layer on top of what we already have. In terms of allowing platform independence, just wrapping these three (platform_cpu_kill, platform_cpu_die, platform_cpu_disable) into some smp_ops structure may seem a nice idea, but there's more SMP platform specifics than just that. Eg, there's the cross-call support (which must be initialized with the IRQ controller.) I don't think wrapping them into something which purports to be a generic smp_ops structure, and then having to play games with ifdefs because they're spread across several files with different compilation options is a sane way forward. I still come back to my original point though. Let's first get the existing support fixed so that we know what we are dealing with, so we can first see whether the existing APIs are right for everyone, rather than just blindly believing that they are because most people have mindlessly cut'n'pasted the Realview code. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-11-30 16:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <AANLkTi=fJeOUhH0xUJF8qx-fTJ5popE7ZuVxDZXpTH-i@mail.gmail.com>
2010-11-29 9:54 ` [PATCH][ ARM cpu hotplug 1/2 ] extract common code for arm cpu hotplug Vincent Guittot
2010-11-29 10:41 ` Russell King - ARM Linux
2010-11-29 17:27 ` Vincent Guittot
2010-11-29 19:24 ` Russell King - ARM Linux
2010-11-30 10:47 ` Amit Kucheria
2010-11-30 11:03 ` Russell King - ARM Linux
2010-11-30 11:51 ` Russell King - ARM Linux
2010-11-30 14:05 ` Will Deacon
2010-11-30 16:24 ` Rob Herring
2010-11-30 16:58 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).