[PATCH v3 0/6] move s3c24xx-irq to drivers/irqchip and add dt support

[PATCH v3 0/6] move s3c24xx-irq to drivers/irqchip and add dt support

[Heiko Stübner]

In contrast to the previous versions, this variant gets rid of the irq
mappings in the dt files and provides individual compatible properties
for the different s3c24xx variants and therefore does not need to
introduce any new dt-properties.

It also now encapsulates the different sub-controllers inside a
node to make it possible to map the register region completely.

This series depends on the latest s3c24xx-irq changes currently in the
linux-samsung tree, and should therefore probably go thru there.

Tested on a s3c2416-based machine with preliminary dt support.

Heiko Stuebner (6):
  ARM: S3C24XX: move irq driver to drivers/irqchip
  irqchip: s3c24xx: fix comments on some camera interrupts
  irqchip: s3c24xx: fix irqlist of second s3c2416 controller
  irqchip: s3c24xx: use irq_create_mapping for parent irqs
  irqchip: s3c24xx: add devicetree support
  irqchip: s3c24xx: add s3c2450 interrupt definitions

 .../interrupt-controller/samsung,s3c24xx-irq.txt   |   54 ++++
 arch/arm/mach-s3c24xx/Makefile                     |    2 +-
 drivers/irqchip/Makefile                           |    1 +
 .../irq.c => drivers/irqchip/irq-s3c24xx.c         |  293 +++++++++++++++++++-
 4 files changed, 342 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
 rename arch/arm/mach-s3c24xx/irq.c => drivers/irqchip/irq-s3c24xx.c (80%)

-- 
1.7.2.3

[PATCH v3 1/6] ARM: S3C24XX: move irq driver to drivers/irqchip

[Heiko Stübner]

This move is necessary to make use of the irqchip infrastructure
for the following devicetree support for s3c24xx architectures.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 arch/arm/mach-s3c24xx/Makefile                     |    2 +-
 drivers/irqchip/Makefile                           |    1 +
 .../irq.c => drivers/irqchip/irq-s3c24xx.c         |    0
 3 files changed, 2 insertions(+), 1 deletions(-)
 rename arch/arm/mach-s3c24xx/irq.c => drivers/irqchip/irq-s3c24xx.c (100%)

diff --git a/arch/arm/mach-s3c24xx/Makefile b/arch/arm/mach-s3c24xx/Makefile
index be6e4d0..6f46ecf 100644
--- a/arch/arm/mach-s3c24xx/Makefile
+++ b/arch/arm/mach-s3c24xx/Makefile
@@ -14,7 +14,7 @@ obj-				:=
 
 # core
 
-obj-y				+= common.o irq.o
+obj-y				+= common.o
 
 obj-$(CONFIG_CPU_S3C2410)	+= s3c2410.o
 obj-$(CONFIG_S3C2410_CPUFREQ)	+= cpufreq-s3c2410.o
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 98e3b87..4d65a21 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_IRQCHIP)			+= irqchip.o
 
 obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2835.o
 obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
+obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
 obj-$(CONFIG_METAG)			+= irq-metag-ext.o
 obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi.o
diff --git a/arch/arm/mach-s3c24xx/irq.c b/drivers/irqchip/irq-s3c24xx.c
similarity index 100%
rename from arch/arm/mach-s3c24xx/irq.c
rename to drivers/irqchip/irq-s3c24xx.c
-- 
1.7.2.3

[PATCH v3 2/6] irqchip: s3c24xx: fix comments on some camera interrupts

[Heiko Stübner]

Might be confusing for people to read the code without having the
datasheet nearby.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/irqchip/irq-s3c24xx.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index 5c9f8b7..84afbc1 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -916,8 +916,8 @@ static struct s3c_irq_data init_s3c2440subint[32] = {
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-ERR */
 	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* TC */
 	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* ADC */
-	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* TC */
-	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* ADC */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_C */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_P */
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
 };
@@ -991,8 +991,8 @@ static struct s3c_irq_data init_s3c2442subint[32] = {
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-ERR */
 	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* TC */
 	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* ADC */
-	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* TC */
-	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* ADC */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_C */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_P */
 };
 
 void __init s3c2442_init_irq(void)
-- 
1.7.2.3

[PATCH v3 3/6] irqchip: s3c24xx: fix irqlist of second s3c2416 controller

[Heiko Stübner]

