[PATCH 0/4] at91 : cleanup pm.h

[PATCH 0/4] at91 : cleanup pm.h

[Daniel Lezcano]

This patchset makes some cleanup and fix the rm9200 code by replacing
the standby routine by a asm routine where we have a better control.

This patchset has been tested on a sam9263, compiled on the other
platform but not tested because I don't have the hardware. 

This is my first attempt for an asm arm routine ...

Daniel Lezcano (4):
  at91 : coding style fixes
  at91 : declare header name
  at91 : remove wait_for_interrupt definition
  at91 : implement the standby function for pm/cpuidle

 arch/arm/mach-at91/cpuidle.c |   11 +----
 arch/arm/mach-at91/pm.c      |   12 +-----
 arch/arm/mach-at91/pm.h      |   86 +++++++++++++++++++++++++++---------------
 3 files changed, 59 insertions(+), 50 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] 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.5.4

[PATCH 2/4] 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.5.4

[PATCH 3/4] at91 : remove wait_for_interrupt definition

[Daniel Lezcano]

All the "wait_for_interrupt" definition are aliases to cpu_do_idle.
Only the rm9200 has an asm routine to switch to wfi. But the cpu_do_idle
for this platform has exactly the same asm routine.

arch/arm/mm/proc-arm920.S
...
ENTRY(cpu_arm920_do_idle)
        mcr     p15, 0, r0, c7, c0, 4           @ Wait for interrupt
...

Then it is safe to invoke cpu_do_idle for this platform. As all the
wait_for_interrupts are definition for cpu_do_idle, let's remove it
and replace its invokation by cpu_do_idle.

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

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 62ad955..acf32c7 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -272,7 +272,7 @@ static int at91_pm_enter(suspend_state_t state)
 					: /* no input */
 					: "r0");
 			saved_lpr = sdram_selfrefresh_enable();
-			wait_for_interrupt_enable();
+			cpu_do_idle();
 			sdram_selfrefresh_disable(saved_lpr);
 			break;
 
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 325ef76..624c99c 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -36,10 +36,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 #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>
 
@@ -58,9 +54,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 #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>
 
@@ -97,8 +90,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1); \
 	} while (0)
 
-#define wait_for_interrupt_enable()		cpu_do_idle()
-
 #else
 #include <mach/at91sam9_sdramc.h>
 
@@ -125,9 +116,6 @@ static inline u32 sdram_selfrefresh_enable(void)
 #define sdram_selfrefresh_disable(saved_lpr) \
 	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
 
-#define wait_for_interrupt_enable() \
-	cpu_do_idle()
-
 #endif
 
 #endif
-- 
1.7.5.4

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Daniel Lezcano]

This patch groups the self-refresh on/cpu_do_idle/self-refresh off into
a single 'standby' function.

The standby routine for rm9200 has been turned into an asm routine to have
a better control of the self refresh and to prevent a memory access when
running this code.

Draining the write buffer is done automatically when switching for the self
refresh on sam9, so the instruction is added to the rm9200 only.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/mach-at91/cpuidle.c |   11 ++-----
 arch/arm/mach-at91/pm.c      |   12 +-------
 arch/arm/mach-at91/pm.h      |   62 ++++++++++++++++++++++++-----------------
 3 files changed, 40 insertions(+), 45 deletions(-)

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index a851e6c..555d956 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -39,20 +39,15 @@ static int at91_enter_idle(struct cpuidle_device *dev,
 {
 	struct timeval before, after;
 	int idle_time;
-	u32 saved_lpr;
 
 	local_irq_disable();
 	do_gettimeofday(&before);
 	if (index == 0)
 		/* Wait for interrupt state */
 		cpu_do_idle();
-	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();
-		cpu_do_idle();
-		sdram_selfrefresh_disable(saved_lpr);
-	}
+	else if (index == 1)
+		at91_standby();
+
 	do_gettimeofday(&after);
 	local_irq_enable();
 	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index acf32c7..94acc12 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -208,7 +208,6 @@ extern u32 at91_slow_clock_sz;
 
 static int at91_pm_enter(suspend_state_t state)
 {
-	u32 saved_lpr;
 	at91_gpio_suspend();
 	at91_irq_suspend();
 
@@ -264,16 +263,7 @@ static int at91_pm_enter(suspend_state_t state)
 			 * For ARM 926 based chips, this requirement is weaker
 			 * as at91sam9 can access a RAM in self-refresh mode.
 			 */
-			asm volatile (	"mov r0, #0\n\t"
-					"b 1f\n\t"
-					".align 5\n\t"
-					"1: mcr p15, 0, r0, c7, c10, 4\n\t"
-					: /* no output */
-					: /* no input */
-					: "r0");
-			saved_lpr = sdram_selfrefresh_enable();
-			cpu_do_idle();
-			sdram_selfrefresh_disable(saved_lpr);
+			at91_standby();
 			break;
 
 		case PM_SUSPEND_ON:
diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
index 624c99c..b451822 100644
--- a/arch/arm/mach-at91/pm.h
+++ b/arch/arm/mach-at91/pm.h
@@ -24,22 +24,31 @@
  * still in self-refresh is "not recommended", but seems to work.
  */
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void at91rm9200_standby(void)
 {
-	u32 saved_lpr = at91_sys_read(AT91_SDRAMC_LPR);
 
-	at91_sys_write(AT91_SDRAMC_LPR, 0);
-	at91_sys_write(AT91_SDRAMC_SRR, 1);
-	return saved_lpr;
+	u32 lpr = at91_sys_read(AT91_SDRAMC_LPR);
+
+	asm volatile(
+		"b    1f\n\t"
+		".align    5\n\t"
+		"1:  mcr    p15, 0, %0, c7, c10, 4\n\t"
+		"    str    %0, [%1, %2]\n\t"
+		"    str    %3, [%1, %4]\n\t"
+		"    mcr    p15, 0, %0, c7, c0, 4\n\t"
+		"    str    %5, [%1, %2]"
+		:
+		: "r" (0), "r" (AT91_BASE_SYS), "r" (AT91_SDRAMC_LPR),
+		  "r" (1), "r" (AT91_SDRAMC_SRR),
+		  "r" (lpr));
 }
 
-#define sdram_selfrefresh_disable(saved_lpr) \
-	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
+#define at91_standby at91rm9200_standby
 
 #elif defined(CONFIG_ARCH_AT91CAP9)
 #include <mach/at91cap9_ddrsdr.h>
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void at91cap9_standby(void)
 {
 	u32 saved_lpr, lpr;
 
@@ -48,11 +57,13 @@ static inline u32 sdram_selfrefresh_enable(void)
 	lpr = saved_lpr & ~AT91_DDRSDRC_LPCB;
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr |
 			AT91_DDRSDRC_LPCB_SELF_REFRESH);
-	return saved_lpr;
+
+	cpu_do_idle();
+
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
 }
 
-#define sdram_selfrefresh_disable(saved_lpr) \
-	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr)
+#define at91_standby at91cap9_standby
 
 #elif defined(CONFIG_ARCH_AT91SAM9G45)
 #include <mach/at91sam9_ddrsdr.h>
@@ -60,14 +71,12 @@ static inline u32 sdram_selfrefresh_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 at91sam9g45_standby(void)
 {
-	/* Those tow values allow us to delay self-refresh activation
+	/* Those two values allow us to delay self-refresh activation
 	 * to the maximum. */
 	u32 lpr0, lpr1;
-	u32 saved_lpr0;
+	u32 saved_lpr0, saved_lpr1;
 
 	saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
 	lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
@@ -81,14 +90,13 @@ static inline u32 sdram_selfrefresh_enable(void)
 	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
 	at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
 
-	return saved_lpr0;
+	cpu_do_idle();
+
+	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
+	at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
 }
 
-#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)
+#define at91_standby at91sam9g45_standby
 
 #else
 #include <mach/at91sam9_sdramc.h>
@@ -101,7 +109,7 @@ static inline u32 sdram_selfrefresh_enable(void)
 #warning Assuming EB1 SDRAM controller is *NOT* used
 #endif
 
-static inline u32 sdram_selfrefresh_enable(void)
+static inline void at91sam9_standby(void)
 {
 	u32 saved_lpr, lpr;
 
@@ -110,11 +118,13 @@ static inline u32 sdram_selfrefresh_enable(void)
 	lpr = saved_lpr & ~AT91_SDRAMC_LPCB;
 	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr |
 			AT91_SDRAMC_LPCB_SELF_REFRESH);
-	return saved_lpr;
-}
 
-#define sdram_selfrefresh_disable(saved_lpr) \
+	cpu_do_idle();
+
 	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
+}
+
+#define at91_standby at91sam9_standby
 
 #endif
 
-- 
1.7.5.4

[PATCH 3/4] at91 : remove wait_for_interrupt definition

[Ryan Mallon]

On 18/01/12 10:40, Daniel Lezcano wrote:

> All the "wait_for_interrupt" definition are aliases to cpu_do_idle.
> Only the rm9200 has an asm routine to switch to wfi. But the cpu_do_idle
> for this platform has exactly the same asm routine.
> 
> arch/arm/mm/proc-arm920.S
> ...
> ENTRY(cpu_arm920_do_idle)
>         mcr     p15, 0, r0, c7, c0, 4           @ Wait for interrupt
> ...
> 
> Then it is safe to invoke cpu_do_idle for this platform. As all the
> wait_for_interrupts are definition for cpu_do_idle, let's remove it
> and replace its invokation by cpu_do_idle.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>


This patch looks like an improvement. Though, because you are removing
all of the definitions of wait_for_interrupt_enable in this patch, it
doesn't seem worth fixing the coding style of them in patch 1 of this
series.

Reviewed-by: Ryan Mallon <rmallon@gmail.com>

> ---
>  arch/arm/mach-at91/pm.c |    2 +-
>  arch/arm/mach-at91/pm.h |   12 ------------
>  2 files changed, 1 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index 62ad955..acf32c7 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -272,7 +272,7 @@ static int at91_pm_enter(suspend_state_t state)
>  					: /* no input */
>  					: "r0");
>  			saved_lpr = sdram_selfrefresh_enable();
> -			wait_for_interrupt_enable();
> +			cpu_do_idle();
>  			sdram_selfrefresh_disable(saved_lpr);
>  			break;
>  
> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
> index 325ef76..624c99c 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -36,10 +36,6 @@ static inline u32 sdram_selfrefresh_enable(void)
>  #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>
>  
> @@ -58,9 +54,6 @@ static inline u32 sdram_selfrefresh_enable(void)
>  #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>
>  
> @@ -97,8 +90,6 @@ static inline u32 sdram_selfrefresh_enable(void)
>  		at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1); \
>  	} while (0)
>  
> -#define wait_for_interrupt_enable()		cpu_do_idle()
> -
>  #else
>  #include <mach/at91sam9_sdramc.h>
>  
> @@ -125,9 +116,6 @@ static inline u32 sdram_selfrefresh_enable(void)
>  #define sdram_selfrefresh_disable(saved_lpr) \
>  	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
>  
> -#define wait_for_interrupt_enable() \
> -	cpu_do_idle()
> -
>  #endif
>  
>  #endif

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Ryan Mallon]

On 18/01/12 10:40, Daniel Lezcano wrote:

> This patch groups the self-refresh on/cpu_do_idle/self-refresh off into
> a single 'standby' function.
> 
> The standby routine for rm9200 has been turned into an asm routine to have
> a better control of the self refresh and to prevent a memory access when
> running this code.
> 
> Draining the write buffer is done automatically when switching for the self
> refresh on sam9, so the instruction is added to the rm9200 only.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>


