[PATCH 00/17] Suspend to RAM support for Armada XP

[PATCH 00/17] Suspend to RAM support for Armada XP

[Thomas Petazzoni]

Hello,

This set of patches implement suspend to RAM for the Marvell Armada XP
platform, and specifically for the Armada XP GP board. For now, it
does not implement suspend/resume for all device drivers, it only
implements the core of the suspend/resume functionality: enough to
make sure that the system can enter suspend and resume from suspend,
restart all CPUs, and get back to a shell prompt. Additional work in
individual device drivers will be needed as follow-up patches.

There are two important things to understand about the hardware before
reading this patch series:

 - The Armada XP SoC cannot put itself into a suspend to RAM state. It
   requires an external circuit, which in most cases, is a PIC
   micro-controller, to turn off the SoC. This PIC is connected to the
   SoC using a set of GPIOs, which allow the SoC to talk to the PIC
   and request certain things to be done.

   This means that the Armada XP suspend/resume logic has some
   board-specific code, which is fairly uncommon for suspend/resume
   code in the ARM world.

   We have chosen to represent the PIC and the fact that it's
   connected to the SoC using 3 GPIOs in the Device
   Tree.

 - When exiting from suspend, the SoC is actually powered up again
   completely from scratch, so it goes through the bootloader. The
   kernel has to fill a bootloader-specific structure at a fixed
   physical address to pass information to the bootloader that will
   tell the bootloader to do a resume and therefore jump back directly
   into the kernel resume entry point, instead of doing a normal boot.

 - The bootloader has to reconfigure the DRAM controller, which
   involves executing some DDR3 training code that has the unfortunate
   side effect of overwriting the first 10 KB of each DRAM
   chip-select. We therefore have to ensure that the kernel does not
   use the first 10 KB of each DRAM chip select.

So, in sequence:

 * PATCH 1 to 3 are mainly preparation patches: Device Tree binding
   documentation, fixing an errata of the Armada XP, doing a minor
   improvement in the irqchip driver.

 * PATCH 4 to 9 add suspend/resume support to some core drivers:
   irqchip, clocksource, gpio, mvebu-mbus and clock.

 * PATCH 10 implements the core of the SoC suspend/resume logic.

 * PATCH 11 makes sure the first 10 KB of each DRAM chip select isn't
   used by the kernel.

 * PATCH 12 implements the board-specific suspend/resume logic,
   especially talking with the PIC micro-controller over GPIOs.

 * PATCH 13 and 14 make some other additional changes to the Armada XP
   SoC support to make suspend/resume work properly.

 * PATCH 15 to 17 modify the Armada XP and Armada XP GP Device Tree
   description to add the relevant hardware blocks that are needed for
   suspend/resume: description of the PIC micro-controller and the 3
   GPIOs connecting it to the SoC, an additional set of the register
   for mvebu-mbus, and the registers for the SDRAM controller.

Best regards,

Thomas

Nadav Haklai (1):
  ARM: mvebu: enable strex backoff delay

Thomas Petazzoni (16):
  Documentation: dt-bindings: minimal documentation for MVEBU SDRAM
    controller
  irqchip: irq-armada-370-xp: use proper return value for
    ->set_affinity()
  irqchip: irq-armada-370-xp: suspend/resume support
  clocksource: time-armada-370-xp: add suspend/resume support
  gpio: mvebu: add suspend/resume support
  bus: mvebu-mbus: suspend/resume support
  bus: mvebu-mbus: provide a mechanism to save SDRAM window
    configuration
  clk: mvebu: add suspend/resume for gatable clocks
  ARM: mvebu: implement suspend/resume support for Armada XP
  ARM: mvebu: reserve the first 10 KB of each memory bank for
    suspend/resume
  ARM: mvebu: Armada XP GP specific suspend/resume code
  ARM: mvebu: make sure MMU is disabled in armada_370_xp_cpu_resume
  ARM: mvebu: synchronize secondary CPU clocks on resume
  ARM: mvebu: add suspend/resume DT information for Armada XP GP
  ARM: mvebu: adjust mbus controller description on Armada 370/XP
  ARM: mvebu: add SDRAM controller description for Armada XP

 .../devicetree/bindings/bus/mvebu-mbus.txt         |  17 +-
 .../memory-controllers/mvebu-sdram-controller.txt  |  21 ++
 arch/arm/boot/dts/armada-370-xp.dtsi               |   3 +-
 arch/arm/boot/dts/armada-xp-gp.dts                 |  19 +-
 arch/arm/boot/dts/armada-xp.dtsi                   |   5 +
 arch/arm/mach-mvebu/Makefile                       |   2 +-
 arch/arm/mach-mvebu/board-v7.c                     |  51 +++++
 arch/arm/mach-mvebu/common.h                       |   2 +
 arch/arm/mach-mvebu/platsmp.c                      |  31 ++-
 arch/arm/mach-mvebu/pm-board.c                     | 134 +++++++++++++
 arch/arm/mach-mvebu/pm.c                           | 220 +++++++++++++++++++++
 arch/arm/mach-mvebu/pmsu.h                         |   1 +
 arch/arm/mach-mvebu/pmsu_ll.S                      |   8 +
 arch/arm/mm/proc-v7.S                              |   2 -
 drivers/bus/mvebu-mbus.c                           | 181 ++++++++++++++++-
 drivers/clk/mvebu/common.c                         |  30 ++-
 drivers/clocksource/time-armada-370-xp.c           |  25 +++
 drivers/gpio/gpio-mvebu.c                          |  99 ++++++++++
 drivers/irqchip/irq-armada-370-xp.c                |  54 ++++-
 include/linux/mbus.h                               |   1 +
 20 files changed, 871 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
 create mode 100644 arch/arm/mach-mvebu/pm-board.c
 create mode 100644 arch/arm/mach-mvebu/pm.c

-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/17] Documentation: dt-bindings: minimal documentation for MVEBU SDRAM controller

[Thomas Petazzoni]

The suspend/resume code for Armada XP has to modify certain registers
of the SDRAM controller. Therefore, we need to define a Device Tree
binding for this hardware block.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: devicetree@vger.kernel.org
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
---
 .../memory-controllers/mvebu-sdram-controller.txt   | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt

diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
new file mode 100644
index 0000000..89657d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
@@ -0,0 +1,21 @@
+Device Tree bindings for MVEBU SDRAM controllers
+
+The Marvell EBU SoCs all have a SDRAM controller. The SDRAM controller
+differs from one SoC variant to another, but they also share a number
+of commonalities.
+
+For now, this Device Tree binding documentation only documents the
+Armada XP SDRAM controller.
+
+Required properties:
+
+ - compatible: for Armada XP, "marvell,armada-xp-sdram-controller"
+ - reg: a resource specifier for the register space, which should
+   include all SDRAM controller registers as per the datasheet.
+
+Example:
+
+sdramc@1400 {
+	compatible = "marvell,armada-xp-sdram-controller";
+	reg = <0x1400 0x500>;
+};
-- 
2.0.0

Re: [PATCH 01/17] Documentation: dt-bindings: minimal documentation for MVEBU SDRAM controller

[Gregory CLEMENT]

Hi Thomas,

This binding looks coorect for me

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> The suspend/resume code for Armada XP has to modify certain registers
> of the SDRAM controller. Therefore, we need to define a Device Tree
> binding for this hardware block.

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  .../memory-controllers/mvebu-sdram-controller.txt   | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> new file mode 100644
> index 0000000..89657d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> @@ -0,0 +1,21 @@
> +Device Tree bindings for MVEBU SDRAM controllers
> +
> +The Marvell EBU SoCs all have a SDRAM controller. The SDRAM controller
> +differs from one SoC variant to another, but they also share a number
> +of commonalities.
> +
> +For now, this Device Tree binding documentation only documents the
> +Armada XP SDRAM controller.
> +
> +Required properties:
> +
> + - compatible: for Armada XP, "marvell,armada-xp-sdram-controller"
> + - reg: a resource specifier for the register space, which should
> +   include all SDRAM controller registers as per the datasheet.
> +
> +Example:
> +
> +sdramc@1400 {
> +	compatible = "marvell,armada-xp-sdram-controller";
> +	reg = <0x1400 0x500>;
> +};
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/17] ARM: mvebu: enable strex backoff delay

[Thomas Petazzoni]

From: Nadav Haklai <nadavh@marvell.com>

Under extremely rare conditions, in an MPCore node consisting of at
least 3 CPUs, two CPUs trying to perform a STREX to data on the same
shared cache line can enter a livelock situation.

This patch enables the HW mechanism that overcomes the bug. This fixes
the incorrect setup of the STREX backoff delay bit due to a wrong
description in the specification.

Note that enabling the STREX backoff delay mechanism is done by
leaving the bit *cleared*, while the bit was currently being set by
the proc-v7.S code.

[Thomas: adapt to latest mainline, slightly reword the commit log, add
stable markers.]

Cc: Russell King <linux@arm.linux.org.uk>
Cc: <stable@vger.kernel.org> # v3.8+
Fixes: de4901933f6d ("arm: mm: Add support for PJ4B cpu and init routines")
Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This patch is submitted as part of the suspend/resume work, because
the suspend/resume path is triggering this rare bug in a very
reproducible fashion.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mm/proc-v7.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index b3a9478..22ac2a6 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -270,7 +270,6 @@ __v7_pj4b_setup:
 /* Auxiliary Debug Modes Control 1 Register */
 #define PJ4B_STATIC_BP (1 << 2) /* Enable Static BP */
 #define PJ4B_INTER_PARITY (1 << 8) /* Disable Internal Parity Handling */
-#define PJ4B_BCK_OFF_STREX (1 << 5) /* Enable the back off of STREX instr */
 #define PJ4B_CLEAN_LINE (1 << 16) /* Disable data transfer for clean line */
 
 /* Auxiliary Debug Modes Control 2 Register */
@@ -293,7 +292,6 @@ __v7_pj4b_setup:
 	/* Auxiliary Debug Modes Control 1 Register */
 	mrc	p15, 1,	r0, c15, c1, 1
 	orr     r0, r0, #PJ4B_CLEAN_LINE
-	orr     r0, r0, #PJ4B_BCK_OFF_STREX
 	orr     r0, r0, #PJ4B_INTER_PARITY
 	bic	r0, r0, #PJ4B_STATIC_BP
 	mcr	p15, 1,	r0, c15, c1, 1
-- 
2.0.0

Re: [PATCH 02/17] ARM: mvebu: enable strex backoff delay

[Gregory CLEMENT]

Hi Thomas,
On 24/10/2014 13:59, Thomas Petazzoni wrote:
> From: Nadav Haklai <nadavh@marvell.com>
> 
> Under extremely rare conditions, in an MPCore node consisting of at
> least 3 CPUs, two CPUs trying to perform a STREX to data on the same
> shared cache line can enter a livelock situation.
> 
> This patch enables the HW mechanism that overcomes the bug. This fixes
> the incorrect setup of the STREX backoff delay bit due to a wrong
> description in the specification.
> 
> Note that enabling the STREX backoff delay mechanism is done by
> leaving the bit *cleared*, while the bit was currently being set by
> the proc-v7.S code.
> 
> [Thomas: adapt to latest mainline, slightly reword the commit log, add
> stable markers.]
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: <stable@vger.kernel.org> # v3.8+
> Fixes: de4901933f6d ("arm: mm: Add support for PJ4B cpu and init routines")
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> This patch is submitted as part of the suspend/resume work, because
> the suspend/resume path is triggering this rare bug in a very
> reproducible fashion.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mm/proc-v7.S | 2 --
>  1 file changed, 2 deletions(-)
> 

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 03/17] irqchip: irq-armada-370-xp: use proper return value for ->set_affinity()

[Thomas Petazzoni]

The ->set_affinity() hook of 'struct irq_chip' is supposed to return
one of IRQ_SET_MASK_OK or IRQ_SET_MASK_OK_NOCOPY. However, the code
currently simply returns 0. This patch fixes that by using
IRQ_SET_MASK_OK, which tells the IRQ core that it is responsible for
updating irq_data.affinity.

Note that this patch does not cause any change to the compiled code,
as IRQ_SET_MASK_OK has the value 0. This is therefore just a simple
cleanup.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: linux-kernel@vger.kernel.org
---
 drivers/irqchip/irq-armada-370-xp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 3e238cd..9e630f2 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -265,7 +265,7 @@ static int armada_xp_set_affinity(struct irq_data *d,
 	writel(reg, main_int_base + ARMADA_370_XP_INT_SOURCE_CTL(hwirq));
 	raw_spin_unlock(&irq_controller_lock);
 
-	return 0;
+	return IRQ_SET_MASK_OK;
 }
 #endif
 
-- 
2.0.0

Re: [PATCH 03/17] irqchip: irq-armada-370-xp: use proper return value for ->set_affinity()

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> The ->set_affinity() hook of 'struct irq_chip' is supposed to return
> one of IRQ_SET_MASK_OK or IRQ_SET_MASK_OK_NOCOPY. However, the code
> currently simply returns 0. This patch fixes that by using
> IRQ_SET_MASK_OK, which tells the IRQ core that it is responsible for
> updating irq_data.affinity.

It seems that this driver is not the only one to return 0 instead of
IRQ_SET_MASK_OK in the >set_affinity(). I found also irq-xtensa-mx.c,
irq-metag.c and irq-metag-ext.c.


> 
> Note that this patch does not cause any change to the compiled code,
> as IRQ_SET_MASK_OK has the value 0. This is therefore just a simple
> cleanup.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 03/17] irqchip: irq-armada-370-xp: use proper return value for ->set_affinity()

[Jason Cooper]

On Fri, Oct 24, 2014 at 01:59:16PM +0200, Thomas Petazzoni wrote:
> The ->set_affinity() hook of 'struct irq_chip' is supposed to return
> one of IRQ_SET_MASK_OK or IRQ_SET_MASK_OK_NOCOPY. However, the code
> currently simply returns 0. This patch fixes that by using
> IRQ_SET_MASK_OK, which tells the IRQ core that it is responsible for
> updating irq_data.affinity.
> 
> Note that this patch does not cause any change to the compiled code,
> as IRQ_SET_MASK_OK has the value 0. This is therefore just a simple
> cleanup.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to irqchip/core with Gregory's Reviewed-by.

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/17] irqchip: irq-armada-370-xp: suspend/resume support

[Thomas Petazzoni]

This commit adds suspend/resume support to the irqchip driver used on
Armada XP platforms (amongst others). It does so by adding a set of
suspend/resume syscore_ops, that will respectively save and restore
the necessary registers to ensure interrupts continue to work after
resume.

It is worth mentioning that the affinity is lost during a
suspend/resume cycle, because when a secondary CPU is brought
off-line, all interrupts that are assigned to this CPU in terms of
affinity gets re-assigned to a still running CPU. Therefore, right
before entering suspend, all interrupts are assigned to the boot CPU.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/irqchip/irq-armada-370-xp.c | 52 +++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 9e630f2..3c2b89d 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -26,6 +26,7 @@
 #include <linux/of_pci.h>
 #include <linux/irqdomain.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 #include <linux/msi.h>
 #include <asm/mach/arch.h>
 #include <asm/exception.h>
@@ -66,6 +67,7 @@
 static void __iomem *per_cpu_int_base;
 static void __iomem *main_int_base;
 static struct irq_domain *armada_370_xp_mpic_domain;
+static u32 doorbell_mask_reg;
 #ifdef CONFIG_PCI_MSI
 static struct irq_domain *armada_370_xp_msi_domain;
 static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
@@ -474,6 +476,54 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
 	} while (1);
 }
 
