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

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

[Heiko Stübner]

As suggested by Rob Herring move back to get the parent releationship from dt
like in the first versions, but set the handler from the interrupt property
rather than from a specific list.

This version also implements (hopefully correctly) an idea from him and
Arnd Bergmann to have the parent relationship not described in the controller
node but the device nodes instead.

Therefore the main controller continues to use a two-cell descriptor to set
hwirq and trigger type but the sub-controller switches to a three-cell
descriptor where the third bit describes the hwirq of its parent irq in the
main controller.

As a result a serial node would then look like:
	serial@50000000 {
		compatible = "samsung,s3c2410-uart";
		reg = <0x50000000 0x4000>;
		interrupt-parent = <&subintc>;
		interrupts = <0 4 28>, <1 4 28>;
	};

Tested on a s3c2416-based board.

As it depends on changes already pending for 3.10 it should probably go thru
the samsung tree.


Heiko Stuebner (5):
  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: add irq_set_type callback for basic interrupt types
  irqchip: s3c24xx: add devicetree support

 .../interrupt-controller/samsung,s3c24xx-irq.txt   |   64 ++++++
 arch/arm/mach-s3c24xx/Makefile                     |    2 +-
 drivers/irqchip/Makefile                           |    1 +
 .../irq.c => drivers/irqchip/irq-s3c24xx.c         |  218 +++++++++++++++++++-
 4 files changed, 277 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 (87%)

-- 
1.7.2.3

[PATCH v4 1/5] 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@sntech.de>
---
 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 v4 2/5] 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 v4 3/5] 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.
As it's not possible to distinguish between the s3c2416 and s3c2450
the additional interrupts of the s3c2450 will only be available thru
devicetree later.

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 v4 4/5] irqchip: s3c24xx: add irq_set_type callback for basic interrupt types

[Heiko Stübner]

Enables post-init setting of the desired typehandler for the interrupt.

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

diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index a565eb8..7cba4f0 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -123,6 +123,28 @@ static inline void s3c_irq_ack(struct irq_data *data)
 		__raw_writel(bitval, intc->reg_intpnd);
 }
 
