[PATCH v4 0/3] irqchip: xilinx: Add support for multiple instances

[PATCH v4 0/3] irqchip: xilinx: Add support for multiple instances

[Mubin Usman Sayyed]

Created series by rebasing inter-dependent patches
from Michal (https://lkml.org/lkml/2020/3/8/164) on top
of v3 (https://lkml.org/lkml/2020/2/14/2680)

Changes for v4:
        - Fixed review comments from Thomas - updated commit
          message, variable declarations changed to reverse
          fir tree and cleaned up some code.
        - Added inter-dependendent patches 2/3 and 3/3 from Michal
          https://lkml.org/lkml/2020/3/8/164

Changes for v3:
        - Modified prototype of xintc_write/xintc_read
        - Fixed review comments regarding indentation/variable
          names, used BIT
        - Modified xintc_get_irq_local to return 0
          in case of failure/no pending interrupts
        - Fixed return type of xintc_read
        - Reverted changes related to device name and
          kept intc_dev as static

Changes for v2:
        - Removed write_fn/read_fn hooks, used xintc_write/
          xintc_read directly
        - Moved primary_intc declaration after xintc_irq_chip




Michal Simek (2):
  irqchip: xilinx: Fill error code when irq domain registration fails
  irqchip: xilinx: Enable generic irq multi handler

Mubin Sayyed (1):
  irqchip: xilinx: Add support for multiple instances

 arch/microblaze/Kconfig           |   2 +
 arch/microblaze/include/asm/irq.h |   3 -
 arch/microblaze/kernel/irq.c      |  21 +----
 drivers/irqchip/irq-xilinx-intc.c | 130 ++++++++++++++++++------------
 4 files changed, 82 insertions(+), 74 deletions(-)

-- 
2.25.0

[PATCH v4 1/3] irqchip: xilinx: Add support for multiple instances

[Mubin Usman Sayyed]

From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>

Added support for cascaded interrupt controllers.

Following cascaded configurations have been tested,

- peripheral->xilinx-intc->xilinx-intc->gic->Cortexa53 processor
  on zcu102 board
- peripheral->xilinx-intc->xilinx-intc->microblaze processor
  on kcu105 board

Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
---
Changes for v4:
	- Fixed review comments from Thomas - updated commit
	  message, variable declarations changed to reverse
	  fir tree and cleaned up some code.

Changes for v3:
	- Modified prototype of xintc_write/xintc_read
	- Fixed review comments regarding indentation/variable
	  names, used BIT
	- Modified xintc_get_irq_local to return 0
	  in case of failure/no pending interrupts
	- Fixed return type of xintc_read
	- Reverted changes related to device name and
	  kept intc_dev as static

Changes for v2:
        - Removed write_fn/read_fn hooks, used xintc_write/
	  xintc_read directly
        - Moved primary_intc declaration after xintc_irq_chip
---
 drivers/irqchip/irq-xilinx-intc.c | 121 ++++++++++++++++++------------
 1 file changed, 71 insertions(+), 50 deletions(-)

diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index e3043ded8973..65b77720ef2e 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -38,29 +38,32 @@ struct xintc_irq_chip {
 	void		__iomem *base;
 	struct		irq_domain *root_domain;
 	u32		intr_mask;
+	struct		irq_chip *intc_dev;
+	u32		nr_irq;
 };
 
-static struct xintc_irq_chip *xintc_irqc;
+static struct xintc_irq_chip *primary_intc;
 
-static void xintc_write(int reg, u32 data)
+static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data)
 {
 	if (static_branch_unlikely(&xintc_is_be))
-		iowrite32be(data, xintc_irqc->base + reg);
+		iowrite32be(data, irqc->base + reg);
 	else
-		iowrite32(data, xintc_irqc->base + reg);
+		iowrite32(data, irqc->base + reg);
 }
 
-static unsigned int xintc_read(int reg)
+static u32 xintc_read(struct xintc_irq_chip *irqc, int reg)
 {
 	if (static_branch_unlikely(&xintc_is_be))
-		return ioread32be(xintc_irqc->base + reg);
+		return ioread32be(irqc->base + reg);
 	else
-		return ioread32(xintc_irqc->base + reg);
+		return ioread32(irqc->base + reg);
 }
 
 static void intc_enable_or_unmask(struct irq_data *d)
 {
-	unsigned long mask = 1 << d->hwirq;
+	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+	unsigned long mask = BIT(d->hwirq);
 
 	pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
 
@@ -69,30 +72,35 @@ static void intc_enable_or_unmask(struct irq_data *d)
 	 * acks the irq before calling the interrupt handler
 	 */
 	if (irqd_is_level_type(d))
-		xintc_write(IAR, mask);
+		xintc_write(irqc, IAR, mask);
 
-	xintc_write(SIE, mask);
+	xintc_write(irqc, SIE, mask);
 }
 
 static void intc_disable_or_mask(struct irq_data *d)
 {
+	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+
 	pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
-	xintc_write(CIE, 1 << d->hwirq);
+	xintc_write(irqc, CIE, BIT(d->hwirq));
 }
 
 static void intc_ack(struct irq_data *d)
 {
+	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+
 	pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
-	xintc_write(IAR, 1 << d->hwirq);
+	xintc_write(irqc, IAR, BIT(d->hwirq));
 }
 
 static void intc_mask_ack(struct irq_data *d)
 {
-	unsigned long mask = 1 << d->hwirq;
+	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
+	unsigned long mask = BIT(d->hwirq);
 
 	pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
-	xintc_write(CIE, mask);
-	xintc_write(IAR, mask);
+	xintc_write(irqc, CIE, mask);
+	xintc_write(irqc, IAR, mask);
 }
 
 static struct irq_chip intc_dev = {
@@ -103,13 +111,28 @@ static struct irq_chip intc_dev = {
 	.irq_mask_ack = intc_mask_ack,
 };
 
+static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc)
+{
+	unsigned int irq = 0;
+	u32 hwirq;
+
+	hwirq = xintc_read(irqc, IVR);
+	if (hwirq != -1U)
+		irq = irq_find_mapping(irqc->root_domain, hwirq);
+
+	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
+
+	return irq;
+}
+
 unsigned int xintc_get_irq(void)
 {
-	unsigned int hwirq, irq = -1;
+	unsigned int irq = -1;
+	u32 hwirq;
 
-	hwirq = xintc_read(IVR);
+	hwirq = xintc_read(primary_intc, IVR);
 	if (hwirq != -1U)
-		irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
+		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
 
 	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
 
@@ -118,15 +141,18 @@ unsigned int xintc_get_irq(void)
 
 static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
-	if (xintc_irqc->intr_mask & (1 << hw)) {
-		irq_set_chip_and_handler_name(irq, &intc_dev,
-						handle_edge_irq, "edge");
+	struct xintc_irq_chip *irqc = d->host_data;
+
+	if (irqc->intr_mask & BIT(hw)) {
+		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
+					      handle_edge_irq, "edge");
 		irq_clear_status_flags(irq, IRQ_LEVEL);
 	} else {
-		irq_set_chip_and_handler_name(irq, &intc_dev,
-						handle_level_irq, "level");
+		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
+					      handle_level_irq, "level");
 		irq_set_status_flags(irq, IRQ_LEVEL);
 	}
+	irq_set_chip_data(irq, irqc);
 	return 0;
 }
 
