Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 






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.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux