[PATCH 0/7] at91 : pm.h cleanups

[PATCH 0/7] at91 : pm.h cleanups

[Daniel Lezcano]

This patchset is the first series to cleanup some code around pm.h, pm.c and
cpuidle. The next series will bring more cleanups and finally the third series
will change the different functions into ops where we can export the structure
definition in order to encapsulate the code and move the at91's cpuidle driver
to the drivers/cpuidle directory.

Tested on Calao Systems - USB-A9263
Compiled for AT91RM9200

Daniel Lezcano (7):
  at91 : coding style fixes
  at91 : declare header name
  at91 : group headers inclusion for the memory controller
  at91 : convert pm.h macros to static inline functions
  at91 : fix dirty hack for the selfrefresh function
  at91 : group selfrefresh functions
  at91 : fix compilation warning

 arch/arm/mach-at91/cpuidle.c              |    6 +-
 arch/arm/mach-at91/include/mach/at91_mc.h |   22 +++++
 arch/arm/mach-at91/pm.c                   |   46 ++++++----
 arch/arm/mach-at91/pm.h                   |  136 +++++++++++++++--------------
 4 files changed, 122 insertions(+), 88 deletions(-)
 create mode 100644 arch/arm/mach-at91/include/mach/at91_mc.h

-- 
1.7.4.1

[PATCH 1/7] at91 : coding style fixes

[Daniel Lezcano]

This patch is mindless and does only fix the line length.
The purpose is to facilitate the review of the next patches.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/pm.h |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index ce9a206..92d2223 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -20,14 +20,16 @@ static inline u32 sdram_selfrefresh_enable(void)
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr)	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
-#define wait_for_interrupt_enable()		asm volatile ("mcr p15, 0, %0, c7, c0, 4" \
-								: : "r" (0))
+#define sdram_selfrefresh_disable(saved_lpr) \
+	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
+
+#define wait_for_interrupt_enable() \
+	asm volatile ("mcr p15, 0, %0, c7, c0, 4" \
+		      : : "r" (0))
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
 #include <mach/at91cap9_ddrsdr.h>
 
-
 static inline u32 sdram_selfrefresh_enable(void)
 {
 	u32 saved_lpr, lpr;
@@ -35,12 +37,16 @@ static inline u32 sdram_selfrefresh_enable(void)
 	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
 
 	lpr = saved_lpr & ~AT91_DDRSDRC_LPCB;
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr | AT91_DDRSDRC_LPCB_SELF_REFRESH);
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr |
+			AT91_DDRSDRC_LPCB_SELF_REFRESH);
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr)	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr)
-#define wait_for_interrupt_enable()		cpu_do_idle()
+#define sdram_selfrefresh_disable(saved_lpr) \
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr)
+
+#define wait_for_interrupt_enable() \
+	cpu_do_idle()
 
 #elif defined(CONFIG_ARCH_AT91SAM9G45)
 #include <mach/at91sam9_ddrsdr.h>
@@ -77,6 +83,7 @@ static inline u32 sdram_selfrefresh_enable(void)
 		at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0); \
 		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1); \
 	} while (0)
+
 #define wait_for_interrupt_enable()		cpu_do_idle()
 
 #else
@@ -97,11 +104,15 @@ static inline u32 sdram_selfrefresh_enable(void)
 	saved_lpr = at91_ramc_read(0, AT91_SDRAMC_LPR);
 
 	lpr = saved_lpr & ~AT91_SDRAMC_LPCB;
-	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr | AT91_SDRAMC_LPCB_SELF_REFRESH);
+	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr |
+			AT91_SDRAMC_LPCB_SELF_REFRESH);
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr)	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
-#define wait_for_interrupt_enable()		cpu_do_idle()
+#define sdram_selfrefresh_disable(saved_lpr) \
+	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
+
+#define wait_for_interrupt_enable() \
+	cpu_do_idle()
 
 #endif
-- 
1.7.4.1

[PATCH 2/7] at91 : declare header name

[Daniel Lezcano]

Add the header and define the macro to prevent multiple inclusion
like the others headers.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/pm.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 92d2223..325ef76 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -1,3 +1,16 @@
+/*
+ * AT91 Power Management
+ *
+ * Copyright (C) 2005 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __ARCH_ARM_MACH_AT91_PM
+#define __ARCH_ARM_MACH_AT91_PM
+
 #ifdef CONFIG_ARCH_AT91RM9200
 #include <mach/at91rm9200_mc.h>
 
@@ -116,3 +129,5 @@ static inline u32 sdram_selfrefresh_enable(void)
 	cpu_do_idle()
 
 #endif
+
+#endif
-- 
1.7.4.1

[PATCH 3/7] at91 : group headers inclusion for the memory controller

[Daniel Lezcano]

Group the headers inclusion depending on the arch in a single memory
controller file in order to clarify the pm.h header.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/include/mach/at91_mc.h |   22 ++++++++++++++++++++++
 arch/arm/mach-at91/pm.h                   |    6 ++----
 2 files changed, 24 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/mach-at91/include/mach/at91_mc.h

diff --git a/arch/arm/mach-at91/include/mach/at91_mc.h b/arch/arm/mach-at91/include/mach/at91_mc.h
new file mode 100644
index 0000000..c705ba9
--- /dev/null
+++ b/arch/arm/mach-at91/include/mach/at91_mc.h
@@ -0,0 +1,22 @@
+/*
+ * AT91 Memory controller
+ *
+ * Copyright (C) 2012 Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __ARCH_ARM_MACH_AT91_MC
+#define __ARCH_ARM_MACH_AT91_MC
+
+#ifdef CONFIG_ARCH_AT91RM9200
+#include <mach/at91rm9200_mc.h>
+#elif defined(CONFIG_ARCH_AT91CAP9)
+#include <mach/at91cap9_ddrsdr.h>
+#else
+#include <mach/at91sam9_sdramc.h>
+#endif
+
+#endif
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 325ef76..fc48bef 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -11,8 +11,9 @@
 #ifndef __ARCH_ARM_MACH_AT91_PM
 #define __ARCH_ARM_MACH_AT91_PM
 
+#include <mach/at91_mc.h>
+
 #ifdef CONFIG_ARCH_AT91RM9200
-#include <mach/at91rm9200_mc.h>
 
 /*
  * The AT91RM9200 goes into self-refresh mode with this command, and will
@@ -41,7 +42,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 		      : : "r" (0))
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
-#include <mach/at91cap9_ddrsdr.h>
 
 static inline u32 sdram_selfrefresh_enable(void)
 {
@@ -62,7 +62,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 	cpu_do_idle()
 
 #elif defined(CONFIG_ARCH_AT91SAM9G45)
-#include <mach/at91sam9_ddrsdr.h>
 
 /* We manage both DDRAM/SDRAM controllers, we need more than one value to
  * remember.
@@ -100,7 +99,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 #define wait_for_interrupt_enable()		cpu_do_idle()
 
 #else
-#include <mach/at91sam9_sdramc.h>
 
 #ifdef CONFIG_ARCH_AT91SAM9263
 /*
-- 
1.7.4.1

[PATCH 4/7] at91 : convert pm.h macros to static inline functions

[Daniel Lezcano]

Change the macros to inline functions. That will allow to group
the different functions definitions in a single function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/pm.h |   52 +++++++++++++++++++++++++++++-----------------
 1 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index fc48bef..3aa8b66 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -34,12 +34,15 @@ static inline u32 sdram_selfrefresh_enable(void)
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr) \
-	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
+static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+{
+	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr);
+}
 
-#define wait_for_interrupt_enable() \
-	asm volatile ("mcr p15, 0, %0, c7, c0, 4" \
-		      : : "r" (0))
+static inline void wait_for_interrupt_enable(void)
+{
+	asm volatile ("mcr p15, 0, %0, c7, c0, 4" : : "r" (0));
+}
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
 
@@ -55,11 +58,15 @@ static inline u32 sdram_selfrefresh_enable(void)
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr) \
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr)
+static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+{
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
+}
 
-#define wait_for_interrupt_enable() \
-	cpu_do_idle()
+static inline void wait_for_interrupt_enable(void)
+{
+	cpu_do_idle();
+}
 
 #elif defined(CONFIG_ARCH_AT91SAM9G45)
 
@@ -90,13 +97,16 @@ static inline u32 sdram_selfrefresh_enable(void)
 	return saved_lpr0;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr0)	\
-	do { \
-		at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0); \
-		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1); \
-	} while (0)
+static inline void sdram_selfrefresh_disable(u32 saved_lpr0)
+{
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
+	at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
+}
 
-#define wait_for_interrupt_enable()		cpu_do_idle()
+static inline void wait_for_interrupt_enable(void)
+{
+	cpu_do_idle();
+}
 
 #else
 
@@ -120,11 +130,15 @@ static inline u32 sdram_selfrefresh_enable(void)
 	return saved_lpr;
 }
 
-#define sdram_selfrefresh_disable(saved_lpr) \
-	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
+static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+{
+	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr);
+}
 
-#define wait_for_interrupt_enable() \
-	cpu_do_idle()
+static inline void wait_for_interrupt_enable(void)
+{
+	cpu_do_idle();
+}
 
 #endif
 
-- 
1.7.4.1

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Daniel Lezcano]

Remove the static variable saved_lpr1 defined in the header and
define a structure to be common with all the functions.
That will cleanly unify the function definitions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/cpuidle.c |    6 ++--
 arch/arm/mach-at91/pm.c      |    6 ++--
 arch/arm/mach-at91/pm.h      |   61 ++++++++++++++++++++----------------------
 3 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index a851e6c..e3eac45 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -38,8 +38,8 @@ static int at91_enter_idle(struct cpuidle_device *dev,
 			       int index)
 {
 	struct timeval before, after;
+	struct ram_saved rs;
 	int idle_time;
-	u32 saved_lpr;
 
 	local_irq_disable();
 	do_gettimeofday(&before);
@@ -49,9 +49,9 @@ static int at91_enter_idle(struct cpuidle_device *dev,
 	else if (index == 1) {
 		asm("b 1f; .align 5; 1:");
 		asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
-		saved_lpr = sdram_selfrefresh_enable();
+		sdram_selfrefresh_enable(&rs);
 		cpu_do_idle();
-		sdram_selfrefresh_disable(saved_lpr);
+		sdram_selfrefresh_disable(&rs);
 	}
 	do_gettimeofday(&after);
 	local_irq_enable();
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 62ad955..f97bbfa 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -208,7 +208,7 @@ extern u32 at91_slow_clock_sz;
 
 static int at91_pm_enter(suspend_state_t state)
 {
-	u32 saved_lpr;
+	struct ram_saved rs;
 	at91_gpio_suspend();
 	at91_irq_suspend();
 
@@ -271,9 +271,9 @@ static int at91_pm_enter(suspend_state_t state)
 					: /* no output */
 					: /* no input */
 					: "r0");
-			saved_lpr = sdram_selfrefresh_enable();
+			sdram_selfrefresh_enable(&rs);
 			wait_for_interrupt_enable();
-			sdram_selfrefresh_disable(saved_lpr);
+			sdram_selfrefresh_disable(&rs);
 			break;
 
 		case PM_SUSPEND_ON:
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 3aa8b66..b9de247 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -13,6 +13,11 @@
 
 #include <mach/at91_mc.h>
 
+struct ram_saved {
+	u32 lpr0;
+	u32 lpr1;
+};
+
 #ifdef CONFIG_ARCH_AT91RM9200
 
 /*
@@ -25,18 +30,17 @@
  * still in self-refresh is "not recommended", but seems to work.
  */
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 {
-	u32 saved_lpr = at91_sys_read(AT91_SDRAMC_LPR);
+	rs->lpr0 = at91_sys_read(AT91_SDRAMC_LPR);
 
 	at91_sys_write(AT91_SDRAMC_LPR, 0);
 	at91_sys_write(AT91_SDRAMC_SRR, 1);
-	return saved_lpr;
 }
 
-static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
 {
-	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr);
+	at91_sys_write(AT91_SDRAMC_LPR, rs->lpr0);
 }
 
 static inline void wait_for_interrupt_enable(void)
