[PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Daniel Lezcano]

When there are several cpus online, the AFTR state does not work.

The AFTR must be selected only when there is one cpu online.

The previous code was already handling this case.

Simplified the code, especially the init routine to have the same init
pattern than the other drivers.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 17a18ff..cc4b097 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -147,16 +147,11 @@ 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;
+		return drv->states[0].enter(dev, drv, 0);
 
-	if (new_index == 0)
-		return arm_cpuidle_simple_enter(dev, drv, new_index);
-	else
-		return exynos4_enter_core0_aftr(dev, drv, new_index);
+	return exynos4_enter_core0_aftr(dev, drv, index);
 }
 
 static void __init exynos5_core_down_clk(void)
@@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
 		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");
-- 
1.7.9.5

[PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Daniel Lezcano]

When there are several cpus online, the AFTR state does not work.

The AFTR must be selected only when there is one cpu online.

The previous code was already handling this case.

Simplified the code, especially the init routine to have the same init
pattern than the other drivers.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 17a18ff..cc4b097 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -147,16 +147,11 @@ 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;
+		return drv->states[0].enter(dev, drv, 0);
 
-	if (new_index == 0)
-		return arm_cpuidle_simple_enter(dev, drv, new_index);
-	else
-		return exynos4_enter_core0_aftr(dev, drv, new_index);
+	return exynos4_enter_core0_aftr(dev, drv, index);
 }
 
 static void __init exynos5_core_down_clk(void)
@@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
 		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");
-- 
1.7.9.5

[PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Daniel Lezcano]

Now we have the same routine than the one handled by the cpuidle framework.

Let's use it.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index cc4b097..d8fc1a2 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
 	.name			= "exynos4_idle",
 	.owner			= THIS_MODULE,
