diff mbox series

[v2,2/2] pinctrl: stm32: Add level interrupt support to gpio irq chip

Message ID 20200218131218.10789-3-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 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>

Comments

Marek Vasut Feb. 18, 2020, 5:51 p.m. UTC | #1 
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)
Alexandre TORGUE Feb. 19, 2020, 11:34 a.m. UTC | #2 
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,
>
Marc Zyngier Feb. 19, 2020, 12:07 p.m. UTC | #3 
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.
Alexandre TORGUE Feb. 19, 2020, 12:59 p.m. UTC | #4 
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.
diff mbox series

Patch

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,