From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751989AbcAERnJ (ORCPT ); Tue, 5 Jan 2016 12:43:09 -0500 Received: from exsmtp01.microchip.com ([198.175.253.37]:50491 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751137AbcAERnH (ORCPT ); Tue, 5 Jan 2016 12:43:07 -0500 From: Joshua Henderson Subject: Re: [PATCH v2 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller To: Marc Zyngier , References: <1450133093-7053-1-git-send-email-joshua.henderson@microchip.com> <1450133093-7053-3-git-send-email-joshua.henderson@microchip.com> <5670471B.6090608@arm.com> CC: , , Cristian Birsan , Thomas Gleixner , Jason Cooper Message-ID: <568C0271.2020007@microchip.com> Date: Tue, 5 Jan 2016 10:50:41 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <5670471B.6090608@arm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Marc, On 12/15/2015 10:00 AM, Marc Zyngier wrote: > On 14/12/15 22:42, Joshua Henderson wrote: >> From: Cristian Birsan >> >> 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 >> Signed-off-by: Joshua Henderson >> Cc: Ralf Baechle >> --- >> 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