Skip to contentSkip to conversation

Interrupt chip consolidation

RFC PATCH In review Thomas Gleixner 9 participants 46 messages Apr 16, 2011 – Apr 25, 2011

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support
lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

cover Interrupt chip consolidation

A search for patterns of interrupt chips and their implementations came up with at least two very common variants:

- mask register based, where a bit flips the masked state of the corresponding interrupt line. Most common is to unmask with the bit set, but the inverted version exists as well. Interestingly enough almost none of the chips implementations cache the register value and read it back from the chip before every write.

- clear/set register based, where a bit written to one of the registers either clears or sets the interrupt mask.

There are more patterns, but for now I restricted the patches on those two.

The core code provides now a generic irq chip mechanism which dynamically allocates the data structures. The chip assumes that the interrupt hardware is using 32bit wide registers. For each interrupt bank (32 irqs) a separate chip is allocated and configured. If possible the generic callbacks for mask/unmask/ack/eoi are used. There is also a generic version for irq_set_wake, which just caches the wakeup sources in the chip data structure to replace home brewn caching mechanisms.

The core code has new callbacks irq_suspend/irq_resume as well, which can be used to replace the sys device implementations for the various irq chips.

I converted and cleaned up a bunch of irq chip implementations in arch/arm to see whether this works out as intended.

Note, that this is mostly compile tested only, but I'd like to get some feedback on that before going further. There is more room for consolidation (especially a bunch of slightly different VIC copies poked my eyes).

The core code adds about 500 lines, but it's massivly commented. The binary size with all functionality compiled in is about 1500 bytes which could be reduced with some ifdeffery, but it's probably not worth the mess.

The diffstat for the arm part is:

 arch/arm/mach-msm/irq.c                                      |  151 ----
 linux-2.6/arch/arm/common/sa1111.c                           |  193 ------
 linux-2.6/arch/arm/common/vic.c                              |  262 +-------
 linux-2.6/arch/arm/mach-bcmring/irq.c                        |   98 ---
 linux-2.6/arch/arm/mach-davinci/irq.c                        |   90 --
 linux-2.6/arch/arm/mach-msm/Kconfig                          |    1 
 linux-2.6/arch/arm/mach-msm/Makefile                         |    2 
 linux-2.6/arch/arm/mach-msm/include/mach/irqs.h              |    3 
 linux-2.6/arch/arm/mach-msm/include/mach/sirc.h              |   43 -
 linux-2.6/arch/arm/mach-msm/irq-vic.c                        |  344 +----------
 linux-2.6/arch/arm/mach-msm/sirc.c                           |  172 +----
 linux-2.6/arch/arm/mach-pxa/irq.c                            |  192 ++----
 linux-2.6/arch/arm/mach-s3c64xx/irq.c                        |    7 
 linux-2.6/arch/arm/mach-tcc8k/irq.c                          |   83 --
 linux-2.6/arch/arm/plat-orion/gpio.c                         |  110 +--
 linux-2.6/arch/arm/plat-orion/include/plat/gpio.h            |    1 
 linux-2.6/arch/arm/plat-orion/irq.c                          |   49 -
 linux-2.6/arch/arm/plat-s5p/irq-gpioint.c                    |  115 ---
 linux-2.6/arch/arm/plat-s5p/irq.c                            |    6 
 linux-2.6/arch/arm/plat-samsung/include/plat/irq-vic-timer.h |    2 
 linux-2.6/arch/arm/plat-samsung/irq-uart.c                   |   82 --
 linux-2.6/arch/arm/plat-samsung/irq-vic-timer.c              |   73 --
 22 files changed, 455 insertions(+), 1624 deletions(-)

That converts 18 irq_chips and removes at least one duplicated VIC implementation.

Please have a look and check also the not yet converted chips whether they could make use of such an generic implementation. We better have some oddball callback functions in the generic code than ten slightly different instances of them all over the place.

Thanks,

tglx

#2 lore raw

genirq: Implement a generic interrupt chip

Quoted 15 lines · Thomas Gleixner

Implement a generic interrupt chip, which is configurable and is able to handle the most common irq chip implementations.

Signed-off-by Thomas Gleixner <[email protected]>
---
 include/linux/irq.h       |  123 ++++++++++++++++++++++
 kernel/irq/Makefile       |    1 
 kernel/irq/generic-chip.c |  251 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)

Index: linux-2.6/include/linux/irq.h
===================================================================

include/linux/irq.h +0 -0

[snip]

Quoted 18 lines
include/linux/irq.h +18 -0
+/**
+ * struct irq_chip_type - Generic interrupt chip instance for a flow type
+ * @chip:		The real interrupt chip which provides the callbacks
+ * @regs:		Register offsets for this chip
+ * @handler:		Flow handler associated with this chip
+ * @type:		Chip can handle these flow types
+ *
+ * A irq_generic_chip can have several instances of irq_chip_type when
+ * it requires different functions and register offsets for different
+ * flow types.
+ */
+struct irq_chip_type {
+	struct irq_chip		chip;
+	struct irq_chip_regs	regs;
+	irq_flow_handler_t	handler;
+	u32			type;
+};
+

[snip]

Quoted 4 lines

Index: linux-2.6/kernel/irq/generic-chip.c
===================================================================

kernel/irq/generic-chip.c new file +0 -0

[snip]

Quoted 10 lines
kernel/irq/generic-chip.c new file +10 -0
+/**
+ * irq_setup_generic_chip - Setup a range of interrupts with a generic chip
+ * @gc:		Generic irq chip holding all data
+ * @msk:	Bitmask holding the irqs to initialize relative to gc->irq_base
+ * @clr:	IRQ_* bits to clear
+ * @set:	IRQ_* bits to set
+ *
+ * Set up max. 32 interrupts starting from gc->irq_base. Note, this
+ * initializes all interrupts to the primary irq_chip_type and its
+ * associated handler.

kernel/irq/generic-chip.c

Quoted 10 lines
kernel/irq/generic-chip.c new file +10 -0
+ */
+void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
+			    unsigned int clr, unsigned int set)
+{
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned int i;
+
+	/* Init mask cache */
+	if (ct->irq_mask == irq_gc_mask_clr_bit ||
+	    ct->irq_mask == irq_gc_mask_set_bit)

struct irq_chip_type does not have a member named 'irq_mask'

Regards,
Hartley

author
#3 lore raw

genirq: Implement a generic interrupt chip

Quoted 12 lines · H Hartley Sweeten

Review

Quoted 9 lines · Thomas Gleixner
(quoted diff) +9 -0
+void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
+			    unsigned int clr, unsigned int set)
+{
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned int i;
+
+	/* Init mask cache */
+	if (ct->irq_mask == irq_gc_mask_clr_bit ||
+	    ct->irq_mask == irq_gc_mask_set_bit)

struct irq_chip_type does not have a member named 'irq_mask'

Found that already. Needs to be ct->chip_types->irq_mask of course.

Someday I hope that the patch to quilt which forces a refresh of the queue _BEFORE_ generating mail will surface :)

Thanks,

tglx

author
#4 lore raw

genirq: Implement a generic interrupt chip

Quoted 14 lines · Thomas Gleixner
Quoted 12 lines · H Hartley Sweeten

Review

Quoted 9 lines · Thomas Gleixner
(quoted diff) +9 -0
+void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
+			    unsigned int clr, unsigned int set)
+{
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned int i;
+
+	/* Init mask cache */
+	if (ct->irq_mask == irq_gc_mask_clr_bit ||
+	    ct->irq_mask == irq_gc_mask_set_bit)

struct irq_chip_type does not have a member named 'irq_mask'

Quoted 1 line

Found that already. Needs to be ct->chip_types->irq_mask of course.

Gah, my brain is seriously damaged from staring into that code. ct->chip.irq_mask of course.

Quoted 7 lines

Someday I hope that the patch to quilt which forces a refresh of the queue _BEFORE_ generating mail will surface :)

Thanks,

tglx

Tested-by
#5 lore raw

genirq: Implement a generic interrupt chip

Quoted 16 lines · Thomas Gleixner
Quoted 14 lines · Thomas Gleixner
Quoted 12 lines · H Hartley Sweeten

Review

