[PATCH v2 0/2] Kirkwood suspend to RAM

[PATCH v2 0/2] Kirkwood suspend to RAM

[Ezequiel Garcia]

This patchset implements a very basic suspend/resume support on
Kirkwood SoC's, based on the work of Simon Guinot.

Given the CPU itself is not stopped on suspend, but just sits
waiting for interruption, this is more similar to a standby-mode,
than to a real suspend-to-RAM.

The first patch implements suspend/resume procedures in Feroceon CPU.
This implementation has been copy-pasted from the one in ARM926, since
I failed to spot any relevant differences between the CPU's.

The only change from the previous RFC/PATCH version is the removal
of clock gating. Each device driver is in charge of such clock handling.

In other words, this patchset only implements the most basic support for
suspend/resume, any additional power savings must be added to each driver
by implementing proper suspend/resume support.

Given the amount of changes queued for Kirkwood, I've based this patchset
on mevbu's for-next (git://git.infradead.org/linux-mvebu.git for-next)

Tested on Openblocks A6.

Changes from v1:

  * Removed clock gating. Each peripheral is in charge of implementing
    a proper suspend/resume path, including gateable clock handling.

Ezequiel Garcia (2):
  ARM: feroceon: Add suspend/resume operation
  ARM: kirkwood: Add basic suspend-to-RAM support

 arch/arm/Kconfig                                  |  2 +-
 arch/arm/mach-kirkwood/Makefile                   |  1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  2 +
 arch/arm/mach-kirkwood/pm.c                       | 69 +++++++++++++++++++++++
 arch/arm/mm/proc-feroceon.S                       | 26 +++++++++
 5 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-kirkwood/pm.c

-- 
1.8.1.5

[PATCH v2 1/2] ARM: feroceon: Add suspend/resume operation

[Ezequiel Garcia]

Add support for suspend/resume operations. The implemented procedures
are identical to the ones for ARM926.

Cc: Assaf Hoffman <hoffman@marvell.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/Kconfig            |  2 +-
 arch/arm/mm/proc-feroceon.S | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 98538e1..c57b437 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -2198,7 +2198,7 @@ source "kernel/power/Kconfig"
 
 config ARCH_SUSPEND_POSSIBLE
 	depends on !ARCH_S5PC100
-	depends on CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || \
+	depends on CPU_ARM920T || CPU_ARM926T || CPU_FEROCEON || CPU_SA1100 || \
 		CPU_V6 || CPU_V6K || CPU_V7 || CPU_XSC3 || CPU_XSCALE || CPU_MOHAWK
 	def_bool y
 
diff --git a/arch/arm/mm/proc-feroceon.S b/arch/arm/mm/proc-feroceon.S
index d5146b9..db79b62 100644
--- a/arch/arm/mm/proc-feroceon.S
+++ b/arch/arm/mm/proc-feroceon.S
@@ -514,6 +514,32 @@ ENTRY(cpu_feroceon_set_pte_ext)
 #endif
 	mov	pc, lr
 
+/* Suspend/resume support: taken from arch/arm/mm/proc-arm926.S */
+.globl	cpu_feroceon_suspend_size
+.equ	cpu_feroceon_suspend_size, 4 * 3
+#ifdef CONFIG_ARM_CPU_SUSPEND
+ENTRY(cpu_feroceon_do_suspend)
+	stmfd	sp!, {r4 - r6, lr}
+	mrc	p15, 0, r4, c13, c0, 0	@ PID
+	mrc	p15, 0, r5, c3, c0, 0	@ Domain ID
+	mrc	p15, 0, r6, c1, c0, 0	@ Control register
+	stmia	r0, {r4 - r6}
+	ldmfd	sp!, {r4 - r6, pc}
+ENDPROC(cpu_feroceon_do_suspend)
+
+ENTRY(cpu_feroceon_do_resume)
+	mov	ip, #0
+	mcr	p15, 0, ip, c8, c7, 0	@ invalidate I+D TLBs
+	mcr	p15, 0, ip, c7, c7, 0	@ invalidate I+D caches
+	ldmia	r0, {r4 - r6}
+	mcr	p15, 0, r4, c13, c0, 0	@ PID
+	mcr	p15, 0, r5, c3, c0, 0	@ Domain ID
+	mcr	p15, 0, r1, c2, c0, 0	@ TTB address
+	mov	r0, r6			@ control register
+	b	cpu_resume_mmu
+ENDPROC(cpu_feroceon_do_resume)
+#endif
+
 	.type	__feroceon_setup, #function
 __feroceon_setup:
 	mov	r0, #0
-- 
1.8.1.5

[PATCH v2 1/2] ARM: feroceon: Add suspend/resume operation

[Ezequiel Garcia]

Russell,

On Sat, Aug 10, 2013 at 11:27:18AM -0300, Ezequiel Garcia wrote:
> Add support for suspend/resume operations. The implemented procedures
> are identical to the ones for ARM926.
> 
> Cc: Assaf Hoffman <hoffman@marvell.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/Kconfig            |  2 +-
>  arch/arm/mm/proc-feroceon.S | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 98538e1..c57b437 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -2198,7 +2198,7 @@ source "kernel/power/Kconfig"
>  
>  config ARCH_SUSPEND_POSSIBLE
>  	depends on !ARCH_S5PC100
> -	depends on CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || \
> +	depends on CPU_ARM920T || CPU_ARM926T || CPU_FEROCEON || CPU_SA1100 || \
>  		CPU_V6 || CPU_V6K || CPU_V7 || CPU_XSC3 || CPU_XSCALE || CPU_MOHAWK
>  	def_bool y
>  
> diff --git a/arch/arm/mm/proc-feroceon.S b/arch/arm/mm/proc-feroceon.S
> index d5146b9..db79b62 100644
> --- a/arch/arm/mm/proc-feroceon.S
> +++ b/arch/arm/mm/proc-feroceon.S
> @@ -514,6 +514,32 @@ ENTRY(cpu_feroceon_set_pte_ext)
>  #endif
>  	mov	pc, lr
>  
> +/* Suspend/resume support: taken from arch/arm/mm/proc-arm926.S */
> +.globl	cpu_feroceon_suspend_size
> +.equ	cpu_feroceon_suspend_size, 4 * 3
> +#ifdef CONFIG_ARM_CPU_SUSPEND
> +ENTRY(cpu_feroceon_do_suspend)
> +	stmfd	sp!, {r4 - r6, lr}
> +	mrc	p15, 0, r4, c13, c0, 0	@ PID
> +	mrc	p15, 0, r5, c3, c0, 0	@ Domain ID
> +	mrc	p15, 0, r6, c1, c0, 0	@ Control register
> +	stmia	r0, {r4 - r6}
> +	ldmfd	sp!, {r4 - r6, pc}
> +ENDPROC(cpu_feroceon_do_suspend)
> +
> +ENTRY(cpu_feroceon_do_resume)
> +	mov	ip, #0
> +	mcr	p15, 0, ip, c8, c7, 0	@ invalidate I+D TLBs
> +	mcr	p15, 0, ip, c7, c7, 0	@ invalidate I+D caches
> +	ldmia	r0, {r4 - r6}
> +	mcr	p15, 0, r4, c13, c0, 0	@ PID
> +	mcr	p15, 0, r5, c3, c0, 0	@ Domain ID
> +	mcr	p15, 0, r1, c2, c0, 0	@ TTB address
> +	mov	r0, r6			@ control register
> +	b	cpu_resume_mmu
> +ENDPROC(cpu_feroceon_do_resume)
> +#endif
> +
>  	.type	__feroceon_setup, #function
>  __feroceon_setup:
>  	mov	r0, #0
> -- 
> 1.8.1.5
> 

Any comments for this one?

If this is OK, then I'd like to add it in the tracking system
to be merged in either v3.12 or v3.13, and be able to introduce
suspend/resume later.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v2 2/2] ARM: kirkwood: Add basic suspend-to-RAM support

[Ezequiel Garcia]

Implements suspend-to-RAM support in a standby-like fashion:
when the SoC enters suspend state the memory PM units are disabled,
the DDR is set in self-refresh mode, and the CPU is set in WFI.

