ARM: S3C64xx: cpuidle cleanups

PATCH New Daniel Lezcano 3 participants 8 messages May 14, 2012 – May 19, 2012

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm Samsung Exynos

lore

Share & tools

lore

Daniel Lezcano

author

2012-05-14 14:06 UTC #0 lore raw

cover ARM: S3C64xx: cpuidle cleanups

These couple of patches use the new cpuidle core api to refactor some part of the code. The first one declares the states directly in the driver declaration and the second one use the timekeeping flag to let the cpuidle core to compute the idle time.

I don't have this board, I was not able to test these patches.

Daniel Lezcano (2):
ARM: s3c64xx: cpuidle - declare the states with the new api
ARM: s3c64xx: cpuidle - use timekeeping wrapper

 arch/arm/mach-s3c64xx/cpuidle.c |   45 +++++++++++++--------------------------
 1 files changed, 15 insertions(+), 30 deletions(-)

Signature · 1 line

1.7.5.4

Daniel Lezcano

author

2012-05-14 14:06 UTC #1 lore raw

PATCH 1/2 ARM: s3c64xx: cpuidle - declare the states with the new api

The states are now part of the cpuidle_driver structure, so we can declare the states in this structure directly. That saves us an extra variable declaration and a memcpy.

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

---
 arch/arm/mach-s3c64xx/cpuidle.c |   33 ++++++++++++++-------------------
 1 files changed, 14 insertions(+), 19 deletions(-)

arch/arm/mach-s3c64xx/cpuidle.c +14 ~~-19~~

index 179460f..2750e54 100644
@@ -51,33 +51,28 @@ static int s3c64xx_enter_idle(struct cpuidle_device *dev,
 	return index;
 }
 
-static struct cpuidle_state s3c64xx_cpuidle_set[] = {
-	[0] = {
-		.enter			= s3c64xx_enter_idle,
-		.exit_latency		= 1,
-		.target_residency	= 1,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "IDLE",
-		.desc			= "System active, ARM gated",
-	},
-};
+static DEFINE_PER_CPU(struct cpuidle_device, s3c64xx_cpuidle_device);
 
 static struct cpuidle_driver s3c64xx_cpuidle_driver = {
-	.name		= "s3c64xx_cpuidle",
-	.owner		= THIS_MODULE,
-	.state_count	= ARRAY_SIZE(s3c64xx_cpuidle_set),
-};
-
-static struct cpuidle_device s3c64xx_cpuidle_device = {
-	.state_count	= ARRAY_SIZE(s3c64xx_cpuidle_set),
+	.name	= "s3c64xx_cpuidle",
+	.owner  = THIS_MODULE,
+	.states = {
+		{
+			.enter            = s3c64xx_enter_idle,
+			.exit_latency     = 1,
+			.target_residency = 1,
+			.flags            = CPUIDLE_FLAG_TIME_VALID,
+			.name             = "IDLE",
+			.desc             = "System active, ARM gated",
+		},
+	},
+	.state_count = 1,
 };
 
 static int __init s3c64xx_init_cpuidle(void)
 {
 	int ret;
 
-	memcpy(s3c64xx_cpuidle_driver.states, s3c64xx_cpuidle_set,
-	       sizeof(s3c64xx_cpuidle_set));
 	cpuidle_register_driver(&s3c64xx_cpuidle_driver);
 
 	ret = cpuidle_register_device(&s3c64xx_cpuidle_device);

Signature · 1 line

1.7.5.4

Kukjin Kim

2012-05-17 07:02 UTC #2 lore raw

PATCH 1/2 ARM: s3c64xx: cpuidle - declare the states with the new api

Quoted 32 lines · Daniel Lezcano

The states are now part of the cpuidle_driver structure, so we can declare the states in this structure directly. That saves us an extra variable declaration and a memcpy.
Signed-off-by Daniel Lezcano <[email protected]>
---
 arch/arm/mach-s3c64xx/cpuidle.c |   33 ++++++++++++++-------------------
 1 files changed, 14 insertions(+), 19 deletions(-)
arch/arm/mach- renamed from arch/arm/mach-s3c64xx/cpuidle.c +0 -0
s3c64xx/cpuidle.c
index 179460f..2750e54 100644
arch/arm/mach-s3c64xx/cpuidle.c +1 ~~-10~~
@@ -51,33 +51,28 @@ static int s3c64xx_enter_idle(struct cpuidle_device
 *dev,
 	return index;
 }
 
-static struct cpuidle_state s3c64xx_cpuidle_set[] = {
-	[0] = {
-		.enter			= s3c64xx_enter_idle,
-		.exit_latency		= 1,
-		.target_residency	= 1,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "IDLE",
-		.desc			= "System active, ARM gated",
-	},
-};
+static DEFINE_PER_CPU(struct cpuidle_device, s3c64xx_cpuidle_device);
 

arch/arm/mach-s3c64xx/cpuidle.c static int s3c64xx_enter_idle(struct cpuidle_device · line 59

Quoted 10 lines

arch/arm/mach-s3c64xx/cpuidle.c +2 -7

 static struct cpuidle_driver s3c64xx_cpuidle_driver = {
-	.name		= "s3c64xx_cpuidle",
-	.owner		= THIS_MODULE,
-	.state_count	= ARRAY_SIZE(s3c64xx_cpuidle_set),
-};
-
-static struct cpuidle_device s3c64xx_cpuidle_device = {
-	.state_count	= ARRAY_SIZE(s3c64xx_cpuidle_set),
+	.name	= "s3c64xx_cpuidle",
+	.owner  = THIS_MODULE,

I'd preferred to use original 'tab'

Quoted 21 lines

arch/arm/mach-s3c64xx/cpuidle.c +11 -2

+	.states = {
+		{
+			.enter            = s3c64xx_enter_idle,
+			.exit_latency     = 1,
+			.target_residency = 1,
+			.flags            = CPUIDLE_FLAG_TIME_VALID,
+			.name             = "IDLE",
+			.desc             = "System active, ARM gated",
+		},
+	},
+	.state_count = 1,
 };
 
 static int __init s3c64xx_init_cpuidle(void)
 {
 	int ret;
 
-	memcpy(s3c64xx_cpuidle_driver.states, s3c64xx_cpuidle_set,
-	       sizeof(s3c64xx_cpuidle_set));
 	cpuidle_register_driver(&s3c64xx_cpuidle_driver);

Quoted 3 lines

ret = cpuidle_register_device(&s3c64xx_cpuidle_device);
--
1.7.5.4

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Mark Brown

2012-05-17 12:28 UTC #3 lore raw

PATCH 1/2 ARM: s3c64xx: cpuidle - declare the states with the new api

In reply to Daniel Lezcano

Quoted 3 lines · Daniel Lezcano

The states are now part of the cpuidle_driver structure, so we can declare the states in this structure directly. That saves us an extra variable declaration and a memcpy.

Tested-by Mark Brown <[email protected]>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120517/5806f7b3/attachment.sig>

Kukjin Kim

2012-05-19 17:35 UTC #4 lore raw

PATCH 1/2 ARM: s3c64xx: cpuidle - declare the states with the new api

Quoted 6 lines · Mark Brown

Quoted 3 lines · Daniel Lezcano
The states are now part of the cpuidle_driver structure, so we can declare the states in this structure directly. That saves us an extra variable declaration and a memcpy.
Tested-by Mark Brown<[email protected]>

Thanks for testing, applied this series.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Daniel Lezcano

author

2012-05-14 14:06 UTC #5 lore raw

PATCH 2/2 ARM: s3c64xx: cpuidle - use timekeeping wrapper

In reply to Daniel Lezcano

The timekeeping is computed from the cpuidle core if we set the .en_core_tk_irqen flag. Let's use it and remove the duplicated code.

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

---
 arch/arm/mach-s3c64xx/cpuidle.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

arch/arm/mach-s3c64xx/cpuidle.c +1 ~~-11~~

index 2750e54..acb197c 100644
@@ -27,12 +27,7 @@ static int s3c64xx_enter_idle(struct cpuidle_device *dev,
 			      struct cpuidle_driver *drv,
 			      int index)
 {
-	struct timeval before, after;
 	unsigned long tmp;
-	int idle_time;
-
-	local_irq_disable();
-	do_gettimeofday(&before);
 
 	/* Setup PWRCFG to enter idle mode */
 	tmp = __raw_readl(S3C64XX_PWR_CFG);

@@ -42,12 +37,6 @@ static int s3c64xx_enter_idle(struct cpuidle_device *dev,
 
 	cpu_do_idle();
 
-	do_gettimeofday(&after);
-	local_irq_enable();
-	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
-		    (after.tv_usec - before.tv_usec);
-
-	dev->last_residency = idle_time;
 	return index;
 }

@@ -56,6 +45,7 @@ static DEFINE_PER_CPU(struct cpuidle_device, s3c64xx_cpuidle_device);
 static struct cpuidle_driver s3c64xx_cpuidle_driver = {
 	.name	= "s3c64xx_cpuidle",
 	.owner  = THIS_MODULE,
+	.en_core_tk_irqen = 1,
 	.states = {
 		{
 			.enter            = s3c64xx_enter_idle,

Signature · 1 line

1.7.5.4

Mark Brown

2012-05-17 12:28 UTC #6 lore raw

PATCH 2/2 ARM: s3c64xx: cpuidle - use timekeeping wrapper

Quoted 3 lines · Daniel Lezcano

The timekeeping is computed from the cpuidle core if we set the .en_core_tk_irqen flag. Let's use it and remove the duplicated code.

Tested-by Mark Brown <[email protected]>

Kukjin Kim

2012-05-17 06:57 UTC #7 lore raw

cover ARM: S3C64xx: cpuidle cleanups

In reply to Daniel Lezcano

Quoted 6 lines · Daniel Lezcano

These couple of patches use the new cpuidle core api to refactor some part of the code. The first one declares the states directly in the driver declaration and the second one use the timekeeping flag to let the cpuidle core to compute the idle time.

Basically, looks ok to me.

Quoted 2 lines

I don't have this board, I was not able to test these patches.

Mark, could you please check this patches on your board? Now, my smdk6410 has some problem to test this :(

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Quoted 5 lines

Daniel Lezcano (2):
ARM: s3c64xx: cpuidle - declare the states with the new api
ARM: s3c64xx: cpuidle - use timekeeping wrapper
 arch/arm/mach-s3c64xx/cpuidle.c |   45

+++++++++++++----------------------

Quoted 5 lines

----

 1 files changed, 15 insertions(+), 30 deletions(-)

--
1.7.5.4