@@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
 
 static int __init exynos4_init_cpuidle(void)
 {
-	int cpu_id, ret;
-	struct cpuidle_device *device;
-
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
 
-	ret = cpuidle_register_driver(&exynos4_idle_driver);
-	if (ret) {
-		printk(KERN_ERR "CPUidle failed to register driver\n");
-		return ret;
-	}
-
-	for_each_online_cpu(cpu_id) {
-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-		ret = cpuidle_register_device(device);
-		if (ret) {
-			printk(KERN_ERR "CPUidle register device failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
+	return cpuidle_register(&exynos4_idle_driver, NULL);
 }
 device_initcall(exynos4_init_cpuidle);
-- 
1.7.9.5

[PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Daniel Lezcano]

Now we have the same routine than the one handled by the cpuidle framework.

Let's use it.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index cc4b097..d8fc1a2 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
 	.name			= "exynos4_idle",
 	.owner			= THIS_MODULE,
@@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
 
 static int __init exynos4_init_cpuidle(void)
 {
-	int cpu_id, ret;
-	struct cpuidle_device *device;
-
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
 
-	ret = cpuidle_register_driver(&exynos4_idle_driver);
-	if (ret) {
-		printk(KERN_ERR "CPUidle failed to register driver\n");
-		return ret;
-	}
-
-	for_each_online_cpu(cpu_id) {
-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-		ret = cpuidle_register_device(device);
-		if (ret) {
-			printk(KERN_ERR "CPUidle register device failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
+	return cpuidle_register(&exynos4_idle_driver, NULL);
 }
 device_initcall(exynos4_init_cpuidle);
-- 
1.7.9.5

[PATCH 3/4] ARM: exynos: cpuidle: Move exynos4_idle_driver declaration below in the code

[Daniel Lezcano]

In order to prevent a pointless forward declaration, move the driver declation
after the idle function callback, right before the init function.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index d8fc1a2..8d06128 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -37,28 +37,6 @@
 
 #define S5P_CHECK_AFTR		0xFCBA0D10
 
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index);
-
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name			= "exynos4_idle",
-	.owner			= THIS_MODULE,
-	.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)
 {
@@ -184,6 +162,24 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
+static struct cpuidle_driver exynos4_idle_driver = {
+	.name			= "exynos4_idle",
+	.owner			= THIS_MODULE,
+	.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)
 {
 	if (soc_is_exynos5250())
-- 
1.7.9.5

[PATCH 3/4] ARM: exynos: cpuidle: Move exynos4_idle_driver declaration below in the code

[Daniel Lezcano]

In order to prevent a pointless forward declaration, move the driver declation
after the idle function callback, right before the init function.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index d8fc1a2..8d06128 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -37,28 +37,6 @@
 
 #define S5P_CHECK_AFTR		0xFCBA0D10
 
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index);
-
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name			= "exynos4_idle",
-	.owner			= THIS_MODULE,
-	.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)
 {
@@ -184,6 +162,24 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
+static struct cpuidle_driver exynos4_idle_driver = {
+	.name			= "exynos4_idle",
+	.owner			= THIS_MODULE,
+	.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)
 {
 	if (soc_is_exynos5250())
-- 
1.7.9.5

[PATCH 4/4] ARM: exynos: cpuidle: Remove useless headers

[Daniel Lezcano]

The headers:

#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/time.h>
#include <asm/unified.h>

are not needed. Removed them.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 8d06128..027f5e7 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -8,18 +8,14 @@
  * published by the Free Software Foundation.
 */
 
-#include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/cpuidle.h>
 #include <linux/cpu_pm.h>
 #include <linux/io.h>
-#include <linux/export.h>
-#include <linux/time.h>
 
 #include <asm/proc-fns.h>
 #include <asm/smp_scu.h>
 #include <asm/suspend.h>
-#include <asm/unified.h>
 #include <asm/cpuidle.h>
 #include <mach/regs-clock.h>
 #include <mach/regs-pmu.h>
-- 
1.7.9.5

[PATCH 4/4] ARM: exynos: cpuidle: Remove useless headers

[Daniel Lezcano]

The headers:

#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/time.h>
#include <asm/unified.h>

are not needed. Removed them.

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

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 8d06128..027f5e7 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -8,18 +8,14 @@
  * published by the Free Software Foundation.
 */
 
-#include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/cpuidle.h>
 #include <linux/cpu_pm.h>
 #include <linux/io.h>
-#include <linux/export.h>
-#include <linux/time.h>
 
 #include <asm/proc-fns.h>
 #include <asm/smp_scu.h>
 #include <asm/suspend.h>
-#include <asm/unified.h>
 #include <asm/cpuidle.h>
 #include <mach/regs-clock.h>
 #include <mach/regs-pmu.h>
-- 
1.7.9.5

Re: [PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
> Now we have the same routine than the one handled by the cpuidle framework.

Small nitpick:

To be exact the routine is not the same, on error cpuidle_register() does
complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
just stops the registration procedure. 

The change itself is okay but the patch description could be better.

Anyway all patches (#1-4) look fine to me. Thanks for this work.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
 
> Let's use it.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index cc4b097..d8fc1a2 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>  	.name			= "exynos4_idle",
>  	.owner			= THIS_MODULE,
> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
>  
>  static int __init exynos4_init_cpuidle(void)
>  {
> -	int cpu_id, ret;
> -	struct cpuidle_device *device;
> -
>  	if (soc_is_exynos5250())
>  		exynos5_core_down_clk();
>  
> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> -	if (ret) {
> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> -		return ret;
> -	}
> -
> -	for_each_online_cpu(cpu_id) {
> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> -		device->cpu = cpu_id;
> -
> -		ret = cpuidle_register_device(device);
> -		if (ret) {
> -			printk(KERN_ERR "CPUidle register device failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>  }
>  device_initcall(exynos4_init_cpuidle);

[PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
> Now we have the same routine than the one handled by the cpuidle framework.

Small nitpick:

To be exact the routine is not the same, on error cpuidle_register() does
complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
just stops the registration procedure. 

The change itself is okay but the patch description could be better.

Anyway all patches (#1-4) look fine to me. Thanks for this work.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
 
> Let's use it.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index cc4b097..d8fc1a2 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>  	.name			= "exynos4_idle",
>  	.owner			= THIS_MODULE,
> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
>  
>  static int __init exynos4_init_cpuidle(void)
>  {
> -	int cpu_id, ret;
> -	struct cpuidle_device *device;
> -
>  	if (soc_is_exynos5250())
>  		exynos5_core_down_clk();
>  
> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> -	if (ret) {
> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> -		return ret;
> -	}
> -
> -	for_each_online_cpu(cpu_id) {
> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> -		device->cpu = cpu_id;
> -
> -		ret = cpuidle_register_device(device);
> -		if (ret) {
> -			printk(KERN_ERR "CPUidle register device failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>  }
>  device_initcall(exynos4_init_cpuidle);

Re: [PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Daniel Lezcano]

On 07/19/2013 06:03 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
>> Now we have the same routine than the one handled by the cpuidle framework.
> 
> Small nitpick:
> 
> To be exact the routine is not the same, on error cpuidle_register() does
> complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
> just stops the registration procedure. 
> 
> The change itself is okay but the patch description could be better.

Ok, I will fix the changelog.

> Anyway all patches (#1-4) look fine to me. Thanks for this work.

Thanks for the review. I assume it is an acked-by right ?

> Best regards,

Kukjin,

shall I take the patchset in my tree ?

Thanks
  -- Daniel

> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>  
>> Let's use it.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>>  1 file changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
>> index cc4b097..d8fc1a2 100644
>> --- a/arch/arm/mach-exynos/cpuidle.c
>> +++ b/arch/arm/mach-exynos/cpuidle.c
>> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>>  	.name			= "exynos4_idle",
>>  	.owner			= THIS_MODULE,
>> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
>>  
>>  static int __init exynos4_init_cpuidle(void)
>>  {
>> -	int cpu_id, ret;
>> -	struct cpuidle_device *device;
>> -
>>  	if (soc_is_exynos5250())
>>  		exynos5_core_down_clk();
>>  
>> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
>> -	if (ret) {
>> -		printk(KERN_ERR "CPUidle failed to register driver\n");
>> -		return ret;
>> -	}
>> -
>> -	for_each_online_cpu(cpu_id) {
>> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>> -		device->cpu = cpu_id;
>> -
>> -		ret = cpuidle_register_device(device);
>> -		if (ret) {
>> -			printk(KERN_ERR "CPUidle register device failed\n");
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	return 0;
>> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>>  }
>>  device_initcall(exynos4_init_cpuidle);
> 


-- 
 <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 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Daniel Lezcano]

On 07/19/2013 06:03 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
>> Now we have the same routine than the one handled by the cpuidle framework.
> 
> Small nitpick:
> 
> To be exact the routine is not the same, on error cpuidle_register() does
> complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
> just stops the registration procedure. 
> 
> The change itself is okay but the patch description could be better.

Ok, I will fix the changelog.

> Anyway all patches (#1-4) look fine to me. Thanks for this work.

Thanks for the review. I assume it is an acked-by right ?

> Best regards,

Kukjin,

shall I take the patchset in my tree ?

Thanks
  -- Daniel

> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>  
>> Let's use it.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>>  1 file changed, 1 insertion(+), 23 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
>> index cc4b097..d8fc1a2 100644
>> --- a/arch/arm/mach-exynos/cpuidle.c
>> +++ b/arch/arm/mach-exynos/cpuidle.c
>> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>>  	.name			= "exynos4_idle",
>>  	.owner			= THIS_MODULE,
>> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
>>  
>>  static int __init exynos4_init_cpuidle(void)
>>  {
>> -	int cpu_id, ret;
>> -	struct cpuidle_device *device;
>> -
>>  	if (soc_is_exynos5250())
>>  		exynos5_core_down_clk();
>>  
>> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
>> -	if (ret) {
>> -		printk(KERN_ERR "CPUidle failed to register driver\n");
>> -		return ret;
>> -	}
>> -
>> -	for_each_online_cpu(cpu_id) {
>> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>> -		device->cpu = cpu_id;
>> -
>> -		ret = cpuidle_register_device(device);
>> -		if (ret) {
>> -			printk(KERN_ERR "CPUidle register device failed\n");
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	return 0;
>> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>>  }
>>  device_initcall(exynos4_init_cpuidle);
> 


-- 
 <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: Re: [PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Bartlomiej Zolnierkiewicz]

On Monday, July 22, 2013 06:36:46 AM Daniel Lezcano wrote:
> On 07/19/2013 06:03 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
> >> Now we have the same routine than the one handled by the cpuidle framework.
> > 
> > Small nitpick:
> > 
> > To be exact the routine is not the same, on error cpuidle_register() does
> > complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
> > just stops the registration procedure. 
> > 
> > The change itself is okay but the patch description could be better.
> 
> Ok, I will fix the changelog.
> 
> > Anyway all patches (#1-4) look fine to me. Thanks for this work.
> 
> Thanks for the review. I assume it is an acked-by right ?

Yes.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> > Best regards,
> 
> Kukjin,
> 
> shall I take the patchset in my tree ?
> 
> Thanks
>   -- Daniel
> 
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >  
> >> Let's use it.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
> >>  1 file changed, 1 insertion(+), 23 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> >> index cc4b097..d8fc1a2 100644
> >> --- a/arch/arm/mach-exynos/cpuidle.c
> >> +++ b/arch/arm/mach-exynos/cpuidle.c
> >> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
> >>  	.name			= "exynos4_idle",
> >>  	.owner			= THIS_MODULE,
> >> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
> >>  
> >>  static int __init exynos4_init_cpuidle(void)
> >>  {
> >> -	int cpu_id, ret;
> >> -	struct cpuidle_device *device;
> >> -
> >>  	if (soc_is_exynos5250())
> >>  		exynos5_core_down_clk();
> >>  
> >> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> >> -	if (ret) {
> >> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> >> -		return ret;
> >> -	}
> >> -
> >> -	for_each_online_cpu(cpu_id) {
> >> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >> -		device->cpu = cpu_id;
> >> -
> >> -		ret = cpuidle_register_device(device);
> >> -		if (ret) {
> >> -			printk(KERN_ERR "CPUidle register device failed\n");
> >> -			return ret;
> >> -		}
> >> -	}
> >> -
> >> -	return 0;
> >> +	return cpuidle_register(&exynos4_idle_driver, NULL);
> >>  }
> >>  device_initcall(exynos4_init_cpuidle);

[PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Bartlomiej Zolnierkiewicz]

On Monday, July 22, 2013 06:36:46 AM Daniel Lezcano wrote:
> On 07/19/2013 06:03 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Thursday, July 18, 2013 02:54:28 PM Daniel Lezcano wrote:
> >> Now we have the same routine than the one handled by the cpuidle framework.
> > 
> > Small nitpick:
> > 
> > To be exact the routine is not the same, on error cpuidle_register() does
> > complete unregister operation of the cpuidle driver, exynos4_init_cpuidle()
> > just stops the registration procedure. 
> > 
> > The change itself is okay but the patch description could be better.
> 
> Ok, I will fix the changelog.
> 
> > Anyway all patches (#1-4) look fine to me. Thanks for this work.
> 
> Thanks for the review. I assume it is an acked-by right ?

Yes.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> > Best regards,
> 
> Kukjin,
> 
> shall I take the patchset in my tree ?
> 
> Thanks
>   -- Daniel
> 
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >  
> >> Let's use it.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
> >>  1 file changed, 1 insertion(+), 23 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> >> index cc4b097..d8fc1a2 100644
> >> --- a/arch/arm/mach-exynos/cpuidle.c
> >> +++ b/arch/arm/mach-exynos/cpuidle.c
> >> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
> >>  	.name			= "exynos4_idle",
> >>  	.owner			= THIS_MODULE,
> >> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
> >>  
> >>  static int __init exynos4_init_cpuidle(void)
> >>  {
> >> -	int cpu_id, ret;
> >> -	struct cpuidle_device *device;
> >> -
> >>  	if (soc_is_exynos5250())
> >>  		exynos5_core_down_clk();
> >>  
> >> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> >> -	if (ret) {
> >> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> >> -		return ret;
> >> -	}
> >> -
> >> -	for_each_online_cpu(cpu_id) {
> >> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >> -		device->cpu = cpu_id;
> >> -
> >> -		ret = cpuidle_register_device(device);
> >> -		if (ret) {
> >> -			printk(KERN_ERR "CPUidle register device failed\n");
> >> -			return ret;
> >> -		}
> >> -	}
> >> -
> >> -	return 0;
> >> +	return cpuidle_register(&exynos4_idle_driver, NULL);
> >>  }
> >>  device_initcall(exynos4_init_cpuidle);

Re: [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Tomasz Figa]

Hi Daniel,

On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> When there are several cpus online, the AFTR state does not work.
> 
> The AFTR must be selected only when there is one cpu online.
> 
> The previous code was already handling this case.
> 
> Simplified the code, especially the init routine to have the same init
> pattern than the other drivers.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -147,16 +147,11 @@ 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;
> +		return drv->states[0].enter(dev, drv, 0);
> 
> -	if (new_index == 0)
> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> -	else
> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> +	return exynos4_enter_core0_aftr(dev, drv, index);
>  }
> 
>  static void __init exynos5_core_down_clk(void)
> @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>  		device->cpu = cpu_id;
> 
> -		/* Support IDLE only */
> -		if (cpu_id != 0)
> -			device->state_count = 1;
> -

Are you sure that this is okay? It means, assuming that you have CPU0 
hotplug enabled, that you can enter the AFTR state if _any_ single core 
remains plugged in, which is technically possible, but then wake-up will 
occur on CPU0, which is not what cpuidle and hotplug code expect.

P.S. Please keep linux-samsung-soc@vger.kernel.org mailing list on Cc for 
patches related to Samsung SoCs.

Best regards,
Tomasz

[PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Tomasz Figa]

Hi Daniel,

On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> When there are several cpus online, the AFTR state does not work.
> 
> The AFTR must be selected only when there is one cpu online.
> 
> The previous code was already handling this case.
> 
> Simplified the code, especially the init routine to have the same init
> pattern than the other drivers.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -147,16 +147,11 @@ 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;
> +		return drv->states[0].enter(dev, drv, 0);
> 
> -	if (new_index == 0)
> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> -	else
> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> +	return exynos4_enter_core0_aftr(dev, drv, index);
>  }
> 
>  static void __init exynos5_core_down_clk(void)
> @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>  		device->cpu = cpu_id;
> 
> -		/* Support IDLE only */
> -		if (cpu_id != 0)
> -			device->state_count = 1;
> -

Are you sure that this is okay? It means, assuming that you have CPU0 
hotplug enabled, that you can enter the AFTR state if _any_ single core 
remains plugged in, which is technically possible, but then wake-up will 
occur on CPU0, which is not what cpuidle and hotplug code expect.

P.S. Please keep linux-samsung-soc at vger.kernel.org mailing list on Cc for 
patches related to Samsung SoCs.

Best regards,
Tomasz

Re: [PATCH 3/4] ARM: exynos: cpuidle: Move exynos4_idle_driver declaration below in the code

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:29 Daniel Lezcano wrote:
> In order to prevent a pointless forward declaration, move the driver
> declation after the idle function callback, right before the init
> function.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   40
> ++++++++++++++++++---------------------- 1 file changed, 18
> insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index d8fc1a2..8d06128 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -37,28 +37,6 @@
> 
>  #define S5P_CHECK_AFTR		0xFCBA0D10
> 
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -				struct cpuidle_driver *drv,
> -				int index);
> -
> -static struct cpuidle_driver exynos4_idle_driver = {
> -	.name			= "exynos4_idle",
> -	.owner			= THIS_MODULE,
> -	.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)
>  {
> @@ -184,6 +162,24 @@ static void __init exynos5_core_down_clk(void)
>  	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>  }
> 
> +static struct cpuidle_driver exynos4_idle_driver = {
> +	.name			= "exynos4_idle",
> +	.owner			= THIS_MODULE,
> +	.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)
>  {
>  	if (soc_is_exynos5250())

Makes sense.

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

[PATCH 3/4] ARM: exynos: cpuidle: Move exynos4_idle_driver declaration below in the code

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:29 Daniel Lezcano wrote:
> In order to prevent a pointless forward declaration, move the driver
> declation after the idle function callback, right before the init
> function.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   40
> ++++++++++++++++++---------------------- 1 file changed, 18
> insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index d8fc1a2..8d06128 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -37,28 +37,6 @@
> 
>  #define S5P_CHECK_AFTR		0xFCBA0D10
> 
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -				struct cpuidle_driver *drv,
> -				int index);
> -
> -static struct cpuidle_driver exynos4_idle_driver = {
> -	.name			= "exynos4_idle",
> -	.owner			= THIS_MODULE,
> -	.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)
>  {
> @@ -184,6 +162,24 @@ static void __init exynos5_core_down_clk(void)
>  	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>  }
> 
> +static struct cpuidle_driver exynos4_idle_driver = {
> +	.name			= "exynos4_idle",
> +	.owner			= THIS_MODULE,
> +	.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)
>  {
>  	if (soc_is_exynos5250())

Makes sense.

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

Re: [PATCH 4/4] ARM: exynos: cpuidle: Remove useless headers

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:30 Daniel Lezcano wrote:
> The headers:
> 
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/time.h>
> #include <asm/unified.h>
> 
> are not needed. Removed them.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 8d06128..027f5e7 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -8,18 +8,14 @@
>   * published by the Free Software Foundation.
>  */
> 
> -#include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/io.h>
> -#include <linux/export.h>
> -#include <linux/time.h>
> 
>  #include <asm/proc-fns.h>
>  #include <asm/smp_scu.h>
>  #include <asm/suspend.h>
> -#include <asm/unified.h>
>  #include <asm/cpuidle.h>
>  #include <mach/regs-clock.h>
>  #include <mach/regs-pmu.h>

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

[PATCH 4/4] ARM: exynos: cpuidle: Remove useless headers

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:30 Daniel Lezcano wrote:
> The headers:
> 
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/time.h>
> #include <asm/unified.h>
> 
> are not needed. Removed them.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 8d06128..027f5e7 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -8,18 +8,14 @@
>   * published by the Free Software Foundation.
>  */
> 
> -#include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/cpuidle.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/io.h>
> -#include <linux/export.h>
> -#include <linux/time.h>
> 
>  #include <asm/proc-fns.h>
>  #include <asm/smp_scu.h>
>  #include <asm/suspend.h>
> -#include <asm/unified.h>
>  #include <asm/cpuidle.h>
>  #include <mach/regs-clock.h>
>  #include <mach/regs-pmu.h>

Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

Re: [PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:28 Daniel Lezcano wrote:
> Now we have the same routine than the one handled by the cpuidle
> framework.
> 
> Let's use it.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index cc4b097..d8fc1a2 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>  	.name			= "exynos4_idle",
>  	.owner			= THIS_MODULE,
> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
> 
>  static int __init exynos4_init_cpuidle(void)
>  {
> -	int cpu_id, ret;
> -	struct cpuidle_device *device;
> -
>  	if (soc_is_exynos5250())
>  		exynos5_core_down_clk();
> 
> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> -	if (ret) {
> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> -		return ret;
> -	}
> -
> -	for_each_online_cpu(cpu_id) {
> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> -		device->cpu = cpu_id;
> -
> -		ret = cpuidle_register_device(device);
> -		if (ret) {
> -			printk(KERN_ERR "CPUidle register device 
failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>  }
>  device_initcall(exynos4_init_cpuidle);

This is nice, but I would like to see clarification for my question posted 
to patch 1/4.

Best regards,
Tomasz

[PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine

[Tomasz Figa]

On Thursday 18 of July 2013 14:54:28 Daniel Lezcano wrote:
> Now we have the same routine than the one handled by the cpuidle
> framework.
> 
> Let's use it.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index cc4b097..d8fc1a2 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -41,8 +41,6 @@ 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_driver exynos4_idle_driver = {
>  	.name			= "exynos4_idle",
>  	.owner			= THIS_MODULE,
> @@ -188,29 +186,9 @@ static void __init exynos5_core_down_clk(void)
> 
>  static int __init exynos4_init_cpuidle(void)
>  {
> -	int cpu_id, ret;
> -	struct cpuidle_device *device;
> -
>  	if (soc_is_exynos5250())
>  		exynos5_core_down_clk();
> 
> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> -	if (ret) {
> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> -		return ret;
> -	}
> -
> -	for_each_online_cpu(cpu_id) {
> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> -		device->cpu = cpu_id;
> -
> -		ret = cpuidle_register_device(device);
> -		if (ret) {
> -			printk(KERN_ERR "CPUidle register device 
failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return cpuidle_register(&exynos4_idle_driver, NULL);
>  }
>  device_initcall(exynos4_init_cpuidle);

This is nice, but I would like to see clarification for my question posted 
to patch 1/4.

Best regards,
Tomasz

Re: [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Bartlomiej Zolnierkiewicz]

On Wednesday, July 24, 2013 10:32:47 AM Tomasz Figa wrote:
> Hi Daniel,
> 
> On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> > When there are several cpus online, the AFTR state does not work.
> > 
> > The AFTR must be selected only when there is one cpu online.
> > 
> > The previous code was already handling this case.
> > 
> > Simplified the code, especially the init routine to have the same init
> > pattern than the other drivers.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/cpuidle.c
> > b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> > --- a/arch/arm/mach-exynos/cpuidle.c
> > +++ b/arch/arm/mach-exynos/cpuidle.c
> > @@ -147,16 +147,11 @@ 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;
> > +		return drv->states[0].enter(dev, drv, 0);
> > 
> > -	if (new_index == 0)
> > -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> > -	else
> > -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> > +	return exynos4_enter_core0_aftr(dev, drv, index);
> >  }
> > 
> >  static void __init exynos5_core_down_clk(void)
> > @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
> >  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >  		device->cpu = cpu_id;
> > 
> > -		/* Support IDLE only */
> > -		if (cpu_id != 0)
> > -			device->state_count = 1;
> > -
> 
> Are you sure that this is okay? It means, assuming that you have CPU0 
> hotplug enabled, that you can enter the AFTR state if _any_ single core 
> remains plugged in, which is technically possible, but then wake-up will 
> occur on CPU0, which is not what cpuidle and hotplug code expect.

I worry that the current code is already broken in this respect as cpuidle
core is using driver->state_count for everything except creating/destroying
sysfs state entries. This is probably something that needs to be fixed in
the cpuidle core (I suspect that governors should use device->state_count
instead of driver->state_count but it would need confirmation from someone
that knows this code better) but in the meantime it could be fixed by adding
CPU0 check to exynos4_enter_lowpower() (if the last CPU != 0 then don't go
into AFTR mode).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> P.S. Please keep linux-samsung-soc@vger.kernel.org mailing list on Cc for 
> patches related to Samsung SoCs.
> 
> Best regards,
> Tomasz

[PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected

[Bartlomiej Zolnierkiewicz]

On Wednesday, July 24, 2013 10:32:47 AM Tomasz Figa wrote:
> Hi Daniel,
> 
> On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> > When there are several cpus online, the AFTR state does not work.
> > 
> > The AFTR must be selected only when there is one cpu online.
> > 
> > The previous code was already handling this case.
> > 
> > Simplified the code, especially the init routine to have the same init
> > pattern than the other drivers.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/cpuidle.c
> > b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> > --- a/arch/arm/mach-exynos/cpuidle.c
> > +++ b/arch/arm/mach-exynos/cpuidle.c
> > @@ -147,16 +147,11 @@ 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;
> > +		return drv->states[0].enter(dev, drv, 0);
> > 
> > -	if (new_index == 0)
> > -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> > -	else
> > -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> > +	return exynos4_enter_core0_aftr(dev, drv, index);
> >  }
> > 
> >  static void __init exynos5_core_down_clk(void)
> > @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
> >  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >  		device->cpu = cpu_id;
> > 
> > -		/* Support IDLE only */
> > -		if (cpu_id != 0)
> > -			device->state_count = 1;
> > -
> 
> Are you sure that this is okay? It means, assuming that you have CPU0 
> hotplug enabled, that you can enter the AFTR state if _any_ single core 
> remains plugged in, which is technically possible, but then wake-up will 
> occur on CPU0, which is not what cpuidle and hotplug code expect.

I worry that the current code is already broken in this respect as cpuidle
core is using driver->state_count for everything except creating/destroying
sysfs state entries. This is probably something that needs to be fixed in
the cpuidle core (I suspect that governors should use device->state_count
instead of driver->state_count but it would need confirmation from someone
that knows this code better) but in the meantime it could be fixed by adding
CPU0 check to exynos4_enter_lowpower() (if the last CPU != 0 then don't go
into AFTR mode).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> P.S. Please keep linux-samsung-soc at vger.kernel.org mailing list on Cc for 
> patches related to Samsung SoCs.
> 
> Best regards,
> Tomasz