| Message ID | 20200908071134.2578-3-thunder.leizhen@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip: dw-apb-ictl: support hierarchy irq domain | expand |
On 2020-09-08 08:11, Zhen Lei wrote: > Add support to use dw-apb-ictl as primary interrupt controller. > > Suggested-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > Tested-by: Haoyu Lv <lvhaoyu@huawei.com> > --- > drivers/irqchip/Kconfig | 2 +- > drivers/irqchip/irq-dw-apb-ictl.c | 75 +++++++++++++++++++++++++++++-- > 2 files changed, 73 insertions(+), 4 deletions(-) > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index bfc9719dbcdc..7c2d1c8fa551 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC > config DW_APB_ICTL > bool > select GENERIC_IRQ_CHIP > - select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > > config FARADAY_FTINTC010 > bool > diff --git a/drivers/irqchip/irq-dw-apb-ictl.c > b/drivers/irqchip/irq-dw-apb-ictl.c > index aa6214da0b1f..405861322596 100644 > --- a/drivers/irqchip/irq-dw-apb-ictl.c > +++ b/drivers/irqchip/irq-dw-apb-ictl.c > @@ -17,6 +17,7 @@ > #include <linux/irqchip/chained_irq.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > +#include <asm/exception.h> > > #define APB_INT_ENABLE_L 0x00 > #define APB_INT_ENABLE_H 0x04 > @@ -26,6 +27,30 @@ > #define APB_INT_FINALSTATUS_H 0x34 > #define APB_INT_BASE_OFFSET 0x04 > > +/* > + * irq domain of the primary interrupt controller. Currently, only one > is > + * supported. By definition, there is only one primary interrupt controller. > + */ > +static struct irq_domain *dw_apb_ictl_irq_domain; > + > +static void __exception_irq_entry dw_apb_ictl_handle_irq(struct > pt_regs *regs) > +{ > + struct irq_domain *d = dw_apb_ictl_irq_domain; > + int n; > + > + for (n = 0; n < d->revmap_size; n += 32) { > + struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n); > + u32 stat = readl_relaxed(gc->reg_base + APB_INT_FINALSTATUS_L); > + > + while (stat) { > + u32 hwirq = ffs(stat) - 1; > + > + handle_domain_irq(d, hwirq, regs); > + stat &= ~(1 << hwirq); nit: prefer BIT(hwirq) > + } > + } > +} > + > static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc) > { > struct irq_domain *d = irq_desc_get_handler_data(desc); > @@ -50,6 +75,30 @@ static void dw_apb_ictl_handle_irq_cascaded(struct > irq_desc *desc) > chained_irq_exit(chip, desc); > } > > +static int dw_apb_ictl_irq_domain_alloc(struct irq_domain *domain, > unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + int i, ret; > + irq_hw_number_t hwirq; > + unsigned int type = IRQ_TYPE_NONE; > + struct irq_fwspec *fwspec = arg; > + > + ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type); > + if (ret) > + return ret; > + > + for (i = 0; i < nr_irqs; i++) > + irq_map_generic_chip(domain, virq + i, hwirq + i); > + > + return 0; > +} > + > +static const struct irq_domain_ops dw_apb_ictl_irq_domain_ops = { > + .translate = irq_domain_translate_onecell, > + .alloc = dw_apb_ictl_irq_domain_alloc, > + .free = irq_domain_free_irqs_top, > +}; > + > #ifdef CONFIG_PM > static void dw_apb_ictl_resume(struct irq_data *d) > { > @@ -78,11 +127,24 @@ static int __init dw_apb_ictl_init(struct > device_node *np, > const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops; > irq_flow_handler_t flow_handler = handle_level_irq; > > + if (dw_apb_ictl_irq_domain) { > + pr_err("%pOF: a hierarchy irq domain is already exist.\n", np); > + return -EBUSY; How can this happen? > + } > + > /* Map the parent interrupt for the chained handler */ > parent_irq = irq_of_parse_and_map(np, 0); > if (parent_irq <= 0) { > - pr_err("%pOF: unable to parse irq\n", np); > - return -EINVAL Checking for an output interrupt is not the way to check for a chained interrupt controller. That's what the parent device_node is for (no parent or parent == self denotes a primary controller). ; > + /* It's used as secondary interrupt controller */ > + if (of_find_property(np, "interrupts", NULL)) { > + pr_err("%pOF: unable to parse irq\n", np); > + return -EINVAL; > + } > + > + /* It's used as the primary interrupt controller */ > + parent_irq = 0; > + domain_ops = &dw_apb_ictl_irq_domain_ops; > + flow_handler = handle_fasteoi_irq; Why? This irqchip obviously doesn't support an EOI method since you setting it to a NOP callback below. From what I understand, this controller should use handle_level_irq, just like its chained version. > } > > ret = of_address_to_resource(np, 0, &r); > @@ -145,10 +207,17 @@ static int __init dw_apb_ictl_init(struct > device_node *np, > gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; > gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; > gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume; > + if (!parent_irq) > + gc->chip_types[0].chip.irq_eoi = irq_gc_noop; > } > > - irq_set_chained_handler_and_data(parent_irq, > + if (parent_irq) { > + irq_set_chained_handler_and_data(parent_irq, > dw_apb_ictl_handle_irq_cascaded, domain); > + } else { > + dw_apb_ictl_irq_domain = domain; > + set_handle_irq(dw_apb_ictl_handle_irq); > + } > > return 0; Thanks, M. -- Jazz is not dead. It just smells funny...
Hi Zhen,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/irq/core]
[also build test ERROR on robh/for-next v5.9-rc4 next-20200903]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Zhen-Lei/irqchip-dw-apb-ictl-support-hierarchy-irq-domain/20200908-151343
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 3d5128c1deb5d27993fb11ba5e517798f8021046
config: csky-defconfig (attached as .config)
compiler: csky-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=csky
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/irqchip/irq-dw-apb-ictl.c:20:10: fatal error: asm/exception.h: No such file or directory
20 | #include <asm/exception.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
# https://github.com/0day-ci/linux/commit/6d382c797ad19f8d30b18962a255f9114601f55e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Zhen-Lei/irqchip-dw-apb-ictl-support-hierarchy-irq-domain/20200908-151343
git checkout 6d382c797ad19f8d30b18962a255f9114601f55e
vim +20 drivers/irqchip/irq-dw-apb-ictl.c
> 20 #include <asm/exception.h>
21
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 2020/9/8 15:41, Marc Zyngier wrote: > On 2020-09-08 08:11, Zhen Lei wrote: >> Add support to use dw-apb-ictl as primary interrupt controller. >> >> Suggested-by: Marc Zyngier <maz@kernel.org> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> Tested-by: Haoyu Lv <lvhaoyu@huawei.com> >> --- >> drivers/irqchip/Kconfig | 2 +- >> drivers/irqchip/irq-dw-apb-ictl.c | 75 +++++++++++++++++++++++++++++-- >> 2 files changed, 73 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >> index bfc9719dbcdc..7c2d1c8fa551 100644 >> --- a/drivers/irqchip/Kconfig >> +++ b/drivers/irqchip/Kconfig >> @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC >> config DW_APB_ICTL >> bool >> select GENERIC_IRQ_CHIP >> - select IRQ_DOMAIN >> + select IRQ_DOMAIN_HIERARCHY >> >> config FARADAY_FTINTC010 >> bool >> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c >> b/drivers/irqchip/irq-dw-apb-ictl.c >> index aa6214da0b1f..405861322596 100644 >> --- a/drivers/irqchip/irq-dw-apb-ictl.c >> +++ b/drivers/irqchip/irq-dw-apb-ictl.c >> @@ -17,6 +17,7 @@ >> #include <linux/irqchip/chained_irq.h> >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> +#include <asm/exception.h> >> >> #define APB_INT_ENABLE_L 0x00 >> #define APB_INT_ENABLE_H 0x04 >> @@ -26,6 +27,30 @@ >> #define APB_INT_FINALSTATUS_H 0x34 >> #define APB_INT_BASE_OFFSET 0x04 >> >> +/* >> + * irq domain of the primary interrupt controller. Currently, only one is >> + * supported. > > By definition, there is only one primary interrupt controller. OK, I will delete the comment "Currently, only one is supported". Should I replace it with your commend above? > >> + */ >> +static struct irq_domain *dw_apb_ictl_irq_domain; >> + >> +static void __exception_irq_entry dw_apb_ictl_handle_irq(struct pt_regs *regs) >> +{ >> + struct irq_domain *d = dw_apb_ictl_irq_domain; >> + int n; >> + >> + for (n = 0; n < d->revmap_size; n += 32) { >> + struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n); >> + u32 stat = readl_relaxed(gc->reg_base + APB_INT_FINALSTATUS_L); >> + >> + while (stat) { >> + u32 hwirq = ffs(stat) - 1; >> + >> + handle_domain_irq(d, hwirq, regs); >> + stat &= ~(1 << hwirq); > > nit: prefer BIT(hwirq) OK, I will correct it. > >> + } >> + } >> +} >> + >> static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc) >> { >> struct irq_domain *d = irq_desc_get_handler_data(desc); >> @@ -50,6 +75,30 @@ static void dw_apb_ictl_handle_irq_cascaded(struct >> irq_desc *desc) >> chained_irq_exit(chip, desc); >> } >> >> +static int dw_apb_ictl_irq_domain_alloc(struct irq_domain *domain, >> unsigned int virq, >> + unsigned int nr_irqs, void *arg) >> +{ >> + int i, ret; >> + irq_hw_number_t hwirq; >> + unsigned int type = IRQ_TYPE_NONE; >> + struct irq_fwspec *fwspec = arg; >> + >> + ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type); >> + if (ret) >> + return ret; >> + >> + for (i = 0; i < nr_irqs; i++) >> + irq_map_generic_chip(domain, virq + i, hwirq + i); >> + >> + return 0; >> +} >> + >> +static const struct irq_domain_ops dw_apb_ictl_irq_domain_ops = { >> + .translate = irq_domain_translate_onecell, >> + .alloc = dw_apb_ictl_irq_domain_alloc, >> + .free = irq_domain_free_irqs_top, >> +}; >> + >> #ifdef CONFIG_PM >> static void dw_apb_ictl_resume(struct irq_data *d) >> { >> @@ -78,11 +127,24 @@ static int __init dw_apb_ictl_init(struct device_node *np, >> const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops; >> irq_flow_handler_t flow_handler = handle_level_irq; >> >> + if (dw_apb_ictl_irq_domain) { >> + pr_err("%pOF: a hierarchy irq domain is already exist.\n", np); >> + return -EBUSY; > > How can this happen? Just afraid the users maybe define two primary interrupt controller nodes in dts by mistake, or maybe mixed with secondary interrupt controller nodes. But an error maybe reported before when parse devicetree nodes. So I'll just delete it. > >> + } >> + >> /* Map the parent interrupt for the chained handler */ >> parent_irq = irq_of_parse_and_map(np, 0); >> if (parent_irq <= 0) { >> - pr_err("%pOF: unable to parse irq\n", np); >> - return -EINVAL > > Checking for an output interrupt is not the way to check for a chained > interrupt controller. That's what the parent device_node is for (no > parent or parent == self denotes a primary controller). OK, Thank you for the point. I will modify it. > ; >> + /* It's used as secondary interrupt controller */ >> + if (of_find_property(np, "interrupts", NULL)) { >> + pr_err("%pOF: unable to parse irq\n", np); >> + return -EINVAL; >> + } >> + >> + /* It's used as the primary interrupt controller */ >> + parent_irq = 0; >> + domain_ops = &dw_apb_ictl_irq_domain_ops; >> + flow_handler = handle_fasteoi_irq; > > Why? This irqchip obviously doesn't support an EOI method since you > setting it to a NOP callback below. From what I understand, this > controller should use handle_level_irq, just like its chained version. OK, I will try to use handle_level_irq(). > >> } >> >> ret = of_address_to_resource(np, 0, &r); >> @@ -145,10 +207,17 @@ static int __init dw_apb_ictl_init(struct device_node *np, >> gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; >> gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; >> gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume; >> + if (!parent_irq) >> + gc->chip_types[0].chip.irq_eoi = irq_gc_noop; >> } >> >> - irq_set_chained_handler_and_data(parent_irq, >> + if (parent_irq) { >> + irq_set_chained_handler_and_data(parent_irq, >> dw_apb_ictl_handle_irq_cascaded, domain); >> + } else { >> + dw_apb_ictl_irq_domain = domain; >> + set_handle_irq(dw_apb_ictl_handle_irq); >> + } >> >> return 0; > > Thanks, > > M.
On 2020-09-08 10:40, Leizhen (ThunderTown) wrote: > On 2020/9/8 15:41, Marc Zyngier wrote: >> On 2020-09-08 08:11, Zhen Lei wrote: >>> Add support to use dw-apb-ictl as primary interrupt controller. >>> >>> Suggested-by: Marc Zyngier <maz@kernel.org> >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> Tested-by: Haoyu Lv <lvhaoyu@huawei.com> >>> --- >>> drivers/irqchip/Kconfig | 2 +- >>> drivers/irqchip/irq-dw-apb-ictl.c | 75 >>> +++++++++++++++++++++++++++++-- >>> 2 files changed, 73 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >>> index bfc9719dbcdc..7c2d1c8fa551 100644 >>> --- a/drivers/irqchip/Kconfig >>> +++ b/drivers/irqchip/Kconfig >>> @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC >>> config DW_APB_ICTL >>> bool >>> select GENERIC_IRQ_CHIP >>> - select IRQ_DOMAIN >>> + select IRQ_DOMAIN_HIERARCHY >>> >>> config FARADAY_FTINTC010 >>> bool >>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c >>> b/drivers/irqchip/irq-dw-apb-ictl.c >>> index aa6214da0b1f..405861322596 100644 >>> --- a/drivers/irqchip/irq-dw-apb-ictl.c >>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c >>> @@ -17,6 +17,7 @@ >>> #include <linux/irqchip/chained_irq.h> >>> #include <linux/of_address.h> >>> #include <linux/of_irq.h> >>> +#include <asm/exception.h> >>> >>> #define APB_INT_ENABLE_L 0x00 >>> #define APB_INT_ENABLE_H 0x04 >>> @@ -26,6 +27,30 @@ >>> #define APB_INT_FINALSTATUS_H 0x34 >>> #define APB_INT_BASE_OFFSET 0x04 >>> >>> +/* >>> + * irq domain of the primary interrupt controller. Currently, only >>> one is >>> + * supported. >> >> By definition, there is only one primary interrupt controller. > > OK, I will delete the comment "Currently, only one is supported". > Should I replace it with your commend above? No, just delete it. Thanks, M. -- Jazz is not dead. It just smells funny...
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index bfc9719dbcdc..7c2d1c8fa551 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC config DW_APB_ICTL bool select GENERIC_IRQ_CHIP - select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY config FARADAY_FTINTC010 bool diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c index aa6214da0b1f..405861322596 100644 --- a/drivers/irqchip/irq-dw-apb-ictl.c +++ b/drivers/irqchip/irq-dw-apb-ictl.c @@ -17,6 +17,7 @@ #include <linux/irqchip/chained_irq.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <asm/exception.h> #define APB_INT_ENABLE_L 0x00 #define APB_INT_ENABLE_H 0x04 @@ -26,6 +27,30 @@ #define APB_INT_FINALSTATUS_H 0x34 #define APB_INT_BASE_OFFSET 0x04 +/* + * irq domain of the primary interrupt controller. Currently, only one is + * supported. + */ +static struct irq_domain *dw_apb_ictl_irq_domain; + +static void __exception_irq_entry dw_apb_ictl_handle_irq(struct pt_regs *regs) +{ + struct irq_domain *d = dw_apb_ictl_irq_domain; + int n; + + for (n = 0; n < d->revmap_size; n += 32) { + struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n); + u32 stat = readl_relaxed(gc->reg_base + APB_INT_FINALSTATUS_L); + + while (stat) { + u32 hwirq = ffs(stat) - 1; + + handle_domain_irq(d, hwirq, regs); + stat &= ~(1 << hwirq); + } + } +} + static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc) { struct irq_domain *d = irq_desc_get_handler_data(desc); @@ -50,6 +75,30 @@ static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static int dw_apb_ictl_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + int i, ret; + irq_hw_number_t hwirq; + unsigned int type = IRQ_TYPE_NONE; + struct irq_fwspec *fwspec = arg; + + ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type); + if (ret) + return ret; + + for (i = 0; i < nr_irqs; i++) + irq_map_generic_chip(domain, virq + i, hwirq + i); + + return 0; +} + +static const struct irq_domain_ops dw_apb_ictl_irq_domain_ops = { + .translate = irq_domain_translate_onecell, + .alloc = dw_apb_ictl_irq_domain_alloc, + .free = irq_domain_free_irqs_top, +}; + #ifdef CONFIG_PM static void dw_apb_ictl_resume(struct irq_data *d) { @@ -78,11 +127,24 @@ static int __init dw_apb_ictl_init(struct device_node *np, const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops; irq_flow_handler_t flow_handler = handle_level_irq; + if (dw_apb_ictl_irq_domain) { + pr_err("%pOF: a hierarchy irq domain is already exist.\n", np); + return -EBUSY; + } + /* Map the parent interrupt for the chained handler */ parent_irq = irq_of_parse_and_map(np, 0); if (parent_irq <= 0) { - pr_err("%pOF: unable to parse irq\n", np); - return -EINVAL; + /* It's used as secondary interrupt controller */ + if (of_find_property(np, "interrupts", NULL)) { + pr_err("%pOF: unable to parse irq\n", np); + return -EINVAL; + } + + /* It's used as the primary interrupt controller */ + parent_irq = 0; + domain_ops = &dw_apb_ictl_irq_domain_ops; + flow_handler = handle_fasteoi_irq; } ret = of_address_to_resource(np, 0, &r); @@ -145,10 +207,17 @@ static int __init dw_apb_ictl_init(struct device_node *np, gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume; + if (!parent_irq) + gc->chip_types[0].chip.irq_eoi = irq_gc_noop; } - irq_set_chained_handler_and_data(parent_irq, + if (parent_irq) { + irq_set_chained_handler_and_data(parent_irq, dw_apb_ictl_handle_irq_cascaded, domain); + } else { + dw_apb_ictl_irq_domain = domain; + set_handle_irq(dw_apb_ictl_handle_irq); + } return 0;