@@ -138,12 +164,14 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
 static void xil_intc_irq_handler(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct xintc_irq_chip *irqc;
 	u32 pending;
 
+	irqc = irq_data_get_irq_handler_data(&desc->irq_data);
 	chained_irq_enter(chip, desc);
 	do {
-		pending = xintc_get_irq();
-		if (pending == -1U)
+		pending = xintc_get_irq_local(irqc);
+		if (pending == 0U)
 			break;
 		generic_handle_irq(pending);
 	} while (true);
@@ -153,28 +181,19 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
 static int __init xilinx_intc_of_init(struct device_node *intc,
 					     struct device_node *parent)
 {
-	u32 nr_irq;
-	int ret, irq;
 	struct xintc_irq_chip *irqc;
-
-	if (xintc_irqc) {
-		pr_err("irq-xilinx: Multiple instances aren't supported\n");
-		return -EINVAL;
-	}
+	int ret, irq;
 
 	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
 	if (!irqc)
 		return -ENOMEM;
-
-	xintc_irqc = irqc;
-
 	irqc->base = of_iomap(intc, 0);
 	BUG_ON(!irqc->base);
 
-	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
+	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq);
 	if (ret < 0) {
 		pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
-		goto err_alloc;
+		goto error;
 	}
 
 	ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
@@ -183,34 +202,35 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 		irqc->intr_mask = 0;
 	}
 
-	if (irqc->intr_mask >> nr_irq)
+	if (irqc->intr_mask >> irqc->nr_irq)
 		pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
 
 	pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
-		intc, nr_irq, irqc->intr_mask);
+		intc, irqc->nr_irq, irqc->intr_mask);
 
+	irqc->intc_dev = &intc_dev;
 
 	/*
 	 * Disable all external interrupts until they are
 	 * explicity requested.
 	 */
-	xintc_write(IER, 0);
+	xintc_write(irqc, IER, 0);
 
 	/* Acknowledge any pending interrupts just in case. */
-	xintc_write(IAR, 0xffffffff);
+	xintc_write(irqc, IAR, 0xffffffff);
 
 	/* Turn on the Master Enable. */
-	xintc_write(MER, MER_HIE | MER_ME);
-	if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
+	xintc_write(irqc, MER, MER_HIE | MER_ME);
+	if (xintc_read(irqc, MER) != (MER_HIE | MER_ME)) {
 		static_branch_enable(&xintc_is_be);
-		xintc_write(MER, MER_HIE | MER_ME);
+		xintc_write(irqc, MER, MER_HIE | MER_ME);
 	}
 
-	irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
+	irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
 						  &xintc_irq_domain_ops, irqc);
 	if (!irqc->root_domain) {
 		pr_err("irq-xilinx: Unable to create IRQ domain\n");
-		goto err_alloc;
+		goto error;
 	}
 
 	if (parent) {
@@ -222,16 +242,17 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 		} else {
 			pr_err("irq-xilinx: interrupts property not in DT\n");
 			ret = -EINVAL;
-			goto err_alloc;
+			goto error;
 		}
 	} else {
-		irq_set_default_host(irqc->root_domain);
+		primary_intc = irqc;
+		irq_set_default_host(primary_intc->root_domain);
 	}
 
 	return 0;
 
-err_alloc:
-	xintc_irqc = NULL;
+error:
+	iounmap(irqc->base);
 	kfree(irqc);
 	return ret;
 
-- 
2.25.0

[PATCH v4 2/3] irqchip: xilinx: Fill error code when irq domain registration fails

[Mubin Usman Sayyed]

From: Michal Simek <michal.simek@xilinx.com>

There is no ret filled in case of irq_domain_add_linear() failure.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Stefan Asserhall <stefan.asserhall@xilinx.com>
---
 drivers/irqchip/irq-xilinx-intc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index 65b77720ef2e..f2a359e2feaa 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -230,6 +230,7 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 						  &xintc_irq_domain_ops, irqc);
 	if (!irqc->root_domain) {
 		pr_err("irq-xilinx: Unable to create IRQ domain\n");
+		ret = -EINVAL;
 		goto error;
 	}
 
-- 
2.25.0

[PATCH v4 3/3] irqchip: xilinx: Enable generic irq multi handler

[Mubin Usman Sayyed]

From: Michal Simek <michal.simek@xilinx.com>

Register default arch handler via driver instead of directly pointing to
xilinx intc controller. This patch makes architecture code more generic.

Driver calls generic domain specific irq handler which does the most of
things self. Also get rid of concurrent_irq counting which hasn't been
exported anywhere.
Based on this loop was also optimized by using do/while loop instead of
goto loop.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Stefan Asserhall <stefan.asserhall@xilinx.com>
---
 arch/microblaze/Kconfig           |  2 ++
 arch/microblaze/include/asm/irq.h |  3 ---
 arch/microblaze/kernel/irq.c      | 21 +------------------
 drivers/irqchip/irq-xilinx-intc.c | 34 ++++++++++++++++++-------------
 4 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
index a75896f18e58..b5a87c294925 100644
--- a/arch/microblaze/Kconfig
+++ b/arch/microblaze/Kconfig
@@ -47,6 +47,8 @@ config MICROBLAZE
 	select CPU_NO_EFFICIENT_FFS
 	select MMU_GATHER_NO_RANGE if MMU
 	select SPARSE_IRQ
+	select GENERIC_IRQ_MULTI_HANDLER
+	select HANDLE_DOMAIN_IRQ
 
 # Endianness selection
 choice
diff --git a/arch/microblaze/include/asm/irq.h b/arch/microblaze/include/asm/irq.h
index eac2fb4b3fb9..5166f0893e2b 100644
--- a/arch/microblaze/include/asm/irq.h
+++ b/arch/microblaze/include/asm/irq.h
@@ -14,7 +14,4 @@
 struct pt_regs;
 extern void do_IRQ(struct pt_regs *regs);
 
-/* should be defined in each interrupt controller driver */
-extern unsigned int xintc_get_irq(void);
-
 #endif /* _ASM_MICROBLAZE_IRQ_H */
diff --git a/arch/microblaze/kernel/irq.c b/arch/microblaze/kernel/irq.c
index 903dad822fad..0b37dde60a1e 100644
--- a/arch/microblaze/kernel/irq.c
+++ b/arch/microblaze/kernel/irq.c
@@ -20,29 +20,10 @@
 #include <linux/irqchip.h>
 #include <linux/of_irq.h>
 
-static u32 concurrent_irq;
-
 void __irq_entry do_IRQ(struct pt_regs *regs)
 {
-	unsigned int irq;
-	struct pt_regs *old_regs = set_irq_regs(regs);
 	trace_hardirqs_off();
-
-	irq_enter();
-	irq = xintc_get_irq();
-next_irq:
-	BUG_ON(!irq);
-	generic_handle_irq(irq);
-
-	irq = xintc_get_irq();
-	if (irq != -1U) {
-		pr_debug("next irq: %d\n", irq);
-		++concurrent_irq;
-		goto next_irq;
-	}
-
-	irq_exit();
-	set_irq_regs(old_regs);
+	handle_arch_irq(regs);
 	trace_hardirqs_on();
 }
 
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index f2a359e2feaa..390476b8d719 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -125,20 +125,6 @@ static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc)
 	return irq;
 }
 
