[PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[Matt Fleming]

Use struct irq_chip for the interrupt handler for the HD64461. Also
convert some in{b,w} and out{b,w} calls to the equivalent __raw_* calls.
Include <linux/io.h> and not <asm/io.h> to stop checkpatch.pl
complaining.

This change should now allow machines with HD64461 to define
GENERIC_HARDIRQS_NO__DO_IRQ.

Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
Signed-off-by: Matt Fleming <mjf@gentoo.org>
---
 arch/sh/cchips/hd6446x/hd64461.c |  115 ++++++++-----------------------------
 1 files changed, 25 insertions(+), 90 deletions(-)

diff --git a/arch/sh/cchips/hd6446x/hd64461.c b/arch/sh/cchips/hd6446x/hd64461.c
index f1a4a07..27ceeb9 100644
--- a/arch/sh/cchips/hd6446x/hd64461.c
+++ b/arch/sh/cchips/hd6446x/hd64461.c
@@ -10,99 +10,49 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/irq.h>
-#include <asm/io.h>
+#include <linux/io.h>
 #include <asm/irq.h>
 #include <asm/hd64461.h>
 
 /* This belongs in cpu specific */
 #define INTC_ICR1 0xA4140010UL
 
-static void disable_hd64461_irq(unsigned int irq)
+static void hd64461_mask_irq(unsigned int irq)
 {
 	unsigned short nimr;
 	unsigned short mask = 1 << (irq - HD64461_IRQBASE);
 
-	nimr = inw(HD64461_NIMR);
+	nimr = __raw_readw(HD64461_NIMR);
 	nimr |= mask;
-	outw(nimr, HD64461_NIMR);
+	__raw_writew(nimr, HD64461_NIMR);
 }
 
-static void enable_hd64461_irq(unsigned int irq)
+static void hd64461_unmask_irq(unsigned int irq)
 {
 	unsigned short nimr;
 	unsigned short mask = 1 << (irq - HD64461_IRQBASE);
 
-	nimr = inw(HD64461_NIMR);
+	nimr = __raw_readw(HD64461_NIMR);
 	nimr &= ~mask;
-	outw(nimr, HD64461_NIMR);
+	__raw_writew(nimr, HD64461_NIMR);
 }
 
-static void mask_and_ack_hd64461(unsigned int irq)
+static void hd64461_mask_and_ack_irq(unsigned int irq)
 {
-	disable_hd64461_irq(irq);
+	hd64461_mask_irq(irq);
 #ifdef CONFIG_HD64461_ENABLER
 	if (irq = HD64461_IRQBASE + 13)
-		outb(0x00, HD64461_PCC1CSCR);
+		__raw_writeb(0x00, HD64461_PCC1CSCR);
 #endif
 }
 
-static void end_hd64461_irq(unsigned int irq)
-{
-	if (!(irq_desc[irq].status & (IRQ_DISABLED|IRQ_INPROGRESS)))
-		enable_hd64461_irq(irq);
-}
-
-static unsigned int startup_hd64461_irq(unsigned int irq)
-{
-	enable_hd64461_irq(irq);
-	return 0;
-}
-
-static void shutdown_hd64461_irq(unsigned int irq)
-{
-	disable_hd64461_irq(irq);
-}
-
-static struct hw_interrupt_type hd64461_irq_type = {
-	.typename	= "HD64461-IRQ",
-	.startup	= startup_hd64461_irq,
-	.shutdown	= shutdown_hd64461_irq,
-	.enable		= enable_hd64461_irq,
-	.disable	= disable_hd64461_irq,
-	.ack		= mask_and_ack_hd64461,
-	.end		= end_hd64461_irq,
+static struct irq_chip hd64461_irq_chip = {
+	.name		= "HD64461-IRQ",
+	.mask		= hd64461_mask_irq,
+	.mask_ack	= hd64461_mask_and_ack_irq,
+	.unmask		= hd64461_unmask_irq,
 };
 
-static irqreturn_t hd64461_interrupt(int irq, void *dev_id)
-{
-	printk(KERN_INFO
-	       "HD64461: spurious interrupt, nirr: 0x%x nimr: 0x%x\n",
-	       inw(HD64461_NIRR), inw(HD64461_NIMR));
-
-	return IRQ_NONE;
-}
-
-static struct {
-	int (*func) (int, void *);
-	void *dev;
-} hd64461_demux[HD64461_IRQ_NUM];
-
-void hd64461_register_irq_demux(int irq,
-				int (*demux) (int irq, void *dev), void *dev)
-{
-	hd64461_demux[irq - HD64461_IRQBASE].func = demux;
-	hd64461_demux[irq - HD64461_IRQBASE].dev = dev;
-}
-
-EXPORT_SYMBOL(hd64461_register_irq_demux);
-
-void hd64461_unregister_irq_demux(int irq)
-{
-	hd64461_demux[irq - HD64461_IRQBASE].func = 0;
-}
-
-EXPORT_SYMBOL(hd64461_unregister_irq_demux);
-
 int hd64461_irq_demux(int irq)
 {
 	if (irq = CONFIG_HD64461_IRQ) {
@@ -115,25 +65,11 @@ int hd64461_irq_demux(int irq)
 		for (bit = 1, i = 0; i < 16; bit <<= 1, i++)
 			if (nirr & bit)
 				break;
-		if (i = 16)
-			irq = CONFIG_HD64461_IRQ;
-		else {
-			irq = HD64461_IRQBASE + i;
-			if (hd64461_demux[i].func != 0) {
-				irq = hd64461_demux[i].func(irq, hd64461_demux[i].dev);
-			}
-		}
+		irq = HD64461_IRQBASE + i;
 	}
 	return irq;
 }
 
-static struct irqaction irq0 = {
-	.handler = hd64461_interrupt,
-	.flags = IRQF_DISABLED,
-	.mask = CPU_MASK_NONE,
-	.name = "HD64461",
-};
-
 int __init setup_hd64461(void)
 {
 	int i;
@@ -146,22 +82,21 @@ int __init setup_hd64461(void)
 	       CONFIG_HD64461_IOBASE, CONFIG_HD64461_IRQ, HD64461_IRQBASE,
 	       HD64461_IRQBASE + 15);
 
-#if defined(CONFIG_CPU_SUBTYPE_SH7709)	/* Should be at processor specific part.. */
-	outw(0x2240, INTC_ICR1);
+/* Should be at processor specific part.. */
+#if defined(CONFIG_CPU_SUBTYPE_SH7709)
+	__raw_writew(0x2240, INTC_ICR1);
 #endif
-	outw(0xffff, HD64461_NIMR);
+	__raw_writew(0xffff, HD64461_NIMR);
 
 	/*  IRQ 80 -> 95 belongs to HD64461  */
-	for (i = HD64461_IRQBASE; i < HD64461_IRQBASE + 16; i++) {
-		irq_desc[i].chip = &hd64461_irq_type;
-	}
-
-	setup_irq(CONFIG_HD64461_IRQ, &irq0);
+	for (i = HD64461_IRQBASE; i < HD64461_IRQBASE + 16; i++)
+		set_irq_chip_and_handler(i, &hd64461_irq_chip,
+					 handle_level_irq);
 
 #ifdef CONFIG_HD64461_ENABLER
 	printk(KERN_INFO "HD64461: enabling PCMCIA devices\n");
-	outb(0x4c, HD64461_PCC1CSCIER);
-	outb(0x00, HD64461_PCC1CSCR);
+	__raw_writeb(0x4c, HD64461_PCC1CSCIER);
+	__raw_writeb(0x00, HD64461_PCC1CSCR);
 #endif
 
 	return 0;
-- 
1.5.6.4

Re: [PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[Paul Mundt]

On Fri, Nov 28, 2008 at 08:58:30AM +0000, Matt Fleming wrote:
> Use struct irq_chip for the interrupt handler for the HD64461. Also
> convert some in{b,w} and out{b,w} calls to the equivalent __raw_* calls.
> Include <linux/io.h> and not <asm/io.h> to stop checkpatch.pl
> complaining.
> 
> This change should now allow machines with HD64461 to define
> GENERIC_HARDIRQS_NO__DO_IRQ.
> 
> Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
> Signed-off-by: Matt Fleming <mjf@gentoo.org>

Applied, thanks.

Re: [PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[Magnus Damm]

On Fri, Nov 28, 2008 at 5:58 PM, Matt Fleming <mjf@gentoo.org> wrote:
> Use struct irq_chip for the interrupt handler for the HD64461. Also
> convert some in{b,w} and out{b,w} calls to the equivalent __raw_* calls.
> Include <linux/io.h> and not <asm/io.h> to stop checkpatch.pl
> complaining.
>
> This change should now allow machines with HD64461 to define
> GENERIC_HARDIRQS_NO__DO_IRQ.
>
> Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
> Signed-off-by: Matt Fleming <mjf@gentoo.org>

Hi Matt and Kristoffer,

> diff --git a/arch/sh/cchips/hd6446x/hd64461.c b/arch/sh/cchips/hd6446x/hd64461.c
> index f1a4a07..27ceeb9 100644
> --- a/arch/sh/cchips/hd6446x/hd64461.c
> +++ b/arch/sh/cchips/hd6446x/hd64461.c

[snip]

> -       setup_irq(CONFIG_HD64461_IRQ, &irq0);

It looks like the interrupt driving the hd64461 chip is changed from
being requested with setup_irq()/request_irq() to using the machvec
demux. So the interrupt is handled by hooking up hd64461_irq_demux()
to the sh_machine_vector member mv_irq_demux in the
arch/sh/boards/mach-hp6xx/setup.c.

Using the demux for IRL interrupts that cannot be acknowledged and
masked by the processor is a good idea. The interrupt handling for the
r2d board which can be found in arch/sh/boards/mach-r2d/irq.c may
serve as an example. If the interrupt is a regular single IRQ pin
configured in IRQ mode then you should not use the demux.

Judging by the hp6xx board code you use IRQ3->0 in IRQ mode. If you
should use IRL or IRQ can be determined by looking at the board
schematics for your platform. If you can point me out where those are
then i can help you figure that part out.

Now when you're using the demux function then the intc handler will
never be invoked for your CONFIG_HD64461_IRQ interrupt. This means
that you won't get any acknowledge from the intc code.

If the hd64461 chip is hooked up to a single interrupt line in IRQ
mode, then I recommend you to use set_irq_chained_handler() or
set_irq_handler() to associate the interrupt line with a demux
function. This should make the intc code acknowledge the interrupt for
you.

Cheers,

/ magnus

Re: [PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[Kristoffer Ericson]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Tue, 2 Dec 2008 15:40:46 +0900
"Magnus Damm" <magnus.damm@gmail.com> wrote:

> On Fri, Nov 28, 2008 at 5:58 PM, Matt Fleming <mjf@gentoo.org> wrote:
> > Use struct irq_chip for the interrupt handler for the HD64461. Also
> > convert some in{b,w} and out{b,w} calls to the equivalent __raw_* calls.
> > Include <linux/io.h> and not <asm/io.h> to stop checkpatch.pl
> > complaining.
> >
> > This change should now allow machines with HD64461 to define
> > GENERIC_HARDIRQS_NO__DO_IRQ.
> >
> > Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
> > Signed-off-by: Matt Fleming <mjf@gentoo.org>
> 
> Hi Matt and Kristoffer,

Hi,

Sorry for my late reply, been away from school for
a week.

> 
> > diff --git a/arch/sh/cchips/hd6446x/hd64461.c b/arch/sh/cchips/hd6446x/hd64461.c
> > index f1a4a07..27ceeb9 100644
> > --- a/arch/sh/cchips/hd6446x/hd64461.c
> > +++ b/arch/sh/cchips/hd6446x/hd64461.c
> 
> [snip]
> 
> > -       setup_irq(CONFIG_HD64461_IRQ, &irq0);
> 
> It looks like the interrupt driving the hd64461 chip is changed from
> being requested with setup_irq()/request_irq() to using the machvec
> demux. So the interrupt is handled by hooking up hd64461_irq_demux()
> to the sh_machine_vector member mv_irq_demux in the
> arch/sh/boards/mach-hp6xx/setup.c.
> 

Correct.

> Using the demux for IRL interrupts that cannot be acknowledged and
> masked by the processor is a good idea. The interrupt handling for the
> r2d board which can be found in arch/sh/boards/mach-r2d/irq.c may
> serve as an example. If the interrupt is a regular single IRQ pin
> configured in IRQ mode then you should not use the demux.
> 

I'm fine with using a demuxer and maintaining the virtual
IRQ range. Would also simplify the mfd driver.

> Judging by the hp6xx board code you use IRQ3->0 in IRQ mode. If you
> should use IRL or IRQ can be determined by looking at the board
> schematics for your platform. If you can point me out where those are
> then i can help you figure that part out.

The code is somewhat misleading in that it uses
the term irq0 while requesting irq. The thought is
to think of irq0 as the base irq (for demux) and not
the actual mapping (IRQ0->IRQ5) on the hd64461.

It would be more correct to instead refer to the
hd64461 IRQ as IRQ4 (IRQ 36 on hp6xx).

We currently set IRQ0->IRQ3 to IRQ mode as you say,
we do not touch IRQ4 or IRQ5 at all. I'm not exactly
sure what the "default" puts it in. Whatever it starts
with seems to be the correct thing, we just need
to make sure it acks through IRR0 also.

You can access copies of the relevant datasheets
from :
http://jlime.com/downloads/development/datasheets/6xx/

> 
> Now when you're using the demux function then the intc handler will
> never be invoked for your CONFIG_HD64461_IRQ interrupt. This means
> that you won't get any acknowledge from the intc code.
>

Oki,

 
> If the hd64461 chip is hooked up to a single interrupt line in IRQ
> mode, then I recommend you to use set_irq_chained_handler() or
> set_irq_handler() to associate the interrupt line with a demux
> function. This should make the intc code acknowledge the interrupt for
> you.
> 
> Cheers,
> 
> / magnus


-- 
Kristoffer Ericson <kristoffer.ericson@gmail.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] sh: Switch HD64461 from hw_interrupt_type to irq_chip

[Magnus Damm]

On Fri, Dec 5, 2008 at 11:04 PM, Kristoffer Ericson
<kristoffer.ericson@gmail.com> wrote:
> On Tue, 2 Dec 2008 15:40:46 +0900
> "Magnus Damm" <magnus.damm@gmail.com> wrote:
>
>> On Fri, Nov 28, 2008 at 5:58 PM, Matt Fleming <mjf@gentoo.org> wrote:
>> > Use struct irq_chip for the interrupt handler for the HD64461. Also
>> > convert some in{b,w} and out{b,w} calls to the equivalent __raw_* calls.
>> > Include <linux/io.h> and not <asm/io.h> to stop checkpatch.pl
>> > complaining.
>> >
>> > This change should now allow machines with HD64461 to define
>> > GENERIC_HARDIRQS_NO__DO_IRQ.
>> >
>> > Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
>> > Signed-off-by: Matt Fleming <mjf@gentoo.org>

Hi Kristoffer,

> Sorry for my late reply, been away from school for
> a week.

No worries, thanks for following up.

>> > diff --git a/arch/sh/cchips/hd6446x/hd64461.c b/arch/sh/cchips/hd6446x/hd64461.c
>> > index f1a4a07..27ceeb9 100644
>> > --- a/arch/sh/cchips/hd6446x/hd64461.c
>> > +++ b/arch/sh/cchips/hd6446x/hd64461.c
>>
>> [snip]
>>
>> > -       setup_irq(CONFIG_HD64461_IRQ, &irq0);
>>
>> It looks like the interrupt driving the hd64461 chip is changed from
>> being requested with setup_irq()/request_irq() to using the machvec
>> demux. So the interrupt is handled by hooking up hd64461_irq_demux()
>> to the sh_machine_vector member mv_irq_demux in the
>> arch/sh/boards/mach-hp6xx/setup.c.
>>
>
> Correct.
>
>> Using the demux for IRL interrupts that cannot be acknowledged and
>> masked by the processor is a good idea. The interrupt handling for the
>> r2d board which can be found in arch/sh/boards/mach-r2d/irq.c may
>> serve as an example. If the interrupt is a regular single IRQ pin
>> configured in IRQ mode then you should not use the demux.
>>
>
> I'm fine with using a demuxer and maintaining the virtual
> IRQ range. Would also simplify the mfd driver.

Heh, I'm not trying to convince you to stop demuxing your interrupts.
=) Maintaining the range of interrupts and demuxing them is a good
idea, but the way you connect the demux function doesn't seem
consistent with the interrupt mode you configure the hardware in.

Using the machvec is correct if you use IRL mode. If you are using IRQ
mode you should _not_ hook in using the machvec.

IRL mode interrupts cannot be masked using the internal processor
hardware, so it's not possible to support them as regular intc
interrupt sources. The only way to support them is by using the
machvec demux hook together with board specific code that masks the
interrupts using external hardware.

IRQ mode always supports masking in the processor, so you can let intc
handle all details about the interrupt for you in that case. Note that
this only applies to IRQ5 in your specific case with the companion
chip, you still need to mask and manage interrupts in your own private
range like today.

>> Judging by the hp6xx board code you use IRQ3->0 in IRQ mode. If you
>> should use IRL or IRQ can be determined by looking at the board
>> schematics for your platform. If you can point me out where those are
>> then i can help you figure that part out.
>
> The code is somewhat misleading in that it uses
> the term irq0 while requesting irq. The thought is
> to think of irq0 as the base irq (for demux) and not
> the actual mapping (IRQ0->IRQ5) on the hd64461.
>
> It would be more correct to instead refer to the
> hd64461 IRQ as IRQ4 (IRQ 36 on hp6xx).

Using IRQ4 as name would maybe be a bit more clear, yes. =) The
interrupt pin name seems to be IRQ0 on the companion chip though so
the name is not so strange from that perspective.

You should be able to use intc as is today. Judging by this patch you
seem to be using intc in an inconsistent fashion. Or rather - since
you hook in your demux function using the machvec you don't use intc
for IRQ5 at all. The demux function in the machvec gets called before
intc so you never let intc get the change to handle the acknowledge
for you.

> We currently set IRQ0->IRQ3 to IRQ mode as you say,
> we do not touch IRQ4 or IRQ5 at all. I'm not exactly
> sure what the "default" puts it in. Whatever it starts
> with seems to be the correct thing, we just need
> to make sure it acks through IRR0 also.

The default depends on processor type. Processors that only have one
configuration should use that by default. This may vary a bit though
depending on the level of support. Please have a look at the intc code
to see which sources that get installed by default.

> You can access copies of the relevant datasheets
> from :
> http://jlime.com/downloads/development/datasheets/6xx/

Thanks. It looks like the companion chip only has a single IRQ output
so then IRL mode is out of the question. IRQ mode seems correct. So
please try yo use set_irq_chained_handler() and see if your interrupts
get acked properly.

This is how I add support for external interrupt controllers
step-by-step, I recommend you to follow the same way if you run into
trouble.

1. Change your kernel config to only include support for a single
piece of hardware that is using your parent interrupt, ie IRQ5 in your
case. For instance, if your hardware platform makes use of the serial
port in the companion chip then I recommend you to just use that to
begin with. Maybe it's difficult to boot your kernel fully with only a
single driver, but remember that you don't really need any user space
to hack up this code. You can do everything using compiled-in drivers.
If you need user space to trigger your interrupt then i recommend
initramfs.

2. Modify your demux code to only allow unmasking of the interrupt
associated with your hardware device. Only one interrupt source is
allowed to begin with.

3. Trigger the interrupt somehow - this depends on the type of
hardware. This may happen during probe() of the driver, but that
depends. Make sure you can mask, unmask and ack the interrupt
properly, both using the parent interrupt (IRQ5) and your own external
interrupt. You can use printk or toggle leds or something fancier to
check that the interrupts are triggered as you expect. If you can't
get this working then it may be good to skip this particular hardware
device for now and move on to a different one and come back later.

4. When interrupts for a device is working then make a patch and save
that somewhere. Then start over with another hardware device.

5. Repeat until all devices are supported or until you're satisfied.

6. Combine all patches and make sure you can use the interrupts together.

7. Post patch. =)

Good luck. If you get stuck at step 3 then let me know and i'll do my
best to help out.

Cheers,

/ magnus