[PATCH v2 0/5] pinctrl: sunxi: Fix level triggered interrupt support

[PATCH v2 0/5] pinctrl: sunxi: Fix level triggered interrupt support

[Hans de Goede]

Hi All,

Here is v2 of my patch-series to fix the level triggered interrupt support
for sunxi pinctrl. I've decided to make this a separate series now, rather then
keep posting it as part of the sdio oob irq series for easier tracking.

Note this series is based on top of pinctrl-next + Maxmime's
"[PATCH v2 0/5] pinctrl: sunxi: Add A31 external interrupts support" series,
since the 2 series conflict I thought it would be best to just do my series
on top.

Changes since v1:
-Dropped the pinctrl: sunxi: create irq/pin mapping during init patch,
 as this is in pinctrl-next now
-Added "pinctrl: sunxi: Fix sunxi_irq_cfg_reg not working for irqs >= 8"
 patch, this fixes a regression caused by Maxmime's set and should probably
 be squashed into Maxime's "pinctrl: sunxi: Implement multiple interrupt banks support"
 patch
-Dropped the setting of the irq_chip name from:
 "pinctrl: sunxi: Add IRQCHIP_SKIP_SET_WAKE flag for pinctrl irq chip"
-Use irq_request_resources callback in:
 "pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resources"
-Rebased on top of Maxime's series

Regards,

Hans

[PATCH v2 1/5] pinctrl: sunxi: Fix sunxi_irq_cfg_reg not working for irqs >= 8

[Hans de Goede]

Rename SUNXI_IRQ_NUMBER to IRQ_PER_BANK as it no longer represents the total
number of irqs and use it instead of the number of irqs-per-reg when
calculating the bank to use for the various sunxi_irq_xxx_reg functions.
This fixes sunxi_irq_cfg_reg using the wrong bank for irqs >= 8.

Also sunxi_irq_cfg_reg_from_bank returns the address of pio_int_cfg0, add an
offset to the register returned for irqs living in the pio_int_cfg[1-3].

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 12 ++++++------
 drivers/pinctrl/sunxi/pinctrl-sunxi.h | 11 ++++++-----
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 8dc2cd9..d989a10 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -655,9 +655,9 @@ static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
 		int irqoffset;
 
 		chained_irq_enter(chip, desc);
-		for_each_set_bit(irqoffset, &val, SUNXI_IRQ_NUMBER) {
+		for_each_set_bit(irqoffset, &val, IRQ_PER_BANK) {
 			int pin_irq = irq_find_mapping(pctl->domain,
-						       bank * SUNXI_IRQ_NUMBER + irqoffset);
+						       bank * IRQ_PER_BANK + irqoffset);
 			generic_handle_irq(pin_irq);
 		}
 		chained_irq_exit(chip, desc);
@@ -726,7 +726,7 @@ static int sunxi_pinctrl_build_state(struct platform_device *pdev)
 		while (func->name) {
 			/* Create interrupt mapping while we're at it */
 			if (!strcmp(func->name, "irq")) {
-				int irqnum = func->irqnum + func->irqbank * SUNXI_IRQ_NUMBER;
+				int irqnum = func->irqnum + func->irqbank * IRQ_PER_BANK;
 				pctl->irq_array[irqnum] = pin->pin.number;
 			}
 
@@ -800,7 +800,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
 	pctl->desc = desc;
 
 	pctl->irq_array = devm_kcalloc(&pdev->dev,
-				       SUNXI_IRQ_NUMBER * pctl->desc->irq_banks,
+				       IRQ_PER_BANK * pctl->desc->irq_banks,
 				       sizeof(*pctl->irq_array),
 				       GFP_KERNEL);
 	if (!pctl->irq_array)
@@ -908,7 +908,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
 	}
 
 	pctl->domain = irq_domain_add_linear(node,
-					     pctl->desc->irq_banks * SUNXI_IRQ_NUMBER,
+					     pctl->desc->irq_banks * IRQ_PER_BANK,
 					     &irq_domain_simple_ops,
 					     NULL);
 	if (!pctl->domain) {
@@ -917,7 +917,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
 		goto clk_error;
 	}
 
-	for (i = 0; i < (pctl->desc->irq_banks * SUNXI_IRQ_NUMBER); i++) {
+	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,
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 8490a45..19b3ae8 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -53,7 +53,7 @@
 #define PULL_PINS_BITS		2
 #define PULL_PINS_MASK		0x03
 
-#define SUNXI_IRQ_NUMBER	32
+#define IRQ_PER_BANK		32
 
 #define IRQ_CFG_REG		0x200
 #define IRQ_CFG_IRQ_PER_REG		8
@@ -235,9 +235,10 @@ static inline u32 sunxi_irq_cfg_reg_from_bank(u8 bank)
 
 static inline u32 sunxi_irq_cfg_reg(u16 irq)
 {
-	u8 bank = irq / IRQ_CFG_IRQ_PER_REG;
+	u8 bank = irq / IRQ_PER_BANK;
+	u32 offset = ((irq % IRQ_PER_BANK) / IRQ_CFG_IRQ_PER_REG) * 0x04;
 
-	return sunxi_irq_cfg_reg_from_bank(bank);
+	return sunxi_irq_cfg_reg_from_bank(bank) + offset;
 }
 
 static inline u32 sunxi_irq_cfg_offset(u16 irq)
@@ -253,7 +254,7 @@ static inline u32 sunxi_irq_ctrl_reg_from_bank(u8 bank)
 
 static inline u32 sunxi_irq_ctrl_reg(u16 irq)
 {
-	u8 bank = irq / IRQ_CTRL_IRQ_PER_REG;
+	u8 bank = irq / IRQ_PER_BANK;
 
 	return sunxi_irq_ctrl_reg_from_bank(bank);
 }
@@ -271,7 +272,7 @@ static inline u32 sunxi_irq_status_reg_from_bank(u8 bank)
 
 static inline u32 sunxi_irq_status_reg(u16 irq)
 {
-	u8 bank = irq / IRQ_STATUS_IRQ_PER_REG;
+	u8 bank = irq / IRQ_PER_BANK;
 
 	return sunxi_irq_status_reg_from_bank(bank);
 }
-- 
2.0.0

[PATCH v2 2/5] 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>
---
 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 v2 3/5] 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>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index c199337..61d3246 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -531,6 +531,19 @@ 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");
+
+	/* 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 +616,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 +634,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 v2 4/5] 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 | 57 +++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 61d3246..418a430 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -31,6 +31,11 @@
 #include "../core.h"
 #include "pinctrl-sunxi.h"
 
+static void sunxi_pinctrl_irq_ack(struct irq_data *d);
+static void sunxi_pinctrl_irq_mask(struct irq_data *d);
+static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
+static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
+
 static struct sunxi_pinctrl_group *
 sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
 {
@@ -545,10 +550,29 @@ 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 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,
+	.irq_request_resources	= sunxi_pinctrl_irq_request_resources,
+	.irq_set_type		= sunxi_pinctrl_irq_set_type,
+	.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 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;
@@ -575,6 +599,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);
@@ -630,15 +662,6 @@ static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
 	spin_unlock_irqrestore(&pctl->lock, flags);
 }
 
-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,
-};
-
 static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_get_chip(irq);
@@ -655,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;
 
@@ -927,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 v2 5/5] 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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 418a430..6ccbe43 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -34,6 +34,7 @@
 static void sunxi_pinctrl_irq_ack(struct irq_data *d);
 static void sunxi_pinctrl_irq_mask(struct irq_data *d);
 static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
+static void sunxi_pinctrl_irq_ack_unmask(struct irq_data *d);
 static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
 
 static struct sunxi_pinctrl_group *
@@ -563,6 +564,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
@@ -662,6 +667,28 @@ 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)
+{
+	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+	u32 ctrl_reg = sunxi_irq_ctrl_reg(d->hwirq);
+	u8 ctrl_idx = sunxi_irq_ctrl_offset(d->hwirq);
+	u32 status_reg = sunxi_irq_status_reg(d->hwirq);
+	u8 status_idx = sunxi_irq_status_offset(d->hwirq);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&pctl->lock, flags);
+
+	/* Clear the IRQ */
+	writel(1 << status_idx, pctl->membase + status_reg);
+
+	/* Unmask the IRQ */
+	val = readl(pctl->membase + ctrl_reg);
+	writel(val | (1 << ctrl_idx), pctl->membase + ctrl_reg);
+
+	spin_unlock_irqrestore(&pctl->lock, flags);
+}
+
 static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_get_chip(irq);
