[PATCH v2 0/3] powerpc: Open PIC binding and 'no-reset' implementation

[PATCH v2 0/3] powerpc: Open PIC binding and 'no-reset' implementation

[Meador Inge]

This patch set provides a binding for Open PIC and implements support for
a new property, specified by that binding, called 'no-reset'.  With 'no-reset'
in place the 'protected-sources' property is no longer needed and was removed.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
CC: Hollis Blanchard <hollis_blanchard@mentor.com>

Meador Inge (3):
  powerpc: Removing support for 'protected-sources'
  powerpc: document the Open PIC device tree binding
  powerpc: make MPIC honor the 'no-reset' device tree property

 Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
 arch/powerpc/include/asm/mpic.h                 |    7 +-
 arch/powerpc/sysdev/mpic.c                      |   92 ++++++++----------
 3 files changed, 160 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt

[PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Meador Inge]

In a recent discussion [1, 2] concerning device trees for AMP systems, the
question of whether we really need 'protected-sources' arose.  The general
consensus was that if you don't want a source to be used, then it should *not*
be mentioned in an 'interrupts' property.  If a source really needs to be
mentioned, then it should be put in a property other than 'interrupts' with
a specific binding for that use case.

[1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
[2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html

Signed-off-by: Meador Inge <meador_inge@mentor.com>
CC: Hollis Blanchard <hollis_blanchard@mentor.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/mpic.h |    3 ---
 arch/powerpc/sysdev/mpic.c      |   38 --------------------------------------
 2 files changed, 0 insertions(+), 41 deletions(-)

diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index e000cce..9b94f18 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -301,9 +301,6 @@ struct mpic
 	struct mpic_reg_bank	cpuregs[MPIC_MAX_CPUS];
 	struct mpic_reg_bank	isus[MPIC_MAX_ISU];
 
-	/* Protected sources */
-	unsigned long		*protected;
-
 #ifdef CONFIG_MPIC_WEIRD
 	/* Pointer to HW info array */
 	u32			*hw_set;
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 7c13426..a98f41d 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -947,8 +947,6 @@ static int mpic_host_map(struct irq_host *h, unsigned int virq,
 
 	if (hw == mpic->spurious_vec)
 		return -EINVAL;
-	if (mpic->protected && test_bit(hw, mpic->protected))
-		return -EINVAL;
 
 #ifdef CONFIG_SMP
 	else if (hw >= mpic->ipi_vecs[0]) {
@@ -1095,26 +1093,6 @@ struct mpic * __init mpic_alloc(struct device_node *node,
 	if (node && of_get_property(node, "big-endian", NULL) != NULL)
 		mpic->flags |= MPIC_BIG_ENDIAN;
 
-	/* Look for protected sources */
-	if (node) {
-		int psize;
-		unsigned int bits, mapsize;
-		const u32 *psrc =
-			of_get_property(node, "protected-sources", &psize);
-		if (psrc) {
-			psize /= 4;
-			bits = intvec_top + 1;
-			mapsize = BITS_TO_LONGS(bits) * sizeof(unsigned long);
-			mpic->protected = kzalloc(mapsize, GFP_KERNEL);
-			BUG_ON(mpic->protected == NULL);
-			for (i = 0; i < psize; i++) {
-				if (psrc[i] > intvec_top)
-					continue;
-				__set_bit(psrc[i], mpic->protected);
-			}
-		}
-	}
-
 #ifdef CONFIG_MPIC_WEIRD
 	mpic->hw_set = mpic_infos[MPIC_GET_REGSET(flags)];
 #endif
@@ -1321,9 +1299,6 @@ void __init mpic_init(struct mpic *mpic)
 		u32 vecpri = MPIC_VECPRI_MASK | i |
 			(8 << MPIC_VECPRI_PRIORITY_SHIFT);
 		
-		/* check if protected */
-		if (mpic->protected && test_bit(i, mpic->protected))
-			continue;
 		/* init hw */
 		mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
 		mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION), 1 << cpu);
@@ -1492,13 +1467,6 @@ static unsigned int _mpic_get_one_irq(struct mpic *mpic, int reg)
 			mpic_eoi(mpic);
 		return NO_IRQ;
 	}
-	if (unlikely(mpic->protected && test_bit(src, mpic->protected))) {
-		if (printk_ratelimit())
-			printk(KERN_WARNING "%s: Got protected source %d !\n",
-			       mpic->name, (int)src);
-		mpic_eoi(mpic);
-		return NO_IRQ;
-	}
 
 	return irq_linear_revmap(mpic->irqhost, src);
 }
@@ -1532,12 +1500,6 @@ unsigned int mpic_get_coreint_irq(void)
 			mpic_eoi(mpic);
 		return NO_IRQ;
 	}
-	if (unlikely(mpic->protected && test_bit(src, mpic->protected))) {
-		if (printk_ratelimit())
-			printk(KERN_WARNING "%s: Got protected source %d !\n",
-			       mpic->name, (int)src);
-		return NO_IRQ;
-	}
 
 	return irq_linear_revmap(mpic->irqhost, src);
 #else
-- 
1.6.3.3

[PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Meador Inge]

This binding documents several properties that have been in use for quite
some time, and adds one new property 'no-reset', which controls whether the
Open PIC should be reset during runtime initialization.

The general formatting and interrupt specifier definition is based off of
Stuart Yoder's FSL MPIC binding.

Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
CC: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
CC: Stuart Yoder <stuart.yoder-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt

diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt b/Documentation/powerpc/dts-bindings/open-pic.txt
new file mode 100644
index 0000000..447ef65
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/open-pic.txt
@@ -0,0 +1,115 @@
+* Open PIC Binding
+
+This binding specifies what properties must be available in the device tree
+representation of an Open PIC compliant interrupt controller.  This binding is
+based on the binding defined for Open PIC in [1] and is a superset of that
+binding.
+
+PROPERTIES
+
+  NOTE: Many of these descriptions were paraphrased here from [1] to aid
+        readability.
+
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: Specifies the compatibility list for the PIC.  The
+          property value shall include "open-pic".
+
+  - reg
+      Usage: required
+      Value type: <prop-encoded-array>
+      Definition: Specifies the base physical address(s) and size(s) of this
+          PIC's addressable register space.
+
+  - interrupt-controller
+      Usage: required
+      Value type: <empty>
+      Definition: The presence of this property identifies the node
+          as an Open PIC.  No property value should be defined.
+
+  - #interrupt-cells
+      Usage: required
+      Value type: <u32>
+      Definition: Specifies the number of cells needed to encode an
+          interrupt source.  Shall be 2.
+
+  - #address-cells
+      Usage: required
+      Value type: <u32>
+      Definition: Specifies the number of cells needed to encode an
+          address.  The value of this property shall always be 0.
+          As such, 'interrupt-map' nodes do not have to specify a
+          parent unit address.
+
+  - no-reset
+      Usage: optional
+      Value type: <empty>
+      Definition: The presence of this property indicates that the PIC
+          should not be reset during runtime initialization.  The presence of
+          this property also mandates that any initialization related to
+          interrupt sources shall be limited to sources explicitly referenced
+          in the device tree.
+
+INTERRUPT SPECIFIER DEFINITION
+
+  Interrupt specifiers consists of 2 cells encoded as
+  follows:
+
+   <1st-cell>   interrupt-number
+
+                Identifies the interrupt source.
+
+   <2nd-cell>   level-sense information, encoded as follows:
+                    0 = low-to-high edge triggered
+                    1 = active low level-sensitive
+                    2 = active high level-sensitive
+                    3 = high-to-low edge triggered
+
+EXAMPLE 1
+
+    /*
+     * An Open PIC interrupt controller
+     */
+	mpic: pic@40000 {
+        // This is an interrupt controller node.
+		interrupt-controller;
+
+        // No address cells so that 'interrupt-map' nodes which reference
+        // this Open PIC node do not need a parent address specifier.
+		#address-cells = <0>;
+
+        // Two cells to encode interrupt sources.
+		#interrupt-cells = <2>;
+
+        // Offset address of 0x40000 and size of 0x40000.
+		reg = <0x40000 0x40000>;
+
+        // Compatible with Open PIC.
+		compatible = "open-pic";
+
+        // The PIC should not be reset.
+		no-reset;
+	};
+
+EXAMPLE 2
+
+    /*
+     * An interrupt generating device that is wired to an Open PIC.
+     */
+    serial0: serial@4500 {
+        // Interrupt source '42' that is active high level-sensitive.
+        // Note that there are only two cells as specified in the interrupt
+        // parent's '#interrupt-cells' property.
+        interrupts = <42 2>;
+
+        // The interrupt controller that this device is wired to.
+        interrupt-parent = <&mpic>;
+    };
+
+REFERENCES
+
+[1] Power.org (TM) Standard for Embedded Power Architecture (TM) Platform
+    Requirements (ePAPR), Version 1.0, July 2008.
+    (http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf)
+
-- 
1.6.3.3

[PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Meador Inge]

This binding documents several properties that have been in use for quite
some time, and adds one new property 'no-reset', which controls whether the
Open PIC should be reset during runtime initialization.

The general formatting and interrupt specifier definition is based off of
Stuart Yoder's FSL MPIC binding.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
CC: Hollis Blanchard <hollis_blanchard@mentor.com>
CC: Stuart Yoder <stuart.yoder@freescale.com>
---
 Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt

diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt b/Documentation/powerpc/dts-bindings/open-pic.txt
new file mode 100644
index 0000000..447ef65
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/open-pic.txt
@@ -0,0 +1,115 @@
+* Open PIC Binding
+
+This binding specifies what properties must be available in the device tree
+representation of an Open PIC compliant interrupt controller.  This binding is
+based on the binding defined for Open PIC in [1] and is a superset of that
+binding.
+
+PROPERTIES
+
+  NOTE: Many of these descriptions were paraphrased here from [1] to aid
+        readability.
+
+  - compatible
+      Usage: required
+      Value type: <string>
+      Definition: Specifies the compatibility list for the PIC.  The
+          property value shall include "open-pic".
+
+  - reg
+      Usage: required
+      Value type: <prop-encoded-array>
+      Definition: Specifies the base physical address(s) and size(s) of this
+          PIC's addressable register space.
+
+  - interrupt-controller
+      Usage: required
+      Value type: <empty>
+      Definition: The presence of this property identifies the node
+          as an Open PIC.  No property value should be defined.
+
+  - #interrupt-cells
+      Usage: required
+      Value type: <u32>
+      Definition: Specifies the number of cells needed to encode an
+          interrupt source.  Shall be 2.
+
+  - #address-cells
+      Usage: required
+      Value type: <u32>
+      Definition: Specifies the number of cells needed to encode an
+          address.  The value of this property shall always be 0.
+          As such, 'interrupt-map' nodes do not have to specify a
+          parent unit address.
+
+  - no-reset
+      Usage: optional
+      Value type: <empty>
+      Definition: The presence of this property indicates that the PIC
+          should not be reset during runtime initialization.  The presence of
+          this property also mandates that any initialization related to
+          interrupt sources shall be limited to sources explicitly referenced
+          in the device tree.
+
+INTERRUPT SPECIFIER DEFINITION
+
+  Interrupt specifiers consists of 2 cells encoded as
+  follows:
+
+   <1st-cell>   interrupt-number
+
+                Identifies the interrupt source.
+
+   <2nd-cell>   level-sense information, encoded as follows:
+                    0 = low-to-high edge triggered
+                    1 = active low level-sensitive
+                    2 = active high level-sensitive
+                    3 = high-to-low edge triggered
+
+EXAMPLE 1
+
+    /*
+     * An Open PIC interrupt controller
+     */
+	mpic: pic@40000 {
+        // This is an interrupt controller node.
+		interrupt-controller;
+
+        // No address cells so that 'interrupt-map' nodes which reference
+        // this Open PIC node do not need a parent address specifier.
+		#address-cells = <0>;
+
+        // Two cells to encode interrupt sources.
+		#interrupt-cells = <2>;
+
+        // Offset address of 0x40000 and size of 0x40000.
+		reg = <0x40000 0x40000>;
+
+        // Compatible with Open PIC.
+		compatible = "open-pic";
+
+        // The PIC should not be reset.
+		no-reset;
+	};
+
+EXAMPLE 2
+
+    /*
+     * An interrupt generating device that is wired to an Open PIC.
+     */
+    serial0: serial@4500 {
+        // Interrupt source '42' that is active high level-sensitive.
+        // Note that there are only two cells as specified in the interrupt
+        // parent's '#interrupt-cells' property.
+        interrupts = <42 2>;
+
+        // The interrupt controller that this device is wired to.
+        interrupt-parent = <&mpic>;
+    };
+
+REFERENCES
+
+[1] Power.org (TM) Standard for Embedded Power Architecture (TM) Platform
+    Requirements (ePAPR), Version 1.0, July 2008.
+    (http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf)
+
-- 
1.6.3.3

[PATCH v2 3/3] powerpc: make MPIC honor the 'no-reset' device tree property

[Meador Inge]

This property, defined in the Open PIC binding, tells the kernel not to use the
reset bit in the global configuration register.  Additionally, its presence
mandates that only sources which are actually used (i.e. appear in the device
tree) should have their VECPRI bits initialized.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
CC: Hollis Blanchard <hollis_blanchard@mentor.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/mpic.h |    4 ++-
 arch/powerpc/sysdev/mpic.c      |   54 ++++++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index 9b94f18..688e3e0 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -322,6 +322,8 @@ struct mpic
 #ifdef CONFIG_PM
 	struct mpic_irq_save	*save_data;
 #endif
+
+	int cpu;
 };
 
 /*
@@ -333,7 +335,7 @@ struct mpic
  */
 
 /* This is the primary controller, only that one has IPIs and
- * has afinity control. A non-primary MPIC always uses CPU0
+ * has affinity control. A non-primary MPIC always uses CPU0
  * registers only
  */
 #define MPIC_PRIMARY			0x00000001
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index a98f41d..5f17022 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -308,6 +308,15 @@ static inline void mpic_map(struct mpic *mpic, struct device_node *node,
 #define mpic_map(m,n,p,b,o,s)	_mpic_map_mmio(m,p,b,o,s)
 #endif /* !CONFIG_PPC_DCR */
 
+static inline void mpic_init_vector(struct mpic *mpic, int source)
+{
+	/* start with vector = source number, and masked */
+	u32 vecpri = MPIC_VECPRI_MASK | source | (8 << MPIC_VECPRI_PRIORITY_SHIFT);
+		
+	/* init hw */
+	mpic_irq_write(source, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
+	mpic_irq_write(source, MPIC_INFO(IRQ_DESTINATION), 1 << mpic->cpu);
+}
 
 
 /* Check if we have one of those nice broken MPICs with a flipped endian on
@@ -622,6 +631,14 @@ static unsigned int mpic_is_ipi(struct mpic *mpic, unsigned int irq)
 	return (src >= mpic->ipi_vecs[0] && src <= mpic->ipi_vecs[3]);
 }
 
+/* Determine if the linux irq is a timer interrupt */
+static unsigned int mpic_is_timer_interrupt(struct mpic *mpic, unsigned int irq)
+{
+	unsigned int src = mpic_irq_to_hw(irq);
+
+	return (src >= mpic->timer_vecs[0] && src <= mpic->timer_vecs[3]);
+}
+
 
 /* Convert a cpu mask from logical to physical cpu numbers. */
 static inline u32 mpic_physmask(u32 cpumask)
@@ -963,6 +980,15 @@ static int mpic_host_map(struct irq_host *h, unsigned int virq,
 	if (hw >= mpic->irq_count)
 		return -EINVAL;
 
+	/* If the MPIC was reset, then all vectors have already been
+	 * initialized.  Otherwise, the appropriate vector needs to be
+	 * initialized here to ensure that only used sources are setup with
+	 * a vector.
+	 */
+	if (!(mpic->flags & MPIC_WANTS_RESET))
+		if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw)))
+			mpic_init_vector(mpic, hw);
+
 	mpic_msi_reserve_hwirq(mpic, hw);
 
 	/* Default chip */
