[PATCH] irq: convert generic-chip to use irq_domain

[PATCH] irq: convert generic-chip to use irq_domain

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add irq domain support to irq generic-chip. This enables users of
generic-chip to support dynamic irq assignment needed for DT interrupt
binding. Users must be converted to use irq_data.hwirq for determining
local interrupt numbers rather than using the Linux irq number.

irq_base is kept for now as there are a few users of it. Once they
are converted to use the irq domain, it can be removed.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 include/linux/irq.h       |    2 +-
 kernel/irq/Kconfig        |    1 +
 kernel/irq/generic-chip.c |   54 +++++++++++++++++++++++++++-----------------
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index bff29c5..9ba8a30 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -664,7 +664,7 @@ struct irq_chip_generic {
 	raw_spinlock_t		lock;
 	void __iomem		*reg_base;
 	unsigned int		irq_base;
-	unsigned int		irq_cnt;
+	struct irq_domain	*domain;
 	u32			mask_cache;
 	u32			type_cache;
 	u32			polarity_cache;
diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 5a38bf4..861f2fe 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -51,6 +51,7 @@ config IRQ_EDGE_EOI_HANDLER
 # Generic configurable interrupt chip implementation
 config GENERIC_IRQ_CHIP
        bool
+       select IRQ_DOMAIN
 
 # Generic irq_domain hw <--> linux irq number translation
 config IRQ_DOMAIN
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index c89295a..48eeb29 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -5,6 +5,7 @@
  */
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/interrupt.h>
@@ -39,7 +40,7 @@ void irq_gc_noop(struct irq_data *d)
 void irq_gc_mask_disable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable);
@@ -57,7 +58,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
 void irq_gc_mask_set_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	gc->mask_cache |= mask;
@@ -75,7 +76,7 @@ void irq_gc_mask_set_bit(struct irq_data *d)
 void irq_gc_mask_clr_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	gc->mask_cache &= ~mask;
@@ -93,7 +94,7 @@ void irq_gc_mask_clr_bit(struct irq_data *d)
 void irq_gc_unmask_enable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable);
@@ -108,7 +109,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
 void irq_gc_ack_set_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
@@ -122,7 +123,7 @@ void irq_gc_ack_set_bit(struct irq_data *d)
 void irq_gc_ack_clr_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = ~(1 << (d->irq - gc->irq_base));
+	u32 mask = ~(1 << d->hwirq);
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
@@ -136,7 +137,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask);
@@ -151,7 +152,7 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 void irq_gc_eoi(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi);
@@ -169,7 +170,7 @@ void irq_gc_eoi(struct irq_data *d)
 int irq_gc_set_wake(struct irq_data *d, unsigned int on)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = 1 << d->hwirq;
 
 	if (!(mask & gc->wake_enabled))
 		return -EINVAL;