-- 
2.0.0

[PATCH v2 1/5] pinctrl: sunxi: Fix sunxi_irq_cfg_reg not working for irqs >= 8

[Maxime Ripard]

On Sat, May 31, 2014 at 04:01:35PM +0200, Hans de Goede wrote:
> Rename SUNXI_IRQ_NUMBER to IRQ_PER_BANK as it no longer represents the total
> number of irqs and use it instead of the number of irqs-per-reg when
> calculating the bank to use for the various sunxi_irq_xxx_reg functions.
> This fixes sunxi_irq_cfg_reg using the wrong bank for irqs >= 8.
> 
> Also sunxi_irq_cfg_reg_from_bank returns the address of pio_int_cfg0, add an
> offset to the register returned for irqs living in the pio_int_cfg[1-3].
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Good catch, I've squashed it into my serie with minor modifications
(remove the cfg_reg_from_bank that doesn't make any sense).

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/20140602/52ee3706/attachment.sig>

[PATCH v2 2/5] pinctrl: sunxi: Add IRQCHIP_SKIP_SET_WAKE flag for pinctrl irq chip

[Maxime Ripard]

On Sat, May 31, 2014 at 04:01:36PM +0200, Hans de Goede 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>

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/20140602/7a94d4d1/attachment.sig>

[PATCH v2 3/5] pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resources

[Maxime Ripard]

On Sat, May 31, 2014 at 04:01:37PM +0200, Hans de Goede 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>
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index c199337..61d3246 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -531,6 +531,19 @@ 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");

I know you're just moving code here, but we should add a check for
func being !NULL here, otherwise, the kernel is going to blow up at
the next line.

And since gpio_to_irq might not be called before the request_irq, you
might be in such a case.

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/20140602/f95a9644/attachment.sig>

[PATCH v2 4/5] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Maxime Ripard]

On Sat, May 31, 2014 at 04:01:38PM +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>
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 57 +++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 61d3246..418a430 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -31,6 +31,11 @@
>  #include "../core.h"
>  #include "pinctrl-sunxi.h"
>  
> +static void sunxi_pinctrl_irq_ack(struct irq_data *d);
> +static void sunxi_pinctrl_irq_mask(struct irq_data *d);
> +static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
> +static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
> +
>  static struct sunxi_pinctrl_group *
>  sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
>  {
> @@ -545,10 +550,29 @@ 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 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,
> +	.irq_request_resources	= sunxi_pinctrl_irq_request_resources,
> +	.irq_set_type		= sunxi_pinctrl_irq_set_type,
> +	.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,
> +};

Maybe we can just forward declare these two structures, instead of all
their functions.

> +
> +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;
> @@ -575,6 +599,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;

irq_set_chip_and_handler?

