Re: [PATCH v3 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip

[Marek Vasut]

On 3/23/20 8:31 PM, Marc Zyngier wrote:
> On Mon, 23 Mar 2020 20:19:39 +0100
> Marek Vasut <marex@xxxxxxx> wrote:
> 
>> On 3/23/20 8:04 PM, Marek Vasut wrote:
>>> On 2/20/20 10:17 AM, Marc Zyngier wrote:
>>>> On 2020-02-20 09:04, Linus Walleij wrote:
>>>>> On Wed, Feb 19, 2020 at 3:32 PM Alexandre Torgue
>>>>> <alexandre.torgue@xxxxxx> wrote:
>>>>>
>>>>>> 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 <alexandre.torgue@xxxxxx>
>>>>>> Tested-by: Marek Vasut <marex@xxxxxxx>
>>>>>
>>>>> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>>>>
>>>>> 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 ?

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