All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/5] of: Add NVIDIA Tegra Legacy Interrupt Controller binding
@ 2014-08-28 15:31 ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The Legacy Interrupt Controller found on NVIDIA Tegra SoCs is used by
the AVP coprocessor and can also serve as a backup for the ARM Cortex
CPU's local interrupt controller (GIC).

The LIC is subdivided into multiple identical units, each handling 32
possible interrupt sources.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- bracket individual tuples in the "reg" property

 .../interrupt-controller/nvidia,tegra20-ictlr.txt     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt
new file mode 100644
index 000000000000..1639389b7360
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt
@@ -0,0 +1,19 @@
+NVIDIA Tegra Legacy Interrupt Controller
+
+The legacy interrupt controller is divided into units that serve 32 interrupts
+each. Tegra20 implements four units, whereas Tegra30 and later implement five.
+
+Required properties:
+- compatible: "nvidia,tegra<chip>-ictlr"
+- reg: Physical base address and length of the controller's registers. There
+  should be one entry for each unit.
+
+Example:
+
+	interrupt-controller@60004000 {
+		compatible = "nvidia,tegra20-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>; /* quaternary controller */
+	};
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 1/5] of: Add NVIDIA Tegra Legacy Interrupt Controller binding
@ 2014-08-28 15:31 ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

The Legacy Interrupt Controller found on NVIDIA Tegra SoCs is used by
the AVP coprocessor and can also serve as a backup for the ARM Cortex
CPU's local interrupt controller (GIC).

The LIC is subdivided into multiple identical units, each handling 32
possible interrupt sources.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- bracket individual tuples in the "reg" property

 .../interrupt-controller/nvidia,tegra20-ictlr.txt     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt
new file mode 100644
index 000000000000..1639389b7360
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/nvidia,tegra20-ictlr.txt
@@ -0,0 +1,19 @@
+NVIDIA Tegra Legacy Interrupt Controller
+
+The legacy interrupt controller is divided into units that serve 32 interrupts
+each. Tegra20 implements four units, whereas Tegra30 and later implement five.
+
+Required properties:
+- compatible: "nvidia,tegra<chip>-ictlr"
+- reg: Physical base address and length of the controller's registers. There
+  should be one entry for each unit.
+
+Example:
+
+	interrupt-controller at 60004000 {
+		compatible = "nvidia,tegra20-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>; /* quaternary controller */
+	};
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
  2014-08-28 15:31 ` Thierry Reding
@ 2014-08-28 15:31     ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Add device tree nodes for the legacy interrupt controller so that the
driver can get the register ranges from device tree rather than hard-
coding them.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- bracket individual tuples in the "reg" property

Changes in v2:
- add chip-specific compatible string
- drop quinary controller on Tegra20

 arch/arm/boot/dts/tegra114.dtsi | 9 +++++++++
 arch/arm/boot/dts/tegra124.dtsi | 9 +++++++++
 arch/arm/boot/dts/tegra20.dtsi  | 8 ++++++++
 arch/arm/boot/dts/tegra30.dtsi  | 9 +++++++++
 4 files changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index a147fa2bfdd2..9bfab8bb765a 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -141,6 +141,15 @@
 			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 	};
 
+	interrupt-controller@60004000 {
+		compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>, /* quaternary controller */
+		      <0x60004400 0x40>; /* quinary controller */
+	};
+
 	timer@60005000 {
 		compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
 		reg = <0x60005000 0x400>;
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 82dc48b530e6..8c7523225fd9 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -190,6 +190,15 @@
 		status = "disabled";
 	};
 
+	interrupt-controller@0,60004000 {
+		compatible = "nvidia,tegra124-ictlr", "nvidia,tegra30-ictlr";
+		reg = <0x0 0x60004000 0x0 0x40>, /* primary controller */
+		      <0x0 0x60004100 0x0 0x40>, /* secondary controller */
+		      <0x0 0x60004200 0x0 0x40>, /* tertiary controller */
+		      <0x0 0x60004300 0x0 0x40>, /* quaternary controller */
+		      <0x0 0x60004400 0x0 0x40>; /* quinary controller */
+	};
+
 	timer@0,60005000 {
 		compatible = "nvidia,tegra124-timer", "nvidia,tegra20-timer";
 		reg = <0x0 0x60005000 0x0 0x400>;
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index c6a2d078bdf4..fe2f57d19438 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -183,6 +183,14 @@
 		cache-level = <2>;
 	};
 
+	interrupt-controller@60004000 {
+		compatible = "nvidia,tegra20-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>; /* quaternary controller */
+	};
+
 	timer@60005000 {
 		compatible = "nvidia,tegra20-timer";
 		reg = <0x60005000 0x60>;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index f4693c9c070e..e5da2d252220 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -262,6 +262,15 @@
 		cache-level = <2>;
 	};
 
+	interrupt-controller@60004000 {
+		compatible = "nvidia,tegra30-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>, /* quaternary controller */
+		      <0x60004400 0x40>; /* quinary controller */
+	};
+
 	timer@60005000 {
 		compatible = "nvidia,tegra30-timer", "nvidia,tegra20-timer";
 		reg = <0x60005000 0x400>;
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
@ 2014-08-28 15:31     ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

Add device tree nodes for the legacy interrupt controller so that the
driver can get the register ranges from device tree rather than hard-
coding them.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- bracket individual tuples in the "reg" property

Changes in v2:
- add chip-specific compatible string
- drop quinary controller on Tegra20

 arch/arm/boot/dts/tegra114.dtsi | 9 +++++++++
 arch/arm/boot/dts/tegra124.dtsi | 9 +++++++++
 arch/arm/boot/dts/tegra20.dtsi  | 8 ++++++++
 arch/arm/boot/dts/tegra30.dtsi  | 9 +++++++++
 4 files changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index a147fa2bfdd2..9bfab8bb765a 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -141,6 +141,15 @@
 			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 	};
 
+	interrupt-controller at 60004000 {
+		compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>, /* quaternary controller */
+		      <0x60004400 0x40>; /* quinary controller */
+	};
+
 	timer at 60005000 {
 		compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
 		reg = <0x60005000 0x400>;
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 82dc48b530e6..8c7523225fd9 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -190,6 +190,15 @@
 		status = "disabled";
 	};
 
+	interrupt-controller at 0,60004000 {
+		compatible = "nvidia,tegra124-ictlr", "nvidia,tegra30-ictlr";
+		reg = <0x0 0x60004000 0x0 0x40>, /* primary controller */
+		      <0x0 0x60004100 0x0 0x40>, /* secondary controller */
+		      <0x0 0x60004200 0x0 0x40>, /* tertiary controller */
+		      <0x0 0x60004300 0x0 0x40>, /* quaternary controller */
+		      <0x0 0x60004400 0x0 0x40>; /* quinary controller */
+	};
+
 	timer at 0,60005000 {
 		compatible = "nvidia,tegra124-timer", "nvidia,tegra20-timer";
 		reg = <0x0 0x60005000 0x0 0x400>;
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index c6a2d078bdf4..fe2f57d19438 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -183,6 +183,14 @@
 		cache-level = <2>;
 	};
 
+	interrupt-controller at 60004000 {
+		compatible = "nvidia,tegra20-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>; /* quaternary controller */
+	};
+
 	timer at 60005000 {
 		compatible = "nvidia,tegra20-timer";
 		reg = <0x60005000 0x60>;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index f4693c9c070e..e5da2d252220 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -262,6 +262,15 @@
 		cache-level = <2>;
 	};
 
+	interrupt-controller at 60004000 {
+		compatible = "nvidia,tegra30-ictlr";
+		reg = <0x60004000 0x40>, /* primary controller */
+		      <0x60004100 0x40>, /* secondary controller */
+		      <0x60004200 0x40>, /* tertiary controller */
+		      <0x60004300 0x40>, /* quaternary controller */
+		      <0x60004400 0x40>; /* quinary controller */
+	};
+
 	timer at 60005000 {
 		compatible = "nvidia,tegra30-timer", "nvidia,tegra20-timer";
 		reg = <0x60005000 0x400>;
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-28 15:31 ` Thierry Reding
@ 2014-08-28 15:31     ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Obtains the register ranges for the legacy interrupt controller from DT
and provide hard-coded values as fallback.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v4:
- check for Tegra20 in non-DT fallback using of_machine_is_compatible()
- cleanup warning messages

Changes in v3:
- fixup subject prefix

Changes in v2:
- check for the exact number of controllers expected
- fallback to tegra_chip_id for non-DT
- warn on parsing errors

 arch/arm/mach-tegra/iomap.h |  18 -------
 arch/arm/mach-tegra/irq.c   | 122 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 101 insertions(+), 39 deletions(-)

diff --git a/arch/arm/mach-tegra/iomap.h b/arch/arm/mach-tegra/iomap.h
index ee79808e93a3..52bbb5c8fe84 100644
--- a/arch/arm/mach-tegra/iomap.h
+++ b/arch/arm/mach-tegra/iomap.h
@@ -28,24 +28,6 @@
 #define TEGRA_ARM_PERIF_BASE		0x50040000
 #define TEGRA_ARM_PERIF_SIZE		SZ_8K
 
-#define TEGRA_ARM_INT_DIST_BASE		0x50041000
-#define TEGRA_ARM_INT_DIST_SIZE		SZ_4K
-
-#define TEGRA_PRIMARY_ICTLR_BASE	0x60004000
-#define TEGRA_PRIMARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_SECONDARY_ICTLR_BASE	0x60004100
-#define TEGRA_SECONDARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_TERTIARY_ICTLR_BASE	0x60004200
-#define TEGRA_TERTIARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_QUATERNARY_ICTLR_BASE	0x60004300
-#define TEGRA_QUATERNARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_QUINARY_ICTLR_BASE	0x60004400
-#define TEGRA_QUINARY_ICTLR_SIZE	SZ_64
-
 #define TEGRA_TMR1_BASE			0x60005000
 #define TEGRA_TMR1_SIZE			SZ_8
 
diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index da7be13aecce..636ebfd466cb 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -27,8 +27,7 @@
 #include <linux/of.h>
 #include <linux/syscore_ops.h>
 
-#include "board.h"
-#include "iomap.h"
+#include <soc/tegra/fuse.h>
 
 #define ICTLR_CPU_IEP_VFIQ	0x08
 #define ICTLR_CPU_IEP_FIR	0x14
@@ -52,13 +51,7 @@
 
 static int num_ictlrs;
 
-static void __iomem *ictlr_reg_base[] = {
-	IO_ADDRESS(TEGRA_PRIMARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_SECONDARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_TERTIARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_QUATERNARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_QUINARY_ICTLR_BASE),
-};
+static void __iomem *ictlr_reg_base[] = { NULL, NULL, NULL, NULL, NULL };
 
 #ifdef CONFIG_PM_SLEEP
 static u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
@@ -70,10 +63,11 @@ static u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
 static void __iomem *tegra_gic_cpu_base;
 #endif
 
