Messages in this thread | | | Subject | Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 | From | Wan Zongshun <> | Date | Fri, 15 Jul 2016 13:15:58 +0800 |
| |
On 2016年07月14日 21:54, Jason Cooper wrote: > Hi Wan Zongshun, > > On Thu, Jul 14, 2016 at 11:36:53AM +0800, Wan Zongshun wrote: >> On 2016年07月14日 04:09, Jason Cooper wrote: >>> On Sun, Jul 10, 2016 at 03:27:22PM +0800, Wan Zongshun wrote: >>>> This patch is to add irqchip driver support for nuc900 plat, >>>> current this driver only supports nuc970 SoC. >>>> >>>> Signed-off-by: Wan Zongshun <mcuos.com@gmail.com> >>>> --- >>>> arch/arm/mach-w90x900/include/mach/irqs.h | 5 + >>>> drivers/irqchip/Makefile | 1 + >>>> drivers/irqchip/irq-nuc900.c | 150 ++++++++++++++++++++++++++++++ >>>> 3 files changed, 156 insertions(+) >>>> create mode 100644 drivers/irqchip/irq-nuc900.c >>>> >>>> diff --git a/arch/arm/mach-w90x900/include/mach/irqs.h b/arch/arm/mach-w90x900/include/mach/irqs.h >>>> index 9d5cba3..3b035c6 100644 >>>> --- a/arch/arm/mach-w90x900/include/mach/irqs.h >>>> +++ b/arch/arm/mach-w90x900/include/mach/irqs.h >>>> @@ -59,7 +59,12 @@ >>>> #define IRQ_KPI W90X900_IRQ(29) >>>> #define IRQ_P2SGROUP W90X900_IRQ(30) >>>> #define IRQ_ADC W90X900_IRQ(31) >>>> + >>>> +#if !defined(CONFIG_SOC_NUC900) >>>> #define NR_IRQS (IRQ_ADC+1) >>>> +#else >>>> +#define NR_IRQS 62 >>>> +#endif >>> >>> Arnd already covered this... >>> >>>> >>>> /*for irq group*/ >>>> >>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >>>> index 38853a1..9ccd5af8a 100644 >>>> --- a/drivers/irqchip/Makefile >>>> +++ b/drivers/irqchip/Makefile >>>> @@ -69,3 +69,4 @@ obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o >>>> obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o >>>> obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o >>>> obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o >>>> +obj-$(CONFIG_SOC_NUC970) += irq-nuc900.o >>>> diff --git a/drivers/irqchip/irq-nuc900.c b/drivers/irqchip/irq-nuc900.c >>>> new file mode 100644 >>>> index 0000000..c4b2e39 >>>> --- /dev/null >>>> +++ b/drivers/irqchip/irq-nuc900.c >>>> @@ -0,0 +1,150 @@ >>>> +/* >>>> + * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com> >>>> + * >>>> + * The code contained herein is licensed under the GNU General Public >>>> + * License. You may obtain a copy of the GNU General Public License >>>> + * Version 2 or later at the following locations: >>>> + * >>>> + * http://www.opensource.org/licenses/gpl-license.html >>>> + * http://www.gnu.org/copyleft/gpl.html >>>> + */ >>>> + >>>> +#include <linux/module.h> >>>> +#include <linux/init.h> >>>> +#include <linux/irq.h> >>>> +#include <linux/irqchip.h> >>>> +#include <linux/irqdomain.h> >>>> +#include <linux/io.h> >>>> +#include <linux/ioport.h> >>>> +#include <linux/of_address.h> >>>> +#include <linux/of_irq.h> >>>> + >>>> +#include <asm/exception.h> >>>> +#include <asm/hardirq.h> >>>> + >>>> +#define REG_AIC_SCR1 0x00 >>>> +#define REG_AIC_SCR2 0x04 >>>> +#define REG_AIC_SCR3 0x08 >>>> +#define REG_AIC_SCR4 0x0C >>>> +#define REG_AIC_SCR5 0x10 >>>> +#define REG_AIC_SCR6 0x14 >>>> +#define REG_AIC_SCR7 0x18 >>>> +#define REG_AIC_SCR8 0x1C >>>> +#define REG_AIC_SCR9 0x20 >>>> +#define REG_AIC_SCR10 0x24 >>>> +#define REG_AIC_SCR11 0x28 >>>> +#define REG_AIC_SCR12 0x2C >>>> +#define REG_AIC_SCR13 0x30 >>>> +#define REG_AIC_SCR14 0x34 >>>> +#define REG_AIC_SCR15 0x38 >>>> +#define REG_AIC_IRSR 0x100 >>>> +#define REG_AIC_IRSRH 0x104 >>>> +#define REG_AIC_IASR 0x108 >>>> +#define REG_AIC_IASRH 0x10C >>>> +#define REG_AIC_ISR 0x110 >>>> +#define REG_AIC_ISRH 0x114 >>>> +#define REG_AIC_IPER 0x118 >>>> +#define REG_AIC_ISNR 0x120 >>>> +#define REG_AIC_OISR 0x124 >>>> +#define REG_AIC_IMR 0x128 >>>> +#define REG_AIC_IMRH 0x12C >>>> +#define REG_AIC_MECR 0x130 >>>> +#define REG_AIC_MECRH 0x134 >>>> +#define REG_AIC_MDCR 0x138 >>>> +#define REG_AIC_MDCRH 0x13C >>>> +#define REG_AIC_SSCR 0x140 >>>> +#define REG_AIC_SSCRH 0x144 >>>> +#define REG_AIC_SCCR 0x148 >>>> +#define REG_AIC_SCCRH 0x14C >>>> +#define REG_AIC_EOSCR 0x150 >>> >>> Please remove unused defines. >> >> Okay. >> >>> >>>> + >>>> +static void __iomem *aic_base; >>>> +static struct irq_domain *aic_domain; >>>> + >>>> +static void nuc900_irq_mask(struct irq_data *d) >>>> +{ >>>> + if (d->irq < 32) >>>> + writel(1 << (d->irq), aic_base + REG_AIC_MDCR); >>>> + else >>>> + writel(1 << (d->irq - 32), aic_base + REG_AIC_MDCRH); >>>> +} >>>> + >>>> +static void nuc900_irq_ack(struct irq_data *d) >>>> +{ >>>> + writel(0x01, aic_base + REG_AIC_EOSCR); >>>> +} >>>> + >>>> +static void nuc900_irq_unmask(struct irq_data *d) >>>> +{ >>>> + if (d->irq < 32) >>>> + writel(1 << (d->irq), aic_base + REG_AIC_MECR); >>>> + else >>>> + writel(1 << (d->irq - 32), aic_base + REG_AIC_MECRH); >>>> +} >>>> + >>>> +static struct irq_chip nuc900_irq_chip = { >>>> + .irq_ack = nuc900_irq_ack, >>>> + .irq_mask = nuc900_irq_mask, >>>> + .irq_unmask = nuc900_irq_unmask, >>>> +}; >>>> + >>>> +void __exception_irq_entry aic_handle_irq(struct pt_regs *regs) >>>> +{ >>>> + u32 hwirq; >>>> + >>>> + hwirq = readl(aic_base + REG_AIC_IPER); >>>> + hwirq = readl(aic_base + REG_AIC_ISNR); >>>> + if (!hwirq) >>>> + writel(0x01, aic_base + REG_AIC_EOSCR); >>> >>> Could you explain what's going on here? >> >> AIC_IPER, When the AIC generates the interrupt, VECTOR represents >> the interrupt channel number that is active, enabled, and has the >> highest priority. >> >> The value of VECTOR is copied to the register AIC_ISNR thereafter by >> the AIC. This register was restored a value 0 after it was read by >> the >> interrupt handler. > > So, iiuc, the first readl() of AIC_IPER kicks the AIC to copy the value > of VECTOR into AIC_ISNR? Then we can read it? > > If so, please add a comment above the code explaining that. > >> So I must read two registers for one irq number, after read IPER, >> the ISNR(bit0:5) will be updated. ISNR value has no need do offset, >> it is better than read IPER(bit2:7). > > But you aren't using the value from the first readl(...AIC_IPER)? You > overwrite hwirq with the value from the second readl(...AIC_ISNR) > ... > > The first part of your explaination makes sense, but this part isn't > clear to me.
Actually, I have two choice to implement this function:
option1:
void __exception_irq_entry aic_handle_irq(struct pt_regs *regs) { u32 hwirq;
(void)readl(aic_base + REG_AIC_IPER); hwirq = readl(aic_base + REG_AIC_ISNR);
handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs); }
option2:
void __exception_irq_entry aic_handle_irq(struct pt_regs *regs) { u32 hwirq;
hwirq = readl(aic_base + REG_AIC_IPER); hwirq <<= 2;
handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs); }
Though the option2 do shift for hwirq, but it seems better than do io operation by readl,so I prefer to option2, agree?
> > >> “writel(0x01, aic_base + REG_AIC_EOSCR);” seems not necessary here, >> since irqchip has the ack interface. I will remove it. > > Ok, thanks. > >> This AIC_EOSCR register is used by the interrupt service routine to >> indicate that it is completely served. Thus, the >> interrupt handler can write any value to this register to indicate >> the end of its interrupt service. > > A short comment in the ack routine would be helpful. > > >>>> +IRQCHIP_DECLARE(nuc900, "nuvoton,nuc900-aic", aic_of_init); >>> >>> afaict, there is no 'nuc900', that's a family. If so, this compatible >>> needs to be 'nuvoton,nuc970-aic'. >>> >> >> IRQCHIP_DECLARE(nuvoton, "nuvoton,nuc900-aic", aic_of_init);, change >> nuc900 to nuvoton, ok? > > No, I was only talking about the second argument. The compatible > string. For devicetree, it needs to refer to a specific model number. > I suspect 'nuc900' is equivalent to saying 'nuc9xx'. Meaning it refers > to a family of devices. The compatible string needs to refer > specifically to the first model that the binding is compatible with. In > this case, I presume that would be 'nuvoton,nuc970-aic'. > > thx, > > Jason. > >
| |