diff mbox

[RFC,7/8] genirq: generic chip: Add linear irq domain support

Message ID 20130503214629.810207749@linutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Gleixner May 3, 2013, 9:50 p.m. UTC
Provide infrastructure for irq chip implementations which work on
linear irq domains.

- Interface to allocate multiple generic chips which are associated to
  the irq domain.

- Interface to get the generic chip pointer for a particular hardware
  interrupt in the domain.

- irq domain mapping function to install the chip for a particular
  interrupt.

Note: This lacks a removal function for now, but this is a draft patch
the ARM folks to work on.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/irq.h       |   30 +++++++
 include/linux/irqdomain.h |   12 +++
 kernel/irq/generic-chip.c |  179 ++++++++++++++++++++++++++++++++++++++++++++--
 kernel/irq/irqdomain.c    |    6 -
 4 files changed, 215 insertions(+), 12 deletions(-)

Comments

Russell King - ARM Linux May 3, 2013, 10:23 p.m. UTC | #1
On Fri, May 03, 2013 at 09:50:53PM -0000, Thomas Gleixner wrote:
> +	/* Init mask cache ? */
> +	if (dgc->gc_flags & IRQ_GC_INIT_MASK_CACHE) {
> +		raw_spin_lock_irqsave(&gc->lock, flags);
> +		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
> +		raw_spin_unlock_irqrestore(&gc->lock, flags);
> +	}

This looks a little weird to me - it seems that it'll re-read this
each time any irq is mapped in the domain, which is probably not
wanted.
Thomas Gleixner May 3, 2013, 10:38 p.m. UTC | #2
On Fri, 3 May 2013, Russell King - ARM Linux wrote:

> On Fri, May 03, 2013 at 09:50:53PM -0000, Thomas Gleixner wrote:
> > +	/* Init mask cache ? */
> > +	if (dgc->gc_flags & IRQ_GC_INIT_MASK_CACHE) {
> > +		raw_spin_lock_irqsave(&gc->lock, flags);
> > +		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
> > +		raw_spin_unlock_irqrestore(&gc->lock, flags);
> > +	}
> 
> This looks a little weird to me - it seems that it'll re-read this
> each time any irq is mapped in the domain, which is probably not
> wanted.

Yes, it's sloppy in two aspects.

1) It does not respect the per irq type mask cache, which got
   introduced in the same series

2) It rereads the mask cache for each mapping, but thats harmless
   because it's proper serialized. We can avoid that by clearing the
   IRQ_GC_INIT_MASK_CACHE bit when the first irq of that chip is
   mapped.

Congrats, you found a bug and as I said:

	  WARNING: It's compile tested only. So if you find bugs you can keep
	  them and fix them yourself :)

Thanks,

	tglx
Sebastian Hesselbarth May 4, 2013, 2:30 a.m. UTC | #3
On 05/03/2013 11:50 PM, Thomas Gleixner wrote:
 > Provide infrastructure for irq chip implementations which work on
 > linear irq domains.

Thomas,

I am happy that I put you into rant mode. It took me little more
than an hour to read through your patches, prepare orion irqchip
driver on top of them and finally got it working.

Anyway, I found some more issues.

 > Index: linux-2.6/kernel/irq/generic-chip.c
 > ===================================================================
 > --- linux-2.6.orig/kernel/irq/generic-chip.c
 > +++ linux-2.6/kernel/irq/generic-chip.c
 > @@ -7,6 +7,7 @@
[...]
 > +int irq_alloc_domain_generic_chips(struct irq_domain *d, int 
irqs_per_chip,
 > +				   int num_ct, const char *name,
 > +				   irq_flow_handler_t handler,
 > +				   unsigned int clr, unsigned int set,
 > +				   enum irq_gc_flags gcflags)
 > +{
 > +	struct irq_domain_chip_generic *dgc;
 > +	struct irq_chip_generic *gc;
 > +	int numchips, sz, i;
 > +	unsigned long flags;
 > +
 > +	if (d->gc)
 > +		return -EBUSY;
 > +
 > +	if (d->revmap_type != IRQ_DOMAIN_MAP_LINEAR)
 > +		return -EINVAL;
 > +
 > +	numchips = d->revmap_data.linear.size / irqs_per_chip;
 > +	if (!numchips)
 > +		return -EINVAL;
 > +
 > +	sz = sizeof(*gc) + num_ct * sizeof(struct irq_chip_type);
 > +	sz *= numchips;
 > +	sz += sizeof(*dgc);
 > +
 > +	dgc = kzalloc(sz, GFP_KERNEL);
 > +	if (!dgc)
 > +		return -ENOMEM;
 > +	dgc->irqs_per_chip = irqs_per_chip;
 > +	dgc->num_chips = numchips;
 > +	dgc->irq_flags_to_set = set;
 > +	dgc->irq_flags_to_clear = clr;
 > +	dgc->gc_flags = gcflags;
 > +	gc = dgc->gc;
 > +
 > +	for (i = 0; i < numchips; i++, gc++) {

The memory you allocated for gc, num_ct * ct, and dgc doesn't allow
to increment through gc. gc is struct irq_chip_generic * but next
gc is at sizeof(*gc) + num_ct * sizeof(struct irq_chip_type).
This also affects indexing dgc->gc later.

I chose to fix it by having an index helper but that first maps
dgc-gc to unsigned char * and then adds the correct offset. Not
nice but it works. Maybe having real array of ptr to gc is more
intuitive here even if we will have to have split kzallocs.

 > +		irq_init_generic_chip(gc, name, num_ct, i * irqs_per_chip,
 > +				      NULL, handler);

irq_init_generic_chip does not take care of initalizing ct
mask_cache ptr. This should be done here.

 > +		gc->domain = d;
 > +		raw_spin_lock_irqsave(&gc_lock, flags);
 > +		list_add_tail(&gc->list, &gc_list);
 > +		raw_spin_unlock_irqrestore(&gc_lock, flags);
 > +	}
 > +	d->gc = dgc;

Moving this assignment above the for loop allows to get
gc by index as indexing helper relies on domain, not domain
generic chip.

You want me to prepare patches for the above? Maybe you can
split your RFC into 1-6 and 7-8. Then you can have 1-6 applied
independently of irq_domain_generic_chip stuff.

Thanks for the RFC again!

Sebastian
Thomas Gleixner May 4, 2013, 8:04 a.m. UTC | #4
On Sat, 4 May 2013, Sebastian Hesselbarth wrote:

> On 05/03/2013 11:50 PM, Thomas Gleixner wrote:
> > Provide infrastructure for irq chip implementations which work on
> > linear irq domains.
> 
> Thomas,
> 
> I am happy that I put you into rant mode. It took me little more
> than an hour to read through your patches, prepare orion irqchip
> driver on top of them and finally got it working.

Cool.
 
> Anyway, I found some more issues.

That was expected. :)

> > +	for (i = 0; i < numchips; i++, gc++) {
> 
> The memory you allocated for gc, num_ct * ct, and dgc doesn't allow
> to increment through gc. gc is struct irq_chip_generic * but next
> gc is at sizeof(*gc) + num_ct * sizeof(struct irq_chip_type).
> This also affects indexing dgc->gc later.

Indeed.
 
> I chose to fix it by having an index helper but that first maps
> dgc-gc to unsigned char * and then adds the correct offset. Not

void * is the preferred over uchar *

> nice but it works. Maybe having real array of ptr to gc is more
> intuitive here even if we will have to have split kzallocs.

No, you still can have a single kzalloc. It's just a matter of setting
the pointers correctly.

> > +		irq_init_generic_chip(gc, name, num_ct, i * irqs_per_chip,
> > +				      NULL, handler);
> 
> irq_init_generic_chip does not take care of initalizing ct
> mask_cache ptr. This should be done here.

Right.
 
> > +		gc->domain = d;
> > +		raw_spin_lock_irqsave(&gc_lock, flags);
> > +		list_add_tail(&gc->list, &gc_list);
> > +		raw_spin_unlock_irqrestore(&gc_lock, flags);
> > +	}
> > +	d->gc = dgc;
> 
> Moving this assignment above the for loop allows to get
> gc by index as indexing helper relies on domain, not domain
> generic chip.
> 
> You want me to prepare patches for the above? Maybe you can

That'd be nice.