+static void __iomem *distbase;
+
 bool tegra_pending_sgi(void)
 {
 	u32 pending_set;
-	void __iomem *distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
 
 	pending_set = readl_relaxed(distbase + GIC_DIST_PENDING_SET);
 
@@ -255,24 +249,109 @@ static void tegra114_gic_cpu_pm_registration(void)
 static void tegra114_gic_cpu_pm_registration(void) { }
 #endif
 
+static struct resource ictlr_regs[] = {
+	{ .start = 0x60004000, .end = 0x6000403f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004100, .end = 0x6000413f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004200, .end = 0x6000423f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004300, .end = 0x6000433f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004400, .end = 0x6000443f, .flags = IORESOURCE_MEM },
+};
+
+struct tegra_ictlr_soc {
+	unsigned int num_ictlrs;
+};
+
+static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
+	.num_ictlrs = 4,
+};
+
+static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
+	.num_ictlrs = 5,
+};
+
+static const struct of_device_id ictlr_matches[] = {
+	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
+	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
+	{ }
+};
+
+static const struct of_device_id gic_matches[] = {
+	{ .compatible = "arm,cortex-a15-gic", },
+	{ .compatible = "arm,cortex-a9-gic", },
+	{ }
+};
+
 void __init tegra_init_irq(void)
 {
-	int i;
-	void __iomem *distbase;
+	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
+	const struct of_device_id *match;
+	struct device_node *np;
+	struct resource res;
+
+	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
+	if (np) {
+		const struct tegra_ictlr_soc *soc = match->data;
+
+		for (i = 0; i < soc->num_ictlrs; i++) {
+			if (of_address_to_resource(np, i, &res) < 0)
+				break;
+
+			ictlr_regs[i] = res;
+		}
+
+		WARN(i != soc->num_ictlrs,
+		     "Found %u interrupt controllers in DT; expected %u.\n",
+		     i, soc->num_ictlrs);
+
+		max_ictlrs = soc->num_ictlrs;
+		of_node_put(np);
+	} else {
+		/*
+		 * If no matching device node was found, fall back to using
+		 * the chip ID.
+		 */
+
+		/* Tegra30 and later have five interrupt controllers, ... */
+		max_ictlrs = ARRAY_SIZE(ictlr_regs);
+
+		/* ..., but Tegra20 only has four. */
+		if (of_machine_is_compatible("nvidia,tegra20"))
+			max_ictlrs--;
+	}
 
-	distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
+	memset(&res, 0, sizeof(res));
+
+	np = of_find_matching_node(NULL, gic_matches);
+	if (np) {
+		if (of_address_to_resource(np, 0, &res) < 0)
+			WARN(1, "GIC registers are missing from DT\n");
+
+		of_node_put(np);
+	}
+
+	if (res.start == 0 || res.end == 0) {
+		res.start = 0x50041000;
+		res.end = 0x50041fff;
+		res.flags = IORESOURCE_MEM;
+	}
+
+	distbase = ioremap_nocache(res.start, resource_size(&res));
 	num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;
 
-	if (num_ictlrs > ARRAY_SIZE(ictlr_reg_base)) {
-		WARN(1, "Too many (%d) interrupt controllers found. Maximum is %d.",
-			num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
-		num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
+	if (num_ictlrs != max_ictlrs) {
+		WARN(1, "Found %u interrupt controllers; expected %u.\n",
+		     num_ictlrs, max_ictlrs);
+		num_ictlrs = max_ictlrs;
 	}
 
 	for (i = 0; i < num_ictlrs; i++) {
-		void __iomem *ictlr = ictlr_reg_base[i];
+		struct resource *regs = &ictlr_regs[i];
+		void __iomem *ictlr;
+
+		ictlr = ioremap_nocache(regs->start, resource_size(regs));
 		writel(~0, ictlr + ICTLR_CPU_IER_CLR);
 		writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
+		ictlr_reg_base[i] = ictlr;
 	}
 
 	gic_arch_extn.irq_ack = tegra_ack;
@@ -287,9 +366,10 @@ void __init tegra_init_irq(void)
 	 * Check if there is a devicetree present, since the GIC will be
 	 * initialized elsewhere under DT.
 	 */
-	if (!of_have_populated_dt())
-		gic_init(0, 29, distbase,
-			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+	if (!of_have_populated_dt()) {
+		void __iomem *cpubase = ioremap_nocache(0x50040000, 0x2000);
+		gic_init(0, 29, distbase, cpubase);
+	}
 
 	tegra114_gic_cpu_pm_registration();
 }
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-28 15:31     ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

Obtains the register ranges for the legacy interrupt controller from DT
and provide hard-coded values as fallback.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v4:
- check for Tegra20 in non-DT fallback using of_machine_is_compatible()
- cleanup warning messages

Changes in v3:
- fixup subject prefix

Changes in v2:
- check for the exact number of controllers expected
- fallback to tegra_chip_id for non-DT
- warn on parsing errors

 arch/arm/mach-tegra/iomap.h |  18 -------
 arch/arm/mach-tegra/irq.c   | 122 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 101 insertions(+), 39 deletions(-)

diff --git a/arch/arm/mach-tegra/iomap.h b/arch/arm/mach-tegra/iomap.h
index ee79808e93a3..52bbb5c8fe84 100644
--- a/arch/arm/mach-tegra/iomap.h
+++ b/arch/arm/mach-tegra/iomap.h
@@ -28,24 +28,6 @@
 #define TEGRA_ARM_PERIF_BASE		0x50040000
 #define TEGRA_ARM_PERIF_SIZE		SZ_8K
 
-#define TEGRA_ARM_INT_DIST_BASE		0x50041000
-#define TEGRA_ARM_INT_DIST_SIZE		SZ_4K
-
-#define TEGRA_PRIMARY_ICTLR_BASE	0x60004000
-#define TEGRA_PRIMARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_SECONDARY_ICTLR_BASE	0x60004100
-#define TEGRA_SECONDARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_TERTIARY_ICTLR_BASE	0x60004200
-#define TEGRA_TERTIARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_QUATERNARY_ICTLR_BASE	0x60004300
-#define TEGRA_QUATERNARY_ICTLR_SIZE	SZ_64
-
-#define TEGRA_QUINARY_ICTLR_BASE	0x60004400
-#define TEGRA_QUINARY_ICTLR_SIZE	SZ_64
-
 #define TEGRA_TMR1_BASE			0x60005000
 #define TEGRA_TMR1_SIZE			SZ_8
 
diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index da7be13aecce..636ebfd466cb 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -27,8 +27,7 @@
 #include <linux/of.h>
 #include <linux/syscore_ops.h>
 
-#include "board.h"
-#include "iomap.h"
+#include <soc/tegra/fuse.h>
 
 #define ICTLR_CPU_IEP_VFIQ	0x08
 #define ICTLR_CPU_IEP_FIR	0x14
@@ -52,13 +51,7 @@
 
 static int num_ictlrs;
 
-static void __iomem *ictlr_reg_base[] = {
-	IO_ADDRESS(TEGRA_PRIMARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_SECONDARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_TERTIARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_QUATERNARY_ICTLR_BASE),
-	IO_ADDRESS(TEGRA_QUINARY_ICTLR_BASE),
-};
+static void __iomem *ictlr_reg_base[] = { NULL, NULL, NULL, NULL, NULL };
 
 #ifdef CONFIG_PM_SLEEP
 static u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
@@ -70,10 +63,11 @@ static u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
 static void __iomem *tegra_gic_cpu_base;
 #endif
 
+static void __iomem *distbase;
+
 bool tegra_pending_sgi(void)
 {
 	u32 pending_set;
-	void __iomem *distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
 
 	pending_set = readl_relaxed(distbase + GIC_DIST_PENDING_SET);
 
@@ -255,24 +249,109 @@ static void tegra114_gic_cpu_pm_registration(void)
 static void tegra114_gic_cpu_pm_registration(void) { }
 #endif
 
+static struct resource ictlr_regs[] = {
+	{ .start = 0x60004000, .end = 0x6000403f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004100, .end = 0x6000413f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004200, .end = 0x6000423f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004300, .end = 0x6000433f, .flags = IORESOURCE_MEM },
+	{ .start = 0x60004400, .end = 0x6000443f, .flags = IORESOURCE_MEM },
+};
+
+struct tegra_ictlr_soc {
+	unsigned int num_ictlrs;
+};
+
+static const struct tegra_ictlr_soc tegra20_ictlr_soc = {
+	.num_ictlrs = 4,
+};
+
+static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
+	.num_ictlrs = 5,
+};
+
+static const struct of_device_id ictlr_matches[] = {
+	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
+	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
+	{ }
+};
+
+static const struct of_device_id gic_matches[] = {
+	{ .compatible = "arm,cortex-a15-gic", },
+	{ .compatible = "arm,cortex-a9-gic", },
+	{ }
+};
+
 void __init tegra_init_irq(void)
 {
-	int i;
-	void __iomem *distbase;
+	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
+	const struct of_device_id *match;
+	struct device_node *np;
+	struct resource res;
+
+	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
+	if (np) {
+		const struct tegra_ictlr_soc *soc = match->data;
+
+		for (i = 0; i < soc->num_ictlrs; i++) {
+			if (of_address_to_resource(np, i, &res) < 0)
+				break;
+
+			ictlr_regs[i] = res;
+		}
+
+		WARN(i != soc->num_ictlrs,
+		     "Found %u interrupt controllers in DT; expected %u.\n",
+		     i, soc->num_ictlrs);
+
+		max_ictlrs = soc->num_ictlrs;
+		of_node_put(np);
+	} else {
+		/*
+		 * If no matching device node was found, fall back to using
+		 * the chip ID.
+		 */
+
+		/* Tegra30 and later have five interrupt controllers, ... */
+		max_ictlrs = ARRAY_SIZE(ictlr_regs);
+
+		/* ..., but Tegra20 only has four. */
+		if (of_machine_is_compatible("nvidia,tegra20"))
+			max_ictlrs--;
+	}
 
-	distbase = IO_ADDRESS(TEGRA_ARM_INT_DIST_BASE);
+	memset(&res, 0, sizeof(res));
+
+	np = of_find_matching_node(NULL, gic_matches);
+	if (np) {
+		if (of_address_to_resource(np, 0, &res) < 0)
+			WARN(1, "GIC registers are missing from DT\n");
+
+		of_node_put(np);
+	}
+
+	if (res.start == 0 || res.end == 0) {
+		res.start = 0x50041000;
+		res.end = 0x50041fff;
+		res.flags = IORESOURCE_MEM;
+	}
+
+	distbase = ioremap_nocache(res.start, resource_size(&res));
 	num_ictlrs = readl_relaxed(distbase + GIC_DIST_CTR) & 0x1f;
 
-	if (num_ictlrs > ARRAY_SIZE(ictlr_reg_base)) {
-		WARN(1, "Too many (%d) interrupt controllers found. Maximum is %d.",
-			num_ictlrs, ARRAY_SIZE(ictlr_reg_base));
-		num_ictlrs = ARRAY_SIZE(ictlr_reg_base);
+	if (num_ictlrs != max_ictlrs) {
+		WARN(1, "Found %u interrupt controllers; expected %u.\n",
+		     num_ictlrs, max_ictlrs);
+		num_ictlrs = max_ictlrs;
 	}
 
 	for (i = 0; i < num_ictlrs; i++) {
-		void __iomem *ictlr = ictlr_reg_base[i];
+		struct resource *regs = &ictlr_regs[i];
+		void __iomem *ictlr;
+
+		ictlr = ioremap_nocache(regs->start, resource_size(regs));
 		writel(~0, ictlr + ICTLR_CPU_IER_CLR);
 		writel(0, ictlr + ICTLR_CPU_IEP_CLASS);
+		ictlr_reg_base[i] = ictlr;
 	}
 
 	gic_arch_extn.irq_ack = tegra_ack;
@@ -287,9 +366,10 @@ void __init tegra_init_irq(void)
 	 * Check if there is a devicetree present, since the GIC will be
 	 * initialized elsewhere under DT.
 	 */
-	if (!of_have_populated_dt())
-		gic_init(0, 29, distbase,
-			IO_ADDRESS(TEGRA_ARM_PERIF_BASE + 0x100));
+	if (!of_have_populated_dt()) {
+		void __iomem *cpubase = ioremap_nocache(0x50040000, 0x2000);
+		gic_init(0, 29, distbase, cpubase);
+	}
 
 	tegra114_gic_cpu_pm_registration();
 }
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 4/5] ARM: tegra: Remove unused GIC initialization
  2014-08-28 15:31 ` Thierry Reding
@ 2014-08-28 15:31     ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Tegra has been booting from DT exclusively for quite some time, so this
old code setting up the GIC for non-DT boot is unused and can therefore
be removed.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/irq.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 636ebfd466cb..b1760aa162f1 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -362,14 +362,5 @@ void __init tegra_init_irq(void)
 	gic_arch_extn.irq_set_wake = tegra_set_wake;
 	gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 
-	/*
-	 * Check if there is a devicetree present, since the GIC will be
-	 * initialized elsewhere under DT.
-	 */
-	if (!of_have_populated_dt()) {
-		void __iomem *cpubase = ioremap_nocache(0x50040000, 0x2000);
-		gic_init(0, 29, distbase, cpubase);
-	}
-
 	tegra114_gic_cpu_pm_registration();
 }
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 4/5] ARM: tegra: Remove unused GIC initialization
@ 2014-08-28 15:31     ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

Tegra has been booting from DT exclusively for quite some time, so this
old code setting up the GIC for non-DT boot is unused and can therefore
be removed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm/mach-tegra/irq.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/arm/mach-tegra/irq.c b/arch/arm/mach-tegra/irq.c
index 636ebfd466cb..b1760aa162f1 100644
--- a/arch/arm/mach-tegra/irq.c
+++ b/arch/arm/mach-tegra/irq.c
@@ -362,14 +362,5 @@ void __init tegra_init_irq(void)
 	gic_arch_extn.irq_set_wake = tegra_set_wake;
 	gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
 
-	/*
-	 * Check if there is a devicetree present, since the GIC will be
-	 * initialized elsewhere under DT.
-	 */
-	if (!of_have_populated_dt()) {
-		void __iomem *cpubase = ioremap_nocache(0x50040000, 0x2000);
-		gic_init(0, 29, distbase, cpubase);
-	}
-
 	tegra114_gic_cpu_pm_registration();
 }
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 5/5] ARM: tegra: Remove unused defines
  2014-08-28 15:31 ` Thierry Reding
@ 2014-08-28 15:31     ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Eventually the aim is to get rid of this file because it cannot be built
on ARM64. Until it can be completely removed, eliminate unused entries
to make it easier to see which are still used.

Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v3:
- fixup subject prefix

 arch/arm/mach-tegra/iomap.h | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/arch/arm/mach-tegra/iomap.h b/arch/arm/mach-tegra/iomap.h
index 52bbb5c8fe84..c2fd86a9b43f 100644
--- a/arch/arm/mach-tegra/iomap.h
+++ b/arch/arm/mach-tegra/iomap.h
@@ -28,21 +28,9 @@
 #define TEGRA_ARM_PERIF_BASE		0x50040000
 #define TEGRA_ARM_PERIF_SIZE		SZ_8K
 
-#define TEGRA_TMR1_BASE			0x60005000
-#define TEGRA_TMR1_SIZE			SZ_8
-
-#define TEGRA_TMR2_BASE			0x60005008
-#define TEGRA_TMR2_SIZE			SZ_8
-
 #define TEGRA_TMRUS_BASE		0x60005010
 #define TEGRA_TMRUS_SIZE		SZ_64
 
-#define TEGRA_TMR3_BASE			0x60005050
-#define TEGRA_TMR3_SIZE			SZ_8
-
-#define TEGRA_TMR4_BASE			0x60005058
-#define TEGRA_TMR4_SIZE			SZ_8
-
 #define TEGRA_CLK_RESET_BASE		0x60006000
 #define TEGRA_CLK_RESET_SIZE		SZ_4K
 
@@ -58,21 +46,6 @@
 #define TEGRA_APB_MISC_BASE		0x70000000
 #define TEGRA_APB_MISC_SIZE		SZ_4K
 
-#define TEGRA_UARTA_BASE		0x70006000
-#define TEGRA_UARTA_SIZE		SZ_64
-
-#define TEGRA_UARTB_BASE		0x70006040
-#define TEGRA_UARTB_SIZE		SZ_64
-
-#define TEGRA_UARTC_BASE		0x70006200
-#define TEGRA_UARTC_SIZE		SZ_256
-
-#define TEGRA_UARTD_BASE		0x70006300
-#define TEGRA_UARTD_SIZE		SZ_256
-
-#define TEGRA_UARTE_BASE		0x70006400
-#define TEGRA_UARTE_SIZE		SZ_256
-
 #define TEGRA_PMC_BASE			0x7000E400
 #define TEGRA_PMC_SIZE			SZ_256
 
@@ -91,9 +64,6 @@
 #define TEGRA124_EMC_BASE		0x7001B000
 #define TEGRA124_EMC_SIZE		SZ_2K
 
-#define TEGRA_CSITE_BASE		0x70040000
-#define TEGRA_CSITE_SIZE		SZ_256K
-
 /* On TEGRA, many peripherals are very closely packed in
  * two 256MB io windows (that actually only use about 64KB
  * at the start of each).
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH v4 5/5] ARM: tegra: Remove unused defines
@ 2014-08-28 15:31     ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-28 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

From: Thierry Reding <treding@nvidia.com>

Eventually the aim is to get rid of this file because it cannot be built
on ARM64. Until it can be completely removed, eliminate unused entries
to make it easier to see which are still used.

Reviewed-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- fixup subject prefix

 arch/arm/mach-tegra/iomap.h | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/arch/arm/mach-tegra/iomap.h b/arch/arm/mach-tegra/iomap.h
index 52bbb5c8fe84..c2fd86a9b43f 100644
--- a/arch/arm/mach-tegra/iomap.h
+++ b/arch/arm/mach-tegra/iomap.h
@@ -28,21 +28,9 @@
 #define TEGRA_ARM_PERIF_BASE		0x50040000
 #define TEGRA_ARM_PERIF_SIZE		SZ_8K
 
-#define TEGRA_TMR1_BASE			0x60005000
-#define TEGRA_TMR1_SIZE			SZ_8
-
-#define TEGRA_TMR2_BASE			0x60005008
-#define TEGRA_TMR2_SIZE			SZ_8
-
 #define TEGRA_TMRUS_BASE		0x60005010
 #define TEGRA_TMRUS_SIZE		SZ_64
 
-#define TEGRA_TMR3_BASE			0x60005050
-#define TEGRA_TMR3_SIZE			SZ_8
-
-#define TEGRA_TMR4_BASE			0x60005058
-#define TEGRA_TMR4_SIZE			SZ_8
-
 #define TEGRA_CLK_RESET_BASE		0x60006000
 #define TEGRA_CLK_RESET_SIZE		SZ_4K
 
@@ -58,21 +46,6 @@
 #define TEGRA_APB_MISC_BASE		0x70000000
 #define TEGRA_APB_MISC_SIZE		SZ_4K
 
-#define TEGRA_UARTA_BASE		0x70006000
-#define TEGRA_UARTA_SIZE		SZ_64
-
-#define TEGRA_UARTB_BASE		0x70006040
-#define TEGRA_UARTB_SIZE		SZ_64
-
-#define TEGRA_UARTC_BASE		0x70006200
-#define TEGRA_UARTC_SIZE		SZ_256
-
-#define TEGRA_UARTD_BASE		0x70006300
-#define TEGRA_UARTD_SIZE		SZ_256
-
-#define TEGRA_UARTE_BASE		0x70006400
-#define TEGRA_UARTE_SIZE		SZ_256
-
 #define TEGRA_PMC_BASE			0x7000E400
 #define TEGRA_PMC_SIZE			SZ_256
 
@@ -91,9 +64,6 @@
 #define TEGRA124_EMC_BASE		0x7001B000
 #define TEGRA124_EMC_SIZE		SZ_2K
 
-#define TEGRA_CSITE_BASE		0x70040000
-#define TEGRA_CSITE_SIZE		SZ_256K
-
 /* On TEGRA, many peripherals are very closely packed in
  * two 256MB io windows (that actually only use about 64KB
  * at the start of each).
-- 
2.0.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-28 15:31     ` Thierry Reding
@ 2014-08-28 16:10         ` Arnd Bergmann
  -1 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-28 16:10 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Thierry Reding, Stephen Warren,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:

>  void __init tegra_init_irq(void)
>  {
> -	int i;
> -	void __iomem *distbase;
> +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> +	const struct of_device_id *match;
> +	struct device_node *np;
> +	struct resource res;
> +
> +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> +	if (np) {
> +		const struct tegra_ictlr_soc *soc = match->data;
> +
> +		for (i = 0; i < soc->num_ictlrs; i++) {
> +			if (of_address_to_resource(np, i, &res) < 0)
> +				break;
> +
> +			ictlr_regs[i] = res;
> +		}
> +
> +		WARN(i != soc->num_ictlrs,
> +		     "Found %u interrupt controllers in DT; expected %u.\n",
> +		     i, soc->num_ictlrs);
> +
> +		max_ictlrs = soc->num_ictlrs;
> +		of_node_put(np);
> +	} else {
> +		/*
> +		 * If no matching device node was found, fall back to using
> +		 * the chip ID.
> +		 */
> +
> +		/* Tegra30 and later have five interrupt controllers, ... */
> +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> +
> +		/* ..., but Tegra20 only has four. */
> +		if (of_machine_is_compatible("nvidia,tegra20"))
> +			max_ictlrs--;
> +	}

How about moving the entire file to drivers/irqchip and using the
IRQCHIP_DECLARE() helper for the DT case?

For the fallback, you can have an entry into that file that just takes
the address and number, which you can call from platform code here.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-28 16:10         ` Arnd Bergmann
  0 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-28 16:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:

>  void __init tegra_init_irq(void)
>  {
> -	int i;
> -	void __iomem *distbase;
> +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> +	const struct of_device_id *match;
> +	struct device_node *np;
> +	struct resource res;
> +
> +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> +	if (np) {
> +		const struct tegra_ictlr_soc *soc = match->data;
> +
> +		for (i = 0; i < soc->num_ictlrs; i++) {
> +			if (of_address_to_resource(np, i, &res) < 0)
> +				break;
> +
> +			ictlr_regs[i] = res;
> +		}
> +
> +		WARN(i != soc->num_ictlrs,
> +		     "Found %u interrupt controllers in DT; expected %u.\n",
> +		     i, soc->num_ictlrs);
> +
> +		max_ictlrs = soc->num_ictlrs;
> +		of_node_put(np);
> +	} else {
> +		/*
> +		 * If no matching device node was found, fall back to using
> +		 * the chip ID.
> +		 */
> +
> +		/* Tegra30 and later have five interrupt controllers, ... */
> +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> +
> +		/* ..., but Tegra20 only has four. */
> +		if (of_machine_is_compatible("nvidia,tegra20"))
> +			max_ictlrs--;
> +	}

How about moving the entire file to drivers/irqchip and using the
IRQCHIP_DECLARE() helper for the DT case?

For the fallback, you can have an entry into that file that just takes
the address and number, which you can call from platform code here.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
  2014-08-28 15:31     ` Thierry Reding
@ 2014-08-28 16:11         ` Arnd Bergmann
  -1 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-28 16:11 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Thierry Reding, Stephen Warren,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Thursday 28 August 2014 17:31:16 Thierry Reding wrote:
> +       interrupt-controller@60004000 {
> +               compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
> +               reg = <0x60004000 0x40>, /* primary controller */
> +                     <0x60004100 0x40>, /* secondary controller */
> +                     <0x60004200 0x40>, /* tertiary controller */
> +                     <0x60004300 0x40>, /* quaternary controller */
> +                     <0x60004400 0x40>; /* quinary controller */
> +       };
> +
>         timer@60005000 {
>                 compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
> 

Don't you need an interrupt-parent and interrupts property here to point to
the GIC interrupts? I would assume this is a nested irqchip.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
@ 2014-08-28 16:11         ` Arnd Bergmann
  0 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-28 16:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Thursday 28 August 2014 17:31:16 Thierry Reding wrote:
> +       interrupt-controller at 60004000 {
> +               compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
> +               reg = <0x60004000 0x40>, /* primary controller */
> +                     <0x60004100 0x40>, /* secondary controller */
> +                     <0x60004200 0x40>, /* tertiary controller */
> +                     <0x60004300 0x40>, /* quaternary controller */
> +                     <0x60004400 0x40>; /* quinary controller */
> +       };
> +
>         timer at 60005000 {
>                 compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
> 

Don't you need an interrupt-parent and interrupts property here to point to
the GIC interrupts? I would assume this is a nested irqchip.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-28 15:31     ` Thierry Reding
@ 2014-08-29  3:27         ` Varka Bhadram
  -1 siblings, 0 replies; 34+ messages in thread
From: Varka Bhadram @ 2014-08-29  3:27 UTC (permalink / raw)
  To: Thierry Reding, Stephen Warren
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On 08/28/2014 09:01 PM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Obtains the register ranges for the legacy interrupt controller from DT
> and provide hard-coded values as fallback.
>
(...)

> +static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
> +	.num_ictlrs = 5,
> +};
> +
> +static const struct of_device_id ictlr_matches[] = {
> +	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
> +	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
> +	{ }
> +};
> +
> +static const struct of_device_id gic_matches[] = {
> +	{ .compatible = "arm,cortex-a15-gic", },
> +	{ .compatible = "arm,cortex-a9-gic", },
> +	{ }
> +};
> +
>
Missed MODULE_DEVICE_TABLE(of, ...)....?

-- 
Regards,
Varka Bhadram.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29  3:27         ` Varka Bhadram
  0 siblings, 0 replies; 34+ messages in thread
From: Varka Bhadram @ 2014-08-29  3:27 UTC (permalink / raw)
  To: linux-arm-kernel

On 08/28/2014 09:01 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Obtains the register ranges for the legacy interrupt controller from DT
> and provide hard-coded values as fallback.
>
(...)

> +static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
> +	.num_ictlrs = 5,
> +};
> +
> +static const struct of_device_id ictlr_matches[] = {
> +	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
> +	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
> +	{ }
> +};
> +
> +static const struct of_device_id gic_matches[] = {
> +	{ .compatible = "arm,cortex-a15-gic", },
> +	{ .compatible = "arm,cortex-a9-gic", },
> +	{ }
> +};
> +
>
Missed MODULE_DEVICE_TABLE(of, ...)....?

-- 
Regards,
Varka Bhadram.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29  3:27         ` Varka Bhadram
@ 2014-08-29  7:29             ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29  7:29 UTC (permalink / raw)
  To: Varka Bhadram
  Cc: Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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

On Fri, Aug 29, 2014 at 08:57:49AM +0530, Varka Bhadram wrote:
> On 08/28/2014 09:01 PM, Thierry Reding wrote:
> >From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> >Obtains the register ranges for the legacy interrupt controller from DT
> >and provide hard-coded values as fallback.
> >
> (...)
> 
> >+static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
> >+	.num_ictlrs = 5,
> >+};
> >+
> >+static const struct of_device_id ictlr_matches[] = {
> >+	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
> >+	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
> >+	{ }
> >+};
> >+
> >+static const struct of_device_id gic_matches[] = {
> >+	{ .compatible = "arm,cortex-a15-gic", },
> >+	{ .compatible = "arm,cortex-a9-gic", },
> >+	{ }
> >+};
> >+
> >
> Missed MODULE_DEVICE_TABLE(of, ...)....?

The driver isn't useful as a module, so I don't think that's necessary.
I'm not even sure it could be loaded that late in the process. Without
it peripheral interrupts don't work properly, so loading this from the
filesystem doesn't work (unless it's in an initrd), but perhaps even
that won't work. I'm not aware of anyone actually ever trying to build
it as a module.

Thierry

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29  7:29             ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29  7:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 29, 2014 at 08:57:49AM +0530, Varka Bhadram wrote:
> On 08/28/2014 09:01 PM, Thierry Reding wrote:
> >From: Thierry Reding <treding@nvidia.com>
> >
> >Obtains the register ranges for the legacy interrupt controller from DT
> >and provide hard-coded values as fallback.
> >
> (...)
> 
> >+static const struct tegra_ictlr_soc tegra30_ictlr_soc = {
> >+	.num_ictlrs = 5,
> >+};
> >+
> >+static const struct of_device_id ictlr_matches[] = {
> >+	{ .compatible = "nvidia,tegra30-ictlr", .data = &tegra30_ictlr_soc },
> >+	{ .compatible = "nvidia,tegra20-ictlr", .data = &tegra20_ictlr_soc },
> >+	{ }
> >+};
> >+
> >+static const struct of_device_id gic_matches[] = {
> >+	{ .compatible = "arm,cortex-a15-gic", },
> >+	{ .compatible = "arm,cortex-a9-gic", },
> >+	{ }
> >+};
> >+
> >
> Missed MODULE_DEVICE_TABLE(of, ...)....?

The driver isn't useful as a module, so I don't think that's necessary.
I'm not even sure it could be loaded that late in the process. Without
it peripheral interrupts don't work properly, so loading this from the
filesystem doesn't work (unless it's in an initrd), but perhaps even
that won't work. I'm not aware of anyone actually ever trying to build
it as a module.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140829/3f12e3f7/attachment.sig>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-28 16:10         ` Arnd Bergmann
@ 2014-08-29  7:31           ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29  7:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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

On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> 
> >  void __init tegra_init_irq(void)
> >  {
> > -	int i;
> > -	void __iomem *distbase;
> > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > +	const struct of_device_id *match;
> > +	struct device_node *np;
> > +	struct resource res;
> > +
> > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > +	if (np) {
> > +		const struct tegra_ictlr_soc *soc = match->data;
> > +
> > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > +			if (of_address_to_resource(np, i, &res) < 0)
> > +				break;
> > +
> > +			ictlr_regs[i] = res;
> > +		}
> > +
> > +		WARN(i != soc->num_ictlrs,
> > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > +		     i, soc->num_ictlrs);
> > +
> > +		max_ictlrs = soc->num_ictlrs;
> > +		of_node_put(np);
> > +	} else {
> > +		/*
> > +		 * If no matching device node was found, fall back to using
> > +		 * the chip ID.
> > +		 */
> > +
> > +		/* Tegra30 and later have five interrupt controllers, ... */
> > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > +
> > +		/* ..., but Tegra20 only has four. */
> > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > +			max_ictlrs--;
> > +	}
> 
> How about moving the entire file to drivers/irqchip and using the
> IRQCHIP_DECLARE() helper for the DT case?
> 
> For the fallback, you can have an entry into that file that just takes
> the address and number, which you can call from platform code here.

I think I did try that at some point, but there were issues that I don't
remember. I'll give it another shot.

Thierry

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29  7:31           ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29  7:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> 
> >  void __init tegra_init_irq(void)
> >  {
> > -	int i;
> > -	void __iomem *distbase;
> > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > +	const struct of_device_id *match;
> > +	struct device_node *np;
> > +	struct resource res;
> > +
> > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > +	if (np) {
> > +		const struct tegra_ictlr_soc *soc = match->data;
> > +
> > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > +			if (of_address_to_resource(np, i, &res) < 0)
> > +				break;
> > +
> > +			ictlr_regs[i] = res;
> > +		}
> > +
> > +		WARN(i != soc->num_ictlrs,
> > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > +		     i, soc->num_ictlrs);
> > +
> > +		max_ictlrs = soc->num_ictlrs;
> > +		of_node_put(np);
> > +	} else {
> > +		/*
> > +		 * If no matching device node was found, fall back to using
> > +		 * the chip ID.
> > +		 */
> > +
> > +		/* Tegra30 and later have five interrupt controllers, ... */
> > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > +
> > +		/* ..., but Tegra20 only has four. */
> > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > +			max_ictlrs--;
> > +	}
> 
> How about moving the entire file to drivers/irqchip and using the
> IRQCHIP_DECLARE() helper for the DT case?
> 
> For the fallback, you can have an entry into that file that just takes
> the address and number, which you can call from platform code here.

I think I did try that at some point, but there were issues that I don't
remember. I'll give it another shot.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140829/6793f259/attachment.sig>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29  7:31           ` Thierry Reding
@ 2014-08-29 14:24             ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29 14:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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

