[PATCH] cpuidle: add freescale e500 family porcessors idle support

[PATCH] cpuidle: add freescale e500 family porcessors idle support

[Dongsheng Wang]

From: Wang Dongsheng <dongsheng.wang@freescale.com>

Add cpuidle support for e500 family, using cpuidle framework to
manage various low power modes. The new implementation will remain
compatible with original idle method.

I have done test about power consumption and latency. Cpuidle framework
will make CPU response time faster than original method, but power
consumption is higher than original method.

Power consumption:
The original method, power consumption is 10.51202 (W).
The cpuidle framework, power consumption is 10.5311 (W).

Latency:
The original method, avg latency is 6782 (us).
The cpuidle framework, avg latency is 6482 (us).

Initially, this supports PW10, PW20 and subsequent patches will support
DOZE/NAP and PH10, PH20.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 5b6c03f..9301420 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -294,6 +294,15 @@ extern void power7_idle(void);
 extern void ppc6xx_idle(void);
 extern void book3e_idle(void);
 
+static inline void cpuidle_wait(void)
+{
+#ifdef CONFIG_PPC64
+	book3e_idle();
+#else
+	e500_idle();
+#endif
+}
+
 /*
  * ppc_md contains a copy of the machine description structure for the
  * current platform. machine_id contains the initial address where the
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 97e1dc9..edd193f 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device *dev,
 	return sprintf(buf, "%llu\n", time > 0 ? time : 0);
 }
 
+#ifdef CONFIG_CPU_IDLE_E500
+u32 cpuidle_entry_bit;
+#endif
 static void set_pw20_wait_entry_bit(void *val)
 {
 	u32 *value = val;
@@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val)
 	/* set count */
 	pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT);
 
+#ifdef CONFIG_CPU_IDLE_E500
+	cpuidle_entry_bit = *value;
+#else
 	mtspr(SPRN_PWRMGTCR0, pw20_idle);
+#endif
 }
 
 static ssize_t store_pw20_wait_time(struct device *dev,
diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc
index 66c3a09..0949dbf 100644
--- a/drivers/cpuidle/Kconfig.powerpc
+++ b/drivers/cpuidle/Kconfig.powerpc
@@ -18,3 +18,10 @@ config POWERNV_CPUIDLE
 	help
 	  Select this option to enable processor idle state management
 	  through cpuidle subsystem.
+
+config CPU_IDLE_E500
+	bool "CPU Idle Driver for E500 family processors"
+	depends on CPU_IDLE
+	depends on FSL_SOC_BOOKE
+	help
+	  Select this to enable cpuidle on e500 family processors.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f71ae1b..7e6adea 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 # POWERPC drivers
 obj-$(CONFIG_PSERIES_CPUIDLE)		+= cpuidle-pseries.o
 obj-$(CONFIG_POWERNV_CPUIDLE)		+= cpuidle-powernv.o
+obj-$(CONFIG_CPU_IDLE_E500)		+= cpuidle-e500.o
diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c
new file mode 100644
index 0000000..ddc0def
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-e500.c
@@ -0,0 +1,194 @@
+/*
+ * CPU Idle driver for Freescale PowerPC e500 family processors.
+ *
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ *
+ * Author: Dongsheng Wang <dongsheng.wang@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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();
+}
+
+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;
+}
+
+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
+	},
+};
+
+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,
+};
+
+static void e500_cpuidle_driver_init(void)
+{
+	int idle_state;
+	struct cpuidle_driver *drv = &e500_idle_driver;
+
+	drv->state_count = 0;
+
+	for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
+		if (!cpuidle_state_table[idle_state].enter)
+			break;
+
+		drv->states[drv->state_count] = cpuidle_state_table[idle_state];
+		drv->state_count++;
+	}
+}
+
+static int e500_idle_state_probe(void)
+{
+	if (cpuidle_disable != IDLE_NO_OVERRIDE)
+		return -ENODEV;
+
+	cpuidle_state_table = pw_idle_states;
+	max_idle_state = ARRAY_SIZE(pw_idle_states);
+
+	/* Disable PW20 feature for e500mc, e5500 */
+	if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
+		cpuidle_state_table[1].enter = NULL;
+
+	if (!cpuidle_state_table || !max_idle_state)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void replace_orig_idle(void *dummy)
+{
+	return;
+}
+
+static int __init e500_idle_init(void)
+{
+	struct cpuidle_driver *drv = &e500_idle_driver;
+	int err;
+
+	if (e500_idle_state_probe())
+		return -ENODEV;
+
+	e500_cpuidle_driver_init();
+	if (!drv->state_count)
+		return -ENODEV;
+
+	err = cpuidle_register(drv, NULL);
+	if (err) {
+		pr_err("Register e500 family cpuidle driver failed.\n");
+
+		return err;
+	}
+
+	err = register_cpu_notifier(&cpu_hotplug_notifier);
+	if (err)
+		pr_warn("Cpuidle driver: register cpu notifier failed.\n");
+
+	/* Replace the original way of idle after cpuidle registered. */
+	ppc_md.power_save = e500_cpuidle;
+	on_each_cpu(replace_orig_idle, NULL, 1);
+
+	pr_info("e500_idle_driver registered.\n");
+
+	return 0;
+}
+late_initcall(e500_idle_init);
-- 
1.8.5

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano]

On 04/01/2014 10:33 AM, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>
> Add cpuidle support for e500 family, using cpuidle framework to
> manage various low power modes. The new implementation will remain
> compatible with original idle method.
>
> I have done test about power consumption and latency. Cpuidle framework
> will make CPU response time faster than original method, but power
> consumption is higher than original method.
>
> Power consumption:
> The original method, power consumption is 10.51202 (W).
> The cpuidle framework, power consumption is 10.5311 (W).
>
> Latency:
> The original method, avg latency is 6782 (us).
> The cpuidle framework, avg latency is 6482 (us).
>
> Initially, this supports PW10, PW20 and subsequent patches will support
> DOZE/NAP and PH10, PH20.
>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index 5b6c03f..9301420 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -294,6 +294,15 @@ extern void power7_idle(void);
>   extern void ppc6xx_idle(void);
>   extern void book3e_idle(void);
>
> +static inline void cpuidle_wait(void)
> +{
> +#ifdef CONFIG_PPC64
> +	book3e_idle();
> +#else
> +	e500_idle();
> +#endif
> +}
> +
>   /*
>    * ppc_md contains a copy of the machine description structure for the
>    * current platform. machine_id contains the initial address where the
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 97e1dc9..edd193f 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device *dev,
>   	return sprintf(buf, "%llu\n", time > 0 ? time : 0);
>   }
>
> +#ifdef CONFIG_CPU_IDLE_E500
> +u32 cpuidle_entry_bit;
> +#endif
>   static void set_pw20_wait_entry_bit(void *val)
>   {
>   	u32 *value = val;
> @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val)
>   	/* set count */
>   	pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT);
>
> +#ifdef CONFIG_CPU_IDLE_E500
> +	cpuidle_entry_bit = *value;
> +#else
>   	mtspr(SPRN_PWRMGTCR0, pw20_idle);
> +#endif
>   }
>
>   static ssize_t store_pw20_wait_time(struct device *dev,
> diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc
> index 66c3a09..0949dbf 100644
> --- a/drivers/cpuidle/Kconfig.powerpc
> +++ b/drivers/cpuidle/Kconfig.powerpc
> @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE
>   	help
>   	  Select this option to enable processor idle state management
>   	  through cpuidle subsystem.
> +
> +config CPU_IDLE_E500
> +	bool "CPU Idle Driver for E500 family processors"
> +	depends on CPU_IDLE
> +	depends on FSL_SOC_BOOKE
> +	help
> +	  Select this to enable cpuidle on e500 family processors.
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index f71ae1b..7e6adea 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
>   # POWERPC drivers
>   obj-$(CONFIG_PSERIES_CPUIDLE)		+= cpuidle-pseries.o
>   obj-$(CONFIG_POWERNV_CPUIDLE)		+= cpuidle-powernv.o
> +obj-$(CONFIG_CPU_IDLE_E500)		+= cpuidle-e500.o
> diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c
> new file mode 100644
> index 0000000..ddc0def
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-e500.c
> @@ -0,0 +1,194 @@
> +/*
> + * CPU Idle driver for Freescale PowerPC e500 family processors.
> + *
> + * Copyright 2014 Freescale Semiconductor, Inc.
> + *
> + * Author: Dongsheng Wang <dongsheng.wang@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#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.

> +
> +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.

> +
> +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:


struct cpuidle_driver e500_idle_driver = {
	.name = "e500_idle",
	.owner = THIS_MODULE,
	.states = {
		.name = "pw10",
		.desc = "pw10",
		.flags = CPUIDLE_FLAG_TIME_VALID,
		.target_residency = 0,
		.enter = &pw10_enter,
	},

	....

	.state_count = 2,	
};

Then in the init function you initialize the state_count consequently:

if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
	drv->state_count = 1;

Then you can kill:

max_idle_state, cpuidle_state_table, e500_idle_state_probe and 
pw_idle_states.

> +
> +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 ?

> +static void e500_cpuidle_driver_init(void)
> +{
> +	int idle_state;
> +	struct cpuidle_driver *drv = &e500_idle_driver;

Pass the cpuidle_driver as parameter to the function.

> +
> +	drv->state_count = 0;
> +
> +	for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
> +		if (!cpuidle_state_table[idle_state].enter)
> +			break;
> +
> +		drv->states[drv->state_count] = cpuidle_state_table[idle_state];
> +		drv->state_count++;
> +	}

This code should disappear.

As this function will just initialize state_count, you can move it in 
caller and kill this function.

> +}
> +
> +static int e500_idle_state_probe(void)
> +{
> +	if (cpuidle_disable != IDLE_NO_OVERRIDE)
> +		return -ENODEV;
> +
> +	cpuidle_state_table = pw_idle_states;
> +	max_idle_state = ARRAY_SIZE(pw_idle_states);
> +
> +	/* Disable PW20 feature for e500mc, e5500 */
> +	if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
> +		cpuidle_state_table[1].enter = NULL;
> +
> +	if (!cpuidle_state_table || !max_idle_state)
> +		return -ENODEV;
> +
> +	return 0;
> +}

This code should disappear.

> +static void replace_orig_idle(void *dummy)
> +{
> +	return;
> +}
> +
> +static int __init e500_idle_init(void)
> +{
> +	struct cpuidle_driver *drv = &e500_idle_driver;
> +	int err;
> +
> +	if (e500_idle_state_probe())
> +		return -ENODEV;
> +
> +	e500_cpuidle_driver_init();
> +	if (!drv->state_count)
> +		return -ENODEV;

No need of this check, because:

1. you know how you initialized the driver (1 or 2 states)
2. this is already by the cpuidle framework

> +
> +	err = cpuidle_register(drv, NULL);
> +	if (err) {
> +		pr_err("Register e500 family cpuidle driver failed.\n");

extra carriage return.
> +
> +		return err;
> +	}
> +
> +	err = register_cpu_notifier(&cpu_hotplug_notifier);
> +	if (err)
> +		pr_warn("Cpuidle driver: register cpu notifier failed.\n");
> +
> +	/* Replace the original way of idle after cpuidle registered. */
> +	ppc_md.power_save = e500_cpuidle;
> +	on_each_cpu(replace_orig_idle, NULL, 1);

Why ?

> +	pr_info("e500_idle_driver registered.\n");
> +
> +	return 0;
> +}
> +late_initcall(e500_idle_init);
>

Thanks

   -- 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

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano]

On 04/02/2014 11:36 AM, Daniel Lezcano wrote:

> On 04/01/2014 10:33 AM, Dongsheng Wang wrote:
>> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>>
>> Add cpuidle support for e500 family, using cpuidle framework to
>> manage various low power modes. The new implementation will remain
>> compatible with original idle method.
>>
>> I have done test about power consumption and latency. Cpuidle framework
>> will make CPU response time faster than original method, but power
>> consumption is higher than original method.
>>
>> Power consumption:
>> The original method, power consumption is 10.51202 (W).
>> The cpuidle framework, power consumption is 10.5311 (W).
>>
>> Latency:
>> The original method, avg latency is 6782 (us).
>> The cpuidle framework, avg latency is 6482 (us).
>>
>> Initially, this supports PW10, PW20 and subsequent patches will support
>> DOZE/NAP and PH10, PH20.
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

Please fix Rafael's email when resending/answering.

Thanks

   -- Daniel

>> diff --git a/arch/powerpc/include/asm/machdep.h
>> b/arch/powerpc/include/asm/machdep.h
>> index 5b6c03f..9301420 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -294,6 +294,15 @@ extern void power7_idle(void);
>>   extern void ppc6xx_idle(void);
>>   extern void book3e_idle(void);
>>
>> +static inline void cpuidle_wait(void)
>> +{
>> +#ifdef CONFIG_PPC64
>> +    book3e_idle();
>> +#else
>> +    e500_idle();
>> +#endif
>> +}
>> +
>>   /*
>>    * ppc_md contains a copy of the machine description structure for the
>>    * current platform. machine_id contains the initial address where the
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 97e1dc9..edd193f 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device
>> *dev,
>>       return sprintf(buf, "%llu\n", time > 0 ? time : 0);
>>   }
>>
>> +#ifdef CONFIG_CPU_IDLE_E500
>> +u32 cpuidle_entry_bit;
>> +#endif
>>   static void set_pw20_wait_entry_bit(void *val)
>>   {
>>       u32 *value = val;
>> @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val)
>>       /* set count */
>>       pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT);
>>
>> +#ifdef CONFIG_CPU_IDLE_E500
>> +    cpuidle_entry_bit = *value;
>> +#else
>>       mtspr(SPRN_PWRMGTCR0, pw20_idle);
>> +#endif
>>   }
>>
>>   static ssize_t store_pw20_wait_time(struct device *dev,
>> diff --git a/drivers/cpuidle/Kconfig.powerpc
>> b/drivers/cpuidle/Kconfig.powerpc
>> index 66c3a09..0949dbf 100644
>> --- a/drivers/cpuidle/Kconfig.powerpc
>> +++ b/drivers/cpuidle/Kconfig.powerpc
>> @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE
>>       help
>>         Select this option to enable processor idle state management
>>         through cpuidle subsystem.
>> +
>> +config CPU_IDLE_E500
>> +    bool "CPU Idle Driver for E500 family processors"
>> +    depends on CPU_IDLE
>> +    depends on FSL_SOC_BOOKE
>> +    help
>> +      Select this to enable cpuidle on e500 family processors.
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index f71ae1b..7e6adea 100644
>> --- a/drivers/cpuidle/Makefile
>> +++ b/drivers/cpuidle/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE)          +=
>> cpuidle-at91.o
>>   # POWERPC drivers
>>   obj-$(CONFIG_PSERIES_CPUIDLE)        += cpuidle-pseries.o
>>   obj-$(CONFIG_POWERNV_CPUIDLE)        += cpuidle-powernv.o
>> +obj-$(CONFIG_CPU_IDLE_E500)        += cpuidle-e500.o
>> diff --git a/drivers/cpuidle/cpuidle-e500.c
>> b/drivers/cpuidle/cpuidle-e500.c
>> new file mode 100644
>> index 0000000..ddc0def
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-e500.c
>> @@ -0,0 +1,194 @@
>> +/*
>> + * CPU Idle driver for Freescale PowerPC e500 family processors.
>> + *
>> + * Copyright 2014 Freescale Semiconductor, Inc.
>> + *
>> + * Author: Dongsheng Wang <dongsheng.wang@freescale.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#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.
>
>> +
>> +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.
>
>> +
>> +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:
>
>
> struct cpuidle_driver e500_idle_driver = {
>      .name = "e500_idle",
>      .owner = THIS_MODULE,
>      .states = {
>          .name = "pw10",
>          .desc = "pw10",
>          .flags = CPUIDLE_FLAG_TIME_VALID,
>          .target_residency = 0,
>          .enter = &pw10_enter,
>      },
>
>      ....
>
>      .state_count = 2,
> };
>
> Then in the init function you initialize the state_count consequently:
>
> if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
>      drv->state_count = 1;
>
> Then you can kill:
>
> max_idle_state, cpuidle_state_table, e500_idle_state_probe and
> pw_idle_states.
>
>> +
>> +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 ?
>
>> +static void e500_cpuidle_driver_init(void)
>> +{
>> +    int idle_state;
>> +    struct cpuidle_driver *drv = &e500_idle_driver;
>
> Pass the cpuidle_driver as parameter to the function.
>
>> +
>> +    drv->state_count = 0;
>> +
>> +    for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
>> +        if (!cpuidle_state_table[idle_state].enter)
>> +            break;
>> +
>> +        drv->states[drv->state_count] = cpuidle_state_table[idle_state];
>> +        drv->state_count++;
>> +    }
>
> This code should disappear.
>
> As this function will just initialize state_count, you can move it in
> caller and kill this function.
>
>> +}
>> +
>> +static int e500_idle_state_probe(void)
>> +{
>> +    if (cpuidle_disable != IDLE_NO_OVERRIDE)
>> +        return -ENODEV;
>> +
>> +    cpuidle_state_table = pw_idle_states;
>> +    max_idle_state = ARRAY_SIZE(pw_idle_states);
>> +
>> +    /* Disable PW20 feature for e500mc, e5500 */
>> +    if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
>> +        cpuidle_state_table[1].enter = NULL;
>> +
>> +    if (!cpuidle_state_table || !max_idle_state)
>> +        return -ENODEV;
>> +
>> +    return 0;
>> +}
>
> This code should disappear.
>
>> +static void replace_orig_idle(void *dummy)
>> +{
>> +    return;
>> +}
>> +
>> +static int __init e500_idle_init(void)
>> +{
>> +    struct cpuidle_driver *drv = &e500_idle_driver;
>> +    int err;
>> +
>> +    if (e500_idle_state_probe())
>> +        return -ENODEV;
>> +
>> +    e500_cpuidle_driver_init();
>> +    if (!drv->state_count)
>> +        return -ENODEV;
>
> No need of this check, because:
>
> 1. you know how you initialized the driver (1 or 2 states)
> 2. this is already by the cpuidle framework
>
>> +
>> +    err = cpuidle_register(drv, NULL);
>> +    if (err) {
>> +        pr_err("Register e500 family cpuidle driver failed.\n");
>
> extra carriage return.
>> +
>> +        return err;
>> +    }
>> +
>> +    err = register_cpu_notifier(&cpu_hotplug_notifier);
>> +    if (err)
>> +        pr_warn("Cpuidle driver: register cpu notifier failed.\n");
>> +
>> +    /* Replace the original way of idle after cpuidle registered. */
>> +    ppc_md.power_save = e500_cpuidle;
>> +    on_each_cpu(replace_orig_idle, NULL, 1);
>
> Why ?
>
>> +    pr_info("e500_idle_driver registered.\n");
>> +
>> +    return 0;
>> +}
>> +late_initcall(e500_idle_init);
>>
>
> Thanks
>
>    -- 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

RE: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Dongsheng.Wang]

SGkgRGFuaWVsLA0KDQpUaGFua3MgZm9yIHlvdXIgcmV2aWV3LiBJIHdpbGwgZml4IHlvdXIgY29t
bWVudHMuDQoNCkJUVywgZml4IFJhZmFlbCdzIGVtYWlsLiA6KQ0KDQo+ID4gKyNpbmNsdWRlIDxs
aW51eC9jcHUuaD4NCj4gPiArI2luY2x1ZGUgPGxpbnV4L2NwdWlkbGUuaD4NCj4gPiArI2luY2x1
ZGUgPGxpbnV4L2luaXQuaD4NCj4gPiArI2luY2x1ZGUgPGxpbnV4L2tlcm5lbC5oPg0KPiA+ICsj
aW5jbHVkZSA8bGludXgvbm90aWZpZXIuaD4NCj4gPiArDQo+ID4gKyNpbmNsdWRlIDxhc20vY3B1
dGFibGUuaD4NCj4gPiArI2luY2x1ZGUgPGFzbS9tYWNoZGVwLmg+DQo+ID4gKyNpbmNsdWRlIDxh
c20vbXBjODV4eC5oPg0KPiA+ICsNCj4gPiArc3RhdGljIHVuc2lnbmVkIGludCBtYXhfaWRsZV9z
dGF0ZTsNCj4gPiArc3RhdGljIHN0cnVjdCBjcHVpZGxlX3N0YXRlICpjcHVpZGxlX3N0YXRlX3Rh
YmxlOw0KPiA+ICsNCj4gPiArc3RydWN0IGNwdWlkbGVfZHJpdmVyIGU1MDBfaWRsZV9kcml2ZXIg
PSB7DQo+ID4gKwkubmFtZSA9ICJlNTAwX2lkbGUiLA0KPiA+ICsJLm93bmVyID0gVEhJU19NT0RV
TEUsDQo+ID4gK307DQo+ID4gKw0KPiA+ICtzdGF0aWMgdm9pZCBlNTAwX2NwdWlkbGUodm9pZCkN
Cj4gPiArew0KPiA+ICsJaWYgKGNwdWlkbGVfaWRsZV9jYWxsKCkpDQo+ID4gKwkJY3B1aWRsZV93
YWl0KCk7DQo+ID4gK30NCj4gDQo+IE5vcGUsIHRoYXQgaGFzIGJlZW4gY2hhbmdlZC4gTm8gbW9y
ZSBjYWxsIHRvIGNwdWlkbGVfaWRsZV9jYWxsIGluIGEgZHJpdmVyLg0KPiANCg0KVGhhbmtzLg0K
DQo+ID4gKw0KPiA+ICtzdGF0aWMgaW50IHB3MTBfZW50ZXIoc3RydWN0IGNwdWlkbGVfZGV2aWNl
ICpkZXYsDQo+ID4gKwkJCXN0cnVjdCBjcHVpZGxlX2RyaXZlciAqZHJ2LCBpbnQgaW5kZXgpDQo+
ID4gK3sNCj4gPiArCWNwdWlkbGVfd2FpdCgpOw0KPiA+ICsJcmV0dXJuIGluZGV4Ow0KPiA+ICt9
DQo+ID4gKw0KPiA+ICsjZGVmaW5lIE1BWF9CSVQJNjMNCj4gPiArI2RlZmluZSBNSU5fQklUCTEN
Cj4gPiArZXh0ZXJuIHUzMiBjcHVpZGxlX2VudHJ5X2JpdDsNCj4gPiArc3RhdGljIGludCBwdzIw
X2VudGVyKHN0cnVjdCBjcHVpZGxlX2RldmljZSAqZGV2LA0KPiA+ICsJCXN0cnVjdCBjcHVpZGxl
X2RyaXZlciAqZHJ2LCBpbnQgaW5kZXgpDQo+ID4gK3sNCj4gPiArCXUzMiBwdzIwX2lkbGU7DQo+
ID4gKwl1MzIgZW50cnlfYml0Ow0KPiA+ICsJcHcyMF9pZGxlID0gbWZzcHIoU1BSTl9QV1JNR1RD
UjApOw0KPiA+ICsJaWYgKChwdzIwX2lkbGUgJiBQV1JNR1RDUjBfUFcyMF9FTlQpICE9IFBXUk1H
VENSMF9QVzIwX0VOVCkgew0KPiA+ICsJCXB3MjBfaWRsZSAmPSB+UFdSTUdUQ1IwX1BXMjBfRU5U
Ow0KPiA+ICsJCWVudHJ5X2JpdCA9IE1BWF9CSVQgLSBjcHVpZGxlX2VudHJ5X2JpdDsNCj4gPiAr
CQlwdzIwX2lkbGUgfD0gKGVudHJ5X2JpdCA8PCBQV1JNR1RDUjBfUFcyMF9FTlRfU0hJRlQpOw0K
PiA+ICsJCW10c3ByKFNQUk5fUFdSTUdUQ1IwLCBwdzIwX2lkbGUpOw0KPiA+ICsJfQ0KPiA+ICsN
Cj4gPiArCWNwdWlkbGVfd2FpdCgpOw0KPiA+ICsNCj4gPiArCXB3MjBfaWRsZSAmPSB+UFdSTUdU
Q1IwX1BXMjBfRU5UOw0KPiA+ICsJcHcyMF9pZGxlIHw9IChNSU5fQklUIDw8IFBXUk1HVENSMF9Q
VzIwX0VOVF9TSElGVCk7DQo+ID4gKwltdHNwcihTUFJOX1BXUk1HVENSMCwgcHcyMF9pZGxlKTsN
Cj4gPiArDQo+ID4gKwlyZXR1cm4gaW5kZXg7DQo+ID4gK30NCj4gDQo+IElzIGl0IHBvc3NpYmxl
IHRvIGdpdmUgc29tZSBjb21tZW50cyBhbmQgZW5jYXBzdWxhdGUgdGhlIGNvZGUgd2l0aA0KPiBl
eHBsaWNpdCBmdW5jdGlvbiBuYW1lcyB0byBiZSBpbXBsZW1lbnRlZCBpbiBhbiBhcmNoIHNwZWNp
ZmljIGRpcmVjdG9yeQ0KPiBmaWxlIChlZy4gcG0uYykgYW5kIGV4cG9ydCB0aGVzZSBmdW5jdGlv
bnMgaW4gYSBsaW51eC8gaGVhZGVyID8gV2UgdHJ5DQo+IHRvIHByZXZlbnQgdG8gaW5jbHVkZSBh
c20gaWYgcG9zc2libGUuDQo+IA0KDQpZZXAsIExvb2tzIGJldHRlci4gVGhhbmtzLg0KDQo+ID4g
Kw0KPiA+ICtzdGF0aWMgc3RydWN0IGNwdWlkbGVfc3RhdGUgcHdfaWRsZV9zdGF0ZXNbXSA9IHsN
Cj4gPiArCXsNCj4gPiArCQkubmFtZSA9ICJwdzEwIiwNCj4gPiArCQkuZGVzYyA9ICJwdzEwIiwN
Cj4gPiArCQkuZmxhZ3MgPSBDUFVJRExFX0ZMQUdfVElNRV9WQUxJRCwNCj4gPiArCQkuZXhpdF9s
YXRlbmN5ID0gMCwNCj4gPiArCQkudGFyZ2V0X3Jlc2lkZW5jeSA9IDAsDQo+ID4gKwkJLmVudGVy
ID0gJnB3MTBfZW50ZXINCj4gPiArCX0sDQo+ID4gKw0KPiA+ICsJew0KPiA+ICsJCS5uYW1lID0g
InB3MjAiLA0KPiA+ICsJCS5kZXNjID0gInB3MjAtY29yZS1pZGxlIiwNCj4gPiArCQkuZmxhZ3Mg
PSBDUFVJRExFX0ZMQUdfVElNRV9WQUxJRCwNCj4gPiArCQkuZXhpdF9sYXRlbmN5ID0gMSwNCj4g
PiArCQkudGFyZ2V0X3Jlc2lkZW5jeSA9IDUwLA0KPiA+ICsJCS5lbnRlciA9ICZwdzIwX2VudGVy
DQo+ID4gKwl9LA0KPiA+ICt9Ow0KPiANCj4gTm8gbmVlZCB0byBkZWZpbmUgdGhpcyBpbnRlcm1l
ZGlhdGUgc3RydWN0dXJlIGhlcmUsIHlvdSBjYW4gZGlyZWN0bHkNCj4gaW5pdGlhbGl6ZSB0aGUg
Y3B1aWRsZV9kcml2ZXI6DQo+IA0KDQpUaGFua3MuIDopDQoNCj4gPiArc3RhdGljIGludCBjcHVf
aG90cGx1Z19ub3RpZnkoc3RydWN0IG5vdGlmaWVyX2Jsb2NrICpuLA0KPiA+ICsJCQl1bnNpZ25l
ZCBsb25nIGFjdGlvbiwgdm9pZCAqaGNwdSkNCj4gPiArew0KPiA+ICsJdW5zaWduZWQgbG9uZyBo
b3RjcHUgPSAodW5zaWduZWQgbG9uZyloY3B1Ow0KPiA+ICsJc3RydWN0IGNwdWlkbGVfZGV2aWNl
ICpkZXYgPQ0KPiA+ICsJCQlwZXJfY3B1X3B0cihjcHVpZGxlX2RldmljZXMsIGhvdGNwdSk7DQo+
ID4gKw0KPiA+ICsJaWYgKGRldiAmJiBjcHVpZGxlX2dldF9kcml2ZXIoKSkgew0KPiA+ICsJCXN3
aXRjaCAoYWN0aW9uKSB7DQo+ID4gKwkJY2FzZSBDUFVfT05MSU5FOg0KPiA+ICsJCWNhc2UgQ1BV
X09OTElORV9GUk9aRU46DQo+ID4gKwkJCWNwdWlkbGVfcGF1c2VfYW5kX2xvY2soKTsNCj4gPiAr
CQkJY3B1aWRsZV9lbmFibGVfZGV2aWNlKGRldik7DQo+ID4gKwkJCWNwdWlkbGVfcmVzdW1lX2Fu
ZF91bmxvY2soKTsNCj4gPiArCQkJYnJlYWs7DQo+ID4gKw0KPiA+ICsJCWNhc2UgQ1BVX0RFQUQ6
DQo+ID4gKwkJY2FzZSBDUFVfREVBRF9GUk9aRU46DQo+ID4gKwkJCWNwdWlkbGVfcGF1c2VfYW5k
X2xvY2soKTsNCj4gPiArCQkJY3B1aWRsZV9kaXNhYmxlX2RldmljZShkZXYpOw0KPiA+ICsJCQlj
cHVpZGxlX3Jlc3VtZV9hbmRfdW5sb2NrKCk7DQo+ID4gKwkJCWJyZWFrOw0KPiA+ICsNCj4gPiAr
CQlkZWZhdWx0Og0KPiA+ICsJCQlyZXR1cm4gTk9USUZZX0RPTkU7DQo+ID4gKwkJfQ0KPiA+ICsJ
fQ0KPiA+ICsNCj4gPiArCXJldHVybiBOT1RJRllfT0s7DQo+ID4gK30NCj4gPiArDQo+ID4gK3N0
YXRpYyBzdHJ1Y3Qgbm90aWZpZXJfYmxvY2sgY3B1X2hvdHBsdWdfbm90aWZpZXIgPSB7DQo+ID4g
Kwkubm90aWZpZXJfY2FsbCA9IGNwdV9ob3RwbHVnX25vdGlmeSwNCj4gPiArfTsNCj4gDQo+IENh
biB5b3UgZXhwbGFpbiB3aHkgdGhpcyBpcyBuZWVkZWQgPw0KPiANCg0KSWYgYSBjcHUgd2lsbCBi
ZSBwbHVnZ2VkIG91dC9pbiwgV2Ugc2hvdWxkIGJlIGxldCBDcHVpZGxlIGtub3cgdG8gcmVtb3Zl
DQpjb3JyZXNwb25kaW5nIHN5cyBpbnRlcmZhY2UgYW5kIGRpc2FibGUvZW5hYmxlIGNwdWRpbGUt
Z292ZXJub3IgZm9yIGN1cnJlbnQgY3B1Lg0KDQo+ID4gKwllcnIgPSByZWdpc3Rlcl9jcHVfbm90
aWZpZXIoJmNwdV9ob3RwbHVnX25vdGlmaWVyKTsNCj4gPiArCWlmIChlcnIpDQo+ID4gKwkJcHJf
d2FybigiQ3B1aWRsZSBkcml2ZXI6IHJlZ2lzdGVyIGNwdSBub3RpZmllciBmYWlsZWQuXG4iKTsN
Cj4gPiArDQo+ID4gKwkvKiBSZXBsYWNlIHRoZSBvcmlnaW5hbCB3YXkgb2YgaWRsZSBhZnRlciBj
cHVpZGxlIHJlZ2lzdGVyZWQuICovDQo+ID4gKwlwcGNfbWQucG93ZXJfc2F2ZSA9IGU1MDBfY3B1
aWRsZTsNCj4gPiArCW9uX2VhY2hfY3B1KHJlcGxhY2Vfb3JpZ19pZGxlLCBOVUxMLCAxKTsNCj4g
DQo+IFdoeSA/DQo+IA0KDQpJIGNoZWNrZWQgYWdhaW4sIGlmIHdlIHB1dCBjcHVpZGxlX2lkbGVf
Y2FsbCBpbiBhc20sIHRoaXMgaXMgbm90IG5lZWQuDQoNClJlZ2FyZHMsDQotRG9uZ3NoZW5nDQoN
Cj4gPiArCXByX2luZm8oImU1MDBfaWRsZV9kcml2ZXIgcmVnaXN0ZXJlZC5cbiIpOw0KPiA+ICsN
Cj4gPiArCXJldHVybiAwOw0KPiA+ICt9DQo+ID4gK2xhdGVfaW5pdGNhbGwoZTUwMF9pZGxlX2lu
aXQpOw0KPiA+DQo+IA0KPiBUaGFua3MNCj4gDQo+ICAgIC0tIERhbmllbA0KPiANCj4gDQo+IC0t
DQo+ICAgPGh0dHA6Ly93d3cubGluYXJvLm9yZy8+IExpbmFyby5vcmcg4pSCIE9wZW4gc291cmNl
IHNvZnR3YXJlIGZvciBBUk0gU29Dcw0KPiANCj4gRm9sbG93IExpbmFybzogIDxodHRwOi8vd3d3
LmZhY2Vib29rLmNvbS9wYWdlcy9MaW5hcm8+IEZhY2Vib29rIHwNCj4gPGh0dHA6Ly90d2l0dGVy
LmNvbS8jIS9saW5hcm9vcmc+IFR3aXR0ZXIgfA0KPiA8aHR0cDovL3d3dy5saW5hcm8ub3JnL2xp
bmFyby1ibG9nLz4gQmxvZw0KPiANCj4gDQoNCg==

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano]

