lkml.org 
[lkml]   [2020]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
From
Date
On 3/23/20 8:31 PM, Marc Zyngier wrote:
> On Mon, 23 Mar 2020 20:19:39 +0100
> Marek Vasut <marex@denx.de> 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@st.com> 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@st.com>
>>>>>> Tested-by: Marek Vasut <marex@denx.de>
>>>>>
>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>
>>>>> 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 ?

\
 
 \ /
  Last update: 2020-03-23 20:38    [W:0.058 / U:3.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site