On Fri, Aug 29, 2014 at 09:31:40AM +0200, Thierry Reding wrote:
> On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> > On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> > 
> > >  void __init tegra_init_irq(void)
> > >  {
> > > -	int i;
> > > -	void __iomem *distbase;
> > > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > > +	const struct of_device_id *match;
> > > +	struct device_node *np;
> > > +	struct resource res;
> > > +
> > > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > > +	if (np) {
> > > +		const struct tegra_ictlr_soc *soc = match->data;
> > > +
> > > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > > +			if (of_address_to_resource(np, i, &res) < 0)
> > > +				break;
> > > +
> > > +			ictlr_regs[i] = res;
> > > +		}
> > > +
> > > +		WARN(i != soc->num_ictlrs,
> > > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > > +		     i, soc->num_ictlrs);
> > > +
> > > +		max_ictlrs = soc->num_ictlrs;
> > > +		of_node_put(np);
> > > +	} else {
> > > +		/*
> > > +		 * If no matching device node was found, fall back to using
> > > +		 * the chip ID.
> > > +		 */
> > > +
> > > +		/* Tegra30 and later have five interrupt controllers, ... */
> > > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > > +
> > > +		/* ..., but Tegra20 only has four. */
> > > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > > +			max_ictlrs--;
> > > +	}
> > 
> > How about moving the entire file to drivers/irqchip and using the
> > IRQCHIP_DECLARE() helper for the DT case?
> > 
> > For the fallback, you can have an entry into that file that just takes
> > the address and number, which you can call from platform code here.
> 
> I think I did try that at some point, but there were issues that I don't
> remember. I'll give it another shot.

So I got pretty far with this and the system still boots. But for some
reason suspend/resume is now broken. The difference seems to be that
earlier the legacy interrupt controller would be registered first, and
the GIC second. When the legacy interrupt controller is initialized
after the GIC (which happens when I use IRQCHIP_DECLARE), then suspend
and resume won't work (for some yet unknown reason). Unfortunately the
of_irq_init() code is too clever, so I can't even work around it by
changing link order or device tree order.

