[PATCH 1/5] ARM: exynos: factor out the idle states

[PATCH 1/5] ARM: exynos: factor out the idle states

[Daniel Lezcano]

The states are defined in the driver. We can get rid of the
intermediate cpuidle states initialization and the memcpy by
directly initializing the driver states.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |   45 +++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 0509241..f8dd1cd 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -40,24 +40,25 @@ static int exynos4_enter_lowpower(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index);
 
-static struct cpuidle_state exynos4_cpuidle_set[] __initdata = {
-	[0] = ARM_CPUIDLE_WFI_STATE,
-	[1] = {
-		.enter			= exynos4_enter_lowpower,
-		.exit_latency		= 300,
-		.target_residency	= 100000,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "C1",
-		.desc			= "ARM power down",
-	},
-};
-
 static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
 
 static struct cpuidle_driver exynos4_idle_driver = {
 	.name			= "exynos4_idle",
 	.owner			= THIS_MODULE,
 	.en_core_tk_irqen	= 1,
+	.states = {
+		[0] = ARM_CPUIDLE_WFI_STATE,
+		[1] = {
+			.enter            = exynos4_enter_lowpower,
+			.exit_latency     = 300,
+			.target_residency = 100000,
+			.flags            = CPUIDLE_FLAG_TIME_VALID,
+			.name             = "C1",
+			.desc             = "ARM power down",
+		},
+	},
+	.state_count = 2,
+	.safe_state_index = 0,
 };
 
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
@@ -192,33 +193,21 @@ static void __init exynos5_core_down_clk(void)
 
 static int __init exynos4_init_cpuidle(void)
 {
-	int i, max_cpuidle_state, cpu_id;
+	int cpu_id;
 	struct cpuidle_device *device;
-	struct cpuidle_driver *drv = &exynos4_idle_driver;
 
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
 
-	/* Setup cpuidle driver */
-	drv->state_count = (sizeof(exynos4_cpuidle_set) /
-				       sizeof(struct cpuidle_state));
-	max_cpuidle_state = drv->state_count;
-	for (i = 0; i < max_cpuidle_state; i++) {
-		memcpy(&drv->states[i], &exynos4_cpuidle_set[i],
-				sizeof(struct cpuidle_state));
-	}
-	drv->safe_state_index = 0;
 	cpuidle_register_driver(&exynos4_idle_driver);
 
 	for_each_cpu(cpu_id, cpu_online_mask) {
 		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
 		device->cpu = cpu_id;
 
-		if (cpu_id == 0)
-			device->state_count = (sizeof(exynos4_cpuidle_set) /
-					       sizeof(struct cpuidle_state));
-		else
-			device->state_count = 1;	/* Support IDLE only */
+                /* Support IDLE only */
+		if (cpu_id != 0)
+			device->state_count = 1;
 
 		if (cpuidle_register_device(device)) {
 			printk(KERN_ERR "CPUidle register device failed\n,");
-- 
1.7.9.5

[PATCH 2/5] ARM: exynos: handle properly the return values

[Daniel Lezcano]

The cpuidle_register_driver return value is not checked.
The init function returns always -EIO when cpuidle_register_device
fails but the error could be different.

This patch fixes that by checking the cpuidle_register_driver properly
and returning the correct value when cpuidle_register_device fails.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index f8dd1cd..a5fe194 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -193,13 +193,17 @@ static void __init exynos5_core_down_clk(void)
 
 static int __init exynos4_init_cpuidle(void)
 {
-	int cpu_id;
+	int cpu_id, ret;
 	struct cpuidle_device *device;
 
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
 
-	cpuidle_register_driver(&exynos4_idle_driver);
+	ret = cpuidle_register_driver(&exynos4_idle_driver);
+	if (ret) {
+		printk(KERN_ERR "CPUidle failed to register driver\n");
+		return ret;
+	}
 
 	for_each_cpu(cpu_id, cpu_online_mask) {
 		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
@@ -209,9 +213,10 @@ static int __init exynos4_init_cpuidle(void)
 		if (cpu_id != 0)
 			device->state_count = 1;
 
-		if (cpuidle_register_device(device)) {
-			printk(KERN_ERR "CPUidle register device failed\n,");
-			return -EIO;
+		ret = cpuidle_register_device(device);
+		if (ret) {
+			printk(KERN_ERR "CPUidle register device failed\n");
+			return ret;
 		}
 	}
 
-- 
1.7.9.5

[PATCH 3/5] ARM: exynos: replace cpumask by the corresponding macro

[Daniel Lezcano]

A trivial patch to replace for_each_cpu(cpu_id, cpu_online_mask) by
the corresponding macro.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index a5fe194..6e90bed 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -205,7 +205,7 @@ static int __init exynos4_init_cpuidle(void)
 		return ret;
 	}
 
-	for_each_cpu(cpu_id, cpu_online_mask) {
+	for_each_online_cpu(cpu_id) {
 		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
 		device->cpu = cpu_id;
 
-- 
1.7.9.5

[PATCH 4/5] ARM: exynos: only register cpuidle for cpu0

[Daniel Lezcano]

We register the device for cpu1 but with only one state which
is actually WFI. This one is already the default idle function
when no cpuidle device is set for the cpu.

We can remove the cpuidle device for this cpu as it is the same
code path than the pm_idle callback.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |   22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 6e90bed..e6f006b 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -40,7 +40,7 @@ static int exynos4_enter_lowpower(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index);
 
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
+static struct cpuidle_device exynos4_cpuidle_device;
 
 static struct cpuidle_driver exynos4_idle_driver = {
 	.name			= "exynos4_idle",
@@ -193,8 +193,7 @@ static void __init exynos5_core_down_clk(void)
 
 static int __init exynos4_init_cpuidle(void)
 {
-	int cpu_id, ret;
-	struct cpuidle_device *device;
+	int ret;
 
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
@@ -205,19 +204,10 @@ static int __init exynos4_init_cpuidle(void)
 		return ret;
 	}
 
-	for_each_online_cpu(cpu_id) {
-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-                /* Support IDLE only */
-		if (cpu_id != 0)
-			device->state_count = 1;
-
-		ret = cpuidle_register_device(device);
-		if (ret) {
-			printk(KERN_ERR "CPUidle register device failed\n");
-			return ret;
-		}
+	ret = cpuidle_register_device(&exynos4_cpuidle_device);
+	if (ret) {
+		printk(KERN_ERR "CPUidle register device failed\n");
+		return ret;
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[Daniel Lezcano]

What we have now is (1) cpu0 going always to WFI when cpu1 is up,
(2) cpu0 going to all states when cpu1 is down.

In other words, cpuidle is disabled when cpu1 is up and enabled
when cpu1 is down.

This patch use the cpu hotplug notifier to enable/disable cpuidle,
when the cpu1 is plugged or unplugged. That clarifies the code and
make it simpler.

Signed-off: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-exynos/cpuidle.c |  117 +++++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 48 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index e6f006b..d8f6f33 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -12,6 +12,8 @@
 #include <linux/init.h>
 #include <linux/cpuidle.h>
 #include <linux/cpu_pm.h>
+#include <linux/notifier.h>
+#include <linux/cpu.h>
 #include <linux/io.h>
 #include <linux/export.h>
 #include <linux/time.h>
@@ -36,31 +38,8 @@
 
 #define S5P_CHECK_AFTR		0xFCBA0D10
 
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index);
-
 static struct cpuidle_device exynos4_cpuidle_device;
 
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name			= "exynos4_idle",
-	.owner			= THIS_MODULE,
-	.en_core_tk_irqen	= 1,
-	.states = {
-		[0] = ARM_CPUIDLE_WFI_STATE,
-		[1] = {
-			.enter            = exynos4_enter_lowpower,
-			.exit_latency     = 300,
-			.target_residency = 100000,
-			.flags            = CPUIDLE_FLAG_TIME_VALID,
-			.name             = "C1",
-			.desc             = "ARM power down",
-		},
-	},
-	.state_count = 2,
-	.safe_state_index = 0,
-};
-
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
 static void exynos4_set_wakeupmask(void)
 {
@@ -93,9 +72,9 @@ static int idle_finisher(unsigned long flags)
 	return 1;
 }
 
-static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index)
+static int exynos4_enter_lowpower(struct cpuidle_device *dev,
+				  struct cpuidle_driver *drv,
+				  int index)
 {
 	unsigned long tmp;
 
@@ -143,22 +122,6 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
 	return index;
 }
 
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index)
-{
-	int new_index = index;
-
-	/* This mode only can be entered when other core's are offline */
-	if (num_online_cpus() > 1)
-		new_index = drv->safe_state_index;
-
-	if (new_index == 0)
-		return arm_cpuidle_simple_enter(dev, drv, new_index);
-	else
-		return exynos4_enter_core0_aftr(dev, drv, new_index);
-}
-
 static void __init exynos5_core_down_clk(void)
 {
 	unsigned int tmp;
@@ -191,6 +154,62 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
+{
+	int ret;
+
+	/*
+	 * cpu0 can't be shutdown on Origen, so this routine is
+	 * called only for cpu1.
+	 */
+
+	switch (action & 0xf) {
+
+	case CPU_ONLINE:
+		cpuidle_pause_and_lock();
+		cpuidle_disable_device(&exynos4_cpuidle_device);
+		cpuidle_resume_and_unlock();
+		break;
+
+	case CPU_DEAD:
+		if (!exynos4_cpuidle_device.registered) {
+			ret = cpuidle_register_device(&exynos4_cpuidle_device);
+			WARN_ONCE(ret, "Failed to register cpuidle device");
+		} else {
+			cpuidle_pause_and_lock();
+			cpuidle_enable_device(&exynos4_cpuidle_device);
+			cpuidle_resume_and_unlock();
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
+};
+
+static struct cpuidle_driver exynos4_idle_driver = {
+	.name			= "exynos4_idle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states = {
+		[0] = ARM_CPUIDLE_WFI_STATE,
+		[1] = {
+			.enter            = exynos4_enter_lowpower,
+			.exit_latency     = 300,
+			.target_residency = 100000,
+			.flags            = CPUIDLE_FLAG_TIME_VALID,
+			.name             = "C1",
+			.desc             = "ARM power down",
+		},
+	},
+	.state_count = 2,
+	.safe_state_index = 0,
+};
+
 static int __init exynos4_init_cpuidle(void)
 {
 	int ret;
@@ -204,12 +223,14 @@ static int __init exynos4_init_cpuidle(void)
 		return ret;
 	}
 
-	ret = cpuidle_register_device(&exynos4_cpuidle_device);
-	if (ret) {
-		printk(KERN_ERR "CPUidle register device failed\n");
-		return ret;
-	}
+	ret = register_cpu_notifier(&cpu_hotplug_notifier);
+	if (ret)
+		goto out_unregister_driver;
+out:
+	return ret;
 
-	return 0;
+out_unregister_driver:
+	cpuidle_unregister_driver(&exynos4_idle_driver);
+	goto out;
 }
 device_initcall(exynos4_init_cpuidle);
-- 
1.7.9.5

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[amit daniel kachhap]

Hi Daniel,

This hotplug noifiers looks fine. I suppose it should add extra state
C1 in cpu0. If it is done like below than for normal cases(when all
cpu's are online) there wont be any statistics for C0 state also which
is required. Other patches look good.

Thanks,
Amit

On Fri, Jan 4, 2013 at 8:59 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> What we have now is (1) cpu0 going always to WFI when cpu1 is up,
> (2) cpu0 going to all states when cpu1 is down.
>
> In other words, cpuidle is disabled when cpu1 is up and enabled
> when cpu1 is down.
>
> This patch use the cpu hotplug notifier to enable/disable cpuidle,
> when the cpu1 is plugged or unplugged. That clarifies the code and
> make it simpler.
>
> Signed-off: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |  117 +++++++++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 48 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index e6f006b..d8f6f33 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -12,6 +12,8 @@
>  #include <linux/init.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpu_pm.h>
> +#include <linux/notifier.h>
> +#include <linux/cpu.h>
>  #include <linux/io.h>
>  #include <linux/export.h>
>  #include <linux/time.h>
> @@ -36,31 +38,8 @@
>
>  #define S5P_CHECK_AFTR         0xFCBA0D10
>
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -                               struct cpuidle_driver *drv,
> -                               int index);
> -
>  static struct cpuidle_device exynos4_cpuidle_device;
>
> -static struct cpuidle_driver exynos4_idle_driver = {
> -       .name                   = "exynos4_idle",
> -       .owner                  = THIS_MODULE,
> -       .en_core_tk_irqen       = 1,
> -       .states = {
> -               [0] = ARM_CPUIDLE_WFI_STATE,
> -               [1] = {
> -                       .enter            = exynos4_enter_lowpower,
> -                       .exit_latency     = 300,
> -                       .target_residency = 100000,
> -                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> -                       .name             = "C1",
> -                       .desc             = "ARM power down",
> -               },
> -       },
> -       .state_count = 2,
> -       .safe_state_index = 0,
> -};
> -
>  /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>  static void exynos4_set_wakeupmask(void)
>  {
> @@ -93,9 +72,9 @@ static int idle_finisher(unsigned long flags)
>         return 1;
>  }
>
> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
> -                               struct cpuidle_driver *drv,
> -                               int index)
> +static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> +                                 struct cpuidle_driver *drv,
> +                                 int index)
>  {
>         unsigned long tmp;
>
> @@ -143,22 +122,6 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>         return index;
>  }
>
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -                               struct cpuidle_driver *drv,
> -                               int index)
> -{
> -       int new_index = index;
> -
> -       /* This mode only can be entered when other core's are offline */
> -       if (num_online_cpus() > 1)
> -               new_index = drv->safe_state_index;
> -
> -       if (new_index == 0)
> -               return arm_cpuidle_simple_enter(dev, drv, new_index);
> -       else
> -               return exynos4_enter_core0_aftr(dev, drv, new_index);
> -}
> -
>  static void __init exynos5_core_down_clk(void)
>  {
>         unsigned int tmp;
> @@ -191,6 +154,62 @@ static void __init exynos5_core_down_clk(void)
>         __raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>  }
>
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +                             unsigned long action, void *hcpu)
> +{
> +       int ret;
> +
> +       /*
> +        * cpu0 can't be shutdown on Origen, so this routine is
> +        * called only for cpu1.
> +        */
> +
> +       switch (action & 0xf) {
> +
> +       case CPU_ONLINE:
> +               cpuidle_pause_and_lock();
> +               cpuidle_disable_device(&exynos4_cpuidle_device);
> +               cpuidle_resume_and_unlock();
> +               break;
> +
> +       case CPU_DEAD:
> +               if (!exynos4_cpuidle_device.registered) {
> +                       ret = cpuidle_register_device(&exynos4_cpuidle_device);
> +                       WARN_ONCE(ret, "Failed to register cpuidle device");
> +               } else {
> +                       cpuidle_pause_and_lock();
> +                       cpuidle_enable_device(&exynos4_cpuidle_device);
> +                       cpuidle_resume_and_unlock();
> +               }
> +               break;
> +       }
> +
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block cpu_hotplug_notifier = {
> +       .notifier_call = cpu_hotplug_notify,
> +};
> +
> +static struct cpuidle_driver exynos4_idle_driver = {
> +       .name                   = "exynos4_idle",
> +       .owner                  = THIS_MODULE,
> +       .en_core_tk_irqen       = 1,
> +       .states = {
> +               [0] = ARM_CPUIDLE_WFI_STATE,
> +               [1] = {
> +                       .enter            = exynos4_enter_lowpower,
> +                       .exit_latency     = 300,
> +                       .target_residency = 100000,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C1",
> +                       .desc             = "ARM power down",
> +               },
> +       },
> +       .state_count = 2,
> +       .safe_state_index = 0,
> +};
> +
>  static int __init exynos4_init_cpuidle(void)
>  {
>         int ret;
> @@ -204,12 +223,14 @@ static int __init exynos4_init_cpuidle(void)
>                 return ret;
>         }
>
> -       ret = cpuidle_register_device(&exynos4_cpuidle_device);
> -       if (ret) {
> -               printk(KERN_ERR "CPUidle register device failed\n");
> -               return ret;
> -       }
> +       ret = register_cpu_notifier(&cpu_hotplug_notifier);
> +       if (ret)
> +               goto out_unregister_driver;
> +out:
> +       return ret;
>
> -       return 0;
> +out_unregister_driver:
> +       cpuidle_unregister_driver(&exynos4_idle_driver);
> +       goto out;
>  }
>  device_initcall(exynos4_init_cpuidle);
> --
> 1.7.9.5
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[Daniel Lezcano]

On 01/10/2013 09:07 PM, amit daniel kachhap wrote:
> Hi Daniel,

Hi Amit Daniel,

> This hotplug noifiers looks fine. I suppose it should add extra state
> C1 in cpu0. If it is done like below than for normal cases(when all
> cpu's are online) there wont be any statistics for C0 state 

I guess you meant state 0 which is WFI, right ?
C0 state is the intel semantic for cpu fully turned on.

> also which
> is required. Other patches look good.

Ok, that makes sense to have statistics even if they are only doing WFI.

Then the patch 4/5 is not ok, no ?

Thanks for review
  -- Daniel

>
> Thanks,
> Amit
>
> On Fri, Jan 4, 2013 at 8:59 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> What we have now is (1) cpu0 going always to WFI when cpu1 is up,
>> (2) cpu0 going to all states when cpu1 is down.
>>
>> In other words, cpuidle is disabled when cpu1 is up and enabled
>> when cpu1 is down.
>>
>> This patch use the cpu hotplug notifier to enable/disable cpuidle,
>> when the cpu1 is plugged or unplugged. That clarifies the code and
>> make it simpler.
>>
>> Signed-off: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  arch/arm/mach-exynos/cpuidle.c |  117 +++++++++++++++++++++++-----------------
>>  1 file changed, 69 insertions(+), 48 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
>> index e6f006b..d8f6f33 100644
>> --- a/arch/arm/mach-exynos/cpuidle.c
>> +++ b/arch/arm/mach-exynos/cpuidle.c
>> @@ -12,6 +12,8 @@
>>  #include <linux/init.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/cpu_pm.h>
>> +#include <linux/notifier.h>
>> +#include <linux/cpu.h>
>>  #include <linux/io.h>
>>  #include <linux/export.h>
>>  #include <linux/time.h>
>> @@ -36,31 +38,8 @@
>>
>>  #define S5P_CHECK_AFTR         0xFCBA0D10
>>
>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>> -                               struct cpuidle_driver *drv,
>> -                               int index);
>> -
>>  static struct cpuidle_device exynos4_cpuidle_device;
>>
>> -static struct cpuidle_driver exynos4_idle_driver = {
>> -       .name                   = "exynos4_idle",
>> -       .owner                  = THIS_MODULE,
>> -       .en_core_tk_irqen       = 1,
>> -       .states = {
>> -               [0] = ARM_CPUIDLE_WFI_STATE,
>> -               [1] = {
>> -                       .enter            = exynos4_enter_lowpower,
>> -                       .exit_latency     = 300,
>> -                       .target_residency = 100000,
>> -                       .flags            = CPUIDLE_FLAG_TIME_VALID,
>> -                       .name             = "C1",
>> -                       .desc             = "ARM power down",
>> -               },
>> -       },
>> -       .state_count = 2,
>> -       .safe_state_index = 0,
>> -};
>> -
>>  /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>>  static void exynos4_set_wakeupmask(void)
>>  {
>> @@ -93,9 +72,9 @@ static int idle_finisher(unsigned long flags)
>>         return 1;
>>  }
>>
>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>> -                               struct cpuidle_driver *drv,
>> -                               int index)
>> +static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>> +                                 struct cpuidle_driver *drv,
>> +                                 int index)
>>  {
>>         unsigned long tmp;
>>
>> @@ -143,22 +122,6 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>>         return index;
>>  }
>>
>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>> -                               struct cpuidle_driver *drv,
>> -                               int index)
>> -{
>> -       int new_index = index;
>> -
>> -       /* This mode only can be entered when other core's are offline */
>> -       if (num_online_cpus() > 1)
>> -               new_index = drv->safe_state_index;
>> -
>> -       if (new_index == 0)
>> -               return arm_cpuidle_simple_enter(dev, drv, new_index);
>> -       else
>> -               return exynos4_enter_core0_aftr(dev, drv, new_index);
>> -}
>> -
>>  static void __init exynos5_core_down_clk(void)
>>  {
>>         unsigned int tmp;
>> @@ -191,6 +154,62 @@ static void __init exynos5_core_down_clk(void)
>>         __raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>>  }
>>
>> +static int cpu_hotplug_notify(struct notifier_block *n,
>> +                             unsigned long action, void *hcpu)
>> +{
>> +       int ret;
>> +
>> +       /*
>> +        * cpu0 can't be shutdown on Origen, so this routine is
>> +        * called only for cpu1.
>> +        */
>> +
>> +       switch (action & 0xf) {
>> +
>> +       case CPU_ONLINE:
>> +               cpuidle_pause_and_lock();
>> +               cpuidle_disable_device(&exynos4_cpuidle_device);
>> +               cpuidle_resume_and_unlock();
>> +               break;
>> +
>> +       case CPU_DEAD:
>> +               if (!exynos4_cpuidle_device.registered) {
>> +                       ret = cpuidle_register_device(&exynos4_cpuidle_device);
>> +                       WARN_ONCE(ret, "Failed to register cpuidle device");
>> +               } else {
>> +                       cpuidle_pause_and_lock();
>> +                       cpuidle_enable_device(&exynos4_cpuidle_device);
>> +                       cpuidle_resume_and_unlock();
>> +               }
>> +               break;
>> +       }
>> +
>> +       return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block cpu_hotplug_notifier = {
>> +       .notifier_call = cpu_hotplug_notify,
>> +};
>> +
>> +static struct cpuidle_driver exynos4_idle_driver = {
>> +       .name                   = "exynos4_idle",
>> +       .owner                  = THIS_MODULE,
>> +       .en_core_tk_irqen       = 1,
>> +       .states = {
>> +               [0] = ARM_CPUIDLE_WFI_STATE,
>> +               [1] = {
>> +                       .enter            = exynos4_enter_lowpower,
>> +                       .exit_latency     = 300,
>> +                       .target_residency = 100000,
>> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
>> +                       .name             = "C1",
>> +                       .desc             = "ARM power down",
>> +               },
>> +       },
>> +       .state_count = 2,
>> +       .safe_state_index = 0,
>> +};
>> +
>>  static int __init exynos4_init_cpuidle(void)
>>  {
>>         int ret;
>> @@ -204,12 +223,14 @@ static int __init exynos4_init_cpuidle(void)
>>                 return ret;
>>         }
>>
>> -       ret = cpuidle_register_device(&exynos4_cpuidle_device);
>> -       if (ret) {
>> -               printk(KERN_ERR "CPUidle register device failed\n");
>> -               return ret;
>> -       }
>> +       ret = register_cpu_notifier(&cpu_hotplug_notifier);
>> +       if (ret)
>> +               goto out_unregister_driver;
>> +out:
>> +       return ret;
>>
>> -       return 0;
>> +out_unregister_driver:
>> +       cpuidle_unregister_driver(&exynos4_idle_driver);
>> +       goto out;
>>  }
>>  device_initcall(exynos4_init_cpuidle);
>> --
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> linaro-dev mailing list
>> linaro-dev at lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-dev
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[amit daniel kachhap]

