at91 : cleanup pm.h

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 <[email protected]>

---
 arch/arm/mach-at91/pm.c |    2 +-
 arch/arm/mach-at91/pm.h |   12 ------------
 2 files changed, 1 insertions(+), 13 deletions(-)

arch/arm/mach-at91/pm.c +1 -1

index 62ad955..acf32c7 100644
@@ -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;
 

arch/arm/mach-at91/pm.h +0 -12

index 325ef76..624c99c 100644
@@ -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

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 <[email protected]>

From there, move the pm functions for each platform to their respective

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

arch/arm/mach-at91/cpuidle.c +3 -8

index a851e6c..555d956 100644
@@ -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 +

arch/arm/mach-at91/pm.c +1 -11

index acf32c7..94acc12 100644
@@ -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:

arch/arm/mach-at91/pm.h +36 -26

index 624c99c..b451822 100644
@@ -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
 

Quoted 11 lines · 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 <[email protected]>

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);
};

Quoted 18 lines · Ryan Mallon

Quoted 11 lines · 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 <[email protected]>

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

Quoted 4 lines

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.

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.

Quoted 6 lines · Ryan Mallon

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.

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