ARM: exynos: cpuidle: Move the driver to drivers/cpuidle

PATCH Superseded Daniel Lezcano 4 participants 46 messages Apr 4, 2014 – Apr 8, 2014

Subsystems (5)

Arm (32-bit) Arm sub-architectures Arm Samsung Exynos CPU idle time management framework +1 more subsystems

lore

Share & tools

lore

Daniel Lezcano

author

2014-04-04 13:42 UTC #0 lore raw

v2 cover ARM: exynos: cpuidle: Move the driver to drivers/cpuidle

Changelog:

V2:
* Added comment in changelog for calls order (5/17)
* Call the powerdown only for cpu0 in the pm notifier
* Set the pm notifier for all boards

V1: initial post

This patchset relies on the cpm_pm notifier to initiate the powerdown sequence operations from pm.c instead cpuidle.c. Thus the cpuidle driver is no longer dependent from arch specific code as everything is called from the pm.c file.

The patchset applies on top of linux-samsung/for-next.

Tested on exynos4: 4210
Tested on exynos5: 5250 (without AFTR)

Daniel Lezcano (17):
ARM: exynos: cpuidle: Prevent forward declaration
ARM: exynos: cpuidle: use cpuidle_register
ARM: exynos: cpuidle: change function name prefix
ARM: exynos: cpuidle: encapsulate register access inside a function
ARM: exynos: cpuidle: Move some code inside the idle_finisher
ARM: exynos: cpuidle: Fix S5P_WAKEUP_STAT call
ARM: exynos: cpuidle: Use the cpu_pm notifier
ARM: exynos: cpuidle: Move scu_enable in the cpu_pm notifier
ARM: exynos: cpuidle: Remove ifdef for scu_enable
ARM: exynos: cpuidle: Move exynos_set_wakeupmask in the cpu_pm
notifier
ARM: exynos: cpuidle: Move the power sequence call in the cpu_pm
notifier
ARM: exynos: cpuidle: Move S5P_CHECK_AFTR in a header
ARM: exynos: cpuidle: Move clock setup to pm.c
ARM: exynos: cpuidle: Move the boot vector in pm.c
ARM: exynos: cpuidle: Pass the AFTR callback to the platform_data
ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory
ARM: exynos: config: Enable cpuidle

 arch/arm/configs/exynos_defconfig |    1 +
 arch/arm/mach-exynos/Makefile     |    1 -
 arch/arm/mach-exynos/common.c     |    5 +-
 arch/arm/mach-exynos/common.h     |    1 +
 arch/arm/mach-exynos/cpuidle.c    |  256 -------------------------------------
 arch/arm/mach-exynos/pm.c         |  191 ++++++++++++++++++++++-----
 arch/arm/mach-exynos/pmu.c        |    6 +
 arch/arm/mach-exynos/regs-pmu.h   |    1 +
 drivers/cpuidle/Kconfig.arm       |    7 +
 drivers/cpuidle/Makefile          |    1 +
 drivers/cpuidle/cpuidle-exynos.c  |  102 +++++++++++++++
 11 files changed, 281 insertions(+), 291 deletions(-)
 delete mode 100644 arch/arm/mach-exynos/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-exynos.c

Signature · 1 line

1.7.9.5

Daniel Lezcano

author

2014-04-04 13:42 UTC #1 lore raw

PATCH v2 1/17 ARM: exynos: cpuidle: Prevent forward declaration

Move the structure below the 'exynos4_enter_lowpower' function so no more need of forward declaration.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

arch/arm/mach-exynos/cpuidle.c +18 ~~-22~~

index b530231..9623a05 100644
@@ -59,30 +59,8 @@
 #define PWR_CTRL2_CORE2_UP_RATIO		(1 << 4)
 #define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
 
