at91 : pm.h cleanups

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

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

---
 arch/arm/mach-at91/pm.h |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

index ce9a206..92d2223 100644
@@ -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

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

---
 arch/arm/mach-at91/pm.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

index 92d2223..325ef76 100644
@@ -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

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

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

new file mode 100644
index 0000000..c705ba9
@@ -0,0 +1,22 @@
+/*
+ * AT91 Memory controller
+ *
+ * Copyright (C) 2012 Daniel Lezcano <[email protected]>
+ *
+ * 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

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

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

---
 arch/arm/mach-at91/pm.h |   52 +++++++++++++++++++++++++++++-----------------
 1 files changed, 33 insertions(+), 19 deletions(-)

index fc48bef..3aa8b66 100644
@@ -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

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.

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

index a851e6c..e3eac45 100644
@@ -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();

index 62ad955..f97bbfa 100644
@@ -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:

1.7.4.1

Good catch!

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

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

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.

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

[..]

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.

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

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

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?

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

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

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.

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

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

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

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.

Group in a single function the multiple functions declaration.

---
 arch/arm/mach-at91/pm.h |  105 ++++++++++++++++------------------------------
 1 files changed, 37 insertions(+), 68 deletions(-)

index b9de247..aaa8e14 100644
@@ -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

This isn't an improvement.

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

---
 arch/arm/mach-at91/pm.c |   40 ++++++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 16 deletions(-)

index f97bbfa..2250773 100644
@@ -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

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

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

Thanks Arnd for reviewing the patchset.

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

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

we may have much simple way move it to sram as done in slow clock

Best Regards,
J.

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.