Date: Tue, 15 Apr 2014 10:41:42 -0600 From: Ian Lepore <ian@FreeBSD.org> To: Jakub Klama <jakub.klama@uj.edu.pl> Cc: freebsd-arm@FreeBSD.org Subject: Re: [RFC] Refactored interrupt handling on ARM Message-ID: <1397580102.1124.121.camel@revolution.hippie.lan> In-Reply-To: <619da7d72d2345b1fcac5426b45c6ead@uj.edu.pl> References: <3e7f866f4bc774975ae3c85e0df78ec2@uj.edu.pl> <53418D13.7030107@freebsd.org> <534C0F48.2090302@freebsd.org> <f2bebfa812ecb70f423b6be4779b217b@uj.edu.pl> <534C5A6A.1090707@freebsd.org> <246c2ef842c2b47eb2400c1f700ad441@uj.edu.pl> <534CC733.7010009@freebsd.org> <ACD52C94-A44A-42FD-8016-61B0B21B12D9@bsdimp.com> <619da7d72d2345b1fcac5426b45c6ead@uj.edu.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
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=92ve done a small portion of > > this sort of work for the Atmel port, so I=92ll see how well this can= =20 > > be > > extended to work there. Unlike the gic device, there=92s 3 or 4 pio > > controllers that act as interrupt handlers, plus the need to wire up > > pins relatively early... > > > > I=92d clean up the name =91intrng.c=92 though, since ng things tend t= o > > become dated=85 ARM_INTRNG also suffers from this naming flaw. Is the= re > > any reason not to have it on all the time? Also, the ARM_INTRNG ifdef > > is inconsistently applied. >=20 > Well, the naming thing is the least significant one I guess... and > one which can be fixed in a moment. >=20 > 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. >=20 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. > > 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. >=20 > Can you give an example? >=20 > > Linux specifies the GPIO more directly (though each SoC is different > > in the details). I haven=92t looked at LPC (where I noticed the chang= e) > > 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? >=20 > LPC changes purpose was rather to give an example, surely other > platforms can benefit more from these. >=20 > > While I appreciate the aesthetic appeal of having purely an interrupt > > number and PIC (interrupt parent), I=92m not sure that will work > > everywhere. > > > > dosoftints() does nothing, is that on purpose? >=20 > It's present to not break compatibility and I'm not even sure what > should it do. However: >=20 > % ack dosoftints > arm/arm/intrng.c > 417:void dosoftints(void); > 419:dosoftints(void) >=20 > arm/arm/intr.c > 138:void dosoftints(void); > 140:dosoftints(void) >=20 > is it needed anyways? >=20 > > 255 is too few interrupts to have. We need easily twice that for > > Atmel. And chance we can remove NIRQ as well? >=20 > 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 eac= h > or 256 pics with 2^24 irqs on each maximum. >=20 > 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. >=20 > > Finally, I=92m not sure what the change to kern_intr.c is supposed to > > accomplish. It seems weird to me. Can you explain it? I=92d think tha= t > > would have a bad effect on other platforms. >=20 > 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@. 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? 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). -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1397580102.1124.121.camel>