+static int armada_370_xp_mpic_suspend(void)
+{
+	doorbell_mask_reg = readl(per_cpu_int_base +
+				  ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+	return 0;
+}
+
+static void armada_370_xp_mpic_resume(void)
+{
+	int nirqs;
+	irq_hw_number_t irq;
+
+	/* Re-enable interrupts */
+	nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff;
+	for (irq = 0; irq < nirqs; irq++) {
+		struct irq_data *data;
+		int virq;
+
+		virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq);
+		if (virq == 0)
+			continue;
+
+		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
+			writel(irq, per_cpu_int_base +
+			       ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+		else
+			writel(irq, main_int_base +
+			       ARMADA_370_XP_INT_SET_ENABLE_OFFS);
+
+		data = irq_get_irq_data(virq);
+		if (!irqd_irq_disabled(data))
+			armada_370_xp_irq_unmask(data);
+	}
+
+	/* Reconfigure doorbells for IPIs and MSIs */
+	writel(doorbell_mask_reg,
+	       per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+	if (doorbell_mask_reg & IPI_DOORBELL_MASK)
+		writel(0, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+	if (doorbell_mask_reg & PCI_MSI_DOORBELL_MASK)
+		writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+}
+
+struct syscore_ops armada_370_xp_mpic_syscore_ops = {
+	.suspend	= armada_370_xp_mpic_suspend,
+	.resume		= armada_370_xp_mpic_resume,
+};
+
 static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 					     struct device_node *parent)
 {
@@ -530,6 +580,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 					armada_370_xp_mpic_handle_cascade_irq);
 	}
 
+	register_syscore_ops(&armada_370_xp_mpic_syscore_ops);
+
 	return 0;
 }
 
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/17] irqchip: irq-armada-370-xp: suspend/resume support

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> This commit adds suspend/resume support to the irqchip driver used on
> Armada XP platforms (amongst others). It does so by adding a set of
> suspend/resume syscore_ops, that will respectively save and restore
> the necessary registers to ensure interrupts continue to work after
> resume.
> 
> It is worth mentioning that the affinity is lost during a
> suspend/resume cycle, because when a secondary CPU is brought
> off-line, all interrupts that are assigned to this CPU in terms of
> affinity gets re-assigned to a still running CPU. Therefore, right
> before entering suspend, all interrupts are assigned to the boot CPU.

So what about /proc/irq/*/smp_affinity ?

Do this files still represent accurate information?

Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 04/17] irqchip: irq-armada-370-xp: suspend/resume support

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 03 Nov 2014 18:38:10 +0100, Gregory CLEMENT wrote:

> > It is worth mentioning that the affinity is lost during a
> > suspend/resume cycle, because when a secondary CPU is brought
> > off-line, all interrupts that are assigned to this CPU in terms of
> > affinity gets re-assigned to a still running CPU. Therefore, right
> > before entering suspend, all interrupts are assigned to the boot CPU.
> 
> So what about /proc/irq/*/smp_affinity ?
> 
> Do this files still represent accurate information?

See migrate_irqs() in arch/arm/kernel/irq.c. Basically, when you do a
suspend, the kernel hot unplugs the non-boot CPUs: in our case CPU1,
CPU2 and CPU3. The migrate_irqs() function above makes sure that the
affinity used for each interrupt continue to contain at least one
online CPU. Therefore, it modifies the affinity information across
suspend/resume.

For example, let's say you initially have an affinity of 0xf (i.e all
CPUs can receive the interrupt) and you modify it to 0x2 (i.e only CPU1
can take the interrupt).

Then, when migrate_irqs() is called when CPU1 is brought offline, it
will realize that the interrupt will no longer be affine to any CPU,
and it will re-assign the affinity to the current cpu_online_mask
(which will be 0xd, i.e all CPUs except CPU1). Therefore, when you get
out of suspend, the affinity for this interrupt will be 0xd.

Note that in this case, a message is displayed during the suspend
procedure:

	IRQ19 no longer affine to CPU1

On the other hand, if you leave the affinity to its default of 0xf, it
remains to 0xf, because there's always at least one online CPU in the
affinity (the boot CPU). Same thing if you change the affinity to 0x3
for example, since 0x3 is an affinity that contains the boot CPU (of
course, assuming the boot CPU is CPU0).

That's not a logic that is controlled in the irqchip driver, it's
entirely handled by the kernel core. I hope this explanation clarifies
the situation.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 07/17] bus: mvebu-mbus: suspend/resume support

[Thomas Petazzoni]

This commit extends the mvebu-mbus driver to provide suspend/resume
support. Since mvebu-mbus is not a platform_driver, the syscore_ops
mechanism is used to get ->suspend() and ->resume() hooks called into
the driver.

In those hooks, we save and restore the MBus windows state, to make
sure after resume all Mbus windows are properly restored. Note that
while the state of some windows could be gathered by looking again at
the Device Tree (for statically described windows), it is not the case
of dynamically described windows such as the PCIe memory and I/O
windows. Therefore, we take the simple approach of saving and
restoring the registers for all MBus windows.

In addition, the commit extends the Device Tree binding of the MBus
controller, to control the MBus bridge registers (which define which
parts of the physical address space is routed to MBus windows
vs. normal RAM memory). Those registers must be saved and restored
during suspend/resume. The Device Tree binding extension is made is a
backward compatible fashion, but of course, suspend/resume will not
work without the Device Tree update.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/bus/mvebu-mbus.txt         |  17 +--
 drivers/bus/mvebu-mbus.c                           | 125 ++++++++++++++++++++-
 2 files changed, 131 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/bus/mvebu-mbus.txt b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
index 5fa44f5..5e16c3c 100644
--- a/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
+++ b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
@@ -48,9 +48,12 @@ Required properties:
 - compatible:	Should be set to "marvell,mbus-controller".
 
 - reg:          Device's register space.
-		Two entries are expected (see the examples below):
-		the first one controls the devices decoding window and
-		the second one controls the SDRAM decoding window.
+		Two or three entries are expected (see the examples below):
+		the first one controls the devices decoding window,
+		the second one controls the SDRAM decoding window and
+		the third controls the MBus bridge (only with the
+		marvell,armada370-mbus and marvell,armadaxp-mbus
+		compatible strings)
 
 Example:
 
@@ -67,7 +70,7 @@ Example:
 
 			mbusc: mbus-controller@20000 {
 				compatible = "marvell,mbus-controller";
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>, <0x20250 0x8>;
 			};
 
 			/* more children ...*/
@@ -126,7 +129,7 @@ are skipped.
 
 			mbusc: mbus-controller@20000 {
 				compatible = "marvell,mbus-controller";
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>, <0x20250 0x8>;
 			};
 
 			/* more children ...*/
@@ -170,7 +173,7 @@ Using this macro, the above example would be:
 
 			mbusc: mbus-controller@20000 {
 				compatible = "marvell,mbus-controller";
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>, <0x20250 0x8>;
 			};
 
 			/* other children */
@@ -266,7 +269,7 @@ See the example below, where a more complete device tree is shown:
 			ranges = <0 MBUS_ID(0xf0, 0x01) 0 0x100000>;
 
 			mbusc: mbus-controller@20000 {
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>, <0x20250 0x8>;
 			};
 
 			interrupt-controller@20000 {
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 26c3779d..25b5b35 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -57,6 +57,7 @@
 #include <linux/of_address.h>
 #include <linux/debugfs.h>
 #include <linux/log2.h>
+#include <linux/syscore_ops.h>
 
 /*
  * DDR target is the same on all platforms.
@@ -94,20 +95,39 @@
 
 #define DOVE_DDR_BASE_CS_OFF(n) ((n) << 4)
 
+/* Relative to mbusbridge_base */
+#define MBUS_BRIDGE_CTRL_OFF	0x0
+#define MBUS_BRIDGE_BASE_OFF	0x4
+
+/* Maximum number of windows, for all known platforms */
+#define MBUS_WINS_MAX           20
+
 struct mvebu_mbus_state;
 
 struct mvebu_mbus_soc_data {
 	unsigned int num_wins;
 	unsigned int num_remappable_wins;
+	bool has_mbus_bridge;
 	unsigned int (*win_cfg_offset)(const int win);
 	void (*setup_cpu_target)(struct mvebu_mbus_state *s);
 	int (*show_cpu_target)(struct mvebu_mbus_state *s,
 			       struct seq_file *seq, void *v);
 };
 
+/*
+ * Used to store the state of one MBus window accross suspend/resume.
+ */
+struct mvebu_mbus_win_data {
+	u32 ctrl;
+	u32 base;
+	u32 remap_lo;
+	u32 remap_hi;
+};
+
 struct mvebu_mbus_state {
 	void __iomem *mbuswins_base;
 	void __iomem *sdramwins_base;
+	void __iomem *mbusbridge_base;
 	struct dentry *debugfs_root;
 	struct dentry *debugfs_sdram;
 	struct dentry *debugfs_devs;
@@ -115,6 +135,11 @@ struct mvebu_mbus_state {
 	struct resource pcie_io_aperture;
 	const struct mvebu_mbus_soc_data *soc;
 	int hw_io_coherency;
+
+	/* Used during suspend/resume */
+	u32 mbus_bridge_ctrl;
+	u32 mbus_bridge_base;
+	struct mvebu_mbus_win_data wins[MBUS_WINS_MAX];
 };
 
 static struct mvebu_mbus_state mbus_state;
@@ -549,6 +574,7 @@ mvebu_mbus_dove_setup_cpu_target(struct mvebu_mbus_state *mbus)
 static const struct mvebu_mbus_soc_data armada_370_xp_mbus_data = {
 	.num_wins            = 20,
 	.num_remappable_wins = 8,
+	.has_mbus_bridge     = true,
 	.win_cfg_offset      = armada_370_xp_mbus_win_offset,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
@@ -698,11 +724,74 @@ static __init int mvebu_mbus_debugfs_init(void)
 }
 fs_initcall(mvebu_mbus_debugfs_init);
 
+static int mvebu_mbus_suspend(void)
+{
+	struct mvebu_mbus_state *s = &mbus_state;
+	int win;
+
+	for (win = 0; win < s->soc->num_wins; win++) {
+		void __iomem *addr = s->mbuswins_base +
+			s->soc->win_cfg_offset(win);
+
+		s->wins[win].base = readl(addr + WIN_BASE_OFF);
+		s->wins[win].ctrl = readl(addr + WIN_CTRL_OFF);
+
+		if (win >= s->soc->num_remappable_wins)
+			continue;
+
+		s->wins[win].remap_lo = readl(addr + WIN_REMAP_LO_OFF);
+		s->wins[win].remap_hi = readl(addr + WIN_REMAP_HI_OFF);
+	}
+
+	if (s->mbusbridge_base) {
+		s->mbus_bridge_ctrl = readl(s->mbusbridge_base +
+					    MBUS_BRIDGE_CTRL_OFF);
+		s->mbus_bridge_base = readl(s->mbusbridge_base +
+					    MBUS_BRIDGE_BASE_OFF);
+	}
+
+	return 0;
+}
+
+static void mvebu_mbus_resume(void)
+{
+	struct mvebu_mbus_state *s = &mbus_state;
+	int win;
+
+	if (s->mbusbridge_base) {
+		writel(s->mbus_bridge_ctrl,
+		       s->mbusbridge_base + MBUS_BRIDGE_CTRL_OFF);
+		writel(s->mbus_bridge_base,
+		       s->mbusbridge_base + MBUS_BRIDGE_BASE_OFF);
+	}
+
+	for (win = 0; win < s->soc->num_wins; win++) {
+		void __iomem *addr = s->mbuswins_base +
+			s->soc->win_cfg_offset(win);
+
+		writel(s->wins[win].base, addr + WIN_BASE_OFF);
+		writel(s->wins[win].ctrl, addr + WIN_CTRL_OFF);
+
+		if (win >= s->soc->num_remappable_wins)
+			continue;
+
+		writel(s->wins[win].remap_lo, addr + WIN_REMAP_LO_OFF);
+		writel(s->wins[win].remap_hi, addr + WIN_REMAP_HI_OFF);
+	}
+}
+
+struct syscore_ops mvebu_mbus_syscore_ops = {
+	.suspend	= mvebu_mbus_suspend,
+	.resume		= mvebu_mbus_resume,
+};
+
 static int __init mvebu_mbus_common_init(struct mvebu_mbus_state *mbus,
 					 phys_addr_t mbuswins_phys_base,
 					 size_t mbuswins_size,
 					 phys_addr_t sdramwins_phys_base,
-					 size_t sdramwins_size)
+					 size_t sdramwins_size,
+					 phys_addr_t mbusbridge_phys_base,
+					 size_t mbusbridge_size)
 {
 	int win;
 
@@ -716,11 +805,24 @@ static int __init mvebu_mbus_common_init(struct mvebu_mbus_state *mbus,
 		return -ENOMEM;
 	}
 
+	if (mbusbridge_phys_base) {
+		mbus->mbusbridge_base = ioremap(mbusbridge_phys_base,
+						mbusbridge_size);
+		if (!mbus->mbusbridge_base) {
+			iounmap(mbus->sdramwins_base);
+			iounmap(mbus->mbuswins_base);
+			return -ENOMEM;
+		}
+	} else
+		mbus->mbusbridge_base = NULL;
+
 	for (win = 0; win < mbus->soc->num_wins; win++)
 		mvebu_mbus_disable_window(mbus, win);
 
 	mbus->soc->setup_cpu_target(mbus);
 
+	register_syscore_ops(&mvebu_mbus_syscore_ops);
+
 	return 0;
 }
 
@@ -746,7 +848,7 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 			mbuswins_phys_base,
 			mbuswins_size,
 			sdramwins_phys_base,
-			sdramwins_size);
+			sdramwins_size, 0, 0);
 }
 
 #ifdef CONFIG_OF
@@ -887,7 +989,7 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np,
 
 int __init mvebu_mbus_dt_init(bool is_coherent)
 {
-	struct resource mbuswins_res, sdramwins_res;
+	struct resource mbuswins_res, sdramwins_res, mbusbridge_res;
 	struct device_node *np, *controller;
 	const struct of_device_id *of_id;
 	const __be32 *prop;
@@ -923,6 +1025,19 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
 		return -EINVAL;
 	}
 
+	/*
+	 * Set the resource to 0 so that it can be left unmapped by
+	 * mvebu_mbus_common_init() if the DT doesn't carry the
+	 * necessary information. This is needed to preserve backward
+	 * compatibility.
+	 */
+	memset(&mbusbridge_res, 0, sizeof(mbusbridge_res));
+
+	if (mbus_state.soc->has_mbus_bridge) {
+		if (of_address_to_resource(controller, 2, &mbusbridge_res))
+			pr_warn(FW_WARN "deprecated mbus-mvebu Device Tree, suspend/resume will not work\n");
+	}
+
 	mbus_state.hw_io_coherency = is_coherent;
 
 	/* Get optional pcie-{mem,io}-aperture properties */
@@ -933,7 +1048,9 @@ int __init mvebu_mbus_dt_init(bool is_coherent)
 				     mbuswins_res.start,
 				     resource_size(&mbuswins_res),
 				     sdramwins_res.start,
-				     resource_size(&sdramwins_res));
+				     resource_size(&sdramwins_res),
+				     mbusbridge_res.start,
+				     resource_size(&mbusbridge_res));
 	if (ret)
 		return ret;
 
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/17] bus: mvebu-mbus: suspend/resume support

[Gregory CLEMENT]

Hi Thomas,

[...]