@@ -1129,7 +1155,16 @@ struct mpic * __init mpic_alloc(struct device_node *node,
 	mpic_map(mpic, node, paddr, &mpic->tmregs, MPIC_INFO(TIMER_BASE), 0x1000);
 
 	/* Reset */
-	if (flags & MPIC_WANTS_RESET) {
+
+	/* When using a device-node, reset requests are only honored if the MPIC
+	 * is allowed to reset.
+	 */
+	if (node && of_get_property(node, "no-reset", NULL)) {
+		mpic->flags &= ~MPIC_WANTS_RESET;
+	}
+
+	if (mpic->flags & MPIC_WANTS_RESET) {
+		printk(KERN_DEBUG "mpic: Resetting\n");
 		mpic_write(mpic->gregs, MPIC_INFO(GREG_GLOBAL_CONF_0),
 			   mpic_read(mpic->gregs, MPIC_INFO(GREG_GLOBAL_CONF_0))
 			   | MPIC_GREG_GCONF_RESET);
@@ -1246,7 +1281,6 @@ void __init mpic_set_default_senses(struct mpic *mpic, u8 *senses, int count)
 void __init mpic_init(struct mpic *mpic)
 {
 	int i;
-	int cpu;
 
 	BUG_ON(mpic->num_sources == 0);
 
@@ -1290,18 +1324,14 @@ void __init mpic_init(struct mpic *mpic)
 	mpic_pasemi_msi_init(mpic);
 
 	if (mpic->flags & MPIC_PRIMARY)
-		cpu = hard_smp_processor_id();
+		mpic->cpu = hard_smp_processor_id();
 	else
-		cpu = 0;
+		mpic->cpu = 0;
 
-	for (i = 0; i < mpic->num_sources; i++) {
-		/* start with vector = source number, and masked */
-		u32 vecpri = MPIC_VECPRI_MASK | i |
-			(8 << MPIC_VECPRI_PRIORITY_SHIFT);
-		
-		/* init hw */
-		mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
-		mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION), 1 << cpu);
+	if (mpic->flags & MPIC_WANTS_RESET) {
+		for (i = 0; i < mpic->num_sources; i++) {
+			mpic_init_vector(mpic, i);
+		}
 	}
 	
 	/* Init spurious vector */
-- 
1.6.3.3

Re: [PATCH v2 0/3] powerpc: Open PIC binding and 'no-reset' implementation

[Kumar Gala]

On Feb 2, 2011, at 7:51 PM, Meador Inge wrote:

> This patch set provides a binding for Open PIC and implements support for
> a new property, specified by that binding, called 'no-reset'.  With 'no-reset'
> in place the 'protected-sources' property is no longer needed and was removed.
> 
> Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> CC: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> 
> Meador Inge (3):
>  powerpc: Removing support for 'protected-sources'
>  powerpc: document the Open PIC device tree binding
>  powerpc: make MPIC honor the 'no-reset' device tree property
> 
> Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
> arch/powerpc/include/asm/mpic.h                 |    7 +-
> arch/powerpc/sysdev/mpic.c                      |   92 ++++++++----------
> 3 files changed, 160 insertions(+), 54 deletions(-)
> create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
> 

Please cleanup all the .dts that are impacted by this.

- k

Re: [PATCH v2 0/3] powerpc: Open PIC binding and 'no-reset' implementation

[Kumar Gala]

On Feb 2, 2011, at 7:51 PM, Meador Inge wrote:

> This patch set provides a binding for Open PIC and implements support =
for
> a new property, specified by that binding, called 'no-reset'.  With =
'no-reset'
> in place the 'protected-sources' property is no longer needed and was =
removed.
>=20
> Signed-off-by: Meador Inge <meador_inge@mentor.com>
> CC: Hollis Blanchard <hollis_blanchard@mentor.com>
>=20
> Meador Inge (3):
>  powerpc: Removing support for 'protected-sources'
>  powerpc: document the Open PIC device tree binding
>  powerpc: make MPIC honor the 'no-reset' device tree property
>=20
> Documentation/powerpc/dts-bindings/open-pic.txt |  115 =
+++++++++++++++++++++++
> arch/powerpc/include/asm/mpic.h                 |    7 +-
> arch/powerpc/sysdev/mpic.c                      |   92 =
++++++++----------
> 3 files changed, 160 insertions(+), 54 deletions(-)
> create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>=20

Please cleanup all the .dts that are impacted by this.

- k

Re: [PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Arnd Bergmann]

On Thursday 03 February 2011, Meador Inge wrote:
> In a recent discussion [1, 2] concerning device trees for AMP systems, the
> question of whether we really need 'protected-sources' arose.  The general
> consensus was that if you don't want a source to be used, then it should *not*
> be mentioned in an 'interrupts' property.  If a source really needs to be
> mentioned, then it should be put in a property other than 'interrupts' with
> a specific binding for that use case.
> 
> [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
> [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html

That doesn't work in the case that this code was written for:

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg01394.html

The problem is that you don't want the mpic to initialize the interrupt
line to the default, but instead leave it at whatever the boot firmware
has set up. Note that interrupt is not listed in any "interrupts"
property of any of the devices on the CPU interpreting the device
tree, but it may be mentioned in the device tree that another CPU
uses to access the same MPIC.

	Arnd

Re: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Grant Likely]

On Wed, Feb 2, 2011 at 6:51 PM, Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> This binding documents several properties that have been in use for quite
> some time, and adds one new property 'no-reset', which controls whether the
> Open PIC should be reset during runtime initialization.
>
> The general formatting and interrupt specifier definition is based off of
> Stuart Yoder's FSL MPIC binding.
>
> Signed-off-by: Meador Inge <meador_inge-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> CC: Hollis Blanchard <hollis_blanchard-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> CC: Stuart Yoder <stuart.yoder-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
>  1 files changed, 115 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>
> diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt b/Documentation/powerpc/dts-bindings/open-pic.txt
> new file mode 100644
> index 0000000..447ef65
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/open-pic.txt
> @@ -0,0 +1,115 @@
> +* Open PIC Binding
> +
> +This binding specifies what properties must be available in the device tree
> +representation of an Open PIC compliant interrupt controller.  This binding is
> +based on the binding defined for Open PIC in [1] and is a superset of that
> +binding.
> +
> +PROPERTIES
> +
> +  NOTE: Many of these descriptions were paraphrased here from [1] to aid
> +        readability.
> +
> +  - compatible
> +      Usage: required
> +      Value type: <string>
> +      Definition: Specifies the compatibility list for the PIC.  The
> +          property value shall include "open-pic".
> +
> +  - reg
> +      Usage: required
> +      Value type: <prop-encoded-array>
> +      Definition: Specifies the base physical address(s) and size(s) of this
> +          PIC's addressable register space.
> +
> +  - interrupt-controller
> +      Usage: required
> +      Value type: <empty>
> +      Definition: The presence of this property identifies the node
> +          as an Open PIC.  No property value should be defined.
> +
> +  - #interrupt-cells
> +      Usage: required
> +      Value type: <u32>
> +      Definition: Specifies the number of cells needed to encode an
> +          interrupt source.  Shall be 2.
> +
> +  - #address-cells
> +      Usage: required
> +      Value type: <u32>
> +      Definition: Specifies the number of cells needed to encode an
> +          address.  The value of this property shall always be 0.
> +          As such, 'interrupt-map' nodes do not have to specify a
> +          parent unit address.
> +
> +  - no-reset
> +      Usage: optional
> +      Value type: <empty>
> +      Definition: The presence of this property indicates that the PIC
> +          should not be reset during runtime initialization.  The presence of
> +          this property also mandates that any initialization related to
> +          interrupt sources shall be limited to sources explicitly referenced
> +          in the device tree.

Please follow the lead set by the other binding documentation which is
more concise and tends to be of the form:

    Required properties:
        - reg : <description>
        - interrupt-controller : <description>

    Optional Properties:
        - no-reset : blah

I'm considering formalizing the binding format so that fully specified
and cross-referenced documentation can be generated from the bindings
directory.

Also, to avoid the potential of a future namespace collision, it would
not be a bad idea to name this openpic-no-reset or something that
makes it clear that this is a binding specific property.  "no-reset"
sounds generic enough to give me pause.

> +
> +INTERRUPT SPECIFIER DEFINITION
> +
> +  Interrupt specifiers consists of 2 cells encoded as
> +  follows:
> +
> +   <1st-cell>   interrupt-number
> +
> +                Identifies the interrupt source.
> +
> +   <2nd-cell>   level-sense information, encoded as follows:
> +                    0 = low-to-high edge triggered
> +                    1 = active low level-sensitive
> +                    2 = active high level-sensitive
> +                    3 = high-to-low edge triggered
> +
> +EXAMPLE 1
> +
> +    /*
> +     * An Open PIC interrupt controller
> +     */
> +       mpic: pic@40000 {
> +        // This is an interrupt controller node.
> +               interrupt-controller;
> +
> +        // No address cells so that 'interrupt-map' nodes which reference
> +        // this Open PIC node do not need a parent address specifier.
> +               #address-cells = <0>;
> +
> +        // Two cells to encode interrupt sources.
> +               #interrupt-cells = <2>;
> +
> +        // Offset address of 0x40000 and size of 0x40000.
> +               reg = <0x40000 0x40000>;
> +
> +        // Compatible with Open PIC.
> +               compatible = "open-pic";
> +
> +        // The PIC should not be reset.
> +               no-reset;
> +       };
> +
> +EXAMPLE 2
> +
> +    /*
> +     * An interrupt generating device that is wired to an Open PIC.
> +     */
> +    serial0: serial@4500 {
> +        // Interrupt source '42' that is active high level-sensitive.
> +        // Note that there are only two cells as specified in the interrupt
> +        // parent's '#interrupt-cells' property.
> +        interrupts = <42 2>;
> +
> +        // The interrupt controller that this device is wired to.
> +        interrupt-parent = <&mpic>;
> +    };
> +
> +REFERENCES
> +
> +[1] Power.org (TM) Standard for Embedded Power Architecture (TM) Platform
> +    Requirements (ePAPR), Version 1.0, July 2008.
> +    (http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf)
> +
> --
> 1.6.3.3
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Grant Likely]

On Wed, Feb 2, 2011 at 6:51 PM, Meador Inge <meador_inge@mentor.com> wrote:
> This binding documents several properties that have been in use for quite
> some time, and adds one new property 'no-reset', which controls whether t=
he
> Open PIC should be reset during runtime initialization.
>
> The general formatting and interrupt specifier definition is based off of
> Stuart Yoder's FSL MPIC binding.
>
> Signed-off-by: Meador Inge <meador_inge@mentor.com>
> CC: Hollis Blanchard <hollis_blanchard@mentor.com>
> CC: Stuart Yoder <stuart.yoder@freescale.com>
> ---
> =A0Documentation/powerpc/dts-bindings/open-pic.txt | =A0115 +++++++++++++=
++++++++++
> =A01 files changed, 115 insertions(+), 0 deletions(-)
> =A0create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>
> diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt b/Documentat=
ion/powerpc/dts-bindings/open-pic.txt
> new file mode 100644
> index 0000000..447ef65
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/open-pic.txt
> @@ -0,0 +1,115 @@
> +* Open PIC Binding
> +
> +This binding specifies what properties must be available in the device t=
ree
> +representation of an Open PIC compliant interrupt controller. =A0This bi=
nding is
> +based on the binding defined for Open PIC in [1] and is a superset of th=
at
> +binding.
> +
> +PROPERTIES
> +
> + =A0NOTE: Many of these descriptions were paraphrased here from [1] to a=
id
> + =A0 =A0 =A0 =A0readability.
> +
> + =A0- compatible
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <string>
> + =A0 =A0 =A0Definition: Specifies the compatibility list for the PIC. =
=A0The
> + =A0 =A0 =A0 =A0 =A0property value shall include "open-pic".
> +
> + =A0- reg
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <prop-encoded-array>
> + =A0 =A0 =A0Definition: Specifies the base physical address(s) and size(=
s) of this
> + =A0 =A0 =A0 =A0 =A0PIC's addressable register space.
> +
> + =A0- interrupt-controller
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <empty>
> + =A0 =A0 =A0Definition: The presence of this property identifies the nod=
e
> + =A0 =A0 =A0 =A0 =A0as an Open PIC. =A0No property value should be defin=
ed.
> +
> + =A0- #interrupt-cells
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <u32>
> + =A0 =A0 =A0Definition: Specifies the number of cells needed to encode a=
n
> + =A0 =A0 =A0 =A0 =A0interrupt source. =A0Shall be 2.
> +
> + =A0- #address-cells
> + =A0 =A0 =A0Usage: required
> + =A0 =A0 =A0Value type: <u32>
> + =A0 =A0 =A0Definition: Specifies the number of cells needed to encode a=
n
> + =A0 =A0 =A0 =A0 =A0address. =A0The value of this property shall always =
be 0.
> + =A0 =A0 =A0 =A0 =A0As such, 'interrupt-map' nodes do not have to specif=
y a
> + =A0 =A0 =A0 =A0 =A0parent unit address.
> +
> + =A0- no-reset
> + =A0 =A0 =A0Usage: optional
> + =A0 =A0 =A0Value type: <empty>
> + =A0 =A0 =A0Definition: The presence of this property indicates that the=
 PIC
> + =A0 =A0 =A0 =A0 =A0should not be reset during runtime initialization. =
=A0The presence of
> + =A0 =A0 =A0 =A0 =A0this property also mandates that any initialization =
related to
> + =A0 =A0 =A0 =A0 =A0interrupt sources shall be limited to sources explic=
itly referenced
> + =A0 =A0 =A0 =A0 =A0in the device tree.

Please follow the lead set by the other binding documentation which is
more concise and tends to be of the form:

    Required properties:
        - reg : <description>
        - interrupt-controller : <description>

    Optional Properties:
        - no-reset : blah

I'm considering formalizing the binding format so that fully specified
and cross-referenced documentation can be generated from the bindings
directory.

Also, to avoid the potential of a future namespace collision, it would
not be a bad idea to name this openpic-no-reset or something that
makes it clear that this is a binding specific property.  "no-reset"
sounds generic enough to give me pause.

> +
> +INTERRUPT SPECIFIER DEFINITION
> +
> + =A0Interrupt specifiers consists of 2 cells encoded as
> + =A0follows:
> +
> + =A0 <1st-cell> =A0 interrupt-number
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Identifies the interrupt source.
> +
> + =A0 <2nd-cell> =A0 level-sense information, encoded as follows:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00 =3D low-to-high edge triggered
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A01 =3D active low level-sensitive
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A02 =3D active high level-sensitiv=
e
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A03 =3D high-to-low edge triggered
> +
> +EXAMPLE 1
> +
> + =A0 =A0/*
> + =A0 =A0 * An Open PIC interrupt controller
> + =A0 =A0 */
> + =A0 =A0 =A0 mpic: pic@40000 {
> + =A0 =A0 =A0 =A0// This is an interrupt controller node.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupt-controller;
> +
> + =A0 =A0 =A0 =A0// No address cells so that 'interrupt-map' nodes which =
reference
> + =A0 =A0 =A0 =A0// this Open PIC node do not need a parent address speci=
fier.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <0>;
> +
> + =A0 =A0 =A0 =A0// Two cells to encode interrupt sources.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #interrupt-cells =3D <2>;
> +
> + =A0 =A0 =A0 =A0// Offset address of 0x40000 and size of 0x40000.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x40000 0x40000>;
> +
> + =A0 =A0 =A0 =A0// Compatible with Open PIC.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "open-pic";
> +
> + =A0 =A0 =A0 =A0// The PIC should not be reset.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 no-reset;
> + =A0 =A0 =A0 };
> +
> +EXAMPLE 2
> +
> + =A0 =A0/*
> + =A0 =A0 * An interrupt generating device that is wired to an Open PIC.
> + =A0 =A0 */
> + =A0 =A0serial0: serial@4500 {
> + =A0 =A0 =A0 =A0// Interrupt source '42' that is active high level-sensi=
tive.
> + =A0 =A0 =A0 =A0// Note that there are only two cells as specified in th=
e interrupt
> + =A0 =A0 =A0 =A0// parent's '#interrupt-cells' property.
> + =A0 =A0 =A0 =A0interrupts =3D <42 2>;
> +
> + =A0 =A0 =A0 =A0// The interrupt controller that this device is wired to=
.
> + =A0 =A0 =A0 =A0interrupt-parent =3D <&mpic>;
> + =A0 =A0};
> +
> +REFERENCES
> +
> +[1] Power.org (TM) Standard for Embedded Power Architecture (TM) Platfor=
m
> + =A0 =A0Requirements (ePAPR), Version 1.0, July 2008.
> + =A0 =A0(http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v=
1.0.pdf)
> +
> --
> 1.6.3.3
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Meador Inge]

On 02/03/2011 09:56 AM, Grant Likely wrote:
> On Wed, Feb 2, 2011 at 6:51 PM, Meador Inge<meador_inge@mentor.com>  wrote:
>> This binding documents several properties that have been in use for quite
>> some time, and adds one new property 'no-reset', which controls whether the
>> Open PIC should be reset during runtime initialization.
>>
>> The general formatting and interrupt specifier definition is based off of
>> Stuart Yoder's FSL MPIC binding.
>>
>> Signed-off-by: Meador Inge<meador_inge@mentor.com>
>> CC: Hollis Blanchard<hollis_blanchard@mentor.com>
>> CC: Stuart Yoder<stuart.yoder@freescale.com>
>> ---
>>   Documentation/powerpc/dts-bindings/open-pic.txt |  115 +++++++++++++++++++++++
>>   1 files changed, 115 insertions(+), 0 deletions(-)
>>   create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>>
>> diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt b/Documentation/powerpc/dts-bindings/open-pic.txt
>> new file mode 100644
>> index 0000000..447ef65
>> --- /dev/null
>> +++ b/Documentation/powerpc/dts-bindings/open-pic.txt
>> @@ -0,0 +1,115 @@
>> +* Open PIC Binding
>> +
>> +This binding specifies what properties must be available in the device tree
>> +representation of an Open PIC compliant interrupt controller.  This binding is
>> +based on the binding defined for Open PIC in [1] and is a superset of that
>> +binding.
>> +
>> +PROPERTIES
>> +
>> +  NOTE: Many of these descriptions were paraphrased here from [1] to aid
>> +        readability.
>> +
>> +  - compatible
>> +      Usage: required
>> +      Value type:<string>
>> +      Definition: Specifies the compatibility list for the PIC.  The
>> +          property value shall include "open-pic".
>> +
>> +  - reg
>> +      Usage: required
>> +      Value type:<prop-encoded-array>
>> +      Definition: Specifies the base physical address(s) and size(s) of this
>> +          PIC's addressable register space.
>> +
>> +  - interrupt-controller
>> +      Usage: required
>> +      Value type:<empty>
>> +      Definition: The presence of this property identifies the node
>> +          as an Open PIC.  No property value should be defined.
>> +
>> +  - #interrupt-cells
>> +      Usage: required
>> +      Value type:<u32>
>> +      Definition: Specifies the number of cells needed to encode an
>> +          interrupt source.  Shall be 2.
>> +
>> +  - #address-cells
>> +      Usage: required
>> +      Value type:<u32>
>> +      Definition: Specifies the number of cells needed to encode an
>> +          address.  The value of this property shall always be 0.
>> +          As such, 'interrupt-map' nodes do not have to specify a
>> +          parent unit address.
>> +
>> +  - no-reset
>> +      Usage: optional
>> +      Value type:<empty>
>> +      Definition: The presence of this property indicates that the PIC
>> +          should not be reset during runtime initialization.  The presence of
>> +          this property also mandates that any initialization related to
>> +          interrupt sources shall be limited to sources explicitly referenced
>> +          in the device tree.
>
> Please follow the lead set by the other binding documentation which is
> more concise and tends to be of the form:
>
>      Required properties:
>          - reg :<description>
>          - interrupt-controller :<description>
>
>      Optional Properties:
>          - no-reset : blah

OK, will do.  The one thing that I like about the other format, though, 
is that it specifies the value type.  That is a useful addition.

> I'm considering formalizing the binding format so that fully specified
> and cross-referenced documentation can be generated from the bindings
> directory.

Formalizing the binding format would be great.  Perhaps we should add a 
HOWTO write a new binding document to the "Documentation" directory? 
The would be a great place to capture some of the common pitfalls that 
have been coming up on the list lately (versioned compatibility tags, 
for example).

> Also, to avoid the potential of a future namespace collision, it would
> not be a bad idea to name this openpic-no-reset or something that
> makes it clear that this is a binding specific property.  "no-reset"
> sounds generic enough to give me pause.

Isn't that a little redundant, though (e.g. 
"/soc/pic/openpic-no-reset")?  It is already scoped to the PIC node:

    mpic: pic@40000 {
       compatible = "open-pic";
       no-reset;
    };

Or are you worried that someone will find the wrong "no-reset" property 
when searching from a location higher in the tree than the PIC node?

I don't have a serious objection to the idea, but it seems slightly odd 
to partially flatten the hierarchy back into the property names.  On the 
other hand, I do see the practical consideration of having a more unique 
property which might prevent programming confusion/errors.

-- 
Meador Inge     | meador_inge AT mentor.com
Mentor Embedded | http://www.mentor.com/embedded-software

Re: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Grant Likely]