On 04/03/2014 05:20 AM, Dongsheng.Wang@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 ?


>>> +	err = register_cpu_notifier(&cpu_hotplug_notifier);
>>> +	if (err)
>>> +		pr_warn("Cpuidle driver: register cpu notifier failed.\n");
>>> +
>>> +	/* Replace the original way of idle after cpuidle registered. */
>>> +	ppc_md.power_save = e500_cpuidle;
>>> +	on_each_cpu(replace_orig_idle, NULL, 1);
>>
>> Why ?
>>
>
> I checked again, if we put cpuidle_idle_call in asm, this is not need.
>
> Regards,
> -Dongsheng
>
>>> +	pr_info("e500_idle_driver registered.\n");
>>> +
>>> +	return 0;
>>> +}
>>> +late_initcall(e500_idle_init);
>>>
>>
>> Thanks
>>
>>     -- 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

RE: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Dongsheng.Wang]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRGFuaWVsIExlemNhbm8g
W21haWx0bzpkYW5pZWwubGV6Y2Fub0BsaW5hcm8ub3JnXQ0KPiBTZW50OiBUaHVyc2RheSwgQXBy
aWwgMDMsIDIwMTQgMjoyOSBQTQ0KPiBUbzogV2FuZyBEb25nc2hlbmctQjQwNTM0OyBXb29kIFNj
b3R0LUIwNzQyMQ0KPiBDYzogcmp3QHJqd3lzb2NraS5uZXQ7IExpIFlhbmctTGVvLVI1ODQ3Mjsg
SmluIFpoZW5neGlvbmctUjY0MTg4OyBaaGFvIENoZW5odWktDQo+IEIzNTMzNjsgbGludXgtcG1A
dmdlci5rZXJuZWwub3JnOyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZw0KPiBTdWJqZWN0
OiBSZTogW1BBVENIXSBjcHVpZGxlOiBhZGQgZnJlZXNjYWxlIGU1MDAgZmFtaWx5IHBvcmNlc3Nv
cnMgaWRsZSBzdXBwb3J0DQo+IA0KPiBPbiAwNC8wMy8yMDE0IDA1OjIwIEFNLCBEb25nc2hlbmcu
V2FuZ0BmcmVlc2NhbGUuY29tIHdyb3RlOg0KPiA+IEhpIERhbmllbCwNCj4gPg0KPiA+IFRoYW5r
cyBmb3IgeW91ciByZXZpZXcuIEkgd2lsbCBmaXggeW91ciBjb21tZW50cy4NCj4gPg0KPiA+IEJU
VywgZml4IFJhZmFlbCdzIGVtYWlsLiA6KQ0KPiA+DQo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L2Nw
dS5oPg0KPiA+Pj4gKyNpbmNsdWRlIDxsaW51eC9jcHVpZGxlLmg+DQo+ID4+PiArI2luY2x1ZGUg
PGxpbnV4L2luaXQuaD4NCj4gPj4+ICsjaW5jbHVkZSA8bGludXgva2VybmVsLmg+DQo+ID4+PiAr
I2luY2x1ZGUgPGxpbnV4L25vdGlmaWVyLmg+DQo+ID4+PiArDQo+ID4+PiArI2luY2x1ZGUgPGFz
bS9jcHV0YWJsZS5oPg0KPiA+Pj4gKyNpbmNsdWRlIDxhc20vbWFjaGRlcC5oPg0KPiA+Pj4gKyNp
bmNsdWRlIDxhc20vbXBjODV4eC5oPg0KPiA+Pj4gKw0KPiA+Pj4gK3N0YXRpYyB1bnNpZ25lZCBp
bnQgbWF4X2lkbGVfc3RhdGU7IHN0YXRpYyBzdHJ1Y3QgY3B1aWRsZV9zdGF0ZQ0KPiA+Pj4gKypj
cHVpZGxlX3N0YXRlX3RhYmxlOw0KPiA+Pj4gKw0KPiA+Pj4gK3N0cnVjdCBjcHVpZGxlX2RyaXZl
ciBlNTAwX2lkbGVfZHJpdmVyID0gew0KPiA+Pj4gKwkubmFtZSA9ICJlNTAwX2lkbGUiLA0KPiA+
Pj4gKwkub3duZXIgPSBUSElTX01PRFVMRSwNCj4gPj4+ICt9Ow0KPiA+Pj4gKw0KPiA+Pj4gK3N0
YXRpYyB2b2lkIGU1MDBfY3B1aWRsZSh2b2lkKQ0KPiA+Pj4gK3sNCj4gPj4+ICsJaWYgKGNwdWlk
bGVfaWRsZV9jYWxsKCkpDQo+ID4+PiArCQljcHVpZGxlX3dhaXQoKTsNCj4gPj4+ICt9DQo+ID4+
DQo+ID4+IE5vcGUsIHRoYXQgaGFzIGJlZW4gY2hhbmdlZC4gTm8gbW9yZSBjYWxsIHRvIGNwdWlk
bGVfaWRsZV9jYWxsIGluIGEgZHJpdmVyLg0KPiA+Pg0KPiA+DQo+ID4gVGhhbmtzLg0KPiA+DQo+
ID4+PiArDQo+ID4+PiArc3RhdGljIGludCBwdzEwX2VudGVyKHN0cnVjdCBjcHVpZGxlX2Rldmlj
ZSAqZGV2LA0KPiA+Pj4gKwkJCXN0cnVjdCBjcHVpZGxlX2RyaXZlciAqZHJ2LCBpbnQgaW5kZXgp
IHsNCj4gPj4+ICsJY3B1aWRsZV93YWl0KCk7DQo+ID4+PiArCXJldHVybiBpbmRleDsNCj4gPj4+
ICt9DQo+ID4+PiArDQo+ID4+PiArI2RlZmluZSBNQVhfQklUCTYzDQo+ID4+PiArI2RlZmluZSBN
SU5fQklUCTENCj4gPj4+ICtleHRlcm4gdTMyIGNwdWlkbGVfZW50cnlfYml0Ow0KPiA+Pj4gK3N0
YXRpYyBpbnQgcHcyMF9lbnRlcihzdHJ1Y3QgY3B1aWRsZV9kZXZpY2UgKmRldiwNCj4gPj4+ICsJ
CXN0cnVjdCBjcHVpZGxlX2RyaXZlciAqZHJ2LCBpbnQgaW5kZXgpIHsNCj4gPj4+ICsJdTMyIHB3
MjBfaWRsZTsNCj4gPj4+ICsJdTMyIGVudHJ5X2JpdDsNCj4gPj4+ICsJcHcyMF9pZGxlID0gbWZz
cHIoU1BSTl9QV1JNR1RDUjApOw0KPiA+Pj4gKwlpZiAoKHB3MjBfaWRsZSAmIFBXUk1HVENSMF9Q
VzIwX0VOVCkgIT0gUFdSTUdUQ1IwX1BXMjBfRU5UKSB7DQo+ID4+PiArCQlwdzIwX2lkbGUgJj0g
flBXUk1HVENSMF9QVzIwX0VOVDsNCj4gPj4+ICsJCWVudHJ5X2JpdCA9IE1BWF9CSVQgLSBjcHVp
ZGxlX2VudHJ5X2JpdDsNCj4gPj4+ICsJCXB3MjBfaWRsZSB8PSAoZW50cnlfYml0IDw8IFBXUk1H
VENSMF9QVzIwX0VOVF9TSElGVCk7DQo+ID4+PiArCQltdHNwcihTUFJOX1BXUk1HVENSMCwgcHcy
MF9pZGxlKTsNCj4gPj4+ICsJfQ0KPiA+Pj4gKw0KPiA+Pj4gKwljcHVpZGxlX3dhaXQoKTsNCj4g
Pj4+ICsNCj4gPj4+ICsJcHcyMF9pZGxlICY9IH5QV1JNR1RDUjBfUFcyMF9FTlQ7DQo+ID4+PiAr
CXB3MjBfaWRsZSB8PSAoTUlOX0JJVCA8PCBQV1JNR1RDUjBfUFcyMF9FTlRfU0hJRlQpOw0KPiA+
Pj4gKwltdHNwcihTUFJOX1BXUk1HVENSMCwgcHcyMF9pZGxlKTsNCj4gPj4+ICsNCj4gPj4+ICsJ
cmV0dXJuIGluZGV4Ow0KPiA+Pj4gK30NCj4gPj4NCj4gPj4gSXMgaXQgcG9zc2libGUgdG8gZ2l2
ZSBzb21lIGNvbW1lbnRzIGFuZCBlbmNhcHN1bGF0ZSB0aGUgY29kZSB3aXRoDQo+ID4+IGV4cGxp
Y2l0IGZ1bmN0aW9uIG5hbWVzIHRvIGJlIGltcGxlbWVudGVkIGluIGFuIGFyY2ggc3BlY2lmaWMN
Cj4gPj4gZGlyZWN0b3J5IGZpbGUgKGVnLiBwbS5jKSBhbmQgZXhwb3J0IHRoZXNlIGZ1bmN0aW9u
cyBpbiBhIGxpbnV4Lw0KPiA+PiBoZWFkZXIgPyBXZSB0cnkgdG8gcHJldmVudCB0byBpbmNsdWRl
IGFzbSBpZiBwb3NzaWJsZS4NCj4gPj4NCj4gPg0KPiA+IFllcCwgTG9va3MgYmV0dGVyLiBUaGFu
a3MuDQo+ID4NCj4gPj4+ICsNCj4gPj4+ICtzdGF0aWMgc3RydWN0IGNwdWlkbGVfc3RhdGUgcHdf
aWRsZV9zdGF0ZXNbXSA9IHsNCj4gPj4+ICsJew0KPiA+Pj4gKwkJLm5hbWUgPSAicHcxMCIsDQo+
ID4+PiArCQkuZGVzYyA9ICJwdzEwIiwNCj4gPj4+ICsJCS5mbGFncyA9IENQVUlETEVfRkxBR19U
SU1FX1ZBTElELA0KPiA+Pj4gKwkJLmV4aXRfbGF0ZW5jeSA9IDAsDQo+ID4+PiArCQkudGFyZ2V0
X3Jlc2lkZW5jeSA9IDAsDQo+ID4+PiArCQkuZW50ZXIgPSAmcHcxMF9lbnRlcg0KPiA+Pj4gKwl9
LA0KPiA+Pj4gKw0KPiA+Pj4gKwl7DQo+ID4+PiArCQkubmFtZSA9ICJwdzIwIiwNCj4gPj4+ICsJ
CS5kZXNjID0gInB3MjAtY29yZS1pZGxlIiwNCj4gPj4+ICsJCS5mbGFncyA9IENQVUlETEVfRkxB
R19USU1FX1ZBTElELA0KPiA+Pj4gKwkJLmV4aXRfbGF0ZW5jeSA9IDEsDQo+ID4+PiArCQkudGFy
Z2V0X3Jlc2lkZW5jeSA9IDUwLA0KPiA+Pj4gKwkJLmVudGVyID0gJnB3MjBfZW50ZXINCj4gPj4+
ICsJfSwNCj4gPj4+ICt9Ow0KPiA+Pg0KPiA+PiBObyBuZWVkIHRvIGRlZmluZSB0aGlzIGludGVy
bWVkaWF0ZSBzdHJ1Y3R1cmUgaGVyZSwgeW91IGNhbiBkaXJlY3RseQ0KPiA+PiBpbml0aWFsaXpl
IHRoZSBjcHVpZGxlX2RyaXZlcjoNCj4gPj4NCj4gPg0KPiA+IFRoYW5rcy4gOikNCj4gPg0KPiA+
Pj4gK3N0YXRpYyBpbnQgY3B1X2hvdHBsdWdfbm90aWZ5KHN0cnVjdCBub3RpZmllcl9ibG9jayAq
biwNCj4gPj4+ICsJCQl1bnNpZ25lZCBsb25nIGFjdGlvbiwgdm9pZCAqaGNwdSkgew0KPiA+Pj4g
Kwl1bnNpZ25lZCBsb25nIGhvdGNwdSA9ICh1bnNpZ25lZCBsb25nKWhjcHU7DQo+ID4+PiArCXN0
cnVjdCBjcHVpZGxlX2RldmljZSAqZGV2ID0NCj4gPj4+ICsJCQlwZXJfY3B1X3B0cihjcHVpZGxl
X2RldmljZXMsIGhvdGNwdSk7DQo+ID4+PiArDQo+ID4+PiArCWlmIChkZXYgJiYgY3B1aWRsZV9n
ZXRfZHJpdmVyKCkpIHsNCj4gPj4+ICsJCXN3aXRjaCAoYWN0aW9uKSB7DQo+ID4+PiArCQljYXNl
IENQVV9PTkxJTkU6DQo+ID4+PiArCQljYXNlIENQVV9PTkxJTkVfRlJPWkVOOg0KPiA+Pj4gKwkJ
CWNwdWlkbGVfcGF1c2VfYW5kX2xvY2soKTsNCj4gPj4+ICsJCQljcHVpZGxlX2VuYWJsZV9kZXZp
Y2UoZGV2KTsNCj4gPj4+ICsJCQljcHVpZGxlX3Jlc3VtZV9hbmRfdW5sb2NrKCk7DQo+ID4+PiAr
CQkJYnJlYWs7DQo+ID4+PiArDQo+ID4+PiArCQljYXNlIENQVV9ERUFEOg0KPiA+Pj4gKwkJY2Fz
ZSBDUFVfREVBRF9GUk9aRU46DQo+ID4+PiArCQkJY3B1aWRsZV9wYXVzZV9hbmRfbG9jaygpOw0K
PiA+Pj4gKwkJCWNwdWlkbGVfZGlzYWJsZV9kZXZpY2UoZGV2KTsNCj4gPj4+ICsJCQljcHVpZGxl
X3Jlc3VtZV9hbmRfdW5sb2NrKCk7DQo+ID4+PiArCQkJYnJlYWs7DQo+ID4+PiArDQo+ID4+PiAr
CQlkZWZhdWx0Og0KPiA+Pj4gKwkJCXJldHVybiBOT1RJRllfRE9ORTsNCj4gPj4+ICsJCX0NCj4g
Pj4+ICsJfQ0KPiA+Pj4gKw0KPiA+Pj4gKwlyZXR1cm4gTk9USUZZX09LOw0KPiA+Pj4gK30NCj4g
Pj4+ICsNCj4gPj4+ICtzdGF0aWMgc3RydWN0IG5vdGlmaWVyX2Jsb2NrIGNwdV9ob3RwbHVnX25v
dGlmaWVyID0gew0KPiA+Pj4gKwkubm90aWZpZXJfY2FsbCA9IGNwdV9ob3RwbHVnX25vdGlmeSwg
fTsNCj4gPj4NCj4gPj4gQ2FuIHlvdSBleHBsYWluIHdoeSB0aGlzIGlzIG5lZWRlZCA/DQo+ID4+
DQo+ID4NCj4gPiBJZiBhIGNwdSB3aWxsIGJlIHBsdWdnZWQgb3V0L2luLCBXZSBzaG91bGQgYmUg
bGV0IENwdWlkbGUga25vdyB0bw0KPiA+IHJlbW92ZSBjb3JyZXNwb25kaW5nIHN5cyBpbnRlcmZh
Y2UgYW5kIGRpc2FibGUvZW5hYmxlIGNwdWRpbGUtZ292ZXJub3IgZm9yDQo+IGN1cnJlbnQgY3B1
Lg0KPiANCj4gT2ssIHRoaXMgY29kZSBpcyBhIGNvcHktcGFzdGUgb2YgdGhlIHBvd2VycycgY3B1
aWRsZSBkcml2ZXIuDQo+IA0KPiBJSVJDLCBJIHBvc3RlZCBhIHBhdGNoc2V0IHRvIG1vdmUgdGhp
cyBwb3J0aW9uIG9mIGNvZGUgaW4gdGhlIGNwdWlkbGUgY29tbW9uDQo+IGZyYW1ld29yayBzb21l
IHRpbWUgYWdvLg0KPiANCj4gQ291bGQgeW91IHBsZWFzZSBnZXQgcmlkIG9mIHRoaXMgcGFydCBv
ZiBjb2RlID8NCj4gDQoNClllcywgSSBjYW4uIDopIENvdWxkIHlvdSBzaGFyZSBtZSB5b3VyIHBh
dGNoc2V0IGxpbms/IEkgY2FuJ3QgZm91bmQgdGhlbSBvbiB5b3VyIHRyZWUuDQoNClJlZ2FyZHMs
DQotRG9uZ3NoZW5nDQoNCj4gDQo+ID4+PiArCWVyciA9IHJlZ2lzdGVyX2NwdV9ub3RpZmllcigm
Y3B1X2hvdHBsdWdfbm90aWZpZXIpOw0KPiA+Pj4gKwlpZiAoZXJyKQ0KPiA+Pj4gKwkJcHJfd2Fy
bigiQ3B1aWRsZSBkcml2ZXI6IHJlZ2lzdGVyIGNwdSBub3RpZmllciBmYWlsZWQuXG4iKTsNCj4g
Pj4+ICsNCj4gPj4+ICsJLyogUmVwbGFjZSB0aGUgb3JpZ2luYWwgd2F5IG9mIGlkbGUgYWZ0ZXIg
Y3B1aWRsZSByZWdpc3RlcmVkLiAqLw0KPiA+Pj4gKwlwcGNfbWQucG93ZXJfc2F2ZSA9IGU1MDBf
Y3B1aWRsZTsNCj4gPj4+ICsJb25fZWFjaF9jcHUocmVwbGFjZV9vcmlnX2lkbGUsIE5VTEwsIDEp
Ow0KPiA+Pg0KPiA+PiBXaHkgPw0KPiA+Pg0KPiA+DQo+ID4gSSBjaGVja2VkIGFnYWluLCBpZiB3
ZSBwdXQgY3B1aWRsZV9pZGxlX2NhbGwgaW4gYXNtLCB0aGlzIGlzIG5vdCBuZWVkLg0KPiA+DQo+
ID4gUmVnYXJkcywNCj4gPiAtRG9uZ3NoZW5nDQo+ID4NCj4gPj4+ICsJcHJfaW5mbygiZTUwMF9p
ZGxlX2RyaXZlciByZWdpc3RlcmVkLlxuIik7DQo+ID4+PiArDQo+ID4+PiArCXJldHVybiAwOw0K
PiA+Pj4gK30NCj4gPj4+ICtsYXRlX2luaXRjYWxsKGU1MDBfaWRsZV9pbml0KTsNCj4gPj4+DQo+
ID4+DQo+ID4+IFRoYW5rcw0KPiA+Pg0KPiA+PiAgICAgLS0gRGFuaWVsDQo+ID4+DQo+ID4+DQo+
ID4+IC0tDQo+ID4+ICAgIDxodHRwOi8vd3d3LmxpbmFyby5vcmcvPiBMaW5hcm8ub3JnIOKUgiBP
cGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNDQo+ID4+IFNvQ3MNCj4gPj4NCj4gPj4gRm9sbG93
IExpbmFybzogIDxodHRwOi8vd3d3LmZhY2Vib29rLmNvbS9wYWdlcy9MaW5hcm8+IEZhY2Vib29r
IHwNCj4gPj4gPGh0dHA6Ly90d2l0dGVyLmNvbS8jIS9saW5hcm9vcmc+IFR3aXR0ZXIgfA0KPiA+
PiA8aHR0cDovL3d3dy5saW5hcm8ub3JnL2xpbmFyby1ibG9nLz4gQmxvZw0KPiA+Pg0KPiA+Pg0K
PiA+DQo+IA0KPiANCj4gLS0NCj4gICA8aHR0cDovL3d3dy5saW5hcm8ub3JnLz4gTGluYXJvLm9y
ZyDilIIgT3BlbiBzb3VyY2Ugc29mdHdhcmUgZm9yIEFSTSBTb0NzDQo+IA0KPiBGb2xsb3cgTGlu
YXJvOiAgPGh0dHA6Ly93d3cuZmFjZWJvb2suY29tL3BhZ2VzL0xpbmFybz4gRmFjZWJvb2sgfA0K
PiA8aHR0cDovL3R3aXR0ZXIuY29tLyMhL2xpbmFyb29yZz4gVHdpdHRlciB8IDxodHRwOi8vd3d3
LmxpbmFyby5vcmcvbGluYXJvLWJsb2cvPg0KPiBCbG9nDQo+IA0KPiANCg0K

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano]

On 04/03/2014 10:03 AM, Dongsheng.Wang@freescale.com wrote:
>
>
>> -----Original Message-----
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> Sent: Thursday, April 03, 2014 2:29 PM
>> To: Wang Dongsheng-B40534; Wood Scott-B07421
>> Cc: rjw@rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao Chenhui-
>> B35336; linux-pm@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.Wang@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

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Scott Wood]

On Tue, 2014-04-01 at 16:33 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> Add cpuidle support for e500 family, using cpuidle framework to
> manage various low power modes. The new implementation will remain
> compatible with original idle method.
> 
> I have done test about power consumption and latency. Cpuidle framework
> will make CPU response time faster than original method, but power
> consumption is higher than original method.
> 
> Power consumption:
> The original method, power consumption is 10.51202 (W).
> The cpuidle framework, power consumption is 10.5311 (W).
> 
> Latency:
> The original method, avg latency is 6782 (us).
> The cpuidle framework, avg latency is 6482 (us).
> 
> Initially, this supports PW10, PW20 and subsequent patches will support
> DOZE/NAP and PH10, PH20.

Have you tried tuning the timebase bit for the original method, to match
what you're doing in cpuidle and/or for general optimization?

Do you get similar results for a variety of workloads?

> +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
> +	},
> +};

Where did exit_latency and target_residency come from?  It looks rather
different from the ~1ms delay before entering PW20 with the original
method.

Though, I'd have expected a shorter PW20 delay to have longer wake
latency, due to entering the lower power state more often...

> +static void replace_orig_idle(void *dummy)
> +{
> +	return;
> +}

Why?  If you're using this as some sort of barrier to ensure that all
CPUs have done something before continuing, explain that, along with why
it matters.

-Scott

[PATCH] cpuidle: add freescale e500 family porcessors idle support

[Dongsheng Wang]

From: Wang Dongsheng <dongsheng.wang@freescale.com>

Add cpuidle support for e500 family, using cpuidle framework to
manage various low power modes. The new implementation will remain
compatible with original idle method.

Initially, this supports PW10, and subsequent patches will support
PW20/DOZE/NAP.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
This patch keep using cpuidle_register_device(), because we need to support cpu
hotplug. I will fix "device" issue in this driver, after
Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler into cpuidle
freamwork.

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 8b48090..cbdbe25 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -271,6 +271,16 @@ extern void power7_idle(void);
 extern void ppc6xx_idle(void);
 extern void book3e_idle(void);
 
