Re: [PATCH v3 1/7] ARM: davinci, intc: Add OF support for TI interrupt controller

Hello Grant,

Am 26.05.2012 03:05, schrieb Grant Likely:

On Mon,  5 Mar 2012 12:09:58 +0100, Heiko Schocher[off-list ref]  wrote:

quoted

Add a function to initialize the Common Platform Interrupt Controller
(cp_intc) from TI used on OMAP-L1x SoCs using a device tree node.

Signed-off-by: Heiko Schocher<redacted>
Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Sekhar Nori<redacted>
Cc: Wolfgang Denk<redacted>
Cc: Sergei Shtylyov<redacted>

---
- changes for v2:
- add comment from Grant Likely:
   - migrate the whole interrupt controller to natively use an
     irq_domain. Rebased complete patchserie to:
     git://git.secretlab.ca/git/linux-2.6.git irqdomain/next

     commit 3a806bfcde2cc3e4853f2807b2e3c94e7ccaf450
     Author: Grant Likely[off-list ref]
     Date:   Fri Jan 27 06:44:34 2012 -0700

     irq_domain: mostly eliminate slow-path revmap lookups
- changes for v3:
   - add comments from Sergei Shtylyov:
     - rename compatible" prop to "ti,cp_intc"

Nit: DT preference is to use '-' instead of '_'

fixed.

[...]

quoted

diff --git a/Documentation/devicetree/bindings/arm/davinci/intc.txt b/Documentation/devicetree/bindings/arm/davinci/intc.txt
new file mode 100644
index 0000000..dfd6a560
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/davinci/intc.txt

@@ -0,0 +1,27 @@

[...]

quoted

+- compatible : should be:
+	"ti,cp_intc"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The type shall be a<u32>  and the value shall be 1.

Is there any configuration for irq inputs (edge/level) or are they
fixed mode?  If configurable, then you'll want another cell for flags.

As I know, they are fixed...

[...]

quoted

diff --git a/arch/arm/mach-davinci/cp_intc.c b/arch/arm/mach-davinci/cp_intc.c
index f83152d..585114a 100644
--- a/arch/arm/mach-davinci/cp_intc.c
+++ b/arch/arm/mach-davinci/cp_intc.c

@@ -9,9 +9,14 @@

[...]

quoted

+		/* create a legacy host */
+		cp_intc_domain = irq_domain_add_legacy(node, num_irq,
+					irq_base, 0,&cp_intc_host_ops, NULL);
+		if (cp_intc_domain == NULL) {
+			pr_err("CP INTC: failed to allocate irq host!\n");
+			return -EINVAL;
+		}
+	} else {
+		/* Set up genirq dispatching for cp_intc */
+		for (i = 0; i<  num_irq; i++) {
+			irq_set_chip(i,&cp_intc_irq_chip);
+			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+			irq_set_handler(i, handle_edge_irq);
+		}

Instead of the else clause here, even the non-DT mode should still
call irq_domain_add.  irq_domain is not just for DT.  It gets the
driver away from manually managing its irq mappings.

Ok, I try this.

quoted

  	/* Enable global interrupt */
  	cp_intc_write(1, CP_INTC_GLOBAL_ENABLE);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id irq_match[] __initdata = {
+	{ .compatible = "ti,cp_intc", .data = __cp_intc_init, },
+	{ }
+};
+
+void __init cp_intc_init(void)
+{
+	of_irq_init(irq_match);
+}
+#else
+void __init cp_intc_init(void)
+{
+	__cp_intc_init(NULL, NULL);
  }
+#endif

Nack on this hunk because it means you have either a DT kernel or a
non-DT kernel.  Turning on CONFIG_OF must not break booting on a
non-DT platform.  This is a policy decision to improve test coverage.
Instead the init hook should check at runtime if a DT is available,
and if it is call of_irq_init.  Otherwise call __cp_initc_init()
directly.

fixed in v4

Thanks for the review!

bye,
Heiko

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany