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

[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@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(-)

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

thx,

Jason.

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

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

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@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 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@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(-)
> 

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