+/* Wait for Interrupt */
+static inline void fsl_cpuidle_wait(void)
+{
+#ifdef CONFIG_PPC64
+	book3e_idle();
+#else
+	e500_idle();
+#endif
+}
+
 /*
  * ppc_md contains a copy of the machine description structure for the
  * current platform. machine_id contains the initial address where the
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index b3fb81d..7ed114b 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -35,6 +35,11 @@ depends on ARM
 source "drivers/cpuidle/Kconfig.arm"
 endmenu
 
+menu "PowerPC CPU Idle Drivers"
+depends on PPC32 || PPC64
+source "drivers/cpuidle/Kconfig.powerpc"
+endmenu
+
 endif
 
 config ARCH_NEEDS_CPU_IDLE_COUPLED
diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc
new file mode 100644
index 0000000..9f3f5ef
--- /dev/null
+++ b/drivers/cpuidle/Kconfig.powerpc
@@ -0,0 +1,9 @@
+#
+# PowerPC CPU Idle drivers
+#
+
+config PPC_E500_CPUIDLE
+	bool "CPU Idle Driver for E500 family processors"
+	depends on FSL_SOC_BOOKE
+	help
+	  Select this to enable cpuidle on e500 family processors.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 0b9d200..0dde3db 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -11,3 +11,7 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
+
+##################################################################################
+# PowerPC platform drivers
+obj-$(CONFIG_PPC_E500_CPUIDLE)		+= cpuidle-e500.o
diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c
new file mode 100644
index 0000000..1919cea
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-e500.c
@@ -0,0 +1,222 @@
+/*
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * CPU Idle driver for Freescale PowerPC e500 family processors.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
+ */
+
+#include <linux/cpu.h>
+#include <linux/cpuidle.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+
+#include <asm/machdep.h>
+
+static struct cpuidle_driver e500_idle_driver = {
+	.name = "e500_idle",
+	.owner = THIS_MODULE,
+};
+
+static struct cpuidle_device __percpu *e500_cpuidle_devices;
+
+static void e500_cpuidle(void)
+{
+	/*
+	 * This would call on the cpuidle framework, and the back-end
+	 * driver to go to idle states.
+	 */
+	if (cpuidle_idle_call()) {
+		/*
+		 * On error, execute default handler
+		 * to go into low thread priority and possibly
+		 * low power mode.
+		 */
+		HMT_low();
+		HMT_very_low();
+	}
+}
+
+static int pw10_enter(struct cpuidle_device *dev,
+			struct cpuidle_driver *drv, int index)
+{
+	fsl_cpuidle_wait();
+	return index;
+}
+
+static struct cpuidle_state fsl_pw_idle_states[] = {
+	{
+		.name = "pw10",
+		.desc = "pw10",
+		.flags = CPUIDLE_FLAG_TIME_VALID,
+		.exit_latency = 0,
+		.target_residency = 0,
+		.enter = &pw10_enter
+	},
+};
+
+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(e500_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,
+};
+
+/*
+ * e500_idle_devices_init(void)
+ * allocate, initialize and register cpuidle device
+ */
+static int e500_idle_devices_init(void)
+{
+	int i;
+	struct cpuidle_driver *drv = &e500_idle_driver;
+	struct cpuidle_device *dev;
+
+	e500_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (!e500_cpuidle_devices)
+		return -ENOMEM;
+
+	for_each_possible_cpu(i) {
+		dev = per_cpu_ptr(e500_cpuidle_devices, i);
+		dev->state_count = drv->state_count;
+		dev->cpu = i;
+
+		if (cpuidle_register_device(dev)) {
+			pr_err("cpuidle_register_device %d failed!\n", i);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * e500_idle_devices_uninit(void)
+ * unregister cpuidle devices and de-allocate memory
+ */
+static void e500_idle_devices_uninit(void)
+{
+	int i;
+	struct cpuidle_device *dev;
+
+	if (!e500_cpuidle_devices)
+		return;
+
+	for_each_possible_cpu(i) {
+		dev = per_cpu_ptr(e500_cpuidle_devices, i);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(e500_cpuidle_devices);
+}
+
+static void e500_cpuidle_driver_init(unsigned int max_idle_state,
+		struct cpuidle_state *cpuidle_state_table)
+{
+	int idle_state;
+	struct cpuidle_driver *drv = &e500_idle_driver;
+
+	drv->state_count = 0;
+
+	for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
+		if (!cpuidle_state_table[idle_state].enter)
+			break;
+
+		drv->states[drv->state_count] = cpuidle_state_table[idle_state];
+		drv->state_count++;
+	}
+}
+
+static int cpu_is_feature(unsigned long feature)
+{
+	return (cur_cpu_spec->cpu_features == feature);
+}
+
+static int __init e500_idle_init(void)
+{
+	struct cpuidle_state *cpuidle_state_table = NULL;
+	struct cpuidle_driver *drv = &e500_idle_driver;
+	int err;
+	unsigned int max_idle_state = 0;
+
+	if (cpuidle_disable != IDLE_NO_OVERRIDE)
+		return -ENODEV;
+
+	if (cpu_is_feature(CPU_FTRS_E500MC) || cpu_is_feature(CPU_FTRS_E5500) ||
+			cpu_is_feature(CPU_FTRS_E6500)) {
+		cpuidle_state_table = fsl_pw_idle_states;
+		max_idle_state = ARRAY_SIZE(fsl_pw_idle_states);
+	}
+
+	if (!cpuidle_state_table || !max_idle_state)
+		return -EPERM;
+
+	e500_cpuidle_driver_init(max_idle_state, cpuidle_state_table);
+
+	if (!drv->state_count)
+		return -EPERM;
+
+	err = cpuidle_register_driver(drv);
+	if (err) {
+		pr_err("Register e500 family cpuidle driver failed.\n");
+
+		return err;
+	}
+
+	err = e500_idle_devices_init();
+	if (err)
+		goto out;
+
+	err = register_cpu_notifier(&cpu_hotplug_notifier);
+	if (err)
+		goto out;
+
+	ppc_md.power_save = e500_cpuidle;
+
+	pr_info("e500_idle_driver registered.\n");
+
+	return 0;
+
+out:
+	e500_idle_devices_uninit();
+	cpuidle_unregister_driver(drv);
+
+	pr_err("Register e500 family cpuidle driver failed.\n");
+
+	return err;
+}
+device_initcall(e500_idle_init);
-- 
1.8.0

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano]

On 07/30/2013 09:00 AM, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
> 
> Add cpuidle support for e500 family, using cpuidle framework to
> manage various low power modes. The new implementation will remain
> compatible with original idle method.
> 
> Initially, this supports PW10, and subsequent patches will support
> PW20/DOZE/NAP.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
> This patch keep using cpuidle_register_device(), because we need to support cpu
> hotplug. I will fix "device" issue in this driver, after
> Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler into cpuidle
> freamwork.
> 
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index 8b48090..cbdbe25 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -271,6 +271,16 @@ extern void power7_idle(void);
>  extern void ppc6xx_idle(void);
>  extern void book3e_idle(void);
>  
> +/* Wait for Interrupt */
> +static inline void fsl_cpuidle_wait(void)
> +{
> +#ifdef CONFIG_PPC64
> +	book3e_idle();
> +#else
> +	e500_idle();
> +#endif
> +}
> +
>  /*
>   * ppc_md contains a copy of the machine description structure for the
>   * current platform. machine_id contains the initial address where the
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index b3fb81d..7ed114b 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -35,6 +35,11 @@ depends on ARM
>  source "drivers/cpuidle/Kconfig.arm"
>  endmenu
>  
> +menu "PowerPC CPU Idle Drivers"
> +depends on PPC32 || PPC64
> +source "drivers/cpuidle/Kconfig.powerpc"
> +endmenu
> +
>  endif
>  
>  config ARCH_NEEDS_CPU_IDLE_COUPLED
> diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc
> new file mode 100644
> index 0000000..9f3f5ef
> --- /dev/null
> +++ b/drivers/cpuidle/Kconfig.powerpc
> @@ -0,0 +1,9 @@
> +#
> +# PowerPC CPU Idle drivers
> +#
> +
> +config PPC_E500_CPUIDLE
> +	bool "CPU Idle Driver for E500 family processors"
> +	depends on FSL_SOC_BOOKE
> +	help
> +	  Select this to enable cpuidle on e500 family processors.
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 0b9d200..0dde3db 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -11,3 +11,7 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
>  obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>  obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>  obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
> +
> +##################################################################################
> +# PowerPC platform drivers
> +obj-$(CONFIG_PPC_E500_CPUIDLE)		+= cpuidle-e500.o
> diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c
> new file mode 100644
> index 0000000..1919cea
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-e500.c
> @@ -0,0 +1,222 @@
> +/*
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * CPU Idle driver for Freescale PowerPC e500 family processors.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/cpuidle.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +
> +#include <asm/machdep.h>
> +
> +static struct cpuidle_driver e500_idle_driver = {
> +	.name = "e500_idle",
> +	.owner = THIS_MODULE,
> +};
> +
> +static struct cpuidle_device __percpu *e500_cpuidle_devices;
> +
> +static void e500_cpuidle(void)
> +{
> +	/*
> +	 * This would call on the cpuidle framework, and the back-end
> +	 * driver to go to idle states.
> +	 */
> +	if (cpuidle_idle_call()) {
> +		/*
> +		 * On error, execute default handler
> +		 * to go into low thread priority and possibly
> +		 * low power mode.
> +		 */
> +		HMT_low();
> +		HMT_very_low();
> +	}
> +}

Nope, this is not the place to add such function.

> +static int pw10_enter(struct cpuidle_device *dev,
> +			struct cpuidle_driver *drv, int index)
> +{
> +	fsl_cpuidle_wait();
> +	return index;
> +}
> +
> +static struct cpuidle_state fsl_pw_idle_states[] = {
> +	{
> +		.name = "pw10",
> +		.desc = "pw10",
> +		.flags = CPUIDLE_FLAG_TIME_VALID,
> +		.exit_latency = 0,
> +		.target_residency = 0,
> +		.enter = &pw10_enter
> +	},
> +};
> +
> +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(e500_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,
> +};

This should go to the cpuidle framework.

> +
> +/*
> + * e500_idle_devices_init(void)
> + * allocate, initialize and register cpuidle device
> + */
> +static int e500_idle_devices_init(void)
> +{
> +	int i;
> +	struct cpuidle_driver *drv = &e500_idle_driver;
> +	struct cpuidle_device *dev;
> +
> +	e500_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (!e500_cpuidle_devices)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(i) {
> +		dev = per_cpu_ptr(e500_cpuidle_devices, i);
> +		dev->state_count = drv->state_count;
> +		dev->cpu = i;
> +
> +		if (cpuidle_register_device(dev)) {
> +			pr_err("cpuidle_register_device %d failed!\n", i);
> +			return -EIO;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * e500_idle_devices_uninit(void)
> + * unregister cpuidle devices and de-allocate memory
> + */
> +static void e500_idle_devices_uninit(void)
> +{
> +	int i;
> +	struct cpuidle_device *dev;
> +
> +	if (!e500_cpuidle_devices)
> +		return;
> +
> +	for_each_possible_cpu(i) {
> +		dev = per_cpu_ptr(e500_cpuidle_devices, i);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(e500_cpuidle_devices);
> +}
> +
> +static void e500_cpuidle_driver_init(unsigned int max_idle_state,
> +		struct cpuidle_state *cpuidle_state_table)
> +{
> +	int idle_state;
> +	struct cpuidle_driver *drv = &e500_idle_driver;
> +
> +	drv->state_count = 0;
> +
> +	for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
> +		if (!cpuidle_state_table[idle_state].enter)
> +			break;
> +
> +		drv->states[drv->state_count] = cpuidle_state_table[idle_state];
> +		drv->state_count++;
> +	}
> +}
> +
> +static int cpu_is_feature(unsigned long feature)
> +{
> +	return (cur_cpu_spec->cpu_features == feature);
> +}
> +
> +static int __init e500_idle_init(void)
> +{
> +	struct cpuidle_state *cpuidle_state_table = NULL;
> +	struct cpuidle_driver *drv = &e500_idle_driver;
> +	int err;
> +	unsigned int max_idle_state = 0;
> +
> +	if (cpuidle_disable != IDLE_NO_OVERRIDE)
> +		return -ENODEV;
> +
> +	if (cpu_is_feature(CPU_FTRS_E500MC) || cpu_is_feature(CPU_FTRS_E5500) ||
> +			cpu_is_feature(CPU_FTRS_E6500)) {
> +		cpuidle_state_table = fsl_pw_idle_states;
> +		max_idle_state = ARRAY_SIZE(fsl_pw_idle_states);
> +	}
> +
> +	if (!cpuidle_state_table || !max_idle_state)
> +		return -EPERM;

Please use different drivers for each and then register the right one
instead of state count convolutions.

Then use cpuidle_register(&mydriver, NULL) and get rid of the duplicate
initialization routine.


> +	e500_cpuidle_driver_init(max_idle_state, cpuidle_state_table);
> +
> +	if (!drv->state_count)
> +		return -EPERM;
> +
> +	err = cpuidle_register_driver(drv);
> +	if (err) {
> +		pr_err("Register e500 family cpuidle driver failed.\n");
> +
> +		return err;
> +	}
> +
> +	err = e500_idle_devices_init();
> +	if (err)
> +		goto out;
> +
> +	err = register_cpu_notifier(&cpu_hotplug_notifier);
> +	if (err)
> +		goto out;
> +
> +	ppc_md.power_save = e500_cpuidle;

This is not the place.

> +
> +	pr_info("e500_idle_driver registered.\n");
> +
> +	return 0;
> +
> +out:
> +	e500_idle_devices_uninit();
> +	cpuidle_unregister_driver(drv);
> +
> +	pr_err("Register e500 family cpuidle driver failed.\n");
> +
> +	return err;
> +}
> +device_initcall(e500_idle_init);
> 


-- 
 <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

RE: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Wang Dongsheng-B40534]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogRGFuaWVsIExlemNhbm8g
W21haWx0bzpkYW5pZWwubGV6Y2Fub0BsaW5hcm8ub3JnXQ0KPiBTZW50OiBUdWVzZGF5LCBKdWx5
IDMwLCAyMDEzIDU6NTEgUE0NCj4gVG86IFdhbmcgRG9uZ3NoZW5nLUI0MDUzNA0KPiBDYzogV29v
ZCBTY290dC1CMDc0MjE7IHJqd0BzaXNrLnBsOyBiZW5oQGtlcm5lbC5jcmFzaGluZy5vcmc7IExp
IFlhbmctDQo+IFI1ODQ3MjsgWmhhbyBDaGVuaHVpLUIzNTMzNjsgbGludXgtcG1Admdlci5rZXJu
ZWwub3JnOyBsaW51eHBwYy0NCj4gZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gU3ViamVjdDogUmU6
IFtQQVRDSF0gY3B1aWRsZTogYWRkIGZyZWVzY2FsZSBlNTAwIGZhbWlseSBwb3JjZXNzb3JzIGlk
bGUNCj4gc3VwcG9ydA0KPiANCj4gT24gMDcvMzAvMjAxMyAwOTowMCBBTSwgRG9uZ3NoZW5nIFdh
bmcgd3JvdGU6DQo+ID4gRnJvbTogV2FuZyBEb25nc2hlbmcgPGRvbmdzaGVuZy53YW5nQGZyZWVz
Y2FsZS5jb20+DQo+ID4NCj4gPiBBZGQgY3B1aWRsZSBzdXBwb3J0IGZvciBlNTAwIGZhbWlseSwg
dXNpbmcgY3B1aWRsZSBmcmFtZXdvcmsgdG8gbWFuYWdlDQo+ID4gdmFyaW91cyBsb3cgcG93ZXIg
bW9kZXMuIFRoZSBuZXcgaW1wbGVtZW50YXRpb24gd2lsbCByZW1haW4gY29tcGF0aWJsZQ0KPiA+
IHdpdGggb3JpZ2luYWwgaWRsZSBtZXRob2QuDQo+ID4NCj4gPiBJbml0aWFsbHksIHRoaXMgc3Vw
cG9ydHMgUFcxMCwgYW5kIHN1YnNlcXVlbnQgcGF0Y2hlcyB3aWxsIHN1cHBvcnQNCj4gPiBQVzIw
L0RPWkUvTkFQLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogV2FuZyBEb25nc2hlbmcgPGRvbmdz
aGVuZy53YW5nQGZyZWVzY2FsZS5jb20+DQo+ID4gLS0tDQo+ID4gVGhpcyBwYXRjaCBrZWVwIHVz
aW5nIGNwdWlkbGVfcmVnaXN0ZXJfZGV2aWNlKCksIGJlY2F1c2Ugd2UgbmVlZCB0bw0KPiA+IHN1
cHBvcnQgY3B1IGhvdHBsdWcuIEkgd2lsbCBmaXggImRldmljZSIgaXNzdWUgaW4gdGhpcyBkcml2
ZXIsIGFmdGVyDQo+ID4gRGVlcHRoaSBEaGFyd2FyIDxkZWVwdGhpQGxpbnV4LnZuZXQuaWJtLmNv
bT4gYWRkIGEgaG90cGx1ZyBoYW5kbGVyDQo+ID4gaW50byBjcHVpZGxlIGZyZWFtd29yay4NCj4g
Pg0KPiA+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vbWFjaGRlcC5oDQo+
ID4gYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vbWFjaGRlcC5oDQo+ID4gaW5kZXggOGI0ODA5
MC4uY2JkYmUyNSAxMDA2NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vbWFj
aGRlcC5oDQo+ID4gKysrIGIvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL21hY2hkZXAuaA0KPiA+
IEBAIC0yNzEsNiArMjcxLDE2IEBAIGV4dGVybiB2b2lkIHBvd2VyN19pZGxlKHZvaWQpOyAgZXh0
ZXJuIHZvaWQNCj4gPiBwcGM2eHhfaWRsZSh2b2lkKTsgIGV4dGVybiB2b2lkIGJvb2szZV9pZGxl
KHZvaWQpOw0KPiA+DQo+ID4gKy8qIFdhaXQgZm9yIEludGVycnVwdCAqLw0KPiA+ICtzdGF0aWMg
aW5saW5lIHZvaWQgZnNsX2NwdWlkbGVfd2FpdCh2b2lkKSB7ICNpZmRlZiBDT05GSUdfUFBDNjQN
Cj4gPiArCWJvb2szZV9pZGxlKCk7DQo+ID4gKyNlbHNlDQo+ID4gKwllNTAwX2lkbGUoKTsNCj4g
PiArI2VuZGlmDQo+ID4gK30NCj4gPiArDQo+ID4gIC8qDQo+ID4gICAqIHBwY19tZCBjb250YWlu
cyBhIGNvcHkgb2YgdGhlIG1hY2hpbmUgZGVzY3JpcHRpb24gc3RydWN0dXJlIGZvciB0aGUNCj4g
PiAgICogY3VycmVudCBwbGF0Zm9ybS4gbWFjaGluZV9pZCBjb250YWlucyB0aGUgaW5pdGlhbCBh
ZGRyZXNzIHdoZXJlIHRoZQ0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2NwdWlkbGUvS2NvbmZp
ZyBiL2RyaXZlcnMvY3B1aWRsZS9LY29uZmlnDQo+ID4gaW5kZXggYjNmYjgxZC4uN2VkMTE0YiAx
MDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2NwdWlkbGUvS2NvbmZpZw0KPiA+ICsrKyBiL2RyaXZl
cnMvY3B1aWRsZS9LY29uZmlnDQo+ID4gQEAgLTM1LDYgKzM1LDExIEBAIGRlcGVuZHMgb24gQVJN
DQo+ID4gIHNvdXJjZSAiZHJpdmVycy9jcHVpZGxlL0tjb25maWcuYXJtIg0KPiA+ICBlbmRtZW51
DQo+ID4NCj4gPiArbWVudSAiUG93ZXJQQyBDUFUgSWRsZSBEcml2ZXJzIg0KPiA+ICtkZXBlbmRz
IG9uIFBQQzMyIHx8IFBQQzY0DQo+ID4gK3NvdXJjZSAiZHJpdmVycy9jcHVpZGxlL0tjb25maWcu
cG93ZXJwYyINCj4gPiArZW5kbWVudQ0KPiA+ICsNCj4gPiAgZW5kaWYNCj4gPg0KPiA+ICBjb25m
aWcgQVJDSF9ORUVEU19DUFVfSURMRV9DT1VQTEVEDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMv
Y3B1aWRsZS9LY29uZmlnLnBvd2VycGMNCj4gYi9kcml2ZXJzL2NwdWlkbGUvS2NvbmZpZy5wb3dl
cnBjDQo+ID4gbmV3IGZpbGUgbW9kZSAxMDA2NDQNCj4gPiBpbmRleCAwMDAwMDAwLi45ZjNmNWVm
DQo+ID4gLS0tIC9kZXYvbnVsbA0KPiA+ICsrKyBiL2RyaXZlcnMvY3B1aWRsZS9LY29uZmlnLnBv
d2VycGMNCj4gPiBAQCAtMCwwICsxLDkgQEANCj4gPiArIw0KPiA+ICsjIFBvd2VyUEMgQ1BVIElk
bGUgZHJpdmVycw0KPiA+ICsjDQo+ID4gKw0KPiA+ICtjb25maWcgUFBDX0U1MDBfQ1BVSURMRQ0K
PiA+ICsJYm9vbCAiQ1BVIElkbGUgRHJpdmVyIGZvciBFNTAwIGZhbWlseSBwcm9jZXNzb3JzIg0K
PiA+ICsJZGVwZW5kcyBvbiBGU0xfU09DX0JPT0tFDQo+ID4gKwloZWxwDQo+ID4gKwkgIFNlbGVj
dCB0aGlzIHRvIGVuYWJsZSBjcHVpZGxlIG9uIGU1MDAgZmFtaWx5IHByb2Nlc3NvcnMuDQo+ID4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvY3B1aWRsZS9NYWtlZmlsZSBiL2RyaXZlcnMvY3B1aWRsZS9N
YWtlZmlsZQ0KPiA+IGluZGV4IDBiOWQyMDAuLjBkZGUzZGIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJp
dmVycy9jcHVpZGxlL01ha2VmaWxlDQo+ID4gKysrIGIvZHJpdmVycy9jcHVpZGxlL01ha2VmaWxl
DQo+ID4gQEAgLTExLDMgKzExLDcgQEAgb2JqLSQoQ09ORklHX0FSTV9ISUdIQkFOS19DUFVJRExF
KQkrPSBjcHVpZGxlLQ0KPiBjYWx4ZWRhLm8NCj4gPiAgb2JqLSQoQ09ORklHX0FSTV9LSVJLV09P
RF9DUFVJRExFKQkrPSBjcHVpZGxlLWtpcmt3b29kLm8NCj4gPiAgb2JqLSQoQ09ORklHX0FSTV9a
WU5RX0NQVUlETEUpCQkrPSBjcHVpZGxlLXp5bnEubw0KPiA+ICBvYmotJChDT05GSUdfQVJNX1U4
NTAwX0NQVUlETEUpICAgICAgICAgKz0gY3B1aWRsZS11eDUwMC5vDQo+ID4gKw0KPiA+DQo+ICsj
IyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMjIyMj
IyMjIyMjIyMjIyMjIyMNCj4gIyMjIyMjIyMjIw0KPiA+ICsjIFBvd2VyUEMgcGxhdGZvcm0gZHJp
dmVycw0KPiA+ICtvYmotJChDT05GSUdfUFBDX0U1MDBfQ1BVSURMRSkJCSs9IGNwdWlkbGUtZTUw
MC5vDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvY3B1aWRsZS9jcHVpZGxlLWU1MDAuYyBiL2Ry
aXZlcnMvY3B1aWRsZS9jcHVpZGxlLQ0KPiBlNTAwLmMNCj4gPiBuZXcgZmlsZSBtb2RlIDEwMDY0
NA0KPiA+IGluZGV4IDAwMDAwMDAuLjE5MTljZWENCj4gPiAtLS0gL2Rldi9udWxsDQo+ID4gKysr
IGIvZHJpdmVycy9jcHVpZGxlL2NwdWlkbGUtZTUwMC5jDQo+ID4gQEAgLTAsMCArMSwyMjIgQEAN
Cj4gPiArLyoNCj4gPiArICogQ29weXJpZ2h0IDIwMTMgRnJlZXNjYWxlIFNlbWljb25kdWN0b3Is
IEluYy4NCj4gPiArICoNCj4gPiArICogQ1BVIElkbGUgZHJpdmVyIGZvciBGcmVlc2NhbGUgUG93
ZXJQQyBlNTAwIGZhbWlseSBwcm9jZXNzb3JzLg0KPiA+ICsgKg0KPiA+ICsgKiBUaGlzIHByb2dy
YW0gaXMgZnJlZSBzb2Z0d2FyZTsgeW91IGNhbiByZWRpc3RyaWJ1dGUgaXQgYW5kL29yDQo+IG1v
ZGlmeQ0KPiA+ICsgKiBpdCB1bmRlciB0aGUgdGVybXMgb2YgdGhlIEdOVSBHZW5lcmFsIFB1Ymxp
YyBMaWNlbnNlIHZlcnNpb24gMiBhcw0KPiA+ICsgKiBwdWJsaXNoZWQgYnkgdGhlIEZyZWUgU29m
dHdhcmUgRm91bmRhdGlvbi4NCj4gPiArICoNCj4gPiArICogQXV0aG9yOiBXYW5nIERvbmdzaGVu
ZyA8ZG9uZ3NoZW5nLndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPiArICovDQo+ID4gKw0KPiA+ICsj
aW5jbHVkZSA8bGludXgvY3B1Lmg+DQo+ID4gKyNpbmNsdWRlIDxsaW51eC9jcHVpZGxlLmg+DQo+
ID4gKyNpbmNsdWRlIDxsaW51eC9pbml0Lmg+DQo+ID4gKyNpbmNsdWRlIDxsaW51eC9rZXJuZWwu
aD4NCj4gPiArI2luY2x1ZGUgPGxpbnV4L21vZHVsZS5oPg0KPiA+ICsjaW5jbHVkZSA8bGludXgv
bm90aWZpZXIuaD4NCj4gPiArDQo+ID4gKyNpbmNsdWRlIDxhc20vbWFjaGRlcC5oPg0KPiA+ICsN
Cj4gPiArc3RhdGljIHN0cnVjdCBjcHVpZGxlX2RyaXZlciBlNTAwX2lkbGVfZHJpdmVyID0gew0K
PiA+ICsJLm5hbWUgPSAiZTUwMF9pZGxlIiwNCj4gPiArCS5vd25lciA9IFRISVNfTU9EVUxFLA0K
PiA+ICt9Ow0KPiA+ICsNCj4gPiArc3RhdGljIHN0cnVjdCBjcHVpZGxlX2RldmljZSBfX3BlcmNw
dSAqZTUwMF9jcHVpZGxlX2RldmljZXM7DQo+ID4gKw0KPiA+ICtzdGF0aWMgdm9pZCBlNTAwX2Nw
dWlkbGUodm9pZCkNCj4gPiArew0KPiA+ICsJLyoNCj4gPiArCSAqIFRoaXMgd291bGQgY2FsbCBv
biB0aGUgY3B1aWRsZSBmcmFtZXdvcmssIGFuZCB0aGUgYmFjay1lbmQNCj4gPiArCSAqIGRyaXZl
ciB0byBnbyB0byBpZGxlIHN0YXRlcy4NCj4gPiArCSAqLw0KPiA+ICsJaWYgKGNwdWlkbGVfaWRs
ZV9jYWxsKCkpIHsNCj4gPiArCQkvKg0KPiA+ICsJCSAqIE9uIGVycm9yLCBleGVjdXRlIGRlZmF1
bHQgaGFuZGxlcg0KPiA+ICsJCSAqIHRvIGdvIGludG8gbG93IHRocmVhZCBwcmlvcml0eSBhbmQg
cG9zc2libHkNCj4gPiArCQkgKiBsb3cgcG93ZXIgbW9kZS4NCj4gPiArCQkgKi8NCj4gPiArCQlI
TVRfbG93KCk7DQo+ID4gKwkJSE1UX3ZlcnlfbG93KCk7DQo+ID4gKwl9DQo+ID4gK30NCj4gDQo+
IE5vcGUsIHRoaXMgaXMgbm90IHRoZSBwbGFjZSB0byBhZGQgc3VjaCBmdW5jdGlvbi4NCj4gDQpU
aGFua3MuDQoNCkJ1dCBXaHkgbm90PyBPbiBwb3dlcnBjIHRoZXJlIGlzIGFscmVhZHkgaGF2ZSBh
IGNwdWlkbGUgbWV0aG9kLiBXZSBuZWVkIHRvIGNvbXBhdGlibGUgdGhlbS4NCg0KVW0uLiBhcmNo
L3Bvd2VycGMvcGxhdGZvcm1zL3h4eCA/DQoNCj4gPiArc3RhdGljIGludCBwdzEwX2VudGVyKHN0
cnVjdCBjcHVpZGxlX2RldmljZSAqZGV2LA0KPiA+ICsJCQlzdHJ1Y3QgY3B1aWRsZV9kcml2ZXIg
KmRydiwgaW50IGluZGV4KQ0KPiA+ICt7DQo+ID4gKwlmc2xfY3B1aWRsZV93YWl0KCk7DQo+ID4g
KwlyZXR1cm4gaW5kZXg7DQo+ID4gK30NCj4gPiArDQo+ID4gK3N0YXRpYyBzdHJ1Y3QgY3B1aWRs
ZV9zdGF0ZSBmc2xfcHdfaWRsZV9zdGF0ZXNbXSA9IHsNCj4gPiArCXsNCj4gPiArCQkubmFtZSA9
ICJwdzEwIiwNCj4gPiArCQkuZGVzYyA9ICJwdzEwIiwNCj4gPiArCQkuZmxhZ3MgPSBDUFVJRExF
X0ZMQUdfVElNRV9WQUxJRCwNCj4gPiArCQkuZXhpdF9sYXRlbmN5ID0gMCwNCj4gPiArCQkudGFy
Z2V0X3Jlc2lkZW5jeSA9IDAsDQo+ID4gKwkJLmVudGVyID0gJnB3MTBfZW50ZXINCj4gPiArCX0s
DQo+ID4gK307DQo+ID4gKw0KPiA+ICtzdGF0aWMgaW50IGNwdV9ob3RwbHVnX25vdGlmeShzdHJ1
Y3Qgbm90aWZpZXJfYmxvY2sgKm4sDQo+ID4gKwkJCXVuc2lnbmVkIGxvbmcgYWN0aW9uLCB2b2lk
ICpoY3B1KQ0KPiA+ICt7DQo+ID4gKwl1bnNpZ25lZCBsb25nIGhvdGNwdSA9ICh1bnNpZ25lZCBs
b25nKWhjcHU7DQo+ID4gKwlzdHJ1Y3QgY3B1aWRsZV9kZXZpY2UgKmRldiA9DQo+ID4gKwkJCXBl
cl9jcHVfcHRyKGU1MDBfY3B1aWRsZV9kZXZpY2VzLCBob3RjcHUpOw0KPiA+ICsNCj4gPiArCWlm
IChkZXYgJiYgY3B1aWRsZV9nZXRfZHJpdmVyKCkpIHsNCj4gPiArCQlzd2l0Y2ggKGFjdGlvbikg
ew0KPiA+ICsJCWNhc2UgQ1BVX09OTElORToNCj4gPiArCQljYXNlIENQVV9PTkxJTkVfRlJPWkVO
Og0KPiA+ICsJCQljcHVpZGxlX3BhdXNlX2FuZF9sb2NrKCk7DQo+ID4gKwkJCWNwdWlkbGVfZW5h
YmxlX2RldmljZShkZXYpOw0KPiA+ICsJCQljcHVpZGxlX3Jlc3VtZV9hbmRfdW5sb2NrKCk7DQo+
ID4gKwkJCWJyZWFrOw0KPiA+ICsNCj4gPiArCQljYXNlIENQVV9ERUFEOg0KPiA+ICsJCWNhc2Ug
Q1BVX0RFQURfRlJPWkVOOg0KPiA+ICsJCQljcHVpZGxlX3BhdXNlX2FuZF9sb2NrKCk7DQo+ID4g
KwkJCWNwdWlkbGVfZGlzYWJsZV9kZXZpY2UoZGV2KTsNCj4gPiArCQkJY3B1aWRsZV9yZXN1bWVf
YW5kX3VubG9jaygpOw0KPiA+ICsJCQlicmVhazsNCj4gPiArDQo+ID4gKwkJZGVmYXVsdDoNCj4g
PiArCQkJcmV0dXJuIE5PVElGWV9ET05FOw0KPiA+ICsJCX0NCj4gPiArCX0NCj4gPiArDQo+ID4g
KwlyZXR1cm4gTk9USUZZX09LOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICtzdGF0aWMgc3RydWN0IG5v
dGlmaWVyX2Jsb2NrIGNwdV9ob3RwbHVnX25vdGlmaWVyID0gew0KPiA+ICsJLm5vdGlmaWVyX2Nh
bGwgPSBjcHVfaG90cGx1Z19ub3RpZnksDQo+ID4gK307DQo+IA0KPiBUaGlzIHNob3VsZCBnbyB0
byB0aGUgY3B1aWRsZSBmcmFtZXdvcmsuDQo+IA0KWWVzLCBJIHNlZSwgRGVlcHRoaSB3aWxsIHBv
c3QgaXQuLi4NCjooLCBUaGlzIHBhdGNoIGlzIHJlbHlpbmcgb24gZm9yIERlZXB0aGkncyBwYXRj
aCBhYm91dCAiaG90cGx1ZyBub3RpZmllciIuDQoNCj4gPiArDQo+ID4gKy8qDQo+ID4gKyAqIGU1
MDBfaWRsZV9kZXZpY2VzX2luaXQodm9pZCkNCj4gPiArICogYWxsb2NhdGUsIGluaXRpYWxpemUg
YW5kIHJlZ2lzdGVyIGNwdWlkbGUgZGV2aWNlDQo+ID4gKyAqLw0KPiA+ICtzdGF0aWMgaW50IGU1
MDBfaWRsZV9kZXZpY2VzX2luaXQodm9pZCkNCj4gPiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlz
dHJ1Y3QgY3B1aWRsZV9kcml2ZXIgKmRydiA9ICZlNTAwX2lkbGVfZHJpdmVyOw0KPiA+ICsJc3Ry
dWN0IGNwdWlkbGVfZGV2aWNlICpkZXY7DQo+ID4gKw0KPiA+ICsJZTUwMF9jcHVpZGxlX2Rldmlj
ZXMgPSBhbGxvY19wZXJjcHUoc3RydWN0IGNwdWlkbGVfZGV2aWNlKTsNCj4gPiArCWlmICghZTUw
MF9jcHVpZGxlX2RldmljZXMpDQo+ID4gKwkJcmV0dXJuIC1FTk9NRU07DQo+ID4gKw0KPiA+ICsJ
Zm9yX2VhY2hfcG9zc2libGVfY3B1KGkpIHsNCj4gPiArCQlkZXYgPSBwZXJfY3B1X3B0cihlNTAw
X2NwdWlkbGVfZGV2aWNlcywgaSk7DQo+ID4gKwkJZGV2LT5zdGF0ZV9jb3VudCA9IGRydi0+c3Rh
dGVfY291bnQ7DQo+ID4gKwkJZGV2LT5jcHUgPSBpOw0KPiA+ICsNCj4gPiArCQlpZiAoY3B1aWRs
ZV9yZWdpc3Rlcl9kZXZpY2UoZGV2KSkgew0KPiA+ICsJCQlwcl9lcnIoImNwdWlkbGVfcmVnaXN0
ZXJfZGV2aWNlICVkIGZhaWxlZCFcbiIsIGkpOw0KPiA+ICsJCQlyZXR1cm4gLUVJTzsNCj4gPiAr
CQl9DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICsJcmV0dXJuIDA7DQo+ID4gK30NCj4gPiArDQo+ID4g
Ky8qDQo+ID4gKyAqIGU1MDBfaWRsZV9kZXZpY2VzX3VuaW5pdCh2b2lkKQ0KPiA+ICsgKiB1bnJl
Z2lzdGVyIGNwdWlkbGUgZGV2aWNlcyBhbmQgZGUtYWxsb2NhdGUgbWVtb3J5DQo+ID4gKyAqLw0K
PiA+ICtzdGF0aWMgdm9pZCBlNTAwX2lkbGVfZGV2aWNlc191bmluaXQodm9pZCkNCj4gPiArew0K
PiA+ICsJaW50IGk7DQo+ID4gKwlzdHJ1Y3QgY3B1aWRsZV9kZXZpY2UgKmRldjsNCj4gPiArDQo+
ID4gKwlpZiAoIWU1MDBfY3B1aWRsZV9kZXZpY2VzKQ0KPiA+ICsJCXJldHVybjsNCj4gPiArDQo+
ID4gKwlmb3JfZWFjaF9wb3NzaWJsZV9jcHUoaSkgew0KPiA+ICsJCWRldiA9IHBlcl9jcHVfcHRy
KGU1MDBfY3B1aWRsZV9kZXZpY2VzLCBpKTsNCj4gPiArCQljcHVpZGxlX3VucmVnaXN0ZXJfZGV2
aWNlKGRldik7DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICsJZnJlZV9wZXJjcHUoZTUwMF9jcHVpZGxl
X2RldmljZXMpOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICtzdGF0aWMgdm9pZCBlNTAwX2NwdWlkbGVf
ZHJpdmVyX2luaXQodW5zaWduZWQgaW50IG1heF9pZGxlX3N0YXRlLA0KPiA+ICsJCXN0cnVjdCBj
cHVpZGxlX3N0YXRlICpjcHVpZGxlX3N0YXRlX3RhYmxlKQ0KPiA+ICt7DQo+ID4gKwlpbnQgaWRs
ZV9zdGF0ZTsNCj4gPiArCXN0cnVjdCBjcHVpZGxlX2RyaXZlciAqZHJ2ID0gJmU1MDBfaWRsZV9k
cml2ZXI7DQo+ID4gKw0KPiA+ICsJZHJ2LT5zdGF0ZV9jb3VudCA9IDA7DQo+ID4gKw0KPiA+ICsJ
Zm9yIChpZGxlX3N0YXRlID0gMDsgaWRsZV9zdGF0ZSA8IG1heF9pZGxlX3N0YXRlOyArK2lkbGVf
c3RhdGUpIHsNCj4gPiArCQlpZiAoIWNwdWlkbGVfc3RhdGVfdGFibGVbaWRsZV9zdGF0ZV0uZW50
ZXIpDQo+ID4gKwkJCWJyZWFrOw0KPiA+ICsNCj4gPiArCQlkcnYtPnN0YXRlc1tkcnYtPnN0YXRl
X2NvdW50XSA9DQo+IGNwdWlkbGVfc3RhdGVfdGFibGVbaWRsZV9zdGF0ZV07DQo+ID4gKwkJZHJ2
LT5zdGF0ZV9jb3VudCsrOw0KPiA+ICsJfQ0KPiA+ICt9DQo+ID4gKw0KPiA+ICtzdGF0aWMgaW50
IGNwdV9pc19mZWF0dXJlKHVuc2lnbmVkIGxvbmcgZmVhdHVyZSkNCj4gPiArew0KPiA+ICsJcmV0
dXJuIChjdXJfY3B1X3NwZWMtPmNwdV9mZWF0dXJlcyA9PSBmZWF0dXJlKTsNCj4gPiArfQ0KPiA+
ICsNCj4gPiArc3RhdGljIGludCBfX2luaXQgZTUwMF9pZGxlX2luaXQodm9pZCkNCj4gPiArew0K
PiA+ICsJc3RydWN0IGNwdWlkbGVfc3RhdGUgKmNwdWlkbGVfc3RhdGVfdGFibGUgPSBOVUxMOw0K
PiA+ICsJc3RydWN0IGNwdWlkbGVfZHJpdmVyICpkcnYgPSAmZTUwMF9pZGxlX2RyaXZlcjsNCj4g
PiArCWludCBlcnI7DQo+ID4gKwl1bnNpZ25lZCBpbnQgbWF4X2lkbGVfc3RhdGUgPSAwOw0KPiA+
ICsNCj4gPiArCWlmIChjcHVpZGxlX2Rpc2FibGUgIT0gSURMRV9OT19PVkVSUklERSkNCj4gPiAr
CQlyZXR1cm4gLUVOT0RFVjsNCj4gPiArDQo+ID4gKwlpZiAoY3B1X2lzX2ZlYXR1cmUoQ1BVX0ZU
UlNfRTUwME1DKSB8fA0KPiBjcHVfaXNfZmVhdHVyZShDUFVfRlRSU19FNTUwMCkgfHwNCj4gPiAr
CQkJY3B1X2lzX2ZlYXR1cmUoQ1BVX0ZUUlNfRTY1MDApKSB7DQo+ID4gKwkJY3B1aWRsZV9zdGF0
ZV90YWJsZSA9IGZzbF9wd19pZGxlX3N0YXRlczsNCj4gPiArCQltYXhfaWRsZV9zdGF0ZSA9IEFS
UkFZX1NJWkUoZnNsX3B3X2lkbGVfc3RhdGVzKTsNCj4gPiArCX0NCj4gPiArDQo+ID4gKwlpZiAo
IWNwdWlkbGVfc3RhdGVfdGFibGUgfHwgIW1heF9pZGxlX3N0YXRlKQ0KPiA+ICsJCXJldHVybiAt
RVBFUk07DQo+IA0KPiBQbGVhc2UgdXNlIGRpZmZlcmVudCBkcml2ZXJzIGZvciBlYWNoIGFuZCB0
aGVuIHJlZ2lzdGVyIHRoZSByaWdodCBvbmUNCj4gaW5zdGVhZCBvZiBzdGF0ZSBjb3VudCBjb252
b2x1dGlvbnMuDQo+IA0KPiBUaGVuIHVzZSBjcHVpZGxlX3JlZ2lzdGVyKCZteWRyaXZlciwgTlVM
TCkgYW5kIGdldCByaWQgb2YgdGhlIGR1cGxpY2F0ZQ0KPiBpbml0aWFsaXphdGlvbiByb3V0aW5l
Lg0KPiANClllcywgVGhhbmtzLCBUaGlzIHBhdGNoIGtlZXAgdXNpbmcgY3B1aWRsZV9yZWdpc3Rl
cl9kZXZpY2UoKSwgYmVjYXVzZSB3ZSBuZWVkIHRvDQpzdXBwb3J0IGNwdSBob3RwbHVnLiBJIHdp
bGwgZml4ICJkZXZpY2UiIGlzc3VlIGluIHRoaXMgZHJpdmVyLCBhZnRlcg0KRGVlcHRoaSBEaGFy
d2FyIDxkZWVwdGhpQGxpbnV4LnZuZXQuaWJtLmNvbT4gYWRkIGEgaG90cGx1ZyBoYW5kbGVyDQpp
bnRvIGNwdWlkbGUgZnJlYW13b3JrLg0KDQo+IA0KPiA+ICsJZTUwMF9jcHVpZGxlX2RyaXZlcl9p
bml0KG1heF9pZGxlX3N0YXRlLCBjcHVpZGxlX3N0YXRlX3RhYmxlKTsNCj4gPiArDQo+ID4gKwlp
ZiAoIWRydi0+c3RhdGVfY291bnQpDQo+ID4gKwkJcmV0dXJuIC1FUEVSTTsNCj4gPiArDQo+ID4g
KwllcnIgPSBjcHVpZGxlX3JlZ2lzdGVyX2RyaXZlcihkcnYpOw0KPiA+ICsJaWYgKGVycikgew0K
PiA+ICsJCXByX2VycigiUmVnaXN0ZXIgZTUwMCBmYW1pbHkgY3B1aWRsZSBkcml2ZXIgZmFpbGVk
LlxuIik7DQo+ID4gKw0KPiA+ICsJCXJldHVybiBlcnI7DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICsJ
ZXJyID0gZTUwMF9pZGxlX2RldmljZXNfaW5pdCgpOw0KPiA+ICsJaWYgKGVycikNCj4gPiArCQln
b3RvIG91dDsNCj4gPiArDQo+ID4gKwllcnIgPSByZWdpc3Rlcl9jcHVfbm90aWZpZXIoJmNwdV9o
b3RwbHVnX25vdGlmaWVyKTsNCj4gPiArCWlmIChlcnIpDQo+ID4gKwkJZ290byBvdXQ7DQo+ID4g
Kw0KPiA+ICsJcHBjX21kLnBvd2VyX3NhdmUgPSBlNTAwX2NwdWlkbGU7DQo+IA0KPiBUaGlzIGlz
IG5vdCB0aGUgcGxhY2UuDQo+IA0KPiA+ICsNCj4gPiArCXByX2luZm8oImU1MDBfaWRsZV9kcml2
ZXIgcmVnaXN0ZXJlZC5cbiIpOw0KPiA+ICsNCj4gPiArCXJldHVybiAwOw0KPiA+ICsNCj4gPiAr
b3V0Og0KPiA+ICsJZTUwMF9pZGxlX2RldmljZXNfdW5pbml0KCk7DQo+ID4gKwljcHVpZGxlX3Vu
cmVnaXN0ZXJfZHJpdmVyKGRydik7DQo+ID4gKw0KPiA+ICsJcHJfZXJyKCJSZWdpc3RlciBlNTAw
IGZhbWlseSBjcHVpZGxlIGRyaXZlciBmYWlsZWQuXG4iKTsNCj4gPiArDQo+ID4gKwlyZXR1cm4g
ZXJyOw0KPiA+ICt9DQo+ID4gK2RldmljZV9pbml0Y2FsbChlNTAwX2lkbGVfaW5pdCk7DQo+ID4N
Cj4gDQo+IA0KPiAtLQ0KPiAgPGh0dHA6Ly93d3cubGluYXJvLm9yZy8+IExpbmFyby5vcmcg4pSC
IE9wZW4gc291cmNlIHNvZnR3YXJlIGZvciBBUk0gU29Dcw0KPiANCj4gRm9sbG93IExpbmFybzog
IDxodHRwOi8vd3d3LmZhY2Vib29rLmNvbS9wYWdlcy9MaW5hcm8+IEZhY2Vib29rIHwNCj4gPGh0
dHA6Ly90d2l0dGVyLmNvbS8jIS9saW5hcm9vcmc+IFR3aXR0ZXIgfA0KPiA8aHR0cDovL3d3dy5s
aW5hcm8ub3JnL2xpbmFyby1ibG9nLz4gQmxvZw0KPiANCg0K

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Deepthi Dharwar]

On 07/30/2013 04:32 PM, Wang Dongsheng-B40534 wrote:

Hi Dongsheng,

I have posted out a generic powerpc idle backend driver ccing you.
Please take a look at it and integrate your platform idle states into it.

Right now, cpu hotplug notifier is common for all powerpc. This
shouldn't hinder you.
This will reduce code duplication and would make it easier to
plug into the framework. All you need is platform specific
idle states table and config changes.

Regards,
Deepthi


> 
>> -----Original Message-----
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> Sent: Tuesday, July 30, 2013 5:51 PM
>> To: Wang Dongsheng-B40534
>> Cc: Wood Scott-B07421; rjw@sisk.pl; benh@kernel.crashing.org; Li Yang-
>> R58472; Zhao Chenhui-B35336; linux-pm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org
>> Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle
>> support
>>
>> On 07/30/2013 09:00 AM, Dongsheng Wang wrote:
>>> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>>>
>>> Add cpuidle support for e500 family, using cpuidle framework to manage
>>> various low power modes. The new implementation will remain compatible
>>> with original idle method.
>>>
>>> Initially, this supports PW10, and subsequent patches will support
>>> PW20/DOZE/NAP.
>>>
>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>>> ---
>>> This patch keep using cpuidle_register_device(), because we need to
>>> support cpu hotplug. I will fix "device" issue in this driver, after
>>> Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler
>>> into cpuidle freamwork.
>>>
>>> diff --git a/arch/powerpc/include/asm/machdep.h
>>> b/arch/powerpc/include/asm/machdep.h
>>> index 8b48090..cbdbe25 100644
>>> --- a/arch/powerpc/include/asm/machdep.h
>>> +++ b/arch/powerpc/include/asm/machdep.h
>>> @@ -271,6 +271,16 @@ extern void power7_idle(void);  extern void
>>> ppc6xx_idle(void);  extern void book3e_idle(void);
>>>
>>> +/* Wait for Interrupt */
>>> +static inline void fsl_cpuidle_wait(void) { #ifdef CONFIG_PPC64
>>> +	book3e_idle();
>>> +#else
>>> +	e500_idle();
>>> +#endif
>>> +}
>>> +
>>>  /*
>>>   * ppc_md contains a copy of the machine description structure for the
>>>   * current platform. machine_id contains the initial address where the
>>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>>> index b3fb81d..7ed114b 100644
>>> --- a/drivers/cpuidle/Kconfig
>>> +++ b/drivers/cpuidle/Kconfig
>>> @@ -35,6 +35,11 @@ depends on ARM
>>>  source "drivers/cpuidle/Kconfig.arm"
>>>  endmenu
>>>
>>> +menu "PowerPC CPU Idle Drivers"
>>> +depends on PPC32 || PPC64
>>> +source "drivers/cpuidle/Kconfig.powerpc"
>>> +endmenu
>>> +
>>>  endif
>>>
>>>  config ARCH_NEEDS_CPU_IDLE_COUPLED
>>> diff --git a/drivers/cpuidle/Kconfig.powerpc
>> b/drivers/cpuidle/Kconfig.powerpc
>>> new file mode 100644
>>> index 0000000..9f3f5ef
>>> --- /dev/null
>>> +++ b/drivers/cpuidle/Kconfig.powerpc
>>> @@ -0,0 +1,9 @@
>>> +#
>>> +# PowerPC CPU Idle drivers
>>> +#
>>> +
>>> +config PPC_E500_CPUIDLE
>>> +	bool "CPU Idle Driver for E500 family processors"
>>> +	depends on FSL_SOC_BOOKE
>>> +	help
>>> +	  Select this to enable cpuidle on e500 family processors.
>>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>>> index 0b9d200..0dde3db 100644
>>> --- a/drivers/cpuidle/Makefile
>>> +++ b/drivers/cpuidle/Makefile
>>> @@ -11,3 +11,7 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-
>> calxeda.o
>>>  obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
>>>  obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>>>  obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>>> +
>>>
>> +########################################################################
>> ##########
>>> +# PowerPC platform drivers
>>> +obj-$(CONFIG_PPC_E500_CPUIDLE)		+= cpuidle-e500.o
>>> diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-
>> e500.c
>>> new file mode 100644
>>> index 0000000..1919cea
>>> --- /dev/null
>>> +++ b/drivers/cpuidle/cpuidle-e500.c
>>> @@ -0,0 +1,222 @@
>>> +/*
>>> + * Copyright 2013 Freescale Semiconductor, Inc.
>>> + *
>>> + * CPU Idle driver for Freescale PowerPC e500 family processors.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>> modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
>>> + */
>>> +
>>> +#include <linux/cpu.h>
>>> +#include <linux/cpuidle.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/notifier.h>
>>> +
>>> +#include <asm/machdep.h>
>>> +
>>> +static struct cpuidle_driver e500_idle_driver = {
>>> +	.name = "e500_idle",
>>> +	.owner = THIS_MODULE,
>>> +};
>>> +
>>> +static struct cpuidle_device __percpu *e500_cpuidle_devices;
>>> +
>>> +static void e500_cpuidle(void)
>>> +{
>>> +	/*
>>> +	 * This would call on the cpuidle framework, and the back-end
>>> +	 * driver to go to idle states.
>>> +	 */
>>> +	if (cpuidle_idle_call()) {
>>> +		/*
>>> +		 * On error, execute default handler
>>> +		 * to go into low thread priority and possibly
>>> +		 * low power mode.
>>> +		 */
>>> +		HMT_low();
>>> +		HMT_very_low();
>>> +	}
>>> +}
>>
>> Nope, this is not the place to add such function.
>>
> Thanks.
> 
> But Why not? On powerpc there is already have a cpuidle method. We need to compatible them.
> 
> Um.. arch/powerpc/platforms/xxx ?
> 
>>> +static int pw10_enter(struct cpuidle_device *dev,
>>> +			struct cpuidle_driver *drv, int index)
>>> +{
>>> +	fsl_cpuidle_wait();
>>> +	return index;
>>> +}
>>> +
>>> +static struct cpuidle_state fsl_pw_idle_states[] = {
>>> +	{
>>> +		.name = "pw10",
>>> +		.desc = "pw10",
>>> +		.flags = CPUIDLE_FLAG_TIME_VALID,
>>> +		.exit_latency = 0,
>>> +		.target_residency = 0,
>>> +		.enter = &pw10_enter
>>> +	},
>>> +};
>>> +
>>> +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(e500_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,
>>> +};
>>
>> This should go to the cpuidle framework.
>>
> Yes, I see, Deepthi will post it...
> :(, This patch is relying on for Deepthi's patch about "hotplug notifier".
> 
>>> +
>>> +/*
>>> + * e500_idle_devices_init(void)
>>> + * allocate, initialize and register cpuidle device
>>> + */
>>> +static int e500_idle_devices_init(void)
>>> +{
>>> +	int i;
>>> +	struct cpuidle_driver *drv = &e500_idle_driver;
>>> +	struct cpuidle_device *dev;
>>> +
>>> +	e500_cpuidle_devices = alloc_percpu(struct cpuidle_device);
>>> +	if (!e500_cpuidle_devices)
>>> +		return -ENOMEM;
>>> +
>>> +	for_each_possible_cpu(i) {
>>> +		dev = per_cpu_ptr(e500_cpuidle_devices, i);
>>> +		dev->state_count = drv->state_count;
>>> +		dev->cpu = i;
>>> +
>>> +		if (cpuidle_register_device(dev)) {
>>> +			pr_err("cpuidle_register_device %d failed!\n", i);
>>> +			return -EIO;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * e500_idle_devices_uninit(void)
>>> + * unregister cpuidle devices and de-allocate memory
>>> + */
>>> +static void e500_idle_devices_uninit(void)
>>> +{
>>> +	int i;
>>> +	struct cpuidle_device *dev;
>>> +
>>> +	if (!e500_cpuidle_devices)
>>> +		return;
>>> +
>>> +	for_each_possible_cpu(i) {
>>> +		dev = per_cpu_ptr(e500_cpuidle_devices, i);
>>> +		cpuidle_unregister_device(dev);
>>> +	}
>>> +
>>> +	free_percpu(e500_cpuidle_devices);
>>> +}
>>> +
>>> +static void e500_cpuidle_driver_init(unsigned int max_idle_state,
>>> +		struct cpuidle_state *cpuidle_state_table)
>>> +{
>>> +	int idle_state;
>>> +	struct cpuidle_driver *drv = &e500_idle_driver;
>>> +
>>> +	drv->state_count = 0;
>>> +
>>> +	for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
>>> +		if (!cpuidle_state_table[idle_state].enter)
>>> +			break;
>>> +
>>> +		drv->states[drv->state_count] =
>> cpuidle_state_table[idle_state];
>>> +		drv->state_count++;
>>> +	}
>>> +}
>>> +
>>> +static int cpu_is_feature(unsigned long feature)
>>> +{
>>> +	return (cur_cpu_spec->cpu_features == feature);
>>> +}
>>> +
>>> +static int __init e500_idle_init(void)
>>> +{
>>> +	struct cpuidle_state *cpuidle_state_table = NULL;
>>> +	struct cpuidle_driver *drv = &e500_idle_driver;
>>> +	int err;
>>> +	unsigned int max_idle_state = 0;
>>> +
>>> +	if (cpuidle_disable != IDLE_NO_OVERRIDE)
>>> +		return -ENODEV;
>>> +
>>> +	if (cpu_is_feature(CPU_FTRS_E500MC) ||
>> cpu_is_feature(CPU_FTRS_E5500) ||
>>> +			cpu_is_feature(CPU_FTRS_E6500)) {
>>> +		cpuidle_state_table = fsl_pw_idle_states;
>>> +		max_idle_state = ARRAY_SIZE(fsl_pw_idle_states);
>>> +	}
>>> +
>>> +	if (!cpuidle_state_table || !max_idle_state)
>>> +		return -EPERM;
>>
>> Please use different drivers for each and then register the right one
>> instead of state count convolutions.
>>
>> Then use cpuidle_register(&mydriver, NULL) and get rid of the duplicate
>> initialization routine.
>>
> Yes, Thanks, This patch keep using cpuidle_register_device(), because we need to
> support cpu hotplug. I will fix "device" issue in this driver, after
> Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler
> into cpuidle freamwork.
> 
>>
>>> +	e500_cpuidle_driver_init(max_idle_state, cpuidle_state_table);
>>> +
>>> +	if (!drv->state_count)
>>> +		return -EPERM;
>>> +
>>> +	err = cpuidle_register_driver(drv);
>>> +	if (err) {
>>> +		pr_err("Register e500 family cpuidle driver failed.\n");
>>> +
>>> +		return err;
>>> +	}
>>> +
>>> +	err = e500_idle_devices_init();
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	err = register_cpu_notifier(&cpu_hotplug_notifier);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	ppc_md.power_save = e500_cpuidle;
>>
>> This is not the place.
>>
>>> +
>>> +	pr_info("e500_idle_driver registered.\n");
>>> +
>>> +	return 0;
>>> +
>>> +out:
>>> +	e500_idle_devices_uninit();
>>> +	cpuidle_unregister_driver(drv);
>>> +
>>> +	pr_err("Register e500 family cpuidle driver failed.\n");
>>> +
>>> +	return err;
>>> +}
>>> +device_initcall(e500_idle_init);
>>>
>>
>>
>> --
>>  <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
> 

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Scott Wood]

On 07/30/2013 02:00:03 AM, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>=20
> Add cpuidle support for e500 family, using cpuidle framework to
> manage various low power modes. The new implementation will remain
> compatible with original idle method.
>=20
> Initially, this supports PW10, and subsequent patches will support
> PW20/DOZE/NAP.

Could you explain what the cpuidle framework does for us that the =20
current
idle code doesn't?

In particular, what scenario do you see where we would require a =20
software
governor to choose between idle states, and how much power is saved
compared to a simpler approach?  There is timer that can be used to
automatically enter PW20 after a certain amount of time in PW10.  How
much better results do you get from a software governor?  Do we even =20
have
the right data to characterize each state so that a software governor
could make good decisions?  Is cpuidle capable of governing the interval
of such a timer, rather than directly governing states?

As for doze/nap, why would we want to use those on newer cores?  Do you
have numbers for how much power each mode saves?

Active governors may be useful on older cores that only have doze/nap, =20
to
select between them, but if that's the use case then why start with =20
pw10?
And I'd want to see numbers for how much power nap saves versus doze.

> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
> This patch keep using cpuidle_register_device(), because we need to =20
> support cpu
> hotplug. I will fix "device" issue in this driver, after
> Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler =20
> into cpuidle
> freamwork.

Where's the diffstat?

> diff --git a/arch/powerpc/include/asm/machdep.h =20
> b/arch/powerpc/include/asm/machdep.h
> index 8b48090..cbdbe25 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -271,6 +271,16 @@ extern void power7_idle(void);
>  extern void ppc6xx_idle(void);
>  extern void book3e_idle(void);
>=20
> +/* Wait for Interrupt */
> +static inline void fsl_cpuidle_wait(void)
> +{
> +#ifdef CONFIG_PPC64
> +	book3e_idle();
> +#else
> +	e500_idle();
> +#endif
> +}

Where is this used?

> +
>  /*
>   * ppc_md contains a copy of the machine description structure for =20
> the
>   * current platform. machine_id contains the initial address where =20
> the
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index b3fb81d..7ed114b 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -35,6 +35,11 @@ depends on ARM
>  source "drivers/cpuidle/Kconfig.arm"
>  endmenu
>=20
> +menu "PowerPC CPU Idle Drivers"
> +depends on PPC32 || PPC64

depends on PPC

> diff --git a/drivers/cpuidle/Kconfig.powerpc =20
> b/drivers/cpuidle/Kconfig.powerpc
> new file mode 100644
> index 0000000..9f3f5ef
> --- /dev/null
> +++ b/drivers/cpuidle/Kconfig.powerpc
> @@ -0,0 +1,9 @@
> +#
> +# PowerPC CPU Idle drivers
> +#
> +
> +config PPC_E500_CPUIDLE
> +	bool "CPU Idle Driver for E500 family processors"
> +	depends on FSL_SOC_BOOKE
> +	help
> +	  Select this to enable cpuidle on e500 family processors.

FSL_SOC_BOOKE is more than just e500

> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 0b9d200..0dde3db 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -11,3 +11,7 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+=3D =20
> cpuidle-calxeda.o
>  obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+=3D cpuidle-kirkwood.o
>  obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+=3D cpuidle-zynq.o
>  obj-$(CONFIG_ARM_U8500_CPUIDLE)         +=3D cpuidle-ux500.o
> +
> +########################################################################=
##########
> +# PowerPC platform drivers
> +obj-$(CONFIG_PPC_E500_CPUIDLE)		+=3D cpuidle-e500.o
> diff --git a/drivers/cpuidle/cpuidle-e500.c =20
> b/drivers/cpuidle/cpuidle-e500.c
> new file mode 100644
> index 0000000..1919cea
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-e500.c
> @@ -0,0 +1,222 @@
> +/*
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * CPU Idle driver for Freescale PowerPC e500 family processors.
> + *
> + * This program is free software; you can redistribute it and/or =20
> modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
> + */

Is this derived from some other file?  It looks like it...  Where's the
attribution?

> +#include <linux/cpu.h>
> +#include <linux/cpuidle.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +
> +#include <asm/machdep.h>
> +
> +static struct cpuidle_driver e500_idle_driver =3D {
> +	.name =3D "e500_idle",
> +	.owner =3D THIS_MODULE,
> +};
> +
> +static struct cpuidle_device __percpu *e500_cpuidle_devices;
> +
> +static void e500_cpuidle(void)
> +{
> +	/*
> +	 * This would call on the cpuidle framework, and the back-end
> +	 * driver to go to idle states.
> +	 */
> +	if (cpuidle_idle_call()) {
> +		/*
> +		 * On error, execute default handler
> +		 * to go into low thread priority and possibly
> +		 * low power mode.
> +		 */
> +		HMT_low();
> +		HMT_very_low();

This HMT stuff doesn't do anything on e500 derivatives AFAIK.

> +static struct cpuidle_state fsl_pw_idle_states[] =3D {
> +	{
> +		.name =3D "pw10",
> +		.desc =3D "pw10",
> +		.flags =3D CPUIDLE_FLAG_TIME_VALID,
> +		.exit_latency =3D 0,
> +		.target_residency =3D 0,
> +		.enter =3D &pw10_enter

Where is pw10_enter defined?

> +static int cpu_is_feature(unsigned long feature)
> +{
> +	return (cur_cpu_spec->cpu_features =3D=3D feature);
> +}
> +
> +static int __init e500_idle_init(void)
> +{
> +	struct cpuidle_state *cpuidle_state_table =3D NULL;
> +	struct cpuidle_driver *drv =3D &e500_idle_driver;
> +	int err;
> +	unsigned int max_idle_state =3D 0;
> +
> +	if (cpuidle_disable !=3D IDLE_NO_OVERRIDE)
> +		return -ENODEV;
> +
> +	if (cpu_is_feature(CPU_FTRS_E500MC) || =20
> cpu_is_feature(CPU_FTRS_E5500) ||
> +			cpu_is_feature(CPU_FTRS_E6500)) {

There's no guarantee that a CPU with the same set of features is the
exact same CPU.

What specific feature are you looking for here?

> +		cpuidle_state_table =3D fsl_pw_idle_states;
> +		max_idle_state =3D ARRAY_SIZE(fsl_pw_idle_states);
> +	}
> +
> +	if (!cpuidle_state_table || !max_idle_state)
> +		return -EPERM;

ENODEV?

-Scott=

RE: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, July 31, 2013 3:38 AM
> To: Wang Dongsheng-B40534
> Cc: rjw@sisk.pl; daniel.lezcano@linaro.org; benh@kernel.crashing.org; Li
> Yang-R58472; Zhao Chenhui-B35336; linux-pm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle
> support
>=20
> On 07/30/2013 02:00:03 AM, Dongsheng Wang wrote:
> > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> >
> > Add cpuidle support for e500 family, using cpuidle framework to
> > manage various low power modes. The new implementation will remain
> > compatible with original idle method.
> >
> > Initially, this supports PW10, and subsequent patches will support
> > PW20/DOZE/NAP.
>=20
> Could you explain what the cpuidle framework does for us that the
> current idle code doesn't?
>=20

The current idle code, Only a state of low power can make the core idle.
The core can't get into more low power state.

> In particular, what scenario do you see where we would require a
> software
> governor to choose between idle states, and how much power is saved
> compared to a simpler approach?  There is timer that can be used to
> automatically enter PW20 after a certain amount of time in PW10.

Yes, the hardware can automatically enter PW20 state. But this is hardware
feature, we need to software to manage it. Only for PW20 state, we can drop
this cpuidle and using the hardware to control it. But if we need to suppor=
t
PH10/PH15/PH20/PH30, the current idle code cannot support them.=20

> How much better results do you get from a software governor?  Do we even
> have the right data to characterize each state so that a software governo=
r
> could make good decisions?  Is cpuidle capable of governing the interval
> of such a timer, rather than directly governing states?
>=20
>From now on we did not benchmark these data, because we only have PW10 stat=
e.

I can do support doze/nap for e6500. To get some data to show you.

> As for doze/nap, why would we want to use those on newer cores?  Do you
> have numbers for how much power each mode saves?
>=20
The PH state is plan to support, if the core can make into more low power s=
tate,
why not to do this.

PH10(doze)/PH15(nap)/PH20/PH30, These states can save more CPU power.

> Active governors may be useful on older cores that only have doze/nap,
> to
> select between them, but if that's the use case then why start with
> pw10?
Pw10 is supported on E500MC/E5500/E6500. And we plan to support PW20 for E6=
5OO core.
I will take doze/nap up a bit later.

> And I'd want to see numbers for how much power nap saves versus doze.
>=20
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > ---
> > This patch keep using cpuidle_register_device(), because we need to
> > support cpu
> > hotplug. I will fix "device" issue in this driver, after
> > Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler
> > into cpuidle
> > freamwork.
>=20
> Where's the diffstat?
>=20
See, http://patchwork.ozlabs.org/patch/260997/

> > @@ -0,0 +1,222 @@
> > +/*
> > + * Copyright 2013 Freescale Semiconductor, Inc.
> > + *
> > + * CPU Idle driver for Freescale PowerPC e500 family processors.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
> > + */
>=20
> Is this derived from some other file?  It looks like it...  Where's the
> attribution?
>=20
The copyright is from drivers/cpufreq/ppc-corenet-cpufreq.c

> > +#include <linux/cpu.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/notifier.h>
> > +
> > +#include <asm/machdep.h>
> > +
> > +static struct cpuidle_driver e500_idle_driver =3D {
> > +	.name =3D "e500_idle",
> > +	.owner =3D THIS_MODULE,
> > +};
> > +
> > +static struct cpuidle_device __percpu *e500_cpuidle_devices;
> > +
> > +static void e500_cpuidle(void)
> > +{
> > +	/*
> > +	 * This would call on the cpuidle framework, and the back-end
> > +	 * driver to go to idle states.
> > +	 */
> > +	if (cpuidle_idle_call()) {
> > +		/*
> > +		 * On error, execute default handler
> > +		 * to go into low thread priority and possibly
> > +		 * low power mode.
> > +		 */
> > +		HMT_low();
> > +		HMT_very_low();
>=20
> This HMT stuff doesn't do anything on e500 derivatives AFAIK.
>=20
Yes, there should do nothing, let arch_cpu_idle to do the failed.

> > +static struct cpuidle_state fsl_pw_idle_states[] =3D {
> > +	{
> > +		.name =3D "pw10",
> > +		.desc =3D "pw10",
> > +		.flags =3D CPUIDLE_FLAG_TIME_VALID,
> > +		.exit_latency =3D 0,
> > +		.target_residency =3D 0,
> > +		.enter =3D &pw10_enter
>=20
> Where is pw10_enter defined?
>=20
In this patch..

> > +static int cpu_is_feature(unsigned long feature)
> > +{
> > +	return (cur_cpu_spec->cpu_features =3D=3D feature);
> > +}
> > +
> > +static int __init e500_idle_init(void)
> > +{
> > +	struct cpuidle_state *cpuidle_state_table =3D NULL;
> > +	struct cpuidle_driver *drv =3D &e500_idle_driver;
> > +	int err;
> > +	unsigned int max_idle_state =3D 0;
> > +
> > +	if (cpuidle_disable !=3D IDLE_NO_OVERRIDE)
> > +		return -ENODEV;
> > +
> > +	if (cpu_is_feature(CPU_FTRS_E500MC) ||
> > cpu_is_feature(CPU_FTRS_E5500) ||
> > +			cpu_is_feature(CPU_FTRS_E6500)) {
>=20
> There's no guarantee that a CPU with the same set of features is the
> exact same CPU.
>=20
> What specific feature are you looking for here?
>=20
Here is the type of the core. E500MC,E5500,E6500 do wait.

> > +		cpuidle_state_table =3D fsl_pw_idle_states;
> > +		max_idle_state =3D ARRAY_SIZE(fsl_pw_idle_states);
> > +	}
> > +
> > +	if (!cpuidle_state_table || !max_idle_state)
> > +		return -EPERM;
>=20
> ENODEV?
>=20
Looks better than EPERM.

-dongsheng

Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Scott Wood]

On 07/31/2013 01:30:06 AM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, July 31, 2013 3:38 AM
> > To: Wang Dongsheng-B40534
> > Cc: rjw@sisk.pl; daniel.lezcano@linaro.org; =20
> benh@kernel.crashing.org; Li
> > Yang-R58472; Zhao Chenhui-B35336; linux-pm@vger.kernel.org; =20
> linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors =20
> idle
> > support
> >
> > On 07/30/2013 02:00:03 AM, Dongsheng Wang wrote:
> > > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > >
> > > Add cpuidle support for e500 family, using cpuidle framework to
> > > manage various low power modes. The new implementation will remain
> > > compatible with original idle method.
> > >
> > > Initially, this supports PW10, and subsequent patches will support
> > > PW20/DOZE/NAP.
> >
> > Could you explain what the cpuidle framework does for us that the
> > current idle code doesn't?
> >
>=20
> The current idle code, Only a state of low power can make the core =20
> idle.
> The core can't get into more low power state.
>=20
> > In particular, what scenario do you see where we would require a
> > software
> > governor to choose between idle states, and how much power is saved
> > compared to a simpler approach?  There is timer that can be used to
> > automatically enter PW20 after a certain amount of time in PW10.
>=20
> Yes, the hardware can automatically enter PW20 state. But this is =20
> hardware
> feature, we need to software to manage it. Only for PW20 state, we =20
> can drop
> this cpuidle and using the hardware to control it. But if we need to =20
> support
> PH10/PH15/PH20/PH30, the current idle code cannot support them.

PH30 wasn't really meant as idle state, but rather a CPU hotplug =20
state.  This isn't just about enter/exit latency (and complexity) but =20
also about wakeup events.

PH15 and PH20 were mainly intended as intermediate states on the way to =20
PH30 or debug halt.  It looks like PH15 is the deepest PH state in =20
which core timers continue to run.

The intended idle states on e6500 are PW10 and PW20.

> > How much better results do you get from a software governor?  Do we =20
> even
> > have the right data to characterize each state so that a software =20
> governor
> > could make good decisions?  Is cpuidle capable of governing the =20
> interval
> > of such a timer, rather than directly governing states?
> >
> >From now on we did not benchmark these data, because we only have =20
> PW10 state.
>=20
> I can do support doze/nap for e6500. To get some data to show you.
>=20
> > As for doze/nap, why would we want to use those on newer cores?  Do =20
> you
> > have numbers for how much power each mode saves?
> >
> The PH state is plan to support, if the core can make into more low =20
> power state,
> why not to do this.

We don't do things just to do them.  We do things to make things better.

> PH10(doze)/PH15(nap)/PH20/PH30, These states can save more CPU power.

If you have evidence that PH15 saves more power than PW20, please =20
provide it.

> > Active governors may be useful on older cores that only have =20
> doze/nap,
> > to
> > select between them, but if that's the use case then why start with
> > pw10?
> Pw10 is supported on E500MC/E5500/E6500. And we plan to support PW20 =20
> for E65OO core.
> I will take doze/nap up a bit later.

By "older cores" I meant before e500mc.

> > And I'd want to see numbers for how much power nap saves versus =20
> doze.
> >
> > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > ---
> > > This patch keep using cpuidle_register_device(), because we need =20
> to
> > > support cpu
> > > hotplug. I will fix "device" issue in this driver, after
> > > Deepthi Dharwar <deepthi@linux.vnet.ibm.com> add a hotplug handler
> > > into cpuidle
> > > freamwork.
> >
> > Where's the diffstat?
> >
> See, http://patchwork.ozlabs.org/patch/260997/

That's a totally different patch.

> > > @@ -0,0 +1,222 @@
> > > +/*
> > > + * Copyright 2013 Freescale Semiconductor, Inc.
> > > + *
> > > + * CPU Idle driver for Freescale PowerPC e500 family processors.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > modify
> > > + * it under the terms of the GNU General Public License version =20
> 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * Author: Wang Dongsheng <dongsheng.wang@freescale.com>
> > > + */
> >
> > Is this derived from some other file?  It looks like it...  Where's =20
> the
> > attribution?
> >
> The copyright is from drivers/cpufreq/ppc-corenet-cpufreq.c

But the code isn't.

> > > +static void e500_cpuidle(void)
> > > +{
> > > +	/*
> > > +	 * This would call on the cpuidle framework, and the back-end
> > > +	 * driver to go to idle states.
> > > +	 */
> > > +	if (cpuidle_idle_call()) {
> > > +		/*
> > > +		 * On error, execute default handler
> > > +		 * to go into low thread priority and possibly
> > > +		 * low power mode.
> > > +		 */
> > > +		HMT_low();
> > > +		HMT_very_low();
> >
> > This HMT stuff doesn't do anything on e500 derivatives AFAIK.
> >
> Yes, there should do nothing, let arch_cpu_idle to do the failed.

I can't parse this.

> > > +static struct cpuidle_state fsl_pw_idle_states[] =3D {
> > > +	{
> > > +		.name =3D "pw10",
> > > +		.desc =3D "pw10",
> > > +		.flags =3D CPUIDLE_FLAG_TIME_VALID,
> > > +		.exit_latency =3D 0,
> > > +		.target_residency =3D 0,
> > > +		.enter =3D &pw10_enter
> >
> > Where is pw10_enter defined?
> >
> In this patch..

OK, I see it now -- sorry about that.

> > > +static int cpu_is_feature(unsigned long feature)
> > > +{
> > > +	return (cur_cpu_spec->cpu_features =3D=3D feature);
> > > +}
> > > +
> > > +static int __init e500_idle_init(void)
> > > +{
> > > +	struct cpuidle_state *cpuidle_state_table =3D NULL;
> > > +	struct cpuidle_driver *drv =3D &e500_idle_driver;
> > > +	int err;
> > > +	unsigned int max_idle_state =3D 0;
> > > +
> > > +	if (cpuidle_disable !=3D IDLE_NO_OVERRIDE)
> > > +		return -ENODEV;
> > > +
> > > +	if (cpu_is_feature(CPU_FTRS_E500MC) ||
> > > cpu_is_feature(CPU_FTRS_E5500) ||
> > > +			cpu_is_feature(CPU_FTRS_E6500)) {
> >
> > There's no guarantee that a CPU with the same set of features is the
> > exact same CPU.
> >
> > What specific feature are you looking for here?
> >
> Here is the type of the core. E500MC,E5500,E6500 do wait.

There's no guarantee that a CPU with the same set of features is the =20
exact same CPU.

The CPU feature mechanism is for checking *features*, not identity.

-Scott=