irqchip/armada-370-xp: Do not touch Performance Counter Overflow on A375, A38x, A39x

Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific and on new Armada platforms it has different meaning. It does not configure Performance Counter Overflow interrupt masking. So do not touch this register on non-A370/XP platforms (A375, A38x and A39x).

---
 drivers/irqchip/irq-armada-370-xp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

index 5b8d571c041d..1120084cba09 100644
@@ -308,7 +308,16 @@ static inline int armada_370_xp_msi_init(struct device_node *node,
 
 static void armada_xp_mpic_perf_init(void)
 {
-	unsigned long cpuid = cpu_logical_map(smp_processor_id());
+	unsigned long cpuid;
+
+	/*
+	 * This Performance Counter Overflow interrupt is specific for
+	 * Armada 370 and XP. It is not available on Armada 375, 38x and 39x.
+	 */
+	if (!of_machine_is_compatible("marvell,armada-370-xp"))
+		return;
+
+	cpuid = cpu_logical_map(smp_processor_id());
 
 	/* Enable Performance Counter Overflow interrupts */
 	writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),

2.20.1

IRQs 0 and 1 cannot be mapped, they are handled internally by this driver and this driver does not call generic_handle_domain_irq() for these IRQs. So do not allow mapping these IRQs and correctly propagate error from the .irq_map callback.

---
 drivers/irqchip/irq-armada-370-xp.c | 4 ++++
 1 file changed, 4 insertions(+)

index 1120084cba09..ebd76ea1c69b 100644
@@ -546,6 +546,10 @@ static struct irq_chip armada_370_xp_irq_chip = {
 static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 				      unsigned int virq, irq_hw_number_t hw)
 {
+	/* IRQs 0 and 1 cannot be mapped, they are handled internally */
+	if (hw <= 1)
+		return -EINVAL;
+
 	armada_370_xp_irq_mask(irq_get_irq_data(virq));
 	if (!is_percpu_irq(hw))
 		writel(hw, per_cpu_int_base +

2.20.1

So you are referring to this?

                /* Check if the interrupt is not masked on current CPU.
                 * Test IRQ (0-1) and FIQ (8-9) mask bits.
                 */
                if (!(irqsrc & ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)))
                        continue;

                if (irqn == 1) {
                        armada_370_xp_handle_msi_irq(NULL, true);
                        continue;
                }

Should the two FIQ interrupts also return -EINVAL?

Andrew

I'm referring to irqn, which is not handled on armada_370_xp_mpic_domain via generic_handle_domain_irq() when it is == 1.

Also I'm referring to another section:

		if (irqnr > 1) {
			generic_handle_domain_irq(armada_370_xp_mpic_domain,
						  irqnr);
			continue;
		}

		/* MSI handling */
		if (irqnr == 1)
			armada_370_xp_handle_msi_irq(regs, false);

#ifdef CONFIG_SMP
		/* IPI Handling */
		if (irqnr == 0) {
			unsigned long ipimask;
			int ipi;

			ipimask = readl_relaxed(per_cpu_int_base +
						ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
				& IPI_DOORBELL_MASK;

			for_each_set_bit(ipi, &ipimask, IPI_DOORBELL_END)
				generic_handle_domain_irq(ipi_domain, ipi);
		}

#endif

First 'if (irqnr > 1)' just cause that irqnr 0 and 1 are not handled on armada_370_xp_mpic_domain via generic_handle_domain_irq().

No. Following code

		irqsrc = readl_relaxed(main_int_base +
				       ARMADA_370_XP_INT_SOURCE_CTL(irqn));

		/* Check if the interrupt is not masked on current CPU.
		 * Test IRQ (0-1) and FIQ (8-9) mask bits.
		 */
		if (!(irqsrc & ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)))
			continue;

skips processing irqn interrupt if it is masked for the current CPU. Interrupt irqn is **unmasked** on CPU0 if either bit 0 or 8 is set in register ARMADA_370_XP_INT_IRQ_FIQ_MASK. And **unmasked** on CPU1 if either bit 1 or 9 is set.

The reason for this check and skipping is because some per-cpu interrupts on A375, A38x and A39x can be handled directly via GIC and also via MPIC subhierarchy (it depends what you put into DTS, both options are possible and working, just some interrupts are MPIC-only). So if you unmask some interrupt on GIC and interrupt is triggered then also MPIC per-CPU cause register contains in bit for that triggered interrupt (even you have not asked MPIC to unmask interrupt). And once another MPIC interrupt is triggered then routine for handling MPIC subhierarchy see that some cause bits are set and tried to call generic_handle_domain_irq() for all of them, also for those which are masked. That is why it is needed to check if MPIC interrupt is masked on the current CPU or not prior processing it.

So those bits 0-1 and 8-9 have nothing with mapping interrupts.

Given that this is completely academic and obviously doesn't affect anyone, I have removed the Cc: stable from the patch when applying it.

	M.

Without deviation from the norm, progress is not possible.

Hi Pali

Do the Armada 375, 38x and 39x have an over flow interrupt? I assume not.

Does this need a fixes tag? Should it be back ported in stable?

Andrew

Hello! According to documentation there is something named performance counter interrupt, but it is in different register... and this register is not per-cpu.

git blame show that this functionality appeared in commit 28da06dfd9e4 ("irqchip: armada-370-xp: Enable the PMU interrupts").

O.K, not something which can be quickly added.

It is more a question of:

o It must fix a real bug that bothers people (not a, “This could be a problem…” type thing).

From https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Have you seen bad things happen because of this?

Andrew

On those new Armada SoCs that register is used for unmasking interrupts from another hierarchy. Until you start using those new interrupts there probably could not be any issue. But in any case unmasking unwanted interrupt is not a wise idea...

I spotted this because I started using those extended interrupts from another hierarchy. I'm planning to send a patch which properly export them via DTS. Consumer would be pci-mvebu.c driver as in this extended hierarchy are PCIe AER and PME interrupts.

Anyway, Fixes tag is probably better in this situation than stable tag. So would it be fine to replace it by?

O.K. Please add the Fixes tag.

Andrew