Patchwork [4/4] sparc32: genirq conversion (buggy)

login
register
mail settings
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

Sam Ravnborg - Feb. 13, 2011, 9:06 p.m.
On Sun, Feb 13, 2011 at 12:54:01PM -0800, David Miller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Sun, 13 Feb 2011 20:29:52 +0100
> 
> > +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)
> > +			return i;
> > +	}
>  ...
> > +	irq_table[i].irq = i;
> > +	irq_table[i].pil = pil;
> > +
> > +	return i;
> > +}
> 
> Well, what is it?  Are we storing "real_irq" in irq_table[].irq or
> "i"?  I think it's supposed to be "real_irq" and that's one of the
> reasons your implementation isn't working :-)

Good catch. I changed the above as I require irq in handler_irq()
to lookup the relevant irq_desc. But I obviously scrwed up.

From handler_irq():

                unsigned int irq;

                irq = p->irq;
                desc = irq_to_desc(irq);

                if (!(desc->status & IRQ_DISABLED))
                        desc->handle_irq(irq, desc);

So I fixed this - but result is the same - we hang in calibrate_dealy().

	Sam

My fix looks like this:




--
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
David Miller - Feb. 13, 2011, 9:19 p.m.
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
Sam Ravnborg - Feb. 13, 2011, 9:48 p.m.
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
David Miller - Feb. 13, 2011, 10 p.m.
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
Sam Ravnborg - Feb. 13, 2011, 10:30 p.m.
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
David Miller - Feb. 13, 2011, 11:05 p.m.
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;