diff mbox

[PATCHv2,12/17] cpuidle: mvebu: make the cpuidle driver capable of handling multiple SoCs

Message ID 1404913221-17343-13-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Thomas Petazzoni July 9, 2014, 1:40 p.m. UTC
From: Gregory CLEMENT <gregory.clement@free-electrons.com>

In order to prepare the add of new SoCs supports for this cpuidle
driver, this patch extends the platform_data understood by the
cpuidle-mvebu-v7 driver to contain a "type" identifying which specific
SoC the cpuidle driver is being probed for. It will be used by the
cpuidle driver to know the list of states for the current SoC.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/pmsu.c         | 20 +++++++++++++-------
 drivers/cpuidle/cpuidle-mvebu-v7.c | 32 ++++++++++++++++++++------------
 include/linux/mvebu-v7-cpuidle.h   | 26 ++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 19 deletions(-)
 create mode 100644 include/linux/mvebu-v7-cpuidle.h

Comments

Daniel Lezcano July 21, 2014, 10:59 a.m. UTC | #1
On 07/09/2014 03:40 PM, Thomas Petazzoni wrote:
> From: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> In order to prepare the add of new SoCs supports for this cpuidle
> driver, this patch extends the platform_data understood by the
> cpuidle-mvebu-v7 driver to contain a "type" identifying which specific
> SoC the cpuidle driver is being probed for. It will be used by the
> cpuidle driver to know the list of states for the current SoC.

It makes more sense to use/implement a 'soc_is_xxx' macro or 
'of_machine_is_compatible', like the other cpuidle drivers, no ?

Is there a good reason to implement a new way to check the board ?

It isn't possible to do:

if (of_machine_is_compatible("marvell,armada-370-xp-pmsu"))
	cpuidle_register(&armadaxp_cpuidle_driver, NULL);

?

That will prevent the creation of the new single-declaration header file.
	


> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>   arch/arm/mach-mvebu/pmsu.c         | 20 +++++++++++++-------
>   drivers/cpuidle/cpuidle-mvebu-v7.c | 32 ++++++++++++++++++++------------
>   include/linux/mvebu-v7-cpuidle.h   | 26 ++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+), 19 deletions(-)
>   create mode 100644 include/linux/mvebu-v7-cpuidle.h
>
> diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> index 802edc8..0a24073 100644
> --- a/arch/arm/mach-mvebu/pmsu.c
> +++ b/arch/arm/mach-mvebu/pmsu.c
> @@ -19,10 +19,12 @@
>   #define pr_fmt(fmt) "mvebu-pmsu: " fmt
>
>   #include <linux/cpu_pm.h>
> +#include <linux/cpuidle.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
>   #include <linux/kernel.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>
> @@ -75,10 +77,6 @@ extern void armada_370_xp_cpu_resume(void);
>
>   static void *mvebu_cpu_resume;
>
> -static struct platform_device mvebu_v7_cpuidle_device = {
> -	.name = "cpuidle-mvebu-v7",
> -};
> -
>   static struct of_device_id of_pmsu_table[] = {
>   	{ .compatible = "marvell,armada-370-pmsu", },
>   	{ .compatible = "marvell,armada-370-xp-pmsu", },
> @@ -325,17 +323,25 @@ static struct notifier_block mvebu_v7_cpu_pm_notifier = {
>   	.notifier_call = mvebu_v7_cpu_pm_notify,
>   };
>
> -static int __init armada_xp_cpuidle_init(void)
> +static struct mvebu_v7_cpuidle armada_xp_cpuidle = {
> +	.type = CPUIDLE_ARMADA_XP,
> +	.cpu_suspend = armada_370_xp_cpu_suspend,
> +};
> +
> +static struct platform_device mvebu_v7_cpuidle_device = {
> +	.name = "cpuidle-mvebu-v7",
> +};
> +
> +static __init int armada_xp_cpuidle_init(void)
>   {
>   	struct device_node *np;
> -
>   	np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric");
>   	if (!np)
>   		return -ENODEV;
>   	of_node_put(np);
>
>   	mvebu_cpu_resume = armada_370_xp_cpu_resume;
> -	mvebu_v7_cpuidle_device.dev.platform_data = armada_370_xp_cpu_suspend;
> +	mvebu_v7_cpuidle_device.dev.platform_data = &armada_xp_cpuidle;
>
>   	return 0;
>   }
> diff --git a/drivers/cpuidle/cpuidle-mvebu-v7.c b/drivers/cpuidle/cpuidle-mvebu-v7.c
> index 302596e..82c545bb 100644
> --- a/drivers/cpuidle/cpuidle-mvebu-v7.c
> +++ b/drivers/cpuidle/cpuidle-mvebu-v7.c
> @@ -16,15 +16,15 @@
>   #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 +32,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->cpu_suspend(deepidle);
>   	if (ret)
>   		return ret;
>
> @@ -46,8 +47,8 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
>   	return index;
>   }
>
> -static struct cpuidle_driver mvebu_v7_idle_driver = {
> -	.name			= "mvebu_v7_idle",
> +static struct cpuidle_driver armadaxp_cpuidle_driver = {
> +	.name			= "armada_xp_idle",
>   	.states[0]		= ARM_CPUIDLE_WFI_STATE,
>   	.states[1]		= {
>   		.enter			= mvebu_v7_enter_idle,
> @@ -55,7 +56,7 @@ static struct cpuidle_driver mvebu_v7_idle_driver = {
>   		.power_usage		= 50,
>   		.target_residency	= 100,
>   		.flags			= CPUIDLE_FLAG_TIME_VALID,
> -		.name			= "MV CPU IDLE",
> +		.name			= "Idle",
>   		.desc			= "CPU power down",
>   	},
>   	.states[2]		= {
> @@ -63,19 +64,26 @@ static struct cpuidle_driver mvebu_v7_idle_driver = {
>   		.exit_latency		= 100,
>   		.power_usage		= 5,
>   		.target_residency	= 1000,
> -		.flags			= CPUIDLE_FLAG_TIME_VALID |
> -						MVEBU_V7_FLAG_DEEP_IDLE,
> -		.name			= "MV CPU DEEP IDLE",
> +		.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,
> +	.state_count = 3,
>   };
>
>   static int mvebu_v7_cpuidle_probe(struct platform_device *pdev)
>   {
> +	struct cpuidle_driver *drv;
> +
> +	pcpuidle = pdev->dev.platform_data;
> +
> +	if (pcpuidle->type == CPUIDLE_ARMADA_XP)
> +		drv = &armadaxp_cpuidle_driver;
> +	else
> +		return -EINVAL;
>
> -	mvebu_v7_cpu_suspend = (void *)(pdev->dev.platform_data);
> -	return cpuidle_register(&mvebu_v7_idle_driver, NULL);
> +	return cpuidle_register(drv, 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 0000000..00fde86
> --- /dev/null
> +++ b/include/linux/mvebu-v7-cpuidle.h
> @@ -0,0 +1,26 @@
> +/*
> + * 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__
> +
> +enum mvebu_v7_cpuidle_types {
> +	CPUIDLE_ARMADA_XP,
> +};
> +
> +struct mvebu_v7_cpuidle {
> +	int type;
> +	int (*cpu_suspend)(unsigned long deepidle);
> +};
> +
> +#endif
>
Thomas Petazzoni July 21, 2014, 11:12 a.m. UTC | #2
Dear Daniel Lezcano,

On Mon, 21 Jul 2014 12:59:33 +0200, Daniel Lezcano wrote:

> > In order to prepare the add of new SoCs supports for this cpuidle
> > driver, this patch extends the platform_data understood by the
> > cpuidle-mvebu-v7 driver to contain a "type" identifying which specific
> > SoC the cpuidle driver is being probed for. It will be used by the
> > cpuidle driver to know the list of states for the current SoC.
> 
> It makes more sense to use/implement a 'soc_is_xxx' macro or 
> 'of_machine_is_compatible', like the other cpuidle drivers, no ?
> 
> Is there a good reason to implement a new way to check the board ?
> 
> It isn't possible to do:
> 
> if (of_machine_is_compatible("marvell,armada-370-xp-pmsu"))
> 	cpuidle_register(&armadaxp_cpuidle_driver, NULL);

So you suggest to have a different cpuidle driver, with a different
name, one for each SoC ?

Or do you suggest to have the cpuidle probe() function call
of_machine_is_compatible() to find out the SoC type?

Thanks,

Thomas
Arnd Bergmann July 21, 2014, 11:16 a.m. UTC | #3
On Monday 21 July 2014 12:59:33 Daniel Lezcano wrote:
> On 07/09/2014 03:40 PM, Thomas Petazzoni wrote:
> > From: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >
> > In order to prepare the add of new SoCs supports for this cpuidle
> > driver, this patch extends the platform_data understood by the
> > cpuidle-mvebu-v7 driver to contain a "type" identifying which specific
> > SoC the cpuidle driver is being probed for. It will be used by the
> > cpuidle driver to know the list of states for the current SoC.
> 
> It makes more sense to use/implement a 'soc_is_xxx' macro or 
> 'of_machine_is_compatible', like the other cpuidle drivers, no ?
>
> Is there a good reason to implement a new way to check the board ?

In all other subsystems, we try to do this per-device: We have almost
killed off all soc_is_* macros, and we strongly discourage anybody
from using of_machine_is_compatible() in driver code, as this goes
against the goals of multiplatform kernels on which we treat every
device as a separate thing that may get reused on any number of
machines or SoCs.

> It isn't possible to do:
> 
> if (of_machine_is_compatible("marvell,armada-370-xp-pmsu"))
>         cpuidle_register(&armadaxp_cpuidle_driver, NULL);
> 
> ?
> 
> That will prevent the creation of the new single-declaration header file.

It would be best to have a way to read a property (or multiple
properties) from DT instead, to identify the requirements of the
device individually. However, I guess that would also require
changing the DT representation in an incompatible way, which we
normally don't.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni July 21, 2014, 11:19 a.m. UTC | #4
Dear Arnd Bergmann,

On Mon, 21 Jul 2014 13:16:22 +0200, Arnd Bergmann wrote:

> > It isn't possible to do:
> > 
> > if (of_machine_is_compatible("marvell,armada-370-xp-pmsu"))
> >         cpuidle_register(&armadaxp_cpuidle_driver, NULL);
> > 
> > ?
> > 
> > That will prevent the creation of the new single-declaration header file.
> 
> It would be best to have a way to read a property (or multiple
> properties) from DT instead, to identify the requirements of the
> device individually. However, I guess that would also require
> changing the DT representation in an incompatible way, which we
> normally don't.

cpuidle is not represented in DT, so besides checking the global
compatible string with of_machine_is_compatible(), or passing data
through platform_data (as proposed in the patch series), I don't really
see how the cpuidle driver could find out which SoC variant is being
used.

Best regards,

Thomas
Arnd Bergmann July 21, 2014, 11:30 a.m. UTC | #5
On Monday 21 July 2014 13:19:39 Thomas Petazzoni wrote:
> On Mon, 21 Jul 2014 13:16:22 +0200, Arnd Bergmann wrote:
> 
> > > It isn't possible to do:
> > > 
> > > if (of_machine_is_compatible("marvell,armada-370-xp-pmsu"))
> > >         cpuidle_register(&armadaxp_cpuidle_driver, NULL);
> > > 
> > > ?
> > > 
> > > That will prevent the creation of the new single-declaration header file.
> > 
> > It would be best to have a way to read a property (or multiple
> > properties) from DT instead, to identify the requirements of the
> > device individually. However, I guess that would also require
> > changing the DT representation in an incompatible way, which we
> > normally don't.
> 
> cpuidle is not represented in DT, so besides checking the global
> compatible string with of_machine_is_compatible(), or passing data
> through platform_data (as proposed in the patch series), I don't really
> see how the cpuidle driver could find out which SoC variant is being
> used.

One way I think it can be done is by looking up the pmsu node
and then looking at some of the properties in there.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni July 21, 2014, 11:35 a.m. UTC | #6
Dear Arnd Bergmann,

On Mon, 21 Jul 2014 13:30:47 +0200, Arnd Bergmann wrote:

> > > It would be best to have a way to read a property (or multiple
> > > properties) from DT instead, to identify the requirements of the
> > > device individually. However, I guess that would also require
> > > changing the DT representation in an incompatible way, which we
> > > normally don't.
> > 
> > cpuidle is not represented in DT, so besides checking the global
> > compatible string with of_machine_is_compatible(), or passing data
> > through platform_data (as proposed in the patch series), I don't really
> > see how the cpuidle driver could find out which SoC variant is being
> > used.
> 
> One way I think it can be done is by looking up the pmsu node
> and then looking at some of the properties in there.

Which properties do you have in mind? Should we simply use different
compatible strings for the PMSU node, per SoC ? Some other suggestions ?

Thanks,

Thomas
Arnd Bergmann July 21, 2014, noon UTC | #7
On Monday 21 July 2014 13:35:34 Thomas Petazzoni wrote:
> On Mon, 21 Jul 2014 13:30:47 +0200, Arnd Bergmann wrote:
> 
> > > > It would be best to have a way to read a property (or multiple
> > > > properties) from DT instead, to identify the requirements of the
> > > > device individually. However, I guess that would also require
> > > > changing the DT representation in an incompatible way, which we
> > > > normally don't.
> > > 
> > > cpuidle is not represented in DT, so besides checking the global
> > > compatible string with of_machine_is_compatible(), or passing data
> > > through platform_data (as proposed in the patch series), I don't really
> > > see how the cpuidle driver could find out which SoC variant is being
> > > used.
> > 
> > One way I think it can be done is by looking up the pmsu node
> > and then looking at some of the properties in there.
> 
> Which properties do you have in mind? Should we simply use different
> compatible strings for the PMSU node, per SoC ? Some other suggestions ?

I don't know, it really depends on what the differences are between
the SoCs, and I haven't looked at them.

Using the compatible strings would make it work best if you have one
driver per variant, and then share some common code, as opposed to
having one shared driver with a number of exceptions.

If the differences are just a few parameters, it might be better
to encode those parameters in DT properties instead.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni July 21, 2014, 12:09 p.m. UTC | #8
Dear Arnd Bergmann,

On Mon, 21 Jul 2014 14:00:22 +0200, Arnd Bergmann wrote:

> I don't know, it really depends on what the differences are between
> the SoCs, and I haven't looked at them.
> 
> Using the compatible strings would make it work best if you have one
> driver per variant, and then share some common code, as opposed to
> having one shared driver with a number of exceptions.
> 
> If the differences are just a few parameters, it might be better
> to encode those parameters in DT properties instead.

The differences are in the cpuidle states that are supported, see
patches "cpuidle: mvebu: add Armada 370 support" and "cpuidle: mvebu:
add Armada 38x support" in the series.

I honestly believe that since cpuidle functionality is not described in
the Device Tree and therefore probed using a statically defined
platform_device, the good way to pass these informations is to simply
use platform_data.

Best regards,

Thomas
Daniel Lezcano July 21, 2014, 12:34 p.m. UTC | #9
On 07/21/2014 02:09 PM, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
>
> On Mon, 21 Jul 2014 14:00:22 +0200, Arnd Bergmann wrote:
>
>> I don't know, it really depends on what the differences are between
>> the SoCs, and I haven't looked at them.
>>
>> Using the compatible strings would make it work best if you have one
>> driver per variant, and then share some common code, as opposed to
>> having one shared driver with a number of exceptions.
>>
>> If the differences are just a few parameters, it might be better
>> to encode those parameters in DT properties instead.
>
> The differences are in the cpuidle states that are supported, see
> patches "cpuidle: mvebu: add Armada 370 support" and "cpuidle: mvebu:
> add Armada 38x support" in the series.
>
> I honestly believe that since cpuidle functionality is not described in
> the Device Tree and therefore probed using a statically defined
> platform_device, the good way to pass these informations is to simply
> use platform_data.

Ok, so for the record the cpuidle functionality described via DT is 
under discussion [1].

I understand you need several drivers for the different SoC because of 
the different latencies.

I admit passing an extra flag via the platform_data is a valid approach 
but I have been unifying the different drivers across the existing SoC 
and there is still a lot of things to do. So accepting this patch brings 
another way to discriminate the SoC variant I would like to avoid.

Due the different latencies, I don't think the DT property is enough and 
that may enter in conflict with Lorenzo's work.

So there are three solutions:

1. Pass the flag through the platform data, I am not really in favor of 
that as mentioned above

2. Use the compatible string like the cpuidle-big-little.c driver, but 
Arnd is not in favor of that

3. Register 3 platform drivers, in cpuidle-mvebu-v7.c, and let the 
registering of the cpuidle's platform device to enable the right one


[1] http://www.spinics.net/lists/arm-kernel/msg341541.html
Thomas Petazzoni July 21, 2014, 12:38 p.m. UTC | #10
Dear Daniel Lezcano,

On Mon, 21 Jul 2014 14:34:23 +0200, Daniel Lezcano wrote:

> So there are three solutions:
> 
> 1. Pass the flag through the platform data, I am not really in favor of 
> that as mentioned above

That's what is already implemented.

> 
> 2. Use the compatible string like the cpuidle-big-little.c driver, but 
> Arnd is not in favor of that
> 
> 3. Register 3 platform drivers, in cpuidle-mvebu-v7.c, and let the 
> registering of the cpuidle's platform device to enable the right one

I'm fine with doing (3). Daniel, Arnd, let me know if that's ok for
you, and I'll respin the patch series accordingly.

Thanks,

Thomas
Arnd Bergmann July 21, 2014, 12:51 p.m. UTC | #11
On Monday 21 July 2014 14:38:13 Thomas Petazzoni wrote:
> Dear Daniel Lezcano,
> 
> On Mon, 21 Jul 2014 14:34:23 +0200, Daniel Lezcano wrote:
> 
> > So there are three solutions:
> > 
> > 1. Pass the flag through the platform data, I am not really in favor of 
> > that as mentioned above
> 
> That's what is already implemented.

It's certainly better than checking a global compatible string in
driver code, IMHO.

Instead of a global header file, I would have put the structure
definition under linux/platform-data, as we do for most other
legacy drivers.

> > 2. Use the compatible string like the cpuidle-big-little.c driver, but 
> > Arnd is not in favor of that
> > 
> > 3. Register 3 platform drivers, in cpuidle-mvebu-v7.c, and let the 
> > registering of the cpuidle's platform device to enable the right one
> 
> I'm fine with doing (3). Daniel, Arnd, let me know if that's ok for
> you, and I'll respin the patch series accordingly.

If I understood Daniel correctly, this is temporary anyway until
we have a proper binding, right?

If so, I don't really mind either approach.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano July 21, 2014, 1:12 p.m. UTC | #12
On 07/21/2014 02:38 PM, Thomas Petazzoni wrote:
> Dear Daniel Lezcano,
>
> On Mon, 21 Jul 2014 14:34:23 +0200, Daniel Lezcano wrote:
>
>> So there are three solutions:
>>
>> 1. Pass the flag through the platform data, I am not really in favor of
>> that as mentioned above
>
> That's what is already implemented.

Yes, that was the point :)

>> 2. Use the compatible string like the cpuidle-big-little.c driver, but
>> Arnd is not in favor of that
>>
>> 3. Register 3 platform drivers, in cpuidle-mvebu-v7.c, and let the
>> registering of the cpuidle's platform device to enable the right one
>
> I'm fine with doing (3). Daniel, Arnd, let me know if that's ok for
> you, and I'll respin the patch series accordingly.

Ok for me.

Thanks Thomas

   -- Daniel
diff mbox

Patch

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 802edc8..0a24073 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -19,10 +19,12 @@ 
 #define pr_fmt(fmt) "mvebu-pmsu: " fmt
 
 #include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.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>
@@ -75,10 +77,6 @@  extern void armada_370_xp_cpu_resume(void);
 
 static void *mvebu_cpu_resume;
 
-static struct platform_device mvebu_v7_cpuidle_device = {
-	.name = "cpuidle-mvebu-v7",
-};
-
 static struct of_device_id of_pmsu_table[] = {
 	{ .compatible = "marvell,armada-370-pmsu", },
 	{ .compatible = "marvell,armada-370-xp-pmsu", },
@@ -325,17 +323,25 @@  static struct notifier_block mvebu_v7_cpu_pm_notifier = {
 	.notifier_call = mvebu_v7_cpu_pm_notify,
 };
 
-static int __init armada_xp_cpuidle_init(void)
+static struct mvebu_v7_cpuidle armada_xp_cpuidle = {
+	.type = CPUIDLE_ARMADA_XP,
+	.cpu_suspend = armada_370_xp_cpu_suspend,
+};
+
+static struct platform_device mvebu_v7_cpuidle_device = {
+	.name = "cpuidle-mvebu-v7",
+};
+
+static __init int armada_xp_cpuidle_init(void)
 {
 	struct device_node *np;
-
 	np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric");
 	if (!np)
 		return -ENODEV;
 	of_node_put(np);
 
 	mvebu_cpu_resume = armada_370_xp_cpu_resume;
-	mvebu_v7_cpuidle_device.dev.platform_data = armada_370_xp_cpu_suspend;
+	mvebu_v7_cpuidle_device.dev.platform_data = &armada_xp_cpuidle;
 
 	return 0;
 }
diff --git a/drivers/cpuidle/cpuidle-mvebu-v7.c b/drivers/cpuidle/cpuidle-mvebu-v7.c
index 302596e..82c545bb 100644
--- a/drivers/cpuidle/cpuidle-mvebu-v7.c
+++ b/drivers/cpuidle/cpuidle-mvebu-v7.c
@@ -16,15 +16,15 @@ 
 #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 +32,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->cpu_suspend(deepidle);
 	if (ret)
 		return ret;
 
@@ -46,8 +47,8 @@  static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 	return index;
 }
 
-static struct cpuidle_driver mvebu_v7_idle_driver = {
-	.name			= "mvebu_v7_idle",
+static struct cpuidle_driver armadaxp_cpuidle_driver = {
+	.name			= "armada_xp_idle",
 	.states[0]		= ARM_CPUIDLE_WFI_STATE,
 	.states[1]		= {
 		.enter			= mvebu_v7_enter_idle,
@@ -55,7 +56,7 @@  static struct cpuidle_driver mvebu_v7_idle_driver = {
 		.power_usage		= 50,
 		.target_residency	= 100,
 		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "MV CPU IDLE",
+		.name			= "Idle",
 		.desc			= "CPU power down",
 	},
 	.states[2]		= {
@@ -63,19 +64,26 @@  static struct cpuidle_driver mvebu_v7_idle_driver = {
 		.exit_latency		= 100,
 		.power_usage		= 5,
 		.target_residency	= 1000,
-		.flags			= CPUIDLE_FLAG_TIME_VALID |
-						MVEBU_V7_FLAG_DEEP_IDLE,
-		.name			= "MV CPU DEEP IDLE",
+		.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,
+	.state_count = 3,
 };
 
 static int mvebu_v7_cpuidle_probe(struct platform_device *pdev)
 {
+	struct cpuidle_driver *drv;
+
+	pcpuidle = pdev->dev.platform_data;
+
+	if (pcpuidle->type == CPUIDLE_ARMADA_XP)
+		drv = &armadaxp_cpuidle_driver;
+	else
+		return -EINVAL;
 
-	mvebu_v7_cpu_suspend = (void *)(pdev->dev.platform_data);
-	return cpuidle_register(&mvebu_v7_idle_driver, NULL);
+	return cpuidle_register(drv, 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 0000000..00fde86
--- /dev/null
+++ b/include/linux/mvebu-v7-cpuidle.h
@@ -0,0 +1,26 @@ 
+/*
+ * 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__
+
+enum mvebu_v7_cpuidle_types {
+	CPUIDLE_ARMADA_XP,
+};
+
+struct mvebu_v7_cpuidle {
+	int type;
+	int (*cpu_suspend)(unsigned long deepidle);
+};
+
+#endif