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 <[email protected]>
----- Forwarded message from Russell King <[email protected]> -----
Date: Fri, 28 Oct 2005 22:57:47 +0100
From: Russell King <[email protected]>
To: Linux Kernel List <[email protected]>
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.
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.
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
/*
* Real Time signals may be queued.
On Sul, 2005-11-06 at 08:40 +0000, Russell King wrote:
> 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?
This is actually true of some x86 hardware in the EISA space where there
is a control register for level v edge that we sort of half deal with.
On Sun, Nov 06, 2005 at 10:41:37PM +0000, Alan Cox wrote:
> On Sul, 2005-11-06 at 08:40 +0000, Russell King wrote:
> > 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?
>
> This is actually true of some x86 hardware in the EISA space where there
> is a control register for level v edge that we sort of half deal with.
Thanks Alan. Can I assume you're happy with the patch, even if x86
currently ignores the flags?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Sul, 2005-11-06 at 22:16 +0000, Russell King wrote:
> > This is actually true of some x86 hardware in the EISA space where there
> > is a control register for level v edge that we sort of half deal with.
>
> Thanks Alan. Can I assume you're happy with the patch, even if x86
> currently ignores the flags?
I'm certainly happy with it. Both the APIC and EISA IRQ control
registers could even be made to honour it if anyone ever needed to.
Should platforms that don't support the flags be patched to error
requests they don't support however ?
Alan
On Sun, Nov 06, 2005 at 10:59:58PM +0000, Alan Cox wrote:
> On Sul, 2005-11-06 at 22:16 +0000, Russell King wrote:
> > > This is actually true of some x86 hardware in the EISA space where there
> > > is a control register for level v edge that we sort of half deal with.
> >
> > Thanks Alan. Can I assume you're happy with the patch, even if x86
> > currently ignores the flags?
>
> I'm certainly happy with it. Both the APIC and EISA IRQ control
> registers could even be made to honour it if anyone ever needed to.
>
> Should platforms that don't support the flags be patched to error
> requests they don't support however ?
Good question - I'm not sure currently. In the places where set_irq_type
is used on ARM, we're mainly interested in setting the input according
to rising/falling edge or high/low levels rather than switching between
edge and level mode.
We could do as you suggest, but my concern would be adding extra
complexity to drivers, causing them to do something like:
ret = request_irq(..., SA_TRIGGER_HIGH, ...);
if (ret == -E<whatever>)
ret = request_irq(..., SA_TRIGGER_RISING, ...);
The alternative is:
ret = request_irq(..., SA_TRIGGER_HIGH | SA_TRIGGER_RISING, ...);
at which point the driver is telling the IRQ layer what it can support,
and allowing the IRQ layer to select the most appropriate type given
the driver and hardware constraints.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Sul, 2005-11-06 at 22:42 +0000, Russell King wrote:
> We could do as you suggest, but my concern would be adding extra
> complexity to drivers, causing them to do something like:
>
> ret = request_irq(..., SA_TRIGGER_HIGH, ...);
> if (ret == -E<whatever>)
> ret = request_irq(..., SA_TRIGGER_RISING, ...);
>
> The alternative is:
>
> ret = request_irq(..., SA_TRIGGER_HIGH | SA_TRIGGER_RISING, ...);
I was thinking that specifying neither would imply 'don't care' or
'system default'. That would mean existing drivers just worked and
driver authors who didnt care need take no specific action.
> Good question - I'm not sure currently. In the places where set_irq_type
> is used on ARM, we're mainly interested in setting the input according
> to rising/falling edge or high/low levels rather than switching between
> edge and level mode.
>
> We could do as you suggest, but my concern would be adding extra
> complexity to drivers, causing them to do something like:
>
> ret = request_irq(..., SA_TRIGGER_HIGH, ...);
> if (ret == -E<whatever>)
> ret = request_irq(..., SA_TRIGGER_RISING, ...);
>
> The alternative is:
>
> ret = request_irq(..., SA_TRIGGER_HIGH | SA_TRIGGER_RISING, ...);
>
> at which point the driver is telling the IRQ layer what it can support,
> and allowing the IRQ layer to select the most appropriate type given
> the driver and hardware constraints.
We have similar things on ppc but dealt differently. The type of
interrupt sense supported depends on the PIC (and you can have more than
one PIC around). On Open Firmware based machines at least, the
device-tree tells us the sense setting to use on all devices in the
system, we program our PIC based on that information.
Your proposal though is interesting as it would allow individual to
override that setting (it may be broken, firmware occasionally are), and
would probably be useful for embedded PPCs as well.
However, I would suggest defining the absence of explicit trigger (0) as
a constant rather than 0 (SA_TRIGGER_DEFAULT), just for consistency.That
means using whatever the platform considers as a good default for this
interrupt. In addition, I would still add a get_irq_trigger() function
for a driver to enquire the kind of sense that was set by default by the
platfrom for a given irq line.
Ben.
> I was thinking that specifying neither would imply 'don't care' or
> 'system default'. That would mean existing drivers just worked and
> driver authors who didnt care need take no specific action.
Not only that, but as I wrote in my other mail, on some archs, the
system already got all the necessary triggers from the firmware and has
setup sane defaults, in which case it's more up to the driver to either
not care or adapt to what was set (some IRQ controllers don't support
all possible settings for example).
I would also expect PCI drivers to always pass default (rather than
explicitly level negative)
Ben.
On Mon, Nov 07, 2005 at 12:03:22AM +0000, Alan Cox wrote:
> On Sul, 2005-11-06 at 22:42 +0000, Russell King wrote:
> > We could do as you suggest, but my concern would be adding extra
> > complexity to drivers, causing them to do something like:
> >
> > ret = request_irq(..., SA_TRIGGER_HIGH, ...);
> > if (ret == -E<whatever>)
> > ret = request_irq(..., SA_TRIGGER_RISING, ...);
> >
> > The alternative is:
> >
> > ret = request_irq(..., SA_TRIGGER_HIGH | SA_TRIGGER_RISING, ...);
>
> I was thinking that specifying neither would imply 'don't care' or
> 'system default'. That would mean existing drivers just worked and
> driver authors who didnt care need take no specific action.
Yes, this is exactly what the ARM implementation already does. I'll
add a comment to that effect.
As per benh's suggestion, I don't see the point of adding a definition
- not unless we're going to fix up all drivers which call request_irq().
That would be a very big task.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
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 <[email protected]>
>
> ----- Forwarded message from Russell King <[email protected]> -----
> Date: Fri, 28 Oct 2005 22:57:47 +0100
> From: Russell King <[email protected]>
> To: Linux Kernel List <[email protected]>
> 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 ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Mon, Nov 07, 2005 at 12:42:20PM +0000, Ben Dooks wrote:
> 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 <[email protected]>
> >
> > ----- Forwarded message from Russell King <[email protected]> -----
> > Date: Fri, 28 Oct 2005 22:57:47 +0100
> > From: Russell King <[email protected]>
> > To: Linux Kernel List <[email protected]>
> > 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.
No. How can an edge triggered interrupt possibly cause a storm?
If the interrupt signal is at logic '0' and it was previously
configured for a low level to interrupt, when you call request_irq()
with your handler, the interrupt will be enabled.
Because it's level sensitive, it'll immediately interrupt, and
the handler will be invoked. When the handler returns, the interrupt
signal will still be at logic '0' so it's still active. So you
immediately get another interrupt and the handler will be re-invoked.
Repeat infinitely.
So it is exactly as I describe.
> How about making these compatible with the
> triggers compatible with the flags from
> include/linux/ioport.h definitions for the
> IRQ resource (IORESOURCE_IRQ_*).
We could do, but I took the set_irq_type() implementation. 8) We
could change both to conform.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
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 <[email protected]>
---
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.
+ */
+#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)
/*
* Real Time signals may be queued.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
Arrange for drivers which do:
request_irq(irq, ...);
set_irq_type(irq, ...);
or vice versa to use the new SA_TRIGGER flags with request_irq().
Signed-off-by: Russell King <[email protected]>
---
arch/arm/mach-omap1/serial.c | 3 +--
arch/arm/mach-pxa/corgi.c | 7 +++----
arch/arm/mach-pxa/poodle.c | 7 +++----
arch/arm/mach-pxa/spitz.c | 7 +++----
arch/arm/mach-s3c2410/usb-simtec.c | 6 +++---
drivers/i2c/chips/tps65010.c | 11 ++++++-----
drivers/input/keyboard/corgikbd.c | 6 ++----
drivers/input/keyboard/spitzkbd.c | 27 ++++++++++++++-------------
drivers/mfd/ucb1x00-core.c | 5 ++---
drivers/net/smc91x.c | 5 +----
drivers/net/smc91x.h | 18 +++++++++---------
11 files changed, 47 insertions(+), 55 deletions(-)
diff --git a/arch/arm/mach-omap1/serial.c b/arch/arm/mach-omap1/serial.c
--- a/arch/arm/mach-omap1/serial.c
+++ b/arch/arm/mach-omap1/serial.c
@@ -252,9 +252,8 @@ static void __init omap_serial_set_port_
return;
}
omap_set_gpio_direction(gpio_nr, 1);
- set_irq_type(OMAP_GPIO_IRQ(gpio_nr), IRQT_RISING);
ret = request_irq(OMAP_GPIO_IRQ(gpio_nr), &omap_serial_wake_interrupt,
- 0, "serial wakeup", NULL);
+ SA_TRIGGER_RISING, "serial wakeup", NULL);
if (ret) {
omap_free_gpio(gpio_nr);
printk(KERN_ERR "No interrupt for UART wake GPIO: %i\n",
diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
--- a/arch/arm/mach-pxa/corgi.c
+++ b/arch/arm/mach-pxa/corgi.c
@@ -213,15 +213,14 @@ static int corgi_mci_init(struct device
corgi_mci_platform_data.detect_delay = msecs_to_jiffies(250);
- err = request_irq(CORGI_IRQ_GPIO_nSD_DETECT, corgi_detect_int, SA_INTERRUPT,
- "MMC card detect", data);
+ err = request_irq(CORGI_IRQ_GPIO_nSD_DETECT, corgi_detect_int,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "MMC card detect", data);
if (err) {
printk(KERN_ERR "corgi_mci_init: MMC/SD: can't request MMC card detect IRQ\n");
return -1;
}
- set_irq_type(CORGI_IRQ_GPIO_nSD_DETECT, IRQT_BOTHEDGE);
-
return 0;
}
diff --git a/arch/arm/mach-pxa/poodle.c b/arch/arm/mach-pxa/poodle.c
--- a/arch/arm/mach-pxa/poodle.c
+++ b/arch/arm/mach-pxa/poodle.c
@@ -146,15 +146,14 @@ static int poodle_mci_init(struct device
poodle_mci_platform_data.detect_delay = msecs_to_jiffies(250);
- err = request_irq(POODLE_IRQ_GPIO_nSD_DETECT, poodle_detect_int, SA_INTERRUPT,
- "MMC card detect", data);
+ err = request_irq(POODLE_IRQ_GPIO_nSD_DETECT, poodle_detect_int,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "MMC card detect", data);
if (err) {
printk(KERN_ERR "poodle_mci_init: MMC/SD: can't request MMC card detect IRQ\n");
return -1;
}
- set_irq_type(POODLE_IRQ_GPIO_nSD_DETECT, IRQT_BOTHEDGE);
-
return 0;
}
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -293,15 +293,14 @@ static int spitz_mci_init(struct device
spitz_mci_platform_data.detect_delay = msecs_to_jiffies(250);
- err = request_irq(SPITZ_IRQ_GPIO_nSD_DETECT, spitz_detect_int, SA_INTERRUPT,
- "MMC card detect", data);
+ err = request_irq(SPITZ_IRQ_GPIO_nSD_DETECT, spitz_detect_int,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "MMC card detect", data);
if (err) {
printk(KERN_ERR "spitz_mci_init: MMC/SD: can't request MMC card detect IRQ\n");
return -1;
}
- set_irq_type(SPITZ_IRQ_GPIO_nSD_DETECT, IRQT_BOTHEDGE);
-
return 0;
}
diff --git a/arch/arm/mach-s3c2410/usb-simtec.c b/arch/arm/mach-s3c2410/usb-simtec.c
--- a/arch/arm/mach-s3c2410/usb-simtec.c
+++ b/arch/arm/mach-s3c2410/usb-simtec.c
@@ -84,13 +84,13 @@ static void usb_simtec_enableoc(struct s
int ret;
if (on) {
- ret = request_irq(IRQ_USBOC, usb_simtec_ocirq, SA_INTERRUPT,
+ ret = request_irq(IRQ_USBOC, usb_simtec_ocirq,
+ SA_INTERRUPT | SA_TRIGGER_RISING |
+ SA_TRIGGER_FALLING,
"USB Over-current", info);
if (ret != 0) {
printk(KERN_ERR "failed to request usb oc irq\n");
}
-
- set_irq_type(IRQ_USBOC, IRQT_BOTHEDGE);
} else {
free_irq(IRQ_USBOC, info);
}
diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
--- a/drivers/i2c/chips/tps65010.c
+++ b/drivers/i2c/chips/tps65010.c
@@ -494,6 +494,7 @@ tps65010_probe(struct i2c_adapter *bus,
{
struct tps65010 *tps;
int status;
+ unsigned long irqflags;
if (the_tps) {
dev_dbg(&bus->dev, "only one %s for now\n", DRIVER_NAME);
@@ -520,13 +521,14 @@ tps65010_probe(struct i2c_adapter *bus,
}
#ifdef CONFIG_ARM
+ irqflags = SA_SAMPLE_RANDOM | SA_TRIGGER_LOW;
if (machine_is_omap_h2()) {
tps->model = TPS65010;
omap_cfg_reg(W4_GPIO58);
tps->irq = OMAP_GPIO_IRQ(58);
omap_request_gpio(58);
omap_set_gpio_direction(58, 1);
- set_irq_type(tps->irq, IRQT_FALLING);
+ irqflags |= SA_TRIGGER_FALLING;
}
if (machine_is_omap_osk()) {
tps->model = TPS65010;
@@ -534,7 +536,7 @@ tps65010_probe(struct i2c_adapter *bus,
tps->irq = OMAP_GPIO_IRQ(OMAP_MPUIO(1));
omap_request_gpio(OMAP_MPUIO(1));
omap_set_gpio_direction(OMAP_MPUIO(1), 1);
- set_irq_type(tps->irq, IRQT_FALLING);
+ irqflags |= SA_TRIGGER_FALLING;
}
if (machine_is_omap_h3()) {
tps->model = TPS65013;
@@ -542,13 +544,12 @@ tps65010_probe(struct i2c_adapter *bus,
// FIXME set up this board's IRQ ...
}
#else
-#define set_irq_type(num,trigger) do{}while(0)
+ irqflags = SA_SAMPLE_RANDOM;
#endif
if (tps->irq > 0) {
- set_irq_type(tps->irq, IRQT_LOW);
status = request_irq(tps->irq, tps65010_irq,
- SA_SAMPLE_RANDOM, DRIVER_NAME, tps);
+ irqflags, DRIVER_NAME, tps);
if (status < 0) {
dev_dbg(&tps->client.dev, "can't get IRQ %d, err %d\n",
tps->irq, status);
diff --git a/drivers/input/keyboard/corgikbd.c b/drivers/input/keyboard/corgikbd.c
--- a/drivers/input/keyboard/corgikbd.c
+++ b/drivers/input/keyboard/corgikbd.c
@@ -19,7 +19,6 @@
#include <linux/jiffies.h>
#include <linux/module.h>
#include <linux/slab.h>
-#include <asm/irq.h>
#include <asm/arch/corgi.h>
#include <asm/arch/hardware.h>
@@ -343,10 +342,9 @@ static int __init corgikbd_probe(struct
for (i = 0; i < CORGI_KEY_SENSE_NUM; i++) {
pxa_gpio_mode(CORGI_GPIO_KEY_SENSE(i) | GPIO_IN);
if (request_irq(CORGI_IRQ_GPIO_KEY_SENSE(i), corgikbd_interrupt,
- SA_INTERRUPT, "corgikbd", corgikbd))
+ SA_INTERRUPT | SA_TRIGGER_RISING,
+ "corgikbd", corgikbd))
printk(KERN_WARNING "corgikbd: Can't get IRQ: %d!\n", i);
- else
- set_irq_type(CORGI_IRQ_GPIO_KEY_SENSE(i),IRQT_RISING);
}
/* Set Strobe lines as outputs - set high */
diff --git a/drivers/input/keyboard/spitzkbd.c b/drivers/input/keyboard/spitzkbd.c
--- a/drivers/input/keyboard/spitzkbd.c
+++ b/drivers/input/keyboard/spitzkbd.c
@@ -19,7 +19,6 @@
#include <linux/jiffies.h>
#include <linux/module.h>
#include <linux/slab.h>
-#include <asm/irq.h>
#include <asm/arch/spitz.h>
#include <asm/arch/hardware.h>
@@ -407,10 +406,9 @@ static int __init spitzkbd_probe(struct
for (i = 0; i < SPITZ_KEY_SENSE_NUM; i++) {
pxa_gpio_mode(spitz_senses[i] | GPIO_IN);
if (request_irq(IRQ_GPIO(spitz_senses[i]), spitzkbd_interrupt,
- SA_INTERRUPT, "Spitzkbd Sense", spitzkbd))
+ SA_INTERRUPT|SA_TRIGGER_RISING,
+ "Spitzkbd Sense", spitzkbd))
printk(KERN_WARNING "spitzkbd: Can't get Sense IRQ: %d!\n", i);
- else
- set_irq_type(IRQ_GPIO(spitz_senses[i]),IRQT_RISING);
}
/* Set Strobe lines as outputs - set high */
@@ -422,15 +420,18 @@ static int __init spitzkbd_probe(struct
pxa_gpio_mode(SPITZ_GPIO_SWA | GPIO_IN);
pxa_gpio_mode(SPITZ_GPIO_SWB | GPIO_IN);
- request_irq(SPITZ_IRQ_GPIO_SYNC, spitzkbd_interrupt, SA_INTERRUPT, "Spitzkbd Sync", spitzkbd);
- request_irq(SPITZ_IRQ_GPIO_ON_KEY, spitzkbd_interrupt, SA_INTERRUPT, "Spitzkbd PwrOn", spitzkbd);
- request_irq(SPITZ_IRQ_GPIO_SWA, spitzkbd_hinge_isr, SA_INTERRUPT, "Spitzkbd SWA", spitzkbd);
- request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr, SA_INTERRUPT, "Spitzkbd SWB", spitzkbd);
-
- set_irq_type(SPITZ_IRQ_GPIO_SYNC, IRQT_BOTHEDGE);
- set_irq_type(SPITZ_IRQ_GPIO_ON_KEY, IRQT_BOTHEDGE);
- set_irq_type(SPITZ_IRQ_GPIO_SWA, IRQT_BOTHEDGE);
- set_irq_type(SPITZ_IRQ_GPIO_SWB, IRQT_BOTHEDGE);
+ request_irq(SPITZ_IRQ_GPIO_SYNC, spitzkbd_interrupt,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "Spitzkbd Sync", spitzkbd);
+ request_irq(SPITZ_IRQ_GPIO_ON_KEY, spitzkbd_interrupt,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "Spitzkbd PwrOn", spitzkbd);
+ request_irq(SPITZ_IRQ_GPIO_SWA, spitzkbd_hinge_isr,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "Spitzkbd SWA", spitzkbd);
+ request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr,
+ SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING,
+ "Spitzkbd SWB", spitzkbd);
printk(KERN_INFO "input: Spitz Keyboard Registered\n");
diff --git a/drivers/mfd/ucb1x00-core.c b/drivers/mfd/ucb1x00-core.c
--- a/drivers/mfd/ucb1x00-core.c
+++ b/drivers/mfd/ucb1x00-core.c
@@ -27,7 +27,6 @@
#include <asm/dma.h>
#include <asm/hardware.h>
-#include <asm/irq.h>
#include "ucb1x00.h"
@@ -507,14 +506,14 @@ static int ucb1x00_probe(struct mcp *mcp
goto err_free;
}
- ret = request_irq(ucb->irq, ucb1x00_irq, 0, "UCB1x00", ucb);
+ ret = request_irq(ucb->irq, ucb1x00_irq, SA_TRIGGER_RISING,
+ "UCB1x00", ucb);
if (ret) {
printk(KERN_ERR "ucb1x00: unable to grab irq%d: %d\n",
ucb->irq, ret);
goto err_free;
}
- set_irq_type(ucb->irq, IRQT_RISING);
mcp_set_drvdata(mcp, ucb);
ret = class_device_register(&ucb->cdev);
diff --git a/drivers/net/smc91x.c b/drivers/net/smc91x.c
--- a/drivers/net/smc91x.c
+++ b/drivers/net/smc91x.c
@@ -88,7 +88,6 @@ static const char version[] =
#include <linux/skbuff.h>
#include <asm/io.h>
-#include <asm/irq.h>
#include "smc91x.h"
@@ -2007,12 +2006,10 @@ static int __init smc_probe(struct net_d
}
/* Grab the IRQ */
- retval = request_irq(dev->irq, &smc_interrupt, 0, dev->name, dev);
+ retval = request_irq(dev->irq, &smc_interrupt, SMC_IRQ_FLAGS, dev->name, dev);
if (retval)
goto err_out;
- set_irq_type(dev->irq, SMC_IRQ_TRIGGER_TYPE);
-
#ifdef SMC_USE_PXA_DMA
{
int dma = pxa_request_dma(dev->name, DMA_PRIO_LOW,
diff --git a/drivers/net/smc91x.h b/drivers/net/smc91x.h
--- a/drivers/net/smc91x.h
+++ b/drivers/net/smc91x.h
@@ -90,7 +90,7 @@
__l--; \
} \
} while (0)
-#define set_irq_type(irq, type)
+#define SMC_IRQ_FLAGS (0)
#elif defined(CONFIG_SA1100_PLEB)
/* We can only do 16-bit reads and writes in the static memory space. */
@@ -109,7 +109,7 @@
#define SMC_outw(v, a, r) writew(v, (a) + (r))
#define SMC_outsw(a, r, p, l) writesw((a) + (r), p, l)
-#define set_irq_type(irq, type) do {} while (0)
+#define SMC_IRQ_FLAGS (0)
#elif defined(CONFIG_SA1100_ASSABET)
@@ -185,11 +185,11 @@ SMC_outw(u16 val, void __iomem *ioaddr,
#include <asm/mach-types.h>
#include <asm/arch/cpu.h>
-#define SMC_IRQ_TRIGGER_TYPE (( \
+#define SMC_IRQ_FLAGS (( \
machine_is_omap_h2() \
|| machine_is_omap_h3() \
|| (machine_is_omap_innovator() && !cpu_is_omap1510()) \
- ) ? IRQT_FALLING : IRQT_RISING)
+ ) ? SA_TRIGGER_FALLING : SA_TRIGGER_RISING)
#elif defined(CONFIG_SH_SH4202_MICRODEV)
@@ -209,7 +209,7 @@ SMC_outw(u16 val, void __iomem *ioaddr,
#define SMC_insw(a, r, p, l) insw((a) + (r) - 0xa0000000, p, l)
#define SMC_outsw(a, r, p, l) outsw((a) + (r) - 0xa0000000, p, l)
-#define set_irq_type(irq, type) do {} while(0)
+#define SMC_IRQ_FLAGS (0)
#elif defined(CONFIG_ISA)
@@ -237,7 +237,7 @@ SMC_outw(u16 val, void __iomem *ioaddr,
#define SMC_insw(a, r, p, l) insw(((u32)a) + (r), p, l)
#define SMC_outsw(a, r, p, l) outsw(((u32)a) + (r), p, l)
-#define set_irq_type(irq, type) do {} while(0)
+#define SMC_IRQ_FLAGS (0)
#define RPC_LSA_DEFAULT RPC_LED_TX_RX
#define RPC_LSB_DEFAULT RPC_LED_100_10
@@ -319,7 +319,7 @@ static inline void SMC_outsw (unsigned l
au_writew(*_p++ , _a); \
} while(0)
-#define set_irq_type(irq, type) do {} while (0)
+#define SMC_IRQ_FLAGS (0)
#else
@@ -342,8 +342,8 @@ static inline void SMC_outsw (unsigned l
#endif
-#ifndef SMC_IRQ_TRIGGER_TYPE
-#define SMC_IRQ_TRIGGER_TYPE IRQT_RISING
+#ifndef SMC_IRQ_FLAGS
+#define SMC_IRQ_FLAGS SA_TRIGGER_RISING
#endif
#ifdef SMC_USE_PXA_DMA
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
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 <[email protected]>
> ---
>
> 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
Hi Russell,
On Mon, 12 Dec 2005, Russell King wrote:
> * Move SA_TRIGGER into linux/signal.h
...
> @@ -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;
Hmm, where is SA_TRIGGER defined?
> +#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)
Thanks
On Tue, Dec 13, 2005 at 08:49:50AM -0600, Kumar Gala wrote:
> >+ * These correspond with the SA_TRIGGER_* defines, and therefore the
> >+ * IRQRESOURCE_IRQ_* defines.
>
> comment nit. Should be IORESOURCE_IRQ_* not IRQRESOURCE_IRQ_*
Added, thanks.
> >+/*
> >+ * 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.
> >+ */
>
> 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.
Also added. Thanks.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core