Quoted 9 lines · Thomas Gleixner
(quoted diff) +9 -0
+void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
+			    unsigned int clr, unsigned int set)
+{
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned int i;
+
+	/* Init mask cache */
+	if (ct->irq_mask == irq_gc_mask_clr_bit ||
+	    ct->irq_mask == irq_gc_mask_set_bit)

struct irq_chip_type does not have a member named 'irq_mask'

Quoted 4 lines
Quoted 1 line

Found that already. Needs to be ct->chip_types->irq_mask of course.

Gah, my brain is seriously damaged from staring into that code. ct->chip.irq_mask of course.

OK. With that fix, I compiled and boot tested these patches on an ep93xx based board:

genirq: Implement a generic interrupt chip
vic: Use generic interrupt chip

I do not have CONFIG_PM enabled so I'm not sure about the vic_id++ issue I pointed out earlier in vic: Use generic interrupt chip.

Other than that, for these two patches you can add (if you wish):

Tested-by H Hartley Sweeten <[email protected]>
maintainer
#6 lore raw

genirq: Implement a generic interrupt chip

In reply to Thomas Gleixner
Quoted 2 lines · Thomas Gleixner
include/linux/irq.h +0 -0

...

Quoted 7 lines

+/**
+ * struct irq_chip_regs - register offsets for struct irq_gci
+ * @enable: Enable register offset to reg_base
+ * @disable: Disable register offset to reg_base
+ * @mask: Mask register offset to reg_base
+ * @ack: Ack register offset to reg_base
+ * @ack: Secondary Ack register offset to reg_base

Two @ack lines here?

Tony

maintainer Tested-by
#7 lore raw

genirq: Implement a generic interrupt chip

Quoted 13 lines · Tony Lindgren
Quoted 2 lines · Thomas Gleixner
include/linux/irq.h +0 -0

...

Quoted 7 lines

+/**
+ * struct irq_chip_regs - register offsets for struct irq_gci
+ * @enable: Enable register offset to reg_base
+ * @disable: Disable register offset to reg_base
+ * @mask: Mask register offset to reg_base
+ * @ack: Ack register offset to reg_base
+ * @ack: Secondary Ack register offset to reg_base

Quoted 1 line

Two @ack lines here?

Other than that seems to work for omap2 & 3. Posted a patch for you too.

Tested-by Tony Lindgren <[email protected]>
#8 lore raw

genirq: Implement a generic interrupt chip

In reply to Thomas Gleixner
Quoted 131 lines · Thomas Gleixner

Implement a generic interrupt chip, which is configurable and is able to handle the most common irq chip implementations.

Signed-off-by Thomas Gleixner<[email protected]>
---
  include/linux/irq.h       |  123 ++++++++++++++++++++++
  kernel/irq/Makefile       |    1
  kernel/irq/generic-chip.c |  251 ++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 375 insertions(+)

Index: linux-2.6/include/linux/irq.h
===================================================================

include/linux/irq.h +113 -0
@@ -573,6 +573,129 @@ static inline int irq_reserve_irq(unsign
  	return irq_reserve_irqs(irq, 1);
  }
 
+#ifndef irq_reg_writel
+# define irq_reg_writel(val, addr)	writel(val, addr)
+#endif
+#ifndef irq_reg_readl
+# define irq_reg_readl(addr)		readl(addr)
+#endif
+
+/**
+ * struct irq_chip_regs - register offsets for struct irq_gci
+ * @enable:	Enable register offset to reg_base
+ * @disable:	Disable register offset to reg_base
+ * @mask:	Mask register offset to reg_base
+ * @ack:	Ack register offset to reg_base
+ * @ack:	Secondary Ack register offset to reg_base
+ * @eoi:	Eoi register offset to reg_base
+ * @type:	Type configuration register offset to reg_base
+ * @polarity:	Polarity configuration register offset to reg_base
+ */
+struct irq_chip_regs {
+	unsigned long		enable;
+	unsigned long		disable;
+	unsigned long		mask;
+	unsigned long		ack;
+	unsigned long		eoi;
+	unsigned long		type;
+	unsigned long		polarity;
+};
+
+/**
+ * struct irq_chip_type - Generic interrupt chip instance for a flow type
+ * @chip:		The real interrupt chip which provides the callbacks
+ * @regs:		Register offsets for this chip
+ * @handler:		Flow handler associated with this chip
+ * @type:		Chip can handle these flow types
+ *
+ * A irq_generic_chip can have several instances of irq_chip_type when
+ * it requires different functions and register offsets for different
+ * flow types.
+ */
+struct irq_chip_type {
+	struct irq_chip		chip;
+	struct irq_chip_regs	regs;
+	irq_flow_handler_t	handler;
+	u32			type;
+};
+
+/**
+ * struct irq_chip_generic - Generic irq chip data structure
+ * @lock:		Lock to protect register and cache data access
+ * @reg_base:		Register base address (virtual)
+ * @irq_base:		Interrupt base nr for this chip
+ * @irq_cnt:		Number of interrupts handled by this chip
+ * @mask_cache:		Cached mask register
+ * @type_cache:		Cached type register
+ * @polarity_cache:	Cached polarity register
+ * @wake_enabled:	Interrupt can wakeup from suspend
+ * @wake_active:	Interrupt is marked as an wakeup from suspend source
+ * @num_ct:		Number of available irq_chip_type instances (usually 1)
+ * @private:		Private data for non generic chip callbacks
+ * @chip_types:		Array of interrupt irq_chip_types
+ *
+ * Note, that irq_chip_generic can have multiple irq_chip_type
+ * implementations which can be associated to a particular irq line of
+ * an irq_chip_generic instance. That allows to share and protect
+ * state in an irq_chip_generic instance when we need to implement
+ * different flow mechanisms (level/edge) for it.
+ */
+struct irq_chip_generic {
+	raw_spinlock_t		lock;
+	void __iomem		*reg_base;
+	unsigned int		irq_base;
+	unsigned int		irq_cnt;
+	u32			mask_cache;
+	u32			type_cache;
+	u32			polarity_cache;
+	u32			wake_enabled;
+	u32			wake_active;
+	unsigned int		num_ct;
+	void			*private;
+	struct irq_chip_type	chip_types[0];
+};
+
+/* Generic chip callback functions */
+void irq_gc_noop(struct irq_data *d);
+void irq_gc_mask_disable_reg(struct irq_data *d);
+void irq_gc_mask_set_bit(struct irq_data *d);
+void irq_gc_mask_clr_bit(struct irq_data *d);
+void irq_gc_unmask_enable_reg(struct irq_data *d);
+void irq_gc_ack(struct irq_data *d);
+void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
+void irq_gc_eoi(struct irq_data *d);
+int irq_gc_set_wake(struct irq_data *d, unsigned int on);
+
+/* Setup functions for irq_chip_generic */
+struct irq_chip_generic *
+irq_alloc_generic_chip(const char *name, int nr_ct, unsigned int irq_base,
+		       void __iomem *reg_base, irq_flow_handler_t handler);
+void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
+			    unsigned int clr, unsigned int set);
+int irq_setup_alt_chip(struct irq_data *d, unsigned int type);
+
+static inline struct irq_chip_type *irq_data_get_chip_type(struct irq_data *d)
+{
+	return container_of(d->chip, struct irq_chip_type, chip);
+}
+
+#define IRQ_MSK(n) (u32)((n)<  32 ? ((1<<  (n)) - 1) : UINT_MAX)
+
+#ifdef CONFIG_SMP
+static inline void irq_gc_lock(struct irq_chip_generic *gc)
+{
+	raw_spin_lock(&gc->lock);
+}