> +	}
> +
>  	spin_lock_irqsave(&pctl->lock, flags);
>  
>  	regval = readl(pctl->membase + reg);
> @@ -630,15 +662,6 @@ static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
>  	spin_unlock_irqrestore(&pctl->lock, flags);
>  }
>  
> -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,
> -};
> -
>  static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
>  {
>  	struct irq_chip *chip = irq_get_chip(irq);
> @@ -655,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;
>  
> @@ -927,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
> 

Looks fine otherwise, 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/20140603/49077e41/attachment.sig>

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

[Maxime Ripard]

On Sat, May 31, 2014 at 04:01:39PM +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.

Judging by the comments there, it seems more like a feature.

> 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.

I don't think enable_irq offers any warranty regarding the state of
the interrupt. It's up to the driver to clear the interrupts before
calling enable_irq if it doesn't want any irrelevant/spurious
interrupts.

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 418a430..6ccbe43 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -34,6 +34,7 @@
>  static void sunxi_pinctrl_irq_ack(struct irq_data *d);
>  static void sunxi_pinctrl_irq_mask(struct irq_data *d);
>  static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
> +static void sunxi_pinctrl_irq_ack_unmask(struct irq_data *d);
>  static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
>  
>  static struct sunxi_pinctrl_group *
> @@ -563,6 +564,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 */

What "drivers" are we talking about? I grepped for a while, and didn't
any obvious candidates.

But again, I feel like if a driver wants to work outside of the usual
interrupt workflow, it's up to the driver itself to know what it's
doing.

> +	.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
> @@ -662,6 +667,28 @@ 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)
> +{
> +	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
> +	u32 ctrl_reg = sunxi_irq_ctrl_reg(d->hwirq);
> +	u8 ctrl_idx = sunxi_irq_ctrl_offset(d->hwirq);
> +	u32 status_reg = sunxi_irq_status_reg(d->hwirq);
> +	u8 status_idx = sunxi_irq_status_offset(d->hwirq);
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&pctl->lock, flags);
> +
> +	/* Clear the IRQ */
> +	writel(1 << status_idx, pctl->membase + status_reg);
> +
> +	/* Unmask the IRQ */
> +	val = readl(pctl->membase + ctrl_reg);
> +	writel(val | (1 << ctrl_idx), pctl->membase + ctrl_reg);
> +
> +	spin_unlock_irqrestore(&pctl->lock, flags);
> +}

Please at least call sunxi_pinctrl_irq_ack and
sunxi_pinctrl_irq_unmask if you're doing this.

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/20140603/08da55c8/attachment.sig>

[PATCH v2 4/5] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Hans de Goede]

Hi,

On 06/03/2014 03:30 PM, Maxime Ripard wrote:
> On Sat, May 31, 2014 at 04:01:38PM +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>
>> ---
>>   drivers/pinctrl/sunxi/pinctrl-sunxi.c | 57 +++++++++++++++++++++++++----------
>>   1 file changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index 61d3246..418a430 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -31,6 +31,11 @@
>>   #include "../core.h"
>>   #include "pinctrl-sunxi.h"
>>
>> +static void sunxi_pinctrl_irq_ack(struct irq_data *d);
>> +static void sunxi_pinctrl_irq_mask(struct irq_data *d);
>> +static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
>> +static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
>> +
>>   static struct sunxi_pinctrl_group *
>>   sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
>>   {
>> @@ -545,10 +550,29 @@ 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 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,
>> +	.irq_request_resources	= sunxi_pinctrl_irq_request_resources,
>> +	.irq_set_type		= sunxi_pinctrl_irq_set_type,
>> +	.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,
>> +};
>
> Maybe we can just forward declare these two structures, instead of all
> their functions.

Variables cannot be forward declared, they can be declared extern, but last
time I checked extern and static don't mix.

>
>> +
>> +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;
>> @@ -575,6 +599,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;
>
> irq_set_chip_and_handler?

Does a lookup of the irqdata which we already have here, and
that lookup takes a lock which is already held here.