@@ -201,10 +202,13 @@ irq_alloc_generic_chip(const char *name, int num_ct, unsigned int irq_base,
 	struct irq_chip_generic *gc;
 	unsigned long sz = sizeof(*gc) + num_ct * sizeof(struct irq_chip_type);
 
-	gc = kzalloc(sz, GFP_KERNEL);
+	gc = kzalloc(sz + sizeof(struct irq_domain), GFP_KERNEL);
 	if (gc) {
 		raw_spin_lock_init(&gc->lock);
 		gc->num_ct = num_ct;
+		gc->domain = (void *)gc + sz;
+		gc->domain->irq_base = irq_base;
+		gc->domain->ops = &irq_domain_simple_ops;
 		gc->irq_base = irq_base;
 		gc->reg_base = reg_base;
 		gc->chip_types->chip.name = name;
@@ -237,7 +241,7 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			    unsigned int set)
 {
 	struct irq_chip_type *ct = gc->chip_types;
-	unsigned int i;
+	unsigned int i, irq;
 
 	raw_spin_lock(&gc_lock);
 	list_add_tail(&gc->list, &gc_list);
@@ -247,18 +251,26 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
 	if (flags & IRQ_GC_INIT_MASK_CACHE)
 		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
 
-	for (i = gc->irq_base; msk; msk >>= 1, i++) {
+	gc->domain->nr_irq = fls(msk);
+	irq = irq_alloc_descs(gc->domain->irq_base, 1, gc->domain->nr_irq,
+			      numa_node_id());
+	if (irq > 0)
+		gc->domain->irq_base = irq;
+
+	irq_domain_add(gc->domain);
+
+	for (i = 0; msk; msk >>= 1, i++) {
 		if (!(msk & 0x01))
 			continue;
 
+		irq = irq_domain_to_irq(gc->domain, i);
 		if (flags & IRQ_GC_INIT_NESTED_LOCK)
-			irq_set_lockdep_class(i, &irq_nested_lock_class);
+			irq_set_lockdep_class(irq, &irq_nested_lock_class);
 
-		irq_set_chip_and_handler(i, &ct->chip, ct->handler);
-		irq_set_chip_data(i, gc);
-		irq_modify_status(i, clr, set);
+		irq_set_chip_and_handler(irq, &ct->chip, ct->handler);
+		irq_set_chip_data(irq, gc);
+		irq_modify_status(irq, clr, set);
 	}
-	gc->irq_cnt = i - gc->irq_base;
 }
 EXPORT_SYMBOL_GPL(irq_setup_generic_chip);
 
@@ -298,7 +310,7 @@ EXPORT_SYMBOL_GPL(irq_setup_alt_chip);
 void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
 			     unsigned int clr, unsigned int set)
 {
-	unsigned int i = gc->irq_base;
+	unsigned int i = irq_domain_to_irq(gc->domain, 0);
 
 	raw_spin_lock(&gc_lock);
 	list_del(&gc->list);
@@ -326,7 +338,7 @@ static int irq_gc_suspend(void)
 		struct irq_chip_type *ct = gc->chip_types;
 
 		if (ct->chip.irq_suspend)
-			ct->chip.irq_suspend(irq_get_irq_data(gc->irq_base));
+			ct->chip.irq_suspend(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
 	}
 	return 0;
 }
@@ -339,7 +351,7 @@ static void irq_gc_resume(void)
 		struct irq_chip_type *ct = gc->chip_types;
 
 		if (ct->chip.irq_resume)
-			ct->chip.irq_resume(irq_get_irq_data(gc->irq_base));
+			ct->chip.irq_resume(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
 	}
 }
 #else
@@ -355,7 +367,7 @@ static void irq_gc_shutdown(void)
 		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));
+			ct->chip.irq_pm_shutdown(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
 	}
 }
 
-- 
1.7.5.4

[PATCH] irq: convert generic-chip to use irq_domain

[Shawn Guo]

Hi Rob,

I installed the patch and played it a little bit, and I some feedback
below.

* It breaks some existing irq_chip_generic users like imx5, which
  is currently numbering irq from 0.  For such users, irq_alloc_descs()
  will fail because it's asked to search irq #0 starting from irq #1.
  Yes, I know numbering irq from 0 is a bad idea and really needs to
  get fixed.  But is it your intention to break such users and force
  them get fixed with this patch?

* I thought your patch is taking care of all irqdomain stuff inside
  irq_chip_generic in a way transparent to its users.  But it really
  took me some time to figure out that users still need to populate
  the .of_node for irq_domain encapsulated in irq_chip_generic.

* If I understand irq_chip_generic correctly, it only supports up to
  32 irqs in a chip.  The imx5 tzic supports 128 interrupt lines, and
  the current driver implements it as 4 generic irq chips.  But from
  hardware and device tree point of view, it's really just one
  controller, so we have only one tzic node in dts, and hence we only
  have the same one .of_node for 4 irq domains.  I'm afraid such
  implementation will break irq_create_of_mapping()?
  (For gpio interrupt controller, it should fit perfectly.)

Regards,
Shawn

