Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc

From: Zubair Lutfullah Kakakhel
Date: Thu Sep 01 2016 - 09:53:15 EST


Hi,

On 09/01/2016 01:14 PM, Marc Zyngier wrote:
On 01/09/16 12:01, Zubair Lutfullah Kakakhel wrote:
Hi,

Thanks for the review
Comments inline.

On 08/31/2016 05:57 PM, Marc Zyngier wrote:
On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
The MIPS based xilfpga platform has the following IRQ structure

Peripherals --> xilinx_intcontroller -> mips_cpu_int controller

Add support for the driver to chain the irq handler

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@xxxxxxxxxx>

---
V2 -> V3
Reused existing parent node instead of finding again.
Cleanup up handler based on review

V1 -> V2

No change
---
drivers/irqchip/irq-axi-intc.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-axi-intc.c b/drivers/irqchip/irq-axi-intc.c
index cb69241..30bb084 100644
--- a/drivers/irqchip/irq-axi-intc.c
+++ b/drivers/irqchip/irq-axi-intc.c
@@ -15,6 +15,7 @@
#include <linux/of_address.h>
#include <linux/io.h>
#include <linux/bug.h>
+#include <linux/of_irq.h>

/* No one else should require these constants, so define them locally here. */
#define ISR 0x00 /* Interrupt Status Register */
@@ -154,11 +155,23 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
.map = xintc_map,
};

+static void xil_intc_irq_handler(struct irq_desc *desc)
+{
+ u32 pending;
+
+ do {
+ pending = get_irq();
+ if (pending == -1U)
+ break;
+ generic_handle_irq(pending);
+ } while (true);
+}
+
static int __init xilinx_intc_of_init(struct device_node *intc,
struct device_node *parent)
{
u32 nr_irq;
- int ret;
+ int ret, irq;
struct xintc_irq_chip *irqc;

irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
@@ -211,6 +224,15 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
irqc);

+ if (parent) {
+ irq = irq_of_parse_and_map(intc, 0);
+ if (irq)
+ irq_set_chained_handler_and_data(irq,
+ xil_intc_irq_handler,
+ root_domain);
+
+ }
+
irq_set_default_host(root_domain);

return 0;


This doesn't seem right. You've now overridden the xintc_irqc pointer,
so I don't know how you can still process interrupts once you've
discovered a secondary interrupt controller. You've also allocated a
second root_domain, changed the default domain to point to the secondary
controller...

Have you tested this code? Or am I missing something obvious?

Yes it works. I'll try to explain the platform setup a bit.
Perhaps that will make it clear about what I'm trying to do.

UART IRQ -> AXI INTC -> MIPS internal INTC -> CPU

MIPS Internal Interrupt controller in drivers/irqchip/irq-mips-cpu.c
uses irq_domain_add_legacy while AXI Intc uses irq_domain_add_linear

My aim was to set up a chained irq handler with least disturbance.

Hence the above code.

Your concerns are valid. The code is working because read/writes rely
on the static xintc_irqc in the file.
And the second root domain is also not breaking the platform because
the irq-mips-cpu.c uses irq_domain_add_legacy and doesn't use
irq_set_default_host.

# cat /proc/interrupts
CPU0
7: 43493 MIPS 7 timer
8: 83 Xilinx INTC 1-level eth0
9: 417 Xilinx INTC 0-level serial
10: 15 Xilinx INTC 4-level 10a00000.i2c
ERR: 0
#

Given the above concerns. How about doing things this way?

if (parent) {
irq = irq_of_parse_and_map(intc, 0);
if (irq)
irq_set_chained_handler_and_data(irq,
xil_intc_irq_handler,
irqc);

} else
irq_set_default_host(root_domain);

default host is only set if no parent exists.
And the irqc pointer is passed as the data.

But that still doesn't address the case I had in mind, which is when you
have *two* AXI-intc, one cascaded into the other. Is that something that
could be built? You should at least make sure that there is a big fat
warning if you don't want to support that case, because that will be
hell to debug.

Oo. I didn't think of that one. tbh, I'm not sure if that is currently supported
because this driver came out of arch/microblaze. And it didn't have any code that
looked supportive of chained interrupt handling.

'Technically', it should be possible to synthesize two daisy chained axi interrupt
controllers on an FPGA. But I don't see it being supported before.

A warning would be nice. Any suggestions on the most suitable way?

Thanks,
ZubairLK


Thanks,

M.