-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,
-	.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)
 {

@@ -213,6 +191,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 exynos_cpuidle_probe(struct platform_device *pdev)
 {
 	int cpu_id, ret;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:24 UTC #2 lore raw

PATCH v2 1/17 ARM: exynos: cpuidle: Prevent forward declaration

Quoted 5 lines · Daniel Lezcano

Move the structure below the 'exynos4_enter_lowpower' function so no more need of forward declaration.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #3 lore raw

PATCH v2 2/17 ARM: exynos: cpuidle: use cpuidle_register

In reply to Daniel Lezcano

Use the cpuidle generic function 'cpuidle_register'. That saves us from some extra lines of code and unneeded variables.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

arch/arm/mach-exynos/cpuidle.c +2 ~~-16~~

index 9623a05..b7cd75b 100644
@@ -59,8 +59,6 @@
 #define PWR_CTRL2_CORE2_UP_RATIO		(1 << 4)
 #define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
 
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
 static void exynos4_set_wakeupmask(void)
 {

@@ -211,8 +209,7 @@ static struct cpuidle_driver exynos4_idle_driver = {
 
 static int exynos_cpuidle_probe(struct platform_device *pdev)
 {
-	int cpu_id, ret;
-	struct cpuidle_device *device;
+	int ret;
 
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();

@@ -220,23 +217,12 @@ static int exynos_cpuidle_probe(struct platform_device *pdev)
 	if (soc_is_exynos5440())
 		exynos4_idle_driver.state_count = 1;
 
-	ret = cpuidle_register_driver(&exynos4_idle_driver);
+	ret = cpuidle_register(&exynos4_idle_driver, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register cpuidle 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) {
-			dev_err(&pdev->dev, "failed to register cpuidle device\n");
-			return ret;
-		}
-	}
-
 	return 0;
 }

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:28 UTC #4 lore raw

PATCH v2 2/17 ARM: exynos: cpuidle: use cpuidle_register

Quoted 34 lines · Daniel Lezcano

Use the cpuidle generic function 'cpuidle_register'. That saves us from some extra lines of code and unneeded variables.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

arch/arm/mach-exynos/cpuidle.c +1 -4

index 9623a05..b7cd75b 100644
@@ -59,8 +59,6 @@
 #define PWR_CTRL2_CORE2_UP_RATIO		(1 << 4)
 #define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
 
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
 static void exynos4_set_wakeupmask(void)
 {

@@ -211,8 +209,7 @@ static struct cpuidle_driver exynos4_idle_driver = {
 
 static int exynos_cpuidle_probe(struct platform_device *pdev)
 {
-	int cpu_id, ret;
-	struct cpuidle_device *device;
+	int ret;
 
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();

@@ -220,23 +217,12 @@ static int exynos_cpuidle_probe(struct platform_device *pdev)
 	if (soc_is_exynos5440())

arch/arm/mach-exynos/cpuidle.c static int exynos_cpuidle_probe(struct platform_device *pdev) · line 229

Quoted 10 lines

arch/arm/mach-exynos/cpuidle.c +1 -2

 		exynos4_idle_driver.state_count = 1;
 
-	ret = cpuidle_register_driver(&exynos4_idle_driver);
+	ret = cpuidle_register(&exynos4_idle_driver, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register cpuidle driver\n");
 		return ret;
 	}
 
-	for_each_online_cpu(cpu_id) {

cpuidle_register() does setup for_each_possible_cpu(). This is a good thing and your patch has a nice bugfix side effect that is worth mentioning in the patch description IMHO.

arch/arm/mach-exynos/cpuidle.c static int exynos_cpuidle_probe(struct platform_device *pdev) · line 227

Quoted 12 lines

arch/arm/mach-exynos/cpuidle.c +0 ~~-10~~

-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-		ret = cpuidle_register_device(device);
-		if (ret) {
-			dev_err(&pdev->dev, "failed to register cpuidle device\n");
-			return ret;
-		}
-	}
-
 	return 0;
 }

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #5 lore raw

PATCH v2 3/17 ARM: exynos: cpuidle: change function name prefix

In reply to Daniel Lezcano

The driver was initially written for exynos4 but the driver is used also for exynos5.

Change the function prefix name exynos4 -> exynos

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

arch/arm/mach-exynos/cpuidle.c +10 ~~-10~~

index b7cd75b..16e3325 100644
@@ -60,7 +60,7 @@
 #define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
 
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
-static void exynos4_set_wakeupmask(void)
+static void exynos_set_wakeupmask(void)
 {
 	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
 }

@@ -91,13 +91,13 @@ static int idle_finisher(unsigned long flags)
 	return 1;
 }
 
-static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
+static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
 	unsigned long tmp;
 
-	exynos4_set_wakeupmask();
+	exynos_set_wakeupmask();
 
 	/* Set value of power down register for aftr mode */
 	exynos_sys_powerdown_conf(SYS_AFTR);

@@ -141,7 +141,7 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
 	return index;
 }
 
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
+static int exynos_enter_lowpower(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {

@@ -154,7 +154,7 @@ static int exynos4_enter_lowpower(struct cpuidle_device *dev,
 	if (new_index == 0)
 		return arm_cpuidle_simple_enter(dev, drv, new_index);
 	else
-		return exynos4_enter_core0_aftr(dev, drv, new_index);
+		return exynos_enter_core0_aftr(dev, drv, new_index);
 }
 
 static void __init exynos5_core_down_clk(void)

@@ -189,13 +189,13 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name			= "exynos4_idle",
+static struct cpuidle_driver exynos_idle_driver = {
+	.name			= "exynos_idle",
 	.owner			= THIS_MODULE,
 	.states = {
 		[0] = ARM_CPUIDLE_WFI_STATE,
 		[1] = {
-			.enter			= exynos4_enter_lowpower,
+			.enter			= exynos_enter_lowpower,
 			.exit_latency		= 300,
 			.target_residency	= 100000,
 			.flags			= CPUIDLE_FLAG_TIME_VALID,

@@ -215,9 +215,9 @@ static int exynos_cpuidle_probe(struct platform_device *pdev)
 		exynos5_core_down_clk();
 
 	if (soc_is_exynos5440())
-		exynos4_idle_driver.state_count = 1;
+		exynos_idle_driver.state_count = 1;
 
-	ret = cpuidle_register(&exynos4_idle_driver, NULL);
+	ret = cpuidle_register(&exynos_idle_driver, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register cpuidle driver\n");
 		return ret;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:28 UTC #6 lore raw

PATCH v2 3/17 ARM: exynos: cpuidle: change function name prefix

Quoted 7 lines · Daniel Lezcano

The driver was initially written for exynos4 but the driver is used also for exynos5.
Change the function prefix name exynos4 -> exynos
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #7 lore raw

PATCH v2 4/17 ARM: exynos: cpuidle: encapsulate register access inside a function

In reply to Daniel Lezcano

That makes the code cleaner and encapsulted. The function will be reused in the next patch.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/pm.c |   65 ++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 24 deletions(-)

arch/arm/mach-exynos/pm.c +41 ~~-24~~

index 15af0ce..adfdf4b 100644
@@ -103,6 +103,42 @@ static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
 /* For Cortex-A9 Diagnostic and Power control register */
 static unsigned int save_arm_register[2];
 
+static void exynos_cpu_save_register(void)
+{
+	unsigned long tmp;
+
+	/* Save Power control register */
+	asm ("mrc p15, 0, %0, c15, c0, 0"
+	     : "=r" (tmp) : : "cc");
+
+	save_arm_register[0] = tmp;
+
+	/* Save Diagnostic register */
+	asm ("mrc p15, 0, %0, c15, c0, 1"
+	     : "=r" (tmp) : : "cc");
+
+	save_arm_register[1] = tmp;
+}
+
+static void exynos_cpu_restore_register(void)
+{
+	unsigned long tmp;
+
+	/* Restore Power control register */
+	tmp = save_arm_register[0];
+
+	asm volatile ("mcr p15, 0, %0, c15, c0, 0"
+		      : : "r" (tmp)
+		      : "cc");
+
+	/* Restore Diagnostic register */
+	tmp = save_arm_register[1];
+
+	asm volatile ("mcr p15, 0, %0, c15, c0, 1"
+		      : : "r" (tmp)
+		      : "cc");
+}
+
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0

@@ -162,17 +198,8 @@ static int exynos_pm_suspend(void)
 	tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
 	__raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
 
-	if (!soc_is_exynos5250()) {
-		/* Save Power control register */
-		asm ("mrc p15, 0, %0, c15, c0, 0"
-		     : "=r" (tmp) : : "cc");
-		save_arm_register[0] = tmp;
-
-		/* Save Diagnostic register */
-		asm ("mrc p15, 0, %0, c15, c0, 1"
-		     : "=r" (tmp) : : "cc");
-		save_arm_register[1] = tmp;
-	}
+	if (!soc_is_exynos5250())
+		exynos_cpu_save_register();
 
 	return 0;
 }

@@ -196,19 +223,9 @@ static void exynos_pm_resume(void)
 		/* No need to perform below restore code */
 		goto early_wakeup;
 	}
-	if (!soc_is_exynos5250()) {
-		/* Restore Power control register */
-		tmp = save_arm_register[0];
-		asm volatile ("mcr p15, 0, %0, c15, c0, 0"
-			      : : "r" (tmp)
-			      : "cc");
-
-		/* Restore Diagnostic register */
-		tmp = save_arm_register[1];
-		asm volatile ("mcr p15, 0, %0, c15, c0, 1"
-			      : : "r" (tmp)
-			      : "cc");
-	}
+
+	if (!soc_is_exynos5250())
+		exynos_cpu_restore_register();
 
 	/* For release retention */

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:28 UTC #8 lore raw

PATCH v2 4/17 ARM: exynos: cpuidle: encapsulate register access inside a function

Quoted 5 lines · Daniel Lezcano

That makes the code cleaner and encapsulted. The function will be reused in the next patch.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #9 lore raw

PATCH v2 5/17 ARM: exynos: cpuidle: Move some code inside the idle_finisher

In reply to Daniel Lezcano

Move the code around to differentiate different section of code and prepare it to be factored out in the next patches.

The call order changed but hat doesn't have a side effect because they are independent. The important call is cpu_do_idle() which must be done the last.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

arch/arm/mach-exynos/cpuidle.c +9 -8

index 16e3325..cdfb1ae 100644
@@ -87,7 +87,16 @@ static void restore_cpu_arch_register(void)
 
 static int idle_finisher(unsigned long flags)
 {
+	exynos_set_wakeupmask();
+
+	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
+	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
+
+	/* Set value of power down register for aftr mode */
+	exynos_sys_powerdown_conf(SYS_AFTR);
+
 	cpu_do_idle();
+
 	return 1;
 }

@@ -97,14 +106,6 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 {
 	unsigned long tmp;
 
-	exynos_set_wakeupmask();
-
-	/* Set value of power down register for aftr mode */
-	exynos_sys_powerdown_conf(SYS_AFTR);
-
-	__raw_writel(virt_to_phys(exynos_cpu_resume), REG_DIRECTGO_ADDR);
-	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
-
 	save_cpu_arch_register();
 
 	/* Setting Central Sequence Register for power down mode */

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:29 UTC #10 lore raw

PATCH v2 5/17 ARM: exynos: cpuidle: Move some code inside the idle_finisher

Quoted 8 lines · Daniel Lezcano

Move the code around to differentiate different section of code and prepare it to be factored out in the next patches.
The call order changed but hat doesn't have a side effect because they are independent. The important call is cpu_do_idle() which must be done the last.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #11 lore raw

PATCH v2 6/17 ARM: exynos: cpuidle: Fix S5P_WAKEUP_STAT call

In reply to Daniel Lezcano

This function should be called only when the powerdown sequence fails.

Even if the current code does not hurt, by moving this line, we have the same code than the one in pm.c.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

arch/arm/mach-exynos/cpuidle.c +2 -3

index cdfb1ae..6663349 100644
@@ -134,11 +134,10 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
 		tmp |= S5P_CENTRAL_LOWPWR_CFG;
 		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
+		/* Clear wakeup state register */
+		__raw_writel(0x0, S5P_WAKEUP_STAT);
 	}
 
-	/* Clear wakeup state register */
-	__raw_writel(0x0, S5P_WAKEUP_STAT);
-
 	return index;
 }

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:29 UTC #12 lore raw

PATCH v2 6/17 ARM: exynos: cpuidle: Fix S5P_WAKEUP_STAT call

Quoted 7 lines · Daniel Lezcano

This function should be called only when the powerdown sequence fails.
Even if the current code does not hurt, by moving this line, we have the same code than the one in pm.c.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:42 UTC #13 lore raw

PATCH v2 7/17 ARM: exynos: cpuidle: Use the cpu_pm notifier

In reply to Daniel Lezcano

Use the cpu_pm_enter/exit notifier to group some pm code inside the pm file.

The save and restore code is duplicated across pm.c and cpuidle.c. By using the cpu_pm notifier, we can factor out the routine.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   24 ------------------------
 arch/arm/mach-exynos/pm.c      |   29 +++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 24 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 ~~-24~~

index 6663349..7f1f4ef 100644
@@ -65,26 +65,6 @@ static void exynos_set_wakeupmask(void)
 	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
 }
 
-static unsigned int g_pwr_ctrl, g_diag_reg;
-
-static void save_cpu_arch_register(void)
-{
-	/*read power control register*/
-	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
-	/*read diagnostic register*/
-	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
-	return;
-}
-
-static void restore_cpu_arch_register(void)
-{
-	/*write power control register*/
-	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
-	/*write diagnostic register*/
-	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
-	return;
-}
-
 static int idle_finisher(unsigned long flags)
 {
 	exynos_set_wakeupmask();

@@ -106,8 +86,6 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 {
 	unsigned long tmp;
 
-	save_cpu_arch_register();
-
 	/* Setting Central Sequence Register for power down mode */
 	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
 	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;

@@ -122,8 +100,6 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 #endif
 	cpu_pm_exit();
 
-	restore_cpu_arch_register();
-
 	/*
 	 * If PMU failed while entering sleep mode, WFI will be
 	 * ignored by PMU and then exiting cpu_do_idle().

arch/arm/mach-exynos/pm.c +29 -0

index adfdf4b..67d75fe 100644
@@ -16,6 +16,7 @@
 #include <linux/init.h>
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
+#include <linux/cpu_pm.h>
 #include <linux/io.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/err.h>

@@ -321,10 +322,38 @@ static const struct platform_suspend_ops exynos_suspend_ops = {
 	.valid		= suspend_valid_only_mem,
 };
 
+static int exynos_cpu_pm_notifier(struct notifier_block *self,
+				  unsigned long cmd, void *v)
+{
+	int cpu = smp_processor_id();
+
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		if (cpu == 0) {
+			exynos_cpu_save_register();
+		}
+		break;
+
+	case CPU_PM_EXIT:
+		if (cpu == 0) {
+			exynos_cpu_restore_register();
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block exynos_cpu_pm_notifier_block = {
+	.notifier_call = exynos_cpu_pm_notifier,
+};
+
 void __init exynos_pm_init(void)
 {
 	u32 tmp;
 
+	cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block);
+
 	/* Platform-specific GIC callback */
 	gic_arch_extn.irq_set_wake = exynos_irq_set_wake;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:30 UTC #14 lore raw

PATCH v2 7/17 ARM: exynos: cpuidle: Use the cpu_pm notifier

Quoted 7 lines · Daniel Lezcano

Use the cpu_pm_enter/exit notifier to group some pm code inside the pm file.
The save and restore code is duplicated across pm.c and cpuidle.c. By using the cpu_pm notifier, we can factor out the routine.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Thanks for fixing Exynos5250 support in V2.

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #15 lore raw

PATCH v2 8/17 ARM: exynos: cpuidle: Move scu_enable in the cpu_pm notifier

In reply to Daniel Lezcano

We make the cpuidle code less arch dependent.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |    6 ------
 arch/arm/mach-exynos/pm.c      |    4 ++++
 2 files changed, 4 insertions(+), 6 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 -6

index 7f1f4ef..ce31004 100644
@@ -18,7 +18,6 @@
 #include <linux/platform_device.h>
 
 #include <asm/proc-fns.h>
-#include <asm/smp_scu.h>
 #include <asm/suspend.h>
 #include <asm/unified.h>
 #include <asm/cpuidle.h>

@@ -93,11 +92,6 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 
 	cpu_pm_enter();
 	cpu_suspend(0, idle_finisher);
-
-#ifdef CONFIG_SMP
-	if (!soc_is_exynos5250())
-		scu_enable(S5P_VA_SCU);
-#endif
 	cpu_pm_exit();
 
 	/*

arch/arm/mach-exynos/pm.c +4 -0

index 67d75fe..aba577f 100644
@@ -336,6 +336,10 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 
 	case CPU_PM_EXIT:
 		if (cpu == 0) {
+#ifdef CONFIG_SMP
+			if (!soc_is_exynos5250())
+				scu_enable(S5P_VA_SCU);
+#endif
 			exynos_cpu_restore_register();
 		}
 		break;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:30 UTC #16 lore raw

PATCH v2 8/17 ARM: exynos: cpuidle: Move scu_enable in the cpu_pm notifier

Quoted 4 lines · Daniel Lezcano

We make the cpuidle code less arch dependent.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #17 lore raw

PATCH v2 9/17 ARM: exynos: cpuidle: Remove ifdef for scu_enable

In reply to Daniel Lezcano

The scu_enable function is already a noop in the scu's header file is CONFIG_SMP=n, so no need to use these macros in the code.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/pm.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/arm/mach-exynos/pm.c +1 -3

index aba577f..9773a00 100644
@@ -244,7 +244,7 @@ static void exynos_pm_resume(void)
 
 	s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 
-	if (IS_ENABLED(CONFIG_SMP) && !soc_is_exynos5250())
+	if (!soc_is_exynos5250())
 		scu_enable(S5P_VA_SCU);
 
 early_wakeup:

@@ -336,10 +336,8 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 
 	case CPU_PM_EXIT:
 		if (cpu == 0) {
-#ifdef CONFIG_SMP
 			if (!soc_is_exynos5250())
 				scu_enable(S5P_VA_SCU);
-#endif
 			exynos_cpu_restore_register();
 		}
 		break;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:31 UTC #18 lore raw

PATCH v2 9/17 ARM: exynos: cpuidle: Remove ifdef for scu_enable

Quoted 5 lines · Daniel Lezcano

The scu_enable function is already a noop in the scu's header file is CONFIG_SMP=n, so no need to use these macros in the code.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #19 lore raw

PATCH v2 10/17 ARM: exynos: cpuidle: Move exynos_set_wakeupmask in the cpu_pm notifier

In reply to Daniel Lezcano

Let's encapsulate more the PM code inside the PM file by moving the 'exynos_set_wakeupmask' function inside the pm.c and the call in the cpu_pm notifier.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |    7 -------
 arch/arm/mach-exynos/pm.c      |    7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 -7

index ce31004..01444ed 100644
@@ -58,15 +58,8 @@
 #define PWR_CTRL2_CORE2_UP_RATIO		(1 << 4)
 #define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
 
-/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
-static void exynos_set_wakeupmask(void)
-{
-	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
-}
-
 static int idle_finisher(unsigned long flags)
 {
-	exynos_set_wakeupmask();
 
 	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
 	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);

arch/arm/mach-exynos/pm.c +7 -0

index 9773a00..c8b3dc4 100644
@@ -322,6 +322,12 @@ static const struct platform_suspend_ops exynos_suspend_ops = {
 	.valid		= suspend_valid_only_mem,
 };
 
+/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
+static void exynos_set_wakeupmask(void)
+{
+	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
+}
+
 static int exynos_cpu_pm_notifier(struct notifier_block *self,
 				  unsigned long cmd, void *v)
 {

@@ -331,6 +337,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 	case CPU_PM_ENTER:
 		if (cpu == 0) {
 			exynos_cpu_save_register();
+			exynos_set_wakeupmask();
 		}
 		break;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:31 UTC #20 lore raw

PATCH v2 10/17 ARM: exynos: cpuidle: Move exynos_set_wakeupmask in the cpu_pm notifier

Quoted 6 lines · Daniel Lezcano

Let's encapsulate more the PM code inside the PM file by moving the 'exynos_set_wakeupmask' function inside the pm.c and the call in the cpu_pm notifier.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #21 lore raw

PATCH v2 11/17 ARM: exynos: cpuidle: Move the power sequence call in the cpu_pm notifier

In reply to Daniel Lezcano

The code to initiate and exit the powerdown sequence is the same in pm.c and cpuidle.c.

Let's split the common part in the pm.c and reuse it from the cpu_pm notifier.

That is one more step forward to make the cpuidle driver arch indenpendant.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   21 ---------------------
 arch/arm/mach-exynos/pm.c      |   22 ++++++++++++++++++----
 2 files changed, 18 insertions(+), 25 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 ~~-21~~

index 01444ed..81d7197 100644
@@ -76,31 +76,10 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
-	unsigned long tmp;
-
-	/* Setting Central Sequence Register for power down mode */
-	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
-	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-
 	cpu_pm_enter();
 	cpu_suspend(0, idle_finisher);
 	cpu_pm_exit();
 
-	/*
-	 * If PMU failed while entering sleep mode, WFI will be
-	 * ignored by PMU and then exiting cpu_do_idle().
-	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
-	 * in this situation.
-	 */
-	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
-		tmp |= S5P_CENTRAL_LOWPWR_CFG;
-		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-		/* Clear wakeup state register */
-		__raw_writel(0x0, S5P_WAKEUP_STAT);
-	}
-
 	return index;
 }

arch/arm/mach-exynos/pm.c +18 -4

index c8b3dc4..0e73591 100644
@@ -184,15 +184,19 @@ static void exynos_pm_prepare(void)
 	__raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0);
 }
 
-static int exynos_pm_suspend(void)
+static void exynos_pm_central_suspend(void)
 {
 	unsigned long tmp;
 
 	/* Setting Central Sequence Register for power down mode */
-
 	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
 	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
 	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
+}
+
+static int exynos_pm_suspend(void)
+{
+	unsigned long tmp;
 
 	/* Setting SEQ_OPTION register */

@@ -205,7 +209,7 @@ static int exynos_pm_suspend(void)
 	return 0;
 }
 
-static void exynos_pm_resume(void)
+static int exynos_pm_central_resume(void)
 {
 	unsigned long tmp;

@@ -222,9 +226,17 @@ static void exynos_pm_resume(void)
 		/* clear the wakeup state register */
 		__raw_writel(0x0, S5P_WAKEUP_STAT);
 		/* No need to perform below restore code */
-		goto early_wakeup;
+		return -1;
 	}
 
+	return 0;
+}
+
+static void exynos_pm_resume(void)
+{
+	if (exynos_pm_central_resume())
+		goto early_wakeup;
+
 	if (!soc_is_exynos5250())
 		exynos_cpu_restore_register();

@@ -336,6 +348,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 	switch (cmd) {
 	case CPU_PM_ENTER:
 		if (cpu == 0) {
+			exynos_pm_central_suspend();
 			exynos_cpu_save_register();
 			exynos_set_wakeupmask();
 		}

@@ -346,6 +359,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 			if (!soc_is_exynos5250())
 				scu_enable(S5P_VA_SCU);
 			exynos_cpu_restore_register();
+			exynos_pm_central_resume();
 		}
 		break;
 	}

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:31 UTC #22 lore raw

PATCH v2 11/17 ARM: exynos: cpuidle: Move the power sequence call in the cpu_pm notifier

Quoted 9 lines · Daniel Lezcano

The code to initiate and exit the powerdown sequence is the same in pm.c and cpuidle.c.
Let's split the common part in the pm.c and reuse it from the cpu_pm notifier.
That is one more step forward to make the cpuidle driver arch indenpendant.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #23 lore raw

PATCH v2 12/17 ARM: exynos: cpuidle: Move S5P_CHECK_AFTR in a header

In reply to Daniel Lezcano

Move the S5P_CHECK_AFTR definition to the header it belongs to.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c  |    2 --
 arch/arm/mach-exynos/regs-pmu.h |    1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 -2

index 81d7197..cd27dbf 100644
@@ -37,8 +37,6 @@
 			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
 			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
 
-#define S5P_CHECK_AFTR		0xFCBA0D10
-
 #define EXYNOS5_PWR_CTRL1			(S5P_VA_CMU + 0x01020)
 #define EXYNOS5_PWR_CTRL2			(S5P_VA_CMU + 0x01024)

arch/arm/mach-exynos/regs-pmu.h +1 -0

index 4f6a256..09c43c3 100644
@@ -120,6 +120,7 @@
 #define S5P_INT_LOCAL_PWR_EN			0x7
 
 #define S5P_CHECK_SLEEP				0x00000BAD
+#define S5P_CHECK_AFTR				0xFCBA0D10
 
 /* Only for EXYNOS4210 */
 #define S5P_CMU_CLKSTOP_LCD1_LOWPWR	S5P_PMUREG(0x1154)

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:32 UTC #24 lore raw

PATCH v2 12/17 ARM: exynos: cpuidle: Move S5P_CHECK_AFTR in a header

Quoted 4 lines · Daniel Lezcano

Move the S5P_CHECK_AFTR definition to the header it belongs to.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #25 lore raw

PATCH v2 13/17 ARM: exynos: cpuidle: Move clock setup to pm.c

In reply to Daniel Lezcano

One more step is moving the clock ratio setting at idle time in pm.c

The macro names have been changed to be consistent with the other macros name in the file.

Note, the clock divider was working only when cpuidle was enabled because it was in its init routine. With this change, the clock divider is set in the pm's init routine, so it will also operate when the cpuidle driver is not set, which is good.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c  |   54 ---------------------------------------
 arch/arm/mach-exynos/pm.c       |   35 +++++++++++++++++++++++++
 arch/arm/mach-exynos/regs-pmu.h |   19 ++++++++++++++
 3 files changed, 54 insertions(+), 54 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 ~~-54~~

index cd27dbf..44d169b 100644
@@ -37,25 +37,6 @@
 			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
 			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
 
-#define EXYNOS5_PWR_CTRL1			(S5P_VA_CMU + 0x01020)
-#define EXYNOS5_PWR_CTRL2			(S5P_VA_CMU + 0x01024)
-
-#define PWR_CTRL1_CORE2_DOWN_RATIO		(7 << 28)
-#define PWR_CTRL1_CORE1_DOWN_RATIO		(7 << 16)
-#define PWR_CTRL1_DIV2_DOWN_EN			(1 << 9)
-#define PWR_CTRL1_DIV1_DOWN_EN			(1 << 8)
-#define PWR_CTRL1_USE_CORE1_WFE			(1 << 5)
-#define PWR_CTRL1_USE_CORE0_WFE			(1 << 4)
-#define PWR_CTRL1_USE_CORE1_WFI			(1 << 1)
-#define PWR_CTRL1_USE_CORE0_WFI			(1 << 0)
-
-#define PWR_CTRL2_DIV2_UP_EN			(1 << 25)
-#define PWR_CTRL2_DIV1_UP_EN			(1 << 24)
-#define PWR_CTRL2_DUR_STANDBY2_VAL		(1 << 16)
-#define PWR_CTRL2_DUR_STANDBY1_VAL		(1 << 8)
-#define PWR_CTRL2_CORE2_UP_RATIO		(1 << 4)
-#define PWR_CTRL2_CORE1_UP_RATIO		(1 << 0)
-
 static int idle_finisher(unsigned long flags)
 {

@@ -97,38 +78,6 @@ static int exynos_enter_lowpower(struct cpuidle_device *dev,
 		return exynos_enter_core0_aftr(dev, drv, new_index);
 }
 
-static void __init exynos5_core_down_clk(void)
-{
-	unsigned int tmp;
-
-	/*
-	 * Enable arm clock down (in idle) and set arm divider
-	 * ratios in WFI/WFE state.
-	 */
-	tmp = PWR_CTRL1_CORE2_DOWN_RATIO | \
-	      PWR_CTRL1_CORE1_DOWN_RATIO | \
-	      PWR_CTRL1_DIV2_DOWN_EN	 | \
-	      PWR_CTRL1_DIV1_DOWN_EN	 | \
-	      PWR_CTRL1_USE_CORE1_WFE	 | \
-	      PWR_CTRL1_USE_CORE0_WFE	 | \
-	      PWR_CTRL1_USE_CORE1_WFI	 | \
-	      PWR_CTRL1_USE_CORE0_WFI;
-	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
-
-	/*
-	 * Enable arm clock up (on exiting idle). Set arm divider
-	 * ratios when not in idle along with the standby duration
-	 * ratios.
-	 */
-	tmp = PWR_CTRL2_DIV2_UP_EN	 | \
-	      PWR_CTRL2_DIV1_UP_EN	 | \
-	      PWR_CTRL2_DUR_STANDBY2_VAL | \
-	      PWR_CTRL2_DUR_STANDBY1_VAL | \
-	      PWR_CTRL2_CORE2_UP_RATIO	 | \
-	      PWR_CTRL2_CORE1_UP_RATIO;
-	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
-}
-
 static struct cpuidle_driver exynos_idle_driver = {
 	.name			= "exynos_idle",
 	.owner			= THIS_MODULE,

@@ -151,9 +100,6 @@ static int exynos_cpuidle_probe(struct platform_device *pdev)
 {
 	int ret;
 
-	if (soc_is_exynos5250())
-		exynos5_core_down_clk();
-
 	if (soc_is_exynos5440())
 		exynos_idle_driver.state_count = 1;

arch/arm/mach-exynos/pm.c +35 -0

index 0e73591..90fb692 100644
@@ -140,6 +140,38 @@ static void exynos_cpu_restore_register(void)
 		      : "cc");
 }
 
+static void __init exynos5_core_down_clk(void)
+{
+	unsigned int tmp;
+
+	/*
+	 * Enable arm clock down (in idle) and set arm divider
+	 * ratios in WFI/WFE state.
+	 */
+	tmp = EXYNOS5_PWR_CTRL1_CORE2_DOWN_RATIO | \
+	      EXYNOS5_PWR_CTRL1_CORE1_DOWN_RATIO | \
+	      EXYNOS5_PWR_CTRL1_DIV2_DOWN_EN	 | \
+	      EXYNOS5_PWR_CTRL1_DIV1_DOWN_EN	 | \
+	      EXYNOS5_PWR_CTRL1_USE_CORE1_WFE	 | \
+	      EXYNOS5_PWR_CTRL1_USE_CORE0_WFE	 | \
+	      EXYNOS5_PWR_CTRL1_USE_CORE1_WFI	 | \
+	      EXYNOS5_PWR_CTRL1_USE_CORE0_WFI;
+	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
+
+	/*
+	 * Enable arm clock up (on exiting idle). Set arm divider
+	 * ratios when not in idle along with the standby duration
+	 * ratios.
+	 */
+	tmp = EXYNOS5_PWR_CTRL2_DIV2_UP_EN	 | \
+	      EXYNOS5_PWR_CTRL2_DIV1_UP_EN	 | \
+	      EXYNOS5_PWR_CTRL2_DUR_STANDBY2_VAL | \
+	      EXYNOS5_PWR_CTRL2_DUR_STANDBY1_VAL | \
+	      EXYNOS5_PWR_CTRL2_CORE2_UP_RATIO	 | \
+	      EXYNOS5_PWR_CTRL2_CORE1_UP_RATIO;
+	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
+}
+
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0

@@ -256,6 +288,9 @@ static void exynos_pm_resume(void)
 
 	s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save));
 
+	if (soc_is_exynos5250())
+		exynos5_core_down_clk();
+
 	if (!soc_is_exynos5250())
 		scu_enable(S5P_VA_SCU);

arch/arm/mach-exynos/regs-pmu.h +19 -0

index 09c43c3..ba5f038 100644
@@ -314,4 +314,23 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#define EXYNOS5_PWR_CTRL1			(S5P_VA_CMU + 0x01020)
+#define EXYNOS5_PWR_CTRL2			(S5P_VA_CMU + 0x01024)
+
+#define EXYNOS5_PWR_CTRL1_CORE2_DOWN_RATIO	(7 << 28)
+#define EXYNOS5_PWR_CTRL1_CORE1_DOWN_RATIO	(7 << 16)
+#define EXYNOS5_PWR_CTRL1_DIV2_DOWN_EN		(1 << 9)
+#define EXYNOS5_PWR_CTRL1_DIV1_DOWN_EN		(1 << 8)
+#define EXYNOS5_PWR_CTRL1_USE_CORE1_WFE		(1 << 5)
+#define EXYNOS5_PWR_CTRL1_USE_CORE0_WFE		(1 << 4)
+#define EXYNOS5_PWR_CTRL1_USE_CORE1_WFI		(1 << 1)
+#define EXYNOS5_PWR_CTRL1_USE_CORE0_WFI		(1 << 0)
+
+#define EXYNOS5_PWR_CTRL2_DIV2_UP_EN		(1 << 25)
+#define EXYNOS5_PWR_CTRL2_DIV1_UP_EN		(1 << 24)
+#define EXYNOS5_PWR_CTRL2_DUR_STANDBY2_VAL	(1 << 16)
+#define EXYNOS5_PWR_CTRL2_DUR_STANDBY1_VAL	(1 << 8)
+#define EXYNOS5_PWR_CTRL2_CORE2_UP_RATIO	(1 << 4)
+#define EXYNOS5_PWR_CTRL2_CORE1_UP_RATIO	(1 << 0)
+
 #endif /* __ASM_ARCH_REGS_PMU_H */

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:35 UTC #26 lore raw

PATCH v2 13/17 ARM: exynos: cpuidle: Move clock setup to pm.c

Quoted 5 lines · Daniel Lezcano

One more step is moving the clock ratio setting at idle time in pm.c
The macro names have been changed to be consistent with the other macros name in the file.

Quoted 7 lines

Note, the clock divider was working only when cpuidle was enabled because it was in its init routine. With this change, the clock divider is set in the pm's init routine, so it will also operate when the cpuidle driver is not set, which is good.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

There was the earlier patch which was moving this code to clock driver however it fell through cracks. Either way is fine with me so..

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #27 lore raw

PATCH v2 14/17 ARM: exynos: cpuidle: Move the boot vector in pm.c

In reply to Daniel Lezcano

As usual, move the boot vector setting in the pm.c file and use the cpu_pm notifier to set it up.

Remove the unused headers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/cpuidle.c |   23 -----------------------
 arch/arm/mach-exynos/pm.c      |   15 +++++++++++++++
 2 files changed, 15 insertions(+), 23 deletions(-)

arch/arm/mach-exynos/cpuidle.c +0 ~~-23~~

index 44d169b..4b94181 100644
@@ -8,46 +8,23 @@
  * 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 <linux/platform_device.h>
 
 #include <asm/proc-fns.h>
 #include <asm/suspend.h>
-#include <asm/unified.h>
 #include <asm/cpuidle.h>
 
 #include <plat/cpu.h>
-#include <plat/pm.h>
-
-#include <mach/map.h>
 
 #include "common.h"
-#include "regs-pmu.h"
-
-#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
-#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
 
 static int idle_finisher(unsigned long flags)
 {
-
-	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
-	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
-
 	/* Set value of power down register for aftr mode */
 	exynos_sys_powerdown_conf(SYS_AFTR);
-
 	cpu_do_idle();
-
 	return 1;
 }

arch/arm/mach-exynos/pm.c +15 -0

index 90fb692..e4ecd8c 100644
@@ -172,6 +172,20 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
+#define EXYNOS_BOOT_VECTOR_ADDR (samsung_rev() == EXYNOS4210_REV_1_1 ? \
+			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
+			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
+
+#define EXYNOS_BOOT_VECTOR_FLAG (samsung_rev() == EXYNOS4210_REV_1_1 ? \
+			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
+			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
+
+static void exynos_cpu_set_boot_vector(void)
+{
+	__raw_writel(virt_to_phys(exynos_cpu_resume), EXYNOS_BOOT_VECTOR_ADDR);
+	__raw_writel(S5P_CHECK_AFTR, EXYNOS_BOOT_VECTOR_FLAG);
+}
+
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0

@@ -387,6 +401,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 			exynos_cpu_save_register();
 			exynos_set_wakeupmask();
 		}