On Fri, Dec 09, 2011 at 04:44:49PM -0600, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Add irq domain support to irq generic-chip. This enables users of
> generic-chip to support dynamic irq assignment needed for DT interrupt
> binding. Users must be converted to use irq_data.hwirq for determining
> local interrupt numbers rather than using the Linux irq number.
> 
> irq_base is kept for now as there are a few users of it. Once they
> are converted to use the irq domain, it can be removed.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  include/linux/irq.h       |    2 +-
>  kernel/irq/Kconfig        |    1 +
>  kernel/irq/generic-chip.c |   54 +++++++++++++++++++++++++++-----------------
>  3 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index bff29c5..9ba8a30 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -664,7 +664,7 @@ struct irq_chip_generic {
>  	raw_spinlock_t		lock;
>  	void __iomem		*reg_base;
>  	unsigned int		irq_base;
> -	unsigned int		irq_cnt;
> +	struct irq_domain	*domain;
>  	u32			mask_cache;
>  	u32			type_cache;
>  	u32			polarity_cache;
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index 5a38bf4..861f2fe 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -51,6 +51,7 @@ config IRQ_EDGE_EOI_HANDLER
>  # Generic configurable interrupt chip implementation
>  config GENERIC_IRQ_CHIP
>         bool
> +       select IRQ_DOMAIN
>  
>  # Generic irq_domain hw <--> linux irq number translation
>  config IRQ_DOMAIN
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index c89295a..48eeb29 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -5,6 +5,7 @@
>   */
>  #include <linux/io.h>
>  #include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/interrupt.h>
> @@ -39,7 +40,7 @@ void irq_gc_noop(struct irq_data *d)
>  void irq_gc_mask_disable_reg(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable);
> @@ -57,7 +58,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
>  void irq_gc_mask_set_bit(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	gc->mask_cache |= mask;
> @@ -75,7 +76,7 @@ void irq_gc_mask_set_bit(struct irq_data *d)
>  void irq_gc_mask_clr_bit(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	gc->mask_cache &= ~mask;
> @@ -93,7 +94,7 @@ void irq_gc_mask_clr_bit(struct irq_data *d)
>  void irq_gc_unmask_enable_reg(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable);
> @@ -108,7 +109,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
>  void irq_gc_ack_set_bit(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
> @@ -122,7 +123,7 @@ void irq_gc_ack_set_bit(struct irq_data *d)
>  void irq_gc_ack_clr_bit(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = ~(1 << (d->irq - gc->irq_base));
> +	u32 mask = ~(1 << d->hwirq);
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
> @@ -136,7 +137,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
>  void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask);
> @@ -151,7 +152,7 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
>  void irq_gc_eoi(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	irq_gc_lock(gc);
>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi);
> @@ -169,7 +170,7 @@ void irq_gc_eoi(struct irq_data *d)
>  int irq_gc_set_wake(struct irq_data *d, unsigned int on)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	u32 mask = 1 << (d->irq - gc->irq_base);
> +	u32 mask = 1 << d->hwirq;
>  
>  	if (!(mask & gc->wake_enabled))
>  		return -EINVAL;
> @@ -201,10 +202,13 @@ irq_alloc_generic_chip(const char *name, int num_ct, unsigned int irq_base,
>  	struct irq_chip_generic *gc;
>  	unsigned long sz = sizeof(*gc) + num_ct * sizeof(struct irq_chip_type);
>  
> -	gc = kzalloc(sz, GFP_KERNEL);
> +	gc = kzalloc(sz + sizeof(struct irq_domain), GFP_KERNEL);
>  	if (gc) {
>  		raw_spin_lock_init(&gc->lock);
>  		gc->num_ct = num_ct;
> +		gc->domain = (void *)gc + sz;
> +		gc->domain->irq_base = irq_base;
> +		gc->domain->ops = &irq_domain_simple_ops;
>  		gc->irq_base = irq_base;
>  		gc->reg_base = reg_base;
>  		gc->chip_types->chip.name = name;
> @@ -237,7 +241,7 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
>  			    unsigned int set)
>  {
>  	struct irq_chip_type *ct = gc->chip_types;
> -	unsigned int i;
> +	unsigned int i, irq;
>  
>  	raw_spin_lock(&gc_lock);
>  	list_add_tail(&gc->list, &gc_list);
> @@ -247,18 +251,26 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
>  	if (flags & IRQ_GC_INIT_MASK_CACHE)
>  		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
>  
> -	for (i = gc->irq_base; msk; msk >>= 1, i++) {
> +	gc->domain->nr_irq = fls(msk);
> +	irq = irq_alloc_descs(gc->domain->irq_base, 1, gc->domain->nr_irq,
> +			      numa_node_id());
> +	if (irq > 0)
> +		gc->domain->irq_base = irq;
> +
> +	irq_domain_add(gc->domain);
> +
> +	for (i = 0; msk; msk >>= 1, i++) {
>  		if (!(msk & 0x01))
>  			continue;
>  
> +		irq = irq_domain_to_irq(gc->domain, i);
>  		if (flags & IRQ_GC_INIT_NESTED_LOCK)
> -			irq_set_lockdep_class(i, &irq_nested_lock_class);
> +			irq_set_lockdep_class(irq, &irq_nested_lock_class);
>  
> -		irq_set_chip_and_handler(i, &ct->chip, ct->handler);
> -		irq_set_chip_data(i, gc);
> -		irq_modify_status(i, clr, set);
> +		irq_set_chip_and_handler(irq, &ct->chip, ct->handler);
> +		irq_set_chip_data(irq, gc);
> +		irq_modify_status(irq, clr, set);
>  	}
> -	gc->irq_cnt = i - gc->irq_base;
>  }
>  EXPORT_SYMBOL_GPL(irq_setup_generic_chip);
>  
> @@ -298,7 +310,7 @@ EXPORT_SYMBOL_GPL(irq_setup_alt_chip);
>  void irq_remove_generic_chip(struct irq_chip_generic *gc, u32 msk,
>  			     unsigned int clr, unsigned int set)
>  {
> -	unsigned int i = gc->irq_base;
> +	unsigned int i = irq_domain_to_irq(gc->domain, 0);
>  
>  	raw_spin_lock(&gc_lock);
>  	list_del(&gc->list);
> @@ -326,7 +338,7 @@ static int irq_gc_suspend(void)
>  		struct irq_chip_type *ct = gc->chip_types;
>  
>  		if (ct->chip.irq_suspend)
> -			ct->chip.irq_suspend(irq_get_irq_data(gc->irq_base));
> +			ct->chip.irq_suspend(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
>  	}
>  	return 0;
>  }
> @@ -339,7 +351,7 @@ static void irq_gc_resume(void)
>  		struct irq_chip_type *ct = gc->chip_types;
>  
>  		if (ct->chip.irq_resume)
> -			ct->chip.irq_resume(irq_get_irq_data(gc->irq_base));
> +			ct->chip.irq_resume(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
>  	}
>  }
>  #else
> @@ -355,7 +367,7 @@ static void irq_gc_shutdown(void)
>  		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));
> +			ct->chip.irq_pm_shutdown(irq_get_irq_data(irq_domain_to_irq(gc->domain, 0)));
>  	}
>  }
>  
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH] irq: convert generic-chip to use irq_domain

