[PATCH v2 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[PATCH v2 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Zhen Lei]

v1 --> v2:
According to Marc Zyngier's suggestion, discard adding an independent SD5203-VIC
driver, but make the dw-apb-ictl irqchip driver to support hierarchy irq domain.
It was originally available only for secondary interrupt controller, now it can
also be used as primary interrupt controller. The related dt-bindings is updated
appropriately.

Add "Suggested-by: Marc Zyngier <maz@kernel.org>".
Add "Tested-by: Haoyu Lv <lvhaoyu@huawei.com>".


v1:
The interrupt controller of SD5203 SoC is VIC(vector interrupt controller), it's
based on Synopsys DesignWare APB interrupt controller (dw_apb_ictl) IP, but it
can not directly use dw_apb_ictl driver. The main reason is that VIC is used as
primary interrupt controller and dw_apb_ictl driver worked for secondary
interrupt controller. So add a new driver: "hisilicon,sd5203-vic".


Zhen Lei (3):
  irqchip: dw-apb-ictl: prepare for support hierarchy irq domain
  irqchip: dw-apb-ictl: support hierarchy irq domain
  dt-bindings: dw-apb-ictl: support hierarchy irq domain

 .../interrupt-controller/snps,dw-apb-ictl.txt | 14 ++-
 drivers/irqchip/Kconfig                       |  2 +-
 drivers/irqchip/irq-dw-apb-ictl.c             | 91 +++++++++++++++++--
 3 files changed, 95 insertions(+), 12 deletions(-)

-- 
2.26.0.106.g9fadedd

[PATCH v2 1/3] irqchip: dw-apb-ictl: prepare for support hierarchy irq domain

[Zhen Lei]

Rename some functions and variables in advance, to make the next patch
looks more clear. The details are as follows:
1. rename dw_apb_ictl_handler() to dw_apb_ictl_handle_irq_cascaded().

In function dw_apb_ictl_init():
1. rename local variable irq to parent_irq.
2. add "const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops",
   then replace &irq_generic_chip_ops in other places with domain_ops.
3. add "irq_flow_handler_t flow_handler = handle_level_irq",
   then replace handle_level_irq in other places with flow_handler.

No functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Tested-by: Haoyu Lv <lvhaoyu@huawei.com>
---
 drivers/irqchip/irq-dw-apb-ictl.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index e4550e9c810b..aa6214da0b1f 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -26,7 +26,7 @@
 #define APB_INT_FINALSTATUS_H	0x34
 #define APB_INT_BASE_OFFSET	0x04
 
-static void dw_apb_ictl_handler(struct irq_desc *desc)
+static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc)
 {
 	struct irq_domain *d = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -73,12 +73,14 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
 	void __iomem *iobase;
-	int ret, nrirqs, irq, i;
+	int ret, nrirqs, parent_irq, i;
 	u32 reg;
+	const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
+	irq_flow_handler_t flow_handler = handle_level_irq;
 
 	/* Map the parent interrupt for the chained handler */
-	irq = irq_of_parse_and_map(np, 0);
-	if (irq <= 0) {
+	parent_irq = irq_of_parse_and_map(np, 0);
+	if (parent_irq <= 0) {
 		pr_err("%pOF: unable to parse irq\n", np);
 		return -EINVAL;
 	}
@@ -120,8 +122,7 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	else
 		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
 
-	domain = irq_domain_add_linear(np, nrirqs,
-				       &irq_generic_chip_ops, NULL);
+	domain = irq_domain_add_linear(np, nrirqs, domain_ops, NULL);
 	if (!domain) {
 		pr_err("%pOF: unable to add irq domain\n", np);
 		ret = -ENOMEM;
@@ -129,7 +130,7 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	}
 
 	ret = irq_alloc_domain_generic_chips(domain, 32, 1, np->name,
-					     handle_level_irq, clr, 0,
+					     flow_handler, clr, 0,
 					     IRQ_GC_INIT_MASK_CACHE);
 	if (ret) {
 		pr_err("%pOF: unable to alloc irq domain gc\n", np);
@@ -146,7 +147,8 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 		gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume;
 	}
 
-	irq_set_chained_handler_and_data(irq, dw_apb_ictl_handler, domain);
+	irq_set_chained_handler_and_data(parent_irq,
+				dw_apb_ictl_handle_irq_cascaded, domain);
 
 	return 0;
 
-- 
2.26.0.106.g9fadedd

[PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Zhen Lei]

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.
+ */
+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;
 
-- 
2.26.0.106.g9fadedd

[PATCH v2 3/3] dt-bindings: dw-apb-ictl: support hierarchy irq domain

[Zhen Lei]

Add support to use dw-apb-ictl as primary interrupt controller.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 .../interrupt-controller/snps,dw-apb-ictl.txt      | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt b/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
index 086ff08322db..2db59df9408f 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/snps,dw-apb-ictl.txt
@@ -2,7 +2,8 @@ Synopsys DesignWare APB interrupt controller (dw_apb_ictl)
 
 Synopsys DesignWare provides interrupt controller IP for APB known as
 dw_apb_ictl. The IP is used as secondary interrupt controller in some SoCs with
-APB bus, e.g. Marvell Armada 1500.
+APB bus, e.g. Marvell Armada 1500. It can also be used as primary interrupt
+controller in some SoCs, e.g. Hisilicon SD5203.
 
 Required properties:
 - compatible: shall be "snps,dw-apb-ictl"
@@ -10,6 +11,8 @@ Required properties:
   region starting with ENABLE_LOW register
 - interrupt-controller: identifies the node as an interrupt controller
 - #interrupt-cells: number of cells to encode an interrupt-specifier, shall be 1
+
+Additional required property when it's used as secondary interrupt controller:
 - interrupts: interrupt reference to primary interrupt controller
 
 The interrupt sources map to the corresponding bits in the interrupt
@@ -21,6 +24,7 @@ registers, i.e.
 - (optional) fast interrupts start at 64.
 
 Example:
+	/* dw_apb_ictl is used as secondary interrupt controller */
 	aic: interrupt-controller@3000 {
 		compatible = "snps,dw-apb-ictl";
 		reg = <0x3000 0xc00>;
@@ -29,3 +33,11 @@ Example:
 		interrupt-parent = <&gic>;
 		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
 	};
+
+	/* dw_apb_ictl is used as primary interrupt controller */
+	vic: interrupt-controller@10130000 {
+		compatible = "snps,dw-apb-ictl";
+		reg = <0x10130000 0x1000>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+	};
-- 
2.26.0.106.g9fadedd

Re: [PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Marc Zyngier]

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...

Re: [PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10071 bytes --]

Re: [PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Leizhen (ThunderTown)]

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.

Re: [PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Marc Zyngier]

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...