On Thu, Feb 3, 2011 at 9:29 AM, Meador Inge <meador_inge@mentor.com> wrote:
> On 02/03/2011 09:56 AM, Grant Likely wrote:
>>
>> On Wed, Feb 2, 2011 at 6:51 PM, Meador Inge<meador_inge@mentor.com>
>>  wrote:
>>>
>>> This binding documents several properties that have been in use for quite
>>> some time, and adds one new property 'no-reset', which controls whether
>>> the
>>> Open PIC should be reset during runtime initialization.
>>>
>>> The general formatting and interrupt specifier definition is based off of
>>> Stuart Yoder's FSL MPIC binding.
>>>
>>> Signed-off-by: Meador Inge<meador_inge@mentor.com>
>>> CC: Hollis Blanchard<hollis_blanchard@mentor.com>
>>> CC: Stuart Yoder<stuart.yoder@freescale.com>
>>> ---
>>>  Documentation/powerpc/dts-bindings/open-pic.txt |  115
>>> +++++++++++++++++++++++
>>>  1 files changed, 115 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>>>
>>> diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt
>>> b/Documentation/powerpc/dts-bindings/open-pic.txt
>>> new file mode 100644
>>> index 0000000..447ef65
>>> --- /dev/null
>>> +++ b/Documentation/powerpc/dts-bindings/open-pic.txt
>>> @@ -0,0 +1,115 @@
>>> +* Open PIC Binding
>>> +
>>> +This binding specifies what properties must be available in the device
>>> tree
>>> +representation of an Open PIC compliant interrupt controller.  This
>>> binding is
>>> +based on the binding defined for Open PIC in [1] and is a superset of
>>> that
>>> +binding.
>>> +
>>> +PROPERTIES
>>> +
>>> +  NOTE: Many of these descriptions were paraphrased here from [1] to aid
>>> +        readability.
>>> +
>>> +  - compatible
>>> +      Usage: required
>>> +      Value type:<string>
>>> +      Definition: Specifies the compatibility list for the PIC.  The
>>> +          property value shall include "open-pic".
>>> +
>>> +  - reg
>>> +      Usage: required
>>> +      Value type:<prop-encoded-array>
>>> +      Definition: Specifies the base physical address(s) and size(s) of
>>> this
>>> +          PIC's addressable register space.
>>> +
>>> +  - interrupt-controller
>>> +      Usage: required
>>> +      Value type:<empty>
>>> +      Definition: The presence of this property identifies the node
>>> +          as an Open PIC.  No property value should be defined.
>>> +
>>> +  - #interrupt-cells
>>> +      Usage: required
>>> +      Value type:<u32>
>>> +      Definition: Specifies the number of cells needed to encode an
>>> +          interrupt source.  Shall be 2.
>>> +
>>> +  - #address-cells
>>> +      Usage: required
>>> +      Value type:<u32>
>>> +      Definition: Specifies the number of cells needed to encode an
>>> +          address.  The value of this property shall always be 0.
>>> +          As such, 'interrupt-map' nodes do not have to specify a
>>> +          parent unit address.
>>> +
>>> +  - no-reset
>>> +      Usage: optional
>>> +      Value type:<empty>
>>> +      Definition: The presence of this property indicates that the PIC
>>> +          should not be reset during runtime initialization.  The
>>> presence of
>>> +          this property also mandates that any initialization related to
>>> +          interrupt sources shall be limited to sources explicitly
>>> referenced
>>> +          in the device tree.
>>
>> Please follow the lead set by the other binding documentation which is
>> more concise and tends to be of the form:
>>
>>     Required properties:
>>         - reg :<description>
>>         - interrupt-controller :<description>
>>
>>     Optional Properties:
>>         - no-reset : blah
>
> OK, will do.  The one thing that I like about the other format, though, is
> that it specifies the value type.  That is a useful addition.
>
>> I'm considering formalizing the binding format so that fully specified
>> and cross-referenced documentation can be generated from the bindings
>> directory.
>
> Formalizing the binding format would be great.  Perhaps we should add a
> HOWTO write a new binding document to the "Documentation" directory? The
> would be a great place to capture some of the common pitfalls that have been
> coming up on the list lately (versioned compatibility tags, for example).
>
>> Also, to avoid the potential of a future namespace collision, it would
>> not be a bad idea to name this openpic-no-reset or something that
>> makes it clear that this is a binding specific property.  "no-reset"
>> sounds generic enough to give me pause.
>
> Isn't that a little redundant, though (e.g. "/soc/pic/openpic-no-reset")?
>  It is already scoped to the PIC node:
>
>   mpic: pic@40000 {
>      compatible = "open-pic";
>      no-reset;
>   };
>
> Or are you worried that someone will find the wrong "no-reset" property when
> searching from a location higher in the tree than the PIC node?
>
> I don't have a serious objection to the idea, but it seems slightly odd to
> partially flatten the hierarchy back into the property names.  On the other
> hand, I do see the practical consideration of having a more unique property
> which might prevent programming confusion/errors.

