| Message ID | 1325860112-22051-1-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | New |
| Headers | show |
On 01/06/2012 03:28 PM, Thierry Reding : > The irq_domain_add() function needs the number of interrupts in the > domain to properly initialize them. In addition the allocated domain > is now returned by the irq_domain_{add,generate}_simple() helpers. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> Hi Thierry, Good to see this patch ;-) Thanks for having done this modification. For AT91 and irqdomain (as far as I can tell) Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Best regards, > --- > arch/arm/mach-at91/board-dt.c | 6 +++++- > arch/arm/mach-imx/imx51-dt.c | 13 +++++++++++-- > arch/arm/mach-imx/imx53-dt.c | 13 +++++++++++-- > arch/arm/mach-imx/mach-imx6q.c | 4 +++- > arch/arm/mach-msm/board-msm8x60.c | 7 +++++-- > arch/arm/mach-omap2/board-generic.c | 7 +++++-- > arch/arm/mach-prima2/irq.c | 4 +++- > arch/arm/mach-versatile/core.c | 10 ++++++++-- > arch/arm/plat-mxc/include/mach/irqs.h | 1 + > arch/arm/plat-mxc/tzic.c | 2 -- > include/linux/irqdomain.h | 27 ++++++++++++++++++++++----- > kernel/irq/irqdomain.c | 30 +++++++++++++++++++----------- > 12 files changed, 93 insertions(+), 31 deletions(-) > > diff --git a/arch/arm/mach-at91/board-dt.c b/arch/arm/mach-at91/board-dt.c > index bb6b434..12f003d 100644 > --- a/arch/arm/mach-at91/board-dt.c > +++ b/arch/arm/mach-at91/board-dt.c > @@ -95,7 +95,11 @@ static const struct of_device_id aic_of_match[] __initconst = { > > static void __init at91_dt_init_irq(void) > { > - irq_domain_generate_simple(aic_of_match, 0xfffff000, 0); > + struct irq_domain *domain; > + > + domain = irq_domain_generate_simple(aic_of_match, 0xfffff000, 0, > + NR_AIC_IRQS); > + WARN_ON(IS_ERR(domain)); > at91_init_irq_default(); > } > > diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c > index e6bad17..72bf94c 100644 > --- a/arch/arm/mach-imx/imx51-dt.c > +++ b/arch/arm/mach-imx/imx51-dt.c > @@ -47,7 +47,12 @@ static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = { > static int __init imx51_tzic_add_irq_domain(struct device_node *np, > struct device_node *interrupt_parent) > { > - irq_domain_add_simple(np, 0); > + struct irq_domain *domain; > + > + domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS); > + if (IS_ERR(domain)) > + return PTR_ERR(domain); > + > return 0; > } > > @@ -55,9 +60,13 @@ static int __init imx51_gpio_add_irq_domain(struct device_node *np, > struct device_node *interrupt_parent) > { > static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; > + struct irq_domain *domain; > > gpio_irq_base -= 32; > - irq_domain_add_simple(np, gpio_irq_base); > + > + domain = irq_domain_add_simple(np, gpio_irq_base, 32); > + if (IS_ERR(domain)) > + return PTR_ERR(domain); > > return 0; > } > diff --git a/arch/arm/mach-imx/imx53-dt.c b/arch/arm/mach-imx/imx53-dt.c > index 05ebb3e..ccae620 100644 > --- a/arch/arm/mach-imx/imx53-dt.c > +++ b/arch/arm/mach-imx/imx53-dt.c > @@ -51,7 +51,12 @@ static const struct of_dev_auxdata imx53_auxdata_lookup[] __initconst = { > static int __init imx53_tzic_add_irq_domain(struct device_node *np, > struct device_node *interrupt_parent) > { > - irq_domain_add_simple(np, 0); > + struct irq_domain *domain; > + > + domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS); > + if (IS_ERR(domain)) > + return PTR_ERR(domain); > + > return 0; > } > > @@ -59,9 +64,13 @@ static int __init imx53_gpio_add_irq_domain(struct device_node *np, > struct device_node *interrupt_parent) > { > static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; > + struct irq_domain *domain; > > gpio_irq_base -= 32; > - irq_domain_add_simple(np, gpio_irq_base); > + > + domain = irq_domain_add_simple(np, gpio_irq_base, 32); > + if (IS_ERR(domain)) > + return PTR_ERR(domain); > > return 0; > } > diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c > index c257281..9ed7812 100644 > --- a/arch/arm/mach-imx/mach-imx6q.c > +++ b/arch/arm/mach-imx/mach-imx6q.c > @@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np, > struct device_node *interrupt_parent) > { > static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; > + struct irq_domain *domain; > > gpio_irq_base -= 32; > - irq_domain_add_simple(np, gpio_irq_base); > + domain = irq_domain_add_simple(np, gpio_irq_base, 32); > + WARN_ON(IS_ERR(domain)); > > return 0; > } > diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c > index 0a11342..a50c7e2 100644 > --- a/arch/arm/mach-msm/board-msm8x60.c > +++ b/arch/arm/mach-msm/board-msm8x60.c > @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) > > node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, > MSM8X60_QGIC_DIST_PHYS); > - if (node) > - irq_domain_add_simple(node, GIC_SPI_START); > + if (node) { > + struct irq_domain *domain = irq_domain_add_simple(node, > + GIC_SPI_START, NR_MSM_IRQS); > + WARN_ON(IS_ERR(domain)); > + } > > if (of_machine_is_compatible("qcom,msm8660-surf")) { > printk(KERN_INFO "Init surf UART registers\n"); > diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c > index d587560..bf67781 100644 > --- a/arch/arm/mach-omap2/board-generic.c > +++ b/arch/arm/mach-omap2/board-generic.c > @@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = { > static void __init omap_generic_init(void) > { > struct device_node *node = of_find_matching_node(NULL, intc_match); > - if (node) > - irq_domain_add_simple(node, 0); > + if (node) { > + struct irq_domain *domain; > + domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS); > + WARN_ON(IS_ERR(domain)); > + } > > omap_sdrc_init(NULL, NULL); > > diff --git a/arch/arm/mach-prima2/irq.c b/arch/arm/mach-prima2/irq.c > index d93ceef..d3c5136 100644 > --- a/arch/arm/mach-prima2/irq.c > +++ b/arch/arm/mach-prima2/irq.c > @@ -58,6 +58,7 @@ static struct of_device_id intc_ids[] = { > > void __init sirfsoc_of_irq_init(void) > { > + struct irq_domain *domain; > struct device_node *np; > > np = of_find_matching_node(NULL, intc_ids); > @@ -68,7 +69,8 @@ void __init sirfsoc_of_irq_init(void) > if (!sirfsoc_intc_base) > panic("unable to map intc cpu registers\n"); > > - irq_domain_add_simple(np, 0); > + domain = irq_domain_add_simple(np, 0, NR_IRQS); > + WARN_ON(IS_ERR(domain)); > > of_node_put(np); > > diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c > index 02b7b93..6c40be4 100644 > --- a/arch/arm/mach-versatile/core.c > +++ b/arch/arm/mach-versatile/core.c > @@ -98,13 +98,19 @@ static const struct of_device_id sic_of_match[] __initconst = { > > void __init versatile_init_irq(void) > { > + struct irq_domain *domain; > + > vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0); > - irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START); > + domain = irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, > + IRQ_VIC_START, IRQ_VIC_END - IRQ_VIC_START + 1); > + WARN_ON(IS_ERR(domain)); > > writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR); > > fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq); > - irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, IRQ_SIC_START); > + domain = irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, > + IRQ_SIC_START, IRQ_SIC_END - IRQ_SIC_START + 1); > + WARN_ON(IS_ERR(domain)); > > /* > * Interrupts on secondary controller from 0 to 8 are routed to > diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h > index fd9efb0..2fda5aa 100644 > --- a/arch/arm/plat-mxc/include/mach/irqs.h > +++ b/arch/arm/plat-mxc/include/mach/irqs.h > @@ -54,6 +54,7 @@ > /* REVISIT: Add IPU irqs on IMX51 */ > > #define NR_IRQS (MXC_IPU_IRQ_START + MX3_IPU_IRQS) > +#define TZIC_NUM_IRQS 128 > > extern int imx_irq_set_priority(unsigned char irq, unsigned char prio); > > diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c > index 98308ec..98b23b8 100644 > --- a/arch/arm/plat-mxc/tzic.c > +++ b/arch/arm/plat-mxc/tzic.c > @@ -50,8 +50,6 @@ > > void __iomem *tzic_base; /* Used as irq controller base in entry-macro.S */ > > -#define TZIC_NUM_IRQS 128 > - > #ifdef CONFIG_FIQ > static int tzic_set_irq_fiq(unsigned int irq, unsigned int type) > { > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index bd4272b..1538d4c 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -15,6 +15,7 @@ > #ifndef _LINUX_IRQDOMAIN_H > #define _LINUX_IRQDOMAIN_H > > +#include <linux/err.h> > #include <linux/irq.h> > #include <linux/mod_devicetable.h> > > @@ -96,12 +97,28 @@ extern struct irq_domain_ops irq_domain_simple_ops; > #endif /* CONFIG_IRQ_DOMAIN */ > > #if defined(CONFIG_IRQ_DOMAIN) && defined(CONFIG_OF_IRQ) > -extern void irq_domain_add_simple(struct device_node *controller, int irq_base); > -extern void irq_domain_generate_simple(const struct of_device_id *match, > - u64 phys_base, unsigned int irq_start); > +extern struct irq_domain *irq_domain_add_simple(struct device_node *controller, > + unsigned int irq_base, > + unsigned int nr_irq); > +extern struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, > + u64 phys_base, > + unsigned int irq_start, > + unsigned int nr_irq); > #else /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */ > -static inline void irq_domain_generate_simple(const struct of_device_id *match, > - u64 phys_base, unsigned int irq_start) { } > +static inline struct irq_domain *irq_domain_add_simple(struct device_node *controller, > + int irq_base, > + unsigned int nr_irq) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > +static inline struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, > + u64 phys_base, > + unsigned int irq_start, > + unsigned int nr_irq) > +{ > + return ERR_PTR(-ENOSYS); > +} > #endif /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */ > > #endif /* _LINUX_IRQDOMAIN_H */ > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 1f9e265..807c44b 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -147,36 +147,44 @@ int irq_domain_simple_dt_translate(struct irq_domain *d, > } > > /** > - * irq_domain_create_simple() - Set up a 'simple' translation range > + * irq_domain_add_simple() - Set up a 'simple' translation range > */ > -void irq_domain_add_simple(struct device_node *controller, int irq_base) > +struct irq_domain *irq_domain_add_simple(struct device_node *controller, > + unsigned int irq_base, > + unsigned int nr_irq) > { > struct irq_domain *domain; > > domain = kzalloc(sizeof(*domain), GFP_KERNEL); > - if (!domain) { > - WARN_ON(1); > - return; > - } > + if (!domain) > + return ERR_PTR(-ENOMEM); > > domain->irq_base = irq_base; > + domain->nr_irq = nr_irq; > domain->of_node = of_node_get(controller); > domain->ops = &irq_domain_simple_ops; > irq_domain_add(domain); > + > + return domain; > } > EXPORT_SYMBOL_GPL(irq_domain_add_simple); > > -void irq_domain_generate_simple(const struct of_device_id *match, > - u64 phys_base, unsigned int irq_start) > +struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, > + u64 phys_base, > + unsigned int irq_start, > + unsigned int nr_irq) > { > + struct irq_domain *domain = ERR_PTR(-ENODEV); > struct device_node *node; > - pr_info("looking for phys_base=%llx, irq_start=%i\n", > - (unsigned long long) phys_base, (int) irq_start); > + pr_info("looking for phys_base=%llx, irq_start=%u, nr_irq=%u\n", > + (unsigned long long) phys_base, irq_start, nr_irq); > node = of_find_matching_node_by_address(NULL, match, phys_base); > if (node) > - irq_domain_add_simple(node, irq_start); > + domain = irq_domain_add_simple(node, irq_start, nr_irq); > else > pr_info("no node found\n"); > + > + return domain; > } > EXPORT_SYMBOL_GPL(irq_domain_generate_simple); > #endif /* CONFIG_OF_IRQ */
Hi Thierry, On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > The irq_domain_add() function needs the number of interrupts in the > domain to properly initialize them. In addition the allocated domain > is now returned by the irq_domain_{add,generate}_simple() helpers. The commit text should also include the justification for renaming irq_domain_create_simple() -> irq_domain_add_simple() > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > --- > /** > - * irq_domain_create_simple() - Set up a 'simple' translation range > + * irq_domain_add_simple() - Set up a 'simple' translation range > */ > -void irq_domain_add_simple(struct device_node *controller, int irq_base) > +struct irq_domain *irq_domain_add_simple(struct device_node *controller, > + unsigned int irq_base, > + unsigned int nr_irq) > { > struct irq_domain *domain; > > domain = kzalloc(sizeof(*domain), GFP_KERNEL); > - if (!domain) { > - WARN_ON(1); > - return; > - } > + if (!domain) > + return ERR_PTR(-ENOMEM); Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO). Return NULL on failure and keep the WARN_ON() in this function. Otherwise looks good. Thanks for writing this patch. g.
On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote: > diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c > index 0a11342..a50c7e2 100644 > --- a/arch/arm/mach-msm/board-msm8x60.c > +++ b/arch/arm/mach-msm/board-msm8x60.c > @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) > > node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, > MSM8X60_QGIC_DIST_PHYS); > - if (node) > - irq_domain_add_simple(node, GIC_SPI_START); > + if (node) { > + struct irq_domain *domain = irq_domain_add_simple(node, > + GIC_SPI_START, NR_MSM_IRQS); > + WARN_ON(IS_ERR(domain)); > + } > > if (of_machine_is_compatible("qcom,msm8660-surf")) { > printk(KERN_INFO "Init surf UART registers\n"); This is probably a consequence of MSM not really being "simple", but just using that. However, NR_MSM_IRQS is only the number of IRQs on the MSM core. There are also GPIO irqs, and potentially board IRQs (the board has an I2C-based chip with a bunch of IRQ lines on it). The only define that captures this now is 'NR_IRQS', even though we're trying to get rid of that. Ultimately, the correct answer will be to get the various interrupt controllers using their own domains, but for now, this needs to be a larger value to avoid missing a bunch of the interrupts. David
* David Brown wrote: > On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote: > > diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c > > index 0a11342..a50c7e2 100644 > > --- a/arch/arm/mach-msm/board-msm8x60.c > > +++ b/arch/arm/mach-msm/board-msm8x60.c > > @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) > > > > node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, > > MSM8X60_QGIC_DIST_PHYS); > > - if (node) > > - irq_domain_add_simple(node, GIC_SPI_START); > > + if (node) { > > + struct irq_domain *domain = irq_domain_add_simple(node, > > + GIC_SPI_START, NR_MSM_IRQS); > > + WARN_ON(IS_ERR(domain)); > > + } > > > > if (of_machine_is_compatible("qcom,msm8660-surf")) { > > printk(KERN_INFO "Init surf UART registers\n"); > > This is probably a consequence of MSM not really being "simple", but > just using that. However, NR_MSM_IRQS is only the number of IRQs on > the MSM core. There are also GPIO irqs, and potentially board IRQs > (the board has an I2C-based chip with a bunch of IRQ lines on it). > > The only define that captures this now is 'NR_IRQS', even though we're > trying to get rid of that. > > Ultimately, the correct answer will be to get the various interrupt > controllers using their own domains, Yes. > but for now, this needs to be a larger value to avoid missing a bunch of > the interrupts. Okay, I'll make it NR_IRQS then. Thierry
* Grant Likely wrote: > Hi Thierry, > > On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > The irq_domain_add() function needs the number of interrupts in the > > domain to properly initialize them. In addition the allocated domain > > is now returned by the irq_domain_{add,generate}_simple() helpers. > > The commit text should also include the justification for renaming > irq_domain_create_simple() -> irq_domain_add_simple() Actually the commit only fixes up the comment. The function has always been called irq_domain_add_simple(). For reference, this was introduced in commit 7e71330. > > domain = kzalloc(sizeof(*domain), GFP_KERNEL); > > - if (!domain) { > > - WARN_ON(1); > > - return; > > - } > > + if (!domain) > > + return ERR_PTR(-ENOMEM); > > Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO). Returning NULL here is probably okay. Can the ERR_PTR stay in irq_domain_generate_simple(), though? It has two error conditions and handling both by returning NULL may not be what we want. > Return NULL on failure and keep the WARN_ON() in this function. > Otherwise looks good. Thanks for writing this patch. I thought it would be better to pull the WARN_ON out of the function because we now actually have a way to determine if the call succeeded in the caller. In most cases I assume the caller will be much better suited to handle the situation gracefully such that the WARN_ON is not required. Thierry
On 01/06/2012 10:07 AM, David Brown wrote: > On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote: >> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c >> index 0a11342..a50c7e2 100644 >> --- a/arch/arm/mach-msm/board-msm8x60.c >> +++ b/arch/arm/mach-msm/board-msm8x60.c >> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) >> >> node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, >> MSM8X60_QGIC_DIST_PHYS); >> - if (node) >> - irq_domain_add_simple(node, GIC_SPI_START); >> + if (node) { >> + struct irq_domain *domain = irq_domain_add_simple(node, >> + GIC_SPI_START, NR_MSM_IRQS); >> + WARN_ON(IS_ERR(domain)); >> + } >> >> if (of_machine_is_compatible("qcom,msm8660-surf")) { >> printk(KERN_INFO "Init surf UART registers\n"); > > This is probably a consequence of MSM not really being "simple", but > just using that. However, NR_MSM_IRQS is only the number of IRQs on > the MSM core. There are also GPIO irqs, and potentially board IRQs > (the board has an I2C-based chip with a bunch of IRQ lines on it). > > The only define that captures this now is 'NR_IRQS', even though we're > trying to get rid of that. > > Ultimately, the correct answer will be to get the various interrupt > controllers using their own domains, but for now, this needs to be a > larger value to avoid missing a bunch of the interrupts. No. This should only be the number of interrupts for a controller as the interrupt numbers in the device tree should be relative to a controller and not the Linux virq number. The numbering in the dts needs to be correct. You don't need a domain until you start getting the interrupts from the dts. Rob
Hi Thierry, On 1/6/2012 3:28 PM, Thierry Reding wrote: > The irq_domain_add() function needs the number of interrupts in the > domain to properly initialize them. In addition the allocated domain > is now returned by the irq_domain_{add,generate}_simple() helpers. [...] > diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c > index d587560..bf67781 100644 > --- a/arch/arm/mach-omap2/board-generic.c > +++ b/arch/arm/mach-omap2/board-generic.c > @@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = { > static void __init omap_generic_init(void) > { > struct device_node *node = of_find_matching_node(NULL, intc_match); > - if (node) > - irq_domain_add_simple(node, 0); > + if (node) { > + struct irq_domain *domain; > + domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS); The number of interrupts will depend on the OMAP generation. That one is just valid for the 3430 INTC controller. Since the previous code was using zero, I guess that using 0 there should be fine. Moreover, that piece of code should not exist anymore on 3.3 if the series I sent last month to leverage Rob's DT interrupt init is merged [1]. I've just ping Rob and Grant on that series to get a status. Regards, Benoit [1] http://www.spinics.net/lists/linux-omap/msg62124.html
On Fri, Jan 06, 2012 at 10:26:17AM -0600, Rob Herring wrote: > On 01/06/2012 10:07 AM, David Brown wrote: > > On Fri, Jan 06, 2012 at 03:28:25PM +0100, Thierry Reding wrote: > >> diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c > >> index 0a11342..a50c7e2 100644 > >> --- a/arch/arm/mach-msm/board-msm8x60.c > >> +++ b/arch/arm/mach-msm/board-msm8x60.c > >> @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) > >> > >> node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, > >> MSM8X60_QGIC_DIST_PHYS); > >> - if (node) > >> - irq_domain_add_simple(node, GIC_SPI_START); > >> + if (node) { > >> + struct irq_domain *domain = irq_domain_add_simple(node, > >> + GIC_SPI_START, NR_MSM_IRQS); > >> + WARN_ON(IS_ERR(domain)); > >> + } > >> > >> if (of_machine_is_compatible("qcom,msm8660-surf")) { > >> printk(KERN_INFO "Init surf UART registers\n"); > > > > This is probably a consequence of MSM not really being "simple", but > > just using that. However, NR_MSM_IRQS is only the number of IRQs on > > the MSM core. There are also GPIO irqs, and potentially board IRQs > > (the board has an I2C-based chip with a bunch of IRQ lines on it). > > > > The only define that captures this now is 'NR_IRQS', even though we're > > trying to get rid of that. > > > > Ultimately, the correct answer will be to get the various interrupt > > controllers using their own domains, but for now, this needs to be a > > larger value to avoid missing a bunch of the interrupts. > > No. This should only be the number of interrupts for a controller as the > interrupt numbers in the device tree should be relative to a controller > and not the Linux virq number. The numbering in the dts needs to be > correct. You don't need a domain until you start getting the interrupts > from the dts. Yes, _should only be_. And, you're probably right that we shouldn't change this, and should use the NR_MSM_IRQS. When a DTS device needs to use a GPIO IRQ, the GPIO controller should properly register its own domain. The only IRQ in the DT is an MSM IRQ, so this doesn't actually break anything. So, Thierry, please leave it as NR_MSM_IRQS as in your original patch. In that form. Acked-by: David Brown <davidb@codeaurora.org>
On Fri, Jan 06, 2012 at 05:20:16PM +0100, Thierry Reding wrote: > * Grant Likely wrote: > > Hi Thierry, > > > > On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding > > <thierry.reding@avionic-design.de> wrote: > > > The irq_domain_add() function needs the number of interrupts in the > > > domain to properly initialize them. In addition the allocated domain > > > is now returned by the irq_domain_{add,generate}_simple() helpers. > > > > The commit text should also include the justification for renaming > > irq_domain_create_simple() -> irq_domain_add_simple() > > Actually the commit only fixes up the comment. The function has always been > called irq_domain_add_simple(). > > For reference, this was introduced in commit 7e71330. Hahaha. Oops, you're right. :-) > > > > domain = kzalloc(sizeof(*domain), GFP_KERNEL); > > > - if (!domain) { > > > - WARN_ON(1); > > > - return; > > > - } > > > + if (!domain) > > > + return ERR_PTR(-ENOMEM); > > > > Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO). > > Returning NULL here is probably okay. Can the ERR_PTR stay in > irq_domain_generate_simple(), though? It has two error conditions and > handling both by returning NULL may not be what we want. No. ERR_PTR is a horrible pattern because you cannot tell by looking at a prototype that returns a pointer whether or not the correct failure test is "if (!ptr)" or "if (IS_ERR(ptr))". Unless it is absolutely critical for an error code to be returned (which isn't the case here) I will not accept new code that uses ERR_PTR(). In this case, if irq_domain_add_simple() fails, then something is very wrong. I'd much rather the routine complain loudly regardless of the error condition. Actually, looking again at irq_domain_generate_simple() it should probably succeed even if it cannot find a matching node since an irq_domain does more than just device tree translation. Although, irq_domain_generate_simple() is a stop-gap solution that will eventually be removed. g.
* Grant Likely wrote: > No. ERR_PTR is a horrible pattern because you cannot tell by looking > at a prototype that returns a pointer whether or not the correct > failure test is "if (!ptr)" or "if (IS_ERR(ptr))". Unless it is > absolutely critical for an error code to be returned (which isn't the > case here) I will not accept new code that uses ERR_PTR(). > > In this case, if irq_domain_add_simple() fails, then something is very > wrong. I'd much rather the routine complain loudly regardless of the > error condition. Okay, I'll keep the WARN_ON(1) and simply return NULL. > Actually, looking again at irq_domain_generate_simple() it should > probably succeed even if it cannot find a matching node since an > irq_domain does more than just device tree translation. Although, > irq_domain_generate_simple() is a stop-gap solution that will > eventually be removed. So I'll just handle errors in irq_domain_generate_simple() the same way as in irq_domain_add_simple() and will return NULL if no matching node is found. Thierry
* Cousson, Benoit wrote: > Hi Thierry, > > On 1/6/2012 3:28 PM, Thierry Reding wrote: > >The irq_domain_add() function needs the number of interrupts in the > >domain to properly initialize them. In addition the allocated domain > >is now returned by the irq_domain_{add,generate}_simple() helpers. > > [...] > > >diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c > >index d587560..bf67781 100644 > >--- a/arch/arm/mach-omap2/board-generic.c > >+++ b/arch/arm/mach-omap2/board-generic.c > >@@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = { > > static void __init omap_generic_init(void) > > { > > struct device_node *node = of_find_matching_node(NULL, intc_match); > >- if (node) > >- irq_domain_add_simple(node, 0); > >+ if (node) { > >+ struct irq_domain *domain; > >+ domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS); > > The number of interrupts will depend on the OMAP generation. That > one is just valid for the 3430 INTC controller. > Since the previous code was using zero, I guess that using 0 there > should be fine. > > Moreover, that piece of code should not exist anymore on 3.3 if the > series I sent last month to leverage Rob's DT interrupt init is > merged [1]. > > I've just ping Rob and Grant on that series to get a status. Okay, so I guess we should wait for you patch to go in first and I'll update this patch on top of that. I assume we can coordinate things in linux-next? Thierry
diff --git a/arch/arm/mach-at91/board-dt.c b/arch/arm/mach-at91/board-dt.c index bb6b434..12f003d 100644 --- a/arch/arm/mach-at91/board-dt.c +++ b/arch/arm/mach-at91/board-dt.c @@ -95,7 +95,11 @@ static const struct of_device_id aic_of_match[] __initconst = { static void __init at91_dt_init_irq(void) { - irq_domain_generate_simple(aic_of_match, 0xfffff000, 0); + struct irq_domain *domain; + + domain = irq_domain_generate_simple(aic_of_match, 0xfffff000, 0, + NR_AIC_IRQS); + WARN_ON(IS_ERR(domain)); at91_init_irq_default(); } diff --git a/arch/arm/mach-imx/imx51-dt.c b/arch/arm/mach-imx/imx51-dt.c index e6bad17..72bf94c 100644 --- a/arch/arm/mach-imx/imx51-dt.c +++ b/arch/arm/mach-imx/imx51-dt.c @@ -47,7 +47,12 @@ static const struct of_dev_auxdata imx51_auxdata_lookup[] __initconst = { static int __init imx51_tzic_add_irq_domain(struct device_node *np, struct device_node *interrupt_parent) { - irq_domain_add_simple(np, 0); + struct irq_domain *domain; + + domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS); + if (IS_ERR(domain)) + return PTR_ERR(domain); + return 0; } @@ -55,9 +60,13 @@ static int __init imx51_gpio_add_irq_domain(struct device_node *np, struct device_node *interrupt_parent) { static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; + struct irq_domain *domain; gpio_irq_base -= 32; - irq_domain_add_simple(np, gpio_irq_base); + + domain = irq_domain_add_simple(np, gpio_irq_base, 32); + if (IS_ERR(domain)) + return PTR_ERR(domain); return 0; } diff --git a/arch/arm/mach-imx/imx53-dt.c b/arch/arm/mach-imx/imx53-dt.c index 05ebb3e..ccae620 100644 --- a/arch/arm/mach-imx/imx53-dt.c +++ b/arch/arm/mach-imx/imx53-dt.c @@ -51,7 +51,12 @@ static const struct of_dev_auxdata imx53_auxdata_lookup[] __initconst = { static int __init imx53_tzic_add_irq_domain(struct device_node *np, struct device_node *interrupt_parent) { - irq_domain_add_simple(np, 0); + struct irq_domain *domain; + + domain = irq_domain_add_simple(np, 0, TZIC_NUM_IRQS); + if (IS_ERR(domain)) + return PTR_ERR(domain); + return 0; } @@ -59,9 +64,13 @@ static int __init imx53_gpio_add_irq_domain(struct device_node *np, struct device_node *interrupt_parent) { static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; + struct irq_domain *domain; gpio_irq_base -= 32; - irq_domain_add_simple(np, gpio_irq_base); + + domain = irq_domain_add_simple(np, gpio_irq_base, 32); + if (IS_ERR(domain)) + return PTR_ERR(domain); return 0; } diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index c257281..9ed7812 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -95,9 +95,11 @@ static int __init imx6q_gpio_add_irq_domain(struct device_node *np, struct device_node *interrupt_parent) { static int gpio_irq_base = MXC_GPIO_IRQ_START + ARCH_NR_GPIOS; + struct irq_domain *domain; gpio_irq_base -= 32; - irq_domain_add_simple(np, gpio_irq_base); + domain = irq_domain_add_simple(np, gpio_irq_base, 32); + WARN_ON(IS_ERR(domain)); return 0; } diff --git a/arch/arm/mach-msm/board-msm8x60.c b/arch/arm/mach-msm/board-msm8x60.c index 0a11342..a50c7e2 100644 --- a/arch/arm/mach-msm/board-msm8x60.c +++ b/arch/arm/mach-msm/board-msm8x60.c @@ -84,8 +84,11 @@ static void __init msm8x60_dt_init(void) node = of_find_matching_node_by_address(NULL, msm_dt_gic_match, MSM8X60_QGIC_DIST_PHYS); - if (node) - irq_domain_add_simple(node, GIC_SPI_START); + if (node) { + struct irq_domain *domain = irq_domain_add_simple(node, + GIC_SPI_START, NR_MSM_IRQS); + WARN_ON(IS_ERR(domain)); + } if (of_machine_is_compatible("qcom,msm8660-surf")) { printk(KERN_INFO "Init surf UART registers\n"); diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index d587560..bf67781 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -66,8 +66,11 @@ static struct of_device_id intc_match[] __initdata = { static void __init omap_generic_init(void) { struct device_node *node = of_find_matching_node(NULL, intc_match); - if (node) - irq_domain_add_simple(node, 0); + if (node) { + struct irq_domain *domain; + domain = irq_domain_add_simple(node, 0, INTCPS_NR_IRQS); + WARN_ON(IS_ERR(domain)); + } omap_sdrc_init(NULL, NULL); diff --git a/arch/arm/mach-prima2/irq.c b/arch/arm/mach-prima2/irq.c index d93ceef..d3c5136 100644 --- a/arch/arm/mach-prima2/irq.c +++ b/arch/arm/mach-prima2/irq.c @@ -58,6 +58,7 @@ static struct of_device_id intc_ids[] = { void __init sirfsoc_of_irq_init(void) { + struct irq_domain *domain; struct device_node *np; np = of_find_matching_node(NULL, intc_ids); @@ -68,7 +69,8 @@ void __init sirfsoc_of_irq_init(void) if (!sirfsoc_intc_base) panic("unable to map intc cpu registers\n"); - irq_domain_add_simple(np, 0); + domain = irq_domain_add_simple(np, 0, NR_IRQS); + WARN_ON(IS_ERR(domain)); of_node_put(np); diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c index 02b7b93..6c40be4 100644 --- a/arch/arm/mach-versatile/core.c +++ b/arch/arm/mach-versatile/core.c @@ -98,13 +98,19 @@ static const struct of_device_id sic_of_match[] __initconst = { void __init versatile_init_irq(void) { + struct irq_domain *domain; + vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0); - irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START); + domain = irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, + IRQ_VIC_START, IRQ_VIC_END - IRQ_VIC_START + 1); + WARN_ON(IS_ERR(domain)); writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR); fpga_irq_init(IRQ_VICSOURCE31, ~PIC_MASK, &sic_irq); - irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, IRQ_SIC_START); + domain = irq_domain_generate_simple(sic_of_match, VERSATILE_SIC_BASE, + IRQ_SIC_START, IRQ_SIC_END - IRQ_SIC_START + 1); + WARN_ON(IS_ERR(domain)); /* * Interrupts on secondary controller from 0 to 8 are routed to diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h index fd9efb0..2fda5aa 100644 --- a/arch/arm/plat-mxc/include/mach/irqs.h +++ b/arch/arm/plat-mxc/include/mach/irqs.h @@ -54,6 +54,7 @@ /* REVISIT: Add IPU irqs on IMX51 */ #define NR_IRQS (MXC_IPU_IRQ_START + MX3_IPU_IRQS) +#define TZIC_NUM_IRQS 128 extern int imx_irq_set_priority(unsigned char irq, unsigned char prio); diff --git a/arch/arm/plat-mxc/tzic.c b/arch/arm/plat-mxc/tzic.c index 98308ec..98b23b8 100644 --- a/arch/arm/plat-mxc/tzic.c +++ b/arch/arm/plat-mxc/tzic.c @@ -50,8 +50,6 @@ void __iomem *tzic_base; /* Used as irq controller base in entry-macro.S */ -#define TZIC_NUM_IRQS 128 - #ifdef CONFIG_FIQ static int tzic_set_irq_fiq(unsigned int irq, unsigned int type) { diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index bd4272b..1538d4c 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -15,6 +15,7 @@ #ifndef _LINUX_IRQDOMAIN_H #define _LINUX_IRQDOMAIN_H +#include <linux/err.h> #include <linux/irq.h> #include <linux/mod_devicetable.h> @@ -96,12 +97,28 @@ extern struct irq_domain_ops irq_domain_simple_ops; #endif /* CONFIG_IRQ_DOMAIN */ #if defined(CONFIG_IRQ_DOMAIN) && defined(CONFIG_OF_IRQ) -extern void irq_domain_add_simple(struct device_node *controller, int irq_base); -extern void irq_domain_generate_simple(const struct of_device_id *match, - u64 phys_base, unsigned int irq_start); +extern struct irq_domain *irq_domain_add_simple(struct device_node *controller, + unsigned int irq_base, + unsigned int nr_irq); +extern struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, + u64 phys_base, + unsigned int irq_start, + unsigned int nr_irq); #else /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */ -static inline void irq_domain_generate_simple(const struct of_device_id *match, - u64 phys_base, unsigned int irq_start) { } +static inline struct irq_domain *irq_domain_add_simple(struct device_node *controller, + int irq_base, + unsigned int nr_irq) +{ + return ERR_PTR(-ENOSYS); +} + +static inline struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, + u64 phys_base, + unsigned int irq_start, + unsigned int nr_irq) +{ + return ERR_PTR(-ENOSYS); +} #endif /* CONFIG_IRQ_DOMAIN && CONFIG_OF_IRQ */ #endif /* _LINUX_IRQDOMAIN_H */ diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 1f9e265..807c44b 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -147,36 +147,44 @@ int irq_domain_simple_dt_translate(struct irq_domain *d, } /** - * irq_domain_create_simple() - Set up a 'simple' translation range + * irq_domain_add_simple() - Set up a 'simple' translation range */ -void irq_domain_add_simple(struct device_node *controller, int irq_base) +struct irq_domain *irq_domain_add_simple(struct device_node *controller, + unsigned int irq_base, + unsigned int nr_irq) { struct irq_domain *domain; domain = kzalloc(sizeof(*domain), GFP_KERNEL); - if (!domain) { - WARN_ON(1); - return; - } + if (!domain) + return ERR_PTR(-ENOMEM); domain->irq_base = irq_base; + domain->nr_irq = nr_irq; domain->of_node = of_node_get(controller); domain->ops = &irq_domain_simple_ops; irq_domain_add(domain); + + return domain; } EXPORT_SYMBOL_GPL(irq_domain_add_simple); -void irq_domain_generate_simple(const struct of_device_id *match, - u64 phys_base, unsigned int irq_start) +struct irq_domain *irq_domain_generate_simple(const struct of_device_id *match, + u64 phys_base, + unsigned int irq_start, + unsigned int nr_irq) { + struct irq_domain *domain = ERR_PTR(-ENODEV); struct device_node *node; - pr_info("looking for phys_base=%llx, irq_start=%i\n", - (unsigned long long) phys_base, (int) irq_start); + pr_info("looking for phys_base=%llx, irq_start=%u, nr_irq=%u\n", + (unsigned long long) phys_base, irq_start, nr_irq); node = of_find_matching_node_by_address(NULL, match, phys_base); if (node) - irq_domain_add_simple(node, irq_start); + domain = irq_domain_add_simple(node, irq_start, nr_irq); else pr_info("no node found\n"); + + return domain; } EXPORT_SYMBOL_GPL(irq_domain_generate_simple); #endif /* CONFIG_OF_IRQ */
The irq_domain_add() function needs the number of interrupts in the domain to properly initialize them. In addition the allocated domain is now returned by the irq_domain_{add,generate}_simple() helpers. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- arch/arm/mach-at91/board-dt.c | 6 +++++- arch/arm/mach-imx/imx51-dt.c | 13 +++++++++++-- arch/arm/mach-imx/imx53-dt.c | 13 +++++++++++-- arch/arm/mach-imx/mach-imx6q.c | 4 +++- arch/arm/mach-msm/board-msm8x60.c | 7 +++++-- arch/arm/mach-omap2/board-generic.c | 7 +++++-- arch/arm/mach-prima2/irq.c | 4 +++- arch/arm/mach-versatile/core.c | 10 ++++++++-- arch/arm/plat-mxc/include/mach/irqs.h | 1 + arch/arm/plat-mxc/tzic.c | 2 -- include/linux/irqdomain.h | 27 ++++++++++++++++++++++----- kernel/irq/irqdomain.c | 30 +++++++++++++++++++----------- 12 files changed, 93 insertions(+), 31 deletions(-)