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 <hdego...@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
signature.asc
Description: Digital signature