Re: [RFC] IRQ type flags

[Kumar Gala <galak@kernel.crashing.org>]

On Dec 12, 2005, at 5:47 AM, Russell King wrote:

> Here's an updated patch, taking account of folks comments.  The  
> changes
> from the previous patch are:
>
> * Move SA_TRIGGER into linux/signal.h
> * Change SA_TRIGGER_* to match IORESOURCE_IRQ_* definitions
> * Change ARM __IRQT_* definitions to match IORESOURCE_IRQ_*  
> definitions
> * Add a comment to explain the case where no SA_TRIGGER_* flags are
>   passed to request_irq.
>
> ----
> Some ARM platforms have the ability to program the interrupt  
> controller to
> detect various interrupt edges and/or levels.  For some platforms,  
> this is
> critical to setup correctly, particularly those which the setting is
> dependent on the device.
>
> Currently, ARM drivers do (eg) the following:
>
> 	err = request_irq(irq, ...);
>
> 	set_irq_type(irq, IRQT_RISING);
>
> However, if the interrupt has previously been programmed to be level
> sensitive (for whatever reason) then this will cause an interrupt  
> storm.
>
> Hence, if we combine set_irq_type() with request_irq(), we can then  
> safely
> set the type prior to unmasking the interrupt.  The unfortunate  
> problem is
> that in order to support this, these flags need to be visible  
> outside of
> the ARM architecture - drivers such as smc91x need these flags and  
> they're
> cross-architecture.
>
> Finally, the SA_TRIGGER_* flag passed to request_irq() should  
> reflect the
> property that the device would like.  The IRQ controller code  
> should do its
> best to select the most appropriate supported mode.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>
>  arch/arm/kernel/irq.c  |   14 ++++++++++++--
>  include/asm-arm/irq.h  |   12 ++++++++----
>  include/linux/signal.h |   13 +++++++++++++
>  3 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
> --- a/arch/arm/kernel/irq.c
> +++ b/arch/arm/kernel/irq.c
> @@ -684,8 +684,12 @@ int setup_irq(unsigned int irq, struct i
>  	spin_lock_irqsave(&irq_controller_lock, flags);
>  	p = &desc->action;
>  	if ((old = *p) != NULL) {
> -		/* Can't share interrupts unless both agree to */
> -		if (!(old->flags & new->flags & SA_SHIRQ)) {
> +		/*
> +		 * Can't share interrupts unless both agree to and are
> +		 * the same type.
> +		 */
> +		if (!(old->flags & new->flags & SA_SHIRQ) ||
> +		    (~old->flags & new->flags) & SA_TRIGGER_MASK) {
>  			spin_unlock_irqrestore(&irq_controller_lock, flags);
>  			return -EBUSY;
>  		}
> @@ -705,6 +709,12 @@ int setup_irq(unsigned int irq, struct i
>  		desc->running = 0;
>  		desc->pending = 0;
>  		desc->disable_depth = 1;
> +
> +		if (new->flags & SA_TRIGGER_MASK) {
> +			unsigned int type = new->flags & SA_TRIGGER;
> +			desc->chip->set_type(irq, type);
> +		}
> +
>  		if (!desc->noautoenable) {
>  			desc->disable_depth = 0;
>  			desc->chip->unmask(irq);
> diff --git a/include/asm-arm/irq.h b/include/asm-arm/irq.h
> --- a/include/asm-arm/irq.h
> +++ b/include/asm-arm/irq.h
> @@ -25,10 +25,14 @@ extern void disable_irq_nosync(unsigned
>  extern void disable_irq(unsigned int);
>  extern void enable_irq(unsigned int);
>
> -#define __IRQT_FALEDGE	(1 << 0)
> -#define __IRQT_RISEDGE	(1 << 1)
> -#define __IRQT_LOWLVL	(1 << 2)
> -#define __IRQT_HIGHLVL	(1 << 3)
> +/*
> + * These correspond with the SA_TRIGGER_* defines, and therefore the
> + * IRQRESOURCE_IRQ_* defines.

comment nit.  Should be IORESOURCE_IRQ_* not IRQRESOURCE_IRQ_*

> + */
> +#define __IRQT_RISEDGE	(1 << 0)
> +#define __IRQT_FALEDGE	(1 << 1)
> +#define __IRQT_HIGHLVL	(1 << 2)
> +#define __IRQT_LOWLVL	(1 << 3)
>
>  #define IRQT_NOEDGE	(0)
>  #define IRQT_RISING	(__IRQT_RISEDGE)
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -18,6 +18,19 @@
>  #define SA_PROBE		SA_ONESHOT
>  #define SA_SAMPLE_RANDOM	SA_RESTART
>  #define SA_SHIRQ		0x04000000
> +/*
> + * As above, these correspond to the IORESOURCE_IRQ_* defines in
> + * linux/ioport.h to select the interrupt line behaviour.  When
> + * requesting an interrupt without specifying a SA_TRIGGER, the
> + * setting should be assumed to be "as already configured", which
> + * may be as per machine or firmware initialisation.
> + */
> +#define SA_TRIGGER_LOW		0x00000008
> +#define SA_TRIGGER_HIGH		0x00000004
> +#define SA_TRIGGER_FALLING	0x00000002
> +#define SA_TRIGGER_RISING	0x00000001
> +#define SA_TRIGGER_MASK	(SA_TRIGGER_HIGH|SA_TRIGGER_LOW|\
> +				 SA_TRIGGER_RISING|SA_TRIGGER_FALLING)

Do you mind expand the comment to convey that LOW/HIGH are related to  
level sensitive interrupts and FALLING/RISING are for edge.  This is  
different naming that I'm used to with PowerPC and it can get a  
little confusing.

- kumar