+		exynos_cpu_set_boot_vector();
 		break;
 
 	case CPU_PM_EXIT:

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:35 UTC #28 lore raw

PATCH v2 14/17 ARM: exynos: cpuidle: Move the boot vector in pm.c

Quoted 7 lines · Daniel Lezcano

As usual, move the boot vector setting in the pm.c file and use the cpu_pm notifier to set it up.
Remove the unused headers.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #29 lore raw

PATCH v2 15/17 ARM: exynos: cpuidle: Pass the AFTR callback to the platform_data

In reply to Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/common.h  |    1 +
 arch/arm/mach-exynos/cpuidle.c |    6 ++++--
 arch/arm/mach-exynos/exynos.c  |    5 +++--
 arch/arm/mach-exynos/pmu.c     |    6 ++++++
 4 files changed, 14 insertions(+), 4 deletions(-)

arch/arm/mach-exynos/common.h +1 -0

index 9ef3f83..7d9432e 100644
@@ -62,5 +62,6 @@ struct exynos_pmu_conf {
 };
 
 extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
+extern void exynos_sys_powerdown_aftr(void);
 
 #endif /* __ARCH_ARM_MACH_EXYNOS_COMMON_H */

arch/arm/mach-exynos/cpuidle.c +4 -2

index 4b94181..149fdeb 100644
@@ -18,12 +18,12 @@
 
 #include <plat/cpu.h>
 
-#include "common.h"
+static void (*exynos_aftr)(void);
 
 static int idle_finisher(unsigned long flags)
 {
 	/* Set value of power down register for aftr mode */
-	exynos_sys_powerdown_conf(SYS_AFTR);
+	exynos_aftr();
 	cpu_do_idle();
 	return 1;
 }

@@ -80,6 +80,8 @@ static int exynos_cpuidle_probe(struct platform_device *pdev)
 	if (soc_is_exynos5440())
 		exynos_idle_driver.state_count = 1;
 
+	exynos_aftr = (void *)(pdev->dev.platform_data);
+
 	ret = cpuidle_register(&exynos_idle_driver, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register cpuidle driver\n");

arch/arm/mach-exynos/exynos.c +3 -2

index b567361..451b371 100644
@@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
 }
 
 static struct platform_device exynos_cpuidle = {
-	.name		= "exynos_cpuidle",
-	.id		= -1,
+	.name              = "exynos_cpuidle",
+	.dev.platform_data = exynos_sys_powerdown_aftr,
+	.id                = -1,
 };
 
 void __init exynos_cpuidle_init(void)

arch/arm/mach-exynos/pmu.c +6 -0

index 05c7ce1..fd1ae22 100644
@@ -389,6 +389,12 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
 	}
 }
 
