[RESEND PATCH v2 0/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[RESEND PATCH v2 0/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

[Sorry, resend to include all maintainers.]

This series add support for mediatek GIC interrupt polarity extension.
Several components in mediatek SoC have low level triggered interrupt and
require this support.

This also correct 6589 timer irq polarity. Previous version works because
6589 boot loader already set correct polarity for timer interrupt.

The patch set is based on Matthias's Mediatek basic support for v3.17 [1].

v2:
- Make mt6589.dtsi changes as a separate commit as Matthias suggest.

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

Joe.C (4):
  irqchip: gic: Change irq type check when extension is
  arm: mediatek: Add support for GIC interrupt polarity
  arm: mediatek: Add intpol in mt6589.dtsi
  dt-bindings: add bindings for mediatek intpol

 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt |   16 ++
 arch/arm/boot/dts/mt6589.dtsi                                              |    7 -
 arch/arm/mach-mediatek/Makefile                                            |    2
 arch/arm/mach-mediatek/common.h                                            |   19 +++
 arch/arm/mach-mediatek/intpol.c                                            |   61 ++++++++++
 arch/arm/mach-mediatek/mediatek.c                                          |   10 +
 drivers/irqchip/irq-gic.c                                                  |   27 ++--
 7 files changed, 131 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
 create mode 100644 arch/arm/mach-mediatek/common.h
 create mode 100644 arch/arm/mach-mediatek/intpol.c

[RESEND PATCH v2 0/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

[Sorry, resend to include all maintainers.]

This series add support for mediatek GIC interrupt polarity extension.
Several components in mediatek SoC have low level triggered interrupt and
require this support.

This also correct 6589 timer irq polarity. Previous version works because
6589 boot loader already set correct polarity for timer interrupt.

The patch set is based on Matthias's Mediatek basic support for v3.17 [1].

v2:
- Make mt6589.dtsi changes as a separate commit as Matthias suggest.

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

Joe.C (4):
  irqchip: gic: Change irq type check when extension is
  arm: mediatek: Add support for GIC interrupt polarity
  arm: mediatek: Add intpol in mt6589.dtsi
  dt-bindings: add bindings for mediatek intpol

 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt |   16 ++
 arch/arm/boot/dts/mt6589.dtsi                                              |    7 -
 arch/arm/mach-mediatek/Makefile                                            |    2
 arch/arm/mach-mediatek/common.h                                            |   19 +++
 arch/arm/mach-mediatek/intpol.c                                            |   61 ++++++++++
 arch/arm/mach-mediatek/mediatek.c                                          |   10 +
 drivers/irqchip/irq-gic.c                                                  |   27 ++--
 7 files changed, 131 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
 create mode 100644 arch/arm/mach-mediatek/common.h
 create mode 100644 arch/arm/mach-mediatek/intpol.c

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

GIC supports the combination with external extensions. But this
is not reflected in the checks of the interrupt type flag.
This patch allows interrupt types other than the one supported by GIC,
if an architecture extension is present and supports them.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 57d165e..66485ab 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	u32 confoff = (gicirq / 16) * 4;
 	bool enabled = false;
 	u32 val;
+	int ret = 0;
 
 	/* Interrupt configuration for SGIs can't be changed */
 	if (gicirq < 16)
 		return -EINVAL;
 
-	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
-		return -EINVAL;
-
 	raw_spin_lock(&irq_controller_lock);
 
-	if (gic_arch_extn.irq_set_type)
-		gic_arch_extn.irq_set_type(d, type);
+	if (gic_arch_extn.irq_set_type) {
+		ret = gic_arch_extn.irq_set_type(d, type);
+		if (ret)
+			goto out;
+	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
+		type != IRQ_TYPE_EDGE_RISING) {
+			ret = -EINVAL;
+			goto out;
+	}
 
 	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
-	if (type == IRQ_TYPE_LEVEL_HIGH)
+	/* Check for both edge and level here, so we can support GIC irq
+	   polarity extension in gic_arch_extn.irq_set_type. If arch
+	   doesn't support polarity extension, the check above will reject
+	   improper type. */
+	if (type & IRQ_TYPE_LEVEL_MASK)
 		val &= ~confmask;
-	else if (type == IRQ_TYPE_EDGE_RISING)
+	else if (type & IRQ_TYPE_EDGE_BOTH)
 		val |= confmask;
 
 	/*
@@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 
 	if (enabled)
 		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
-
+out:
 	raw_spin_unlock(&irq_controller_lock);
 
-	return 0;
+	return ret;
 }
 
 static int gic_retrigger(struct irq_data *d)
-- 
1.8.1.1.dirty

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

GIC supports the combination with external extensions. But this
is not reflected in the checks of the interrupt type flag.
This patch allows interrupt types other than the one supported by GIC,
if an architecture extension is present and supports them.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 57d165e..66485ab 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	u32 confoff = (gicirq / 16) * 4;
 	bool enabled = false;
 	u32 val;
+	int ret = 0;
 
 	/* Interrupt configuration for SGIs can't be changed */
 	if (gicirq < 16)
 		return -EINVAL;
 
-	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
-		return -EINVAL;
-
 	raw_spin_lock(&irq_controller_lock);
 
-	if (gic_arch_extn.irq_set_type)
-		gic_arch_extn.irq_set_type(d, type);
+	if (gic_arch_extn.irq_set_type) {
+		ret = gic_arch_extn.irq_set_type(d, type);
+		if (ret)
+			goto out;
+	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
+		type != IRQ_TYPE_EDGE_RISING) {
+			ret = -EINVAL;
+			goto out;
+	}
 
 	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
-	if (type == IRQ_TYPE_LEVEL_HIGH)
+	/* Check for both edge and level here, so we can support GIC irq
+	   polarity extension in gic_arch_extn.irq_set_type. If arch
+	   doesn't support polarity extension, the check above will reject
+	   improper type. */
+	if (type & IRQ_TYPE_LEVEL_MASK)
 		val &= ~confmask;
-	else if (type == IRQ_TYPE_EDGE_RISING)
+	else if (type & IRQ_TYPE_EDGE_BOTH)
 		val |= confmask;
 
 	/*
@@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 
 	if (enabled)
 		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
-
+out:
 	raw_spin_unlock(&irq_controller_lock);
 
-	return 0;
+	return ret;
 }
 
 static int gic_retrigger(struct irq_data *d)
-- 
1.8.1.1.dirty

[RESEND PATCH v2 2/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

Mediatek SoCs have an interrupt polarity extension which allows
to swap the polarity for given interrupts.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 arch/arm/mach-mediatek/Makefile   |  2 +-
 arch/arm/mach-mediatek/common.h   | 19 ++++++++++++
 arch/arm/mach-mediatek/intpol.c   | 61 +++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mediatek/mediatek.c | 10 +++++++
 4 files changed, 91 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mediatek/common.h
 create mode 100644 arch/arm/mach-mediatek/intpol.c

diff --git a/arch/arm/mach-mediatek/Makefile b/arch/arm/mach-mediatek/Makefile
index 43e619f..82c39d8 100644
--- a/arch/arm/mach-mediatek/Makefile
+++ b/arch/arm/mach-mediatek/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_ARCH_MEDIATEK) += mediatek.o
+obj-$(CONFIG_ARCH_MEDIATEK) += mediatek.o intpol.o
diff --git a/arch/arm/mach-mediatek/common.h b/arch/arm/mach-mediatek/common.h
new file mode 100644
index 0000000..8f2bbeb
--- /dev/null
+++ b/arch/arm/mach-mediatek/common.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2014 Mediatek Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+#ifndef __MEDIATEK_COMMON_H__
+#define __MEDIATEK_COMMON_H__
+
+extern char init_intpol(void);
+
+#endif /* __MEDIATEK_COMMON_H__ */
diff --git a/arch/arm/mach-mediatek/intpol.c b/arch/arm/mach-mediatek/intpol.c
new file mode 100644
index 0000000..65ccc7c
--- /dev/null
+++ b/arch/arm/mach-mediatek/intpol.c
@@ -0,0 +1,61 @@
+/*
+ * This file contains common code that is intended to be used across
+ * boards so that it's not replicated.
+ *
+ * Copyright (C) 2014 Mediatek Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/of_address.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/arm-gic.h>
+
+#define GIC_HW_IRQ_BASE  32
+#define INT_POL_INDEX(a)   ((a) - GIC_HW_IRQ_BASE)
+
+static void __iomem *int_pol_base;
+
+static int mtk_int_pol_set_type(struct irq_data *d, unsigned int type)
+{
+	unsigned int irq = d->hwirq;
+	u32 offset, reg_index, value;
+
+	offset = INT_POL_INDEX(irq) & 0x1F;
+	reg_index = INT_POL_INDEX(irq) >> 5;
+
+	/* This arch extension was called with irq_controller_lock held,
+	   so the read-modify-write will be atomic */
+	value = readl(int_pol_base + reg_index * 4);
+	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING)
+		value |= (1 << offset);
+	else
+		value &= ~(1 << offset);
+	writel(value, int_pol_base + reg_index * 4);
+
+	return 0;
+}
+
+void init_intpol(void)
+{
+	struct device_node *node;
+
+	node = of_find_compatible_node(NULL, NULL, "mediatek,mt6577-intpol");
+	if (!node)
+		return;
+
+	int_pol_base = of_io_request_and_map(node, 0, "intpol");
+	if (IS_ERR(int_pol_base)) {
+		pr_warn("Can't get resource\n");
+		return;
+	}
+
+	gic_arch_extn.irq_set_type = mtk_int_pol_set_type;
+}
diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
index 48051a2..aa10c70 100644
--- a/arch/arm/mach-mediatek/mediatek.c
+++ b/arch/arm/mach-mediatek/mediatek.c
@@ -16,6 +16,15 @@
  */
 #include <linux/init.h>
 #include <asm/mach/arch.h>
+#include <linux/irqchip.h>
+
+#include "common.h"
+
+static void __init mediatek_init_irq(void)
+{
+	init_intpol();
+	irqchip_init();
+}
 
 static const char * const mediatek_board_dt_compat[] = {
 	"mediatek,mt6589",
@@ -26,4 +35,5 @@ static const char * const mediatek_board_dt_compat[] = {
 
 DT_MACHINE_START(MEDIATEK_DT, "Mediatek Cortex-A7 (Device Tree)")
 	.dt_compat	= mediatek_board_dt_compat,
+	.init_irq       = mediatek_init_irq,
 MACHINE_END
-- 
1.8.1.1.dirty

[RESEND PATCH v2 2/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

Mediatek SoCs have an interrupt polarity extension which allows
to swap the polarity for given interrupts.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 arch/arm/mach-mediatek/Makefile   |  2 +-
 arch/arm/mach-mediatek/common.h   | 19 ++++++++++++
 arch/arm/mach-mediatek/intpol.c   | 61 +++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mediatek/mediatek.c | 10 +++++++
 4 files changed, 91 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mediatek/common.h
 create mode 100644 arch/arm/mach-mediatek/intpol.c

diff --git a/arch/arm/mach-mediatek/Makefile b/arch/arm/mach-mediatek/Makefile
index 43e619f..82c39d8 100644
--- a/arch/arm/mach-mediatek/Makefile
+++ b/arch/arm/mach-mediatek/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_ARCH_MEDIATEK) += mediatek.o
+obj-$(CONFIG_ARCH_MEDIATEK) += mediatek.o intpol.o
diff --git a/arch/arm/mach-mediatek/common.h b/arch/arm/mach-mediatek/common.h
new file mode 100644
index 0000000..8f2bbeb
--- /dev/null
+++ b/arch/arm/mach-mediatek/common.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2014 Mediatek Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+#ifndef __MEDIATEK_COMMON_H__
+#define __MEDIATEK_COMMON_H__
+
+extern char init_intpol(void);
+
+#endif /* __MEDIATEK_COMMON_H__ */
diff --git a/arch/arm/mach-mediatek/intpol.c b/arch/arm/mach-mediatek/intpol.c
new file mode 100644
index 0000000..65ccc7c
--- /dev/null
+++ b/arch/arm/mach-mediatek/intpol.c
@@ -0,0 +1,61 @@
+/*
+ * This file contains common code that is intended to be used across
+ * boards so that it's not replicated.
+ *
+ * Copyright (C) 2014 Mediatek Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/of_address.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/arm-gic.h>
+
+#define GIC_HW_IRQ_BASE  32
+#define INT_POL_INDEX(a)   ((a) - GIC_HW_IRQ_BASE)
+
+static void __iomem *int_pol_base;
+
+static int mtk_int_pol_set_type(struct irq_data *d, unsigned int type)
+{
+	unsigned int irq = d->hwirq;
+	u32 offset, reg_index, value;
+
+	offset = INT_POL_INDEX(irq) & 0x1F;
+	reg_index = INT_POL_INDEX(irq) >> 5;
+
+	/* This arch extension was called with irq_controller_lock held,
+	   so the read-modify-write will be atomic */
+	value = readl(int_pol_base + reg_index * 4);
+	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING)
+		value |= (1 << offset);
+	else
+		value &= ~(1 << offset);
+	writel(value, int_pol_base + reg_index * 4);
+
+	return 0;
+}
+
+void init_intpol(void)
+{
+	struct device_node *node;
+
+	node = of_find_compatible_node(NULL, NULL, "mediatek,mt6577-intpol");
+	if (!node)
+		return;
+
+	int_pol_base = of_io_request_and_map(node, 0, "intpol");
+	if (IS_ERR(int_pol_base)) {
+		pr_warn("Can't get resource\n");
+		return;
+	}
+
+	gic_arch_extn.irq_set_type = mtk_int_pol_set_type;
+}
diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c
index 48051a2..aa10c70 100644
--- a/arch/arm/mach-mediatek/mediatek.c
+++ b/arch/arm/mach-mediatek/mediatek.c
@@ -16,6 +16,15 @@
  */
 #include <linux/init.h>
 #include <asm/mach/arch.h>
+#include <linux/irqchip.h>
+
+#include "common.h"
+
+static void __init mediatek_init_irq(void)
+{
+	init_intpol();
+	irqchip_init();
+}
 
 static const char * const mediatek_board_dt_compat[] = {
 	"mediatek,mt6589",
@@ -26,4 +35,5 @@ static const char * const mediatek_board_dt_compat[] = {
 
 DT_MACHINE_START(MEDIATEK_DT, "Mediatek Cortex-A7 (Device Tree)")
 	.dt_compat	= mediatek_board_dt_compat,
+	.init_irq       = mediatek_init_irq,
 MACHINE_END
-- 
1.8.1.1.dirty

[RESEND PATCH v2 3/4] arm: mediatek: Add intpol in mt6589.dtsi

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

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

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 arch/arm/boot/dts/mt6589.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
index d0297a0..18df47f 100644
--- a/arch/arm/boot/dts/mt6589.dtsi
+++ b/arch/arm/boot/dts/mt6589.dtsi
@@ -76,11 +76,16 @@
 		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";
 		};
 
+		intpol: intpol@10200100 {
+			compatible = "mediatek,mt6577-intpol";
+			reg = <0x10200100 0x1c>;
+		};
+
 		gic: interrupt-controller@10212000 {
 			compatible = "arm,cortex-a15-gic";
 			interrupt-controller;
-- 
1.8.1.1.dirty

[RESEND PATCH v2 3/4] arm: mediatek: Add intpol in mt6589.dtsi

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

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

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 arch/arm/boot/dts/mt6589.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
index d0297a0..18df47f 100644
--- a/arch/arm/boot/dts/mt6589.dtsi
+++ b/arch/arm/boot/dts/mt6589.dtsi
@@ -76,11 +76,16 @@
 		timer: timer at 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";
 		};
 
+		intpol: intpol at 10200100 {
+			compatible = "mediatek,mt6577-intpol";
+			reg = <0x10200100 0x1c>;
+		};
+
 		gic: interrupt-controller at 10212000 {
 			compatible = "arm,cortex-a15-gic";
 			interrupt-controller;
-- 
1.8.1.1.dirty

[RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

Add binding documentation for Mediatek SoC GIC interrupt polarity extension.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
new file mode 100644
index 0000000..16ea372
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
@@ -0,0 +1,16 @@
+Mediatek 65xx/81xx GIC interrupt polarity extension
+
+Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
+these can be used as GIC interrupt polarity extension.
+
+Required properties:
+- compatible: Compatible property value should be "mediatek,mt6577-intpol"
+
+- reg: Physical base address of the int pol registers and length of memory
+  mapped region.
+
+Example:
+       intpol: intpol@10200100 {
+               compatible = "mediatek,mt6577-intpol";
+               reg = <0x10200100 0x1c>;
+       };
-- 
1.8.1.1.dirty

[RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Joe.C]

From: "Joe.C" <yingjoe.chen@mediatek.com>

Add binding documentation for Mediatek SoC GIC interrupt polarity extension.

Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
---
 .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
new file mode 100644
index 0000000..16ea372
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
@@ -0,0 +1,16 @@
+Mediatek 65xx/81xx GIC interrupt polarity extension
+
+Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
+these can be used as GIC interrupt polarity extension.
+
+Required properties:
+- compatible: Compatible property value should be "mediatek,mt6577-intpol"
+
+- reg: Physical base address of the int pol registers and length of memory
+  mapped region.
+
+Example:
+       intpol: intpol at 10200100 {
+               compatible = "mediatek,mt6577-intpol";
+               reg = <0x10200100 0x1c>;
+       };
-- 
1.8.1.1.dirty

Re: [RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Matthias Brugger]

2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
>
> Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
>
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

I think the place for the file should be in
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
it is a interrupt-controller extension of the mediatek architecture.

>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> new file mode 100644
> index 0000000..16ea372
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> @@ -0,0 +1,16 @@
> +Mediatek 65xx/81xx GIC interrupt polarity extension
> +
> +Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
> +these can be used as GIC interrupt polarity extension.
> +
> +Required properties:
> +- compatible: Compatible property value should be "mediatek,mt6577-intpol"
> +
> +- reg: Physical base address of the int pol registers and length of memory
> +  mapped region.
> +
> +Example:
> +       intpol: intpol@10200100 {
> +               compatible = "mediatek,mt6577-intpol";
> +               reg = <0x10200100 0x1c>;
> +       };
> --
> 1.8.1.1.dirty
>



-- 
motzblog.wordpress.com

Re: [RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Matthias Brugger]

2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
>
> Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
>
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

I think the place for the file should be in
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
it is a interrupt-controller extension of the mediatek architecture.

>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> new file mode 100644
> index 0000000..16ea372
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> @@ -0,0 +1,16 @@
> +Mediatek 65xx/81xx GIC interrupt polarity extension
> +
> +Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
> +these can be used as GIC interrupt polarity extension.
> +
> +Required properties:
> +- compatible: Compatible property value should be "mediatek,mt6577-intpol"
> +
> +- reg: Physical base address of the int pol registers and length of memory
> +  mapped region.
> +
> +Example:
> +       intpol: intpol@10200100 {
> +               compatible = "mediatek,mt6577-intpol";
> +               reg = <0x10200100 0x1c>;
> +       };
> --
> 1.8.1.1.dirty
>



-- 
motzblog.wordpress.com

[RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Matthias Brugger]

2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
>
> Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
>
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt

I think the place for the file should be in
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
it is a interrupt-controller extension of the mediatek architecture.

>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> new file mode 100644
> index 0000000..16ea372
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> @@ -0,0 +1,16 @@
> +Mediatek 65xx/81xx GIC interrupt polarity extension
> +
> +Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
> +these can be used as GIC interrupt polarity extension.
> +
> +Required properties:
> +- compatible: Compatible property value should be "mediatek,mt6577-intpol"
> +
> +- reg: Physical base address of the int pol registers and length of memory
> +  mapped region.
> +
> +Example:
> +       intpol: intpol at 10200100 {
> +               compatible = "mediatek,mt6577-intpol";
> +               reg = <0x10200100 0x1c>;
> +       };
> --
> 1.8.1.1.dirty
>



-- 
motzblog.wordpress.com

Re: [RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Joe.C]

Hi, Rob,

Would you help to review this DT bindings and give us some
suggestions? Matthias suggests moving this to
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt
I think that's a better place for this. What do you think?

Joe.C

On Thu, 2014-08-21 at 17:02 +0200, Matthias Brugger wrote:
> 2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> > From: "Joe.C" <yingjoe.chen@mediatek.com>
> >
> > Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
> >
> > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> > ---
> >  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> 
> I think the place for the file should be in
> Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
> it is a interrupt-controller extension of the mediatek architecture.
> 
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> > new file mode 100644
> > index 0000000..16ea372
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> > @@ -0,0 +1,16 @@
> > +Mediatek 65xx/81xx GIC interrupt polarity extension
> > +
> > +Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
> > +these can be used as GIC interrupt polarity extension.
> > +
> > +Required properties:
> > +- compatible: Compatible property value should be "mediatek,mt6577-intpol"
> > +
> > +- reg: Physical base address of the int pol registers and length of memory
> > +  mapped region.
> > +
> > +Example:
> > +       intpol: intpol@10200100 {
> > +               compatible = "mediatek,mt6577-intpol";
> > +               reg = <0x10200100 0x1c>;
> > +       };
> > --
> > 1.8.1.1.dirty
> >
> 
> 
> 

[RESEND PATCH v2 4/4] dt-bindings: add bindings for mediatek intpol

[Joe.C]

Hi, Rob,

Would you help to review this DT bindings and give us some
suggestions? Matthias suggests moving this to
Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt
I think that's a better place for this. What do you think?

Joe.C

On Thu, 2014-08-21 at 17:02 +0200, Matthias Brugger wrote:
> 2014-08-13 4:11 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>:
> > From: "Joe.C" <yingjoe.chen@mediatek.com>
> >
> > Add binding documentation for Mediatek SoC GIC interrupt polarity extension.
> >
> > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> > ---
> >  .../bindings/interrupt-controller/mediatek,intpol.txt    | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> 
> I think the place for the file should be in
> Documentation/devicetree/bindings/arm/mediatek/mediatek,intpol.txt as
> it is a interrupt-controller extension of the mediatek architecture.
> 
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> > new file mode 100644
> > index 0000000..16ea372
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
> > @@ -0,0 +1,16 @@
> > +Mediatek 65xx/81xx GIC interrupt polarity extension
> > +
> > +Mediatek SOCs contain controllable inverter for each GIC SPI interrupt,
> > +these can be used as GIC interrupt polarity extension.
> > +
> > +Required properties:
> > +- compatible: Compatible property value should be "mediatek,mt6577-intpol"
> > +
> > +- reg: Physical base address of the int pol registers and length of memory
> > +  mapped region.
> > +
> > +Example:
> > +       intpol: intpol at 10200100 {
> > +               compatible = "mediatek,mt6577-intpol";
> > +               reg = <0x10200100 0x1c>;
> > +       };
> > --
> > 1.8.1.1.dirty
> >
> 
> 
> 

Re: [RESEND PATCH v2 0/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

Hi, ARM soc maintainers,

Please help to review this series, I think this should go through 
ARM soc tree.

We plan to upstream drivers for MT8135. MT8135 is a big/little soc
featuring 2 CA7 + 2 CA15, and sharing many similar IP components with
mt65xx. Many components of 65xx series & 8135 require this support.

Joe.C


On Wed, 2014-08-13 at 10:11 +0800, Joe.C wrote:
> [Sorry, resend to include all maintainers.]
> 
> This series add support for mediatek GIC interrupt polarity extension.
> Several components in mediatek SoC have low level triggered interrupt and
> require this support.
> 
> This also correct 6589 timer irq polarity. Previous version works because
> 6589 boot loader already set correct polarity for timer interrupt.
> 
> The patch set is based on Matthias's Mediatek basic support for v3.17 [1].
> 
> v2:
> - Make mt6589.dtsi changes as a separate commit as Matthias suggest.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272561.html
> 
> Joe.C (4):
>   irqchip: gic: Change irq type check when extension is
>   arm: mediatek: Add support for GIC interrupt polarity
>   arm: mediatek: Add intpol in mt6589.dtsi
>   dt-bindings: add bindings for mediatek intpol
> 
>  Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt |   16 ++
>  arch/arm/boot/dts/mt6589.dtsi                                              |    7 -
>  arch/arm/mach-mediatek/Makefile                                            |    2
>  arch/arm/mach-mediatek/common.h                                            |   19 +++
>  arch/arm/mach-mediatek/intpol.c                                            |   61 ++++++++++
>  arch/arm/mach-mediatek/mediatek.c                                          |   10 +
>  drivers/irqchip/irq-gic.c                                                  |   27 ++--
>  7 files changed, 131 insertions(+), 11 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
>  create mode 100644 arch/arm/mach-mediatek/common.h
>  create mode 100644 arch/arm/mach-mediatek/intpol.c

[RESEND PATCH v2 0/4] arm: mediatek: Add support for GIC interrupt polarity extension.

[Joe.C]

Hi, ARM soc maintainers,

Please help to review this series, I think this should go through 
ARM soc tree.

We plan to upstream drivers for MT8135. MT8135 is a big/little soc
featuring 2 CA7 + 2 CA15, and sharing many similar IP components with
mt65xx. Many components of 65xx series & 8135 require this support.

Joe.C


On Wed, 2014-08-13 at 10:11 +0800, Joe.C wrote:
> [Sorry, resend to include all maintainers.]
> 
> This series add support for mediatek GIC interrupt polarity extension.
> Several components in mediatek SoC have low level triggered interrupt and
> require this support.
> 
> This also correct 6589 timer irq polarity. Previous version works because
> 6589 boot loader already set correct polarity for timer interrupt.
> 
> The patch set is based on Matthias's Mediatek basic support for v3.17 [1].
> 
> v2:
> - Make mt6589.dtsi changes as a separate commit as Matthias suggest.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/272561.html
> 
> Joe.C (4):
>   irqchip: gic: Change irq type check when extension is
>   arm: mediatek: Add support for GIC interrupt polarity
>   arm: mediatek: Add intpol in mt6589.dtsi
>   dt-bindings: add bindings for mediatek intpol
> 
>  Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt |   16 ++
>  arch/arm/boot/dts/mt6589.dtsi                                              |    7 -
>  arch/arm/mach-mediatek/Makefile                                            |    2
>  arch/arm/mach-mediatek/common.h                                            |   19 +++
>  arch/arm/mach-mediatek/intpol.c                                            |   61 ++++++++++
>  arch/arm/mach-mediatek/mediatek.c                                          |   10 +
>  drivers/irqchip/irq-gic.c                                                  |   27 ++--
>  7 files changed, 131 insertions(+), 11 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,intpol.txt
>  create mode 100644 arch/arm/mach-mediatek/common.h
>  create mode 100644 arch/arm/mach-mediatek/intpol.c

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Joe,

On 13/08/14 03:11, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> GIC supports the combination with external extensions. But this
> is not reflected in the checks of the interrupt type flag.
> This patch allows interrupt types other than the one supported by GIC,
> if an architecture extension is present and supports them.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57d165e..66485ab 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	u32 confoff = (gicirq / 16) * 4;
>  	bool enabled = false;
>  	u32 val;
> +	int ret = 0;
>  
>  	/* Interrupt configuration for SGIs can't be changed */
>  	if (gicirq < 16)
>  		return -EINVAL;
>  
> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> -		return -EINVAL;
> -
>  	raw_spin_lock(&irq_controller_lock);
>  
> -	if (gic_arch_extn.irq_set_type)
> -		gic_arch_extn.irq_set_type(d, type);
> +	if (gic_arch_extn.irq_set_type) {
> +		ret = gic_arch_extn.irq_set_type(d, type);
> +		if (ret)
> +			goto out;
> +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> +		type != IRQ_TYPE_EDGE_RISING) {
> +			ret = -EINVAL;
> +			goto out;
> +	}
>  
>  	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> -	if (type == IRQ_TYPE_LEVEL_HIGH)
> +	/* Check for both edge and level here, so we can support GIC irq
> +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> +	   doesn't support polarity extension, the check above will reject
> +	   improper type. */
> +	if (type & IRQ_TYPE_LEVEL_MASK)
>  		val &= ~confmask;
> -	else if (type == IRQ_TYPE_EDGE_RISING)
> +	else if (type & IRQ_TYPE_EDGE_BOTH)
>  		val |= confmask;
>  
>  	/*
> @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  
>  	if (enabled)
>  		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
> -
> +out:
>  	raw_spin_unlock(&irq_controller_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int gic_retrigger(struct irq_data *d)
> 

You're really abusing the gic_arch_extn feature. I know this is
tempting, but this is pushing it a bit too far.

This feature exist for one particular reason: if your GIC is in the same
power-domain as the CPUs, it will go down as well when you suspend the
system, hence being enable to wake the CPU up. You then need a shadow
interrupt controller to take over. This is exactly why we call the hook
on every GIC-related operation.

Here, you're using it to program something that sits between the device
and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.

You can probably model this as a separate irqchip for the few interrupts
that require this, or have it configured at boot time (assuming the
configuration never changes).

Thanks,

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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Joe,

On 13/08/14 03:11, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> GIC supports the combination with external extensions. But this
> is not reflected in the checks of the interrupt type flag.
> This patch allows interrupt types other than the one supported by GIC,
> if an architecture extension is present and supports them.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57d165e..66485ab 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	u32 confoff = (gicirq / 16) * 4;
>  	bool enabled = false;
>  	u32 val;
> +	int ret = 0;
>  
>  	/* Interrupt configuration for SGIs can't be changed */
>  	if (gicirq < 16)
>  		return -EINVAL;
>  
> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> -		return -EINVAL;
> -
>  	raw_spin_lock(&irq_controller_lock);
>  
> -	if (gic_arch_extn.irq_set_type)
> -		gic_arch_extn.irq_set_type(d, type);
> +	if (gic_arch_extn.irq_set_type) {
> +		ret = gic_arch_extn.irq_set_type(d, type);
> +		if (ret)
> +			goto out;
> +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> +		type != IRQ_TYPE_EDGE_RISING) {
> +			ret = -EINVAL;
> +			goto out;
> +	}
>  
>  	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> -	if (type == IRQ_TYPE_LEVEL_HIGH)
> +	/* Check for both edge and level here, so we can support GIC irq
> +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> +	   doesn't support polarity extension, the check above will reject
> +	   improper type. */
> +	if (type & IRQ_TYPE_LEVEL_MASK)
>  		val &= ~confmask;
> -	else if (type == IRQ_TYPE_EDGE_RISING)
> +	else if (type & IRQ_TYPE_EDGE_BOTH)
>  		val |= confmask;
>  
>  	/*
> @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  
>  	if (enabled)
>  		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
> -
> +out:
>  	raw_spin_unlock(&irq_controller_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int gic_retrigger(struct irq_data *d)
> 

You're really abusing the gic_arch_extn feature. I know this is
tempting, but this is pushing it a bit too far.

This feature exist for one particular reason: if your GIC is in the same
power-domain as the CPUs, it will go down as well when you suspend the
system, hence being enable to wake the CPU up. You then need a shadow
interrupt controller to take over. This is exactly why we call the hook
on every GIC-related operation.

Here, you're using it to program something that sits between the device
and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.

You can probably model this as a separate irqchip for the few interrupts
that require this, or have it configured at boot time (assuming the
configuration never changes).

Thanks,

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

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Joe,

On 13/08/14 03:11, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> GIC supports the combination with external extensions. But this
> is not reflected in the checks of the interrupt type flag.
> This patch allows interrupt types other than the one supported by GIC,
> if an architecture extension is present and supports them.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57d165e..66485ab 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	u32 confoff = (gicirq / 16) * 4;
>  	bool enabled = false;
>  	u32 val;
> +	int ret = 0;
>  
>  	/* Interrupt configuration for SGIs can't be changed */
>  	if (gicirq < 16)
>  		return -EINVAL;
>  
> -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> -		return -EINVAL;
> -
>  	raw_spin_lock(&irq_controller_lock);
>  
> -	if (gic_arch_extn.irq_set_type)
> -		gic_arch_extn.irq_set_type(d, type);
> +	if (gic_arch_extn.irq_set_type) {
> +		ret = gic_arch_extn.irq_set_type(d, type);
> +		if (ret)
> +			goto out;
> +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> +		type != IRQ_TYPE_EDGE_RISING) {
> +			ret = -EINVAL;
> +			goto out;
> +	}
>  
>  	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> -	if (type == IRQ_TYPE_LEVEL_HIGH)
> +	/* Check for both edge and level here, so we can support GIC irq
> +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> +	   doesn't support polarity extension, the check above will reject
> +	   improper type. */
> +	if (type & IRQ_TYPE_LEVEL_MASK)
>  		val &= ~confmask;
> -	else if (type == IRQ_TYPE_EDGE_RISING)
> +	else if (type & IRQ_TYPE_EDGE_BOTH)
>  		val |= confmask;
>  
>  	/*
> @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  
>  	if (enabled)
>  		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
> -
> +out:
>  	raw_spin_unlock(&irq_controller_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int gic_retrigger(struct irq_data *d)
> 

You're really abusing the gic_arch_extn feature. I know this is
tempting, but this is pushing it a bit too far.

This feature exist for one particular reason: if your GIC is in the same
power-domain as the CPUs, it will go down as well when you suspend the
system, hence being enable to wake the CPU up. You then need a shadow
interrupt controller to take over. This is exactly why we call the hook
on every GIC-related operation.

Here, you're using it to program something that sits between the device
and the GIC. This is a separate block, with its own hardware
configuration, that modifies the interrupt signal. This should be
reflected in the device-tree and the code paths.

You can probably model this as a separate irqchip for the few interrupts
that require this, or have it configured at boot time (assuming the
configuration never changes).

Thanks,

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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Joe.C]

Hi,

Thanks for your suggestions.

On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
> Hi Joe,
> 
> On 13/08/14 03:11, Joe.C wrote:
> > From: "Joe.C" <yingjoe.chen@mediatek.com>
> > 
> > GIC supports the combination with external extensions. But this
> > is not reflected in the checks of the interrupt type flag.
> > This patch allows interrupt types other than the one supported by GIC,
> > if an architecture extension is present and supports them.
> > 
> > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> > ---
> >  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 57d165e..66485ab 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> >  	u32 confoff = (gicirq / 16) * 4;
> >  	bool enabled = false;
> >  	u32 val;
> > +	int ret = 0;
> >  
> >  	/* Interrupt configuration for SGIs can't be changed */
> >  	if (gicirq < 16)
> >  		return -EINVAL;
> >  
> > -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > -		return -EINVAL;
> > -
> >  	raw_spin_lock(&irq_controller_lock);
> >  
> > -	if (gic_arch_extn.irq_set_type)
> > -		gic_arch_extn.irq_set_type(d, type);
> > +	if (gic_arch_extn.irq_set_type) {
> > +		ret = gic_arch_extn.irq_set_type(d, type);
> > +		if (ret)
> > +			goto out;
> > +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> > +		type != IRQ_TYPE_EDGE_RISING) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +	}
> >  
> >  	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> > -	if (type == IRQ_TYPE_LEVEL_HIGH)
> > +	/* Check for both edge and level here, so we can support GIC irq
> > +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> > +	   doesn't support polarity extension, the check above will reject
> > +	   improper type. */
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> >  		val &= ~confmask;
> > -	else if (type == IRQ_TYPE_EDGE_RISING)
> > +	else if (type & IRQ_TYPE_EDGE_BOTH)
> >  		val |= confmask;
> >  
> >  	/*
> > @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> >  
> >  	if (enabled)
> >  		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
> > -
> > +out:
> >  	raw_spin_unlock(&irq_controller_lock);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int gic_retrigger(struct irq_data *d)
> > 
> 
> You're really abusing the gic_arch_extn feature. I know this is
> tempting, but this is pushing it a bit too far.
> 
> This feature exist for one particular reason: if your GIC is in the same
> power-domain as the CPUs, it will go down as well when you suspend the
> system, hence being enable to wake the CPU up. You then need a shadow
> interrupt controller to take over. This is exactly why we call the hook
> on every GIC-related operation.

Actually we are doing this too, it is called SYS_CIRQ in our IC, and
we'll need to support that in the future. This intpol is part of CIRQ
function block. Does it make more senses if I add skeleton for CIRQ
support, and implement intpol inside it?

> Here, you're using it to program something that sits between the device
> and the GIC. This is a separate block, with its own hardware
> configuration, that modifies the interrupt signal. This should be
> reflected in the device-tree and the code paths.
> 
> You can probably model this as a separate irqchip for the few interrupts
> that require this, or have it configured at boot time (assuming the
> configuration never changes).

The boot loader did setup interrupt polarity for those used in boot
loader, but not all of them. 

Datasheet lists components irqs as low active, so I think it makes more
sense to allow driver to use IRQF_TRIGGER_LOW instead of having them to
notice GIC only support high active and use IRQF_TRIGGER_HIGH. This
rule out configure it at boot loader or kernel init irq time.

If I implement this as a separate irqchip, I need to reuse most gic
irqchip functions. Without changing irq-gic.c to make them global,
I can only think of hack like this:

        gic_init_bases(..) /* init gic */
        gic_chip = irq_get_chip(0);  /* to get gic irq_chip */
        org_gic_set_type = gic_chip->irq_set_type;
        gic_chip->irq_set_type = mt_irq_polarity_set_type;

and calling original gic_set_type from mt_irq_polarity_set_type with irq
type fixup.

Joe.C

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Joe.C]

Hi,

Thanks for your suggestions.

On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
> Hi Joe,
> 
> On 13/08/14 03:11, Joe.C wrote:
> > From: "Joe.C" <yingjoe.chen@mediatek.com>
> > 
> > GIC supports the combination with external extensions. But this
> > is not reflected in the checks of the interrupt type flag.
> > This patch allows interrupt types other than the one supported by GIC,
> > if an architecture extension is present and supports them.
> > 
> > Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> > ---
> >  drivers/irqchip/irq-gic.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 57d165e..66485ab 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -194,23 +194,32 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> >  	u32 confoff = (gicirq / 16) * 4;
> >  	bool enabled = false;
> >  	u32 val;
> > +	int ret = 0;
> >  
> >  	/* Interrupt configuration for SGIs can't be changed */
> >  	if (gicirq < 16)
> >  		return -EINVAL;
> >  
> > -	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > -		return -EINVAL;
> > -
> >  	raw_spin_lock(&irq_controller_lock);
> >  
> > -	if (gic_arch_extn.irq_set_type)
> > -		gic_arch_extn.irq_set_type(d, type);
> > +	if (gic_arch_extn.irq_set_type) {
> > +		ret = gic_arch_extn.irq_set_type(d, type);
> > +		if (ret)
> > +			goto out;
> > +	} else if (type != IRQ_TYPE_LEVEL_HIGH &&
> > +		type != IRQ_TYPE_EDGE_RISING) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +	}
> >  
> >  	val = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> > -	if (type == IRQ_TYPE_LEVEL_HIGH)
> > +	/* Check for both edge and level here, so we can support GIC irq
> > +	   polarity extension in gic_arch_extn.irq_set_type. If arch
> > +	   doesn't support polarity extension, the check above will reject
> > +	   improper type. */
> > +	if (type & IRQ_TYPE_LEVEL_MASK)
> >  		val &= ~confmask;
> > -	else if (type == IRQ_TYPE_EDGE_RISING)
> > +	else if (type & IRQ_TYPE_EDGE_BOTH)
> >  		val |= confmask;
> >  
> >  	/*
> > @@ -226,10 +235,10 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> >  
> >  	if (enabled)
> >  		writel_relaxed(enablemask, base + GIC_DIST_ENABLE_SET + enableoff);
> > -
> > +out:
> >  	raw_spin_unlock(&irq_controller_lock);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int gic_retrigger(struct irq_data *d)
> > 
> 
> You're really abusing the gic_arch_extn feature. I know this is
> tempting, but this is pushing it a bit too far.
> 
> This feature exist for one particular reason: if your GIC is in the same
> power-domain as the CPUs, it will go down as well when you suspend the
> system, hence being enable to wake the CPU up. You then need a shadow
> interrupt controller to take over. This is exactly why we call the hook
> on every GIC-related operation.

Actually we are doing this too, it is called SYS_CIRQ in our IC, and
we'll need to support that in the future. This intpol is part of CIRQ
function block. Does it make more senses if I add skeleton for CIRQ
support, and implement intpol inside it?

> Here, you're using it to program something that sits between the device
> and the GIC. This is a separate block, with its own hardware
> configuration, that modifies the interrupt signal. This should be
> reflected in the device-tree and the code paths.
> 
> You can probably model this as a separate irqchip for the few interrupts
> that require this, or have it configured at boot time (assuming the
> configuration never changes).

The boot loader did setup interrupt polarity for those used in boot
loader, but not all of them. 

Datasheet lists components irqs as low active, so I think it makes more
sense to allow driver to use IRQF_TRIGGER_LOW instead of having them to
notice GIC only support high active and use IRQF_TRIGGER_HIGH. This
rule out configure it at boot loader or kernel init irq time.

If I implement this as a separate irqchip, I need to reuse most gic
irqchip functions. Without changing irq-gic.c to make them global,
I can only think of hack like this:

        gic_init_bases(..) /* init gic */
        gic_chip = irq_get_chip(0);  /* to get gic irq_chip */
        org_gic_set_type = gic_chip->irq_set_type;
        gic_chip->irq_set_type = mt_irq_polarity_set_type;

and calling original gic_set_type from mt_irq_polarity_set_type with irq
type fixup.

Joe.C

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Jan Lübbe]

Marc,

On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
> Here, you're using it to program something that sits between the
> device and the GIC. This is a separate block, with its own hardware
> configuration, that modifies the interrupt signal. This should be
> reflected in the device-tree and the code paths.
> 
> You can probably model this as a separate irqchip for the few
> interrupts that require this, or have it configured at boot time
> (assuming the configuration never changes).

It seems to me that using a separate irqchip for a simple inverter would
add the run-time overhead of passing through wrapper functions on every
IRQ. Do you have an idea how this could be avoided without using the
gic_arch_extn feature?

As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Jan Lübbe]

