Re: [PATCH 5/5] ARM: gic: add OF based initialization

Hi Rob,

On 14/09/11 18:57, Rob Herring wrote:

Marc,

On 09/14/2011 12:46 PM, Marc Zyngier wrote:

quoted

On 14/09/11 17:31, Rob Herring wrote:

quoted

From: Rob Herring <redacted>

This adds gic initialization using device tree data. The initialization
functions are intended to be called by a generic OF interrupt
controller parsing function once the right pieces are in place.

PPIs are handled using 3rd cell of interrupts properties to specify the cpu
mask the PPI is assigned to.

Signed-off-by: Rob Herring <redacted>
---
 Documentation/devicetree/bindings/arm/gic.txt |   53 ++++++++++++++++++++++++
 arch/arm/common/gic.c                         |   55 +++++++++++++++++++++++--
 arch/arm/include/asm/hardware/gic.h           |   10 +++++
 3 files changed, 114 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
new file mode 100644
index 0000000..6c513de
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gic.txt

@@ -0,0 +1,53 @@
+* ARM Generic Interrupt Controller
+
+ARM SMP cores are often associated with a GIC, providing per processor
+interrupts (PPI), shared processor interrupts (SPI) and software
+generated interrupts (SGI).
+
+Primary GIC is attached directly to the CPU and typically has PPIs and SGIs.
+Secondary GICs are cascaded into the upward interrupt controller and do not
+have PPIs or SGIs.
+
+Main node required properties:
+
+- compatible : should be one of:
+	"arm,cortex-a9-gic"
+	"arm,arm11mp-gic"
+- 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 3.
+
+  The 1st cell is the interrupt number. 0-15 are reserved for SGIs. 16-31 are
+  for PPIs.
+
+  The 2nd cell is the level-sense information, encoded as follows:
+                    1 = low-to-high edge triggered
+                    2 = high-to-low edge triggered
+                    4 = active high level-sensitive
+                    8 = active low level-sensitive
+
+  Only values of 1 and 4 are valid for GIC 1.0 spec.
+
+  The 3rd cell contains the mask of the cpu number for the interrupt source.
+  The cpu mask is only valid for PPIs and shall be 0 for SPIs. This value shall
+  be 0 for PPIs.

     ^^^^^^^^^^^^^

Typo here ? The way I understand it, it should read "For PPIs, this
value shall be the mask of the possible CPU numbers for the interrupt
source" (or something to similar effect...).

Cut and paste error. This sentence goes in the previous paragraph. What
I meant is the 2nd cell should contain 0 for PPIs as you cannot set the
edge/level on PPIs (that is always true, right?). I probably should also
add 0 in the list of values.

Ah, right. It makes sense indeed. You're correct about PPIs polarity,
this is defined by the hardware and cannot be configured. But it may be
interesting to have the DT to reflect the way the hardware is actually
configured (on the Cortex-A9, some PPIs are configured active-low, and
others are rising-edge).

I take it you are otherwise fine with this binding?

I very much like the fact that it (or at least that's the way I
understand it...) allows for a very compact expression of peripherals
connected to PPIs.

What I'd like to write is the following:

twd@1f000600 {
	compatible = "arm,11mpcore-twd";
	reg = <0x1f000600 0x100>;
	interrupt-parent = <&intc>;
	interrupt = <29 0 0xf>;
}

where 0xf would indicate that the TWD is connected to all four cores. Is
that correct?

Thanks,

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