Signed-off-by: Simon Guinot <sguinot@lacie.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Changes from v1:

  * Removed clock gating. Each peripheral is in charge of implementing
    a proper suspend/resume path, including gateable clock handling.

 arch/arm/mach-kirkwood/Makefile                   |  1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  2 +
 arch/arm/mach-kirkwood/pm.c                       | 69 +++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100644 arch/arm/mach-kirkwood/pm.c

diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile
index 937d4e1..788db9c 100644
--- a/arch/arm/mach-kirkwood/Makefile
+++ b/arch/arm/mach-kirkwood/Makefile
@@ -1,4 +1,5 @@
 obj-y				+= common.o pcie.o
+obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_KIRKWOOD_LEGACY)	+= irq.o mpp.o
 obj-$(CONFIG_MACH_D2NET_V2)		+= d2net_v2-setup.o lacie_v2-common.o
 obj-$(CONFIG_MACH_NET2BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 91242c9..8b9d1c9 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -78,4 +78,6 @@
 #define CGC_TDM			(1 << 20)
 #define CGC_RESERVED		(0x6 << 21)
 
+#define MEMORY_PM_CTRL		(BRIDGE_VIRT_BASE + 0x118)
+
 #endif
diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c
new file mode 100644
index 0000000..364734c
--- /dev/null
+++ b/arch/arm/mach-kirkwood/pm.c
@@ -0,0 +1,69 @@
+/*
+ * Power Management driver for Marvell Kirkwood SoCs
+ *
+ * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
+ * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License,
+ * version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/suspend.h>
+#include <linux/io.h>
+#include <mach/bridge-regs.h>
+
+static void __iomem *ddr_operation_base;
+
+static void kirkwood_suspend_mem(void)
+{
+	u32 mem_pm_ctrl;
+
+	mem_pm_ctrl = readl(MEMORY_PM_CTRL);
+
+	/* Set peripherals to low-power mode */
+	writel_relaxed(~0, MEMORY_PM_CTRL);
+
+	/* Set DDR in self-refresh */
+	writel_relaxed(0x7, ddr_operation_base);
+
+	/*
+	 * Set CPU in wait-for-interrupt state.
+	 * This disables the CPU core clocks,
+	 * the array clocks, and also the L2 controller.
+	 */
+	cpu_do_idle();
+
+	writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL);
+}
+
+static int kirkwood_suspend_enter(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_MEM:
+		kirkwood_suspend_mem();
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static const struct platform_suspend_ops kirkwood_suspend_ops = {
+	.enter = kirkwood_suspend_enter,
+	.valid = suspend_valid_only_mem,
+};
+
+static int __init kirkwood_pm_init(void)
+{
+	ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4);
+	suspend_set_ops(&kirkwood_suspend_ops);
+	return 0;
+}
+arch_initcall(kirkwood_pm_init);
-- 
1.8.1.5

[PATCH v2 2/2] ARM: kirkwood: Add basic suspend-to-RAM support

[Andrew Lunn]

