[PATCH v3 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI

[PATCH v3 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI

[Carlo Caione]

Allwinner A20/A31 SoCs have a special interrupt controller for managing NMI.
Three register are present to (un)mask, control and acknowledge NMI.
These two patches add a new irqchip driver in cascade with GIC.

Changes since v1:
	- added binding document

Changes since v2:
	- fixed trigger type in DTS
	- new explanations in binding documentation
	- added support for A31 (sun6i)

Carlo Caione (3):
  ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller
  ARM: sun7i/sun6i: dts: Add NMI irqchip support
  ARM: sun7i/sun6i: irqchip: Update the documentation

 .../allwinner,sun67i-sc-nmi.txt                    |  26 +++
 arch/arm/boot/dts/sun6i-a31.dtsi                   |   9 +
 arch/arm/boot/dts/sun7i-a20.dtsi                   |   9 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-sunxi-nmi.c                    | 228 +++++++++++++++++++++
 5 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt
 create mode 100644 drivers/irqchip/irq-sunxi-nmi.c

-- 
1.8.5.2

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Carlo Caione]

Allwinner A20/A31 SoCs have special registers to control / (un)mask /
acknowledge NMI. This NMI controller is separated and independent from GIC.
This patch adds a new irqchip to manage NMI.

Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
---
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-sunxi-nmi.c | 228 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 229 insertions(+)
 create mode 100644 drivers/irqchip/irq-sunxi-nmi.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index c60b901..e31d4d6 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
 obj-$(CONFIG_ARCH_MOXART)		+= irq-moxart.o
 obj-$(CONFIG_ORION_IRQCHIP)		+= irq-orion.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
+obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
 obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
 obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
 obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
new file mode 100644
index 0000000..a2b7373
--- /dev/null
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -0,0 +1,228 @@
+/*
+ * Allwinner A20/A31 SoCs NMI IRQ chip driver.
+ *
+ * Carlo Caione <carlo.caione@gmail.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/bitops.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/irqchip/chained_irq.h>
+#include "irqchip.h"
+
+#define SUNXI_NMI_SRC_TYPE_MASK	0x00000003
+
+enum {
+	SUNXI_SRC_TYPE_LEVEL_LOW = 0,
+	SUNXI_SRC_TYPE_EDGE_FALLING,
+	SUNXI_SRC_TYPE_LEVEL_HIGH,
+	SUNXI_SRC_TYPE_EDGE_RISING,
+};
+
+struct sunxi_sc_nmi_reg_offs {
+	u32 ctrl;
+	u32 pend;
+	u32 enable;
+};
+
+static struct sunxi_sc_nmi_reg_offs sun7i_reg_offs = {
+	.ctrl	= 0x00,
+	.pend	= 0x04,
+	.enable	= 0x08,
+};
+
+static struct sunxi_sc_nmi_reg_offs sun6i_reg_offs = {
+	.ctrl	= 0x00,
+	.pend	= 0x04,
+	.enable	= 0x34,
+};
+
+/*
+ * Ack level interrupts right before unmask
+ *
+ * In case of level-triggered interrupt, IRQ line must be acked before it
+ * is unmasked or else a double-interrupt is triggered
+ */
+
+static void sunxi_sc_nmi_ack_and_unmask(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+
+	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
+		ct->chip.irq_ack(d);
+
+	irq_gc_lock(gc);
+	irq_reg_writel(mask, gc->reg_base + ct->regs.mask);
+	irq_gc_unlock(gc);
+}
+
+static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
+				      u32 val)
+{
+	irq_reg_writel(val, gc->reg_base + off);
+}
+
+static inline u32 sunxi_sc_nmi_read(struct irq_chip_generic *gc, u32 off)
+{
+	return irq_reg_readl(gc->reg_base + off);
+}
+
+static void sunxi_sc_nmi_handle_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_get_chip(irq);
+	unsigned int virq = irq_find_mapping(domain, 0);
+
+	chained_irq_enter(chip, desc);
+	generic_handle_irq(virq);
+	chained_irq_exit(chip, desc);
+}
+
+static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	struct irq_chip_type *ct = gc->chip_types;
+	u32 src_type_reg;
+	u32 ctrl_off = ct->regs.type;
+	unsigned int src_type;
+	unsigned int i;
+
+	irq_gc_lock(gc);
+
+	switch (flow_type & IRQF_TRIGGER_MASK) {
+	case IRQ_TYPE_EDGE_FALLING:
+		src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		src_type = SUNXI_SRC_TYPE_EDGE_RISING;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_NONE:
+	case IRQ_TYPE_LEVEL_LOW:
+		src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
+		break;
+	default:
+		irq_gc_unlock(gc);
+		pr_err("%s: Cannot assign multiple trigger modes to IRQ %d.\n",
+			__func__, data->irq);
+		return -EBADR;
+	}
+
+	irqd_set_trigger_type(data, flow_type);
+	irq_setup_alt_chip(data, flow_type);
+
+	for (i = 0; i <= gc->num_ct; i++, ct++)
+		if (ct->type & flow_type)
+			ctrl_off = ct->regs.type;
+
+	src_type_reg = sunxi_sc_nmi_read(gc, ctrl_off);
+	src_type_reg &= ~SUNXI_NMI_SRC_TYPE_MASK;
+	src_type_reg |= src_type;
+	sunxi_sc_nmi_write(gc, ctrl_off, src_type_reg);
+
+	irq_gc_unlock(gc);
+
+	return IRQ_SET_MASK_OK;
+}
+
+static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
+					struct sunxi_sc_nmi_reg_offs *reg_offs)
+{
+	struct irq_domain *domain;
+	struct irq_chip_generic *gc;
+	unsigned int irq;
+	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
+	int ret;
+
+
+	domain = irq_domain_add_linear(node, 1, &irq_generic_chip_ops, NULL);
+	if (!domain) {
+		pr_err("%s: Could not register interrupt domain.\n", node->name);
+		return -ENOMEM;
+	}
+
+	ret = irq_alloc_domain_generic_chips(domain, 1, 2, node->name,
+					     handle_level_irq, clr, 0,
+					     IRQ_GC_INIT_MASK_CACHE);
+	if (ret) {
+		 pr_err("%s: Could not allocate generic interrupt chip.\n",
+			 node->name);
+		 goto fail_irqd_remove;
+	}
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0) {
+		pr_err("%s: unable to parse irq\n", node->name);
+		ret = -EINVAL;
+		goto fail_irqd_remove;
+	}
+
+	gc = irq_get_domain_generic_chip(domain, 0);
+	gc->reg_base = of_iomap(node, 0);
+	if (!gc->reg_base) {
+		pr_err("%s: unable to map resource\n", node->name);
+		ret = -ENOMEM;
+		goto fail_irqd_remove;
+	}
+
+	gc->chip_types[0].type			= IRQ_TYPE_LEVEL_MASK;
+	gc->chip_types[0].chip.irq_ack		= irq_gc_ack_set_bit;
+	gc->chip_types[0].chip.irq_mask		= irq_gc_mask_clr_bit;
+	gc->chip_types[0].chip.irq_unmask	= sunxi_sc_nmi_ack_and_unmask;
+	gc->chip_types[0].chip.irq_set_type	= sunxi_sc_nmi_set_type;
+	gc->chip_types[0].regs.ack		= reg_offs->pend;
+	gc->chip_types[0].regs.mask		= reg_offs->enable;
+	gc->chip_types[0].regs.type		= reg_offs->ctrl;
+
+	gc->chip_types[1].type			= IRQ_TYPE_EDGE_BOTH;
+	gc->chip_types[1].chip.name		= gc->chip_types[0].chip.name;
+	gc->chip_types[1].chip.irq_ack		= irq_gc_ack_set_bit;
+	gc->chip_types[1].chip.irq_mask		= irq_gc_mask_clr_bit;
+	gc->chip_types[1].chip.irq_unmask	= sunxi_sc_nmi_ack_and_unmask;
+	gc->chip_types[1].chip.irq_set_type	= sunxi_sc_nmi_set_type;
+	gc->chip_types[1].regs.ack		= reg_offs->pend;
+	gc->chip_types[1].regs.mask		= reg_offs->enable;
+	gc->chip_types[1].regs.type		= reg_offs->ctrl;
+	gc->chip_types[1].handler		= handle_edge_irq;
+
+	irq_set_handler_data(irq, domain);
+	irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
+
+	sunxi_sc_nmi_write(gc, reg_offs->enable, 0);
+	sunxi_sc_nmi_write(gc, reg_offs->pend, 0x1);
+
+	return 0;
+
+fail_irqd_remove:
+	irq_domain_remove(domain);
+
+	return ret;
+}
+
+static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
+					struct device_node *parent)
+{
+	return sunxi_sc_nmi_irq_init(node, &sun6i_reg_offs);
+}
+IRQCHIP_DECLARE(sun6i_sc_nmi, "allwinner,sun6i-sc-nmi", sun6i_sc_nmi_irq_init);
+
+static int __init sun7i_sc_nmi_irq_init(struct device_node *node,
+					struct device_node *parent)
+{
+	return sunxi_sc_nmi_irq_init(node, &sun7i_reg_offs);
+}
+IRQCHIP_DECLARE(sun7i_sc_nmi, "allwinner,sun7i-sc-nmi", sun7i_sc_nmi_irq_init);
-- 
1.8.5.2

[PATCH v3 2/3] ARM: sun7i/sun6i: dts: Add NMI irqchip support

[Carlo Caione]

This patch adds DTS entries for NMI controller as child of GIC.

Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 9 +++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 5256ad9..23541be 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -190,6 +190,15 @@
 		#size-cells = <1>;
 		ranges;
 
+		nmi_intc: sc-nmi-intc at 01f00c0c {
+			compatible = "allwinner,sun6i-sc-nmi";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x01f00c0c 0x38>;
+			interrupt-parent = <&gic>;
+			interrupts = <0 0 4>;
+		};
+
 		pio: pinctrl at 01c20800 {
 			compatible = "allwinner,sun6i-a31-pinctrl";
 			reg = <0x01c20800 0x400>;
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 4c25f81..2bf766c 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -310,6 +310,15 @@
 		#size-cells = <1>;
 		ranges;
 
+		nmi_intc: sc-nmi-intc at 01c00030 {
+			compatible = "allwinner,sun7i-sc-nmi";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x01c00030 0x0c>;
+			interrupt-parent = <&gic>;
+			interrupts = <0 0 4>;
+		};
+
 		emac: ethernet at 01c0b000 {
 			compatible = "allwinner,sun4i-emac";
 			reg = <0x01c0b000 0x1000>;
-- 
1.8.5.2

[PATCH v3 3/3] ARM: sun7i/sun6i: irqchip: Update the documentation

[Carlo Caione]

Added documentation for NMI irqchip.

Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
---
 .../allwinner,sun67i-sc-nmi.txt                    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt
new file mode 100644
index 0000000..5fb0f72
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt
@@ -0,0 +1,26 @@
+Allwinner Sunxi NMI Controller
+==============================
+
+Required properties:
+
+- compatible : should be "allwinner,sun7i-sc-nmi" or "allwinner,sun6i-sc-nmi"
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 2. The first cell is the IRQ number, the
+  second cell the trigger type.
+- interrupt-parent: Specifies the parent interrupt controller.
+- interrupts: Specifies the list of interrupt lines which are handled by
+  the interrupt controller in the parent controller's notation. This value
+  shall be the NMI.
+
+Example:
+
+sc-nmi-intc at 01c00030 {
+	compatible = "allwinner,sun7i-sc-nmi";
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	reg = <0x01c00030 0x0c>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 0 4>;
+};
-- 
1.8.5.2

[PATCH v3 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI

[Carlo Caione]

On Sat, Jan 11, 2014 at 4:19 PM, Carlo Caione <carlo.caione@gmail.com> wrote:
> Allwinner A20/A31 SoCs have a special interrupt controller for managing NMI.
> Three register are present to (un)mask, control and acknowledge NMI.
> These two patches add a new irqchip driver in cascade with GIC.
>
> Changes since v1:
>         - added binding document
>
> Changes since v2:
>         - fixed trigger type in DTS
>         - new explanations in binding documentation
>         - added support for A31 (sun6i)

Ping

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Maxime Ripard]

Hi Carlo,

On Sat, Jan 11, 2014 at 04:19:06PM +0100, Carlo Caione wrote:
> Allwinner A20/A31 SoCs have special registers to control / (un)mask /
> acknowledge NMI. This NMI controller is separated and independent from GIC.
> This patch adds a new irqchip to manage NMI.
> 
> Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
> ---
>  drivers/irqchip/Makefile        |   1 +
>  drivers/irqchip/irq-sunxi-nmi.c | 228 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 229 insertions(+)
>  create mode 100644 drivers/irqchip/irq-sunxi-nmi.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c60b901..e31d4d6 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
>  obj-$(CONFIG_ARCH_MOXART)		+= irq-moxart.o
>  obj-$(CONFIG_ORION_IRQCHIP)		+= irq-orion.o
>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
> +obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
>  obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
> diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
> new file mode 100644
> index 0000000..a2b7373
> --- /dev/null
> +++ b/drivers/irqchip/irq-sunxi-nmi.c
> @@ -0,0 +1,228 @@
> +/*
> + * Allwinner A20/A31 SoCs NMI IRQ chip driver.
> + *
> + * Carlo Caione <carlo.caione@gmail.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/bitops.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include "irqchip.h"
> +
> +#define SUNXI_NMI_SRC_TYPE_MASK	0x00000003
> +
> +enum {
> +	SUNXI_SRC_TYPE_LEVEL_LOW = 0,
> +	SUNXI_SRC_TYPE_EDGE_FALLING,
> +	SUNXI_SRC_TYPE_LEVEL_HIGH,
> +	SUNXI_SRC_TYPE_EDGE_RISING,
> +};
> +
> +struct sunxi_sc_nmi_reg_offs {
> +	u32 ctrl;
> +	u32 pend;
> +	u32 enable;
> +};
> +
> +static struct sunxi_sc_nmi_reg_offs sun7i_reg_offs = {
> +	.ctrl	= 0x00,
> +	.pend	= 0x04,
> +	.enable	= 0x08,
> +};
> +
> +static struct sunxi_sc_nmi_reg_offs sun6i_reg_offs = {
> +	.ctrl	= 0x00,
> +	.pend	= 0x04,
> +	.enable	= 0x34,
> +};
> +
> +/*
> + * Ack level interrupts right before unmask
> + *
> + * In case of level-triggered interrupt, IRQ line must be acked before it
> + * is unmasked or else a double-interrupt is triggered
> + */
> +
> +static void sunxi_sc_nmi_ack_and_unmask(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +
> +	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
> +		ct->chip.irq_ack(d);
> +
> +	irq_gc_lock(gc);
> +	irq_reg_writel(mask, gc->reg_base + ct->regs.mask);
> +	irq_gc_unlock(gc);
> +}

Hmmm, handle_level_irq seems to be doing exactly that already. It
first masks and acks the interrupts, and then unmask it, so we should
be fine, don't we?

> +static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
> +				      u32 val)
> +{
> +	irq_reg_writel(val, gc->reg_base + off);
> +}
> +
> +static inline u32 sunxi_sc_nmi_read(struct irq_chip_generic *gc, u32 off)
> +{
> +	return irq_reg_readl(gc->reg_base + off);
> +}
> +
> +static void sunxi_sc_nmi_handle_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_get_chip(irq);
> +	unsigned int virq = irq_find_mapping(domain, 0);
> +
> +	chained_irq_enter(chip, desc);
> +	generic_handle_irq(virq);
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> +	struct irq_chip_type *ct = gc->chip_types;
> +	u32 src_type_reg;
> +	u32 ctrl_off = ct->regs.type;
> +	unsigned int src_type;
> +	unsigned int i;
> +
> +	irq_gc_lock(gc);
> +
> +	switch (flow_type & IRQF_TRIGGER_MASK) {
> +	case IRQ_TYPE_EDGE_FALLING:
> +		src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		src_type = SUNXI_SRC_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
> +		break;
> +	case IRQ_TYPE_NONE:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
> +		break;
> +	default:
> +		irq_gc_unlock(gc);
> +		pr_err("%s: Cannot assign multiple trigger modes to IRQ %d.\n",
> +			__func__, data->irq);
> +		return -EBADR;
> +	}
> +
> +	irqd_set_trigger_type(data, flow_type);
> +	irq_setup_alt_chip(data, flow_type);
> +
> +	for (i = 0; i <= gc->num_ct; i++, ct++)
> +		if (ct->type & flow_type)
> +			ctrl_off = ct->regs.type;
> +
> +	src_type_reg = sunxi_sc_nmi_read(gc, ctrl_off);
> +	src_type_reg &= ~SUNXI_NMI_SRC_TYPE_MASK;
> +	src_type_reg |= src_type;
> +	sunxi_sc_nmi_write(gc, ctrl_off, src_type_reg);
> +
> +	irq_gc_unlock(gc);
> +
> +	return IRQ_SET_MASK_OK;
> +}
> +
> +static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
> +					struct sunxi_sc_nmi_reg_offs *reg_offs)
> +{
> +	struct irq_domain *domain;
> +	struct irq_chip_generic *gc;
> +	unsigned int irq;
> +	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> +	int ret;
> +
> +
> +	domain = irq_domain_add_linear(node, 1, &irq_generic_chip_ops, NULL);
> +	if (!domain) {
> +		pr_err("%s: Could not register interrupt domain.\n", node->name);
> +		return -ENOMEM;
> +	}
> +
> +	ret = irq_alloc_domain_generic_chips(domain, 1, 2, node->name,
> +					     handle_level_irq, clr, 0,
> +					     IRQ_GC_INIT_MASK_CACHE);
> +	if (ret) {
> +		 pr_err("%s: Could not allocate generic interrupt chip.\n",
> +			 node->name);
> +		 goto fail_irqd_remove;
> +	}
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0) {
> +		pr_err("%s: unable to parse irq\n", node->name);
> +		ret = -EINVAL;
> +		goto fail_irqd_remove;
> +	}
> +
> +	gc = irq_get_domain_generic_chip(domain, 0);
> +	gc->reg_base = of_iomap(node, 0);
> +	if (!gc->reg_base) {
> +		pr_err("%s: unable to map resource\n", node->name);
> +		ret = -ENOMEM;
> +		goto fail_irqd_remove;
> +	}
> +
> +	gc->chip_types[0].type			= IRQ_TYPE_LEVEL_MASK;
> +	gc->chip_types[0].chip.irq_ack		= irq_gc_ack_set_bit;
> +	gc->chip_types[0].chip.irq_mask		= irq_gc_mask_clr_bit;
> +	gc->chip_types[0].chip.irq_unmask	= sunxi_sc_nmi_ack_and_unmask;
> +	gc->chip_types[0].chip.irq_set_type	= sunxi_sc_nmi_set_type;
> +	gc->chip_types[0].regs.ack		= reg_offs->pend;
> +	gc->chip_types[0].regs.mask		= reg_offs->enable;
> +	gc->chip_types[0].regs.type		= reg_offs->ctrl;
> +
> +	gc->chip_types[1].type			= IRQ_TYPE_EDGE_BOTH;
> +	gc->chip_types[1].chip.name		= gc->chip_types[0].chip.name;
> +	gc->chip_types[1].chip.irq_ack		= irq_gc_ack_set_bit;
> +	gc->chip_types[1].chip.irq_mask		= irq_gc_mask_clr_bit;
> +	gc->chip_types[1].chip.irq_unmask	= sunxi_sc_nmi_ack_and_unmask;
> +	gc->chip_types[1].chip.irq_set_type	= sunxi_sc_nmi_set_type;
> +	gc->chip_types[1].regs.ack		= reg_offs->pend;
> +	gc->chip_types[1].regs.mask		= reg_offs->enable;
> +	gc->chip_types[1].regs.type		= reg_offs->ctrl;
> +	gc->chip_types[1].handler		= handle_edge_irq;
> +
> +	irq_set_handler_data(irq, domain);
> +	irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
> +
> +	sunxi_sc_nmi_write(gc, reg_offs->enable, 0);
> +	sunxi_sc_nmi_write(gc, reg_offs->pend, 0x1);

