VIC DT binding and MULTI_IRQ_HANDLER

This adds a device tree binding for the VIC based on the of_irq_init() support. This adds an irqdomain to the vic and always registers all vics in the static vic array rather than for pm only to keep track of the irq domain. struct irq_data::hwirq is used where appropriate rather than runtime masking.

v2: - use irq_domain_simple_ops
- remove stub implementation of vic_of_init for !CONFIG_OF
- Make VIC select IRQ_DOMAIN

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
 Documentation/devicetree/bindings/arm/vic.txt |   29 +++++++
 arch/arm/common/Kconfig                       |    1 +
 arch/arm/common/vic.c                         |  106 ++++++++++++++++++-------
 arch/arm/include/asm/hardware/vic.h           |   10 ++-
 4 files changed, 117 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/vic.txt

Documentation/devicetree/bindings/arm/vic.txt new file +29 -0

new file mode 100644
index 0000000..266716b
@@ -0,0 +1,29 @@
+* ARM Vectored Interrupt Controller
+
+One or more Vectored Interrupt Controllers (VIC's) can be connected in an ARM
+system for interrupt routing.  For multiple controllers they can either be
+nested or have the outputs wire-OR'd together.
+
+Required properties:
+
+- compatible : should be one of
+	"arm,pl190-vic"
+	"arm,pl192-vic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : The number of cells to define the interrupts.  Must be 1 as
+  the VIC has no configuration options for interrupt sources.  The cell is a u32
+  and defines the interrupt number.
+- reg : The register bank for the VIC.
+
+Optional properties:
+
+- interrupts : Interrupt source for parent controllers if the VIC is nested.
+
+Example:
+
+	vic0: interrupt-controller at 60000 {
+		compatible = "arm,pl192-vic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0x60000 0x1000>;
+	};

arch/arm/common/Kconfig +1 -0

index 4b71766..43e9d1a 100644
@@ -2,6 +2,7 @@ config ARM_GIC
 	bool
 
 config ARM_VIC
+	select IRQ_DOMAIN
 	bool
 
 config ARM_VIC_NR

arch/arm/common/vic.c +78 -28

index 7aa4262..3f9c8f2 100644
@@ -22,6 +22,10 @@
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/syscore_ops.h>
 #include <linux/device.h>
 #include <linux/amba/bus.h>

@@ -29,7 +33,6 @@
 #include <asm/mach/irq.h>
 #include <asm/hardware/vic.h>
 
-#ifdef CONFIG_PM
 /**
  * struct vic_device - VIC PM device
  * @irq: The IRQ number for the base of the VIC.

@@ -40,6 +43,7 @@
  * @int_enable: Save for VIC_INT_ENABLE.
  * @soft_int: Save for VIC_INT_SOFT.
  * @protect: Save for VIC_PROTECT.
+ * @domain: The IRQ domain for the VIC.
  */
 struct vic_device {
 	void __iomem	*base;

@@ -50,13 +54,13 @@ struct vic_device {
 	u32		int_enable;
 	u32		soft_int;
 	u32		protect;
+	struct irq_domain domain;
 };
 
 /* we cannot allocate memory when VICs are initially registered */
 static struct vic_device vic_devices[CONFIG_ARM_VIC_NR];
 
 static int vic_id;
-#endif /* CONFIG_PM */
 
 /**
  * vic_init2 - common initialisation code

@@ -156,39 +160,50 @@ static int __init vic_pm_init(void)
 	return 0;
 }
 late_initcall(vic_pm_init);
+#endif /* CONFIG_PM */
 
 /**
- * vic_pm_register - Register a VIC for later power management control
+ * vic_register() - Register a VIC.
  * @base: The base address of the VIC.
  * @irq: The base IRQ for the VIC.
  * @resume_sources: bitmask of interrupts allowed for resume sources.
+ * @node: The device tree node associated with the VIC.
  *
  * Register the VIC with the system device tree so that it can be notified
  * of suspend and resume requests and ensure that the correct actions are
  * taken to re-instate the settings on resume.
+ *
+ * This also configures the IRQ domain for the VIC.
  */
-static void __init vic_pm_register(void __iomem *base, unsigned int irq, u32 resume_sources)
+static void __init vic_register(void __iomem *base, unsigned int irq,
+				u32 resume_sources, struct device_node *node)
 {
 	struct vic_device *v;
 
-	if (vic_id >= ARRAY_SIZE(vic_devices))
+	if (vic_id >= ARRAY_SIZE(vic_devices)) {
 		printk(KERN_ERR "%s: too few VICs, increase CONFIG_ARM_VIC_NR\n", __func__);
-	else {
-		v = &vic_devices[vic_id];
-		v->base = base;
-		v->resume_sources = resume_sources;
-		v->irq = irq;
-		vic_id++;
+		return;
 	}
+
+	v = &vic_devices[vic_id];
+	v->base = base;
+	v->resume_sources = resume_sources;
+	v->irq = irq;
+	vic_id++;
+
+	v->domain.irq_base = irq;
+	v->domain.nr_irq = 32;
+#ifdef CONFIG_OF_IRQ
+	v->domain.of_node = of_node_get(node);
+	v->domain.ops = &irq_domain_simple_ops;
+#endif /* CONFIG_OF */
+	irq_domain_add(&v->domain);
 }
-#else
-static inline void vic_pm_register(void __iomem *base, unsigned int irq, u32 arg1) { }
-#endif /* CONFIG_PM */
 
 static void vic_ack_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
 	/* moreover, clear the soft-triggered, in case it was the reason */
 	writel(1 << irq, base + VIC_INT_SOFT_CLEAR);

@@ -197,14 +212,14 @@ static void vic_ack_irq(struct irq_data *d)
 static void vic_mask_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
 }
 
 static void vic_unmask_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE);
 }
 