[Rob Herring]

Shawn,

On 12/11/2011 10:00 AM, Shawn Guo wrote:
> Hi Rob,
> 
> I installed the patch and played it a little bit, and I some feedback
> below.

Thanks!

> * It breaks some existing irq_chip_generic users like imx5, which
>   is currently numbering irq from 0.  For such users, irq_alloc_descs()
>   will fail because it's asked to search irq #0 starting from irq #1.
>   Yes, I know numbering irq from 0 is a bad idea and really needs to
>   get fixed.  But is it your intention to break such users and force
>   them get fixed with this patch?

I was trying not to break things and allow the existing irq base.

My problem is irq 0-15 are unallocated, 16-159 are the GIC. So when I
try to allocate 8 irqdescs for gpio, I don't really want irq_alloc_descs
to pick 0-15. Ideally we would just globally reserve 0-15 on ARM, but
this will cause problems. So starting at 1 was at least slightly better
than 0.

I think I'll just skip irq_alloc_descs call when the irq_base is >= 0.

> * I thought your patch is taking care of all irqdomain stuff inside
>   irq_chip_generic in a way transparent to its users.  But it really
>   took me some time to figure out that users still need to populate
>   the .of_node for irq_domain encapsulated in irq_chip_generic.

Sorry about that. It would have been evident in my follow on patches for
pl061 gpio.