It's the sort of thing where properties with really generic names,
like no-reset, I could potentially see as gaining a meaning across the
whole tree.  For instance, in the not so distant past the 'status'
property was defined for all nodes to indicate whether or not the
device is usable.  If any binding defined status for its own purposes,
then it would now be broken.  It is worth a little bit of
consideration to avoid collisions with names that might gain a meaning
in the global domain.  I don't care much about what the specific name
is, and openpic-no-reset may indeed be a little long, so feel free to
suggest something that you like better.

g.

>
> --
> Meador Inge     | meador_inge AT mentor.com
> Mentor Embedded | http://www.mentor.com/embedded-software
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Grant Likely]

On Thu, Feb 3, 2011 at 9:29 AM, Meador Inge <meador_inge@mentor.com> wrote:
> On 02/03/2011 09:56 AM, Grant Likely wrote:
>>
>> On Wed, Feb 2, 2011 at 6:51 PM, Meador Inge<meador_inge@mentor.com>
>> =A0wrote:
>>>
>>> This binding documents several properties that have been in use for qui=
te
>>> some time, and adds one new property 'no-reset', which controls whether
>>> the
>>> Open PIC should be reset during runtime initialization.
>>>
>>> The general formatting and interrupt specifier definition is based off =
of
>>> Stuart Yoder's FSL MPIC binding.
>>>
>>> Signed-off-by: Meador Inge<meador_inge@mentor.com>
>>> CC: Hollis Blanchard<hollis_blanchard@mentor.com>
>>> CC: Stuart Yoder<stuart.yoder@freescale.com>
>>> ---
>>> =A0Documentation/powerpc/dts-bindings/open-pic.txt | =A0115
>>> +++++++++++++++++++++++
>>> =A01 files changed, 115 insertions(+), 0 deletions(-)
>>> =A0create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt
>>>
>>> diff --git a/Documentation/powerpc/dts-bindings/open-pic.txt
>>> b/Documentation/powerpc/dts-bindings/open-pic.txt
>>> new file mode 100644
>>> index 0000000..447ef65
>>> --- /dev/null
>>> +++ b/Documentation/powerpc/dts-bindings/open-pic.txt
>>> @@ -0,0 +1,115 @@
>>> +* Open PIC Binding
>>> +
>>> +This binding specifies what properties must be available in the device
>>> tree
>>> +representation of an Open PIC compliant interrupt controller. =A0This
>>> binding is
>>> +based on the binding defined for Open PIC in [1] and is a superset of
>>> that
>>> +binding.
>>> +
>>> +PROPERTIES
>>> +
>>> + =A0NOTE: Many of these descriptions were paraphrased here from [1] to=
 aid