-unsigned int xintc_get_irq(void)
-{
-	unsigned int irq = -1;
-	u32 hwirq;
-
-	hwirq = xintc_read(primary_intc, IVR);
-	if (hwirq != -1U)
-		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
-
-	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
-
-	return irq;
-}
-
 static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
 	struct xintc_irq_chip *irqc = d->host_data;
@@ -178,6 +164,25 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static void xil_intc_handle_irq(struct pt_regs *regs)
+{
+	u32 hwirq;
+	struct xintc_irq_chip *irqc = primary_intc;
+
+	do {
+		hwirq = xintc_read(irqc, IVR);
+		if (likely(hwirq != -1U)) {
+			int ret;
+
+			ret = handle_domain_irq(irqc->root_domain, hwirq, regs);
+			WARN_ONCE(ret, "Unhandled HWIRQ %d\n", hwirq);
+			continue;
+		}
+
+		break;
+	} while (1);
+}
+
 static int __init xilinx_intc_of_init(struct device_node *intc,
 					     struct device_node *parent)
 {
@@ -248,6 +253,7 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
 	} else {
 		primary_intc = irqc;
 		irq_set_default_host(primary_intc->root_domain);
+		set_handle_irq(xil_intc_handle_irq);
 	}
 
 	return 0;
-- 
2.25.0

Re: [PATCH v4 1/3] irqchip: xilinx: Add support for multiple instances

[Marc Zyngier]

On 2020-03-16 13:54, Mubin Usman Sayyed wrote:
> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> 
> Added support for cascaded interrupt controllers.
> 
> Following cascaded configurations have been tested,
> 
> - peripheral->xilinx-intc->xilinx-intc->gic->Cortexa53 processor
>   on zcu102 board
> - peripheral->xilinx-intc->xilinx-intc->microblaze processor
>   on kcu105 board
> 
> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> ---
> Changes for v4:
> 	- Fixed review comments from Thomas - updated commit
> 	  message, variable declarations changed to reverse
> 	  fir tree and cleaned up some code.
> 
> Changes for v3:
> 	- Modified prototype of xintc_write/xintc_read
> 	- Fixed review comments regarding indentation/variable
> 	  names, used BIT
> 	- Modified xintc_get_irq_local to return 0
> 	  in case of failure/no pending interrupts
> 	- Fixed return type of xintc_read
> 	- Reverted changes related to device name and
> 	  kept intc_dev as static
> 
> Changes for v2:
>         - Removed write_fn/read_fn hooks, used xintc_write/
> 	  xintc_read directly
>         - Moved primary_intc declaration after xintc_irq_chip
> ---
>  drivers/irqchip/irq-xilinx-intc.c | 121 ++++++++++++++++++------------
>  1 file changed, 71 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-xilinx-intc.c
> b/drivers/irqchip/irq-xilinx-intc.c
> index e3043ded8973..65b77720ef2e 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
>  	void		__iomem *base;
>  	struct		irq_domain *root_domain;
>  	u32		intr_mask;
> +	struct		irq_chip *intc_dev;

What is the need for this pointer? As far as I can see, all the 
interrupts
have the same callbacks, and even then, there should be no need to keep
a pointer to that.