Marc,

On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
> Here, you're using it to program something that sits between the
> device and the GIC. This is a separate block, with its own hardware
> configuration, that modifies the interrupt signal. This should be
> reflected in the device-tree and the code paths.
> 
> You can probably model this as a separate irqchip for the few
> interrupts that require this, or have it configured at boot time
> (assuming the configuration never changes).

It seems to me that using a separate irqchip for a simple inverter would
add the run-time overhead of passing through wrapper functions on every
IRQ. Do you have an idea how this could be avoided without using the
gic_arch_extn feature?

As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Jan Lübbe]

Marc,

On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
> Here, you're using it to program something that sits between the
> device and the GIC. This is a separate block, with its own hardware
> configuration, that modifies the interrupt signal. This should be
> reflected in the device-tree and the code paths.
> 
> You can probably model this as a separate irqchip for the few
> interrupts that require this, or have it configured at boot time
> (assuming the configuration never changes).

It seems to me that using a separate irqchip for a simple inverter would
add the run-time overhead of passing through wrapper functions on every
IRQ. Do you have an idea how this could be avoided without using the
gic_arch_extn feature?

As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Jan,

On 27/08/14 10:55, Jan Lübbe wrote:
> Marc,
> 
> On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
>> Here, you're using it to program something that sits between the
>> device and the GIC. This is a separate block, with its own hardware
>> configuration, that modifies the interrupt signal. This should be
>> reflected in the device-tree and the code paths.
>>
>> You can probably model this as a separate irqchip for the few
>> interrupts that require this, or have it configured at boot time
>> (assuming the configuration never changes).
> 
> It seems to me that using a separate irqchip for a simple inverter would
> add the run-time overhead of passing through wrapper functions on every
> IRQ. Do you have an idea how this could be avoided without using the
> gic_arch_extn feature?