On Thu, Jan 10, 2013 at 1:32 PM, Daniel Lezcano <daniel.lezcano@free.fr> wrote:
> On 01/10/2013 09:07 PM, amit daniel kachhap wrote:
>> Hi Daniel,
>
> Hi Amit Daniel,
>
>> This hotplug noifiers looks fine. I suppose it should add extra state
>> C1 in cpu0. If it is done like below than for normal cases(when all
>> cpu's are online) there wont be any statistics for C0 state
>
> I guess you meant state 0 which is WFI, right ?
> C0 state is the intel semantic for cpu fully turned on.
Yes I meant C0 as wfi
>
>> also which
>> is required. Other patches look good.
>
> Ok, that makes sense to have statistics even if they are only doing WFI.
>
> Then the patch 4/5 is not ok, no ?
yes I suppose patch 4 and patch 5 are related and depends how you
frame patch 5. I think it is better to create C0/C1 sysfs and other
things in the beginning because it is a filesystem call and may
increase the cpu hotplug time which is not worth. May be if cpuidle
framework exposes some API to enable/disable states then it is better.

For patch 1,2 and 3,
Acked-by: Amit Daniel Kachhap <amit.daniel@samsung.com>

Thanks,
Amit Daniel
>
> Thanks for review
>   -- Daniel
>
>>
>> Thanks,
>> Amit
>>
>> On Fri, Jan 4, 2013 at 8:59 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>> What we have now is (1) cpu0 going always to WFI when cpu1 is up,
>>> (2) cpu0 going to all states when cpu1 is down.
>>>
>>> In other words, cpuidle is disabled when cpu1 is up and enabled
>>> when cpu1 is down.
>>>
>>> This patch use the cpu hotplug notifier to enable/disable cpuidle,
>>> when the cpu1 is plugged or unplugged. That clarifies the code and
>>> make it simpler.
>>>
>>> Signed-off: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>  arch/arm/mach-exynos/cpuidle.c |  117 +++++++++++++++++++++++-----------------
>>>  1 file changed, 69 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
>>> index e6f006b..d8f6f33 100644
>>> --- a/arch/arm/mach-exynos/cpuidle.c
>>> +++ b/arch/arm/mach-exynos/cpuidle.c
>>> @@ -12,6 +12,8 @@
>>>  #include <linux/init.h>
>>>  #include <linux/cpuidle.h>
>>>  #include <linux/cpu_pm.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/cpu.h>
>>>  #include <linux/io.h>
>>>  #include <linux/export.h>
>>>  #include <linux/time.h>
>>> @@ -36,31 +38,8 @@
>>>
>>>  #define S5P_CHECK_AFTR         0xFCBA0D10
>>>
>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>> -                               struct cpuidle_driver *drv,
>>> -                               int index);
>>> -
>>>  static struct cpuidle_device exynos4_cpuidle_device;
>>>
>>> -static struct cpuidle_driver exynos4_idle_driver = {
>>> -       .name                   = "exynos4_idle",
>>> -       .owner                  = THIS_MODULE,
>>> -       .en_core_tk_irqen       = 1,
>>> -       .states = {
>>> -               [0] = ARM_CPUIDLE_WFI_STATE,
>>> -               [1] = {
>>> -                       .enter            = exynos4_enter_lowpower,
>>> -                       .exit_latency     = 300,
>>> -                       .target_residency = 100000,
>>> -                       .flags            = CPUIDLE_FLAG_TIME_VALID,
>>> -                       .name             = "C1",
>>> -                       .desc             = "ARM power down",
>>> -               },
>>> -       },
>>> -       .state_count = 2,
>>> -       .safe_state_index = 0,
>>> -};
>>> -
>>>  /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>>>  static void exynos4_set_wakeupmask(void)
>>>  {
>>> @@ -93,9 +72,9 @@ static int idle_finisher(unsigned long flags)
>>>         return 1;
>>>  }
>>>
>>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>>> -                               struct cpuidle_driver *drv,
>>> -                               int index)
>>> +static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>> +                                 struct cpuidle_driver *drv,
>>> +                                 int index)
>>>  {
>>>         unsigned long tmp;
>>>
>>> @@ -143,22 +122,6 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>>>         return index;
>>>  }
>>>
>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>> -                               struct cpuidle_driver *drv,
>>> -                               int index)
>>> -{
>>> -       int new_index = index;
>>> -
>>> -       /* This mode only can be entered when other core's are offline */
>>> -       if (num_online_cpus() > 1)
>>> -               new_index = drv->safe_state_index;
>>> -
>>> -       if (new_index == 0)
>>> -               return arm_cpuidle_simple_enter(dev, drv, new_index);
>>> -       else
>>> -               return exynos4_enter_core0_aftr(dev, drv, new_index);
>>> -}
>>> -
>>>  static void __init exynos5_core_down_clk(void)
>>>  {
>>>         unsigned int tmp;
>>> @@ -191,6 +154,62 @@ static void __init exynos5_core_down_clk(void)
>>>         __raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>>>  }
>>>
>>> +static int cpu_hotplug_notify(struct notifier_block *n,
>>> +                             unsigned long action, void *hcpu)
>>> +{
>>> +       int ret;
>>> +
>>> +       /*
>>> +        * cpu0 can't be shutdown on Origen, so this routine is
>>> +        * called only for cpu1.
>>> +        */
>>> +
>>> +       switch (action & 0xf) {
>>> +
>>> +       case CPU_ONLINE:
>>> +               cpuidle_pause_and_lock();
>>> +               cpuidle_disable_device(&exynos4_cpuidle_device);
>>> +               cpuidle_resume_and_unlock();
>>> +               break;
>>> +
>>> +       case CPU_DEAD:
>>> +               if (!exynos4_cpuidle_device.registered) {
>>> +                       ret = cpuidle_register_device(&exynos4_cpuidle_device);
>>> +                       WARN_ONCE(ret, "Failed to register cpuidle device");
>>> +               } else {
>>> +                       cpuidle_pause_and_lock();
>>> +                       cpuidle_enable_device(&exynos4_cpuidle_device);
>>> +                       cpuidle_resume_and_unlock();
>>> +               }
>>> +               break;
>>> +       }
>>> +
>>> +       return NOTIFY_OK;
>>> +}
>>> +
>>> +static struct notifier_block cpu_hotplug_notifier = {
>>> +       .notifier_call = cpu_hotplug_notify,
>>> +};
>>> +
>>> +static struct cpuidle_driver exynos4_idle_driver = {
>>> +       .name                   = "exynos4_idle",
>>> +       .owner                  = THIS_MODULE,
>>> +       .en_core_tk_irqen       = 1,
>>> +       .states = {
>>> +               [0] = ARM_CPUIDLE_WFI_STATE,
>>> +               [1] = {
>>> +                       .enter            = exynos4_enter_lowpower,
>>> +                       .exit_latency     = 300,
>>> +                       .target_residency = 100000,
>>> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
>>> +                       .name             = "C1",
>>> +                       .desc             = "ARM power down",
>>> +               },
>>> +       },
>>> +       .state_count = 2,
>>> +       .safe_state_index = 0,
>>> +};
>>> +
>>>  static int __init exynos4_init_cpuidle(void)
>>>  {
>>>         int ret;
>>> @@ -204,12 +223,14 @@ static int __init exynos4_init_cpuidle(void)
>>>                 return ret;
>>>         }
>>>
>>> -       ret = cpuidle_register_device(&exynos4_cpuidle_device);
>>> -       if (ret) {
>>> -               printk(KERN_ERR "CPUidle register device failed\n");
>>> -               return ret;
>>> -       }
>>> +       ret = register_cpu_notifier(&cpu_hotplug_notifier);
>>> +       if (ret)
>>> +               goto out_unregister_driver;
>>> +out:
>>> +       return ret;
>>>
>>> -       return 0;
>>> +out_unregister_driver:
>>> +       cpuidle_unregister_driver(&exynos4_idle_driver);
>>> +       goto out;
>>>  }
>>>  device_initcall(exynos4_init_cpuidle);
>>> --
>>> 1.7.9.5
>>>
>>>
>>> _______________________________________________
>>> linaro-dev mailing list
>>> linaro-dev at lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/linaro-dev
>> _______________________________________________
>> linaro-dev mailing list
>> linaro-dev at lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-dev
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[Daniel Lezcano]