> * If I understand irq_chip_generic correctly, it only supports up to
>   32 irqs in a chip.  The imx5 tzic supports 128 interrupt lines, and
>   the current driver implements it as 4 generic irq chips.  But from
>   hardware and device tree point of view, it's really just one
>   controller, so we have only one tzic node in dts, and hence we only
>   have the same one .of_node for 4 irq domains.  I'm afraid such
>   implementation will break irq_create_of_mapping()?
>   (For gpio interrupt controller, it should fit perfectly.)
> 

Yeah, that's a problem that needs addressing. You can override the
domain ops with a custom dt_translate function. Perhaps this can be done
generically:

	struct irq_chip_generic *gc;

	list_for_each_entry(gc, &gc_list, list) {
		if (gc->domain != d || d->of_node != controller)
			continue;
		
		// Got a match, fill in data.

		return 0;
	}

	return -EINVAL;


There's still the problem that hwirq is not going to be 0-127 for the
AVIC as you probably want. For that you can initialize hwirq_base for
each domain. The AVIC code is still free to override parts of the domain
setup.

Rob

[PATCH] irq: convert generic-chip to use irq_domain

[Rob Herring]

On 12/11/2011 07:11 PM, Rob Herring wrote:
> Shawn,
> 
> On 12/11/2011 10:00 AM, Shawn Guo wrote:
>> Hi Rob,
>>
>> I installed the patch and played it a little bit, and I some feedback
>> below.
> 
> Thanks!
> 
>> * It breaks some existing irq_chip_generic users like imx5, which
>>   is currently numbering irq from 0.  For such users, irq_alloc_descs()
>>   will fail because it's asked to search irq #0 starting from irq #1.
>>   Yes, I know numbering irq from 0 is a bad idea and really needs to
>>   get fixed.  But is it your intention to break such users and force
>>   them get fixed with this patch?
> 
> I was trying not to break things and allow the existing irq base.
> 
> My problem is irq 0-15 are unallocated, 16-159 are the GIC. So when I
> try to allocate 8 irqdescs for gpio, I don't really want irq_alloc_descs
> to pick 0-15. Ideally we would just globally reserve 0-15 on ARM, but
> this will cause problems. So starting at 1 was at least slightly better
> than 0.
> 
> I think I'll just skip irq_alloc_descs call when the irq_base is >= 0.
> 
>> * I thought your patch is taking care of all irqdomain stuff inside
>>   irq_chip_generic in a way transparent to its users.  But it really
>>   took me some time to figure out that users still need to populate
>>   the .of_node for irq_domain encapsulated in irq_chip_generic.
> 
> Sorry about that. It would have been evident in my follow on patches for
> pl061 gpio.
> 
>> * If I understand irq_chip_generic correctly, it only supports up to
>>   32 irqs in a chip.  The imx5 tzic supports 128 interrupt lines, and
>>   the current driver implements it as 4 generic irq chips.  But from
>>   hardware and device tree point of view, it's really just one
>>   controller, so we have only one tzic node in dts, and hence we only
>>   have the same one .of_node for 4 irq domains.  I'm afraid such
>>   implementation will break irq_create_of_mapping()?
>>   (For gpio interrupt controller, it should fit perfectly.)
>>
> 
> Yeah, that's a problem that needs addressing. You can override the
> domain ops with a custom dt_translate function. Perhaps this can be done
> generically:
> 
> 	struct irq_chip_generic *gc;
> 
> 	list_for_each_entry(gc, &gc_list, list) {
> 		if (gc->domain != d || d->of_node != controller)
> 			continue;
> 		
> 		// Got a match, fill in data.
> 
> 		return 0;
> 	}
> 
> 	return -EINVAL;

