All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain
@ 2020-09-09  6:58 Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 1/3] irqchip: dw-apb-ictl: prepare for " Zhen Lei
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Zhen Lei @ 2020-09-09  6:58 UTC (permalink / raw)
  To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	devicetree, linux-kernel
  Cc: Zhen Lei, Sebastian Hesselbarth, Haoyu Lv, Libin, Kefeng Wang

v2 --> v3:
1. change (1 << hwirq) to BIT(hwirq).
2. change __exception_irq_entry to __irq_entry, so we can "#include <linux/interrupt.h>"
   instead of "#include <asm/exception.h>". Ohterwise, an compilation error will be
   reported on arch/csky.
   drivers/irqchip/irq-dw-apb-ictl.c:20:10: fatal error: asm/exception.h: No such file or directory
3. use "if (!parent || (np == parent))" to determine whether it is primary interrupt controller.
4. make the primary interrupt controller case also use function handle_level_irq(), I used 
   handle_fasteoi_irq() as flow_handler before.
5. Other minor changes are not detailed.

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             | 85 ++++++++++++++++---
 3 files changed, 87 insertions(+), 14 deletions(-)

-- 
2.26.0.106.g9fadedd



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/3] irqchip: dw-apb-ictl: prepare for support hierarchy irq domain
  2020-09-09  6:58 [PATCH v3 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain Zhen Lei
@ 2020-09-09  6:58 ` Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 2/3] irqchip: dw-apb-ictl: " Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 3/3] dt-bindings: " Zhen Lei
  2 siblings, 0 replies; 6+ messages in thread
From: Zhen Lei @ 2020-09-09  6:58 UTC (permalink / raw)
  To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	devicetree, linux-kernel
  Cc: Zhen Lei, Sebastian Hesselbarth, Haoyu Lv, Libin, Kefeng Wang

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().
2. change (1 << hwirq) to BIT(hwirq).

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.

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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index e4550e9c810b..5458004242e9 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);
@@ -43,7 +43,7 @@ static void dw_apb_ictl_handler(struct irq_desc *desc)
 			u32 virq = irq_find_mapping(d, gc->irq_base + hwirq);
 
 			generic_handle_irq(virq);
-			stat &= ~(1 << hwirq);
+			stat &= ~BIT(hwirq);
 		}
 	}
 
@@ -73,12 +73,13 @@ 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;
 
 	/* 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 +121,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;
@@ -146,7 +146,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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain
  2020-09-09  6:58 [PATCH v3 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 1/3] irqchip: dw-apb-ictl: prepare for " Zhen Lei
@ 2020-09-09  6:58 ` Zhen Lei
  2020-09-13 15:10   ` Marc Zyngier
  2020-09-09  6:58 ` [PATCH v3 3/3] dt-bindings: " Zhen Lei
  2 siblings, 1 reply; 6+ messages in thread
From: Zhen Lei @ 2020-09-09  6:58 UTC (permalink / raw)
  To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	devicetree, linux-kernel
  Cc: Zhen Lei, Sebastian Hesselbarth, Haoyu Lv, Libin, Kefeng Wang

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 | 76 +++++++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 9 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 5458004242e9..3c7bebe1b947 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 <linux/interrupt.h>
 
 #define APB_INT_ENABLE_L	0x00
 #define APB_INT_ENABLE_H	0x04
@@ -26,6 +27,27 @@
 #define APB_INT_FINALSTATUS_H	0x34
 #define APB_INT_BASE_OFFSET	0x04
 
+/* irq domain of the primary interrupt controller. */
+static struct irq_domain *dw_apb_ictl_irq_domain;
+
+static void __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 &= ~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 +72,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)
 {
@@ -75,13 +121,20 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	void __iomem *iobase;
 	int ret, nrirqs, parent_irq, i;
 	u32 reg;
-	const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
-
-	/* 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;
+	const struct irq_domain_ops *domain_ops;
+
+	if (!parent || (np == parent)) {
+		/* It's used as the primary interrupt controller */
+		parent_irq = 0;
+		domain_ops = &dw_apb_ictl_irq_domain_ops;
+	} else {
+		/* 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;
+		}
+		domain_ops = &irq_generic_chip_ops;
 	}
 
 	ret = of_address_to_resource(np, 0, &r);
@@ -144,10 +197,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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] dt-bindings: dw-apb-ictl: support hierarchy irq domain
  2020-09-09  6:58 [PATCH v3 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 1/3] irqchip: dw-apb-ictl: prepare for " Zhen Lei
  2020-09-09  6:58 ` [PATCH v3 2/3] irqchip: dw-apb-ictl: " Zhen Lei
@ 2020-09-09  6:58 ` Zhen Lei
  2 siblings, 0 replies; 6+ messages in thread
From: Zhen Lei @ 2020-09-09  6:58 UTC (permalink / raw)
  To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	devicetree, linux-kernel
  Cc: Zhen Lei, Sebastian Hesselbarth, Haoyu Lv, Libin, Kefeng Wang

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain
  2020-09-09  6:58 ` [PATCH v3 2/3] irqchip: dw-apb-ictl: " Zhen Lei
@ 2020-09-13 15:10   ` Marc Zyngier
  2020-09-14  9:07     ` Leizhen (ThunderTown)
  0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2020-09-13 15:10 UTC (permalink / raw)
  To: Zhen Lei
  Cc: Thomas Gleixner, Jason Cooper, Rob Herring, devicetree,
	linux-kernel, Sebastian Hesselbarth, Haoyu Lv, Libin,
	Kefeng Wang

On Wed, 09 Sep 2020 07:58:35 +0100,
Zhen Lei <thunder.leizhen@huawei.com> 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 | 76 +++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 9 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 5458004242e9..3c7bebe1b947 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 <linux/interrupt.h>
>  
>  #define APB_INT_ENABLE_L	0x00
>  #define APB_INT_ENABLE_H	0x04
> @@ -26,6 +27,27 @@
>  #define APB_INT_FINALSTATUS_H	0x34
>  #define APB_INT_BASE_OFFSET	0x04
>  
> +/* irq domain of the primary interrupt controller. */
> +static struct irq_domain *dw_apb_ictl_irq_domain;
> +
> +static void __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 &= ~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 +72,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)
>  {
> @@ -75,13 +121,20 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>  	void __iomem *iobase;
>  	int ret, nrirqs, parent_irq, i;
>  	u32 reg;
> -	const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
> -
> -	/* 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;
> +	const struct irq_domain_ops *domain_ops;
> +
> +	if (!parent || (np == parent)) {
> +		/* It's used as the primary interrupt controller */
> +		parent_irq = 0;
> +		domain_ops = &dw_apb_ictl_irq_domain_ops;
> +	} else {
> +		/* 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;
> +		}
> +		domain_ops = &irq_generic_chip_ops;
>  	}
>  
>  	ret = of_address_to_resource(np, 0, &r);
> @@ -144,10 +197,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;

Again: what is that for? The level flow doesn't use any EOI callback.

>  	}
>  
> -	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);

This only exists on architectures that select GENERIC_IRQ_MULTI_HANDLER,
and yet this driver is used on some arc system (AXS10x), which doesn't
have this option selected.

Please make sure this at least builds on all supported architectures.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain
  2020-09-13 15:10   ` Marc Zyngier
@ 2020-09-14  9:07     ` Leizhen (ThunderTown)
  0 siblings, 0 replies; 6+ messages in thread
From: Leizhen (ThunderTown) @ 2020-09-14  9:07 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Thomas Gleixner, Jason Cooper, Rob Herring, devicetree,
	linux-kernel, Sebastian Hesselbarth, Haoyu Lv, Libin,
	Kefeng Wang



On 2020/9/13 23:10, Marc Zyngier wrote:
> On Wed, 09 Sep 2020 07:58:35 +0100,
> Zhen Lei <thunder.leizhen@huawei.com> 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 | 76 +++++++++++++++++++++++++++----
>>  2 files changed, 69 insertions(+), 9 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 5458004242e9..3c7bebe1b947 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 <linux/interrupt.h>
>>  
>>  #define APB_INT_ENABLE_L	0x00
>>  #define APB_INT_ENABLE_H	0x04
>> @@ -26,6 +27,27 @@
>>  #define APB_INT_FINALSTATUS_H	0x34
>>  #define APB_INT_BASE_OFFSET	0x04
>>  
>> +/* irq domain of the primary interrupt controller. */
>> +static struct irq_domain *dw_apb_ictl_irq_domain;
>> +
>> +static void __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 &= ~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 +72,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)
>>  {
>> @@ -75,13 +121,20 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>>  	void __iomem *iobase;
>>  	int ret, nrirqs, parent_irq, i;
>>  	u32 reg;
>> -	const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
>> -
>> -	/* 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;
>> +	const struct irq_domain_ops *domain_ops;
>> +
>> +	if (!parent || (np == parent)) {
>> +		/* It's used as the primary interrupt controller */
>> +		parent_irq = 0;
>> +		domain_ops = &dw_apb_ictl_irq_domain_ops;
>> +	} else {
>> +		/* 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;
>> +		}
>> +		domain_ops = &irq_generic_chip_ops;
>>  	}
>>  
>>  	ret = of_address_to_resource(np, 0, &r);
>> @@ -144,10 +197,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;
> 
> Again: what is that for? The level flow doesn't use any EOI callback.

OK, I will remove it. Yes, irq_eoi is only needed by handle_fasteoi_irq().

> 
>>  	}
>>  
>> -	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);
> 
> This only exists on architectures that select GENERIC_IRQ_MULTI_HANDLER,
> and yet this driver is used on some arc system (AXS10x), which doesn't
> have this option selected.
> 
> Please make sure this at least builds on all supported architectures.

OK, I will intsall the gcc of arc and csky, and build it.

> 
> 	M.
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-09-14  9:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  6:58 [PATCH v3 0/3] irqchip: dw-apb-ictl: support hierarchy irq domain Zhen Lei
2020-09-09  6:58 ` [PATCH v3 1/3] irqchip: dw-apb-ictl: prepare for " Zhen Lei
2020-09-09  6:58 ` [PATCH v3 2/3] irqchip: dw-apb-ictl: " Zhen Lei
2020-09-13 15:10   ` Marc Zyngier
2020-09-14  9:07     ` Leizhen (ThunderTown)
2020-09-09  6:58 ` [PATCH v3 3/3] dt-bindings: " Zhen Lei

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.