Date: Tue, 15 Apr 2014 14:02:05 -0600 From: Warner Losh <imp@bsdimp.com> To: Ian Lepore <ian@FreeBSD.org> Cc: freebsd-arm@FreeBSD.org Subject: Re: [RFC] Refactored interrupt handling on ARM Message-ID: <763F152E-B261-4BB4-A289-4E841C50BD81@bsdimp.com> In-Reply-To: <1397580102.1124.121.camel@revolution.hippie.lan> 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> <1397580102.1124.121.camel@revolution.hippie.lan>
index | next in thread | previous in thread | raw e-mail
On Apr 15, 2014, at 10:41 AM, Ian Lepore <ian@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@. > > 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. Warnerhelp
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?763F152E-B261-4BB4-A289-4E841C50BD81>