On Mon, 23 Mar 2020 20:37:54 +0100
Marek Vasut [off-list ref] wrote:

On 3/23/20 8:31 PM, Marc Zyngier wrote:

quoted

On Mon, 23 Mar 2020 20:19:39 +0100
Marek Vasut [off-list ref] wrote:

quoted

On 3/23/20 8:04 PM, Marek Vasut wrote:

quoted

On 2/20/20 10:17 AM, Marc Zyngier wrote:

quoted

On 2020-02-20 09:04, Linus Walleij wrote:

quoted

On Wed, Feb 19, 2020 at 3:32 PM Alexandre Torgue
[off-list ref] wrote:

quoted

GPIO hardware block is directly linked to EXTI block but EXTI handles
external interrupts only on edge. To be able to handle GPIO interrupt on
level a "hack" is done in gpio irq chip: parent interrupt (exti irq
chip)
is retriggered following interrupt type and gpio line value.

Signed-off-by: Alexandre Torgue <redacted>
Tested-by: Marek Vasut <marex@denx.de>

Reviewed-by: Linus Walleij <redacted>

If Marc want to merge it with patch 1/2 go ahead!

I'll queue the whole thing for 5.7.

I have a feeling this doesn't work with threaded interrupts.

If the interrupt handler runs in a thread context, the EOI will happen
almost right away (while the IRQ handler runs) and so will the code
handling the IRQ retriggering. But since the IRQ handler still runs and
didn't return yet, the retriggering doesn't cause the IRQ handler to be
called again once it finishes, even if the IRQ line is still asserted.
And that could result in some of the retriggers now happening I think.
Or am I doing something wrong ?

The patch below makes my usecase work, but I don't know whether it's
correct. Basically once the threaded IRQ handler finishes and unmasks
the IRQ, check whether the line is asserted and retrigger if so.

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c

b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 9ac9ecfc2f34..060dbcb7ae72 100644

--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c

@@ -371,12 +371,26 @@ static void

stm32_gpio_irq_release_resources(struct irq_data *irq_data)
        gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
 }

+static void stm32_gpio_irq_unmask(struct irq_data *d)
+{
+       struct stm32_gpio_bank *bank = d->domain->host_data;
+       int level;
+
+       irq_chip_unmask_parent(d);
+
+       /* If level interrupt type then retrig */
+       level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
+       if ((level == 0 && bank->irq_type[d->hwirq] ==
IRQ_TYPE_LEVEL_LOW) ||
+           (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
+               irq_chip_retrigger_hierarchy(d);
+}
+
 static struct irq_chip stm32_gpio_irq_chip = {
        .name           = "stm32gpio",
        .irq_eoi        = stm32_gpio_irq_eoi,
        .irq_ack        = irq_chip_ack_parent,
        .irq_mask       = irq_chip_mask_parent,
-       .irq_unmask     = irq_chip_unmask_parent,
+       .irq_unmask     = stm32_gpio_irq_unmask,
        .irq_set_type   = stm32_gpio_set_type,
        .irq_set_wake   = irq_chip_set_wake_parent,
        .irq_request_resources = stm32_gpio_irq_request_resources,

OK, I see your problem now.

The usual flow is along the line of Ack+Eoi, and that's what the
current code guarantees.

Threaded interrupts do Ack+Mask+Eoi, followed by an Unmask once the
thread finishes. This unmask needs to do the retrigger as well, as you
found out.

Can you please refactor the above so that we have the common code
between unmask and eoi in a separate function, send a proper patch, and
I'll apply it on top of the current irq/irqchip-5.7 branch.

Sure, I can. Do we still need this retriggering in the irq_eoi too ?

Absolutely, because that's what matters for the non-threaded case
(there is no mask/unmask on that path). It is also never wrong to
over-resample (it just slows things down).

Also, are there any other hidden details I might've missed ?

Probably. But let's fix one bug at a time, shall we? ;-) And let's hope
that ST doesn't take this as a excuse not to clean up their act in
their next SoC!

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel