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.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
