diff mbox

[13/16] cpuidle: mvebu: Move the description of the cpuidle states in the platform part

Message ID 1403875377-940-14-git-send-email-gregory.clement@free-electrons.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Gregory CLEMENT June 27, 2014, 1:22 p.m. UTC 
In order to prepare the add of new SoCs supports for this cpuidle
driver, this patch moves the description of the state in the platform
part. Indeed the number of the cpuidle state, and the value of the
flag used will vary depending of the SoC.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c         | 32 +++++++++++++++++++++++--
 drivers/cpuidle/cpuidle-mvebu-v7.c | 49 +++++++++++++-------------------------
 include/linux/mvebu-v7-cpuidle.h   | 24 +++++++++++++++++++
 3 files changed, 70 insertions(+), 35 deletions(-)
 create mode 100644 include/linux/mvebu-v7-cpuidle.h

Comments

Thomas Petazzoni June 30, 2014, 1:32 p.m. UTC | #1 
Dear Gregory CLEMENT,

On Fri, 27 Jun 2014 15:22:54 +0200, Gregory CLEMENT wrote:
> In order to prepare the add of new SoCs supports for this cpuidle

"the add of new SoCs supports for this cpuidle" -> "the addition of the
support for more SoCs in this cpuidle".

> driver, this patch moves the description of the state in the platform
> part. Indeed the number of the cpuidle state, and the value of the
> flag used will vary depending of the SoC.

I actually don't really agree with the reasoning here. We can perfectly
fine have several separate compatible strings, one for each SoC. We do
this for pinctrl, for clocks, and for many other drivers. Why should it
be different for the cpuidle driver? I know the function to enter idle
will most likely implemented in arch/arm/mach-mvebu/ because those
functions are generally too tightly coupled with a bunch of system
registers changes, but the description of the different idle states can
just as well be inside the cpuidle driver itself.

Thanks,

Thomas
Gregory CLEMENT July 3, 2014, 1:23 p.m. UTC | #2 
Hi Thomas,

>> In order to prepare the add of new SoCs supports for this cpuidle
> 
> "the add of new SoCs supports for this cpuidle" -> "the addition of the
> support for more SoCs in this cpuidle".
> 
>> driver, this patch moves the description of the state in the platform
>> part. Indeed the number of the cpuidle state, and the value of the
>> flag used will vary depending of the SoC.
> 
> I actually don't really agree with the reasoning here. We can perfectly
> fine have several separate compatible strings, one for each SoC. We do
> this for pinctrl, for clocks, and for many other drivers. Why should it
> be different for the cpuidle driver? I know the function to enter idle
> will most likely implemented in arch/arm/mach-mvebu/ because those
> functions are generally too tightly coupled with a bunch of system
> registers changes, but the description of the different idle states can
> just as well be inside the cpuidle driver itself.

So that would means a different probe and driver name for each SOC?
Indeed it is doable. With my solution adding the support for a new Soc
won't modify the cpu idle driver itself, so the merge would be easier.
But I admit that it is a misuse of the mach- directory.


Thanks,

Gregory
diff mbox

Patch

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 00ebd5638633..9396839e162e 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -19,15 +19,18 @@ 
 #define pr_fmt(fmt) "mvebu-pmsu: " fmt
 
 #include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/mbus.h>
+#include <linux/mvebu-v7-cpuidle.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
 #include <linux/smp.h>
 #include <asm/cacheflush.h>