@@ -226,7 +241,7 @@ static struct vic_device *vic_from_irq(unsigned int irq)
 static int vic_set_wake(struct irq_data *d, unsigned int on)
 {
 	struct vic_device *v = vic_from_irq(d->irq);
-	unsigned int off = d->irq & 31;
+	unsigned int off = d->hwirq;
 	u32 bit = 1 << off;
 
 	if (!v)

@@ -331,15 +346,9 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start,
 	vic_set_irq_sources(base, irq_start, vic_sources);
 }
 
-/**
- * vic_init - initialise a vectored interrupt controller
- * @base: iomem base address
- * @irq_start: starting interrupt number, must be muliple of 32
- * @vic_sources: bitmask of interrupt sources to allow
- * @resume_sources: bitmask of interrupt sources to allow for resume
- */
-void __init vic_init(void __iomem *base, unsigned int irq_start,
-		     u32 vic_sources, u32 resume_sources)
+static void __init __vic_init(void __iomem *base, unsigned int irq_start,
+			      u32 vic_sources, u32 resume_sources,
+			      struct device_node *node)
 {
 	unsigned int i;
 	u32 cellid = 0;

@@ -375,5 +384,46 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
 
 	vic_set_irq_sources(base, irq_start, vic_sources);
 
-	vic_pm_register(base, irq_start, resume_sources);
+	vic_register(base, irq_start, resume_sources, node);
+}
+
+/**
+ * vic_init() - initialise a vectored interrupt controller
+ * @base: iomem base address
+ * @irq_start: starting interrupt number, must be muliple of 32
+ * @vic_sources: bitmask of interrupt sources to allow
+ * @resume_sources: bitmask of interrupt sources to allow for resume
+ */
+void __init vic_init(void __iomem *base, unsigned int irq_start,
+		     u32 vic_sources, u32 resume_sources)
+{
+	__vic_init(base, irq_start, vic_sources, resume_sources, NULL);
+}
+
+#ifdef CONFIG_OF
+int __init vic_of_init(struct device_node *node, struct device_node *parent)
+{
+	void __iomem *regs;
+	int irq_base;
+
+	if (WARN(parent, "non-root VICs are not supported"))
+		return -EINVAL;
+
+	regs = of_iomap(node, 0);
+	if (WARN_ON(!regs))
+		return -EIO;
+
+	irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(irq_base < 0))
+		goto out_unmap;
+
+	__vic_init(regs, irq_base, ~0, ~0, node);
+
+	return 0;
+
+ out_unmap:
+	iounmap(regs);
+
+	return -EIO;
 }
+#endif /* CONFIG OF */

arch/arm/include/asm/hardware/vic.h +9 -1

index 5d72550..0135215 100644
@@ -41,7 +41,15 @@
 #define VIC_PL192_VECT_ADDR		0xF00
 
 #ifndef __ASSEMBLY__
+#include <linux/compiler.h>
+#include <linux/types.h>
+
+struct device_node;
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
-#endif
 
+#ifdef CONFIG_OF
+int vic_of_init(struct device_node *node, struct device_node *parent);
+#endif /* CONFIG_OF */
+
+#endif /* __ASSEMBLY__ */
 #endif

Signature · 1 line

1.7.4.1

This adds a device tree binding for the VIC based on the of_irq_init() support. This adds an irqdomain to the vic and always registers all vics in the static vic array rather than for pm only to keep track of the irq domain. struct irq_data::hwirq is used where appropriate rather than runtime masking.

v2: - use irq_domain_simple_ops
- remove stub implementation of vic_of_init for !CONFIG_OF
- Make VIC select IRQ_DOMAIN

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
 Documentation/devicetree/bindings/arm/vic.txt |   29 +++++++
 arch/arm/common/Kconfig                       |    1 +
 arch/arm/common/vic.c                         |  106 ++++++++++++++++++-------
 arch/arm/include/asm/hardware/vic.h           |   10 ++-
 4 files changed, 117 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/vic.txt