>  struct mvebu_mbus_state {
>  	void __iomem *mbuswins_base;
>  	void __iomem *sdramwins_base;
> +	void __iomem *mbusbridge_base;

Here you store the information about the availability of the register needed
for suspend/resume ...

[...]


> +static int mvebu_mbus_suspend(void)
> +{
> +	struct mvebu_mbus_state *s = &mbus_state;
> +	int win;
> +
> +	for (win = 0; win < s->soc->num_wins; win++) {
> +		void __iomem *addr = s->mbuswins_base +
> +			s->soc->win_cfg_offset(win);
> +
> +		s->wins[win].base = readl(addr + WIN_BASE_OFF);
> +		s->wins[win].ctrl = readl(addr + WIN_CTRL_OFF);
> +
> +		if (win >= s->soc->num_remappable_wins)
> +			continue;
> +
> +		s->wins[win].remap_lo = readl(addr + WIN_REMAP_LO_OFF);
> +		s->wins[win].remap_hi = readl(addr + WIN_REMAP_HI_OFF);
> +	}
> +
> +	if (s->mbusbridge_base) {
> +		s->mbus_bridge_ctrl = readl(s->mbusbridge_base +
> +					    MBUS_BRIDGE_CTRL_OFF);
> +		s->mbus_bridge_base = readl(s->mbusbridge_base +
> +					    MBUS_BRIDGE_BASE_OFF);
> +	}

... so here could you exit with an error if the register is not available.
Then it would prevent to enter in suspend instead of crashing the system.


Thanks,

Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/17] bus: mvebu-mbus: suspend/resume support

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 03 Nov 2014 19:08:21 +0100, Gregory CLEMENT wrote:

> >  struct mvebu_mbus_state {
> >  	void __iomem *mbuswins_base;
> >  	void __iomem *sdramwins_base;
> > +	void __iomem *mbusbridge_base;
> 
> Here you store the information about the availability of the register needed
> for suspend/resume ...
> 
> [...]
> 
> 
> > +static int mvebu_mbus_suspend(void)
> > +{
> > +	struct mvebu_mbus_state *s = &mbus_state;
> > +	int win;
> > +
> > +	for (win = 0; win < s->soc->num_wins; win++) {
> > +		void __iomem *addr = s->mbuswins_base +
> > +			s->soc->win_cfg_offset(win);
> > +
> > +		s->wins[win].base = readl(addr + WIN_BASE_OFF);
> > +		s->wins[win].ctrl = readl(addr + WIN_CTRL_OFF);
> > +
> > +		if (win >= s->soc->num_remappable_wins)
> > +			continue;
> > +
> > +		s->wins[win].remap_lo = readl(addr + WIN_REMAP_LO_OFF);
> > +		s->wins[win].remap_hi = readl(addr + WIN_REMAP_HI_OFF);
> > +	}
> > +
> > +	if (s->mbusbridge_base) {
> > +		s->mbus_bridge_ctrl = readl(s->mbusbridge_base +
> > +					    MBUS_BRIDGE_CTRL_OFF);
> > +		s->mbus_bridge_base = readl(s->mbusbridge_base +
> > +					    MBUS_BRIDGE_BASE_OFF);
> > +	}
> 
> ... so here could you exit with an error if the register is not available.
> Then it would prevent to enter in suspend instead of crashing the system.

Hum, yeah, good point. Returning something like -ENODEV will abort the
suspend procedure. I'll fix that. Thanks for the idea.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/17] bus: mvebu-mbus: provide a mechanism to save SDRAM window configuration

[Thomas Petazzoni]

On Marvell EBU platforms, when doing suspend/resume, the SDRAM window
configuration must be saved on suspend, and restored on
resume. However, it needs to be restored on resume *before*
re-entering the kernel, because the SDRAM window configuration defines
the layout of the memory. For this reason, it cannot simply be done in
the ->suspend() and ->resume() hooks of the mvebu-mbus driver.

Instead, it needs to be restored by the bootloader "boot info"
mechanism used when resuming. This mechanism allows the kernel to
define a list of (address, value) pairs when suspending, that the
bootloader will restore on resume before jumping back into the kernel.

This commit therefore adds a new function to the mvebu-mbus driver,
called mvebu_mbus_save_cpu_target(), which will be called by the
platform code to make the mvebu-mbus driver save the SDRAM window
configuration in a way that can be understood by the bootloader "boot
info" mechanism.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/bus/mvebu-mbus.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mbus.h     |  1 +
 2 files changed, 57 insertions(+)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 25b5b35..1d354e7 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -110,6 +110,8 @@ struct mvebu_mbus_soc_data {
 	bool has_mbus_bridge;
 	unsigned int (*win_cfg_offset)(const int win);
 	void (*setup_cpu_target)(struct mvebu_mbus_state *s);
+	int (*save_cpu_target)(struct mvebu_mbus_state *s,
+			       u32 *store_addr);
 	int (*show_cpu_target)(struct mvebu_mbus_state *s,
 			       struct seq_file *seq, void *v);
 };
@@ -128,6 +130,7 @@ struct mvebu_mbus_state {
 	void __iomem *mbuswins_base;
 	void __iomem *sdramwins_base;
 	void __iomem *mbusbridge_base;
+	phys_addr_t sdramwins_phys_base;
 	struct dentry *debugfs_root;
 	struct dentry *debugfs_sdram;
 	struct dentry *debugfs_devs;
@@ -541,6 +544,28 @@ mvebu_mbus_default_setup_cpu_target(struct mvebu_mbus_state *mbus)
 	mvebu_mbus_dram_info.num_cs = cs;
 }
 
+static int
+mvebu_mbus_default_save_cpu_target(struct mvebu_mbus_state *mbus,
+				   u32 *store_addr)
+{
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		u32 base = readl(mbus->sdramwins_base + DDR_BASE_CS_OFF(i));
+		u32 size = readl(mbus->sdramwins_base + DDR_SIZE_CS_OFF(i));
+
+		writel(mbus->sdramwins_phys_base + DDR_BASE_CS_OFF(i),
+		       store_addr++);
+		writel(base, store_addr++);
+		writel(mbus->sdramwins_phys_base + DDR_SIZE_CS_OFF(i),
+		       store_addr++);
+		writel(size, store_addr++);
+	}
+
+	/* We've written 16 words to the store address */
+	return 16;
+}
+
 static void __init
 mvebu_mbus_dove_setup_cpu_target(struct mvebu_mbus_state *mbus)
 {
@@ -571,11 +596,35 @@ mvebu_mbus_dove_setup_cpu_target(struct mvebu_mbus_state *mbus)
 	mvebu_mbus_dram_info.num_cs = cs;
 }
 
+static int
+mvebu_mbus_dove_save_cpu_target(struct mvebu_mbus_state *mbus,
+				u32 *store_addr)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		u32 map = readl(mbus->sdramwins_base + DOVE_DDR_BASE_CS_OFF(i));
+
+		writel(mbus->sdramwins_phys_base + DOVE_DDR_BASE_CS_OFF(i),
+		       store_addr++);
+		writel(map, store_addr++);
+	}
+
+	/* We've written 4 words to the store address */
+	return 4;
+}
+
+int mvebu_mbus_save_cpu_target(u32 *store_addr)
+{
+	return mbus_state.soc->save_cpu_target(&mbus_state, store_addr);
+}
+
 static const struct mvebu_mbus_soc_data armada_370_xp_mbus_data = {
 	.num_wins            = 20,
 	.num_remappable_wins = 8,
 	.has_mbus_bridge     = true,
 	.win_cfg_offset      = armada_370_xp_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_default_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
 };
@@ -584,6 +633,7 @@ static const struct mvebu_mbus_soc_data kirkwood_mbus_data = {
 	.num_wins            = 8,
 	.num_remappable_wins = 4,
 	.win_cfg_offset      = orion_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_default_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
 };
@@ -592,6 +642,7 @@ static const struct mvebu_mbus_soc_data dove_mbus_data = {
 	.num_wins            = 8,
 	.num_remappable_wins = 4,
 	.win_cfg_offset      = orion_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_dove_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_dove_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_dove,
 };
@@ -604,6 +655,7 @@ static const struct mvebu_mbus_soc_data orion5x_4win_mbus_data = {
 	.num_wins            = 8,
 	.num_remappable_wins = 4,
 	.win_cfg_offset      = orion_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_default_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
 };
@@ -612,6 +664,7 @@ static const struct mvebu_mbus_soc_data orion5x_2win_mbus_data = {
 	.num_wins            = 8,
 	.num_remappable_wins = 2,
 	.win_cfg_offset      = orion_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_default_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
 };
@@ -620,6 +673,7 @@ static const struct mvebu_mbus_soc_data mv78xx0_mbus_data = {
 	.num_wins            = 14,
 	.num_remappable_wins = 8,
 	.win_cfg_offset      = mv78xx0_mbus_win_offset,
+	.save_cpu_target     = mvebu_mbus_default_save_cpu_target,
 	.setup_cpu_target    = mvebu_mbus_default_setup_cpu_target,
 	.show_cpu_target     = mvebu_sdram_debug_show_orion,
 };
@@ -805,6 +859,8 @@ static int __init mvebu_mbus_common_init(struct mvebu_mbus_state *mbus,
 		return -ENOMEM;
 	}
 
+	mbus->sdramwins_phys_base = sdramwins_phys_base;
+
 	if (mbusbridge_phys_base) {
 		mbus->mbusbridge_base = ioremap(mbusbridge_phys_base,
 						mbusbridge_size);
diff --git a/include/linux/mbus.h b/include/linux/mbus.h
index 550c88f..611b69f 100644
--- a/include/linux/mbus.h
+++ b/include/linux/mbus.h
@@ -61,6 +61,7 @@ static inline const struct mbus_dram_target_info *mv_mbus_dram_info(void)
 }
 #endif
 
+int mvebu_mbus_save_cpu_target(u32 *store_addr);
 void mvebu_mbus_get_pcie_mem_aperture(struct resource *res);
 void mvebu_mbus_get_pcie_io_aperture(struct resource *res);
 int mvebu_mbus_add_window_remap_by_id(unsigned int target,
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/17] bus: mvebu-mbus: provide a mechanism to save SDRAM window configuration

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> On Marvell EBU platforms, when doing suspend/resume, the SDRAM window
> configuration must be saved on suspend, and restored on
> resume. However, it needs to be restored on resume *before*
> re-entering the kernel, because the SDRAM window configuration defines
> the layout of the memory. For this reason, it cannot simply be done in
> the ->suspend() and ->resume() hooks of the mvebu-mbus driver.
> 
> Instead, it needs to be restored by the bootloader "boot info"
> mechanism used when resuming. This mechanism allows the kernel to
> define a list of (address, value) pairs when suspending, that the
> bootloader will restore on resume before jumping back into the kernel.
> 
> This commit therefore adds a new function to the mvebu-mbus driver,
> called mvebu_mbus_save_cpu_target(), which will be called by the
> platform code to make the mvebu-mbus driver save the SDRAM window
> configuration in a way that can be understood by the bootloader "boot
> info" mechanism.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/bus/mvebu-mbus.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mbus.h     |  1 +
>  2 files changed, 57 insertions(+)

Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/17] ARM: mvebu: implement suspend/resume support for Armada XP

[Thomas Petazzoni]

This commit implements the core of the platform code to enable
suspend/resume on Armada XP. It registers the platform_suspend_ops
structure, and implements the ->enter() hook of this structure.

It is worth mentioning that this commit only provides the SoC-level
part of suspend/resume, which calls into some board-specific code
provided in a follow-up commit.

The most important thing that this SoC-level code has to do is to
build an in-memory structure that contains a magic number, the return
address in the kernel after resume, and a set of address/value
pairs. This structure is used by the bootloader to restore a certain
number of registers (according to the set of address/value pairs) and
then jump back into the kernel at the provided location.

The code also puts the SDRAM into self-refresh mode, before calling
into board-specific code to actually enter the suspend to RAM state.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/Makefile |   2 +-
 arch/arm/mach-mvebu/common.h |   2 +
 arch/arm/mach-mvebu/pm.c     | 220 +++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/pmsu.h   |   1 +
 4 files changed, 224 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/pm.c

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index e24136b..e2bd96f 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -7,7 +7,7 @@ CFLAGS_pmsu.o			:= -march=armv7-a
 obj-$(CONFIG_MACH_MVEBU_ANY)	 += system-controller.o mvebu-soc-id.o
 
 ifeq ($(CONFIG_MACH_MVEBU_V7),y)
-obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o
+obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o pm.o
 obj-$(CONFIG_SMP)		 += platsmp.o headsmp.o platsmp-a9.o headsmp-a9.o
 endif
 
diff --git a/arch/arm/mach-mvebu/common.h b/arch/arm/mach-mvebu/common.h
index 3ccb40c..3e0aca1 100644
--- a/arch/arm/mach-mvebu/common.h
+++ b/arch/arm/mach-mvebu/common.h
@@ -25,4 +25,6 @@ int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev);
 
 void __iomem *mvebu_get_scu_base(void);
 
+int mvebu_pm_init(void (*board_pm_enter)(void __iomem *sdram_reg, u32 srcmd));
+
 #endif
diff --git a/arch/arm/mach-mvebu/pm.c b/arch/arm/mach-mvebu/pm.c
new file mode 100644
index 0000000..19b6fb8
--- /dev/null
+++ b/arch/arm/mach-mvebu/pm.c
@@ -0,0 +1,220 @@
+/*
+ * Suspend/resume support. Currently supporting Armada XP only.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mbus.h>
+#include <linux/of_address.h>
+#include <linux/suspend.h>
+#include <asm/cacheflush.h>
+#include <asm/outercache.h>
+#include <asm/suspend.h>
+
+#include "coherency.h"
+#include "pmsu.h"
+
+#define SDRAM_CONFIG_OFFS                  0x0
+#define  SDRAM_CONFIG_SR_MODE_BIT          BIT(24)
+#define SDRAM_OPERATION_OFFS               0x18
+#define  SDRAM_OPERATION_SELF_REFRESH      0x7
+#define SDRAM_DLB_EVICTION_OFFS            0x30c
+#define  SDRAM_DLB_EVICTION_THRESHOLD_MASK 0xff
+
+static void (*mvebu_board_pm_enter)(void __iomem *sdram_reg, u32 srcmd);
+static void __iomem *sdram_ctrl;
+
+static int mvebu_pm_powerdown(unsigned long data)
+{
+	u32 reg, srcmd;
+
+	flush_cache_all();
+	outer_flush_all();
+
+	/*
+	 * Issue a Data Synchronization Barrier instruction to ensure
+	 * that all state saving has been completed.
+	 */
+	dsb();
+
+	/* Flush the DLB and wait ~7 usec */
+	reg = readl(sdram_ctrl + SDRAM_DLB_EVICTION_OFFS);
+	reg &= ~SDRAM_DLB_EVICTION_THRESHOLD_MASK;
+	writel(reg, sdram_ctrl + SDRAM_DLB_EVICTION_OFFS);
+
+	udelay(7);
+
+	/* Set DRAM in battery backup mode */
+	reg = readl(sdram_ctrl + SDRAM_CONFIG_OFFS);
+	reg &= ~SDRAM_CONFIG_SR_MODE_BIT;
+	writel(reg, sdram_ctrl + SDRAM_CONFIG_OFFS);
+
+	/* Prepare to go to self-refresh */
+
+	srcmd = readl(sdram_ctrl + SDRAM_OPERATION_OFFS);
+	srcmd &= ~0x1F;
+	srcmd |= SDRAM_OPERATION_SELF_REFRESH;
+
+	mvebu_board_pm_enter(sdram_ctrl + SDRAM_OPERATION_OFFS, srcmd);
+
+	return 0;
+}
+
+#define BOOT_INFO_ADDR      0x3000
+#define BOOT_MAGIC_WORD	    0xdeadb002
+#define BOOT_MAGIC_LIST_END 0xffffffff
+
+/*
+ * Those registers are accessed before switching the internal register
+ * base, which is why we hardcode the 0xd0000000 base address, the one
+ * used by the SoC out of reset.
+ */
+#define MBUS_WINDOW_12_CTRL       0xd00200b0
+#define MBUS_INTERNAL_REG_ADDRESS 0xd0020080
+
+#define SDRAM_WIN_BASE_REG(x)	(0x20180 + (0x8*x))
+#define SDRAM_WIN_CTRL_REG(x)	(0x20184 + (0x8*x))
+
+static phys_addr_t mvebu_internal_reg_base(void)
+{
+	struct device_node *np;
+	__be32 in_addr[2];
+
+	np = of_find_node_by_name(NULL, "internal-regs");
+	BUG_ON(!np);
+
+	/*
+	 * Ask the DT what is the internal register address on this
+	 * platform. In the mvebu-mbus DT binding, 0xf0010000
+	 * corresponds to the internal register window.
+	 */
+	in_addr[0] = cpu_to_be32(0xf0010000);
+	in_addr[1] = 0x0;
+
+	return of_translate_address(np, in_addr);
+}
+
+static void mvebu_pm_store_bootinfo(void)
+{
+	u32 *store_addr;
+	phys_addr_t resume_pc;
+
+	store_addr = phys_to_virt(BOOT_INFO_ADDR);
+	resume_pc = virt_to_phys(armada_370_xp_cpu_resume);
+
+	/*
+	 * The bootloader expects the first two words to be a magic
+	 * value (BOOT_MAGIC_WORD), followed by the address of the
+	 * resume code to jump to. Then, it expects a sequence of
+	 * (address, value) pairs, which can be used to restore the
+	 * value of certain registers. This sequence must end with the
+	 * BOOT_MAGIC_LIST_END magic value.
+	 */
+
+	writel(BOOT_MAGIC_WORD, store_addr++);
+	writel(resume_pc, store_addr++);
+
+	/*
+	 * Some platforms remap their internal register base address
+	 * to 0xf1000000. However, out of reset, window 12 starts at
+	 * 0xf0000000 and ends at 0xf7ffffff, which would overlap with
+	 * the internal registers. Therefore, disable window 12.
+	 */
+	writel(MBUS_WINDOW_12_CTRL, store_addr++);
+	writel(0x0, store_addr++);
+
+	/*
+	 * Set the internal register base address to the value
+	 * expected by Linux, as read from the Device Tree.
+	 */
+	writel(MBUS_INTERNAL_REG_ADDRESS, store_addr++);
+	writel(mvebu_internal_reg_base(), store_addr++);
+
+	/*
+	 * Ask the mvebu-mbus driver to store the SDRAM window
+	 * configuration, which has to be restored by the bootloader
+	 * before re-entering the kernel on resume.
+	 */
+	store_addr += mvebu_mbus_save_cpu_target(store_addr);
+
+	writel(BOOT_MAGIC_LIST_END, store_addr);
+}
+
+static int mvebu_pm_enter(suspend_state_t state)
+{
+	if (state != PM_SUSPEND_MEM)
+		return -EINVAL;
+
+	cpu_pm_enter();
+	cpu_cluster_pm_enter();
+
+	mvebu_pm_store_bootinfo();
+	cpu_suspend(0, mvebu_pm_powerdown);
+
+	outer_resume();
+
+	mvebu_v7_pmsu_idle_exit();
+
+	set_cpu_coherent();
+
+	cpu_cluster_pm_exit();
+	cpu_pm_exit();
+
+	return 0;
+}
+
+static const struct platform_suspend_ops mvebu_pm_ops = {
+	.enter = mvebu_pm_enter,
+	.valid = suspend_valid_only_mem,
+};
+
+int mvebu_pm_init(void (*board_pm_enter)(void __iomem *sdram_reg, u32 srcmd))
+{
+	struct device_node *np;
+	struct resource res;
+
+	if (!of_machine_is_compatible("marvell,armadaxp"))
+		return -ENODEV;
+
+	np = of_find_compatible_node(NULL, NULL,
+				     "marvell,armada-xp-sdram-controller");
+	if (!np)
+		return -ENODEV;
+
+	if (of_address_to_resource(np, 0, &res)) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	if (!request_mem_region(res.start, resource_size(&res),
+				np->full_name)) {
+		of_node_put(np);
+		return -EBUSY;
+	}
+
+	sdram_ctrl = ioremap(res.start, resource_size(&res));
+	if (!sdram_ctrl) {
+		release_mem_region(res.start, resource_size(&res));
+		of_node_put(np);
+		return -ENOMEM;
+	}
+
+	of_node_put(np);
+
+	mvebu_board_pm_enter = board_pm_enter;
+
+	suspend_set_ops(&mvebu_pm_ops);
+
+	return 0;
+}
diff --git a/arch/arm/mach-mvebu/pmsu.h b/arch/arm/mach-mvebu/pmsu.h
index 6b58c1f..a7c242d 100644
--- a/arch/arm/mach-mvebu/pmsu.h
+++ b/arch/arm/mach-mvebu/pmsu.h
@@ -17,5 +17,6 @@ int mvebu_setup_boot_addr_wa(unsigned int crypto_eng_target,
                              phys_addr_t resume_addr_reg);
 
 void mvebu_v7_pmsu_idle_exit(void);