The list in used was from the s3c2450, a close cousin of the s3c2416.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/irqchip/irq-s3c24xx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index 84afbc1..a565eb8 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -835,13 +835,12 @@ static struct s3c_irq_data init_s3c2416subint[32] = {
 
 static struct s3c_irq_data init_s3c2416_second[32] = {
 	{ .type = S3C_IRQTYPE_EDGE }, /* 2D */
-	{ .type = S3C_IRQTYPE_EDGE }, /* IIC1 */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
 	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
 	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
 	{ .type = S3C_IRQTYPE_EDGE }, /* PCM0 */
-	{ .type = S3C_IRQTYPE_EDGE }, /* PCM1 */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
 	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
-	{ .type = S3C_IRQTYPE_EDGE }, /* I2S1 */
 };
 
 void __init s3c2416_init_irq(void)
-- 
1.7.2.3

[PATCH v3 4/6] irqchip: s3c24xx: use irq_create_mapping for parent irqs

[Heiko Stübner]

Getting the parent irq number thru irq_find_mapping will fail for non-legacy
irq_domains, as the mapping of the parent irq won't have been created.

As irq_create_mapping will just output an already mapped irq number if its
already mapped, this does also not influence the legacy mappings.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/irqchip/irq-s3c24xx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index a565eb8..1eba289 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -429,7 +429,7 @@ static int s3c24xx_irq_map(struct irq_domain *h, unsigned int virq,
 		parent_irq_data->sub_bits |= (1UL << hw);
 
 		/* attach the demuxer to the parent irq */
-		irqno = irq_find_mapping(parent_intc->domain,
+		irqno = irq_create_mapping(parent_intc->domain,
 					 irq_data->parent_irq);
 		if (!irqno) {
 			pr_err("irq-s3c24xx: could not find mapping for parent irq %lu\n",
-- 
1.7.2.3

[PATCH v3 5/6] irqchip: s3c24xx: add devicetree support

[Heiko Stübner]

Add the necessary code to initialize the interrupt controller
thru devicetree data using the irqchip infrastructure.

On dt machines the eint-type interrupts in the main interrupt controller
get mapped as regular edge-types, as their wakeup and interrupt type
properties will be handled by the upcoming pinctrl driver.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../interrupt-controller/samsung,s3c24xx-irq.txt   |   54 +++++
 drivers/irqchip/irq-s3c24xx.c                      |  222 ++++++++++++++++++++
 2 files changed, 276 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
new file mode 100644
index 0000000..be5dead
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
@@ -0,0 +1,54 @@
+Samsung S3C24XX Interrupt Controllers
+
+The S3C24XX SoCs contain a custom set of interrupt controllers providing a
+varying number of interrupt sources. The set consists of a main- and sub-
+controller and on newer SoCs even a second main controller.
+
+Required properties:
+- compatible: Compatible property value should be one of "samsung,s3c2410-irq",
+  "samsung,s3c2412-irq", "samsung,s3c2416-irq", "samsung,s3c2440-irq",
+  "samsung,s3c2442-irq", "samsung,s3c2443-irq" depending on the SoC variant.
+
+- reg: Physical base address of the controller and length of memory mapped
+  region.
+
+- interrupt-controller : Identifies the node as an interrupt controller
+
+Sub-controllers as child nodes:
+  The interrupt controllers that should be referenced by device nodes are
+  represented by child nodes. Valid names are intc, subintc and intc2.
+  The interrupt values in device nodes are then mapped directly to the
+  bit-numbers of the pending register of the named interrupt controller.
+
+Required properties:
+- interrupt-controller : Identifies the node as an interrupt controller
+
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 2.
+
+Example:
+
+	interrupt-controller@4a000000 {
+		compatible = "samsung,s3c2416-irq";
+		reg = <0x4a000000 0x100>;
+		interrupt-controller;
+
+		intc:intc {
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		subintc:subintc {
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
+
+	[...]
+
+	serial@50000000 {
+		compatible = "samsung,s3c2410-uart";
+		reg = <0x50000000 0x4000>;
+		interrupt-parent = <&subintc>;
+		interrupts = <0 0>, <1 0>;
+	};
diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index 1eba289..55cb363 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -25,6 +25,9 @@
 #include <linux/ioport.h>
 #include <linux/device.h>
 #include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
 
 #include <asm/exception.h>
 #include <asm/mach/irq.h>
@@ -36,6 +39,8 @@
 #include <plat/regs-irqtype.h>
 #include <plat/pm.h>
 
+#include "irqchip.h"
+
 #define S3C_IRQTYPE_NONE	0
 #define S3C_IRQTYPE_EINT	1
 #define S3C_IRQTYPE_EDGE	2
@@ -380,6 +385,10 @@ static int s3c24xx_irq_map(struct irq_domain *h, unsigned int virq,
 
 	parent_intc = intc->parent;
 
+	/* on dt platforms the extints get handled by the pinctrl driver */
+	if (h->of_node && irq_data->type == S3C_IRQTYPE_EINT)
+		irq_data->type = S3C_IRQTYPE_EDGE;
+
 	/* set handler and flags */
 	switch (irq_data->type) {
 	case S3C_IRQTYPE_NONE:
@@ -1104,3 +1113,216 @@ void __init s3c2443_init_irq(void)
 	s3c24xx_init_intc(NULL, &init_s3c2443subint[0], main_intc, 0x4a000018);
 }
 #endif
+
+#ifdef CONFIG_OF
+struct s3c24xx_irq_of_ctrl {
+	char			*name;
+	unsigned long		offset;
+	struct s3c_irq_data	*irq_data;
+	struct s3c_irq_intc	**handle;
+	struct s3c_irq_intc	**parent;
+};
+
+#define S3C24XX_IRQCTRL(n, o, d, h, p)		\
+{						\
+	.name		= n,			\
+	.offset		= o,			\
+	.irq_data	= d,			\
+	.handle		= h,			\
+	.parent		= p,			\
+}
+
+struct s3c24xx_irq_of_data {
+	struct s3c24xx_irq_of_ctrl	*irq_ctrl;
+	int				num_ctrl;
+};
+
+#ifdef CONFIG_CPU_S3C2410
+static struct s3c24xx_irq_of_ctrl s3c2410_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2410base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2410subint, NULL, &main_intc),
+};
+
+static struct s3c24xx_irq_of_data s3c2410_irq_data = {
+	.irq_ctrl = s3c2410_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2410_ctrl),
+};
+#endif
+
+#ifdef CONFIG_CPU_S3C2412
+static struct s3c24xx_irq_of_ctrl s3c2412_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2412base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2412subint, NULL, &main_intc),
+};
+
+static struct s3c24xx_irq_of_data s3c2412_irq_data = {
+	.irq_ctrl = s3c2412_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2412_ctrl),
+};
+#endif
+
+#ifdef CONFIG_CPU_S3C2416
+static struct s3c24xx_irq_of_ctrl s3c2416_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2416base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2416subint, NULL, &main_intc),
+	S3C24XX_IRQCTRL("intc2", 0x40, init_s3c2416_second, &main_intc2, NULL),
+};
+
+static struct s3c24xx_irq_of_data s3c2416_irq_data = {
+	.irq_ctrl = s3c2416_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2416_ctrl),
+};
+#endif
+
+#ifdef CONFIG_CPU_S3C2440
+static struct s3c24xx_irq_of_ctrl s3c2440_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2440base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2440subint, NULL, &main_intc),
+};
+
+static struct s3c24xx_irq_of_data s3c2440_irq_data = {
+	.irq_ctrl = s3c2440_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2440_ctrl),
+};
+#endif
+
+#ifdef CONFIG_CPU_S3C2442
+static struct s3c24xx_irq_of_ctrl s3c2442_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2442base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2442subint, NULL, &main_intc),
+};
+
+static struct s3c24xx_irq_of_data s3c2442_irq_data = {
+	.irq_ctrl = s3c2442_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2442_ctrl),
+};
+#endif
+
+#ifdef CONFIG_CPU_S3C2443
+static struct s3c24xx_irq_of_ctrl s3c2443_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2443base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2443subint, NULL, &main_intc),
+};
+
+static struct s3c24xx_irq_of_data s3c2443_irq_data = {
+	.irq_ctrl = s3c2443_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2443_ctrl),
+};
+#endif
+
+static const struct of_device_id intc_list[] = {
+#ifdef CONFIG_CPU_S3C2410
+	{ .compatible = "samsung,s3c2410-irq", .data = &s3c2410_irq_data },
+#endif
+#ifdef CONFIG_CPU_S3C2412
+	{ .compatible = "samsung,s3c2412-irq", .data = &s3c2412_irq_data },
+#endif
+#ifdef CONFIG_CPU_S3C2416
+	{ .compatible = "samsung,s3c2416-irq", .data = &s3c2416_irq_data },
+#endif
+#ifdef CONFIG_CPU_S3C2440
+	{ .compatible = "samsung,s3c2440-irq", .data = &s3c2440_irq_data },
+#endif
+#ifdef CONFIG_CPU_S3C2442
+	{ .compatible = "samsung,s3c2442-irq", .data = &s3c2442_irq_data },
+#endif
+#ifdef CONFIG_CPU_S3C2443
+	{ .compatible = "samsung,s3c2443-irq", .data = &s3c2443_irq_data },
+#endif
+	{},
+};
+
+int __init s3c24xx_init_intc_of(struct device_node *np,
+				 struct device_node *interrupt_parent)
+{
+	const struct of_device_id *match;
+	const struct s3c24xx_irq_of_data *ctrl_data;
+	struct device_node *intc_node;
+	struct s3c24xx_irq_of_ctrl *ctrl;
+	struct s3c_irq_intc *intc;
+	void __iomem *reg_base;
+	int i;
+
+	match = of_match_node(intc_list, np);
+	if (!match) {
+		pr_err("irq-s3c24xx: could not find matching irqdata\n");
+		return -EINVAL;
+	}
+
+	ctrl_data = match->data;
+
+	reg_base = of_iomap(np, 0);
+	if (!reg_base) {
+		pr_err("irq-s3c24xx: could not map irq memory\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ctrl_data->num_ctrl; i++) {
+		ctrl = &ctrl_data->irq_ctrl[i];
+
+		intc_node = of_find_node_by_name(np, ctrl->name);
+		if (!intc_node) {
+			pr_debug("irq: no device node for %s\n",
+				ctrl->name);
+			continue;
+		}
+
+		intc = kzalloc(sizeof(struct s3c_irq_intc), GFP_KERNEL);
+		if (!intc) {
+			of_node_put(intc_node);
+			return -ENOMEM;
+		}
+
+		pr_debug("irq: found controller %s\n", ctrl->name);
+
+		intc->irqs = ctrl->irq_data;
+
+		if (ctrl->parent) {
+			if (*(ctrl->parent)) {
+				intc->parent = *(ctrl->parent);
+			} else {
+				pr_warn("irq: parent of %s missing\n",
+					ctrl->name);
+				kfree(intc);
+				of_node_put(intc_node);
+				continue;
+			}
+		}
+
+		if (!ctrl->parent) {
+			intc->reg_pending = reg_base + ctrl->offset;
+			intc->reg_mask = reg_base + ctrl->offset + 0x08;
+			intc->reg_intpnd = reg_base + ctrl->offset + 0x10;
+		} else {
+			intc->reg_pending = reg_base + ctrl->offset;
+			intc->reg_mask = reg_base + ctrl->offset + 0x4;
+		}
+
+		/* now that all the data is complete, init the irq-domain */
+		s3c24xx_clear_intc(intc);
+		intc->domain = irq_domain_add_linear(intc_node, 32,
+						     &s3c24xx_irq_ops, intc);
+		if (!intc->domain) {
+			pr_err("irq: could not create irq-domain\n");
+			kfree(intc);
+			of_node_put(intc_node);
+			continue;
+		}
+
+		if (ctrl->handle)
+			*(ctrl->handle) = intc;
+	}
+
+	set_handle_irq(s3c24xx_handle_irq);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(s3c2410_irq, "samsung,s3c2410-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2412_irq, "samsung,s3c2412-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2416_irq, "samsung,s3c2416-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2440_irq, "samsung,s3c2440-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2442_irq, "samsung,s3c2442-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2443_irq, "samsung,s3c2443-irq", s3c24xx_init_intc_of);
+IRQCHIP_DECLARE(s3c2450_irq, "samsung,s3c2450-irq", s3c24xx_init_intc_of);
+#endif
-- 
1.7.2.3

Re: [PATCH v3 5/6] irqchip: s3c24xx: add devicetree support

[Heiko Stübner]

Am Sonntag, 17. März 2013, 14:07:58 schrieb Heiko Stübner:
> Add the necessary code to initialize the interrupt controller
> thru devicetree data using the irqchip infrastructure.
> 
> On dt machines the eint-type interrupts in the main interrupt controller
> get mapped as regular edge-types, as their wakeup and interrupt type
> properties will be handled by the upcoming pinctrl driver.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../interrupt-controller/samsung,s3c24xx-irq.txt   |   54 +++++
>  drivers/irqchip/irq-s3c24xx.c                      |  222
> ++++++++++++++++++++ 2 files changed, 276 insertions(+), 0 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq
> .txt
> 
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-i
> rq.txt
> b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-i
> rq.txt new file mode 100644
> index 0000000..be5dead
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-i
> rq.txt @@ -0,0 +1,54 @@
> +Samsung S3C24XX Interrupt Controllers
> +
> +The S3C24XX SoCs contain a custom set of interrupt controllers providing a
> +varying number of interrupt sources. The set consists of a main- and sub-
> +controller and on newer SoCs even a second main controller.
> +
> +Required properties:
> +- compatible: Compatible property value should be one of
> "samsung,s3c2410-irq", +  "samsung,s3c2412-irq", "samsung,s3c2416-irq",
> "samsung,s3c2440-irq", +  "samsung,s3c2442-irq", "samsung,s3c2443-irq"
> depending on the SoC variant. +
> +- reg: Physical base address of the controller and length of memory mapped
> +  region.
> +
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +Sub-controllers as child nodes:
> +  The interrupt controllers that should be referenced by device nodes are
> +  represented by child nodes. Valid names are intc, subintc and intc2.
> +  The interrupt values in device nodes are then mapped directly to the
> +  bit-numbers of the pending register of the named interrupt controller.
> +
> +Required properties:
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 2.

just noticed, that with the eint handling moving to the pinctrl driver, there 
are no interrupt trigger settings to be made here anymore, so this can 
probably move to xlate_onecell and #interrupt-cells to <1>.

Re: [PATCH v3 5/6] irqchip: s3c24xx: add devicetree support

[Rob Herring]

On 03/17/2013 08:07 AM, Heiko Stübner wrote:
> Add the necessary code to initialize the interrupt controller
> thru devicetree data using the irqchip infrastructure.
> 
> On dt machines the eint-type interrupts in the main interrupt controller
> get mapped as regular edge-types, as their wakeup and interrupt type
> properties will be handled by the upcoming pinctrl driver.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../interrupt-controller/samsung,s3c24xx-irq.txt   |   54 +++++
>  drivers/irqchip/irq-s3c24xx.c                      |  222 ++++++++++++++++++++
>  2 files changed, 276 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
> new file mode 100644
> index 0000000..be5dead
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
> @@ -0,0 +1,54 @@
> +Samsung S3C24XX Interrupt Controllers
> +
> +The S3C24XX SoCs contain a custom set of interrupt controllers providing a
> +varying number of interrupt sources. The set consists of a main- and sub-
> +controller and on newer SoCs even a second main controller.
> +
> +Required properties:
> +- compatible: Compatible property value should be one of "samsung,s3c2410-irq",
> +  "samsung,s3c2412-irq", "samsung,s3c2416-irq", "samsung,s3c2440-irq",
> +  "samsung,s3c2442-irq", "samsung,s3c2443-irq" depending on the SoC variant.
> +
> +- reg: Physical base address of the controller and length of memory mapped
> +  region.
> +
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +Sub-controllers as child nodes:
> +  The interrupt controllers that should be referenced by device nodes are
> +  represented by child nodes. Valid names are intc, subintc and intc2.
> +  The interrupt values in device nodes are then mapped directly to the
> +  bit-numbers of the pending register of the named interrupt controller.
> +
> +Required properties:
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 2.
> +
> +Example:
> +
> +	interrupt-controller@4a000000 {
> +		compatible = "samsung,s3c2416-irq";
> +		reg = <0x4a000000 0x100>;
> +		interrupt-controller;
> +
> +		intc:intc {
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		subintc:subintc {
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> +	[...]
> +
> +	serial@50000000 {
> +		compatible = "samsung,s3c2410-uart";
> +		reg = <0x50000000 0x4000>;
> +		interrupt-parent = <&subintc>;
> +		interrupts = <0 0>, <1 0>;
> +	};
> diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
> index 1eba289..55cb363 100644
> --- a/drivers/irqchip/irq-s3c24xx.c
> +++ b/drivers/irqchip/irq-s3c24xx.c
> @@ -25,6 +25,9 @@
>  #include <linux/ioport.h>
>  #include <linux/device.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
>  
>  #include <asm/exception.h>
>  #include <asm/mach/irq.h>
> @@ -36,6 +39,8 @@
>  #include <plat/regs-irqtype.h>
>  #include <plat/pm.h>
>  
> +#include "irqchip.h"
> +
>  #define S3C_IRQTYPE_NONE	0
>  #define S3C_IRQTYPE_EINT	1
>  #define S3C_IRQTYPE_EDGE	2
> @@ -380,6 +385,10 @@ static int s3c24xx_irq_map(struct irq_domain *h, unsigned int virq,
>  
>  	parent_intc = intc->parent;
>  
> +	/* on dt platforms the extints get handled by the pinctrl driver */
> +	if (h->of_node && irq_data->type == S3C_IRQTYPE_EINT)
> +		irq_data->type = S3C_IRQTYPE_EDGE;
> +
>  	/* set handler and flags */
>  	switch (irq_data->type) {
>  	case S3C_IRQTYPE_NONE:
> @@ -1104,3 +1113,216 @@ void __init s3c2443_init_irq(void)
>  	s3c24xx_init_intc(NULL, &init_s3c2443subint[0], main_intc, 0x4a000018);
>  }
>  #endif
> +
> +#ifdef CONFIG_OF
> +struct s3c24xx_irq_of_ctrl {
> +	char			*name;
> +	unsigned long		offset;
> +	struct s3c_irq_data	*irq_data;
> +	struct s3c_irq_intc	**handle;
> +	struct s3c_irq_intc	**parent;
> +};
> +
> +#define S3C24XX_IRQCTRL(n, o, d, h, p)		\
> +{						\
> +	.name		= n,			\
> +	.offset		= o,			\
> +	.irq_data	= d,			\
> +	.handle		= h,			\
> +	.parent		= p,			\
> +}
> +
> +struct s3c24xx_irq_of_data {
> +	struct s3c24xx_irq_of_ctrl	*irq_ctrl;
> +	int				num_ctrl;
> +};
> +
> +#ifdef CONFIG_CPU_S3C2410
> +static struct s3c24xx_irq_of_ctrl s3c2410_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2410base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2410subint, NULL, &main_intc),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2410_irq_data = {
> +	.irq_ctrl = s3c2410_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2410_ctrl),
> +};
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2412
> +static struct s3c24xx_irq_of_ctrl s3c2412_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2412base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2412subint, NULL, &main_intc),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2412_irq_data = {
> +	.irq_ctrl = s3c2412_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2412_ctrl),
> +};
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2416
> +static struct s3c24xx_irq_of_ctrl s3c2416_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2416base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2416subint, NULL, &main_intc),
> +	S3C24XX_IRQCTRL("intc2", 0x40, init_s3c2416_second, &main_intc2, NULL),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2416_irq_data = {
> +	.irq_ctrl = s3c2416_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2416_ctrl),
> +};
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2440
> +static struct s3c24xx_irq_of_ctrl s3c2440_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2440base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2440subint, NULL, &main_intc),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2440_irq_data = {
> +	.irq_ctrl = s3c2440_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2440_ctrl),
> +};
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2442
> +static struct s3c24xx_irq_of_ctrl s3c2442_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2442base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2442subint, NULL, &main_intc),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2442_irq_data = {
> +	.irq_ctrl = s3c2442_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2442_ctrl),
> +};
> +#endif
> +
> +#ifdef CONFIG_CPU_S3C2443
> +static struct s3c24xx_irq_of_ctrl s3c2443_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2443base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2443subint, NULL, &main_intc),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2443_irq_data = {
> +	.irq_ctrl = s3c2443_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2443_ctrl),
> +};
> +#endif
> +
> +static const struct of_device_id intc_list[] = {
> +#ifdef CONFIG_CPU_S3C2410
> +	{ .compatible = "samsung,s3c2410-irq", .data = &s3c2410_irq_data },
> +#endif
> +#ifdef CONFIG_CPU_S3C2412
> +	{ .compatible = "samsung,s3c2412-irq", .data = &s3c2412_irq_data },
> +#endif
> +#ifdef CONFIG_CPU_S3C2416
> +	{ .compatible = "samsung,s3c2416-irq", .data = &s3c2416_irq_data },
> +#endif
> +#ifdef CONFIG_CPU_S3C2440
> +	{ .compatible = "samsung,s3c2440-irq", .data = &s3c2440_irq_data },
> +#endif
> +#ifdef CONFIG_CPU_S3C2442
> +	{ .compatible = "samsung,s3c2442-irq", .data = &s3c2442_irq_data },
> +#endif
> +#ifdef CONFIG_CPU_S3C2443
> +	{ .compatible = "samsung,s3c2443-irq", .data = &s3c2443_irq_data },
> +#endif
> +	{},
> +};
> +
> +int __init s3c24xx_init_intc_of(struct device_node *np,
> +				 struct device_node *interrupt_parent)
> +{
> +	const struct of_device_id *match;
> +	const struct s3c24xx_irq_of_data *ctrl_data;
> +	struct device_node *intc_node;
> +	struct s3c24xx_irq_of_ctrl *ctrl;
> +	struct s3c_irq_intc *intc;
> +	void __iomem *reg_base;
> +	int i;
> +
> +	match = of_match_node(intc_list, np);

Rather than matching twice, create a wrapper function that passes in the
necessary data pointer. Something like this:

s3c2410_init_intc_of(struct device_node *np,
				 struct device_node *interrupt_parent)
{
	return s3c24xx_init_intc_of(np, interrupt_parent, &s3c2410_irq_data);
}

> +	if (!match) {
> +		pr_err("irq-s3c24xx: could not find matching irqdata\n");
> +		return -EINVAL;
> +	}
> +
> +	ctrl_data = match->data;
> +
> +	reg_base = of_iomap(np, 0);
> +	if (!reg_base) {
> +		pr_err("irq-s3c24xx: could not map irq memory\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ctrl_data->num_ctrl; i++) {
> +		ctrl = &ctrl_data->irq_ctrl[i];
> +
> +		intc_node = of_find_node_by_name(np, ctrl->name);
> +		if (!intc_node) {
> +			pr_debug("irq: no device node for %s\n",
> +				ctrl->name);
> +			continue;
> +		}
> +
> +		intc = kzalloc(sizeof(struct s3c_irq_intc), GFP_KERNEL);
> +		if (!intc) {
> +			of_node_put(intc_node);
> +			return -ENOMEM;
> +		}
> +
> +		pr_debug("irq: found controller %s\n", ctrl->name);
> +
> +		intc->irqs = ctrl->irq_data;
> +
> +		if (ctrl->parent) {
> +			if (*(ctrl->parent)) {
> +				intc->parent = *(ctrl->parent);
> +			} else {
> +				pr_warn("irq: parent of %s missing\n",
> +					ctrl->name);
> +				kfree(intc);
> +				of_node_put(intc_node);
> +				continue;
> +			}
> +		}
> +
> +		if (!ctrl->parent) {
> +			intc->reg_pending = reg_base + ctrl->offset;
> +			intc->reg_mask = reg_base + ctrl->offset + 0x08;
> +			intc->reg_intpnd = reg_base + ctrl->offset + 0x10;
> +		} else {
> +			intc->reg_pending = reg_base + ctrl->offset;
> +			intc->reg_mask = reg_base + ctrl->offset + 0x4;
> +		}

These if statements could be simplified some (move this else to the "if
(ctrl->parent)" above).

> +
> +		/* now that all the data is complete, init the irq-domain */
> +		s3c24xx_clear_intc(intc);
> +		intc->domain = irq_domain_add_linear(intc_node, 32,
> +						     &s3c24xx_irq_ops, intc);
> +		if (!intc->domain) {
> +			pr_err("irq: could not create irq-domain\n");
> +			kfree(intc);
> +			of_node_put(intc_node);
> +			continue;
> +		}
> +
> +		if (ctrl->handle)
> +			*(ctrl->handle) = intc;
> +	}
> +
> +	set_handle_irq(s3c24xx_handle_irq);
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(s3c2410_irq, "samsung,s3c2410-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2412_irq, "samsung,s3c2412-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2416_irq, "samsung,s3c2416-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2440_irq, "samsung,s3c2440-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2442_irq, "samsung,s3c2442-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2443_irq, "samsung,s3c2443-irq", s3c24xx_init_intc_of);
> +IRQCHIP_DECLARE(s3c2450_irq, "samsung,s3c2450-irq", s3c24xx_init_intc_of);
> +#endif
> 

[PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Heiko Stübner]

The s3c2450 is special in that it shares the cpu identification with the
s3c2416 but provides more interrupts for its additional components.

It also shares the layout of the main interrupt register with the s3c2443
and therefore reuses this definition.

As no non-dt boards are present, the s3c2450 irqs will only be
accessible thru devicetree.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/irqchip/irq-s3c24xx.c |   62 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index 55cb363..7ddf8e8 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -852,6 +852,51 @@ static struct s3c_irq_data init_s3c2416_second[32] = {
 	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
 };
 
+static struct s3c_irq_data init_s3c2450subint[32] = {
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-ERR */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-RX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-TX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-ERR */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-RX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-TX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-ERR */
+	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* TC */
+	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* ADC */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_C */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_P */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD2 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD3 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD4 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA0 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA1 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA2 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA3 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
+	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */
+};
+
+static struct s3c_irq_data init_s3c2450second[32] = {
+	{ .type = S3C_IRQTYPE_EDGE }, /* 2D */
+	{ .type = S3C_IRQTYPE_EDGE }, /* IIC1 */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
+	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
+	{ .type = S3C_IRQTYPE_EDGE }, /* PCM0 */
+	{ .type = S3C_IRQTYPE_EDGE }, /* PCM1 */
+	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
+	{ .type = S3C_IRQTYPE_EDGE }, /* I2S1 */
+};
+
 void __init s3c2416_init_irq(void)
 {
 	struct s3c_irq_intc *main_intc;
@@ -1024,7 +1069,7 @@ void __init s3c2442_init_irq(void)
 }
 #endif
 
-#ifdef CONFIG_CPU_S3C2443
+#if defined(CONFIG_CPU_S3C2443) || defined(CONFIG_CPU_S3C2416)
 static struct s3c_irq_data init_s3c2443base[32] = {
 	{ .type = S3C_IRQTYPE_EINT, }, /* EINT0 */
 	{ .type = S3C_IRQTYPE_EINT, }, /* EINT1 */
@@ -1059,8 +1104,9 @@ static struct s3c_irq_data init_s3c2443base[32] = {
 	{ .type = S3C_IRQTYPE_EDGE, }, /* RTC */
 	{ .type = S3C_IRQTYPE_LEVEL, }, /* ADCPARENT */
 };
+#endif
 
-
+#ifdef CONFIG_CPU_S3C2443
 static struct s3c_irq_data init_s3c2443subint[32] = {
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
 	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
@@ -1172,6 +1218,17 @@ struct s3c24xx_irq_of_data s3c2416_irq_data = {
 	.irq_ctrl = s3c2416_ctrl,
 	.num_ctrl = ARRAY_SIZE(s3c2416_ctrl),
 };
+
+static struct s3c24xx_irq_of_ctrl s3c2450_ctrl[] = {
+	S3C24XX_IRQCTRL("intc", 0, init_s3c2443base, &main_intc, NULL),
+	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2450subint, NULL, &main_intc),
+	S3C24XX_IRQCTRL("intc2", 0x40, init_s3c2450second, &main_intc2, NULL),
+};
+
+static struct s3c24xx_irq_of_data s3c2450_irq_data = {
+	.irq_ctrl = s3c2450_ctrl,
+	.num_ctrl = ARRAY_SIZE(s3c2450_ctrl),
+};
 #endif
 
 #ifdef CONFIG_CPU_S3C2440
@@ -1219,6 +1276,7 @@ static const struct of_device_id intc_list[] = {
 #endif
 #ifdef CONFIG_CPU_S3C2416
 	{ .compatible = "samsung,s3c2416-irq", .data = &s3c2416_irq_data },
+	{ .compatible = "samsung,s3c2450-irq", .data = &s3c2450_irq_data },
 #endif
 #ifdef CONFIG_CPU_S3C2440
 	{ .compatible = "samsung,s3c2440-irq", .data = &s3c2440_irq_data },
-- 
1.7.2.3

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Rob Herring]

On 03/17/2013 08:09 AM, Heiko Stübner wrote:
> The s3c2450 is special in that it shares the cpu identification with the
> s3c2416 but provides more interrupts for its additional components.
> 
> It also shares the layout of the main interrupt register with the s3c2443
> and therefore reuses this definition.
> 
> As no non-dt boards are present, the s3c2450 irqs will only be
> accessible thru devicetree.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/irqchip/irq-s3c24xx.c |   62 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
> index 55cb363..7ddf8e8 100644
> --- a/drivers/irqchip/irq-s3c24xx.c
> +++ b/drivers/irqchip/irq-s3c24xx.c
> @@ -852,6 +852,51 @@ static struct s3c_irq_data init_s3c2416_second[32] = {
>  	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
>  };
>  
> +static struct s3c_irq_data init_s3c2450subint[32] = {
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-ERR */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-RX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-TX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-ERR */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-RX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-TX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-ERR */
> +	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* TC */
> +	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* ADC */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_C */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_P */
> +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD2 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD3 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD4 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA0 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA1 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA2 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA3 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */

This all seems like information that should come from DT.

> +};
> +
> +static struct s3c_irq_data init_s3c2450second[32] = {
> +	{ .type = S3C_IRQTYPE_EDGE }, /* 2D */
> +	{ .type = S3C_IRQTYPE_EDGE }, /* IIC1 */
> +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> +	{ .type = S3C_IRQTYPE_EDGE }, /* PCM0 */
> +	{ .type = S3C_IRQTYPE_EDGE }, /* PCM1 */
> +	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
> +	{ .type = S3C_IRQTYPE_EDGE }, /* I2S1 */
> +};
> +
>  void __init s3c2416_init_irq(void)
>  {
>  	struct s3c_irq_intc *main_intc;
> @@ -1024,7 +1069,7 @@ void __init s3c2442_init_irq(void)
>  }
>  #endif
>  
> -#ifdef CONFIG_CPU_S3C2443
> +#if defined(CONFIG_CPU_S3C2443) || defined(CONFIG_CPU_S3C2416)
>  static struct s3c_irq_data init_s3c2443base[32] = {
>  	{ .type = S3C_IRQTYPE_EINT, }, /* EINT0 */
>  	{ .type = S3C_IRQTYPE_EINT, }, /* EINT1 */
> @@ -1059,8 +1104,9 @@ static struct s3c_irq_data init_s3c2443base[32] = {
>  	{ .type = S3C_IRQTYPE_EDGE, }, /* RTC */
>  	{ .type = S3C_IRQTYPE_LEVEL, }, /* ADCPARENT */
>  };
> +#endif
>  
> -
> +#ifdef CONFIG_CPU_S3C2443
>  static struct s3c_irq_data init_s3c2443subint[32] = {
>  	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
>  	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
> @@ -1172,6 +1218,17 @@ struct s3c24xx_irq_of_data s3c2416_irq_data = {
>  	.irq_ctrl = s3c2416_ctrl,
>  	.num_ctrl = ARRAY_SIZE(s3c2416_ctrl),
>  };
> +
> +static struct s3c24xx_irq_of_ctrl s3c2450_ctrl[] = {
> +	S3C24XX_IRQCTRL("intc", 0, init_s3c2443base, &main_intc, NULL),
> +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2450subint, NULL, &main_intc),
> +	S3C24XX_IRQCTRL("intc2", 0x40, init_s3c2450second, &main_intc2, NULL),
> +};
> +
> +static struct s3c24xx_irq_of_data s3c2450_irq_data = {
> +	.irq_ctrl = s3c2450_ctrl,
> +	.num_ctrl = ARRAY_SIZE(s3c2450_ctrl),
> +};
>  #endif
>  
>  #ifdef CONFIG_CPU_S3C2440
> @@ -1219,6 +1276,7 @@ static const struct of_device_id intc_list[] = {
>  #endif
>  #ifdef CONFIG_CPU_S3C2416
>  	{ .compatible = "samsung,s3c2416-irq", .data = &s3c2416_irq_data },
> +	{ .compatible = "samsung,s3c2450-irq", .data = &s3c2450_irq_data },

Why are you not using IRQCHIP_OF_DECLARE?

Rob

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Heiko Stübner]

Hi Rob,

Am Montag, 18. März 2013, 16:54:03 schrieb Rob Herring:
> On 03/17/2013 08:09 AM, Heiko Stübner wrote:
> > The s3c2450 is special in that it shares the cpu identification with the
> > s3c2416 but provides more interrupts for its additional components.
> > 
> > It also shares the layout of the main interrupt register with the s3c2443
> > and therefore reuses this definition.
> > 
> > As no non-dt boards are present, the s3c2450 irqs will only be
> > accessible thru devicetree.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >  drivers/irqchip/irq-s3c24xx.c |   62
> >  +++++++++++++++++++++++++++++++++++++++- 1 files changed, 60
> >  insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-s3c24xx.c
> > b/drivers/irqchip/irq-s3c24xx.c index 55cb363..7ddf8e8 100644
> > --- a/drivers/irqchip/irq-s3c24xx.c
> > +++ b/drivers/irqchip/irq-s3c24xx.c
> > @@ -852,6 +852,51 @@ static struct s3c_irq_data init_s3c2416_second[32] =
> > {
> > 
> >  	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
> >  
> >  };
> > 
> > +static struct s3c_irq_data init_s3c2450subint[32] = {
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-ERR */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-RX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-TX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 23 }, /* UART1-ERR */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-RX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-TX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 15 }, /* UART2-ERR */
> > +	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* TC */
> > +	{ .type = S3C_IRQTYPE_EDGE, .parent_irq = 31 }, /* ADC */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_C */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 6 }, /* CAM_P */
> > +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> > +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD2 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD3 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 16 }, /* LCD4 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA0 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA1 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA2 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA3 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
> > +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */
> 
> This all seems like information that should come from DT.

In the first iterations [0] of theis series it was done this way, but was 
suggested that these informations _might_ be implementation specific and not 
describing the hardware.

As I didn't get any "final" feedback on the matter, I tried it this way this 
time. Personally I also did like the previous variant better, as the driver 
could loose all the declaration stuff when platforms move to dt.

I would be glad for a hint if the first approach was the correct one.


[0] "irqchip: irq-s3c24xx: add devicetree support" from 2013-02-18, also with 
you in the recipient list

> 
> > +};
> > +
> > +static struct s3c_irq_data init_s3c2450second[32] = {
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* 2D */
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* IIC1 */
> > +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> > +	{ .type = S3C_IRQTYPE_NONE }, /* reserved */
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* PCM0 */
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* PCM1 */
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* I2S0 */
> > +	{ .type = S3C_IRQTYPE_EDGE }, /* I2S1 */
> > +};
> > +
> > 
> >  void __init s3c2416_init_irq(void)
> >  {
> >  
> >  	struct s3c_irq_intc *main_intc;
> > 
> > @@ -1024,7 +1069,7 @@ void __init s3c2442_init_irq(void)
> > 
> >  }
> >  #endif
> > 
> > -#ifdef CONFIG_CPU_S3C2443
> > +#if defined(CONFIG_CPU_S3C2443) || defined(CONFIG_CPU_S3C2416)
> > 
> >  static struct s3c_irq_data init_s3c2443base[32] = {
> >  
> >  	{ .type = S3C_IRQTYPE_EINT, }, /* EINT0 */
> >  	{ .type = S3C_IRQTYPE_EINT, }, /* EINT1 */
> > 
> > @@ -1059,8 +1104,9 @@ static struct s3c_irq_data init_s3c2443base[32] = {
> > 
> >  	{ .type = S3C_IRQTYPE_EDGE, }, /* RTC */
> >  	{ .type = S3C_IRQTYPE_LEVEL, }, /* ADCPARENT */
> >  
> >  };
> > 
> > +#endif
> > 
> > -
> > +#ifdef CONFIG_CPU_S3C2443
> > 
> >  static struct s3c_irq_data init_s3c2443subint[32] = {
> >  
> >  	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-RX */
> >  	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 28 }, /* UART0-TX */
> > 
> > @@ -1172,6 +1218,17 @@ struct s3c24xx_irq_of_data s3c2416_irq_data = {
> > 
> >  	.irq_ctrl = s3c2416_ctrl,
> >  	.num_ctrl = ARRAY_SIZE(s3c2416_ctrl),
> >  
> >  };
> > 
> > +
> > +static struct s3c24xx_irq_of_ctrl s3c2450_ctrl[] = {
> > +	S3C24XX_IRQCTRL("intc", 0, init_s3c2443base, &main_intc, NULL),
> > +	S3C24XX_IRQCTRL("subintc", 0x18, init_s3c2450subint, NULL, 
&main_intc),
> > +	S3C24XX_IRQCTRL("intc2", 0x40, init_s3c2450second, &main_intc2, 
NULL),
> > +};
> > +
> > +static struct s3c24xx_irq_of_data s3c2450_irq_data = {
> > +	.irq_ctrl = s3c2450_ctrl,
> > +	.num_ctrl = ARRAY_SIZE(s3c2450_ctrl),
> > +};
> > 
> >  #endif
> >  
> >  #ifdef CONFIG_CPU_S3C2440
> > 
> > @@ -1219,6 +1276,7 @@ static const struct of_device_id intc_list[] = {
> > 
> >  #endif
> >  #ifdef CONFIG_CPU_S3C2416
> >  
> >  	{ .compatible = "samsung,s3c2416-irq", .data = &s3c2416_irq_data },
> > 
> > +	{ .compatible = "samsung,s3c2450-irq", .data = &s3c2450_irq_data },
> 
> Why are you not using IRQCHIP_OF_DECLARE?

As you can see in patch 5/6 the driver itself is using irqchip_declare, which 
I seem to have forgotten here. This matching table is simply meant to help the 
common init function to select the correct mapping table.

All this stuff can of course go away if the correct way is to gather this 
information from dt.


Thanks
Heiko

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Rob Herring]

On 03/18/2013 11:53 AM, Heiko Stübner wrote:
> Hi Rob,
> 
> Am Montag, 18. März 2013, 16:54:03 schrieb Rob Herring:
>> On 03/17/2013 08:09 AM, Heiko Stübner wrote:
>>> The s3c2450 is special in that it shares the cpu identification with the
>>> s3c2416 but provides more interrupts for its additional components.
>>>
>>> It also shares the layout of the main interrupt register with the s3c2443
>>> and therefore reuses this definition.
>>>
>>> As no non-dt boards are present, the s3c2450 irqs will only be
>>> accessible thru devicetree.

[snip]

>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */
>>
>> This all seems like information that should come from DT.
> 
> In the first iterations [0] of theis series it was done this way, but was 
> suggested that these informations _might_ be implementation specific and not 
> describing the hardware.
> 
> As I didn't get any "final" feedback on the matter, I tried it this way this 
> time. Personally I also did like the previous variant better, as the driver 
> could loose all the declaration stuff when platforms move to dt.
> 
> I would be glad for a hint if the first approach was the correct one.
> 
> 
> [0] "irqchip: irq-s3c24xx: add devicetree support" from 2013-02-18, also with 
> you in the recipient list

I'm inclined to say the prior way is more in the right direction.
However, I'm not really clear what you are trying to describe.

> +		s3c24xx,irqlist = <2 0 /* 2D */
> +				   2 0 /* IIC1 */
> +				   0 0 /* reserved */
> +				   0 0 /* reserved */
> +				   2 0 /* PCM0 */
> +				   2 0 /* PCM1 */
> +				   2 0 /* I2S0 */
> +				   2 0>; /* I2S1 */

My first thought here is this information should not be centralized in
the controller node, but placed with each source node (2D, I2C1, etc).

Rob

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Arnd Bergmann]

On Monday 18 March 2013 17:14:52 Rob Herring wrote:
> 
> > +             s3c24xx,irqlist = <2 0 /* 2D */
> > +                                2 0 /* IIC1 */
> > +                                0 0 /* reserved */
> > +                                0 0 /* reserved */
> > +                                2 0 /* PCM0 */
> > +                                2 0 /* PCM1 */
> > +                                2 0 /* I2S0 */
> > +                                2 0>; /* I2S1 */
> 
> My first thought here is this information should not be centralized in
> the controller node, but placed with each source node (2D, I2C1, etc).

Seconded. Let's do this the same way we have it for all other irq chips
and put it all into the irq descriptor referring to the controller, not
the controller.

	Arnd

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Heiko Stübner]

Am Montag, 18. März 2013, 23:14:52 schrieb Rob Herring:
> On 03/18/2013 11:53 AM, Heiko Stübner wrote:
> > Hi Rob,
> > 
> > Am Montag, 18. März 2013, 16:54:03 schrieb Rob Herring:
> >> On 03/17/2013 08:09 AM, Heiko Stübner wrote:
> >>> The s3c2450 is special in that it shares the cpu identification with
> >>> the s3c2416 but provides more interrupts for its additional
> >>> components.
> >>> 
> >>> It also shares the layout of the main interrupt register with the
> >>> s3c2443 and therefore reuses this definition.
> >>> 
> >>> As no non-dt boards are present, the s3c2450 irqs will only be
> >>> accessible thru devicetree.
> 
> [snip]
> 
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
> >>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */
> >> 
> >> This all seems like information that should come from DT.
> > 
> > In the first iterations [0] of theis series it was done this way, but was
> > suggested that these informations _might_ be implementation specific and
> > not describing the hardware.
> > 
> > As I didn't get any "final" feedback on the matter, I tried it this way
> > this time. Personally I also did like the previous variant better, as
> > the driver could loose all the declaration stuff when platforms move to
> > dt.
> > 
> > I would be glad for a hint if the first approach was the correct one.
> > 
> > 
> > [0] "irqchip: irq-s3c24xx: add devicetree support" from 2013-02-18, also
> > with you in the recipient list
> 
> I'm inclined to say the prior way is more in the right direction.
> However, I'm not really clear what you are trying to describe.
> 
> > +		s3c24xx,irqlist = <2 0 /* 2D */
> > +				   2 0 /* IIC1 */
> > +				   0 0 /* reserved */
> > +				   0 0 /* reserved */
> > +				   2 0 /* PCM0 */
> > +				   2 0 /* PCM1 */
> > +				   2 0 /* I2S0 */
> > +				   2 0>; /* I2S1 */

The s3c24xx SoCs contain one (or two) main interrupt controllers. That are the 
nodes that gets checked by handle_irq. Additionally they contain something 
called a sub-register which provides more specific irq for some of them.

The list you quoted, is meant to describe which bits of the interrupt register 
contain a valid interrupt at all (!= 0), which handler should be used (2 for 
the edge handler, 3 for level handler). The second argument contains the bit 
in the parent interrupt register that contains the parent interrupt, that gets 
checked in the irq_entry function.

The original code (which I reworked into this more declarative form) 
distinguished very much between using the edge handler for some and the level 
handler for other interrupts.

This list is essentially the same representation of the list you quoted above 
and which also can be found in [0]

Going this way makes it easy to map datasheet values to interrupt number. When 
I read the ac97 interrupt is in bit 28 of the sub-register I can simply 
reference it as
		interrupt-parent = <&subintc>;
		interrupts = <28>;


So these lists only desribe the internal structure of the interrupt controller 
registers, which vary for each s3c24xx variant.


> My first thought here is this information should not be centralized in
> the controller node, but placed with each source node (2D, I2C1, etc).

I'm not sure I can follow currently :-)

I'll try an example:

The s3c serial driver expects the interrupts for uart tx and rx and the dt 
entry would look like:

	serial@50000000 {
		compatible = "samsung,s3c2410-uart";
		reg = <0x50000000 0x4000>;
		interrupt-parent = <&subintc>;
		interrupts = <0>, <1>;
	};

the map for these in the subintc looks like
               s3c24xx,irqlist = <3 28 /* UART0-RX */
                                  3 28 /* UART0-TX */
                                  3 28 /* UART0-ERR */

marking them as using the level-handler and being children of the interrupt in 
bit 28 of the intc handler.

So the irq_entry would check the intc, see the waiting interrupt in bit 28, 
jump to the demux function which would then handle which ever of rx,tx or err 
would be waiting, which then get sent to the serial driver.

These mappings between sub- and parent irqs also vary very much between the 
different s3c24xx variants, as can be seen by the multitude of lists in [0] 
and the parent interrupts are only used for demuxing purposes.

-----
A notable speciality are the AC97 (sound) and watchdog interrupts (bits 27 and 
28 of the subregister), as they share a common parent interrupt (bit 9 of the 
main interrupt register).

So irq_entry checks the main register, sees bit 9 (ac97/watchdog), demuxes it 
to either coming from the ac97 or watchdog and sends it to the relevant 
driver.

I don't think this should be exposed to the drivers at all :-) .
-------

So I'm not sure, how this would be able to go in the driver nodes.


The only thing I could think of, would be to make the handler adjustable via 
the regular interrupts properties (i.e. as two_cell) which would bring the 
list down to only list the parent relationships.

Hmm ... and this parent list might be doable via the regular interrupts 
property, so the subintc would look like:

subintc: subintc = {
	interrupt-parent = <&intc>;
	interrupts = <28 0>, <28 0>, <28 0>, <23 0>, <23 0>, .....
	/*		     bit0    bit1    bit2    bit3    bit4   ..... */
}

i.e. naming the parent interrupt for each of the interrupt bits of the sub-
controller. Would this be the right direction?



[0] https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-
samsung.git/tree/arch/arm/mach-s3c24xx/irq.c?h=for-next#n603

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Rob Herring]

