[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

Move the location of the pm.h header file to the include directory,
so it can be included from another place from the current one.

That will allow the next patch which moves the cpuidle code to the
drivers/cpuidle directory.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0
 arch/arm/mach-at91/cpuidle.c                       |    2 +-
 arch/arm/mach-at91/pm.c                            |    2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} (100%)

diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/include/asm/at91_pm.h
similarity index 100%
rename from arch/arm/mach-at91/pm.h
rename to arch/arm/include/asm/at91_pm.h
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index a851e6c..8281eec 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -21,7 +21,7 @@
 #include <linux/io.h>
 #include <linux/export.h>
 
-#include "pm.h"
+#include <asm/at91_pm.h>
 
 #define AT91_MAX_STATES	2
 
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 62ad955..86bc243 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 
 #include <asm/irq.h>
+#include <asm/at91_pm.h>
 #include <linux/atomic.h>
 #include <asm/mach/time.h>
 #include <asm/mach/irq.h>
@@ -29,7 +30,6 @@
 #include <mach/cpu.h>
 
 #include "generic.h"
-#include "pm.h"
 
 /*
  * Show the reason for the previous system reset.
-- 
1.7.4.1

[PATCH 2/2] at91 : move cpuidle driver to drivers/cpuidle directory

[Daniel Lezcano]

This patch follows the discussion about moving the cpuidle code which
is located in the architecture specific to the drivers directory.

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

diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index 4f991f2..ddeaf0e 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -18,6 +18,11 @@ config HAVE_AT91_USART4
 config HAVE_AT91_USART5
 	bool
 
+config AT91_IDLE
+	bool
+	default y
+	depends on CPU_IDLE
+
 menu "Atmel AT91 System-on-Chip"
 
 choice
diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile
index 242174f..ce0cd4c 100644
--- a/arch/arm/mach-at91/Makefile
+++ b/arch/arm/mach-at91/Makefile
@@ -91,7 +91,6 @@ obj-y				+= leds.o
 # Power Management
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_AT91_SLOW_CLOCK)	+= pm_slowclock.o
-obj-$(CONFIG_CPU_IDLE)	+= cpuidle.o
 
 ifeq ($(CONFIG_PM_DEBUG),y)
 CFLAGS_pm.o += -DDEBUG
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 5634f88..31c9b5f 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-$(CONFIG_AT91_IDLE) += at91_idle.o
diff --git a/arch/arm/mach-at91/cpuidle.c b/drivers/cpuidle/at91_idle.c
similarity index 100%
rename from arch/arm/mach-at91/cpuidle.c
rename to drivers/cpuidle/at91_idle.c
-- 
1.7.4.1

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Arnd Bergmann]

On Friday 06 January 2012, Daniel Lezcano wrote:
> Move the location of the pm.h header file to the include directory,
> so it can be included from another place from the current one.
> 
> That will allow the next patch which moves the cpuidle code to the
> drivers/cpuidle directory.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0
>  arch/arm/mach-at91/cpuidle.c                       |    2 +-
>  arch/arm/mach-at91/pm.c                            |    2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} (100%)

Moving the driver is great, as we have previously discussed, but
the header file does not belong into include/asm really, because it
is too hardware specific and we are trying to keep such stuff out
of that place.

One option would be to move the pm.c along with the cpuidle.c file,
in particular that might turn out to be a good idea if other
platforms have the same requirement.

Aside from that, it would be good to hear from the AT91 maintainers
how they want to deal with the removal of CONFIG_ARCH_AT91_*
conditionals, which AFACIT will be necessary anyway in order to allow
building a single kernel across multiple at91 targets. Once that
problem is solved for pm.h, the question of where to place that header
may be resolved, too.

	Arnd

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

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

On 01/06/2012 06:30 PM, Arnd Bergmann wrote:
> On Friday 06 January 2012, Daniel Lezcano wrote:
>> Move the location of the pm.h header file to the include directory,
>> so it can be included from another place from the current one.
>>
>> That will allow the next patch which moves the cpuidle code to the
>> drivers/cpuidle directory.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0
>>  arch/arm/mach-at91/cpuidle.c                       |    2 +-
>>  arch/arm/mach-at91/pm.c                            |    2 +-
>>  3 files changed, 2 insertions(+), 2 deletions(-)
>>  rename arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} (100%)
> 
> Moving the driver is great, as we have previously discussed, but
> the header file does not belong into include/asm really, because it
> is too hardware specific and we are trying to keep such stuff out
> of that place.
> 
> One option would be to move the pm.c along with the cpuidle.c file,
> in particular that might turn out to be a good idea if other
> platforms have the same requirement.
> 
> Aside from that, it would be good to hear from the AT91 maintainers
> how they want to deal with the removal of CONFIG_ARCH_AT91_*
> conditionals, which AFACIT will be necessary anyway in order to allow
> building a single kernel across multiple at91 targets. Once that
> problem is solved for pm.h, the question of where to place that header
> may be resolved, too.

Mmmh, the problem will be more complex with other architectures I think.

For instance, the omap cpuidle driver includes

#include "powerdomain.h"
#include "clockdomain.h"

#include "pm.h"
#include "control.h"
#include "common.h"

These include files are used by the other .c files in the same directory.



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

iQEcBAEBAgAGBQJPB4GZAAoJEAKBbMCpUGYAI1YH/jJIoBtKku7YgXfBTj9Mqf92
nQQqDoGiQGNNXM2TDBKS4t8EyiRa9YuHYHAYS16JaoUPDTj1Zwzo3E3fTFLQcFi6
x0l3H82c6VvnqtY6qwO5ym9bB8m9kSdJjAFqIDPO987hOB4jeFBA6EE4SgcMuiHR
4rWzQVZjHtA1nf/9yrcVgwush75psWNHudnLn/0TyM14efuys0xUx+EMWqQzmpzj
aBc+TgWn/HUXX261VCYmTgUIo/L/xV7eU/IRSR7Nd8AWPp6hrm4x2C4SO5Kt8JFE
OA2peOGEIlNqre4tuKf4QXT1/QkiwTW6j194wag3kaLHBaARnXT/vX+xnYVqzjU=
=2H/1
-----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

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

On 01/06/2012 06:30 PM, Arnd Bergmann wrote:
> On Friday 06 January 2012, Daniel Lezcano wrote:
>> Move the location of the pm.h header file to the include directory,
>> so it can be included from another place from the current one.
>>
>> That will allow the next patch which moves the cpuidle code to the
>> drivers/cpuidle directory.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0
>>  arch/arm/mach-at91/cpuidle.c                       |    2 +-
>>  arch/arm/mach-at91/pm.c                            |    2 +-
>>  3 files changed, 2 insertions(+), 2 deletions(-)
>>  rename arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} (100%)
> 
> Moving the driver is great, as we have previously discussed, but
> the header file does not belong into include/asm really, because it
> is too hardware specific and we are trying to keep such stuff out
> of that place.

Hi Arnd,

I just want to clarify because I am not sure there is no confusion here
where the header will be moved to. The diff annotation is not clear and
suggests the file is moved to "next/include/asm".

Actually, the header moves from :

	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.

This place and the renaming of the file complies with the comments of
Russell, where this place is shared across the different ARM
architectures, Rob where the file is renamed for a single kernel image
and you where the cpuidle driver is moved to the drivers directory.
Looking at different drivers, it appears that is the case for the other
drivers.

For instance:

i7300_idle.c includes arch/x86/include/asm/idle.h
intel_idle.c includes arch/x86/include/asm/msr.h

So, from my POV, that makes sense to move pm.h to this directory, no ?

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/

iQEcBAEBAgAGBQJPCs01AAoJEAKBbMCpUGYABb4IAMUuSl8Q0s6cKr8E7sgTGp3F
r758UVNTYR5X4AX67kmBRDeQGvnG/yifFAmTZ31G6fk5NKLuUMm6giiXua83PYfV
KvOKX30JHtqYhutcZyYemMTm5fgS1PbRmjEvEillaP9W7/SiKmg3hoOpPXPFW1k7
8Td6rN3SosCblBYcmq0BMqb3oHwjEXnfBJmkxELA37I5J6D33X8v4YUJ0QYANlcp
MK8yNg3U6IAqLXAa8jvzWVbDLwaewj+kYkHwr4cFCBLnX5mKuBx387AsA8GD6dzt
VmvoAi2PyX5D6hmSX58CVxHKnjke+iOsDKU9jWhwVRccPNrnstl1DlINknvZnk4=
=1otd
-----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Russell King - ARM Linux]

On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
> Actually, the header moves from :
> 
> 	arch/arm/mach-at91/pm.h
> to:
> 	arch/arm/include/asm/at91_pm.h.
> 
> This place and the renaming of the file complies with the comments of
> Russell,

No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
to be littered with hundreds of crappy platform specific header files.

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

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

On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
>> Actually, the header moves from :
>>
>> 	arch/arm/mach-at91/pm.h
>> to:
>> 	arch/arm/include/asm/at91_pm.h.
>>
>> This place and the renaming of the file complies with the comments of
>> Russell,
> 
> No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
> to be littered with hundreds of crappy platform specific header files.

Ok. Actually there are 9 pm.h files but I agree with a domino effect we
can have more header files brought to this directory like "control.h",
"powerdomain.h", etc ...

Does it make sense to merge all the pm.h file in a single pm.h which
will be located in arch/arm/include/asm ?

And we separate the different archs specific with #ifdef
CONFIG_ARCH_AT91, CONFIG_ARCH_OMAP, etc ...

The resulting file will be bigger but that will be easier to find a
pattern we can factor out in the header file and that will encourage the
developers to share the code across the different arch.

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/

iQEcBAEBAgAGBQJPCvGYAAoJEAKBbMCpUGYAGTMH/iLSep3kiUfxJiDLlxLoPkcs
pyvJGnD/leJOvykxd6QQ3dCs+4i2tbYnMEGVtfYWbTHOraOTa1u5oRcS/F0yNOYu
kifKYyLKJPT9lkYlaDBU9/cWZDhHdAQpu+2vKnVDIJLeSwBDxCEROoPQaWGs2cnw
0k06iCE/Zq9V5duFE/u7n0RX7XpJCLmdMcRDRw2GW7WuZ6DA2BeU2MEFudQ1NLK1
bB9zyVVCBxPVVdqjgaSagU0LTCPUeHALsYQVUqpPYMuYK30/ACyakyA/jLFn52lD
x47ybOuLOGSl4c0/wS1duCBbIFNYsT/kBcVXFXA/usAaCjW12H7OV+9MEA7lB8E=
=y5HS
-----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Russell King - ARM Linux]

On Mon, Jan 09, 2012 at 02:54:32PM +0100, Daniel Lezcano wrote:
> On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
> > On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
> >> Actually, the header moves from :
> >>
> >> 	arch/arm/mach-at91/pm.h
> >> to:
> >> 	arch/arm/include/asm/at91_pm.h.
> >>
> >> This place and the renaming of the file complies with the comments of
> >> Russell,
> > 
> > No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
> > to be littered with hundreds of crappy platform specific header files.
> 
> Ok. Actually there are 9 pm.h files but I agree with a domino effect we
> can have more header files brought to this directory like "control.h",
> "powerdomain.h", etc ...
> 
> Does it make sense to merge all the pm.h file in a single pm.h which
> will be located in arch/arm/include/asm ?

No it doesn't.  If moving something out of arch/arm means that we have to
buggerize the header files, then moving it out of arch/arm is the wrong
thing to do.  What the need to bugger about with header files is telling
you is that the code you're moving (in its existing form) is intimitely
tied to the SoC.

There's two solutions to that: either leave it where it is, or first
sort out why it's intimitely tied, and what can be done to remove its
dependence on the SoC.

I've finally taken a deeper look at what's going on here... 

arch/arm/mach-at91/pm.h is full of crap:

#ifdef CONFIG_ARCH_AT91RM9200
static inline u32 sdram_selfrefresh_enable(void)
{
...
}
#elif defined(CONFIG_ARCH_AT91CAP9)
static inline u32 sdram_selfrefresh_enable(void)
{
...
}
#elif defined(CONFIG_ARCH_AT91SAM9G45)
static inline u32 sdram_selfrefresh_enable(void)
{
...
}
#else
static inline u32 sdram_selfrefresh_enable(void)
{
...
}
#endif

And there's no way this should ever move out of arch/arm/mach-at91,
ever.  It's a totally broken structure, and it needs fixing.  A first
step, from just looking at the file, to fix this within the bounds of
AT91 would be:

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

in a header file, and then have the AT91RM9200, etc code providing
a set of functions and operation structure to be registered with the
core code.

That reduces this pm.h file right down to just this structure
definition.

However, looking at pm.c though, is any of this really necessary?

static int at91_pm_enter(suspend_state_t state)
{
        u32 saved_lpr;

        at91_gpio_suspend();
        at91_irq_suspend();

Argh, why are these specific suspend calls here - why aren't these
implemented using the syscore_ops stuff?  (Treat that as a separate
complaint.)

        switch (state) {
                case PM_SUSPEND_MEM:

Argh, why the extra indent (it's something which CodingStyle recommends
against.)

			...
                case PM_SUSPEND_STANDBY:
                        /*
                         * NOTE: the Wait-for-Interrupt instruction needs to be
                         * in icache so no SDRAM accesses are needed until the
                         * wakeup IRQ occurs and self-refresh is terminated.
                         * 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();
                        wait_for_interrupt_enable();
                        sdram_selfrefresh_disable(saved_lpr);
                        break;

This code has no guarantees what so ever that the compiler won't break
it - and doing my suggestion above will break it.  Your
wait_for_interrupt_enable() even calls out to other code in some cases.

So, what I suggest instead is to have the 'standby' state call a SoC
specific function, where you can have much better control over the
code generation:

void (*at91_standby)(void);
...

static int at91_pm_enter(suspend_state_t state)
{
        switch (state) {
...
	case PM_SUSPEND_STANDBY:
		if (at91_standby)
			at91_standby();
		break;

Then, have the SoC specific code implement a function in each SoC
specific file:

static void at91rm9200_standby(void)
{
	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	%1, [%2]\n\t"
	"	str	%3, [%4]\n\t"
	"	mcr	p15, 0, %0, c7, c0, 4\n\t"
	"	str	%5, [%2]"
		:
		: "r" (0), "r" (0), "r" (virt-addr-of-LPR),
		  "r" (1), "r" (virt-addr-of-SRR),
		  "r" (lpr));
}

...

void at91rm9200_init_early(void)
{
	at91_standby = at91rm9200_standby;
}

This makes sure that the code is aligned, and you know how those register
writes are going to be done - and you know that they're going to be kept
as one chunk of assembly.  This can also replace the buggy code found in
cpuidle.c:

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

with a call to at91_standby().

I think this has to be done _first_ before there's any thought about
moving cpuidle out of arch/arm/mach-at91.

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

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

On 01/09/2012 03:44 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2012 at 02:54:32PM +0100, Daniel Lezcano wrote:
>> On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
>>>> Actually, the header moves from :
>>>>
>>>> 	arch/arm/mach-at91/pm.h
>>>> to:
>>>> 	arch/arm/include/asm/at91_pm.h.
>>>>
>>>> This place and the renaming of the file complies with the comments of
>>>> Russell,
>>>
>>> No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
>>> to be littered with hundreds of crappy platform specific header files.
>>
>> Ok. Actually there are 9 pm.h files but I agree with a domino effect we
>> can have more header files brought to this directory like "control.h",
>> "powerdomain.h", etc ...
>>
>> Does it make sense to merge all the pm.h file in a single pm.h which
>> will be located in arch/arm/include/asm ?
> 
> No it doesn't.  If moving something out of arch/arm means that we have to
> buggerize the header files, then moving it out of arch/arm is the wrong
> thing to do.  What the need to bugger about with header files is telling
> you is that the code you're moving (in its existing form) is intimitely
> tied to the SoC.
> 
> There's two solutions to that: either leave it where it is, or first
> sort out why it's intimitely tied, and what can be done to remove its
> dependence on the SoC.

[ ... ]

> I think this has to be done _first_ before there's any thought about
> moving cpuidle out of arch/arm/mach-at91.

Thanks Russell for your detailed explanation. I will follow your idea
and propose something based on ops. That looks definitively nicer and
better for a long term maintenance.

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

iQEcBAEBAgAGBQJPCwEgAAoJEAKBbMCpUGYAni4H/3M4LTAAcSphSbmsBrB8CNjP
o7XL+B6RP1nG0Yd2Sisv7RbPftqHEyDNN9NhoSYD9TconnNYJvgrySmjSPLNCd28
HnXmPMgQD/UTECDs1KydJx8LtaRwPRiwfYdmG/IOUXPc1HO6n/tZ0HTzq9ZJpsym
mrT2G6b+hLjCrFwdRO//PXaHjVR1GbId3G5wg0PmNR7nlg0Ec1nQCdbc1kYopMZy
B0hE/Blmg0XGxn9FUkl6UXI2zLmPtWn/1IE0n99s5kmA3tW1CSqnuQRsjTbtWP/l
cx05TV70DtBh1j37s42nVCWZwmPAVmy+L8qty8277/QIVc3HAqiZ6/QXc1cgi6w=
=qgml
-----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:44 Mon 09 Jan     , Russell King - ARM Linux wrote:
> On Mon, Jan 09, 2012 at 02:54:32PM +0100, Daniel Lezcano wrote:
> > On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
> > > On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
> > >> Actually, the header moves from :
> > >>
> > >> 	arch/arm/mach-at91/pm.h
> > >> to:
> > >> 	arch/arm/include/asm/at91_pm.h.
> > >>
> > >> This place and the renaming of the file complies with the comments of
> > >> Russell,
> > > 
> > > No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
> > > to be littered with hundreds of crappy platform specific header files.
> > 
> > Ok. Actually there are 9 pm.h files but I agree with a domino effect we
> > can have more header files brought to this directory like "control.h",
> > "powerdomain.h", etc ...
> > 
> > Does it make sense to merge all the pm.h file in a single pm.h which
> > will be located in arch/arm/include/asm ?
> 
> No it doesn't.  If moving something out of arch/arm means that we have to
> buggerize the header files, then moving it out of arch/arm is the wrong
> thing to do.  What the need to bugger about with header files is telling
> you is that the code you're moving (in its existing form) is intimitely
> tied to the SoC.
> 
> There's two solutions to that: either leave it where it is, or first
> sort out why it's intimitely tied, and what can be done to remove its
> dependence on the SoC.
> 
> I've finally taken a deeper look at what's going on here... 
> 
> arch/arm/mach-at91/pm.h is full of crap:
I work on it but work on other clean up first

Best Regards,
J.

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:48 Mon 09 Jan     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:44 Mon 09 Jan     , Russell King - ARM Linux wrote:
> > On Mon, Jan 09, 2012 at 02:54:32PM +0100, Daniel Lezcano wrote:
> > > On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
> > > > On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
> > > >> Actually, the header moves from :
> > > >>
> > > >> 	arch/arm/mach-at91/pm.h
> > > >> to:
> > > >> 	arch/arm/include/asm/at91_pm.h.
> > > >>
> > > >> This place and the renaming of the file complies with the comments of
> > > >> Russell,
> > > > 
> > > > No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
> > > > to be littered with hundreds of crappy platform specific header files.
> > > 
> > > Ok. Actually there are 9 pm.h files but I agree with a domino effect we
> > > can have more header files brought to this directory like "control.h",
> > > "powerdomain.h", etc ...
> > > 
> > > Does it make sense to merge all the pm.h file in a single pm.h which
> > > will be located in arch/arm/include/asm ?
> > 
> > No it doesn't.  If moving something out of arch/arm means that we have to
> > buggerize the header files, then moving it out of arch/arm is the wrong
> > thing to do.  What the need to bugger about with header files is telling
> > you is that the code you're moving (in its existing form) is intimitely
> > tied to the SoC.
> > 
> > There's two solutions to that: either leave it where it is, or first
> > sort out why it's intimitely tied, and what can be done to remove its
> > dependence on the SoC.
> > 
> > I've finally taken a deeper look at what's going on here... 
> > 
> > arch/arm/mach-at91/pm.h is full of crap:
> I work on it but work on other clean up first
this code need to be clean with the pm_slowclock.S too to support multiple soc

and we need to drop the at91_sys_read/write stuff too

I work on this right now

Best Regards,
J.

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Nicolas Ferre]

On 01/09/2012 06:09 PM, Jean-Christophe PLAGNIOL-VILLARD :
> On 17:48 Mon 09 Jan     , Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 14:44 Mon 09 Jan     , Russell King - ARM Linux wrote:
>>> On Mon, Jan 09, 2012 at 02:54:32PM +0100, Daniel Lezcano wrote:
>>>> On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
>>>>> On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
>>>>>> Actually, the header moves from :
>>>>>>
>>>>>> 	arch/arm/mach-at91/pm.h
>>>>>> to:
>>>>>> 	arch/arm/include/asm/at91_pm.h.
>>>>>>
>>>>>> This place and the renaming of the file complies with the comments of
>>>>>> Russell,
>>>>>
>>>>> No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
>>>>> to be littered with hundreds of crappy platform specific header files.
>>>>
>>>> Ok. Actually there are 9 pm.h files but I agree with a domino effect we
>>>> can have more header files brought to this directory like "control.h",
>>>> "powerdomain.h", etc ...
>>>>
>>>> Does it make sense to merge all the pm.h file in a single pm.h which
>>>> will be located in arch/arm/include/asm ?
>>>
>>> No it doesn't.  If moving something out of arch/arm means that we have to
>>> buggerize the header files, then moving it out of arch/arm is the wrong
>>> thing to do.  What the need to bugger about with header files is telling
>>> you is that the code you're moving (in its existing form) is intimitely
>>> tied to the SoC.
>>>
>>> There's two solutions to that: either leave it where it is, or first
>>> sort out why it's intimitely tied, and what can be done to remove its
>>> dependence on the SoC.
>>>
>>> I've finally taken a deeper look at what's going on here... 
>>>
>>> arch/arm/mach-at91/pm.h is full of crap:
>> I work on it but work on other clean up first
> this code need to be clean with the pm_slowclock.S too to support multiple soc
> 
> and we need to drop the at91_sys_read/write stuff too
> 
> I work on this right now

Yes, but as patches are not already there so please Daniel, feel free to
contribute ;-)

Bye,
-- 
Nicolas Ferre

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Rob Herring]

On 01/09/2012 07:54 AM, Daniel Lezcano wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
>> On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
>>> Actually, the header moves from :
>>>
>>> 	arch/arm/mach-at91/pm.h
>>> to:
>>> 	arch/arm/include/asm/at91_pm.h.
>>>
>>> This place and the renaming of the file complies with the comments of
>>> Russell,
>>
>> No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
>> to be littered with hundreds of crappy platform specific header files.
> 
> Ok. Actually there are 9 pm.h files but I agree with a domino effect we
> can have more header files brought to this directory like "control.h",
> "powerdomain.h", etc ...
> 
> Does it make sense to merge all the pm.h file in a single pm.h which
> will be located in arch/arm/include/asm ?
> 
> And we separate the different archs specific with #ifdef
> CONFIG_ARCH_AT91, CONFIG_ARCH_OMAP, etc ...
> 
> The resulting file will be bigger but that will be easier to find a
> pattern we can factor out in the header file and that will encourage the
> developers to share the code across the different arch.
> 

No!! Is moving this even necessary with Rob Lee's common cpuidle driver?
There obviously needs to be some coordination here.

wait_for_interrupt_enable is not used by cpuidle, so you can move that
into pm.c. SDRAM self-refresh setup may be common enough we can define
standard function ptrs for that and move more code into the common
cpuidle driver.

Rob

> 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/
> 
> iQEcBAEBAgAGBQJPCvGYAAoJEAKBbMCpUGYAGTMH/iLSep3kiUfxJiDLlxLoPkcs
> pyvJGnD/leJOvykxd6QQ3dCs+4i2tbYnMEGVtfYWbTHOraOTa1u5oRcS/F0yNOYu
> kifKYyLKJPT9lkYlaDBU9/cWZDhHdAQpu+2vKnVDIJLeSwBDxCEROoPQaWGs2cnw
> 0k06iCE/Zq9V5duFE/u7n0RX7XpJCLmdMcRDRw2GW7WuZ6DA2BeU2MEFudQ1NLK1
> bB9zyVVCBxPVVdqjgaSagU0LTCPUeHALsYQVUqpPYMuYK30/ACyakyA/jLFn52lD
> x47ybOuLOGSl4c0/wS1duCBbIFNYsT/kBcVXFXA/usAaCjW12H7OV+9MEA7lB8E=
> =y5HS
> -----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Daniel Lezcano]

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

On 01/09/2012 03:46 PM, Rob Herring wrote:
> On 01/09/2012 07:54 AM, Daniel Lezcano wrote:
> On 01/09/2012 12:29 PM, Russell King - ARM Linux wrote:
>>>> On Mon, Jan 09, 2012 at 12:19:17PM +0100, Daniel Lezcano wrote:
>>>>> Actually, the header moves from :
>>>>>
>>>>> 	arch/arm/mach-at91/pm.h
>>>>> to:
>>>>> 	arch/arm/include/asm/at91_pm.h.
>>>>>
>>>>> This place and the renaming of the file complies with the comments of
>>>>> Russell,
>>>>
>>>> No it doesn't.  There's absolutely no way in hell I want arch/arm/include/asm
>>>> to be littered with hundreds of crappy platform specific header files.
> 
> Ok. Actually there are 9 pm.h files but I agree with a domino effect we
> can have more header files brought to this directory like "control.h",
> "powerdomain.h", etc ...
> 
> Does it make sense to merge all the pm.h file in a single pm.h which
> will be located in arch/arm/include/asm ?
> 
> And we separate the different archs specific with #ifdef
> CONFIG_ARCH_AT91, CONFIG_ARCH_OMAP, etc ...
> 
> The resulting file will be bigger but that will be easier to find a
> pattern we can factor out in the header file and that will encourage the
> developers to share the code across the different arch.
> 
> 
>> No!! Is moving this even necessary with Rob Lee's common cpuidle driver?
>> There obviously needs to be some coordination here.

No, this is independent with Rob's work. It is about moving cpuidle to
the drivers directory. I am planning to respin this future patchset on
top of Rob's one.

>> wait_for_interrupt_enable is not used by cpuidle, so you can move that
>> into pm.c. SDRAM self-refresh setup may be common enough we can define
>> standard function ptrs for that and move more code into the common
>> cpuidle driver.

Ok. Thanks. I will take that into account and follow Russell's idea by
making the header less SOC specific.

Thanks for your comments.

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

iQEcBAEBAgAGBQJPCwL6AAoJEAKBbMCpUGYAvlUIALBDffJLPaiDZgJBvlb5vIj/
9ZMvURcSCSIxTrdkdUwm2BsqhrWmqlNmYpF3EUB8RABAnaXs4vw8Zdyn4LJtmrfx
OtjRS9ieBaQOIl/c22g75iF08XhtElG0SI+oACBhcuW+N67Ps8flekk2AKuyOFv0
tczAzhQU2op8hjKRkq84iZVC+hKhSHtS0dan49to/iC4ynHmjN+keTZEtbPEwI36
YF/gyJcgmA7PsAQts3ShHPUJ3NpLh4vL7xB267WGNdY3cGqA3nSLq2eeM39gPBa+
PLWhJg7PoGEVepOPl1doCM07vk9v0PoZW5klsAIP3uQ1HuVvY1Rb8wHGc1KqHX8=
=KntQ
-----END PGP SIGNATURE-----

[PATCH 1/2] at91 : move pm.h header to arch/arm/include/asm

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:48 Fri 06 Jan     , Daniel Lezcano wrote:
> Move the location of the pm.h header file to the include directory,
> so it can be included from another place from the current one.
> 
> That will allow the next patch which moves the cpuidle code to the
> drivers/cpuidle directory.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0
include/linux/at91_pm.h
>  arch/arm/mach-at91/cpuidle.c                       |    2 +-
>  arch/arm/mach-at91/pm.c                            |    2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} (100%)
sorry I've patch in the quere that touch this part so you will have to wait

Best Regards,
J.