Messages in this thread | | | Subject | Re: [Patch v3 03/11] irqchip: axi-intc: Add support for parent intc | From | Zubair Lutfullah Kakakhel <> | Date | Thu, 1 Sep 2016 12:01:47 +0100 |
| |
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@imgtec.com> >> >> --- >> 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.
Thanks ZubairLK
> > Thanks, > > M. >
| |