[PATCH v3 0/4] pinctrl: sunxi: Properly handle level triggered gpio irqs

[PATCH v3 0/4] pinctrl: sunxi: Properly handle level triggered gpio irqs

[Hans de Goede]

Hi all,

Here is v3 of my sunxi-pinctrl gpio irq patches, mainly aimed at get level
triggered irqs to work properly for use with oob sdio irqs.

Changes since v1:
-Post these patches as a separate series, rather then as part of a larger
 patchset touching multiple subsystems
-"pinctrl: sunxi: Add IRQCHIP_SKIP_SET_WAKE flag for pinctrl irq chip"
 Dropped the setting of the irq_chip name
-"pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resourc
 -Use irq_request_resources callback
 -Added a check for func == NULL
-"pinctrl: sunxi: Define enable / disable irq callbacks for level triggered irqs
 -Use sunxi_pinctrl_irq_ack() and sunxi_pinctrl_irq_unmask()

Changes since v2:
-"pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resourc
 Added Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
-"pinctrl: sunxi: Properly handle level triggered gpio interrupts"
 Use tentative declaration of the irq_chip structs to avoid the need for moving
 their real declaration around

Regards,

Hans 

[PATCH v3 1/4] pinctrl: sunxi: Add IRQCHIP_SKIP_SET_WAKE flag for pinctrl irq chip

[Hans de Goede]

From: Chen-Yu Tsai <wens@csie.org>

The sunxi pinctrl irq chip driver does not support wakeup at the
moment. Adding IRQCHIP_SKIP_SET_WAKE lets the irqs work with drivers
using wakeup.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index d989a10..c199337 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -630,6 +630,7 @@ static struct irq_chip sunxi_pinctrl_irq_chip = {
 	.irq_mask	= sunxi_pinctrl_irq_mask,
 	.irq_unmask	= sunxi_pinctrl_irq_unmask,
 	.irq_set_type	= sunxi_pinctrl_irq_set_type,
+	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
-- 
2.0.0

[PATCH v3 2/4] pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resources

[Hans de Goede]

With level triggered interrupt mask / unmask will get called for each
interrupt, doing the somewhat expensive mux setting on each unmask thus is
not a good idea. Instead add a request_resources callback and do it there.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index c199337..8bdd65b 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -531,6 +531,21 @@ static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 	return irq_find_mapping(pctl->domain, desc->irqnum);
 }
 
+static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
+{
+	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+	struct sunxi_desc_function *func;
+
+	func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
+					pctl->irq_array[d->hwirq], "irq");
+	if (!func)
+		return -EINVAL;
+
+	/* Change muxing to INT mode */
+	sunxi_pmx_set(pctl->pctl_dev, pctl->irq_array[d->hwirq], func->muxval);
+
+	return 0;
+}
 
 static int sunxi_pinctrl_irq_set_type(struct irq_data *d,
 				      unsigned int type)
@@ -603,19 +618,11 @@ static void sunxi_pinctrl_irq_mask(struct irq_data *d)
 static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
 {
 	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
-	struct sunxi_desc_function *func;
 	u32 reg = sunxi_irq_ctrl_reg(d->hwirq);
 	u8 idx = sunxi_irq_ctrl_offset(d->hwirq);
 	unsigned long flags;
 	u32 val;
 
-	func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
-						       pctl->irq_array[d->hwirq],
-						       "irq");
-
-	/* Change muxing to INT mode */
-	sunxi_pmx_set(pctl->pctl_dev, pctl->irq_array[d->hwirq], func->muxval);
-
 	spin_lock_irqsave(&pctl->lock, flags);
 
 	/* Unmask the IRQ */
@@ -629,6 +636,7 @@ static struct irq_chip sunxi_pinctrl_irq_chip = {
 	.irq_ack	= sunxi_pinctrl_irq_ack,
 	.irq_mask	= sunxi_pinctrl_irq_mask,
 	.irq_unmask	= sunxi_pinctrl_irq_unmask,
+	.irq_request_resources = sunxi_pinctrl_irq_request_resources,
 	.irq_set_type	= sunxi_pinctrl_irq_set_type,
 	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };
-- 
2.0.0

[PATCH v3 3/4] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Hans de Goede]

For level triggered gpio interrupts we need to use handle_fasteoi_irq,
like we do with the irq-sunxi-nmi driver. This is necessary to give threaded
interrupt handlers a chance to actuall clear the source of the interrupt
(which may involve sleeping waiting for i2c / spi / mmc transfers), before
acknowledging the interrupt.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 39 ++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 8bdd65b..c1f0530 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -31,6 +31,9 @@
 #include "../core.h"
 #include "pinctrl-sunxi.h"
 
+static struct irq_chip sunxi_pinctrl_edge_irq_chip;
+static struct irq_chip sunxi_pinctrl_level_irq_chip;
+
 static struct sunxi_pinctrl_group *
 sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
 {
@@ -547,10 +550,10 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
 	return 0;
 }
 
