All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-12 15:51 ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-12 15:51 UTC (permalink / raw)
  To: linux, nicolas.ferre, plagnioj
  Cc: linux-arm-kernel, patches, linaro-kernel, linux-pm

Use the platform driver model to separate the cpuidle specific code from
the low level pm code. It allows to remove the dependency between
these two components.

Tested-on usb-a9263 (at91sam9263)

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
 arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 4ec6a6d..6cdc76d 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -21,26 +21,17 @@
 #include <linux/export.h>
 #include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
-#include <mach/cpu.h>
-
-#include "pm.h"
 
 #define AT91_MAX_STATES	2
 
+static void (*at91_standby)(void);
+
 /* Actual code that puts the SoC in different idle states */
 static int at91_enter_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			       int index)
 {
-	if (cpu_is_at91rm9200())
-		at91rm9200_standby();
-	else if (cpu_is_at91sam9g45())
-		at91sam9g45_standby();
-	else if (cpu_is_at91sam9263())
-		at91sam9263_standby();
-	else
-		at91sam9_standby();
-
+	at91_standby();
 	return index;
 }
 
@@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
 };
 
 /* Initialize CPU idle by registering the idle states */
-static int __init at91_init_cpuidle(void)
+static int __init at91_cpuidle_probe(struct platform_device *dev)
 {
+	at91_standby = (void *)(dev->dev.platform_data);
+	
 	return cpuidle_register(&at91_idle_driver, NULL);
 }
 
-device_initcall(at91_init_cpuidle);
+static struct platform_driver at91_cpuidle_driver = {
+	.driver = {
+		.name = "cpuidle-at91",
+		.owner = THIS_MODULE,
+	},
+	.probe = at91_cpuidle_probe,
+};
+
+module_platform_driver(at91_cpuidle_driver);
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 15afb5d..debdbf8 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
 	.end	= at91_pm_end,
 };
 
+static struct platform_device at91_cpuidle_device = {
+	.name = "cpuidle-at91",
+};
+
 static int __init at91_pm_init(void)
 {
 #ifdef CONFIG_AT91_SLOW_CLOCK
@@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
 	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
 
 	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
-	if (cpu_is_at91rm9200())
+	if (cpu_is_at91rm9200()) {
+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
 		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
+	} else if (cpu_is_at91sam9g45()) {
+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
+	} else if (cpu_is_at91sam9263()) {
+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
+	} else {
+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
+	}
+	
+	platform_device_register(&at91_cpuidle_device);
 
 	suspend_set_ops(&at91_pm_ops);
 
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-12 15:51 ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-12 15:51 UTC (permalink / raw)
  To: linux-arm-kernel

Use the platform driver model to separate the cpuidle specific code from
the low level pm code. It allows to remove the dependency between
these two components.

Tested-on usb-a9263 (at91sam9263)

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
 arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 4ec6a6d..6cdc76d 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -21,26 +21,17 @@
 #include <linux/export.h>
 #include <asm/proc-fns.h>
 #include <asm/cpuidle.h>
-#include <mach/cpu.h>
-
-#include "pm.h"
 
 #define AT91_MAX_STATES	2
 
+static void (*at91_standby)(void);
+
 /* Actual code that puts the SoC in different idle states */
 static int at91_enter_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			       int index)
 {
-	if (cpu_is_at91rm9200())
-		at91rm9200_standby();
-	else if (cpu_is_at91sam9g45())
-		at91sam9g45_standby();
-	else if (cpu_is_at91sam9263())
-		at91sam9263_standby();
-	else
-		at91sam9_standby();
-
+	at91_standby();
 	return index;
 }
 
@@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
 };
 
 /* Initialize CPU idle by registering the idle states */
-static int __init at91_init_cpuidle(void)
+static int __init at91_cpuidle_probe(struct platform_device *dev)
 {
+	at91_standby = (void *)(dev->dev.platform_data);
+	
 	return cpuidle_register(&at91_idle_driver, NULL);
 }
 
-device_initcall(at91_init_cpuidle);
+static struct platform_driver at91_cpuidle_driver = {
+	.driver = {
+		.name = "cpuidle-at91",
+		.owner = THIS_MODULE,
+	},
+	.probe = at91_cpuidle_probe,
+};
+
+module_platform_driver(at91_cpuidle_driver);
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 15afb5d..debdbf8 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
 	.end	= at91_pm_end,
 };
 