>>> + =A0 =A0 =A0 =A0readability.
>>> +
>>> + =A0- compatible
>>> + =A0 =A0 =A0Usage: required
>>> + =A0 =A0 =A0Value type:<string>
>>> + =A0 =A0 =A0Definition: Specifies the compatibility list for the PIC. =
=A0The
>>> + =A0 =A0 =A0 =A0 =A0property value shall include "open-pic".
>>> +
>>> + =A0- reg
>>> + =A0 =A0 =A0Usage: required
>>> + =A0 =A0 =A0Value type:<prop-encoded-array>
>>> + =A0 =A0 =A0Definition: Specifies the base physical address(s) and siz=
e(s) of
>>> this
>>> + =A0 =A0 =A0 =A0 =A0PIC's addressable register space.
>>> +
>>> + =A0- interrupt-controller
>>> + =A0 =A0 =A0Usage: required
>>> + =A0 =A0 =A0Value type:<empty>
>>> + =A0 =A0 =A0Definition: The presence of this property identifies the n=
ode
>>> + =A0 =A0 =A0 =A0 =A0as an Open PIC. =A0No property value should be def=
ined.
>>> +
>>> + =A0- #interrupt-cells
>>> + =A0 =A0 =A0Usage: required
>>> + =A0 =A0 =A0Value type:<u32>
>>> + =A0 =A0 =A0Definition: Specifies the number of cells needed to encode=
 an
>>> + =A0 =A0 =A0 =A0 =A0interrupt source. =A0Shall be 2.
>>> +
>>> + =A0- #address-cells
>>> + =A0 =A0 =A0Usage: required
>>> + =A0 =A0 =A0Value type:<u32>
>>> + =A0 =A0 =A0Definition: Specifies the number of cells needed to encode=
 an