I don't think is an improvement. Russell's suggestion was to convert the
pm operations into a structure, like:

struct at91_pm_ops {
	u32  (*sdram_selfrefresh_enable)(void);
	void (*sdram_selfrefresh_disable)(u32 saved_lpr);
};

This structure should be integrated into soc.h. Note that at91_init_soc
is __initdata, so you can't use that directly, but the basic principle
is the same.

>From there, move the pm functions for each platform to their respective
core file. For example, move the at91rm9200 functions to at91rm9200.c,
something like this:

#ifdef CONFIG_PM
static u32 at91rm9200_sdram_selfrefresh_enable(void)
{
        u32 saved_lpr = at91_sys_read(AT91_SDRAMC_LPR);

        at91_sys_write(AT91_SDRAMC_LPR, 0);
        at91_sys_write(AT91_SDRAMC_SRR, 1);
        return saved_lpr;
}

static void at91rm9200_sdram_selfrefresh_disable(u32 saved_lpr)
{
	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr);
}

static struct at91_pm_ops at91rm9200_pm_ops = {
	.sdram_selfrefresh_enable  = at91rm9200_selfrefresh_enable,
	.sdram_selfrefresh_disable = at91rm9200_selfrefresh_disable,
};
#endif /* CONFIG_PM */

Then register at91rm9200_pm_ops structure some way similar to how the
at91_init_soc is done. This will mean that the pm ops for all of the
at91 platforms can be compiled into a single kernel without all of the
ifdef crud.

Thanks,
~Ryan

> ---
>  arch/arm/mach-at91/cpuidle.c |   11 ++-----
>  arch/arm/mach-at91/pm.c      |   12 +-------
>  arch/arm/mach-at91/pm.h      |   62 ++++++++++++++++++++++++-----------------
>  3 files changed, 40 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index a851e6c..555d956 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
> @@ -39,20 +39,15 @@ static int at91_enter_idle(struct cpuidle_device *dev,
>  {
>  	struct timeval before, after;
>  	int idle_time;
> -	u32 saved_lpr;
>  
>  	local_irq_disable();
>  	do_gettimeofday(&before);
>  	if (index == 0)
>  		/* Wait for interrupt state */
>  		cpu_do_idle();
> -	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();
> -		cpu_do_idle();
> -		sdram_selfrefresh_disable(saved_lpr);
> -	}
> +	else if (index == 1)
> +		at91_standby();
> +
>  	do_gettimeofday(&after);
>  	local_irq_enable();
>  	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index acf32c7..94acc12 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -208,7 +208,6 @@ extern u32 at91_slow_clock_sz;
>  
>  static int at91_pm_enter(suspend_state_t state)
>  {
> -	u32 saved_lpr;
>  	at91_gpio_suspend();
>  	at91_irq_suspend();
>  
> @@ -264,16 +263,7 @@ static int at91_pm_enter(suspend_state_t state)
>  			 * For ARM 926 based chips, this requirement is weaker
>  			 * as at91sam9 can access a RAM in self-refresh mode.
>  			 */
> -			asm volatile (	"mov r0, #0\n\t"
> -					"b 1f\n\t"
> -					".align 5\n\t"
> -					"1: mcr p15, 0, r0, c7, c10, 4\n\t"
> -					: /* no output */
> -					: /* no input */
> -					: "r0");
> -			saved_lpr = sdram_selfrefresh_enable();
> -			cpu_do_idle();
> -			sdram_selfrefresh_disable(saved_lpr);
> +			at91_standby();
>  			break;
>  
>  		case PM_SUSPEND_ON:
> diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h
> index 624c99c..b451822 100644
> --- a/arch/arm/mach-at91/pm.h
> +++ b/arch/arm/mach-at91/pm.h
> @@ -24,22 +24,31 @@
>   * still in self-refresh is "not recommended", but seems to work.
>   */
>  
> -static inline u32 sdram_selfrefresh_enable(void)
> +static inline void at91rm9200_standby(void)
>  {
> -	u32 saved_lpr = at91_sys_read(AT91_SDRAMC_LPR);
>  
> -	at91_sys_write(AT91_SDRAMC_LPR, 0);
> -	at91_sys_write(AT91_SDRAMC_SRR, 1);
> -	return saved_lpr;
> +	u32 lpr = at91_sys_read(AT91_SDRAMC_LPR);
> +
> +	asm volatile(
> +		"b    1f\n\t"
> +		".align    5\n\t"
> +		"1:  mcr    p15, 0, %0, c7, c10, 4\n\t"
> +		"    str    %0, [%1, %2]\n\t"
> +		"    str    %3, [%1, %4]\n\t"
> +		"    mcr    p15, 0, %0, c7, c0, 4\n\t"
> +		"    str    %5, [%1, %2]"
> +		:
> +		: "r" (0), "r" (AT91_BASE_SYS), "r" (AT91_SDRAMC_LPR),
> +		  "r" (1), "r" (AT91_SDRAMC_SRR),
> +		  "r" (lpr));
>  }
>  
> -#define sdram_selfrefresh_disable(saved_lpr) \
> -	at91_sys_write(AT91_SDRAMC_LPR, saved_lpr)
> +#define at91_standby at91rm9200_standby
>  
>  #elif defined(CONFIG_ARCH_AT91CAP9)
>  #include <mach/at91cap9_ddrsdr.h>
>  
> -static inline u32 sdram_selfrefresh_enable(void)
> +static inline void at91cap9_standby(void)
>  {
>  	u32 saved_lpr, lpr;
>  
> @@ -48,11 +57,13 @@ static inline u32 sdram_selfrefresh_enable(void)
>  	lpr = saved_lpr & ~AT91_DDRSDRC_LPCB;
>  	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr |
>  			AT91_DDRSDRC_LPCB_SELF_REFRESH);
> -	return saved_lpr;
> +
> +	cpu_do_idle();
> +
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr);
>  }
>  
> -#define sdram_selfrefresh_disable(saved_lpr) \
> -	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr)
> +#define at91_standby at91cap9_standby
>  
>  #elif defined(CONFIG_ARCH_AT91SAM9G45)
>  #include <mach/at91sam9_ddrsdr.h>
> @@ -60,14 +71,12 @@ static inline u32 sdram_selfrefresh_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 at91sam9g45_standby(void)
>  {
> -	/* Those tow values allow us to delay self-refresh activation
> +	/* Those two values allow us to delay self-refresh activation
>  	 * to the maximum. */
>  	u32 lpr0, lpr1;
> -	u32 saved_lpr0;
> +	u32 saved_lpr0, saved_lpr1;
>  
>  	saved_lpr1 = at91_ramc_read(1, AT91_DDRSDRC_LPR);
>  	lpr1 = saved_lpr1 & ~AT91_DDRSDRC_LPCB;
> @@ -81,14 +90,13 @@ static inline u32 sdram_selfrefresh_enable(void)
>  	at91_ramc_write(0, AT91_DDRSDRC_LPR, lpr0);
>  	at91_ramc_write(1, AT91_DDRSDRC_LPR, lpr1);
>  
> -	return saved_lpr0;
> +	cpu_do_idle();
> +
> +	at91_ramc_write(0, AT91_DDRSDRC_LPR, saved_lpr0);
> +	at91_ramc_write(1, AT91_DDRSDRC_LPR, saved_lpr1);
>  }
>  
> -#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)
> +#define at91_standby at91sam9g45_standby
>  
>  #else
>  #include <mach/at91sam9_sdramc.h>
> @@ -101,7 +109,7 @@ static inline u32 sdram_selfrefresh_enable(void)
>  #warning Assuming EB1 SDRAM controller is *NOT* used
>  #endif
>  
> -static inline u32 sdram_selfrefresh_enable(void)
> +static inline void at91sam9_standby(void)
>  {
>  	u32 saved_lpr, lpr;
>  
> @@ -110,11 +118,13 @@ static inline u32 sdram_selfrefresh_enable(void)
>  	lpr = saved_lpr & ~AT91_SDRAMC_LPCB;
>  	at91_ramc_write(0, AT91_SDRAMC_LPR, lpr |
>  			AT91_SDRAMC_LPCB_SELF_REFRESH);
> -	return saved_lpr;
> -}
>  
> -#define sdram_selfrefresh_disable(saved_lpr) \
> +	cpu_do_idle();
> +
>  	at91_ramc_write(0, AT91_SDRAMC_LPR, saved_lpr)
> +}
> +
> +#define at91_standby at91sam9_standby
>  
>  #endif
>  

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Russell King - ARM Linux]