Documentation/devicetree/bindings/arm/vic.txt new file +29 -0

new file mode 100644
index 0000000..266716b
@@ -0,0 +1,29 @@
+* ARM Vectored Interrupt Controller
+
+One or more Vectored Interrupt Controllers (VIC's) can be connected in an ARM
+system for interrupt routing.  For multiple controllers they can either be
+nested or have the outputs wire-OR'd together.
+
+Required properties:
+
+- compatible : should be one of
+	"arm,pl190-vic"
+	"arm,pl192-vic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : The number of cells to define the interrupts.  Must be 1 as
+  the VIC has no configuration options for interrupt sources.  The cell is a u32
+  and defines the interrupt number.
+- reg : The register bank for the VIC.
+
+Optional properties:
+
+- interrupts : Interrupt source for parent controllers if the VIC is nested.
+
+Example:
+
+	vic0: interrupt-controller at 60000 {
+		compatible = "arm,pl192-vic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0x60000 0x1000>;
+	};

arch/arm/common/Kconfig +1 -0

index 4b71766..43e9d1a 100644
@@ -2,6 +2,7 @@ config ARM_GIC
 	bool
 
 config ARM_VIC
+	select IRQ_DOMAIN
 	bool
 
 config ARM_VIC_NR

arch/arm/common/vic.c +78 -28

index 7aa4262..3f9c8f2 100644
@@ -22,6 +22,10 @@
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/syscore_ops.h>
 #include <linux/device.h>
 #include <linux/amba/bus.h>

@@ -29,7 +33,6 @@
 #include <asm/mach/irq.h>
 #include <asm/hardware/vic.h>
 
-#ifdef CONFIG_PM
 /**
  * struct vic_device - VIC PM device
  * @irq: The IRQ number for the base of the VIC.

@@ -40,6 +43,7 @@
  * @int_enable: Save for VIC_INT_ENABLE.
  * @soft_int: Save for VIC_INT_SOFT.
  * @protect: Save for VIC_PROTECT.
+ * @domain: The IRQ domain for the VIC.
  */
 struct vic_device {
 	void __iomem	*base;

@@ -50,13 +54,13 @@ struct vic_device {
 	u32		int_enable;
 	u32		soft_int;
 	u32		protect;
+	struct irq_domain domain;
 };
 
 /* we cannot allocate memory when VICs are initially registered */
 static struct vic_device vic_devices[CONFIG_ARM_VIC_NR];
 
 static int vic_id;
-#endif /* CONFIG_PM */
 
 /**
  * vic_init2 - common initialisation code

@@ -156,39 +160,50 @@ static int __init vic_pm_init(void)
 	return 0;
 }
 late_initcall(vic_pm_init);
+#endif /* CONFIG_PM */
 
 /**
- * vic_pm_register - Register a VIC for later power management control
+ * vic_register() - Register a VIC.
  * @base: The base address of the VIC.
  * @irq: The base IRQ for the VIC.
  * @resume_sources: bitmask of interrupts allowed for resume sources.
+ * @node: The device tree node associated with the VIC.
  *
  * Register the VIC with the system device tree so that it can be notified
  * of suspend and resume requests and ensure that the correct actions are
  * taken to re-instate the settings on resume.
+ *
+ * This also configures the IRQ domain for the VIC.
  */
-static void __init vic_pm_register(void __iomem *base, unsigned int irq, u32 resume_sources)
+static void __init vic_register(void __iomem *base, unsigned int irq,
+				u32 resume_sources, struct device_node *node)
 {
 	struct vic_device *v;
 
-	if (vic_id >= ARRAY_SIZE(vic_devices))
+	if (vic_id >= ARRAY_SIZE(vic_devices)) {
 		printk(KERN_ERR "%s: too few VICs, increase CONFIG_ARM_VIC_NR\n", __func__);
-	else {
-		v = &vic_devices[vic_id];
-		v->base = base;
-		v->resume_sources = resume_sources;
-		v->irq = irq;
-		vic_id++;
+		return;
 	}
+
+	v = &vic_devices[vic_id];
+	v->base = base;
+	v->resume_sources = resume_sources;
+	v->irq = irq;
+	vic_id++;
+
+	v->domain.irq_base = irq;
+	v->domain.nr_irq = 32;
+#ifdef CONFIG_OF_IRQ
+	v->domain.of_node = of_node_get(node);
+	v->domain.ops = &irq_domain_simple_ops;
+#endif /* CONFIG_OF */
+	irq_domain_add(&v->domain);
 }
-#else
-static inline void vic_pm_register(void __iomem *base, unsigned int irq, u32 arg1) { }
-#endif /* CONFIG_PM */
 
 static void vic_ack_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
 	/* moreover, clear the soft-triggered, in case it was the reason */
 	writel(1 << irq, base + VIC_INT_SOFT_CLEAR);

@@ -197,14 +212,14 @@ static void vic_ack_irq(struct irq_data *d)
 static void vic_mask_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE_CLEAR);
 }
 
 static void vic_unmask_irq(struct irq_data *d)
 {
 	void __iomem *base = irq_data_get_irq_chip_data(d);
-	unsigned int irq = d->irq & 31;
+	unsigned int irq = d->hwirq;
 	writel(1 << irq, base + VIC_INT_ENABLE);
 }
 

@@ -226,7 +241,7 @@ static struct vic_device *vic_from_irq(unsigned int irq)
 static int vic_set_wake(struct irq_data *d, unsigned int on)
 {
 	struct vic_device *v = vic_from_irq(d->irq);
-	unsigned int off = d->irq & 31;
+	unsigned int off = d->hwirq;
 	u32 bit = 1 << off;
 
 	if (!v)

@@ -331,15 +346,9 @@ static void __init vic_init_st(void __iomem *base, unsigned int irq_start,
 	vic_set_irq_sources(base, irq_start, vic_sources);
 }
 
-/**
- * vic_init - initialise a vectored interrupt controller
- * @base: iomem base address
- * @irq_start: starting interrupt number, must be muliple of 32
- * @vic_sources: bitmask of interrupt sources to allow
- * @resume_sources: bitmask of interrupt sources to allow for resume
- */
-void __init vic_init(void __iomem *base, unsigned int irq_start,
-		     u32 vic_sources, u32 resume_sources)
+static void __init __vic_init(void __iomem *base, unsigned int irq_start,
+			      u32 vic_sources, u32 resume_sources,
+			      struct device_node *node)
 {
 	unsigned int i;
 	u32 cellid = 0;

@@ -375,5 +384,46 @@ void __init vic_init(void __iomem *base, unsigned int irq_start,
 
 	vic_set_irq_sources(base, irq_start, vic_sources);
 
-	vic_pm_register(base, irq_start, resume_sources);
+	vic_register(base, irq_start, resume_sources, node);
+}
+
+/**
+ * vic_init() - initialise a vectored interrupt controller
+ * @base: iomem base address
+ * @irq_start: starting interrupt number, must be muliple of 32
+ * @vic_sources: bitmask of interrupt sources to allow
+ * @resume_sources: bitmask of interrupt sources to allow for resume
+ */
+void __init vic_init(void __iomem *base, unsigned int irq_start,
+		     u32 vic_sources, u32 resume_sources)
+{
+	__vic_init(base, irq_start, vic_sources, resume_sources, NULL);
+}
+
+#ifdef CONFIG_OF
+int __init vic_of_init(struct device_node *node, struct device_node *parent)
+{
+	void __iomem *regs;
+	int irq_base;
+
+	if (WARN(parent, "non-root VICs are not supported"))
+		return -EINVAL;
+
+	regs = of_iomap(node, 0);
+	if (WARN_ON(!regs))
+		return -EIO;
+
+	irq_base = irq_alloc_descs(-1, 0, 32, numa_node_id());
+	if (WARN_ON(irq_base < 0))
+		goto out_unmap;
+
+	__vic_init(regs, irq_base, ~0, ~0, node);
+
+	return 0;
+
+ out_unmap:
+	iounmap(regs);
+
+	return -EIO;
 }
+#endif /* CONFIG OF */

arch/arm/include/asm/hardware/vic.h +0 -0

index 5d72550..0135215 100644
@@ -41,7 +41,15 @@
 #define VIC_PL192_VECT_ADDR		0xF00
 
 #ifndef __ASSEMBLY__

Review

Quoted 12 lines · Jamie Iles

(quoted diff) +12 -0

+static void vic_single_handle_irq(struct vic_device *vic, struct pt_regs *regs)
+{
+ ? ? ? u32 stat, irq;
+ ? ? ? bool handled = false;
+
+ ? ? ? while (!handled) {
+ ? ? ? ? ? ? ? stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
+ ? ? ? ? ? ? ? if (!stat)
+ ? ? ? ? ? ? ? ? ? ? ? break;
+
+ ? ? ? ? ? ? ? while (stat) {
+ ? ? ? ? ? ? ? ? ? ? ? irq = fls(stat) - 1;

Isn't fls "find last set"?

That means IRQs with higher numbers will be handled first will it not?

For U300 IRQs with lower numbers will be handled first by iteratively testing bit 0 and shifting right:

-1002: tst \irqstat, #1

(quoted diff) +0 -4

-       bne     1003f
-       add     \irqnr, \irqnr, #1
-       movs    \irqstat, \irqstat, lsr #1
-       bne     1002b

So I would use ffs() for this to work the same way as before in U300.

Since this can have some performance impact, if the platforms differ in whether they handle IRQs from low to high or from high to low might need to be a flag passed in to vic_init() or so...

Add a handler for the VIC that is suitable for MULTI_IRQ_HANDLER platforms. This can replace the ASM entry macros for platforms that use the VIC.

v2: - allow the handler be used for !CONFIG_OF
- use irq_domain_to_irq()

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
 arch/arm/common/vic.c               |   29 +++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/vic.h |    4 ++++
 2 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/common/vic.c +9 -0

index 3f9c8f2..71adced 100644
@@ -427,3 +427,32 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
 	return -EIO;
 }
 #endif /* CONFIG OF */
+
+#ifdef CONFIG_MULTI_IRQ_HANDLER
+static void vic_single_handle_irq(struct vic_device *vic, struct pt_regs *regs)
+{
+	u32 stat, irq;
+	bool handled = false;
+
+	while (!handled) {
+		stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);

Why isn't it simply written this way:

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
? ? ? ?while (stat) {
? ? ? ? ? ? ? ?irq = fls(stat) - 1;
? ? ? ? ? ? ? ?handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs); ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ?}

Quoted 8 lines · Grant Likely

Why isn't it simply written this way:

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
? ? ? ?while (stat) {
? ? ? ? ? ? ? ?irq = fls(stat) - 1;
? ? ? ? ? ? ? ?handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs); ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ?}

That is indeed closer to the assembly loop most platforms have implemented.

Jamie can you test this approach? And also use ffs() insteadof fls()...

Quoted 12 lines · Linus Walleij

Quoted 8 lines · Grant Likely

Why isn't it simply written this way:

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
? ? ? ?while (stat) {
? ? ? ? ? ? ? ?irq = fls(stat) - 1;
? ? ? ? ? ? ? ?handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs); ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ?}

Quoted 5 lines

That is indeed closer to the assembly loop most platforms have implemented.

Jamie can you test this approach? And also use ffs() insteadof fls()...

OK, here it is (and it works)! That does make it a lot simpler, thanks guys! I've updated the vic-dt branch in my repo too.

Jamie

8<------
From: Jamie Iles <[email protected]>
Subject: [PATCHv3] ARM: vic: MULTI_IRQ_HANDLER handler

Add a handler for the VIC that is suitable for MULTI_IRQ_HANDLER platforms. This can replace the ASM entry macros for platforms that use the VIC.

v3: - simplify irq handling loop as suggested by Grant
- service interrupts from msb->lsb order
v2: - allow the handler be used for !CONFIG_OF
- use irq_domain_to_irq()

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
 arch/arm/common/vic.c               |   22 ++++++++++++++++++++++
 arch/arm/include/asm/hardware/vic.h |    4 ++++
 2 files changed, 26 insertions(+), 0 deletions(-)

arch/arm/common/vic.c +22 -0

index 3f9c8f2..b22b83d 100644
@@ -427,3 +427,25 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent)
 	return -EIO;
 }
 #endif /* CONFIG OF */
+
+#ifdef CONFIG_MULTI_IRQ_HANDLER
+static void vic_single_handle_irq(struct vic_device *vic, struct pt_regs *regs)
+{
+	u32 stat, irq;
+
+	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
+	while (stat) {
+		irq = ffs(stat) - 1;
+		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
+		stat &= ~(1 << irq);
+	}
+}
+
+asmlinkage void __exception_irq_entry vic_handle_irq(struct pt_regs *regs)
+{
+	int i;
+
+	for (i = 0; i < vic_id; ++i)
+		vic_single_handle_irq(&vic_devices[i], regs);
+}
+#endif /* CONFIG_MULTI_IRQ_HANDLER */

arch/arm/include/asm/hardware/vic.h +4 -0

index 0135215..c02fd6f 100644
@@ -45,11 +45,15 @@
 #include <linux/types.h>
 
 struct device_node;
+struct pt_regs;
+
 void vic_init(void __iomem *base, unsigned int irq_start, u32 vic_sources, u32 resume_sources);
 
 #ifdef CONFIG_OF
 int vic_of_init(struct device_node *node, struct device_node *parent);
 #endif /* CONFIG_OF */
 
+void vic_handle_irq(struct pt_regs *regs);
+
 #endif /* __ASSEMBLY__ */
 #endif

Signature · 1 line

1.7.4.1

arch/arm/common/vic.c +10 -0

+#ifdef CONFIG_MULTI_IRQ_HANDLER
+static void vic_single_handle_irq(struct vic_device *vic, struct pt_regs *regs)
+{
+	u32 stat, irq;
+
+	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
+	while (stat) {
+		irq = ffs(stat) - 1;
+		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
+		stat &= ~(1 << irq);

Quoted 16 lines · Jamie Iles

arch/arm/common/vic.c +16 -0

+#ifdef CONFIG_MULTI_IRQ_HANDLER
+static void vic_single_handle_irq(struct vic_device *vic, struct pt_regs *regs)
+{
+	u32 stat, irq;
+
+	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
+	while (stat) {
+		irq = ffs(stat) - 1;
+		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
+		stat &= ~(1 << irq);
+	}
+}
+
+asmlinkage void __exception_irq_entry vic_handle_irq(struct pt_regs *regs)
+{
+	int i;

arch/arm/common/vic.c int __init vic_of_init(struct device_node *node, struct device_node *parent) · line 450

Quoted 4 lines

arch/arm/common/vic.c +4 -0

+
+	for (i = 0; i < vic_id; ++i)
+		vic_single_handle_irq(&vic_devices[i], regs);
+}

And if we receive another interrupt after the read of the register, we'll have to exit all the way back (possibly to userspace) before re-entering the IRQ handling paths back to this point to process it.

Is there any particular reason folk are destroying the built-in efficiency of the IRQ handling which is common-place in the existing assembly approach?

Quoted 4 lines · Russell King - ARM Linux

And if we receive another interrupt after the read of the register, we'll have to exit all the way back (possibly to userspace) before re-entering the IRQ handling paths back to this point to process it.

OK, so how about something like this instead:

static int vic_single_handle_irq(struct vic_device *vic, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct pt_regs *regs)

{
? ? ? ?u32 stat, irq;
? ? ? ?int handled = 0;

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
? ? ? ?while (stat) {
? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ?handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs); ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ?handled = 1;
? ? ? ?}

? ? ? ?return handled;
}

No, if we receive another IRQ *after* the read of the register was the question, right?

Just replace

stat &= ~(1 << irq);

with a second

stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);

It'll work just fine, the IRQ line should be low when you read it the second time, else it is probably fully proper to call the IRQ handler again anyway.

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?while (stat) {
? ? ? ? ? ? ? ?while (stat) {
? ? ? ? ? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ? ? ? ? ?handle_irq(irq);
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?}

This ensures that we process all interrupts found pending before we re-check for any new interrupts pending. ?Arguably this is a much fairer implementation (and may mean if things get irrevokably stuck, things like sysrq via the console uart may still work.)

Quoted 10 lines · Russell King - ARM Linux

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?while (stat) {
? ? ? ? ? ? ? ?while (stat) {
? ? ? ? ? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ? ? ? ? ?handle_irq(irq);
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?}

Quoted 4 lines

This ensures that we process all interrupts found pending before we re-check for any new interrupts pending. ?Arguably this is a much fairer implementation (and may mean if things get irrevokably stuck, things like sysrq via the console uart may still work.)

I really like the looks of this, Jamie can you do it like that?

Maybe some smallish comment about what's going on can be good for future generations reading that code...

Quoted 10 lines · Russell King - ARM Linux

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?while (stat) {
? ? ? ? ? ? ? ?while (stat) {
? ? ? ? ? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ? ? ? ? ?handle_irq(irq);
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?}

Quoted 4 lines

This ensures that we process all interrupts found pending before we re-check for any new interrupts pending. ?Arguably this is a much fairer implementation (and may mean if things get irrevokably stuck, things like sysrq via the console uart may still work.)

I really like the looks of this, Jamie can you do it like that?

Maybe some smallish comment about what's going on can be good for future generations reading that code...

Quoted 18 lines · Linus Walleij

Quoted 14 lines · Russell King - ARM Linux

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?while (stat) {
? ? ? ? ? ? ? ?while (stat) {
? ? ? ? ? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ? ? ? ? ?handle_irq(irq);
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?}

This ensures that we process all interrupts found pending before we re-check for any new interrupts pending. ?Arguably this is a much fairer implementation (and may mean if things get irrevokably stuck, things like sysrq via the console uart may still work.)

Quoted 4 lines

I really like the looks of this, Jamie can you do it like that?

Maybe some smallish comment about what's going on can be good for future generations reading that code...

Bear in mind that it gets a little more complex when you have more than one VIC, because the outer loop should be across all VICs.

Quoted 24 lines · Russell King - ARM Linux

Quoted 22 lines · Linus Walleij

Quoted 10 lines · Russell King - ARM Linux

? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?while (stat) {
? ? ? ? ? ? ? ?while (stat) {
? ? ? ? ? ? ? ? ? ? ? ?irq = ffs(stat) - 1;
? ? ? ? ? ? ? ? ? ? ? ?stat &= ~(1 << irq);
? ? ? ? ? ? ? ? ? ? ? ?handle_irq(irq);
? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?stat = readl_relaxed(vic->base + VIC_IRQ_STATUS); ? ? ? ?}

Quoted 4 lines

This ensures that we process all interrupts found pending before we re-check for any new interrupts pending. ?Arguably this is a much fairer implementation (and may mean if things get irrevokably stuck, things like sysrq via the console uart may still work.)

I really like the looks of this, Jamie can you do it like that?

Maybe some smallish comment about what's going on can be good for future generations reading that code...

Quoted 2 lines

Bear in mind that it gets a little more complex when you have more than one VIC, because the outer loop should be across all VICs.

OK, so I think what I posted yesterday does that (updated for slightly better naming) and with a description. In the spirit of fairness iterating over the VIC's this way seemed right to me.

	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
	while (stat) {
		while (stat) {
			irq = ffs(stat) - 1;
			stat &= ~(1 << irq);
			handle_irq(irq);
		}
		stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
	}

Add a handler for the VIC that is suitable for MULTI_IRQ_HANDLER platforms. ?This can replace the ASM entry macros for platforms that use the VIC.

v2: ? ? - allow the handler be used for !CONFIG_OF ? ? ? ?- use irq_domain_to_irq()

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
?arch/arm/common/vic.c ? ? ? ? ? ? ? | ? 29 +++++++++++++++++++++++++++++ ?arch/arm/include/asm/hardware/vic.h | ? ?4 ++++
?2 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/common/vic.c +0 -0

index 3f9c8f2..71adced 100644

2011/9/28 Jamie Iles <[email protected]>:

Quoted 19 lines

Add a handler for the VIC that is suitable for MULTI_IRQ_HANDLER platforms. ?This can replace the ASM entry macros for platforms that use the VIC.

v2: ? ? - allow the handler be used for !CONFIG_OF ? ? ? ?- use irq_domain_to_irq()

Cc Rob Herring <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Jamie Iles <[email protected]>

---
?arch/arm/common/vic.c ? ? ? ? ? ? ? | ? 29 +++++++++++++++++++++++++++++ ?arch/arm/include/asm/hardware/vic.h | ? ?4 ++++
?2 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/common/vic.c +0 -0

index 3f9c8f2..71adced 100644

arch/arm/common/vic.c int __init vic_of_init(struct device_node *node, struct device_node *parent) · line 458

Quoted 8 lines

arch/arm/common/vic.c +8 -0

+asmlinkage void __exception_irq_entry vic_handle_irq(struct pt_regs *regs)
+{
+ ? ? ? int i;
+
+ ? ? ? for (i = 0; i < vic_id; ++i)
+ ? ? ? ? ? ? ? vic_single_handle_irq(&vic_devices[i], regs);
+}
+#endif /* CONFIG_MULTI_IRQ_HANDLER */

__exception_irq_entry doesn't compile with CONFIG_FUNCTION_GRAPH_TRACER enabled. Either this file, or arch/arm/include/asm/system.h needs an #include <linux/ftrace.h>

- ? ? ? ? ? ? ? .macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
- ? ? ? ? ? ? ? ldr ? ? \base, =(EP93XX_AHB_VIRT_BASE)
- ? ? ? ? ? ? ? orr ? ? \base, \base, #0x000b0000
- ? ? ? ? ? ? ? mov ? ? \irqnr, #0
- ? ? ? ? ? ? ? ldr ? ? \irqstat, [\base] ? ? ? ? ? ? ? @ lower 32 interrupts

- ? ? ? ? ? ? ? cmp ? ? \irqstat, #0
- ? ? ? ? ? ? ? bne ? ? 1001f
-
- ? ? ? ? ? ? ? eor ? ? \base, \base, #0x00070000
- ? ? ? ? ? ? ? ldr ? ? \irqstat, [\base] ? ? ? ? ? ? ? @ upper 32 interrupts
- ? ? ? ? ? ? ? cmp ? ? \irqstat, #0
- ? ? ? ? ? ? ? beq ? ? 1002f
- ? ? ? ? ? ? ? mov ? ? \irqnr, #0x20

- ? ? ? /* This stanza gets the irq mask from one of two status registers */
- ? ? ? mov ? ? \irqnr, #0
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_REG_IRQSR0] ? ? ?@ get masked status
- ? ? ? cmp ? ? \irqstat, #0
- ? ? ? bne ? ? 1001f
- ? ? ? add ? ? \irqnr, \irqnr, #32
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_REG_IRQSR1] ? ? ?@ get masked status

arch/arm/mach-u300/include/mach/entry-macro.S +0 -0

index 20731ae..7181d6a 100644
@@ -8,33 +8,9 @@

?* Low-level IRQ helper macros for ST-Ericsson U300 ?* Author: Linus Walleij <[email protected]> ?*/

arch/arm/mach-u300/include/mach/entry-macro.S +0 -2

-#include <mach/hardware.h>
-#include <asm/hardware/vic.h>

? ? ? ?.macro ?disable_fiq
? ? ? ?.endm

- ? ? ? .macro ?get_irqnr_preamble, base, tmp
- ? ? ? .endm
-
? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
? ? ? ?.endm
-
- ? ? ? .macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
- ? ? ? ldr ? ? \base, = U300_AHB_PER_VIRT_BASE-U300_AHB_PER_PHYS_BASE+U300_INTCON0_BASE
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_IRQ_STATUS] @ get masked status
- ? ? ? mov ? ? \irqnr, #0
- ? ? ? teq ? ? \irqstat, #0
- ? ? ? bne ? ? 1002f
-1001: ?ldr ? ? \base, = U300_AHB_PER_VIRT_BASE-U300_AHB_PER_PHYS_BASE+U300_INTCON1_BASE
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_IRQ_STATUS] @ get masked status
- ? ? ? mov ? ? \irqnr, #32
- ? ? ? teq ? ? \irqstat, #0

- ? ? ? beq ? ? 1003f
-1002: ?tst ? ? \irqstat, #1
- ? ? ? bne ? ? 1003f
- ? ? ? add ? ? \irqnr, \irqnr, #1
- ? ? ? movs ? ?\irqstat, \irqstat, lsr #1
- ? ? ? bne ? ? 1002b
-1003: ? ? ? ? ?/* EQ will be set if no irqs pending */
- ? ? ? .endm

Hold your horses:

Quoted 36 lines · Jamie Iles

arch/arm/mach-u300/include/mach/entry-macro.S +0 -0

index 20731ae..7181d6a 100644
@@ -8,33 +8,9 @@

?* Low-level IRQ helper macros for ST-Ericsson U300 ?* Author: Linus Walleij <[email protected]> ?*/

arch/arm/mach-u300/include/mach/entry-macro.S +0 -2

-#include <mach/hardware.h>
-#include <asm/hardware/vic.h>

? ? ? ?.macro ?disable_fiq
? ? ? ?.endm

- ? ? ? .macro ?get_irqnr_preamble, base, tmp
- ? ? ? .endm
-
? ? ? ?.macro ?arch_ret_to_user, tmp1, tmp2
? ? ? ?.endm
-
- ? ? ? .macro ?get_irqnr_and_base, irqnr, irqstat, base, tmp
- ? ? ? ldr ? ? \base, = U300_AHB_PER_VIRT_BASE-U300_AHB_PER_PHYS_BASE+U300_INTCON0_BASE
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_IRQ_STATUS] @ get masked status
- ? ? ? mov ? ? \irqnr, #0
- ? ? ? teq ? ? \irqstat, #0
- ? ? ? bne ? ? 1002f
-1001: ?ldr ? ? \base, = U300_AHB_PER_VIRT_BASE-U300_AHB_PER_PHYS_BASE+U300_INTCON1_BASE
- ? ? ? ldr ? ? \irqstat, [\base, #VIC_IRQ_STATUS] @ get masked status
- ? ? ? mov ? ? \irqnr, #32
- ? ? ? teq ? ? \irqstat, #0
- ? ? ? beq ? ? 1003f
-1002: ?tst ? ? \irqstat, #1
- ? ? ? bne ? ? 1003f
- ? ? ? add ? ? \irqnr, \irqnr, #1
- ? ? ? movs ? ?\irqstat, \irqstat, lsr #1
- ? ? ? bne ? ? 1002b

Quoted 2 lines

-1003: ? ? ? ? ?/* EQ will be set if no irqs pending */
- ? ? ? .endm

When I inspect patch 2 in this series I get the feeling that it assumes that there is one and only one VIC bank with 32 interrupts involved. This is not the case in the U300, it has 64 possible IRQ sources by OR:in the output IRQ signal from two VIC:s and feeding the resulting IRQ line into the CPU.

So in the code above we first check the 32 bits at the first VIC instance, and if that is zero we go on to check the other 32 bits.

vic_single_handle_irq() needs to be modified to handle several ranges or atleast two.

Note that in mach-u300/core.c we initialize each VIC like this:
vic_init((void __iomem *) U300_INTCON0_VBASE, 0, mask[0], mask[0]); vic_init((void __iomem *) U300_INTCON1_VBASE, 32, mask[1], mask[1]);

So I think the easiest may be to let vic_init() add registered VIC ranges to a list or array, and increas some num_vics variable to that vic_single_handle_irq() can traverse both ranges in order.

Quoted 2 lines

vic_single_handle_irq() needs to be modified to handle several ranges or atleast two.

The platform IRQ handler is actually vic_handle_irq() that internally calls vic_single_handle_irq() for each registered vic (in the order of registration).

Quoted 6 lines · Jamie Iles

Quoted 2 lines

vic_single_handle_irq() needs to be modified to handle several ranges or atleast two.

The platform IRQ handler is actually vic_handle_irq() that internally calls vic_single_handle_irq() for each registered vic (in the order of registration).

Ah I get it I'm dead wrong as usual :-)

I tried to apply them to test but Iget into dependency trouble with the referenced required patches, do you have some handy git branch with all stuff applied in order that I can pull in and test off?