> split your RFC into 1-6 and 7-8. Then you can have 1-6 applied
> independently of irq_domain_generic_chip stuff.

> Thanks for the RFC again!

Welcome. Have fun!

	 tglx
diff mbox

Patch

Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -678,6 +678,8 @@  struct irq_chip_type {
  * @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
+ * @installed:		bitfield to denote installed interrupts
+ * @domain:		irq domain pointer
  * @list:		List head for keeping track of instances
  * @chip_types:		Array of interrupt irq_chip_types
  *
@@ -699,6 +701,8 @@  struct irq_chip_generic {
 	u32			wake_active;
 	unsigned int		num_ct;
 	void			*private;
+	unsigned long		installed;
+	struct irq_domain	*domain;
 	struct list_head	list;
 	struct irq_chip_type	chip_types[0];
 };
@@ -719,6 +723,24 @@  enum irq_gc_flags {
 	IRQ_GC_NO_MASK			= 1 << 3,
 };
 
+/*
+ * struct irq_domain_chip_generic - Generic irq chip data structure for irq domains
+ * @irqs_per_chip:	Number of interrupts per chip
+ * @num_chips:		Number of chips
+ * @irq_flags_to_set:	IRQ* flags to set on irq setup
+ * @irq_flags_to_clear:	IRQ* flags to clear on irq setup
+ * @gc_flags:		Generic chip specific setup flags
+ * @gc:			Array of generic interrupt chips
+ */
+struct irq_domain_chip_generic {
+	unsigned int		irqs_per_chip;
+	unsigned int		num_chips;
+	unsigned int		irq_flags_to_clear;
+	unsigned int		irq_flags_to_set;
+	enum irq_gc_flags	gc_flags;
+	struct irq_chip_generic	gc[0];
+};
+
 /* Generic chip callback functions */
 void irq_gc_noop(struct irq_data *d);
 void irq_gc_mask_disable_reg(struct irq_data *d);
@@ -742,6 +764,14 @@  int irq_setup_alt_chip(struct irq_data *
 void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set);
 
+struct irq_chip_generic *irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq);
+int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				   int num_ct, const char *name,
+				   irq_flow_handler_t handler,
+				   unsigned int clr, unsigned int set,
+				   enum irq_gc_flags flags);
+
+
 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);
Index: linux-2.6/include/linux/irqdomain.h
===================================================================
--- linux-2.6.orig/include/linux/irqdomain.h
+++ linux-2.6/include/linux/irqdomain.h
@@ -66,6 +66,10 @@  struct irq_domain_ops {
 		     unsigned long *out_hwirq, unsigned int *out_type);
 };
 
+extern struct irq_domain_ops irq_generic_chip_ops;
+
+struct irq_domain_chip_generic;
+
 /**
  * struct irq_domain - Hardware interrupt number translation object
  * @link: Element in global irq_domain list.
@@ -109,8 +113,16 @@  struct irq_domain {
 
 	/* Optional device node pointer */
 	struct device_node *of_node;
+	/* Optional pointer to generic interrupt chips */
+	struct irq_domain_chip_generic *gc;
 };
 
+#define IRQ_DOMAIN_MAP_LEGACY 0 /* driver allocated fixed range of irqs.
+				 * ie. legacy 8259, gets irqs 1..15 */
+#define IRQ_DOMAIN_MAP_NOMAP 1 /* no fast reverse mapping */
+#define IRQ_DOMAIN_MAP_LINEAR 2 /* linear map of interrupts */
+#define IRQ_DOMAIN_MAP_TREE 3 /* radix tree */
+
 #ifdef CONFIG_IRQ_DOMAIN
 struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 					 unsigned int size,
Index: linux-2.6/kernel/irq/generic-chip.c
===================================================================
--- linux-2.6.orig/kernel/irq/generic-chip.c
+++ linux-2.6/kernel/irq/generic-chip.c
@@ -7,6 +7,7 @@ 
 #include <linux/irq.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
 #include <linux/syscore_ops.h>
@@ -226,6 +227,84 @@  irq_alloc_generic_chip(const char *name,
 }
 EXPORT_SYMBOL_GPL(irq_alloc_generic_chip);
 