Well, from the rather vague description, it could be slightly more than
a simple inverter, like being able to generate interrupts on both rising
and falling edges. Sorry, but this is not the GIC as ARM has architected it.

Yes, the additional irqchip adds some overhead. But the DT has to
reflect the fact that there is something on the interrupt path that does
some form of conversion.

> As in the DT the actual IRQ polarity should be used, simply configuring
> the HW IRQ polarity in the bootloader is not enough without telling the
> GIC driver which polarity is supported on which IRQ, right?

Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.

So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.

Something will have to configure the polarity widget though, but that
can be left outside of the GIC.

Thanks,

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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Jan,

On 27/08/14 10:55, Jan Lübbe wrote:
> Marc,
> 
> On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
>> Here, you're using it to program something that sits between the
>> device and the GIC. This is a separate block, with its own hardware
>> configuration, that modifies the interrupt signal. This should be
>> reflected in the device-tree and the code paths.
>>
>> You can probably model this as a separate irqchip for the few
>> interrupts that require this, or have it configured at boot time
>> (assuming the configuration never changes).
> 
> It seems to me that using a separate irqchip for a simple inverter would
> add the run-time overhead of passing through wrapper functions on every
> IRQ. Do you have an idea how this could be avoided without using the
> gic_arch_extn feature?