On 03/18/2013 06:34 PM, Heiko Stübner wrote:
> Am Montag, 18. März 2013, 23:14:52 schrieb Rob Herring:
>> On 03/18/2013 11:53 AM, Heiko Stübner wrote:
>>> Hi Rob,
>>>
>>> Am Montag, 18. März 2013, 16:54:03 schrieb Rob Herring:
>>>> On 03/17/2013 08:09 AM, Heiko Stübner wrote:
>>>>> The s3c2450 is special in that it shares the cpu identification with
>>>>> the s3c2416 but provides more interrupts for its additional
>>>>> components.
>>>>>
>>>>> It also shares the layout of the main interrupt register with the
>>>>> s3c2443 and therefore reuses this definition.
>>>>>
>>>>> As no non-dt boards are present, the s3c2450 irqs will only be
>>>>> accessible thru devicetree.
>>
>> [snip]
>>
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA4 */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA5 */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-RX */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-TX */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 18 }, /* UART3-ERR */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* WDT */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 9 }, /* AC97 */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA6 */
>>>>> +	{ .type = S3C_IRQTYPE_LEVEL, .parent_irq = 17 }, /* DMA7 */
>>>>
>>>> This all seems like information that should come from DT.
>>>
>>> In the first iterations [0] of theis series it was done this way, but was
>>> suggested that these informations _might_ be implementation specific and
>>> not describing the hardware.
>>>
>>> As I didn't get any "final" feedback on the matter, I tried it this way
>>> this time. Personally I also did like the previous variant better, as
>>> the driver could loose all the declaration stuff when platforms move to
>>> dt.
>>>
>>> I would be glad for a hint if the first approach was the correct one.
>>>
>>>
>>> [0] "irqchip: irq-s3c24xx: add devicetree support" from 2013-02-18, also
>>> with you in the recipient list
>>
>> I'm inclined to say the prior way is more in the right direction.
>> However, I'm not really clear what you are trying to describe.
>>
>>> +		s3c24xx,irqlist = <2 0 /* 2D */
>>> +				   2 0 /* IIC1 */
>>> +				   0 0 /* reserved */
>>> +				   0 0 /* reserved */
>>> +				   2 0 /* PCM0 */
>>> +				   2 0 /* PCM1 */
>>> +				   2 0 /* I2S0 */
>>> +				   2 0>; /* I2S1 */
> 
> The s3c24xx SoCs contain one (or two) main interrupt controllers. That are the 
> nodes that gets checked by handle_irq. Additionally they contain something 
> called a sub-register which provides more specific irq for some of them.
> 
> The list you quoted, is meant to describe which bits of the interrupt register 
> contain a valid interrupt at all (!= 0), which handler should be used (2 for 
> the edge handler, 3 for level handler). The second argument contains the bit 
> in the parent interrupt register that contains the parent interrupt, that gets 
> checked in the irq_entry function.
> 
> The original code (which I reworked into this more declarative form) 
> distinguished very much between using the edge handler for some and the level 
> handler for other interrupts.
> 
> This list is essentially the same representation of the list you quoted above 
> and which also can be found in [0]
> 
> Going this way makes it easy to map datasheet values to interrupt number. When 
> I read the ac97 interrupt is in bit 28 of the sub-register I can simply 
> reference it as
> 		interrupt-parent = <&subintc>;
> 		interrupts = <28>;
> 
> 
> So these lists only desribe the internal structure of the interrupt controller 
> registers, which vary for each s3c24xx variant.
> 
> 
>> My first thought here is this information should not be centralized in
>> the controller node, but placed with each source node (2D, I2C1, etc).
> 
> I'm not sure I can follow currently :-)
> 
> I'll try an example:
> 
> The s3c serial driver expects the interrupts for uart tx and rx and the dt 
> entry would look like:
> 
> 	serial@50000000 {
> 		compatible = "samsung,s3c2410-uart";
> 		reg = <0x50000000 0x4000>;
> 		interrupt-parent = <&subintc>;
> 		interrupts = <0>, <1>;

So what does 0 and 1 correspond to? These are the bits in the subintc?

> 	};
> 
> the map for these in the subintc looks like
>                s3c24xx,irqlist = <3 28 /* UART0-RX */
>                                   3 28 /* UART0-TX */
>                                   3 28 /* UART0-ERR */
> 
> marking them as using the level-handler and being children of the interrupt in 
> bit 28 of the intc handler.
> 
> So the irq_entry would check the intc, see the waiting interrupt in bit 28, 
> jump to the demux function which would then handle which ever of rx,tx or err 
> would be waiting, which then get sent to the serial driver.
> 
> These mappings between sub- and parent irqs also vary very much between the 
> different s3c24xx variants, as can be seen by the multitude of lists in [0] 
> and the parent interrupts are only used for demuxing purposes.
> 
> -----
> A notable speciality are the AC97 (sound) and watchdog interrupts (bits 27 and 
> 28 of the subregister), as they share a common parent interrupt (bit 9 of the 
> main interrupt register).
> 
> So irq_entry checks the main register, sees bit 9 (ac97/watchdog), demuxes it 
> to either coming from the ac97 or watchdog and sends it to the relevant 
> driver.
> 
> I don't think this should be exposed to the drivers at all :-) .
> -------
> 
> So I'm not sure, how this would be able to go in the driver nodes.

