Re: [PATCH v2 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

[Joshua Henderson <joshua.henderson@microchip.com>]

Marc,

On 12/15/2015 10:00 AM, Marc Zyngier wrote:
> On 14/12/15 22:42, Joshua Henderson wrote:
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>
>> This adds support for the interrupt controller present on PIC32 class
>> devices.
>>
>> The following features are supported:
>>  - DT properties for EVIC and for devices that use interrupt lines
>>  - Persistent and non-persistent interrupt handling
>>  - irqdomain support
>>
>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> ---
>>  drivers/irqchip/Makefile           |    1 +
>>  drivers/irqchip/irq-pic32-evic.c   |  321 ++++++++++++++++++++++++++++++++++++
>>  include/linux/irqchip/pic32-evic.h |   19 +++
>>  3 files changed, 341 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-pic32-evic.c
>>  create mode 100644 include/linux/irqchip/pic32-evic.h
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 177f78f..e3608fc 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
>>  obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>> +obj-$(CONFIG_MACH_PIC32)		+= irq-pic32-evic.o
>> diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c
>> new file mode 100644
>> index 0000000..6a7747c
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-pic32-evic.c

[...]

>> +
>> +static struct irq_chip pic32_irq_chip = {
>> +	.name = "PIC32-EVIC",
>> +	.irq_ack = ack_pic32_irq,
>> +	.irq_mask = mask_pic32_irq,
>> +	.irq_mask_ack = mask_ack_pic32_irq,
>> +	.irq_unmask = unmask_pic32_irq,
>> +	.irq_eoi = ack_pic32_irq,
>> +	.irq_set_type = set_type_pic32_irq,
>> +	.irq_enable = unmask_pic32_irq,
>> +	.irq_disable = mask_pic32_irq,
> 
> I'm not sure I see the point of having all these methods. First, there
> is a lot of duplication: no need to provide enable/disable if all you
> have is mask/unmask - the core code can deal with that.

This is to avoid the lazy disable approach in irq_disable(). The .irq_enable is there for symmetry.  .irq_mask_ack is also redundant excluding performance.  These can be removed if the preference is to end up with:

static struct irq_chip pic32_irq_chip = {
	.name = "PIC32-EVIC",
	.irq_ack = ack_pic32_irq,
	.irq_mask = mask_pic32_irq,
	.irq_unmask = unmask_pic32_irq,
	.irq_eoi = ack_pic32_irq,
	.irq_set_type = set_type_pic32_irq,
};

> 
> Then, your EOI method is not really an EOI. It doesn't terminate the
> handling, or at least that's not what the name suggest. If this is
> really an EOI, then you should be able to simplify the whole thing on
> only use the fasteoi handler, including for edge interrupts.

There are two types of hardware interrupts: persistent and non persistent. For the persistent ones we need to clear the condition that caused the interrupt before clearing the interrupt flag and this one is mapped to the fasteoi handler. For the non persistent ones we can clear the interrupt flag as soon as we enter the interrupt handler, but we need to re-enable the interrupt to avoid missing any event that occur during servicing the interrupt. This one is mapped to the edge handler. This is needed, for example, with the core timer interrupt.

>
> It would be good to have an insight on how this thing actually works
> (I've tried to read the only documentation, but this is vague at best),
> that would help picking the right design for your use case.
> 

We're in agreement here on the lack of documentation.  While this chip is not released to the public yet, we do have a generic document that outlines the interrupt controllers across the PIC32 family that will shed some light on how this thing operates:  http://ww1.microchip.com/downloads/en/DeviceDoc/60001108H.pdf

> Thanks,
> 
> 	M.
> 

Josh