Well, from the rather vague description, it could be slightly more than
a simple inverter, like being able to generate interrupts on both rising
and falling edges. Sorry, but this is not the GIC as ARM has architected it.

Yes, the additional irqchip adds some overhead. But the DT has to
reflect the fact that there is something on the interrupt path that does
some form of conversion.

> As in the DT the actual IRQ polarity should be used, simply configuring
> the HW IRQ polarity in the bootloader is not enough without telling the
> GIC driver which polarity is supported on which IRQ, right?

Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.

So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.

Something will have to configure the polarity widget though, but that
can be left outside of the GIC.

Thanks,

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

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

Hi Jan,

On 27/08/14 10:55, Jan L?bbe wrote:
> Marc,
> 
> On Fri, 2014-08-22 at 12:09 +0100, Marc Zyngier wrote:
>> Here, you're using it to program something that sits between the
>> device and the GIC. This is a separate block, with its own hardware
>> configuration, that modifies the interrupt signal. This should be
>> reflected in the device-tree and the code paths.
>>
>> You can probably model this as a separate irqchip for the few
>> interrupts that require this, or have it configured at boot time
>> (assuming the configuration never changes).
> 
> It seems to me that using a separate irqchip for a simple inverter would
> add the run-time overhead of passing through wrapper functions on every
> IRQ. Do you have an idea how this could be avoided without using the
> gic_arch_extn feature?

Well, from the rather vague description, it could be slightly more than
a simple inverter, like being able to generate interrupts on both rising
and falling edges. Sorry, but this is not the GIC as ARM has architected it.

Yes, the additional irqchip adds some overhead. But the DT has to
reflect the fact that there is something on the interrupt path that does
some form of conversion.

> As in the DT the actual IRQ polarity should be used, simply configuring
> the HW IRQ polarity in the bootloader is not enough without telling the
> GIC driver which polarity is supported on which IRQ, right?

Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.

So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.

Something will have to configure the polarity widget though, but that
can be left outside of the GIC.

Thanks,

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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> > As in the DT the actual IRQ polarity should be used, simply configuring
> > the HW IRQ polarity in the bootloader is not enough without telling the
> > GIC driver which polarity is supported on which IRQ, right?
> 
> Looking a bit closer at things, what you describe in DT is the IRQ
> polarity the interrupt controller sees. Nothing else should interpret
> that field.
> 
> So it is legal (IMO) to have a device with an interrupt specifier
> describing a rising edge interrupt, and yet have the device generating a
> falling edge, with Mediatek's special sauce doing the conversion in between.
> 
> Something will have to configure the polarity widget though, but that
> can be left outside of the GIC.

This seems to become a popular topic and it looks like the whole GIC
extension thing is going to explode sooner than later.

We are currently discussing hierarchical irq domains to solve a
different issue in x86 land. See the related discussion here:

	  https://lkml.org/lkml/2014/8/1/67

Now looking at these GIC plus extra sauce problems, I wonder whether
this wouldn't be solvable in a similar way. If you look at it from the
HW perspective you have:

   ---------      ---------
---| MAGIC |------|ARM GIC|
---|       |------|       |
---|       |------|       |
---|       |------|       |
---|       |------|       |
   ---------      ---------

The MAGIC interrupt controller only provides functionality which is
not available from the ARM architected GIC but maps 1:1 to the ARM GIC
interrupt lines. So it looks like a variation to the more dynamic
mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.

The idea is to have two irq domains: magic_domain and armgic_domain.

The magic_domain is the frontend facing the devices and the
armgic_domain is the parent domain. This is also reflected in
hierarchical data structures, i.e. irq_desc->irq_data will get a new
field parent_data, which points to the irq_data of the parent
interrupt controller, which is allocated separately when the interrupt
line is instantiated.

So in the above case the hotpath ack/eoi functions of the irq chip
associated to the device interrupt, i.e. magic_chip, would do:

irq_ack(struct irq_data *d)
{
	struct irq_data *pd = d->parent_data;

	pd->chip->irq_ack(pd);
}

Granted, that's another level of indirection, but this is going to be
more efficient than a boatload of conditional hooks in the GIC code
proper. Not to talk about the maintainability of the whole maze.

The irq_set_type() function would do:

irq_set_type(struct irq_data *d, int type)
{
	struct irq_data *pd = d->parent_data;

	gic_type = set_magic_chip_type(d, type);

	return pd->chip->irq_set_type(d, gic_type);
}

Switching to this allows to completely avoid the gazillion of hooks in
the gic code and should work nicely with multiplatform kernels by
simpling hooking up the domain parent relation ship to the proper
magic domain or leave the armgic as the direct device interface in
case the SoC does not have the magic chip in front of the arm gic.

Thoughts?

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> > As in the DT the actual IRQ polarity should be used, simply configuring
> > the HW IRQ polarity in the bootloader is not enough without telling the
> > GIC driver which polarity is supported on which IRQ, right?
> 
> Looking a bit closer at things, what you describe in DT is the IRQ
> polarity the interrupt controller sees. Nothing else should interpret
> that field.
> 
> So it is legal (IMO) to have a device with an interrupt specifier
> describing a rising edge interrupt, and yet have the device generating a
> falling edge, with Mediatek's special sauce doing the conversion in between.
> 
> Something will have to configure the polarity widget though, but that
> can be left outside of the GIC.

This seems to become a popular topic and it looks like the whole GIC
extension thing is going to explode sooner than later.

We are currently discussing hierarchical irq domains to solve a
different issue in x86 land. See the related discussion here:

	  https://lkml.org/lkml/2014/8/1/67

Now looking at these GIC plus extra sauce problems, I wonder whether
this wouldn't be solvable in a similar way. If you look at it from the
HW perspective you have:

   ---------      ---------
---| MAGIC |------|ARM GIC|
---|       |------|       |
---|       |------|       |
---|       |------|       |
---|       |------|       |
   ---------      ---------

The MAGIC interrupt controller only provides functionality which is
not available from the ARM architected GIC but maps 1:1 to the ARM GIC
interrupt lines. So it looks like a variation to the more dynamic
mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.

The idea is to have two irq domains: magic_domain and armgic_domain.