The information in an interrupts property is transparent to the driver,
but can contain extra cells with whatever information you need. The GIC
binding has SPI or PPI interrupt type information for example.

> The only thing I could think of, would be to make the handler adjustable via 
> the regular interrupts properties (i.e. as two_cell) which would bring the 
> list down to only list the parent relationships.
> 
> Hmm ... and this parent list might be doable via the regular interrupts 
> property, so the subintc would look like:
> 
> subintc: subintc = {
> 	interrupt-parent = <&intc>;
> 	interrupts = <28 0>, <28 0>, <28 0>, <23 0>, <23 0>, .....
> 	/*		     bit0    bit1    bit2    bit3    bit4   ..... */
> }
> 
> i.e. naming the parent interrupt for each of the interrupt bits of the sub-
> controller. Would this be the right direction?

I think you may want to use an interrupt-map property. That should allow
you to do arbitrary mappings from one interrupt controller's numbering
to another's numbering. The VExpress and several PPC platforms have
examples.

Rob

Re: [PATCH v3 6/6] irqchip: s3c24xx: add s3c2450 interrupt definitions

[Heiko Stübner]

Am Dienstag, 19. März 2013, 03:28:59 schrieb Rob Herring:
> On 03/18/2013 06:34 PM, Heiko Stübner wrote:
> > Am Montag, 18. März 2013, 23:14:52 schrieb Rob Herring:
> >> On 03/18/2013 11:53 AM, Heiko Stübner wrote:

[...]

> >> My first thought here is this information should not be centralized in
> >> the controller node, but placed with each source node (2D, I2C1, etc).
> > 
> > I'm not sure I can follow currently :-)
> > 
> > I'll try an example:
> > 
> > The s3c serial driver expects the interrupts for uart tx and rx and the
> > dt
> > 
> > entry would look like:
> > 	serial@50000000 {
> > 	
> > 		compatible = "samsung,s3c2410-uart";
> > 		reg = <0x50000000 0x4000>;
> > 		interrupt-parent = <&subintc>;
> > 		interrupts = <0>, <1>;
> 
> So what does 0 and 1 correspond to? These are the bits in the subintc?

Yep, the bits in the subintc register.


> > 	};
> > 
> > the map for these in the subintc looks like
> > 
> >                s3c24xx,irqlist = <3 28 /* UART0-RX */
> >                
> >                                   3 28 /* UART0-TX */
> >                                   3 28 /* UART0-ERR */
> > 
> > marking them as using the level-handler and being children of the
> > interrupt in bit 28 of the intc handler.
> > 
> > So the irq_entry would check the intc, see the waiting interrupt in bit
> > 28, jump to the demux function which would then handle which ever of
> > rx,tx or err would be waiting, which then get sent to the serial driver.
> > 
> > These mappings between sub- and parent irqs also vary very much between
> > the different s3c24xx variants, as can be seen by the multitude of lists
> > in [0] and the parent interrupts are only used for demuxing purposes.
> > 
> > -----
> > A notable speciality are the AC97 (sound) and watchdog interrupts (bits
> > 27 and 28 of the subregister), as they share a common parent interrupt
> > (bit 9 of the main interrupt register).
> > 
> > So irq_entry checks the main register, sees bit 9 (ac97/watchdog),
> > demuxes it to either coming from the ac97 or watchdog and sends it to
> > the relevant driver.
> > 
> > I don't think this should be exposed to the drivers at all :-) .
> > -------
> > 
> > So I'm not sure, how this would be able to go in the driver nodes.
> 
> The information in an interrupts property is transparent to the driver,
> but can contain extra cells with whatever information you need. The GIC
> binding has SPI or PPI interrupt type information for example.

