* [PATCH] irq: convert generic-chip to use irq_domain @ 2011-12-09 22:44 Rob Herring 2011-12-11 16:00 ` Shawn Guo 0 siblings, 1 reply; 6+ messages in thread From: Rob Herring @ 2011-12-09 22:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] irq: convert generic-chip to use irq_domain 2011-12-09 22:44 [PATCH] irq: convert generic-chip to use irq_domain Rob Herring @ 2011-12-11 16:00 ` Shawn Guo 2011-12-12 1:11 ` Rob Herring 0 siblings, 1 reply; 6+ messages in thread From: Shawn Guo @ 2011-12-11 16:00 UTC (permalink / raw) To: linux-arm-kernel 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/ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irq: convert generic-chip to use irq_domain 2011-12-11 16:00 ` Shawn Guo @ 2011-12-12 1:11 ` Rob Herring 2011-12-12 2:52 ` Rob Herring 2011-12-12 9:49 ` Shawn Guo 0 siblings, 2 replies; 6+ messages in thread From: Rob Herring @ 2011-12-12 1:11 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irq: convert generic-chip to use irq_domain 2011-12-12 1:11 ` Rob Herring @ 2011-12-12 2:52 ` Rob Herring 2011-12-12 9:54 ` Shawn Guo 2011-12-12 9:49 ` Shawn Guo 1 sibling, 1 reply; 6+ messages in thread From: Rob Herring @ 2011-12-12 2:52 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] irq: convert generic-chip to use irq_domain 2011-12-12 2:52 ` Rob Herring @ 2011-12-12 9:54 ` Shawn Guo 0 siblings, 0 replies; 6+ messages in thread From: Shawn Guo @ 2011-12-12 9:54 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] irq: convert generic-chip to use irq_domain 2011-12-12 1:11 ` Rob Herring 2011-12-12 2:52 ` Rob Herring @ 2011-12-12 9:49 ` Shawn Guo 1 sibling, 0 replies; 6+ messages in thread From: Shawn Guo @ 2011-12-12 9:49 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-12 9:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-09 22:44 [PATCH] irq: convert generic-chip to use irq_domain Rob Herring 2011-12-11 16:00 ` Shawn Guo 2011-12-12 1:11 ` Rob Herring 2011-12-12 2:52 ` Rob Herring 2011-12-12 9:54 ` Shawn Guo 2011-12-12 9:49 ` Shawn Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).