@@ -46,21 +50,20 @@ static inline void wait_for_interrupt_enable(void)
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 {
-	u32 saved_lpr, lpr;
+	u32 lpr;
 
-	saved_lpr = at91_ramc_read(0, AT91_DDRSDRC_LPR);
+	rs->lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR);
 
-	lpr = saved_lpr & ~AT91_DDRSDRC_LPCB;
+	lpr = rs->lpr0 & ~AT91_DDRSDRC_LPCB;
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr |
 			AT91_DDRSDRC_LPCB_SELF_REFRESH);
-	return saved_lpr;
 }
 
-static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
 {
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, rs->lpr0);
 }
 
 static inline void wait_for_interrupt_enable(void)
@@ -73,34 +76,29 @@ static inline void wait_for_interrupt_enable(void)
 /* We manage both DDRAM/SDRAM controllers, we need more than one value to
  * remember.
  */
-static u32 saved_lpr1;
-
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 {
 	/* Those tow values allow us to delay self-refresh activation
 	 * to the maximum. */
 	u32 lpr0, lpr1;
-	u32 saved_lpr0;
 
-	saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
-	lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
+	rs->lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
+	lpr1 = rs->lpr1 & ~AT91_DDRSDRC_LPCB;
 	lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH;
 
-	saved_lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR);
-	lpr0 = saved_lpr0 & ~AT91_DDRSDRC_LPCB;
+	rs->lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR);
+	lpr0 = rs->lpr0 & ~AT91_DDRSDRC_LPCB;
 	lpr0 |= AT91_DDRSDRC_LPCB_SELF_REFRESH;
 
 	/* self-refresh mode now */
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
 	at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
-
-	return saved_lpr0;
 }
 
-static inline void sdram_selfrefresh_disable(u32 saved_lpr0)
+static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
 {
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
-	at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, rs->lpr0);
+	at91_ramc_write(1, AT91_DDRSDRC_LPR, rs->lpr1);
 }
 
 static inline void wait_for_interrupt_enable(void)
@@ -118,21 +116,20 @@ static inline void wait_for_interrupt_enable(void)
 #warning Assuming EB1 SDRAM controller is *NOT* used
 #endif
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 {
-	u32 saved_lpr, lpr;
+	u32 lpr;
 
-	saved_lpr = at91_ramc_read(0, AT91_SDRAMC_LPR);
+	rs->lpr0 = at91_ramc_read(0, AT91_SDRAMC_LPR);
 
-	lpr = saved_lpr & ~AT91_SDRAMC_LPCB;
+	lpr = rs->lpr0 & ~AT91_SDRAMC_LPCB;
 	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr |
 			AT91_SDRAMC_LPCB_SELF_REFRESH);
-	return saved_lpr;
 }
 
-static inline void sdram_selfrefresh_disable(u32 saved_lpr)
+static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
 {
-	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr);
+	at91_ramc_write(0, AT91_SDRAMC_LPR, rs->lpr0);
 }
 
 static inline void wait_for_interrupt_enable(void)
-- 
1.7.4.1

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Arnd Bergmann]

On Wednesday 11 January 2012, Daniel Lezcano wrote:
> Remove the static variable saved_lpr1 defined in the header and
> define a structure to be common with all the functions.
> That will cleanly unify the function definitions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Good catch!

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Russell King - ARM Linux]

On Wed, Jan 11, 2012 at 03:55:38PM +0100, Daniel Lezcano wrote:
> Remove the static variable saved_lpr1 defined in the header and
> define a structure to be common with all the functions.
> That will cleanly unify the function definitions.

I don't think this is in any way a correct way to do things.

> +	struct ram_saved rs;
> @@ -49,9 +49,9 @@ static int at91_enter_idle(struct cpuidle_device *dev,
>  	else if (index == 1) {
>  		asm("b 1f; .align 5; 1:");
>  		asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
> -		saved_lpr = sdram_selfrefresh_enable();
> +		sdram_selfrefresh_enable(&rs);

What's the point of draining the write buffer if you then pass a buffer
to this function to write data to?

If the requirement is that the write buffer is drained before issue a
wait-for-interrupt instruction (in cpu_do_idle()) then this code
violates that.

That's why I went on in my discussion to a second solution.

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Arnd Bergmann]

On Wednesday 11 January 2012, Russell King - ARM Linux wrote:
> > @@ -49,9 +49,9 @@ static int at91_enter_idle(struct cpuidle_device *dev,
> >       else if (index == 1) {
> >               asm("b 1f; .align 5; 1:");
> >               asm("mcr p15, 0, r0, c7, c10, 4");      /* drain write buffer */
> > -             saved_lpr = sdram_selfrefresh_enable();
> > +             sdram_selfrefresh_enable(&rs);
> 
> What's the point of draining the write buffer if you then pass a buffer
> to this function to write data to?
> 
> If the requirement is that the write buffer is drained before issue a
> wait-for-interrupt instruction (in cpu_do_idle()) then this code
> violates that.

Does that mean the existing AT91SAM9G45 version of sdram_selfrefresh_enable
is broken already? It writes to the static saved_lpr1 variable after
the write buffers are drained, which is what this patch was trying to
clean up.

	Arnd

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Russell King - ARM Linux]

On Wed, Jan 11, 2012 at 06:27:18PM +0000, Arnd Bergmann wrote:
> On Wednesday 11 January 2012, Russell King - ARM Linux wrote:
> > > @@ -49,9 +49,9 @@ static int at91_enter_idle(struct cpuidle_device *dev,
> > >       else if (index == 1) {
> > >               asm("b 1f; .align 5; 1:");
> > >               asm("mcr p15, 0, r0, c7, c10, 4");      /* drain write buffer */
> > > -             saved_lpr = sdram_selfrefresh_enable();
> > > +             sdram_selfrefresh_enable(&rs);
> > 
> > What's the point of draining the write buffer if you then pass a buffer
> > to this function to write data to?
> > 
> > If the requirement is that the write buffer is drained before issue a
> > wait-for-interrupt instruction (in cpu_do_idle()) then this code
> > violates that.
> 
> Does that mean the existing AT91SAM9G45 version of sdram_selfrefresh_enable
> is broken already? It writes to the static saved_lpr1 variable after
> the write buffers are drained, which is what this patch was trying to
> clean up.

It works because we're lucky - this is what the compiler is producing
for my test build of this file (for AT91SAM9RL):

 100:   e3a00000        mov     r0, #0  ; 0x0
 104:   ea000005        b       120 <at91_pm_enter+0x84>
 108:   e1a00000        nop                     (mov r0,r0)
 10c:   e1a00000        nop                     (mov r0,r0)
 110:   e1a00000        nop                     (mov r0,r0)
 114:   e1a00000        nop                     (mov r0,r0)
 118:   e1a00000        nop                     (mov r0,r0)
 11c:   e1a00000        nop                     (mov r0,r0)
 120:   ee070f9a        mcr     15, 0, r0, cr7, cr10, {4}
 124:   e59f404c        ldr     r4, [pc, #76]   ; 178 <at91_pm_enter+0xdc>
 128:   e51455ef        ldr     r5, [r4, #-1519]
 12c:   e3c53003        bic     r3, r5, #3      ; 0x3
 130:   e3833001        orr     r3, r3, #1      ; 0x1
 134:   e50435ef        str     r3, [r4, #-1519]
 138:   ebfffffe        bl      0 <cpu_arm926_do_idle>
 13c:   e50455ef        str     r5, [r4, #-1519]
...
 178:   feffefff        .word   0xfeffefff

The two str instructions there are to access virtual address 0xfeffefff -
1519 = 0xfeffea10, which is a device register.  It keeps 'saved_lpr' in
r5, ready for the write to restore the value after the idle call.

Thankfully, *my* compiler hasn't decided to add many additional
instructions into that code path, but that's not to say that a newer
(or older) compiler may not do.

For SAM9G45:

 100:   e3a00000        mov     r0, #0  ; 0x0
 104:   ea000005        b       120 <at91_pm_enter+0x84>
 108:   e1a00000        nop                     (mov r0,r0)
 10c:   e1a00000        nop                     (mov r0,r0)
 110:   e1a00000        nop                     (mov r0,r0)
 114:   e1a00000        nop                     (mov r0,r0)
 118:   e1a00000        nop                     (mov r0,r0)
 11c:   e1a00000        nop                     (mov r0,r0)
 120:   ee070f9a        mcr     15, 0, r0, cr7, cr10, {4}
 124:   e59f406c        ldr     r4, [pc, #108]  ; 198 <at91_pm_enter+0xfc>
 128:   e59f606c        ldr     r6, [pc, #108]  ; 19c <at91_pm_enter+0x100>
 12c:   e5141be3        ldr     r1, [r4, #-3043]
 130:   e51459e3        ldr     r5, [r4, #-2531]
 134:   e3c12003        bic     r2, r1, #3      ; 0x3
 138:   e3c53003        bic     r3, r5, #3      ; 0x3
 13c:   e3833001        orr     r3, r3, #1      ; 0x1
 140:   e3822001        orr     r2, r2, #1      ; 0x1
 144:   e50439e3        str     r3, [r4, #-2531]
 148:   e5042be3        str     r2, [r4, #-3043]
 14c:   e5861000        str     r1, [r6]
 150:   ebfffffe        bl      0 <cpu_arm926_do_idle>
 154:   e5963000        ldr     r3, [r6]
 158:   e50459e3        str     r5, [r4, #-2531]
 15c:   e5043be3        str     r3, [r4, #-3043]

 198:   feffefff        .word   0xfeffefff
 19c:   00000000        .word   0x00000000
                        19c: R_ARM_ABS32        .bss

And there is an additional store in there to a BSS initialized variable
(via r6).

My guess is that r6 is sharing a cache line which is already cached, and
so the write is hitting that cache line and remaining unwritten out to
memory.  In other words, this also is probably mere luck.

On the other hand, we have another DWB in cpu_arm926_do_idle itself.

Whether any of this matters depends on _why_ that DWB is in the AT91
code itself - is it something that needs to be done before placing the
SDRAM into self-refresh mode, or is it being done merely because the
ARM926 docs say that a DWB is needed before WFI?

If the latter, it can be dispensed with because the CPU specific code
is already doing that.

In any case, I think we need someone to speak up who knows this bit of
the AT91 code, and it needs fixing so that it's less reliant on luck -
otherwise cleanups could introduce some rather horrible bugs.

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Nicolas Ferre]

On 01/11/2012 08:43 PM, Russell King - ARM Linux :

[..]

> On the other hand, we have another DWB in cpu_arm926_do_idle itself.
> 
> Whether any of this matters depends on _why_ that DWB is in the AT91
> code itself - is it something that needs to be done before placing the
> SDRAM into self-refresh mode, or is it being done merely because the
> ARM926 docs say that a DWB is needed before WFI?

We have two cases here:

For the venerable at91rm9200: DWB is needed before putting SDRAM into
self-refresh because any subsequent access to SDRAM will force it to
resume from self-refresh state. Of course for this case, it is important
to make sure that no access to SDRAM is made before the
wait-for-interrupt instruction.

For all other SAM9 SoCs: no additional DWB is needed because RAM
controller manages self-refresh state even if accesses are still done to
the memory.

> If the latter, it can be dispensed with because the CPU specific code
> is already doing that.

Yes, exactly, but only for SAM9, not for RM9200.

> In any case, I think we need someone to speak up who knows this bit of
> the AT91 code, and it needs fixing so that it's less reliant on luck -
> otherwise cleanups could introduce some rather horrible bugs.

Ok, Daniel, tell me how I can help you. Is there any information that is
missing on your side?

BTW, you may save time by skipping all CAP9 related changes (and remove
the code): We shall remove its support in kernel for 3.4 release:
https://lkml.org/lkml/2012/1/6/161

Best regards,
-- 
Nicolas Ferre

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 03:41:29PM +0100, Nicolas Ferre wrote:
> On 01/11/2012 08:43 PM, Russell King - ARM Linux :
> > On the other hand, we have another DWB in cpu_arm926_do_idle itself.
> > 
> > Whether any of this matters depends on _why_ that DWB is in the AT91
> > code itself - is it something that needs to be done before placing the
> > SDRAM into self-refresh mode, or is it being done merely because the
> > ARM926 docs say that a DWB is needed before WFI?
> 
> We have two cases here:
> 
> For the venerable at91rm9200: DWB is needed before putting SDRAM into
> self-refresh because any subsequent access to SDRAM will force it to
> resume from self-refresh state. Of course for this case, it is important
> to make sure that no access to SDRAM is made before the
> wait-for-interrupt instruction.
> 
> For all other SAM9 SoCs: no additional DWB is needed because RAM
> controller manages self-refresh state even if accesses are still done to
> the memory.

Okay, that makes sense (because everything but rm9200 branches out to
one of the cpu idle functions.)

It also supports my second idea described in
http://lists.arm.linux.org.uk/lurker/message/20120109.144443.3626e5a6.en.html
See the paragraph starting "So, what I suggest instead" at that URL and
following text.

The reason I think this is the best solution is:
(a) we move the CPU dependencies into each CPU file
(b) we localize the quirks needed for each CPU into its own specific code
(c) we can select at run time between the various standby functions

These are all AT91 specific wins.  What it also gets us is _much_ less
exported code from arch/arm/mach-at91 when the CPU idle stuff moves out,
reducing it down to just a mere function pointer, and, because the AT91
specific idle stuff is hidden behind this it potentially opens the door
towards some consolidation in this area between different SoCs.

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Rob Lee]

On Thu, Jan 12, 2012 at 1:36 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Okay, that makes sense (because everything but rm9200 branches out to
> one of the cpu idle functions.)
>
> It also supports my second idea described in
> http://lists.arm.linux.org.uk/lurker/message/20120109.144443.3626e5a6.en.html
> See the paragraph starting "So, what I suggest instead" at that URL and
> following text.
>
> The reason I think this is the best solution is:
> (a) we move the CPU dependencies into each CPU file
> (b) we localize the quirks needed for each CPU into its own specific code
> (c) we can select at run time between the various standby functions
>
> These are all AT91 specific wins. ?What it also gets us is _much_ less
> exported code from arch/arm/mach-at91 when the CPU idle stuff moves out,
> reducing it down to just a mere function pointer, and, because the AT91
> specific idle stuff is hidden behind this it potentially opens the door
> towards some consolidation in this area between different SoCs.

Apologies if this is a dumb question, but for the arch/arm/mach-at91
function pointer that needs to be exported, what is the recommended
method for exporting it to a file that needs it in drivers/cpuidle?

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Daniel Lezcano]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/13/2012 01:38 AM, Rob Lee wrote:
> On Thu, Jan 12, 2012 at 1:36 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
>> Okay, that makes sense (because everything but rm9200 branches out to
>> one of the cpu idle functions.)
>>
>> It also supports my second idea described in
>> http://lists.arm.linux.org.uk/lurker/message/20120109.144443.3626e5a6.en.html
>> See the paragraph starting "So, what I suggest instead" at that URL and
>> following text.
>>
>> The reason I think this is the best solution is:
>> (a) we move the CPU dependencies into each CPU file
>> (b) we localize the quirks needed for each CPU into its own specific code
>> (c) we can select at run time between the various standby functions
>>
>> These are all AT91 specific wins.  What it also gets us is _much_ less
>> exported code from arch/arm/mach-at91 when the CPU idle stuff moves out,
>> reducing it down to just a mere function pointer, and, because the AT91
>> specific idle stuff is hidden behind this it potentially opens the door
>> towards some consolidation in this area between different SoCs.
> 
> Apologies if this is a dumb question, but for the arch/arm/mach-at91
> function pointer that needs to be exported, what is the recommended
> method for exporting it to a file that needs it in drivers/cpuidle?

I was asking me the same question. I am wondering if that makes sense to
create an arch/arm/include/asm/pm.h file where we move from the system.h
file these two functions:

extern void (*arm_pm_restart)(char str, const char *cmd);
extern void (*arm_pm_idle)(void);

and we add:

extern void (*arm_pm_standby)(void);



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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPD/mNAAoJEAKBbMCpUGYAHFAH/0yXlX49BUW1nDqZPiP777tc
rEajf5vD2lvvDzF3aeqmwfxLhNKBkppeasxFX8KCukQGq4SmDgC34NSbHAKi3SqX
/Yv3xcWEPqnp3PgPacNCFX7kGqHIr9XWHjG/p38+82rGCOgtWn0DslOuzFa5JlJu
ih2Yh+2hh2low4h/21no8rF08EJefh0PuMn1c58Jg04txe8iATpXZHU9ujeIlvUg
NdHr8q1gzfGsnFb1gV26NHapERLwCJlj2ilttYMnNyHZYxTssDrnI4HsMDcyjcEQ
0BQYV0VMAHGSPiv9AIMx0ykCwQrjuDZPBEJIFS0ReZ1ItcGwVFFEmqb9X+OdJdg=
=g+rR
-----END PGP SIGNATURE-----

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Russell King - ARM Linux]

On Fri, Jan 13, 2012 at 10:29:49AM +0100, Daniel Lezcano wrote:
> I was asking me the same question. I am wondering if that makes sense to
> create an arch/arm/include/asm/pm.h file where we move from the system.h
> file these two functions:
> 
> extern void (*arm_pm_restart)(char str, const char *cmd);

Ideally, I want this one to go away - which it can do once its usage in
the AT91 code has been fixed up.  I don't know how to do that because
I don't know which machines/platforms end up with which SoCs.

The idea of exporting arm_pm_standby() is one I thought about, but I'm
not sure if it makes sense for all SoCs to call their standby function
from the CPU idle code.

I'm coming to the conclusion that moving the AT91 CPU idle driver out
of arch/arm/mach-at91 is the wrong thing to do - it seems that the CPU
idle driver is intimately linked to core AT91 code, and moving it out
just makes those linkages a lot more complicated to handle.  And for
what benefit - just to have this located under the drivers/ subtree
because it's now seen to be a driver ?

I think this is one of the times where the 'solution' is worse than the
problem.

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Arnd Bergmann]

On Friday 13 January 2012, Russell King - ARM Linux wrote:
> The idea of exporting arm_pm_standby() is one I thought about, but I'm
> not sure if it makes sense for all SoCs to call their standby function
> from the CPU idle code.

It doesn't have to be all SoCs, but it would be nice if we could
do it for a few of them.

> I'm coming to the conclusion that moving the AT91 CPU idle driver out
> of arch/arm/mach-at91 is the wrong thing to do - it seems that the CPU
> idle driver is intimately linked to core AT91 code, and moving it out
> just makes those linkages a lot more complicated to handle.  And for
> what benefit - just to have this located under the drivers/ subtree
> because it's now seen to be a driver ?
> 
> I think this is one of the times where the 'solution' is worse than the
> problem.

One of the benefits of moving all cpuidle drivers into that directory
was so that we could spot commonalities between them and share code across
the implementation.

Maybe the answer is that we should do the second step before the first
one instead, in particular since the at91 implementation is so simple.
Could we have a common cpuidle driver that just calls cpu_do_idle or a
new global pm_standby function when that is available for two disinct
levels? Then again, there really isn't that much code left, so maybe
it doesn't make much sense after all to consolidate it this one.

One more idea: We can put the driver into drivers/cpuidle/at91_cpuidle.c
but put the functions that get called into the arch/arm/mach-at91/pm.c
and initialize it with the standby function provided as driver_data:

struct cpuidle_device at91_cpuidle_device = {
	.count = 2,
	.states_usage = {
	[0] = { .driver_data = &cpu_do_idle },
	[1] = { .driver_data = &at91_standby },
	},
};

Not really how it's meant to be used, but separates the cpu-specific parts
from the main driver.

	Arnd

[PATCH 5/7] at91 : fix dirty hack for the selfrefresh function

[Rob Lee]

On Fri, Jan 13, 2012 at 9:48 AM, Arnd Bergmann <arnd.bergmann@linaro.org> wrote:
> On Friday 13 January 2012, Russell King - ARM Linux wrote:
>> The idea of exporting arm_pm_standby() is one I thought about, but I'm
>> not sure if it makes sense for all SoCs to call their standby function
>> from the CPU idle code.
>
> It doesn't have to be all SoCs, but it would be nice if we could
> do it for a few of them.
>
>> I'm coming to the conclusion that moving the AT91 CPU idle driver out
>> of arch/arm/mach-at91 is the wrong thing to do - it seems that the CPU
>> idle driver is intimately linked to core AT91 code, and moving it out
>> just makes those linkages a lot more complicated to handle. ?And for
>> what benefit - just to have this located under the drivers/ subtree
>> because it's now seen to be a driver ?
>>
>> I think this is one of the times where the 'solution' is worse than the
>> problem.
>
> One of the benefits of moving all cpuidle drivers into that directory
> was so that we could spot commonalities between them and share code across
> the implementation.
>
> Maybe the answer is that we should do the second step before the first
> one instead, in particular since the at91 implementation is so simple.
> Could we have a common cpuidle driver that just calls cpu_do_idle or a
> new global pm_standby function when that is available for two disinct
> levels? Then again, there really isn't that much code left, so maybe
> it doesn't make much sense after all to consolidate it this one.
>

This patch provides a common cpuidle interface with an optional
default state that just calls cpu_do_idle.  Take a look at the
drivers/cpuidle/common.c file.  I hope to release the next version
within day or two, so if you have any additional suggestions that
could help with this situation, please let me know.

http://www.spinics.net/lists/arm-kernel/msg151843.html

> One more idea: We can put the driver into drivers/cpuidle/at91_cpuidle.c
> but put the functions that get called into the arch/arm/mach-at91/pm.c
> and initialize it with the standby function provided as driver_data:
>
> struct cpuidle_device at91_cpuidle_device = {
> ? ? ? ?.count = 2,
> ? ? ? ?.states_usage = {
> ? ? ? ?[0] = { .driver_data = &cpu_do_idle },
> ? ? ? ?[1] = { .driver_data = &at91_standby },
> ? ? ? ?},
> };
>
> Not really how it's meant to be used, but separates the cpu-specific parts
> from the main driver.
>
> ? ? ? ?Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

It seems that clock driver functionality for some platforms are
implemented in arch/arm/mach-xxx and then exported through calls to
clk_get.  The platform_driver system also allows exporting of platform
data to non platform located files.  Perhaps each platform could have
one specific platform driver structure instance whose sole purpose is
to export platform specific functions by a call to
platform_get_drvdata.  Or, a lighter weight system similar to clk_get
could be created for this purpose.  Of course both of these are
heavier than just adding extern statements to a drivers/cpuidle/blah.c
file but I just wanted to throw those options out there in case it is
the "extern" method of exporting that is an issue and not the actual
exporting platform specific located functionality to non-platform
areas.

[PATCH 6/7] at91 : group selfrefresh functions

[Daniel Lezcano]

Group in a single function the multiple functions declaration.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/pm.h |  105 ++++++++++++++++------------------------------
 1 files changed, 37 insertions(+), 68 deletions(-)

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index b9de247..aaa8e14 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -18,7 +18,13 @@ struct ram_saved {
 	u32 lpr1;
 };
 
-#ifdef CONFIG_ARCH_AT91RM9200
+#ifdef CONFIG_ARCH_AT91SAM9263
+/*
+ * FIXME either or both the SDRAM controllers (EB0, EB1) might be in use;
+ * handle those cases both here and in the Suspend-To-RAM support.
+ */
+#warning Assuming EB1 SDRAM controller is *NOT* used
+#endif
 
 /*
  * The AT91RM9200 goes into self-refresh mode with this command, and will
@@ -32,56 +38,32 @@ struct ram_saved {
 
 static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 {
-	rs->lpr0 = at91_sys_read(AT91_SDRAMC_LPR);
+	u32 lpr0;
+
+#ifdef CONFIG_ARCH_AT91RM9200
+
+	lpr0 = at91_sys_read(AT91_SDRAMC_LPR);
 
 	at91_sys_write(AT91_SDRAMC_LPR, 0);
 	at91_sys_write(AT91_SDRAMC_SRR, 1);
-}
 
-static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
-{
-	at91_sys_write(AT91_SDRAMC_LPR, rs->lpr0);
-}
-
-static inline void wait_for_interrupt_enable(void)
-{
-	asm volatile ("mcr p15, 0, %0, c7, c0, 4" : : "r" (0));
-}
+	rs->lpr0 = lpr0;
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
 
-static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
-{
-	u32 lpr;
-
 	rs->lpr0 = at91_ramc_read(0, AT91_DDRSDRC_LPR);
 
-	lpr = rs->lpr0 & ~AT91_DDRSDRC_LPCB;
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr |
+	lpr0 = rs->lpr0 & ~AT91_DDRSDRC_LPCB;
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0 |
 			AT91_DDRSDRC_LPCB_SELF_REFRESH);
-}
-
-static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
-{
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, rs->lpr0);
-}
-
-static inline void wait_for_interrupt_enable(void)
-{
-	cpu_do_idle();
-}
-
 #elif defined(CONFIG_ARCH_AT91SAM9G45)
 
-/* We manage both DDRAM/SDRAM controllers, we need more than one value to
- * remember.
- */
-static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
-{
-	/* Those tow values allow us to delay self-refresh activation
-	 * to the maximum. */
-	u32 lpr0, lpr1;
+	/* We manage both DDRAM/SDRAM controllers, we need more than one value
+	 * to remember */
+	u32  lpr1;
 
+	/* Those two values allow us to delay self-refresh activation
+	 * to the maximum. */
 	rs->lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
 	lpr1 = rs->lpr1 & ~AT91_DDRSDRC_LPCB;
 	lpr1 |= AT91_DDRSDRC_LPCB_SELF_REFRESH;
@@ -93,50 +75,37 @@ static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
 	/* self-refresh mode now */
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
 	at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
+#else
+	rs->lpr0 = at91_ramc_read(0, AT91_SDRAMC_LPR);
+	lpr0 = rs->lpr0 & ~AT91_SDRAMC_LPCB;
+	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr0 |
+			AT91_SDRAMC_LPCB_SELF_REFRESH);
+
+#endif
 }
 
 static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
 {
+#ifdef CONFIG_ARCH_AT91RM9200
+	at91_sys_write(AT91_SDRAMC_LPR, rs->lpr0);
+#elif defined(CONFIG_ARCH_AT91CAP9)
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, rs->lpr0);
+#elif defined(CONFIG_ARCH_AT91SAM9G45)
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, rs->lpr0);
 	at91_ramc_write(1, AT91_DDRSDRC_LPR, rs->lpr1);
-}
-
-static inline void wait_for_interrupt_enable(void)
-{
-	cpu_do_idle();
-}
-
 #else
-
-#ifdef CONFIG_ARCH_AT91SAM9263
-/*
- * FIXME either or both the SDRAM controllers (EB0, EB1) might be in use;
- * handle those cases both here and in the Suspend-To-RAM support.
- */
-#warning Assuming EB1 SDRAM controller is *NOT* used
+	at91_ramc_write(0, AT91_SDRAMC_LPR, rs->lpr0);
 #endif
 
-static inline void sdram_selfrefresh_enable(struct ram_saved *rs)
-{
-	u32 lpr;
-
-	rs->lpr0 = at91_ramc_read(0, AT91_SDRAMC_LPR);
-
-	lpr = rs->lpr0 & ~AT91_SDRAMC_LPCB;
-	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr |
-			AT91_SDRAMC_LPCB_SELF_REFRESH);
-}
-
-static inline void sdram_selfrefresh_disable(struct ram_saved *rs)
-{
-	at91_ramc_write(0, AT91_SDRAMC_LPR, rs->lpr0);
 }
 
 static inline void wait_for_interrupt_enable(void)
 {
+#ifdef CONFIG_ARCH_AT91RM9200
+	asm volatile ("mcr p15, 0, %0, c7, c0, 4" : : "r" (0));
+#else
 	cpu_do_idle();
-}
-
 #endif
+}
 
 #endif
-- 
1.7.4.1

[PATCH 6/7] at91 : group selfrefresh functions

[Russell King - ARM Linux]

On Wed, Jan 11, 2012 at 03:55:39PM +0100, Daniel Lezcano wrote:
> Group in a single function the multiple functions declaration.

This isn't an improvement.

[PATCH 7/7] at91 : fix compilation warning

[Daniel Lezcano]

Move the clock routine in a function. That will prevent to have
a warning from the compiler to say there is unused variable.

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

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index f97bbfa..2250773 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -132,6 +132,28 @@ static int at91_pm_begin(suspend_state_t state)
 	return 0;
 }
 
+static inline int at91_program_clock(unsigned long scsr)
+{
+#ifdef CONFIG_AT91_PROGRAMMABLE_CLOCKS
+	int i;
+
+	/* PCK0..PCK3 must be disabled, or configured to use clk32k */
+	for (i = 0; i < 4; i++) {
+		u32 css;
+
+		if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
+			continue;
+
+		css = at91_sys_read(AT91_PMC_PCKR(i)) & AT91_PMC_CSS;
+		if (css != AT91_PMC_CSS_SLOW) {
+			pr_err("AT91: PM - Suspend-to-RAM with PCK%d src %d\n", i, css);
+			return -1;
+		}
+	}
+#endif
+	return 0;
+}
+
 /*
  * Verify that all the clocks are correct before entering
  * slow-clock mode.
@@ -139,7 +161,6 @@ static int at91_pm_begin(suspend_state_t state)
 static int at91_pm_verify_clocks(void)
 {
 	unsigned long scsr;
-	int i;
 
 	scsr = at91_sys_read(AT91_PMC_SCSR);
 
@@ -162,21 +183,8 @@ static int at91_pm_verify_clocks(void)
 		}
 	}
 
-#ifdef CONFIG_AT91_PROGRAMMABLE_CLOCKS
-	/* PCK0..PCK3 must be disabled, or configured to use clk32k */
-	for (i = 0; i < 4; i++) {
-		u32 css;
-
-		if ((scsr & (AT91_PMC_PCK0 << i)) == 0)
-			continue;
-
-		css = at91_sys_read(AT91_PMC_PCKR(i)) & AT91_PMC_CSS;
-		if (css != AT91_PMC_CSS_SLOW) {
-			pr_err("AT91: PM - Suspend-to-RAM with PCK%d src %d\n", i, css);
-			return 0;
-		}
-	}
-#endif
+	if (at91_program_clock(scsr))
+		return 0;
 
 	return 1;
 }
-- 
1.7.4.1

[PATCH 0/7] at91 : pm.h cleanups

[Arnd Bergmann]

On Wednesday 11 January 2012, Daniel Lezcano wrote:
> 
> This patchset is the first series to cleanup some code around pm.h, pm.c and
> cpuidle. The next series will bring more cleanups and finally the third series
> will change the different functions into ops where we can export the structure
> definition in order to encapsulate the code and move the at91's cpuidle driver
> to the drivers/cpuidle directory.

Hi Daniel,

These all look like useful cleanups. You don't really have to split them
up into so small units, but it doesn't hurt if you do. Patch 5 seems to
actually fix a bug, but probably a harmless one.

It's not clear where you're headed though, I hope that becomes more obvious
in the next patches. The tricky bit that will have to be done is to turn
all the #ifdef checks into runtime here. You have moved the #include for the
memory controller into a new header, but that is not actually progress
on this larger problem. It would be nice to move the
sdram_selfrefresh_enable/disable functions into a .c file that uses
cpu_is_at91...() to do runtime detection, but AFAICT that won't work
because you have to guarantee that all the code between these is
in the cache, right?

	Arnd

[PATCH 0/7] at91 : pm.h cleanups

[Daniel Lezcano]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/11/2012 04:23 PM, Arnd Bergmann wrote:
> On Wednesday 11 January 2012, Daniel Lezcano wrote:
>>
>> This patchset is the first series to cleanup some code around pm.h, pm.c and
>> cpuidle. The next series will bring more cleanups and finally the third series
>> will change the different functions into ops where we can export the structure
>> definition in order to encapsulate the code and move the at91's cpuidle driver
>> to the drivers/cpuidle directory.
> 
> Hi Daniel,
> 
> These all look like useful cleanups. You don't really have to split them
> up into so small units, but it doesn't hurt if you do. Patch 5 seems to
> actually fix a bug, but probably a harmless one.

Thanks Arnd for reviewing the patchset.

> It's not clear where you're headed though, I hope that becomes more obvious
> in the next patches. 

Yes, I think that will become more obvious in the next patches. The
objective is to cleanup the code first, unify the api and change that to
some ops we will register at boot time.

Also, I will address Russell's comment about the standby function to be
defined.

> The tricky bit that will have to be done is to turn
> all the #ifdef checks into runtime here. You have moved the #include for the
> memory controller into a new header, but that is not actually progress
> on this larger problem.

The next patches will include the header from the .c file. IMHO,
including a single file in a .c is more clear than a lot of #ifdef.

> It would be nice to move the
> sdram_selfrefresh_enable/disable functions into a .c file that uses
> cpu_is_at91...() to do runtime detection.

These patches are about separating the SoC specific code from the
cpuidle driver code in order to move this one to the drivers directory.

AFAIU, we agreed the drivers directory is the right place for the
cpuidle drivers. There is a lot of cleanup to be done on the different
SoC/arch and I would like to focus on that rather than fixing different
problems at the same time. By the way achieving this goal will benefit
to the single kernel image goal.

The next patchset will move these functions to the .c file and define an
ops structure which will be included from the cpuidle drivers.

I am not sure how to do runtime detection but as soon as this is defined
with ops, it will be trivial to register the right ops at boot time and
prevent unneeded cpu_is_at91 checks in the idle functions.

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPDbjoAAoJEAKBbMCpUGYATkQIAL6vVMgYQjeMcsXxD1VC+Xem
5e+xQEjGcItj0rbj5LAgAVrTxLAw9p+33xx5vjwOEa65mVm10bkkb3put9zKkKVz
QjilwywAzQ8LMsapJ5mIIQEOfXWOC41+cNbyKr150ydplSjuXIfgs255nCEsojtc
8kNGsgKjh7+vxE/3LCwa+kMz39rVHMxgle2JB+ThLZpv+c/wz/JXhZwOfjgH5ERL
K9neVmSS5N/LIzVh1IcXk0UxF4NDnSnoSadKWpxvxtnI1kb3oDiHXersq3BlIa8x
7wgohka5NlhzEmLeWzZ49DlOfZ5zyHIgzKeU8sY13ccZvxHDQuzqrv49AaM2PEM=
=4six
-----END PGP SIGNATURE-----

[PATCH 0/7] at91 : pm.h cleanups

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:23 Wed 11 Jan     , Arnd Bergmann wrote:
> On Wednesday 11 January 2012, Daniel Lezcano wrote:
> > 
> > This patchset is the first series to cleanup some code around pm.h, pm.c and
> > cpuidle. The next series will bring more cleanups and finally the third series
> > will change the different functions into ops where we can export the structure
> > definition in order to encapsulate the code and move the at91's cpuidle driver
> > to the drivers/cpuidle directory.
> 
> Hi Daniel,
> 
> These all look like useful cleanups. You don't really have to split them
> up into so small units, but it doesn't hurt if you do. Patch 5 seems to
> actually fix a bug, but probably a harmless one.
> 
> It's not clear where you're headed though, I hope that becomes more obvious
> in the next patches. The tricky bit that will have to be done is to turn
> all the #ifdef checks into runtime here. You have moved the #include for the
> memory controller into a new header, but that is not actually progress
> on this larger problem. It would be nice to move the
> sdram_selfrefresh_enable/disable functions into a .c file that uses
> cpu_is_at91...() to do runtime detection, but AFAICT that won't work
> because you have to guarantee that all the code between these is
> in the cache, right?
we may have much simple way move it to sram as done in slow clock

Best Regards,
J.

[PATCH 0/7] at91 : pm.h cleanups

[Russell King - ARM Linux]

On Wed, Jan 11, 2012 at 03:55:33PM +0100, Daniel Lezcano wrote:
> This patchset is the first series to cleanup some code around pm.h, pm.c and
> cpuidle. The next series will bring more cleanups and finally the third series
> will change the different functions into ops where we can export the structure
> definition in order to encapsulate the code and move the at91's cpuidle driver
> to the drivers/cpuidle directory.

Really this isn't an improvement.  Either the current code is complete
bollocks with respect to draining the write buffer, or this patch set
introduces bugs.

Please sort this out using the second solution I referred to - which
also fixes this part of the code wrt allowing several AT91 SoCs to
co-exist in the same kernel binary.