+void armada_370_xp_cpu_resume(void);
 
 #endif	/* __MACH_370_XP_PMSU_H */
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/17] ARM: mvebu: implement suspend/resume support for Armada XP

[Gregory CLEMENT]

Hi Thomas,

[...]

> +static int mvebu_pm_enter(suspend_state_t state)
> +{
> +	if (state != PM_SUSPEND_MEM)
> +		return -EINVAL;
> +
> +	cpu_pm_enter();
> +	cpu_cluster_pm_enter();

Why do you use this function?

It notifies the listeners with CPU_CLUSTER_PM_ENTER but currently only omap2
and the gic are waiting for it.

Do you already plan to be compatible with the cortex A9 based mvebu SoC?

By the way the gic doesn't use the suspend and resume operation from syscore_ops
but the notification. Is there any particular reason for using syscore_ops in
the irq-armada-370-xp driver?


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/17] ARM: mvebu: implement suspend/resume support for Armada XP

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Tue, 04 Nov 2014 11:00:46 +0100, Gregory CLEMENT wrote:

> > +static int mvebu_pm_enter(suspend_state_t state)
> > +{
> > +	if (state != PM_SUSPEND_MEM)
> > +		return -EINVAL;
> > +
> > +	cpu_pm_enter();
> > +	cpu_cluster_pm_enter();
> 
> Why do you use this function?
> 
> It notifies the listeners with CPU_CLUSTER_PM_ENTER but currently only omap2
> and the gic are waiting for it.
> 
> Do you already plan to be compatible with the cortex A9 based mvebu SoC?

Indeed, using cpu_cluster_pm_enter() is not necessary for the moment,
it was just a stupid copy and paste from the PM code for other SoCs.

Support for the Cortex A9 based mvebu SoCs is indeed planned at some
point, but for the moment I don't know how much PM code will be shared,
and it will anyway be time to re-add these cpu_cluster_pm_enter and
cpu_cluster_pm_exit calls, so for now I removed them.

> By the way the gic doesn't use the suspend and resume operation from syscore_ops
> but the notification. Is there any particular reason for using
> syscore_ops in the irq-armada-370-xp driver?

Well, the syscore_ops strategy was already used in irq-metag-ext,
irq-vic and irq-sirfsoc, while the notifier-based strategy is only used
by irq-gic. Plus I'm using the syscore_ops strategy in all other core
drivers (clock, mvebu-mbus, etc.), so to me it makes sense to use the
same strategy in the irqchip driver, especially if other irqchip
drivers are doing the same thing.

Though this is definitely not a very strong opinion, and if the irqchip
maintainers would like to see the notifier-based strategy used here,
I'd do the change without a problem.

Thanks for your comments!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/17] ARM: mvebu: reserve the first 10 KB of each memory bank for suspend/resume

[Thomas Petazzoni]

When going out of suspend to RAM, the Marvell EBU platforms go through
the bootloader, which re-configures the DRAM controller. To achieve
this, the bootloader executes a piece of code called the "DDR3
training code". It does some reads/writes to the memory to find out
the optimal timings for the memory chip being used.

This has the nasty side effect that the first 10 KB of each DRAM
chip-select are overwritten by the bootloader when exiting the suspend
to RAM state.

Therefore, this commit implements the ->reserve() hook for the 'struct
machine_desc' used on Armada XP, to reserve the 10 KB of each DRAM
chip-select using the memblock API.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/board-v7.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 6478626..bc52231 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -16,10 +16,12 @@
 #include <linux/init.h>
 #include <linux/clk-provider.h>
 #include <linux/of_address.h>
+#include <linux/of_fdt.h>
 #include <linux/of_platform.h>
 #include <linux/io.h>
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
+#include <linux/memblock.h>
 #include <linux/mbus.h>
 #include <linux/signal.h>
 #include <linux/slab.h>
@@ -57,6 +59,54 @@ void __iomem *mvebu_get_scu_base(void)
 }
 
 /*
+ * When returning from suspend, the platform goes through the
+ * bootloader, which executes its DDR3 training code. This code has
+ * the unfortunate idea of using the first 10 KB of each DRAM bank to
+ * exercise the RAM and calculate the optimal timings. Therefore, this
+ * area of RAM is overwritten, and shouldn't be used by the kernel if
+ * suspend/resume is supported.
+ */
+
+#ifdef CONFIG_SUSPEND
+#define MVEBU_DDR_TRAINING_AREA_SZ (10 * SZ_1K)
+static int __init mvebu_scan_mem(unsigned long node, const char *uname,
+				 int depth, void *data)
+{
+	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
+	const __be32 *reg, *endp;
+	int l;
+
+	if (type == NULL || strcmp(type, "memory"))
+		return 0;
+
+	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
+	if (reg == NULL)
+		reg = of_get_flat_dt_prop(node, "reg", &l);
+	if (reg == NULL)
+		return 0;
+
+	endp = reg + (l / sizeof(__be32));
+	while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
+		u64 base, size;
+
+		base = dt_mem_next_cell(dt_root_addr_cells, &reg);
+		size = dt_mem_next_cell(dt_root_size_cells, &reg);
+
+		memblock_reserve(base, MVEBU_DDR_TRAINING_AREA_SZ);
+	}
+
+	return 0;
+}
+
+static void __init mvebu_memblock_reserve(void)
+{
+	of_scan_flat_dt(mvebu_scan_mem, NULL);
+}
+#else
+static void __init mvebu_memblock_reserve(void) {}
+#endif
+
+/*
  * Early versions of Armada 375 SoC have a bug where the BootROM
  * leaves an external data abort pending. The kernel is hit by this
  * data abort as soon as it enters userspace, because it unmasks the
@@ -210,6 +260,7 @@ DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
 	.restart	= mvebu_restart,
+	.reserve        = mvebu_memblock_reserve,
 	.dt_compat	= armada_370_xp_dt_compat,
 MACHINE_END
 
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/17] ARM: mvebu: reserve the first 10 KB of each memory bank for suspend/resume

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> When going out of suspend to RAM, the Marvell EBU platforms go through
> the bootloader, which re-configures the DRAM controller. To achieve
> this, the bootloader executes a piece of code called the "DDR3
> training code". It does some reads/writes to the memory to find out
> the optimal timings for the memory chip being used.
> 
> This has the nasty side effect that the first 10 KB of each DRAM
> chip-select are overwritten by the bootloader when exiting the suspend
> to RAM state.
> 
> Therefore, this commit implements the ->reserve() hook for the 'struct
> machine_desc' used on Armada XP, to reserve the 10 KB of each DRAM
> chip-select using the memblock API.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/mach-mvebu/board-v7.c | 51 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)

It is unfortunate to put these "holes" in the memory but it is a design constraint.
However, your solution is nice and very well contained however.

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Thomas Petazzoni]

On the Armada XP GP platform, entering suspend to RAM state is
triggering by talking to an external PIC micro-controller connected to
the SoC using 3 GPIOs. There is then a small magic sequence of GPIO
toggling that needs to be used to tell the PIC to turn off the SoC.

The code uses the Device Tree to find out which GPIOs are used to
connect to the PIC micro-controller, and then registers its
mvebu_armada_xp_gp_pm_enter() callback to the SoC-level PM code. The
SoC PM code will call back into this registered function at the very
end of the suspend procedure.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/Makefile   |   2 +-
 arch/arm/mach-mvebu/pm-board.c | 134 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/pm-board.c

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index e2bd96f..b4f0149 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -7,7 +7,7 @@ CFLAGS_pmsu.o			:= -march=armv7-a
 obj-$(CONFIG_MACH_MVEBU_ANY)	 += system-controller.o mvebu-soc-id.o
 
 ifeq ($(CONFIG_MACH_MVEBU_V7),y)
-obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o pm.o
+obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o pm.o pm-board.o
 obj-$(CONFIG_SMP)		 += platsmp.o headsmp.o platsmp-a9.o headsmp-a9.o
 endif
 
diff --git a/arch/arm/mach-mvebu/pm-board.c b/arch/arm/mach-mvebu/pm-board.c
new file mode 100644
index 0000000..e1a9792
--- /dev/null
+++ b/arch/arm/mach-mvebu/pm-board.c
@@ -0,0 +1,134 @@
+/*
+ * Board-level suspend/resume support.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
+#include "common.h"
+
+#define ARMADA_XP_GP_PIC_NR_GPIOS 3
+
+static void __iomem *gpio_ctrl;
+static int pic_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
+static int pic_raw_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
+
+static void mvebu_armada_xp_gp_pm_enter(void __iomem *sdram_reg, u32 srcmd)
+{
+	u32 reg, ackcmd;
+	int i;
+
+	/* Put 001 as value on the GPIOs */
+	reg = readl(gpio_ctrl);
+	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
+		reg &= ~BIT(pic_raw_gpios[i]);
+	reg |= BIT(pic_raw_gpios[0]);
+	writel(reg, gpio_ctrl);
+
+	/* Prepare writing 111 to the GPIOs */
+	ackcmd = readl(gpio_ctrl);
+	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
+		ackcmd |= BIT(pic_raw_gpios[i]);
+
+	/* Wait a while */
+	mdelay(250);
+
+	asm volatile (
+		/* Align to a cache line */
+		".balign 32\n\t"
+
+		/* Enter self refresh */
+		"str %[srcmd], [%[sdram_reg]]\n\t"
+
+		/* Wait 100 cycles for DDR to enter self refresh */
+		"1: subs r1, r1, #1\n\t"
+		"bne 1b\n\t"
+
+		/* Issue the command ACK */
+		"str %[ackcmd], [%[gpio_ctrl]]\n\t"
+
+		/* Trap the processor */
+		"b .\n\t"
+		: : [srcmd] "r" (srcmd), [sdram_reg] "r" (sdram_reg),
+		  [ackcmd] "r" (ackcmd), [gpio_ctrl] "r" (gpio_ctrl) : "r1");
+}
+
+static int mvebu_armada_xp_gp_pm_init(void)
+{
+	struct device_node *np;
+	struct device_node *gpio_ctrl_np;
+	int ret = 0, i;
+
+	if (!of_machine_is_compatible("marvell,axp-gp"))
+		return -ENODEV;
+
+	np = of_find_node_by_name(NULL, "pm_pic");
+	if (!np)
+		return -ENODEV;
+
+	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++) {
+		char *name;
+		struct of_phandle_args args;
+
+		pic_gpios[i] = of_get_named_gpio(np, "ctrl-gpios", i);
+		if (pic_gpios[i] < 0) {
+			ret = -ENODEV;
+			goto out;
+		}
+
+		name = kasprintf(GFP_KERNEL, "pic-pin%d", i);
+		if (!name) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		ret = gpio_request(pic_gpios[i], name);
+		if (ret < 0) {
+			kfree(name);
+			goto out;
+		}
+
+		ret = gpio_direction_output(pic_gpios[i], 0);
+		if (ret < 0) {
+			gpio_free(pic_gpios[i]);
+			kfree(name);
+			goto out;
+		}
+
+		ret = of_parse_phandle_with_fixed_args(np, "ctrl-gpios", 2,
+						       i, &args);
+		if (ret < 0) {
+			gpio_free(pic_gpios[i]);
+			kfree(name);
+			goto out;
+		}
+
+		gpio_ctrl_np = args.np;
+		pic_raw_gpios[i] = args.args[0];
+	}
+
+	gpio_ctrl = of_iomap(gpio_ctrl_np, 0);
+	if (!gpio_ctrl)
+		return -ENOMEM;
+
+	mvebu_pm_init(mvebu_armada_xp_gp_pm_enter);
+
+out:
+	of_node_put(np);
+	return ret;
+}
+
+late_initcall(mvebu_armada_xp_gp_pm_init);
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Andrew Lunn]

On Fri, Oct 24, 2014 at 01:59:25PM +0200, Thomas Petazzoni wrote:
> On the Armada XP GP platform, entering suspend to RAM state is
> triggering by talking to an external PIC micro-controller connected to
> the SoC using 3 GPIOs. There is then a small magic sequence of GPIO
> toggling that needs to be used to tell the PIC to turn off the SoC.

Hi Thomas

Does Marvell mandate this PIC and gpio interface? Or is a board
designer free to implement it some other way? It seems to me, this
should be considered specific to the Marvell reference design.

I'm wondering if this code should be a power driver, living in
drivers/power/reset/.