The magic_domain is the frontend facing the devices and the
armgic_domain is the parent domain. This is also reflected in
hierarchical data structures, i.e. irq_desc->irq_data will get a new
field parent_data, which points to the irq_data of the parent
interrupt controller, which is allocated separately when the interrupt
line is instantiated.

So in the above case the hotpath ack/eoi functions of the irq chip
associated to the device interrupt, i.e. magic_chip, would do:

irq_ack(struct irq_data *d)
{
	struct irq_data *pd = d->parent_data;

	pd->chip->irq_ack(pd);
}

Granted, that's another level of indirection, but this is going to be
more efficient than a boatload of conditional hooks in the GIC code
proper. Not to talk about the maintainability of the whole maze.

The irq_set_type() function would do:

irq_set_type(struct irq_data *d, int type)
{
	struct irq_data *pd = d->parent_data;

	gic_type = set_magic_chip_type(d, type);

	return pd->chip->irq_set_type(d, gic_type);
}

Switching to this allows to completely avoid the gazillion of hooks in
the gic code and should work nicely with multiplatform kernels by
simpling hooking up the domain parent relation ship to the proper
magic domain or leave the armgic as the direct device interface in
case the SoC does not have the magic chip in front of the arm gic.

Thoughts?

Thanks,

	tglx

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> > As in the DT the actual IRQ polarity should be used, simply configuring
> > the HW IRQ polarity in the bootloader is not enough without telling the
> > GIC driver which polarity is supported on which IRQ, right?
> 
> Looking a bit closer at things, what you describe in DT is the IRQ
> polarity the interrupt controller sees. Nothing else should interpret
> that field.
> 
> So it is legal (IMO) to have a device with an interrupt specifier
> describing a rising edge interrupt, and yet have the device generating a
> falling edge, with Mediatek's special sauce doing the conversion in between.
> 
> Something will have to configure the polarity widget though, but that
> can be left outside of the GIC.

This seems to become a popular topic and it looks like the whole GIC
extension thing is going to explode sooner than later.

We are currently discussing hierarchical irq domains to solve a
different issue in x86 land. See the related discussion here:

	  https://lkml.org/lkml/2014/8/1/67

Now looking at these GIC plus extra sauce problems, I wonder whether
this wouldn't be solvable in a similar way. If you look at it from the
HW perspective you have:

   ---------      ---------
---| MAGIC |------|ARM GIC|
---|       |------|       |
---|       |------|       |
---|       |------|       |
---|       |------|       |
   ---------      ---------

The MAGIC interrupt controller only provides functionality which is
not available from the ARM architected GIC but maps 1:1 to the ARM GIC
interrupt lines. So it looks like a variation to the more dynamic
mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.

The idea is to have two irq domains: magic_domain and armgic_domain.

The magic_domain is the frontend facing the devices and the
armgic_domain is the parent domain. This is also reflected in
hierarchical data structures, i.e. irq_desc->irq_data will get a new
field parent_data, which points to the irq_data of the parent
interrupt controller, which is allocated separately when the interrupt
line is instantiated.

So in the above case the hotpath ack/eoi functions of the irq chip
associated to the device interrupt, i.e. magic_chip, would do:

irq_ack(struct irq_data *d)
{
	struct irq_data *pd = d->parent_data;

	pd->chip->irq_ack(pd);
}

Granted, that's another level of indirection, but this is going to be
more efficient than a boatload of conditional hooks in the GIC code
proper. Not to talk about the maintainability of the whole maze.

The irq_set_type() function would do:

irq_set_type(struct irq_data *d, int type)
{
	struct irq_data *pd = d->parent_data;

	gic_type = set_magic_chip_type(d, type);

	return pd->chip->irq_set_type(d, gic_type);
}

Switching to this allows to completely avoid the gazillion of hooks in
the gic code and should work nicely with multiplatform kernels by
simpling hooking up the domain parent relation ship to the proper
magic domain or leave the armgic as the direct device interface in
case the SoC does not have the magic chip in front of the arm gic.

Thoughts?

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

[Adding Jiang Liu to the spam-fest]

Hi Thomas,

On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 27 Aug 2014, Marc Zyngier wrote:
>> > As in the DT the actual IRQ polarity should be used, simply configuring
>> > the HW IRQ polarity in the bootloader is not enough without telling the
>> > GIC driver which polarity is supported on which IRQ, right?
>> 
>> Looking a bit closer at things, what you describe in DT is the IRQ
>> polarity the interrupt controller sees. Nothing else should interpret
>> that field.
>> 
>> So it is legal (IMO) to have a device with an interrupt specifier
>> describing a rising edge interrupt, and yet have the device generating a
>> falling edge, with Mediatek's special sauce doing the conversion in between.
>> 
>> Something will have to configure the polarity widget though, but that
>> can be left outside of the GIC.
>
> This seems to become a popular topic and it looks like the whole GIC
> extension thing is going to explode sooner than later.
>
> We are currently discussing hierarchical irq domains to solve a
> different issue in x86 land. See the related discussion here:
>
> 	  https://lkml.org/lkml/2014/8/1/67

Ah, very interesting. Thanks for pointing that out.

> Now looking at these GIC plus extra sauce problems, I wonder whether
> this wouldn't be solvable in a similar way. If you look at it from the
> HW perspective you have:
>
>    ---------      ---------
> ---| MAGIC |------|ARM GIC|
> ---|       |------|       |
> ---|       |------|       |
> ---|       |------|       |
> ---|       |------|       |
>    ---------      ---------
>
> The MAGIC interrupt controller only provides functionality which is
> not available from the ARM architected GIC but maps 1:1 to the ARM GIC
> interrupt lines. So it looks like a variation to the more dynamic
> mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
>
> The idea is to have two irq domains: magic_domain and armgic_domain.
>
> The magic_domain is the frontend facing the devices and the
> armgic_domain is the parent domain. This is also reflected in
> hierarchical data structures, i.e. irq_desc->irq_data will get a new
> field parent_data, which points to the irq_data of the parent
> interrupt controller, which is allocated separately when the interrupt
> line is instantiated.
>
> So in the above case the hotpath ack/eoi functions of the irq chip
> associated to the device interrupt, i.e. magic_chip, would do:
>
> irq_ack(struct irq_data *d)
> {
> 	struct irq_data *pd = d->parent_data;
>
> 	pd->chip->irq_ack(pd);
> }
>
> Granted, that's another level of indirection, but this is going to be
> more efficient than a boatload of conditional hooks in the GIC code
> proper. Not to talk about the maintainability of the whole maze.
>
> The irq_set_type() function would do:
>
> irq_set_type(struct irq_data *d, int type)
> {
> 	struct irq_data *pd = d->parent_data;
>
> 	gic_type = set_magic_chip_type(d, type);
>
> 	return pd->chip->irq_set_type(d, gic_type);
> }
>
> Switching to this allows to completely avoid the gazillion of hooks in
> the gic code and should work nicely with multiplatform kernels by
> simpling hooking up the domain parent relation ship to the proper
> magic domain or leave the armgic as the direct device interface in
> case the SoC does not have the magic chip in front of the arm gic.
>
> Thoughts?

I very much like that kind of approach. Stacking domains seems to solve
a number of issues at once:

- NVIDIA's gic extension
- TI's crossbar
- ARM's GICv2m
- Mediatek's glorified line inverter
- ... and probably the next madness that's going to land tomorrow

I haven't completly groked the way we're going to allocate domains and
irq_data structures, but this is definitely something worth
investigating. I'm not too worried about the indirection either - at
least we end up with maintainable code.

We need to work out how to drive the whole stacking from a DT
perspective: Mark, any idea?

Jiang, would you mind keeping us ARM folks posted when you resend your
patch series? That'd be much appreciated.

In the meantime, I'll furbish an axe and aim it squarely at the GIC
code... ;-)

Thanks again,

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

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Marc Zyngier]

[Adding Jiang Liu to the spam-fest]

Hi Thomas,

On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 27 Aug 2014, Marc Zyngier wrote:
>> > As in the DT the actual IRQ polarity should be used, simply configuring
>> > the HW IRQ polarity in the bootloader is not enough without telling the
>> > GIC driver which polarity is supported on which IRQ, right?
>> 
>> Looking a bit closer at things, what you describe in DT is the IRQ
>> polarity the interrupt controller sees. Nothing else should interpret
>> that field.
>> 
>> So it is legal (IMO) to have a device with an interrupt specifier
>> describing a rising edge interrupt, and yet have the device generating a
>> falling edge, with Mediatek's special sauce doing the conversion in between.
>> 
>> Something will have to configure the polarity widget though, but that
>> can be left outside of the GIC.
>
> This seems to become a popular topic and it looks like the whole GIC
> extension thing is going to explode sooner than later.
>
> We are currently discussing hierarchical irq domains to solve a
> different issue in x86 land. See the related discussion here:
>
> 	  https://lkml.org/lkml/2014/8/1/67

Ah, very interesting. Thanks for pointing that out.

> Now looking at these GIC plus extra sauce problems, I wonder whether
> this wouldn't be solvable in a similar way. If you look at it from the
> HW perspective you have:
>
>    ---------      ---------
> ---| MAGIC |------|ARM GIC|
> ---|       |------|       |
> ---|       |------|       |
> ---|       |------|       |
> ---|       |------|       |
>    ---------      ---------
>
> The MAGIC interrupt controller only provides functionality which is
> not available from the ARM architected GIC but maps 1:1 to the ARM GIC
> interrupt lines. So it looks like a variation to the more dynamic
> mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
>
> The idea is to have two irq domains: magic_domain and armgic_domain.
>
> The magic_domain is the frontend facing the devices and the
> armgic_domain is the parent domain. This is also reflected in
> hierarchical data structures, i.e. irq_desc->irq_data will get a new
> field parent_data, which points to the irq_data of the parent
> interrupt controller, which is allocated separately when the interrupt
> line is instantiated.
>
> So in the above case the hotpath ack/eoi functions of the irq chip
> associated to the device interrupt, i.e. magic_chip, would do:
>
> irq_ack(struct irq_data *d)
> {
> 	struct irq_data *pd = d->parent_data;
>
> 	pd->chip->irq_ack(pd);
> }
>
> Granted, that's another level of indirection, but this is going to be
> more efficient than a boatload of conditional hooks in the GIC code
> proper. Not to talk about the maintainability of the whole maze.
>
> The irq_set_type() function would do:
>
> irq_set_type(struct irq_data *d, int type)
> {
> 	struct irq_data *pd = d->parent_data;
>
> 	gic_type = set_magic_chip_type(d, type);
>
> 	return pd->chip->irq_set_type(d, gic_type);
> }
>
> Switching to this allows to completely avoid the gazillion of hooks in
> the gic code and should work nicely with multiplatform kernels by
> simpling hooking up the domain parent relation ship to the proper
> magic domain or leave the armgic as the direct device interface in
> case the SoC does not have the magic chip in front of the arm gic.
>
> Thoughts?