>
>> +	}
>> +
>>   	spin_lock_irqsave(&pctl->lock, flags);
>>
>>   	regval = readl(pctl->membase + reg);
>> @@ -630,15 +662,6 @@ static void sunxi_pinctrl_irq_unmask(struct irq_data *d)
>>   	spin_unlock_irqrestore(&pctl->lock, flags);
>>   }
>>
>> -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,
>> -};
>> -
>>   static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
>>   {
>>   	struct irq_chip *chip = irq_get_chip(irq);
>> @@ -655,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;
>>
>> @@ -927,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
>>
>
> Looks fine otherwise, thanks.


Regards,

Hans

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

[Hans de Goede]

Hi,

On 06/03/2014 03:47 PM, Maxime Ripard wrote:
> On Sat, May 31, 2014 at 04:01:39PM +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.
>
> Judging by the comments there, it seems more like a feature.

It is a feature in cases where the interrupt re-firing leading to the unmask
is unlikely to happen, because then delaying the unmask saves a slow iowrite,
for level triggered interrupts however, the re-fire is the likely case, so
it is better to define a disable function.

>> 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.
>
> I don't think enable_irq offers any warranty regarding the state of
> the interrupt. It's up to the driver to clear the interrupts before
> calling enable_irq if it doesn't want any irrelevant/spurious
> interrupts.

Yes the driver must clear the source of the irq, but there is no way for the
driver to do the irqchip ack, so it makes sense to do that in the irqchip
enable function to avoid spurious irqs. Note that this is only done for
level irqs, where the ack is a nop if the irq source is not cleared, so this
cannot cause lost irqs.

>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/pinctrl/sunxi/pinctrl-sunxi.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index 418a430..6ccbe43 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -34,6 +34,7 @@
>>   static void sunxi_pinctrl_irq_ack(struct irq_data *d);
>>   static void sunxi_pinctrl_irq_mask(struct irq_data *d);
>>   static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
>> +static void sunxi_pinctrl_irq_ack_unmask(struct irq_data *d);
>>   static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
>>
>>   static struct sunxi_pinctrl_group *
>> @@ -563,6 +564,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 */
>
> What "drivers" are we talking about? I grepped for a while, and didn't
> any obvious candidates.

Any driver dealing with level irqs and not doing so using a threaded interrupt
handler. Threaded interrupt handlers are relatively new, drivers dealing with
i2c / spi / sdio busses with an oob irq, where clearing the irq involves
a bus read/write which may sleep, are a lot older then threaded interrupt
support. So the "design pattern" for these used to be to disable the irq in the
handler, wakeup a tasklet / workqueue which does the work of handling + clearing
the irq, and then re-enables it.

The example I've been specifically testing with is:

drivers/net/wireless/brcm80211/brcmfmac

> But again, I feel like if a driver wants to work outside of the usual
> interrupt workflow, it's up to the driver itself to know what it's
> doing.

As said above, the scenario I'm trying to deal with here is quite normal
for ie i2c drivers using an oob irq. There is no other way for them to
deal with this, other then using a threaded interrupt handler, which in
many cases would mean heavy restructuring of a driver which works fine
as is.

>> +	.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
>> @@ -662,6 +667,28 @@ 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)
>> +{
>> +	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
>> +	u32 ctrl_reg = sunxi_irq_ctrl_reg(d->hwirq);
>> +	u8 ctrl_idx = sunxi_irq_ctrl_offset(d->hwirq);
>> +	u32 status_reg = sunxi_irq_status_reg(d->hwirq);
>> +	u8 status_idx = sunxi_irq_status_offset(d->hwirq);
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	spin_lock_irqsave(&pctl->lock, flags);
>> +
>> +	/* Clear the IRQ */
>> +	writel(1 << status_idx, pctl->membase + status_reg);
>> +
>> +	/* Unmask the IRQ */
>> +	val = readl(pctl->membase + ctrl_reg);
>> +	writel(val | (1 << ctrl_idx), pctl->membase + ctrl_reg);
>> +
>> +	spin_unlock_irqrestore(&pctl->lock, flags);
>> +}
>
> Please at least call sunxi_pinctrl_irq_ack and
> sunxi_pinctrl_irq_unmask if you're doing this.

Ok, I'll change this for the next version.

Regards,

Hans

[PATCH v2 3/5] pinctrl: sunxi: Move setting of mux to irq type from unmask to request_resources

[Hans de Goede]

Hi,

On 06/02/2014 01:59 PM, Maxime Ripard wrote:
> On Sat, May 31, 2014 at 04:01:37PM +0200, Hans de Goede 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>
>> ---
>>   drivers/pinctrl/sunxi/pinctrl-sunxi.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index c199337..61d3246 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -531,6 +531,19 @@ 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");
>
> I know you're just moving code here, but we should add a check for
> func being !NULL here, otherwise, the kernel is going to blow up at
> the next line.
>
> And since gpio_to_irq might not be called before the request_irq, you
> might be in such a case.

Ok, will do for v3 of the patch-set.

Regards,

Hans

[PATCH v2 1/5] pinctrl: sunxi: Fix sunxi_irq_cfg_reg not working for irqs >= 8

[Linus Walleij]

On Mon, Jun 2, 2014 at 12:18 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sat, May 31, 2014 at 04:01:35PM +0200, Hans de Goede wrote:
>> Rename SUNXI_IRQ_NUMBER to IRQ_PER_BANK as it no longer represents the total
>> number of irqs and use it instead of the number of irqs-per-reg when
>> calculating the bank to use for the various sunxi_irq_xxx_reg functions.
>> This fixes sunxi_irq_cfg_reg using the wrong bank for irqs >= 8.
>>
>> Also sunxi_irq_cfg_reg_from_bank returns the address of pio_int_cfg0, add an
>> offset to the register returned for irqs living in the pio_int_cfg[1-3].
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Good catch, I've squashed it into my serie with minor modifications
> (remove the cfg_reg_from_bank that doesn't make any sense).

I'm more than a little confused by your and Hans parallell patch
series to sunxi now :-)

I will try to apply your patches first, then Hans' as you're the
maintainer of this driver.

Yours,
Linus Walleij

[PATCH v2 1/5] pinctrl: sunxi: Fix sunxi_irq_cfg_reg not working for irqs >= 8

[Hans de Goede]

Hi,

On 06/09/2014 03:21 PM, Linus Walleij wrote:
> On Mon, Jun 2, 2014 at 12:18 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Sat, May 31, 2014 at 04:01:35PM +0200, Hans de Goede wrote:
>>> Rename SUNXI_IRQ_NUMBER to IRQ_PER_BANK as it no longer represents the total
>>> number of irqs and use it instead of the number of irqs-per-reg when
>>> calculating the bank to use for the various sunxi_irq_xxx_reg functions.
>>> This fixes sunxi_irq_cfg_reg using the wrong bank for irqs >= 8.
>>>
>>> Also sunxi_irq_cfg_reg_from_bank returns the address of pio_int_cfg0, add an
>>> offset to the register returned for irqs living in the pio_int_cfg[1-3].
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Good catch, I've squashed it into my serie with minor modifications
>> (remove the cfg_reg_from_bank that doesn't make any sense).
> 
> I'm more than a little confused by your and Hans parallell patch
> series to sunxi now :-)
> 
> I will try to apply your patches first, then Hans' as you're the
> maintainer of this driver.

Thanks, that was the intention all along, I've based my patch series
on top Maxime's series to avoid conflicts.

Note please wait with applying my series, Maxime had some review
comments, so I'm going to do a new version soon.

Regards,

Hans

[PATCH v2 4/5] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Maxime Ripard]

On Tue, Jun 03, 2014 at 05:13:17PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/03/2014 03:30 PM, Maxime Ripard wrote:
> >On Sat, May 31, 2014 at 04:01:38PM +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>
> >>---
> >>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 57 +++++++++++++++++++++++++----------
> >>  1 file changed, 41 insertions(+), 16 deletions(-)
> >>
> >>diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >>index 61d3246..418a430 100644
> >>--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >>+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> >>@@ -31,6 +31,11 @@
> >>  #include "../core.h"
> >>  #include "pinctrl-sunxi.h"
> >>
> >>+static void sunxi_pinctrl_irq_ack(struct irq_data *d);
> >>+static void sunxi_pinctrl_irq_mask(struct irq_data *d);
> >>+static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
> >>+static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
> >>+
> >>  static struct sunxi_pinctrl_group *
> >>  sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
> >>  {
> >>@@ -545,10 +550,29 @@ 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 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,
> >>+	.irq_request_resources	= sunxi_pinctrl_irq_request_resources,
> >>+	.irq_set_type		= sunxi_pinctrl_irq_set_type,
> >>+	.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,
> >>+};
> >
> >Maybe we can just forward declare these two structures, instead of all
> >their functions.
> 
> Variables cannot be forward declared, they can be declared extern, but last
> time I checked extern and static don't mix.

Actually, you can. It's called a tentative definition, see
http://c0x.coding-guidelines.com/6.9.2.html and
http://c0x.coding-guidelines.com/6.2.2.html for the definition of
internal linkage.

Maxime

> 
> >
> >>+
> >>+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;
> >>@@ -575,6 +599,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;
> >
> >irq_set_chip_and_handler?
> 
> Does a lookup of the irqdata which we already have here, and
> that lookup takes a lock which is already held here.

Ah, yes.

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/20140617/e3d07fb2/attachment.sig>

[PATCH v2 4/5] pinctrl: sunxi: Properly handle level triggered gpio interrupts

[Hans de Goede]

Hi,

On 06/17/2014 08:43 PM, Maxime Ripard wrote:
> On Tue, Jun 03, 2014 at 05:13:17PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/03/2014 03:30 PM, Maxime Ripard wrote:
>>> On Sat, May 31, 2014 at 04:01:38PM +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>
>>>> ---
>>>>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 57 +++++++++++++++++++++++++----------
>>>>  1 file changed, 41 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>> index 61d3246..418a430 100644
>>>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>> @@ -31,6 +31,11 @@
>>>>  #include "../core.h"
>>>>  #include "pinctrl-sunxi.h"
>>>>
>>>> +static void sunxi_pinctrl_irq_ack(struct irq_data *d);
>>>> +static void sunxi_pinctrl_irq_mask(struct irq_data *d);
>>>> +static void sunxi_pinctrl_irq_unmask(struct irq_data *d);
>>>> +static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
>>>> +
>>>>  static struct sunxi_pinctrl_group *
>>>>  sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
>>>>  {
>>>> @@ -545,10 +550,29 @@ 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 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,
>>>> +	.irq_request_resources	= sunxi_pinctrl_irq_request_resources,
>>>> +	.irq_set_type		= sunxi_pinctrl_irq_set_type,
>>>> +	.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,
>>>> +};
>>>
>>> Maybe we can just forward declare these two structures, instead of all
>>> their functions.
>>
>> Variables cannot be forward declared, they can be declared extern, but last
>> time I checked extern and static don't mix.
> 
> Actually, you can. It's called a tentative definition, see
> http://c0x.coding-guidelines.com/6.9.2.html and
> http://c0x.coding-guidelines.com/6.2.2.html for the definition of
> internal linkage.

Ah, I've been programming C since 1996, and I did not know about that :)

Good to have learned something new today. I'll do a v3 patch-set with this
fixed (well a v4 really as I accidentally did a second v2, but lets be
consistent and number the next one v3).

Regards,

Hans

> 
> Maxime
> 
>>
>>>
>>>> +
>>>> +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;
>>>> @@ -575,6 +599,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;
>>>
>>> irq_set_chip_and_handler?
>>
>> Does a lookup of the irqdata which we already have here, and
>> that lookup takes a lock which is already held here.
> 
> Ah, yes.
> 
> Thanks,
> Maxime
>