A few kirkwood boards have a PIC controlling the power. QNAP devices
use a serial port to talk to it. Others use a single gpio line. Is
there something to stop a designer doing this for XP?

It seems like you would have a more generic solution by allowing DT to
specify a power off driver to use.

	Andrew

> 
> The code uses the Device Tree to find out which GPIOs are used to
> connect to the PIC micro-controller, and then registers its
> mvebu_armada_xp_gp_pm_enter() callback to the SoC-level PM code. The
> SoC PM code will call back into this registered function at the very
> end of the suspend procedure.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/mach-mvebu/Makefile   |   2 +-
>  arch/arm/mach-mvebu/pm-board.c | 134 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 135 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-mvebu/pm-board.c
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index e2bd96f..b4f0149 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -7,7 +7,7 @@ CFLAGS_pmsu.o			:= -march=armv7-a
>  obj-$(CONFIG_MACH_MVEBU_ANY)	 += system-controller.o mvebu-soc-id.o
>  
>  ifeq ($(CONFIG_MACH_MVEBU_V7),y)
> -obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o pm.o
> +obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o pm.o pm-board.o
>  obj-$(CONFIG_SMP)		 += platsmp.o headsmp.o platsmp-a9.o headsmp-a9.o
>  endif
>  
> diff --git a/arch/arm/mach-mvebu/pm-board.c b/arch/arm/mach-mvebu/pm-board.c
> new file mode 100644
> index 0000000..e1a9792
> --- /dev/null
> +++ b/arch/arm/mach-mvebu/pm-board.c
> @@ -0,0 +1,134 @@
> +/*
> + * Board-level suspend/resume support.
> + *
> + * Copyright (C) 2014 Marvell
> + *
> + * Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
> +#include "common.h"
> +
> +#define ARMADA_XP_GP_PIC_NR_GPIOS 3
> +
> +static void __iomem *gpio_ctrl;
> +static int pic_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
> +static int pic_raw_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
> +
> +static void mvebu_armada_xp_gp_pm_enter(void __iomem *sdram_reg, u32 srcmd)
> +{
> +	u32 reg, ackcmd;
> +	int i;
> +
> +	/* Put 001 as value on the GPIOs */
> +	reg = readl(gpio_ctrl);
> +	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
> +		reg &= ~BIT(pic_raw_gpios[i]);
> +	reg |= BIT(pic_raw_gpios[0]);
> +	writel(reg, gpio_ctrl);
> +
> +	/* Prepare writing 111 to the GPIOs */
> +	ackcmd = readl(gpio_ctrl);
> +	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
> +		ackcmd |= BIT(pic_raw_gpios[i]);
> +
> +	/* Wait a while */
> +	mdelay(250);
> +
> +	asm volatile (
> +		/* Align to a cache line */
> +		".balign 32\n\t"
> +
> +		/* Enter self refresh */
> +		"str %[srcmd], [%[sdram_reg]]\n\t"
> +
> +		/* Wait 100 cycles for DDR to enter self refresh */
> +		"1: subs r1, r1, #1\n\t"
> +		"bne 1b\n\t"
> +
> +		/* Issue the command ACK */
> +		"str %[ackcmd], [%[gpio_ctrl]]\n\t"
> +
> +		/* Trap the processor */
> +		"b .\n\t"
> +		: : [srcmd] "r" (srcmd), [sdram_reg] "r" (sdram_reg),
> +		  [ackcmd] "r" (ackcmd), [gpio_ctrl] "r" (gpio_ctrl) : "r1");
> +}
> +
> +static int mvebu_armada_xp_gp_pm_init(void)
> +{
> +	struct device_node *np;
> +	struct device_node *gpio_ctrl_np;
> +	int ret = 0, i;
> +
> +	if (!of_machine_is_compatible("marvell,axp-gp"))
> +		return -ENODEV;
> +
> +	np = of_find_node_by_name(NULL, "pm_pic");
> +	if (!np)
> +		return -ENODEV;
> +
> +	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++) {
> +		char *name;
> +		struct of_phandle_args args;
> +
> +		pic_gpios[i] = of_get_named_gpio(np, "ctrl-gpios", i);
> +		if (pic_gpios[i] < 0) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +
> +		name = kasprintf(GFP_KERNEL, "pic-pin%d", i);
> +		if (!name) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		ret = gpio_request(pic_gpios[i], name);
> +		if (ret < 0) {
> +			kfree(name);
> +			goto out;
> +		}
> +
> +		ret = gpio_direction_output(pic_gpios[i], 0);
> +		if (ret < 0) {
> +			gpio_free(pic_gpios[i]);
> +			kfree(name);
> +			goto out;
> +		}
> +
> +		ret = of_parse_phandle_with_fixed_args(np, "ctrl-gpios", 2,
> +						       i, &args);
> +		if (ret < 0) {
> +			gpio_free(pic_gpios[i]);
> +			kfree(name);
> +			goto out;
> +		}
> +
> +		gpio_ctrl_np = args.np;
> +		pic_raw_gpios[i] = args.args[0];
> +	}
> +
> +	gpio_ctrl = of_iomap(gpio_ctrl_np, 0);
> +	if (!gpio_ctrl)
> +		return -ENOMEM;
> +
> +	mvebu_pm_init(mvebu_armada_xp_gp_pm_enter);
> +
> +out:
> +	of_node_put(np);
> +	return ret;
> +}
> +
> +late_initcall(mvebu_armada_xp_gp_pm_init);
> -- 
> 2.0.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Thomas Petazzoni]

Dear Andrew Lunn,

On Fri, 24 Oct 2014 16:20:44 +0200, Andrew Lunn wrote:

> Does Marvell mandate this PIC and gpio interface? Or is a board
> designer free to implement it some other way? It seems to me, this
> should be considered specific to the Marvell reference design.

They don't mandate this interface, it's really a board-specific
decision, which is why I've split my implementation between:

 * SoC-specific code, in mach-mvebu/pm.c.

 * Board-specific code, in mach-mvebu/pm-board.c.

> I'm wondering if this code should be a power driver, living in
> drivers/power/reset/.

I'm fine with that, but have you seen the *very* tight interaction
between the SoC-specific code and the board-specific code? The problem
is that the board-specific code needs to put the SDRAM into
self-refresh *right* before shutting down the SoC, and all that while
making sure the code doing both of these operations remains in the
I-Cache, and does not touch any other location in memory (which has
become inaccessible due to being in self-refresh mode).

Look at the mvebu_armada_xp_gp_pm_enter() function: it takes two
arguments, received from the SoC-level code. How to handle this thing
with a driver in drivers/power/reset/ ?

> A few kirkwood boards have a PIC controlling the power. QNAP devices
> use a serial port to talk to it. Others use a single gpio line. Is
> there something to stop a designer doing this for XP?

Nothing stops that. I think Marvell told me they wanted to standardize
this GPIO interface with a PIC, but that's only for their own
development boards. Since it remains something outside the SoC, it is
by nature board-specific, and designers can do pretty much what they
want.

> It seems like you would have a more generic solution by allowing DT to
> specify a power off driver to use.

Sure, but see above. If you have some suggestion, I'm surely interested.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Andrew Lunn]

On Fri, Oct 24, 2014 at 04:28:24PM +0200, Thomas Petazzoni wrote:
> Dear Andrew Lunn,
> 
> On Fri, 24 Oct 2014 16:20:44 +0200, Andrew Lunn wrote:
> 
> > Does Marvell mandate this PIC and gpio interface? Or is a board
> > designer free to implement it some other way? It seems to me, this
> > should be considered specific to the Marvell reference design.
> 
> They don't mandate this interface, it's really a board-specific
> decision, which is why I've split my implementation between:
> 
>  * SoC-specific code, in mach-mvebu/pm.c.
> 
>  * Board-specific code, in mach-mvebu/pm-board.c.
> 
> > I'm wondering if this code should be a power driver, living in
> > drivers/power/reset/.
> 
> I'm fine with that, but have you seen the *very* tight interaction
> between the SoC-specific code and the board-specific code? The problem
> is that the board-specific code needs to put the SDRAM into
> self-refresh *right* before shutting down the SoC, and all that while
> making sure the code doing both of these operations remains in the
> I-Cache, and does not touch any other location in memory (which has
> become inaccessible due to being in self-refresh mode).
> 
> Look at the mvebu_armada_xp_gp_pm_enter() function: it takes two
> arguments, received from the SoC-level code. How to handle this thing
> with a driver in drivers/power/reset/ ?

It looks like reset drivers can register a notifier block, and you can
pass this notifier a void * parameter. So you should be able to pass
parameters. Nobody currently does this, so it might not work....

	    Andrew

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Thomas Petazzoni]

Dear Andrew Lunn,

On Fri, 24 Oct 2014 16:51:19 +0200, Andrew Lunn wrote:

> > > I'm wondering if this code should be a power driver, living in
> > > drivers/power/reset/.
> > 
> > I'm fine with that, but have you seen the *very* tight interaction
> > between the SoC-specific code and the board-specific code? The problem
> > is that the board-specific code needs to put the SDRAM into
> > self-refresh *right* before shutting down the SoC, and all that while
> > making sure the code doing both of these operations remains in the
> > I-Cache, and does not touch any other location in memory (which has
> > become inaccessible due to being in self-refresh mode).
> > 
> > Look at the mvebu_armada_xp_gp_pm_enter() function: it takes two
> > arguments, received from the SoC-level code. How to handle this thing
> > with a driver in drivers/power/reset/ ?
> 
> It looks like reset drivers can register a notifier block, and you can
> pass this notifier a void * parameter. So you should be able to pass
> parameters. Nobody currently does this, so it might not work....

I had a closer look, but I'm sorry, I don't really see how
drivers/power/reset can help here. In drivers/power/reset, there are
only drivers that handle powering off a platform, or rebooting a
platform. They hook up by setting a value to the pm_power_off and/or
arm_pm_restart function pointers.

But in my case, what's needed is neither a power off nor a reboot, but
an entry to suspend to RAM, which is a different state. I don't see how
the drivers/power/reset driver would get "called" by the suspend/resume
procedure.

I also don't see how notifiers can help here. On which notification
would the drivers/power/reset driver register itself? By creating a new
notification type, specific to the mvebu platform?

I'm fine with reworking the implementation I've proposed, but if you
could shed some more light on what your proposal would look like, it
would be useful, as I currently don't see how drivers/power/reset fits
the need of the Armada XP suspend/resume mechanism.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Andrew Lunn]

Hi Thomas
 
> I had a closer look, but I'm sorry, I don't really see how
> drivers/power/reset can help here. In drivers/power/reset, there are
> only drivers that handle powering off a platform, or rebooting a
> platform. They hook up by setting a value to the pm_power_off and/or
> arm_pm_restart function pointers.

This has changed recently. I think pm_power_off and arm_pm_restart
have been replaced by a notifier chain.
 
> But in my case, what's needed is neither a power off nor a reboot, but
> an entry to suspend to RAM, which is a different state. I don't see how
> the drivers/power/reset driver would get "called" by the suspend/resume
> procedure.

I'm not sure it will work. It is just an idea.

I'm making some assumptions here, which could be wrong.

#1 This GPIO interface does more than suspend to RAM power off. I
assume it also does full power off? It might also support Wake on LAN?

#2 Any code can call a notifier chain and pass parameters to that
chain?

If #1 is true, you are going to write a drivers/power/reset driver at
some point, so you can power off the board. So once you have that
code, all you need to do is pass an additional parameter, saying this
is a suspend to RAM power off, not a full power off. It then puts the
DRAM controller into self refresh before bit banging the gpios.

If #2 is correct, you have a mechanism to do this. In your suspend
function, call the notifier chain passing the needed parameter.  If
the notifier chain is called for a normal power off, the parameters
will be missing, and you know it is a full power off.

Anyway, it is just an idea. Feel free it ignore it.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Thomas Petazzoni]

Dear Andrew Lunn,

On Mon, 27 Oct 2014 15:19:58 +0100, Andrew Lunn wrote:

> > But in my case, what's needed is neither a power off nor a reboot, but
> > an entry to suspend to RAM, which is a different state. I don't see how
> > the drivers/power/reset driver would get "called" by the suspend/resume
> > procedure.
> 
> I'm not sure it will work. It is just an idea.
> 
> I'm making some assumptions here, which could be wrong.
> 
> #1 This GPIO interface does more than suspend to RAM power off. I
> assume it also does full power off? It might also support Wake on LAN?

This I don't know, but we can probably assume it's the case.

> #2 Any code can call a notifier chain and pass parameters to that
> chain?

Right. But for existing notifier chains, the existence, semantic and
meaning of the parameters are already defined, and the gazillions users
of that notifier chain in the kernel rely on those parameters to not
change.

> If #1 is true, you are going to write a drivers/power/reset driver at
> some point, so you can power off the board. So once you have that
> code, all you need to do is pass an additional parameter, saying this
> is a suspend to RAM power off, not a full power off. It then puts the
> DRAM controller into self refresh before bit banging the gpios.
> 
> If #2 is correct, you have a mechanism to do this. In your suspend
> function, call the notifier chain passing the needed parameter.  If
> the notifier chain is called for a normal power off, the parameters
> will be missing, and you know it is a full power off.
> 
> Anyway, it is just an idea. Feel free it ignore it.

Are we talking about  the reboot_notifier_list, implemented by
kernel/reboot.c? It seems to me it's used to let drivers know that they
should quiesce all DMA transfers and things like that before doing a
shutdown. It's also tied to the system states that are defined in
<linux/kernel.h> :

/* Values used for system_state */
extern enum system_states {
        SYSTEM_BOOTING,
        SYSTEM_RUNNING,
        SYSTEM_HALT,
        SYSTEM_POWER_OFF,
        SYSTEM_RESTART,
} system_state;

Is it really reasonable to add one more state here? I can certainly
draft something, but it looks like a lot of core kernel changes which I
believe have a very small chance of getting accepted.

I'm really trying to understand better what you suggest, and see how it
can work for our situation, but for now, it's not very clear to me how
that would work.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Andrew Lunn]

> > I'm making some assumptions here, which could be wrong.
> > 
> > #1 This GPIO interface does more than suspend to RAM power off. I
> > assume it also does full power off? It might also support Wake on LAN?
> 
> This I don't know, but we can probably assume it's the case.
> 
> > #2 Any code can call a notifier chain and pass parameters to that
> > chain?
> 
> Right. But for existing notifier chains, the existence, semantic and
> meaning of the parameters are already defined, and the gazillions users
> of that notifier chain in the kernel rely on those parameters to not
> change.

That is not really true. Lets start off with:

https://lkml.org/lkml/2014/10/21/56

This is brand new code, implementing the poweroff handler call chain.
Take a look at do_kernel_power_off(). It passes a NULL pointer as the
parameter to the handler function. So there are not gazillions users
of that notifier chain in the kernel rely on those parameters to not
change.

What i'm suggesting is to extend this new code and make it more
general. Currently it only handle full power off. Make it also handle
suspend to RAM power off as well.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Thomas Petazzoni]

Dear Andrew Lunn,

On Mon, 27 Oct 2014 15:59:39 +0100, Andrew Lunn wrote:

> > Right. But for existing notifier chains, the existence, semantic and
> > meaning of the parameters are already defined, and the gazillions users
> > of that notifier chain in the kernel rely on those parameters to not
> > change.
> 
> That is not really true. Lets start off with:
> 
> https://lkml.org/lkml/2014/10/21/56
> 
> This is brand new code, implementing the poweroff handler call chain.
> Take a look at do_kernel_power_off(). It passes a NULL pointer as the
> parameter to the handler function. So there are not gazillions users
> of that notifier chain in the kernel rely on those parameters to not
> change.

Ok now I understand why I didn't see it. This code is not in mainline.
And actually, it doesn't seem to be close from hitting mainline when
reading the reaction of Rafael Wysocki on the main patch:

"""
Well, I must admit to having second thoughts regarding this particular
mechanism.  Namely, notifiers don't seem to be the best way of
expressing what's needed from the design standpoint.
"""

So I'm a bit reluctant to create a dependency of the Armada XP
suspend/resume code to a very large unmerged patch series that doesn't
seem to even be close of having a consensus amongst the maintainers.