so you mean something like <bit flags parent-bit>, right?


> > The only thing I could think of, would be to make the handler adjustable
> > via the regular interrupts properties (i.e. as two_cell) which would
> > bring the list down to only list the parent relationships.
> > 
> > Hmm ... and this parent list might be doable via the regular interrupts
> > property, so the subintc would look like:
> > 
> > subintc: subintc = {
> > 
> > 	interrupt-parent = <&intc>;
> > 	interrupts = <28 0>, <28 0>, <28 0>, <23 0>, <23 0>, .....
> > 	/*		     bit0    bit1    bit2    bit3    bit4   ..... */
> > 
> > }
> > 
> > i.e. naming the parent interrupt for each of the interrupt bits of the
> > sub- controller. Would this be the right direction?
> 
> I think you may want to use an interrupt-map property. That should allow
> you to do arbitrary mappings from one interrupt controller's numbering
> to another's numbering. The VExpress and several PPC platforms have
> examples.

Yep, I've read the examples and also a bit more in-depth on devicetree.org and 
it seems, as you suggested, interrupt-maps are the right concept to describe 
such mappings.


In general, what would be the preferred way to solve this?
Like described above, having the parent bit encoded in the interrupt property 
or doing it with a mapping of sorts like we discussed down here?

I don't have a preference for one or the other right now and both look like 
interesting concepts.


Thanks
Heiko