| Message ID | 1343330187-20049-5-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 27, 2012 at 3:16 AM, Daniel Mack <zonque@gmail.com> wrote: > Properly register on-chip interrupt using the irqdomain logic. The > number of interrupts is taken from the devicetree node. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > arch/arm/mach-pxa/irq.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-pxa/pxa3xx.c | 17 +++++++++-- > 2 files changed, 88 insertions(+), 2 deletions(-) > > +#ifdef CONFIG_OF > +static struct irq_domain *pxa_irq_domain; > + > +static int pxa_irq_map(struct irq_domain *h, unsigned int virq, > + irq_hw_number_t hw) > +{ > + int irq, i = hw % 32; > + void __iomem *base = irq_base(hw / 32); > + > + /* initialize interrupt priority */ > + if (cpu_has_ipr()) > + __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); Since we have DT support at here. Could we use property for interrupt priority? > + > + irq = PXA_IRQ(virq); #ifdef CONFIG_PXA_HAVE_ISA_IRQS #define PXA_ISA_IRQ(x) (x) #define PXA_ISA_IRQ_NUM (16) #else #define PXA_ISA_IRQ_NUM (0) #endif Could we avoid to use PXA_IRQ() at here? We can make use of NR_IRQS_LEGACY that is 16. Since you already use irq_alloc_descs() to allocate irqs that virtual irq number starts from 16. So you needn't use PXA_IRQ() any more.
Hi Haojian, On 28.07.2012 09:17, Haojian Zhuang wrote: > On Fri, Jul 27, 2012 at 3:16 AM, Daniel Mack <zonque@gmail.com> wrote: >> Properly register on-chip interrupt using the irqdomain logic. The >> number of interrupts is taken from the devicetree node. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> >> --- >> arch/arm/mach-pxa/irq.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ >> arch/arm/mach-pxa/pxa3xx.c | 17 +++++++++-- >> 2 files changed, 88 insertions(+), 2 deletions(-) >> >> +#ifdef CONFIG_OF >> +static struct irq_domain *pxa_irq_domain; >> + >> +static int pxa_irq_map(struct irq_domain *h, unsigned int virq, >> + irq_hw_number_t hw) >> +{ >> + int irq, i = hw % 32; >> + void __iomem *base = irq_base(hw / 32); >> + >> + /* initialize interrupt priority */ >> + if (cpu_has_ipr()) >> + __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); > Since we have DT support at here. Could we use property for interrupt priority? Not sure what you mean here. Can you elaborate? I couldn't find any reference to IRQ priorities in other platforms either. Maybe we can also add that in a separate patch, which would also help in tracking possible regressions du to such a change? >> + irq = PXA_IRQ(virq); > #ifdef CONFIG_PXA_HAVE_ISA_IRQS > #define PXA_ISA_IRQ(x) (x) > #define PXA_ISA_IRQ_NUM (16) > #else > #define PXA_ISA_IRQ_NUM (0) > #endif > > Could we avoid to use PXA_IRQ() at here? We can make use of > NR_IRQS_LEGACY that is 16. Since you already use irq_alloc_descs() > to allocate irqs that virtual irq number starts from 16. So you needn't > use PXA_IRQ() any more. Ok, I changed this. Note that there's still need to subtract NR_IRQS_LEGACY from the virq that is passed in to the .map function, because early_irq_init() in kernel/irq/irqdesc.c will pre-allocate the IRQs the platform claims to have natively, which defaults to 16 on PXA, unless the machine descriptor sets nr_irqs, which it doesn't in case of DT. I also found another hunk that I forgot to squash in the v2 series. I will repost the whole thing and also include the two GPIO patches that should go through your tree. Thanks for the review, Daniel
On Sat, Jul 28, 2012 at 5:56 PM, Daniel Mack <zonque@gmail.com> wrote: > Hi Haojian, > > On 28.07.2012 09:17, Haojian Zhuang wrote: >> On Fri, Jul 27, 2012 at 3:16 AM, Daniel Mack <zonque@gmail.com> wrote: >>> Properly register on-chip interrupt using the irqdomain logic. The >>> number of interrupts is taken from the devicetree node. >>> >>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>> --- >>> arch/arm/mach-pxa/irq.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ >>> arch/arm/mach-pxa/pxa3xx.c | 17 +++++++++-- >>> 2 files changed, 88 insertions(+), 2 deletions(-) >>> >>> +#ifdef CONFIG_OF >>> +static struct irq_domain *pxa_irq_domain; >>> + >>> +static int pxa_irq_map(struct irq_domain *h, unsigned int virq, >>> + irq_hw_number_t hw) >>> +{ >>> + int irq, i = hw % 32; >>> + void __iomem *base = irq_base(hw / 32); >>> + >>> + /* initialize interrupt priority */ >>> + if (cpu_has_ipr()) >>> + __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); >> Since we have DT support at here. Could we use property for interrupt priority? > > Not sure what you mean here. Can you elaborate? I couldn't find any > reference to IRQ priorities in other platforms either. > > Maybe we can also add that in a separate patch, which would also help in > tracking possible regressions du to such a change? > cpu_has_ipr() returns true if CPU isn't PXA25x. My point is that we can avoid to use cpu_is_xxx() while DT is used. We only need to append a property "marvell,intc-priority" is DTS. So the code could be changed in below. if (of_find_property(np, "marvell,intc-priority", NULL)) __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); >>> + irq = PXA_IRQ(virq); >> #ifdef CONFIG_PXA_HAVE_ISA_IRQS >> #define PXA_ISA_IRQ(x) (x) >> #define PXA_ISA_IRQ_NUM (16) >> #else >> #define PXA_ISA_IRQ_NUM (0) >> #endif >> >> Could we avoid to use PXA_IRQ() at here? We can make use of >> NR_IRQS_LEGACY that is 16. Since you already use irq_alloc_descs() >> to allocate irqs that virtual irq number starts from 16. So you needn't >> use PXA_IRQ() any more. > > Ok, I changed this. Note that there's still need to subtract > NR_IRQS_LEGACY from the virq that is passed in to the .map function, > because early_irq_init() in kernel/irq/irqdesc.c will pre-allocate the > IRQs the platform claims to have natively, which defaults to 16 on PXA, > unless the machine descriptor sets nr_irqs, which it doesn't in case of DT. > You needn't subtract NR_IRQS_LEGACY. PXA25x hwirq starts from 16 & PXA27x/PXA3xx hwirq starts from 0. While DT is used, irq_alloc_descs() allocates virq from NR_IRQS_LEGACY. For PXA25x, there's exactly match. For PXA27x/PXA3xx, there's a little different. But it doesn't matter. We needn't force virq starting from 0 on PXA27x/PXA3xx. The first virq starts from 16 is also OK. Although I use this subtract in arch-mmp, it's a little different in arch-pxa because of PXA25x. > I also found another hunk that I forgot to squash in the v2 series. I > will repost the whole thing and also include the two GPIO patches that > should go through your tree. > > > Thanks for the review, > > Daniel >
On Saturday 28 July 2012, Haojian Zhuang wrote: > On Sat, Jul 28, 2012 at 5:56 PM, Daniel Mack <zonque@gmail.com> wrote: > >> Since we have DT support at here. Could we use property for interrupt priority? > > > > Not sure what you mean here. Can you elaborate? I couldn't find any > > reference to IRQ priorities in other platforms either. > > > > Maybe we can also add that in a separate patch, which would also help in > > tracking possible regressions du to such a change? > > > cpu_has_ipr() returns true if CPU isn't PXA25x. > My point is that we can avoid to use cpu_is_xxx() while DT is used. We only need > to append a property "marvell,intc-priority" is DTS. So the code could > be changed > in below. > if (of_find_property(np, "marvell,intc-priority", NULL)) > __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); I think we can do even better if we extend the binding for this interrupt controller driver to have either #interrupt-cells=<1> or #interrupt-cells=<2>, depending on the chip that is being used. If the it is <1> (for pxa25x), then any driver would just use a bare interrupt number. If it's <2> (for all others), the driver would specify both the interrupt number and the priority, and we set up the register at the time when the interrupt gets enabled. Do you think that would work? Arnd
On 29.07.2012 16:09, Arnd Bergmann wrote: > On Saturday 28 July 2012, Haojian Zhuang wrote: >> On Sat, Jul 28, 2012 at 5:56 PM, Daniel Mack <zonque@gmail.com> wrote: >>>> Since we have DT support at here. Could we use property for interrupt priority? >>> >>> Not sure what you mean here. Can you elaborate? I couldn't find any >>> reference to IRQ priorities in other platforms either. >>> >>> Maybe we can also add that in a separate patch, which would also help in >>> tracking possible regressions du to such a change? >>> >> cpu_has_ipr() returns true if CPU isn't PXA25x. >> My point is that we can avoid to use cpu_is_xxx() while DT is used. We only need >> to append a property "marvell,intc-priority" is DTS. So the code could >> be changed >> in below. >> if (of_find_property(np, "marvell,intc-priority", NULL)) >> __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); > > I think we can do even better if we extend the binding for this interrupt controller > driver to have either #interrupt-cells=<1> or #interrupt-cells=<2>, depending on the > chip that is being used. If the it is <1> (for pxa25x), then any driver would > just use a bare interrupt number. If it's <2> (for all others), the driver would > specify both the interrupt number and the priority, and we set up the register at > the time when the interrupt gets enabled. > > Do you think that would work? Hmm, PXA25x (which does not feature IRQ priorities) shares a fair amount of peripherals with other PXA series (which do have support for that). I would much like to reflect that fact by inherhiting device nodes from one dtsi to the other. Hence, if at all, we would need to have two cells always, and just ignore the second argument on PXA25x. And I also wonder whether using the second spec value for a priority wouldn't be somehow abusive? Isn't that considered to denote the trigger flags in contexts of interrupt controllers? At least, that is what irq_domain_xlate_twocell() assumes. Daniel
On Sunday 29 July 2012, Daniel Mack wrote: > Hmm, PXA25x (which does not feature IRQ priorities) shares a fair amount > of peripherals with other PXA series (which do have support for that). I > would much like to reflect that fact by inherhiting device nodes from > one dtsi to the other. Hence, if at all, we would need to have two cells > always, and just ignore the second argument on PXA25x. But that can only work if the interrupt numbers are identical between PXA25x and the other SoCs. Are they? > And I also wonder whether using the second spec value for a priority > wouldn't be somehow abusive? Isn't that considered to denote the trigger > flags in contexts of interrupt controllers? At least, that is what > irq_domain_xlate_twocell() assumes. You would not use irq_domain_xlate_twocell in that scenario but provide your own, which is ok. Interpreting the second cell as the trigger flags is just a convenient default because it's the most common use for that. Arnd
On 30.07.2012 10:31, Arnd Bergmann wrote: > On Sunday 29 July 2012, Daniel Mack wrote: >> Hmm, PXA25x (which does not feature IRQ priorities) shares a fair amount >> of peripherals with other PXA series (which do have support for that). I >> would much like to reflect that fact by inherhiting device nodes from >> one dtsi to the other. Hence, if at all, we would need to have two cells >> always, and just ignore the second argument on PXA25x. > > But that can only work if the interrupt numbers are identical between PXA25x > and the other SoCs. Are they? Yes. >> And I also wonder whether using the second spec value for a priority >> wouldn't be somehow abusive? Isn't that considered to denote the trigger >> flags in contexts of interrupt controllers? At least, that is what >> irq_domain_xlate_twocell() assumes. > > You would not use irq_domain_xlate_twocell in that scenario but provide your > own, which is ok. Interpreting the second cell as the trigger flags is just > a convenient default because it's the most common use for that. I see. Don't know how much sense it makes to have that detail configurable though. Haojian? And I think we can still change that detail later. Daniel
On Mon, Jul 30, 2012 at 4:34 PM, Daniel Mack <zonque@gmail.com> wrote: > On 30.07.2012 10:31, Arnd Bergmann wrote: >> On Sunday 29 July 2012, Daniel Mack wrote: >>> And I also wonder whether using the second spec value for a priority >>> wouldn't be somehow abusive? Isn't that considered to denote the trigger >>> flags in contexts of interrupt controllers? At least, that is what >>> irq_domain_xlate_twocell() assumes. >> >> You would not use irq_domain_xlate_twocell in that scenario but provide your >> own, which is ok. Interpreting the second cell as the trigger flags is just >> a convenient default because it's the most common use for that. > > I see. Don't know how much sense it makes to have that detail > configurable though. Haojian? And I think we can still change that > detail later. > Arnd's suggestion is good. So we can setup each interrupt's priority while parsing all these pxa interrupts. In current code, we only assign priority with the irq number. Maybe it's not perfect solution. For example, Timer interrupt should have highest priority. LCD interrupt also has higher priority. It's worth to do. And it's also OK if you want to queue it into your TODO list. By the way, which patches that you prefer not to go through pxa git tree? Regards Haojian
On 30.07.2012 10:55, Haojian Zhuang wrote: > On Mon, Jul 30, 2012 at 4:34 PM, Daniel Mack <zonque@gmail.com> wrote: >> On 30.07.2012 10:31, Arnd Bergmann wrote: >>> On Sunday 29 July 2012, Daniel Mack wrote: >>>> And I also wonder whether using the second spec value for a priority >>>> wouldn't be somehow abusive? Isn't that considered to denote the trigger >>>> flags in contexts of interrupt controllers? At least, that is what >>>> irq_domain_xlate_twocell() assumes. >>> >>> You would not use irq_domain_xlate_twocell in that scenario but provide your >>> own, which is ok. Interpreting the second cell as the trigger flags is just >>> a convenient default because it's the most common use for that. >> >> I see. Don't know how much sense it makes to have that detail >> configurable though. Haojian? And I think we can still change that >> detail later. >> > Arnd's suggestion is good. So we can setup each interrupt's priority > while parsing > all these pxa interrupts. In current code, we only assign priority > with the irq number. > Maybe it's not perfect solution. For example, Timer interrupt should > have highest > priority. LCD interrupt also has higher priority. Arnd mentioned that instead of using the default irq_domain_xlate_onecell(), we can hook up our own translation function. While that is true, I wonder how that value that we send back in *out_type will ever appear in the irq_chip callbacks. Looking at the code that calls ->xlate(), I can see that irq_create_of_mapping() would call irq_set_irq_type() with our passed value, which will then &= it with IRQ_TYPE_SENSE_MASK (which is 0xf which doesn't suffice for our up to 96 interrupts). Arnd, either I don't get your point, or this would need some changes in the irqdomain core. Could you elaborate a little? > It's worth to do. And it's also OK if you want to queue it into your TODO list. Then let's do it as a separate patch later. It's easy to change once we agree on how to do it. > By the way, which patches that you prefer not to go through pxa git tree? I would say you can take all 9 patches that I prepared in the branch now. I got some feedback on the first round but didn't hear back from anyone since then. For the OHCI part (which is not part of it), I don't know if it might be better to let it go through the USB tree. Daniel
On Monday 30 July 2012, Daniel Mack wrote: > On 30.07.2012 10:55, Haojian Zhuang wrote: > > On Mon, Jul 30, 2012 at 4:34 PM, Daniel Mack <zonque@gmail.com> wrote: > >> On 30.07.2012 10:31, Arnd Bergmann wrote: > >>> On Sunday 29 July 2012, Daniel Mack wrote: > >>>> And I also wonder whether using the second spec value for a priority > >>>> wouldn't be somehow abusive? Isn't that considered to denote the trigger > >>>> flags in contexts of interrupt controllers? At least, that is what > >>>> irq_domain_xlate_twocell() assumes. > >>> > >>> You would not use irq_domain_xlate_twocell in that scenario but provide your > >>> own, which is ok. Interpreting the second cell as the trigger flags is just > >>> a convenient default because it's the most common use for that. > >> > >> I see. Don't know how much sense it makes to have that detail > >> configurable though. Haojian? And I think we can still change that > >> detail later. > >> > > Arnd's suggestion is good. So we can setup each interrupt's priority > > while parsing > > all these pxa interrupts. In current code, we only assign priority > > with the irq number. > > Maybe it's not perfect solution. For example, Timer interrupt should > > have highest > > priority. LCD interrupt also has higher priority. > > Arnd mentioned that instead of using the default > irq_domain_xlate_onecell(), we can hook up our own translation function. > While that is true, I wonder how that value that we send back in > *out_type will ever appear in the irq_chip callbacks. Looking at the > code that calls ->xlate(), I can see that irq_create_of_mapping() would > call irq_set_irq_type() with our passed value, which will then &= it > with IRQ_TYPE_SENSE_MASK (which is 0xf which doesn't suffice for our up > to 96 interrupts). > > Arnd, either I don't get your point, or this would need some changes in > the irqdomain core. Could you elaborate a little? When you have your own xlate function, you would still always set the default flags (IRQ_TYPE_NONE), but you do record the priority from the flags in the same way that at91_aic_irq_domain_xlate does. Arnd
diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c index 5dae15e..7753d09 100644 --- a/arch/arm/mach-pxa/irq.c +++ b/arch/arm/mach-pxa/irq.c @@ -17,6 +17,8 @@ #include <linux/syscore_ops.h> #include <linux/io.h> #include <linux/irq.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> #include <asm/exception.h> @@ -202,3 +204,74 @@ struct syscore_ops pxa_irq_syscore_ops = { .suspend = pxa_irq_suspend, .resume = pxa_irq_resume, }; + +#ifdef CONFIG_OF +static struct irq_domain *pxa_irq_domain; + +static int pxa_irq_map(struct irq_domain *h, unsigned int virq, + irq_hw_number_t hw) +{ + int irq, i = hw % 32; + void __iomem *base = irq_base(hw / 32); + + /* initialize interrupt priority */ + if (cpu_has_ipr()) + __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i)); + + irq = PXA_IRQ(virq); + irq_set_chip_and_handler(irq, &pxa_internal_irq_chip, + handle_level_irq); + irq_set_chip_data(virq, base); + set_irq_flags(virq, IRQF_VALID); + + return 0; +} + +static struct irq_domain_ops pxa_irq_ops = { + .map = pxa_irq_map, + .xlate = irq_domain_xlate_onecell, +}; + +static const struct of_device_id intc_ids[] __initconst = { + { .compatible = "marvell,pxa-intc", .data = NULL }, + {} +}; + +void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int)) +{ + struct device_node *node; + const struct of_device_id *of_id; + struct pxa_intc_conf *conf; + int nr_irqs, irq_base, ret; + + node = of_find_matching_node(NULL, intc_ids); + if (!node) { + pr_err("Failed to find interrupt controller in arch-pxa\n"); + return; + } + of_id = of_match_node(intc_ids, node); + conf = of_id->data; + + ret = of_property_read_u32(node, "mrvl,intc-nr-irqs", &nr_irqs); + if (ret) { + pr_err("Not found mrvl,intc-nr-irqs property\n"); + return; + } + + irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0); + if (irq_base < 0) { + pr_err("Failed to allocate IRQ numbers\n"); + return; + } + + pxa_irq_domain = irq_domain_add_legacy(node, nr_irqs, 0, 0, + &pxa_irq_ops, NULL); + if (!pxa_irq_domain) + panic("Unable to add PXA IRQ domain\n"); + + irq_set_default_host(pxa_irq_domain); + pxa_init_irq(nr_irqs, fn); + + return; +} +#endif /* CONFIG_OF */ diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c index dffb7e8..1827d3c 100644 --- a/arch/arm/mach-pxa/pxa3xx.c +++ b/arch/arm/mach-pxa/pxa3xx.c @@ -40,6 +40,8 @@ #define PECR_IE(n) ((1 << ((n) * 2)) << 28) #define PECR_IS(n) ((1 << ((n) * 2)) << 29) +extern void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int)); + static DEFINE_PXA3_CKEN(pxa3xx_ffuart, FFUART, 14857000, 1); static DEFINE_PXA3_CKEN(pxa3xx_btuart, BTUART, 14857000, 1); static DEFINE_PXA3_CKEN(pxa3xx_stuart, STUART, 14857000, 1); @@ -382,7 +384,7 @@ static void __init pxa_init_ext_wakeup_irq(int (*fn)(struct irq_data *, pxa_ext_wakeup_chip.irq_set_wake = fn; } -void __init pxa3xx_init_irq(void) +static void __init __pxa3xx_init_irq(void) { /* enable CP6 access */ u32 value; @@ -390,10 +392,21 @@ void __init pxa3xx_init_irq(void) value |= (1 << 6); __asm__ __volatile__("mcr p15, 0, %0, c15, c1, 0\n": :"r"(value)); - pxa_init_irq(56, pxa3xx_set_wake); pxa_init_ext_wakeup_irq(pxa3xx_set_wake); } +void __init pxa3xx_init_irq(void) +{ + __pxa3xx_init_irq(); + pxa_init_irq(56, pxa3xx_set_wake); +} + +void __init pxa3xx_dt_init_irq(void) +{ + __pxa3xx_init_irq(); + pxa_dt_irq_init(pxa3xx_set_wake); +} + static struct map_desc pxa3xx_io_desc[] __initdata = { { /* Mem Ctl */ .virtual = (unsigned long)SMEMC_VIRT,
Properly register on-chip interrupt using the irqdomain logic. The number of interrupts is taken from the devicetree node. Signed-off-by: Daniel Mack <zonque@gmail.com> --- arch/arm/mach-pxa/irq.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-pxa/pxa3xx.c | 17 +++++++++-- 2 files changed, 88 insertions(+), 2 deletions(-)