> +	u32		nr_irq;
>  };
> 
> -static struct xintc_irq_chip *xintc_irqc;
> +static struct xintc_irq_chip *primary_intc;
> 
> -static void xintc_write(int reg, u32 data)
> +static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 
> data)
>  {
>  	if (static_branch_unlikely(&xintc_is_be))
> -		iowrite32be(data, xintc_irqc->base + reg);
> +		iowrite32be(data, irqc->base + reg);
>  	else
> -		iowrite32(data, xintc_irqc->base + reg);
> +		iowrite32(data, irqc->base + reg);
>  }
> 
> -static unsigned int xintc_read(int reg)
> +static u32 xintc_read(struct xintc_irq_chip *irqc, int reg)
>  {
>  	if (static_branch_unlikely(&xintc_is_be))
> -		return ioread32be(xintc_irqc->base + reg);
> +		return ioread32be(irqc->base + reg);
>  	else
> -		return ioread32(xintc_irqc->base + reg);
> +		return ioread32(irqc->base + reg);
>  }
> 
>  static void intc_enable_or_unmask(struct irq_data *d)
>  {
> -	unsigned long mask = 1 << d->hwirq;
> +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +	unsigned long mask = BIT(d->hwirq);
> 
>  	pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> 
> @@ -69,30 +72,35 @@ static void intc_enable_or_unmask(struct irq_data 
> *d)
>  	 * acks the irq before calling the interrupt handler
>  	 */
>  	if (irqd_is_level_type(d))
> -		xintc_write(IAR, mask);
> +		xintc_write(irqc, IAR, mask);
> 
> -	xintc_write(SIE, mask);
> +	xintc_write(irqc, SIE, mask);
>  }
> 
>  static void intc_disable_or_mask(struct irq_data *d)
>  {
> +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
>  	pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> -	xintc_write(CIE, 1 << d->hwirq);
> +	xintc_write(irqc, CIE, BIT(d->hwirq));
>  }
> 
>  static void intc_ack(struct irq_data *d)
>  {
> +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +
>  	pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> -	xintc_write(IAR, 1 << d->hwirq);
> +	xintc_write(irqc, IAR, BIT(d->hwirq));
>  }
> 
>  static void intc_mask_ack(struct irq_data *d)
>  {
> -	unsigned long mask = 1 << d->hwirq;
> +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> +	unsigned long mask = BIT(d->hwirq);
> 
>  	pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> -	xintc_write(CIE, mask);
> -	xintc_write(IAR, mask);
> +	xintc_write(irqc, CIE, mask);
> +	xintc_write(irqc, IAR, mask);
>  }
> 
>  static struct irq_chip intc_dev = {
> @@ -103,13 +111,28 @@ static struct irq_chip intc_dev = {
>  	.irq_mask_ack = intc_mask_ack,
>  };
> 
> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc)
> +{
> +	unsigned int irq = 0;
> +	u32 hwirq;
> +
> +	hwirq = xintc_read(irqc, IVR);
> +	if (hwirq != -1U)
> +		irq = irq_find_mapping(irqc->root_domain, hwirq);
> +
> +	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +
> +	return irq;
> +}
> +
>  unsigned int xintc_get_irq(void)
>  {
> -	unsigned int hwirq, irq = -1;
> +	unsigned int irq = -1;
> +	u32 hwirq;
> 
> -	hwirq = xintc_read(IVR);
> +	hwirq = xintc_read(primary_intc, IVR);
>  	if (hwirq != -1U)
> -		irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> +		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> 
>  	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> 
> @@ -118,15 +141,18 @@ unsigned int xintc_get_irq(void)
> 
>  static int xintc_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hw)
>  {
> -	if (xintc_irqc->intr_mask & (1 << hw)) {
> -		irq_set_chip_and_handler_name(irq, &intc_dev,
> -						handle_edge_irq, "edge");
> +	struct xintc_irq_chip *irqc = d->host_data;
> +
> +	if (irqc->intr_mask & BIT(hw)) {
> +		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
> +					      handle_edge_irq, "edge");
>  		irq_clear_status_flags(irq, IRQ_LEVEL);
>  	} else {
> -		irq_set_chip_and_handler_name(irq, &intc_dev,
> -						handle_level_irq, "level");
> +		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
> +					      handle_level_irq, "level");
>  		irq_set_status_flags(irq, IRQ_LEVEL);
>  	}
> +	irq_set_chip_data(irq, irqc);
>  	return 0;
>  }
> 
> @@ -138,12 +164,14 @@ static const struct irq_domain_ops
> xintc_irq_domain_ops = {
>  static void xil_intc_irq_handler(struct irq_desc *desc)
>  {
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct xintc_irq_chip *irqc;
>  	u32 pending;
> 
> +	irqc = irq_data_get_irq_handler_data(&desc->irq_data);
>  	chained_irq_enter(chip, desc);
>  	do {
> -		pending = xintc_get_irq();
> -		if (pending == -1U)
> +		pending = xintc_get_irq_local(irqc);
> +		if (pending == 0U)

nit: I don't think we need to consider the sign of zero.

>  			break;
>  		generic_handle_irq(pending);
>  	} while (true);
> @@ -153,28 +181,19 @@ static void xil_intc_irq_handler(struct irq_desc 
> *desc)
>  static int __init xilinx_intc_of_init(struct device_node *intc,
>  					     struct device_node *parent)
>  {
> -	u32 nr_irq;
> -	int ret, irq;
>  	struct xintc_irq_chip *irqc;
> -
> -	if (xintc_irqc) {
> -		pr_err("irq-xilinx: Multiple instances aren't supported\n");
> -		return -EINVAL;
> -	}
> +	int ret, irq;
> 
>  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>  	if (!irqc)
>  		return -ENOMEM;
> -
> -	xintc_irqc = irqc;
> -
>  	irqc->base = of_iomap(intc, 0);
>  	BUG_ON(!irqc->base);
> 
> -	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> +	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", 
> &irqc->nr_irq);
>  	if (ret < 0) {
>  		pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> -		goto err_alloc;
> +		goto error;
>  	}
> 
>  	ret = of_property_read_u32(intc, "xlnx,kind-of-intr", 
> &irqc->intr_mask);
> @@ -183,34 +202,35 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
>  		irqc->intr_mask = 0;
>  	}
> 
> -	if (irqc->intr_mask >> nr_irq)
> +	if (irqc->intr_mask >> irqc->nr_irq)
>  		pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
> 
>  	pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> -		intc, nr_irq, irqc->intr_mask);
> +		intc, irqc->nr_irq, irqc->intr_mask);
> 
> +	irqc->intc_dev = &intc_dev;