Isn't this something we can rework afterwards once the poweroff
discussion has settled? I wouldn't mind declaring the particular
aspects of the DT bindings related to the PIC GPIOs as "staging", so
that we keep the freedom to change them for a few kernel releases until
we settle on the final solution for that.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Andrew Lunn]

> So I'm a bit reluctant to create a dependency of the Armada XP
> suspend/resume code to a very large unmerged patch series that doesn't
> seem to even be close of having a consensus amongst the maintainers.

I totally agree.

> Isn't this something we can rework afterwards once the poweroff
> discussion has settled? I wouldn't mind declaring the particular
> aspects of the DT bindings related to the PIC GPIOs as "staging", so
> that we keep the freedom to change them for a few kernel releases until
> we settle on the final solution for that.

Yes. That seems very reasonable.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/17] ARM: mvebu: Armada XP GP specific suspend/resume code

[Gregory CLEMENT]

Hi Thomas,

[...]

> +#include "common.h"
> +
> +#define ARMADA_XP_GP_PIC_NR_GPIOS 3
> +
> +static void __iomem *gpio_ctrl;
> +static int pic_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
> +static int pic_raw_gpios[ARMADA_XP_GP_PIC_NR_GPIOS];
> +
> +static void mvebu_armada_xp_gp_pm_enter(void __iomem *sdram_reg, u32 srcmd)
> +{
> +	u32 reg, ackcmd;
> +	int i;
> +
> +	/* Put 001 as value on the GPIOs */
> +	reg = readl(gpio_ctrl);
> +	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
> +		reg &= ~BIT(pic_raw_gpios[i]);
> +	reg |= BIT(pic_raw_gpios[0]);
> +	writel(reg, gpio_ctrl);
> +
> +	/* Prepare writing 111 to the GPIOs */
> +	ackcmd = readl(gpio_ctrl);
> +	for (i = 0; i < ARMADA_XP_GP_PIC_NR_GPIOS; i++)
> +		ackcmd |= BIT(pic_raw_gpios[i]);
> +
> +	/* Wait a while */
> +	mdelay(250);
> +
> +	asm volatile (
> +		/* Align to a cache line */
> +		".balign 32\n\t"
> +
> +		/* Enter self refresh */
> +		"str %[srcmd], [%[sdram_reg]]\n\t"
> +
> +		/* Wait 100 cycles for DDR to enter self refresh */
> +		"1: subs r1, r1, #1\n\t"

I should miss something obvious, but I don't see where you load 100 in
the r1 register. According to your comment and the code, you remove 1 from
r1 until it reaches 0, so I expected that just before you have loaded 100 in r1.

Thanks,

Gregory

> +		"bne 1b\n\t"
> +
> +		/* Issue the command ACK */
> +		"str %[ackcmd], [%[gpio_ctrl]]\n\t"
> +
> +		/* Trap the processor */
> +		"b .\n\t"
> +		: : [srcmd] "r" (srcmd), [sdram_reg] "r" (sdram_reg),
> +		  [ackcmd] "r" (ackcmd), [gpio_ctrl] "r" (gpio_ctrl) : "r1");
> +}

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/17] ARM: mvebu: make sure MMU is disabled in armada_370_xp_cpu_resume

[Thomas Petazzoni]

The armada_370_xp_cpu_resume() until now was used only as the function
called by the SoC when returning from a deep idle state (as used in
cpuidle, or when the CPU is brought offline using CPU hotplug).

However, it is now also used when exiting the suspend to RAM state. In
this case, it is the bootloader that calls back into this function,
with the MMU left enabled by the BootROM. Having the MMU enabled when
entering this function confuses the kerrnel because we are not using
the kernel page tables at this point, but in other mvebu functions we
use the information on whether the MMU is enabled or not to find out
whether we should talk to the coherency fabric using a physical
address or a virtual address. To fix that, we simply disable the MMU
when entering this function, so that the kernel is in an expected
situation.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/pmsu_ll.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-mvebu/pmsu_ll.S b/arch/arm/mach-mvebu/pmsu_ll.S
index a945756..ae7e6b0 100644
--- a/arch/arm/mach-mvebu/pmsu_ll.S
+++ b/arch/arm/mach-mvebu/pmsu_ll.S
@@ -18,6 +18,14 @@
  */
 ENTRY(armada_370_xp_cpu_resume)
 ARM_BE8(setend	be )			@ go BE8 if entered LE
+	/*
+	 * Disable the MMU that might have been enabled in BootROM if
+	 * this code is used in the resume path of a suspend/resume
+	 * cycle.
+	 */
+	mrc	p15, 0, r1, c1, c0, 0
+	bic	r1, #1
+	mcr	p15, 0, r1, c1, c0, 0
 	bl	ll_add_cpu_to_smp_group
 	bl	ll_enable_coherency
 	b	cpu_resume
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/17] ARM: mvebu: make sure MMU is disabled in armada_370_xp_cpu_resume

[Gregory CLEMENT]

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> The armada_370_xp_cpu_resume() until now was used only as the function
> called by the SoC when returning from a deep idle state (as used in
> cpuidle, or when the CPU is brought offline using CPU hotplug).
> 
> However, it is now also used when exiting the suspend to RAM state. In
> this case, it is the bootloader that calls back into this function,
> with the MMU left enabled by the BootROM. Having the MMU enabled when
> entering this function confuses the kerrnel because we are not using
                                      kernel


> the kernel page tables at this point, but in other mvebu functions we
> use the information on whether the MMU is enabled or not to find out
> whether we should talk to the coherency fabric using a physical
> address or a virtual address. To fix that, we simply disable the MMU
> when entering this function, so that the kernel is in an expected
> situation.
> 


Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>



> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/mach-mvebu/pmsu_ll.S | 8 ++++++++
>  1 file changed, 8 insertions(+)


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/17] ARM: mvebu: synchronize secondary CPU clocks on resume

[Thomas Petazzoni]

The Armada XP has multiple cores clocked by independent clocks. The
SMP startup code contains a function called set_secondary_cpus_clock()
called in armada_xp_smp_prepare_cpus() to ensure the clocks of the
secondary CPUs match the clock of the boot CPU.

With the introduction of suspend/resume, this operation is no longer
needed when booting the system, but also when existing the suspend to
RAM state. Therefore this commit reworks a bit the logic: instead of
configuring the clock of all secondary CPUs in
armada_xp_smp_prepare_cpus(), we do it on a per-secondary CPU basis in
armada_xp_boot_secondary(), as this function gets called when existing
suspend to RAM for each secondary CPU.

Since the function now only takes care of one CPU, we rename it from
set_secondary_cpus_clock() to set_secondary_cpu_clock(), and it looses
its __init marker, as it is now used beyond the system initialization.

Note that we can't use smp_processor_id() directly, because when
exiting from suspend to RAM, the code is apparently executed with
preemption enabled, so smp_processor_id() is not happy (prints a
warning). We therefore switch to using get_cpu()/put_cpu(), even
though we pretty much have the guarantee that the code starting the
secondary CPUs is going to run on the boot CPU and will not be
migrated.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/platsmp.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index 895dc37..e65e69a 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -33,7 +33,7 @@
 #define AXP_BOOTROM_BASE 0xfff00000
 #define AXP_BOOTROM_SIZE 0x100000
 
-static struct clk *__init get_cpu_clk(int cpu)
+static struct clk *get_cpu_clk(int cpu)
 {
 	struct clk *cpu_clk;
 	struct device_node *np = of_get_cpu_node(cpu, NULL);
@@ -46,29 +46,28 @@ static struct clk *__init get_cpu_clk(int cpu)
 	return cpu_clk;
 }
 
-static void __init set_secondary_cpus_clock(void)
+static void set_secondary_cpu_clock(unsigned int cpu)
 {
-	int thiscpu, cpu;
+	int thiscpu;
 	unsigned long rate;
 	struct clk *cpu_clk;
 
-	thiscpu = smp_processor_id();
+	thiscpu = get_cpu();
+
 	cpu_clk = get_cpu_clk(thiscpu);
 	if (!cpu_clk)
-		return;
+		goto out;
 	clk_prepare_enable(cpu_clk);
 	rate = clk_get_rate(cpu_clk);
 
-	/* set all the other CPU clk to the same rate than the boot CPU */
-	for_each_possible_cpu(cpu) {
-		if (cpu == thiscpu)
-			continue;
-		cpu_clk = get_cpu_clk(cpu);
-		if (!cpu_clk)
-			return;
-		clk_set_rate(cpu_clk, rate);
-		clk_prepare_enable(cpu_clk);
-	}
+	cpu_clk = get_cpu_clk(cpu);
+	if (!cpu_clk)
+		goto out;
+	clk_set_rate(cpu_clk, rate);
+	clk_prepare_enable(cpu_clk);
+
+out:
+	put_cpu();
 }
 
 static int armada_xp_boot_secondary(unsigned int cpu, struct task_struct *idle)
@@ -78,6 +77,7 @@ static int armada_xp_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	pr_info("Booting CPU %d\n", cpu);
 
 	hw_cpu = cpu_logical_map(cpu);
+	set_secondary_cpu_clock(hw_cpu);
 	mvebu_pmsu_set_cpu_boot_addr(hw_cpu, armada_xp_secondary_startup);
 
 	/*
@@ -126,7 +126,6 @@ static void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 	struct resource res;
 	int err;
 
-	set_secondary_cpus_clock();
 	flush_cache_all();
 	set_cpu_coherent();
 
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] ARM: mvebu: synchronize secondary CPU clocks on resume

[Gregory CLEMENT]

Hi Thomas,
On 24/10/2014 13:59, Thomas Petazzoni wrote:
> The Armada XP has multiple cores clocked by independent clocks. The
> SMP startup code contains a function called set_secondary_cpus_clock()
> called in armada_xp_smp_prepare_cpus() to ensure the clocks of the
> secondary CPUs match the clock of the boot CPU.
> 
> With the introduction of suspend/resume, this operation is no longer
> needed when booting the system, but also when existing the suspend to
> RAM state. Therefore this commit reworks a bit the logic: instead of
> configuring the clock of all secondary CPUs in
> armada_xp_smp_prepare_cpus(), we do it on a per-secondary CPU basis in
> armada_xp_boot_secondary(), as this function gets called when existing
> suspend to RAM for each secondary CPU.
> 
> Since the function now only takes care of one CPU, we rename it from
> set_secondary_cpus_clock() to set_secondary_cpu_clock(), and it looses
> its __init marker, as it is now used beyond the system initialization.
> 
> Note that we can't use smp_processor_id() directly, because when
> exiting from suspend to RAM, the code is apparently executed with
> preemption enabled, so smp_processor_id() is not happy (prints a
> warning). We therefore switch to using get_cpu()/put_cpu(), even
> though we pretty much have the guarantee that the code starting the
> secondary CPUs is going to run on the boot CPU and will not be
> migrated.
> 

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/mach-mvebu/platsmp.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/17] ARM: mvebu: add suspend/resume DT information for Armada XP GP

[Thomas Petazzoni]

This commit improves the Armada XP GP Device Tree description to
describe the 3 GPIOs that are used to connect the SoC to the PIC
micro-controller that we talk to shutdown the SoC when entering
suspend to RAM.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

---
This probably needs some Device Tree binding documentation, but I'm
not sure where to put it
exactly. Documentation/devicetree/bindings/arm/armada-xp-gp-pic.txt ?

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-xp-gp.dts | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 0478c55..ea86736 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -23,6 +23,7 @@
  */
 
 /dts-v1/;
+#include <dt-bindings/gpio/gpio.h>
 #include "armada-xp-mv78460.dtsi"
 
 / {
@@ -48,6 +49,14 @@
 		      <0x00000001 0x00000000 0x00000001 0x00000000>;
 	};
 
+	cpus {
+		pm_pic {
+			ctrl-gpios = <&gpio0 16 GPIO_ACTIVE_LOW>,
+				     <&gpio0 17 GPIO_ACTIVE_LOW>,
+				     <&gpio0 18 GPIO_ACTIVE_LOW>;
+		};
+	};
+
 	soc {
 		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000
 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
@@ -115,7 +124,15 @@
 			serial@12300 {
 				status = "okay";
 			};
-
+			pinctrl {
+				pinctrl-0 = <&pic_pins>;
+				pinctrl-names = "default";
+				pic_pins: pic-pins-0 {
+					marvell,pins = "mpp16", "mpp17",
+						       "mpp18";
+					marvell,function = "gpio";
+				};
+			};
 			sata@a0000 {
 				nr-ports = <2>;
 				status = "okay";
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/17] ARM: mvebu: add suspend/resume DT information for Armada XP GP

[Gregory CLEMENT]

Hi Thomas,
On 24/10/2014 13:59, Thomas Petazzoni wrote:
> This commit improves the Armada XP GP Device Tree description to
> describe the 3 GPIOs that are used to connect the SoC to the PIC
> micro-controller that we talk to shutdown the SoC when entering
> suspend to RAM.
> 

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> ---
> This probably needs some Device Tree binding documentation, but I'm
> not sure where to put it
> exactly. Documentation/devicetree/bindings/arm/armada-xp-gp-pic.txt ?
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/armada-xp-gp.dts | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 16/17] ARM: mvebu: adjust mbus controller description on Armada 370/XP

[Thomas Petazzoni]

In order to support suspend/resume on Armada XP, an additional set of
registers need to be described at the MBus controller level. This
commit therefore adjusts the Device Tree of the Armada 370/XP SoC to
include those registers in the MBus controller description;

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 83286ec..90dba78 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -180,7 +180,8 @@
 
 			mbusc: mbus-controller@20000 {
 				compatible = "marvell,mbus-controller";
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>,
+				      <0x20250 0x8>;
 			};
 
 			mpic: interrupt-controller@20000 {
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/17] ARM: mvebu: adjust mbus controller description on Armada 370/XP

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> In order to support suspend/resume on Armada XP, an additional set of
> registers need to be described at the MBus controller level. This
> commit therefore adjusts the Device Tree of the Armada 370/XP SoC to
> include those registers in the MBus controller description;
> 

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 83286ec..90dba78 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -180,7 +180,8 @@
>  
>  			mbusc: mbus-controller@20000 {
>  				compatible = "marvell,mbus-controller";
> -				reg = <0x20000 0x100>, <0x20180 0x20>;
> +				reg = <0x20000 0x100>, <0x20180 0x20>,
> +				      <0x20250 0x8>;
>  			};
>  
>  			mpic: interrupt-controller@20000 {
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 17/17] ARM: mvebu: add SDRAM controller description for Armada XP

[Thomas Petazzoni]

