Hello all,
This patch is intended to fix compilation errors when I tried to add in
power management to my Atmel AT91SAM9260 board. First, there is no
separate memory controller in the 9260 as there is in the 9200, so calls
to do anything with it in pm.c fail.
Secondly, at91_suspend_entering_slow_clock() is not declared extern by
drivers that use it. This used to cause only a warning, but in
2.6.23-rc1 and beyond, it is an error.
Signed-off-by: Andrew J. Herzig <[email protected]>
---
arch/arm/mach-at91/pm.c | 5 ++++-
drivers/serial/atmel_serial.c | 2 ++
drivers/usb/gadget/at91_udc.c | 2 ++
drivers/usb/host/ohci-at91.c | 1 +
4 files changed, 9 insertions(+), 1 deletion(-)
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c
linux-2.6.23-rc2/arch/arm/mach-at91/pm.c
--- linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c 2007-08-08
13:28:37.000000000 -0400
+++ linux-2.6.23-rc2/arch/arm/mach-at91/pm.c 2007-08-08
13:36:43.000000000 -0400
@@ -176,7 +176,9 @@ static int at91_pm_enter(suspend_state_t
*/
asm("b 1f; .align 5; 1:");
asm("mcr p15, 0, r0, c7, c10, 4"); /* drain
write buffer */
+#if defined(CONFIG_ARCH_AT91RM9200)
at91_sys_write(AT91_SDRAMC_SRR, 1); /*
self-refresh mode */
+#endif
/* fall though to next state */
case PM_SUSPEND_ON:
@@ -218,8 +220,9 @@ static int __init at91_pm_init(void)
#endif
/* Disable SDRAM low-power mode. Cannot be used with
self-refresh. */
+#if defined(CONFIG_ARCH_AT91RM9200)
at91_sys_write(AT91_SDRAMC_LPR, 0);
-
+#endif
pm_set_ops(&at91_pm_ops);
return 0;
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
linux-2.6.23-rc2/drivers/serial/atmel_serial.c
--- linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
2007-08-08 13:28:33.000000000 -0400
+++ linux-2.6.23-rc2/drivers/serial/atmel_serial.c 2007-08-08
13:32:52.000000000 -0400
@@ -916,6 +916,8 @@ static struct uart_driver atmel_uart = {
};
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
+
static int atmel_serial_suspend(struct platform_device *pdev,
pm_message_t state)
{
struct uart_port *port = platform_get_drvdata(pdev);
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c
--- linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
2007-08-08 13:28:27.000000000 -0400
+++ linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c 2007-08-08
13:33:21.000000000 -0400
@@ -1770,6 +1770,8 @@ static int __exit at91udc_remove(struct
}
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
+
static int at91udc_suspend(struct platform_device *pdev, pm_message_t
mesg)
{
struct at91_udc *udc = platform_get_drvdata(pdev);
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c
--- linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
2007-08-08 13:28:27.000000000 -0400
+++ linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c 2007-08-08
13:32:45.000000000 -0400
@@ -282,6 +282,7 @@ static int ohci_hcd_at91_drv_remove(stru
}
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
static int
ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t
mesg)
On Thu, 2007-08-09 at 08:10 -0400, Andy Herzig wrote:
> Hello all,
>
> This patch is intended to fix compilation errors when I tried to add in
> power management to my Atmel AT91SAM9260 board. First, there is no
> separate memory controller in the 9260 as there is in the 9200, so calls
> to do anything with it in pm.c fail.
>
> Secondly, at91_suspend_entering_slow_clock() is not declared extern by
> drivers that use it. This used to cause only a warning, but in
> 2.6.23-rc1 and beyond, it is an error.
Sorry, the previous patch was fouled up.
Signed-off-by: Andrew J. Herzig <[email protected]>
---
arch/arm/mach-at91/pm.c | 5 ++++-
drivers/serial/atmel_serial.c | 2 ++
drivers/usb/gadget/at91_udc.c | 2 ++
drivers/usb/host/ohci-at91.c | 1 +
4 files changed, 9 insertions(+), 1 deletion(-)
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c
linux-2.6.23-rc2/arch/arm/mach-at91/pm.c
--- linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c 2007-08-08
13:28:37.000000000 -0400
+++ linux-2.6.23-rc2/arch/arm/mach-at91/pm.c 2007-08-08
13:36:43.000000000 -0400
@@ -176,7 +176,9 @@ static int at91_pm_enter(suspend_state_t
*/
asm("b 1f; .align 5; 1:");
asm("mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */
+#if defined(CONFIG_ARCH_AT91RM9200)
at91_sys_write(AT91_SDRAMC_SRR, 1); /* self-refresh mode */
+#endif
/* fall though to next state */
case PM_SUSPEND_ON:
@@ -218,8 +220,9 @@ static int __init at91_pm_init(void)
#endif
/* Disable SDRAM low-power mode. Cannot be used with self-refresh. */
+#if defined(CONFIG_ARCH_AT91RM9200)
at91_sys_write(AT91_SDRAMC_LPR, 0);
-
+#endif
pm_set_ops(&at91_pm_ops);
return 0;
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
linux-2.6.23-rc2/drivers/serial/atmel_serial.c
--- linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c 2007-08-08
13:28:33.000000000 -0400
+++ linux-2.6.23-rc2/drivers/serial/atmel_serial.c 2007-08-08
13:32:52.000000000 -0400
@@ -916,6 +916,8 @@ static struct uart_driver atmel_uart = {
};
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
+
static int atmel_serial_suspend(struct platform_device *pdev,
pm_message_t state)
{
struct uart_port *port = platform_get_drvdata(pdev);
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c
--- linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c 2007-08-08
13:28:27.000000000 -0400
+++ linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c 2007-08-08
13:33:21.000000000 -0400
@@ -1770,6 +1770,8 @@ static int __exit at91udc_remove(struct
}
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
+
static int at91udc_suspend(struct platform_device *pdev, pm_message_t
mesg)
{
struct at91_udc *udc = platform_get_drvdata(pdev);
diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c
--- linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c 2007-08-08
13:28:27.000000000 -0400
+++ linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c 2007-08-08
13:32:45.000000000 -0400
@@ -282,6 +282,7 @@ static int ohci_hcd_at91_drv_remove(stru
}
#ifdef CONFIG_PM
+extern int at91_suspend_entering_slow_clock(void);
static int
ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t
mesg)
(added linux-arm-kernel to CC)
Am Donnerstag 09 August 2007 14:10 schrieb Andy Herzig:
> Hello all,
>
> This patch is intended to fix compilation errors when I tried to add in
> power management to my Atmel AT91SAM9260 board. First, there is no
> separate memory controller in the 9260 as there is in the 9200, so calls
> to do anything with it in pm.c fail.
Correct. I made the same discovery with my AT91SAM9263 board.
>
> Secondly, at91_suspend_entering_slow_clock() is not declared extern by
> drivers that use it. This used to cause only a warning, but in
> 2.6.23-rc1 and beyond, it is an error.
Patch looks good to me. Kernel compiles for my 9263. Didn't test
suspend/resume yet, though.
>
> Signed-off-by: Andrew J. Herzig <[email protected]>
Acked-by: Hans J. Koch <[email protected]>
> ---
> arch/arm/mach-at91/pm.c | 5 ++++-
> drivers/serial/atmel_serial.c | 2 ++
> drivers/usb/gadget/at91_udc.c | 2 ++
> drivers/usb/host/ohci-at91.c | 1 +
> 4 files changed, 9 insertions(+), 1 deletion(-)
>
> diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c
> linux-2.6.23-rc2/arch/arm/mach-at91/pm.c
> --- linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c 2007-08-08
> 13:28:37.000000000 -0400
> +++ linux-2.6.23-rc2/arch/arm/mach-at91/pm.c 2007-08-08
> 13:36:43.000000000 -0400
> @@ -176,7 +176,9 @@ static int at91_pm_enter(suspend_state_t
> */
> asm("b 1f; .align 5; 1:");
> asm("mcr p15, 0, r0, c7, c10, 4"); /* drain
> write buffer */
> +#if defined(CONFIG_ARCH_AT91RM9200)
> at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> self-refresh mode */
> +#endif
> /* fall though to next state */
>
> case PM_SUSPEND_ON:
> @@ -218,8 +220,9 @@ static int __init at91_pm_init(void)
> #endif
>
> /* Disable SDRAM low-power mode. Cannot be used with
> self-refresh. */
> +#if defined(CONFIG_ARCH_AT91RM9200)
> at91_sys_write(AT91_SDRAMC_LPR, 0);
> -
> +#endif
> pm_set_ops(&at91_pm_ops);
>
> return 0;
> diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
> linux-2.6.23-rc2/drivers/serial/atmel_serial.c
> --- linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
> 2007-08-08 13:28:33.000000000 -0400
> +++ linux-2.6.23-rc2/drivers/serial/atmel_serial.c 2007-08-08
> 13:32:52.000000000 -0400
> @@ -916,6 +916,8 @@ static struct uart_driver atmel_uart = {
> };
>
> #ifdef CONFIG_PM
> +extern int at91_suspend_entering_slow_clock(void);
> +
> static int atmel_serial_suspend(struct platform_device *pdev,
> pm_message_t state)
> {
> struct uart_port *port = platform_get_drvdata(pdev);
> diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
> linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c
> --- linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
> 2007-08-08 13:28:27.000000000 -0400
> +++ linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c 2007-08-08
> 13:33:21.000000000 -0400
> @@ -1770,6 +1770,8 @@ static int __exit at91udc_remove(struct
> }
>
> #ifdef CONFIG_PM
> +extern int at91_suspend_entering_slow_clock(void);
> +
> static int at91udc_suspend(struct platform_device *pdev, pm_message_t
> mesg)
> {
> struct at91_udc *udc = platform_get_drvdata(pdev);
> diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
> linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c
> --- linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
> 2007-08-08 13:28:27.000000000 -0400
> +++ linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c 2007-08-08
> 13:32:45.000000000 -0400
> @@ -282,6 +282,7 @@ static int ohci_hcd_at91_drv_remove(stru
> }
>
> #ifdef CONFIG_PM
> +extern int at91_suspend_entering_slow_clock(void);
>
> static int
> ohci_hcd_at91_drv_suspend(struct platform_device *pdev, pm_message_t
> mesg)
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hello!
On Thursday 09 August 2007 16:53, Hans-J?rgen Koch wrote:
> (added linux-arm-kernel to CC)
>
> Am Donnerstag 09 August 2007 14:10 schrieb Andy Herzig:
> > Hello all,
> >
> > This patch is intended to fix compilation errors when I tried to add in
> > power management to my Atmel AT91SAM9260 board. First, there is no
> > separate memory controller in the 9260 as there is in the 9200, so calls
> > to do anything with it in pm.c fail.
>
> Correct. I made the same discovery with my AT91SAM9263 board.
>
> >
> > Secondly, at91_suspend_entering_slow_clock() is not declared extern by
> > drivers that use it. This used to cause only a warning, but in
> > 2.6.23-rc1 and beyond, it is an error.
>
> Patch looks good to me. Kernel compiles for my 9263. Didn't test
> suspend/resume yet, though.
>
> >
> > Signed-off-by: Andrew J. Herzig <[email protected]>
>
> Acked-by: Hans J. Koch <[email protected]>
>
>
> > ---
> > arch/arm/mach-at91/pm.c | 5 ++++-
> > drivers/serial/atmel_serial.c | 2 ++
> > drivers/usb/gadget/at91_udc.c | 2 ++
> > drivers/usb/host/ohci-at91.c | 1 +
> > 4 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> > linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c
> > linux-2.6.23-rc2/arch/arm/mach-at91/pm.c
> > --- linux-2.6.23-rc2-vanilla/arch/arm/mach-at91/pm.c 2007-08-08
> > 13:28:37.000000000 -0400
> > +++ linux-2.6.23-rc2/arch/arm/mach-at91/pm.c 2007-08-08
> > 13:36:43.000000000 -0400
> > @@ -176,7 +176,9 @@ static int at91_pm_enter(suspend_state_t
> > */
> > asm("b 1f; .align 5; 1:");
> > asm("mcr p15, 0, r0, c7, c10, 4"); /* drain
> > write buffer */
> > +#if defined(CONFIG_ARCH_AT91RM9200)
> > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > self-refresh mode */
Why don't use:
if (cpu_is_at91rm9200())
at91_sys_write(AT91_SDRAMC_SRR, 1);
> > +#endif
> > /* fall though to next state */
> >
> > case PM_SUSPEND_ON:
> > @@ -218,8 +220,9 @@ static int __init at91_pm_init(void)
> > #endif
> >
> > /* Disable SDRAM low-power mode. Cannot be used with
> > self-refresh. */
> > +#if defined(CONFIG_ARCH_AT91RM9200)
> > at91_sys_write(AT91_SDRAMC_LPR, 0);
> > -
> > +#endif
same comment
...
> > --- linux-2.6.23-rc2-vanilla/drivers/serial/atmel_serial.c
> > 2007-08-08 13:28:33.000000000 -0400
> > +++ linux-2.6.23-rc2/drivers/serial/atmel_serial.c 2007-08-08
> > 13:32:52.000000000 -0400
> > @@ -916,6 +916,8 @@ static struct uart_driver atmel_uart = {
> > };
> >
> > #ifdef CONFIG_PM
> > +extern int at91_suspend_entering_slow_clock(void);
> > +
> > static int atmel_serial_suspend(struct platform_device *pdev,
> > pm_message_t state)
> > {
> > struct uart_port *port = platform_get_drvdata(pdev);
> > diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> > linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
> > linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c
> > --- linux-2.6.23-rc2-vanilla/drivers/usb/gadget/at91_udc.c
> > 2007-08-08 13:28:27.000000000 -0400
> > +++ linux-2.6.23-rc2/drivers/usb/gadget/at91_udc.c 2007-08-08
> > 13:33:21.000000000 -0400
> > @@ -1770,6 +1770,8 @@ static int __exit at91udc_remove(struct
> > }
> >
> > #ifdef CONFIG_PM
> > +extern int at91_suspend_entering_slow_clock(void);
> > +
> > static int at91udc_suspend(struct platform_device *pdev, pm_message_t
> > mesg)
> > {
> > struct at91_udc *udc = platform_get_drvdata(pdev);
> > diff -uprN -X linux-2.6.23-rc2/Documentation/dontdiff
> > linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
> > linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c
> > --- linux-2.6.23-rc2-vanilla/drivers/usb/host/ohci-at91.c
> > 2007-08-08 13:28:27.000000000 -0400
> > +++ linux-2.6.23-rc2/drivers/usb/host/ohci-at91.c 2007-08-08
> > 13:32:45.000000000 -0400
> > @@ -282,6 +282,7 @@ static int ohci_hcd_at91_drv_remove(stru
> > }
> >
> > #ifdef CONFIG_PM
> > +extern int at91_suspend_entering_slow_clock(void);
Why do you define 3 times this function?
Can't you place it in a .h?
Regards
Marc
On Thu, 2007-08-09 at 18:01 +0200, Marc Pignat wrote:
> > > +#if defined(CONFIG_ARCH_AT91RM9200)
> > > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > > self-refresh mode */
> Why don't use:
> if (cpu_is_at91rm9200())
> at91_sys_write(AT91_SDRAMC_SRR, 1);
>
No reason. That looks good to me.
> > > #ifdef CONFIG_PM
> > > +extern int at91_suspend_entering_slow_clock(void);
> Why do you define 3 times this function?
> Can't you place it in a .h?
I considered a .h, but I didn't see one that all drivers included. I
declare it in the 3 separate driver .c files that I saw needed it. There
is no pm.h and I didn't see the point in creating one just for that.
That of course will work if everyone likes that better. In either case,
you still have to add something to all three files.
-Andy H.
Am Donnerstag 09 August 2007 19:17 schrieb Andy Herzig:
> On Thu, 2007-08-09 at 18:01 +0200, Marc Pignat wrote:
>
> > > > +#if defined(CONFIG_ARCH_AT91RM9200)
> > > > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > > > self-refresh mode */
> > Why don't use:
> > if (cpu_is_at91rm9200())
> > at91_sys_write(AT91_SDRAMC_SRR, 1);
> >
> No reason. That looks good to me.
No, it doesn't look good. It won't compile.
Hans
> > +#if defined(CONFIG_ARCH_AT91RM9200)
> > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > self-refresh mode */
> Why don't use:
> if (cpu_is_at91rm9200())
> at91_sys_write(AT91_SDRAMC_SRR, 1);
What is the benefit?
Will the optimizer remove the code if the CPU is not the at91rm9200?
Best Regards
Ulf Samuelsson
Am Freitag 10 August 2007 00:15 schrieb Ulf Samuelsson:
>
> > > +#if defined(CONFIG_ARCH_AT91RM9200)
> > > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > > self-refresh mode */
>
> > Why don't use:
> > if (cpu_is_at91rm9200())
> > at91_sys_write(AT91_SDRAMC_SRR, 1);
>
> What is the benefit?
> Will the optimizer remove the code if the CPU is not the at91rm9200?
No, it won't. cpu_is_something() is intended for runtime decisions.
Remember that the purpose of this patch was to solve a compile time
issue (see subject). AT91_SDRAMC_SRR isn't defined properly for
non-9200 processors because they don't have that register. So we need
something like #ifdef to include this line only on 9200.
Hans
On Friday 10 August 2007 09:12, Hans-J?rgen Koch wrote:
> Am Freitag 10 August 2007 00:15 schrieb Ulf Samuelsson:
> >
> > > > +#if defined(CONFIG_ARCH_AT91RM9200)
> > > > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > > > self-refresh mode */
> >
> > > Why don't use:
> > > if (cpu_is_at91rm9200())
> > > at91_sys_write(AT91_SDRAMC_SRR, 1);
> >
> > What is the benefit?
More readable. (see '#ifdefs are ugly' in Documentation/SubmittingPatches)
> > Will the optimizer remove the code if the CPU is not the at91rm9200?
Optimizer will remove that code if at91rm9200 support is not compiled and
choose at runtime if the cpu support is compiled in.
>
> No, it won't. cpu_is_something() is intended for runtime decisions.
> Remember that the purpose of this patch was to solve a compile time
> issue (see subject). AT91_SDRAMC_SRR isn't defined properly for
> non-9200 processors because they don't have that register. So we need
> something like #ifdef to include this line only on 9200.
Oops, I missed that problem, sorry...
and what about this:
#ifdef CONFIG_ARCH_AT91RM9200
#define sdram_lowpower_enable() at91_sys_write(AT91_SDRAMC_SRR, 1)
#define sdram_lowpower_disable() at91_sys_write(AT91_SDRAMC_SRR, 0)
#else
#define sdram_lowpower_enable()
#define sdram_lowpower_disable()
#endif
and using sdram_lowpower_{enable,disable}() when requiered?
Regards
Marc
Hi Pierre,
I'm also using power management on the at91sam9260.
> About SDRAm self refresh mode on at91sam926x, in fact AT91_SDRAMC_SRR
> doesn't exist but the new register is AT91_SDRAMC_LPR. So for instance, you
> can replace at91_sys_write(AT91_SDRAMC_SRR, 1) by
> at91_sys_write(AT91_SDRAMC_LPR, AT91_SDRAMC_LPCB_SELF_REFRESH).
I have also made that change in pm.c. I've tested it with 2.6.20.4
and it seems to work.
> Currently I work on the power management on at91sam9260 so when I will
> finish my work, I could give you my files.
Please post your mods to the list when you are finished. I will
gladly help you test them.
Are you going to implement slow clock standby mode?
Michel
----- Original Message -----
From: "Marc Pignat" <[email protected]>
To: "Hans-J?rgen Koch" <[email protected]>
Cc: <[email protected]>; "Ulf Samuelsson" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; "Andy Herzig" <[email protected]>
Sent: Friday, August 10, 2007 9:33 AM
Subject: Re: [PATCH] at91 pm: Compilation fix for at91sam926x
On Friday 10 August 2007 09:12, Hans-J?rgen Koch wrote:
> Am Freitag 10 August 2007 00:15 schrieb Ulf Samuelsson:
> >
> > > > +#if defined(CONFIG_ARCH_AT91RM9200)
> > > > at91_sys_write(AT91_SDRAMC_SRR, 1); /*
> > > > self-refresh mode */
> >
> > > Why don't use:
> > > if (cpu_is_at91rm9200())
> > > at91_sys_write(AT91_SDRAMC_SRR, 1);
> >
> > What is the benefit?
More readable. (see '#ifdefs are ugly' in Documentation/SubmittingPatches)
> > Will the optimizer remove the code if the CPU is not the at91rm9200?
Optimizer will remove that code if at91rm9200 support is not compiled and
choose at runtime if the cpu support is compiled in.
>
> No, it won't. cpu_is_something() is intended for runtime decisions.
> Remember that the purpose of this patch was to solve a compile time
> issue (see subject). AT91_SDRAMC_SRR isn't defined properly for
> non-9200 processors because they don't have that register. So we need
> something like #ifdef to include this line only on 9200.
Oops, I missed that problem, sorry...
and what about this:
#ifdef CONFIG_ARCH_AT91RM9200
#define sdram_lowpower_enable() at91_sys_write(AT91_SDRAMC_SRR, 1)
#define sdram_lowpower_disable() at91_sys_write(AT91_SDRAMC_SRR, 0)
#else
#define sdram_lowpower_enable()
#define sdram_lowpower_disable()
#endif
and using sdram_lowpower_{enable,disable}() when requiered?
==> That is hiding the fact that the low power is not performed
when it it not an AT91RM9200.
I think the original approach is the best.
Best Regards
Ulf Samuelsson
hi,
> > About SDRAm self refresh mode on at91sam926x, in fact AT91_SDRAMC_SRR
> > doesn't exist but the new register is AT91_SDRAMC_LPR. So for instance, you
> > can replace at91_sys_write(AT91_SDRAMC_SRR, 1) by
> > at91_sys_write(AT91_SDRAMC_LPR, AT91_SDRAMC_LPCB_SELF_REFRESH).
>
> I have also made that change in pm.c.
I think we will go with the following change for now.
We need to pull in different header files anyway.
(This file will need to be re-looked at if we ever allow compiling a
single kernel image that supports multiple AT91 processors)
--- pm.c 6 Jul 2007 09:18:01 -0000 1.5
+++ pm.c 24 Aug 2007 07:22:13 -0000
@@ -27,12 +27,24 @@
#include <asm/mach-types.h>
#include <asm/arch/at91_pmc.h>
-#include <asm/arch/at91rm9200_mc.h>
#include <asm/arch/gpio.h>
#include <asm/arch/cpu.h>
#include "generic.h"
+#ifdef CONFIG_ARCH_AT91RM9200
+#include <asm/arch/at91rm9200_mc.h>
+
+#define sdram_selfrefresh_enable() at91_sys_write(AT91_SDRAMC_SRR, 1)
+#define sdram_selfrefresh_disable()
+
+#else
+#include <asm/arch/at91sam926x_mc.h>
+
+#define sdram_selfrefresh_enable() at91_sys_write(AT91_SDRAMC_LPR, AT91_SDRAMC_LPCB_SELF_REFRESH)
+#define sdram_selfrefresh_disable() at91_sys_write(AT91_SDRAMC_LPR, AT91_SDRAMC_LPCB_DISABLE)
+
+#endif
static int at91_pm_valid_state(suspend_state_t state)
{
@@ -172,7 +184,7 @@
*/
asm("b 1f; .align 5; 1:");
asm("mcr p15, 0, r0, c7, c10, 4"); /* drain write buffer */
- at91_sys_write(AT91_SDRAMC_SRR, 1); /* self-refresh mode */
+ sdram_selfrefresh_enable(); /* self-refresh mode */
/* fall though to next state */
case PM_SUSPEND_ON:
@@ -188,6 +200,7 @@
at91_sys_read(AT91_AIC_IPR) & at91_sys_read(AT91_AIC_IMR));
error:
+ sdram_selfrefresh_disable();
target_state = PM_SUSPEND_ON;
at91_irq_resume();
at91_gpio_resume();
Regards,
Andrew Victor