I really wonder whether it makes sense to have a generic chip here. It
seems to be much more complicated than it should. It's only about a
single interrupt interrupt chip here.

> +
> +	return 0;
> +
> +fail_irqd_remove:
> +	irq_domain_remove(domain);
> +
> +	return ret;
> +}
> +
> +static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
> +					struct device_node *parent)
> +{
> +	return sunxi_sc_nmi_irq_init(node, &sun6i_reg_offs);
> +}
> +IRQCHIP_DECLARE(sun6i_sc_nmi, "allwinner,sun6i-sc-nmi", sun6i_sc_nmi_irq_init);

I'm curious, where did you get these infos on the A31? :)


> +static int __init sun7i_sc_nmi_irq_init(struct device_node *node,
> +					struct device_node *parent)
> +{
> +	return sunxi_sc_nmi_irq_init(node, &sun7i_reg_offs);
> +}
> +IRQCHIP_DECLARE(sun7i_sc_nmi, "allwinner,sun7i-sc-nmi", sun7i_sc_nmi_irq_init);

The compatibles should be sun6i-a31-* and sun7i-a20-*.

Thanks for your work!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140116/0150fad3/attachment.sig>

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Carlo Caione]

On Thu, Jan 16, 2014 at 1:39 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Carlo,
>
> On Sat, Jan 11, 2014 at 04:19:06PM +0100, Carlo Caione wrote:
>> Allwinner A20/A31 SoCs have special registers to control / (un)mask /
>> acknowledge NMI. This NMI controller is separated and independent from GIC.
>> This patch adds a new irqchip to manage NMI.
>>
>> Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
>> ---
>>  drivers/irqchip/Makefile        |   1 +
>>  drivers/irqchip/irq-sunxi-nmi.c | 228 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 229 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-sunxi-nmi.c
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index c60b901..e31d4d6 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)        += irq-metag.o
>>  obj-$(CONFIG_ARCH_MOXART)            += irq-moxart.o
>>  obj-$(CONFIG_ORION_IRQCHIP)          += irq-orion.o
>>  obj-$(CONFIG_ARCH_SUNXI)             += irq-sun4i.o
>> +obj-$(CONFIG_ARCH_SUNXI)             += irq-sunxi-nmi.o
>>  obj-$(CONFIG_ARCH_SPEAR3XX)          += spear-shirq.o
>>  obj-$(CONFIG_ARM_GIC)                        += irq-gic.o
>>  obj-$(CONFIG_ARM_NVIC)                       += irq-nvic.o
>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
>> new file mode 100644
>> index 0000000..a2b7373
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>> @@ -0,0 +1,228 @@
>> +/*
>> + * Allwinner A20/A31 SoCs NMI IRQ chip driver.
>> + *
>> + * Carlo Caione <carlo.caione@gmail.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/bitops.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include "irqchip.h"
>> +
>> +#define SUNXI_NMI_SRC_TYPE_MASK      0x00000003
>> +
>> +enum {
>> +     SUNXI_SRC_TYPE_LEVEL_LOW = 0,
>> +     SUNXI_SRC_TYPE_EDGE_FALLING,
>> +     SUNXI_SRC_TYPE_LEVEL_HIGH,
>> +     SUNXI_SRC_TYPE_EDGE_RISING,
>> +};
>> +
>> +struct sunxi_sc_nmi_reg_offs {
>> +     u32 ctrl;
>> +     u32 pend;
>> +     u32 enable;
>> +};
>> +
>> +static struct sunxi_sc_nmi_reg_offs sun7i_reg_offs = {
>> +     .ctrl   = 0x00,
>> +     .pend   = 0x04,
>> +     .enable = 0x08,
>> +};
>> +
>> +static struct sunxi_sc_nmi_reg_offs sun6i_reg_offs = {
>> +     .ctrl   = 0x00,
>> +     .pend   = 0x04,
>> +     .enable = 0x34,
>> +};
>> +
>> +/*
>> + * Ack level interrupts right before unmask
>> + *
>> + * In case of level-triggered interrupt, IRQ line must be acked before it
>> + * is unmasked or else a double-interrupt is triggered
>> + */
>> +
>> +static void sunxi_sc_nmi_ack_and_unmask(struct irq_data *d)
>> +{
>> +     struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +     struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> +     u32 mask = d->mask;
>> +
>> +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
>> +             ct->chip.irq_ack(d);
>> +
>> +     irq_gc_lock(gc);
>> +     irq_reg_writel(mask, gc->reg_base + ct->regs.mask);
>> +     irq_gc_unlock(gc);
>> +}
>
> Hmmm, handle_level_irq seems to be doing exactly that already. It
> first masks and acks the interrupts, and then unmask it, so we should
> be fine, don't we?

