On 04/03/2014 10:03 AM, dongsheng.w...@freescale.com wrote:
-----Original Message-----
From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org]
Sent: Thursday, April 03, 2014 2:29 PM
To: Wang Dongsheng-B40534; Wood Scott-B07421
Cc: r...@rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao Chenhui-
B35336; linux...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support
On 04/03/2014 05:20 AM, dongsheng.w...@freescale.com wrote:
Hi Daniel,
Thanks for your review. I will fix your comments.
BTW, fix Rafael's email. :)
+#include <linux/cpu.h>
+#include <linux/cpuidle.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/notifier.h>
+
+#include <asm/cputable.h>
+#include <asm/machdep.h>
+#include <asm/mpc85xx.h>
+
+static unsigned int max_idle_state; static struct cpuidle_state
+*cpuidle_state_table;
+
+struct cpuidle_driver e500_idle_driver = {
+ .name = "e500_idle",
+ .owner = THIS_MODULE,
+};
+
+static void e500_cpuidle(void)
+{
+ if (cpuidle_idle_call())
+ cpuidle_wait();
+}
Nope, that has been changed. No more call to cpuidle_idle_call in a driver.
Thanks.
+
+static int pw10_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index) {
+ cpuidle_wait();
+ return index;
+}
+
+#define MAX_BIT 63
+#define MIN_BIT 1
+extern u32 cpuidle_entry_bit;
+static int pw20_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index) {
+ u32 pw20_idle;
+ u32 entry_bit;
+ pw20_idle = mfspr(SPRN_PWRMGTCR0);
+ if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) {
+ pw20_idle &= ~PWRMGTCR0_PW20_ENT;
+ entry_bit = MAX_BIT - cpuidle_entry_bit;
+ pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT);
+ mtspr(SPRN_PWRMGTCR0, pw20_idle);
+ }
+
+ cpuidle_wait();
+
+ pw20_idle &= ~PWRMGTCR0_PW20_ENT;
+ pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT);
+ mtspr(SPRN_PWRMGTCR0, pw20_idle);
+
+ return index;
+}
Is it possible to give some comments and encapsulate the code with
explicit function names to be implemented in an arch specific
directory file (eg. pm.c) and export these functions in a linux/
header ? We try to prevent to include asm if possible.
Yep, Looks better. Thanks.
+
+static struct cpuidle_state pw_idle_states[] = {
+ {
+ .name = "pw10",
+ .desc = "pw10",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = &pw10_enter
+ },
+
+ {
+ .name = "pw20",
+ .desc = "pw20-core-idle",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 1,
+ .target_residency = 50,
+ .enter = &pw20_enter
+ },
+};
No need to define this intermediate structure here, you can directly
initialize the cpuidle_driver:
Thanks. :)
+static int cpu_hotplug_notify(struct notifier_block *n,
+ unsigned long action, void *hcpu) {
+ unsigned long hotcpu = (unsigned long)hcpu;
+ struct cpuidle_device *dev =
+ per_cpu_ptr(cpuidle_devices, hotcpu);
+
+ if (dev && cpuidle_get_driver()) {
+ switch (action) {
+ case CPU_ONLINE:
+ case CPU_ONLINE_FROZEN:
+ cpuidle_pause_and_lock();
+ cpuidle_enable_device(dev);
+ cpuidle_resume_and_unlock();
+ break;
+
+ case CPU_DEAD:
+ case CPU_DEAD_FROZEN:
+ cpuidle_pause_and_lock();
+ cpuidle_disable_device(dev);
+ cpuidle_resume_and_unlock();
+ break;
+
+ default:
+ return NOTIFY_DONE;
+ }
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block cpu_hotplug_notifier = {
+ .notifier_call = cpu_hotplug_notify, };
Can you explain why this is needed ?
If a cpu will be plugged out/in, We should be let Cpuidle know to
remove corresponding sys interface and disable/enable cpudile-governor for
current cpu.
Ok, this code is a copy-paste of the powers' cpuidle driver.
IIRC, I posted a patchset to move this portion of code in the cpuidle common
framework some time ago.
Could you please get rid of this part of code ?
Yes, I can. :) Could you share me your patchset link? I can't found them on
your tree.
It was a while ago. I should have it somewhere locally. I will find it
out and resend the patch next week when finishing my current task.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/>
Blog
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