+/* Set value of power down register for aftr mode */
+void exynos_sys_powerdown_aftr(void)
+{
+	exynos_sys_powerdown_conf(SYS_AFTR);
+}
+
 static int __init exynos_pmu_init(void)
 {
 	unsigned int value;

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:35 UTC #30 lore raw

PATCH v2 15/17 ARM: exynos: cpuidle: Pass the AFTR callback to the platform_data

Quoted 5 lines · Daniel Lezcano

No more dependency on the arch code. The platform_data field is used to set the PM callback as the other cpuidle drivers.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Daniel Lezcano

author

2014-04-04 13:43 UTC #31 lore raw

PATCH v2 16/17 ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory

In reply to Daniel Lezcano

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/Makefile                      |    1 -
 drivers/cpuidle/Kconfig.arm                        |    7 +++++++
 drivers/cpuidle/Makefile                           |    1 +
 .../cpuidle.c => drivers/cpuidle/cpuidle-exynos.c  |    0
 4 files changed, 8 insertions(+), 1 deletion(-)
 rename arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/cpuidle-exynos.c (100%)

arch/arm/mach-exynos/Makefile +0 -1

index a656dbe..21bd364 100644
@@ -16,7 +16,6 @@ obj-$(CONFIG_ARCH_EXYNOS)	+= exynos.o
 
 obj-$(CONFIG_PM_SLEEP)		+= pm.o sleep.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 
 obj-$(CONFIG_ARCH_EXYNOS)	+= pmu.o

drivers/cpuidle/Kconfig.arm +7 -0

index d988948..92f0c12 100644
@@ -44,3 +44,10 @@ config ARM_AT91_CPUIDLE
 	depends on ARCH_AT91
 	help
 	  Select this to enable cpuidle for AT91 processors
+
+config ARM_EXYNOS_CPUIDLE
+	bool "Cpu Idle Driver for the Exynos processors"
+	default y
+	depends on ARCH_EXYNOS
+	help
+	  Select this to enable cpuidle for Exynos processors

drivers/cpuidle/Makefile +1 -0

index f71ae1b..0d1540a 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE)	+= cpuidle-kirkwood.o
 obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
+obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
 
 ###############################################################################
 # POWERPC drivers

drivers/cpuidle/cpuidle-exynos.c renamed from arch/arm/mach-exynos/cpuidle.c +0 -0

similarity index 100%
rename from arch/arm/mach-exynos/cpuidle.c
rename to drivers/cpuidle/cpuidle-exynos.c

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 15:36 UTC #32 lore raw

PATCH v2 16/17 ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory

Quoted 2 lines · Daniel Lezcano

Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Sachin Kamat

2014-04-07 09:51 UTC #33 lore raw

PATCH v2 16/17 ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory

In reply to Daniel Lezcano

Hi Daniel,

Quoted 9 lines · Daniel Lezcano

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/mach-exynos/Makefile                      |    1 -
 drivers/cpuidle/Kconfig.arm                        |    7 +++++++
 drivers/cpuidle/Makefile                           |    1 +
 .../cpuidle.c => drivers/cpuidle/cpuidle-exynos.c  |    0
 4 files changed, 8 insertions(+), 1 deletion(-)
 rename arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/cpuidle-exynos.c (100%)

This driver now references platform header file (plat/cpu.h) for some Exynos5440 specific check
which is not good considering multiplatform support. Any plan to get rid of this?

Signature · 2 lines

With warm regards,
Sachin

Daniel Lezcano

author

2014-04-07 11:53 UTC #34 lore raw

PATCH v2 16/17 ARM: exynos: cpuidle: Move the driver to drivers/cpuidle directory

Quoted 13 lines · Sachin Kamat

Hi Daniel,
Quoted 9 lines · Daniel Lezcano
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>
---
  arch/arm/mach-exynos/Makefile                      |    1 -
  drivers/cpuidle/Kconfig.arm                        |    7 +++++++
  drivers/cpuidle/Makefile                           |    1 +
  .../cpuidle.c => drivers/cpuidle/cpuidle-exynos.c  |    0
  4 files changed, 8 insertions(+), 1 deletion(-)
rename arch/arm/mach-exynos/cpuidle.c => drivers/cpuidle/cpuidle-exynos.c (100%)

Quoted 4 lines

This driver now references platform header file (plat/cpu.h) for some Exynos5440 specific check
which is not good considering multiplatform support. Any plan to get rid of this?

Yes. Prevent to register the platform device for this board and remove these lines in the driver.

What is the benefit of having a single state WFI cpuidle driver ? That pulls all the governor computation and decision for nothing.

Signature · 5 lines

 <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

Daniel Lezcano

author

2014-04-04 13:43 UTC #35 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

In reply to Daniel Lezcano

The cpuidle driver is broken since v3.11 and now we are at v3.14.

Default the cpuidle driver to favorize a better detection next time.

Signed-off-by Daniel Lezcano <[email protected]>

Reviewed-by Viresh Kumar <[email protected]>

---
 arch/arm/configs/exynos_defconfig |    1 +
 1 file changed, 1 insertion(+)

arch/arm/configs/exynos_defconfig +1 -0

index 4ce7b70..6ed4b34 100644
@@ -132,3 +132,4 @@ CONFIG_DEBUG_INFO=y
 CONFIG_DEBUG_USER=y
 CONFIG_CRYPTO_SHA256=y
 CONFIG_CRC_CCITT=y
+CONFIG_CPU_IDLE=y

Signature · 1 line

1.7.9.5

Bartlomiej Zolnierkiewicz

2014-04-04 16:09 UTC #36 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 15 lines · Daniel Lezcano

The cpuidle driver is broken since v3.11 and now we are at v3.14.
Default the cpuidle driver to favorize a better detection next time.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>
---
 arch/arm/configs/exynos_defconfig |    1 +
 1 file changed, 1 insertion(+)
arch/arm/configs/exynos_defconfig +0 -0
index 4ce7b70..6ed4b34 100644

arch/arm/configs/exynos_defconfig CONFIG_DEBUG_INFO=y · line 135

Quoted 4 lines

arch/arm/configs/exynos_defconfig +1 -0

@@ -132,3 +132,4 @@ CONFIG_DEBUG_INFO=y
 CONFIG_DEBUG_USER=y
 CONFIG_CRYPTO_SHA256=y
 CONFIG_CRC_CCITT=y
+CONFIG_CPU_IDLE=y

Sorry but I have to NAK this change. There are three issues with AFTR mode that should be resolved first before this patch can be merged:

* The upstream Exynos cpuidle code lacks support for secure firmware and it is used on i.e. Trats2 boards (Exynos4412). Attempts to use AFTR on such hardware results in oops + lockup.

I have patches adding secure firmware support to Exynos cpuidle driver but they depend on Tomasz Figa's PM changes which are not yet upstream.

* Somebody needs to verify that the current Exynos cpuidle driver works in the AFTR mode on Exynos5420 for which support has been added recently (I really doubt it looking at some internal trees).

* Some of our u-boot bootloader versions are incompatible with AFTR. We have observed the problem happening on Trats board (Exynos4210) on which it can be fixed by using the upstream u-boot version and Universal C210 board (Exynos4210 with broken SMP support) on which there is no upstream u-boot available (IIRC) and because of the broken SMP support enabling cpuidle results in attempt to enter AFTR during boot + immediate lockup.

I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem. Namely there should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.

For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..

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

Daniel Lezcano

author

2014-04-07 09:43 UTC #37 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 25 lines · Bartlomiej Zolnierkiewicz

Quoted 15 lines · Daniel Lezcano
The cpuidle driver is broken since v3.11 and now we are at v3.14.
Default the cpuidle driver to favorize a better detection next time.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>
---
  arch/arm/configs/exynos_defconfig |    1 +
  1 file changed, 1 insertion(+)
arch/arm/configs/exynos_defconfig +0 -0
index 4ce7b70..6ed4b34 100644
arch/arm/configs/exynos_defconfig CONFIG_DEBUG_INFO=y · line 135
Quoted 4 lines
arch/arm/configs/exynos_defconfig +1 -0
@@ -132,3 +132,4 @@ CONFIG_DEBUG_INFO=y
  CONFIG_DEBUG_USER=y
  CONFIG_CRYPTO_SHA256=y
  CONFIG_CRC_CCITT=y
+CONFIG_CPU_IDLE=y
Sorry but I have to NAK this change. There are three issues with AFTR mode that should be resolved first before this patch can be merged:

Quoted 5 lines

* The upstream Exynos cpuidle code lacks support for secure firmware and it is used on i.e. Trats2 boards (Exynos4412). Attempts to use AFTR on such hardware results in oops + lockup.
I have patches adding secure firmware support to Exynos cpuidle driver but they depend on Tomasz Figa's PM changes which are not yet upstream.

Ok.

Quoted 3 lines

* Somebody needs to verify that the current Exynos cpuidle driver works in the AFTR mode on Exynos5420 for which support has been added recently (I really doubt it looking at some internal trees).