On Sat, Aug 10, 2013 at 11:27:19AM -0300, Ezequiel Garcia wrote:
> Implements suspend-to-RAM support in a standby-like fashion:
> when the SoC enters suspend state the memory PM units are disabled,
> the DDR is set in self-refresh mode, and the CPU is set in WFI.
> 
> Signed-off-by: Simon Guinot <sguinot@lacie.com>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> Changes from v1:
> 
>   * Removed clock gating. Each peripheral is in charge of implementing
>     a proper suspend/resume path, including gateable clock handling.
> 
>  arch/arm/mach-kirkwood/Makefile                   |  1 +
>  arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  2 +
>  arch/arm/mach-kirkwood/pm.c                       | 69 +++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
>  create mode 100644 arch/arm/mach-kirkwood/pm.c
> 
> diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile
> index 937d4e1..788db9c 100644
> --- a/arch/arm/mach-kirkwood/Makefile
> +++ b/arch/arm/mach-kirkwood/Makefile
> @@ -1,4 +1,5 @@
>  obj-y				+= common.o pcie.o
> +obj-$(CONFIG_PM)		+= pm.o
>  obj-$(CONFIG_KIRKWOOD_LEGACY)	+= irq.o mpp.o
>  obj-$(CONFIG_MACH_D2NET_V2)		+= d2net_v2-setup.o lacie_v2-common.o
>  obj-$(CONFIG_MACH_NET2BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
> diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> index 91242c9..8b9d1c9 100644
> --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> @@ -78,4 +78,6 @@
>  #define CGC_TDM			(1 << 20)
>  #define CGC_RESERVED		(0x6 << 21)
>  
> +#define MEMORY_PM_CTRL		(BRIDGE_VIRT_BASE + 0x118)
> +
>  #endif
> diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c
> new file mode 100644
> index 0000000..364734c
> --- /dev/null
> +++ b/arch/arm/mach-kirkwood/pm.c
> @@ -0,0 +1,69 @@
> +/*
> + * Power Management driver for Marvell Kirkwood SoCs
> + *
> + * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> + * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License,
> + * version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/suspend.h>
> +#include <linux/io.h>
> +#include <mach/bridge-regs.h>
> +
> +static void __iomem *ddr_operation_base;
> +
> +static void kirkwood_suspend_mem(void)
> +{
> +	u32 mem_pm_ctrl;
> +
> +	mem_pm_ctrl = readl(MEMORY_PM_CTRL);
> +
> +	/* Set peripherals to low-power mode */
> +	writel_relaxed(~0, MEMORY_PM_CTRL);
> +
> +	/* Set DDR in self-refresh */
> +	writel_relaxed(0x7, ddr_operation_base);
> +
> +	/*
> +	 * Set CPU in wait-for-interrupt state.
> +	 * This disables the CPU core clocks,
> +	 * the array clocks, and also the L2 controller.
> +	 */
> +	cpu_do_idle();
> +
> +	writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL);
> +}
> +
> +static int kirkwood_suspend_enter(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		kirkwood_suspend_mem();
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static const struct platform_suspend_ops kirkwood_suspend_ops = {
> +	.enter = kirkwood_suspend_enter,
> +	.valid = suspend_valid_only_mem,
> +};
> +
> +static int __init kirkwood_pm_init(void)
> +{
> +	ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4);
> +	suspend_set_ops(&kirkwood_suspend_ops);
> +	return 0;
> +}
> +arch_initcall(kirkwood_pm_init);

Hi Ezequiel

Does this work on a multi arch kernel? Should kirkwood_pm_init() be
checking it is actually running on a kirkwood? Or call
kirkwood_pm_init() from kirkwood_dt_init()?

Another issue is that the ddr_operation_base should probably be
accessed using your atomic_io_set_clear() function, since it is used
by the cpuidle drivers as well.

	   Andrew

[PATCH v2 2/2] ARM: kirkwood: Add basic suspend-to-RAM support

[Ezequiel Garcia]

On Sat, Aug 10, 2013 at 05:01:15PM +0200, Andrew Lunn wrote:
> On Sat, Aug 10, 2013 at 11:27:19AM -0300, Ezequiel Garcia wrote:
> > Implements suspend-to-RAM support in a standby-like fashion:
> > when the SoC enters suspend state the memory PM units are disabled,
> > the DDR is set in self-refresh mode, and the CPU is set in WFI.
> > 
> > Signed-off-by: Simon Guinot <sguinot@lacie.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> > Changes from v1:
> > 
> >   * Removed clock gating. Each peripheral is in charge of implementing
> >     a proper suspend/resume path, including gateable clock handling.
> > 
> >  arch/arm/mach-kirkwood/Makefile                   |  1 +
> >  arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  2 +
> >  arch/arm/mach-kirkwood/pm.c                       | 69 +++++++++++++++++++++++
> >  3 files changed, 72 insertions(+)
> >  create mode 100644 arch/arm/mach-kirkwood/pm.c
> > 
> > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile
> > index 937d4e1..788db9c 100644
> > --- a/arch/arm/mach-kirkwood/Makefile
> > +++ b/arch/arm/mach-kirkwood/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-y				+= common.o pcie.o
> > +obj-$(CONFIG_PM)		+= pm.o
> >  obj-$(CONFIG_KIRKWOOD_LEGACY)	+= irq.o mpp.o
> >  obj-$(CONFIG_MACH_D2NET_V2)		+= d2net_v2-setup.o lacie_v2-common.o
> >  obj-$(CONFIG_MACH_NET2BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
> > diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > index 91242c9..8b9d1c9 100644
> > --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> > @@ -78,4 +78,6 @@
> >  #define CGC_TDM			(1 << 20)
> >  #define CGC_RESERVED		(0x6 << 21)
> >  
> > +#define MEMORY_PM_CTRL		(BRIDGE_VIRT_BASE + 0x118)
> > +
> >  #endif
> > diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c
> > new file mode 100644
> > index 0000000..364734c
> > --- /dev/null
> > +++ b/arch/arm/mach-kirkwood/pm.c
> > @@ -0,0 +1,69 @@
> > +/*
> > + * Power Management driver for Marvell Kirkwood SoCs
> > + *
> > + * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > + * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License,
> > + * version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/suspend.h>
> > +#include <linux/io.h>
> > +#include <mach/bridge-regs.h>
> > +
> > +static void __iomem *ddr_operation_base;
> > +
> > +static void kirkwood_suspend_mem(void)
> > +{
> > +	u32 mem_pm_ctrl;
> > +
> > +	mem_pm_ctrl = readl(MEMORY_PM_CTRL);
> > +
> > +	/* Set peripherals to low-power mode */
> > +	writel_relaxed(~0, MEMORY_PM_CTRL);
> > +
> > +	/* Set DDR in self-refresh */
> > +	writel_relaxed(0x7, ddr_operation_base);
> > +
> > +	/*
> > +	 * Set CPU in wait-for-interrupt state.
> > +	 * This disables the CPU core clocks,
> > +	 * the array clocks, and also the L2 controller.
> > +	 */
> > +	cpu_do_idle();
> > +
> > +	writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL);
> > +}
> > +
> > +static int kirkwood_suspend_enter(suspend_state_t state)
> > +{
> > +	switch (state) {
> > +	case PM_SUSPEND_MEM:
> > +		kirkwood_suspend_mem();
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct platform_suspend_ops kirkwood_suspend_ops = {
> > +	.enter = kirkwood_suspend_enter,
> > +	.valid = suspend_valid_only_mem,
> > +};
> > +
> > +static int __init kirkwood_pm_init(void)
> > +{
> > +	ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4);
> > +	suspend_set_ops(&kirkwood_suspend_ops);
> > +	return 0;
> > +}
> > +arch_initcall(kirkwood_pm_init);
> 
> Hi Ezequiel
> 
> Does this work on a multi arch kernel? Should kirkwood_pm_init() be
> checking it is actually running on a kirkwood? Or call
> kirkwood_pm_init() from kirkwood_dt_init()?
> 

Oh, right... I think you already mentioned this. Sorry, forgot about
this completely.

On the other side, kirkwood is not multiplatform-enabled yet, right?
So there's no way this can produce any build error.

> Another issue is that the ddr_operation_base should probably be
> accessed using your atomic_io_set_clear() function, since it is used
> by the cpuidle drivers as well.
> 

I think this is not needed. Both cpuidle suspend core code disables
IRQ before entering any of the suspend or cpuidle states.

I'm not sure this is sufficient in SMP context? And in any case,
do we care about SMP, given kirkwood is UP?

BTW, do you think the approach of *not* messing with clocks is ok?

Thanks for the feedback!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v2 2/2] ARM: kirkwood: Add basic suspend-to-RAM support

[Andrew Lunn]

> > > +static int __init kirkwood_pm_init(void)
> > > +{
> > > +	ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4);
> > > +	suspend_set_ops(&kirkwood_suspend_ops);
> > > +	return 0;
> > > +}
> > > +arch_initcall(kirkwood_pm_init);
> > 
> > Hi Ezequiel
> > 
> > Does this work on a multi arch kernel? Should kirkwood_pm_init() be
> > checking it is actually running on a kirkwood? Or call
> > kirkwood_pm_init() from kirkwood_dt_init()?
> > 
> 
> Oh, right... I think you already mentioned this. Sorry, forgot about
> this completely.
> 
> On the other side, kirkwood is not multiplatform-enabled yet, right?
> So there's no way this can produce any build error.

