* [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
@ 2020-02-18 13:12 Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-18 13:12 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio, marex
This series adds the possibility to handle gpio interrupts on level.
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.
In exti irq chip, retrigger ops function is added.
Changes since v1:
- Fix issue reported by Marek: return statment in "stm32_gpio_set_type"
function.
Regards
Alex
Alexandre Torgue (2):
irqchip/stm32: Add irq retrigger support
pinctrl: stm32: Add level interrupt support to gpio irq chip
drivers/irqchip/irq-stm32-exti.c | 14 ++++++++-
drivers/pinctrl/stm32/pinctrl-stm32.c | 45 +++++++++++++++++++++++++--
2 files changed, 56 insertions(+), 3 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-18 13:12 [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Alexandre Torgue
@ 2020-02-18 13:12 ` Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:33 ` Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip Alexandre Torgue
2020-02-18 16:25 ` [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Marek Vasut
2 siblings, 2 replies; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-18 13:12 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio, marex
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,
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
2020-02-18 13:12 [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
@ 2020-02-18 13:12 ` Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:34 ` Alexandre Torgue
2020-02-18 16:25 ` [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Marek Vasut
2 siblings, 2 replies; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-18 13:12 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio, marex
This patch adds level interrupt support to gpio irq chip.
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>
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 2d5e0435af0a..dae236562543 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -92,6 +92,7 @@ struct stm32_gpio_bank {
u32 bank_nr;
u32 bank_ioport_nr;
u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
+ u32 irq_type[STM32_GPIO_PINS_PER_BANK];
};
struct stm32_pinctrl {
@@ -303,6 +304,46 @@ static const struct gpio_chip stm32_gpio_template = {
.get_direction = stm32_gpio_get_direction,
};
+void stm32_gpio_irq_eoi(struct irq_data *d)
+{
+ struct stm32_gpio_bank *bank = d->domain->host_data;
+ int line;
+
+ irq_chip_eoi_parent(d);
+
+ /* If level interrupt type then retrig */
+ line = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
+ if ((line == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
+ (line == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
+ irq_chip_retrigger_hierarchy(d);
+};
+
+static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
+{
+ struct stm32_gpio_bank *bank = d->domain->host_data;
+ u32 parent_type;
+
+ bank->irq_type[d->hwirq] = type;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_EDGE_FALLING:
+ case IRQ_TYPE_EDGE_BOTH:
+ parent_type = type;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ parent_type = IRQ_TYPE_EDGE_RISING;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ parent_type = IRQ_TYPE_EDGE_FALLING;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return irq_chip_set_type_parent(d, parent_type);
+};
+
static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
{
struct stm32_gpio_bank *bank = irq_data->domain->host_data;
@@ -332,11 +373,11 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
static struct irq_chip stm32_gpio_irq_chip = {
.name = "stm32gpio",
- .irq_eoi = irq_chip_eoi_parent,
+ .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_set_type = irq_chip_set_type_parent,
+ .irq_set_type = stm32_gpio_set_type,
.irq_set_wake = irq_chip_set_wake_parent,
.irq_request_resources = stm32_gpio_irq_request_resources,
.irq_release_resources = stm32_gpio_irq_release_resources,
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-18 13:12 [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip Alexandre Torgue
@ 2020-02-18 16:25 ` Marek Vasut
2020-02-18 18:10 ` Alexandre Torgue
2 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2020-02-18 16:25 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> This series adds the possibility to handle gpio interrupts on level.
>
> 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.
>
> In exti irq chip, retrigger ops function is added.
btw. this might be unrelated, but is it possible to have e.g. gpioC2 set
as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux selection")
prevents that.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
@ 2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:33 ` Alexandre Torgue
1 sibling, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2020-02-18 17:51 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
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)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
2020-02-18 13:12 ` [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip Alexandre Torgue
@ 2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:34 ` Alexandre Torgue
1 sibling, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2020-02-18 17:51 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> This patch adds level interrupt support to gpio irq chip.
>
> 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: Marek Vasut <marex@denx.de>
(and I tested the previous version on STM32MP1)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-18 16:25 ` [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Marek Vasut
@ 2020-02-18 18:10 ` Alexandre Torgue
2020-02-18 18:13 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-18 18:10 UTC (permalink / raw)
To: Marek Vasut, Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
Hi Marek
On 2/18/20 5:25 PM, Marek Vasut wrote:
> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>> This series adds the possibility to handle gpio interrupts on level.
>>
>> 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.
>>
>> In exti irq chip, retrigger ops function is added.
>
> btw. this might be unrelated, but is it possible to have e.g. gpioC2 set
> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux selection")
> prevents that.
>
No it's not possible. Each gpio line doesn't have a dedicated Exti line
Each Exti line is muxing between gpio banks.
Mapping is done as following:
EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
...
regards
Alexandre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-18 18:10 ` Alexandre Torgue
@ 2020-02-18 18:13 ` Marek Vasut
2020-02-19 9:20 ` Alexandre Torgue
0 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2020-02-18 18:13 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/18/20 7:10 PM, Alexandre Torgue wrote:
> Hi Marek
Hi,
> On 2/18/20 5:25 PM, Marek Vasut wrote:
>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>> This series adds the possibility to handle gpio interrupts on level.
>>>
>>> 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.
>>>
>>> In exti irq chip, retrigger ops function is added.
>>
>> btw. this might be unrelated, but is it possible to have e.g. gpioC2 set
>> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
>> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux selection")
>> prevents that.
>>
>
> No it's not possible. Each gpio line doesn't have a dedicated Exti line
> Each Exti line is muxing between gpio banks.
OK, that confirms my assumption.
> Mapping is done as following:
>
> EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
> EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
> EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
> ...
Is it at least possible to have IRQs of the same type on the same exti
line? E.g. gpioA2 of trigger-edge-falling and gpioB2 trigger-edge-falling ?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-18 18:13 ` Marek Vasut
@ 2020-02-19 9:20 ` Alexandre Torgue
2020-02-19 17:24 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 9:20 UTC (permalink / raw)
To: Marek Vasut, Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/18/20 7:13 PM, Marek Vasut wrote:
> On 2/18/20 7:10 PM, Alexandre Torgue wrote:
>> Hi Marek
>
> Hi,
>
>> On 2/18/20 5:25 PM, Marek Vasut wrote:
>>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>>> This series adds the possibility to handle gpio interrupts on level.
>>>>
>>>> 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.
>>>>
>>>> In exti irq chip, retrigger ops function is added.
>>>
>>> btw. this might be unrelated, but is it possible to have e.g. gpioC2 set
>>> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
>>> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux selection")
>>> prevents that.
>>>
>>
>> No it's not possible. Each gpio line doesn't have a dedicated Exti line
>> Each Exti line is muxing between gpio banks.
>
> OK, that confirms my assumption.
>
>> Mapping is done as following:
>>
>> EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
>> EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
>> EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
>> ...
>
> Is it at least possible to have IRQs of the same type on the same exti
> line? E.g. gpioA2 of trigger-edge-falling and gpioB2 trigger-edge-falling ?
>
Sorry I don't catch your point. If you already succeed to get gpioA2,
then you will failed to get gpioB2 but looking at function call stack we
could get an other issue.
Lets take example where you succeed to get gpioa2 as interrupt (using
interrupt bindings) and now you try to do the same for gpiob2, you will
have (roughly):
stm32_gpio_irq_request_resources (for gpiob2) --> succeed
stm32_gpio_set_type
|
|--> stm32_exti_set_type type -> change exti line 2 trigger registers
with gpiob2 binding.
stm32_gpio_domain_activate --> failed as exti line2 is already used
by gpioa2.
So as stm32_gpio_set_type is called before checking than exti line is
available, type could be changed and behavior of gpioa2 interrupt broken.
Solution would be to move the exti line mux check from
stm32_gpio_domain_activate to stm32_gpio_irq_request_resources callback.
Regards
Alex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
@ 2020-02-19 11:33 ` Alexandre Torgue
2020-02-19 11:43 ` Marc Zyngier
1 sibling, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 11:33 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: marex, linux-gpio, linux-kernel, linux-arm-kernel
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,
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
2020-02-18 13:12 ` [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
@ 2020-02-19 11:34 ` Alexandre Torgue
2020-02-19 12:07 ` Marc Zyngier
1 sibling, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 11:34 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: marex, linux-gpio, linux-kernel, linux-arm-kernel
Fix Marc email address.
On 2/18/20 2:12 PM, Alexandre Torgue wrote:
> This patch adds level interrupt support to gpio irq chip.
>
> 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>
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 2d5e0435af0a..dae236562543 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -92,6 +92,7 @@ struct stm32_gpio_bank {
> u32 bank_nr;
> u32 bank_ioport_nr;
> u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
> + u32 irq_type[STM32_GPIO_PINS_PER_BANK];
> };
>
> struct stm32_pinctrl {
> @@ -303,6 +304,46 @@ static const struct gpio_chip stm32_gpio_template = {
> .get_direction = stm32_gpio_get_direction,
> };
>
> +void stm32_gpio_irq_eoi(struct irq_data *d)
> +{
> + struct stm32_gpio_bank *bank = d->domain->host_data;
> + int line;
> +
> + irq_chip_eoi_parent(d);
> +
> + /* If level interrupt type then retrig */
> + line = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> + if ((line == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
> + (line == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
> + irq_chip_retrigger_hierarchy(d);
> +};
> +
> +static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct stm32_gpio_bank *bank = d->domain->host_data;
> + u32 parent_type;
> +
> + bank->irq_type[d->hwirq] = type;
> +
> + switch (type) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_EDGE_FALLING:
> + case IRQ_TYPE_EDGE_BOTH:
> + parent_type = type;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + parent_type = IRQ_TYPE_EDGE_RISING;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + parent_type = IRQ_TYPE_EDGE_FALLING;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return irq_chip_set_type_parent(d, parent_type);
> +};
> +
> static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
> {
> struct stm32_gpio_bank *bank = irq_data->domain->host_data;
> @@ -332,11 +373,11 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>
> static struct irq_chip stm32_gpio_irq_chip = {
> .name = "stm32gpio",
> - .irq_eoi = irq_chip_eoi_parent,
> + .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_set_type = irq_chip_set_type_parent,
> + .irq_set_type = stm32_gpio_set_type,
> .irq_set_wake = irq_chip_set_wake_parent,
> .irq_request_resources = stm32_gpio_irq_request_resources,
> .irq_release_resources = stm32_gpio_irq_release_resources,
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-19 11:33 ` Alexandre Torgue
@ 2020-02-19 11:43 ` Marc Zyngier
2020-02-19 13:07 ` Alexandre Torgue
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-02-19 11:43 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
2020-02-19 11:34 ` Alexandre Torgue
@ 2020-02-19 12:07 ` Marc Zyngier
2020-02-19 12:59 ` Alexandre Torgue
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-02-19 12:07 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
On 2020-02-19 11:34, Alexandre Torgue wrote:
> Fix Marc email address.
>
> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>> This patch adds level interrupt support to gpio irq chip.
A commit message should not contain "this patch".
>>
>> 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>
>>
>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> index 2d5e0435af0a..dae236562543 100644
>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> @@ -92,6 +92,7 @@ struct stm32_gpio_bank {
>> u32 bank_nr;
>> u32 bank_ioport_nr;
>> u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
>> + u32 irq_type[STM32_GPIO_PINS_PER_BANK];
Do you really need a u32 here? an array of u8 seems enough. After all,
you only need two bits of information per interrupts (level or not,
low or high).
>> };
>> struct stm32_pinctrl {
>> @@ -303,6 +304,46 @@ static const struct gpio_chip stm32_gpio_template
>> = {
>> .get_direction = stm32_gpio_get_direction,
>> };
>> +void stm32_gpio_irq_eoi(struct irq_data *d)
>> +{
>> + struct stm32_gpio_bank *bank = d->domain->host_data;
>> + int line;
>> +
>> + irq_chip_eoi_parent(d);
>> +
>> + /* If level interrupt type then retrig */
>> + line = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
>> + if ((line == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
>> + (line == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>> + irq_chip_retrigger_hierarchy(d);
s/line/level/
>> +};
>> +
>> +static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
>> +{
>> + struct stm32_gpio_bank *bank = d->domain->host_data;
>> + u32 parent_type;
>> +
>> + bank->irq_type[d->hwirq] = type;
It would make more sense if this this assignment was done *after*
sanitizing the type value.
>> +
>> + switch (type) {
>> + case IRQ_TYPE_EDGE_RISING:
>> + case IRQ_TYPE_EDGE_FALLING:
>> + case IRQ_TYPE_EDGE_BOTH:
>> + parent_type = type;
>> + break;
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + parent_type = IRQ_TYPE_EDGE_RISING;
>> + break;
>> + case IRQ_TYPE_LEVEL_LOW:
>> + parent_type = IRQ_TYPE_EDGE_FALLING;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return irq_chip_set_type_parent(d, parent_type);
>> +};
>> +
>> static int stm32_gpio_irq_request_resources(struct irq_data
>> *irq_data)
>> {
>> struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>> @@ -332,11 +373,11 @@ static void
>> stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>> static struct irq_chip stm32_gpio_irq_chip = {
>> .name = "stm32gpio",
>> - .irq_eoi = irq_chip_eoi_parent,
>> + .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_set_type = irq_chip_set_type_parent,
>> + .irq_set_type = stm32_gpio_set_type,
>> .irq_set_wake = irq_chip_set_wake_parent,
>> .irq_request_resources = stm32_gpio_irq_request_resources,
>> .irq_release_resources = stm32_gpio_irq_release_resources,
>>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip
2020-02-19 12:07 ` Marc Zyngier
@ 2020-02-19 12:59 ` Alexandre Torgue
0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 12:59 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
Hi Marc
On 2/19/20 1:07 PM, Marc Zyngier wrote:
> On 2020-02-19 11:34, Alexandre Torgue wrote:
>> Fix Marc email address.
>>
>> On 2/18/20 2:12 PM, Alexandre Torgue wrote:
>>> This patch adds level interrupt support to gpio irq chip.
>
> A commit message should not contain "this patch".
Ok I'll change in v3
>
>>>
>>> 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>
>>>
>>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c
>>> b/drivers/pinctrl/stm32/pinctrl-stm32.c
>>> index 2d5e0435af0a..dae236562543 100644
>>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>>> @@ -92,6 +92,7 @@ struct stm32_gpio_bank {
>>> u32 bank_nr;
>>> u32 bank_ioport_nr;
>>> u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
>>> + u32 irq_type[STM32_GPIO_PINS_PER_BANK];
>
> Do you really need a u32 here? an array of u8 seems enough. After all,
> you only need two bits of information per interrupts (level or not,
> low or high).
I agree. No need to have u32.
>
>>> };
>>> struct stm32_pinctrl {
>>> @@ -303,6 +304,46 @@ static const struct gpio_chip
>>> stm32_gpio_template = {
>>> .get_direction = stm32_gpio_get_direction,
>>> };
>>> +void stm32_gpio_irq_eoi(struct irq_data *d)
>>> +{
>>> + struct stm32_gpio_bank *bank = d->domain->host_data;
>>> + int line;
>>> +
>>> + irq_chip_eoi_parent(d);
>>> +
>>> + /* If level interrupt type then retrig */
>>> + line = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
>>> + if ((line == 0 && bank->irq_type[d->hwirq] ==
>>> IRQ_TYPE_LEVEL_LOW) ||
>>> + (line == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>>> + irq_chip_retrigger_hierarchy(d);
>
> s/line/level/
>
correct
>>> +};
>>> +
>>> +static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
>>> +{
>>> + struct stm32_gpio_bank *bank = d->domain->host_data;
>>> + u32 parent_type;
>>> +
>>> + bank->irq_type[d->hwirq] = type;
>
> It would make more sense if this this assignment was done *after*
> sanitizing the type value.
Ok.
>
>>> +
>>> + switch (type) {
>>> + case IRQ_TYPE_EDGE_RISING:
>>> + case IRQ_TYPE_EDGE_FALLING:
>>> + case IRQ_TYPE_EDGE_BOTH:
>>> + parent_type = type;
>>> + break;
>>> + case IRQ_TYPE_LEVEL_HIGH:
>>> + parent_type = IRQ_TYPE_EDGE_RISING;
>>> + break;
>>> + case IRQ_TYPE_LEVEL_LOW:
>>> + parent_type = IRQ_TYPE_EDGE_FALLING;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return irq_chip_set_type_parent(d, parent_type);
>>> +};
>>> +
>>> static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>>> {
>>> struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>>> @@ -332,11 +373,11 @@ static void
>>> stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>>> static struct irq_chip stm32_gpio_irq_chip = {
>>> .name = "stm32gpio",
>>> - .irq_eoi = irq_chip_eoi_parent,
>>> + .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_set_type = irq_chip_set_type_parent,
>>> + .irq_set_type = stm32_gpio_set_type,
>>> .irq_set_wake = irq_chip_set_wake_parent,
>>> .irq_request_resources = stm32_gpio_irq_request_resources,
>>> .irq_release_resources = stm32_gpio_irq_release_resources,
>>>
>
> Thanks,
>
> M.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-19 11:43 ` Marc Zyngier
@ 2020-02-19 13:07 ` Alexandre Torgue
2020-02-19 13:13 ` Marc Zyngier
0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 13:07 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-19 13:07 ` Alexandre Torgue
@ 2020-02-19 13:13 ` Marc Zyngier
2020-02-19 13:17 ` Alexandre Torgue
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2020-02-19 13:13 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support
2020-02-19 13:13 ` Marc Zyngier
@ 2020-02-19 13:17 ` Alexandre Torgue
0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-19 13:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jason Cooper, Linus Walleij, marex, linux-gpio,
linux-kernel, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-19 9:20 ` Alexandre Torgue
@ 2020-02-19 17:24 ` Marek Vasut
2020-02-20 13:09 ` Alexandre Torgue
0 siblings, 1 reply; 20+ messages in thread
From: Marek Vasut @ 2020-02-19 17:24 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/19/20 10:20 AM, Alexandre Torgue wrote:
Hi,
[...]
>>>>> This series adds the possibility to handle gpio interrupts on level.
>>>>>
>>>>> 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.
>>>>>
>>>>> In exti irq chip, retrigger ops function is added.
>>>>
>>>> btw. this might be unrelated, but is it possible to have e.g. gpioC2
>>>> set
>>>> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
>>>> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux
>>>> selection")
>>>> prevents that.
>>>>
>>>
>>> No it's not possible. Each gpio line doesn't have a dedicated Exti line
>>> Each Exti line is muxing between gpio banks.
>>
>> OK, that confirms my assumption.
>>
>>> Mapping is done as following:
>>>
>>> EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
>>> EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
>>> EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
>>> ...
>>
>> Is it at least possible to have IRQs of the same type on the same exti
>> line? E.g. gpioA2 of trigger-edge-falling and gpioB2
>> trigger-edge-falling ?
>>
>
> Sorry I don't catch your point. If you already succeed to get gpioA2,
> then you will failed to get gpioB2 but looking at function call stack we
> could get an other issue.
Considering the EXTI line limitations, I'd like to know what kind of IRQ
input configuration is allowed/valid and what kind of configuration is
not valid.
> Lets take example where you succeed to get gpioa2 as interrupt (using
> interrupt bindings) and now you try to do the same for gpiob2, you will
> have (roughly):
>
> stm32_gpio_irq_request_resources (for gpiob2) --> succeed
>
> stm32_gpio_set_type
> |
> |--> stm32_exti_set_type type -> change exti line 2 trigger registers
> with gpiob2 binding.
>
> stm32_gpio_domain_activate --> failed as exti line2 is already used
> by gpioa2.
>
> So as stm32_gpio_set_type is called before checking than exti line is
> available, type could be changed and behavior of gpioa2 interrupt broken.
>
> Solution would be to move the exti line mux check from
> stm32_gpio_domain_activate to stm32_gpio_irq_request_resources callback.
So the hardware does support using both gpioA2 and gpioB2 as an
interrupt source, for different drivers, if they are of the same
interrupt type. Except the current implementation does not permit that.
If the interrupt types are different, that is not supported by the hardware.
Correct ?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-19 17:24 ` Marek Vasut
@ 2020-02-20 13:09 ` Alexandre Torgue
2020-02-21 16:41 ` Marek Vasut
0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Torgue @ 2020-02-20 13:09 UTC (permalink / raw)
To: Marek Vasut, Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
Hi Marek
On 2/19/20 6:24 PM, Marek Vasut wrote:
> On 2/19/20 10:20 AM, Alexandre Torgue wrote:
> Hi,
> [...]
>>>>>> This series adds the possibility to handle gpio interrupts on level.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> In exti irq chip, retrigger ops function is added.
>>>>>
>>>>> btw. this might be unrelated, but is it possible to have e.g. gpioC2
>>>>> set
>>>>> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It seems
>>>>> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux
>>>>> selection")
>>>>> prevents that.
>>>>>
>>>>
>>>> No it's not possible. Each gpio line doesn't have a dedicated Exti line
>>>> Each Exti line is muxing between gpio banks.
>>>
>>> OK, that confirms my assumption.
>>>
>>>> Mapping is done as following:
>>>>
>>>> EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
>>>> EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
>>>> EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
>>>> ...
>>>
>>> Is it at least possible to have IRQs of the same type on the same exti
>>> line? E.g. gpioA2 of trigger-edge-falling and gpioB2
>>> trigger-edge-falling ?
>>>
>>
>> Sorry I don't catch your point. If you already succeed to get gpioA2,
>> then you will failed to get gpioB2 but looking at function call stack we
>> could get an other issue.
>
> Considering the EXTI line limitations, I'd like to know what kind of IRQ
> input configuration is allowed/valid and what kind of configuration is
> not valid.
As a mux is used to select which GPIO[A..Z]_X has to be mapped on exti_X
line, only one GPIO can be used on the EXTI line.
For example, on EXTI2 you could map either gpioa2 or gpiob2 or
....gpioz2 but not gpioa2 and gpiob2 in the same time.
>
>> Lets take example where you succeed to get gpioa2 as interrupt (using
>> interrupt bindings) and now you try to do the same for gpiob2, you will
>> have (roughly):
>>
>> stm32_gpio_irq_request_resources (for gpiob2) --> succeed
>>
>> stm32_gpio_set_type
>> |
>> |--> stm32_exti_set_type type -> change exti line 2 trigger registers
>> with gpiob2 binding.
>>
>> stm32_gpio_domain_activate --> failed as exti line2 is already used
>> by gpioa2.
>>
>> So as stm32_gpio_set_type is called before checking than exti line is
>> available, type could be changed and behavior of gpioa2 interrupt broken.
>>
>> Solution would be to move the exti line mux check from
>> stm32_gpio_domain_activate to stm32_gpio_irq_request_resources callback.
>
> So the hardware does support using both gpioA2 and gpioB2 as an
> interrupt source, for different drivers, if they are of the same
> interrupt type. Except the current implementation does not permit that.
>
No hardware doesn't allow it for reason explain above.
> If the interrupt types are different, that is not supported by the hardware.
>
> Correct ?
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/2] Add GPIO level-sensitive interrupt support
2020-02-20 13:09 ` Alexandre Torgue
@ 2020-02-21 16:41 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2020-02-21 16:41 UTC (permalink / raw)
To: Alexandre Torgue, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Linus Walleij
Cc: linux-arm-kernel, linux-kernel, linux-gpio
On 2/20/20 2:09 PM, Alexandre Torgue wrote:
> Hi Marek
Hi,
> On 2/19/20 6:24 PM, Marek Vasut wrote:
>> On 2/19/20 10:20 AM, Alexandre Torgue wrote:
>> Hi,
>> [...]
>>>>>>> This series adds the possibility to handle gpio interrupts on level.
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> In exti irq chip, retrigger ops function is added.
>>>>>>
>>>>>> btw. this might be unrelated, but is it possible to have e.g. gpioC2
>>>>>> set
>>>>>> as trigger-level-low and gpioD2 set as trigger-edge-falling ? It
>>>>>> seems
>>>>>> 8eb2dfee9fb1 ("pinctrl: stm32: add lock mechanism for irqmux
>>>>>> selection")
>>>>>> prevents that.
>>>>>>
>>>>>
>>>>> No it's not possible. Each gpio line doesn't have a dedicated Exti
>>>>> line
>>>>> Each Exti line is muxing between gpio banks.
>>>>
>>>> OK, that confirms my assumption.
>>>>
>>>>> Mapping is done as following:
>>>>>
>>>>> EXTI0 = A0 or B0 or C0 .... or Z0 : selected by Mux
>>>>> EXTI1 = A1 or B1 or C1 ....or Z1 : selected by Mux
>>>>> EXTI2 = A2 or B2 or C2 ....or Z2 : selected by Mux
>>>>> ...
>>>>
>>>> Is it at least possible to have IRQs of the same type on the same exti
>>>> line? E.g. gpioA2 of trigger-edge-falling and gpioB2
>>>> trigger-edge-falling ?
>>>>
>>>
>>> Sorry I don't catch your point. If you already succeed to get gpioA2,
>>> then you will failed to get gpioB2 but looking at function call stack we
>>> could get an other issue.
>>
>> Considering the EXTI line limitations, I'd like to know what kind of IRQ
>> input configuration is allowed/valid and what kind of configuration is
>> not valid.
>
> As a mux is used to select which GPIO[A..Z]_X has to be mapped on exti_X
> line, only one GPIO can be used on the EXTI line.
>
> For example, on EXTI2 you could map either gpioa2 or gpiob2 or
> ....gpioz2 but not gpioa2 and gpiob2 in the same time.
Got it, thank you for the clarification.
Maybe that could be something to improve for MP2 :-)
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-02-21 16:41 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 13:12 [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 1/2] irqchip/stm32: Add irq retrigger support Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:33 ` Alexandre Torgue
2020-02-19 11:43 ` Marc Zyngier
2020-02-19 13:07 ` Alexandre Torgue
2020-02-19 13:13 ` Marc Zyngier
2020-02-19 13:17 ` Alexandre Torgue
2020-02-18 13:12 ` [PATCH v2 2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip Alexandre Torgue
2020-02-18 17:51 ` Marek Vasut
2020-02-19 11:34 ` Alexandre Torgue
2020-02-19 12:07 ` Marc Zyngier
2020-02-19 12:59 ` Alexandre Torgue
2020-02-18 16:25 ` [PATCH v2 0/2] Add GPIO level-sensitive interrupt support Marek Vasut
2020-02-18 18:10 ` Alexandre Torgue
2020-02-18 18:13 ` Marek Vasut
2020-02-19 9:20 ` Alexandre Torgue
2020-02-19 17:24 ` Marek Vasut
2020-02-20 13:09 ` Alexandre Torgue
2020-02-21 16:41 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).