On Thu, Jan 19, 2012 at 09:08:40AM +1100, Ryan Mallon wrote:
> On 18/01/12 10:40, Daniel Lezcano wrote:
> 
> > This patch groups the self-refresh on/cpu_do_idle/self-refresh off into
> > a single 'standby' function.
> > 
> > The standby routine for rm9200 has been turned into an asm routine to have
> > a better control of the self refresh and to prevent a memory access when
> > running this code.
> > 
> > Draining the write buffer is done automatically when switching for the self
> > refresh on sam9, so the instruction is added to the rm9200 only.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> 
> I don't think is an improvement. Russell's suggestion was to convert the
> pm operations into a structure, like:
> 
> struct at91_pm_ops {
> 	u32  (*sdram_selfrefresh_enable)(void);
> 	void (*sdram_selfrefresh_disable)(u32 saved_lpr);
> };

Actually no, that was my first suggestion.  I then went on to a second
suggestion, which is what Daniel has implemented.

The improvement is we've gone from this:

		asm("b 1f; .align 5; 1:");
		asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
		cpu_do_idle();
		sdram_selfrefresh_disable(saved_lpr);

which is really undefined, and may break with a later compiler, to:

	u32 lpr = at91_sys_read(AT91_SDRAMC_LPR);

	asm volatile(
		"b    1f\n\t"
		".align    5\n\t"
		"1:  mcr    p15, 0, %0, c7, c10, 4\n\t"
		"    str    %0, [%1, %2]\n\t"
		"    str    %3, [%1, %4]\n\t"
		"    mcr    p15, 0, %0, c7, c0, 4\n\t"
		"    str    %5, [%1, %2]"

which gcc guarantees it won't insert instructions randomly into the
middle of this.

So this is technically a far superior solution.

It also means that the implementation of at91_standby() can preserve
as many registers as are necessary over the standby itself - it seems
some AT91 SoCs require two registers rather than just one.

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Ryan Mallon]

On 19/01/12 09:18, Russell King - ARM Linux wrote:

> On Thu, Jan 19, 2012 at 09:08:40AM +1100, Ryan Mallon wrote:
>> On 18/01/12 10:40, Daniel Lezcano wrote:
>>
>>> This patch groups the self-refresh on/cpu_do_idle/self-refresh off into
>>> a single 'standby' function.
>>>
>>> The standby routine for rm9200 has been turned into an asm routine to have
>>> a better control of the self refresh and to prevent a memory access when
>>> running this code.
>>>
>>> Draining the write buffer is done automatically when switching for the self
>>> refresh on sam9, so the instruction is added to the rm9200 only.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>>
>> I don't think is an improvement. Russell's suggestion was to convert the
>> pm operations into a structure, like:
>>
>> struct at91_pm_ops {
>> 	u32  (*sdram_selfrefresh_enable)(void);
>> 	void (*sdram_selfrefresh_disable)(u32 saved_lpr);
>> };
> 
> Actually no, that was my first suggestion.  I then went on to a second
> suggestion, which is what Daniel has implemented.