+/**
+ * irq_alloc_domain_generic_chip - Allocate generic chips for an irq domain
+ * @d:			irq domain for which to allocate chips
+ * @irqs_per_chip:	Number of interrupts each chip handles
+ * @num_ct:		Number of irq_chip_type instances associated with this
+ * @name:		Name of the irq chip
+ * @handler:		Default flow handler associated with these chips
+ * @clr:		IRQ_* bits to clear in the mapping function
+ * @set:		IRQ_* bits to set in the mapping function
+ */
+int irq_alloc_domain_generic_chips(struct irq_domain *d, int irqs_per_chip,
+				   int num_ct, const char *name,
+				   irq_flow_handler_t handler,
+				   unsigned int clr, unsigned int set,
+				   enum irq_gc_flags gcflags)
+{
+	struct irq_domain_chip_generic *dgc;
+	struct irq_chip_generic *gc;
+	int numchips, sz, i;
+	unsigned long flags;
+
+	if (d->gc)
+		return -EBUSY;
+
+	if (d->revmap_type != IRQ_DOMAIN_MAP_LINEAR)
+		return -EINVAL;
+
+	numchips = d->revmap_data.linear.size / irqs_per_chip;
+	if (!numchips)
+		return -EINVAL;
+
+	sz = sizeof(*gc) + num_ct * sizeof(struct irq_chip_type);
+	sz *= numchips;
+	sz += sizeof(*dgc);
+
+	dgc = kzalloc(sz, GFP_KERNEL);
+	if (!dgc)
+		return -ENOMEM;
+	dgc->irqs_per_chip = irqs_per_chip;
+	dgc->num_chips = numchips;
+	dgc->irq_flags_to_set = set;
+	dgc->irq_flags_to_clear = clr;
+	dgc->gc_flags = gcflags;
+	gc = dgc->gc;
+
+	for (i = 0; i < numchips; i++, gc++) {
+		irq_init_generic_chip(gc, name, num_ct, i * irqs_per_chip,
+				      NULL, handler);
+		gc->domain = d;
+		raw_spin_lock_irqsave(&gc_lock, flags);
+		list_add_tail(&gc->list, &gc_list);
+		raw_spin_unlock_irqrestore(&gc_lock, flags);
+	}
+	d->gc = dgc;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_alloc_domain_generic_chips);
+
+/**
+ * irq_get_domain_generic_chip - Get a pointer to the generic chip of a hw_irq
+ * @d:			irq domain pointer
+ * @hw_irq:		Hardware interrupt number
+ */
+struct irq_chip_generic *
+irq_get_domain_generic_chip(struct irq_domain *d, unsigned int hw_irq)
+{
+	struct irq_domain_chip_generic *dgc = d->gc;
+	int idx;
+
+	if (!dgc)
+		return NULL;
+	idx = hw_irq / dgc->irqs_per_chip;
+	if (idx >= dgc->num_chips)
+		return NULL;
+	return &dgc->gc[idx];
+}
+EXPORT_SYMBOL_GPL(irq_get_domain_generic_chip);
+
 /*
  * Separate lockdep class for interrupt chip which can nest irq_desc
  * lock.
@@ -233,6 +312,64 @@  EXPORT_SYMBOL_GPL(irq_alloc_generic_chip
 static struct lock_class_key irq_nested_lock_class;
 
 /**
+ * irq_map_generic_chip - Map a generic chip for an irq domain
+ */
+static int irq_map_generic_chip(struct irq_domain *d, unsigned int virq,
+			 irq_hw_number_t hw_irq)
+{
+	struct irq_data *data = irq_get_irq_data(virq);
+	struct irq_domain_chip_generic *dgc = d->gc;
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int idx;
+
+	if (!d->gc)
+		return -ENODEV;
+
+	idx = hw_irq / dgc->irqs_per_chip;
+	if (idx >= dgc->num_chips)
+		return -EINVAL;
+	gc = &dgc->gc[idx];
+
+	idx = hw_irq % dgc->irqs_per_chip;
+
+	if (test_bit(idx, &gc->installed))
+		return -EBUSY;
+
+	set_bit(idx, &gc->installed);
+	ct = gc->chip_types;
+	chip = &ct->chip;
+
+	/* Init mask cache ? */
+	if (dgc->gc_flags & IRQ_GC_INIT_MASK_CACHE) {
+		raw_spin_lock_irqsave(&gc->lock, flags);
+		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
+		raw_spin_unlock_irqrestore(&gc->lock, flags);
+	}
+
+	if (dgc->gc_flags & IRQ_GC_INIT_NESTED_LOCK)
+		irq_set_lockdep_class(virq, &irq_nested_lock_class);
+
+	if (chip->irq_calc_mask)
+		chip->irq_calc_mask(data);
+	else
+		data->mask = 1 << idx;
+
+	irq_set_chip_and_handler(virq, chip, ct->handler);
+	irq_set_chip_data(virq, gc);
+	irq_modify_status(virq, dgc->irq_flags_to_clear, dgc->irq_flags_to_set);
+	return 0;
+}
+
+struct irq_domain_ops irq_generic_chip_ops = {
+	.map = irq_map_generic_chip,
+	.xlate = irq_domain_xlate_onecell,
+};
+EXPORT_SYMBOL_GPL(irq_generic_chip_ops);
+
+/**
  * 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
@@ -345,6 +482,24 @@  void irq_remove_generic_chip(struct irq_
 }
 EXPORT_SYMBOL_GPL(irq_remove_generic_chip);
 
+static struct irq_data *irq_gc_get_irq_data(struct irq_chip_generic *gc)
+{
+	unsigned int virq;
+
+	if (!gc->domain)
+		return irq_get_irq_data(gc->irq_base);
+
+	/*
+	 * We don't know which of the irqs has been actually
+	 * installed. Use the first one.
+	 */
+	if (!gc->installed)
+		return NULL;
+
+	virq = irq_find_mapping(gc->domain, gc->irq_base + __ffs(gc->installed));
+	return virq ? irq_get_irq_data(virq) : NULL;
+}
+
 #ifdef CONFIG_PM
 static int irq_gc_suspend(void)
 {
@@ -353,8 +508,12 @@  static int irq_gc_suspend(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
-		if (ct->chip.irq_suspend)
-			ct->chip.irq_suspend(irq_get_irq_data(gc->irq_base));
+		if (ct->chip.irq_suspend) {
+			struct irq_data *data = irq_gc_get_irq_data(gc);
+
+			if (data)
+				ct->chip.irq_suspend(data);
+		}
 	}
 	return 0;
 }