We don't, or at least handle_level_irq doesn't work for all the cases.
Let's say (i.e. this is the cubieboard2 case) that to the irqchip is
connected to the IRQ line of a PMIC accessed by I2C. In this case we
cannot mask/ack the interrupt on the originating device in the hard
interrupt handler (in which handle_level_irq is) since we need to
access the I2C bus in an non-interrupt context. ACKing the IRQ in
handle_level_irq at this point is pretty much useless since we still
have to ACK the IRQs on the originating device and this will be done
in a IRQ thread started after the hard IRQ handler.
sunxi_sc_nmi_ack_and_unmask is therefore called (by
irq_finalize_oneshot) after the IRQ thread has been executed. After
the IRQ thread has ACKed the IRQs on the originating device we can
finally ACK and unmask again the NMI.

>> +static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
>> +                                   u32 val)
>> +{
>> +     irq_reg_writel(val, gc->reg_base + off);
>> +}
>> +
>> +static inline u32 sunxi_sc_nmi_read(struct irq_chip_generic *gc, u32 off)
>> +{
>> +     return irq_reg_readl(gc->reg_base + off);
>> +}
>> +
>> +static void sunxi_sc_nmi_handle_irq(unsigned int irq, struct irq_desc *desc)
>> +{
>> +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> +     struct irq_chip *chip = irq_get_chip(irq);
>> +     unsigned int virq = irq_find_mapping(domain, 0);
>> +
>> +     chained_irq_enter(chip, desc);
>> +     generic_handle_irq(virq);
>> +     chained_irq_exit(chip, desc);
>> +}
>> +
>> +static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type)
>> +{
>> +     struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
>> +     struct irq_chip_type *ct = gc->chip_types;
>> +     u32 src_type_reg;
>> +     u32 ctrl_off = ct->regs.type;
>> +     unsigned int src_type;
>> +     unsigned int i;
>> +
>> +     irq_gc_lock(gc);
>> +
>> +     switch (flow_type & IRQF_TRIGGER_MASK) {
>> +     case IRQ_TYPE_EDGE_FALLING:
>> +             src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
>> +             break;
>> +     case IRQ_TYPE_EDGE_RISING:
>> +             src_type = SUNXI_SRC_TYPE_EDGE_RISING;
>> +             break;
>> +     case IRQ_TYPE_LEVEL_HIGH:
>> +             src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
>> +             break;
>> +     case IRQ_TYPE_NONE:
>> +     case IRQ_TYPE_LEVEL_LOW:
>> +             src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
>> +             break;
>> +     default:
>> +             irq_gc_unlock(gc);
>> +             pr_err("%s: Cannot assign multiple trigger modes to IRQ %d.\n",
>> +                     __func__, data->irq);
>> +             return -EBADR;
>> +     }
>> +
>> +     irqd_set_trigger_type(data, flow_type);
>> +     irq_setup_alt_chip(data, flow_type);
>> +
>> +     for (i = 0; i <= gc->num_ct; i++, ct++)
>> +             if (ct->type & flow_type)
>> +                     ctrl_off = ct->regs.type;
>> +
>> +     src_type_reg = sunxi_sc_nmi_read(gc, ctrl_off);
>> +     src_type_reg &= ~SUNXI_NMI_SRC_TYPE_MASK;
>> +     src_type_reg |= src_type;
>> +     sunxi_sc_nmi_write(gc, ctrl_off, src_type_reg);
>> +
>> +     irq_gc_unlock(gc);
>> +
>> +     return IRQ_SET_MASK_OK;
>> +}
>> +
>> +static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
>> +                                     struct sunxi_sc_nmi_reg_offs *reg_offs)
>> +{
>> +     struct irq_domain *domain;
>> +     struct irq_chip_generic *gc;
>> +     unsigned int irq;
>> +     unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
>> +     int ret;
>> +
>> +
>> +     domain = irq_domain_add_linear(node, 1, &irq_generic_chip_ops, NULL);
>> +     if (!domain) {
>> +             pr_err("%s: Could not register interrupt domain.\n", node->name);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     ret = irq_alloc_domain_generic_chips(domain, 1, 2, node->name,
>> +                                          handle_level_irq, clr, 0,
>> +                                          IRQ_GC_INIT_MASK_CACHE);
>> +     if (ret) {
>> +              pr_err("%s: Could not allocate generic interrupt chip.\n",
>> +                      node->name);
>> +              goto fail_irqd_remove;
>> +     }
>> +
>> +     irq = irq_of_parse_and_map(node, 0);
>> +     if (irq <= 0) {
>> +             pr_err("%s: unable to parse irq\n", node->name);
>> +             ret = -EINVAL;
>> +             goto fail_irqd_remove;
>> +     }
>> +
>> +     gc = irq_get_domain_generic_chip(domain, 0);
>> +     gc->reg_base = of_iomap(node, 0);
>> +     if (!gc->reg_base) {
>> +             pr_err("%s: unable to map resource\n", node->name);
>> +             ret = -ENOMEM;
>> +             goto fail_irqd_remove;
>> +     }
>> +
>> +     gc->chip_types[0].type                  = IRQ_TYPE_LEVEL_MASK;
>> +     gc->chip_types[0].chip.irq_ack          = irq_gc_ack_set_bit;
>> +     gc->chip_types[0].chip.irq_mask         = irq_gc_mask_clr_bit;
>> +     gc->chip_types[0].chip.irq_unmask       = sunxi_sc_nmi_ack_and_unmask;
>> +     gc->chip_types[0].chip.irq_set_type     = sunxi_sc_nmi_set_type;
>> +     gc->chip_types[0].regs.ack              = reg_offs->pend;
>> +     gc->chip_types[0].regs.mask             = reg_offs->enable;
>> +     gc->chip_types[0].regs.type             = reg_offs->ctrl;
>> +
>> +     gc->chip_types[1].type                  = IRQ_TYPE_EDGE_BOTH;
>> +     gc->chip_types[1].chip.name             = gc->chip_types[0].chip.name;
>> +     gc->chip_types[1].chip.irq_ack          = irq_gc_ack_set_bit;
>> +     gc->chip_types[1].chip.irq_mask         = irq_gc_mask_clr_bit;
>> +     gc->chip_types[1].chip.irq_unmask       = sunxi_sc_nmi_ack_and_unmask;
>> +     gc->chip_types[1].chip.irq_set_type     = sunxi_sc_nmi_set_type;
>> +     gc->chip_types[1].regs.ack              = reg_offs->pend;
>> +     gc->chip_types[1].regs.mask             = reg_offs->enable;
>> +     gc->chip_types[1].regs.type             = reg_offs->ctrl;
>> +     gc->chip_types[1].handler               = handle_edge_irq;
>> +
>> +     irq_set_handler_data(irq, domain);
>> +     irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
>> +
>> +     sunxi_sc_nmi_write(gc, reg_offs->enable, 0);
>> +     sunxi_sc_nmi_write(gc, reg_offs->pend, 0x1);
>
> I really wonder whether it makes sense to have a generic chip here. It
> seems to be much more complicated than it should. It's only about a
> single interrupt interrupt chip here.

I agree but is there any other way to manage the NMI without the
driver of the device connected to NMI having to worry about
acking/masking/unmasking/ etc..?

>> +
>> +     return 0;
>> +
>> +fail_irqd_remove:
>> +     irq_domain_remove(domain);
>> +
>> +     return ret;
>> +}
>> +
>> +static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
>> +                                     struct device_node *parent)
>> +{
>> +     return sunxi_sc_nmi_irq_init(node, &sun6i_reg_offs);
>> +}
>> +IRQCHIP_DECLARE(sun6i_sc_nmi, "allwinner,sun6i-sc-nmi", sun6i_sc_nmi_irq_init);
>
> I'm curious, where did you get these infos on the A31? :)