Based on the above, this should go.

> 
>  	/*
>  	 * Disable all external interrupts until they are
>  	 * explicity requested.
>  	 */
> -	xintc_write(IER, 0);
> +	xintc_write(irqc, IER, 0);
> 
>  	/* Acknowledge any pending interrupts just in case. */
> -	xintc_write(IAR, 0xffffffff);
> +	xintc_write(irqc, IAR, 0xffffffff);
> 
>  	/* Turn on the Master Enable. */
> -	xintc_write(MER, MER_HIE | MER_ME);
> -	if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> +	xintc_write(irqc, MER, MER_HIE | MER_ME);
> +	if (xintc_read(irqc, MER) != (MER_HIE | MER_ME)) {
>  		static_branch_enable(&xintc_is_be);
> -		xintc_write(MER, MER_HIE | MER_ME);
> +		xintc_write(irqc, MER, MER_HIE | MER_ME);
>  	}
> 
> -	irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> +	irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
>  						  &xintc_irq_domain_ops, irqc);
>  	if (!irqc->root_domain) {
>  		pr_err("irq-xilinx: Unable to create IRQ domain\n");
> -		goto err_alloc;
> +		goto error;
>  	}
> 
>  	if (parent) {
> @@ -222,16 +242,17 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
>  		} else {
>  			pr_err("irq-xilinx: interrupts property not in DT\n");
>  			ret = -EINVAL;
> -			goto err_alloc;
> +			goto error;
>  		}
>  	} else {
> -		irq_set_default_host(irqc->root_domain);
> +		primary_intc = irqc;
> +		irq_set_default_host(primary_intc->root_domain);

Do you still need this irq_set_default_host() horror? I thought 
microblaze
was fully DT-ified and didn't need this. The use of a non-legacy domain 
tends
to confirm this.

>  	}
> 
>  	return 0;
> 
> -err_alloc:
> -	xintc_irqc = NULL;
> +error:
> +	iounmap(irqc->base);
>  	kfree(irqc);
>  	return ret;

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v4 1/3] irqchip: xilinx: Add support for multiple instances

