[PATCH 2/6] irqchip: sunxi: Add irq controller driver

Hi Stefan,

Le 16/11/2012 08:35, Stefan Roese a ?crit :

On 11/15/2012 11:46 PM, Maxime Ripard wrote:

<snip>

quoted

diff --git a/drivers/irqchip/irq-sunxi.c b/drivers/irqchip/irq-sunxi.c
new file mode 100644
index 0000000..9dcb323
--- /dev/null
+++ b/drivers/irqchip/irq-sunxi.c

@@ -0,0 +1,173 @@
+/*
+ * Allwinner A1X SoCs IRQ chip driver.
+ *
+ * Copyright (C) 2012 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * Based on code from
+ * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
+ * Benn Huang <benn@allwinnertech.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include <linux/irqchip/sunxi.h>
+
+#define SUNXI_IRQ_PROTECTION_REG	0x08
+#define SUNXI_IRQ_NMI_CTRL_REG		0x0c
+#define SUNXI_IRQ_PENDING_REG(x)	(0x10 + 0x4 * x)
+#define SUNXI_IRQ_FIQ_PENDING_REG(x)	(0x20 + 0x4 * x)
+#define SUNXI_IRQ_ENABLE_REG(x)		(0x40 + 0x4 * x)
+#define SUNXI_IRQ_MASK_REG(x)		(0x50 + 0x4 * x)
+
+static void __iomem *sunxi_irq_base;
+static struct irq_domain *sunxi_irq_domain;
+
+void sunxi_irq_ack(struct irq_data *irqd)
+{
+	unsigned int irq = irqd_to_hwirq(irqd);
+	unsigned int irq_off = irq % 32;
+	int reg = irq / 32;
+	u32 val;
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+	writel(val & ~(1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));
+	writel(val | (1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_PENDING_REG(reg));
+	writel(val | (1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_PENDING_REG(reg));

Are you sure that you need to touch all those 23registers to ack the
interrupt? I know that the original code provided by Allwinner does
exactly this. My tests have shown though, that writing to the pending
reg is enough.

Ok, I'll test that and remove the first two writes then.

quoted

+}
+
+static void sunxi_irq_mask(struct irq_data *irqd)
+{
+	unsigned int irq = irqd_to_hwirq(irqd);
+	unsigned int irq_off = irq % 32;
+	int reg = irq / 32;
+	u32 val;
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+	writel(val & ~(1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));
+	writel(val | (1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));

I unmasked all interrupts in the mask register in the init function.
Then only using the enable register for masking/unmasking seems to be
enough. What do you think?

That's reasonable I guess. It will be in the v2.

quoted

+}
+
+static void sunxi_irq_unmask(struct irq_data *irqd)
+{
+	unsigned int irq = irqd_to_hwirq(irqd);
+	unsigned int irq_off = irq % 32;
+	int reg = irq / 32;
+	u32 val;
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+	writel(val | (1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(reg));
+
+	val = readl(sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));
+	writel(val & ~(1 << irq_off),
+	       sunxi_irq_base + SUNXI_IRQ_MASK_REG(reg));
+
+	/* Must clear pending bit when enabled */
+	if (irq == 0)
+		writel(1, sunxi_irq_base + SUNXI_IRQ_PENDING_REG(0));

Again. Might be that one register is enough here.

Ok.

quoted

+}
+
+static struct irq_chip sunxi_irq_chip = {
+	.name		= "sunxi_irq",
+	.irq_ack	= sunxi_irq_ack,
+	.irq_mask	= sunxi_irq_mask,
+	.irq_unmask	= sunxi_irq_unmask,
+};
+
+static int sunxi_irq_map(struct irq_domain *d, unsigned int virq,
+			 irq_hw_number_t hw)
+{
+	irq_set_chip(virq, &sunxi_irq_chip);
+	irq_set_handler(virq, handle_level_irq);

irq_set_chip_and_handler()

Ok.

quoted

+	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
+
+	return 0;
+}
+
+static struct irq_domain_ops sunxi_irq_ops = {
+	.map = sunxi_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int __init sunxi_of_init(struct device_node *node,
+				struct device_node *parent)
+{
+	sunxi_irq_base = of_iomap(node, 0);
+	if (!sunxi_irq_base)
+		panic("%s: unable to map IC registers\n",
+			node->full_name);
+
+	/* Disable all interrupts */
+	writel(0, sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(0));
+	writel(0, sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(1));
+	writel(0, sunxi_irq_base + SUNXI_IRQ_ENABLE_REG(2));
+
+	/* Mask all the interrupts */
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_MASK_REG(0));
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_MASK_REG(1));
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_MASK_REG(2));

Here is where I wrote 0 to the mask registers.

quoted

+	/* Clear all the pending interrupts */
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_PENDING_REG(0));
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_PENDING_REG(1));
+	writel(0xffffffff, sunxi_irq_base + SUNXI_IRQ_PENDING_REG(2));
+
+	/* Enable protection mode */
+	writel(0x01, sunxi_irq_base + SUNXI_IRQ_PROTECTION_REG);
+
+	/* Configure the external interrupt source type */
+	writel(0x00, sunxi_irq_base + SUNXI_IRQ_NMI_CTRL_REG);
+
+	sunxi_irq_domain = irq_domain_add_linear(node, 3 * 32,
+						 &sunxi_irq_ops, NULL);
+	if (!sunxi_irq_domain)
+		panic("%s: unable to create IRQ domain\n", node->full_name);
+
+	return 0;
+}
+
+static struct of_device_id sunxi_irq_dt_ids[] __initconst = {
+	{ .compatible = "allwinner,sunxi-ic", .data = sunxi_of_init }
+};
+
+void __init sunxi_init_irq(void)
+{
+	of_irq_init(sunxi_irq_dt_ids);
+}
+
+asmlinkage void __exception_irq_entry sunxi_handle_irq(struct pt_regs *regs)
+{
+	u32 irq, reg;
+	int i;
+
+	for (i = 0; i < 3; i++) {
+		reg = readl(sunxi_irq_base + SUNXI_IRQ_PENDING_REG(i));
+		if (reg == 0)
+			continue;
+		irq = ilog2(reg);
+		break;
+	}
+	irq = irq_find_mapping(sunxi_irq_domain, irq);
+	handle_IRQ(irq, regs);

Why don't you use the interrupt-vector register to get the active
interrupt source? Here is my version:

asmlinkage void __exception_irq_entry sunxi_handle_irq(struct pt_regs *regs)
{
	u32 irq;

	irq = readl(int_base + SW_INT_VECTOR_REG) >> 2;
	irq = irq_find_mapping(sunxi_vic_domain, irq);
	handle_IRQ(irq, regs);
}

I suggest you give it a try.

It definitely looks nicer. I'll try that and update.

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com