I very much like that kind of approach. Stacking domains seems to solve
a number of issues at once:

- NVIDIA's gic extension
- TI's crossbar
- ARM's GICv2m
- Mediatek's glorified line inverter
- ... and probably the next madness that's going to land tomorrow

I haven't completly groked the way we're going to allocate domains and
irq_data structures, but this is definitely something worth
investigating. I'm not too worried about the indirection either - at
least we end up with maintainable code.

We need to work out how to drive the whole stacking from a DT
perspective: Mark, any idea?

Jiang, would you mind keeping us ARM folks posted when you resend your
patch series? That'd be much appreciated.

In the meantime, I'll furbish an axe and aim it squarely at the GIC
code... ;-)

Thanks again,

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

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow

Its probably already there you just don't know about it yet :)
 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth

All we have for now is a rough idea and some pseudo code in my lengthy
reply to Jiang, but it shouldn't be hard to implement something
useful.

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow

Its probably already there you just don't know about it yet :)
 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth

All we have for now is a rough idea and some pseudo code in my lengthy
reply to Jiang, but it shouldn't be hard to implement something
useful.

Thanks,

	tglx

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Marc Zyngier wrote:
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow

Its probably already there you just don't know about it yet :)
 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth

All we have for now is a rough idea and some pseudo code in my lengthy
reply to Jiang, but it shouldn't be hard to implement something
useful.

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Mark Rutland]

On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> [Adding Jiang Liu to the spam-fest]
> 
> Hi Thomas,
> 
> On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 27 Aug 2014, Marc Zyngier wrote:
> >> > As in the DT the actual IRQ polarity should be used, simply configuring
> >> > the HW IRQ polarity in the bootloader is not enough without telling the
> >> > GIC driver which polarity is supported on which IRQ, right?
> >> 
> >> Looking a bit closer at things, what you describe in DT is the IRQ
> >> polarity the interrupt controller sees. Nothing else should interpret
> >> that field.
> >> 
> >> So it is legal (IMO) to have a device with an interrupt specifier
> >> describing a rising edge interrupt, and yet have the device generating a
> >> falling edge, with Mediatek's special sauce doing the conversion in between.
> >> 
> >> Something will have to configure the polarity widget though, but that
> >> can be left outside of the GIC.
> >
> > This seems to become a popular topic and it looks like the whole GIC
> > extension thing is going to explode sooner than later.
> >
> > We are currently discussing hierarchical irq domains to solve a
> > different issue in x86 land. See the related discussion here:
> >
> > 	  https://lkml.org/lkml/2014/8/1/67
> 
> Ah, very interesting. Thanks for pointing that out.
> 
> > Now looking at these GIC plus extra sauce problems, I wonder whether
> > this wouldn't be solvable in a similar way. If you look at it from the
> > HW perspective you have:
> >
> >    ---------      ---------
> > ---| MAGIC |------|ARM GIC|
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> >    ---------      ---------
> >
> > The MAGIC interrupt controller only provides functionality which is
> > not available from the ARM architected GIC but maps 1:1 to the ARM GIC
> > interrupt lines. So it looks like a variation to the more dynamic
> > mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
> >
> > The idea is to have two irq domains: magic_domain and armgic_domain.
> >
> > The magic_domain is the frontend facing the devices and the
> > armgic_domain is the parent domain. This is also reflected in
> > hierarchical data structures, i.e. irq_desc->irq_data will get a new
> > field parent_data, which points to the irq_data of the parent
> > interrupt controller, which is allocated separately when the interrupt
> > line is instantiated.
> >
> > So in the above case the hotpath ack/eoi functions of the irq chip
> > associated to the device interrupt, i.e. magic_chip, would do:
> >
> > irq_ack(struct irq_data *d)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	pd->chip->irq_ack(pd);
> > }
> >
> > Granted, that's another level of indirection, but this is going to be
> > more efficient than a boatload of conditional hooks in the GIC code
> > proper. Not to talk about the maintainability of the whole maze.
> >
> > The irq_set_type() function would do:
> >
> > irq_set_type(struct irq_data *d, int type)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	gic_type = set_magic_chip_type(d, type);
> >
> > 	return pd->chip->irq_set_type(d, gic_type);
> > }
> >
> > Switching to this allows to completely avoid the gazillion of hooks in
> > the gic code and should work nicely with multiplatform kernels by
> > simpling hooking up the domain parent relation ship to the proper
> > magic domain or leave the armgic as the direct device interface in
> > case the SoC does not have the magic chip in front of the arm gic.
> >
> > Thoughts?
> 
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow
> 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth
> investigating. I'm not too worried about the indirection either - at
> least we end up with maintainable code.
> 
> We need to work out how to drive the whole stacking from a DT
> perspective: Mark, any idea?

Describing the lines the magic irqchip has to its parent irqchip is
simple, the standard interrupts property can handle that:

parent: parent {
	...
	#interrupt-cells = <1>;
};

magic {
	...
	interrupt-parent = <&parent>;
	interrupts = <3>, <6>, <17>, <2>;
	#interrupt-cells = <1>;
};

The harder part is describing the configuration of each interrupt to the
magic irqchip (e.g. edge vs level triggered), which is inseparable form
the line identifier in the interrupt-specifier.

If there interrupts don't share the same configuration then I'm not sure
how we describe that in the DT. That's especially problematic if the
assignment of parent line is dynamic (as with the crossbar iirc).

Mark.

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Mark Rutland]

On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> [Adding Jiang Liu to the spam-fest]
> 
> Hi Thomas,
> 
> On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 27 Aug 2014, Marc Zyngier wrote:
> >> > As in the DT the actual IRQ polarity should be used, simply configuring
> >> > the HW IRQ polarity in the bootloader is not enough without telling the
> >> > GIC driver which polarity is supported on which IRQ, right?
> >> 
> >> Looking a bit closer at things, what you describe in DT is the IRQ
> >> polarity the interrupt controller sees. Nothing else should interpret
> >> that field.
> >> 
> >> So it is legal (IMO) to have a device with an interrupt specifier
> >> describing a rising edge interrupt, and yet have the device generating a
> >> falling edge, with Mediatek's special sauce doing the conversion in between.
> >> 
> >> Something will have to configure the polarity widget though, but that
> >> can be left outside of the GIC.
> >
> > This seems to become a popular topic and it looks like the whole GIC
> > extension thing is going to explode sooner than later.
> >
> > We are currently discussing hierarchical irq domains to solve a
> > different issue in x86 land. See the related discussion here:
> >
> > 	  https://lkml.org/lkml/2014/8/1/67
> 
> Ah, very interesting. Thanks for pointing that out.
> 
> > Now looking at these GIC plus extra sauce problems, I wonder whether
> > this wouldn't be solvable in a similar way. If you look at it from the
> > HW perspective you have:
> >
> >    ---------      ---------
> > ---| MAGIC |------|ARM GIC|
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> >    ---------      ---------
> >
> > The MAGIC interrupt controller only provides functionality which is
> > not available from the ARM architected GIC but maps 1:1 to the ARM GIC
> > interrupt lines. So it looks like a variation to the more dynamic
> > mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
> >
> > The idea is to have two irq domains: magic_domain and armgic_domain.
> >
> > The magic_domain is the frontend facing the devices and the
> > armgic_domain is the parent domain. This is also reflected in
> > hierarchical data structures, i.e. irq_desc->irq_data will get a new
> > field parent_data, which points to the irq_data of the parent
> > interrupt controller, which is allocated separately when the interrupt
> > line is instantiated.
> >
> > So in the above case the hotpath ack/eoi functions of the irq chip
> > associated to the device interrupt, i.e. magic_chip, would do:
> >
> > irq_ack(struct irq_data *d)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	pd->chip->irq_ack(pd);
> > }
> >
> > Granted, that's another level of indirection, but this is going to be
> > more efficient than a boatload of conditional hooks in the GIC code
> > proper. Not to talk about the maintainability of the whole maze.
> >
> > The irq_set_type() function would do:
> >
> > irq_set_type(struct irq_data *d, int type)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	gic_type = set_magic_chip_type(d, type);
> >
> > 	return pd->chip->irq_set_type(d, gic_type);
> > }
> >
> > Switching to this allows to completely avoid the gazillion of hooks in
> > the gic code and should work nicely with multiplatform kernels by
> > simpling hooking up the domain parent relation ship to the proper
> > magic domain or leave the armgic as the direct device interface in
> > case the SoC does not have the magic chip in front of the arm gic.
> >
> > Thoughts?
> 
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow
> 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth
> investigating. I'm not too worried about the indirection either - at
> least we end up with maintainable code.
> 
> We need to work out how to drive the whole stacking from a DT
> perspective: Mark, any idea?

Describing the lines the magic irqchip has to its parent irqchip is
simple, the standard interrupts property can handle that:

parent: parent {
	...
	#interrupt-cells = <1>;
};

magic {
	...
	interrupt-parent = <&parent>;
	interrupts = <3>, <6>, <17>, <2>;
	#interrupt-cells = <1>;
};

The harder part is describing the configuration of each interrupt to the
magic irqchip (e.g. edge vs level triggered), which is inseparable form
the line identifier in the interrupt-specifier.

If there interrupts don't share the same configuration then I'm not sure
how we describe that in the DT. That's especially problematic if the
assignment of parent line is dynamic (as with the crossbar iirc).

Mark.

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Mark Rutland]

