[PATCH v3 0/4] irqchip: armada-370-xp: remove IRQ_NOAUTOEN workaround

[PATCH v3 0/4] irqchip: armada-370-xp: remove IRQ_NOAUTOEN workaround

[Thomas Petazzoni]

Hello,

This is the third version of a patch series initially posted in
October 2015, under the title:

  [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

at the time, the regression was worked around in commit
353d6d6c82e5d2533ba22e7f9fb081582bf50dc2 ("irqchip/armada-370-xp: Fix
regression by clearing IRQ_NOAUTOEN") and this series brings the
proper fix for the original issue, while reverting the work around.

Changes since v2:

 - Rebased on top of v4.12-rc1.

Changes since v1:

 - Rebased on top of v4.10.

 - Patch "kernel: irq: implement is_enabled_percpu_irq()" dropped,
   since it has been merged upstream as commit
   f0cb32207307e9d7b3ee8117078b7a37f8d0166e ("genirq: Implement
   irq_percpu_is_enabled()")

 - Patch "irqchip: armada-370-xp: prepare additions to
   armada_xp_mpic_secondary_init()" dropped, since similar changes
   were merged as part of the overall CPU hotplug rework.

 - Patches "irqchip: armada-370-xp: re-order register definitions" and
   "irqchip: armada-370-xp: document the overall driver logic" moved
   at the beginning of the series.

 - Acked-by added on "irqchip: armada-370-xp: re-order register
   definitions" and "irqchip: armada-370-xp: document the overall
   driver logic"

 - In patch "irqchip: armada-370-xp: document the overall driver
   logic", fix minor presentation detail in the ASCII art diagram.

 - In patch "irqchip: armada-370-xp: re-enable per-CPU interrupts at
   resume time":

   - Move the per-CPU interrupt re-enabling logic for secondary cores
     into a utility function armada_xp_mpic_reenable_percpu() called
     by both armada_xp_mpic_starting_cpu() and
     mpic_cascaded_starting_cpu().

   - Instead of iterating on all IRQs and filtering the per-CPU ones,
     only iterate on the first ARMADA_370_XP_MAX_PER_CPU_IRQS, which
     are the only per-CPU ones. Suggested by Grégory Clement.

   - Use the irq_percpu_is_enabled() API, which was the one actually
     merged, instead of the one originally proposed in the v1 of this
     series.

 - Addition of a patch reverting the work around.

Thanks,

Thomas

Thomas Petazzoni (4):
  irqchip: armada-370-xp: re-order register definitions
  irqchip: armada-370-xp: document the overall driver logic
  irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time
  Revert "irqchip/armada-370-xp: Fix regression by clearing
    IRQ_NOAUTOEN"

 drivers/irqchip/irq-armada-370-xp.c | 146 +++++++++++++++++++++++++++++++-----
 1 file changed, 129 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] irqchip: armada-370-xp: re-order register definitions

[Thomas Petazzoni]

In order to clarify to which register base the various register
definitions apply, this commit re-orders them, and adds a comment that
clearly indicate which registers are relative to "main_int_base" and
which registers are relative to "per_cpu_int_base".

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index eb0d4d4..76147df 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -34,25 +34,24 @@
 #include <asm/smp_plat.h>
 #include <asm/mach/irq.h>
 
-/* Interrupt Controller Registers Map */
-#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
-#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
-#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
-#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
-
+/* Registers relative to main_int_base */
 #define ARMADA_370_XP_INT_CONTROL		(0x00)
+#define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
 #define ARMADA_370_XP_INT_SET_ENABLE_OFFS	(0x30)
 #define ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS	(0x34)
 #define ARMADA_370_XP_INT_SOURCE_CTL(irq)	(0x100 + irq*4)
 #define ARMADA_370_XP_INT_SOURCE_CPU_MASK	0xF
 #define ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)	((BIT(0) | BIT(8)) << cpuid)
 
-#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
+/* Registers relative to per_cpu_int_base */
+#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS	(0x08)
+#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS		(0x0c)
 #define ARMADA_375_PPI_CAUSE			(0x10)
-
-#define ARMADA_370_XP_SW_TRIG_INT_OFFS           (0x4)
-#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS          (0xc)
-#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS        (0x8)
+#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
+#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
+#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
+#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
+#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
 
 #define ARMADA_370_XP_MAX_PER_CPU_IRQS		(28)
 
-- 
2.7.4

[PATCH v3 2/4] irqchip: armada-370-xp: document the overall driver logic

[Thomas Petazzoni]

Since the overall logic of the driver to handle the global and per-CPU
masking of the interrupts is far from trivial, this commit adds a long
comment detailing how the hardware operates and what strategy the
driver implements on top of that.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 80 +++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 76147df..1f7dea6 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -34,6 +34,86 @@
 #include <asm/smp_plat.h>
 #include <asm/mach/irq.h>
 
+/*
+ * Overall diagram of the Armada XP interrupt controller:
+ *
+ *    To CPU 0                 To CPU 1
+ *
+ *       /\                       /\
+ *       ||                       ||
+ * +---------------+     +---------------+
+ * |               |	 |               |
+ * |    per-CPU    |	 |    per-CPU    |
+ * |  mask/unmask  |	 |  mask/unmask  |
+ * |     CPU0      |	 |     CPU1      |
+ * |               |	 |               |
+ * +---------------+	 +---------------+
+ *        /\                       /\
+ *        ||                       ||
+ *        \\_______________________//
+ *                     ||
+ *            +-------------------+
+ *            |                   |
+ *            | Global interrupt  |
+ *            |    mask/unmask    |
+ *            |                   |
+ *            +-------------------+
+ *                     /\
+ *                     ||
+ *               interrupt from
+ *                   device
+ *
+ * The "global interrupt mask/unmask" is modified using the
+ * ARMADA_370_XP_INT_SET_ENABLE_OFFS and
+ * ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS registers, which are relative
+ * to "main_int_base".
+ *
+ * The "per-CPU mask/unmask" is modified using the
+ * ARMADA_370_XP_INT_SET_MASK_OFFS and
+ * ARMADA_370_XP_INT_CLEAR_MASK_OFFS registers, which are relative to
+ * "per_cpu_int_base". This base address points to a special address,
+ * which automatically accesses the registers of the current CPU.
+ *
+ * The per-CPU mask/unmask can also be adjusted using the global
+ * per-interrupt ARMADA_370_XP_INT_SOURCE_CTL register, which we use
+ * to configure interrupt affinity.
+ *
+ * Due to this model, all interrupts need to be mask/unmasked at two
+ * different levels: at the global level and at the per-CPU level.
+ *
+ * This driver takes the following approach to deal with this:
+ *
+ *  - For global interrupts:
+ *
+ *    At ->map() time, a global interrupt is unmasked at the per-CPU
+ *    mask/unmask level. It is therefore unmasked at this level for
+ *    the current CPU, running the ->map() code. This allows to have
+ *    the interrupt unmasked at this level in non-SMP
+ *    configurations. In SMP configurations, the ->set_affinity()
+ *    callback is called, which using the
+ *    ARMADA_370_XP_INT_SOURCE_CTL() readjusts the per-CPU mask/unmask
+ *    for the interrupt.
+ *
+ *    The ->mask() and ->unmask() operations only mask/unmask the
+ *    interrupt at the "global" level.
+ *
+ *    So, a global interrupt is enabled at the per-CPU level as soon
+ *    as it is mapped. At run time, the masking/unmasking takes place
+ *    at the global level.
+ *
+ *  - For per-CPU interrupts
+ *
+ *    At ->map() time, a per-CPU interrupt is unmasked at the global
+ *    mask/unmask level.
+ *
+ *    The ->mask() and ->unmask() operations mask/unmask the interrupt
+ *    at the per-CPU level.
+ *
+ *    So, a per-CPU interrupt is enabled at the global level as soon
+ *    as it is mapped. At run time, the masking/unmasking takes place
+ *    at the per-CPU level.
+ */
+
 /* Registers relative to main_int_base */
 #define ARMADA_370_XP_INT_CONTROL		(0x00)
 #define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
-- 
2.7.4

[PATCH v3 3/4] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed
the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to
irq_set_probe().

While the commit log seems to imply that there are no functional
changes, there are indeed functional changes introduced by this commit:
the IRQ_NOAUTOEN flag is no longer cleared. This functional change
caused a regression on Armada XP, which no longer works properly after
suspend/resume because per-CPU interrupts remain disabled. This
regression was temporarly worked around in commit
353d6d6c82e5d ("irqchip/armada-370-xp: Fix regression by clearing
IRQ_NOAUTOEN"), but it is not the most satisfying solution. This commit
implements the solution that was initially discussed with Thomas
Gleixner.

Due to how the hardware registers work, the irq-armada-370-xp cannot
simply save/restore a bunch of registers at suspend/resume to make sure
that the interrupts remain in the same state after resuming. Therefore,
it relies on the kernel to say whether the interrupt is disabled or not,
using the irqd_irq_disabled() function. This was all working fine while
the IRQ_NOAUTOEN flag was cleared.

With the change introduced by Rob Herring in d17cab4451df1, the
IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled()
returns false for per-CPU interrupts, and therefore our per-CPU
interrupts are no longer re-enabled after resume.

This commit fixes that by using irqd_irq_disabled() only for global
interrupts, and using the newly introduced irq_percpu_is_enabled() for
per-CPU interrupts.

Also, it fixes a related problems that per-CPU interrupts were only
re-enabled on the boot CPU and not other CPUs. Until now this wasn't a
problem since on this platform, only the local timers are using per-CPU
interrupts and the local timers of secondary CPUs are turned off/on
during CPU hotplug before suspend, after after resume. However, since
Linux 4.4, we are also be using per-CPU interrupts for the network
controller, so we need to properly restore the per-CPU interrupts on
secondary CPUs as well.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 46 ++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 1f7dea6..1d4de6e 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -360,7 +360,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 		irq_set_percpu_devid(virq);
 		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
 					handle_percpu_devid_irq);
-
 	} else {
 		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
 					handle_level_irq);
@@ -424,16 +423,40 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
 		ARMADA_370_XP_SW_TRIG_INT_OFFS);
 }
 