On 01/10/2013 11:33 PM, amit daniel kachhap wrote:
> On Thu, Jan 10, 2013 at 1:32 PM, Daniel Lezcano <daniel.lezcano@free.fr> wrote:
>> On 01/10/2013 09:07 PM, amit daniel kachhap wrote:
>>> Hi Daniel,
>>
>> Hi Amit Daniel,
>>
>>> This hotplug noifiers looks fine. I suppose it should add extra state
>>> C1 in cpu0. If it is done like below than for normal cases(when all
>>> cpu's are online) there wont be any statistics for C0 state
>>
>> I guess you meant state 0 which is WFI, right ?
>> C0 state is the intel semantic for cpu fully turned on.
> Yes I meant C0 as wfi
>>
>>> also which
>>> is required. Other patches look good.
>>
>> Ok, that makes sense to have statistics even if they are only doing WFI.
>>
>> Then the patch 4/5 is not ok, no ?
> yes I suppose patch 4 and patch 5 are related and depends how you
> frame patch 5. I think it is better to create C0/C1 sysfs and other
> things in the beginning because it is a filesystem call and may
> increase the cpu hotplug time which is not worth. May be if cpuidle
> framework exposes some API to enable/disable states then it is better.
> 
> For patch 1,2 and 3,
> Acked-by: Amit Daniel Kachhap <amit.daniel@samsung.com>

Hi Kukjin,

is it possible to take these patches [1-3/5] ?

The patches [3-4/5] could be ignored.

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

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[Kukjin Kim]

Daniel Lezcano wrote:
> 
> On 01/10/2013 11:33 PM, amit daniel kachhap wrote:
> > On Thu, Jan 10, 2013 at 1:32 PM, Daniel Lezcano <daniel.lezcano@free.fr>
> wrote:
> >> On 01/10/2013 09:07 PM, amit daniel kachhap wrote:
> >>> Hi Daniel,
> >>
> >> Hi Amit Daniel,
> >>
> >>> This hotplug noifiers looks fine. I suppose it should add extra state
> >>> C1 in cpu0. If it is done like below than for normal cases(when all
> >>> cpu's are online) there wont be any statistics for C0 state
> >>
> >> I guess you meant state 0 which is WFI, right ?
> >> C0 state is the intel semantic for cpu fully turned on.
> > Yes I meant C0 as wfi
> >>
> >>> also which
> >>> is required. Other patches look good.
> >>
> >> Ok, that makes sense to have statistics even if they are only doing WFI.
> >>
> >> Then the patch 4/5 is not ok, no ?
> > yes I suppose patch 4 and patch 5 are related and depends how you
> > frame patch 5. I think it is better to create C0/C1 sysfs and other
> > things in the beginning because it is a filesystem call and may
> > increase the cpu hotplug time which is not worth. May be if cpuidle
> > framework exposes some API to enable/disable states then it is better.
> >
> > For patch 1,2 and 3,
> > Acked-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> 
> Hi Kukjin,
> 
> is it possible to take these patches [1-3/5] ?
> 
Looks OK to me, I will apply with Amit's ack.

> The patches [3-4/5] could be ignored.
> 
Probably, you mean [4-5/5] :-)

Thanks.

- Kukjin

[PATCH 5/5] ARM: exynos: enable/disable cpuidle when cpu1 is down/up

[Daniel Lezcano]

On 01/18/2013 04:51 AM, Kukjin Kim wrote:
> Daniel Lezcano wrote:
>> On 01/10/2013 11:33 PM, amit daniel kachhap wrote:
>>> On Thu, Jan 10, 2013 at 1:32 PM, Daniel Lezcano <daniel.lezcano@free.fr>
>> wrote:
>>>> On 01/10/2013 09:07 PM, amit daniel kachhap wrote:
>>>>> Hi Daniel,
>>>> Hi Amit Daniel,
>>>>
>>>>> This hotplug noifiers looks fine. I suppose it should add extra state
>>>>> C1 in cpu0. If it is done like below than for normal cases(when all
>>>>> cpu's are online) there wont be any statistics for C0 state
>>>> I guess you meant state 0 which is WFI, right ?
>>>> C0 state is the intel semantic for cpu fully turned on.
>>> Yes I meant C0 as wfi
>>>>> also which
>>>>> is required. Other patches look good.
>>>> Ok, that makes sense to have statistics even if they are only doing WFI.
>>>>
>>>> Then the patch 4/5 is not ok, no ?
>>> yes I suppose patch 4 and patch 5 are related and depends how you
>>> frame patch 5. I think it is better to create C0/C1 sysfs and other
>>> things in the beginning because it is a filesystem call and may
>>> increase the cpu hotplug time which is not worth. May be if cpuidle
>>> framework exposes some API to enable/disable states then it is better.
>>>
>>> For patch 1,2 and 3,
>>> Acked-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> Hi Kukjin,
>>
>> is it possible to take these patches [1-3/5] ?
>>
> Looks OK to me, I will apply with Amit's ack.

Cool Thanks !

>> The patches [3-4/5] could be ignored.
>>
> Probably, you mean [4-5/5] :-)

Oh, yes. Right ! :)

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