Ehm actually it was in the mail exchange we have had with the
Allwinner engineers :)

>
>> +static int __init sun7i_sc_nmi_irq_init(struct device_node *node,
>> +                                     struct device_node *parent)
>> +{
>> +     return sunxi_sc_nmi_irq_init(node, &sun7i_reg_offs);
>> +}
>> +IRQCHIP_DECLARE(sun7i_sc_nmi, "allwinner,sun7i-sc-nmi", sun7i_sc_nmi_irq_init);
>
> The compatibles should be sun6i-a31-* and sun7i-a20-*.

Fix in v4.

Thanks,

--
Carlo Caione

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Maxime Ripard]

On Fri, Jan 17, 2014 at 09:54:55AM +0100, Carlo Caione wrote:
> >> +/*
> >> + * Ack level interrupts right before unmask
> >> + *
> >> + * In case of level-triggered interrupt, IRQ line must be acked before it
> >> + * is unmasked or else a double-interrupt is triggered
> >> + */
> >> +
> >> +static void sunxi_sc_nmi_ack_and_unmask(struct irq_data *d)
> >> +{
> >> +     struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> >> +     struct irq_chip_type *ct = irq_data_get_chip_type(d);
> >> +     u32 mask = d->mask;
> >> +
> >> +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
> >> +             ct->chip.irq_ack(d);
> >> +
> >> +     irq_gc_lock(gc);
> >> +     irq_reg_writel(mask, gc->reg_base + ct->regs.mask);
> >> +     irq_gc_unlock(gc);
> >> +}
> >
> > Hmmm, handle_level_irq seems to be doing exactly that already. It
> > first masks and acks the interrupts, and then unmask it, so we should
> > be fine, don't we?
> 
> We don't, or at least handle_level_irq doesn't work for all the cases.

This is what I find weird :)

> Let's say (i.e. this is the cubieboard2 case) that to the irqchip is
> connected to the IRQ line of a PMIC accessed by I2C. In this case we
> cannot mask/ack the interrupt on the originating device in the hard
> interrupt handler (in which handle_level_irq is) since we need to
> access the I2C bus in an non-interrupt context.

We agree here.

> ACKing the IRQ in handle_level_irq at this point is pretty much
> useless since we still have to ACK the IRQs on the originating
> device and this will be done in a IRQ thread started after the hard
> IRQ handler.

Ok, so what you're saying is that you want to adress this case:

        handle_level_irq
               |          device    device
               | mask ack handler irq acked unmask
               |  |    |    |         |       |
               v  v    v    v         v       v 

NMI -> GIC:
               +--------+     +-----------------
---------------+        +-----+

PMIC -> NMI: 
            +-+           +-+
------------+ +-----------+ +--------------------

Right?

But in this case, you would have two events coming from your device
(the two rising edges), so you'd expect two interrupts. And in the
case of a level triggered interrupt, the device would keep the
interrupt line active until it's unmasked, so we wouldn't end up with
this either.

> sunxi_sc_nmi_ack_and_unmask is therefore called (by
> irq_finalize_oneshot) after the IRQ thread has been executed. After
> the IRQ thread has ACKed the IRQs on the originating device we can
> finally ACK and unmask again the NMI.

And what happens if you get a new interrupt right between the end of
the handler and the unmask?

> 
> >> +static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
> >> +                                   u32 val)
> >> +{
> >> +     irq_reg_writel(val, gc->reg_base + off);
> >> +}
> >> +
> >> +static inline u32 sunxi_sc_nmi_read(struct irq_chip_generic *gc, u32 off)
> >> +{
> >> +     return irq_reg_readl(gc->reg_base + off);
> >> +}
> >> +
> >> +static void sunxi_sc_nmi_handle_irq(unsigned int irq, struct irq_desc *desc)
> >> +{
> >> +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
> >> +     struct irq_chip *chip = irq_get_chip(irq);
> >> +     unsigned int virq = irq_find_mapping(domain, 0);
> >> +
> >> +     chained_irq_enter(chip, desc);
> >> +     generic_handle_irq(virq);
> >> +     chained_irq_exit(chip, desc);
> >> +}
> >> +
> >> +static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type)
> >> +{
> >> +     struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
> >> +     struct irq_chip_type *ct = gc->chip_types;
> >> +     u32 src_type_reg;
> >> +     u32 ctrl_off = ct->regs.type;
> >> +     unsigned int src_type;
> >> +     unsigned int i;
> >> +
> >> +     irq_gc_lock(gc);
> >> +
> >> +     switch (flow_type & IRQF_TRIGGER_MASK) {
> >> +     case IRQ_TYPE_EDGE_FALLING:
> >> +             src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
> >> +             break;
> >> +     case IRQ_TYPE_EDGE_RISING:
> >> +             src_type = SUNXI_SRC_TYPE_EDGE_RISING;
> >> +             break;
> >> +     case IRQ_TYPE_LEVEL_HIGH:
> >> +             src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
> >> +             break;
> >> +     case IRQ_TYPE_NONE:
> >> +     case IRQ_TYPE_LEVEL_LOW:
> >> +             src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
> >> +             break;
> >> +     default:
> >> +             irq_gc_unlock(gc);
> >> +             pr_err("%s: Cannot assign multiple trigger modes to IRQ %d.\n",
> >> +                     __func__, data->irq);
> >> +             return -EBADR;
> >> +     }
> >> +
> >> +     irqd_set_trigger_type(data, flow_type);
> >> +     irq_setup_alt_chip(data, flow_type);
> >> +
> >> +     for (i = 0; i <= gc->num_ct; i++, ct++)
> >> +             if (ct->type & flow_type)
> >> +                     ctrl_off = ct->regs.type;
> >> +
> >> +     src_type_reg = sunxi_sc_nmi_read(gc, ctrl_off);
> >> +     src_type_reg &= ~SUNXI_NMI_SRC_TYPE_MASK;
> >> +     src_type_reg |= src_type;
> >> +     sunxi_sc_nmi_write(gc, ctrl_off, src_type_reg);
> >> +
> >> +     irq_gc_unlock(gc);
> >> +
> >> +     return IRQ_SET_MASK_OK;
> >> +}
> >> +
> >> +static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
> >> +                                     struct sunxi_sc_nmi_reg_offs *reg_offs)
> >> +{
> >> +     struct irq_domain *domain;
> >> +     struct irq_chip_generic *gc;
> >> +     unsigned int irq;
> >> +     unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> >> +     int ret;
> >> +
> >> +
> >> +     domain = irq_domain_add_linear(node, 1, &irq_generic_chip_ops, NULL);
> >> +     if (!domain) {
> >> +             pr_err("%s: Could not register interrupt domain.\n", node->name);
> >> +             return -ENOMEM;
> >> +     }
> >> +
> >> +     ret = irq_alloc_domain_generic_chips(domain, 1, 2, node->name,
> >> +                                          handle_level_irq, clr, 0,
> >> +                                          IRQ_GC_INIT_MASK_CACHE);
> >> +     if (ret) {
> >> +              pr_err("%s: Could not allocate generic interrupt chip.\n",
> >> +                      node->name);
> >> +              goto fail_irqd_remove;
> >> +     }
> >> +
> >> +     irq = irq_of_parse_and_map(node, 0);
> >> +     if (irq <= 0) {
> >> +             pr_err("%s: unable to parse irq\n", node->name);
> >> +             ret = -EINVAL;
> >> +             goto fail_irqd_remove;
> >> +     }
> >> +
> >> +     gc = irq_get_domain_generic_chip(domain, 0);
> >> +     gc->reg_base = of_iomap(node, 0);
> >> +     if (!gc->reg_base) {
> >> +             pr_err("%s: unable to map resource\n", node->name);
> >> +             ret = -ENOMEM;
> >> +             goto fail_irqd_remove;
> >> +     }
> >> +
> >> +     gc->chip_types[0].type                  = IRQ_TYPE_LEVEL_MASK;
> >> +     gc->chip_types[0].chip.irq_ack          = irq_gc_ack_set_bit;
> >> +     gc->chip_types[0].chip.irq_mask         = irq_gc_mask_clr_bit;
> >> +     gc->chip_types[0].chip.irq_unmask       = sunxi_sc_nmi_ack_and_unmask;
> >> +     gc->chip_types[0].chip.irq_set_type     = sunxi_sc_nmi_set_type;
> >> +     gc->chip_types[0].regs.ack              = reg_offs->pend;
> >> +     gc->chip_types[0].regs.mask             = reg_offs->enable;
> >> +     gc->chip_types[0].regs.type             = reg_offs->ctrl;
> >> +
> >> +     gc->chip_types[1].type                  = IRQ_TYPE_EDGE_BOTH;
> >> +     gc->chip_types[1].chip.name             = gc->chip_types[0].chip.name;
> >> +     gc->chip_types[1].chip.irq_ack          = irq_gc_ack_set_bit;
> >> +     gc->chip_types[1].chip.irq_mask         = irq_gc_mask_clr_bit;
> >> +     gc->chip_types[1].chip.irq_unmask       = sunxi_sc_nmi_ack_and_unmask;
> >> +     gc->chip_types[1].chip.irq_set_type     = sunxi_sc_nmi_set_type;
> >> +     gc->chip_types[1].regs.ack              = reg_offs->pend;
> >> +     gc->chip_types[1].regs.mask             = reg_offs->enable;
> >> +     gc->chip_types[1].regs.type             = reg_offs->ctrl;
> >> +     gc->chip_types[1].handler               = handle_edge_irq;
> >> +
> >> +     irq_set_handler_data(irq, domain);
> >> +     irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
> >> +
> >> +     sunxi_sc_nmi_write(gc, reg_offs->enable, 0);
> >> +     sunxi_sc_nmi_write(gc, reg_offs->pend, 0x1);
> >
> > I really wonder whether it makes sense to have a generic chip here. It
> > seems to be much more complicated than it should. It's only about a
> > single interrupt interrupt chip here.
> 
> I agree but is there any other way to manage the NMI without the
> driver of the device connected to NMI having to worry about
> acking/masking/unmasking/ etc..?

Yes, just declare a "raw" irqchip. Pretty much like we do on irq-sun4i
for example.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140128/416961c6/attachment.sig>

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Hans de Goede]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

