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.