>>> + =A0 =A0 =A0 =A0 =A0address. =A0The value of this property shall alway=
s be 0.
>>> + =A0 =A0 =A0 =A0 =A0As such, 'interrupt-map' nodes do not have to spec=
ify a
>>> + =A0 =A0 =A0 =A0 =A0parent unit address.
>>> +
>>> + =A0- no-reset
>>> + =A0 =A0 =A0Usage: optional
>>> + =A0 =A0 =A0Value type:<empty>
>>> + =A0 =A0 =A0Definition: The presence of this property indicates that t=
he PIC
>>> + =A0 =A0 =A0 =A0 =A0should not be reset during runtime initialization.=
 =A0The
>>> presence of
>>> + =A0 =A0 =A0 =A0 =A0this property also mandates that any initializatio=
n related to
>>> + =A0 =A0 =A0 =A0 =A0interrupt sources shall be limited to sources expl=
icitly
>>> referenced
>>> + =A0 =A0 =A0 =A0 =A0in the device tree.
>>
>> Please follow the lead set by the other binding documentation which is
>> more concise and tends to be of the form:
>>
>> =A0 =A0 Required properties:
>> =A0 =A0 =A0 =A0 - reg :<description>
>> =A0 =A0 =A0 =A0 - interrupt-controller :<description>
>>
>> =A0 =A0 Optional Properties:
>> =A0 =A0 =A0 =A0 - no-reset : blah
>
> OK, will do. =A0The one thing that I like about the other format, though,=
 is
> that it specifies the value type. =A0That is a useful addition.
>
>> I'm considering formalizing the binding format so that fully specified
>> and cross-referenced documentation can be generated from the bindings
>> directory.
>
> Formalizing the binding format would be great. =A0Perhaps we should add a
> HOWTO write a new binding document to the "Documentation" directory? The
> would be a great place to capture some of the common pitfalls that have b=
een
> coming up on the list lately (versioned compatibility tags, for example).
>
>> Also, to avoid the potential of a future namespace collision, it would
>> not be a bad idea to name this openpic-no-reset or something that
>> makes it clear that this is a binding specific property. =A0"no-reset"
>> sounds generic enough to give me pause.
>
> Isn't that a little redundant, though (e.g. "/soc/pic/openpic-no-reset")?
> =A0It is already scoped to the PIC node:
>
> =A0 mpic: pic@40000 {
> =A0 =A0 =A0compatible =3D "open-pic";
> =A0 =A0 =A0no-reset;
> =A0 };
>
> Or are you worried that someone will find the wrong "no-reset" property w=
hen
> searching from a location higher in the tree than the PIC node?
>
> I don't have a serious objection to the idea, but it seems slightly odd t=
o
> partially flatten the hierarchy back into the property names. =A0On the o=
ther
> hand, I do see the practical consideration of having a more unique proper=
ty
> which might prevent programming confusion/errors.

It's the sort of thing where properties with really generic names,
like no-reset, I could potentially see as gaining a meaning across the
whole tree.  For instance, in the not so distant past the 'status'
property was defined for all nodes to indicate whether or not the
device is usable.  If any binding defined status for its own purposes,
then it would now be broken.  It is worth a little bit of
consideration to avoid collisions with names that might gain a meaning
in the global domain.  I don't care much about what the specific name
is, and openpic-no-reset may indeed be a little long, so feel free to
suggest something that you like better.

g.

>
> --
> Meador Inge =A0 =A0 | meador_inge AT mentor.com
> Mentor Embedded | http://www.mentor.com/embedded-software
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

RE: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Yoder Stuart-B08248]

> > +  - no-reset
> > +      Usage: optional
> > +      Value type: <empty>
> > +      Definition: The presence of this property indicates that the
> > + PIC
> > +          should not be reset during runtime initialization.  The
> > + presence of
> > +          this property also mandates that any initialization related
> > + to
> > +          interrupt sources shall be limited to sources explicitly
> > + referenced
> > +          in the device tree.
> 
> Please follow the lead set by the other binding documentation which is more
> concise and tends to be of the form:
> 
>     Required properties:
>         - reg : <description>
>         - interrupt-controller : <description>
> 
>     Optional Properties:
>         - no-reset : blah
> 
> I'm considering formalizing the binding format so that fully specified and
> cross-referenced documentation can be generated from the bindings
> directory.

Regarding the format-- The definition should also to specify the value
type.  I don't see this being consistently done in existing bindings.  
They are not completely unclear, but using consistent terms might help.

The ePAPR uses this convention:

    <empty>      # no value, a Boolean
     <u32>       # A 32-bit integer in big-endian format
     <u64>       # A 64-bit integer in big-endian format
   <string>      # null terminated
 <prop-encoded-array>    # format specific to the property
   <phandle>    # A <u32> value, referecnes another node
  <stringlist>    # A list of <string> values concatenated together.

The identifier prop-encoded-array came from precedence in other
of binding and ieee1275.   prop-encoded-arrays should be
be specifically defined in terms of # of cells and the 
meaning of each cell.

If you use the above types identifiers, there is no ambiguity.

Also, there are properties that don't necessarily fall in 'required'
and 'optional', but may be required depending on the context.  Thus
the 'Usage' identifier which Meador derived from my mpic binding
posted.   Usage could be:
               Required
               Optional
               See Definition

Stuart

RE: [PATCH v2 2/3] powerpc: document the Open PIC device tree binding

[Yoder Stuart-B08248]

> > + =A0- no-reset
> > + =A0 =A0 =A0Usage: optional
> > + =A0 =A0 =A0Value type: <empty>
> > + =A0 =A0 =A0Definition: The presence of this property indicates that t=
he
> > + PIC
> > + =A0 =A0 =A0 =A0 =A0should not be reset during runtime initialization.=
 =A0The
> > + presence of
> > + =A0 =A0 =A0 =A0 =A0this property also mandates that any initializatio=
n related
> > + to
> > + =A0 =A0 =A0 =A0 =A0interrupt sources shall be limited to sources expl=
icitly
> > + referenced
> > + =A0 =A0 =A0 =A0 =A0in the device tree.
>=20
> Please follow the lead set by the other binding documentation which is mo=
re
> concise and tends to be of the form:
>=20
>     Required properties:
>         - reg : <description>
>         - interrupt-controller : <description>
>=20
>     Optional Properties:
>         - no-reset : blah
>=20
> I'm considering formalizing the binding format so that fully specified an=
d
> cross-referenced documentation can be generated from the bindings
> directory.

Regarding the format-- The definition should also to specify the value
type.  I don't see this being consistently done in existing bindings. =20
They are not completely unclear, but using consistent terms might help.

The ePAPR uses this convention:

    <empty>      # no value, a Boolean
     <u32>       # A 32-bit integer in big-endian format
     <u64>       # A 64-bit integer in big-endian format
   <string>      # null terminated
 <prop-encoded-array>    # format specific to the property
   <phandle>    # A <u32> value, referecnes another node
  <stringlist>    # A list of <string> values concatenated together.

The identifier prop-encoded-array came from precedence in other
of binding and ieee1275.   prop-encoded-arrays should be
be specifically defined in terms of # of cells and the=20
meaning of each cell.

If you use the above types identifiers, there is no ambiguity.

Also, there are properties that don't necessarily fall in 'required'
and 'optional', but may be required depending on the context.  Thus
the 'Usage' identifier which Meador derived from my mpic binding
posted.   Usage could be:
               Required
               Optional
               See Definition

Stuart

Re: [PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Meador Inge]

