| Submitter | Sam Ravnborg |
|---|---|
| Date | Feb. 13, 2011, 9:06 p.m. |
| Message ID | <20110213210651.GA27283@merkur.ravnborg.org> |
| Download | mbox | patch |
| Permalink | /patch/83006/ |
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
Comments
From: Sam Ravnborg <sam@ravnborg.org> Date: Sun, 13 Feb 2011 22:06:51 +0100 > So I fixed this - but result is the same - we hang in calibrate_dealy(). Well, the next problem I noticed is that you only enable the timer IRQ on the cpu on which request_irq() executes, what about all of the other cpus in the system? -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 13, 2011 at 01:19:50PM -0800, David Miller wrote: > From: Sam Ravnborg <sam@ravnborg.org> > Date: Sun, 13 Feb 2011 22:06:51 +0100 > > > So I fixed this - but result is the same - we hang in calibrate_dealy(). > > Well, the next problem I noticed is that you only enable the timer IRQ > on the cpu on which request_irq() executes, what about all of the > other cpus in the system? Trying to investigate this I actually found a bug: @@ -267,7 +268,7 @@ static unsigned int sun4m_build_device_irq(struct platform_device *o } handler_data->mask = sun4m_imask[real_irq]; - handler_data->percpu = irq < OBP_INT_LEVEL_ONBOARD; + handler_data->percpu = real_irq < OBP_INT_LEVEL_ONBOARD; set_irq_chip(irq, &sun4m_irq); set_irq_data(irq, handler_data); percpu is a bool used in the enable/disable handlers to determine if this is a system wide or a percpu specific irq. Before the above change the timer interrupt was considered a cpu specific irq - resulting in the wrong register being set. With the above change I got one step further - now I see a "Watchdog Reset". I am yet to investigate why. > what about all of the other cpus in the system? I really do not know :-( This code is as much as I could a copy of the original implementation (except a floppy hack). In the original implmentation we did the same via __enable_irq(). And as the timer interrupt is enabled in the system wide interrupt register - I think this is OK. But I do not (yet) see how to cope with softint's. As they are unused for now they can wait. Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sam Ravnborg <sam@ravnborg.org> Date: Sun, 13 Feb 2011 22:48:44 +0100 > In the original implmentation we did the same via __enable_irq(). And we did so Via a function that's invoked by every cpu as it is booted. Right? -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 13, 2011 at 02:00:26PM -0800, David Miller wrote: > From: Sam Ravnborg <sam@ravnborg.org> > Date: Sun, 13 Feb 2011 22:48:44 +0100 > > > In the original implmentation we did the same via __enable_irq(). > > And we did so Via a function that's invoked by every cpu as it is booted. > > Right? To the best of my knowledge - no. In irq_32.c:request_irq() we had: __enable_irq(irq); // irq is what we consider real_irq In irq.h: we had: BTFIXUPDEF_CALL(void, enable_irq, unsigned int) static inline void __enable_irq(unsigned int irq) { BTFIXUP_CALL(enable_irq)(irq); } In sun4m_irq.c we had: void __init sun4m_init_IRQ(void) { ... BTFIXUPSET_CALL(enable_irq, sun4m_enable_irq, BTFIXUPCALL_NORM); So unless the btfix stuff does trick I do not understand then I do not see this iterate over all cpu's. I guess there is something obvious I have missed :-( One thing I have left-out on the sun4m side is a call to smp4m_irq_rotate() that for each interrupt let the next be the interrupt target. But I do not see that interrupts are globally enabled. Note: I got one step further. It helped that I called irq_ack(), irq_enable() around my desc->handle_irq(irq, desc); But desc->handle_irq in NULL. That will be for another day - it is getting late. Thanks for all the help so far! Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sam Ravnborg <sam@ravnborg.org> Date: Sun, 13 Feb 2011 23:30:18 +0100 > On Sun, Feb 13, 2011 at 02:00:26PM -0800, David Miller wrote: >> From: Sam Ravnborg <sam@ravnborg.org> >> Date: Sun, 13 Feb 2011 22:48:44 +0100 >> >> > In the original implmentation we did the same via __enable_irq(). >> >> And we did so Via a function that's invoked by every cpu as it is booted. >> >> Right? > > To the best of my knowledge - no. I was thinking of: ofarch/sparc/kernel/sun4m_smp.c:smp_setup_percpu_timer() which enables it explicitly via enable_pil_irq(14). Oh, it's a global IRQ, so we should be Ok nevermind. :-) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
diff --git a/arch/sparc/kernel/irq_32.c b/arch/sparc/kernel/irq_32.c index f92cd34..31658bd 100644 --- a/arch/sparc/kernel/irq_32.c +++ b/arch/sparc/kernel/irq_32.c @@ -123,6 +123,7 @@ static void irq_panic(irq_handler_t handler) struct irq_bucket { struct irq_bucket *next; + unsigned int real_irq; unsigned int irq; unsigned int pil; }; @@ -138,7 +139,7 @@ unsigned int irq_alloc(unsigned int real_irq, unsigned int pil) unsigned int i; for (i = 1; i < NR_IRQS; i++) { - if (irq_table[i].irq == real_irq && irq_table[i].irq == pil) + if (irq_table[i].real_irq == real_irq && irq_table[i].pil == pil) return i; } @@ -151,6 +152,7 @@ unsigned int irq_alloc(unsigned int real_irq, unsigned int pil) printk(KERN_ERR "IRQ: Out of virtual IRQs.\n"); return 0; } + irq_table[i].real_irq = real_irq; irq_table[i].irq = i; irq_table[i].pil = pil;