+static struct platform_device at91_cpuidle_device = {
+	.name = "cpuidle-at91",
+};
+
 static int __init at91_pm_init(void)
 {
 #ifdef CONFIG_AT91_SLOW_CLOCK
@@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
 	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
 
 	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
-	if (cpu_is_at91rm9200())
+	if (cpu_is_at91rm9200()) {
+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
 		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
+	} else if (cpu_is_at91sam9g45()) {
+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
+	} else if (cpu_is_at91sam9263()) {
+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
+	} else {
+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
+	}
+	
+	platform_device_register(&at91_cpuidle_device);
 
 	suspend_set_ops(&at91_pm_ops);
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
  2013-10-12 15:51 ` Daniel Lezcano
@ 2013-10-12 15:51   ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-12 15:51 UTC (permalink / raw)
  To: linux, nicolas.ferre, plagnioj
  Cc: linux-arm-kernel, patches, linaro-kernel, linux-pm

As the cpuidle driver code has no more the dependency with the pm code, the
'standby' callback being passed as a parameter to the device's platform data,
we can move the cpuidle driver in the drivers/cpuidle directory.

Tested-on usb-a9263 (at91sam9263)

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/Makefile                        |    1 -
 drivers/cpuidle/Kconfig.arm                        |    6 ++++++
 drivers/cpuidle/Makefile                           |    1 +
 .../cpuidle.c => drivers/cpuidle/cpuidle-at91.c    |    0
 4 files changed, 7 insertions(+), 1 deletion(-)
 rename arch/arm/mach-at91/cpuidle.c => drivers/cpuidle/cpuidle-at91.c (100%)

diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile
index 3b0a953..c1b7370 100644
--- a/arch/arm/mach-at91/Makefile
+++ b/arch/arm/mach-at91/Makefile
@@ -98,7 +98,6 @@ obj-y				+= leds.o
 # Power Management
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_AT91_SLOW_CLOCK)	+= pm_slowclock.o
-obj-$(CONFIG_CPU_IDLE)	+= cpuidle.o
 
 ifeq ($(CONFIG_PM_DEBUG),y)
 CFLAGS_pm.o += -DDEBUG
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 8e36603..edecfc8 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -27,6 +27,12 @@ config ARM_U8500_CPUIDLE
 	help
 	  Select this to enable cpuidle for ST-E u8500 processors
 
+config ARM_AT91_CPUIDLE
+        bool "Cpu Idle Driver for the AT91 processors"
+	depends on ARCH_AT91
+	help
+	  Select this to enable cpuidle for AT91 processors
+
 config CPU_IDLE_BIG_LITTLE
 	bool "Support for ARM big.LITTLE processors"
 	depends on ARCH_VEXPRESS_TC2_PM
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index cea5ef5..60c4ae5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 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_CPU_IDLE_BIG_LITTLE)	+= cpuidle-big_little.o
diff --git a/arch/arm/mach-at91/cpuidle.c b/drivers/cpuidle/cpuidle-at91.c
similarity index 100%
rename from arch/arm/mach-at91/cpuidle.c
rename to drivers/cpuidle/cpuidle-at91.c
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
@ 2013-10-12 15:51   ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-12 15:51 UTC (permalink / raw)
  To: linux-arm-kernel

As the cpuidle driver code has no more the dependency with the pm code, the
'standby' callback being passed as a parameter to the device's platform data,
we can move the cpuidle driver in the drivers/cpuidle directory.

Tested-on usb-a9263 (at91sam9263)

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/Makefile                        |    1 -
 drivers/cpuidle/Kconfig.arm                        |    6 ++++++
 drivers/cpuidle/Makefile                           |    1 +
 .../cpuidle.c => drivers/cpuidle/cpuidle-at91.c    |    0
 4 files changed, 7 insertions(+), 1 deletion(-)
 rename arch/arm/mach-at91/cpuidle.c => drivers/cpuidle/cpuidle-at91.c (100%)

diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile
index 3b0a953..c1b7370 100644
--- a/arch/arm/mach-at91/Makefile
+++ b/arch/arm/mach-at91/Makefile
@@ -98,7 +98,6 @@ obj-y				+= leds.o
 # Power Management
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_AT91_SLOW_CLOCK)	+= pm_slowclock.o
-obj-$(CONFIG_CPU_IDLE)	+= cpuidle.o
 
 ifeq ($(CONFIG_PM_DEBUG),y)
 CFLAGS_pm.o += -DDEBUG
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 8e36603..edecfc8 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -27,6 +27,12 @@ config ARM_U8500_CPUIDLE
 	help
 	  Select this to enable cpuidle for ST-E u8500 processors
 
+config ARM_AT91_CPUIDLE
+        bool "Cpu Idle Driver for the AT91 processors"
+	depends on ARCH_AT91
+	help
+	  Select this to enable cpuidle for AT91 processors
+
 config CPU_IDLE_BIG_LITTLE
 	bool "Support for ARM big.LITTLE processors"
 	depends on ARCH_VEXPRESS_TC2_PM
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index cea5ef5..60c4ae5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE)	+= cpuidle-calxeda.o
 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_CPU_IDLE_BIG_LITTLE)	+= cpuidle-big_little.o
diff --git a/arch/arm/mach-at91/cpuidle.c b/drivers/cpuidle/cpuidle-at91.c
similarity index 100%
rename from arch/arm/mach-at91/cpuidle.c
rename to drivers/cpuidle/cpuidle-at91.c
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
  2013-10-12 15:51   ` Daniel Lezcano
@ 2013-10-13  9:19     ` Thomas Petazzoni
  -1 siblings, 0 replies; 24+ messages in thread
From: Thomas Petazzoni @ 2013-10-13  9:19 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux, nicolas.ferre, plagnioj, linux-pm, linaro-kernel,
	linux-arm-kernel, patches

Dear Daniel Lezcano,

On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:

> +config ARM_AT91_CPUIDLE
> +        bool "Cpu Idle Driver for the AT91 processors"

Nitpick: indentation is done with spaces here, while it should be one
tab.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
@ 2013-10-13  9:19     ` Thomas Petazzoni
  0 siblings, 0 replies; 24+ messages in thread
From: Thomas Petazzoni @ 2013-10-13  9:19 UTC (permalink / raw)
  To: linux-arm-kernel

Dear Daniel Lezcano,

On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:

> +config ARM_AT91_CPUIDLE
> +        bool "Cpu Idle Driver for the AT91 processors"

Nitpick: indentation is done with spaces here, while it should be one
tab.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
  2013-10-13  9:19     ` Thomas Petazzoni
@ 2013-10-13 10:17       ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-13 10:17 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: linux, nicolas.ferre, plagnioj, linux-pm, linaro-kernel,
	linux-arm-kernel, patches

On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
> Dear Daniel Lezcano,
>
> On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
>
>> +config ARM_AT91_CPUIDLE
>> +        bool "Cpu Idle Driver for the AT91 processors"
>
> Nitpick: indentation is done with spaces here, while it should be one
> tab.

Ah, right. Fixed for the V2.

Thanks !
   -- Daniel


-- 
  <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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
@ 2013-10-13 10:17       ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-13 10:17 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
> Dear Daniel Lezcano,
>
> On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
>
>> +config ARM_AT91_CPUIDLE
>> +        bool "Cpu Idle Driver for the AT91 processors"
>
> Nitpick: indentation is done with spaces here, while it should be one
> tab.

Ah, right. Fixed for the V2.

Thanks !
   -- Daniel


-- 
  <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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
  2013-10-13 10:17       ` Daniel Lezcano
@ 2013-10-14 10:20         ` Bartlomiej Zolnierkiewicz
  -1 siblings, 0 replies; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-10-14 10:20 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Thomas Petazzoni, linux, nicolas.ferre, plagnioj, linux-pm,
	linaro-kernel, linux-arm-kernel, patches


Hi Daniel,

On Sunday, October 13, 2013 12:17:17 PM Daniel Lezcano wrote:
> On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
> > Dear Daniel Lezcano,
> >
> > On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
> >
> >> +config ARM_AT91_CPUIDLE
> >> +        bool "Cpu Idle Driver for the AT91 processors"
> >
> > Nitpick: indentation is done with spaces here, while it should be one
> > tab.
> 
> Ah, right. Fixed for the V2.

Could you also please add 'default y' to preserve the old behavior?
(the at91 cpuidle support was always built-in for CONFIG_CPU_IDLE=y)

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
@ 2013-10-14 10:20         ` Bartlomiej Zolnierkiewicz
  0 siblings, 0 replies; 24+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-10-14 10:20 UTC (permalink / raw)
  To: linux-arm-kernel


Hi Daniel,

On Sunday, October 13, 2013 12:17:17 PM Daniel Lezcano wrote:
> On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
> > Dear Daniel Lezcano,
> >
> > On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
> >
> >> +config ARM_AT91_CPUIDLE
> >> +        bool "Cpu Idle Driver for the AT91 processors"
> >
> > Nitpick: indentation is done with spaces here, while it should be one
> > tab.
> 
> Ah, right. Fixed for the V2.

Could you also please add 'default y' to preserve the old behavior?
(the at91 cpuidle support was always built-in for CONFIG_CPU_IDLE=y)

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
  2013-10-14 10:20         ` Bartlomiej Zolnierkiewicz
@ 2013-10-14 10:53           ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 10:53 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Thomas Petazzoni, linux, nicolas.ferre, plagnioj, linux-pm,
	linaro-kernel, linux-arm-kernel, patches

On 10/14/2013 12:20 PM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Daniel,
>
> On Sunday, October 13, 2013 12:17:17 PM Daniel Lezcano wrote:
>> On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
>>> Dear Daniel Lezcano,
>>>
>>> On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
>>>
>>>> +config ARM_AT91_CPUIDLE
>>>> +        bool "Cpu Idle Driver for the AT91 processors"
>>>
>>> Nitpick: indentation is done with spaces here, while it should be one
>>> tab.
>>
>> Ah, right. Fixed for the V2.
>
> Could you also please add 'default y' to preserve the old behavior?
> (the at91 cpuidle support was always built-in for CONFIG_CPU_IDLE=y)

Yes, sure.

Thanks for the review.

   -- Daniel


-- 
  <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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle
@ 2013-10-14 10:53           ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 10:53 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/14/2013 12:20 PM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Daniel,
>
> On Sunday, October 13, 2013 12:17:17 PM Daniel Lezcano wrote:
>> On 10/13/2013 11:19 AM, Thomas Petazzoni wrote:
>>> Dear Daniel Lezcano,
>>>
>>> On Sat, 12 Oct 2013 17:51:39 +0200, Daniel Lezcano wrote:
>>>
>>>> +config ARM_AT91_CPUIDLE
>>>> +        bool "Cpu Idle Driver for the AT91 processors"
>>>
>>> Nitpick: indentation is done with spaces here, while it should be one
>>> tab.
>>
>> Ah, right. Fixed for the V2.
>
> Could you also please add 'default y' to preserve the old behavior?
> (the at91 cpuidle support was always built-in for CONFIG_CPU_IDLE=y)

Yes, sure.

Thanks for the review.

   -- Daniel


-- 
  <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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-12 15:51 ` Daniel Lezcano
@ 2013-10-14 14:04   ` Jean-Christophe PLAGNIOL-VILLARD
  -1 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 14:04 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux, nicolas.ferre, linux-arm-kernel, patches, linaro-kernel, linux-pm

On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> Use the platform driver model to separate the cpuidle specific code from
> the low level pm code. It allows to remove the dependency between
> these two components.
> 
> Tested-on usb-a9263 (at91sam9263)
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
>  arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index 4ec6a6d..6cdc76d 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
> @@ -21,26 +21,17 @@
>  #include <linux/export.h>
>  #include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
> -#include <mach/cpu.h>
> -
> -#include "pm.h"
>  
>  #define AT91_MAX_STATES	2
>  
> +static void (*at91_standby)(void);
> +
>  /* Actual code that puts the SoC in different idle states */
>  static int at91_enter_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
>  			       int index)
>  {
> -	if (cpu_is_at91rm9200())
> -		at91rm9200_standby();
> -	else if (cpu_is_at91sam9g45())
> -		at91sam9g45_standby();
> -	else if (cpu_is_at91sam9263())
> -		at91sam9263_standby();
> -	else
> -		at91sam9_standby();
> -
> +	at91_standby();
>  	return index;
>  }
>  
> @@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
>  };
>  
>  /* Initialize CPU idle by registering the idle states */
> -static int __init at91_init_cpuidle(void)
> +static int __init at91_cpuidle_probe(struct platform_device *dev)
>  {
> +	at91_standby = (void *)(dev->dev.platform_data);
> +	
>  	return cpuidle_register(&at91_idle_driver, NULL);
>  }
>  
> -device_initcall(at91_init_cpuidle);
> +static struct platform_driver at91_cpuidle_driver = {
> +	.driver = {
> +		.name = "cpuidle-at91",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = at91_cpuidle_probe,
> +};
> +
> +module_platform_driver(at91_cpuidle_driver);
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 15afb5d..debdbf8 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
>  	.end	= at91_pm_end,
>  };
>  
> +static struct platform_device at91_cpuidle_device = {
> +	.name = "cpuidle-at91",
> +};
> +
>  static int __init at91_pm_init(void)
>  {
>  #ifdef CONFIG_AT91_SLOW_CLOCK
> @@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
>  
>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> -	if (cpu_is_at91rm9200())
> +	if (cpu_is_at91rm9200()) {
> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> +	} else if (cpu_is_at91sam9g45()) {
> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> +	} else if (cpu_is_at91sam9263()) {
> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> +	} else {
> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
no this is too dangerous when adding new SoC

you must list the supported SoC

and I prefer to move this code to the SoC init structure

so we can drop the if/else if/elsee

and drop the issue of adding new and the arch_initcall

Best Regards,
J.
> +	}
> +	
> +	platform_device_register(&at91_cpuidle_device);
>  
>  	suspend_set_ops(&at91_pm_ops);
>  
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 14:04   ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 14:04 UTC (permalink / raw)
  To: linux-arm-kernel

On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> Use the platform driver model to separate the cpuidle specific code from
> the low level pm code. It allows to remove the dependency between
> these two components.
> 
> Tested-on usb-a9263 (at91sam9263)
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
>  arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index 4ec6a6d..6cdc76d 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
> @@ -21,26 +21,17 @@
>  #include <linux/export.h>
>  #include <asm/proc-fns.h>
>  #include <asm/cpuidle.h>
> -#include <mach/cpu.h>
> -
> -#include "pm.h"
>  
>  #define AT91_MAX_STATES	2
>  
> +static void (*at91_standby)(void);
> +
>  /* Actual code that puts the SoC in different idle states */
>  static int at91_enter_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv,
>  			       int index)
>  {
> -	if (cpu_is_at91rm9200())
> -		at91rm9200_standby();
> -	else if (cpu_is_at91sam9g45())
> -		at91sam9g45_standby();
> -	else if (cpu_is_at91sam9263())
> -		at91sam9263_standby();
> -	else
> -		at91sam9_standby();
> -
> +	at91_standby();
>  	return index;
>  }
>  
> @@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
>  };
>  
>  /* Initialize CPU idle by registering the idle states */
> -static int __init at91_init_cpuidle(void)
> +static int __init at91_cpuidle_probe(struct platform_device *dev)
>  {
> +	at91_standby = (void *)(dev->dev.platform_data);
> +	
>  	return cpuidle_register(&at91_idle_driver, NULL);
>  }
>  
> -device_initcall(at91_init_cpuidle);
> +static struct platform_driver at91_cpuidle_driver = {
> +	.driver = {
> +		.name = "cpuidle-at91",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = at91_cpuidle_probe,
> +};
> +
> +module_platform_driver(at91_cpuidle_driver);
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 15afb5d..debdbf8 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
>  	.end	= at91_pm_end,
>  };
>  
> +static struct platform_device at91_cpuidle_device = {
> +	.name = "cpuidle-at91",
> +};
> +
>  static int __init at91_pm_init(void)
>  {
>  #ifdef CONFIG_AT91_SLOW_CLOCK
> @@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
>  
>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> -	if (cpu_is_at91rm9200())
> +	if (cpu_is_at91rm9200()) {
> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> +	} else if (cpu_is_at91sam9g45()) {
> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> +	} else if (cpu_is_at91sam9263()) {
> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> +	} else {
> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
no this is too dangerous when adding new SoC

you must list the supported SoC

and I prefer to move this code to the SoC init structure

so we can drop the if/else if/elsee

and drop the issue of adding new and the arch_initcall

Best Regards,
J.
> +	}
> +	
> +	platform_device_register(&at91_cpuidle_device);
>  
>  	suspend_set_ops(&at91_pm_ops);
>  
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-14 14:04   ` Jean-Christophe PLAGNIOL-VILLARD
@ 2013-10-14 14:18     ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 14:18 UTC (permalink / raw)
  To: Jean-Christophe PLAGNIOL-VILLARD
  Cc: linux, nicolas.ferre, linux-arm-kernel, patches, linaro-kernel, linux-pm

On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>> Use the platform driver model to separate the cpuidle specific code from
>> the low level pm code. It allows to remove the dependency between
>> these two components.
>>
>> Tested-on usb-a9263 (at91sam9263)
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
>>   arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
>>   2 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
>> index 4ec6a6d..6cdc76d 100644
>> --- a/arch/arm/mach-at91/cpuidle.c
>> +++ b/arch/arm/mach-at91/cpuidle.c
>> @@ -21,26 +21,17 @@
>>   #include <linux/export.h>
>>   #include <asm/proc-fns.h>
>>   #include <asm/cpuidle.h>
>> -#include <mach/cpu.h>
>> -
>> -#include "pm.h"
>>
>>   #define AT91_MAX_STATES	2
>>
>> +static void (*at91_standby)(void);
>> +
>>   /* Actual code that puts the SoC in different idle states */
>>   static int at91_enter_idle(struct cpuidle_device *dev,
>>   			struct cpuidle_driver *drv,
>>   			       int index)
>>   {
>> -	if (cpu_is_at91rm9200())
>> -		at91rm9200_standby();
>> -	else if (cpu_is_at91sam9g45())
>> -		at91sam9g45_standby();
>> -	else if (cpu_is_at91sam9263())
>> -		at91sam9263_standby();
>> -	else
>> -		at91sam9_standby();
>> -
>> +	at91_standby();
>>   	return index;
>>   }
>>
>> @@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
>>   };
>>
>>   /* Initialize CPU idle by registering the idle states */
>> -static int __init at91_init_cpuidle(void)
>> +static int __init at91_cpuidle_probe(struct platform_device *dev)
>>   {
>> +	at91_standby = (void *)(dev->dev.platform_data);
>> +	
>>   	return cpuidle_register(&at91_idle_driver, NULL);
>>   }
>>
>> -device_initcall(at91_init_cpuidle);
>> +static struct platform_driver at91_cpuidle_driver = {
>> +	.driver = {
>> +		.name = "cpuidle-at91",
>> +		.owner = THIS_MODULE,
>> +	},
>> +	.probe = at91_cpuidle_probe,
>> +};
>> +
>> +module_platform_driver(at91_cpuidle_driver);
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index 15afb5d..debdbf8 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
>>   	.end	= at91_pm_end,
>>   };
>>
>> +static struct platform_device at91_cpuidle_device = {
>> +	.name = "cpuidle-at91",
>> +};
>> +
>>   static int __init at91_pm_init(void)
>>   {
>>   #ifdef CONFIG_AT91_SLOW_CLOCK
>> @@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
>>   	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
>>
>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>> -	if (cpu_is_at91rm9200())
>> +	if (cpu_is_at91rm9200()) {
>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>> +	} else if (cpu_is_at91sam9g45()) {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>> +	} else if (cpu_is_at91sam9263()) {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>> +	} else {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> no this is too dangerous when adding new SoC
>
> you must list the supported SoC
>
> and I prefer to move this code to the SoC init structure

Do you mean register the platform_device in eg. at91rm9200_initialize 
with the right platform data ?


> so we can drop the if/else if/elsee
>
> and drop the issue of adding new and the arch_initcall
>
> Best Regards,
> J.
>> +	}
>> +	
>> +	platform_device_register(&at91_cpuidle_device);
>>
>>   	suspend_set_ops(&at91_pm_ops);
>>
>> --
>> 1.7.9.5
>>


-- 
  <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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 14:18     ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 14:18 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>> Use the platform driver model to separate the cpuidle specific code from
>> the low level pm code. It allows to remove the dependency between
>> these two components.
>>
>> Tested-on usb-a9263 (at91sam9263)
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
>>   arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
>>   2 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
>> index 4ec6a6d..6cdc76d 100644
>> --- a/arch/arm/mach-at91/cpuidle.c
>> +++ b/arch/arm/mach-at91/cpuidle.c
>> @@ -21,26 +21,17 @@
>>   #include <linux/export.h>
>>   #include <asm/proc-fns.h>
>>   #include <asm/cpuidle.h>
>> -#include <mach/cpu.h>
>> -
>> -#include "pm.h"
>>
>>   #define AT91_MAX_STATES	2
>>
>> +static void (*at91_standby)(void);
>> +
>>   /* Actual code that puts the SoC in different idle states */
>>   static int at91_enter_idle(struct cpuidle_device *dev,
>>   			struct cpuidle_driver *drv,
>>   			       int index)
>>   {
>> -	if (cpu_is_at91rm9200())
>> -		at91rm9200_standby();
>> -	else if (cpu_is_at91sam9g45())
>> -		at91sam9g45_standby();
>> -	else if (cpu_is_at91sam9263())
>> -		at91sam9263_standby();
>> -	else
>> -		at91sam9_standby();
>> -
>> +	at91_standby();
>>   	return index;
>>   }
>>
>> @@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
>>   };
>>
>>   /* Initialize CPU idle by registering the idle states */
>> -static int __init at91_init_cpuidle(void)
>> +static int __init at91_cpuidle_probe(struct platform_device *dev)
>>   {
>> +	at91_standby = (void *)(dev->dev.platform_data);
>> +	
>>   	return cpuidle_register(&at91_idle_driver, NULL);
>>   }
>>
>> -device_initcall(at91_init_cpuidle);
>> +static struct platform_driver at91_cpuidle_driver = {
>> +	.driver = {
>> +		.name = "cpuidle-at91",
>> +		.owner = THIS_MODULE,
>> +	},
>> +	.probe = at91_cpuidle_probe,
>> +};
>> +
>> +module_platform_driver(at91_cpuidle_driver);
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index 15afb5d..debdbf8 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
>>   	.end	= at91_pm_end,
>>   };
>>
>> +static struct platform_device at91_cpuidle_device = {
>> +	.name = "cpuidle-at91",
>> +};
>> +
>>   static int __init at91_pm_init(void)
>>   {
>>   #ifdef CONFIG_AT91_SLOW_CLOCK
>> @@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
>>   	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
>>
>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>> -	if (cpu_is_at91rm9200())
>> +	if (cpu_is_at91rm9200()) {
>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>> +	} else if (cpu_is_at91sam9g45()) {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>> +	} else if (cpu_is_at91sam9263()) {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>> +	} else {
>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> no this is too dangerous when adding new SoC
>
> you must list the supported SoC
>
> and I prefer to move this code to the SoC init structure

Do you mean register the platform_device in eg. at91rm9200_initialize 
with the right platform data ?


> so we can drop the if/else if/elsee
>
> and drop the issue of adding new and the arch_initcall
>
> Best Regards,
> J.
>> +	}
>> +	
>> +	platform_device_register(&at91_cpuidle_device);
>>
>>   	suspend_set_ops(&at91_pm_ops);
>>
>> --
>> 1.7.9.5
>>


-- 
  <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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-14 14:18     ` Daniel Lezcano
@ 2013-10-14 14:27       ` Jean-Christophe PLAGNIOL-VILLARD
  -1 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 14:27 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linaro-kernel, linux-pm, patches, nicolas.ferre, linux, linux-arm-kernel

On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> >>Use the platform driver model to separate the cpuidle specific code from
> >>the low level pm code. It allows to remove the dependency between
> >>these two components.
> >>
> >>Tested-on usb-a9263 (at91sam9263)
> >>
> >>Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>---
> >>  arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
> >>  arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
> >>  2 files changed, 30 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> >>index 4ec6a6d..6cdc76d 100644
> >>--- a/arch/arm/mach-at91/cpuidle.c
> >>+++ b/arch/arm/mach-at91/cpuidle.c
> >>@@ -21,26 +21,17 @@
> >>  #include <linux/export.h>
> >>  #include <asm/proc-fns.h>
> >>  #include <asm/cpuidle.h>
> >>-#include <mach/cpu.h>
> >>-
> >>-#include "pm.h"
> >>
> >>  #define AT91_MAX_STATES	2
> >>
> >>+static void (*at91_standby)(void);
> >>+
> >>  /* Actual code that puts the SoC in different idle states */
> >>  static int at91_enter_idle(struct cpuidle_device *dev,
> >>  			struct cpuidle_driver *drv,
> >>  			       int index)
> >>  {
> >>-	if (cpu_is_at91rm9200())
> >>-		at91rm9200_standby();
> >>-	else if (cpu_is_at91sam9g45())
> >>-		at91sam9g45_standby();
> >>-	else if (cpu_is_at91sam9263())
> >>-		at91sam9263_standby();
> >>-	else
> >>-		at91sam9_standby();
> >>-
> >>+	at91_standby();
> >>  	return index;
> >>  }
> >>
> >>@@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
> >>  };
> >>
> >>  /* Initialize CPU idle by registering the idle states */
> >>-static int __init at91_init_cpuidle(void)
> >>+static int __init at91_cpuidle_probe(struct platform_device *dev)
> >>  {
> >>+	at91_standby = (void *)(dev->dev.platform_data);
> >>+	
> >>  	return cpuidle_register(&at91_idle_driver, NULL);
> >>  }
> >>
> >>-device_initcall(at91_init_cpuidle);
> >>+static struct platform_driver at91_cpuidle_driver = {
> >>+	.driver = {
> >>+		.name = "cpuidle-at91",
> >>+		.owner = THIS_MODULE,
> >>+	},
> >>+	.probe = at91_cpuidle_probe,
> >>+};
> >>+
> >>+module_platform_driver(at91_cpuidle_driver);
> >>diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> >>index 15afb5d..debdbf8 100644
> >>--- a/arch/arm/mach-at91/pm.c
> >>+++ b/arch/arm/mach-at91/pm.c
> >>@@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
> >>  	.end	= at91_pm_end,
> >>  };
> >>
> >>+static struct platform_device at91_cpuidle_device = {
> >>+	.name = "cpuidle-at91",
> >>+};
> >>+
> >>  static int __init at91_pm_init(void)
> >>  {
> >>  #ifdef CONFIG_AT91_SLOW_CLOCK
> >>@@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
> >>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
> >>
> >>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> >>-	if (cpu_is_at91rm9200())
> >>+	if (cpu_is_at91rm9200()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> >>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> >>+	} else if (cpu_is_at91sam9g45()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> >>+	} else if (cpu_is_at91sam9263()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> >>+	} else {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> >no this is too dangerous when adding new SoC
> >
> >you must list the supported SoC
> >
> >and I prefer to move this code to the SoC init structure
> 
> Do you mean register the platform_device in eg.
> at91rm9200_initialize with the right platform data ?
> 
yes as example

Best Regards,
J.
> 
> >so we can drop the if/else if/elsee
> >
> >and drop the issue of adding new and the arch_initcall
> >
> >Best Regards,
> >J.
> >>+	}
> >>+	
> >>+	platform_device_register(&at91_cpuidle_device);
> >>
> >>  	suspend_set_ops(&at91_pm_ops);
> >>
> >>--
> >>1.7.9.5
> >>
> 
> 
> -- 
>  <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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 14:27       ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 14:27 UTC (permalink / raw)
  To: linux-arm-kernel

On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> >>Use the platform driver model to separate the cpuidle specific code from
> >>the low level pm code. It allows to remove the dependency between
> >>these two components.
> >>
> >>Tested-on usb-a9263 (at91sam9263)
> >>
> >>Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>---
> >>  arch/arm/mach-at91/cpuidle.c |   29 +++++++++++++++--------------
> >>  arch/arm/mach-at91/pm.c      |   16 +++++++++++++++-
> >>  2 files changed, 30 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> >>index 4ec6a6d..6cdc76d 100644
> >>--- a/arch/arm/mach-at91/cpuidle.c
> >>+++ b/arch/arm/mach-at91/cpuidle.c
> >>@@ -21,26 +21,17 @@
> >>  #include <linux/export.h>
> >>  #include <asm/proc-fns.h>
> >>  #include <asm/cpuidle.h>
> >>-#include <mach/cpu.h>
> >>-
> >>-#include "pm.h"
> >>
> >>  #define AT91_MAX_STATES	2
> >>
> >>+static void (*at91_standby)(void);
> >>+
> >>  /* Actual code that puts the SoC in different idle states */
> >>  static int at91_enter_idle(struct cpuidle_device *dev,
> >>  			struct cpuidle_driver *drv,
> >>  			       int index)
> >>  {
> >>-	if (cpu_is_at91rm9200())
> >>-		at91rm9200_standby();
> >>-	else if (cpu_is_at91sam9g45())
> >>-		at91sam9g45_standby();
> >>-	else if (cpu_is_at91sam9263())
> >>-		at91sam9263_standby();
> >>-	else
> >>-		at91sam9_standby();
> >>-
> >>+	at91_standby();
> >>  	return index;
> >>  }
> >>
> >>@@ -60,9 +51,19 @@ static struct cpuidle_driver at91_idle_driver = {
> >>  };
> >>
> >>  /* Initialize CPU idle by registering the idle states */
> >>-static int __init at91_init_cpuidle(void)
> >>+static int __init at91_cpuidle_probe(struct platform_device *dev)
> >>  {
> >>+	at91_standby = (void *)(dev->dev.platform_data);
> >>+	
> >>  	return cpuidle_register(&at91_idle_driver, NULL);
> >>  }
> >>
> >>-device_initcall(at91_init_cpuidle);
> >>+static struct platform_driver at91_cpuidle_driver = {
> >>+	.driver = {
> >>+		.name = "cpuidle-at91",
> >>+		.owner = THIS_MODULE,
> >>+	},
> >>+	.probe = at91_cpuidle_probe,
> >>+};
> >>+
> >>+module_platform_driver(at91_cpuidle_driver);
> >>diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> >>index 15afb5d..debdbf8 100644
> >>--- a/arch/arm/mach-at91/pm.c
> >>+++ b/arch/arm/mach-at91/pm.c
> >>@@ -314,6 +314,10 @@ static const struct platform_suspend_ops at91_pm_ops = {
> >>  	.end	= at91_pm_end,
> >>  };
> >>
> >>+static struct platform_device at91_cpuidle_device = {
> >>+	.name = "cpuidle-at91",
> >>+};
> >>+
> >>  static int __init at91_pm_init(void)
> >>  {
> >>  #ifdef CONFIG_AT91_SLOW_CLOCK
> >>@@ -323,8 +327,18 @@ static int __init at91_pm_init(void)
> >>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow clock mode)" : ""));
> >>
> >>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> >>-	if (cpu_is_at91rm9200())
> >>+	if (cpu_is_at91rm9200()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> >>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> >>+	} else if (cpu_is_at91sam9g45()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> >>+	} else if (cpu_is_at91sam9263()) {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> >>+	} else {
> >>+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> >no this is too dangerous when adding new SoC
> >
> >you must list the supported SoC
> >
> >and I prefer to move this code to the SoC init structure
> 
> Do you mean register the platform_device in eg.
> at91rm9200_initialize with the right platform data ?
> 
yes as example

Best Regards,
J.
> 
> >so we can drop the if/else if/elsee
> >
> >and drop the issue of adding new and the arch_initcall
> >
> >Best Regards,
> >J.
> >>+	}
> >>+	
> >>+	platform_device_register(&at91_cpuidle_device);
> >>
> >>  	suspend_set_ops(&at91_pm_ops);
> >>
> >>--
> >>1.7.9.5
> >>
> 
> 
> -- 
>  <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
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-14 14:27       ` Jean-Christophe PLAGNIOL-VILLARD
@ 2013-10-14 16:30         ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 16:30 UTC (permalink / raw)
  To: Jean-Christophe PLAGNIOL-VILLARD
  Cc: linux, nicolas.ferre, linux-arm-kernel, patches, linaro-kernel, linux-pm

On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
>> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>>>> Use the platform driver model to separate the cpuidle specific code from
>>>> the low level pm code. It allows to remove the dependency between
>>>> these two components.
>>>>
>>>> Tested-on usb-a9263 (at91sam9263)
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[ ... ]

>>>> ---
>>>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>>>> -	if (cpu_is_at91rm9200())
>>>> +	if (cpu_is_at91rm9200()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>>>> +	} else if (cpu_is_at91sam9g45()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>>>> +	} else if (cpu_is_at91sam9263()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>>>> +	} else {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>>> no this is too dangerous when adding new SoC
>>>
>>> you must list the supported SoC
>>>
>>> and I prefer to move this code to the SoC init structure
>>
>> Do you mean register the platform_device in eg.
>> at91rm9200_initialize with the right platform data ?
>>
> yes as example
>
> Best Regards,

Hi Jean-Christophe,

I did the changes but there are different drawbacks with the approach.
The first one is the AT91_SOC_START is too early to call the 
platform_device_register. The second one is we have multiple declaration 
of the platform_device which is not really nice.

I found the following solution:

The platform_device is static in pm.c, there is a at91_pm_set_standby 
function. All AT91_SOC_START call at91_pm_set_standby with the right 
standby function callback, which in turns set the platform data for the 
cpuidle platform device. When the init call for pm is made, it registers 
the platform device.

That has the benefit to encapsulate the platform device and having it in 
a single place and also we have the information to remove the { if then 
else } statements we have in 'at91_pm_enter's case block for 
PM_SUSPEND_STANDBY.

Does it sound ok for you ?

Thanks
   -- Daniel

diff --git a/arch/arm/mach-at91/at91rm9200.c 
b/arch/arm/mach-at91/at91rm9200.c
index 4aad93d..0d234f2 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
  	/* Initialize GPIO subsystem */
  	at91_gpio_init(at91rm9200_gpio,
  		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
+
+	at91_pm_set_standby(at91rm9200_standby);
  }


diff --git a/arch/arm/mach-at91/at91sam9260.c 
b/arch/arm/mach-at91/at91sam9260.c
index 5de6074..9fd7aa6 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -28,6 +28,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9260 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9xe_map_io(void)
  {
  	unsigned long sram_size;
@@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9260_gpio, 3);
+
+	at91_pm_set_standby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9261.c 
b/arch/arm/mach-at91/at91sam9261.c
index 0e07932..1edbb6f 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9261_gpio, 3);
+
+	at91_pm_set_sandby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9263.c 
b/arch/arm/mach-at91/at91sam9263.c
index 6ce7d18..b222d59 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -26,6 +26,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9263 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9263_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
@@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9263_gpio, 5);
+
+	at91_pm_set_standby(at91sam9263_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9g45.c 
b/arch/arm/mach-at91/at91sam9g45.c
index 474ee04..4186f37 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -26,6 +26,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9G45 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9g45_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
@@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9g45_gpio, 5);
+
+	at91_pm_set_standby(at91sam9g45_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9n12.c 
b/arch/arm/mach-at91/at91sam9n12.c
index c7d670d..cc61e09 100644
--- a/arch/arm/mach-at91/at91sam9n12.c
+++ b/arch/arm/mach-at91/at91sam9n12.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9N12 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9n12_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
  }

+static void __init at91sam9n12_initialize(void)
+{
+	at91_pm_set_standby(at91sam9_standby);
+}
+
  AT91_SOC_START(at91sam9n12)
  	.map_io = at91sam9n12_map_io,
  	.register_clocks = at91sam9n12_register_clocks,
+	.init = at91sam9n12_initialize,
  AT91_SOC_END
diff --git a/arch/arm/mach-at91/at91sam9rl.c 
b/arch/arm/mach-at91/at91sam9rl.c
index d4ec0d9..b0c8cb6 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9RL processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9rl_map_io(void)
  {
  	unsigned long sram_size;
@@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9rl_gpio, 4);
+
+	at91_pm_set_standby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9x5.c 
b/arch/arm/mach-at91/at91sam9x5.c
index 916e5a1..9ef7bc6 100644
--- a/arch/arm/mach-at91/at91sam9x5.c
+++ b/arch/arm/mach-at91/at91sam9x5.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9x5 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9x5_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
  }

+static void __init at91sam9x5_initialize(void)
+{
+	at91_pm_set_standby(at91sam9_standby);
+}
  /* --------------------------------------------------------------------
   *  Interrupt initialization
   * -------------------------------------------------------------------- */
@@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
  AT91_SOC_START(at91sam9x5)
  	.map_io = at91sam9x5_map_io,
  	.register_clocks = at91sam9x5_register_clocks,
+	.init = at91sam9x5_initialize,
  AT91_SOC_END
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index debdbf8..42b955c 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
  	.name = "cpuidle-at91",
  };

+void at91_pm_set_standby(void (*at91_standby)(void))
+{
+	at91_cpuidle_device.dev.platform_data = at91_standby;	
+}
+
  static int __init at91_pm_init(void)
  {
  #ifdef CONFIG_AT91_SLOW_CLOCK
@@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow 
clock mode)" : ""));

  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
-	if (cpu_is_at91rm9200()) {
-		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
+	if (cpu_is_at91rm9200())
  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
-	} else if (cpu_is_at91sam9g45()) {
-		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
-	} else if (cpu_is_at91sam9263()) {
-		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
-	} else {
-		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
-	}
  	
  	platform_device_register(&at91_cpuidle_device);

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 2f5908f..76dd1a7 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -11,9 +11,13 @@
  #ifndef __ARCH_ARM_MACH_AT91_PM
  #define __ARCH_ARM_MACH_AT91_PM

+#include <asm/proc-fns.h>
+
  #include <mach/at91_ramc.h>
  #include <mach/at91rm9200_sdramc.h>

+extern void at91_pm_set_standby(void (*at91_standby)(void));
+
  /*
   * The AT91RM9200 goes into self-refresh mode with this command, and will
   * terminate self-refresh automatically on the next SDRAM access.
diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
index 4012797..9a53db6 100644
--- a/arch/arm/mach-at91/sama5d3.c
+++ b/arch/arm/mach-at91/sama5d3.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9x5 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init sama5d3_map_io(void)
  {
  	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
  }

+static void __init sama5d3_initialize(void)
+{
+	at91_set_standby(at91sam9_standby);
+}
+
  AT91_SOC_START(sama5d3)
  	.map_io = sama5d3_map_io,
  	.register_clocks = sama5d3_register_clocks,
+	.init = sama5d3_initialize,
  AT91_SOC_END


>>   <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
>>


-- 
  <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


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 16:30         ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 16:30 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
>> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>>>> Use the platform driver model to separate the cpuidle specific code from
>>>> the low level pm code. It allows to remove the dependency between
>>>> these two components.
>>>>
>>>> Tested-on usb-a9263 (at91sam9263)
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[ ... ]

>>>> ---
>>>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>>>> -	if (cpu_is_at91rm9200())
>>>> +	if (cpu_is_at91rm9200()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>>>> +	} else if (cpu_is_at91sam9g45()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>>>> +	} else if (cpu_is_at91sam9263()) {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>>>> +	} else {
>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>>> no this is too dangerous when adding new SoC
>>>
>>> you must list the supported SoC
>>>
>>> and I prefer to move this code to the SoC init structure
>>
>> Do you mean register the platform_device in eg.
>> at91rm9200_initialize with the right platform data ?
>>
> yes as example
>
> Best Regards,

Hi Jean-Christophe,

I did the changes but there are different drawbacks with the approach.
The first one is the AT91_SOC_START is too early to call the 
platform_device_register. The second one is we have multiple declaration 
of the platform_device which is not really nice.

I found the following solution:

The platform_device is static in pm.c, there is a at91_pm_set_standby 
function. All AT91_SOC_START call at91_pm_set_standby with the right 
standby function callback, which in turns set the platform data for the 
cpuidle platform device. When the init call for pm is made, it registers 
the platform device.

That has the benefit to encapsulate the platform device and having it in 
a single place and also we have the information to remove the { if then 
else } statements we have in 'at91_pm_enter's case block for 
PM_SUSPEND_STANDBY.

Does it sound ok for you ?

Thanks
   -- Daniel

diff --git a/arch/arm/mach-at91/at91rm9200.c 
b/arch/arm/mach-at91/at91rm9200.c
index 4aad93d..0d234f2 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
  	/* Initialize GPIO subsystem */
  	at91_gpio_init(at91rm9200_gpio,
  		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
+
+	at91_pm_set_standby(at91rm9200_standby);
  }


diff --git a/arch/arm/mach-at91/at91sam9260.c 
b/arch/arm/mach-at91/at91sam9260.c
index 5de6074..9fd7aa6 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -28,6 +28,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9260 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9xe_map_io(void)
  {
  	unsigned long sram_size;
@@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9260_gpio, 3);
+
+	at91_pm_set_standby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9261.c 
b/arch/arm/mach-at91/at91sam9261.c
index 0e07932..1edbb6f 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9261_gpio, 3);
+
+	at91_pm_set_sandby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9263.c 
b/arch/arm/mach-at91/at91sam9263.c
index 6ce7d18..b222d59 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -26,6 +26,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9263 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9263_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
@@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9263_gpio, 5);
+
+	at91_pm_set_standby(at91sam9263_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9g45.c 
b/arch/arm/mach-at91/at91sam9g45.c
index 474ee04..4186f37 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -26,6 +26,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9G45 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9g45_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
@@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9g45_gpio, 5);
+
+	at91_pm_set_standby(at91sam9g45_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9n12.c 
b/arch/arm/mach-at91/at91sam9n12.c
index c7d670d..cc61e09 100644
--- a/arch/arm/mach-at91/at91sam9n12.c
+++ b/arch/arm/mach-at91/at91sam9n12.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9N12 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9n12_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
  }

+static void __init at91sam9n12_initialize(void)
+{
+	at91_pm_set_standby(at91sam9_standby);
+}
+
  AT91_SOC_START(at91sam9n12)
  	.map_io = at91sam9n12_map_io,
  	.register_clocks = at91sam9n12_register_clocks,
+	.init = at91sam9n12_initialize,
  AT91_SOC_END
diff --git a/arch/arm/mach-at91/at91sam9rl.c 
b/arch/arm/mach-at91/at91sam9rl.c
index d4ec0d9..b0c8cb6 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -27,6 +27,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[] 
__initdata = {
  /* --------------------------------------------------------------------
   *  AT91SAM9RL processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9rl_map_io(void)
  {
  	unsigned long sram_size;
@@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)

  	/* Register GPIO subsystem */
  	at91_gpio_init(at91sam9rl_gpio, 4);
+
+	at91_pm_set_standby(at91sam9_standby);
  }

  /* --------------------------------------------------------------------
diff --git a/arch/arm/mach-at91/at91sam9x5.c 
b/arch/arm/mach-at91/at91sam9x5.c
index 916e5a1..9ef7bc6 100644
--- a/arch/arm/mach-at91/at91sam9x5.c
+++ b/arch/arm/mach-at91/at91sam9x5.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9x5 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init at91sam9x5_map_io(void)
  {
  	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
  }

+static void __init at91sam9x5_initialize(void)
+{
+	at91_pm_set_standby(at91sam9_standby);
+}
  /* --------------------------------------------------------------------
   *  Interrupt initialization
   * -------------------------------------------------------------------- */
@@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
  AT91_SOC_START(at91sam9x5)
  	.map_io = at91sam9x5_map_io,
  	.register_clocks = at91sam9x5_register_clocks,
+	.init = at91sam9x5_initialize,
  AT91_SOC_END
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index debdbf8..42b955c 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
  	.name = "cpuidle-at91",
  };

+void at91_pm_set_standby(void (*at91_standby)(void))
+{
+	at91_cpuidle_device.dev.platform_data = at91_standby;	
+}
+
  static int __init at91_pm_init(void)
  {
  #ifdef CONFIG_AT91_SLOW_CLOCK
@@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow 
clock mode)" : ""));

  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
-	if (cpu_is_at91rm9200()) {
-		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
+	if (cpu_is_at91rm9200())
  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
-	} else if (cpu_is_at91sam9g45()) {
-		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
-	} else if (cpu_is_at91sam9263()) {
-		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
-	} else {
-		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
-	}
  	
  	platform_device_register(&at91_cpuidle_device);

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 2f5908f..76dd1a7 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -11,9 +11,13 @@
  #ifndef __ARCH_ARM_MACH_AT91_PM
  #define __ARCH_ARM_MACH_AT91_PM

+#include <asm/proc-fns.h>
+
  #include <mach/at91_ramc.h>
  #include <mach/at91rm9200_sdramc.h>

+extern void at91_pm_set_standby(void (*at91_standby)(void));
+
  /*
   * The AT91RM9200 goes into self-refresh mode with this command, and will
   * terminate self-refresh automatically on the next SDRAM access.
diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
index 4012797..9a53db6 100644
--- a/arch/arm/mach-at91/sama5d3.c
+++ b/arch/arm/mach-at91/sama5d3.c
@@ -21,6 +21,7 @@
  #include "generic.h"
  #include "clock.h"
  #include "sam9_smc.h"
+#include "pm.h"

  /* --------------------------------------------------------------------
   *  Clocks
@@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
  /* --------------------------------------------------------------------
   *  AT91SAM9x5 processor initialization
   * -------------------------------------------------------------------- */
-
  static void __init sama5d3_map_io(void)
  {
  	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
  }

+static void __init sama5d3_initialize(void)
+{
+	at91_set_standby(at91sam9_standby);
+}
+
  AT91_SOC_START(sama5d3)
  	.map_io = sama5d3_map_io,
  	.register_clocks = sama5d3_register_clocks,
+	.init = sama5d3_initialize,
  AT91_SOC_END


>>   <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
>>


-- 
  <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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-14 16:30         ` Daniel Lezcano
@ 2013-10-14 17:18           ` Jean-Christophe PLAGNIOL-VILLARD
  -1 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 17:18 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linaro-kernel, linux-pm, patches, nicolas.ferre, linux, linux-arm-kernel

On 18:30 Mon 14 Oct     , Daniel Lezcano wrote:
> On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
> >>On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> >>>>Use the platform driver model to separate the cpuidle specific code from
> >>>>the low level pm code. It allows to remove the dependency between
> >>>>these two components.
> >>>>
> >>>>Tested-on usb-a9263 (at91sam9263)
> >>>>
> >>>>Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> [ ... ]
> 
> >>>>---
> >>>>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> >>>>-	if (cpu_is_at91rm9200())
> >>>>+	if (cpu_is_at91rm9200()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> >>>>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> >>>>+	} else if (cpu_is_at91sam9g45()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> >>>>+	} else if (cpu_is_at91sam9263()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> >>>>+	} else {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> >>>no this is too dangerous when adding new SoC
> >>>
> >>>you must list the supported SoC
> >>>
> >>>and I prefer to move this code to the SoC init structure
> >>
> >>Do you mean register the platform_device in eg.
> >>at91rm9200_initialize with the right platform data ?
> >>
> >yes as example
> >
> >Best Regards,
> 
> Hi Jean-Christophe,
> 
> I did the changes but there are different drawbacks with the approach.
> The first one is the AT91_SOC_START is too early to call the
> platform_device_register. The second one is we have multiple
> declaration of the platform_device which is not really nice.
> 
> I found the following solution:
> 
> The platform_device is static in pm.c, there is a
> at91_pm_set_standby function. All AT91_SOC_START call
> at91_pm_set_standby with the right standby function callback, which
> in turns set the platform data for the cpuidle platform device. When
> the init call for pm is made, it registers the platform device.
> 
> That has the benefit to encapsulate the platform device and having
> it in a single place and also we have the information to remove the
> { if then else } statements we have in 'at91_pm_enter's case block
> for PM_SUSPEND_STANDBY.
why not

but some comment

and that was one of the reason why the else in the previous implementation was
wrong
> 
> Does it sound ok for you ?
> 
> Thanks
>   -- Daniel
> 
> diff --git a/arch/arm/mach-at91/at91rm9200.c
> b/arch/arm/mach-at91/at91rm9200.c
> index 4aad93d..0d234f2 100644
> --- a/arch/arm/mach-at91/at91rm9200.c
> +++ b/arch/arm/mach-at91/at91rm9200.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
>  	/* Initialize GPIO subsystem */
>  	at91_gpio_init(at91rm9200_gpio,
>  		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
> +
> +	at91_pm_set_standby(at91rm9200_standby);
>  }
> 
> 
> diff --git a/arch/arm/mach-at91/at91sam9260.c
> b/arch/arm/mach-at91/at91sam9260.c
> index 5de6074..9fd7aa6 100644
> --- a/arch/arm/mach-at91/at91sam9260.c
> +++ b/arch/arm/mach-at91/at91sam9260.c
> @@ -28,6 +28,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9260 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9xe_map_io(void)
>  {
>  	unsigned long sram_size;
> @@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9260_gpio, 3);
> +
> +	at91_pm_set_standby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9261.c
> b/arch/arm/mach-at91/at91sam9261.c
> index 0e07932..1edbb6f 100644
> --- a/arch/arm/mach-at91/at91sam9261.c
> +++ b/arch/arm/mach-at91/at91sam9261.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9261_gpio, 3);
> +
> +	at91_pm_set_sandby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9263.c
> b/arch/arm/mach-at91/at91sam9263.c
> index 6ce7d18..b222d59 100644
> --- a/arch/arm/mach-at91/at91sam9263.c
> +++ b/arch/arm/mach-at91/at91sam9263.c
> @@ -26,6 +26,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9263 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9263_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
> @@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9263_gpio, 5);
> +
> +	at91_pm_set_standby(at91sam9263_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9g45.c
> b/arch/arm/mach-at91/at91sam9g45.c
> index 474ee04..4186f37 100644
> --- a/arch/arm/mach-at91/at91sam9g45.c
> +++ b/arch/arm/mach-at91/at91sam9g45.c
> @@ -26,6 +26,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9G45 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9g45_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
> @@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9g45_gpio, 5);
> +
> +	at91_pm_set_standby(at91sam9g45_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9n12.c
> b/arch/arm/mach-at91/at91sam9n12.c
> index c7d670d..cc61e09 100644
> --- a/arch/arm/mach-at91/at91sam9n12.c
> +++ b/arch/arm/mach-at91/at91sam9n12.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9N12 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9n12_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
>  }
> 
> +static void __init at91sam9n12_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_pm_set_standby(at91sam9_standby);
> +}
> +
>  AT91_SOC_START(at91sam9n12)
>  	.map_io = at91sam9n12_map_io,
>  	.register_clocks = at91sam9n12_register_clocks,
> +	.init = at91sam9n12_initialize,
>  AT91_SOC_END
> diff --git a/arch/arm/mach-at91/at91sam9rl.c
> b/arch/arm/mach-at91/at91sam9rl.c
> index d4ec0d9..b0c8cb6 100644
> --- a/arch/arm/mach-at91/at91sam9rl.c
> +++ b/arch/arm/mach-at91/at91sam9rl.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9RL processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9rl_map_io(void)
>  {
>  	unsigned long sram_size;
> @@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9rl_gpio, 4);
> +
> +	at91_pm_set_standby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9x5.c
> b/arch/arm/mach-at91/at91sam9x5.c
> index 916e5a1..9ef7bc6 100644
> --- a/arch/arm/mach-at91/at91sam9x5.c
> +++ b/arch/arm/mach-at91/at91sam9x5.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9x5 processor initialization
>   * -------------------------------------------------------------------- */
> -
??
>  static void __init at91sam9x5_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
>  }
> 
> +static void __init at91sam9x5_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_pm_set_standby(at91sam9_standby);
> +}
>  /* --------------------------------------------------------------------
>   *  Interrupt initialization
>   * -------------------------------------------------------------------- */
> @@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
>  AT91_SOC_START(at91sam9x5)
>  	.map_io = at91sam9x5_map_io,
>  	.register_clocks = at91sam9x5_register_clocks,
> +	.init = at91sam9x5_initialize,
>  AT91_SOC_END
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index debdbf8..42b955c 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
>  	.name = "cpuidle-at91",
>  };
> 
> +void at91_pm_set_standby(void (*at91_standby)(void))
> +{
> +	at91_cpuidle_device.dev.platform_data = at91_standby;	
> +}
> +
>  static int __init at91_pm_init(void)
>  {
>  #ifdef CONFIG_AT91_SLOW_CLOCK
> @@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow
> clock mode)" : ""));
> 
>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> -	if (cpu_is_at91rm9200()) {
> -		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> +	if (cpu_is_at91rm9200())
>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> -	} else if (cpu_is_at91sam9g45()) {
> -		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> -	} else if (cpu_is_at91sam9263()) {
> -		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> -	} else {
> -		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> -	}
>  	

only register it if the call back is set
>  	platform_device_register(&at91_cpuidle_device);
> 
> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
> index 2f5908f..76dd1a7 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -11,9 +11,13 @@
>  #ifndef __ARCH_ARM_MACH_AT91_PM
>  #define __ARCH_ARM_MACH_AT91_PM
> 
> +#include <asm/proc-fns.h>
> +
>  #include <mach/at91_ramc.h>
>  #include <mach/at91rm9200_sdramc.h>
> 
> +extern void at91_pm_set_standby(void (*at91_standby)(void));
> +
>  /*
>   * The AT91RM9200 goes into self-refresh mode with this command, and will
>   * terminate self-refresh automatically on the next SDRAM access.
> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
> index 4012797..9a53db6 100644
> --- a/arch/arm/mach-at91/sama5d3.c
> +++ b/arch/arm/mach-at91/sama5d3.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9x5 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init sama5d3_map_io(void)
>  {
>  	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
>  }
> 
> +static void __init sama5d3_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_set_standby(at91sam9_standby);
> +}
> +
>  AT91_SOC_START(sama5d3)
>  	.map_io = sama5d3_map_io,
>  	.register_clocks = sama5d3_register_clocks,
> +	.init = sama5d3_initialize,
>  AT91_SOC_END
> 
> 

Best Regards,
J.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 17:18           ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 24+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-10-14 17:18 UTC (permalink / raw)
  To: linux-arm-kernel

On 18:30 Mon 14 Oct     , Daniel Lezcano wrote:
> On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
> >>On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>>On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
> >>>>Use the platform driver model to separate the cpuidle specific code from
> >>>>the low level pm code. It allows to remove the dependency between
> >>>>these two components.
> >>>>
> >>>>Tested-on usb-a9263 (at91sam9263)
> >>>>
> >>>>Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> [ ... ]
> 
> >>>>---
> >>>>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> >>>>-	if (cpu_is_at91rm9200())
> >>>>+	if (cpu_is_at91rm9200()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> >>>>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> >>>>+	} else if (cpu_is_at91sam9g45()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> >>>>+	} else if (cpu_is_at91sam9263()) {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> >>>>+	} else {
> >>>>+		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> >>>no this is too dangerous when adding new SoC
> >>>
> >>>you must list the supported SoC
> >>>
> >>>and I prefer to move this code to the SoC init structure
> >>
> >>Do you mean register the platform_device in eg.
> >>at91rm9200_initialize with the right platform data ?
> >>
> >yes as example
> >
> >Best Regards,
> 
> Hi Jean-Christophe,
> 
> I did the changes but there are different drawbacks with the approach.
> The first one is the AT91_SOC_START is too early to call the
> platform_device_register. The second one is we have multiple
> declaration of the platform_device which is not really nice.
> 
> I found the following solution:
> 
> The platform_device is static in pm.c, there is a
> at91_pm_set_standby function. All AT91_SOC_START call
> at91_pm_set_standby with the right standby function callback, which
> in turns set the platform data for the cpuidle platform device. When
> the init call for pm is made, it registers the platform device.
> 
> That has the benefit to encapsulate the platform device and having
> it in a single place and also we have the information to remove the
> { if then else } statements we have in 'at91_pm_enter's case block
> for PM_SUSPEND_STANDBY.
why not

but some comment

and that was one of the reason why the else in the previous implementation was
wrong
> 
> Does it sound ok for you ?
> 
> Thanks
>   -- Daniel
> 
> diff --git a/arch/arm/mach-at91/at91rm9200.c
> b/arch/arm/mach-at91/at91rm9200.c
> index 4aad93d..0d234f2 100644
> --- a/arch/arm/mach-at91/at91rm9200.c
> +++ b/arch/arm/mach-at91/at91rm9200.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
>  	/* Initialize GPIO subsystem */
>  	at91_gpio_init(at91rm9200_gpio,
>  		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
> +
> +	at91_pm_set_standby(at91rm9200_standby);
>  }
> 
> 
> diff --git a/arch/arm/mach-at91/at91sam9260.c
> b/arch/arm/mach-at91/at91sam9260.c
> index 5de6074..9fd7aa6 100644
> --- a/arch/arm/mach-at91/at91sam9260.c
> +++ b/arch/arm/mach-at91/at91sam9260.c
> @@ -28,6 +28,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9260 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9xe_map_io(void)
>  {
>  	unsigned long sram_size;
> @@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9260_gpio, 3);
> +
> +	at91_pm_set_standby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9261.c
> b/arch/arm/mach-at91/at91sam9261.c
> index 0e07932..1edbb6f 100644
> --- a/arch/arm/mach-at91/at91sam9261.c
> +++ b/arch/arm/mach-at91/at91sam9261.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9261_gpio, 3);
> +
> +	at91_pm_set_sandby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9263.c
> b/arch/arm/mach-at91/at91sam9263.c
> index 6ce7d18..b222d59 100644
> --- a/arch/arm/mach-at91/at91sam9263.c
> +++ b/arch/arm/mach-at91/at91sam9263.c
> @@ -26,6 +26,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9263 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9263_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
> @@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9263_gpio, 5);
> +
> +	at91_pm_set_standby(at91sam9263_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9g45.c
> b/arch/arm/mach-at91/at91sam9g45.c
> index 474ee04..4186f37 100644
> --- a/arch/arm/mach-at91/at91sam9g45.c
> +++ b/arch/arm/mach-at91/at91sam9g45.c
> @@ -26,6 +26,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9G45 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9g45_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
> @@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9g45_gpio, 5);
> +
> +	at91_pm_set_standby(at91sam9g45_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9n12.c
> b/arch/arm/mach-at91/at91sam9n12.c
> index c7d670d..cc61e09 100644
> --- a/arch/arm/mach-at91/at91sam9n12.c
> +++ b/arch/arm/mach-at91/at91sam9n12.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9N12 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9n12_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
>  }
> 
> +static void __init at91sam9n12_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_pm_set_standby(at91sam9_standby);
> +}
> +
>  AT91_SOC_START(at91sam9n12)
>  	.map_io = at91sam9n12_map_io,
>  	.register_clocks = at91sam9n12_register_clocks,
> +	.init = at91sam9n12_initialize,
>  AT91_SOC_END
> diff --git a/arch/arm/mach-at91/at91sam9rl.c
> b/arch/arm/mach-at91/at91sam9rl.c
> index d4ec0d9..b0c8cb6 100644
> --- a/arch/arm/mach-at91/at91sam9rl.c
> +++ b/arch/arm/mach-at91/at91sam9rl.c
> @@ -27,6 +27,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[]
> __initdata = {
>  /* --------------------------------------------------------------------
>   *  AT91SAM9RL processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init at91sam9rl_map_io(void)
>  {
>  	unsigned long sram_size;
> @@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)
> 
>  	/* Register GPIO subsystem */
>  	at91_gpio_init(at91sam9rl_gpio, 4);
> +
> +	at91_pm_set_standby(at91sam9_standby);
>  }
> 
>  /* --------------------------------------------------------------------
> diff --git a/arch/arm/mach-at91/at91sam9x5.c
> b/arch/arm/mach-at91/at91sam9x5.c
> index 916e5a1..9ef7bc6 100644
> --- a/arch/arm/mach-at91/at91sam9x5.c
> +++ b/arch/arm/mach-at91/at91sam9x5.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9x5 processor initialization
>   * -------------------------------------------------------------------- */
> -
??
>  static void __init at91sam9x5_map_io(void)
>  {
>  	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
>  }
> 
> +static void __init at91sam9x5_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_pm_set_standby(at91sam9_standby);
> +}
>  /* --------------------------------------------------------------------
>   *  Interrupt initialization
>   * -------------------------------------------------------------------- */
> @@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
>  AT91_SOC_START(at91sam9x5)
>  	.map_io = at91sam9x5_map_io,
>  	.register_clocks = at91sam9x5_register_clocks,
> +	.init = at91sam9x5_initialize,
>  AT91_SOC_END
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index debdbf8..42b955c 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
>  	.name = "cpuidle-at91",
>  };
> 
> +void at91_pm_set_standby(void (*at91_standby)(void))
> +{
> +	at91_cpuidle_device.dev.platform_data = at91_standby;	
> +}
> +
>  static int __init at91_pm_init(void)
>  {
>  #ifdef CONFIG_AT91_SLOW_CLOCK
> @@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
>  	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow
> clock mode)" : ""));
> 
>  	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
> -	if (cpu_is_at91rm9200()) {
> -		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
> +	if (cpu_is_at91rm9200())
>  		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
> -	} else if (cpu_is_at91sam9g45()) {
> -		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
> -	} else if (cpu_is_at91sam9263()) {
> -		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
> -	} else {
> -		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
> -	}
>  	

only register it if the call back is set
>  	platform_device_register(&at91_cpuidle_device);
> 
> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
> index 2f5908f..76dd1a7 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -11,9 +11,13 @@
>  #ifndef __ARCH_ARM_MACH_AT91_PM
>  #define __ARCH_ARM_MACH_AT91_PM
> 
> +#include <asm/proc-fns.h>
> +
>  #include <mach/at91_ramc.h>
>  #include <mach/at91rm9200_sdramc.h>
> 
> +extern void at91_pm_set_standby(void (*at91_standby)(void));
> +
>  /*
>   * The AT91RM9200 goes into self-refresh mode with this command, and will
>   * terminate self-refresh automatically on the next SDRAM access.
> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
> index 4012797..9a53db6 100644
> --- a/arch/arm/mach-at91/sama5d3.c
> +++ b/arch/arm/mach-at91/sama5d3.c
> @@ -21,6 +21,7 @@
>  #include "generic.h"
>  #include "clock.h"
>  #include "sam9_smc.h"
> +#include "pm.h"
> 
>  /* --------------------------------------------------------------------
>   *  Clocks
> @@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
>  /* --------------------------------------------------------------------
>   *  AT91SAM9x5 processor initialization
>   * -------------------------------------------------------------------- */
> -
>  static void __init sama5d3_map_io(void)
>  {
>  	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
>  }
> 
> +static void __init sama5d3_initialize(void)
> +{
this is wrong we use a ddr comtroller not the same as the sam9
> +	at91_set_standby(at91sam9_standby);
> +}
> +
>  AT91_SOC_START(sama5d3)
>  	.map_io = sama5d3_map_io,
>  	.register_clocks = sama5d3_register_clocks,
> +	.init = sama5d3_initialize,
>  AT91_SOC_END
> 
> 

Best Regards,
J.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
  2013-10-14 17:18           ` Jean-Christophe PLAGNIOL-VILLARD
@ 2013-10-14 17:59             ` Daniel Lezcano
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 17:59 UTC (permalink / raw)
  To: Jean-Christophe PLAGNIOL-VILLARD
  Cc: linux, nicolas.ferre, linux-arm-kernel, patches, linaro-kernel, linux-pm

On 10/14/2013 07:18 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:30 Mon 14 Oct     , Daniel Lezcano wrote:
>> On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
>>>> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>>>>>> Use the platform driver model to separate the cpuidle specific code from
>>>>>> the low level pm code. It allows to remove the dependency between
>>>>>> these two components.
>>>>>>
>>>>>> Tested-on usb-a9263 (at91sam9263)
>>>>>>
>>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> [ ... ]
>>
>>>>>> ---
>>>>>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>>>>>> -	if (cpu_is_at91rm9200())
>>>>>> +	if (cpu_is_at91rm9200()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>>>>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>>>>>> +	} else if (cpu_is_at91sam9g45()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>>>>>> +	} else if (cpu_is_at91sam9263()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>>>>>> +	} else {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>>>>> no this is too dangerous when adding new SoC
>>>>>
>>>>> you must list the supported SoC
>>>>>
>>>>> and I prefer to move this code to the SoC init structure
>>>>
>>>> Do you mean register the platform_device in eg.
>>>> at91rm9200_initialize with the right platform data ?
>>>>
>>> yes as example
>>>
>>> Best Regards,
>>
>> Hi Jean-Christophe,
>>
>> I did the changes but there are different drawbacks with the approach.
>> The first one is the AT91_SOC_START is too early to call the
>> platform_device_register. The second one is we have multiple
>> declaration of the platform_device which is not really nice.
>>
>> I found the following solution:
>>
>> The platform_device is static in pm.c, there is a
>> at91_pm_set_standby function. All AT91_SOC_START call
>> at91_pm_set_standby with the right standby function callback, which
>> in turns set the platform data for the cpuidle platform device. When
>> the init call for pm is made, it registers the platform device.
>>
>> That has the benefit to encapsulate the platform device and having
>> it in a single place and also we have the information to remove the
>> { if then else } statements we have in 'at91_pm_enter's case block
>> for PM_SUSPEND_STANDBY.
> why not
>
> but some comment
>
> and that was one of the reason why the else in the previous implementation was
> wrong

Ok, I see.

I will send a V2.

Thanks for the review !

   -- Daniel

>>
>> Does it sound ok for you ?
>>
>> Thanks
>>    -- Daniel
>>
>> diff --git a/arch/arm/mach-at91/at91rm9200.c
>> b/arch/arm/mach-at91/at91rm9200.c
>> index 4aad93d..0d234f2 100644
>> --- a/arch/arm/mach-at91/at91rm9200.c
>> +++ b/arch/arm/mach-at91/at91rm9200.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
>>   	/* Initialize GPIO subsystem */
>>   	at91_gpio_init(at91rm9200_gpio,
>>   		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
>> +
>> +	at91_pm_set_standby(at91rm9200_standby);
>>   }
>>
>>
>> diff --git a/arch/arm/mach-at91/at91sam9260.c
>> b/arch/arm/mach-at91/at91sam9260.c
>> index 5de6074..9fd7aa6 100644
>> --- a/arch/arm/mach-at91/at91sam9260.c
>> +++ b/arch/arm/mach-at91/at91sam9260.c
>> @@ -28,6 +28,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9260 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9xe_map_io(void)
>>   {
>>   	unsigned long sram_size;
>> @@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9260_gpio, 3);
>> +
>> +	at91_pm_set_standby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9261.c
>> b/arch/arm/mach-at91/at91sam9261.c
>> index 0e07932..1edbb6f 100644
>> --- a/arch/arm/mach-at91/at91sam9261.c
>> +++ b/arch/arm/mach-at91/at91sam9261.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9261_gpio, 3);
>> +
>> +	at91_pm_set_sandby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9263.c
>> b/arch/arm/mach-at91/at91sam9263.c
>> index 6ce7d18..b222d59 100644
>> --- a/arch/arm/mach-at91/at91sam9263.c
>> +++ b/arch/arm/mach-at91/at91sam9263.c
>> @@ -26,6 +26,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9263 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9263_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
>> @@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9263_gpio, 5);
>> +
>> +	at91_pm_set_standby(at91sam9263_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9g45.c
>> b/arch/arm/mach-at91/at91sam9g45.c
>> index 474ee04..4186f37 100644
>> --- a/arch/arm/mach-at91/at91sam9g45.c
>> +++ b/arch/arm/mach-at91/at91sam9g45.c
>> @@ -26,6 +26,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9G45 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9g45_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
>> @@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9g45_gpio, 5);
>> +
>> +	at91_pm_set_standby(at91sam9g45_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9n12.c
>> b/arch/arm/mach-at91/at91sam9n12.c
>> index c7d670d..cc61e09 100644
>> --- a/arch/arm/mach-at91/at91sam9n12.c
>> +++ b/arch/arm/mach-at91/at91sam9n12.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9N12 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9n12_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
>>   }
>>
>> +static void __init at91sam9n12_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_pm_set_standby(at91sam9_standby);
>> +}
>> +
>>   AT91_SOC_START(at91sam9n12)
>>   	.map_io = at91sam9n12_map_io,
>>   	.register_clocks = at91sam9n12_register_clocks,
>> +	.init = at91sam9n12_initialize,
>>   AT91_SOC_END
>> diff --git a/arch/arm/mach-at91/at91sam9rl.c
>> b/arch/arm/mach-at91/at91sam9rl.c
>> index d4ec0d9..b0c8cb6 100644
>> --- a/arch/arm/mach-at91/at91sam9rl.c
>> +++ b/arch/arm/mach-at91/at91sam9rl.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9RL processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9rl_map_io(void)
>>   {
>>   	unsigned long sram_size;
>> @@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9rl_gpio, 4);
>> +
>> +	at91_pm_set_standby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9x5.c
>> b/arch/arm/mach-at91/at91sam9x5.c
>> index 916e5a1..9ef7bc6 100644
>> --- a/arch/arm/mach-at91/at91sam9x5.c
>> +++ b/arch/arm/mach-at91/at91sam9x5.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9x5 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
> ??
>>   static void __init at91sam9x5_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
>>   }
>>
>> +static void __init at91sam9x5_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_pm_set_standby(at91sam9_standby);
>> +}
>>   /* --------------------------------------------------------------------
>>    *  Interrupt initialization
>>    * -------------------------------------------------------------------- */
>> @@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
>>   AT91_SOC_START(at91sam9x5)
>>   	.map_io = at91sam9x5_map_io,
>>   	.register_clocks = at91sam9x5_register_clocks,
>> +	.init = at91sam9x5_initialize,
>>   AT91_SOC_END
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index debdbf8..42b955c 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
>>   	.name = "cpuidle-at91",
>>   };
>>
>> +void at91_pm_set_standby(void (*at91_standby)(void))
>> +{
>> +	at91_cpuidle_device.dev.platform_data = at91_standby;	
>> +}
>> +
>>   static int __init at91_pm_init(void)
>>   {
>>   #ifdef CONFIG_AT91_SLOW_CLOCK
>> @@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
>>   	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow
>> clock mode)" : ""));
>>
>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>> -	if (cpu_is_at91rm9200()) {
>> -		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>> +	if (cpu_is_at91rm9200())
>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>> -	} else if (cpu_is_at91sam9g45()) {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>> -	} else if (cpu_is_at91sam9263()) {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>> -	} else {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>> -	}
>>   	
>
> only register it if the call back is set
>>   	platform_device_register(&at91_cpuidle_device);
>>
>> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
>> index 2f5908f..76dd1a7 100644
>> --- a/arch/arm/mach-at91/pm.h
>> +++ b/arch/arm/mach-at91/pm.h
>> @@ -11,9 +11,13 @@
>>   #ifndef __ARCH_ARM_MACH_AT91_PM
>>   #define __ARCH_ARM_MACH_AT91_PM
>>
>> +#include <asm/proc-fns.h>
>> +
>>   #include <mach/at91_ramc.h>
>>   #include <mach/at91rm9200_sdramc.h>
>>
>> +extern void at91_pm_set_standby(void (*at91_standby)(void));
>> +
>>   /*
>>    * The AT91RM9200 goes into self-refresh mode with this command, and will
>>    * terminate self-refresh automatically on the next SDRAM access.
>> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
>> index 4012797..9a53db6 100644
>> --- a/arch/arm/mach-at91/sama5d3.c
>> +++ b/arch/arm/mach-at91/sama5d3.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9x5 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init sama5d3_map_io(void)
>>   {
>>   	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
>>   }
>>
>> +static void __init sama5d3_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_set_standby(at91sam9_standby);
>> +}
>> +
>>   AT91_SOC_START(sama5d3)
>>   	.map_io = sama5d3_map_io,
>>   	.register_clocks = sama5d3_register_clocks,
>> +	.init = sama5d3_initialize,
>>   AT91_SOC_END
>>
>>
>
> Best Regards,
> J.
>


-- 
  <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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver
@ 2013-10-14 17:59             ` Daniel Lezcano
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2013-10-14 17:59 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/14/2013 07:18 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:30 Mon 14 Oct     , Daniel Lezcano wrote:
>> On 10/14/2013 04:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 16:18 Mon 14 Oct     , Daniel Lezcano wrote:
>>>> On 10/14/2013 04:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> On 17:51 Sat 12 Oct     , Daniel Lezcano wrote:
>>>>>> Use the platform driver model to separate the cpuidle specific code from
>>>>>> the low level pm code. It allows to remove the dependency between
>>>>>> these two components.
>>>>>>
>>>>>> Tested-on usb-a9263 (at91sam9263)
>>>>>>
>>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> [ ... ]
>>
>>>>>> ---
>>>>>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>>>>>> -	if (cpu_is_at91rm9200())
>>>>>> +	if (cpu_is_at91rm9200()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>>>>>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>>>>>> +	} else if (cpu_is_at91sam9g45()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>>>>>> +	} else if (cpu_is_at91sam9263()) {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>>>>>> +	} else {
>>>>>> +		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>>>>> no this is too dangerous when adding new SoC
>>>>>
>>>>> you must list the supported SoC
>>>>>
>>>>> and I prefer to move this code to the SoC init structure
>>>>
>>>> Do you mean register the platform_device in eg.
>>>> at91rm9200_initialize with the right platform data ?
>>>>
>>> yes as example
>>>
>>> Best Regards,
>>
>> Hi Jean-Christophe,
>>
>> I did the changes but there are different drawbacks with the approach.
>> The first one is the AT91_SOC_START is too early to call the
>> platform_device_register. The second one is we have multiple
>> declaration of the platform_device which is not really nice.
>>
>> I found the following solution:
>>
>> The platform_device is static in pm.c, there is a
>> at91_pm_set_standby function. All AT91_SOC_START call
>> at91_pm_set_standby with the right standby function callback, which
>> in turns set the platform data for the cpuidle platform device. When
>> the init call for pm is made, it registers the platform device.
>>
>> That has the benefit to encapsulate the platform device and having
>> it in a single place and also we have the information to remove the
>> { if then else } statements we have in 'at91_pm_enter's case block
>> for PM_SUSPEND_STANDBY.
> why not
>
> but some comment
>
> and that was one of the reason why the else in the previous implementation was
> wrong

Ok, I see.

I will send a V2.

Thanks for the review !

   -- Daniel

>>
>> Does it sound ok for you ?
>>
>> Thanks
>>    -- Daniel
>>
>> diff --git a/arch/arm/mach-at91/at91rm9200.c
>> b/arch/arm/mach-at91/at91rm9200.c
>> index 4aad93d..0d234f2 100644
>> --- a/arch/arm/mach-at91/at91rm9200.c
>> +++ b/arch/arm/mach-at91/at91rm9200.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -337,6 +338,8 @@ static void __init at91rm9200_initialize(void)
>>   	/* Initialize GPIO subsystem */
>>   	at91_gpio_init(at91rm9200_gpio,
>>   		cpu_is_at91rm9200_bga() ? AT91RM9200_BGA : AT91RM9200_PQFP);
>> +
>> +	at91_pm_set_standby(at91rm9200_standby);
>>   }
>>
>>
>> diff --git a/arch/arm/mach-at91/at91sam9260.c
>> b/arch/arm/mach-at91/at91sam9260.c
>> index 5de6074..9fd7aa6 100644
>> --- a/arch/arm/mach-at91/at91sam9260.c
>> +++ b/arch/arm/mach-at91/at91sam9260.c
>> @@ -28,6 +28,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -307,7 +308,6 @@ static struct at91_gpio_bank at91sam9260_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9260 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9xe_map_io(void)
>>   {
>>   	unsigned long sram_size;
>> @@ -351,6 +351,8 @@ static void __init at91sam9260_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9260_gpio, 3);
>> +
>> +	at91_pm_set_standby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9261.c
>> b/arch/arm/mach-at91/at91sam9261.c
>> index 0e07932..1edbb6f 100644
>> --- a/arch/arm/mach-at91/at91sam9261.c
>> +++ b/arch/arm/mach-at91/at91sam9261.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -293,6 +294,8 @@ static void __init at91sam9261_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9261_gpio, 3);
>> +
>> +	at91_pm_set_sandby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9263.c
>> b/arch/arm/mach-at91/at91sam9263.c
>> index 6ce7d18..b222d59 100644
>> --- a/arch/arm/mach-at91/at91sam9263.c
>> +++ b/arch/arm/mach-at91/at91sam9263.c
>> @@ -26,6 +26,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -304,7 +305,6 @@ static struct at91_gpio_bank at91sam9263_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9263 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9263_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9263_SRAM0_BASE, AT91SAM9263_SRAM0_SIZE);
>> @@ -330,6 +330,8 @@ static void __init at91sam9263_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9263_gpio, 5);
>> +
>> +	at91_pm_set_standby(at91sam9263_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9g45.c
>> b/arch/arm/mach-at91/at91sam9g45.c
>> index 474ee04..4186f37 100644
>> --- a/arch/arm/mach-at91/at91sam9g45.c
>> +++ b/arch/arm/mach-at91/at91sam9g45.c
>> @@ -26,6 +26,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -355,7 +356,6 @@ static struct at91_gpio_bank at91sam9g45_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9G45 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9g45_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9G45_SRAM_BASE, AT91SAM9G45_SRAM_SIZE);
>> @@ -379,6 +379,8 @@ static void __init at91sam9g45_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9g45_gpio, 5);
>> +
>> +	at91_pm_set_standby(at91sam9g45_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9n12.c
>> b/arch/arm/mach-at91/at91sam9n12.c
>> index c7d670d..cc61e09 100644
>> --- a/arch/arm/mach-at91/at91sam9n12.c
>> +++ b/arch/arm/mach-at91/at91sam9n12.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -217,13 +218,18 @@ static void __init at91sam9n12_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9N12 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9n12_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9N12_SRAM_BASE, AT91SAM9N12_SRAM_SIZE);
>>   }
>>
>> +static void __init at91sam9n12_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_pm_set_standby(at91sam9_standby);
>> +}
>> +
>>   AT91_SOC_START(at91sam9n12)
>>   	.map_io = at91sam9n12_map_io,
>>   	.register_clocks = at91sam9n12_register_clocks,
>> +	.init = at91sam9n12_initialize,
>>   AT91_SOC_END
>> diff --git a/arch/arm/mach-at91/at91sam9rl.c
>> b/arch/arm/mach-at91/at91sam9rl.c
>> index d4ec0d9..b0c8cb6 100644
>> --- a/arch/arm/mach-at91/at91sam9rl.c
>> +++ b/arch/arm/mach-at91/at91sam9rl.c
>> @@ -27,6 +27,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -261,7 +262,6 @@ static struct at91_gpio_bank at91sam9rl_gpio[]
>> __initdata = {
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9RL processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init at91sam9rl_map_io(void)
>>   {
>>   	unsigned long sram_size;
>> @@ -296,6 +296,8 @@ static void __init at91sam9rl_initialize(void)
>>
>>   	/* Register GPIO subsystem */
>>   	at91_gpio_init(at91sam9rl_gpio, 4);
>> +
>> +	at91_pm_set_standby(at91sam9_standby);
>>   }
>>
>>   /* --------------------------------------------------------------------
>> diff --git a/arch/arm/mach-at91/at91sam9x5.c
>> b/arch/arm/mach-at91/at91sam9x5.c
>> index 916e5a1..9ef7bc6 100644
>> --- a/arch/arm/mach-at91/at91sam9x5.c
>> +++ b/arch/arm/mach-at91/at91sam9x5.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -316,12 +317,15 @@ static void __init at91sam9x5_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9x5 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
> ??
>>   static void __init at91sam9x5_map_io(void)
>>   {
>>   	at91_init_sram(0, AT91SAM9X5_SRAM_BASE, AT91SAM9X5_SRAM_SIZE);
>>   }
>>
>> +static void __init at91sam9x5_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_pm_set_standby(at91sam9_standby);
>> +}
>>   /* --------------------------------------------------------------------
>>    *  Interrupt initialization
>>    * -------------------------------------------------------------------- */
>> @@ -329,4 +333,5 @@ static void __init at91sam9x5_map_io(void)
>>   AT91_SOC_START(at91sam9x5)
>>   	.map_io = at91sam9x5_map_io,
>>   	.register_clocks = at91sam9x5_register_clocks,
>> +	.init = at91sam9x5_initialize,
>>   AT91_SOC_END
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index debdbf8..42b955c 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -318,6 +318,11 @@ static struct platform_device at91_cpuidle_device = {
>>   	.name = "cpuidle-at91",
>>   };
>>
>> +void at91_pm_set_standby(void (*at91_standby)(void))
>> +{
>> +	at91_cpuidle_device.dev.platform_data = at91_standby;	
>> +}
>> +
>>   static int __init at91_pm_init(void)
>>   {
>>   #ifdef CONFIG_AT91_SLOW_CLOCK
>> @@ -327,16 +332,8 @@ static int __init at91_pm_init(void)
>>   	pr_info("AT91: Power Management%s\n", (slow_clock ? " (with slow
>> clock mode)" : ""));
>>
>>   	/* AT91RM9200 SDRAM low-power mode cannot be used with self-refresh. */
>> -	if (cpu_is_at91rm9200()) {
>> -		at91_cpuidle_device.dev.platform_data = at91rm9200_standby;
>> +	if (cpu_is_at91rm9200())
>>   		at91_ramc_write(0, AT91RM9200_SDRAMC_LPR, 0);
>> -	} else if (cpu_is_at91sam9g45()) {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9g45_standby;
>> -	} else if (cpu_is_at91sam9263()) {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9263_standby;
>> -	} else {
>> -		at91_cpuidle_device.dev.platform_data = at91sam9_standby;
>> -	}
>>   	
>
> only register it if the call back is set
>>   	platform_device_register(&at91_cpuidle_device);
>>
>> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
>> index 2f5908f..76dd1a7 100644
>> --- a/arch/arm/mach-at91/pm.h
>> +++ b/arch/arm/mach-at91/pm.h
>> @@ -11,9 +11,13 @@
>>   #ifndef __ARCH_ARM_MACH_AT91_PM
>>   #define __ARCH_ARM_MACH_AT91_PM
>>
>> +#include <asm/proc-fns.h>
>> +
>>   #include <mach/at91_ramc.h>
>>   #include <mach/at91rm9200_sdramc.h>
>>
>> +extern void at91_pm_set_standby(void (*at91_standby)(void));
>> +
>>   /*
>>    * The AT91RM9200 goes into self-refresh mode with this command, and will
>>    * terminate self-refresh automatically on the next SDRAM access.
>> diff --git a/arch/arm/mach-at91/sama5d3.c b/arch/arm/mach-at91/sama5d3.c
>> index 4012797..9a53db6 100644
>> --- a/arch/arm/mach-at91/sama5d3.c
>> +++ b/arch/arm/mach-at91/sama5d3.c
>> @@ -21,6 +21,7 @@
>>   #include "generic.h"
>>   #include "clock.h"
>>   #include "sam9_smc.h"
>> +#include "pm.h"
>>
>>   /* --------------------------------------------------------------------
>>    *  Clocks
>> @@ -365,13 +366,18 @@ static void __init sama5d3_register_clocks(void)
>>   /* --------------------------------------------------------------------
>>    *  AT91SAM9x5 processor initialization
>>    * -------------------------------------------------------------------- */
>> -
>>   static void __init sama5d3_map_io(void)
>>   {
>>   	at91_init_sram(0, SAMA5D3_SRAM_BASE, SAMA5D3_SRAM_SIZE);
>>   }
>>
>> +static void __init sama5d3_initialize(void)
>> +{
> this is wrong we use a ddr comtroller not the same as the sam9
>> +	at91_set_standby(at91sam9_standby);
>> +}
>> +
>>   AT91_SOC_START(sama5d3)
>>   	.map_io = sama5d3_map_io,
>>   	.register_clocks = sama5d3_register_clocks,
>> +	.init = sama5d3_initialize,
>>   AT91_SOC_END
>>
>>
>
> Best Regards,
> J.
>


-- 
  <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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2013-10-14 17:59 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-12 15:51 [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver Daniel Lezcano
2013-10-12 15:51 ` Daniel Lezcano
2013-10-12 15:51 ` [PATCH 2/2] ARM: at91: cpuidle: Move driver to drivers/cpuidle Daniel Lezcano
2013-10-12 15:51   ` Daniel Lezcano
2013-10-13  9:19   ` Thomas Petazzoni
2013-10-13  9:19     ` Thomas Petazzoni
2013-10-13 10:17     ` Daniel Lezcano
2013-10-13 10:17       ` Daniel Lezcano
2013-10-14 10:20       ` Bartlomiej Zolnierkiewicz
2013-10-14 10:20         ` Bartlomiej Zolnierkiewicz
2013-10-14 10:53         ` Daniel Lezcano
2013-10-14 10:53           ` Daniel Lezcano
2013-10-14 14:04 ` [PATCH 1/2] ARM: at91: cpuidle: convert to platform driver Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 14:04   ` Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 14:18   ` Daniel Lezcano
2013-10-14 14:18     ` Daniel Lezcano
2013-10-14 14:27     ` Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 14:27       ` Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 16:30       ` Daniel Lezcano
2013-10-14 16:30         ` Daniel Lezcano
2013-10-14 17:18         ` Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 17:18           ` Jean-Christophe PLAGNIOL-VILLARD
2013-10-14 17:59           ` Daniel Lezcano
2013-10-14 17:59             ` Daniel Lezcano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.