Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
From: Wan Zongshun
Date: Fri Jul 15 2016 - 01:16:18 EST
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@xxxxxxxxx>
---
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@xxxxxxxxx>
+ *
+ * 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.