[Mubin Usman Sayyed]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, March 16, 2020 8:04 PM
> To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; Michal Simek
> <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Siva Durga Prasad Paladugu <sivadur@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>; Anirudha Sarangi
> <anirudh@xilinx.com>
> Subject: Re: [PATCH v4 1/3] irqchip: xilinx: Add support for multiple instances
> 
> On 2020-03-16 13:54, Mubin Usman Sayyed wrote:
> > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> >
> > Added support for cascaded interrupt controllers.
> >
> > Following cascaded configurations have been tested,
> >
> > - peripheral->xilinx-intc->xilinx-intc->gic->Cortexa53 processor
> >   on zcu102 board
> > - peripheral->xilinx-intc->xilinx-intc->microblaze processor
> >   on kcu105 board
> >
> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> > ---
> > Changes for v4:
> > 	- Fixed review comments from Thomas - updated commit
> > 	  message, variable declarations changed to reverse
> > 	  fir tree and cleaned up some code.
> >
> > Changes for v3:
> > 	- Modified prototype of xintc_write/xintc_read
> > 	- Fixed review comments regarding indentation/variable
> > 	  names, used BIT
> > 	- Modified xintc_get_irq_local to return 0
> > 	  in case of failure/no pending interrupts
> > 	- Fixed return type of xintc_read
> > 	- Reverted changes related to device name and
> > 	  kept intc_dev as static
> >
> > Changes for v2:
> >         - Removed write_fn/read_fn hooks, used xintc_write/
> > 	  xintc_read directly
> >         - Moved primary_intc declaration after xintc_irq_chip
> > ---
> >  drivers/irqchip/irq-xilinx-intc.c | 121
> > ++++++++++++++++++------------
> >  1 file changed, 71 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c
> > b/drivers/irqchip/irq-xilinx-intc.c
> > index e3043ded8973..65b77720ef2e 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> >  	void		__iomem *base;
> >  	struct		irq_domain *root_domain;
> >  	u32		intr_mask;
> > +	struct		irq_chip *intc_dev;
> 
> What is the need for this pointer? As far as I can see, all the interrupts have
> the same callbacks, and even then, there should be no need to keep a
> pointer to that.
[Mubin]: Agreed, I will fix it in next version.
> 
> > +	u32		nr_irq;
> >  };
> >
> > -static struct xintc_irq_chip *xintc_irqc;
> > +static struct xintc_irq_chip *primary_intc;
> >
> > -static void xintc_write(int reg, u32 data)
> > +static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32
> > data)
> >  {
> >  	if (static_branch_unlikely(&xintc_is_be))
> > -		iowrite32be(data, xintc_irqc->base + reg);
> > +		iowrite32be(data, irqc->base + reg);
> >  	else
> > -		iowrite32(data, xintc_irqc->base + reg);
> > +		iowrite32(data, irqc->base + reg);
> >  }
> >
> > -static unsigned int xintc_read(int reg)
> > +static u32 xintc_read(struct xintc_irq_chip *irqc, int reg)
> >  {
> >  	if (static_branch_unlikely(&xintc_is_be))
> > -		return ioread32be(xintc_irqc->base + reg);
> > +		return ioread32be(irqc->base + reg);
> >  	else
> > -		return ioread32(xintc_irqc->base + reg);
> > +		return ioread32(irqc->base + reg);
> >  }
> >
> >  static void intc_enable_or_unmask(struct irq_data *d)  {
> > -	unsigned long mask = 1 << d->hwirq;
> > +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +	unsigned long mask = BIT(d->hwirq);
> >
> >  	pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> >
> > @@ -69,30 +72,35 @@ static void intc_enable_or_unmask(struct irq_data
> > *d)
> >  	 * acks the irq before calling the interrupt handler
> >  	 */
> >  	if (irqd_is_level_type(d))
> > -		xintc_write(IAR, mask);
> > +		xintc_write(irqc, IAR, mask);
> >
> > -	xintc_write(SIE, mask);
> > +	xintc_write(irqc, SIE, mask);
> >  }
> >
> >  static void intc_disable_or_mask(struct irq_data *d)  {
> > +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> >  	pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> > -	xintc_write(CIE, 1 << d->hwirq);
> > +	xintc_write(irqc, CIE, BIT(d->hwirq));
> >  }
> >
> >  static void intc_ack(struct irq_data *d)  {
> > +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +
> >  	pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> > -	xintc_write(IAR, 1 << d->hwirq);
> > +	xintc_write(irqc, IAR, BIT(d->hwirq));
> >  }
> >
> >  static void intc_mask_ack(struct irq_data *d)  {
> > -	unsigned long mask = 1 << d->hwirq;
> > +	struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> > +	unsigned long mask = BIT(d->hwirq);
> >
> >  	pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> > -	xintc_write(CIE, mask);
> > -	xintc_write(IAR, mask);
> > +	xintc_write(irqc, CIE, mask);
> > +	xintc_write(irqc, IAR, mask);
> >  }
> >
> >  static struct irq_chip intc_dev = {
> > @@ -103,13 +111,28 @@ static struct irq_chip intc_dev = {
> >  	.irq_mask_ack = intc_mask_ack,
> >  };
> >
> > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc)
> > +{
> > +	unsigned int irq = 0;
> > +	u32 hwirq;
> > +
> > +	hwirq = xintc_read(irqc, IVR);
> > +	if (hwirq != -1U)
> > +		irq = irq_find_mapping(irqc->root_domain, hwirq);
> > +
> > +	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> > +
> > +	return irq;
> > +}
> > +
> >  unsigned int xintc_get_irq(void)
> >  {
> > -	unsigned int hwirq, irq = -1;
> > +	unsigned int irq = -1;
> > +	u32 hwirq;
> >
> > -	hwirq = xintc_read(IVR);
> > +	hwirq = xintc_read(primary_intc, IVR);
> >  	if (hwirq != -1U)
> > -		irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> > +		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> >
> >  	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> >
> > @@ -118,15 +141,18 @@ unsigned int xintc_get_irq(void)
> >
> >  static int xintc_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hw)  {
> > -	if (xintc_irqc->intr_mask & (1 << hw)) {
> > -		irq_set_chip_and_handler_name(irq, &intc_dev,
> > -						handle_edge_irq, "edge");
> > +	struct xintc_irq_chip *irqc = d->host_data;
> > +
> > +	if (irqc->intr_mask & BIT(hw)) {
> > +		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
> > +					      handle_edge_irq, "edge");
> >  		irq_clear_status_flags(irq, IRQ_LEVEL);
> >  	} else {
> > -		irq_set_chip_and_handler_name(irq, &intc_dev,
> > -						handle_level_irq, "level");
> > +		irq_set_chip_and_handler_name(irq, irqc->intc_dev,
> > +					      handle_level_irq, "level");
> >  		irq_set_status_flags(irq, IRQ_LEVEL);
> >  	}
> > +	irq_set_chip_data(irq, irqc);
> >  	return 0;
> >  }
> >
> > @@ -138,12 +164,14 @@ static const struct irq_domain_ops
> > xintc_irq_domain_ops = {  static void xil_intc_irq_handler(struct
> > irq_desc *desc)  {
> >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct xintc_irq_chip *irqc;
> >  	u32 pending;
> >
> > +	irqc = irq_data_get_irq_handler_data(&desc->irq_data);
> >  	chained_irq_enter(chip, desc);
> >  	do {
> > -		pending = xintc_get_irq();
> > -		if (pending == -1U)
> > +		pending = xintc_get_irq_local(irqc);
> > +		if (pending == 0U)
> 
> nit: I don't think we need to consider the sign of zero.
[Mubin]: I will fix it in next version.
> 
> >  			break;
> >  		generic_handle_irq(pending);
> >  	} while (true);
> > @@ -153,28 +181,19 @@ static void xil_intc_irq_handler(struct irq_desc
> > *desc)
> >  static int __init xilinx_intc_of_init(struct device_node *intc,
> >  					     struct device_node *parent)  {
> > -	u32 nr_irq;
> > -	int ret, irq;
> >  	struct xintc_irq_chip *irqc;
> > -
> > -	if (xintc_irqc) {
> > -		pr_err("irq-xilinx: Multiple instances aren't supported\n");
> > -		return -EINVAL;
> > -	}
> > +	int ret, irq;
> >
> >  	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> >  	if (!irqc)
> >  		return -ENOMEM;
> > -
> > -	xintc_irqc = irqc;
> > -
> >  	irqc->base = of_iomap(intc, 0);
> >  	BUG_ON(!irqc->base);
> >
> > -	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> > +	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > &irqc->nr_irq);
> >  	if (ret < 0) {
> >  		pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> > -		goto err_alloc;
> > +		goto error;
> >  	}
> >
> >  	ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> > &irqc->intr_mask);
> > @@ -183,34 +202,35 @@ static int __init xilinx_intc_of_init(struct
> > device_node *intc,
> >  		irqc->intr_mask = 0;
> >  	}
> >
> > -	if (irqc->intr_mask >> nr_irq)
> > +	if (irqc->intr_mask >> irqc->nr_irq)
> >  		pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
> >
> >  	pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> > -		intc, nr_irq, irqc->intr_mask);
> > +		intc, irqc->nr_irq, irqc->intr_mask);
> >
> > +	irqc->intc_dev = &intc_dev;
> 
> Based on the above, this should go.
[Mubin]: will do it in next version
> 
> >
> >  	/*
> >  	 * Disable all external interrupts until they are
> >  	 * explicity requested.
> >  	 */
> > -	xintc_write(IER, 0);
> > +	xintc_write(irqc, IER, 0);
> >
> >  	/* Acknowledge any pending interrupts just in case. */
> > -	xintc_write(IAR, 0xffffffff);
> > +	xintc_write(irqc, IAR, 0xffffffff);
> >
> >  	/* Turn on the Master Enable. */
> > -	xintc_write(MER, MER_HIE | MER_ME);
> > -	if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> > +	xintc_write(irqc, MER, MER_HIE | MER_ME);
> > +	if (xintc_read(irqc, MER) != (MER_HIE | MER_ME)) {
> >  		static_branch_enable(&xintc_is_be);
> > -		xintc_write(MER, MER_HIE | MER_ME);
> > +		xintc_write(irqc, MER, MER_HIE | MER_ME);
> >  	}
> >
> > -	irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> > +	irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> >  						  &xintc_irq_domain_ops,
> irqc);
> >  	if (!irqc->root_domain) {
> >  		pr_err("irq-xilinx: Unable to create IRQ domain\n");
> > -		goto err_alloc;
> > +		goto error;
> >  	}
> >
> >  	if (parent) {
> > @@ -222,16 +242,17 @@ static int __init xilinx_intc_of_init(struct
> > device_node *intc,
> >  		} else {
> >  			pr_err("irq-xilinx: interrupts property not in DT\n");
> >  			ret = -EINVAL;
> > -			goto err_alloc;
> > +			goto error;
> >  		}
> >  	} else {
> > -		irq_set_default_host(irqc->root_domain);
> > +		primary_intc = irqc;
> > +		irq_set_default_host(primary_intc->root_domain);
> 
> Do you still need this irq_set_default_host() horror? I thought
> microblaze
> was fully DT-ified and didn't need this. The use of a non-legacy domain
> tends
> to confirm this.
[Mubin]: I will add  patch in next version to remove it.

Thanks,
Mubin
> 
> >  	}
> >
> >  	return 0;
> >
> > -err_alloc:
> > -	xintc_irqc = NULL;
> > +error:
> > +	iounmap(irqc->base);
> >  	kfree(irqc);
> >  	return ret;
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...