[PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity

[PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity

[Yingjoe Chen]

This series is 7th version of interrupt polarity support for MediaTek SoCs.

I rebased previous version on latest tip/irq/irqdomain and use newly added
helper function irq_domain_add_hierarchy(). I also changed according to
Marc's suggestion.

Simplified block diagram for interrupt on my system:

    +-------+      +-------+
 ---| SYSIRQ|------|ARM GIC|
 ---|       |------|       |
 ---|       |------|       |
 ---|       |------|       |
 ---|       |------|       |
    +-------+      +-------+

In device tree, interrupt-parent for other devices is sysirq, child of gic.
This describe HW better and allow device to specify polarity as it is sent
by the device.

When using hierarchy irq domain, gic will use irq_domain_add_linear to
create irqdomain and all interrupt numbers must come from device tree. My
/proc/interrupts looks like this now:

# cat /proc/interrupts
           CPU0
 16:     149578  MT_SYSIRQ 113  mtk_timer
 20:       1082  MT_SYSIRQ  54  serial

Changes in v7:
 - Rebased to tip/irq/irqdomain
 - In mtk_sysirq_domain_alloc, use copy of irq_data to workaound of_node
   check in gic_irq_domain_xlate.

Changes in v6:
 - Discussed in [1]
 - Rebased to tip/irq/irqdomain

Changes in v5:
 - Discussed in [2]
 - Fix bug on mt6589 reported by Matthias
 - Fix bug for irq_find_mapping in irq_create_of_mapping
 - Merge Marc's change to proper handle non-DT case in gic_init_bases

Changes in v4:
 - Discussed in [3]
 - Remove arm,hierarchy-irq-domain. When GIC is probed by DT, it will
support hierarchy irqdomain.

Changes in v3:
 - Discussed in [4]
 - First implementation using hierarchy irqdomain

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302221.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/298161.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296911.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/293766.html

Yingjoe Chen (4):
  irqchip: gic: Support hierarchy irq domain.
  ARM: mediatek: Add sysirq interrupt polarity support
  ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi
  dt-bindings: add bindings for mediatek sysirq

 .../bindings/arm/mediatek/mediatek,sysirq.txt      |  26 ++++
 arch/arm/boot/dts/mt6589.dtsi                      |  14 +-
 arch/arm/boot/dts/mt8127.dtsi                      |  14 +-
 arch/arm/boot/dts/mt8135.dtsi                      |  14 +-
 drivers/irqchip/Kconfig                            |   1 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-gic.c                          |  78 ++++++----
 drivers/irqchip/irq-mtk-sysirq.c                   | 158 +++++++++++++++++++++
 8 files changed, 276 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
 create mode 100644 drivers/irqchip/irq-mtk-sysirq.c

[PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Yingjoe Chen]

Add support to use gic as a parent for stacked irq domain.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 drivers/irqchip/Kconfig   |  1 +
 drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
 2 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b21f12f..7f34138 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -5,6 +5,7 @@ config IRQCHIP
 config ARM_GIC
 	bool
 	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
 	select MULTI_IRQ_HANDLER
 
 config GIC_NON_BANKED
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 38493ff..464dd53 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 {
 	if (hw < 32) {
 		irq_set_percpu_devid(irq);
-		irq_set_chip_and_handler(irq, &gic_chip,
-					 handle_percpu_devid_irq);
+		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
+				    handle_percpu_devid_irq, NULL, NULL);
 		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
 	} else {
-		irq_set_chip_and_handler(irq, &gic_chip,
-					 handle_fasteoi_irq);
+		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
+				    handle_fasteoi_irq, NULL, NULL);
 		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
 
 		gic_routable_irq_domain_ops->map(d, irq, hw);
 	}
-	irq_set_chip_data(irq, d->host_data);
 	return 0;
 }
 
@@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
 };
 #endif
 
+static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				unsigned int nr_irqs, void *arg)
+{
+	int i, ret;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;
+	struct of_phandle_args *irq_data = arg;
+
+	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
+				   irq_data->args_count, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		gic_irq_domain_map(domain, virq+i, hwirq+i);
+
+	return 0;
+}
+
+static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
+	.xlate = gic_irq_domain_xlate,
+	.alloc = gic_irq_domain_alloc,
+	.free = irq_domain_free_irqs_top,
+};
+
 static const struct irq_domain_ops gic_irq_domain_ops = {
 	.map = gic_irq_domain_map,
 	.unmap = gic_irq_domain_unmap,
@@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_cpu_map[i] = 0xff;
 
 	/*
-	 * For primary GICs, skip over SGIs.
-	 * For secondary GICs, skip over PPIs, too.
-	 */
-	if (gic_nr == 0 && (irq_start & 31) > 0) {
-		hwirq_base = 16;
-		if (irq_start != -1)
-			irq_start = (irq_start & ~31) + 16;
-	} else {
-		hwirq_base = 32;
-	}
-
-	/*
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources.
 	 */
@@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_irqs = 1020;
 	gic->gic_irqs = gic_irqs;
 
-	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
+	if (node) {		/* DT case */
+		const struct irq_domain_ops *ops =
+					&gic_irq_domain_hierarchy_ops;
+
+		if (!of_property_read_u32(node, "arm,routable-irqs",
+					  &nr_routable_irqs)) {
+			ops = &gic_irq_domain_ops;
+			gic_irqs = nr_routable_irqs;
+		}
+
+		gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic);
+	} else {		/* Non-DT case */
+		/*
+		 * For primary GICs, skip over SGIs.
+		 * For secondary GICs, skip over PPIs, too.
+		 */
+		if (gic_nr == 0 && (irq_start & 31) > 0) {
+			hwirq_base = 16;
+			if (irq_start != -1)
+				irq_start = (irq_start & ~31) + 16;
+		} else {
+			hwirq_base = 32;
+		}
+
+		gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
 
-	if (of_property_read_u32(node, "arm,routable-irqs",
-				 &nr_routable_irqs)) {
 		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
 					   numa_node_id());
 		if (IS_ERR_VALUE(irq_base)) {
@@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 		gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
 					hwirq_base, &gic_irq_domain_ops, gic);
-	} else {
-		gic->domain = irq_domain_add_linear(node, nr_routable_irqs,
-						    &gic_irq_domain_ops,
-						    gic);
 	}
 
 	if (WARN_ON(!gic->domain))
-- 
1.8.1.1.dirty

[PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support

[Yingjoe Chen]

Mediatek SoCs have interrupt polarity support in sysirq which
allows to invert polarity for given interrupt. Add this support
using hierarchy irq domain.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-mtk-sysirq.c | 158 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+)
 create mode 100644 drivers/irqchip/irq-mtk-sysirq.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 173bb5f..4e0f254 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
 obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o \
 					   irq-bcm7120-l2.o
 obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
+obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
new file mode 100644
index 0000000..93cb945
--- /dev/null
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -0,0 +1,158 @@
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: Joe.C <yingjoe.chen@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "irqchip.h"
+
+#define MT6577_SYS_INTPOL_NUM	(224)
+
+struct mtk_sysirq_chip_data {
+	spinlock_t lock;
+	void __iomem *intpol_base;
+};
+
+static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
+{
+	irq_hw_number_t hwirq = data->hwirq;
+	struct mtk_sysirq_chip_data *chip_data = data->chip_data;
+	u32 offset, reg_index, value;
+	unsigned long flags;
+	int ret;
+
+	offset = hwirq & 0x1f;
+	reg_index = hwirq >> 5;
+
+	spin_lock_irqsave(&chip_data->lock, flags);
+	value = readl_relaxed(chip_data->intpol_base + reg_index * 4);
+	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
+		if (type == IRQ_TYPE_LEVEL_LOW)
+			type = IRQ_TYPE_LEVEL_HIGH;
+		else
+			type = IRQ_TYPE_EDGE_RISING;
+		value |= (1 << offset);
+	} else
+		value &= ~(1 << offset);
+	writel(value, chip_data->intpol_base + reg_index * 4);
+
+	data = data->parent_data;
+	ret = data->chip->irq_set_type(data, type);
+	spin_unlock_irqrestore(&chip_data->lock, flags);
+	return ret;
+}
+
+static struct irq_chip mtk_sysirq_chip = {
+	.name			= "MT_SYSIRQ",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= mtk_sysirq_set_type,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static int mtk_sysirq_domain_xlate(struct irq_domain *d,
+				   struct device_node *controller,
+				   const u32 *intspec, unsigned int intsize,
+				   unsigned long *out_hwirq,
+				   unsigned int *out_type)
+{
+	if (intsize < 3)
+		return -EINVAL;
+
+	/* sysirq doesn't support PPI */
+	if (intspec[0])
+		return -EINVAL;
+
+	*out_hwirq = intspec[1];
+	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+static int mtk_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	int i;
+	irq_hw_number_t hwirq;
+	struct of_phandle_args *irq_data = arg;
+	struct of_phandle_args gic_data = *irq_data;
+
+	if (irq_data->args_count < 3)
+		return -EINVAL;
+
+	hwirq = irq_data->args[1];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &mtk_sysirq_chip,
+					      domain->host_data);
+
+	gic_data.np = domain->parent->of_node;
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_data);
+}
+
+static struct irq_domain_ops sysirq_domain_ops = {
+	.xlate = mtk_sysirq_domain_xlate,
+	.alloc = mtk_sysirq_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+};
+
+static int __init mtk_sysirq_of_init(struct device_node *node,
+				     struct device_node *parent)
+{
+	struct irq_domain *domain, *domain_parent;
+	struct mtk_sysirq_chip_data *chip_data;
+	int ret = 0;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mtk_sysirq: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+	if (!chip_data)
+		return -ENOMEM;
+
+	chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
+	if (!chip_data->intpol_base) {
+		pr_err("mtk_sysirq: unable to map sysirq register\n");
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	domain = irq_domain_add_hierarchy(domain_parent, 0,
+					  MT6577_SYS_INTPOL_NUM, node,
+					  &sysirq_domain_ops, chip_data);
+	if (!domain) {
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
+	spin_lock_init(&chip_data->lock);
+
+	return 0;
+
+out_unmap:
+	iounmap(chip_data->intpol_base);
+out_free:
+	kfree(chip_data);
+	return ret;
+}
+IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);
-- 
1.8.1.1.dirty

[PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi

[Yingjoe Chen]

Add sysirq settings for mt6589/mt8135/mt8127
This also correct timer interrupt flag. The old setting works
because boot loader already set polarity for timer interrupt.
Without intpol support, the setting was not changed so gic
can get the irq correctly.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
 arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
 arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
index e3c7600..c91b2a9 100644
--- a/arch/arm/boot/dts/mt6589.dtsi
+++ b/arch/arm/boot/dts/mt6589.dtsi
@@ -19,7 +19,7 @@
 
 / {
 	compatible = "mediatek,mt6589";
-	interrupt-parent = <&gic>;
+	interrupt-parent = <&sysirq>;
 
 	cpus {
 		#address-cells = <1>;
@@ -76,15 +76,25 @@
 		timer: timer@10008000 {
 			compatible = "mediatek,mt6577-timer";
 			reg = <0x10008000 0x80>;
-			interrupts = <GIC_SPI 113 IRQ_TYPE_EDGE_RISING>;
+			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>;
 			clocks = <&system_clk>, <&rtc_clk>;
 			clock-names = "system-clk", "rtc-clk";
 		};
 
+		sysirq: interrupt-controller@10200100 {
+			compatible = "mediatek,mt6589-sysirq",
+				     "mediatek,mt6577-sysirq";
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			reg = <0x10200100 0x1c>;
+		};
+
 		gic: interrupt-controller@10211000 {
 			compatible = "arm,cortex-a7-gic";
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
 			reg = <0x10211000 0x1000>,
 			      <0x10212000 0x1000>,
 			      <0x10214000 0x2000>,
diff --git a/arch/arm/boot/dts/mt8127.dtsi b/arch/arm/boot/dts/mt8127.dtsi
index c3ee060..49f5976 100644
--- a/arch/arm/boot/dts/mt8127.dtsi
+++ b/arch/arm/boot/dts/mt8127.dtsi
@@ -18,7 +18,7 @@
 
 / {
 	compatible = "mediatek,mt8127";
-	interrupt-parent = <&gic>;
+	interrupt-parent = <&sysirq>;
 
 	cpus {
 		#address-cells = <1>;
@@ -82,15 +82,25 @@
 			compatible = "mediatek,mt8127-timer",
 					"mediatek,mt6577-timer";
 			reg = <0 0x10008000 0 0x80>;
-			interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_LOW>;
 			clocks = <&system_clk>, <&rtc_clk>;
 			clock-names = "system-clk", "rtc-clk";
 		};
 
+		sysirq: interrupt-controller@10200100 {
+			compatible = "mediatek,mt8127-sysirq",
+				     "mediatek,mt6577-sysirq";
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			reg = <0 0x10200100 0 0x1c>;
+		};
+
 		gic: interrupt-controller@10211000 {
 			compatible = "arm,cortex-a7-gic";
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
 			reg = <0 0x10211000 0 0x1000>,
 			      <0 0x10212000 0 0x1000>,
 			      <0 0x10214000 0 0x2000>,
diff --git a/arch/arm/boot/dts/mt8135.dtsi b/arch/arm/boot/dts/mt8135.dtsi
index 5faae6e..60338d9 100644
--- a/arch/arm/boot/dts/mt8135.dtsi
+++ b/arch/arm/boot/dts/mt8135.dtsi
@@ -18,7 +18,7 @@
 
 / {
 	compatible = "mediatek,mt8135";
-	interrupt-parent = <&gic>;
+	interrupt-parent = <&sysirq>;
 
 	cpu-map {
 		cluster0 {
@@ -105,15 +105,25 @@
 			compatible = "mediatek,mt8135-timer",
 					"mediatek,mt6577-timer";
 			reg = <0 0x10008000 0 0x80>;
-			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>;
 			clocks = <&system_clk>, <&rtc_clk>;
 			clock-names = "system-clk", "rtc-clk";
 		};
 
+		sysirq: interrupt-controller@10200030 {
+			compatible = "mediatek,mt8135-sysirq",
+				     "mediatek,mt6577-sysirq";
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
+			reg = <0 0x10200030 0 0x1c>;
+		};
+
 		gic: interrupt-controller@10211000 {
 			compatible = "arm,cortex-a15-gic";
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			interrupt-parent = <&gic>;
 			reg = <0 0x10211000 0 0x1000>,
 			      <0 0x10212000 0 0x1000>,
 			      <0 0x10214000 0 0x2000>,
-- 
1.8.1.1.dirty

[PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq

[Yingjoe Chen]

Add binding documentation for Mediatek SoC SYSIRQ.

Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 .../bindings/arm/mediatek/mediatek,sysirq.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
new file mode 100644
index 0000000..8669536
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
@@ -0,0 +1,26 @@
+Mediatek 65xx/81xx sysirq
+
+Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
+interrupt.
+
+Required properties:
+- compatible: should be one of:
+	"mediatek,mt8135-sysirq"
+	"mediatek,mt8127-sysirq"
+	"mediatek,mt6589-sysirq"
+	"mediatek,mt6582-sysirq"
+	"mediatek,mt6577-sysirq"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Must use the same cells/format as parent controller.
+- interrupt-parent: phandle of irq domain parent for sysirq.
+- reg: Physical base address of the intpol registers and length of memory
+  mapped region.
+
+Example:
+	sysirq: interrupt-controller@10200100 {
+		compatible = "mediatek,mt6589-sysirq", "mediatek,mt6577-sysirq";
+		interrupt-controller;
+		#interrupt-cells = <3>;
+		interrupt-parent = <&gic>;
+		reg = <0 0x10200100 0 0x1c>;
+	};
-- 
1.8.1.1.dirty

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

Hi Yingjoe,

On Wed, Nov 19 2014 at  2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Add support to use gic as a parent for stacked irq domain.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/Kconfig   |  1 +
>  drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
>  2 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index b21f12f..7f34138 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -5,6 +5,7 @@ config IRQCHIP
>  config ARM_GIC
>  	bool
>  	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
>  	select MULTI_IRQ_HANDLER
>  
>  config GIC_NON_BANKED
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 38493ff..464dd53 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  {
>  	if (hw < 32) {
>  		irq_set_percpu_devid(irq);
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_percpu_devid_irq);
> +		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
> +				    handle_percpu_devid_irq, NULL, NULL);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>  	} else {
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_fasteoi_irq);
> +		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
> +				    handle_fasteoi_irq, NULL, NULL);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>  
>  		gic_routable_irq_domain_ops->map(d, irq, hw);
>  	}
> -	irq_set_chip_data(irq, d->host_data);
>  	return 0;
>  }
>  
> @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
>  };
>  #endif
>  
> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct of_phandle_args *irq_data = arg;
> +
> +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
> +				   irq_data->args_count, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		gic_irq_domain_map(domain, virq+i, hwirq+i);

nit: spacing around '+'.

> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> +	.xlate = gic_irq_domain_xlate,
> +	.alloc = gic_irq_domain_alloc,
> +	.free = irq_domain_free_irqs_top,

I'm convinced that irq_domain_free_irqs_top is the wrong function to
call here, because you're calling it from the bottom, not the top-level
(it has no parent).

I cannot verify this with your code as I don't a working platform with
GICv2m, but if I enable something similar on GICv3, it dies a very
painful way:

Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = ffffffc03d059000
[00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
Internal error: Oops: 96000006 [#1] SMP
Modules linked in:
CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
PC is at irq_domain_free_irqs_recursive+0x1c/0x80
LR is at irq_domain_free_irqs_common+0x88/0x9c
pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
[...]
[<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
[<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
[<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
[<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
[<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
[<ffffffc0000ef518>] msi_domain_free+0x70/0x88
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
[<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
[<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
[<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
[...]

and I cannot see how this could work on the standard GIC either.

Thomas, Jiang: could you please confirm or infirm my suspicions? My
understanding is that irq_domain_free_irqs_top can only be called from
the top-level domain.

> +};
> +
>  static const struct irq_domain_ops gic_irq_domain_ops = {
>  	.map = gic_irq_domain_map,
>  	.unmap = gic_irq_domain_unmap,
> @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  		gic_cpu_map[i] = 0xff;
>  
>  	/*
> -	 * For primary GICs, skip over SGIs.
> -	 * For secondary GICs, skip over PPIs, too.
> -	 */
> -	if (gic_nr == 0 && (irq_start & 31) > 0) {
> -		hwirq_base = 16;
> -		if (irq_start != -1)
> -			irq_start = (irq_start & ~31) + 16;
> -	} else {
> -		hwirq_base = 32;
> -	}
> -
> -	/*
>  	 * Find out how many interrupts are supported.
>  	 * The GIC only supports up to 1020 interrupt sources.
>  	 */
> @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  		gic_irqs = 1020;
>  	gic->gic_irqs = gic_irqs;
>  
> -	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
> +	if (node) {		/* DT case */
> +		const struct irq_domain_ops *ops =
> +					&gic_irq_domain_hierarchy_ops;

nit: please put this on the same line, even if it is longer than 80
characters.

> +
> +		if (!of_property_read_u32(node, "arm,routable-irqs",
> +					  &nr_routable_irqs)) {
> +			ops = &gic_irq_domain_ops;
> +			gic_irqs = nr_routable_irqs;
> +		}
> +
> +		gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic);
> +	} else {		/* Non-DT case */
> +		/*
> +		 * For primary GICs, skip over SGIs.
> +		 * For secondary GICs, skip over PPIs, too.
> +		 */
> +		if (gic_nr == 0 && (irq_start & 31) > 0) {
> +			hwirq_base = 16;
> +			if (irq_start != -1)
> +				irq_start = (irq_start & ~31) + 16;
> +		} else {
> +			hwirq_base = 32;
> +		}
> +
> +		gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>  
> -	if (of_property_read_u32(node, "arm,routable-irqs",
> -				 &nr_routable_irqs)) {
>  		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>  					   numa_node_id());
>  		if (IS_ERR_VALUE(irq_base)) {
> @@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  
>  		gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
>  					hwirq_base, &gic_irq_domain_ops, gic);
> -	} else {
> -		gic->domain = irq_domain_add_linear(node, nr_routable_irqs,
> -						    &gic_irq_domain_ops,
> -						    gic);
>  	}
>  
>  	if (WARN_ON(!gic->domain))

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi

[Marc Zyngier]

On Wed, Nov 19 2014 at  2:14:10 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Add sysirq settings for mt6589/mt8135/mt8127
> This also correct timer interrupt flag. The old setting works
> because boot loader already set polarity for timer interrupt.
> Without intpol support, the setting was not changed so gic
> can get the irq correctly.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
>  arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
>  arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
>  3 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
> index e3c7600..c91b2a9 100644
> --- a/arch/arm/boot/dts/mt6589.dtsi
> +++ b/arch/arm/boot/dts/mt6589.dtsi
> @@ -19,7 +19,7 @@
>  
>  / {
>  	compatible = "mediatek,mt6589";
> -	interrupt-parent = <&gic>;
> +	interrupt-parent = <&sysirq>;

This worries me a bit. Your sysirq cannot handle PPIs, and yet you make
it the top-level interrupt controller, without amending any PPI.

Does it mean you do not use *any* PPI? No per-cpu timer, nothing?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support

[Mark Rutland]

On Wed, Nov 19, 2014 at 02:14:09PM +0000, Yingjoe Chen wrote:
> Mediatek SoCs have interrupt polarity support in sysirq which
> allows to invert polarity for given interrupt. Add this support
> using hierarchy irq domain.
> 
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-mtk-sysirq.c | 158 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 159 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mtk-sysirq.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 173bb5f..4e0f254 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o \
>  					   irq-bcm7120-l2.o
>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
> +obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
> diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
> new file mode 100644
> index 0000000..93cb945
> --- /dev/null
> +++ b/drivers/irqchip/irq-mtk-sysirq.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: Joe.C <yingjoe.chen@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "irqchip.h"
> +
> +#define MT6577_SYS_INTPOL_NUM	(224)
> +
> +struct mtk_sysirq_chip_data {
> +	spinlock_t lock;
> +	void __iomem *intpol_base;
> +};
> +
> +static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	irq_hw_number_t hwirq = data->hwirq;
> +	struct mtk_sysirq_chip_data *chip_data = data->chip_data;
> +	u32 offset, reg_index, value;
> +	unsigned long flags;
> +	int ret;
> +
> +	offset = hwirq & 0x1f;
> +	reg_index = hwirq >> 5;
> +
> +	spin_lock_irqsave(&chip_data->lock, flags);
> +	value = readl_relaxed(chip_data->intpol_base + reg_index * 4);
> +	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> +		if (type == IRQ_TYPE_LEVEL_LOW)
> +			type = IRQ_TYPE_LEVEL_HIGH;
> +		else
> +			type = IRQ_TYPE_EDGE_RISING;
> +		value |= (1 << offset);
> +	} else
> +		value &= ~(1 << offset);

Nit: if one branch of an if statements is bracketed, both branches
should be. Please bracket the else case.

> +	writel(value, chip_data->intpol_base + reg_index * 4);
> +
> +	data = data->parent_data;
> +	ret = data->chip->irq_set_type(data, type);
> +	spin_unlock_irqrestore(&chip_data->lock, flags);
> +	return ret;
> +}
> +
> +static struct irq_chip mtk_sysirq_chip = {
> +	.name			= "MT_SYSIRQ",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_type		= mtk_sysirq_set_type,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +};
> +
> +static int mtk_sysirq_domain_xlate(struct irq_domain *d,
> +				   struct device_node *controller,
> +				   const u32 *intspec, unsigned int intsize,
> +				   unsigned long *out_hwirq,
> +				   unsigned int *out_type)
> +{
> +	if (intsize < 3)
> +		return -EINVAL;
> +
> +	/* sysirq doesn't support PPI */
> +	if (intspec[0])
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[1];
> +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;

All of this is very strongly tied to the GIC, but the binding made it
sound like this was more generic (it didn't specify the number of cells
or their meaning). It doesn't look like we validate that this is
attached to a GIC.

Do we expect this to only ever be attached to a GICv2?

It would be nice to either make this generic or to describe the format
in the binding.

Also, surely you expect intsize == 3 if you expect a GICv2? Likewise in
mtk_sysirq_domain_alloc.

Thanks,
Mark.

> +	return 0;
> +}
> +
> +static int mtk_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *arg)
> +{
> +	int i;
> +	irq_hw_number_t hwirq;
> +	struct of_phandle_args *irq_data = arg;
> +	struct of_phandle_args gic_data = *irq_data;
> +
> +	if (irq_data->args_count < 3)
> +		return -EINVAL;
> +
> +	hwirq = irq_data->args[1];
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &mtk_sysirq_chip,
> +					      domain->host_data);
> +
> +	gic_data.np = domain->parent->of_node;
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_data);
> +}
> +
> +static struct irq_domain_ops sysirq_domain_ops = {
> +	.xlate = mtk_sysirq_domain_xlate,
> +	.alloc = mtk_sysirq_domain_alloc,
> +	.free = irq_domain_free_irqs_common,
> +};
> +
> +static int __init mtk_sysirq_of_init(struct device_node *node,
> +				     struct device_node *parent)
> +{
> +	struct irq_domain *domain, *domain_parent;
> +	struct mtk_sysirq_chip_data *chip_data;
> +	int ret = 0;
> +
> +	domain_parent = irq_find_host(parent);
> +	if (!domain_parent) {
> +		pr_err("mtk_sysirq: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
> +	if (!chip_data->intpol_base) {
> +		pr_err("mtk_sysirq: unable to map sysirq register\n");
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	domain = irq_domain_add_hierarchy(domain_parent, 0,
> +					  MT6577_SYS_INTPOL_NUM, node,
> +					  &sysirq_domain_ops, chip_data);
> +	if (!domain) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +	spin_lock_init(&chip_data->lock);
> +
> +	return 0;
> +
> +out_unmap:
> +	iounmap(chip_data->intpol_base);
> +out_free:
> +	kfree(chip_data);
> +	return ret;
> +}
> +IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);
> -- 
> 1.8.1.1.dirty
> 
> 

Re: [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq

[Mark Rutland]

On Wed, Nov 19, 2014 at 02:14:11PM +0000, Yingjoe Chen wrote:
> Add binding documentation for Mediatek SoC SYSIRQ.
> 
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>  .../bindings/arm/mediatek/mediatek,sysirq.txt      | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> new file mode 100644
> index 0000000..8669536
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> @@ -0,0 +1,26 @@
> +Mediatek 65xx/81xx sysirq
> +
> +Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
> +interrupt.
> +
> +Required properties:
> +- compatible: should be one of:
> +	"mediatek,mt8135-sysirq"
> +	"mediatek,mt8127-sysirq"
> +	"mediatek,mt6589-sysirq"
> +	"mediatek,mt6582-sysirq"
> +	"mediatek,mt6577-sysirq"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Must use the same cells/format as parent controller.
> +- interrupt-parent: phandle of irq domain parent for sysirq.

I'm concerned that this sounds very general while the binding assumes
the GICv2 interrupt-specifier format. Either the driver needs to become
more general, or this needs to be tightened up.

It's also odd to say "irq domain parent", as that's purely a Linux
construct and has nothing to do with the HW.

Thanks,
Mark.

> +- reg: Physical base address of the intpol registers and length of memory
> +  mapped region.
> +
> +Example:
> +	sysirq: interrupt-controller@10200100 {
> +		compatible = "mediatek,mt6589-sysirq", "mediatek,mt6577-sysirq";
> +		interrupt-controller;
> +		#interrupt-cells = <3>;
> +		interrupt-parent = <&gic>;
> +		reg = <0 0x10200100 0 0x1c>;
> +	};
> -- 
> 1.8.1.1.dirty
> 
> 

Re: [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi

[Yingjoe Chen]

On Wed, 2014-11-19 at 17:49 +0000, Marc Zyngier wrote:
> On Wed, Nov 19 2014 at  2:14:10 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> > Add sysirq settings for mt6589/mt8135/mt8127
> > This also correct timer interrupt flag. The old setting works
> > because boot loader already set polarity for timer interrupt.
> > Without intpol support, the setting was not changed so gic
> > can get the irq correctly.
> >
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> >  arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
> >  arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
> >  arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
> >  3 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
> > index e3c7600..c91b2a9 100644
> > --- a/arch/arm/boot/dts/mt6589.dtsi
> > +++ b/arch/arm/boot/dts/mt6589.dtsi
> > @@ -19,7 +19,7 @@
> >  
> >  / {
> >  	compatible = "mediatek,mt6589";
> > -	interrupt-parent = <&gic>;
> > +	interrupt-parent = <&sysirq>;
> 
> This worries me a bit. Your sysirq cannot handle PPIs, and yet you make
> it the top-level interrupt controller, without amending any PPI.
> 
> Does it mean you do not use *any* PPI? No per-cpu timer, nothing?

Matthias had a patch to enable arch timer[1], but that is not merged
yet. Node using PPI interrupts must add their own interrupt-parent. This
works if we don't have node that use both PPI & SPI interrupts. For
timer, we could do this:

+		timer2: timer {
+			compatible = "arm,armv7-timer";
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_LOW)>,
+				     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+				     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+				     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+			clock-frequency = <13000000>;
+		};

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/277017.html

Joe.C

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Yingjoe Chen]

Hi Marc,

On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
> > +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > +				unsigned int nr_irqs, void *arg)
> > +{
> > +	int i, ret;
> > +	irq_hw_number_t hwirq;
> > +	unsigned int type = IRQ_TYPE_NONE;
> > +	struct of_phandle_args *irq_data = arg;
> > +
> > +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
> > +				   irq_data->args_count, &hwirq, &type);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < nr_irqs; i++)
> > +		gic_irq_domain_map(domain, virq+i, hwirq+i);
> 
> nit: spacing around '+'.

OK.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> > +	.xlate = gic_irq_domain_xlate,
> > +	.alloc = gic_irq_domain_alloc,
> > +	.free = irq_domain_free_irqs_top,
> 
> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> call here, because you're calling it from the bottom, not the top-level
> (it has no parent).

Base on the name, I though this is helper function for top level
irq_domain?

> I cannot verify this with your code as I don't a working platform with
> GICv2m, but if I enable something similar on GICv3, it dies a very
> painful way:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = ffffffc03d059000
> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> LR is at irq_domain_free_irqs_common+0x88/0x9c
> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> [...]
> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> [...]
> 
> and I cannot see how this could work on the standard GIC either.

I'm sorry, I just realize my testcase was too simple, irqs are populated
by device tree and never got freed. I'll add that and test it again.

> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> understanding is that irq_domain_free_irqs_top can only be called from
> the top-level domain.
> 
> > +};
> > +
> >  static const struct irq_domain_ops gic_irq_domain_ops = {
> >  	.map = gic_irq_domain_map,
> >  	.unmap = gic_irq_domain_unmap,
> > @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  		gic_cpu_map[i] = 0xff;
> >  
> >  	/*
> > -	 * For primary GICs, skip over SGIs.
> > -	 * For secondary GICs, skip over PPIs, too.
> > -	 */
> > -	if (gic_nr == 0 && (irq_start & 31) > 0) {
> > -		hwirq_base = 16;
> > -		if (irq_start != -1)
> > -			irq_start = (irq_start & ~31) + 16;
> > -	} else {
> > -		hwirq_base = 32;
> > -	}
> > -
> > -	/*
> >  	 * Find out how many interrupts are supported.
> >  	 * The GIC only supports up to 1020 interrupt sources.
> >  	 */
> > @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  		gic_irqs = 1020;
> >  	gic->gic_irqs = gic_irqs;
> >  
> > -	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
> > +	if (node) {		/* DT case */
> > +		const struct irq_domain_ops *ops =
> > +					&gic_irq_domain_hierarchy_ops;
> 
> nit: please put this on the same line, even if it is longer than 80
> characters.

ok.

Joe.C

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Jiang Liu]

On 2014/11/20 1:18, Marc Zyngier wrote:
> Hi Yingjoe,
> 
> On Wed, Nov 19 2014 at  2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>> Add support to use gic as a parent for stacked irq domain.
>>
>> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>> ---
>>  drivers/irqchip/Kconfig   |  1 +
>>  drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
>>  2 files changed, 55 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index b21f12f..7f34138 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -5,6 +5,7 @@ config IRQCHIP
>>  config ARM_GIC
>>  	bool
>>  	select IRQ_DOMAIN
>> +	select IRQ_DOMAIN_HIERARCHY
>>  	select MULTI_IRQ_HANDLER
>>  
>>  config GIC_NON_BANKED
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 38493ff..464dd53 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>  {
>>  	if (hw < 32) {
>>  		irq_set_percpu_devid(irq);
>> -		irq_set_chip_and_handler(irq, &gic_chip,
>> -					 handle_percpu_devid_irq);
>> +		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>> +				    handle_percpu_devid_irq, NULL, NULL);
>>  		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>>  	} else {
>> -		irq_set_chip_and_handler(irq, &gic_chip,
>> -					 handle_fasteoi_irq);
>> +		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>> +				    handle_fasteoi_irq, NULL, NULL);
>>  		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>>  
>>  		gic_routable_irq_domain_ops->map(d, irq, hw);
>>  	}
>> -	irq_set_chip_data(irq, d->host_data);
>>  	return 0;
>>  }
>>  
>> @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
>>  };
>>  #endif
>>  
>> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				unsigned int nr_irqs, void *arg)
>> +{
>> +	int i, ret;
>> +	irq_hw_number_t hwirq;
>> +	unsigned int type = IRQ_TYPE_NONE;
>> +	struct of_phandle_args *irq_data = arg;
>> +
>> +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
>> +				   irq_data->args_count, &hwirq, &type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < nr_irqs; i++)
>> +		gic_irq_domain_map(domain, virq+i, hwirq+i);
> 
> nit: spacing around '+'.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>> +	.xlate = gic_irq_domain_xlate,
>> +	.alloc = gic_irq_domain_alloc,
>> +	.free = irq_domain_free_irqs_top,
> 
> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> call here, because you're calling it from the bottom, not the top-level
> (it has no parent).
> 
> I cannot verify this with your code as I don't a working platform with
> GICv2m, but if I enable something similar on GICv3, it dies a very
> painful way:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = ffffffc03d059000
> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> LR is at irq_domain_free_irqs_common+0x88/0x9c
> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> [...]
> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> [...]
> 
> and I cannot see how this could work on the standard GIC either.
> 
> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> understanding is that irq_domain_free_irqs_top can only be called from
> the top-level domain.
Hi Marc,
	It indicates that irq_domain_free_irqs_top() is not a good name.
We have:
1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
   handler_data;
3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
   parent domain's domain_ops.free() callback.
5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
   handler_data and call parent domain's domain_ops.free() callback.

So there two possible improvements here:
1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
   It's named as is because it's always called by the outer-most
   irqdomains on x86.
2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
   to call parent domain's domain_ops.free() callback only if parent
   exists. By this way, they could be used for inner-most irqdomains.
If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
Thoughts?
Regards!
Gerry
> 
>> +};
>> +
>>  static const struct irq_domain_ops gic_irq_domain_ops = {
>>  	.map = gic_irq_domain_map,
>>  	.unmap = gic_irq_domain_unmap,
>> @@ -948,18 +972,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>  		gic_cpu_map[i] = 0xff;
>>  
>>  	/*
>> -	 * For primary GICs, skip over SGIs.
>> -	 * For secondary GICs, skip over PPIs, too.
>> -	 */
>> -	if (gic_nr == 0 && (irq_start & 31) > 0) {
>> -		hwirq_base = 16;
>> -		if (irq_start != -1)
>> -			irq_start = (irq_start & ~31) + 16;
>> -	} else {
>> -		hwirq_base = 32;
>> -	}
>> -
>> -	/*
>>  	 * Find out how many interrupts are supported.
>>  	 * The GIC only supports up to 1020 interrupt sources.
>>  	 */
>> @@ -969,10 +981,32 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>  		gic_irqs = 1020;
>>  	gic->gic_irqs = gic_irqs;
>>  
>> -	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>> +	if (node) {		/* DT case */
>> +		const struct irq_domain_ops *ops =
>> +					&gic_irq_domain_hierarchy_ops;
> 
> nit: please put this on the same line, even if it is longer than 80
> characters.
> 
>> +
>> +		if (!of_property_read_u32(node, "arm,routable-irqs",
>> +					  &nr_routable_irqs)) {
>> +			ops = &gic_irq_domain_ops;
>> +			gic_irqs = nr_routable_irqs;
>> +		}
>> +
>> +		gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic);
>> +	} else {		/* Non-DT case */
>> +		/*
>> +		 * For primary GICs, skip over SGIs.
>> +		 * For secondary GICs, skip over PPIs, too.
>> +		 */
>> +		if (gic_nr == 0 && (irq_start & 31) > 0) {
>> +			hwirq_base = 16;
>> +			if (irq_start != -1)
>> +				irq_start = (irq_start & ~31) + 16;
>> +		} else {
>> +			hwirq_base = 32;
>> +		}
>> +
>> +		gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>>  
>> -	if (of_property_read_u32(node, "arm,routable-irqs",
>> -				 &nr_routable_irqs)) {
>>  		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>>  					   numa_node_id());
>>  		if (IS_ERR_VALUE(irq_base)) {
>> @@ -983,10 +1017,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>  
>>  		gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
>>  					hwirq_base, &gic_irq_domain_ops, gic);
>> -	} else {
>> -		gic->domain = irq_domain_add_linear(node, nr_routable_irqs,
>> -						    &gic_irq_domain_ops,
>> -						    gic);
>>  	}
>>  
>>  	if (WARN_ON(!gic->domain))
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Yingjoe Chen]

Hi Marc,

On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote:
> On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> > > +	.xlate = gic_irq_domain_xlate,
> > > +	.alloc = gic_irq_domain_alloc,
> > > +	.free = irq_domain_free_irqs_top,
> > 
> > I'm convinced that irq_domain_free_irqs_top is the wrong function to
> > call here, because you're calling it from the bottom, not the top-level
> > (it has no parent).
> 
> Base on the name, I though this is helper function for top level
> irq_domain?
> 
> > I cannot verify this with your code as I don't a working platform with
> > GICv2m, but if I enable something similar on GICv3, it dies a very
> > painful way:
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
> > pgd = ffffffc03d059000
> > [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> > Internal error: Oops: 96000006 [#1] SMP
> > Modules linked in:
> > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> > PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> > LR is at irq_domain_free_irqs_common+0x88/0x9c
> > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> > [...]
> > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> > [...]
> > 
> > and I cannot see how this could work on the standard GIC either.
> 
> I'm sorry, I just realize my testcase was too simple, irqs are populated
> by device tree and never got freed. I'll add that and test it again.

On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops
is only used when we use DT, so we probably will never use the free
function. Is it OK to remove the free support here?

Joe.C

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On Thu, Nov 20 2014 at  4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:

Hi Jiang,

> On 2014/11/20 1:18, Marc Zyngier wrote:
>> Hi Yingjoe,
>> 
>> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
>> <yingjoe.chen@mediatek.com> wrote:
>>> +
>>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>>> +	.xlate = gic_irq_domain_xlate,
>>> +	.alloc = gic_irq_domain_alloc,
>>> +	.free = irq_domain_free_irqs_top,
>> 
>> I'm convinced that irq_domain_free_irqs_top is the wrong function to
>> call here, because you're calling it from the bottom, not the top-level
>> (it has no parent).
>> 
>> I cannot verify this with your code as I don't a working platform with
>> GICv2m, but if I enable something similar on GICv3, it dies a very
>> painful way:
>> 
>> Unable to handle kernel NULL pointer dereference at virtual address 00000018
>> pgd = ffffffc03d059000
>> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
>> Internal error: Oops: 96000006 [#1] SMP
>> Modules linked in:
>> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>> LR is at irq_domain_free_irqs_common+0x88/0x9c
>> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>> [...]
>> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>> [...]
>> 
>> and I cannot see how this could work on the standard GIC either.
>> 
>> Thomas, Jiang: could you please confirm or infirm my suspicions? My
>> understanding is that irq_domain_free_irqs_top can only be called from
>> the top-level domain.
> Hi Marc,
> 	It indicates that irq_domain_free_irqs_top() is not a good name.
> We have:
> 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
> 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
>    handler_data;
> 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
> 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
>    parent domain's domain_ops.free() callback.
> 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
>    handler_data and call parent domain's domain_ops.free() callback.

Yes, and this "call parent domain's free callback" is where the problem
lies. Here, it is called from the innermost domain, with no parent.

> So there two possible improvements here:
> 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
>    It's named as is because it's always called by the outer-most
>    irqdomains on x86.
> 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
>    to call parent domain's domain_ops.free() callback only if parent
>    exists. By this way, they could be used for inner-most irqdomains.
> If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
> Thoughts?

Checking the parent is probably a safe solution (this is not a hot path
anyway). I don't care much about the name though, and I the only thing I
can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
not even funny. I'll let the matter rest in your capable hands! ;-)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

Hi Joe,

On Thu, Nov 20 2014 at  9:41:51 am GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Hi Marc,
>
> On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote:
>> On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
>> > > +
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>> > > +	.xlate = gic_irq_domain_xlate,
>> > > +	.alloc = gic_irq_domain_alloc,
>> > > +	.free = irq_domain_free_irqs_top,
>> > 
>> > I'm convinced that irq_domain_free_irqs_top is the wrong function to
>> > call here, because you're calling it from the bottom, not the top-level
>> > (it has no parent).
>> 
>> Base on the name, I though this is helper function for top level
>> irq_domain?
>> 
>> > I cannot verify this with your code as I don't a working platform with
>> > GICv2m, but if I enable something similar on GICv3, it dies a very
>> > painful way:
>> > 
>> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
>> > pgd = ffffffc03d059000
>> > [00000018] *pgd=0000000081356003, *pud=0000000081356003,
>> > *pmd=0000000000000000
>> > Internal error: Oops: 96000006 [#1] SMP
>> > Modules linked in:
>> > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>> > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>> > PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>> > LR is at irq_domain_free_irqs_common+0x88/0x9c
>> > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>> > [...]
>> > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <--
>> > gic_domain.free()
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>> > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>> > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>> > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>> > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>> > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>> > [...]
>> > 
>> > and I cannot see how this could work on the standard GIC either.
>> 
>> I'm sorry, I just realize my testcase was too simple, irqs are populated
>> by device tree and never got freed. I'll add that and test it again.
>
> On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops
> is only used when we use DT, so we probably will never use the free
> function. Is it OK to remove the free support here?

Well, such thing is coming with GICv2m (SPIs are allocated out of
DT). You can drop it if you want, but I will then have to add it back
(which seems like a waste of time).

I'd prefer if you kept it in with the rest of the conversion.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support

[Yingjoe Chen]

Hi Mark,


On Wed, 2014-11-19 at 18:04 +0000, Mark Rutland wrote:
> On Wed, Nov 19, 2014 at 02:14:09PM +0000, Yingjoe Chen wrote:
> > +	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> > +		if (type == IRQ_TYPE_LEVEL_LOW)
> > +			type = IRQ_TYPE_LEVEL_HIGH;
> > +		else
> > +			type = IRQ_TYPE_EDGE_RISING;
> > +		value |= (1 << offset);
> > +	} else
> > +		value &= ~(1 << offset);
> 
> Nit: if one branch of an if statements is bracketed, both branches
> should be. Please bracket the else case.

OK, will change in next version.

> > +	writel(value, chip_data->intpol_base + reg_index * 4);
> > +
> > +	data = data->parent_data;
> > +	ret = data->chip->irq_set_type(data, type);
> > +	spin_unlock_irqrestore(&chip_data->lock, flags);
> > +	return ret;
> > +}
> > +
> > +static struct irq_chip mtk_sysirq_chip = {
> > +	.name			= "MT_SYSIRQ",
> > +	.irq_mask		= irq_chip_mask_parent,
> > +	.irq_unmask		= irq_chip_unmask_parent,
> > +	.irq_eoi		= irq_chip_eoi_parent,
> > +	.irq_set_type		= mtk_sysirq_set_type,
> > +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> > +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> > +};
> > +
> > +static int mtk_sysirq_domain_xlate(struct irq_domain *d,
> > +				   struct device_node *controller,
> > +				   const u32 *intspec, unsigned int intsize,
> > +				   unsigned long *out_hwirq,
> > +				   unsigned int *out_type)
> > +{
> > +	if (intsize < 3)
> > +		return -EINVAL;
> > +
> > +	/* sysirq doesn't support PPI */
> > +	if (intspec[0])
> > +		return -EINVAL;
> > +
> > +	*out_hwirq = intspec[1];
> > +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> 
> All of this is very strongly tied to the GIC, but the binding made it
> sound like this was more generic (it didn't specify the number of cells
> or their meaning). It doesn't look like we validate that this is
> attached to a GIC.
> 
> Do we expect this to only ever be attached to a GICv2?
> 
> It would be nice to either make this generic or to describe the format
> in the binding.

This expect the parent to use the same interrupt cells format as GIC.
I'll fix the binding to make this more clear.

> Also, surely you expect intsize == 3 if you expect a GICv2? Likewise in
> mtk_sysirq_domain_alloc.

OK, will change that in next version.

Joe.C

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Yingjoe Chen]

Hi,

On Thu, 2014-11-20 at 10:07 +0000, Marc Zyngier wrote:
> On Thu, Nov 20 2014 at  4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> 
> Hi Jiang,
> 
> > On 2014/11/20 1:18, Marc Zyngier wrote:
> >> Hi Yingjoe,
> >> 
> >> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
> >> <yingjoe.chen@mediatek.com> wrote:
> >>> +
> >>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> >>> +	.xlate = gic_irq_domain_xlate,
> >>> +	.alloc = gic_irq_domain_alloc,
> >>> +	.free = irq_domain_free_irqs_top,
> >> 
> >> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> >> call here, because you're calling it from the bottom, not the top-level
> >> (it has no parent).
> >> 
> >> I cannot verify this with your code as I don't a working platform with
> >> GICv2m, but if I enable something similar on GICv3, it dies a very
> >> painful way:
> >> 
> >> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> >> pgd = ffffffc03d059000
> >> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> >> Internal error: Oops: 96000006 [#1] SMP
> >> Modules linked in:
> >> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> >> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> >> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> >> LR is at irq_domain_free_irqs_common+0x88/0x9c
> >> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> >> [...]
> >> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> >> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> >> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> >> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> >> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> >> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> >> [...]
> >> 
> >> and I cannot see how this could work on the standard GIC either.
> >> 
> >> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> >> understanding is that irq_domain_free_irqs_top can only be called from
> >> the top-level domain.
> > Hi Marc,
> > 	It indicates that irq_domain_free_irqs_top() is not a good name.
> > We have:
> > 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
> > 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
> >    handler_data;
> > 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
> > 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
> >    parent domain's domain_ops.free() callback.
> > 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
> >    handler_data and call parent domain's domain_ops.free() callback.
> 
> Yes, and this "call parent domain's free callback" is where the problem
> lies. Here, it is called from the innermost domain, with no parent.
> 
> > So there two possible improvements here:
> > 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
> >    It's named as is because it's always called by the outer-most
> >    irqdomains on x86.
> > 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
> >    to call parent domain's domain_ops.free() callback only if parent
> >    exists. By this way, they could be used for inner-most irqdomains.
> > If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
> > Thoughts?
> 
> Checking the parent is probably a safe solution (this is not a hot path
> anyway). I don't care much about the name though, and I the only thing I
> can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
> not even funny. I'll let the matter rest in your capable hands! ;-)

I've applied Jiang's "irqdomain: Enhance irq_domain_free_irqs_common()
to support parentless irqdomain" patch and it did fix the crash.

Thanks Jiang, Marc

Joe.C

Re: [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq

[Yingjoe Chen]

Hi Mark,

On Wed, 2014-11-19 at 18:07 +0000, Mark Rutland wrote:
> On Wed, Nov 19, 2014 at 02:14:11PM +0000, Yingjoe Chen wrote:
> > Add binding documentation for Mediatek SoC SYSIRQ.
> > 
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> >  .../bindings/arm/mediatek/mediatek,sysirq.txt      | 26 ++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> > new file mode 100644
> > index 0000000..8669536
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> > @@ -0,0 +1,26 @@
> > +Mediatek 65xx/81xx sysirq
> > +
> > +Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
> > +interrupt.
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > +	"mediatek,mt8135-sysirq"
> > +	"mediatek,mt8127-sysirq"
> > +	"mediatek,mt6589-sysirq"
> > +	"mediatek,mt6582-sysirq"
> > +	"mediatek,mt6577-sysirq"
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- #interrupt-cells : Must use the same cells/format as parent controller.
> > +- interrupt-parent: phandle of irq domain parent for sysirq.
> 
> I'm concerned that this sounds very general while the binding assumes
> the GICv2 interrupt-specifier format. Either the driver needs to become
> more general, or this needs to be tightened up.
> 
> It's also odd to say "irq domain parent", as that's purely a Linux
> construct and has nothing to do with the HW.

The implementation expect the parent to use the same interrupt-cells
format as GIC. Based on the block diagram in the cover-letter, we could
say GIC is the irq parent of sysirq. So I'm planning to change to this,
hope this is OK.

- #interrupt-cells : Use the same format as specified by GIC in
  Documentation/devicetree/bindings/arm/gic.txt
- interrupt-parent: phandle of irq parent for sysirq. The parent must
  use the same interrupt-cells format as GIC.

Joe.C

Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On 21/11/14 15:51, Yingjoe Chen wrote:
> 
> Hi,
> 
> On Thu, 2014-11-20 at 10:07 +0000, Marc Zyngier wrote:
>> On Thu, Nov 20 2014 at  4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>>
>> Hi Jiang,
>>
>>> On 2014/11/20 1:18, Marc Zyngier wrote:
>>>> Hi Yingjoe,
>>>>
>>>> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
>>>> <yingjoe.chen@mediatek.com> wrote:
>>>>> +
>>>>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>>>>> +	.xlate = gic_irq_domain_xlate,
>>>>> +	.alloc = gic_irq_domain_alloc,
>>>>> +	.free = irq_domain_free_irqs_top,
>>>>
>>>> I'm convinced that irq_domain_free_irqs_top is the wrong function to
>>>> call here, because you're calling it from the bottom, not the top-level
>>>> (it has no parent).
>>>>
>>>> I cannot verify this with your code as I don't a working platform with
>>>> GICv2m, but if I enable something similar on GICv3, it dies a very
>>>> painful way:
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address 00000018
>>>> pgd = ffffffc03d059000
>>>> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
>>>> Internal error: Oops: 96000006 [#1] SMP
>>>> Modules linked in:
>>>> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>>>> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>>>> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>>>> LR is at irq_domain_free_irqs_common+0x88/0x9c
>>>> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>>>> [...]
>>>> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>>>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>>>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c  <-- gic_domain.free()
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>>>> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>>>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>>>> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>>>> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>>>> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>>>> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>>>> [...]
>>>>
>>>> and I cannot see how this could work on the standard GIC either.
>>>>
>>>> Thomas, Jiang: could you please confirm or infirm my suspicions? My
>>>> understanding is that irq_domain_free_irqs_top can only be called from
>>>> the top-level domain.
>>> Hi Marc,
>>> 	It indicates that irq_domain_free_irqs_top() is not a good name.
>>> We have:
>>> 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
>>> 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
>>>    handler_data;
>>> 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
>>> 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
>>>    parent domain's domain_ops.free() callback.
>>> 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
>>>    handler_data and call parent domain's domain_ops.free() callback.
>>
>> Yes, and this "call parent domain's free callback" is where the problem
>> lies. Here, it is called from the innermost domain, with no parent.
>>
>>> So there two possible improvements here:
>>> 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
>>>    It's named as is because it's always called by the outer-most
>>>    irqdomains on x86.
>>> 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
>>>    to call parent domain's domain_ops.free() callback only if parent
>>>    exists. By this way, they could be used for inner-most irqdomains.
>>> If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
>>> Thoughts?
>>
>> Checking the parent is probably a safe solution (this is not a hot path
>> anyway). I don't care much about the name though, and I the only thing I
>> can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
>> not even funny. I'll let the matter rest in your capable hands! ;-)
> 
> I've applied Jiang's "irqdomain: Enhance irq_domain_free_irqs_common()
> to support parentless irqdomain" patch and it did fix the crash.

Excellent.  I think this is pretty much getting there now. Any chance
you could repost this series with the various fixes?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...