[RFC] Refactored interrupt handling on ARM

[Warner Losh]

On Apr 15, 2014, at 10:41 AM, Ian Lepore <ian at FreeBSD.org> wrote:

> On Tue, 2014-04-15 at 17:29 +0200, Jakub Klama wrote:
>> On Tue, 15 Apr 2014 08:11:57 -0600, Warner Losh wrote:
>>> This is looking nice on the surface. I’ve done a small portion of
>>> this sort of work for the Atmel port, so I’ll see how well this can 
>>> be
>>> extended to work there. Unlike the gic device, there’s 3 or 4 pio
>>> controllers that act as interrupt handlers, plus the need to wire up
>>> pins relatively early...
>>> 
>>> I’d clean up the name ‘intrng.c’ though, since ng things tend to
>>> become dated… ARM_INTRNG also suffers from this naming flaw. Is there
>>> any reason not to have it on all the time? Also, the ARM_INTRNG ifdef
>>> is inconsistently applied.
>> 
>> Well, the naming thing is the least significant one I guess... and
>> one which can be fixed in a moment.
>> 
>> ARM_INTRNG should be enabled only on platforms which have pic drivers
>> using pic_if.m interface instead of the old one. The purpose for that
>> option is to not break existing code.
>> 
> 
> IMO we do too much of this.  Unless there's a really good reason not to
> update the older platforms to use this new scheme, I think we should
> just convert everything to the new way.

I’m 100% in agreement here. Those platforms that aren’t upgraded need
to go away.

>>> Looking at the FDT, it appears that the interrupt numbers are hard
>>> coded for things that appear to be connected to GPIOs. This hard
>>> coding (and changes to reflect differences in mapping) is really bad.
>> 
>> Can you give an example?
>> 
>>> Linux specifies the GPIO more directly (though each SoC is different
>>> in the details). I haven’t looked at LPC (where I noticed the change)
>>> on Linux to see if these changes bring us closer to being able to use
>>> the stock FDT for that platform.  Do these changes help with that?
>> 
>> LPC changes purpose was rather to give an example, surely other
>> platforms can benefit more from these.
>> 
>>> While I appreciate the aesthetic appeal of having purely an interrupt
>>> number and PIC (interrupt parent), I’m not sure that will work
>>> everywhere.
>>> 
>>> dosoftints() does nothing, is that on purpose?
>> 
>> It's present to not break compatibility and I'm not even sure what
>> should it do. However:
>> 
>> % ack dosoftints
>> arm/arm/intrng.c
>> 417:void dosoftints(void);
>> 419:dosoftints(void)
>> 
>> arm/arm/intr.c
>> 138:void dosoftints(void);
>> 140:dosoftints(void)
>> 
>> is it needed anyways?
>> 
>>> 255 is too few interrupts to have. We need easily twice that for
>>> Atmel. And chance we can remove NIRQ as well?
>> 
>> Currently we can have 255 pics and 256 irqs on each, but because irq
>> number is stored in int, we easily (down to changing two lines of code)
>> divide it by 16:16 or 8:24, thus having 2^16 pics and 2^16 irqs on each
>> or 256 pics with 2^24 irqs on each maximum.
>> 
>> Regarding NIRQ, there's surely possible to get rid of it, but it will
>> need also removing intrcnt/sintrcnt/intrnames and creating more
>> flexible interface instead (for other archs too). Anyways, it's some
>> step in removing it.
>> 
>>> Finally, I’m not sure what the change to kern_intr.c is supposed to
>>> accomplish. It seems weird to me. Can you explain it? I’d think that
>>> would have a bad effect on other platforms.
>> 
>> trapframe is passed to the first intr_event_handle() call and saved
>> in td->td_intr_frame (just like it was before). If the consecutive
>> calls to intr_event_handle() supply 0/NULL as trapframe, filter
>> function which wants trapframe instead of argument will get the
>> saved one. So the only situation where the behavior is different
>> is case where you call intr_event_handle(ie, NULL). BTW This change
>> was discussed some time ago with cognet at .
> 
> Since this is only needed by the new arm code, and the new arm code only
> calls intr_event_handle() from arm_dispatch_irq(), can't this logic just
> move into there?

I agree with this as well… I don’t think it is a safe assumption that we can just
change the MI behavior here...

> Also, I have a feeling EOI isn't being done in all the right places, but
> it could be a matter of misreading the diff.  I need to actually apply
> the diff and read the code, and I'll get to that soon, really I will (I
> seem to be writing a new definition for "soon" every time I say that).

Yea, I had that on my todo list to check carefully when I had time to
look at this stuff in greater detail.

Warner