The suspend/resume sequence on Armada XP needs to modify a number of
registers in the SDRAM controller. Therefore, this commit updates the
Armada XP Device Tree description to include the SDRAM controller
Device Tree node.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-xp.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index bff9f6c..2be244a 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -35,6 +35,11 @@
 		};
 
 		internal-regs {
+			sdramc@1400 {
+				compatible = "marvell,armada-xp-sdram-controller";
+				reg = <0x1400 0x500>;
+			};
+
 			L2: l2-cache {
 				compatible = "marvell,aurora-system-cache";
 				reg = <0x08000 0x1000>;
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 17/17] ARM: mvebu: add SDRAM controller description for Armada XP

[Gregory CLEMENT]

Hi Thomas,


On 24/10/2014 13:59, Thomas Petazzoni wrote:
> The suspend/resume sequence on Armada XP needs to modify a number of
> registers in the SDRAM controller. Therefore, this commit updates the
> Armada XP Device Tree description to include the SDRAM controller
> Device Tree node.
> 

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/armada-xp.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
> index bff9f6c..2be244a 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -35,6 +35,11 @@
>  		};
>  
>  		internal-regs {
> +			sdramc@1400 {
> +				compatible = "marvell,armada-xp-sdram-controller";
> +				reg = <0x1400 0x500>;

It seemed a quite large area, but indeed there are registers related to the sdram
controller at least until the address 0x18AC.

> +			};
> +
>  			L2: l2-cache {
>  				compatible = "marvell,aurora-system-cache";
>  				reg = <0x08000 0x1000>;
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/17] clocksource: time-armada-370-xp: add suspend/resume support

[Thomas Petazzoni]

This commit adds a set of suspend/resume syscore_ops to respectively
save and restore a number of timer registers, in order to make sure
the clockevent and clocksource devices continue to work properly
accross a suspend/resume cycle.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
---
 drivers/clocksource/time-armada-370-xp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
index 0451e62..1a32dcb 100644
--- a/drivers/clocksource/time-armada-370-xp.c
+++ b/drivers/clocksource/time-armada-370-xp.c
@@ -43,6 +43,7 @@
 #include <linux/module.h>
 #include <linux/sched_clock.h>
 #include <linux/percpu.h>
+#include <linux/syscore_ops.h>
 
 /*
  * Timer block registers.
@@ -223,6 +224,28 @@ static struct notifier_block armada_370_xp_timer_cpu_nb = {
 	.notifier_call = armada_370_xp_timer_cpu_notify,
 };
 
+static u32 timer_ctrl_reg, timer_local_ctrl_reg;
+
+static int armada_370_xp_timer_suspend(void)
+{
+	timer_ctrl_reg = readl(timer_base + TIMER_CTRL_OFF);
+	timer_local_ctrl_reg = readl(local_base + TIMER_CTRL_OFF);
+	return 0;
+}
+
+static void armada_370_xp_timer_resume(void)
+{
+	writel(0xffffffff, timer_base + TIMER0_VAL_OFF);
+	writel(0xffffffff, timer_base + TIMER0_RELOAD_OFF);
+	writel(timer_ctrl_reg, timer_base + TIMER_CTRL_OFF);
+	writel(timer_local_ctrl_reg, local_base + TIMER_CTRL_OFF);
+}
+
+struct syscore_ops armada_370_xp_timer_syscore_ops = {
+	.suspend	= armada_370_xp_timer_suspend,
+	.resume		= armada_370_xp_timer_resume,
+};
+
 static void __init armada_370_xp_timer_common_init(struct device_node *np)
 {
 	u32 clr = 0, set = 0;
@@ -285,6 +308,8 @@ static void __init armada_370_xp_timer_common_init(struct device_node *np)
 	/* Immediately configure the timer on the boot CPU */
 	if (!res)
 		armada_370_xp_timer_setup(this_cpu_ptr(armada_370_xp_evt));
+
+	register_syscore_ops(&armada_370_xp_timer_syscore_ops);
 }
 
 static void __init armada_xp_timer_init(struct device_node *np)
-- 
2.0.0

Re: [PATCH 05/17] clocksource: time-armada-370-xp: add suspend/resume support

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> This commit adds a set of suspend/resume syscore_ops to respectively
> save and restore a number of timer registers, in order to make sure
> the clockevent and clocksource devices continue to work properly
> accross a suspend/resume cycle.
  across

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/clocksource/time-armada-370-xp.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/clocksource/time-armada-370-xp.c b/drivers/clocksource/time-armada-370-xp.c
> index 0451e62..1a32dcb 100644
> --- a/drivers/clocksource/time-armada-370-xp.c
> +++ b/drivers/clocksource/time-armada-370-xp.c
> @@ -43,6 +43,7 @@
>  #include <linux/module.h>
>  #include <linux/sched_clock.h>
>  #include <linux/percpu.h>
> +#include <linux/syscore_ops.h>
>  
>  /*
>   * Timer block registers.
> @@ -223,6 +224,28 @@ static struct notifier_block armada_370_xp_timer_cpu_nb = {
>  	.notifier_call = armada_370_xp_timer_cpu_notify,
>  };
>  
> +static u32 timer_ctrl_reg, timer_local_ctrl_reg;

We don't need to restore any other timer than the timer 0, but it
would worth mentioning that we save and restore only this one
by naming the variable timer0_ctrl_reg and timer0_local_ctrl_reg

Besides this and the typo:

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/17] gpio: mvebu: add suspend/resume support

[Thomas Petazzoni]

This commit adds the implementation of ->suspend() and ->resume()
platform_driver hooks in order to save and restore the state of the
GPIO configuration. In order to achieve that, additional fields are
added to the mvebu_gpio_chip structure.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
---
 drivers/gpio/gpio-mvebu.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 418e386..dd5545c 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -83,6 +83,14 @@ struct mvebu_gpio_chip {
 	int		   irqbase;
 	struct irq_domain *domain;
 	int                soc_variant;
+
+	/* Used to preserve GPIO registers accross suspend/resume */
+	u32                out_reg;
+	u32                io_conf_reg;
+	u32                blink_en_reg;
+	u32                in_pol_reg;
+	u32                edge_mask_regs[4];
+	u32                level_mask_regs[4];
 };
 
 /*
@@ -554,6 +562,93 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mvebu_gpio_of_match);
 
+static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
+	int i;
+
+	mvchip->out_reg = readl(mvebu_gpioreg_out(mvchip));
+	mvchip->io_conf_reg = readl(mvebu_gpioreg_io_conf(mvchip));
+	mvchip->blink_en_reg = readl(mvebu_gpioreg_blink(mvchip));
+	mvchip->in_pol_reg = readl(mvebu_gpioreg_in_pol(mvchip));
+
+	switch (mvchip->soc_variant) {
+	case MVEBU_GPIO_SOC_VARIANT_ORION:
+		mvchip->edge_mask_regs[0] =
+			readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
+		mvchip->level_mask_regs[0] =
+			readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
+		break;
+	case MVEBU_GPIO_SOC_VARIANT_MV78200:
+		for (i = 0; i < 2; i++) {
+			mvchip->edge_mask_regs[i] =
+				readl(mvchip->membase +
+				      GPIO_EDGE_MASK_MV78200_OFF(i));
+			mvchip->level_mask_regs[i] =
+				readl(mvchip->membase +
+				      GPIO_LEVEL_MASK_MV78200_OFF(i));
+		}
+		break;
+	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
+		for (i = 0; i < 4; i++) {
+			mvchip->edge_mask_regs[i] =
+				readl(mvchip->membase +
+				      GPIO_EDGE_MASK_ARMADAXP_OFF(i));
+			mvchip->level_mask_regs[i] =
+				readl(mvchip->membase +
+				      GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
+		}
+		break;
+	default:
+		BUG();
+	}
+
+	return 0;
+}
+
+static int mvebu_gpio_resume(struct platform_device *pdev)
+{
+	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
+	int i;
+
+	writel(mvchip->out_reg, mvebu_gpioreg_out(mvchip));
+	writel(mvchip->io_conf_reg, mvebu_gpioreg_io_conf(mvchip));
+	writel(mvchip->blink_en_reg, mvebu_gpioreg_blink(mvchip));
+	writel(mvchip->in_pol_reg, mvebu_gpioreg_in_pol(mvchip));
+
+	switch (mvchip->soc_variant) {
+	case MVEBU_GPIO_SOC_VARIANT_ORION:
+		writel(mvchip->edge_mask_regs[0],
+		       mvchip->membase + GPIO_EDGE_MASK_OFF);
+		writel(mvchip->level_mask_regs[0],
+		       mvchip->membase + GPIO_LEVEL_MASK_OFF);
+		break;
+	case MVEBU_GPIO_SOC_VARIANT_MV78200:
+		for (i = 0; i < 2; i++) {
+			writel(mvchip->edge_mask_regs[i],
+			       mvchip->membase + GPIO_EDGE_MASK_MV78200_OFF(i));
+			writel(mvchip->level_mask_regs[i],
+			       mvchip->membase +
+			       GPIO_LEVEL_MASK_MV78200_OFF(i));
+		}
+		break;
+	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
+		for (i = 0; i < 4; i++) {
+			writel(mvchip->edge_mask_regs[i],
+			       mvchip->membase +
+			       GPIO_EDGE_MASK_ARMADAXP_OFF(i));
+			writel(mvchip->level_mask_regs[i],
+			       mvchip->membase +
+			       GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
+		}
+		break;
+	default:
+		BUG();
+	}
+
+	return 0;
+}
+
 static int mvebu_gpio_probe(struct platform_device *pdev)
 {
 	struct mvebu_gpio_chip *mvchip;
@@ -577,6 +672,8 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	if (!mvchip)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, mvchip);
+
 	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
 		dev_err(&pdev->dev, "Missing ngpios OF property\n");
 		return -ENODEV;
@@ -735,5 +832,7 @@ static struct platform_driver mvebu_gpio_driver = {
 		.of_match_table = mvebu_gpio_of_match,
 	},
 	.probe		= mvebu_gpio_probe,
+	.suspend        = mvebu_gpio_suspend,
+	.resume         = mvebu_gpio_resume,
 };
 module_platform_driver(mvebu_gpio_driver);
-- 
2.0.0

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[David Cohen]

On Fri, Oct 24, 2014 at 01:59:19PM +0200, Thomas Petazzoni wrote:
> This commit adds the implementation of ->suspend() and ->resume()
> platform_driver hooks in order to save and restore the state of the
> GPIO configuration. In order to achieve that, additional fields are
> added to the mvebu_gpio_chip structure.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org
> ---
>  drivers/gpio/gpio-mvebu.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 418e386..dd5545c 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -83,6 +83,14 @@ struct mvebu_gpio_chip {
>  	int		   irqbase;
>  	struct irq_domain *domain;
>  	int                soc_variant;
> +
> +	/* Used to preserve GPIO registers accross suspend/resume */
> +	u32                out_reg;
> +	u32                io_conf_reg;
> +	u32                blink_en_reg;
> +	u32                in_pol_reg;
> +	u32                edge_mask_regs[4];
> +	u32                level_mask_regs[4];
>  };
>  
>  /*
> @@ -554,6 +562,93 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mvebu_gpio_of_match);
>  
> +static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
> +	int i;
> +
> +	mvchip->out_reg = readl(mvebu_gpioreg_out(mvchip));
> +	mvchip->io_conf_reg = readl(mvebu_gpioreg_io_conf(mvchip));
> +	mvchip->blink_en_reg = readl(mvebu_gpioreg_blink(mvchip));
> +	mvchip->in_pol_reg = readl(mvebu_gpioreg_in_pol(mvchip));
> +
> +	switch (mvchip->soc_variant) {
> +	case MVEBU_GPIO_SOC_VARIANT_ORION:
> +		mvchip->edge_mask_regs[0] =
> +			readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> +		mvchip->level_mask_regs[0] =
> +			readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> +		break;
> +	case MVEBU_GPIO_SOC_VARIANT_MV78200:
> +		for (i = 0; i < 2; i++) {
> +			mvchip->edge_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_EDGE_MASK_MV78200_OFF(i));
> +			mvchip->level_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_LEVEL_MASK_MV78200_OFF(i));
> +		}
> +		break;
> +	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
> +		for (i = 0; i < 4; i++) {
> +			mvchip->edge_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_EDGE_MASK_ARMADAXP_OFF(i));
> +			mvchip->level_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
> +		}
> +		break;
> +	default:
> +		BUG();

Isn't it too severe? Is the platform going too unstable if driver
reaches this case?
I'd consider a WARN() instead.

> +	}
> +
> +	return 0;
> +}
> +
> +static int mvebu_gpio_resume(struct platform_device *pdev)
> +{
> +	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
> +	int i;
> +
> +	writel(mvchip->out_reg, mvebu_gpioreg_out(mvchip));
> +	writel(mvchip->io_conf_reg, mvebu_gpioreg_io_conf(mvchip));
> +	writel(mvchip->blink_en_reg, mvebu_gpioreg_blink(mvchip));
> +	writel(mvchip->in_pol_reg, mvebu_gpioreg_in_pol(mvchip));
> +
> +	switch (mvchip->soc_variant) {
> +	case MVEBU_GPIO_SOC_VARIANT_ORION:
> +		writel(mvchip->edge_mask_regs[0],
> +		       mvchip->membase + GPIO_EDGE_MASK_OFF);
> +		writel(mvchip->level_mask_regs[0],
> +		       mvchip->membase + GPIO_LEVEL_MASK_OFF);
> +		break;
> +	case MVEBU_GPIO_SOC_VARIANT_MV78200:
> +		for (i = 0; i < 2; i++) {
> +			writel(mvchip->edge_mask_regs[i],
> +			       mvchip->membase + GPIO_EDGE_MASK_MV78200_OFF(i));
> +			writel(mvchip->level_mask_regs[i],
> +			       mvchip->membase +
> +			       GPIO_LEVEL_MASK_MV78200_OFF(i));
> +		}
> +		break;
> +	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
> +		for (i = 0; i < 4; i++) {
> +			writel(mvchip->edge_mask_regs[i],
> +			       mvchip->membase +
> +			       GPIO_EDGE_MASK_ARMADAXP_OFF(i));
> +			writel(mvchip->level_mask_regs[i],
> +			       mvchip->membase +
> +			       GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
> +		}
> +		break;
> +	default:
> +		BUG();

Ditto.

Br, David Cohen

> +	}
> +
> +	return 0;
> +}
> +
>  static int mvebu_gpio_probe(struct platform_device *pdev)
>  {
>  	struct mvebu_gpio_chip *mvchip;
> @@ -577,6 +672,8 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	if (!mvchip)
>  		return -ENOMEM;
>  
> +	platform_set_drvdata(pdev, mvchip);
> +
>  	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
>  		dev_err(&pdev->dev, "Missing ngpios OF property\n");
>  		return -ENODEV;
> @@ -735,5 +832,7 @@ static struct platform_driver mvebu_gpio_driver = {
>  		.of_match_table = mvebu_gpio_of_match,
>  	},
>  	.probe		= mvebu_gpio_probe,
> +	.suspend        = mvebu_gpio_suspend,
> +	.resume         = mvebu_gpio_resume,
>  };
>  module_platform_driver(mvebu_gpio_driver);
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Andrew Lunn]

> > +	switch (mvchip->soc_variant) {
> > +	case MVEBU_GPIO_SOC_VARIANT_ORION:
> > +		mvchip->edge_mask_regs[0] =
> > +			readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> > +		mvchip->level_mask_regs[0] =
> > +			readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> > +		break;
> > +	case MVEBU_GPIO_SOC_VARIANT_MV78200:
> > +		for (i = 0; i < 2; i++) {
> > +			mvchip->edge_mask_regs[i] =
> > +				readl(mvchip->membase +
> > +				      GPIO_EDGE_MASK_MV78200_OFF(i));
> > +			mvchip->level_mask_regs[i] =
> > +				readl(mvchip->membase +
> > +				      GPIO_LEVEL_MASK_MV78200_OFF(i));
> > +		}
> > +		break;
> > +	case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
> > +		for (i = 0; i < 4; i++) {
> > +			mvchip->edge_mask_regs[i] =
> > +				readl(mvchip->membase +
> > +				      GPIO_EDGE_MASK_ARMADAXP_OFF(i));
> > +			mvchip->level_mask_regs[i] =
> > +				readl(mvchip->membase +
> > +				      GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
> > +		}
> > +		break;
> > +	default:
> > +		BUG();
> 
> Isn't it too severe? Is the platform going too unstable if driver
> reaches this case?
> I'd consider a WARN() instead.

This is a common pattern in this driver. So i guess Thomas just
cut/pasted the switch statement from _probe(), which also has the
BUG().

Given that _probe() should of thrown a BUG() in this situation, if it
happens here, it means mvchip->soc_variant has been corrupted, and so
bad things are happening. So a BUG() is maybe called for?

	 Andrew

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Alexandre Courbot]

On Sat, Oct 25, 2014 at 5:45 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > +   switch (mvchip->soc_variant) {
>> > +   case MVEBU_GPIO_SOC_VARIANT_ORION:
>> > +           mvchip->edge_mask_regs[0] =
>> > +                   readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
>> > +           mvchip->level_mask_regs[0] =
>> > +                   readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
>> > +           break;
>> > +   case MVEBU_GPIO_SOC_VARIANT_MV78200:
>> > +           for (i = 0; i < 2; i++) {
>> > +                   mvchip->edge_mask_regs[i] =
>> > +                           readl(mvchip->membase +
>> > +                                 GPIO_EDGE_MASK_MV78200_OFF(i));
>> > +                   mvchip->level_mask_regs[i] =
>> > +                           readl(mvchip->membase +
>> > +                                 GPIO_LEVEL_MASK_MV78200_OFF(i));
>> > +           }
>> > +           break;
>> > +   case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
>> > +           for (i = 0; i < 4; i++) {
>> > +                   mvchip->edge_mask_regs[i] =
>> > +                           readl(mvchip->membase +
>> > +                                 GPIO_EDGE_MASK_ARMADAXP_OFF(i));
>> > +                   mvchip->level_mask_regs[i] =
>> > +                           readl(mvchip->membase +
>> > +                                 GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
>> > +           }
>> > +           break;
>> > +   default:
>> > +           BUG();
>>
>> Isn't it too severe? Is the platform going too unstable if driver
>> reaches this case?
>> I'd consider a WARN() instead.
>
> This is a common pattern in this driver. So i guess Thomas just
> cut/pasted the switch statement from _probe(), which also has the
> BUG().
>
> Given that _probe() should of thrown a BUG() in this situation, if it
> happens here, it means mvchip->soc_variant has been corrupted, and so
> bad things are happening. So a BUG() is maybe called for?

I agree that BUG() is adequate here. probe() should recognize the
exact same set of chips - if we reach this point this means that
either the data has been corrupted or we added support for a new chip
in probe() and forgot suspend/resume. In both cases the driver should
express its discontent.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[David Cohen]

On Mon, Oct 27, 2014 at 02:27:16PM +0900, Alexandre Courbot wrote:
> On Sat, Oct 25, 2014 at 5:45 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> >> > +   switch (mvchip->soc_variant) {
> >> > +   case MVEBU_GPIO_SOC_VARIANT_ORION:
> >> > +           mvchip->edge_mask_regs[0] =
> >> > +                   readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> >> > +           mvchip->level_mask_regs[0] =
> >> > +                   readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> >> > +           break;
> >> > +   case MVEBU_GPIO_SOC_VARIANT_MV78200:
> >> > +           for (i = 0; i < 2; i++) {
> >> > +                   mvchip->edge_mask_regs[i] =
> >> > +                           readl(mvchip->membase +
> >> > +                                 GPIO_EDGE_MASK_MV78200_OFF(i));
> >> > +                   mvchip->level_mask_regs[i] =
> >> > +                           readl(mvchip->membase +
> >> > +                                 GPIO_LEVEL_MASK_MV78200_OFF(i));
> >> > +           }
> >> > +           break;
> >> > +   case MVEBU_GPIO_SOC_VARIANT_ARMADAXP:
> >> > +           for (i = 0; i < 4; i++) {
> >> > +                   mvchip->edge_mask_regs[i] =
> >> > +                           readl(mvchip->membase +
> >> > +                                 GPIO_EDGE_MASK_ARMADAXP_OFF(i));
> >> > +                   mvchip->level_mask_regs[i] =
> >> > +                           readl(mvchip->membase +
> >> > +                                 GPIO_LEVEL_MASK_ARMADAXP_OFF(i));
> >> > +           }
> >> > +           break;
> >> > +   default:
> >> > +           BUG();
> >>
> >> Isn't it too severe? Is the platform going too unstable if driver
> >> reaches this case?
> >> I'd consider a WARN() instead.
> >
> > This is a common pattern in this driver. So i guess Thomas just
> > cut/pasted the switch statement from _probe(), which also has the
> > BUG().
> >
> > Given that _probe() should of thrown a BUG() in this situation, if it
> > happens here, it means mvchip->soc_variant has been corrupted, and so
> > bad things are happening. So a BUG() is maybe called for?
> 
> I agree that BUG() is adequate here. probe() should recognize the
> exact same set of chips - if we reach this point this means that
> either the data has been corrupted or we added support for a new chip
> in probe() and forgot suspend/resume. In both cases the driver should
> express its discontent.

Just for the records, since I don't know this platform very well :)

IMHO unless this issue is the source of a serious instability or data
corruption, a WARN() would be a better way for the driver express its
discontent. It's way better to have a functional platform for further
debugging.

This driver can also be compiled as a module. I wonder if it's a good
behavior boot the platform and then crash the kernel when loading the
module driver.

But anyway, that would be just me.

Br, David Cohen

> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Linus Walleij]

On Fri, Oct 24, 2014 at 1:59 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> This commit adds the implementation of ->suspend() and ->resume()
> platform_driver hooks in order to save and restore the state of the
> GPIO configuration. In order to achieve that, additional fields are
> added to the mvebu_gpio_chip structure.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org

(...)
> +       mvchip->out_reg = readl(mvebu_gpioreg_out(mvchip));
> +       mvchip->io_conf_reg = readl(mvebu_gpioreg_io_conf(mvchip));
> +       mvchip->blink_en_reg = readl(mvebu_gpioreg_blink(mvchip));
> +       mvchip->in_pol_reg = readl(mvebu_gpioreg_in_pol(mvchip));

OK...

> +       switch (mvchip->soc_variant) {
> +       case MVEBU_GPIO_SOC_VARIANT_ORION:
> +               mvchip->edge_mask_regs[0] =
> +                       readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> +               mvchip->level_mask_regs[0] =
> +                       readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> +               break;

You are assigning index [0] twice, why? There must be some
reason, and it should be stated in a comment. If the first read
is necessary for hardware reasons, don't assign it but
discard the result.

(void) readl(...);

(This pattern repeats for each save call below.)

> +       switch (mvchip->soc_variant) {
> +       case MVEBU_GPIO_SOC_VARIANT_ORION:
> +               writel(mvchip->edge_mask_regs[0],
> +                      mvchip->membase + GPIO_EDGE_MASK_OFF);
> +               writel(mvchip->level_mask_regs[0],
> +                      mvchip->membase + GPIO_LEVEL_MASK_OFF);

And on the way up same thing. Now you write
each register twice. Why?

Yours,
Linus Walleij

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Gregory CLEMENT]

Hi Linus,


> 
>> +       switch (mvchip->soc_variant) {
>> +       case MVEBU_GPIO_SOC_VARIANT_ORION:
>> +               mvchip->edge_mask_regs[0] =
>> +                       readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
>> +               mvchip->level_mask_regs[0] =
>> +                       readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
>> +               break;
> 
> You are assigning index [0] twice, why? There must be some
> reason, and it should be stated in a comment. If the first read
> is necessary for hardware reasons, don't assign it but
> discard the result.

Maybe I missed something but for me these 2 registers are different:
one is the _EDGE_ mask and the other the _LEVEL_ mask.

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Thomas Petazzoni]

Hello,

On Fri, 31 Oct 2014 08:52:15 +0100, Gregory CLEMENT wrote:

> >> +       switch (mvchip->soc_variant) {
> >> +       case MVEBU_GPIO_SOC_VARIANT_ORION:
> >> +               mvchip->edge_mask_regs[0] =
> >> +                       readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> >> +               mvchip->level_mask_regs[0] =
> >> +                       readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> >> +               break;
> > 
> > You are assigning index [0] twice, why? There must be some
> > reason, and it should be stated in a comment. If the first read
> > is necessary for hardware reasons, don't assign it but
> > discard the result.
> 
> Maybe I missed something but for me these 2 registers are different:
> one is the _EDGE_ mask and the other the _LEVEL_ mask.

Good that you looked at it Greg because I must admit I did not
understand the comment from Linus :-)

I read things again and both the variable *and* the register offset are
different. I guess it's probably due to a -ENOTENOUGHCOFFEE :-)

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Linus Walleij]

On Fri, Oct 31, 2014 at 8:52 AM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Linus,
>>> +       switch (mvchip->soc_variant) {
>>> +       case MVEBU_GPIO_SOC_VARIANT_ORION:
>>> +               mvchip->edge_mask_regs[0] =
>>> +                       readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
>>> +               mvchip->level_mask_regs[0] =
>>> +                       readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
>>> +               break;
>>
>> You are assigning index [0] twice, why? There must be some
>> reason, and it should be stated in a comment. If the first read
>> is necessary for hardware reasons, don't assign it but
>> discard the result.
>
> Maybe I missed something but for me these 2 registers are different:
> one is the _EDGE_ mask and the other the _LEVEL_ mask.

Yeah haha I must have had a bad perception day :D

Yours,
Linus Walleij

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Linus Walleij]

On Fri, Oct 24, 2014 at 1:59 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> This commit adds the implementation of ->suspend() and ->resume()
> platform_driver hooks in order to save and restore the state of the
> GPIO configuration. In order to achieve that, additional fields are
> added to the mvebu_gpio_chip structure.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org

Patch applied with Alexandre's ACK.

Yours,
Linus Walleij

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Gregory CLEMENT]

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> This commit adds the implementation of ->suspend() and ->resume()
> platform_driver hooks in order to save and restore the state of the
> GPIO configuration. In order to achieve that, additional fields are
> added to the mvebu_gpio_chip structure.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org
> ---
>  drivers/gpio/gpio-mvebu.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 418e386..dd5545c 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -83,6 +83,14 @@ struct mvebu_gpio_chip {
>  	int		   irqbase;
>  	struct irq_domain *domain;
>  	int                soc_variant;
> +
> +	/* Used to preserve GPIO registers accross suspend/resume */
                                           across

