[PATCH] Alchemy: modernize Pb1200 IRQ cascade handling code.

[PATCH] Alchemy: modernize Pb1200 IRQ cascade handling code.

[Manuel Lauss]

Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
---
 arch/mips/au1000/pb1200/irqmap.c |   75 +++++++++-----------------------------
 1 files changed, 18 insertions(+), 57 deletions(-)

diff --git a/arch/mips/au1000/pb1200/irqmap.c b/arch/mips/au1000/pb1200/irqmap.c
index 2a505ad..7229f30 100644
--- a/arch/mips/au1000/pb1200/irqmap.c
+++ b/arch/mips/au1000/pb1200/irqmap.c
@@ -48,62 +48,26 @@ int __initdata au1xxx_nr_irqs = ARRAY_SIZE(au1xxx_irq_map);
 /*
  * Support for External interrupts on the Pb1200 Development platform.
  */
-static volatile int pb1200_cascade_en;
 
-irqreturn_t pb1200_cascade_handler(int irq, void *dev_id)
+static void pb1200_cascade_handler(unsigned int irq, struct irq_desc *desc)
 {
 	unsigned short bisr = bcsr->int_status;
-	int extirq_nr = 0;
-
-	/* Clear all the edge interrupts. This has no effect on level. */
-	bcsr->int_status = bisr;
-	for ( ; bisr; bisr &= bisr - 1) {
-		extirq_nr = PB1200_INT_BEGIN + __ffs(bisr);
-		/* Ack and dispatch IRQ */
-		do_IRQ(extirq_nr);
-	}
 
-	return IRQ_RETVAL(1);
+	for ( ; bisr; bisr &= bisr - 1)
+		generic_handle_irq(PB1200_INT_BEGIN + __ffs(bisr));
 }
 
-inline void pb1200_enable_irq(unsigned int irq_nr)
+static void pb1200_unmask_irq(unsigned int irq_nr)
 {
 	bcsr->intset_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
 	bcsr->intset = 1 << (irq_nr - PB1200_INT_BEGIN);
 }
 
-inline void pb1200_disable_irq(unsigned int irq_nr)
+static void pb1200_maskack_irq(unsigned int irq_nr)
 {
 	bcsr->intclr_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
 	bcsr->intclr = 1 << (irq_nr - PB1200_INT_BEGIN);
-}
-
-static unsigned int pb1200_setup_cascade(void)
-{
-	return request_irq(AU1000_GPIO_7, &pb1200_cascade_handler,
-			   0, "Pb1200 Cascade", &pb1200_cascade_handler);
-}
-
-static unsigned int pb1200_startup_irq(unsigned int irq)
-{
-	if (++pb1200_cascade_en == 1) {
-		int res;
-
-		res = pb1200_setup_cascade();
-		if (res)
-			return res;
-	}
-
-	pb1200_enable_irq(irq);
-
-	return 0;
-}
-
-static void pb1200_shutdown_irq(unsigned int irq)
-{
-	pb1200_disable_irq(irq);
-	if (--pb1200_cascade_en == 0)
-		free_irq(AU1000_GPIO_7, &pb1200_cascade_handler);
+	bcsr->int_status = 1 << (irq_nr - PB1200_INT_BEGIN);	/* ack */
 }
 
 static struct irq_chip external_irq_type = {
@@ -113,12 +77,9 @@ static struct irq_chip external_irq_type = {
 #ifdef CONFIG_MIPS_DB1200
 	.name = "Db1200 Ext",
 #endif
-	.startup  = pb1200_startup_irq,
-	.shutdown = pb1200_shutdown_irq,
-	.ack      = pb1200_disable_irq,
-	.mask     = pb1200_disable_irq,
-	.mask_ack = pb1200_disable_irq,
-	.unmask   = pb1200_enable_irq,
+	.mask		= pb1200_maskack_irq,
+	.mask_ack	= pb1200_maskack_irq,
+	.unmask		= pb1200_unmask_irq,
 };
 
 void _board_init_irq(void)
@@ -147,14 +108,14 @@ void _board_init_irq(void)
 	}
 #endif
 