+static void armada_xp_mpic_reenable_percpu(void)
+{
+	unsigned int irq;
+
+	/* Re-enable per-CPU interrupts that were enabled before suspend */
+	for (irq = 0; irq < ARMADA_370_XP_MAX_PER_CPU_IRQS; irq++) {
+		struct irq_data *data;
+		int virq;
+
+		virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq);
+		if (virq == 0)
+			continue;
+
+		data = irq_get_irq_data(virq);
+
+		if (!irq_percpu_is_enabled(virq))
+			continue;
+
+		armada_370_xp_irq_unmask(data);
+	}
+}
+
 static int armada_xp_mpic_starting_cpu(unsigned int cpu)
 {
 	armada_xp_mpic_perf_init();
 	armada_xp_mpic_smp_cpu_init();
+	armada_xp_mpic_reenable_percpu();
 	return 0;
 }
 
 static int mpic_cascaded_starting_cpu(unsigned int cpu)
 {
 	armada_xp_mpic_perf_init();
+	armada_xp_mpic_reenable_percpu();
 	enable_percpu_irq(parent_irq, IRQ_TYPE_NONE);
 	return 0;
 }
@@ -581,16 +604,27 @@ static void armada_370_xp_mpic_resume(void)
 		if (virq == 0)
 			continue;
 
