[PATCH 1/3] gpio: zynq: Take bank offset into account when reporting a IRQ

[PATCH 1/3] gpio: zynq: Take bank offset into account when reporting a IRQ

[Lars-Peter Clausen]

When looking up the IRQ the bank offset needs to be taken into account.
Otherwise interrupts for banks other than bank 0 get incorrectly reported as
interrupts for bank 0.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpio/gpio-zynq.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index c0c53fd..8e6a32f 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -136,6 +136,29 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
 }
 
 /**
+ * zynq_gpio_get_bank_pin - Gets the pin number of the first pin in a bank
+ * @bank: The bank for which to return the first pin number
+ *
+ * Returns the pin number of the first pin in the specified bank
+ */
+static int zynq_gpio_get_bank_offset(unsigned int bank)
+{
+	switch (bank) {
+	case 0:
+		return ZYNQ_GPIO_BANK0_PIN_MIN;
+	case 1:
+		return ZYNQ_GPIO_BANK1_PIN_MIN;
+	case 2:
+		return ZYNQ_GPIO_BANK2_PIN_MIN;
+	case 3:
+		return ZYNQ_GPIO_BANK3_PIN_MIN;
+	default:
+		/* We'll never get here */
+		return -1;
+	}
+}
+
+/**
  * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
  * @chip:	gpio_chip instance to be worked on
  * @pin:	gpio pin number within the device
@@ -419,11 +442,12 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
 		if (int_sts) {
 			int offset;
 			unsigned long pending = int_sts;
+			int bank_offset = zynq_gpio_get_bank_offset(bank_num);
 
 			for_each_set_bit(offset, &pending, 32) {
 				unsigned int gpio_irq =
 					irq_find_mapping(gpio->chip.irqdomain,
-							offset);
+							 offset + bank_offset);
 				generic_handle_irq(gpio_irq);
 			}
 
-- 
1.8.0

[PATCH 2/3] gpio: zynq: Clear pending interrupt when enabling a IRQ

[Lars-Peter Clausen]

The Zynq GPIO controller does not disable the interrupt detection when the
interrupt is masked and only disables the propagation of the interrupt. This
means when the controller detects an interrupt condition while the interrupt is
logically disabled (and masked) it will propagate the recorded interrupt event
once the interrupt is enabled. This will cause the interrupt consumer to see
spurious interrupts to prevent this first make sure that the interrupt is not
asserted and then enable it.

E.g. when a interrupt is requested with request_irq() it will be configured
according to the requested type (edge/level triggered, etc.) after that it will
be enabled. But the detection circuit might have already registered a false
interrupt before the interrupt type was correctly configured and once the
interrupt is unmasked this false interrupt will be propagated and the interrupt
handler for the just request interrupt will called.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/gpio/gpio-zynq.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 8e6a32f..fa3ad23 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -324,6 +324,48 @@ static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
 }
 
 /**
+ * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to ack
+ *
+ * This function calculates gpio pin number from irq number and sets the bit
+ * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
+ */
+static void zynq_gpio_irq_ack(struct irq_data *irq_data)
+{
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	writel_relaxed(BIT(bank_pin_num),
+		       gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
+}
+
+/**
+ * zynq_gpio_irq_enable - Enable the interrupts for a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to enable
+ *
+ * Clears the INTSTS bit and unmasks the given interrrupt.
+ */
+static void zynq_gpio_irq_enable(struct irq_data *irq_data)
+{
+	/*
+	 * The Zynq GPIO controller does not disable interrupt detection when
+	 * the interrupt is masked and only disables the propagation of the
+	 * interrupt. This means when the controller detects an interrupt
+	 * condition while the interrupt is logically disabled it will propagate
+	 * that interrupt event once the interrupt is enabled. This will cause
+	 * the interrupt consumer to see spurious interrupts to prevent this
+	 * first make sure that the interrupt is not asserted and then enable
+	 * it.
+	 */
+	zynq_gpio_irq_ack(irq_data);
+	zynq_gpio_irq_unmask(irq_data);
+}
+
+/**
  * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
  * @irq_data:	irq data containing irq number of gpio pin
  * @type:	interrupt type that is to be set for the gpio pin
@@ -407,6 +449,7 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 /* irq chip descriptor */
 static struct irq_chip zynq_gpio_irqchip = {
 	.name		= DRIVER_NAME,
+	.irq_enable	= zynq_gpio_irq_enable,
 	.irq_mask	= zynq_gpio_irq_mask,
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
-- 
1.8.0

Re: [PATCH 2/3] gpio: zynq: Clear pending interrupt when enabling a IRQ

[Linus Walleij]

On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> The Zynq GPIO controller does not disable the interrupt detection when the
> interrupt is masked and only disables the propagation of the interrupt. This
> means when the controller detects an interrupt condition while the interrupt is
> logically disabled (and masked) it will propagate the recorded interrupt event
> once the interrupt is enabled. This will cause the interrupt consumer to see
> spurious interrupts to prevent this first make sure that the interrupt is not
> asserted and then enable it.
>
> E.g. when a interrupt is requested with request_irq() it will be configured
> according to the requested type (edge/level triggered, etc.) after that it will
> be enabled. But the detection circuit might have already registered a false
> interrupt before the interrupt type was correctly configured and once the
> interrupt is unmasked this false interrupt will be propagated and the interrupt
> handler for the just request interrupt will called.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Seems like you know what you're doing.

Patch tentatively applied unless Harini or Soren protests...

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: zynq: Clear pending interrupt when enabling a IRQ

[Sören Brinkmann]

On Wed, 2014-07-23 at 04:31PM +0200, Linus Walleij wrote:
> On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
> > The Zynq GPIO controller does not disable the interrupt detection when the
> > interrupt is masked and only disables the propagation of the interrupt. This
> > means when the controller detects an interrupt condition while the interrupt is
> > logically disabled (and masked) it will propagate the recorded interrupt event
> > once the interrupt is enabled. This will cause the interrupt consumer to see
> > spurious interrupts to prevent this first make sure that the interrupt is not
> > asserted and then enable it.
> >
> > E.g. when a interrupt is requested with request_irq() it will be configured
> > according to the requested type (edge/level triggered, etc.) after that it will
> > be enabled. But the detection circuit might have already registered a false
> > interrupt before the interrupt type was correctly configured and once the
> > interrupt is unmasked this false interrupt will be propagated and the interrupt
> > handler for the just request interrupt will called.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> Seems like you know what you're doing.
> 
> Patch tentatively applied unless Harini or Soren protests...

All these things look good to me, though I thought I had tested
interrupts on banks other than zero, but it might have slipped through.

Since I'm not subscribed to linux-gpio, I don't have the proper patches
in my mailbox, is there a patchworks instance for linux-gpio? I couldn't
find any :(

	Thanks,
	Soren

Re: [PATCH 2/3] gpio: zynq: Clear pending interrupt when enabling a IRQ

[Alexandre Courbot]

On Thu, Jul 24, 2014 at 2:42 AM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> Since I'm not subscribed to linux-gpio, I don't have the proper patches
> in my mailbox, is there a patchworks instance for linux-gpio? I couldn't
> find any :(

Not yet, but it would be nice to have one. Does anyone knows the
procedure to add a project to http://patchwork.ozlabs.org/ ?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] gpio: zynq: Clear pending interrupt when enabling a IRQ

[Harini Katakam]

Hi Lars-Peter,

On Wed, Jul 23, 2014 at 11:12 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Wed, 2014-07-23 at 04:31PM +0200, Linus Walleij wrote:
>> On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>> > The Zynq GPIO controller does not disable the interrupt detection when the
>> > interrupt is masked and only disables the propagation of the interrupt. This
>> > means when the controller detects an interrupt condition while the interrupt is
>> > logically disabled (and masked) it will propagate the recorded interrupt event
>> > once the interrupt is enabled. This will cause the interrupt consumer to see
>> > spurious interrupts to prevent this first make sure that the interrupt is not
>> > asserted and then enable it.
>> >
>> > E.g. when a interrupt is requested with request_irq() it will be configured
>> > according to the requested type (edge/level triggered, etc.) after that it will
>> > be enabled. But the detection circuit might have already registered a false
>> > interrupt before the interrupt type was correctly configured and once the
>> > interrupt is unmasked this false interrupt will be propagated and the interrupt
>> > handler for the just request interrupt will called.
>> >
>> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>
>> Seems like you know what you're doing.
>>
>> Patch tentatively applied unless Harini or Soren protests...
>
> All these things look good to me, though I thought I had tested
> interrupts on banks other than zero, but it might have slipped through.
>

Sorry for the delay - I was away for a few days.
The change looks OK to me.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] gpio: zynq: Fix IRQ handlers

[Lars-Peter Clausen]

The Zynq GPIO interrupt handling code as two main issues:
1) It does not support IRQF_ONESHOT interrupt since it uses handle_simple_irq()
for the interrupt handler. handle_simple_irq() does not do masking and unmasking
of the IRQ that is required for this chip to be able to support IRQF_ONESHOT
IRQs, causing the CPU to lock up in a interrupt storm if such a interrupt is
requested.
2) Interrupts are acked after the primary interrupt handlers for all asserted
interrupts in a bank have been called. For edge triggered interrupt this is to
late and may cause a interrupt to be missed. For level triggered oneshot
interrupts this is to early and causes the interrupt handler to run twice per
interrupt.

This patch addresses the issue by updating the driver to use the correct IRQ
chip handler functions that are appropriate for this kind of IRQ controller.

The following diagram gives an overview of how the interrupt detection circuit
works, it is not necessarily a accurate depiction of the real hardware though.

     INT_POL/INT_ON_ANY
           |
           | +---+                       INT_STATUS
           `-|   |                            |
             | E |-.                          |
         ,---|   |  \     |\          +----+  |  +---+
         |   +---+   `----| | ,-------|S   | ,*--|   |
GPIO_IN -*                | |-        |   Q|-    | & |-- IRQ_OUT
         |   +---+  ,-----| |       ,-|R   |   ,o|   |
         `---|   | /      |/       |  +----+  |  +---+
             | = |-        |       |          |
           ,-|   |    INT_TYPE    ACK     INT_MASK
           | +---+
           |
        INT_POL

GPIO_IN is the raw signal level connected to the hardware pin. This signal is
routed to a edge detector and to a level detector. The edge detector can be
configured to either detect a rising or falling edge or both edges. The level
detector can detect either a level high or level low event. Depending on the
setting of the INT_TYPE register either the edge or level event will be
propagated to the INT_STATUS register. As long as a interrupt condition is
detected the INT_STATUS register will be set to 1. It can be cleared to 0 if
(and only if) the interrupt condition is no longer detected and software
acknowledges the interrupt by writing a 1 to the address of the INT_STATUS
register. There is also the INT_MASK register which can be used to disable the
propagation of the INT_STATUS signal to the upstream IRQ controller. What is
important to note is that the interrupt detection logic itself can not be
disabled, only the propagation of the INT_STATUS register can be delayed. This
means that for level type interrupts the interrupt must only be acknowledged
after the interrupt source has been cleared otherwise it will stay asserted and
the interrupt handler will be run a second time. For IRQF_ONESHOT interrupts
this means that the IRQ must only be acknowledged after the threaded interrupt
has finished running. If a second interrupt comes in between handling the first
interrupt and acknowledging it the external interrupt will be asserted, which
means trying to acknowledge the first interrupt will not clear the INT_STATUS
register and the interrupt handler will be run a second time when the IRQ is
unmasked, so no interrupts will be lost. The handle_fasteoi_irq() handler in
combination with the IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED flags will
have the desired behavior. For edge triggered interrupts a slightly different
strategy is necessary. For edge triggered interrupts the interrupt condition is
only true when the edge itself is detected, this means this is the only time the
INT_STATUS register is set, acknowledging the interrupt any time after that will
clear the INT_STATUS register until the next interrupt happens. This means in
order to not loose any interrupts the interrupt needs to be acknowledged before
running the interrupt handler. If a second interrupt occurs after the first
interrupt handler has finished but before the interrupt is unmasked the
INT_STATUS register will be re-asserted and the interrupt handler runs a second
time once the interrupt is unmasked. This means with this flow handling strategy
no interrupts are lost for edge triggered interrupts. The handle_level_irq()
handler will have the desired behavior. (Note: The handle_edge_irq() only needs
to be used for edge triggered interrupts where the controller stops detecting
the interrupt event when the interrupt is masked, for this controller the
detection logic still works, while only the propagation is delayed when the
interrupt is masked.)

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Conflicts:
	drivers/gpio/gpio-zynq.c
---
 drivers/gpio/gpio-zynq.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index fa3ad23..bfbcf97 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -95,6 +95,9 @@ struct zynq_gpio {
 	struct clk *clk;
 };
 
+static struct irq_chip zynq_gpio_level_irqchip;
+static struct irq_chip zynq_gpio_edge_irqchip;
+
 /**
  * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
  * for a given pin in the GPIO device
@@ -433,6 +436,15 @@ static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
 		       gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
 	writel_relaxed(int_any,
 		       gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+
+	if (type & IRQ_TYPE_LEVEL_MASK) {
+		__irq_set_chip_handler_name_locked(irq_data->irq,
+			&zynq_gpio_level_irqchip, handle_fasteoi_irq, NULL);
+	} else {
+		__irq_set_chip_handler_name_locked(irq_data->irq,
+			&zynq_gpio_edge_irqchip, handle_level_irq, NULL);
+	}
+
 	return 0;
 }
 
@@ -447,9 +459,21 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 }
 
 /* irq chip descriptor */
-static struct irq_chip zynq_gpio_irqchip = {
+static struct irq_chip zynq_gpio_level_irqchip = {
 	.name		= DRIVER_NAME,
 	.irq_enable	= zynq_gpio_irq_enable,
+	.irq_eoi	= zynq_gpio_irq_ack,
+	.irq_mask	= zynq_gpio_irq_mask,
+	.irq_unmask	= zynq_gpio_irq_unmask,
+	.irq_set_type	= zynq_gpio_set_irq_type,
+	.irq_set_wake	= zynq_gpio_set_wake,
+	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED,
+};
+
+static struct irq_chip zynq_gpio_edge_irqchip = {
+	.name		= DRIVER_NAME,
+	.irq_enable	= zynq_gpio_irq_enable,
+	.irq_ack	= zynq_gpio_irq_ack,
 	.irq_mask	= zynq_gpio_irq_mask,
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
@@ -493,10 +517,6 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
 							 offset + bank_offset);
 				generic_handle_irq(gpio_irq);
 			}
-
-			/* clear IRQ in HW */
-			writel_relaxed(int_sts, gpio->base_addr +
-					ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
 		}
 	}
 
@@ -634,14 +654,14 @@ static int zynq_gpio_probe(struct platform_device *pdev)
 		writel_relaxed(ZYNQ_GPIO_IXR_DISABLE_ALL, gpio->base_addr +
 			       ZYNQ_GPIO_INTDIS_OFFSET(bank_num));
 
-	ret = gpiochip_irqchip_add(chip, &zynq_gpio_irqchip, 0,
-				   handle_simple_irq, IRQ_TYPE_NONE);
+	ret = gpiochip_irqchip_add(chip, &zynq_gpio_edge_irqchip, 0,
+				   handle_level_irq, IRQ_TYPE_NONE);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add irq chip\n");
 		goto err_rm_gpiochip;
 	}
 
-	gpiochip_set_chained_irqchip(chip, &zynq_gpio_irqchip, irq,
+	gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, irq,
 				     zynq_gpio_irqhandler);
 
 	pm_runtime_set_active(&pdev->dev);
-- 
1.8.0

Re: [PATCH 3/3] gpio: zynq: Fix IRQ handlers

[Varka Bhadram]

On 07/18/2014 03:22 PM, Lars-Peter Clausen wrote:
>   drivers/gpio/gpio-zynq.c | 36 ++++++++++++++++++++++++++++--------
>   1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index fa3ad23..bfbcf97 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -95,6 +95,9 @@ struct zynq_gpio {
>   	struct clk *clk;
>   };
>   
> +static struct irq_chip zynq_gpio_level_irqchip;
> +static struct irq_chip zynq_gpio_edge_irqchip;
> +
>   /**
>    * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
>    * for a given pin in the GPIO device
> @@ -433,6 +436,15 @@ static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
>   		       gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
>   	writel_relaxed(int_any,
>   		       gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num));
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK) {
> +		__irq_set_chip_handler_name_locked(irq_data->irq,
> +			&zynq_gpio_level_irqchip, handle_fasteoi_irq, NULL);

Should match open parenthesis:
	__irq_set_chip_handler_name_locked(irq_data->irq,
					   &zynq_gpio_level_irqchip,
					   handle_fasteoi_irq, NULL);

> +	} else {
> +		__irq_set_chip_handler_name_locked(irq_data->irq,
> +			&zynq_gpio_edge_irqchip, handle_level_irq, NULL);

Dto..

> +	}
> +
>   	return 0;
>   }
>   
> @@ -447,9 +459,21 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
>   }
>   
>   /* irq chip descriptor */
> -static struct irq_chip zynq_gpio_irqchip = {
> +static struct irq_chip zynq_gpio_level_irqchip = {
>   	.name		= DRIVER_NAME,
>   	.irq_enable	= zynq_gpio_irq_enable,
> +	.irq_eoi	= zynq_gpio_irq_ack,
> +	.irq_mask	= zynq_gpio_irq_mask,
> +	.irq_unmask	= zynq_gpio_irq_unmask,
> +	.irq_set_type	= zynq_gpio_set_irq_type,
> +	.irq_set_wake	= zynq_gpio_set_wake,
> +	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED,
> +};
> +
> +static struct irq_chip zynq_gpio_edge_irqchip = {
> +	.name		= DRIVER_NAME,
> +	.irq_enable	= zynq_gpio_irq_enable,
> +	.irq_ack	= zynq_gpio_irq_ack,
>   	.irq_mask	= zynq_gpio_irq_mask,
>   	.irq_unmask	= zynq_gpio_irq_unmask,
>   	.irq_set_type	= zynq_gpio_set_irq_type,
> @@ -493,10 +517,6 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
>   							 offset + bank_offset);
>   				generic_handle_irq(gpio_irq);
>   			}
> -
> -			/* clear IRQ in HW */
> -			writel_relaxed(int_sts, gpio->base_addr +
> -					ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
>   		}
>   	}
>   
> @@ -634,14 +654,14 @@ static int zynq_gpio_probe(struct platform_device *pdev)
>   		writel_relaxed(ZYNQ_GPIO_IXR_DISABLE_ALL, gpio->base_addr +
>   			       ZYNQ_GPIO_INTDIS_OFFSET(bank_num));
>   
> -	ret = gpiochip_irqchip_add(chip, &zynq_gpio_irqchip, 0,
> -				   handle_simple_irq, IRQ_TYPE_NONE);
> +	ret = gpiochip_irqchip_add(chip, &zynq_gpio_edge_irqchip, 0,
> +				   handle_level_irq, IRQ_TYPE_NONE);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to add irq chip\n");
>   		goto err_rm_gpiochip;
>   	}
>   
> -	gpiochip_set_chained_irqchip(chip, &zynq_gpio_irqchip, irq,
> +	gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, irq,
>   				     zynq_gpio_irqhandler);
>   
>   	pm_runtime_set_active(&pdev->dev);


-- 
Regards,
Varka Bhadram.

Re: [PATCH 3/3] gpio: zynq: Fix IRQ handlers

[Linus Walleij]

On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> The Zynq GPIO interrupt handling code as two main issues:
> 1) It does not support IRQF_ONESHOT interrupt since it uses handle_simple_irq()
> for the interrupt handler. handle_simple_irq() does not do masking and unmasking
> of the IRQ that is required for this chip to be able to support IRQF_ONESHOT
> IRQs, causing the CPU to lock up in a interrupt storm if such a interrupt is
> requested.
> 2) Interrupts are acked after the primary interrupt handlers for all asserted
> interrupts in a bank have been called. For edge triggered interrupt this is to
> late and may cause a interrupt to be missed. For level triggered oneshot
> interrupts this is to early and causes the interrupt handler to run twice per
> interrupt.
>
> This patch addresses the issue by updating the driver to use the correct IRQ
> chip handler functions that are appropriate for this kind of IRQ controller.

This looks very thought-through. I will give Harini and Soren some days
more to react before applying (unless patch 1/3 is required to apply this).

I can fix up Varka's comment when applying, no need to resend for that
feedback only.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: zynq: Fix IRQ handlers

[Sören Brinkmann]

On Wed, 2014-07-23 at 04:36PM +0200, Linus Walleij wrote:
> On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
> > The Zynq GPIO interrupt handling code as two main issues:
> > 1) It does not support IRQF_ONESHOT interrupt since it uses handle_simple_irq()
> > for the interrupt handler. handle_simple_irq() does not do masking and unmasking
> > of the IRQ that is required for this chip to be able to support IRQF_ONESHOT
> > IRQs, causing the CPU to lock up in a interrupt storm if such a interrupt is
> > requested.
> > 2) Interrupts are acked after the primary interrupt handlers for all asserted
> > interrupts in a bank have been called. For edge triggered interrupt this is to
> > late and may cause a interrupt to be missed. For level triggered oneshot
> > interrupts this is to early and causes the interrupt handler to run twice per
> > interrupt.
> >
> > This patch addresses the issue by updating the driver to use the correct IRQ
> > chip handler functions that are appropriate for this kind of IRQ controller.
> 
> This looks very thought-through. I will give Harini and Soren some days
> more to react before applying (unless patch 1/3 is required to apply this).

Took me a while, but I finally got to pulling the diffs from some
web-archive and applied them on the current linux-next. I didn't test
any of the new/fixed functionality, but it doesn't break any of my old
use-cases. So, I think this is all good.

For the whole series:
Acked-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

	Thanks,
	Sören

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] gpio: zynq: Fix IRQ handlers

[Linus Walleij]

On Fri, Jul 18, 2014 at 11:52 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> The Zynq GPIO interrupt handling code as two main issues:
(...)
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Patch applied with Sören's ACK!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] gpio: zynq: Take bank offset into account when reporting a IRQ

[Alexandre Courbot]

On Fri, Jul 18, 2014 at 6:52 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> When looking up the IRQ the bank offset needs to be taken into account.
> Otherwise interrupts for banks other than bank 0 get incorrectly reported as
> interrupts for bank 0.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/gpio/gpio-zynq.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index c0c53fd..8e6a32f 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -136,6 +136,29 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
>  }
>
>  /**
> + * zynq_gpio_get_bank_pin - Gets the pin number of the first pin in a bank
> + * @bank: The bank for which to return the first pin number
> + *
> + * Returns the pin number of the first pin in the specified bank
> + */
> +static int zynq_gpio_get_bank_offset(unsigned int bank)
> +{
> +       switch (bank) {
> +       case 0:
> +               return ZYNQ_GPIO_BANK0_PIN_MIN;
> +       case 1:
> +               return ZYNQ_GPIO_BANK1_PIN_MIN;
> +       case 2:
> +               return ZYNQ_GPIO_BANK2_PIN_MIN;
> +       case 3:
> +               return ZYNQ_GPIO_BANK3_PIN_MIN;
> +       default:
> +               /* We'll never get here */
> +               return -1;
> +       }
> +}

Wouldn't this be handled better by a simple, static array? I.e.

static int zynq_gpio_bank_offset[] = {
        ZYNQ_GPIO_BANK0_PIN_MIN,
        ZYNQ_GPIO_BANK1_PIN_MIN,
        ZYNQ_GPIO_BANK2_PIN_MIN,
        ZYNQ_GPIO_BANK3_PIN_MIN
};

...

        int bank offset = zynq_gpio_bank_offset(bank_num);

> +
> +/**
>   * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
>   * @chip:      gpio_chip instance to be worked on
>   * @pin:       gpio pin number within the device
> @@ -419,11 +442,12 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
>                 if (int_sts) {
>                         int offset;
>                         unsigned long pending = int_sts;
> +                       int bank_offset = zynq_gpio_get_bank_offset(bank_num);
>
>                         for_each_set_bit(offset, &pending, 32) {
>                                 unsigned int gpio_irq =
>                                         irq_find_mapping(gpio->chip.irqdomain,
> -                                                       offset);
> +                                                        offset + bank_offset);
>                                 generic_handle_irq(gpio_irq);
>                         }
>
> --
> 1.8.0
>

Re: [PATCH 1/3] gpio: zynq: Take bank offset into account when reporting a IRQ

[Linus Walleij]

On Sat, Jul 19, 2014 at 6:14 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 6:52 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:

>> +static int zynq_gpio_get_bank_offset(unsigned int bank)
>> +{
>> +       switch (bank) {
>> +       case 0:
>> +               return ZYNQ_GPIO_BANK0_PIN_MIN;
>> +       case 1:
>> +               return ZYNQ_GPIO_BANK1_PIN_MIN;
>> +       case 2:
>> +               return ZYNQ_GPIO_BANK2_PIN_MIN;
>> +       case 3:
>> +               return ZYNQ_GPIO_BANK3_PIN_MIN;
>> +       default:
>> +               /* We'll never get here */
>> +               return -1;
>> +       }
>> +}
>
> Wouldn't this be handled better by a simple, static array? I.e.
>
> static int zynq_gpio_bank_offset[] = {
>         ZYNQ_GPIO_BANK0_PIN_MIN,
>         ZYNQ_GPIO_BANK1_PIN_MIN,
>         ZYNQ_GPIO_BANK2_PIN_MIN,
>         ZYNQ_GPIO_BANK3_PIN_MIN
> };
>
> ...
>
>         int bank offset = zynq_gpio_bank_offset(bank_num);

I agree, Lars-Peter can  you please rewrite the patch to do it this way instead.

Yours,
Linus Walleij