diff mbox series

[v2,1/2] irqchip/stm32: Add irq retrigger support

Message ID 20200218131218.10789-2-alexandre.torgue@st.com (mailing list archive)
State New, archived
Headers show
Series Add GPIO level-sensitive interrupt support | expand

Commit Message

Alexandre TORGUE Feb. 18, 2020, 1:12 p.m. UTC
This commit introduces retrigger support for stm32_ext_h chip.
It consists to rise the GIC interrupt mapped to an EXTI line.

Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>

Comments

Marek Vasut Feb. 18, 2020, 5:51 p.m. UTC | #1
On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> This commit introduces retrigger support for stm32_ext_h chip.
> It consists to rise the GIC interrupt mapped to an EXTI line.
> 
> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>

Reviewed-by: Marek Vasut <marex@denx.de>
(and I tested the previous version on STM32MP1)
Alexandre TORGUE Feb. 19, 2020, 11:33 a.m. UTC | #2
Fix Marc email address

On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> This commit introduces retrigger support for stm32_ext_h chip.
> It consists to rise the GIC interrupt mapped to an EXTI line.
> 
> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
> 
> diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
> index e00f2fa27f00..c971d115edb4 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -604,12 +604,24 @@ static void stm32_exti_h_syscore_deinit(void)
>   	unregister_syscore_ops(&stm32_exti_h_syscore_ops);
>   }
>   
> +static int stm32_exti_h_retrigger(struct irq_data *d)
> +{
> +	struct stm32_exti_chip_data *chip_data = irq_data_get_irq_chip_data(d);
> +	const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
> +	void __iomem *base = chip_data->host_data->base;
> +	u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
> +
> +	writel_relaxed(mask, base + stm32_bank->swier_ofst);
> +
> +	return irq_chip_retrigger_hierarchy(d);
> +}
> +
>   static struct irq_chip stm32_exti_h_chip = {
>   	.name			= "stm32-exti-h",
>   	.irq_eoi		= stm32_exti_h_eoi,
>   	.irq_mask		= stm32_exti_h_mask,
>   	.irq_unmask		= stm32_exti_h_unmask,
> -	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_retrigger		= stm32_exti_h_retrigger,
>   	.irq_set_type		= stm32_exti_h_set_type,
>   	.irq_set_wake		= stm32_exti_h_set_wake,
>   	.flags			= IRQCHIP_MASK_ON_SUSPEND,
>
Marc Zyngier Feb. 19, 2020, 11:43 a.m. UTC | #3
On 2020-02-19 11:33, Alexandre Torgue wrote:
> Fix Marc email address
> 
> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>> This commit introduces retrigger support for stm32_ext_h chip.
>> It consists to rise the GIC interrupt mapped to an EXTI line.
>> 
>> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
>> 
>> diff --git a/drivers/irqchip/irq-stm32-exti.c 
>> b/drivers/irqchip/irq-stm32-exti.c
>> index e00f2fa27f00..c971d115edb4 100644
>> --- a/drivers/irqchip/irq-stm32-exti.c
>> +++ b/drivers/irqchip/irq-stm32-exti.c
>> @@ -604,12 +604,24 @@ static void stm32_exti_h_syscore_deinit(void)
>>   	unregister_syscore_ops(&stm32_exti_h_syscore_ops);
>>   }
>>   +static int stm32_exti_h_retrigger(struct irq_data *d)
>> +{
>> +	struct stm32_exti_chip_data *chip_data = 
>> irq_data_get_irq_chip_data(d);
>> +	const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
>> +	void __iomem *base = chip_data->host_data->base;
>> +	u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
>> +
>> +	writel_relaxed(mask, base + stm32_bank->swier_ofst);
>> +
>> +	return irq_chip_retrigger_hierarchy(d);

Calling irq_chip_retrigger_hierarchy here is really odd. If the write
above has the effect of making the interrupt pending again, why do you
need to force the retrigger any further?

             M.
Alexandre TORGUE Feb. 19, 2020, 1:07 p.m. UTC | #4
On 2/19/20 12:43 PM, Marc Zyngier wrote:
> On 2020-02-19 11:33, Alexandre Torgue wrote:
>> Fix Marc email address
>>
>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>> This commit introduces retrigger support for stm32_ext_h chip.
>>> It consists to rise the GIC interrupt mapped to an EXTI line.
>>>
>>> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
>>>
>>> diff --git a/drivers/irqchip/irq-stm32-exti.c 
>>> b/drivers/irqchip/irq-stm32-exti.c
>>> index e00f2fa27f00..c971d115edb4 100644
>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>> @@ -604,12 +604,24 @@ static void stm32_exti_h_syscore_deinit(void)
>>>       unregister_syscore_ops(&stm32_exti_h_syscore_ops);
>>>   }
>>>   +static int stm32_exti_h_retrigger(struct irq_data *d)
>>> +{
>>> +    struct stm32_exti_chip_data *chip_data = 
>>> irq_data_get_irq_chip_data(d);
>>> +    const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
>>> +    void __iomem *base = chip_data->host_data->base;
>>> +    u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
>>> +
>>> +    writel_relaxed(mask, base + stm32_bank->swier_ofst);
>>> +
>>> +    return irq_chip_retrigger_hierarchy(d);
> 
> Calling irq_chip_retrigger_hierarchy here is really odd. If the write
> above has the effect of making the interrupt pending again, why do you
> need to force the retrigger any further?

To be honest, as we use hierarchical irq_chip, I thought it was the way 
to follow (to retrigger parent irq_chip). It makes maybe no sens here.
The most important to regenerate gic interrupt (associate to the exti 
line) is to write in SWIER register.

Alex

> 
>              M.
Marc Zyngier Feb. 19, 2020, 1:13 p.m. UTC | #5
On 2020-02-19 13:07, Alexandre Torgue wrote:
> On 2/19/20 12:43 PM, Marc Zyngier wrote:
>> On 2020-02-19 11:33, Alexandre Torgue wrote:
>>> Fix Marc email address
>>> 
>>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>>> This commit introduces retrigger support for stm32_ext_h chip.
>>>> It consists to rise the GIC interrupt mapped to an EXTI line.
>>>> 
>>>> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
>>>> 
>>>> diff --git a/drivers/irqchip/irq-stm32-exti.c 
>>>> b/drivers/irqchip/irq-stm32-exti.c
>>>> index e00f2fa27f00..c971d115edb4 100644
>>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>>> @@ -604,12 +604,24 @@ static void stm32_exti_h_syscore_deinit(void)
>>>>       unregister_syscore_ops(&stm32_exti_h_syscore_ops);
>>>>   }
>>>>   +static int stm32_exti_h_retrigger(struct irq_data *d)
>>>> +{
>>>> +    struct stm32_exti_chip_data *chip_data = 
>>>> irq_data_get_irq_chip_data(d);
>>>> +    const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
>>>> +    void __iomem *base = chip_data->host_data->base;
>>>> +    u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
>>>> +
>>>> +    writel_relaxed(mask, base + stm32_bank->swier_ofst);
>>>> +
>>>> +    return irq_chip_retrigger_hierarchy(d);
>> 
>> Calling irq_chip_retrigger_hierarchy here is really odd. If the write
>> above has the effect of making the interrupt pending again, why do you
>> need to force the retrigger any further?
> 
> To be honest, as we use hierarchical irq_chip, I thought it was the
> way to follow (to retrigger parent irq_chip). It makes maybe no sens
> here.

Indeed, it looks perfectly pointless. What 
irq_chip_retrigger_hierarchy()
does is to look for the first parent irqchip that is able to retrigger
the interrupt. Guess what, you've just done that already. And once 
you've
generated the interrupt, you don't need to ask the other irqchips in the
chain to do the same thing.

> The most important to regenerate gic interrupt (associate to the exti
> line) is to write in SWIER register.

Quite. Hence my question.

         M.
Alexandre TORGUE Feb. 19, 2020, 1:17 p.m. UTC | #6
On 2/19/20 2:13 PM, Marc Zyngier wrote:
> On 2020-02-19 13:07, Alexandre Torgue wrote:
>> On 2/19/20 12:43 PM, Marc Zyngier wrote:
>>> On 2020-02-19 11:33, Alexandre Torgue wrote:
>>>> Fix Marc email address
>>>>
>>>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>>>> This commit introduces retrigger support for stm32_ext_h chip.
>>>>> It consists to rise the GIC interrupt mapped to an EXTI line.
>>>>>
>>>>> Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-stm32-exti.c 
>>>>> b/drivers/irqchip/irq-stm32-exti.c
>>>>> index e00f2fa27f00..c971d115edb4 100644
>>>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>>>> @@ -604,12 +604,24 @@ static void stm32_exti_h_syscore_deinit(void)
>>>>>       unregister_syscore_ops(&stm32_exti_h_syscore_ops);
>>>>>   }
>>>>>   +static int stm32_exti_h_retrigger(struct irq_data *d)
>>>>> +{
>>>>> +    struct stm32_exti_chip_data *chip_data = 
>>>>> irq_data_get_irq_chip_data(d);
>>>>> +    const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
>>>>> +    void __iomem *base = chip_data->host_data->base;
>>>>> +    u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
>>>>> +
>>>>> +    writel_relaxed(mask, base + stm32_bank->swier_ofst);
>>>>> +
>>>>> +    return irq_chip_retrigger_hierarchy(d);
>>>
>>> Calling irq_chip_retrigger_hierarchy here is really odd. If the write
>>> above has the effect of making the interrupt pending again, why do you
>>> need to force the retrigger any further?
>>
>> To be honest, as we use hierarchical irq_chip, I thought it was the
>> way to follow (to retrigger parent irq_chip). It makes maybe no sens
>> here.
> 
> Indeed, it looks perfectly pointless. What irq_chip_retrigger_hierarchy()
> does is to look for the first parent irqchip that is able to retrigger
> the interrupt. Guess what, you've just done that already. And once you've
> generated the interrupt, you don't need to ask the other irqchips in the
> chain to do the same thing.

I agree. I gonna remove it v3.

Thanks for the feeback.
Alex

>> The most important to regenerate gic interrupt (associate to the exti
>> line) is to write in SWIER register.
> 
> Quite. Hence my question.
> 
>          M.
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
index e00f2fa27f00..c971d115edb4 100644
--- a/drivers/irqchip/irq-stm32-exti.c
+++ b/drivers/irqchip/irq-stm32-exti.c
@@ -604,12 +604,24 @@  static void stm32_exti_h_syscore_deinit(void)
 	unregister_syscore_ops(&stm32_exti_h_syscore_ops);
 }
 
+static int stm32_exti_h_retrigger(struct irq_data *d)
+{
+	struct stm32_exti_chip_data *chip_data = irq_data_get_irq_chip_data(d);
+	const struct stm32_exti_bank *stm32_bank = chip_data->reg_bank;
+	void __iomem *base = chip_data->host_data->base;
+	u32 mask = BIT(d->hwirq % IRQS_PER_BANK);
+
+	writel_relaxed(mask, base + stm32_bank->swier_ofst);
+
+	return irq_chip_retrigger_hierarchy(d);
+}
+
 static struct irq_chip stm32_exti_h_chip = {
 	.name			= "stm32-exti-h",
 	.irq_eoi		= stm32_exti_h_eoi,
 	.irq_mask		= stm32_exti_h_mask,
 	.irq_unmask		= stm32_exti_h_unmask,
-	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_retrigger		= stm32_exti_h_retrigger,
 	.irq_set_type		= stm32_exti_h_set_type,
 	.irq_set_wake		= stm32_exti_h_set_wake,
 	.flags			= IRQCHIP_MASK_ON_SUSPEND,