at91 : move pm.h header to arch/arm/include/asm

PATCH New Daniel Lezcano 6 participants 15 messages Jan 6, 2012 – Jan 9, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm/Microchip (AT91) SoC support CPU idle time management framework

lore

Share & tools

lore

Daniel Lezcano

author

2012-01-06 15:48 UTC #0 lore raw

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

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

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

arch/arm/include/asm/at91_pm.h renamed from arch/arm/mach-at91/pm.h +0 -0

similarity index 100%
rename from arch/arm/mach-at91/pm.h
rename to arch/arm/include/asm/at91_pm.h

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

index a851e6c..8281eec 100644
@@ -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

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

index 62ad955..86bc243 100644
@@ -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.

Signature · 1 line

1.7.4.1

Daniel Lezcano

author

2012-01-06 15:48 UTC #1 lore raw

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

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

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

arch/arm/mach-at91/Kconfig +5 -0

index 4f991f2..ddeaf0e 100644
@@ -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

arch/arm/mach-at91/Makefile +0 -1

index 242174f..ce0cd4c 100644
@@ -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

drivers/cpuidle/Makefile +1 -0

index 5634f88..31c9b5f 100644
@@ -3,3 +3,4 @@
 #
 
 obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
+obj-$(CONFIG_AT91_IDLE) += at91_idle.o

drivers/cpuidle/at91_idle.c renamed from arch/arm/mach-at91/cpuidle.c +0 -0

similarity index 100%
rename from arch/arm/mach-at91/cpuidle.c
rename to drivers/cpuidle/at91_idle.c

Signature · 1 line

1.7.4.1

Arnd Bergmann

2012-01-06 17:30 UTC #2 lore raw

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

In reply to Daniel Lezcano

Quoted 6 lines · 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.

Quoted 7 lines

Signed-off-by Daniel Lezcano <[email protected]>

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

Daniel Lezcano

author

2012-01-06 23:19 UTC #3 lore raw

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

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

Quoted 24 lines · Arnd Bergmann

Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by Daniel Lezcano <[email protected]>
---
 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.

Quoted 6 lines

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

Daniel Lezcano

author

2012-01-09 11:19 UTC #4 lore raw

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

In reply to Arnd Bergmann

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

Quoted 15 lines · Arnd Bergmann

Quoted 6 lines · 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.
Quoted 7 lines
Signed-off-by Daniel Lezcano <[email protected]>
---
 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%)

Quoted 4 lines

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

Russell King - ARM Linux

2012-01-09 11:29 UTC #5 lore raw

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

Quoted 8 lines · Daniel Lezcano

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.

Daniel Lezcano

author

2012-01-09 13:54 UTC #6 lore raw

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

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

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.

Quoted 5 lines

Quoted 2 lines
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-----

Russell King - ARM Linux

2012-01-09 14:44 UTC #7 lore raw

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

Quoted 14 lines · Daniel Lezcano

Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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.

Quoted 6 lines

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.

Daniel Lezcano

author

2012-01-09 15:00 UTC #8 lore raw

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

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

Quoted 31 lines · Russell King - ARM Linux

Quoted 14 lines · Daniel Lezcano
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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.

[ ... ]

Quoted 2 lines

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

Jean-Christophe PLAGNIOL-VILLARD

2012-01-09 16:48 UTC #9 lore raw

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

In reply to Russell King - ARM Linux

Quoted 28 lines · Russell King - ARM Linux

Quoted 14 lines · Daniel Lezcano
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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.

Quoted 7 lines

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.

Jean-Christophe PLAGNIOL-VILLARD

2012-01-09 17:09 UTC #10 lore raw

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

Quoted 29 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 28 lines · Russell King - ARM Linux
Quoted 14 lines · Daniel Lezcano
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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.

Quoted 8 lines

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

Nicolas Ferre

2012-01-09 17:41 UTC #11 lore raw

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

On 01/09/2012 06:09 PM, Jean-Christophe PLAGNIOL-VILLARD :

Quoted 36 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 35 lines · Russell King - ARM Linux
Quoted 34 lines · Daniel Lezcano
Quoted 14 lines · Russell King - ARM Linux
Quoted 7 lines · Daniel Lezcano
Quoted 6 lines
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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...

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
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,

Signature · 1 line

Nicolas Ferre

Rob Herring

2012-01-09 14:46 UTC #12 lore raw

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

In reply to Daniel Lezcano

Quoted 24 lines · Daniel Lezcano

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Daniel Lezcano
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 2 lines
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 ?

Quoted 7 lines

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

Quoted 22 lines

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

Daniel Lezcano

author

2012-01-09 15:08 UTC #13 lore raw

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

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

Quoted 22 lines · Rob Herring

On 01/09/2012 07:54 AM, Daniel Lezcano wrote:
Quoted 7 lines · Russell King - ARM Linux
Quoted 7 lines
Quoted 7 lines · Daniel Lezcano
Quoted 6 lines
Actually, the header moves from :
	arch/arm/mach-at91/pm.h
to:
	arch/arm/include/asm/at91_pm.h.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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 ?

Quoted 10 lines

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.
Quoted 2 lines
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.

Quoted 4 lines

Quoted 4 lines
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-----

Jean-Christophe PLAGNIOL-VILLARD

2012-01-07 10:45 UTC #14 lore raw

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

In reply to Daniel Lezcano

Quoted 9 lines · 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 <[email protected]>
---
 arch/arm/{mach-at91/pm.h => include/asm/at91_pm.h} |    0

include/linux/at91_pm.h

Quoted 4 lines

 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.