include/linux/irq.h static inline int irq_reserve_irq(unsign · line 698

Quoted 10 lines
include/linux/irq.h +10 -0
+
+static inline void irq_gc_unlock(struct irq_chip_generic *gc)
+{
+	raw_spin_unlock(&gc->lock);
+}
+#else
+static inline void irq_gc_lock(struct irq_chip_generic *gc) { }
+static inline void irq_gc_unlock(struct irq_chip_generic *gc) { }
+#endif
+

Shall we create a new header file for this, lets say geneirc_irq_chip.h? The generic irq chip api is different from what irq.h provides, I think

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#9 lore raw

genirq: Implement a generic interrupt chip

Quoted 9 lines · Abhijeet Dharmapurikar

Review

Quoted 5 lines · Thomas Gleixner
(quoted diff) +5 -0
+#else
+static inline void irq_gc_lock(struct irq_chip_generic *gc) { }
+static inline void irq_gc_unlock(struct irq_chip_generic *gc) { }
+#endif
+

Shall we create a new header file for this, lets say geneirc_irq_chip.h? The generic irq chip api is different from what irq.h provides, I think

And that header includes irq.h then. Where is the point ?

#12 lore raw

vic: Use generic interrupt chip

Quoted 15 lines · Thomas Gleixner

The VIC chips can be handled by the generic implementation. The implementation uses always uses handle_level_irq. We can replace the ack and mask functions by a combined mask_ack function which also gets rid of the double write to VIC_INT_ENABLE_CLEAR.

Signed-off-by Thomas Gleixner <[email protected]>
---
 arch/arm/common/vic.c |  152 ++++++++++++--------------------------------------
 1 file changed, 39 insertions(+), 113 deletions(-)

Index: linux-2.6/arch/arm/common/vic.c
===================================================================

arch/arm/common/vic.c +0 -0

[snip]

Quoted 19 lines
arch/arm/common/vic.c +2 -4
@@ -178,15 +172,13 @@ late_initcall(vic_pm_init);
 
 /**
  * vic_pm_register - Register a VIC for later power management control
- * @base: The base address of the VIC.
- * @irq: The base IRQ for the VIC.
- * @resume_sources: bitmask of interrupts allowed for resume sources.
+ * @gc: The generic interrupt chip for the VIC
  *
  * Register the VIC with the system device tree so that it can be notified
  * of suspend and resume requests and ensure that the correct actions are
  * taken to re-instate the settings on resume.
  */
-static void __init vic_pm_register(void __iomem *base, unsigned int irq, u32 resume_sources)
+static void __init vic_pm_register(struct irq_chip_generic *gc)
 {
 	struct vic_device *v;

arch/arm/common/vic.c static void __init vic_pm_register(void · line 191

Quoted 10 lines
arch/arm/common/vic.c +1 -4
@@ -194,85 +186,13 @@ static void __init vic_pm_register(void 
 		printk(KERN_ERR "%s: too few VICs, increase CONFIG_ARM_VIC_NR\n", __func__);
 	else {
 		v = &vic_devices[vic_id];
-		v->base = base;
-		v->resume_sources = resume_sources;
-		v->irq = irq;
-		vic_id++;
+		v->gc = gc;
 	}
 }

I think the vic_id++ is still needed here in order to handle multiple VIC devices.

Regards,
Hartley

#15 lore raw

arm: orion: Use generic irq chip

Quoted 129 lines · Thomas Gleixner

The core interrupt chip is a straight forward conversion. The gpio chip is implemented with two instances of the irq_chip_type which can be switched with the irq_set_type function. That allows us to use the generic callbacks and avoids the conditionals in them.

Signed-off-by Thomas Gleixner<[email protected]>
---
  arch/arm/plat-orion/gpio.c              |  110 ++++++++++----------------------
  arch/arm/plat-orion/include/plat/gpio.h |    1
  arch/arm/plat-orion/irq.c               |   49 ++------------
  3 files changed, 44 insertions(+), 116 deletions(-)

Index: linux-2.6/arch/arm/plat-orion/gpio.c
===================================================================

arch/arm/plat-orion/gpio.c +13 -67
@@ -321,59 +321,16 @@ EXPORT_SYMBOL(orion_gpio_set_blink);
   *        polarity    LEVEL          mask
   *
   ****************************************************************************/
-static void gpio_irq_ack(struct irq_data *d)
-{
-	struct orion_gpio_chip *ochip = irq_data_get_irq_chip_data(d);
-	int type = irqd_get_trigger_type(d);
-
-	if (type&  (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) {
-		int pin = d->irq - ochip->secondary_irq_base;
-
-		writel(~(1<<  pin), GPIO_EDGE_CAUSE(ochip));
-	}
-}
-
-static void gpio_irq_mask(struct irq_data *d)
-{
-	struct orion_gpio_chip *ochip = irq_data_get_irq_chip_data(d);
-	int type = irqd_get_trigger_type(d);
-	void __iomem *reg;
-	int pin;
-
-	if (type&  (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
-		reg = GPIO_EDGE_MASK(ochip);
-	else
-		reg = GPIO_LEVEL_MASK(ochip);
-
-	pin = d->irq - ochip->secondary_irq_base;
-
-	writel(readl(reg)&  ~(1<<  pin), reg);
-}
-
-static void gpio_irq_unmask(struct irq_data *d)
-{
-	struct orion_gpio_chip *ochip = irq_data_get_irq_chip_data(d);
-	int type = irqd_get_trigger_type(d);
-	void __iomem *reg;
-	int pin;
-
-	if (type&  (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
-		reg = GPIO_EDGE_MASK(ochip);
-	else
-		reg = GPIO_LEVEL_MASK(ochip);
-
-	pin = d->irq - ochip->secondary_irq_base;
-
-	writel(readl(reg) | (1<<  pin), reg);
-}
 
  static int gpio_irq_set_type(struct irq_data *d, u32 type)
  {
-	struct orion_gpio_chip *ochip = irq_data_get_irq_chip_data(d);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct orion_gpio_chip *ochip = gc->private;
  	int pin;
  	u32 u;
 
-	pin = d->irq - ochip->secondary_irq_base;
+	pin = d->irq - gc->irq_base;
 
  	u = readl(GPIO_IO_CONF(ochip))&  (1<<  pin);
  	if (!u) {

@@ -382,18 +339,14 @@ static int gpio_irq_set_type(struct irq_
  		return -EINVAL;
  	}
 
-	/*
-	 * Set edge/level type.
-	 */
-	if (type&  (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) {
-		__irq_set_handler_locked(d->irq, handle_edge_irq);
-	} else if (type&  (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
-		__irq_set_handler_locked(d->irq, handle_level_irq);
-	} else {
-		printk(KERN_ERR "failed to set irq=%d (type=%d)\n",
-		       d->irq, type);
+	type&= IRQ_TYPE_SENSE_MASK;
+	if (type == IRQ_TYPE_NONE)
  		return -EINVAL;
-	}
+
+	/* Check if we need to change chip and handler */
+	if (!(ct->type&  type))
+		if (irq_setup_alt_chip(d, type))
+			return -EINVAL;
 
  	/*
  	 * Configure interrupt polarity.

@@ -425,19 +378,12 @@ static int gpio_irq_set_type(struct irq_
  	return 0;
  }
 
-struct irq_chip orion_gpio_irq_chip = {
-	.name		= "orion_gpio_irq",
-	.irq_ack	= gpio_irq_ack,
-	.irq_mask	= gpio_irq_mask,
-	.irq_unmask	= gpio_irq_unmask,
-	.irq_set_type	= gpio_irq_set_type,
-};
-
  void __init orion_gpio_init(int gpio_base, int ngpio,
  			    u32 base, int mask_offset, int secondary_irq_base)
  {
  	struct orion_gpio_chip *ochip;
-	int i;
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
 
  	if (orion_gpio_chip_count == ARRAY_SIZE(orion_gpio_chips))
  		return;

arch/arm/plat-orion/gpio.c void __init orion_gpio_init(int gpio_bas · line 421

Quoted 7 lines
arch/arm/plat-orion/gpio.c +2 -2
@@ -471,15 +417,29 @@ void __init orion_gpio_init(int gpio_bas
  	writel(0, GPIO_EDGE_MASK(ochip));
  	writel(0, GPIO_LEVEL_MASK(ochip));
 
-	for (i = 0; i<  ngpio; i++) {
-		unsigned int irq = secondary_irq_base + i;
+	gc = irq_alloc_generic_chip("orion_gpio_irq", 1, secondary_irq_base,
+				    ochip->base, handle_level_irq);

should this be 2 instead of 1 ?

	gc = irq_alloc_generic_chip("orion_gpio_irq", 2,  ...
Quoted 28 lines
arch/arm/plat-orion/gpio.c +20 -6
+	gc->private = ochip;
+
+	ct = gc->chip_types;
+	ct->regs.mask = ochip->mask_offset + GPIO_LEVEL_MASK_OFF;
+	ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
+	ct->chip.irq_mask = irq_gc_mask_clr_bit;
+	ct->chip.irq_unmask = irq_gc_mask_set_bit;
+	ct->chip.irq_set_type = gpio_irq_set_type;
+
+	ct++;
+	ct->regs.mask = ochip->mask_offset + GPIO_EDGE_MASK_OFF;
+	ct->regs.ack = GPIO_EDGE_CAUSE_OFF;
+	ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+	ct->chip.irq_ack = irq_gc_ack;
+	ct->chip.irq_mask = irq_gc_mask_clr_bit;
+	ct->chip.irq_unmask = irq_gc_mask_set_bit;
+	ct->chip.irq_set_type = gpio_irq_set_type;
+	ct->handler = handle_edge_irq;
 
-		irq_set_chip_and_handler(irq,&orion_gpio_irq_chip,
-					 handle_level_irq);
-		irq_set_chip_data(irq, ochip);
-		irq_set_status_flags(irq, IRQ_LEVEL);
-		set_irq_flags(irq, IRQF_VALID);
-	}
+	irq_setup_generic_chip(gc, IRQ_MSK(ngpio), IRQ_NOREQUEST,
+			       IRQ_LEVEL | IRQ_NOPROBE);
  }
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#16 lore raw

arm: orion: Use generic irq chip

Quoted 7 lines · Abhijeet Dharmapurikar

Review

Quoted 4 lines
(quoted diff) +2 -2
-	for (i = 0; i<  ngpio; i++) {
-		unsigned int irq = secondary_irq_base + i;
+	gc = irq_alloc_generic_chip("orion_gpio_irq", 1, secondary_irq_base,
+				    ochip->base, handle_level_irq);

should this be 2 instead of 1 ?

	gc = irq_alloc_generic_chip("orion_gpio_irq", 2,  ...

Ouch, yes.

Tested-by
#20 lore raw

davinci: Use generic irq chip

In reply to Thomas Gleixner

Hi Thomas,

Quoted 4 lines · Thomas Gleixner

Simple conversion which simply uses the fact that the second irq chip base address has offset 0x04 to the first one.

Signed-off-by Thomas Gleixner <[email protected]>

Boot tested it on a DM365 EVM.

Tested-by Sekhar Nori <[email protected]>
Quoted 4 lines
(quoted diff) +0 -0
@@ -152,13 +103,8 @@ void __init davinci_irq_init(void)
 		davinci_irq_writel(pri, i);
 	}

Review void __init davinci_irq_init(void) · line 106

Quoted 10 lines
(quoted diff) +1 -9
-	/* set up genirq dispatch for ARM INTC */
-	for (i = 0; i < davinci_soc_info.intc_irq_num; i++) {
-		irq_set_chip(i, &davinci_irq_chip_0);
-		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
-		if (i != IRQ_TINT1_TINT34)
-			irq_set_handler(i, handle_edge_irq);
-		else
-			irq_set_handler(i, handle_level_irq);
-	}
+	for (i = 0, j= 0; i < davinci_soc_info.intc_irq_num; i += 32, j += 0x04)

                   ^

There is a space needed before the '=' as pointed by checkpatch.

Thanks,
Sekhar

Acked-by
#22 lore raw

samsung: Convert irq-vic-timer to generic irq chip

Quoted 2 lines · Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
Acked-by Kukjin Kim <[email protected]>
Quoted 5 lines
---
 arch/arm/mach-s3c64xx/irq.c                        |    7 --
 arch/arm/plat-s5p/irq.c                            |    6 -
 arch/arm/plat-samsung/include/plat/irq-vic-timer.h |    2
 arch/arm/plat-samsung/irq-vic-timer.c              |   73

++++++++----------

Quoted 3 lines

---

 4 files changed, 31 insertions(+), 57 deletions(-)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

maintainer
#23 lore raw

PATCH 1/1 arm: omap2/3: Use generic irq chip

In reply to Thomas Gleixner

Use generic irq chip for omap2 & 3.

Note that this patch also leaves out the spurious IRQ warning for omap3.

This warning should no longer be needed as the interrupt handlers for various devices have implemented the necessayr read-back of the posted write.

Signed-off-by Tony Lindgren <[email protected]>
arch/arm/mach-omap2/irq.c +25 -72
@@ -73,83 +73,18 @@ static u32 intc_bank_read_reg(struct omap_irq_bank *bank, u16 reg)
 	return __raw_readl(bank->base_reg + reg);
 }
 
-static int previous_irq;
-
-/*
- * On 34xx we can get occasional spurious interrupts if the ack from
- * an interrupt handler does not get posted before we unmask. Warn about
- * the interrupt handlers that need to flush posted writes.
- */
-static int omap_check_spurious(unsigned int irq)
-{
-	u32 sir, spurious;
-
-	sir = intc_bank_read_reg(&irq_banks[0], INTC_SIR);
-	spurious = sir >> 7;
-
-	if (spurious) {
-		printk(KERN_WARNING "Spurious irq %i: 0x%08x, please flush "
-					"posted write for irq %i\n",
-					irq, sir, previous_irq);
-		return spurious;
-	}
-
-	return 0;
-}
-
 /* XXX: FIQ and additional INTC support (only MPU at the moment) */
 static void omap_ack_irq(struct irq_data *d)
 {
 	intc_bank_write_reg(0x1, &irq_banks[0], INTC_CONTROL);
 }
 
-static void omap_mask_irq(struct irq_data *d)
-{
-	unsigned int irq = d->irq;
-	int offset = irq & (~(IRQ_BITS_PER_REG - 1));
-
-	if (cpu_is_omap34xx() && !cpu_is_ti816x()) {
-		int spurious = 0;
-
-		/*
-		 * INT_34XX_GPT12_IRQ is also the spurious irq. Maybe because
-		 * it is the highest irq number?
-		 */
-		if (irq == INT_34XX_GPT12_IRQ)
-			spurious = omap_check_spurious(irq);
-
-		if (!spurious)
-			previous_irq = irq;
-	}
-
-	irq &= (IRQ_BITS_PER_REG - 1);
-
-	intc_bank_write_reg(1 << irq, &irq_banks[0], INTC_MIR_SET0 + offset);
-}
-
-static void omap_unmask_irq(struct irq_data *d)
-{
-	unsigned int irq = d->irq;
-	int offset = irq & (~(IRQ_BITS_PER_REG - 1));
-
-	irq &= (IRQ_BITS_PER_REG - 1);
-
-	intc_bank_write_reg(1 << irq, &irq_banks[0], INTC_MIR_CLEAR0 + offset);
-}
-
 static void omap_mask_ack_irq(struct irq_data *d)
 {
-	omap_mask_irq(d);
+	irq_gc_mask_disable_reg(d);
 	omap_ack_irq(d);
 }
 
-static struct irq_chip omap_irq_chip = {
-	.name		= "INTC",
-	.irq_ack	= omap_mask_ack_irq,
-	.irq_mask	= omap_mask_irq,
-	.irq_unmask	= omap_unmask_irq,
-};
-
 static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
 {
 	unsigned long tmp;

@@ -186,11 +121,31 @@ int omap_irq_pending(void)
 	return 0;
 }
 
+static __init void
+omap_alloc_gc(void __iomem *base, unsigned int irq_start, unsigned int num)
+{
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+
+	gc = irq_alloc_generic_chip("INTC", 1, irq_start, base,
+					handle_level_irq);
+	ct = gc->chip_types;
+	ct->chip.irq_ack = omap_mask_ack_irq;
+	ct->chip.irq_mask = irq_gc_mask_disable_reg;
+	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
+
+	ct->regs.ack = INTC_CONTROL;
+	ct->regs.enable = INTC_MIR_CLEAR0;
+	ct->regs.disable = INTC_MIR_SET0;
+	irq_setup_generic_chip(gc, IRQ_MSK(num),
+				IRQ_NOREQUEST | IRQ_NOPROBE, 0);
+}
+
 void __init omap_init_irq(void)
 {
 	unsigned long nr_of_irqs = 0;
 	unsigned int nr_banks = 0;
-	int i;
+	int i, j;
 
 	for (i = 0; i < ARRAY_SIZE(irq_banks); i++) {
 		unsigned long base = 0;

@@ -215,17 +170,15 @@ void __init omap_init_irq(void)
 
 		omap_irq_bank_init_one(bank);
 
+		for (i = 0, j= 0; i < bank->nr_irqs; i += 32, j += 0x20)
+			omap_alloc_gc(bank->base_reg + j, i, 32);
+
 		nr_of_irqs += bank->nr_irqs;
 		nr_banks++;
 	}
 
 	printk(KERN_INFO "Total of %ld interrupts on %d active controller%s\n",
 	       nr_of_irqs, nr_banks, nr_banks > 1 ? "s" : "");
-
-	for (i = 0; i < nr_of_irqs; i++) {
-		irq_set_chip_and_handler(i, &omap_irq_chip, handle_level_irq);
-		set_irq_flags(i, IRQF_VALID);
-	}
 }
 
 #ifdef CONFIG_ARCH_OMAP3
Acked-by
#25 lore raw

samsung: Convert irq_uart to generic irq chip

Quoted 2 lines · Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
Acked-by Kukjin Kim <[email protected]>
Quoted 2 lines
---
 arch/arm/plat-samsung/irq-uart.c |   82

+++++-------------------------------

Quoted 3 lines

---

 1 file changed, 11 insertions(+), 71 deletions(-)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

Acked-by
#27 lore raw

samsung: s5p: Convert irq-gpioint to generic irq chip

Quoted 2 lines · Thomas Gleixner
Signed-off-by Thomas Gleixner <[email protected]>
Acked-by Kukjin Kim <[email protected]>
Quoted 2 lines
---
 arch/arm/plat-s5p/irq-gpioint.c |  115

+++++++++----------------------------

Quoted 3 lines

---

 1 file changed, 26 insertions(+), 89 deletions(-)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.

#31 lore raw

arm: msm: Convert sirc to generic irq chip

Quoted 270 lines · Thomas Gleixner
Signed-off-by Thomas Gleixner<[email protected]>
---
  arch/arm/mach-msm/include/mach/sirc.h |   43 +++-----
  arch/arm/mach-msm/sirc.c              |  166 ++++++++++------------------------
  2 files changed, 68 insertions(+), 141 deletions(-)

Index: linux-2.6/arch/arm/mach-msm/include/mach/sirc.h
===================================================================

arch/arm/mach-msm/include/mach/sirc.h +18 -25
@@ -13,18 +13,13 @@
  #ifndef __ASM_ARCH_MSM_SIRC_H
  #define __ASM_ARCH_MSM_SIRC_H
 
-struct sirc_regs_t {
-	void    *int_enable;
-	void    *int_enable_clear;
-	void    *int_enable_set;
-	void    *int_type;
-	void    *int_polarity;
-	void    *int_clear;
-};
-
  struct sirc_cascade_regs {
-	void    *int_status;
-	unsigned int    cascade_irq;
+	void __iomem	*reg_base;
+	unsigned long	int_status;
+	u32		status_mask;
+	unsigned int	irq_base;
+	unsigned int	irq_cnt;
+	unsigned int	cascade_irq;
  };
 
  void msm_init_sirc(void);

@@ -78,20 +73,18 @@ void msm_sirc_exit_sleep(void);
  #define SIRC_MASK                     0x007FFFFF
  #endif
 
-#define LAST_SIRC_IRQ                 (FIRST_SIRC_IRQ + NR_SIRC_IRQS - 1)
-
-#define SPSS_SIRC_INT_SELECT          (MSM_SIRC_BASE + 0x00)
-#define SPSS_SIRC_INT_ENABLE          (MSM_SIRC_BASE + 0x04)
-#define SPSS_SIRC_INT_ENABLE_CLEAR    (MSM_SIRC_BASE + 0x08)
-#define SPSS_SIRC_INT_ENABLE_SET      (MSM_SIRC_BASE + 0x0C)
-#define SPSS_SIRC_INT_TYPE            (MSM_SIRC_BASE + 0x10)
-#define SPSS_SIRC_INT_POLARITY        (MSM_SIRC_BASE + 0x14)
-#define SPSS_SIRC_SECURITY            (MSM_SIRC_BASE + 0x18)
-#define SPSS_SIRC_IRQ_STATUS          (MSM_SIRC_BASE + 0x1C)
-#define SPSS_SIRC_IRQ1_STATUS         (MSM_SIRC_BASE + 0x20)
-#define SPSS_SIRC_RAW_STATUS          (MSM_SIRC_BASE + 0x24)
-#define SPSS_SIRC_INT_CLEAR           (MSM_SIRC_BASE + 0x28)
-#define SPSS_SIRC_SOFT_INT            (MSM_SIRC_BASE + 0x2C)
+#define SPSS_SIRC_INT_SELECT          0x00
+#define SPSS_SIRC_INT_ENABLE          0x04
+#define SPSS_SIRC_INT_ENABLE_CLEAR    0x08
+#define SPSS_SIRC_INT_ENABLE_SET      0x0C
+#define SPSS_SIRC_INT_TYPE            0x10
+#define SPSS_SIRC_INT_POLARITY        0x14
+#define SPSS_SIRC_SECURITY            0x18
+#define SPSS_SIRC_IRQ_STATUS          0x1C
+#define SPSS_SIRC_IRQ1_STATUS         0x20
+#define SPSS_SIRC_RAW_STATUS          0x24
+#define SPSS_SIRC_INT_CLEAR           0x28
+#define SPSS_SIRC_SOFT_INT            0x2C
 
  #endif

Index: linux-2.6/arch/arm/mach-msm/sirc.c
===================================================================

arch/arm/mach-msm/sirc.c +48 -108
@@ -15,158 +15,92 @@
   * 02110-1301, USA.
   *
   */
-
-#include<linux/io.h>
-#include<linux/irq.h>
  #include<linux/interrupt.h>
-#include<asm/irq.h>
-
-static unsigned int int_enable;
-static unsigned int wake_enable;
+#include<linux/irq.h>
+#include<linux/io.h>
 
-static struct sirc_regs_t sirc_regs = {
-	.int_enable       = SPSS_SIRC_INT_ENABLE,
-	.int_enable_clear = SPSS_SIRC_INT_ENABLE_CLEAR,
-	.int_enable_set   = SPSS_SIRC_INT_ENABLE_SET,
-	.int_type         = SPSS_SIRC_INT_TYPE,
-	.int_polarity     = SPSS_SIRC_INT_POLARITY,
-	.int_clear        = SPSS_SIRC_INT_CLEAR,
-};
+#include<asm/irq.h>
 
  static struct sirc_cascade_regs sirc_reg_table[] = {
  	{
+		.reg_base    = MSM_SIRC_BASE,
  		.int_status  = SPSS_SIRC_IRQ_STATUS,
+		.status_mask = SIRC_MASK,
+		.irq_base    = FIRST_SIRC_IRQ,
+		.irq_cnt     = NR_SIRC_IRQS,
  		.cascade_irq = INT_SIRC_0,
  	}
  };
 
-/* Mask off the given interrupt. Keep the int_enable mask in sync with
-   the enable reg, so it can be restored after power collapse. */
-static void sirc_irq_mask(struct irq_data *d)
-{
-	unsigned int mask;
-
-	mask = 1<<  (d->irq - FIRST_SIRC_IRQ);
-	writel(mask, sirc_regs.int_enable_clear);
-	int_enable&= ~mask;
-	return;
-}
-
-/* Unmask the given interrupt. Keep the int_enable mask in sync with
-   the enable reg, so it can be restored after power collapse. */
-static void sirc_irq_unmask(struct irq_data *d)
-{
-	unsigned int mask;
-
-	mask = 1<<  (d->irq - FIRST_SIRC_IRQ);
-	writel(mask, sirc_regs.int_enable_set);
-	int_enable |= mask;
-	return;
-}
-
-static void sirc_irq_ack(struct irq_data *d)
-{
-	unsigned int mask;
-
-	mask = 1<<  (d->irq - FIRST_SIRC_IRQ);
-	writel(mask, sirc_regs.int_clear);
-	return;
-}
-
-static int sirc_irq_set_wake(struct irq_data *d, unsigned int on)
-{
-	unsigned int mask;
-
-	/* Used to set the interrupt enable mask during power collapse. */
-	mask = 1<<  (d->irq - FIRST_SIRC_IRQ);
-	if (on)
-		wake_enable |= mask;
-	else
-		wake_enable&= ~mask;
-
-	return 0;
-}
-
  static int sirc_irq_set_type(struct irq_data *d, unsigned int flow_type)
  {
-	unsigned int mask;
-	unsigned int val;
-
-	mask = 1<<  (d->irq - FIRST_SIRC_IRQ);
-	val = readl(sirc_regs.int_polarity);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 mask = 1<<  (d->irq - gc->irq_base);
 
  	if (flow_type&  (IRQF_TRIGGER_LOW | IRQF_TRIGGER_FALLING))
-		val |= mask;
+		gc->polarity_cache |= mask;
  	else
-		val&= ~mask;
-
-	writel(val, sirc_regs.int_polarity);
+		gc->polarity_cache&= ~mask;
+	writel(gc->polarity_cache, gc->reg_base + gc->chip_types->regs.type);
 
-	val = readl(sirc_regs.int_type);
  	if (flow_type&  (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		val |= mask;
+		gc->type_cache |= mask;
  		__irq_set_handler_locked(d->irq, handle_edge_irq);
  	} else {
-		val&= ~mask;
+		gc->type_cache&= ~mask;
  		__irq_set_handler_locked(d->irq, handle_level_irq);
  	}
-
-	writel(val, sirc_regs.int_type);
-
+	writel(gc->type_cache, gc->reg_base + gc->chip_types->regs.polarity);
  	return 0;
  }
 
  /* Finds the pending interrupt on the passed cascade irq and redrives it */
  static void sirc_irq_handler(unsigned int irq, struct irq_desc *desc)
  {
-	unsigned int reg = 0;
-	unsigned int sirq;
-	unsigned int status;
-
-	while ((reg<  ARRAY_SIZE(sirc_reg_table))&&
-		(sirc_reg_table[reg].cascade_irq != irq))
-		reg++;
-
-	status = readl(sirc_reg_table[reg].int_status);
-	status&= SIRC_MASK;
-	if (status == 0)
+	struct sirc_cascade_regs *cr = irq_desc_get_handler_data(desc);
+	u32 status;
+
+	status = readl(cr->reg_base + cr->int_status);
+	status&= cr->status_mask;
+	if (!status)
  		return;
 
-	for (sirq = 0;
-	     (sirq<  NR_SIRC_IRQS)&&  ((status&  (1U<<  sirq)) == 0);
-	     sirq++)
-		;
-	generic_handle_irq(sirq+FIRST_SIRC_IRQ);
+	generic_handle_irq(cr->irq_base + __ffs(status));
 
  	desc->irq_data.chip->irq_ack(&desc->irq_data);
  }
 
-static struct irq_chip sirc_irq_chip = {
-	.name          = "sirc",
-	.irq_ack       = sirc_irq_ack,
-	.irq_mask      = sirc_irq_mask,
-	.irq_unmask    = sirc_irq_unmask,
-	.irq_set_wake  = sirc_irq_set_wake,
-	.irq_set_type  = sirc_irq_set_type,
-};
+static void msm_init_one_sirc(struct sirc_cascade_regs *cr)
+{
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+
+	gc = irq_alloc_generic_chip("sirc", 1, cr->irq_base, cr->reg_base,
+				    handle_level_irq);
+	ct = gc->chip_types;
+	ct->chip.irq_ack = irq_gc_ack;
+	ct->chip.irq_mask = irq_gc_mask_disable_reg;
+	ct->chip.irq_unmask = irq_gc_unmask_enable_reg;
+	ct->chip.irq_set_wake = irq_gc_set_wake;
+	ct->chip.irq_set_type = sirc_irq_set_type;
+	ct->regs.ack = SPSS_SIRC_INT_CLEAR;
+	ct->regs.disable = SPSS_SIRC_INT_ENABLE_CLEAR;
+	ct->regs.enable = SPSS_SIRC_INT_ENABLE_SET;
+	ct->regs.type = SPSS_SIRC_INT_TYPE;
+	ct->regs.polarity = SPSS_SIRC_INT_POLARITY;
+
+	irq_setup_generic_chip(gc, IRQ_MSK(sirc->irq_cnt),
+			       IRQ_NOREQUEST | IRQ_NOPROBE, 0);
+
+	irq_set_chained_handler(cr->cascade_irq, sirc_irq_handler);
+	irq_set_handler_data(cr->cascade_irq, cr);
+	irq_set_irq_wake(cr->cascade_irq, 1);
+}
 
  void __init msm_init_sirc(void)
  {
  	int i;
 
-	int_enable = 0;
-	wake_enable = 0;
-
-	for (i = FIRST_SIRC_IRQ; i<  LAST_SIRC_IRQ; i++) {
-		irq_set_chip_and_handler(i,&sirc_irq_chip, handle_edge_irq);
-		set_irq_flags(i, IRQF_VALID);
-	}

arch/arm/mach-msm/sirc.c line 106

Quoted 10 lines
arch/arm/mach-msm/sirc.c +2 -7
-
-	for (i = 0; i<  ARRAY_SIZE(sirc_reg_table); i++) {
-		irq_set_chained_handler(sirc_reg_table[i].cascade_irq,
-					sirc_irq_handler);
-		irq_set_irq_wake(sirc_reg_table[i].cascade_irq, 1);
-	}
-	return;
+	for (i = 0; i<  ARRAY_SIZE(sirc_reg_table); i++)
+		msm_init_one_sirc(sirc_reg_table + i);
  }

This looks good to me. Thanks a ton for cleaning it up.

Abhijeet

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#34 lore raw

arm: msm: Convert irq-vic to generic irq chip

Quoted 224 lines · Thomas Gleixner

This removes a ton of completely useless code. The shadow registers, which are set but never used are maintained in the generic chip functions on a per chip basis as well.

Signed-off-by Thomas Gleixner<[email protected]>
---
  arch/arm/mach-msm/irq-vic.c |  342 ++++++--------------------------------------
  1 file changed, 51 insertions(+), 291 deletions(-)

Index: linux-2.6/arch/arm/mach-msm/irq-vic.c
===================================================================

arch/arm/mach-msm/irq-vic.c +11 -176
@@ -12,331 +12,93 @@
   * GNU General Public License for more details.
   *
   */
-
-#include<linux/init.h>
-#include<linux/module.h>
-#include<linux/sched.h>
  #include<linux/interrupt.h>
-#include<linux/ptrace.h>
-#include<linux/timer.h>
  #include<linux/irq.h>
  #include<linux/io.h>
 
-#include<asm/cacheflush.h>
-
  #include<mach/hardware.h>
-
  #include<mach/msm_iomap.h>
 
-#include "smd_private.h"
-
-enum {
-	IRQ_DEBUG_SLEEP_INT_TRIGGER = 1U<<  0,
-	IRQ_DEBUG_SLEEP_INT = 1U<<  1,
-	IRQ_DEBUG_SLEEP_ABORT = 1U<<  2,
-	IRQ_DEBUG_SLEEP = 1U<<  3,
-	IRQ_DEBUG_SLEEP_REQUEST = 1U<<  4,
-};
-static int msm_irq_debug_mask;
-module_param_named(debug_mask, msm_irq_debug_mask, int,
-		   S_IRUGO | S_IWUSR | S_IWGRP);
-
  #define VIC_REG(off) (MSM_VIC_BASE + (off))
-#define VIC_INT_TO_REG_ADDR(base, irq) (base + (irq / 32) * 4)
-#define VIC_INT_TO_REG_INDEX(irq) ((irq>>  5)&  3)
 
-#define VIC_INT_SELECT0     VIC_REG(0x0000)  /* 1: FIQ, 0: IRQ */
-#define VIC_INT_SELECT1     VIC_REG(0x0004)  /* 1: FIQ, 0: IRQ */
-#define VIC_INT_SELECT2     VIC_REG(0x0008)  /* 1: FIQ, 0: IRQ */
-#define VIC_INT_SELECT3     VIC_REG(0x000C)  /* 1: FIQ, 0: IRQ */
-#define VIC_INT_EN0         VIC_REG(0x0010)
-#define VIC_INT_EN1         VIC_REG(0x0014)
-#define VIC_INT_EN2         VIC_REG(0x0018)
-#define VIC_INT_EN3         VIC_REG(0x001C)
-#define VIC_INT_ENCLEAR0    VIC_REG(0x0020)
-#define VIC_INT_ENCLEAR1    VIC_REG(0x0024)
-#define VIC_INT_ENCLEAR2    VIC_REG(0x0028)
-#define VIC_INT_ENCLEAR3    VIC_REG(0x002C)
-#define VIC_INT_ENSET0      VIC_REG(0x0030)
-#define VIC_INT_ENSET1      VIC_REG(0x0034)
-#define VIC_INT_ENSET2      VIC_REG(0x0038)
-#define VIC_INT_ENSET3      VIC_REG(0x003C)
-#define VIC_INT_TYPE0       VIC_REG(0x0040)  /* 1: EDGE, 0: LEVEL  */
-#define VIC_INT_TYPE1       VIC_REG(0x0044)  /* 1: EDGE, 0: LEVEL  */
-#define VIC_INT_TYPE2       VIC_REG(0x0048)  /* 1: EDGE, 0: LEVEL  */
-#define VIC_INT_TYPE3       VIC_REG(0x004C)  /* 1: EDGE, 0: LEVEL  */
-#define VIC_INT_POLARITY0   VIC_REG(0x0050)  /* 1: NEG, 0: POS */
-#define VIC_INT_POLARITY1   VIC_REG(0x0054)  /* 1: NEG, 0: POS */
-#define VIC_INT_POLARITY2   VIC_REG(0x0058)  /* 1: NEG, 0: POS */
-#define VIC_INT_POLARITY3   VIC_REG(0x005C)  /* 1: NEG, 0: POS */
-#define VIC_NO_PEND_VAL     VIC_REG(0x0060)
+#define VIC_INT_SELECT0		0x0000		/* 1: FIQ, 0: IRQ */
+#define VIC_INT_EN0		0x0010
+#define VIC_INT_ENCLEAR0	0x0020
+#define VIC_INT_ENSET0		0x0030
+#define VIC_INT_TYPE0		0x0040		/* 1: EDGE, 0: LEVEL  */
+#define VIC_INT_POLARITY0	0x0050		/* 1: NEG, 0: POS */
+#define VIC_INT_CLEAR0		0x00B0
 
  #if defined(CONFIG_ARCH_MSM_SCORPION)
-#define VIC_NO_PEND_VAL_FIQ VIC_REG(0x0064)
-#define VIC_INT_MASTEREN    VIC_REG(0x0068)  /* 1: IRQ, 2: FIQ     */
-#define VIC_CONFIG          VIC_REG(0x006C)  /* 1: USE SC VIC */
+#define VIC_INT_MASTEREN	0x0068		/* 1: IRQ, 2: FIQ     */
+#define VIC_CONFIG		0x006C		/* 1: USE ARM1136 VIC */
  #else
-#define VIC_INT_MASTEREN    VIC_REG(0x0064)  /* 1: IRQ, 2: FIQ     */
-#define VIC_PROTECTION      VIC_REG(0x006C)  /* 1: ENABLE          */
-#define VIC_CONFIG          VIC_REG(0x0068)  /* 1: USE ARM1136 VIC */
+#define VIC_INT_MASTEREN	0x0064		/* 1: IRQ, 2: FIQ     */
+#define VIC_CONFIG		0x0068		/* 1: USE ARM1136 VIC */
  #endif
 
-#define VIC_IRQ_STATUS0     VIC_REG(0x0080)
-#define VIC_IRQ_STATUS1     VIC_REG(0x0084)
-#define VIC_IRQ_STATUS2     VIC_REG(0x0088)
-#define VIC_IRQ_STATUS3     VIC_REG(0x008C)
-#define VIC_FIQ_STATUS0     VIC_REG(0x0090)
-#define VIC_FIQ_STATUS1     VIC_REG(0x0094)
-#define VIC_FIQ_STATUS2     VIC_REG(0x0098)
-#define VIC_FIQ_STATUS3     VIC_REG(0x009C)
-#define VIC_RAW_STATUS0     VIC_REG(0x00A0)
-#define VIC_RAW_STATUS1     VIC_REG(0x00A4)
-#define VIC_RAW_STATUS2     VIC_REG(0x00A8)
-#define VIC_RAW_STATUS3     VIC_REG(0x00AC)
-#define VIC_INT_CLEAR0      VIC_REG(0x00B0)
-#define VIC_INT_CLEAR1      VIC_REG(0x00B4)
-#define VIC_INT_CLEAR2      VIC_REG(0x00B8)
-#define VIC_INT_CLEAR3      VIC_REG(0x00BC)
-#define VIC_SOFTINT0        VIC_REG(0x00C0)
-#define VIC_SOFTINT1        VIC_REG(0x00C4)
-#define VIC_SOFTINT2        VIC_REG(0x00C8)
-#define VIC_SOFTINT3        VIC_REG(0x00CC)
-#define VIC_IRQ_VEC_RD      VIC_REG(0x00D0)  /* pending int # */
-#define VIC_IRQ_VEC_PEND_RD VIC_REG(0x00D4)  /* pending vector addr */
-#define VIC_IRQ_VEC_WR      VIC_REG(0x00D8)
-
-#if defined(CONFIG_ARCH_MSM_SCORPION)
-#define VIC_FIQ_VEC_RD      VIC_REG(0x00DC)
-#define VIC_FIQ_VEC_PEND_RD VIC_REG(0x00E0)
-#define VIC_FIQ_VEC_WR      VIC_REG(0x00E4)
-#define VIC_IRQ_IN_SERVICE  VIC_REG(0x00E8)
-#define VIC_IRQ_IN_STACK    VIC_REG(0x00EC)
-#define VIC_FIQ_IN_SERVICE  VIC_REG(0x00F0)
-#define VIC_FIQ_IN_STACK    VIC_REG(0x00F4)
-#define VIC_TEST_BUS_SEL    VIC_REG(0x00F8)
-#define VIC_IRQ_CTRL_CONFIG VIC_REG(0x00FC)
-#else
-#define VIC_IRQ_IN_SERVICE  VIC_REG(0x00E0)
-#define VIC_IRQ_IN_STACK    VIC_REG(0x00E4)
-#define VIC_TEST_BUS_SEL    VIC_REG(0x00E8)
-#endif
-
-#define VIC_VECTPRIORITY(n) VIC_REG(0x0200+((n) * 4))
-#define VIC_VECTADDR(n)     VIC_REG(0x0400+((n) * 4))
-
  #if defined(CONFIG_ARCH_MSM7X30)
  #define VIC_NUM_REGS	    4
  #else
  #define VIC_NUM_REGS	    2
  #endif
 
-#if VIC_NUM_REGS == 2
-#define DPRINT_REGS(base_reg, format, ...)	      			\
-	printk(KERN_INFO format " %x %x\n", ##__VA_ARGS__,		\
-			readl(base_reg ## 0), readl(base_reg ## 1))
-#define DPRINT_ARRAY(array, format, ...)				\
-	printk(KERN_INFO format " %x %x\n", ##__VA_ARGS__,		\
-			array[0], array[1])
-#elif VIC_NUM_REGS == 4
-#define DPRINT_REGS(base_reg, format, ...) \
-	printk(KERN_INFO format " %x %x %x %x\n", ##__VA_ARGS__,	\
-			readl(base_reg ## 0), readl(base_reg ## 1),	\
-			readl(base_reg ## 2), readl(base_reg ## 3))
-#define DPRINT_ARRAY(array, format, ...)				\
-	printk(KERN_INFO format " %x %x %x %x\n", ##__VA_ARGS__,	\
-			array[0], array[1],				\
-			array[2], array[3])
-#else
-#error "VIC_NUM_REGS set to illegal value"
-#endif
-
-static uint32_t msm_irq_smsm_wake_enable[2];
-static struct {
-	uint32_t int_en[2];
-	uint32_t int_type;
-	uint32_t int_polarity;
-	uint32_t int_select;
-} msm_irq_shadow_reg[VIC_NUM_REGS];
-static uint32_t msm_irq_idle_disable[VIC_NUM_REGS];
-
-#define SMSM_FAKE_IRQ (0xff)
-static uint8_t msm_irq_to_smsm[NR_IRQS] = {
-	[INT_MDDI_EXT] = 1,
-	[INT_MDDI_PRI] = 2,
-	[INT_MDDI_CLIENT] = 3,
-	[INT_USB_OTG] = 4,
-
-	[INT_PWB_I2C] = 5,
-	[INT_SDC1_0] = 6,
-	[INT_SDC1_1] = 7,
-	[INT_SDC2_0] = 8,
-
-	[INT_SDC2_1] = 9,
-	[INT_ADSP_A9_A11] = 10,
-	[INT_UART1] = 11,
-	[INT_UART2] = 12,
-
-	[INT_UART3] = 13,
-	[INT_UART1_RX] = 14,
-	[INT_UART2_RX] = 15,
-	[INT_UART3_RX] = 16,
-
-	[INT_UART1DM_IRQ] = 17,
-	[INT_UART1DM_RX] = 18,
-	[INT_KEYSENSE] = 19,
-#if !defined(CONFIG_ARCH_MSM7X30)
-	[INT_AD_HSSD] = 20,
-#endif
-
-	[INT_NAND_WR_ER_DONE] = 21,
-	[INT_NAND_OP_DONE] = 22,
-	[INT_TCHSCRN1] = 23,
-	[INT_TCHSCRN2] = 24,
-
-	[INT_TCHSCRN_SSBI] = 25,
-	[INT_USB_HS] = 26,
-	[INT_UART2DM_RX] = 27,
-	[INT_UART2DM_IRQ] = 28,
-
-	[INT_SDC4_1] = 29,
-	[INT_SDC4_0] = 30,
-	[INT_SDC3_1] = 31,
-	[INT_SDC3_0] = 32,
-
-	/* fake wakeup interrupts */
-	[INT_GPIO_GROUP1] = SMSM_FAKE_IRQ,
-	[INT_GPIO_GROUP2] = SMSM_FAKE_IRQ,
-	[INT_A9_M2A_0] = SMSM_FAKE_IRQ,
-	[INT_A9_M2A_1] = SMSM_FAKE_IRQ,

arch/arm/mach-msm/irq-vic.c line 220

Quoted 10 lines
arch/arm/mach-msm/irq-vic.c +0 -10
-	[INT_A9_M2A_5] = SMSM_FAKE_IRQ,
-	[INT_GP_TIMER_EXP] = SMSM_FAKE_IRQ,
-	[INT_DEBUG_TIMER_EXP] = SMSM_FAKE_IRQ,
-	[INT_ADSP_A11] = SMSM_FAKE_IRQ,
-#ifdef CONFIG_ARCH_QSD8X50
-	[INT_SIRC_0] = SMSM_FAKE_IRQ,
-	[INT_SIRC_1] = SMSM_FAKE_IRQ,
-#endif
-};
-

The entire patch looks good to me.

However, let me explain that SMSM arrays are. MSM has a different interrupt controller that monitors few of the lines that the VIC does. This parallel interrupt controller is activated when the MSM goes to low power modes.

For idle case, if an interrupt not monitored by the parallel interrupt controller is enabled, we avoid going to low power modes. It is unreasonable to check all the shadow registers to find if such is the case, so we maintain the unmask status of such interrupts in static uint32_t msm_irq_idle_disable[VIC_NUM_REGS]. If any bit is set we dont go to low power mode. This code is not upstreamed yet, I can submit it on top of this patch.

The reason for explaining this is to ask if we should extend the generic chip to have a function that returns if any interrupts besides the wakeup ones are unmasked. It would return

mask_cache & ~wakeup_active if mask_cache bit set means interrupt is unmasked
~mask_cache & ~wakeup_active if mask_cache bit set means interrupts is masked

or you would prefer the driver do that check?

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
author
#35 lore raw

arm: msm: Convert irq-vic to generic irq chip

Quoted 14 lines · Abhijeet Dharmapurikar

On 04/16/2011 02:14 PM, Thomas Gleixner wrote:
However, let me explain that SMSM arrays are. MSM has a different interrupt controller that monitors few of the lines that the VIC does. This parallel interrupt controller is activated when the MSM goes to low power modes.

For idle case, if an interrupt not monitored by the parallel interrupt controller is enabled, we avoid going to low power modes. It is unreasonable to check all the shadow registers to find if such is the case, so we maintain the unmask status of such interrupts in
static uint32_t msm_irq_idle_disable[VIC_NUM_REGS]. If any bit is set we dont go to low power mode. This code is not upstreamed yet, I can submit it on top of this patch.

Quoted 8 lines

The reason for explaining this is to ask if we should extend the generic chip to have a function that returns if any interrupts besides the wakeup ones are unmasked. It would return

mask_cache & ~wakeup_active if mask_cache bit set means interrupt is unmasked ~mask_cache & ~wakeup_active if mask_cache bit set means interrupts is masked

or you would prefer the driver do that check?

That wants to be a function in the core as I assume that there are similar usecases in tree.

Another question is whether you could move to the arm/common/vic.c implementation completely.

Thanks,

tglx

#36 lore raw

arm: msm: Convert irq-vic to generic irq chip

Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Abhijeet Dharmapurikar

On 04/16/2011 02:14 PM, Thomas Gleixner wrote:

Another question is whether you could move to the arm/common/vic.c implementation completely.

arm/common/vic.c doesnt have a set_type callback. We have some edge triggered interrupts as wakeup interrupts and need the PENDING flag in the descriptor set when disabled. Can we move msm_irq_set_type() to arm/common/vic.c ?

Additionally
- the register map is different between msm's vic and the common one.
- the common one uses some PL190 registers, not sure what that is. The VIC can operate in few different modes, perhaps it refers to vectored mode as opposed to non vectored mode, msm uses the non vectored mode.
- vic_init2() seems like it is initializing a cascaded interrupt controller. msm doesnt use cascaded vics.

I dont think msm can move to using the common vic.c.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#37 lore raw

arm: msm: Convert irq-vic to generic irq chip

2011/4/25 Abhijeet Dharmapurikar <[email protected]>:

Quoted 1 line

- the common one uses some PL190 registers, not sure what that is.

PL190 is ARMs name for the VIC: PrimeCell 190.

Every vendor get these PrimeCells as part of their bargin for CPU cores when licensing ARM IP. Then they tweak them to suit their needs and fix hardware bugs.

It seems Qualcomm HW engineers did the same thing as everyone else, tweak a PrimeCell by editing VHDL until it works...

Yours,
Linus Walleij

#42 lore raw

cover Interrupt chip consolidation

In reply to Thomas Gleixner
Quoted 10 lines · Thomas Gleixner

[...]

That converts 18 irq_chips and removes at least one duplicated VIC implementation.

Please have a look and check also the not yet converted chips whether they could make use of such an generic implementation. We better have some oddball callback functions in the generic code than ten slightly different instances of them all over the place.

I've adopted JZ4740 platform which is MIPS based SoC to the generic_irq_chip and the diffstat looks ok:

 arch/mips/jz4740/gpio.c  |  116 +++++++++++++++-------------------------------
 arch/mips/jz4740/irq.c   |   95 +++++++++++++++----------------------
 arch/mips/jz4740/pm.c    |    2 -
 drivers/mfd/jz4740-adc.c |   73 +++++++----------------------
 4 files changed, 94 insertions(+), 192 deletions(-)

I've also noticed that all my chained demuxer handlers follow a similar scheme and a quick grep for generic_handle_irq revealed that quite a few other implementations also follow a similar scheme as well.

Something along the lines of ...

void irq_gc_chained_demux_handler(unsigned int irq, struct irq_desc *desc)

{
	struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
	struct irq_chip_type *ct = gc = irq_chip_generic_current_chip_type(gc);
	unsigned long pending = irq_reg_readl(ct->regs.pending);

	pending = irq_reg_readl(ct->regs.pending) & gc->mask_cache;

	for_each_set_bit (irq, &pending, gc->irq_cnt)
		generic_handle_irq(gc->irq_base + irq);
}

... could be used to replace those custom handlers in an irq_chip_generic based implementation.

With this the diffstat for JZ4740 looks even better:

 4 files changed, 91 insertions(+), 232 deletions(-)

Unfortunately it doesn't completely fit into the current irq_chip_generic implementation. For one there is no irq_chip_generic_current_chip_type, but I guess that could be implementing by adding a field to irq_chip_generic pointing to the current chip type. On the other hand I'm not sure if it is necessary to have different pending registers for different chip types of the same irq_chip_generic.

Another issue is that while the gpio unit on the JZ4740 supports different trigger modes it only supports either rising or falling edge at one time. So since it doesn't makes sense to implement switching between those two in a driver triggering in both edges is emulated by the irq chip. Whenever an interrupt occurs or it is unmask the current state is read the hardware is configured to trigger when the state would change. So if the pin is low the hardware is set to trigger on a rising edge.

And of course this is not the only gpio unit which requires this. On ARM Orion, MSN, MXC and possible others have the same issue.
I wonder if you see an option to handle this in a generic way? Either in the irq core or in the generic_irq_chip implementation, or is this something better put into into a specialized gpio_irq_chip implementation?
This would at least require a callback by which the generic code could get the current irq level, a specialized unmask and demuxer.

Also related to gpio irq chips is the lockdep class issue.
Would it make sense to add a lockdep class parameter to irq_setup_generic_chip or is this something better put into a separate function along the lines of.

void irq_generic_chip_set_lockclass(struct irq_generic_chip *gc,
struct lock_class_key *lockdep_class)

{
	unsigned int irq;
	for (irq = 0; irq < gc->irq_cnt; irq++)
		irq_set_lockdep_class(gc->irq_bas + irq, lockdep_class);
}

- Lars

author
#43 lore raw

cover Interrupt chip consolidation

Quoted 12 lines · Lars-Peter Clausen
Quoted 10 lines · Thomas Gleixner

[...]

That converts 18 irq_chips and removes at least one duplicated VIC implementation.

Please have a look and check also the not yet converted chips whether they could make use of such an generic implementation. We better have some oddball callback functions in the generic code than ten slightly different instances of them all over the place.

Quoted 8 lines

I've adopted JZ4740 platform which is MIPS based SoC to the generic_irq_chip and the diffstat looks ok:

 arch/mips/jz4740/gpio.c  |  116 +++++++++++++++-------------------------------
 arch/mips/jz4740/irq.c   |   95 +++++++++++++++----------------------
 arch/mips/jz4740/pm.c    |    2 -
 drivers/mfd/jz4740-adc.c |   73 +++++++----------------------
 4 files changed, 94 insertions(+), 192 deletions(-)

Nice!

Quoted 21 lines

I've also noticed that all my chained demuxer handlers follow a similar scheme and a quick grep for generic_handle_irq revealed that quite a few other implementations also follow a similar scheme as well.

Something along the lines of ...

void irq_gc_chained_demux_handler(unsigned int irq, struct irq_desc *desc)

{
	struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
	struct irq_chip_type *ct = gc = irq_chip_generic_current_chip_type(gc);
	unsigned long pending = irq_reg_readl(ct->regs.pending);

	pending = irq_reg_readl(ct->regs.pending) & gc->mask_cache;

	for_each_set_bit (irq, &pending, gc->irq_cnt)
		generic_handle_irq(gc->irq_base + irq);
}

... could be used to replace those custom handlers in an irq_chip_generic based implementation.

Quoted 7 lines

With this the diffstat for JZ4740 looks even better:

 4 files changed, 91 insertions(+), 232 deletions(-)

Unfortunately it doesn't completely fit into the current irq_chip_generic implementation. For one there is no irq_chip_generic_current_chip_type, but I guess that could be implementing by adding a field to irq_chip_generic pointing to the current chip type. On the other hand I'm not sure if it is necessary to

No, you can get the current chip from irq_data.chip which always has a pointer to the current active one. container_of will give you the generic chip.

Quoted 7 lines

have different pending registers for different chip types of the same irq_chip_generic.

Another issue is that while the gpio unit on the JZ4740 supports different trigger modes it only supports either rising or falling edge at one time. So since it doesn't makes sense to implement switching between those two in a driver triggering in both edges is emulated by the irq chip.

That's what about 5 dozen other irq chips do as well.

Quoted 11 lines

Also related to gpio irq chips is the lockdep class issue.
Would it make sense to add a lockdep class parameter to irq_setup_generic_chip or is this something better put into a separate function along the lines of.

void irq_generic_chip_set_lockclass(struct irq_generic_chip *gc,
struct lock_class_key *lockdep_class)

{
	unsigned int irq;
	for (irq = 0; irq < gc->irq_cnt; irq++)
		irq_set_lockdep_class(gc->irq_bas + irq, lockdep_class);
}

Probably yes.

Thanks,

tglx

#44 lore raw

cover Interrupt chip consolidation

Quoted 28 lines · Thomas Gleixner
Quoted 27 lines · Lars-Peter Clausen
Quoted 1 line · Thomas Gleixner

[...]

I've also noticed that all my chained demuxer handlers follow a similar scheme and a quick grep for generic_handle_irq revealed that quite a few other implementations also follow a similar scheme as well.

Something along the lines of ...

void irq_gc_chained_demux_handler(unsigned int irq, struct irq_desc *desc)

{
	struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
	struct irq_chip_type *ct = gc = irq_chip_generic_current_chip_type(gc);
	unsigned long pending = irq_reg_readl(ct->regs.pending);

	pending = irq_reg_readl(ct->regs.pending) & gc->mask_cache;

	for_each_set_bit (irq, &pending, gc->irq_cnt)
		generic_handle_irq(gc->irq_base + irq);
}

... could be used to replace those custom handlers in an irq_chip_generic based implementation.

With this the diffstat for JZ4740 looks even better:

 4 files changed, 91 insertions(+), 232 deletions(-)
Quoted 8 lines
Quoted 4 lines

Unfortunately it doesn't completely fit into the current irq_chip_generic implementation. For one there is no irq_chip_generic_current_chip_type, but I guess that could be implementing by adding a field to irq_chip_generic pointing to the current chip type. On the other hand I'm not sure if it is necessary to

No, you can get the current chip from irq_data.chip which always has a pointer to the current active one. container_of will give you the generic chip.

Yes, but the chained demux handler is of course installed on the parent irq chip. I guess we could do something like cur_regs(irq_get_irq_data(gc->irq_base)) though, but that is a lot of indirection.

Quoted 10 lines
Quoted 8 lines

have different pending registers for different chip types of the same irq_chip_generic.

Another issue is that while the gpio unit on the JZ4740 supports different trigger modes it only supports either rising or falling edge at one time. So since it doesn't makes sense to implement switching between those two in a driver triggering in both edges is emulated by the irq chip.

That's what about 5 dozen other irq chips do as well.

Yes, I've noticed as well. That is why I was asking if you see the possibility to handle this in the core somehow.

- Lars

author
#45 lore raw

cover Interrupt chip consolidation

Quoted 34 lines · Lars-Peter Clausen
Quoted 33 lines · Thomas Gleixner
Quoted 31 lines · Lars-Peter Clausen
Quoted 1 line · Thomas Gleixner

[...]

I've also noticed that all my chained demuxer handlers follow a similar scheme and a quick grep for generic_handle_irq revealed that quite a few other implementations also follow a similar scheme as well.

Something along the lines of ...

void irq_gc_chained_demux_handler(unsigned int irq, struct irq_desc *desc)

{
	struct irq_chip_generic *gc = irq_desc_get_handler_data(desc);
	struct irq_chip_type *ct = gc = irq_chip_generic_current_chip_type(gc);
	unsigned long pending = irq_reg_readl(ct->regs.pending);

	pending = irq_reg_readl(ct->regs.pending) & gc->mask_cache;

	for_each_set_bit (irq, &pending, gc->irq_cnt)
		generic_handle_irq(gc->irq_base + irq);
}

... could be used to replace those custom handlers in an irq_chip_generic based implementation.

With this the diffstat for JZ4740 looks even better:

 4 files changed, 91 insertions(+), 232 deletions(-)

Unfortunately it doesn't completely fit into the current irq_chip_generic implementation. For one there is no irq_chip_generic_current_chip_type, but I guess that could be implementing by adding a field to irq_chip_generic pointing to the current chip type. On the other hand I'm not sure if it is necessary to

Quoted 7 lines
Quoted 3 lines

No, you can get the current chip from irq_data.chip which always has a pointer to the current active one. container_of will give you the generic chip.

Yes, but the chained demux handler is of course installed on the parent irq chip. I guess we could do something like cur_regs(irq_get_irq_data(gc->irq_base)) though, but that is a lot of indirection.

Nah. I whip up a inline helper which lets you retrieve that info from *desc.

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines

Another issue is that while the gpio unit on the JZ4740 supports different trigger modes it only supports either rising or falling edge at one time. So since it doesn't makes sense to implement switching between those two in a driver triggering in both edges is emulated by the irq chip.

That's what about 5 dozen other irq chips do as well.

Yes, I've noticed as well. That is why I was asking if you see the possibility to handle this in the core somehow.

If the patterns are similar enough then we definitely should do that.

Thanks,

tglx