[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Maxime Ripard]

Hi,

On Tue, Jan 28, 2014 at 12:02:23PM +0100, Hans de Goede wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi,
> 
> On 01/28/2014 11:40 AM, Maxime Ripard wrote:
> 
> Jumping in here to try and clarify things, or so I hope at least :)

Sure :)

> No, the IRQ from the PMIC is a level sensitive IRQ, so it would look
> like this:

Hmm, your mailer seems to have mangled your drawing :(

> The PMIC irq line won't go low until an i2c write to its irq status
> registers write-clears all status bits for which the corresponding
> bit in the irq-mask register is set.

Which makes sense too

> And the only reason the NMI -> GIC also goes low is because the unmask
> operation writes a second ack to the NMI controller in the unmask
> callback of the NMI controller driver.

Yes, and this is exactly what I don't understand. You shouldn't need
that ack in first place, since it's been done already right after the
unmask.

> Note that we cannot use edge triggered interrupts here because the PMIC
> has the typical setup with multiple irq status bits driving a single
> irq line, so the irq handler does read irq-status, handle stuff,
> write-clear irq-status. And if any other irq-status bits get set
> between the read and write-clear the PMIC -> NMI line will stay
> high, as it should since there are more interrupts to handle.

Yep, the edge-thing was just the only case I could think of where it
could lead to troubles.

In what you're saying, which makes total sense, if we don't do the
ack, as soon as the irq will be unmasked, since the level is high, the
handler will be called again, treat the new interrupts, and so on. I
don't see how this is an issue actually.

> > But in this case, you would have two events coming from your
> > device (the two rising edges), so you'd expect two interrupts. And
> > in the case of a level triggered interrupt, the device would keep
> > the interrupt line active until it's unmasked, so we wouldn't end
> > up with this either.
> > 
> >> sunxi_sc_nmi_ack_and_unmask is therefore called (by
> >> irq_finalize_oneshot) after the IRQ thread has been
> >> executed. After the IRQ thread has ACKed the IRQs on the
> >> originating device we can finally ACK and unmask again the NMI.
> > 
> > And what happens if you get a new interrupt right between the end
> > of the handler and the unmask?
> 
> The implicit ack done by the unmask will be ignored if the NMI line
> is still high, just like the initial ack is ignored (which is why we
> need the mask), and when the unmask completes the irq will
> immediately retrigger, as it should.

Yeah, but why do we need the ack in the first place? I can't think of
a case where the ACK would be doing something useful. Either:
  - there is no new interrupts between the mask/ack and the unmask, so
    there's no interrupt to ack.
  - There's a new interrupt between the mask/ack and the
    unmask. There's two more cases here:
    * The interrupt came before the device handler kicked in, and the
      handler will treat it/ack it: No issue
    * The interrupt comes right after the handler has been acking its
      interrupt, the level stays high, your handler is called once
      again, you can treat it: No issue

> >>> I really wonder whether it makes sense to have a generic chip
> >>> here. It seems to be much more complicated than it should. It's
> >>> only about a single interrupt interrupt chip here.
> >> 
> >> I agree but is there any other way to manage the NMI without the
> >> driver of the device connected to NMI having to worry about
> >> acking/masking/unmasking/ etc..?
> > 
> > Yes, just declare a "raw" irqchip. Pretty much like we do on
> > irq-sun4i for example.
> 
> Hmm, I'm not going to say that using a raw irqchip here is a bad
> idea, it sounds like it would be better.

There's none. It was a separate comment :)

> But I don't think this will solve the need the mask / umask. The
> problem is we need to do an i2c write to clear the interrupt source,
> which needs to be done from a thread / workqueue. So when the
> interrupt handler finishes the source won't be cleared yet, and
> AFAIK then only way to deal with this is to mask the interrupt until
> we've cleared the interrupt source.

Yes, but the interrupt is still masked during the time between the
"hard" handler and the thread, so there's no way you can get an
interrupt flood during that time.

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: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140128/18a6c784/attachment.sig>