-static int sunxi_pinctrl_irq_set_type(struct irq_data *d,
-				      unsigned int type)
+static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+	struct irq_desc *desc = container_of(d, struct irq_desc, irq_data);
 	u32 reg = sunxi_irq_cfg_reg(d->hwirq);
 	u8 index = sunxi_irq_cfg_offset(d->hwirq);
 	unsigned long flags;
@@ -577,6 +580,14 @@ static int sunxi_pinctrl_irq_set_type(struct irq_data *d,
 		return -EINVAL;
 	}
 
+	if (type & IRQ_TYPE_LEVEL_MASK) {
+		d->chip = &sunxi_pinctrl_level_irq_chip;
+		desc->handle_irq = handle_fasteoi_irq;
+	} else {
+		d->chip = &sunxi_pinctrl_edge_irq_chip;
+		desc->handle_irq = handle_edge_irq;
+	}
+
 	spin_lock_irqsave(&pctl->lock, flags);
 
 	regval = readl(pctl->membase + reg);
@@ -632,7 +643,7 @@ static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
 	spin_unlock_irqrestore(&pctl->lock, flags);
 }
 
-static struct irq_chip sunxi_pinctrl_irq_chip = {
+static struct irq_chip sunxi_pinctrl_edge_irq_chip = {
 	.irq_ack	= sunxi_pinctrl_irq_ack,
 	.irq_mask	= sunxi_pinctrl_irq_mask,
 	.irq_unmask	= sunxi_pinctrl_irq_unmask,
@@ -641,6 +652,16 @@ static struct irq_chip sunxi_pinctrl_irq_chip = {
 	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };
 
+static struct irq_chip sunxi_pinctrl_level_irq_chip = {
+	.irq_eoi	= sunxi_pinctrl_irq_ack,
+	.irq_mask	= sunxi_pinctrl_irq_mask,
+	.irq_unmask	= sunxi_pinctrl_irq_unmask,
+	.irq_request_resources = sunxi_pinctrl_irq_request_resources,
+	.irq_set_type	= sunxi_pinctrl_irq_set_type,
+	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_EOI_THREADED |
+			  IRQCHIP_EOI_IF_HANDLED,
+};
+
 static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_get_chip(irq);
@@ -657,9 +678,6 @@ static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
 	reg = sunxi_irq_status_reg_from_bank(bank);
 	val = readl(pctl->membase + reg);
 
-	/* Clear all interrupts */
-	writel(val, pctl->membase + reg);
-
 	if (val) {
 		int irqoffset;
 
@@ -929,12 +947,17 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
 	for (i = 0; i < (pctl->desc->irq_banks * IRQ_PER_BANK); i++) {
 		int irqno = irq_create_mapping(pctl->domain, i);
 
-		irq_set_chip_and_handler(irqno, &sunxi_pinctrl_irq_chip,
-					 handle_simple_irq);
+		irq_set_chip_and_handler(irqno, &sunxi_pinctrl_edge_irq_chip,
+					 handle_edge_irq);
 		irq_set_chip_data(irqno, pctl);
 	};
 
 	for (i = 0; i < pctl->desc->irq_banks; i++) {
+		/* Mask and clear all IRQs before registering a handler */
+		writel(0, pctl->membase + sunxi_irq_ctrl_reg_from_bank(i));
+		writel(0xffffffff,
+			pctl->membase + sunxi_irq_status_reg_from_bank(i));
+
 		irq_set_chained_handler(pctl->irq[i],
 					sunxi_pinctrl_irq_handler);
 		irq_set_handler_data(pctl->irq[i], pctl);
-- 
2.0.0

[PATCH v3 4/4] pinctrl: sunxi: Define enable / disable irq callbacks for level triggered irqs

[Hans de Goede]

Some drivers use disable_irq / enable_irq and do the work clearing the source
in another thread instead of using a threaded interrupt handler.

The irqchip used not having irq_disable and irq_enable callbacks in this case,
will lead to unnecessary spurious interrupts:

On a disable_irq in a chip without a handller for this, the irq core will
remember the disable, but not actually call into the irqchip. With a level
triggered interrupt (where the source has not been cleared) this will lead
to an immediate retrigger, at which point the irq-core will mask the irq.
So having an irq_disable callback in the irqchip will save us the interrupt
firing a 2nd time for nothing.

Drivers using disable / enable_irq like this, will call enable_irq when
they finally have cleared the interrupt source, without an enable_irq callback,
this will turn into an unmask, at which point the irq will trigger immediately
because when it was originally acked the level was still high, so the ack was
a nop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index c1f0530..c641566 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -643,6 +643,12 @@ static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
 	spin_unlock_irqrestore(&pctl->lock, flags);
 }
 
+static void sunxi_pinctrl_irq_ack_unmask(struct irq_data *d)
+{
+	sunxi_pinctrl_irq_ack(d);
+	sunxi_pinctrl_irq_unmask(d);
+}
+
 static struct irq_chip sunxi_pinctrl_edge_irq_chip = {
 	.irq_ack	= sunxi_pinctrl_irq_ack,
 	.irq_mask	= sunxi_pinctrl_irq_mask,
@@ -656,6 +662,10 @@ static struct irq_chip sunxi_pinctrl_level_irq_chip = {
 	.irq_eoi	= sunxi_pinctrl_irq_ack,
 	.irq_mask	= sunxi_pinctrl_irq_mask,
 	.irq_unmask	= sunxi_pinctrl_irq_unmask,
+	/* Define irq_enable / disable to avoid spurious irqs for drivers
+	 * using these to suppress irqs while they clear the irq source */
+	.irq_enable	= sunxi_pinctrl_irq_ack_unmask,
+	.irq_disable	= sunxi_pinctrl_irq_mask,
 	.irq_request_resources = sunxi_pinctrl_irq_request_resources,
 	.irq_set_type	= sunxi_pinctrl_irq_set_type,
 	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_EOI_THREADED |
-- 
2.0.0

[PATCH v3 3/4] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Maxime Ripard]

On Sun, Jun 29, 2014 at 04:11:01PM +0200, Hans de Goede wrote:
> For level triggered gpio interrupts we need to use handle_fasteoi_irq,
> like we do with the irq-sunxi-nmi driver. This is necessary to give threaded
> interrupt handlers a chance to actuall clear the source of the interrupt
> (which may involve sleeping waiting for i2c / spi / mmc transfers), before
> acknowledging the interrupt.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140701/a357d7dd/attachment.sig>

[PATCH v3 4/4] pinctrl: sunxi: Define enable / disable irq callbacks for level triggered irqs

[Maxime Ripard]

On Sun, Jun 29, 2014 at 04:11:02PM +0200, Hans de Goede wrote:
> Some drivers use disable_irq / enable_irq and do the work clearing the source
> in another thread instead of using a threaded interrupt handler.
> 
> The irqchip used not having irq_disable and irq_enable callbacks in this case,
> will lead to unnecessary spurious interrupts:
> 
> On a disable_irq in a chip without a handller for this, the irq core will
> remember the disable, but not actually call into the irqchip. With a level
> triggered interrupt (where the source has not been cleared) this will lead
> to an immediate retrigger, at which point the irq-core will mask the irq.
> So having an irq_disable callback in the irqchip will save us the interrupt
> firing a 2nd time for nothing.
> 
> Drivers using disable / enable_irq like this, will call enable_irq when
> they finally have cleared the interrupt source, without an enable_irq callback,
> this will turn into an unmask, at which point the irq will trigger immediately
> because when it was originally acked the level was still high, so the ack was
> a nop.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140701/00c1b355/attachment.sig>

[PATCH v3 1/4] pinctrl: sunxi: Add IRQCHIP_SKIP_SET_WAKE flag for pinctrl irq chip

[Linus Walleij]

On Sun, Jun 29, 2014 at 4:10 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> From: Chen-Yu Tsai <wens@csie.org>
>
> The sunxi pinctrl irq chip driver does not support wakeup at the
> moment. Adding IRQCHIP_SKIP_SET_WAKE lets the irqs work with drivers
> using wakeup.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 2/4] pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resources

[Linus Walleij]

On Sun, Jun 29, 2014 at 4:11 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> With level triggered interrupt mask / unmask will get called for each
> interrupt, doing the somewhat expensive mux setting on each unmask thus is
> not a good idea. Instead add a request_resources callback and do it there.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 3/4] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Linus Walleij]

On Sun, Jun 29, 2014 at 4:11 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> For level triggered gpio interrupts we need to use handle_fasteoi_irq,
> like we do with the irq-sunxi-nmi driver. This is necessary to give threaded
> interrupt handlers a chance to actuall clear the source of the interrupt
> (which may involve sleeping waiting for i2c / spi / mmc transfers), before
> acknowledging the interrupt.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij

[PATCH v3 4/4] pinctrl: sunxi: Define enable / disable irq callbacks for level triggered irqs

[Linus Walleij]

On Sun, Jun 29, 2014 at 4:11 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Some drivers use disable_irq / enable_irq and do the work clearing the source
> in another thread instead of using a threaded interrupt handler.
>
> The irqchip used not having irq_disable and irq_enable callbacks in this case,
> will lead to unnecessary spurious interrupts:
>
> On a disable_irq in a chip without a handller for this, the irq core will
> remember the disable, but not actually call into the irqchip. With a level
> triggered interrupt (where the source has not been cleared) this will lead
> to an immediate retrigger, at which point the irq-core will mask the irq.
> So having an irq_disable callback in the irqchip will save us the interrupt
> firing a 2nd time for nothing.
>
> Drivers using disable / enable_irq like this, will call enable_irq when
> they finally have cleared the interrupt source, without an enable_irq callback,
> this will turn into an unmask, at which point the irq will trigger immediately
> because when it was originally acked the level was still high, so the ack was
> a nop.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij