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 waitnig for i2c / spi / mmc transfers), before
acknowledging the interrupt.

Signed-off-by: Hans de Goede <[email protected]>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 71 +++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index d1675c9..083691e 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -31,6 +31,12 @@
#include "../core.h"
#include "pinctrl-sunxi.h"

+static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type);
+static void sunxi_pinctrl_irq_mask_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(struct irq_data *d);
+
static struct sunxi_pinctrl_group *
sunxi_pinctrl_find_group_by_name(struct sunxi_pinctrl *pctl, const char *group)
{
@@ -535,11 +541,31 @@ static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
return irq_find_mapping(pctl->domain, desc->irqnum);
}

+static struct irq_chip sunxi_pinctrl_edge_irq_chip = {
+ .irq_mask = sunxi_pinctrl_irq_mask,
+ .irq_mask_ack = sunxi_pinctrl_irq_mask_ack,
+ .irq_unmask = sunxi_pinctrl_irq_unmask,
+ .irq_ack = sunxi_pinctrl_irq_ack,
+ .irq_set_type = sunxi_pinctrl_irq_set_type,
+ .name = "sunxi-pio",
+ .flags = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static struct irq_chip sunxi_pinctrl_level_irq_chip = {
+ .irq_mask = sunxi_pinctrl_irq_mask,
+ .irq_mask_ack = sunxi_pinctrl_irq_mask_ack,
+ .irq_unmask = sunxi_pinctrl_irq_unmask,
+ .irq_eoi = sunxi_pinctrl_irq_ack,
+ .irq_set_type = sunxi_pinctrl_irq_set_type,
+ .name = "sunxi-pio",
+ .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)
+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);
struct sunxi_desc_function *func;
@@ -567,6 +593,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);
@@ -640,14 +674,20 @@ 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_mask = sunxi_pinctrl_irq_mask,
- .irq_mask_ack = sunxi_pinctrl_irq_mask_ack,
- .irq_unmask = sunxi_pinctrl_irq_unmask,
- .irq_set_type = sunxi_pinctrl_irq_set_type,
- .name = "sunxi-pio",
- .flags = IRQCHIP_SKIP_SET_WAKE,
-};
+static void sunxi_pinctrl_irq_ack(struct irq_data *d)
+{
+ struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+ u32 status_reg = sunxi_irq_status_reg(d->hwirq);
+ u8 status_idx = sunxi_irq_status_offset(d->hwirq);
+ unsigned long flags;
+
+ spin_lock_irqsave(&pctl->lock, flags);
+
+ /* Clear the IRQ */
+ writel(1 << status_idx, pctl->membase + status_reg);
+
+ spin_unlock_irqrestore(&pctl->lock, flags);
+}

static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
{
@@ -655,9 +695,6 @@ static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
struct sunxi_pinctrl *pctl = irq_get_handler_data(irq);
const unsigned long reg = readl(pctl->membase + IRQ_STATUS_REG);

- /* Clear all interrupts */
- writel(reg, pctl->membase + IRQ_STATUS_REG);
-
if (reg) {
int irqoffset;

@@ -892,11 +929,15 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
for (i = 0; i < SUNXI_IRQ_NUMBER; 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);
};

+ /* Mask and clear all IRQs before registering a handler */
+ writel(0, pctl->membase + IRQ_CTRL_REG);
+ writel(0xffffffff, pctl->membase + IRQ_STATUS_REG);
+
irq_set_chained_handler(pctl->irq, sunxi_pinctrl_irq_handler);
irq_set_handler_data(pctl->irq, pctl);

--
1.9.3

2014-05-27 08:10:03

On Tue, May 27, 2014 at 11:01:03AM +0200, Hans de Goede wrote:
> Hi,
>
> On 05/27/2014 10:09 AM, Maxime Ripard wrote:
> > On Mon, May 26, 2014 at 09:47:58AM +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 move it to the set_type callback, which is typically
> >> done only once for each irq.
> >
> > *This* is the bad idea. Nothing prevents you from calling
> > gpio_get_value whenever you just got your interrupt, that will change
> > the muxing, and never change it back, effectively breaking the
> > interrupts.
>
> Hmm, interesting point, but your assuming 2 things which are both not true:
>
> 1) That calling gpiod_get_value changes the muxing, which is not true, all
> gpiod_get_value variants end up in drivers/gpio/gpiolib.c _gpiod_get_raw_value()
> which does no such thing

Somehow I was convinced it was the case, but yeah, it doesn't really
make much sense, especially since you can get the value of an output,
without wanting to change it to input.

>
> 2) That unmask will always get called after the gpio_get_value to restore the mux
> setting for us, which is not guaranteed at all. Before this patch set pinctrl-sunxi.c
> was using handle_simple_irq which does not mask / unmask at all.
>
> And even with an irq-handler which masks / unmasks, what if the irq wakes up
> a thread and that thread then does the gpio_get_value ?
>
> Note this is *exactly* what e.g. the mmc gpio card-detect code does, it uses
> a thread to read the gpio for debouncing.
>
> Luckily 2. is not a problem, since 1. means that the mux won't get changed at all
> so we don't need to change it back.

Ok.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.84 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-05-27 14:21:25

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH 01/11] pinctrl: sunxi: create irq/pin mapping during init

On Tue, May 27, 2014 at 10:11 PM, Linus Walleij
<[email protected]> wrote:
> On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <[email protected]> wrote:
>
>> From: Chen-Yu Tsai <[email protected]>
>>
>> The irq/pin mapping is used to lookup the pin to mux to the irq
>> function when the irq is enabled. It is created when gpio_to_irq
>> is called. Creating the mapping during init allows us to map the
>> interrupts directly from the device tree.
>>
>> Signed-off-by: Chen-Yu Tsai <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>
> This is exactly correct. Patch applied.

Thanks.

> I tried to hack the sunxi driver to even use the gpiolib
> irqchip helpers but exactly this complex map thing in
> ->irq_array[] git me stuck. If any of you guys could be so
> nice to take a stab at this (compare to other commits
> converting drivers to use gpiolibs irqchip helpers) then
> I'd be very happy.

We still need to rework the whole irqchip stuff for sun6i/sun8i,
which have 1 parent interrupt per EINT capable pin bank.

Earlier discussions with Maxime (on IRC IIRC) suggested moving this
over to gpiolib irqchip helpers might be more work than using our own
irqchip implementation, requiring 1 gpiochip per pin group? (not sure)

ChenYu

2014-05-27 14:11:10

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 01/11] pinctrl: sunxi: create irq/pin mapping during init

On Mon, May 26, 2014 at 9:47 AM, Hans de Goede <[email protected]> wrote:

> From: Chen-Yu Tsai <[email protected]>
>
> The irq/pin mapping is used to lookup the pin to mux to the irq
> function when the irq is enabled. It is created when gpio_to_irq
> is called. Creating the mapping during init allows us to map the
> interrupts directly from the device tree.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>

This is exactly correct. Patch applied.

I tried to hack the sunxi driver to even use the gpiolib
irqchip helpers but exactly this complex map thing in
->irq_array[] git me stuck. If any of you guys could be so
nice to take a stab at this (compare to other commits
converting drivers to use gpiolibs irqchip helpers) then
I'd be very happy.

Yours,
Linus Walleij

2014-05-28 10:35:04