Ah, sorry. I seem to have missed the middle of the discussion :-).

> 
> The improvement is we've gone from this:
> 
> 		asm("b 1f; .align 5; 1:");
> 		asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */
> 		cpu_do_idle();
> 		sdram_selfrefresh_disable(saved_lpr);
> 
> which is really undefined, and may break with a later compiler, to:
> 
> 	u32 lpr = at91_sys_read(AT91_SDRAMC_LPR);
> 
> 	asm volatile(
> 		"b    1f\n\t"
> 		".align    5\n\t"
> 		"1:  mcr    p15, 0, %0, c7, c10, 4\n\t"
> 		"    str    %0, [%1, %2]\n\t"
> 		"    str    %3, [%1, %4]\n\t"
> 		"    mcr    p15, 0, %0, c7, c0, 4\n\t"
> 		"    str    %5, [%1, %2]"
> 
> which gcc guarantees it won't insert instructions randomly into the
> middle of this.
> 
> So this is technically a far superior solution.
> 
> It also means that the implementation of at91_standby() can preserve
> as many registers as are necessary over the standby itself - it seems
> some AT91 SoCs require two registers rather than just one.


Ok, I see.

After this patch we still have the limitation that only one at91
platform can be built into the kernel (this is an existing problem,
which there is an on going effort to resolve) because the at91_standby
function is defined via ifdefs. So the at91_standby define should still
be converted to a function pointer and assigned via the soc right? Of
course, this can be done in a subsequent patch.

Thanks,
~Ryan

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Russell King - ARM Linux]

On Thu, Jan 19, 2012 at 09:25:34AM +1100, Ryan Mallon wrote:
> After this patch we still have the limitation that only one at91
> platform can be built into the kernel (this is an existing problem,
> which there is an on going effort to resolve) because the at91_standby
> function is defined via ifdefs. So the at91_standby define should still
> be converted to a function pointer and assigned via the soc right? Of
> course, this can be done in a subsequent patch.

Correct; that's something I did suggest.

[PATCH 4/4] at91 : implement the standby function for pm/cpuidle

[Daniel Lezcano]

On 01/18/2012 11:45 PM, Russell King - ARM Linux wrote:
> On Thu, Jan 19, 2012 at 09:25:34AM +1100, Ryan Mallon wrote:
>> After this patch we still have the limitation that only one at91
>> platform can be built into the kernel (this is an existing problem,
>> which there is an on going effort to resolve) because the at91_standby
>> function is defined via ifdefs. So the at91_standby define should still
>> be converted to a function pointer and assigned via the soc right? Of
>> course, this can be done in a subsequent patch.

Yes, this is what I planned to do but I wanted to bring this fix and do 
some code cleanup before switching this to an ops.


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

[PATCH 0/4] at91 : cleanup pm.h

[Nicolas Ferre]

On 01/18/2012 12:40 AM, Daniel Lezcano :
> This patchset makes some cleanup and fix the rm9200 code by replacing
> the standby routine by a asm routine where we have a better control.
> 
> This patchset has been tested on a sam9263, compiled on the other
> platform but not tested because I don't have the hardware. 
> 
> This is my first attempt for an asm arm routine ...
> 
> Daniel Lezcano (4):
>   at91 : coding style fixes
>   at91 : declare header name
>   at91 : remove wait_for_interrupt definition
>   at91 : implement the standby function for pm/cpuidle
> 
>  arch/arm/mach-at91/cpuidle.c |   11 +----
>  arch/arm/mach-at91/pm.c      |   12 +-----
>  arch/arm/mach-at91/pm.h      |   86 +++++++++++++++++++++++++++---------------
>  3 files changed, 59 insertions(+), 50 deletions(-)

Daniel,

You can add my:

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>

to the whole patch series. Thanks for your work (and I am looking
forward for your future improvements ;-)).

Best regards,
-- 
Nicolas Ferre