-		if (!is_percpu_irq(irq))
+		data = irq_get_irq_data(virq);
+
+		if (!is_percpu_irq(irq)) {
+			/* Non per-CPU interrupts */
 			writel(irq, per_cpu_int_base +
 			       ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
-		else
+			if (!irqd_irq_disabled(data))
+				armada_370_xp_irq_unmask(data);
+		} else {
+			/* Per-CPU interrupts */
 			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);
+			/*
+			 * Re-enable on the current CPU,
+			 * armada_xp_mpic_reenable_percpu() will take
+			 * care of secondary CPUs when they come up.
+			 */
+			if (irq_percpu_is_enabled(virq))
+				armada_370_xp_irq_unmask(data);
+		}
 	}
 
 	/* Reconfigure doorbells for IPIs and MSIs */
-- 
2.7.4

[PATCH v3 4/4] Revert "irqchip/armada-370-xp: Fix regression by clearing IRQ_NOAUTOEN"

[Thomas Petazzoni]

This reverts commit 353d6d6c82e5d2533ba22e7f9fb081582bf50dc2, which is
no longer needed, now that the irq-armada-370-xp driver properly
re-enables per-CPU interrupt on both the boot CPU and secondary CPUs
after resume.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 1d4de6e..577b428 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -365,7 +365,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 					handle_level_irq);
 	}
 	irq_set_probe(virq);
-	irq_clear_status_flags(virq, IRQ_NOAUTOEN);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v3 0/4] irqchip: armada-370-xp: remove IRQ_NOAUTOEN workaround

[Marc Zyngier]

On 18/05/17 09:07, Thomas Petazzoni wrote:
> Hello,
> 
> This is the third version of a patch series initially posted in
> October 2015, under the title:
> 
>   [PATCH 0/5] Fix regression introduced by set_irq_flags() removal
> 
> at the time, the regression was worked around in commit
> 353d6d6c82e5d2533ba22e7f9fb081582bf50dc2 ("irqchip/armada-370-xp: Fix
> regression by clearing IRQ_NOAUTOEN") and this series brings the
> proper fix for the original issue, while reverting the work around.

I've queued this for 4.13.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...