-	for (irq = PB1200_INT_BEGIN; irq <= PB1200_INT_END; irq++) {
-		set_irq_chip_and_handler(irq, &external_irq_type,
-					 handle_level_irq);
-		pb1200_disable_irq(irq);
-	}
+	/* mask & disable & ack all */
+	bcsr->intclr_mask = 0xffff;
+	bcsr->intclr = 0xffff;
+	bcsr->int_status = 0xffff;
+
+	for (irq = PB1200_INT_BEGIN; irq <= PB1200_INT_END; irq++)
+		set_irq_chip_and_handler_name(irq, &external_irq_type,
+					 handle_level_irq, "level");
 
-	/*
-	 * GPIO_7 can not be hooked here, so it is hooked upon first
-	 * request of any source attached to the cascade.
-	 */
+	set_irq_chained_handler(AU1000_GPIO_7, pb1200_cascade_handler);
 }
-- 
1.5.6.4

Re: [PATCH] Alchemy: modernize Pb1200 IRQ cascade handling code.

[Sergei Shtylyov]

Hello.

Manuel Lauss wrote:

> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
>   
[...]
> diff --git a/arch/mips/au1000/pb1200/irqmap.c b/arch/mips/au1000/pb1200/irqmap.c
> index 2a505ad..7229f30 100644
> --- a/arch/mips/au1000/pb1200/irqmap.c
> +++ b/arch/mips/au1000/pb1200/irqmap.c
> @@ -48,62 +48,26 @@ int __initdata au1xxx_nr_irqs = ARRAY_SIZE(au1xxx_irq_map);
>  /*
>   * Support for External interrupts on the Pb1200 Development platform.
>   */
> -static volatile int pb1200_cascade_en;
>  
> -irqreturn_t pb1200_cascade_handler(int irq, void *dev_id)
> +static void pb1200_cascade_handler(unsigned int irq, struct irq_desc *desc)
>  {
>  	unsigned short bisr = bcsr->int_status;
> -	int extirq_nr = 0;
> -
> -	/* Clear all the edge interrupts. This has no effect on level. */
>   

   Note this comment...

> -	bcsr->int_status = bisr;
> -	for ( ; bisr; bisr &= bisr - 1) {
> -		extirq_nr = PB1200_INT_BEGIN + __ffs(bisr);
> -		/* Ack and dispatch IRQ */
> -		do_IRQ(extirq_nr);
> -	}
>  
> -	return IRQ_RETVAL(1);
> +	for ( ; bisr; bisr &= bisr - 1)
> +		generic_handle_irq(PB1200_INT_BEGIN + __ffs(bisr));
>  }
>  
> -inline void pb1200_enable_irq(unsigned int irq_nr)
> +static void pb1200_unmask_irq(unsigned int irq_nr)
>  {
>  	bcsr->intset_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
>  	bcsr->intset = 1 << (irq_nr - PB1200_INT_BEGIN);
>  }
>  
> -inline void pb1200_disable_irq(unsigned int irq_nr)
> +static void pb1200_maskack_irq(unsigned int irq_nr)
>  {
>  	bcsr->intclr_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
>  	bcsr->intclr = 1 << (irq_nr - PB1200_INT_BEGIN);
>   

   I wonder what's the difference between int{clr|set} and 
int{clr|set}_mask registers...

[...]
> +	bcsr->int_status = 1 << (irq_nr - PB1200_INT_BEGIN);	/* ack */
>   

   The above comment said that writing to this register has no effect on 
the level-triggered interrupts, so this statement doesn't seem to make 
sense since you're treating all interrupts as level-triggered below.

> @@ -113,12 +77,9 @@ static struct irq_chip external_irq_type = {
>  #ifdef CONFIG_MIPS_DB1200
>  	.name = "Db1200 Ext",
>  #endif
> -	.startup  = pb1200_startup_irq,
> -	.shutdown = pb1200_shutdown_irq,
> -	.ack      = pb1200_disable_irq,
> -	.mask     = pb1200_disable_irq,
> -	.mask_ack = pb1200_disable_irq,
> -	.unmask   = pb1200_enable_irq,
> +	.mask		= pb1200_maskack_irq,
> +	.mask_ack	= pb1200_maskack_irq,
>   

   You can use the same function for the mask() and mask_ack() methods 
but it only should be masking IRQ, as clearing it doesn't make sense...

> @@ -147,14 +108,14 @@ void _board_init_irq(void)
>  	}
>  #endif
>  
> -	for (irq = PB1200_INT_BEGIN; irq <= PB1200_INT_END; irq++) {
> -		set_irq_chip_and_handler(irq, &external_irq_type,
> -					 handle_level_irq);
> -		pb1200_disable_irq(irq);
> -	}
> +	/* mask & disable & ack all */
> +	bcsr->intclr_mask = 0xffff;
> +	bcsr->intclr = 0xffff;
> +	bcsr->int_status = 0xffff;
> +
> +	for (irq = PB1200_INT_BEGIN; irq <= PB1200_INT_END; irq++)
> +		set_irq_chip_and_handler_name(irq, &external_irq_type,
> +					 handle_level_irq, "level");
>   

   Are all those IRQs indeed level-triggered?

WBR, Sergei

Re: [PATCH] Alchemy: modernize Pb1200 IRQ cascade handling code.

[Manuel Lauss]

On Wed, 13 Aug 2008 16:34:19 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:



> > -inline void pb1200_enable_irq(unsigned int irq_nr)
> > +static void pb1200_unmask_irq(unsigned int irq_nr)
> >  {
> >  	bcsr->intset_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
> >  	bcsr->intset = 1 << (irq_nr - PB1200_INT_BEGIN);
> >  }
> >  
> > -inline void pb1200_disable_irq(unsigned int irq_nr)
> > +static void pb1200_maskack_irq(unsigned int irq_nr)
> >  {
> >  	bcsr->intclr_mask = 1 << (irq_nr - PB1200_INT_BEGIN);
> >  	bcsr->intclr = 1 << (irq_nr - PB1200_INT_BEGIN);
> >   
> 
>    I wonder what's the difference between int{clr|set} and 
> int{clr|set}_mask registers...

The irq assertion equation in the CPLD is:

int_condition = inten & intmask & int_input_pin;

In theory, the ->startup() and ->shutdown() methods should fiddle with
the enable bits;  the ->mask()/->unmask() methods should modfiy the mask
bits.  In practice, at least on my DB1200, if not BOTH of the enable
and mask bits are cleared, the CPLD triggers tons of spurious
interrupts.  I read through the verilog sources but could not find a
reason for this...


> [...]
> > +	bcsr->int_status = 1 << (irq_nr - PB1200_INT_BEGIN);	/* ack */
> >   
> 
>    The above comment said that writing to this register has no effect on 
> the level-triggered interrupts, so this statement doesn't seem to make 
> sense since you're treating all interrupts as level-triggered below.

They _all_ behave level-triggered, even the ones that should be edge.
Again, looking at the verilog code, no edge detection or similar is
implemented; for instance the SD card insert/eject irqs should
be edge-triggered, but behave differently: the insert irq stays asserted
as long as a card is in the socket, the eject irq is asserted as long
as no card is present.  The PCMCIA carrdetect irqs exhibit identical
behaviour.


> > @@ -113,12 +77,9 @@ static struct irq_chip external_irq_type = {
> >  #ifdef CONFIG_MIPS_DB1200
> >  	.name = "Db1200 Ext",
> >  #endif
> > -	.startup  = pb1200_startup_irq,
> > -	.shutdown = pb1200_shutdown_irq,
> > -	.ack      = pb1200_disable_irq,
> > -	.mask     = pb1200_disable_irq,
> > -	.mask_ack = pb1200_disable_irq,
> > -	.unmask   = pb1200_enable_irq,
> > +	.mask		= pb1200_maskack_irq,
> > +	.mask_ack	= pb1200_maskack_irq,
> >   
> 
>    You can use the same function for the mask() and mask_ack() methods 
> but it only should be masking IRQ, as clearing it doesn't make sense...

It doesn't make a difference in practice, but I'll create a separate
->mask() method without the acking part of the supposedly-edge ints ;-)


> > +	for (irq = PB1200_INT_BEGIN; irq <= PB1200_INT_END; irq++)
> > +		set_irq_chip_and_handler_name(irq, &external_irq_type,
> > +					 handle_level_irq, "level");
> >   
> 
>    Are all those IRQs indeed level-triggered?

See above.

Thanks,
	Manuel Lauss