Rajeshwari (cc'ed) is working on creating a big.Little driver for this board, so it will be a separate cpuidle driver.

Quoted 14 lines

* Some of our u-boot bootloader versions are incompatible with AFTR. We have observed the problem happening on Trats board (Exynos4210) on which it can be fixed by using the upstream u-boot version and Universal C210 board (Exynos4210 with broken SMP support) on which there is no upstream u-boot available (IIRC) and because of the broken SMP support enabling cpuidle results in attempt to enter AFTR during boot + immediate lockup.
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem. Namely there should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.

Quoted 3 lines

For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..

Ok.

Kukjin, is it possible you merge patches 1->16 ?

Thanks

-- Daniel

Quoted 5 lines

--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Signature · 5 lines

 <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

Bartlomiej Zolnierkiewicz

2014-04-07 16:19 UTC #38 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 55 lines · Daniel Lezcano

Quoted 25 lines · Bartlomiej Zolnierkiewicz
Quoted 15 lines · Daniel Lezcano
The cpuidle driver is broken since v3.11 and now we are at v3.14.
Default the cpuidle driver to favorize a better detection next time.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>
---
  arch/arm/configs/exynos_defconfig |    1 +
  1 file changed, 1 insertion(+)
arch/arm/configs/exynos_defconfig +0 -0
index 4ce7b70..6ed4b34 100644
arch/arm/configs/exynos_defconfig CONFIG_DEBUG_INFO=y · line 135
Quoted 4 lines
arch/arm/configs/exynos_defconfig +1 -0
@@ -132,3 +132,4 @@ CONFIG_DEBUG_INFO=y
  CONFIG_DEBUG_USER=y
  CONFIG_CRYPTO_SHA256=y
  CONFIG_CRC_CCITT=y
+CONFIG_CPU_IDLE=y
Sorry but I have to NAK this change. There are three issues with AFTR mode that should be resolved first before this patch can be merged:
Quoted 5 lines
* The upstream Exynos cpuidle code lacks support for secure firmware and it is used on i.e. Trats2 boards (Exynos4412). Attempts to use AFTR on such hardware results in oops + lockup.
I have patches adding secure firmware support to Exynos cpuidle driver but they depend on Tomasz Figa's PM changes which are not yet upstream.
Ok.
Quoted 3 lines
* Somebody needs to verify that the current Exynos cpuidle driver works in the AFTR mode on Exynos5420 for which support has been added recently (I really doubt it looking at some internal trees).
Rajeshwari (cc'ed) is working on creating a big.Little driver for this board, so it will be a separate cpuidle driver.
Quoted 14 lines
* Some of our u-boot bootloader versions are incompatible with AFTR. We have observed the problem happening on Trats board (Exynos4210) on which it can be fixed by using the upstream u-boot version and Universal C210 board (Exynos4210 with broken SMP support) on which there is no upstream u-boot available (IIRC) and because of the broken SMP support enabling cpuidle results in attempt to enter AFTR during boot + immediate lockup.
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem. Namely there should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.

Quoted 7 lines

Quoted 3 lines
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..
Ok.
Kukjin, is it possible you merge patches 1->16 ?

It is OK given that patch #17 is fixed and merged at the same time (preferably it should go before patches #1-16 to preserve bisectability). Alternatively 'default y' should be removed from patch #16 before the merge. We really don't want to have EXYNOS cpuidle driver enabled by default yet.

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

Olof Johansson

2014-04-07 16:26 UTC #39 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 62 lines · Bartlomiej Zolnierkiewicz

Quoted 61 lines · Daniel Lezcano
Quoted 25 lines · Bartlomiej Zolnierkiewicz
Quoted 15 lines · Daniel Lezcano
The cpuidle driver is broken since v3.11 and now we are at v3.14.
Default the cpuidle driver to favorize a better detection next time.
Signed-off-by Daniel Lezcano <[email protected]>
Reviewed-by Viresh Kumar <[email protected]>
---
  arch/arm/configs/exynos_defconfig |    1 +
  1 file changed, 1 insertion(+)
arch/arm/configs/exynos_defconfig +0 -0
index 4ce7b70..6ed4b34 100644
arch/arm/configs/exynos_defconfig CONFIG_DEBUG_INFO=y · line 135
Quoted 4 lines
arch/arm/configs/exynos_defconfig +1 -0
@@ -132,3 +132,4 @@ CONFIG_DEBUG_INFO=y
  CONFIG_DEBUG_USER=y
  CONFIG_CRYPTO_SHA256=y
  CONFIG_CRC_CCITT=y
+CONFIG_CPU_IDLE=y
Sorry but I have to NAK this change. There are three issues with AFTR mode that should be resolved first before this patch can be merged:
Quoted 5 lines
* The upstream Exynos cpuidle code lacks support for secure firmware and it is used on i.e. Trats2 boards (Exynos4412). Attempts to use AFTR on such hardware results in oops + lockup.
I have patches adding secure firmware support to Exynos cpuidle driver but they depend on Tomasz Figa's PM changes which are not yet upstream.
Ok.
Quoted 3 lines
* Somebody needs to verify that the current Exynos cpuidle driver works in the AFTR mode on Exynos5420 for which support has been added recently (I really doubt it looking at some internal trees).
Rajeshwari (cc'ed) is working on creating a big.Little driver for this board, so it will be a separate cpuidle driver.
Quoted 14 lines
* Some of our u-boot bootloader versions are incompatible with AFTR. We have observed the problem happening on Trats board (Exynos4210) on which it can be fixed by using the upstream u-boot version and Universal C210 board (Exynos4210 with broken SMP support) on which there is no upstream u-boot available (IIRC) and because of the broken SMP support enabling cpuidle results in attempt to enter AFTR during boot + immediate lockup.
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem. Namely there should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.
Quoted 3 lines
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..
Ok.

Quoted 7 lines

Quoted 1 line
Kukjin, is it possible you merge patches 1->16 ?
It is OK given that patch #17 is fixed and merged at the same time (preferably it should go before patches #1-16 to preserve bisectability). Alternatively 'default y' should be removed from patch #16 before the merge. We really don't want to have EXYNOS cpuidle driver enabled by default yet.

Default y is almost always the wrong thing to use, so that should be taken out no matter what.

-Olof

Olof Johansson

2014-04-07 16:28 UTC #40 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

In reply to Bartlomiej Zolnierkiewicz

On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:

[...]

Quoted 3 lines

I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem.

Yes, we need to find a way forward without you guys holding the whole platform hostage with out-of-tree code (here and in general, since I think there are more areas in which this applies).

Quoted 8 lines

Namely there
should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..

Can the code be refactored such that even if CPU_IDLE is on, it won't actually do anything useful on the platforms that have problems above? I.e. determined at runtime, not build time?

(I have not yet looked at the rest of the series).

-Olof

Bartlomiej Zolnierkiewicz

2014-04-07 17:44 UTC #41 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Hi,

Quoted 7 lines · Olof Johansson

On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
[...]
Quoted 3 lines
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem.

On the second look at the cpuidle code there is a "cpuidle.off" kernel parameter which should be sufficient to workaround this particular issue for now.

Quoted 3 lines

Yes, we need to find a way forward without you guys holding the whole platform hostage with out-of-tree code (here and in general, since I think there are more areas in which this applies).

Please explain this more because I really don't know what you're meaning here (at least in case of SRPOL I feel that there are no such issues).

In this particular case we have a problem with a modified uboot bootloader versions being broken and incompatible with the advanced AFTR cpuidle mode. This is not the only / main issue preventing AFTR mode and thus EXYNOS cpuidle driver from being used by default so I really think that the your comment was unfair.

Quoted 5 lines

Quoted 5 lines
Namely there
should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.

Quoted 7 lines

Quoted 3 lines
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..
Can the code be refactored such that even if CPU_IDLE is on, it won't actually do anything useful on the platforms that have problems above? I.e. determined at runtime, not build time?

We can disable AFTR mode by default on EXYNOS4x12 SoCs with secure mode enabled and EXYNOS5420 SoCs (I would like somebody with the hardware to verify that AFTR mode is not working first so we have 100% certainty that we don't regress here).

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

Olof Johansson

2014-04-07 18:33 UTC #42 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 20 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 7 lines · Olof Johansson
On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
[...]
Quoted 3 lines
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem.
On the second look at the cpuidle code there is a "cpuidle.off" kernel parameter which should be sufficient to workaround this particular issue for now.
Quoted 3 lines
Yes, we need to find a way forward without you guys holding the whole platform hostage with out-of-tree code (here and in general, since I think there are more areas in which this applies).

Quoted 8 lines

Please explain this more because I really don't know what you're meaning here (at least in case of SRPOL I feel that there are no such issues).
In this particular case we have a problem with a modified uboot bootloader versions being broken and incompatible with the advanced AFTR cpuidle mode. This is not the only / main issue preventing AFTR mode and thus EXYNOS cpuidle driver from being used by default so I really think that the your comment was unfair.

Holding off features for users of the platform because your firmware is too old (and you can't control the feature per platform) is going to get harder and harder, so being able to enable these kind of things at runtime will be important. I.e. as features go in, it's something that needs to be considered (runtime checking vs ifdef).

Calling that hostage taking? Yeah, maybe a little on the harsh side. But the problem definitely exists.

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Namely there
should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..

Quoted 8 lines

Quoted 3 lines
Can the code be refactored such that even if CPU_IDLE is on, it won't actually do anything useful on the platforms that have problems above? I.e. determined at runtime, not build time?
We can disable AFTR mode by default on EXYNOS4x12 SoCs with secure mode enabled and EXYNOS5420 SoCs (I would like somebody with the hardware to verify that AFTR mode is not working first so we have 100% certainty that we don't regress here).

Does 5420 even work upstream? I have hardware access but nothing that will run an upstream kernel as far as I know. Maybe SLSI has an SMDK they can help out with?

-Olof

Sachin Kamat

2014-04-08 03:57 UTC #43 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 6 lines · Olof Johansson

Quoted 4 lines · Bartlomiej Zolnierkiewicz
Hi,
On Monday, April 07, 2014 09:28:39 AM Olof Johansson wrote:

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
[...]
Does 5420 even work upstream? I have hardware access but nothing that will run an upstream kernel as far as I know. Maybe SLSI has an SMDK they can help out with?

By working upstream if you mean being bootable, then yes I am able to boot both SMDK and Arndale Octa boards based on 5420 with the current upstream kernel. CPU idle however doesn't work though. I get a crash when I hot plug out all secondary CPUs on SMDK.

Signature · 2 lines

With warm regards,
Sachin

Bartlomiej Zolnierkiewicz

2014-04-08 09:40 UTC #44 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

Quoted 12 lines · Sachin Kamat

Quoted 11 lines · Olof Johansson
Quoted 8 lines · Bartlomiej Zolnierkiewicz
Hi,
Quoted 4 lines · Olof Johansson
On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
[...]

Quoted 8 lines

Quoted 3 lines
Does 5420 even work upstream? I have hardware access but nothing that will run an upstream kernel as far as I know. Maybe SLSI has an SMDK they can help out with?
By working upstream if you mean being bootable, then yes I am able to boot both SMDK and Arndale Octa boards based on 5420 with the current upstream kernel. CPU idle however doesn't work though. I get a crash when I hot plug out all secondary CPUs on SMDK.

Thanks for testing. This behavior is an expected one given the current code (=> we need to disable AFTR on 5420 for now).

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

Bartlomiej Zolnierkiewicz

2014-04-08 09:26 UTC #45 lore raw

PATCH v2 17/17 ARM: exynos: config: Enable cpuidle

In reply to Olof Johansson

Quoted 31 lines · Olof Johansson

Quoted 20 lines · Bartlomiej Zolnierkiewicz
Hi,
Quoted 7 lines · Olof Johansson
On Fri, Apr 4, 2014 at 9:09 AM, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
[...]
Quoted 3 lines
I know that this is mainly our problem but the issue is widespread on our targets and I believe that adding some workaround for it in cpuidle core would be beneficial for the whole cpuidle subsystem.
On the second look at the cpuidle code there is a "cpuidle.off" kernel parameter which should be sufficient to workaround this particular issue for now.
Quoted 3 lines
Yes, we need to find a way forward without you guys holding the whole platform hostage with out-of-tree code (here and in general, since I think there are more areas in which this applies).
Quoted 8 lines
Please explain this more because I really don't know what you're meaning here (at least in case of SRPOL I feel that there are no such issues).
In this particular case we have a problem with a modified uboot bootloader versions being broken and incompatible with the advanced AFTR cpuidle mode. This is not the only / main issue preventing AFTR mode and thus EXYNOS cpuidle driver from being used by default so I really think that the your comment was unfair.

Quoted 5 lines

Holding off features for users of the platform because your firmware is too old (and you can't control the feature per platform) is going to get harder and harder, so being able to enable these kind of things at runtime will be important. I.e. as features go in, it's something that needs to be considered (runtime checking vs ifdef).

In this particular case runtime detection of broken uboot versions is not possible, otherwise the issue would have been addressed a long time ago.

Anyway this is not the main problem here and we can deal with it on our side (we will just use "cpuidle.off" on our affected targets).

Quoted 2 lines

Calling that hostage taking? Yeah, maybe a little on the harsh side. But the problem definitely exists.

I don't agree that there is some kind of general problem on our side and you've not provided any specific issues that would fall under this category.

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
Namely there
should be some way of telling cpuidle subsystem to either disable particular state(s) or limit the max available state. I think that this can be also useful for testing and development of other cpuidle drivers.
For now please change this patch to add CONFIG_CPU_IDLE=n instead (since the config option is "default y" it will be auto-enabled if there is no entry in the defconfig)..
Can the code be refactored such that even if CPU_IDLE is on, it won't actually do anything useful on the platforms that have problems above? I.e. determined at runtime, not build time?

Quoted 8 lines

Quoted 4 lines
We can disable AFTR mode by default on EXYNOS4x12 SoCs with secure mode enabled and EXYNOS5420 SoCs (I would like somebody with the hardware to verify that AFTR mode is not working first so we have 100% certainty that we don't regress here).
Does 5420 even work upstream? I have hardware access but nothing that will run an upstream kernel as far as I know. Maybe SLSI has an SMDK they can help out with?

I don't know the current status of 5420. The initial support was added by Linaro (I added Chander and Sachin to cc:).

If the upstream support is working to check the AFTR mode one needs to enable cpuidle driver (CONFIG_CPU_IDLE=y) and offline all CPUs except CPU0 (through sysfs using "echo 0 > /sys/devices/system/cpu/cpuX/online").

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