Kirkwood is not currently multiplatform, but there is nothing i know
of which will break when we do become multi-platform. I've been
keeping an eye out for such issues, and made sure that cpuidle,
cpufreq, etc are multiplatform safe. I think Thomas removed the last
blocker for DT based boards going multiplatform when PCI moved to DT.
So i don't want to add something now, which i know will break with
multiplatform. Lets do it the right way now.

> > Another issue is that the ddr_operation_base should probably be
> > accessed using your atomic_io_set_clear() function, since it is used
> > by the cpuidle drivers as well.
> > 
> 
> I think this is not needed. Both cpuidle suspend core code disables
> IRQ before entering any of the suspend or cpuidle states.

I've no idea how PM works, so i cannot say if its safe or not...
> 
> BTW, do you think the approach of *not* messing with clocks is ok?

That is correct. The drivers should be disabling clocks, were they can
be disabled. We know kirkwood ethernet is broken and we cannot
"easily" disable its clock.

	 Andrew

[PATCH v2 2/2] ARM: kirkwood: Add basic suspend-to-RAM support

[Ezequiel Garcia]

On Sat, Aug 10, 2013 at 06:00:19PM +0200, Andrew Lunn wrote:
> > > > +static int __init kirkwood_pm_init(void)
> > > > +{
> > > > +	ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4);
> > > > +	suspend_set_ops(&kirkwood_suspend_ops);
> > > > +	return 0;
> > > > +}
> > > > +arch_initcall(kirkwood_pm_init);
> > > 
> > > Hi Ezequiel
> > > 
> > > Does this work on a multi arch kernel? Should kirkwood_pm_init() be
> > > checking it is actually running on a kirkwood? Or call
> > > kirkwood_pm_init() from kirkwood_dt_init()?
> > > 
> > 
> > Oh, right... I think you already mentioned this. Sorry, forgot about
> > this completely.
> > 
> > On the other side, kirkwood is not multiplatform-enabled yet, right?
> > So there's no way this can produce any build error.
> 
> Kirkwood is not currently multiplatform, but there is nothing i know
> of which will break when we do become multi-platform. I've been
> keeping an eye out for such issues, and made sure that cpuidle,
> cpufreq, etc are multiplatform safe. I think Thomas removed the last
> blocker for DT based boards going multiplatform when PCI moved to DT.
> So i don't want to add something now, which i know will break with
> multiplatform. Lets do it the right way now.
> 

Of course. And I'd say it's better call kirkwood_pm_init() from some place
kirkwood-specific, rather than leaving the generic arch_initcall checking
for kirkwood machine.

It looks less bloaty, uh?

> > > Another issue is that the ddr_operation_base should probably be
> > > accessed using your atomic_io_set_clear() function, since it is used
> > > by the cpuidle drivers as well.
> > > 
> > 
> > I think this is not needed. Both cpuidle suspend core code disables
> > IRQ before entering any of the suspend or cpuidle states.
> 
> I've no idea how PM works, so i cannot say if its safe or not...

Me neither. But I'm checking the code and it's screaming "local irqs
disabled" before entering any state. Also, the ticker seems to be stopped.

I'd like to know we *really* need such register protection, given
it's not an easy task (see the atomic_io_clear_set discussion).

> > 
> > BTW, do you think the approach of *not* messing with clocks is ok?
> 
> That is correct. The drivers should be disabling clocks, were they can
> be disabled. We know kirkwood ethernet is broken and we cannot
> "easily" disable its clock.
> 

Ok, good.

One more thing I came across while reviewing the PM code.
There's a "standby" state that seems much more appropriate than
the current "mem" state.

I looks like there's not any distinction in the PM code handling
between the two "mem" and "standby" states, but it's a bit misleading
for users to declare "suspend to RAM" when it's not.

So I think we should use that instead. I'll fix all of these and prepare a v3.

Thanks for the feedback!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v2 0/2] Kirkwood suspend to RAM

[Jason Cooper]

Ezequiel,

On Sat, Aug 10, 2013 at 11:27:17AM -0300, Ezequiel Garcia wrote:
...
> Given the amount of changes queued for Kirkwood, I've based this patchset
> on mevbu's for-next (git://git.infradead.org/linux-mvebu.git for-next)

Please base this on a tag in mainline unless you _need_ a patch that
isn't there yet.

thx,

Jason.