+#include <asm/cpuidle.h>
 #include <asm/cp15.h>
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
@@ -299,17 +302,42 @@  static struct notifier_block mvebu_v7_cpu_pm_notifier = {
 
 static bool (*mvebu_v7_cpu_idle_init)(void);
 
+static struct mvebu_v7_cpuidle armada_xp_cpuidle = {
+	.mvebu_v7_idle_driver = {
+		.name			= "armada_xp_idle",
+		.states[0]		= ARM_CPUIDLE_WFI_STATE,
+		.states[1]		= {
+			.exit_latency		= 10,
+			.power_usage		= 50,
+			.target_residency	= 100,
+			.flags			= CPUIDLE_FLAG_TIME_VALID,
+			.name			= "Idle",
+			.desc			= "CPU power down",
+		},
+		.states[2]		= {
+			.exit_latency		= 100,
+			.power_usage		= 5,
+			.target_residency	= 1000,
+			.flags			= CPUIDLE_FLAG_TIME_VALID |
+			MVEBU_V7_FLAG_DEEP_IDLE,
+			.name			= "Deep idle",
+			.desc			= "CPU and L2 Fabric power down",
+		},
+		.state_count = 3,
+	},
+	.mvebu_v7_cpu_suspend = armada_xp_370_cpu_suspend,
+};
+
 static __init bool armada_xp_cpuidle_init(void)
 {
 	struct device_node *np;
-
 	np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric");
 	if (!np)
 		return false;
 	of_node_put(np);
 
 	mvebu_cpu_resume = armada_370_xp_cpu_resume;
-	mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend;
+	mvebu_v7_cpuidle_device.dev.platform_data = &armada_xp_cpuidle;
 	return true;
 }
 
diff --git a/drivers/cpuidle/cpuidle-mvebu-v7.c b/drivers/cpuidle/cpuidle-mvebu-v7.c
index 6066e0d9aabe..2fdc18797c0e 100644
--- a/drivers/cpuidle/cpuidle-mvebu-v7.c
+++ b/drivers/cpuidle/cpuidle-mvebu-v7.c
@@ -16,15 +16,12 @@ 
 #include <linux/cpu_pm.h>
 #include <linux/cpuidle.h>
 #include <linux/module.h>
+#include <linux/mvebu-v7-cpuidle.h>
 #include <linux/of.h>
 #include <linux/suspend.h>
 #include <linux/platform_device.h>
-#include <asm/cpuidle.h>
 
-#define MVEBU_V7_MAX_STATES	3
-#define MVEBU_V7_FLAG_DEEP_IDLE	0x10000
-
-static int (*mvebu_v7_cpu_suspend)(int);
+static struct mvebu_v7_cpuidle *pcpuidle;
 
 static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
@@ -32,12 +29,13 @@  static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 {
 	int ret;
 	bool deepidle = false;
+
 	cpu_pm_enter();
 
 	if (drv->states[index].flags & MVEBU_V7_FLAG_DEEP_IDLE)
 		deepidle = true;
 
-	ret = mvebu_v7_cpu_suspend(deepidle);
+	ret = pcpuidle->mvebu_v7_cpu_suspend(deepidle);
 	if (ret)
 		return ret;
 
@@ -46,36 +44,21 @@  static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 	return index;
 }
 
-static struct cpuidle_driver mvebu_v7_idle_driver = {
-	.name			= "mvebu_v7_idle",
-	.states[0]		= ARM_CPUIDLE_WFI_STATE,
-	.states[1]		= {
-		.enter			= mvebu_v7_enter_idle,
-		.exit_latency		= 10,
-		.power_usage		= 50,
-		.target_residency	= 100,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "Idle",
-		.desc			= "CPU power down",
-	},
-	.states[2]		= {
-		.enter			= mvebu_v7_enter_idle,
-		.exit_latency		= 100,
-		.power_usage		= 5,
-		.target_residency	= 1000,
-		.flags			= CPUIDLE_FLAG_TIME_VALID |
-						MVEBU_V7_FLAG_DEEP_IDLE,
-		.name			= "Deep idle",
-		.desc			= "CPU and L2 Fabric power down",
-	},
-	.state_count = MVEBU_V7_MAX_STATES,
-};
-
 static int mvebu_v7_cpuidle_probe(struct platform_device *pdev)
 {
+	int i;
+
+	pcpuidle = (void *)(pdev->dev.platform_data);
+
+	/*
+	 * The first state is the ARM WFI state, so we don't have to
+	 * provide an enter function
+	 */
+	for (i = 1; i < pcpuidle->mvebu_v7_idle_driver.state_count; i++)
+		pcpuidle->mvebu_v7_idle_driver.states[i].enter =
+			mvebu_v7_enter_idle;
 
-	mvebu_v7_cpu_suspend = (void *)(pdev->dev.platform_data);
-	return cpuidle_register(&mvebu_v7_idle_driver, NULL);
+	return cpuidle_register(&pcpuidle->mvebu_v7_idle_driver, NULL);
 }
 
 static struct platform_driver mvebu_v7_cpuidle_plat_driver = {
diff --git a/include/linux/mvebu-v7-cpuidle.h b/include/linux/mvebu-v7-cpuidle.h
new file mode 100644
index 000000000000..28c8674292ad
--- /dev/null
+++ b/include/linux/mvebu-v7-cpuidle.h
@@ -0,0 +1,24 @@ 
+/*
+ * Marvell EBU cpuidle defintion
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ */
+
+#ifndef __LINUX_MVEBU_V7_CPUIDLE_H__
+#define __LINUX_MVEBU_V7_CPUIDLE_H__
+
+#define MVEBU_V7_FLAG_DEEP_IDLE	0x10000
+
+struct mvebu_v7_cpuidle {
+	struct cpuidle_driver mvebu_v7_idle_driver;
+	int (*mvebu_v7_cpu_suspend)(unsigned long);
+};
+
+#endif