On 01/28/2014 11:40 AM, Maxime Ripard wrote:

Jumping in here to try and clarify things, or so I hope at least :)

> On Fri, Jan 17, 2014 at 09:54:55AM +0100, Carlo Caione wrote:
>>>> +/* + * Ack level interrupts right before unmask + * + * In case of level-triggered interrupt, IRQ line must be acked before it + * is unmasked or else a double-interrupt is triggered + */ + +static void sunxi_sc_nmi_ack_and_unmask(struct irq_data *d) +{ +     struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); +     struct irq_chip_type *ct = irq_data_get_chip_type(d); +     u32 mask = d->mask; + +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) +             ct->chip.irq_ack(d); + +     irq_gc_lock(gc); +     irq_reg_writel(mask, gc->reg_base + ct->regs.mask); +
>>>> irq_gc_unlock(gc); +}
>>> 
>>> Hmmm, handle_level_irq seems to be doing exactly that already. It first masks and acks the interrupts, and then unmask it, so we should be fine, don't we?
>> 
>> We don't, or at least handle_level_irq doesn't work for all the cases.
> 
> This is what I find weird :)
> 
>> Let's say (i.e. this is the cubieboard2 case) that to the irqchip is connected to the IRQ line of a PMIC accessed by I2C. In this case we cannot mask/ack the interrupt on the originating device in the hard interrupt handler (in which handle_level_irq is) since we need to access the I2C bus in an non-interrupt context.
> 
> We agree here.
> 
>> ACKing the IRQ in handle_level_irq at this point is pretty much useless since we still have to ACK the IRQs on the originating device and this will be done in a IRQ thread started after the hard IRQ handler.
> 
> Ok, so what you're saying is that you want to adress this case:
> 
> handle_level_irq |          device    device | mask ack handler irq acked unmask |  |    |    |         |       | v  v    v    v         v       v
> 
> NMI -> GIC: +--------+     +----------------- ---------------+        +-----+
> 
> PMIC -> NMI: +-+           +-+ ------------+ +-----------+ +--------------------
> 
> Right?

No, the IRQ from the PMIC is a level sensitive IRQ, so it would look like this:

> handle_level_irq |          device    device | mask ack handler irq acked unmask |  |    |    |         |       | v  v    v    v         v       v
> 
> NMI -> GIC: +------------------------------+ ---------------+                              +--
> 
> PMIC -> NMI: +-------------------------+ ------------+                         +----------

The PMIC irq line won't go low until an i2c write to its irq status
registers write-clears all status bits for which the corresponding
bit in the irq-mask register is set.

And the only reason the NMI -> GIC also goes low is because the unmask
operation writes a second ack to the NMI controller in the unmask
callback of the NMI controller driver.

Note that we cannot use edge triggered interrupts here because the PMIC
has the typical setup with multiple irq status bits driving a single
irq line, so the irq handler does read irq-status, handle stuff,
write-clear irq-status. And if any other irq-status bits get set
between the read and write-clear the PMIC -> NMI line will stay
high, as it should since there are more interrupts to handle.

> But in this case, you would have two events coming from your device (the two rising edges), so you'd expect two interrupts. And in the case of a level triggered interrupt, the device would keep the interrupt line active until it's unmasked, so we wouldn't end up with this either.
> 
>> sunxi_sc_nmi_ack_and_unmask is therefore called (by irq_finalize_oneshot) after the IRQ thread has been executed. After the IRQ thread has ACKed the IRQs on the originating device we can finally ACK and unmask again the NMI.
> 
> And what happens if you get a new interrupt right between the end of the handler and the unmask?

The implicit ack done by the unmask will be ignored if the NMI line is still
high, just like the initial ack is ignored (which is why we need the mask),
and when the unmask completes the irq will immediately retrigger, as it should.


<snip>

>>> I really wonder whether it makes sense to have a generic chip here. It seems to be much more complicated than it should. It's only about a single interrupt interrupt chip here.
>> 
>> I agree but is there any other way to manage the NMI without the driver of the device connected to NMI having to worry about acking/masking/unmasking/ etc..?
> 
> Yes, just declare a "raw" irqchip. Pretty much like we do on irq-sun4i for example.

Hmm, I'm not going to say that using a raw irqchip here is a bad idea, it
sounds like it would be better.

But I don't think this will solve the need the mask / umask. The problem is
we need to do an i2c write to clear the interrupt source, which needs to
be done from a thread / workqueue. So when the interrupt handler finishes
the source won't be cleared yet, and AFAIK then only way to deal with this
is to mask the interrupt until we've cleared the interrupt source.

I agree that ideally we would be able to hide all this inside the NMI
controller driver, but I'm not sure if we can.

Regards,

Hans
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLnjj8ACgkQF3VEtJrzE/skqACgjGLU2QLQUq9o0pU1DuuWIUpx
YngAoJmbmCGEhkRBy5xFmKuXapilqzmM
=BoL/
-----END PGP SIGNATURE-----

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Maxime Ripard]

Hi,

On Tue, Jan 28, 2014 at 12:02:23PM +0100, Hans de Goede wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi,
> 
> On 01/28/2014 11:40 AM, Maxime Ripard wrote:
> 
> Jumping in here to try and clarify things, or so I hope at least :)

Sure :)

> No, the IRQ from the PMIC is a level sensitive IRQ, so it would look
> like this:

Hmm, your mailer seems to have mangled your drawing :(

> The PMIC irq line won't go low until an i2c write to its irq status
> registers write-clears all status bits for which the corresponding
> bit in the irq-mask register is set.

Which makes sense too

> And the only reason the NMI -> GIC also goes low is because the unmask
> operation writes a second ack to the NMI controller in the unmask
> callback of the NMI controller driver.

Yes, and this is exactly what I don't understand. You shouldn't need
that ack in first place, since it's been done already right after the
unmask.

> Note that we cannot use edge triggered interrupts here because the PMIC
> has the typical setup with multiple irq status bits driving a single
> irq line, so the irq handler does read irq-status, handle stuff,
> write-clear irq-status. And if any other irq-status bits get set
> between the read and write-clear the PMIC -> NMI line will stay
> high, as it should since there are more interrupts to handle.

Yep, the edge-thing was just the only case I could think of where it
could lead to troubles.

In what you're saying, which makes total sense, if we don't do the
ack, as soon as the irq will be unmasked, since the level is high, the
handler will be called again, treat the new interrupts, and so on. I
don't see how this is an issue actually.

> > But in this case, you would have two events coming from your
> > device (the two rising edges), so you'd expect two interrupts. And
> > in the case of a level triggered interrupt, the device would keep
> > the interrupt line active until it's unmasked, so we wouldn't end
> > up with this either.
> > 
> >> sunxi_sc_nmi_ack_and_unmask is therefore called (by
> >> irq_finalize_oneshot) after the IRQ thread has been
> >> executed. After the IRQ thread has ACKed the IRQs on the
> >> originating device we can finally ACK and unmask again the NMI.
> > 
> > And what happens if you get a new interrupt right between the end
> > of the handler and the unmask?
> 
> The implicit ack done by the unmask will be ignored if the NMI line
> is still high, just like the initial ack is ignored (which is why we
> need the mask), and when the unmask completes the irq will
> immediately retrigger, as it should.

Yeah, but why do we need the ack in the first place? I can't think of
a case where the ACK would be doing something useful. Either:
  - there is no new interrupts between the mask/ack and the unmask, so
    there's no interrupt to ack.
  - There's a new interrupt between the mask/ack and the
    unmask. There's two more cases here:
    * The interrupt came before the device handler kicked in, and the
      handler will treat it/ack it: No issue
    * The interrupt comes right after the handler has been acking its
      interrupt, the level stays high, your handler is called once
      again, you can treat it: No issue

> >>> I really wonder whether it makes sense to have a generic chip
> >>> here. It seems to be much more complicated than it should. It's
> >>> only about a single interrupt interrupt chip here.
> >> 
> >> I agree but is there any other way to manage the NMI without the
> >> driver of the device connected to NMI having to worry about
> >> acking/masking/unmasking/ etc..?
> > 
> > Yes, just declare a "raw" irqchip. Pretty much like we do on
> > irq-sun4i for example.
> 
> Hmm, I'm not going to say that using a raw irqchip here is a bad
> idea, it sounds like it would be better.

There's none. It was a separate comment :)

> But I don't think this will solve the need the mask / umask. The
> problem is we need to do an i2c write to clear the interrupt source,
> which needs to be done from a thread / workqueue. So when the
> interrupt handler finishes the source won't be cleared yet, and
> AFAIK then only way to deal with this is to mask the interrupt until
> we've cleared the interrupt source.

Yes, but the interrupt is still masked during the time between the
"hard" handler and the thread, so there's no way you can get an
interrupt flood during that time.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140128/18a6c784/attachment.sig>

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Carlo Caione]

On Tue, Jan 28, 2014 at 12:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> On 01/28/2014 11:40 AM, Maxime Ripard wrote:
>
> Jumping in here to try and clarify things, or so I hope at least :)

Thank you :) I'm sure you will explain this better than me.

> The PMIC irq line won't go low until an i2c write to its irq status
> registers write-clears all status bits for which the corresponding
> bit in the irq-mask register is set.
>
> And the only reason the NMI -> GIC also goes low is because the unmask
> operation writes a second ack to the NMI controller in the unmask
> callback of the NMI controller driver.

Exactly. PMIC -> NMI is cleared when the irq handler write-clears all
the IRQs on PMIC while NMI -> GIC is cleared by the ACK in the unmask
callback.

<snip>

> Hmm, I'm not going to say that using a raw irqchip here is a bad idea, it
> sounds like it would be better.
>
> But I don't think this will solve the need the mask / umask. The problem is
> we need to do an i2c write to clear the interrupt source, which needs to
> be done from a thread / workqueue. So when the interrupt handler finishes
> the source won't be cleared yet, and AFAIK then only way to deal with this
> is to mask the interrupt until we've cleared the interrupt source.

That's why the driver using the NMI controller has to use IRQF_ONESHOT
so that the interrupt is not unmasked after the hard interrupt context
handler but it is unmasked after the thread handler function has been
executed (calling the unmask callback)

Thank you for explaining this mess you couldn't be clearer than that.

-- 
Carlo Caione

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Carlo Caione]

Hi,

On Tue, Jan 28, 2014 at 5:41 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Tue, Jan 28, 2014 at 12:02:23PM +0100, Hans de Goede wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi,
>>
>> On 01/28/2014 11:40 AM, Maxime Ripard wrote:
>>
>> Jumping in here to try and clarify things, or so I hope at least :)
>
> Sure :)
>
>> No, the IRQ from the PMIC is a level sensitive IRQ, so it would look
>> like this:
>
> Hmm, your mailer seems to have mangled your drawing :(
>
>> The PMIC irq line won't go low until an i2c write to its irq status
>> registers write-clears all status bits for which the corresponding
>> bit in the irq-mask register is set.
>
> Which makes sense too
>
>> And the only reason the NMI -> GIC also goes low is because the unmask
>> operation writes a second ack to the NMI controller in the unmask
>> callback of the NMI controller driver.
>
> Yes, and this is exactly what I don't understand. You shouldn't need
> that ack in first place, since it's been done already right after the
> unmask.

But the first ack is ignored since the IRQ line is still maintained
asserted by PMIC.

>> Note that we cannot use edge triggered interrupts here because the PMIC
>> has the typical setup with multiple irq status bits driving a single
>> irq line, so the irq handler does read irq-status, handle stuff,
>> write-clear irq-status. And if any other irq-status bits get set
>> between the read and write-clear the PMIC -> NMI line will stay
>> high, as it should since there are more interrupts to handle.
>
> Yep, the edge-thing was just the only case I could think of where it
> could lead to troubles.
>
> In what you're saying, which makes total sense, if we don't do the
> ack, as soon as the irq will be unmasked, since the level is high, the
> handler will be called again, treat the new interrupts, and so on. I
> don't see how this is an issue actually.

This is exactly why in unmask callback we first ACK and then unmask.
So, if the line is still maintained up by PMIC then a new interrupt is
raised otherwise nothing happens.

>> > But in this case, you would have two events coming from your
>> > device (the two rising edges), so you'd expect two interrupts. And
>> > in the case of a level triggered interrupt, the device would keep
>> > the interrupt line active until it's unmasked, so we wouldn't end
>> > up with this either.
>> >
>> >> sunxi_sc_nmi_ack_and_unmask is therefore called (by
>> >> irq_finalize_oneshot) after the IRQ thread has been
>> >> executed. After the IRQ thread has ACKed the IRQs on the
>> >> originating device we can finally ACK and unmask again the NMI.
>> >
>> > And what happens if you get a new interrupt right between the end
>> > of the handler and the unmask?
>>
>> The implicit ack done by the unmask will be ignored if the NMI line
>> is still high, just like the initial ack is ignored (which is why we
>> need the mask), and when the unmask completes the irq will
>> immediately retrigger, as it should.
>
> Yeah, but why do we need the ack in the first place? I can't think of
> a case where the ACK would be doing something useful. Either:
>   - there is no new interrupts between the mask/ack and the unmask, so
>     there's no interrupt to ack.
>   - There's a new interrupt between the mask/ack and the
>     unmask. There's two more cases here:
>     * The interrupt came before the device handler kicked in, and the
>       handler will treat it/ack it: No issue
>     * The interrupt comes right after the handler has been acking its
>       interrupt, the level stays high, your handler is called once
>       again, you can treat it: No issue

AFAIU the problem here is that the only ACK that is able to assert the
line NMI -> GIC is the ACK by the unmask callback. All the others ACKs
before that one are ignored by the NMI controller since the line PMIC
-> NMI is still asserted.

>> >>> I really wonder whether it makes sense to have a generic chip
>> >>> here. It seems to be much more complicated than it should. It's
>> >>> only about a single interrupt interrupt chip here.
>> >>
>> >> I agree but is there any other way to manage the NMI without the
>> >> driver of the device connected to NMI having to worry about
>> >> acking/masking/unmasking/ etc..?
>> >
>> > Yes, just declare a "raw" irqchip. Pretty much like we do on
>> > irq-sun4i for example.
>>
>> Hmm, I'm not going to say that using a raw irqchip here is a bad
>> idea, it sounds like it would be better.
>
> There's none. It was a separate comment :)
>
>> But I don't think this will solve the need the mask / umask. The
>> problem is we need to do an i2c write to clear the interrupt source,
>> which needs to be done from a thread / workqueue. So when the
>> interrupt handler finishes the source won't be cleared yet, and
>> AFAIK then only way to deal with this is to mask the interrupt until
>> we've cleared the interrupt source.
>
> Yes, but the interrupt is still masked during the time between the
> "hard" handler and the thread, so there's no way you can get an
> interrupt flood during that time.

agree.

BTW fortunately we can talk about this also during FOSDEM ;)

Thank you,

-- 
Carlo Caione

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Maxime Ripard]

On Tue, Jan 28, 2014 at 10:02:46PM +0100, Carlo Caione wrote:
> Hi,
> 
> On Tue, Jan 28, 2014 at 5:41 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Tue, Jan 28, 2014 at 12:02:23PM +0100, Hans de Goede wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Hi,
> >>
> >> On 01/28/2014 11:40 AM, Maxime Ripard wrote:
> >>
> >> Jumping in here to try and clarify things, or so I hope at least :)
> >
> > Sure :)
> >
> >> No, the IRQ from the PMIC is a level sensitive IRQ, so it would look
> >> like this:
> >
> > Hmm, your mailer seems to have mangled your drawing :(
> >
> >> The PMIC irq line won't go low until an i2c write to its irq status
> >> registers write-clears all status bits for which the corresponding
> >> bit in the irq-mask register is set.
> >
> > Which makes sense too
> >
> >> And the only reason the NMI -> GIC also goes low is because the unmask
> >> operation writes a second ack to the NMI controller in the unmask
> >> callback of the NMI controller driver.
> >
> > Yes, and this is exactly what I don't understand. You shouldn't need
> > that ack in first place, since it's been done already right after the
> > unmask.
> 
> But the first ack is ignored since the IRQ line is still maintained
> asserted by PMIC.
> 
> >> Note that we cannot use edge triggered interrupts here because the PMIC
> >> has the typical setup with multiple irq status bits driving a single
> >> irq line, so the irq handler does read irq-status, handle stuff,
> >> write-clear irq-status. And if any other irq-status bits get set
> >> between the read and write-clear the PMIC -> NMI line will stay
> >> high, as it should since there are more interrupts to handle.
> >
> > Yep, the edge-thing was just the only case I could think of where it
> > could lead to troubles.
> >
> > In what you're saying, which makes total sense, if we don't do the
> > ack, as soon as the irq will be unmasked, since the level is high, the
> > handler will be called again, treat the new interrupts, and so on. I
> > don't see how this is an issue actually.
> 
> This is exactly why in unmask callback we first ACK and then unmask.
> So, if the line is still maintained up by PMIC then a new interrupt is
> raised otherwise nothing happens.
> 
> >> > But in this case, you would have two events coming from your
> >> > device (the two rising edges), so you'd expect two interrupts. And
> >> > in the case of a level triggered interrupt, the device would keep
> >> > the interrupt line active until it's unmasked, so we wouldn't end
> >> > up with this either.
> >> >
> >> >> sunxi_sc_nmi_ack_and_unmask is therefore called (by
> >> >> irq_finalize_oneshot) after the IRQ thread has been
> >> >> executed. After the IRQ thread has ACKed the IRQs on the
> >> >> originating device we can finally ACK and unmask again the NMI.
> >> >
> >> > And what happens if you get a new interrupt right between the end
> >> > of the handler and the unmask?
> >>
> >> The implicit ack done by the unmask will be ignored if the NMI line
> >> is still high, just like the initial ack is ignored (which is why we
> >> need the mask), and when the unmask completes the irq will
> >> immediately retrigger, as it should.
> >
> > Yeah, but why do we need the ack in the first place? I can't think of
> > a case where the ACK would be doing something useful. Either:
> >   - there is no new interrupts between the mask/ack and the unmask, so
> >     there's no interrupt to ack.
> >   - There's a new interrupt between the mask/ack and the
> >     unmask. There's two more cases here:
> >     * The interrupt came before the device handler kicked in, and the
> >       handler will treat it/ack it: No issue
> >     * The interrupt comes right after the handler has been acking its
> >       interrupt, the level stays high, your handler is called once
> >       again, you can treat it: No issue
> 
> AFAIU the problem here is that the only ACK that is able to assert the
> line NMI -> GIC is the ACK by the unmask callback. All the others ACKs
> before that one are ignored by the NMI controller since the line PMIC
> -> NMI is still asserted.

So, to sum things up, what you see is something like:

        handle_level_irq
               |              device    device
               | mask ack     handler irq acked unmask
               |  |    |         |         |       |
               v  v    v         v         v       v

NMI -> GIC:
               +--------+     +---------------------
---------------+        +-----+

PMIC -> NMI:
            +-------------------------+
------------+                         +-------------

And you get a "rogue" retrigger because the NMI -> GIC level went up
again.

I'm not exactly sure on how to fix this. Maybe by adding a call to the
irqchip's ack just before the unmask in irq_finalize_oneshot?

Thomas, what are your thoughts on this?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140129/f5612997/attachment.sig>

[PATCH v3 1/3] ARM: sun7i/sun6i: irqchip: Add irqchip driver for NMI controller

[Carlo Caione]

On Wed, Jan 29, 2014 at 1:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

>
> So, to sum things up, what you see is something like:
>
>         handle_level_irq
>                |              device    device
>                | mask ack     handler irq acked unmask
>                |  |    |         |         |       |
>                v  v    v         v         v       v
>
> NMI -> GIC:
>                +--------+     +---------------------
> ---------------+        +-----+
>
> PMIC -> NMI:
>             +-------------------------+
> ------------+                         +-------------
>
> And you get a "rogue" retrigger because the NMI -> GIC level went up
> again.

I'd say something like:

         handle_level_irq
                |              device    device
                | mask ack     handler irq acked unmask
                |  |    |         |         |       |
                v  v    v         v         v       v

 NMI -> GIC:
                +-----------------------------+
 ---------------+                             +------

 PMIC -> NMI:
             +-------------------------+
 ------------+                         +-------------


> I'm not exactly sure on how to fix this. Maybe by adding a call to the
> irqchip's ack just before the unmask in irq_finalize_oneshot?

That is exactly what the unmask callback in the NMI controller driver does.
The unmask_irq() in irq_finalize_oneshot() calls the unmask callback
in the driver (sunxi_sc_nmi_ack_and_unmask()) that ACKs the line
before unmasking it again.

Best,

-- 
Carlo Caione