Re: Fwd: [RFC] IRQ type flags

Ben Dooks
2005-11-07T12:42:26Z

On Sun, Nov 06, 2005 at 08:40:12AM +0000, Russell King wrote:
> I haven't had any feedback on this patch. akpm - can you add it to -mm
> please? Here's the sign-off for it, thanks.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>
> ----- Forwarded message from Russell King <rmk+lkml@arm.linux.org.uk> -----
> Date: Fri, 28 Oct 2005 22:57:47 +0100
> From: Russell King <rmk+lkml@arm.linux.org.uk>
> To: Linux Kernel List <linux-kernel@vger.kernel.org>
> Subject: [RFC] IRQ type flags
>
> Hi,
>
> 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.

surely, thats the other way around, going from edge to level.

> 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.

I agree that the type of IRQ should be considered when registering
the IRQ. On the s3c2410, most boards do set the level/edge correctly
before startup (bootloader) but occasionally, the bootloader cannot
deal with all the cases.

> 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.
>
> Comments?
>
> 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
> @@ -681,10 +681,16 @@ int setup_irq(unsigned int irq, struct i
> */
> desc = irq_desc + irq;
> spin_lock_irqsave(&irq_controller_lock, flags);
> +#define SA_TRIGGER (SA_TRIGGER_HIGH|SA_TRIGGER_LOW|\
> + SA_TRIGGER_RISING|SA_TRIGGER_FALLING)
> 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) {
> spin_unlock_irqrestore(&irq_controller_lock, flags);
> return -EBUSY;
> }
> @@ -704,6 +710,12 @@ int setup_irq(unsigned int irq, struct i
> desc->running = 0;
> desc->pending = 0;
> desc->disable_depth = 1;
> +
> + if (new->flags & SA_TRIGGER) {
> + 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/linux/signal.h b/include/linux/signal.h
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -18,6 +18,14 @@
> #define SA_PROBE SA_ONESHOT
> #define SA_SAMPLE_RANDOM SA_RESTART
> #define SA_SHIRQ 0x04000000
> +/*
> + * As above, these correspond to the __IRQT defines in asm-arm/irq.h
> + * to select the interrupt line behaviour.
> + */
> +#define SA_TRIGGER_HIGH 0x00000008
> +#define SA_TRIGGER_LOW 0x00000004
> +#define SA_TRIGGER_RISING 0x00000002
> +#define SA_TRIGGER_FALLING 0x00000001

How about making these compatible with the
triggers compatible with the flags from
include/linux/ioport.h definitions for the
IRQ resource (IORESOURCE_IRQ_*).

This would make it easier to pass the resource's
flags field to the register irq code, and get
the right IRQ type for the app?

--
Ben (ben@fluff.org, http://www.fluff.org/)

'a smiley only costs 4 bytes'