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

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]

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

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

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.  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 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,  
to
select between them, but if that's the use case then why start with  
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  
> 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?

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

Where is this used?

> +
>  /*
>   * 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

depends on PPC

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

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)	+=  
> 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>
> + */

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 = {
> +	.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();

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

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

Where is pw10_enter defined?

> +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)) {

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 = fsl_pw_idle_states;
> +		max_idle_state = 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

[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

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

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 support
PH10/PH15/PH20/PH30, the current idle code cannot support them. 

> 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 governor
> could make good decisions?  Is cpuidle capable of governing the interval
> of such a timer, rather than directly governing states?
> 
>From now on we did not benchmark these data, because we only have PW10 state.

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?
> 
The PH state is plan to support, if the core can make into more low power state,
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 E65OO 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.
> 
> > 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.
> 
> Where's the diffstat?
> 
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>
> > + */
> 
> Is this derived from some other file?  It looks like it...  Where's the
> attribution?
> 
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 = {
> > +	.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();
> 
> This HMT stuff doesn't do anything on e500 derivatives AFAIK.
> 
Yes, there should do nothing, let arch_cpu_idle to do the failed.

> > +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
> 
> Where is pw10_enter defined?
> 
In this patch..

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

> > +		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;
> 
> ENODEV?
> 
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:
> 
> 
> > -----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
> >
> > 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?
> >
> 
> 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  
> 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  
state.  This isn't just about enter/exit latency (and complexity) but  
also about wakeup events.

PH15 and PH20 were mainly intended as intermediate states on the way to  
PH30 or debug halt.  It looks like PH15 is the deepest PH state in  
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  
> even
> > 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?
> >
> >From now on we did not benchmark these data, because we only have  
> PW10 state.
> 
> 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?
> >
> The PH state is plan to support, if the core can make into more low  
> 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  
provide it.

> > 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 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  
> doze.
> >
> > > 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.
> >
> > 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  
> 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?
> >
> 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[] = {
> > > +	{
> > > +		.name = "pw10",
> > > +		.desc = "pw10",
> > > +		.flags = CPUIDLE_FLAG_TIME_VALID,
> > > +		.exit_latency = 0,
> > > +		.target_residency = 0,
> > > +		.enter = &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 == 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)) {
> >
> > 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  
exact same CPU.

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

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

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


_______________________________________________
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

[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

_______________________________________________
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

[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

_______________________________________________
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

[Dongsheng.Wang]

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.

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

_______________________________________________
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

[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

_______________________________________________
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

[Dongsheng.Wang]

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

Regards,
-Dongsheng

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

_______________________________________________
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

[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

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