I'll see if I can find out what causes this combination to malfunction
when initialized in the opposite order.

Thierry

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29 14:24             ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29 14:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 29, 2014 at 09:31:40AM +0200, Thierry Reding wrote:
> On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> > On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> > 
> > >  void __init tegra_init_irq(void)
> > >  {
> > > -	int i;
> > > -	void __iomem *distbase;
> > > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > > +	const struct of_device_id *match;
> > > +	struct device_node *np;
> > > +	struct resource res;
> > > +
> > > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > > +	if (np) {
> > > +		const struct tegra_ictlr_soc *soc = match->data;
> > > +
> > > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > > +			if (of_address_to_resource(np, i, &res) < 0)
> > > +				break;
> > > +
> > > +			ictlr_regs[i] = res;
> > > +		}
> > > +
> > > +		WARN(i != soc->num_ictlrs,
> > > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > > +		     i, soc->num_ictlrs);
> > > +
> > > +		max_ictlrs = soc->num_ictlrs;
> > > +		of_node_put(np);
> > > +	} else {
> > > +		/*
> > > +		 * If no matching device node was found, fall back to using
> > > +		 * the chip ID.
> > > +		 */
> > > +
> > > +		/* Tegra30 and later have five interrupt controllers, ... */
> > > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > > +
> > > +		/* ..., but Tegra20 only has four. */
> > > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > > +			max_ictlrs--;
> > > +	}
> > 
> > How about moving the entire file to drivers/irqchip and using the
> > IRQCHIP_DECLARE() helper for the DT case?
> > 
> > For the fallback, you can have an entry into that file that just takes
> > the address and number, which you can call from platform code here.
> 
> I think I did try that at some point, but there were issues that I don't
> remember. I'll give it another shot.

So I got pretty far with this and the system still boots. But for some
reason suspend/resume is now broken. The difference seems to be that
earlier the legacy interrupt controller would be registered first, and
the GIC second. When the legacy interrupt controller is initialized
after the GIC (which happens when I use IRQCHIP_DECLARE), then suspend
and resume won't work (for some yet unknown reason). Unfortunately the
of_irq_init() code is too clever, so I can't even work around it by
changing link order or device tree order.

I'll see if I can find out what causes this combination to malfunction
when initialized in the opposite order.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140829/36bec183/attachment.sig>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29 14:24             ` Thierry Reding
@ 2014-08-29 15:04               ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29 15:04 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thomas Gleixner, Jason Cooper,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

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

On Fri, Aug 29, 2014 at 04:24:10PM +0200, Thierry Reding wrote:
> On Fri, Aug 29, 2014 at 09:31:40AM +0200, Thierry Reding wrote:
> > On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> > > On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> > > 
> > > >  void __init tegra_init_irq(void)
> > > >  {
> > > > -	int i;
> > > > -	void __iomem *distbase;
> > > > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > > > +	const struct of_device_id *match;
> > > > +	struct device_node *np;
> > > > +	struct resource res;
> > > > +
> > > > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > > > +	if (np) {
> > > > +		const struct tegra_ictlr_soc *soc = match->data;
> > > > +
> > > > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > > > +			if (of_address_to_resource(np, i, &res) < 0)
> > > > +				break;
> > > > +
> > > > +			ictlr_regs[i] = res;
> > > > +		}
> > > > +
> > > > +		WARN(i != soc->num_ictlrs,
> > > > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > > > +		     i, soc->num_ictlrs);
> > > > +
> > > > +		max_ictlrs = soc->num_ictlrs;
> > > > +		of_node_put(np);
> > > > +	} else {
> > > > +		/*
> > > > +		 * If no matching device node was found, fall back to using
> > > > +		 * the chip ID.
> > > > +		 */
> > > > +
> > > > +		/* Tegra30 and later have five interrupt controllers, ... */
> > > > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > > > +
> > > > +		/* ..., but Tegra20 only has four. */
> > > > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > > > +			max_ictlrs--;
> > > > +	}
> > > 
> > > How about moving the entire file to drivers/irqchip and using the
> > > IRQCHIP_DECLARE() helper for the DT case?
> > > 
> > > For the fallback, you can have an entry into that file that just takes
> > > the address and number, which you can call from platform code here.
> > 
> > I think I did try that at some point, but there were issues that I don't
> > remember. I'll give it another shot.
> 
> So I got pretty far with this and the system still boots. But for some
> reason suspend/resume is now broken. The difference seems to be that
> earlier the legacy interrupt controller would be registered first, and
> the GIC second. When the legacy interrupt controller is initialized
> after the GIC (which happens when I use IRQCHIP_DECLARE), then suspend
> and resume won't work (for some yet unknown reason). Unfortunately the
> of_irq_init() code is too clever, so I can't even work around it by
> changing link order or device tree order.
> 
> I'll see if I can find out what causes this combination to malfunction
> when initialized in the opposite order.

The reason for why ordering matters is this line from the GIC driver
(see drivers/irqchip/irq-gic.c):

	static int gic_init_bases(...)
	{
		...
		gic_chip.flags |= gic_arch_extn.flags;
		...
	}

So there's an implicit dependency on having the architecture-specific
extension initialized before the GIC.

If I manually add in the flags set by the LIC (IRQCHIP_MASK_ON_SUSPEND)
the suspend/resume issue that I observed goes away.

Perhaps a better way to handle this would be to add an API that GIC IRQ
extensions can use to register with the GIC independent of the
initialization order?

In this particular case something like:

	void gic_arch_register(unsigned long flags)
	{
		gic_chip.flags |= flags;
	}

would work irrespective of the ordering. But perhaps something more
elaborate would be more futureproof:

	void gic_arch_register(const struct irqchip *extn)
	{
		gic_chip.flags |= extn->flags;
		gic_arch_extn = *extn;
	}

Or perhaps gic_arch_extn could be turned into a pointer so that checks
for already registered extension drivers can be more easily done. That
is:

	static struct irq_chip *extn;

	void gic_arch_register(const struct irqchip *chip)
	{
		if (WARN(extn != NULL))
			return;

		gic_chip.flags |= chip->flags;
		extn = chip;
	}

Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
can provide more input on how to solve this.

Thierry

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29 15:04               ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-08-29 15:04 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 29, 2014 at 04:24:10PM +0200, Thierry Reding wrote:
> On Fri, Aug 29, 2014 at 09:31:40AM +0200, Thierry Reding wrote:
> > On Thu, Aug 28, 2014 at 06:10:55PM +0200, Arnd Bergmann wrote:
> > > On Thursday 28 August 2014 17:31:17 Thierry Reding wrote:
> > > 
> > > >  void __init tegra_init_irq(void)
> > > >  {
> > > > -	int i;
> > > > -	void __iomem *distbase;
> > > > +	unsigned int max_ictlrs = ARRAY_SIZE(ictlr_regs), i;
> > > > +	const struct of_device_id *match;
> > > > +	struct device_node *np;
> > > > +	struct resource res;
> > > > +
> > > > +	np = of_find_matching_node_and_match(NULL, ictlr_matches, &match);
> > > > +	if (np) {
> > > > +		const struct tegra_ictlr_soc *soc = match->data;
> > > > +
> > > > +		for (i = 0; i < soc->num_ictlrs; i++) {
> > > > +			if (of_address_to_resource(np, i, &res) < 0)
> > > > +				break;
> > > > +
> > > > +			ictlr_regs[i] = res;
> > > > +		}
> > > > +
> > > > +		WARN(i != soc->num_ictlrs,
> > > > +		     "Found %u interrupt controllers in DT; expected %u.\n",
> > > > +		     i, soc->num_ictlrs);
> > > > +
> > > > +		max_ictlrs = soc->num_ictlrs;
> > > > +		of_node_put(np);
> > > > +	} else {
> > > > +		/*
> > > > +		 * If no matching device node was found, fall back to using
> > > > +		 * the chip ID.
> > > > +		 */
> > > > +
> > > > +		/* Tegra30 and later have five interrupt controllers, ... */
> > > > +		max_ictlrs = ARRAY_SIZE(ictlr_regs);
> > > > +
> > > > +		/* ..., but Tegra20 only has four. */
> > > > +		if (of_machine_is_compatible("nvidia,tegra20"))
> > > > +			max_ictlrs--;
> > > > +	}
> > > 
> > > How about moving the entire file to drivers/irqchip and using the
> > > IRQCHIP_DECLARE() helper for the DT case?
> > > 
> > > For the fallback, you can have an entry into that file that just takes
> > > the address and number, which you can call from platform code here.
> > 
> > I think I did try that at some point, but there were issues that I don't
> > remember. I'll give it another shot.
> 
> So I got pretty far with this and the system still boots. But for some
> reason suspend/resume is now broken. The difference seems to be that
> earlier the legacy interrupt controller would be registered first, and
> the GIC second. When the legacy interrupt controller is initialized
> after the GIC (which happens when I use IRQCHIP_DECLARE), then suspend
> and resume won't work (for some yet unknown reason). Unfortunately the
> of_irq_init() code is too clever, so I can't even work around it by
> changing link order or device tree order.
> 
> I'll see if I can find out what causes this combination to malfunction
> when initialized in the opposite order.

The reason for why ordering matters is this line from the GIC driver
(see drivers/irqchip/irq-gic.c):

	static int gic_init_bases(...)
	{
		...
		gic_chip.flags |= gic_arch_extn.flags;
		...
	}

So there's an implicit dependency on having the architecture-specific
extension initialized before the GIC.

If I manually add in the flags set by the LIC (IRQCHIP_MASK_ON_SUSPEND)
the suspend/resume issue that I observed goes away.

Perhaps a better way to handle this would be to add an API that GIC IRQ
extensions can use to register with the GIC independent of the
initialization order?

In this particular case something like:

	void gic_arch_register(unsigned long flags)
	{
		gic_chip.flags |= flags;
	}

would work irrespective of the ordering. But perhaps something more
elaborate would be more futureproof:

	void gic_arch_register(const struct irqchip *extn)
	{
		gic_chip.flags |= extn->flags;
		gic_arch_extn = *extn;
	}

Or perhaps gic_arch_extn could be turned into a pointer so that checks
for already registered extension drivers can be more easily done. That
is:

	static struct irq_chip *extn;

	void gic_arch_register(const struct irqchip *chip)
	{
		if (WARN(extn != NULL))
			return;

		gic_chip.flags |= chip->flags;
		extn = chip;
	}

Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
can provide more input on how to solve this.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140829/1ed9c444/attachment-0001.sig>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29 15:04               ` Thierry Reding
@ 2014-08-29 19:53                 ` Arnd Bergmann
  -1 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-29 19:53 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Thierry Reding, devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Thomas Gleixner

On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
>         static struct irq_chip *extn;
> 
>         void gic_arch_register(const struct irqchip *chip)
>         {
>                 if (WARN(extn != NULL))
>                         return;
> 
>                 gic_chip.flags |= chip->flags;
>                 extn = chip;
>         }
> 
> Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> can provide more input on how to solve this.

I think the entire gic_arch_extn method is done in a rather odd way
and we should try to come up with a replacement.

These are the users at the moment:

arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;

I have to admit I don't really understand how these work, but what
I'd expect to work better is a way to turn the gic code into more
of a library that can be used by specialized drivers. In that
case you would register a driver for the tegra gic using IRQCHIP_DECLARE
and that driver would call a variation of gic_of_init() or gic_init_bases()
with the extra stuff as arguments.

We'd have to hack around the fact that all these platforms currently
don't list a specialized compatible string, but at least for the future
we should be able to do this without special hacks.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-29 19:53                 ` Arnd Bergmann
  0 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-08-29 19:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
>         static struct irq_chip *extn;
> 
>         void gic_arch_register(const struct irqchip *chip)
>         {
>                 if (WARN(extn != NULL))
>                         return;
> 
>                 gic_chip.flags |= chip->flags;
>                 extn = chip;
>         }
> 
> Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> can provide more input on how to solve this.

I think the entire gic_arch_extn method is done in a rather odd way
and we should try to come up with a replacement.

These are the users at the moment:

arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;

I have to admit I don't really understand how these work, but what
I'd expect to work better is a way to turn the gic code into more
of a library that can be used by specialized drivers. In that
case you would register a driver for the tegra gic using IRQCHIP_DECLARE
and that driver would call a variation of gic_of_init() or gic_init_bases()
with the extra stuff as arguments.

We'd have to hack around the fact that all these platforms currently
don't list a specialized compatible string, but at least for the future
we should be able to do this without special hacks.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29 19:53                 ` Arnd Bergmann
@ 2014-08-30 15:54                   ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2014-08-30 15:54 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Thierry Reding, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Thomas Gleixner

Arnd,

On Fri, Aug 29, 2014 at 09:53:42PM +0200, Arnd Bergmann wrote:
> On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
> >         static struct irq_chip *extn;
> > 
> >         void gic_arch_register(const struct irqchip *chip)
> >         {
> >                 if (WARN(extn != NULL))
> >                         return;
> > 
> >                 gic_chip.flags |= chip->flags;
> >                 extn = chip;
> >         }
> > 
> > Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> > can provide more input on how to solve this.
> 
> I think the entire gic_arch_extn method is done in a rather odd way
> and we should try to come up with a replacement.
> 
> These are the users at the moment:
> 
> arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
> arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
> arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> 
> I have to admit I don't really understand how these work, but what
> I'd expect to work better is a way to turn the gic code into more
> of a library that can be used by specialized drivers. In that
> case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> and that driver would call a variation of gic_of_init() or gic_init_bases()
> with the extra stuff as arguments.
> 
> We'd have to hack around the fact that all these platforms currently
> don't list a specialized compatible string, but at least for the future
> we should be able to do this without special hacks.

Thomas was talking about this just the other day:

  https://lkml.kernel.org/r/alpine.DEB.2.10.1408271347210.3323@nanos

thx,

Jason.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-08-30 15:54                   ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2014-08-30 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Arnd,

On Fri, Aug 29, 2014 at 09:53:42PM +0200, Arnd Bergmann wrote:
> On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
> >         static struct irq_chip *extn;
> > 
> >         void gic_arch_register(const struct irqchip *chip)
> >         {
> >                 if (WARN(extn != NULL))
> >                         return;
> > 
> >                 gic_chip.flags |= chip->flags;
> >                 extn = chip;
> >         }
> > 
> > Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> > can provide more input on how to solve this.
> 
> I think the entire gic_arch_extn method is done in a rather odd way
> and we should try to come up with a replacement.
> 
> These are the users at the moment:
> 
> arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
> arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
> arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> 
> I have to admit I don't really understand how these work, but what
> I'd expect to work better is a way to turn the gic code into more
> of a library that can be used by specialized drivers. In that
> case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> and that driver would call a variation of gic_of_init() or gic_init_bases()
> with the extra stuff as arguments.
> 
> We'd have to hack around the fact that all these platforms currently
> don't list a specialized compatible string, but at least for the future
> we should be able to do this without special hacks.

Thomas was talking about this just the other day:

  https://lkml.kernel.org/r/alpine.DEB.2.10.1408271347210.3323 at nanos

thx,

Jason.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
  2014-08-28 16:11         ` Arnd Bergmann
@ 2014-09-01  8:16           ` Peter De Schrijver
  -1 siblings, 0 replies; 34+ messages in thread
From: Peter De Schrijver @ 2014-09-01  8:16 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Thierry Reding, Stephen Warren,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Thu, Aug 28, 2014 at 06:11:53PM +0200, Arnd Bergmann wrote:
> On Thursday 28 August 2014 17:31:16 Thierry Reding wrote:
> > +       interrupt-controller@60004000 {
> > +               compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
> > +               reg = <0x60004000 0x40>, /* primary controller */
> > +                     <0x60004100 0x40>, /* secondary controller */
> > +                     <0x60004200 0x40>, /* tertiary controller */
> > +                     <0x60004300 0x40>, /* quaternary controller */
> > +                     <0x60004400 0x40>; /* quinary controller */
> > +       };
> > +
> >         timer@60005000 {
> >                 compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
> > 
> 
> Don't you need an interrupt-parent and interrupts property here to point to
> the GIC interrupts? I would assume this is a nested irqchip.

No. It's not nested. All SoC interrupts are fed into both the LIC and the
GIC (as SPIs). The LIC also has an interface towards the flowcontroller which
allows waking up the CPU when an interrupt happens, even if the GIC is off. 
LIC can also route every interrupt it controls to the AVP.

See also section 3.4.1.4 Interrupt routing of the TRM.

Cheers,

Peter.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes
@ 2014-09-01  8:16           ` Peter De Schrijver
  0 siblings, 0 replies; 34+ messages in thread