On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> [Adding Jiang Liu to the spam-fest]
> 
> Hi Thomas,
> 
> On Wed, Aug 27 2014 at  1:23:48 pm BST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 27 Aug 2014, Marc Zyngier wrote:
> >> > As in the DT the actual IRQ polarity should be used, simply configuring
> >> > the HW IRQ polarity in the bootloader is not enough without telling the
> >> > GIC driver which polarity is supported on which IRQ, right?
> >> 
> >> Looking a bit closer at things, what you describe in DT is the IRQ
> >> polarity the interrupt controller sees. Nothing else should interpret
> >> that field.
> >> 
> >> So it is legal (IMO) to have a device with an interrupt specifier
> >> describing a rising edge interrupt, and yet have the device generating a
> >> falling edge, with Mediatek's special sauce doing the conversion in between.
> >> 
> >> Something will have to configure the polarity widget though, but that
> >> can be left outside of the GIC.
> >
> > This seems to become a popular topic and it looks like the whole GIC
> > extension thing is going to explode sooner than later.
> >
> > We are currently discussing hierarchical irq domains to solve a
> > different issue in x86 land. See the related discussion here:
> >
> > 	  https://lkml.org/lkml/2014/8/1/67
> 
> Ah, very interesting. Thanks for pointing that out.
> 
> > Now looking at these GIC plus extra sauce problems, I wonder whether
> > this wouldn't be solvable in a similar way. If you look at it from the
> > HW perspective you have:
> >
> >    ---------      ---------
> > ---| MAGIC |------|ARM GIC|
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> > ---|       |------|       |
> >    ---------      ---------
> >
> > The MAGIC interrupt controller only provides functionality which is
> > not available from the ARM architected GIC but maps 1:1 to the ARM GIC
> > interrupt lines. So it looks like a variation to the more dynamic
> > mapping of MSI -> Remap -> CPU-Vector problem we need to solve on x86.
> >
> > The idea is to have two irq domains: magic_domain and armgic_domain.
> >
> > The magic_domain is the frontend facing the devices and the
> > armgic_domain is the parent domain. This is also reflected in
> > hierarchical data structures, i.e. irq_desc->irq_data will get a new
> > field parent_data, which points to the irq_data of the parent
> > interrupt controller, which is allocated separately when the interrupt
> > line is instantiated.
> >
> > So in the above case the hotpath ack/eoi functions of the irq chip
> > associated to the device interrupt, i.e. magic_chip, would do:
> >
> > irq_ack(struct irq_data *d)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	pd->chip->irq_ack(pd);
> > }
> >
> > Granted, that's another level of indirection, but this is going to be
> > more efficient than a boatload of conditional hooks in the GIC code
> > proper. Not to talk about the maintainability of the whole maze.
> >
> > The irq_set_type() function would do:
> >
> > irq_set_type(struct irq_data *d, int type)
> > {
> > 	struct irq_data *pd = d->parent_data;
> >
> > 	gic_type = set_magic_chip_type(d, type);
> >
> > 	return pd->chip->irq_set_type(d, gic_type);
> > }
> >
> > Switching to this allows to completely avoid the gazillion of hooks in
> > the gic code and should work nicely with multiplatform kernels by
> > simpling hooking up the domain parent relation ship to the proper
> > magic domain or leave the armgic as the direct device interface in
> > case the SoC does not have the magic chip in front of the arm gic.
> >
> > Thoughts?
> 
> I very much like that kind of approach. Stacking domains seems to solve
> a number of issues at once:
> 
> - NVIDIA's gic extension
> - TI's crossbar
> - ARM's GICv2m
> - Mediatek's glorified line inverter
> - ... and probably the next madness that's going to land tomorrow
> 
> I haven't completly groked the way we're going to allocate domains and
> irq_data structures, but this is definitely something worth
> investigating. I'm not too worried about the indirection either - at
> least we end up with maintainable code.
> 
> We need to work out how to drive the whole stacking from a DT
> perspective: Mark, any idea?

Describing the lines the magic irqchip has to its parent irqchip is
simple, the standard interrupts property can handle that:

parent: parent {
	...
	#interrupt-cells = <1>;
};

magic {
	...
	interrupt-parent = <&parent>;
	interrupts = <3>, <6>, <17>, <2>;
	#interrupt-cells = <1>;
};

The harder part is describing the configuration of each interrupt to the
magic irqchip (e.g. edge vs level triggered), which is inseparable form
the line identifier in the interrupt-specifier.

If there interrupts don't share the same configuration then I'm not sure
how we describe that in the DT. That's especially problematic if the
assignment of parent line is dynamic (as with the crossbar iirc).

Mark.

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Mark Rutland wrote:
> On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> > We need to work out how to drive the whole stacking from a DT
> > perspective: Mark, any idea?
> 
> Describing the lines the magic irqchip has to its parent irqchip is
> simple, the standard interrupts property can handle that:
> 
> parent: parent {
> 	...
> 	#interrupt-cells = <1>;
> };
> 
> magic {
> 	...
> 	interrupt-parent = <&parent>;
> 	interrupts = <3>, <6>, <17>, <2>;
> 	#interrupt-cells = <1>;
> };
> 
> The harder part is describing the configuration of each interrupt to the
> magic irqchip (e.g. edge vs level triggered), which is inseparable form
> the line identifier in the interrupt-specifier.

Do we really need to decribe that at the this level?

You have the parent ARMGIC and the maGIC with a specified (or even
dynamic) routing of the maGIC lines to the ARMGIC.

Now the device which uses a maGIC interrupt specifies the interrupt
type at the maGIC.

So the type setter function of the maGIC configures the maGIC hardware
in such a way that the output to the ARMGIC results in a type which
the ARMGIC can handle and calls the type setter function of the maGIC
with that type.

I don't think you need to decribe this in the magic/parent relation
ship at all. maGIC should know what kind of output it can provide to
ARMGIC so that should just work, unless I'm missing something.

> If there interrupts don't share the same configuration then I'm not sure
> how we describe that in the DT. That's especially problematic if the
> assignment of parent line is dynamic (as with the crossbar iirc).

Why is this an issue?

There are two ways to solve that:

1) You can do a fully dynamic allocation at the parent level, which of
   course requires that the whole parent range is dynamically
   managed. That's what we are planning for the MSI/Remap/Vector
   stacking

2) You define the irq lines at the parent which are available for
   dynamic stacking and let the allocation code hand one out.

3) You tell the maGIC controller which lines of the parent are
   available for it, so it can only stack onto those lines and instead
   of letting the parent allocate a free one, the maGIC needs to map
   its stuff to one of those ARMGIC lines.

Which one to chose depends on the particular maGIC incarnation, but
its not a big issue to select one ...

Thanks,

	tglx

Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Mark Rutland wrote:
> On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> > We need to work out how to drive the whole stacking from a DT
> > perspective: Mark, any idea?
> 
> Describing the lines the magic irqchip has to its parent irqchip is
> simple, the standard interrupts property can handle that:
> 
> parent: parent {
> 	...
> 	#interrupt-cells = <1>;
> };
> 
> magic {
> 	...
> 	interrupt-parent = <&parent>;
> 	interrupts = <3>, <6>, <17>, <2>;
> 	#interrupt-cells = <1>;
> };
> 
> The harder part is describing the configuration of each interrupt to the
> magic irqchip (e.g. edge vs level triggered), which is inseparable form
> the line identifier in the interrupt-specifier.

Do we really need to decribe that at the this level?

You have the parent ARMGIC and the maGIC with a specified (or even
dynamic) routing of the maGIC lines to the ARMGIC.

Now the device which uses a maGIC interrupt specifies the interrupt
type at the maGIC.

So the type setter function of the maGIC configures the maGIC hardware
in such a way that the output to the ARMGIC results in a type which
the ARMGIC can handle and calls the type setter function of the maGIC
with that type.

I don't think you need to decribe this in the magic/parent relation
ship at all. maGIC should know what kind of output it can provide to
ARMGIC so that should just work, unless I'm missing something.

> If there interrupts don't share the same configuration then I'm not sure
> how we describe that in the DT. That's especially problematic if the
> assignment of parent line is dynamic (as with the crossbar iirc).

Why is this an issue?

There are two ways to solve that:

1) You can do a fully dynamic allocation at the parent level, which of
   course requires that the whole parent range is dynamically
   managed. That's what we are planning for the MSI/Remap/Vector
   stacking

2) You define the irq lines at the parent which are available for
   dynamic stacking and let the allocation code hand one out.

3) You tell the maGIC controller which lines of the parent are
   available for it, so it can only stack onto those lines and instead
   of letting the parent allocate a free one, the maGIC needs to map
   its stuff to one of those ARMGIC lines.

Which one to chose depends on the particular maGIC incarnation, but
its not a big issue to select one ...

Thanks,

	tglx

[RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present

[Thomas Gleixner]

On Wed, 27 Aug 2014, Mark Rutland wrote:
> On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
> > We need to work out how to drive the whole stacking from a DT
> > perspective: Mark, any idea?
> 
> Describing the lines the magic irqchip has to its parent irqchip is
> simple, the standard interrupts property can handle that:
> 
> parent: parent {
> 	...
> 	#interrupt-cells = <1>;
> };
> 
> magic {
> 	...
> 	interrupt-parent = <&parent>;
> 	interrupts = <3>, <6>, <17>, <2>;
> 	#interrupt-cells = <1>;
> };
> 
> The harder part is describing the configuration of each interrupt to the
> magic irqchip (e.g. edge vs level triggered), which is inseparable form
> the line identifier in the interrupt-specifier.

Do we really need to decribe that at the this level?

You have the parent ARMGIC and the maGIC with a specified (or even
dynamic) routing of the maGIC lines to the ARMGIC.

Now the device which uses a maGIC interrupt specifies the interrupt
type at the maGIC.

So the type setter function of the maGIC configures the maGIC hardware
in such a way that the output to the ARMGIC results in a type which
the ARMGIC can handle and calls the type setter function of the maGIC
with that type.

I don't think you need to decribe this in the magic/parent relation
ship at all. maGIC should know what kind of output it can provide to
ARMGIC so that should just work, unless I'm missing something.

> If there interrupts don't share the same configuration then I'm not sure
> how we describe that in the DT. That's especially problematic if the
> assignment of parent line is dynamic (as with the crossbar iirc).

Why is this an issue?

There are two ways to solve that:

1) You can do a fully dynamic allocation at the parent level, which of
   course requires that the whole parent range is dynamically
   managed. That's what we are planning for the MSI/Remap/Vector
   stacking

2) You define the irq lines at the parent which are available for
   dynamic stacking and let the allocation code hand one out.

3) You tell the maGIC controller which lines of the parent are
   available for it, so it can only stack onto those lines and instead
   of letting the parent allocate a free one, the maGIC needs to map
   its stuff to one of those ARMGIC lines.

Which one to chose depends on the particular maGIC incarnation, but
its not a big issue to select one ...

Thanks,

	tglx