@@ -366,8 +525,12 @@  static void irq_gc_resume(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
-		if (ct->chip.irq_resume)
-			ct->chip.irq_resume(irq_get_irq_data(gc->irq_base));
+		if (ct->chip.irq_resume) {
+			struct irq_data *data = irq_gc_get_irq_data(gc);
+
+			if (data)
+				ct->chip.irq_resume(data);
+		}
 	}
 }
 #else
@@ -382,8 +545,12 @@  static void irq_gc_shutdown(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
-		if (ct->chip.irq_pm_shutdown)
-			ct->chip.irq_pm_shutdown(irq_get_irq_data(gc->irq_base));
+		if (ct->chip.irq_pm_shutdown) {
+			struct irq_data *data = irq_gc_get_irq_data(gc);
+
+			if (data)
+				ct->chip.irq_pm_shutdown(data);
+		}
 	}
 }
 
Index: linux-2.6/kernel/irq/irqdomain.c
===================================================================
--- linux-2.6.orig/kernel/irq/irqdomain.c
+++ linux-2.6/kernel/irq/irqdomain.c
@@ -16,12 +16,6 @@ 
 #include <linux/smp.h>
 #include <linux/fs.h>
 
-#define IRQ_DOMAIN_MAP_LEGACY 0 /* driver allocated fixed range of irqs.
-				 * ie. legacy 8259, gets irqs 1..15 */
-#define IRQ_DOMAIN_MAP_NOMAP 1 /* no fast reverse mapping */
-#define IRQ_DOMAIN_MAP_LINEAR 2 /* linear map of interrupts */
-#define IRQ_DOMAIN_MAP_TREE 3 /* radix tree */
-
 static LIST_HEAD(irq_domain_list);
 static DEFINE_MUTEX(irq_domain_mutex);