From: Peter De Schrijver @ 2014-09-01  8:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Aug 28, 2014 at 06:11:53PM +0200, Arnd Bergmann wrote:
> On Thursday 28 August 2014 17:31:16 Thierry Reding wrote:
> > +       interrupt-controller at 60004000 {
> > +               compatible = "nvidia,tegra114-ictlr", "nvidia,tegra30-ictlr";
> > +               reg = <0x60004000 0x40>, /* primary controller */
> > +                     <0x60004100 0x40>, /* secondary controller */
> > +                     <0x60004200 0x40>, /* tertiary controller */
> > +                     <0x60004300 0x40>, /* quaternary controller */
> > +                     <0x60004400 0x40>; /* quinary controller */
> > +       };
> > +
> >         timer at 60005000 {
> >                 compatible = "nvidia,tegra114-timer", "nvidia,tegra20-timer";
> > 
> 
> Don't you need an interrupt-parent and interrupts property here to point to
> the GIC interrupts? I would assume this is a nested irqchip.

No. It's not nested. All SoC interrupts are fed into both the LIC and the
GIC (as SPIs). The LIC also has an interface towards the flowcontroller which
allows waking up the CPU when an interrupt happens, even if the GIC is off. 
LIC can also route every interrupt it controls to the AVP.

See also section 3.4.1.4 Interrupt routing of the TRM.

Cheers,

Peter.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-30 15:54                   ` Jason Cooper
@ 2014-09-01  8:45                       ` Arnd Bergmann
  -1 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-09-01  8:45 UTC (permalink / raw)
  To: Jason Cooper
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Thierry Reding, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Stephen Warren, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	Thomas Gleixner

On Saturday 30 August 2014 11:54:59 Jason Cooper wrote:
> > I have to admit I don't really understand how these work, but what
> > I'd expect to work better is a way to turn the gic code into more
> > of a library that can be used by specialized drivers. In that
> > case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> > and that driver would call a variation of gic_of_init() or gic_init_bases()
> > with the extra stuff as arguments.
> > 
> > We'd have to hack around the fact that all these platforms currently
> > don't list a specialized compatible string, but at least for the future
> > we should be able to do this without special hacks.
> 
> Thomas was talking about this just the other day:
> 
>   https://lkml.kernel.org/r/alpine.DEB.2.10.1408271347210.3323@nanos

Ah, cool. That seems better than what I had in mind. Yes, let's do that.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-09-01  8:45                       ` Arnd Bergmann
  0 siblings, 0 replies; 34+ messages in thread
From: Arnd Bergmann @ 2014-09-01  8:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Saturday 30 August 2014 11:54:59 Jason Cooper wrote:
> > I have to admit I don't really understand how these work, but what
> > I'd expect to work better is a way to turn the gic code into more
> > of a library that can be used by specialized drivers. In that
> > case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> > and that driver would call a variation of gic_of_init() or gic_init_bases()
> > with the extra stuff as arguments.
> > 
> > We'd have to hack around the fact that all these platforms currently
> > don't list a specialized compatible string, but at least for the future
> > we should be able to do this without special hacks.
> 
> Thomas was talking about this just the other day:
> 
>   https://lkml.kernel.org/r/alpine.DEB.2.10.1408271347210.3323 at nanos

Ah, cool. That seems better than what I had in mind. Yes, let's do that.

	Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
  2014-08-29 19:53                 ` Arnd Bergmann
@ 2014-09-01  8:47                   ` Thierry Reding
  -1 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-09-01  8:47 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper, Stephen Warren,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Thomas Gleixner

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

On Fri, Aug 29, 2014 at 09:53:42PM +0200, Arnd Bergmann wrote:
> On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
> >         static struct irq_chip *extn;
> > 
> >         void gic_arch_register(const struct irqchip *chip)
> >         {
> >                 if (WARN(extn != NULL))
> >                         return;
> > 
> >                 gic_chip.flags |= chip->flags;
> >                 extn = chip;
> >         }
> > 
> > Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> > can provide more input on how to solve this.
> 
> I think the entire gic_arch_extn method is done in a rather odd way
> and we should try to come up with a replacement.
> 
> These are the users at the moment:
> 
> arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
> arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
> arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> 
> I have to admit I don't really understand how these work, but what
> I'd expect to work better is a way to turn the gic code into more
> of a library that can be used by specialized drivers. In that
> case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> and that driver would call a variation of gic_of_init() or gic_init_bases()
> with the extra stuff as arguments.
> 
> We'd have to hack around the fact that all these platforms currently
> don't list a specialized compatible string, but at least for the future
> we should be able to do this without special hacks.

Besides the issue with the missing compatible values, the hardware
blocks that provide the additional functionality are separate from the
GIC. So they would have to look up the GIC node explicitly.

I suppose that might actually be to our advantage, since these blocks
will presumably have a specialized compatible string. So perhaps one
solution would be to make the drivers for these separate IP blocks look
up the GIC device tree node and call gic_of_init_ext() with a pointer to
the IRQ chip implementation of this platform-specific glue to replace
gic_arch_extn.

Thierry

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT
@ 2014-09-01  8:47                   ` Thierry Reding
  0 siblings, 0 replies; 34+ messages in thread
From: Thierry Reding @ 2014-09-01  8:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 29, 2014 at 09:53:42PM +0200, Arnd Bergmann wrote:
> On Friday 29 August 2014 17:04:28 Thierry Reding wrote:
> >         static struct irq_chip *extn;
> > 
> >         void gic_arch_register(const struct irqchip *chip)
> >         {
> >                 if (WARN(extn != NULL))
> >                         return;
> > 
> >                 gic_chip.flags |= chip->flags;
> >                 extn = chip;
> >         }
> > 
> > Any preferences, or other ideas? Adding Thomas and Jason, perhaps they
> > can provide more input on how to solve this.
> 
> I think the entire gic_arch_extn method is done in a rather odd way
> and we should try to come up with a replacement.
> 
> These are the users at the moment:
> 
> arch/arm/mach-exynos/pm.c:      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_mask = imx_gpc_irq_mask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_unmask = imx_gpc_irq_unmask;
> arch/arm/mach-imx/gpc.c:        gic_arch_extn.irq_set_wake = imx_gpc_irq_set_wake;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_mask = wakeupgen_mask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.irq_unmask = wakeupgen_unmask;
> arch/arm/mach-omap2/omap-wakeupgen.c:   gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_W
> arch/arm/mach-shmobile/intc-sh73a0.c:   gic_arch_extn.irq_set_wake = sh73a0_set_wake;
> arch/arm/mach-shmobile/setup-r8a7779.c: gic_arch_extn.irq_set_wake = r8a7779_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_ack = tegra_ack;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_eoi = tegra_eoi;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_mask = tegra_mask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_unmask = tegra_unmask;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_retrigger = tegra_retrigger;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.irq_set_wake = tegra_set_wake;
> arch/arm/mach-tegra/irq.c:      gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-ux500/cpu.c:      gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> arch/arm/mach-zynq/common.c:    gic_arch_extn.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
> 
> I have to admit I don't really understand how these work, but what
> I'd expect to work better is a way to turn the gic code into more
> of a library that can be used by specialized drivers. In that
> case you would register a driver for the tegra gic using IRQCHIP_DECLARE
> and that driver would call a variation of gic_of_init() or gic_init_bases()
> with the extra stuff as arguments.
> 
> We'd have to hack around the fact that all these platforms currently
> don't list a specialized compatible string, but at least for the future
> we should be able to do this without special hacks.

Besides the issue with the missing compatible values, the hardware
blocks that provide the additional functionality are separate from the
GIC. So they would have to look up the GIC node explicitly.

I suppose that might actually be to our advantage, since these blocks
will presumably have a specialized compatible string. So perhaps one
solution would be to make the drivers for these separate IP blocks look
up the GIC device tree node and call gic_of_init_ext() with a pointer to
the IRQ chip implementation of this platform-specific glue to replace
gic_arch_extn.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140901/3832df43/attachment.sig>

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2014-09-01  8:47 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-28 15:31 [PATCH v4 1/5] of: Add NVIDIA Tegra Legacy Interrupt Controller binding Thierry Reding
2014-08-28 15:31 ` Thierry Reding
     [not found] ` <1409239879-12376-1-git-send-email-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-08-28 15:31   ` [PATCH v4 2/5] ARM: tegra: Add legacy interrupt controller nodes Thierry Reding
2014-08-28 15:31     ` Thierry Reding
     [not found]     ` <1409239879-12376-2-git-send-email-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-08-28 16:11       ` Arnd Bergmann
2014-08-28 16:11         ` Arnd Bergmann
2014-09-01  8:16         ` Peter De Schrijver
2014-09-01  8:16           ` Peter De Schrijver
2014-08-28 15:31   ` [PATCH v4 3/5] ARM: tegra: Initialize interrupt controller from DT Thierry Reding
2014-08-28 15:31     ` Thierry Reding
     [not found]     ` <1409239879-12376-3-git-send-email-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-08-28 16:10       ` Arnd Bergmann
2014-08-28 16:10         ` Arnd Bergmann
2014-08-29  7:31         ` Thierry Reding
2014-08-29  7:31           ` Thierry Reding
2014-08-29 14:24           ` Thierry Reding
2014-08-29 14:24             ` Thierry Reding
2014-08-29 15:04             ` Thierry Reding
2014-08-29 15:04               ` Thierry Reding
2014-08-29 19:53               ` Arnd Bergmann
2014-08-29 19:53                 ` Arnd Bergmann
2014-08-30 15:54                 ` Jason Cooper
2014-08-30 15:54                   ` Jason Cooper
     [not found]                   ` <20140830155459.GI3683-u4khhh1J0LxI1Ri9qeTfzeTW4wlIGRCZ@public.gmane.org>
2014-09-01  8:45                     ` Arnd Bergmann
2014-09-01  8:45                       ` Arnd Bergmann
2014-09-01  8:47                 ` Thierry Reding
2014-09-01  8:47                   ` Thierry Reding
2014-08-29  3:27       ` Varka Bhadram
2014-08-29  3:27         ` Varka Bhadram
     [not found]         ` <53FFF335.1020402-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-08-29  7:29           ` Thierry Reding
2014-08-29  7:29             ` Thierry Reding
2014-08-28 15:31   ` [PATCH v4 4/5] ARM: tegra: Remove unused GIC initialization Thierry Reding
2014-08-28 15:31     ` Thierry Reding
2014-08-28 15:31   ` [PATCH v4 5/5] ARM: tegra: Remove unused defines Thierry Reding
2014-08-28 15:31     ` Thierry Reding

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.