Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755419Ab3HWL4T (ORCPT ); Fri, 23 Aug 2013 07:56:19 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:33009 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753759Ab3HWL4S (ORCPT ); Fri, 23 Aug 2013 07:56:18 -0400 Message-ID: <52174DB5.5050201@linux.vnet.ibm.com> Date: Fri, 23 Aug 2013 17:25:33 +0530 From: Deepthi Dharwar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Wang Dongsheng-B40534 CC: "benh@kernel.crashing.org" , "daniel.lezcano@linaro.org" , "linux-kernel@vger.kernel.org" , Wood Scott-B07421 , "linux-pm@lists.linux-foundation.org" , "linuxppc-dev@lists.ozlabs.org" , "michael@ellerman.id.au" , "preeti@linux.vnet.ibm.com" , "svaidy@linux.vnet.ibm.com" Subject: Re: [PATCH V5 3/5] POWER/cpuidle: Generic IBM-POWER backend cpuidle driver. References: <20130822095323.27416.79369.stgit@deepthi.in.ibm.com> <20130822095357.27416.48110.stgit@deepthi.in.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13082311-1618-0000-0000-000004811CA7 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4153 Lines: 146 On 08/23/2013 08:46 AM, Wang Dongsheng-B40534 wrote: > >> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig >> index 0e2cd5c..e805dcd 100644 >> --- a/drivers/cpuidle/Kconfig >> +++ b/drivers/cpuidle/Kconfig > > Maybe drivers/cpuidle/Kconfig.powerpc is better? Like arm. > >> +obj-$(CONFIG_CPU_IDLE_IBM_POWER) += cpuidle-ibm-power.o >> diff --git a/drivers/cpuidle/cpuidle-ibm-power.c >> b/drivers/cpuidle/cpuidle-ibm-power.c >> new file mode 100644 >> index 0000000..4ee5a94 >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-ibm-power.c >> @@ -0,0 +1,304 @@ >> +/* >> + * cpuidle-ibm-power - idle state cpuidle driver. >> + * Adapted from drivers/idle/intel_idle.c and >> + * drivers/acpi/processor_idle.c >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +struct cpuidle_driver power_idle_driver = { >> + .name = "IBM-POWER-Idle", >> + .owner = THIS_MODULE, >> +}; >> + >> +#define MAX_IDLE_STATE_COUNT 2 >> + >> +static int max_idle_state = MAX_IDLE_STATE_COUNT - 1; > > Again, do not use the macro. Wanting to re-use CPUIDLE_STATE_MAX macro, defined in linux/cpuidle.h similar to what x86 uses. This is def scalable for various platforms , as demonstrated in drivers/idle/intel_idle.c > >> +static struct cpuidle_state *cpuidle_state_table; >> + >> +static inline void idle_loop_prolog(unsigned long *in_purr) >> +{ >> + *in_purr = mfspr(SPRN_PURR); >> + /* >> + * Indicate to the HV that we are idle. Now would be >> + * a good time to find other work to dispatch. >> + */ >> + get_lppaca()->idle = 1; >> +} >> + >> +static inline void idle_loop_epilog(unsigned long in_purr) >> +{ >> + get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr; >> + get_lppaca()->idle = 0; >> +} >> + >> +static int snooze_loop(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, >> + int index) >> +{ >> + unsigned long in_purr; >> + >> + idle_loop_prolog(&in_purr); >> + local_irq_enable(); > > snooze_loop has already registered in cpuidle framework to handle snooze state. > where disable the irq? Why do "enable" here? > >> +/* >> + * States for dedicated partition case. >> + */ >> +static struct cpuidle_state dedicated_states[MAX_IDLE_STATE_COUNT] = { >> + { /* Snooze */ >> + .name = "snooze", >> + .desc = "snooze", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 0, >> + .target_residency = 0, >> + .enter = &snooze_loop }, >> + { /* CEDE */ >> + .name = "CEDE", >> + .desc = "CEDE", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 10, >> + .target_residency = 100, >> + .enter = &dedicated_cede_loop }, >> +}; >> + >> +/* >> + * States for shared partition case. >> + */ >> +static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = { >> + { /* Shared Cede */ >> + .name = "Shared Cede", >> + .desc = "Shared Cede", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 0, >> + .target_residency = 0, >> + .enter = &shared_cede_loop }, >> +}; >> + >> +static void __exit power_processor_idle_exit(void) >> +{ >> + >> + unregister_cpu_notifier(&setup_hotplug_notifier); > > Remove a blank line. > >> + cpuidle_unregister(&power_idle_driver); >> + return; >> +} >> + >> +module_init(power_processor_idle_init); >> +module_exit(power_processor_idle_exit); >> + > > Did you have tested the module? If not tested, please don't use the module. > >> +MODULE_AUTHOR("Deepthi Dharwar "); >> +MODULE_DESCRIPTION("Cpuidle driver for IBM POWER platforms"); >> +MODULE_LICENSE("GPL"); >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/