On 02/03/2011 09:56 AM, Arnd Bergmann wrote:
> On Thursday 03 February 2011, Meador Inge wrote:
>> In a recent discussion [1, 2] concerning device trees for AMP systems, the
>> question of whether we really need 'protected-sources' arose.  The general
>> consensus was that if you don't want a source to be used, then it should *not*
>> be mentioned in an 'interrupts' property.  If a source really needs to be
>> mentioned, then it should be put in a property other than 'interrupts' with
>> a specific binding for that use case.
>>
>> [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
>> [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html
>
> That doesn't work in the case that this code was written for:
>
> http://www.mail-archive.com/linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org/msg01394.html
>
> The problem is that you don't want the mpic to initialize the interrupt
> line to the default, but instead leave it at whatever the boot firmware
> has set up. Note that interrupt is not listed in any "interrupts"
> property of any of the devices on the CPU interpreting the device
> tree, but it may be mentioned in the device tree that another CPU
> uses to access the same MPIC.
>
> 	Arnd

We touched on that use case before on list.  However, I did a really bad 
job of explaining things in the above patch description.  I understand 
that the sources that are being protected are mentioned in a device tree 
other than the one that actually interprets the 'protected-sources' 
property.

The idea is to try and expand the meaning of the 'no-reset' property to 
cover what 'protected-sources' was taking care of, but without 
explicitly naming the sources.

In the protected sources version of the code, the relevant MPIC 
initialization went something like (in 'mpic_init'):

	for (i = 0; i < mpic->num_sources; i++) {
		/* start with vector = source number, and masked */
		u32 vecpri = MPIC_VECPRI_MASK | i |
			(8 << MPIC_VECPRI_PRIORITY_SHIFT);
		
		/* check if protected */
		if (mpic->protected && test_bit(i, mpic->protected))
			continue;
		/* init hw */
		mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
		mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION), 1 << cpu);
	}

So unless a particular source was marked as protected, it would get the 
VECPRI and CPU binding initialization.  This is the exact behavior that 
you describe above, Arnd.

In the 'no-reset' model, the initialization looks more like (see PATCH 3 
in the set for the full implementation):

	if (mpic->flags & MPIC_WANTS_RESET) {
		for (i = 0; i < mpic->num_sources; i++) {
			mpic_init_vector(mpic, hw);
		}
	}

So in 'mpic_init' we don't initialize anything and then in 
'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with:

	if (!(mpic->flags & MPIC_WANTS_RESET))
		if (!(mpic_is_ipi(mpic, hw)
			|| mpic_is_timer_interrupt(mpic, hw)))
			mpic_init_vector(mpic, hw);

Thus when 'no-reset' is thrown it ensures that only the sources which 
are mentioned in the device tree are actually initialized.  The net 
effect should be the same as what 'protected-sources' was accomplishing, 
but without having to maintain the list of sources in the property cell.


-- 
Meador Inge     | meador_inge AT mentor.com
Mentor Embedded | http://www.mentor.com/embedded-software

Re: [PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Meador Inge]

On 02/03/2011 09:56 AM, Arnd Bergmann wrote:
> On Thursday 03 February 2011, Meador Inge wrote:
>> In a recent discussion [1, 2] concerning device trees for AMP systems, the
>> question of whether we really need 'protected-sources' arose.  The general
>> consensus was that if you don't want a source to be used, then it should *not*
>> be mentioned in an 'interrupts' property.  If a source really needs to be
>> mentioned, then it should be put in a property other than 'interrupts' with
>> a specific binding for that use case.
>>
>> [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
>> [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html
>
> That doesn't work in the case that this code was written for:
>
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg01394.html
>
> The problem is that you don't want the mpic to initialize the interrupt
> line to the default, but instead leave it at whatever the boot firmware
> has set up. Note that interrupt is not listed in any "interrupts"
> property of any of the devices on the CPU interpreting the device
> tree, but it may be mentioned in the device tree that another CPU
> uses to access the same MPIC.
>
> 	Arnd

We touched on that use case before on list.  However, I did a really bad 
job of explaining things in the above patch description.  I understand 
that the sources that are being protected are mentioned in a device tree 
other than the one that actually interprets the 'protected-sources' 
property.

The idea is to try and expand the meaning of the 'no-reset' property to 
cover what 'protected-sources' was taking care of, but without 
explicitly naming the sources.

In the protected sources version of the code, the relevant MPIC 
initialization went something like (in 'mpic_init'):

	for (i = 0; i < mpic->num_sources; i++) {
		/* start with vector = source number, and masked */
		u32 vecpri = MPIC_VECPRI_MASK | i |
			(8 << MPIC_VECPRI_PRIORITY_SHIFT);
		
		/* check if protected */
		if (mpic->protected && test_bit(i, mpic->protected))
			continue;
		/* init hw */
		mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
		mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION), 1 << cpu);
	}

So unless a particular source was marked as protected, it would get the 
VECPRI and CPU binding initialization.  This is the exact behavior that 
you describe above, Arnd.

In the 'no-reset' model, the initialization looks more like (see PATCH 3 
in the set for the full implementation):

	if (mpic->flags & MPIC_WANTS_RESET) {
		for (i = 0; i < mpic->num_sources; i++) {
			mpic_init_vector(mpic, hw);
		}
	}

So in 'mpic_init' we don't initialize anything and then in 
'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with:

	if (!(mpic->flags & MPIC_WANTS_RESET))
		if (!(mpic_is_ipi(mpic, hw)
			|| mpic_is_timer_interrupt(mpic, hw)))
			mpic_init_vector(mpic, hw);

Thus when 'no-reset' is thrown it ensures that only the sources which 
are mentioned in the device tree are actually initialized.  The net 
effect should be the same as what 'protected-sources' was accomplishing, 
but without having to maintain the list of sources in the property cell.


-- 
Meador Inge     | meador_inge AT mentor.com
Mentor Embedded | http://www.mentor.com/embedded-software

Re: [PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Arnd Bergmann]

On Friday 04 February 2011, Meador Inge wrote:
> On 02/03/2011 09:56 AM, Arnd Bergmann wrote:

> So in 'mpic_init' we don't initialize anything and then in 
> 'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with:
> 
> 	if (!(mpic->flags & MPIC_WANTS_RESET))
> 		if (!(mpic_is_ipi(mpic, hw)
> 			|| mpic_is_timer_interrupt(mpic, hw)))
> 			mpic_init_vector(mpic, hw);
> 
> Thus when 'no-reset' is thrown it ensures that only the sources which 
> are mentioned in the device tree are actually initialized.  The net 
> effect should be the same as what 'protected-sources' was accomplishing, 
> but without having to maintain the list of sources in the property cell.

That sounds like a good idea, but unfortunately, it's not what SLOF
implements on QS21/QS22. It's a legacy product and there won't be
any firmware updates. Moreover, it relies on the open firmware
implementation and cannot boot with a flattened device tree image,
so I don't see how your patch can work on the old systems.

Maybe you can treat the presence of a 'protected-sources' property
the same way that you treat the no-reset property?

	Arnd

Re: [PATCH v2 1/3] powerpc: Removing support for 'protected-sources'

[Arnd Bergmann]

On Friday 04 February 2011, Meador Inge wrote:
> On 02/03/2011 09:56 AM, Arnd Bergmann wrote:

> So in 'mpic_init' we don't initialize anything and then in 
> 'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with:
> 
> 	if (!(mpic->flags & MPIC_WANTS_RESET))
> 		if (!(mpic_is_ipi(mpic, hw)
> 			|| mpic_is_timer_interrupt(mpic, hw)))
> 			mpic_init_vector(mpic, hw);
> 
> Thus when 'no-reset' is thrown it ensures that only the sources which 
> are mentioned in the device tree are actually initialized.  The net 
> effect should be the same as what 'protected-sources' was accomplishing, 
> but without having to maintain the list of sources in the property cell.

That sounds like a good idea, but unfortunately, it's not what SLOF
implements on QS21/QS22. It's a legacy product and there won't be
any firmware updates. Moreover, it relies on the open firmware
implementation and cannot boot with a flattened device tree image,
so I don't see how your patch can work on the old systems.

Maybe you can treat the presence of a 'protected-sources' property
the same way that you treat the no-reset property?

	Arnd