> +	u32                out_reg;
> +	u32                io_conf_reg;
> +	u32                blink_en_reg;
> +	u32                in_pol_reg;
> +	u32                edge_mask_regs[4];
> +	u32                level_mask_regs[4];
>  };
>  
>  /*
> @@ -554,6 +562,93 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mvebu_gpio_of_match);
>  
> +static int mvebu_gpio_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct mvebu_gpio_chip *mvchip = platform_get_drvdata(pdev);
> +	int i;
> +
> +	mvchip->out_reg = readl(mvebu_gpioreg_out(mvchip));
> +	mvchip->io_conf_reg = readl(mvebu_gpioreg_io_conf(mvchip));
> +	mvchip->blink_en_reg = readl(mvebu_gpioreg_blink(mvchip));
> +	mvchip->in_pol_reg = readl(mvebu_gpioreg_in_pol(mvchip));
> +
> +	switch (mvchip->soc_variant) {
> +	case MVEBU_GPIO_SOC_VARIANT_ORION:
> +		mvchip->edge_mask_regs[0] =
> +			readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> +		mvchip->level_mask_regs[0] =
> +			readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> +		break;
> +	case MVEBU_GPIO_SOC_VARIANT_MV78200:
> +		for (i = 0; i < 2; i++) {
> +			mvchip->edge_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_EDGE_MASK_MV78200_OFF(i));
> +			mvchip->level_mask_regs[i] =
> +				readl(mvchip->membase +
> +				      GPIO_LEVEL_MASK_MV78200_OFF(i));
> +		}
> +		break;

I don't know if the suspend is supported on this platform but as we were
on the road to remove this platform I don't know it it worth testing it unless
some interested users show up.

I didn't tested it on a mv782xx platform but the patch looks correct:

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 06/17] gpio: mvebu: add suspend/resume support

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Mon, 03 Nov 2014 18:53:29 +0100, Gregory CLEMENT wrote:

> I don't know if the suspend is supported on this platform but as we were
> on the road to remove this platform I don't know it it worth testing it unless
> some interested users show up.

The same can be said of the entire mv78xx0 support in gpio-mvebu: it's
completely unused today. Since the migration to mv78xx0 has not been
started, this platform is still using the old style GPIO driver in
plat-orion/gpio.c. And therefore, all the mv78xx0 specific code in
gpio-mvebu.c is purely "tentative" and has never been actually tested
on HW.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 09/17] clk: mvebu: add suspend/resume for gatable clocks

[Thomas Petazzoni]

This commit adds suspend/resume support for the gatable clock driver
used on Marvell EBU platforms. When getting out of suspend, the
Marvell EBU platforms go through the bootloader, which re-enables all
gatable clocks. However, upon resume, the clock framework will not
disable again all gatable clocks that are not used.

Therefore, if the clock driver does not save/restore the state of the
gatable clocks, all gatable clocks that are not claimed by any device
driver will remain enabled after a resume. This is why this driver
saves and restores the state of those clocks.

Since clocks aren't real devices, we don't have the normal ->suspend()
and ->resume() of the device model, and have to use the ->suspend()
and ->resume() hooks of the syscore_ops mechanism. This mechanism has
the unfortunate idea of not providing a way of passing private data,
which requires us to change the driver to make the assumption that
there is only once instance of the gatable clock control structure.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/mvebu/common.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index b7fcb46..8799fb8 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/syscore_ops.h>
 
 #include "common.h"
 
@@ -177,14 +178,18 @@ struct clk_gating_ctrl {
 	spinlock_t *lock;
 	struct clk **gates;
 	int num_gates;
+	struct syscore_ops syscore_ops;
+	void __iomem *base;
+	u32 saved_reg;
 };
 
 #define to_clk_gate(_hw) container_of(_hw, struct clk_gate, hw)
 
+static struct clk_gating_ctrl *ctrl;
+
 static struct clk *clk_gating_get_src(
 	struct of_phandle_args *clkspec, void *data)
 {
-	struct clk_gating_ctrl *ctrl = (struct clk_gating_ctrl *)data;
 	int n;
 
 	if (clkspec->args_count < 1)
@@ -199,15 +204,30 @@ static struct clk *clk_gating_get_src(
 	return ERR_PTR(-ENODEV);
 }
 
+static int mvebu_clk_gating_suspend(void)
+{
+	ctrl->saved_reg = readl(ctrl->base);
+	return 0;
+}
+
+static void mvebu_clk_gating_resume(void)
+{
+	writel(ctrl->saved_reg, ctrl->base);
+}
+
 void __init mvebu_clk_gating_setup(struct device_node *np,
 				   const struct clk_gating_soc_desc *desc)
 {
-	struct clk_gating_ctrl *ctrl;
 	struct clk *clk;
 	void __iomem *base;
 	const char *default_parent = NULL;
 	int n;
 
+	if (ctrl) {
+		pr_err("mvebu-clk-gating: cannot instantiate more than one gatable clock device\n");
+		return;
+	}
+
 	base = of_iomap(np, 0);
 	if (WARN_ON(!base))
 		return;
@@ -225,6 +245,10 @@ void __init mvebu_clk_gating_setup(struct device_node *np,
 	/* lock must already be initialized */
 	ctrl->lock = &ctrl_gating_lock;
 
+	ctrl->base = base;
+	ctrl->syscore_ops.suspend = mvebu_clk_gating_suspend;
+	ctrl->syscore_ops.resume = mvebu_clk_gating_resume;
+
 	/* Count, allocate, and register clock gates */
 	for (n = 0; desc[n].name;)
 		n++;
@@ -246,6 +270,8 @@ void __init mvebu_clk_gating_setup(struct device_node *np,
 
 	of_clk_add_provider(np, clk_gating_get_src, ctrl);
 
+	register_syscore_ops(&ctrl->syscore_ops);
+
 	return;
 gates_out:
 	kfree(ctrl);
-- 
2.0.0

Re: [PATCH 09/17] clk: mvebu: add suspend/resume for gatable clocks

[Gregory CLEMENT]

Hi Thomas,

On 24/10/2014 13:59, Thomas Petazzoni wrote:
> This commit adds suspend/resume support for the gatable clock driver
> used on Marvell EBU platforms. When getting out of suspend, the
> Marvell EBU platforms go through the bootloader, which re-enables all
> gatable clocks. However, upon resume, the clock framework will not
> disable again all gatable clocks that are not used.
> 
> Therefore, if the clock driver does not save/restore the state of the
> gatable clocks, all gatable clocks that are not claimed by any device
> driver will remain enabled after a resume. This is why this driver
> saves and restores the state of those clocks.
> 
> Since clocks aren't real devices, we don't have the normal ->suspend()
> and ->resume() of the device model, and have to use the ->suspend()
> and ->resume() hooks of the syscore_ops mechanism. This mechanism has
> the unfortunate idea of not providing a way of passing private data,
> which requires us to change the driver to make the assumption that
> there is only once instance of the gatable clock control structure.

It should be possible to store more than one instance in an array, and
to go trough this array during suspend and resume. However until now we
never had needed more than one instance, so I agree to keep the code simple
with this assumption.

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/clk/mvebu/common.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 

Acked-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html