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

[Zubair Lutfullah Kakakhel]

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.

Thanks
ZubairLK

> Thanks,
>
> 	M.