Actually, there's a much simpler fix:

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 200ce83..3832ac6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -135,6 +135,9 @@ int irq_domain_simple_dt_translate(struct irq_domain *d,
                return -EINVAL;
        if (intsize < 1)
                return -EINVAL;
+       if ((intspec[0] < d->hwirq_base) ||
+           (intspec[0] >= d->hwirq_base + d->nr_irq))
+               return -EINVAL;

        *out_hwirq = intspec[0];
        *out_type = IRQ_TYPE_NONE;


Rob

> 
> There's still the problem that hwirq is not going to be 0-127 for the
> AVIC as you probably want. For that you can initialize hwirq_base for
> each domain. The AVIC code is still free to override parts of the domain
> setup.
> 
> Rob

[PATCH] irq: convert generic-chip to use irq_domain

[Shawn Guo]

On Sun, Dec 11, 2011 at 07:11:22PM -0600, Rob Herring wrote:
> Shawn,
> 
> On 12/11/2011 10:00 AM, Shawn Guo wrote:
> > Hi Rob,
> > 
> > I installed the patch and played it a little bit, and I some feedback
> > below.
> 
> Thanks!
> 
> > * It breaks some existing irq_chip_generic users like imx5, which
> >   is currently numbering irq from 0.  For such users, irq_alloc_descs()
> >   will fail because it's asked to search irq #0 starting from irq #1.
> >   Yes, I know numbering irq from 0 is a bad idea and really needs to
> >   get fixed.  But is it your intention to break such users and force
> >   them get fixed with this patch?
> 
> I was trying not to break things and allow the existing irq base.
> 
> My problem is irq 0-15 are unallocated, 16-159 are the GIC. So when I
> try to allocate 8 irqdescs for gpio, I don't really want irq_alloc_descs
> to pick 0-15. Ideally we would just globally reserve 0-15 on ARM, but
> this will cause problems. So starting at 1 was at least slightly better
> than 0.
> 
> I think I'll just skip irq_alloc_descs call when the irq_base is >= 0.
> 
Yes, looks good.  But both parameter irq_base of irq_alloc_generic_chip
and member irq_base of 'struct irq_domain' are unsigned int, which
might be a little problem for irq_chip_generic users to pass -1 for
asking irq_chip_generic to allocate Linux irq numbers for it.

-- 
Regards,
Shawn

[PATCH] irq: convert generic-chip to use irq_domain

[Shawn Guo]

On Sun, Dec 11, 2011 at 08:52:22PM -0600, Rob Herring wrote:
> Actually, there's a much simpler fix:
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 200ce83..3832ac6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -135,6 +135,9 @@ int irq_domain_simple_dt_translate(struct irq_domain *d,
>                 return -EINVAL;
>         if (intsize < 1)
>                 return -EINVAL;
> +       if ((intspec[0] < d->hwirq_base) ||
> +           (intspec[0] >= d->hwirq_base + d->nr_irq))
> +               return -EINVAL;
> 
>         *out_hwirq = intspec[0];
>         *out_type = IRQ_TYPE_NONE;
> 
Thanks, Rob.  It works for me with some additional changes to your
patch.

8<-------------------------

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 48eeb29..3ac7de4 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -252,14 +252,16 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
                gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);

        gc->domain->nr_irq = fls(msk);
-       irq = irq_alloc_descs(gc->domain->irq_base, 1, gc->domain->nr_irq,
-                             numa_node_id());
-       if (irq > 0)
-               gc->domain->irq_base = irq;
+       if ((int) gc->domain->irq_base < 0) {
+               irq = irq_alloc_descs(gc->domain->irq_base, 1,
+                                     gc->domain->nr_irq, numa_node_id());
+               if (irq > 0)
+                       gc->domain->irq_base = irq;
+       }

        irq_domain_add(gc->domain);

-       for (i = 0; msk; msk >>= 1, i++) {
+       for (i = gc->domain->hwirq_base; msk; msk >>= 1, i++) {
                if (!(msk & 0x01))
                        continue;

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 200ce83..7947252 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -39,7 +39,7 @@ void irq_domain_add(struct irq_domain *domain)
                        return;
                }
                d->domain = domain;
-               d->hwirq = hwirq;
+               d->hwirq = hwirq - d->domain->hwirq_base;
        }

        mutex_lock(&irq_domain_mutex);
@@ -136,6 +136,10 @@ int irq_domain_simple_dt_translate(struct irq_domain *d,
        if (intsize < 1)
                return -EINVAL;

+       if ((intspec[0] < d->hwirq_base) ||
+           (intspec[0] >= d->hwirq_base + d->nr_irq))
+               return -EINVAL;
+
        *out_hwirq = intspec[0];
        *out_type = IRQ_TYPE_NONE;
        if (intsize > 1)

------------------------->8

Regards,
Shawn