diff mbox series

[v3] irqchip/exiu: Fix acknowledgment of edge triggered interrupts

Message ID 20220503134541.2566457-1-daniel.thompson@linaro.org (mailing list archive)
State New, archived
Headers show
Series [v3] irqchip/exiu: Fix acknowledgment of edge triggered interrupts | expand

Commit Message

Daniel Thompson May 3, 2022, 1:45 p.m. UTC 
Currently the EXIU uses the fasteoi interrupt flow that is configured by
it's parent (irq-gic-v3.c). With this flow the only chance to clear the
interrupt request happens during .irq_eoi() and (obviously) this happens
after the interrupt handler has run. EXIU requires edge triggered
interrupts to be acked prior to interrupt handling. Without this we
risk incorrect interrupt dismissal when a new interrupt is delivered
after the handler reads and acknowledges the peripheral but before the
irq_eoi() takes place.

Fix this by clearing the interrupt request from .irq_ack() if we are
configured for edge triggered interrupts. This requires adopting the
fasteoi-ack flow instead of the fasteoi to ensure the ack gets called.

These changes have been tested using the power button on a
Developerbox/SC2A11 combined with some hackery in gpio-keys so I can
play with the different trigger mode [and an mdelay(500) so I can
can check what happens on a double click in both modes].

Fixes: 706cffc1b912 ("irqchip/exiu: Add support for Socionext Synquacer EXIU controller")
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---

Notes:
    Changes in v3:
     * Optimize exiu_irq_eoi() to avoid unnecessary register read (Marc Z)
     * Add comment to exiu_irq_eoi() so future adventurers can better
       understand how the EXIU on SC2A11 behaves (Marc Z)
     * Remove redundant trigger mode management from exiu_domain_alloc()
       (Marc Z)
    
    Changes in v2:
    
     * Switch to dynamic selection of handle_fasteoi_irq and
       handle_fasteoi_ack_irq and reintroduce exiu_irq_eoi() since we need
       that for level triggered interrupts (Ard B).
     * Above changes mean we are no longer using sun6i NMI code as a
       template to tidy up the description accordingly.

 arch/arm64/Kconfig.platforms   |  1 +
 drivers/irqchip/irq-sni-exiu.c | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)


base-commit: b2d229d4ddb17db541098b83524d901257e93845
--
2.35.1

Comments