+static int s3c_irq_type(struct irq_data *data, unsigned int type)
+{
+	switch (type) {
+	case IRQ_TYPE_NONE:
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_EDGE_BOTH:
+		irq_set_handler(data->irq, handle_edge_irq);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_set_handler(data->irq, handle_level_irq);
+		break;
+	default:
+		pr_err("No such irq type %d", type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int s3c_irqext_type_set(void __iomem *gpcon_reg,
 			       void __iomem *extint_reg,
 			       unsigned long gpcon_offset,
@@ -228,6 +250,7 @@ static struct irq_chip s3c_irq_chip = {
 	.irq_ack	= s3c_irq_ack,
 	.irq_mask	= s3c_irq_mask,
 	.irq_unmask	= s3c_irq_unmask,
+	.irq_set_type	= s3c_irq_type,
 	.irq_set_wake	= s3c_irq_wake
 };
 
@@ -236,6 +259,7 @@ static struct irq_chip s3c_irq_level_chip = {
 	.irq_mask	= s3c_irq_mask,
 	.irq_unmask	= s3c_irq_unmask,
 	.irq_ack	= s3c_irq_ack,
+	.irq_set_type	= s3c_irq_type,
 };
 
 static struct irq_chip s3c_irqext_chip = {
-- 
1.7.2.3

[PATCH v4 5/5] irqchip: s3c24xx: add devicetree support

[Heiko Stübner]

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

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../interrupt-controller/samsung,s3c24xx-irq.txt   |   64 +++++++
 drivers/irqchip/irq-s3c24xx.c                      |  181 ++++++++++++++++++++
 2 files changed, 245 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..bfd689b
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/samsung,s3c24xx-irq.txt
@@ -0,0 +1,64 @@
+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 "samsung,s3c24xx-irq",
+
+- 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 than mapped directly to the
+  bit-numbers of the pending register of the named interrupt controller.
+
+  Interrupts properties using sub-controller also should contain the hwirq
+  of their parent interrupt in the main controller as third interrupt-cell.
+
+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 for the main controllers and 3
+  for the sub-controller.
+
+Example:
+
+	interrupt-controller@4a000000 {
+		compatible = "samsung,s3c24xx-irq";
+		reg = <0x4a000000 0x100>;
+		interrupt-controller;
+
+		intc:intc {
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		subintc:subintc {
+			interrupt-controller;
+			#interrupt-cells = <3>;
+		};
+	};
+
+	[...]
+
+	serial@50000000 {
+		compatible = "samsung,s3c2410-uart";
+		reg = <0x50000000 0x4000>;
+		interrupt-parent = <&subintc>;
+		interrupts = <0 4 28>, <1 4 28>;
+	};
+
+	rtc@57000000 {
+		compatible = "samsung,s3c2410-rtc";
+		reg = <0x57000000 0x100>;
+		interrupt-parent = <&intc>;
+		interrupts = <30 3>, <8 3>;
+		status = "disabled";
+	};
diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index 7cba4f0..c26eb3f 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
@@ -1128,3 +1133,179 @@ void __init s3c2443_init_irq(void)
 	s3c24xx_init_intc(NULL, &init_s3c2443subint[0], main_intc, 0x4a000018);
 }
 #endif
+
+#ifdef CONFIG_OF
+static int s3c24xx_irq_map_of(struct irq_domain *h, unsigned int virq,
+							irq_hw_number_t hw)
+{
+	struct s3c_irq_intc *intc = h->host_data;
+	struct s3c_irq_intc *parent_intc = intc->parent;
+	struct s3c_irq_data *irq_data = &intc->irqs[hw];
+
+	/* attach controller pointer to irq_data */
+	irq_data->intc = intc;
+
+	if (!parent_intc)
+		irq_set_chip_and_handler(virq, &s3c_irq_chip, handle_edge_irq);
+	else
+		irq_set_chip_and_handler(virq, &s3c_irq_level_chip,
+					 handle_edge_irq);
+
+	set_irq_flags(virq, IRQF_VALID);
+
+	return 0;
+}
+
+static struct irq_domain_ops s3c24xx_irq_ops_of_main = {
+	.map = s3c24xx_irq_map_of,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int s3c_irq_xlate_subintc(struct irq_domain *d, struct device_node *n,
+			const u32 *intspec, unsigned int intsize,
+			irq_hw_number_t *out_hwirq, unsigned int *out_type)
+{
+	struct s3c_irq_intc *intc = d->host_data;
+	struct s3c_irq_intc *parent_intc = intc->parent;
+	struct s3c_irq_data *irq_data = &intc->irqs[intspec[0]];
+	struct s3c_irq_data *parent_irq_data;
+
+	int irqno;
+
+	if (WARN_ON(intsize < 3))
+		return -EINVAL;
+
+	*out_hwirq = intspec[0];
+	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+
+	irqno = irq_create_mapping(parent_intc->domain, intspec[2]);
+	if (irqno < 0) {
+		pr_err("irq: could not map parent interrupt\n");
+		return irqno;
+	}
+
+	irq_data->parent_irq = intspec[2];
+	parent_irq_data = &parent_intc->irqs[irq_data->parent_irq];
+	parent_irq_data->sub_intc = intc;
+	parent_irq_data->sub_bits |= (1UL << intspec[0]);
+
+	irq_set_chained_handler(irqno, s3c_irq_demux);
+
+	return 0;
+}
+
+static struct irq_domain_ops s3c24xx_irq_ops_of_sub = {
+	.map = s3c24xx_irq_map_of,
+	.xlate = s3c_irq_xlate_subintc,
+};
+
+struct s3c24xx_irq_of_ctrl {
+	char			*name;
+	unsigned long		offset;
+	struct s3c_irq_intc	**handle;
+	struct s3c_irq_intc	**parent;
+	struct irq_domain_ops	*ops;
+};
+
+static struct s3c24xx_irq_of_ctrl s3c24xx_ctrl[] = {
+	{
+		.name = "intc",
+		.offset = 0,
+		.handle = &main_intc,
+		.ops = &s3c24xx_irq_ops_of_main,
+	}, {
+		.name = "subintc",
+		.offset = 0x18,
+		.parent = &main_intc,
+		.ops = &s3c24xx_irq_ops_of_sub,
+	}, {
+		.name = "intc2",
+		.offset = 0x40,
+		.handle = &main_intc2,
+		.ops = &s3c24xx_irq_ops_of_main,
+	}
+};
+
+int __init s3c24xx_init_intc_of(struct device_node *np,
+				 struct device_node *interrupt_parent)
+{
+	struct device_node *intc_node;
+	struct s3c_irq_intc *intc;
+	struct s3c24xx_irq_of_ctrl *ctrl;
+	void __iomem *reg_base;
+	int i;
+
+	reg_base = of_iomap(np, 0);
+	if (!reg_base) {
+		pr_err("irq-s3c24xx: could not map irq registers\n");
+		return -EINVAL;
+	}
+
+	set_handle_irq(s3c24xx_handle_irq);
+
+	for (i = 0; i < ARRAY_SIZE(s3c24xx_ctrl); i++) {
+		ctrl = &s3c24xx_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 = kzalloc(sizeof(struct s3c_irq_data) * 32,
+				     GFP_KERNEL);
+		if (!intc->irqs) {
+			kfree(intc);
+			of_node_put(intc_node);
+			return -ENOMEM;
+		}
+
+		if (ctrl->parent) {
+			intc->reg_pending = reg_base + ctrl->offset;
+			intc->reg_mask = reg_base + ctrl->offset + 0x4;
+
+			if (*(ctrl->parent)) {
+				intc->parent = *(ctrl->parent);
+			} else {
+				pr_warn("irq: parent of %s missing\n",
+					ctrl->name);
+				kfree(intc->irqs);
+				kfree(intc);
+				of_node_put(intc_node);
+				continue;
+			}
+		} else {
+			intc->reg_pending = reg_base + ctrl->offset;
+			intc->reg_mask = reg_base + ctrl->offset + 0x08;
+			intc->reg_intpnd = reg_base + ctrl->offset + 0x10;
+		}
+
+		/* now that all the data is complete, init the irq-domain */
+		s3c24xx_clear_intc(intc);
+		intc->domain = irq_domain_add_linear(intc_node, 32,
+						     ctrl->ops, intc);
+		if (!intc->domain) {
+			pr_err("irq: could not create irq-domain\n");
+			kfree(intc->irqs);
+			kfree(intc);
+			of_node_put(intc_node);
+			continue;
+		}
+
+		if (ctrl->handle)
+			*(ctrl->handle) = intc;
+	}
+
+	return 0;
+}
+IRQCHIP_DECLARE(s3c24xx_irq, "samsung,s3c24xx-irq", s3c24xx_init_intc_of);
+#endif
-- 
1.7.2.3

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

[Arnd Bergmann]

On Friday 22 March 2013, Heiko Stübner wrote:
> +	interrupt-controller@4a000000 {
> +		compatible = "samsung,s3c24xx-irq";
> +		reg = <0x4a000000 0x100>;
> +		interrupt-controller;
> +
> +		intc:intc {
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		subintc:subintc {
> +			interrupt-controller;
> +			#interrupt-cells = <3>;
> +		};
> +	};

I think this is not a comformant binding because the parent node
is marked "interrupt-controller" but does not itself have a
#interrupt-cells property.

One way to resolve this would probably be to fold the sub-nodes into
the parent and always use #interrupt-cells = <3> or maybe <4>
so you can identify which sub-node the interrupt is meant for.

Also, I think you are missing a description of what the two or
three cells represent.

> +static int s3c_irq_xlate_subintc(struct irq_domain *d, struct device_node *n,
> +			const u32 *intspec, unsigned int intsize,
> +			irq_hw_number_t *out_hwirq, unsigned int *out_type)
> +{
> +	struct s3c_irq_intc *intc = d->host_data;
> +	struct s3c_irq_intc *parent_intc = intc->parent;
> +	struct s3c_irq_data *irq_data = &intc->irqs[intspec[0]];
> +	struct s3c_irq_data *parent_irq_data;
> +
> +	int irqno;
> +
> +	if (WARN_ON(intsize < 3))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	irqno = irq_create_mapping(parent_intc->domain, intspec[2]);
> +	if (irqno < 0) {
> +		pr_err("irq: could not map parent interrupt\n");
> +		return irqno;
> +	}

So the third cell in this is the interrupt number of the main controller
that the irq cascades into, while the first cell is the number of the
cascaded controller, correct?

If you want to fold everything into one node, it's probably best to
put the irq number of the main controller into the first cell all the
time, and use the second cell to describe the number in the second cell.

The alternative would be to have three completely separate nodes,
and then you can describe the parent-child relationship like

intc: interrupt-controller@4a000000 {
	compatible = "samsung,s3c2416-intc";
	reg = <0x4a000000 0x18>;
	interrupt-controller;
	#interrupt-cells = <2>;
};

subintc: interrupt-controller@4a000018 {
	compatible = "samsung,s3c2416-subintc";
	reg = <0x4a000018 0x28>;
	interrupt-controller;
	#interrupt-cells = <3>;
	interrupt-parent = <&intc>;
	interrupts  = <28 0>, <23 0>, <15 0>, <31 0>, <16 0>, <17 0>, <18 0>, <9 0>;
};

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

       serial@51000000 {
               compatible = "samsung,s3c2410-uart";
               reg = <0x51000000 0x4000>;
               interrupt-parent = <&subintc>;
               interrupts = <0 1 4>, /* first interrupt, second parent, level */
			    <1 1 4>; /* second interrupt, second parent, level */
       };




	Arnd

Re: [PATCH v4 0/5] move s3c24xx-irq to drivers/irqchip and add dt support

[Arnd Bergmann]

On Friday 22 March 2013, Heiko Stübner wrote:
> As suggested by Rob Herring move back to get the parent releationship from dt
> like in the first versions, but set the handler from the interrupt property
> rather than from a specific list.
> 
> This version also implements (hopefully correctly) an idea from him and
> Arnd Bergmann to have the parent relationship not described in the controller
> node but the device nodes instead.
> 
> Therefore the main controller continues to use a two-cell descriptor to set
> hwirq and trigger type but the sub-controller switches to a three-cell
> descriptor where the third bit describes the hwirq of its parent irq in the
> main controller.
> 
> As a result a serial node would then look like:
>         serial@50000000 {
>                 compatible = "samsung,s3c2410-uart";
>                 reg = <0x50000000 0x4000>;
>                 interrupt-parent = <&subintc>;
>                 interrupts = <0 4 28>, <1 4 28>;
>         };
> 
> Tested on a s3c2416-based board.
> 
> As it depends on changes already pending for 3.10 it should probably go thru
> the samsung tree.

Hi Heiko,

I should probably read the original discussion thread again. I already commented
on the binding here, but I may have missed something important that led to
it being the way it is now.

	Arnd
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v4 0/5] move s3c24xx-irq to drivers/irqchip and add dt support

[Heiko Stübner]

Am Freitag, 22. März 2013, 22:04:54 schrieb Arnd Bergmann:
> On Friday 22 March 2013, Heiko Stübner wrote:
> > As suggested by Rob Herring move back to get the parent releationship
> > from dt like in the first versions, but set the handler from the
> > interrupt property rather than from a specific list.
> > 
> > This version also implements (hopefully correctly) an idea from him and
> > Arnd Bergmann to have the parent relationship not described in the
> > controller node but the device nodes instead.
> > 
> > Therefore the main controller continues to use a two-cell descriptor to
> > set hwirq and trigger type but the sub-controller switches to a
> > three-cell descriptor where the third bit describes the hwirq of its
> > parent irq in the main controller.
> > 
> > As a result a serial node would then look like:
> >         serial@50000000 {
> >         
> >                 compatible = "samsung,s3c2410-uart";
> >                 reg = <0x50000000 0x4000>;
> >                 interrupt-parent = <&subintc>;
> >                 interrupts = <0 4 28>, <1 4 28>;
> >         
> >         };
> > 
> > Tested on a s3c2416-based board.
> > 
> > As it depends on changes already pending for 3.10 it should probably go
> > thru the samsung tree.
> 
> Hi Heiko,
> 
> I should probably read the original discussion thread again. I already
> commented on the binding here, but I may have missed something important
> that led to it being the way it is now.

Hi Arnd,

Not really ... it's only my current interpretation of your short suggestion to 
handle the parent relationship in the interrupt descriptor of the device node 
and not in the controller node itself.

Your other mail did contain a lot of interesting thoughts on how to improve 
the current state - thanks for them.


Heiko

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

[Heiko Stübner]

Am Freitag, 22. März 2013, 21:55:57 schrieb Arnd Bergmann:
> On Friday 22 March 2013, Heiko Stübner wrote:
> > +	interrupt-controller@4a000000 {
> > +		compatible = "samsung,s3c24xx-irq";
> > +		reg = <0x4a000000 0x100>;
> > +		interrupt-controller;
> > +
> > +		intc:intc {
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +		};
> > +
> > +		subintc:subintc {
> > +			interrupt-controller;
> > +			#interrupt-cells = <3>;
> > +		};
> > +	};
> 
> I think this is not a comformant binding because the parent node
> is marked "interrupt-controller" but does not itself have a
> #interrupt-cells property.
> 
> One way to resolve this would probably be to fold the sub-nodes into
> the parent and always use #interrupt-cells = <3> or maybe <4>
> so you can identify which sub-node the interrupt is meant for.
> 
> Also, I think you are missing a description of what the two or
> three cells represent.

yep, folding them together sounds interesting, see below.


> > +static int s3c_irq_xlate_subintc(struct irq_domain *d, struct
> > device_node *n, +			const u32 *intspec, unsigned int intsize,
> > +			irq_hw_number_t *out_hwirq, unsigned int *out_type)
> > +{
> > +	struct s3c_irq_intc *intc = d->host_data;
> > +	struct s3c_irq_intc *parent_intc = intc->parent;
> > +	struct s3c_irq_data *irq_data = &intc->irqs[intspec[0]];
> > +	struct s3c_irq_data *parent_irq_data;
> > +
> > +	int irqno;
> > +
> > +	if (WARN_ON(intsize < 3))
> > +		return -EINVAL;
> > +
> > +	*out_hwirq = intspec[0];
> > +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> > +
> > +	irqno = irq_create_mapping(parent_intc->domain, intspec[2]);
> > +	if (irqno < 0) {
> > +		pr_err("irq: could not map parent interrupt\n");
> > +		return irqno;
> > +	}
> 
> So the third cell in this is the interrupt number of the main controller
> that the irq cascades into, while the first cell is the number of the
> cascaded controller, correct?

right

> If you want to fold everything into one node, it's probably best to
> put the irq number of the main controller into the first cell all the
> time, and use the second cell to describe the number in the second cell.

Not all main interrupts are parent interrupts, so it would be difficult to 
distinguish between main interrupts that are a parent and the ones that are 
not - is a "-1" a valid cell-value for interrupts?
	<main_irq -1 trigger_type> /* directly used main interrupt */
	<main_irq child_irq trigger_type> /* sub-interrupt */

Or are you thinking of something like:
	<main_irq is_a_parent child_irq trigger_type>


I looked a bit more thru the other irqchips and it seems the bcm2835 is doing 
something similar but without having a parent relationship:
	<controller-num irq-num>

so this could be adapted to:
	<controller-num irq-num parent-num trigger_type>

controller-num being 0 for intc, 1 for subintc, 2 intc2 . The controller 
itself knows if it's a sub- or main controller - when it should handle the 
parent-number or simply ignore it.


> The alternative would be to have three completely separate nodes,
> and then you can describe the parent-child relationship like
> 
> intc: interrupt-controller@4a000000 {
> 	compatible = "samsung,s3c2416-intc";
> 	reg = <0x4a000000 0x18>;
> 	interrupt-controller;
> 	#interrupt-cells = <2>;
> };
> 
> subintc: interrupt-controller@4a000018 {
> 	compatible = "samsung,s3c2416-subintc";
> 	reg = <0x4a000018 0x28>;
> 	interrupt-controller;
> 	#interrupt-cells = <3>;
> 	interrupt-parent = <&intc>;
> 	interrupts  = <28 0>, <23 0>, <15 0>, <31 0>, <16 0>, <17 0>, <18 0>, <9
> 0>; };


The first two iterations had separate nodes, but the interrupt controller 
posseses more interesting registers that are shared between all of the 
controllers, so it did sound better to have them together.

Also the interrupts property is most likely not able to accurately describe 
the parent relationship, as the interrupts are very much different in all 
s3c24xx SoCs - I would need to tell every sub-interrupt where it cascasdes 
from, because most of them do different things on different s3c24xx SoCs.

I did start with this approach, using the interrupt index as mapping for the 
hwirq - interrupts[0] for hwirq 0 and so on. But it looked ugly. Using only 
one interrupts element per sub-group would require per-SoC mapping data to be 
present in the driver, indicating that interrupts[0] is responsible for bits 
0,1,2 and so on.

Therefore the idea of handling the parent relationship in the device-nodes 
interrupt property sounds much nicer :-)


Heiko

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

[Arnd Bergmann]

On Friday 22 March 2013, Heiko Stübner wrote:
> Not all main interrupts are parent interrupts, so it would be difficult to 
> distinguish between main interrupts that are a parent and the ones that are 
> not - is a "-1" a valid cell-value for interrupts?

I'm actually not sure if negative numbers are valid syntax in dtc, but
you could certainly define some value to mean "none", or you add another
flag in the trigger type cell.

>         <main_irq -1 trigger_type> /* directly used main interrupt */
>         <main_irq child_irq trigger_type> /* sub-interrupt */
> 
> Or are you thinking of something like:
>         <main_irq is_a_parent child_irq trigger_type>

The first one would work, the second one with four cells seems a bit
strange if the second cell is just a bit. My first idea was to use 
a bit mask for the child irq, in which only one bit is set, so
it would be <3 0x40000 4> instead of <3 18 4>, and a zero bitmask
would indicate no sub-interrupt. This is probably harder to read
though and not a good representation.

> I looked a bit more thru the other irqchips and it seems the bcm2835 is doing 
> something similar but without having a parent relationship:
>         <controller-num irq-num>
> 
> so this could be adapted to:
>         <controller-num irq-num parent-num trigger_type>
> 
> controller-num being 0 for intc, 1 for subintc, 2 intc2 . The controller 
> itself knows if it's a sub- or main controller - when it should handle the 
> parent-number or simply ignore it.

Yes, that looks more logical.

You could in theory also compact collapse multiple cells into one, so
instead of using <1 3 18 4> it could be <0x10031204> or <0x10030012 4>
to save a little space while making it a little less readable.
 
> > The alternative would be to have three completely separate nodes,
> > and then you can describe the parent-child relationship like
> > 
> > intc: interrupt-controller@4a000000 {
> >       compatible = "samsung,s3c2416-intc";
> >       reg = <0x4a000000 0x18>;
> >       interrupt-controller;
> >       #interrupt-cells = <2>;
> > };
> > 
> > subintc: interrupt-controller@4a000018 {
> >       compatible = "samsung,s3c2416-subintc";
> >       reg = <0x4a000018 0x28>;
> >       interrupt-controller;
> >       #interrupt-cells = <3>;
> >       interrupt-parent = <&intc>;
> >       interrupts  = <28 0>, <23 0>, <15 0>, <31 0>, <16 0>, <17 0>, <18 0>, <9
> > 0>; };
> 
> 
> The first two iterations had separate nodes, but the interrupt controller 
> posseses more interesting registers that are shared between all of the 
> controllers, so it did sound better to have them together.

Yes, makes sense.

> Also the interrupts property is most likely not able to accurately describe 
> the parent relationship, as the interrupts are very much different in all 
> s3c24xx SoCs - I would need to tell every sub-interrupt where it cascasdes 
> from, because most of them do different things on different s3c24xx SoCs.
>
> I did start with this approach, using the interrupt index as mapping for the 
> hwirq - interrupts[0] for hwirq 0 and so on. But it looked ugly. Using only 
> one interrupts element per sub-group would require per-SoC mapping data to be 
> present in the driver, indicating that interrupts[0] is responsible for bits 
> 0,1,2 and so on.
> 
> Therefore the idea of handling the parent relationship in the device-nodes 
> interrupt property sounds much nicer :-)

Ok.

	Arnd