Suspend to RAM support for Armada XP

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

Cc devicetree at vger.kernel.org

Cc Kumar Gala <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Rob Herring <[email protected]>

---
 .../memory-controllers/mvebu-sdram-controller.txt   | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt

Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt new file +21 -0

new file mode 100644
index 0000000..89657d1
@@ -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 at 1400 {
+	compatible = "marvell,armada-xp-sdram-controller";
+	reg = <0x1400 0x500>;
+};

From: Nadav Haklai <[email protected]>

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

Cc <[email protected]> # v3.8+

Fixes de4901933f6d ("arm: mm: Add support for PJ4B cpu and init routines")

Signed-off-by Nadav Haklai <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

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

---
 arch/arm/mm/proc-v7.S | 2 --
 1 file changed, 2 deletions(-)

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

Cc Thomas Gleixner <[email protected]>

Cc Jason Cooper <[email protected]>

Cc linux-kernel at vger.kernel.org

---
 drivers/irqchip/irq-armada-370-xp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Cc Thomas Gleixner <[email protected]>

Cc Jason Cooper <[email protected]>

Cc linux-kernel at vger.kernel.org

---
 drivers/irqchip/irq-armada-370-xp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.

Quoted 5 lines

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?

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

Cc Daniel Lezcano <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc linux-kernel at vger.kernel.org

---
 drivers/clocksource/time-armada-370-xp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

drivers/clocksource/time-armada-370-xp.c +1 -0

index 0451e62..1a32dcb 100644
@@ -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.

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

Cc Linus Walleij <[email protected]>

Cc Alexandre Courbot <[email protected]>

Cc [email protected]

---
 drivers/gpio/gpio-mvebu.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

drivers/gpio/gpio-mvebu.c +37 -0

index 418e386..dd5545c 100644
@@ -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++) {

drivers/gpio/gpio-mvebu.c +33 -0

+	}
+
+	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++) {

drivers/gpio/gpio-mvebu.c +9 -0

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

Signature · 6 lines

2.0.0

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 25 lines

drivers/gpio/gpio-mvebu.c +25 -0

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

drivers/gpio/gpio-mvebu.c static const struct of_device_id mvebu_gpio_of_match[] = { · line 603

Quoted 4 lines

drivers/gpio/gpio-mvebu.c +4 -0

+		}
+		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.

Quoted 30 lines

Quoted 19 lines

drivers/gpio/gpio-mvebu.c +19 -0

+   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++) {

drivers/gpio/gpio-mvebu.c static const struct of_device_id mvebu_gpio_of_match[] = { · line 603

Quoted 10 lines

drivers/gpio/gpio-mvebu.c +10 -0

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

Quoted 3 lines

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?

Quoted 34 lines · Andrew Lunn

Quoted 30 lines

Quoted 19 lines

drivers/gpio/gpio-mvebu.c +19 -0

+   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++) {

drivers/gpio/gpio-mvebu.c static const struct of_device_id mvebu_gpio_of_match[] = { · line 603

Quoted 10 lines

drivers/gpio/gpio-mvebu.c +10 -0

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

Quoted 3 lines

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

Quoted 7 lines

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

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

Cc Linus Walleij <[email protected]>

Cc Alexandre Courbot <[email protected]>

Cc linux-gpio at vger.kernel.org

drivers/gpio/gpio-mvebu.c static const struct of_device_id mvebu_gpio_of_match[] = { · line 581

Quoted 8 lines

drivers/gpio/gpio-mvebu.c +7 -0

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

Quoted 8 lines

Quoted 7 lines

drivers/gpio/gpio-mvebu.c +7 -0

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

Quoted 4 lines

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.

Hi Linus,

Quoted 8 lines

Quoted 7 lines

drivers/gpio/gpio-mvebu.c +7 -0

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

Quoted 4 lines

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.

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

Cc Linus Walleij <[email protected]>

Cc Alexandre Courbot <[email protected]>

Cc linux-gpio at vger.kernel.org

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

Cc Linus Walleij <[email protected]>

Cc Alexandre Courbot <[email protected]>

Cc linux-gpio at vger.kernel.org

---
 drivers/gpio/gpio-mvebu.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

drivers/gpio/gpio-mvebu.c +0 -0

index 418e386..dd5545c 100644

drivers/gpio/gpio-mvebu.c +23 -0

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

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.

drivers/bus/mvebu-mbus.c +15 -0

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

drivers/bus/mvebu-mbus.c line 130

Quoted 4 lines

struct mvebu_mbus_state {

drivers/bus/mvebu-mbus.c +1 -0

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

[...]

Quoted 20 lines

drivers/bus/mvebu-mbus.c +20 -0

+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) {

drivers/bus/mvebu-mbus.c static __init int mvebu_mbus_debugfs_init(void) · line 751

Quoted 5 lines

drivers/bus/mvebu-mbus.c +5 -0

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

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

---
 drivers/bus/mvebu-mbus.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mbus.h     |  1 +
 2 files changed, 57 insertions(+)

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

Cc Mike Turquette <[email protected]>

Cc linux-kernel at vger.kernel.org

---
 drivers/clk/mvebu/common.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

arch/arm/mach-mvebu/pm.c line 160

Quoted 7 lines

arch/arm/mach-mvebu/pm.c new file +7 -0

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

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

---
 arch/arm/mach-mvebu/board-v7.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

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

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

arch/arm/mach-mvebu/Makefile +1 -1

index e2bd96f..b4f0149 100644
@@ -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
 

arch/arm/mach-mvebu/pm-board.c new file +134 -0

new file mode 100644
index 0000000..e1a9792
@@ -0,0 +1,134 @@
+/*
+ * Board-level suspend/resume support.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Thomas Petazzoni <[email protected]>
+ *
+ * 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);

Signature · 1 line

2.0.0

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.

Dear Andrew Lunn,

Quoted 3 lines · Andrew Lunn

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.

Quoted 2 lines

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

Quoted 11 lines

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 4 lines

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.

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.

arch/arm/mach-mvebu/pm-board.c new file +27 -0

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

arch/arm/mach-mvebu/pm-board.c new file +10 -0

+		"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");
+}

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

---
 arch/arm/mach-mvebu/pmsu_ll.S | 8 ++++++++
 1 file changed, 8 insertions(+)

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

---
 arch/arm/mach-mvebu/platsmp.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

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

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

---
 arch/arm/boot/dts/armada-xp-gp.dts | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

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

---
 arch/arm/boot/dts/armada-370-xp.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/boot/dts/armada-370-xp.dtsi +2 -1

index 83286ec..90dba78 100644
@@ -180,7 +180,8 @@
 
 			mbusc: mbus-controller at 20000 {
 				compatible = "marvell,mbus-controller";
-				reg = <0x20000 0x100>, <0x20180 0x20>;
+				reg = <0x20000 0x100>, <0x20180 0x20>,
+				      <0x20250 0x8>;
 			};
 
 			mpic: interrupt-controller at 20000 {

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

---
 arch/arm/boot/dts/armada-xp.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/boot/dts/armada-xp.dtsi +0 -0

index bff9f6c..2be244a 100644

arch/arm/boot/dts/armada-xp.dtsi +2 -0

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