Ard Biesheuvel May 3, 2022, 2:28 p.m. UTC | #1 
On Tue, 3 May 2022 at 15:45, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
> Currently the EXIU uses the fasteoi interrupt flow that is configured by
> it's parent (irq-gic-v3.c). With this flow the only chance to clear the
> interrupt request happens during .irq_eoi() and (obviously) this happens
> after the interrupt handler has run. EXIU requires edge triggered
> interrupts to be acked prior to interrupt handling. Without this we
> risk incorrect interrupt dismissal when a new interrupt is delivered
> after the handler reads and acknowledges the peripheral but before the
> irq_eoi() takes place.
>
> Fix this by clearing the interrupt request from .irq_ack() if we are
> configured for edge triggered interrupts. This requires adopting the
> fasteoi-ack flow instead of the fasteoi to ensure the ack gets called.
>
> These changes have been tested using the power button on a
> Developerbox/SC2A11 combined with some hackery in gpio-keys so I can
> play with the different trigger mode [and an mdelay(500) so I can
> can check what happens on a double click in both modes].
>
> Fixes: 706cffc1b912 ("irqchip/exiu: Add support for Socionext Synquacer EXIU controller")
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>
> Notes:
>     Changes in v3:
>      * Optimize exiu_irq_eoi() to avoid unnecessary register read (Marc Z)
>      * Add comment to exiu_irq_eoi() so future adventurers can better
>        understand how the EXIU on SC2A11 behaves (Marc Z)
>      * Remove redundant trigger mode management from exiu_domain_alloc()
>        (Marc Z)
>
>     Changes in v2:
>
>      * Switch to dynamic selection of handle_fasteoi_irq and
>        handle_fasteoi_ack_irq and reintroduce exiu_irq_eoi() since we need
>        that for level triggered interrupts (Ard B).
>      * Above changes mean we are no longer using sun6i NMI code as a
>        template to tidy up the description accordingly.
>
>  arch/arm64/Kconfig.platforms   |  1 +
>  drivers/irqchip/irq-sni-exiu.c | 25 ++++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 30b123cde02c..aaeaf57c8222 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -253,6 +253,7 @@ config ARCH_INTEL_SOCFPGA
>
>  config ARCH_SYNQUACER
>         bool "Socionext SynQuacer SoC Family"
> +       select IRQ_FASTEOI_HIERARCHY_HANDLERS
>
>  config ARCH_TEGRA
>         bool "NVIDIA Tegra SoC Family"
> diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
> index abd011fcecf4..c7db617e1a2f 100644
> --- a/drivers/irqchip/irq-sni-exiu.c
> +++ b/drivers/irqchip/irq-sni-exiu.c
> @@ -37,11 +37,26 @@ struct exiu_irq_data {
>         u32             spi_base;
>  };
>
> -static void exiu_irq_eoi(struct irq_data *d)
> +static void exiu_irq_ack(struct irq_data *d)
>  {
>         struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);
>
>         writel(BIT(d->hwirq), data->base + EIREQCLR);
> +}
> +
> +static void exiu_irq_eoi(struct irq_data *d)
> +{
> +       struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +       /*
> +        * Level triggered interrupts are latched and must be cleared during
> +        * EOI or the interrupt will be jammed on. Of course if a level
> +        * triggered interrupt is still asserted then the write will not clear
> +        * the interrupt.
> +        */
> +       if (irqd_is_level_type(d))
> +               writel(BIT(d->hwirq), data->base + EIREQCLR);
> +
>         irq_chip_eoi_parent(d);
>  }
>
> @@ -91,10 +106,13 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
>         writel_relaxed(val, data->base + EILVL);
>
>         val = readl_relaxed(data->base + EIEDG);
> -       if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
> +       if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) {
>                 val &= ~BIT(d->hwirq);
> -       else
> +               irq_set_handler_locked(d, handle_fasteoi_irq);
> +       } else {
>                 val |= BIT(d->hwirq);
> +               irq_set_handler_locked(d, handle_fasteoi_ack_irq);
> +       }
>         writel_relaxed(val, data->base + EIEDG);
>
>         writel_relaxed(BIT(d->hwirq), data->base + EIREQCLR);
> @@ -104,6 +122,7 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
>
>  static struct irq_chip exiu_irq_chip = {
>         .name                   = "EXIU",
> +       .irq_ack                = exiu_irq_ack,
>         .irq_eoi                = exiu_irq_eoi,
>         .irq_enable             = exiu_irq_enable,
>         .irq_mask               = exiu_irq_mask,
>
> base-commit: b2d229d4ddb17db541098b83524d901257e93845
> --
> 2.35.1
>
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 30b123cde02c..aaeaf57c8222 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -253,6 +253,7 @@  config ARCH_INTEL_SOCFPGA

 config ARCH_SYNQUACER
 	bool "Socionext SynQuacer SoC Family"
+	select IRQ_FASTEOI_HIERARCHY_HANDLERS

 config ARCH_TEGRA
 	bool "NVIDIA Tegra SoC Family"
diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
index abd011fcecf4..c7db617e1a2f 100644
--- a/drivers/irqchip/irq-sni-exiu.c
+++ b/drivers/irqchip/irq-sni-exiu.c
@@ -37,11 +37,26 @@  struct exiu_irq_data {
 	u32		spi_base;
 };

-static void exiu_irq_eoi(struct irq_data *d)
+static void exiu_irq_ack(struct irq_data *d)
 {
 	struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);

 	writel(BIT(d->hwirq), data->base + EIREQCLR);
+}
+
+static void exiu_irq_eoi(struct irq_data *d)
+{
+	struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	/*
+	 * Level triggered interrupts are latched and must be cleared during
+	 * EOI or the interrupt will be jammed on. Of course if a level
+	 * triggered interrupt is still asserted then the write will not clear
+	 * the interrupt.
+	 */
+	if (irqd_is_level_type(d))
+		writel(BIT(d->hwirq), data->base + EIREQCLR);
+
 	irq_chip_eoi_parent(d);
 }

@@ -91,10 +106,13 @@  static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
 	writel_relaxed(val, data->base + EILVL);

 	val = readl_relaxed(data->base + EIEDG);
-	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
+	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) {
 		val &= ~BIT(d->hwirq);
-	else
+		irq_set_handler_locked(d, handle_fasteoi_irq);
+	} else {
 		val |= BIT(d->hwirq);
+		irq_set_handler_locked(d, handle_fasteoi_ack_irq);
+	}
 	writel_relaxed(val, data->base + EIEDG);

 	writel_relaxed(BIT(d->hwirq), data->base + EIREQCLR);
@@ -104,6 +122,7 @@  static int exiu_irq_set_type(struct irq_data *d, unsigned int type)

 static struct irq_chip exiu_irq_chip = {
 	.name			= "EXIU",
+	.irq_ack		= exiu_irq_ack,
 	.irq_eoi		= exiu_irq_eoi,
 	.irq_enable		= exiu_irq_enable,
 	.irq_mask		= exiu_irq_mask,