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

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

[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)

Changes since v3:
        - changed compatibles

Changes since v4:
        - fixed binding documentation

Changes since v5:
        - switched to handle_fasteoi_irq handler to avoid the double
          interrupts issue

Changes since v6:
	- changed node name
	- deleted defaulted interrupt-parent property

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                    |  27 +++
 arch/arm/boot/dts/sun6i-a31.dtsi                   |   8 +
 arch/arm/boot/dts/sun7i-a20.dtsi                   |   8 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-sunxi-nmi.c                    | 208 +++++++++++++++++++++
 5 files changed, 252 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.3.2

[PATCH v7 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-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-sunxi-nmi.c | 208 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+)
 create mode 100644 drivers/irqchip/irq-sunxi-nmi.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 5194afb..1c0c151 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -12,6 +12,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..1c8566c
--- /dev/null
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -0,0 +1,208 @@
+/*
+ * Allwinner A20/A31 SoCs NMI IRQ chip driver.
+ *
+ * Carlo Caione <carlo.caione-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * 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,
+};
+
+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_fasteoi_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_mask		= irq_gc_mask_clr_bit;
+	gc->chip_types[0].chip.irq_unmask	= irq_gc_mask_set_bit;
+	gc->chip_types[0].chip.irq_eoi		= irq_gc_ack_set_bit;
+	gc->chip_types[0].chip.irq_set_type	= sunxi_sc_nmi_set_type;
+	gc->chip_types[0].chip.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED;
+	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	= irq_gc_mask_set_bit;
+	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-a31-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-a20-sc-nmi", sun7i_sc_nmi_irq_init);
-- 
1.8.3.2

[PATCH v7 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-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 8 ++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 5256ad9..eea6033 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -190,6 +190,14 @@
 		#size-cells = <1>;
 		ranges;
 
+		nmi_intc: interrupt-controller@01f00c0c {
+			compatible = "allwinner,sun6i-a31-sc-nmi";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x01f00c0c 0x38>;
+			interrupts = <0 0 4>;
+		};
+
 		pio: pinctrl@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 6f25cf5..7637f12 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -339,6 +339,14 @@
 		#size-cells = <1>;
 		ranges;
 
+		nmi_intc: interrupt-controller@01c00030 {
+			compatible = "allwinner,sun7i-a20-sc-nmi";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x01c00030 0x0c>;
+			interrupts = <0 0 4>;
+		};
+
 		emac: ethernet@01c0b000 {
 			compatible = "allwinner,sun4i-a10-emac";
 			reg = <0x01c0b000 0x1000>;
-- 
1.8.3.2

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Wed, Mar 19, 2014 at 08:21:18PM +0100, Carlo Caione wrote:
> This patch adds DTS entries for NMI controller as child of GIC.
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Hans de Goede]

Hi,

On 03/19/2014 08:21 PM, Carlo Caione wrote:
> This patch adds DTS entries for NMI controller as child of GIC.
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

Note this breaks the kernel on sun6i / A31 since we don't have a
pmic driver there yet, and thus the nmi gets constantly fired without
anything clearing it.

So the sun6i section needs a status = "disabled"; until we actually have pmic
support.

Regards,

Hans

> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 8 ++++++++
>  arch/arm/boot/dts/sun7i-a20.dtsi | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 5256ad9..eea6033 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -190,6 +190,14 @@
>  		#size-cells = <1>;
>  		ranges;
>  
> +		nmi_intc: interrupt-controller@01f00c0c {
> +			compatible = "allwinner,sun6i-a31-sc-nmi";
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			reg = <0x01f00c0c 0x38>;
> +			interrupts = <0 0 4>;
> +		};
> +
>  		pio: pinctrl@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 6f25cf5..7637f12 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -339,6 +339,14 @@
>  		#size-cells = <1>;
>  		ranges;
>  
> +		nmi_intc: interrupt-controller@01c00030 {
> +			compatible = "allwinner,sun7i-a20-sc-nmi";
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			reg = <0x01c00030 0x0c>;
> +			interrupts = <0 0 4>;
> +		};
> +
>  		emac: ethernet@01c0b000 {
>  			compatible = "allwinner,sun4i-a10-emac";
>  			reg = <0x01c0b000 0x1000>;
> 

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

On Wed, Mar 26, 2014 at 09:39:31AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 03/19/2014 08:21 PM, Carlo Caione wrote:
> > This patch adds DTS entries for NMI controller as child of GIC.
> > 
> > Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> 
> Note this breaks the kernel on sun6i / A31 since we don't have a
> pmic driver there yet, and thus the nmi gets constantly fired without
> anything clearing it.
> 
> So the sun6i section needs a status = "disabled"; until we actually have pmic
> support.

I guess it also applies to the A20, since the PMIC patches will
probably get merged later on?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Hans de Goede]

Hi,

On 03/26/2014 10:39 AM, Maxime Ripard wrote:
> On Wed, Mar 26, 2014 at 09:39:31AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03/19/2014 08:21 PM, Carlo Caione wrote:
>>> This patch adds DTS entries for NMI controller as child of GIC.
>>>
>>> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
>>
>> Note this breaks the kernel on sun6i / A31 since we don't have a
>> pmic driver there yet, and thus the nmi gets constantly fired without
>> anything clearing it.
>>
>> So the sun6i section needs a status = "disabled"; until we actually have pmic
>> support.
> 
> I guess it also applies to the A20, since the PMIC patches will
> probably get merged later on?

Could be I've never tried it on the A20 without also having the pmic driver
build into the kernel. Thinking more about this, I think this actually is
a bug in the nmi irqchip driver, it should not unmask the gic irq until
it gets an unmask for its child irq itself.

Otherwise we can still get the same problem if ie the pmic driver is
a module, etc.

Hmm, looking at the code I see that it already masks (sets enable to 0)
the irq in sunxi_sc_nmi_irq_init. Note that this really should be
done before the irq_set_chained_handler call though, as from then on
the gic irq is unmasked, so we may get spurious irqs until the
sunxi_sc_nmi_write calls are done.

I don't think this will solve the A31 problem though, I wonder if
the enable reg-offset we've for the A31 is correct, maybe it should
be 8 like with the A20 ?

I'll give this a try when I can find some time for this.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 03/26/2014 11:04 AM, Hans de Goede wrote:
> Hi,
> 
> On 03/26/2014 10:39 AM, Maxime Ripard wrote:
>> On Wed, Mar 26, 2014 at 09:39:31AM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 03/19/2014 08:21 PM, Carlo Caione wrote:
>>>> This patch adds DTS entries for NMI controller as child of GIC.
>>>>
>>>> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
>>>
>>> Note this breaks the kernel on sun6i / A31 since we don't have a
>>> pmic driver there yet, and thus the nmi gets constantly fired without
>>> anything clearing it.
>>>
>>> So the sun6i section needs a status = "disabled"; until we actually have pmic
>>> support.
>>
>> I guess it also applies to the A20, since the PMIC patches will
>> probably get merged later on?
> 
> Could be I've never tried it on the A20 without also having the pmic driver
> build into the kernel. Thinking more about this, I think this actually is
> a bug in the nmi irqchip driver, it should not unmask the gic irq until
> it gets an unmask for its child irq itself.
> 
> Otherwise we can still get the same problem if ie the pmic driver is
> a module, etc.
> 
> Hmm, looking at the code I see that it already masks (sets enable to 0)
> the irq in sunxi_sc_nmi_irq_init. Note that this really should be
> done before the irq_set_chained_handler call though, as from then on
> the gic irq is unmasked, so we may get spurious irqs until the
> sunxi_sc_nmi_write calls are done.
> 
> I don't think this will solve the A31 problem though, I wonder if
> the enable reg-offset we've for the A31 is correct, maybe it should
> be 8 like with the A20 ?
> 
> I'll give this a try when I can find some time for this.

Ok, so I've spend some time debugging this, and the problem is not the
disabling of the NMI in the NMI controller itself. The problem is that
the irq line used in the sun6i-a31.dtsi is wrong, irq 0 is for uart0
the nmi uses irq 32. So this fixes this hang due to unhandled irqs
I've been seeing:

--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -603,8 +603,7 @@
                        interrupt-controller;
                        #interrupt-cells = <2>;
                        reg = <0x01f00c0c 0x38>;
-                       interrupt-parent = <&gic>;
-                       interrupts = <0 0 4>;
+                       interrupts = <0 32 4>;
                };

                cpucfg@01f01c00 {

Note I also remove the interrupt-parent as the gic is the
default interrupt-parent, and we don't explicitly set that
anywhere else either.

Besides that after having looked into this more, I strongly
believe that we should disable the NMI before registering the
irq handler, as registering the irq handler unmasks the irq
on the gic, so if u-boot has left the NMI enabled, and the NMI pin
is active we will immediately get an interrupt, before any
driver has claimed the downstream interrupt of the NMI.

IOW I strongly believe we should do this:

--- a/drivers/irqchip/irq-sunxi-nmi.c
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -179,12 +179,12 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
        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);

+       irq_set_handler_data(irq, domain);
+       irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
+
        return 0;

 fail_irqd_remove:


So it looks like we need a v8, sorry for not spotting this sooner.

Regards,

Hans

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

[Carlo Caione]

On Wed, Mar 26, 2014 at 03:38:05PM +0100, Hans de Goede wrote:
> Hi,

Hi Hans,

> On 03/26/2014 11:04 AM, Hans de Goede wrote:
> > Hi,
> >
> > On 03/26/2014 10:39 AM, Maxime Ripard wrote:
> >> On Wed, Mar 26, 2014 at 09:39:31AM +0100, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 03/19/2014 08:21 PM, Carlo Caione wrote:
> >>>> This patch adds DTS entries for NMI controller as child of GIC.
> >>>>
> >>>> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
> >>>
> >>> Note this breaks the kernel on sun6i / A31 since we don't have a
> >>> pmic driver there yet, and thus the nmi gets constantly fired without
> >>> anything clearing it.
> >>>
> >>> So the sun6i section needs a status = "disabled"; until we actually have pmic
> >>> support.
> >>
> >> I guess it also applies to the A20, since the PMIC patches will
> >> probably get merged later on?
> >
> > Could be I've never tried it on the A20 without also having the pmic driver
> > build into the kernel. Thinking more about this, I think this actually is
> > a bug in the nmi irqchip driver, it should not unmask the gic irq until
> > it gets an unmask for its child irq itself.
> >
> > Otherwise we can still get the same problem if ie the pmic driver is
> > a module, etc.
> >
> > Hmm, looking at the code I see that it already masks (sets enable to 0)
> > the irq in sunxi_sc_nmi_irq_init. Note that this really should be
> > done before the irq_set_chained_handler call though, as from then on
> > the gic irq is unmasked, so we may get spurious irqs until the
> > sunxi_sc_nmi_write calls are done.
> >
> > I don't think this will solve the A31 problem though, I wonder if
> > the enable reg-offset we've for the A31 is correct, maybe it should
> > be 8 like with the A20 ?
> >
> > I'll give this a try when I can find some time for this.
>
> Ok, so I've spend some time debugging this, and the problem is not the
> disabling of the NMI in the NMI controller itself. The problem is that
> the irq line used in the sun6i-a31.dtsi is wrong, irq 0 is for uart0
> the nmi uses irq 32. So this fixes this hang due to unhandled irqs

Unfortunately I don't have a A31 board so I couldn't test it :( sorry for
this mess.
I have tried with my Cubieboard2 and it seems that I don't have any IRQs
storm when the PMIC is not compiled in.

> I've been seeing:
>
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -603,8 +603,7 @@
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>                         reg = <0x01f00c0c 0x38>;
> -                       interrupt-parent = <&gic>;
> -                       interrupts = <0 0 4>;
> +                       interrupts = <0 32 4>;
>                 };
>
>                 cpucfg@01f01c00 {

Agree. I'll fix it.

> Note I also remove the interrupt-parent as the gic is the
> default interrupt-parent, and we don't explicitly set that
> anywhere else either.

Uhm, actually I removed it in v6. Probably you are looking at the wrong
version.

> Besides that after having looked into this more, I strongly
> believe that we should disable the NMI before registering the
> irq handler, as registering the irq handler unmasks the irq
> on the gic, so if u-boot has left the NMI enabled, and the NMI pin
> is active we will immediately get an interrupt, before any
> driver has claimed the downstream interrupt of the NMI.
>
> IOW I strongly believe we should do this:
>
> --- a/drivers/irqchip/irq-sunxi-nmi.c
> +++ b/drivers/irqchip/irq-sunxi-nmi.c
> @@ -179,12 +179,12 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
>         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);
>
> +       irq_set_handler_data(irq, domain);
> +       irq_set_chained_handler(irq, sunxi_sc_nmi_handle_irq);
> +
>         return 0;
>
>  fail_irqd_remove:

ACK

>
> So it looks like we need a v8, sorry for not spotting this sooner.

My bad for not having tested it enough.

Thank you,

-- 
Carlo Caione

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

[Carlo Caione]

Added documentation for NMI irqchip.

Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
---
 .../allwinner,sun67i-sc-nmi.txt                    | 27 ++++++++++++++++++++++
 1 file changed, 27 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..d1c5cda
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/allwinner,sun67i-sc-nmi.txt
@@ -0,0 +1,27 @@
+Allwinner Sunxi NMI Controller
+==============================
+
+Required properties:
+
+- compatible : should be "allwinner,sun7i-a20-sc-nmi" or
+  "allwinner,sun6i-a31-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 as defined in interrupt.txt in this directory.
+- interrupt-parent: Specifies the parent interrupt controller.
+- interrupts: Specifies the interrupt line (NMI) which is handled by
+  the interrupt controller in the parent controller's notation. This value
+  shall be the NMI.
+
+Example:
+
+sc-nmi-intc@01c00030 {
+	compatible = "allwinner,sun7i-a20-sc-nmi";
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	reg = <0x01c00030 0x0c>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 0 4>;
+};
-- 
1.8.3.2

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

On Wed, Mar 19, 2014 at 08:21:19PM +0100, Carlo Caione wrote:
> Added documentation for NMI irqchip.
> 
> Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v7 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI controller

[Carlo Caione]

On Wed, Mar 19, 2014 at 08:21:16PM +0100, Carlo Caione 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.

Hi Thomas,
Is this ok with the Maxime ACKs?

-- 
Carlo Caione