[PATCH v4 0/2] powerpc: Open PIC binding and "pic-no-reset"

[PATCH v4 0/2] powerpc: Open PIC binding and "pic-no-reset"

[Meador Inge]

This patch set provides a binding for Open PIC and implements support for
a new property, specified by that binding, called "pic-no-reset".

v4 - Per Ben's feedback the protected sources implementation was left
     completely intact.  As such, the DTS cleanup and "protected-sources"
     removal patches were dropped.

v3 - the Open PIC binding was changed to be more consistent with existing
     bindings, several DTS files were cleaned up, "no-reset" was changed to
     "pic-no-reset", and a check to treat "protected-sources" as a synonym for
     "pic-no-reset" was added.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>

Meador Inge (2):
  powerpc: document the Open PIC device tree binding
  powerpc: make MPIC honor the "pic-no-reset" device tree property

 Documentation/powerpc/dts-bindings/open-pic.txt |   98 +++++++++++++++++++++++
 arch/powerpc/include/asm/mpic.h                 |    6 ++
 arch/powerpc/sysdev/mpic.c                      |   67 ++++++++++++----
 3 files changed, 155 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt

[PATCH v4 1/2] 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 'pic-no-reset', which controls the runtime
initialization behavior of the PIC.  More specifically, the presence of
'pic-no-reset' mandates that the PIC shall not be reset during runtime
initialization and that any initialization related to interrupt sources
shall be limited to sources explicitly referenced in the device tree.  This
functionality is useful in AMP systems where multiple OSes are sharing the
PIC and the reinitialization of the PIC can interfere with OSes that are
already up and running.

The 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 |   98 +++++++++++++++++++++++
 1 files changed, 98 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..909a902
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/open-pic.txt
@@ -0,0 +1,98 @@
+* 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.
+
+Required properties:
+
+  NOTE: Many of these descriptions were paraphrased here from [1] to aid
+        readability.
+
+    - compatible: Specifies the compatibility list for the PIC.  The type
+      shall be <string> and the value shall include "open-pic".
+
+    - reg: Specifies the base physical address(s) and size(s) of this
+      PIC's addressable register space.  The type shall be <prop-encoded-array>.
+
+    - interrupt-controller: The presence of this property identifies the node
+      as an Open PIC.  No property value shall be defined.
+
+    - #interrupt-cells: Specifies the number of cells needed to encode an
+      interrupt source.  The type shall be a <u32> and the value shall be 2.
+
+    - #address-cells: Specifies the number of cells needed to encode an
+      address.  The type shall be <u32> and the value shall be 0.  As such,
+      'interrupt-map' nodes do not have to specify a parent unit address.
+
+Optional properties:
+
+    - pic-no-reset: The presence of this property indicates that the PIC
+      shall not be reset during runtime initialization.  No property value shall
+      be defined.  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>: The interrupt-number that identifies the interrupt source.
+
+    - <2nd-cell>: The 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
+
+* Examples
+
+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 shall not be reset.
+		pic-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

Re: [PATCH v4 1/2] powerpc: document the Open PIC device tree binding

[Grant Likely]

On Fri, Feb 25, 2011 at 03:59:36PM -0600, Meador Inge wrote:
> This binding documents several properties that have been in use for quite
> some time, and adds one new property 'pic-no-reset', which controls the runtime
> initialization behavior of the PIC.  More specifically, the presence of
> 'pic-no-reset' mandates that the PIC shall not be reset during runtime
> initialization and that any initialization related to interrupt sources
> shall be limited to sources explicitly referenced in the device tree.  This
> functionality is useful in AMP systems where multiple OSes are sharing the
> PIC and the reinitialization of the PIC can interfere with OSes that are
> already up and running.
> 
> The 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>

Looks fine to me, except...

> ---
>  Documentation/powerpc/dts-bindings/open-pic.txt |   98 +++++++++++++++++++++++

Documentation/powerpc/dts-bindings has moved to
Documentation/devicetree/bindings

g.

[PATCH v4 2/2] powerpc: make MPIC honor the "pic-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.

Although, "pic-no-reset" can be used for the same use cases that
"protected-sources" is covering, the "protected-sources" implementation was
left completely intact.  This is a more pragmatic approach as there are
already several existing systems which use protected sources.  If
"pic-no-reset" *and* "protected-sources" are both used, however, then
"pic-no-reset" takes precedence in terms of the init behavior and the
sanity checks done by protected sources will still take place.

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 |    6 +++
 arch/powerpc/sysdev/mpic.c      |   67 +++++++++++++++++++++++++++++---------
 2 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index e000cce..7e1be12 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -325,6 +325,8 @@ struct mpic
 #ifdef CONFIG_PM
 	struct mpic_irq_save	*save_data;
 #endif
+
+	int cpu;
 };
 
 /*
@@ -367,6 +369,10 @@ struct mpic
 #define MPIC_SINGLE_DEST_CPU		0x00001000
 /* Enable CoreInt delivery of interrupts */
 #define MPIC_ENABLE_COREINT		0x00002000
+/* Disable resetting of the MPIC.
+ * NOTE: This flag trumps MPIC_WANTS_RESET.
+ */
+#define MPIC_NO_RESET			0x00004000
 
 /* MPIC HW modification ID */
 #define MPIC_REGSET_MASK		0xf0000000
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index b0c8469..eac1a3b 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)
@@ -967,6 +984,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_NO_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 */
@@ -1033,6 +1059,11 @@ static struct irq_host_ops mpic_host_ops = {
 	.xlate = mpic_host_xlate,
 };
 
+static int mpic_reset_prohibited(struct device_node *node)
+{
+	return node && of_get_property(node, "pic-no-reset", NULL);
+}
+
 /*
  * Exported functions
  */
@@ -1153,7 +1184,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 (mpic_reset_prohibited(node)) {
+		mpic->flags |= MPIC_NO_RESET;
+	}
+
+	if ((flags & MPIC_WANTS_RESET) && !(mpic->flags & MPIC_NO_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);
@@ -1270,7 +1310,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);
 
@@ -1314,21 +1353,17 @@ 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);
-		
-		/* 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);
+	if (!(mpic->flags & MPIC_NO_RESET)) {
+		for (i = 0; i < mpic->num_sources; i++) {
+			/* check if protected */
+			if (mpic->protected && test_bit(i, mpic->protected))
+				continue;
+			mpic_init_vector(mpic, i);
+		}
 	}
 	
 	/* Init spurious vector */
-- 
1.6.3.3

Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property

[Benjamin Herrenschmidt]

> diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
> index e000cce..7e1be12 100644
> --- a/arch/powerpc/include/asm/mpic.h
> +++ b/arch/powerpc/include/asm/mpic.h
> @@ -325,6 +325,8 @@ struct mpic
>  #ifdef CONFIG_PM
>  	struct mpic_irq_save	*save_data;
>  #endif
> +
> +	int cpu;
>  };

Why ? The MPIC isn't specifically associated with a CPU and whatever we
pick as default during boot isn't relevant later on, so I don't see why
we should have global permanent state here.
>  
> +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);
> +}

Just pass the CPU as an argument... but better... just don't do that,
put the code back where it was and ... see below :-)
 
>  /* 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)
> @@ -967,6 +984,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_NO_RESET)
> +		if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw)))
> +			mpic_init_vector(mpic, hw);
> +

The above isn't useful. Of those two registers you want to initialize,
afaik, one (the destination) will be initialized by the core calling
into set_affinity when the interrupt is requested, and the other one is
partially initialized in set_type, I'd say just modify set_type to
initialize the source as well, and problem solved, no ? Or is there a
chance that the interrupt was left unmasked ? I think it would be kosher
to mask it in set_type unconditionally, I don't think it's ever supposed
to be called for an enabled interrupt.

>  	mpic_msi_reserve_hwirq(mpic, hw);
>  
>  	/* Default chip */
> @@ -1033,6 +1059,11 @@ static struct irq_host_ops mpic_host_ops = {
>  	.xlate = mpic_host_xlate,
>  };
>  
> +static int mpic_reset_prohibited(struct device_node *node)
> +{
> +	return node && of_get_property(node, "pic-no-reset", NULL);
> +}
> +
>  /*
>   * Exported functions
>   */
> @@ -1153,7 +1184,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 (mpic_reset_prohibited(node)) {
> +		mpic->flags |= MPIC_NO_RESET;
> +	}

No { } for single line nested statements

> +	if ((flags & MPIC_WANTS_RESET) && !(mpic->flags & MPIC_NO_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);
> @@ -1270,7 +1310,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);
>  
> @@ -1314,21 +1353,17 @@ 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;

Get rid of all that.
 
> -	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);
> +	if (!(mpic->flags & MPIC_NO_RESET)) {
> +		for (i = 0; i < mpic->num_sources; i++) {
> +			/* check if protected */
> +			if (mpic->protected && test_bit(i, mpic->protected))
> +				continue;
> +			mpic_init_vector(mpic, i);
> +		}
>  	}
>  	
>  	/* Init spurious vector */

Cheers,
Ben.

Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property

[Meador Inge]

On 03/01/2011 09:22 PM, Benjamin Herrenschmidt wrote:
>
>> diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
>> index e000cce..7e1be12 100644
>> --- a/arch/powerpc/include/asm/mpic.h
>> +++ b/arch/powerpc/include/asm/mpic.h
>> @@ -325,6 +325,8 @@ struct mpic
>>   #ifdef CONFIG_PM
>>   	struct mpic_irq_save	*save_data;
>>   #endif
>> +
>> +	int cpu;
>>   };
>
> Why ? The MPIC isn't specifically associated with a CPU and whatever we
> pick as default during boot isn't relevant later on, so I don't see why
> we should have global permanent state here.

I agree.  I shouldn't have cached that.  We should probably introduce a 
helper function to get the cpuid, though.  The:

	unsigned int cpu = 0;

	if (mpic->flags & MPIC_PRIMARY)
		cpu = hard_smp_processor_id();

code is scattered in three places: '_mpic_cpu_write', '_mpic_cpu_read', 
and 'mpic_init'.

>>   /* 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)
>> @@ -967,6 +984,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_NO_RESET)
>> +		if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw)))
>> +			mpic_init_vector(mpic, hw);
>> +
>
> The above isn't useful. Of those two registers you want to initialize,
> afaik, one (the destination) will be initialized by the core calling
> into set_affinity when the interrupt is requested, and the other one is

AFAIK, we can't rely on 'set_affinity' always being called.  I don't 
think it is called at all when !defined(CONFIG_SMP) and if it was, then 
that would be an error:

	/* include/linux/irq.h */

	#else /* CONFIG_SMP */

	static inline int irq_set_affinity(unsigned int irq,
		const struct cpumask *m)
	{
		return -EINVAL;
	}

> partially initialized in set_type, I'd say just modify set_type to
> initialize the source as well, and problem solved, no ?

The priority has to be initialized as well.  They could both be done in 
'mpic_set_irq_type', but that seems like a weird place since it has 
nothing to do with actually setting the type.

Since we already have 'mpic_irq_set_priority' and 'mpic_set_vector', 
perhaps a better option is to add 'mpic_set_destination' and put the 
following in 'mpic_host_map' (using the cpuid helper function suggested 
above):

	/* Lazy source init when MPIC_NO_RESET */
	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
		mpic_set_vector(virq, hw);
		mpic_set_destination(virq, mpic_cpuid(mpic));
		mpic_irq_set_priority(virq, 8);
	}

It is more overhead, but it reads well.  Thoughts?

> Or is there a chance that the interrupt was left unmasked ?

There shouldn't be.  The original idea was that either the boot program 
would leave it masked or one of the AMP OSes would boot without 
'pic-no-reset', which would mask everything.  Most likely the boot program.

> I think it would be kosher to mask it in set_type unconditionally, I don't think it's ever supposed
> to be called for an enabled interrupt.

I will look into that.

Thanks,

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

Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property

[Benjamin Herrenschmidt]

On Thu, 2011-03-10 at 11:23 -0600, Meador Inge wrote:
> I agree.  I shouldn't have cached that.  We should probably introduce a 
> helper function to get the cpuid, though.  The:
> 
>         unsigned int cpu = 0;
> 
>         if (mpic->flags & MPIC_PRIMARY)
>                 cpu = hard_smp_processor_id();
> 
> code is scattered in three places: '_mpic_cpu_write', '_mpic_cpu_read', 
> and 'mpic_init'. 

Right, but that code must act on the current CPU, not a cached per-MPIC
variant. Doing a helper to factor the above 2 lines is fine if you wish
to do so but then make it a separate patch.

Cheers,
Ben.

Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property

[Benjamin Herrenschmidt]

On Thu, 2011-03-10 at 11:23 -0600, Meador Inge wrote:
> AFAIK, we can't rely on 'set_affinity' always being called.  I don't 
> think it is called at all when !defined(CONFIG_SMP) and if it was,
> then that would be an error:
> 
>         /* include/linux/irq.h */
> 
>         #else /* CONFIG_SMP */
> 
>         static inline int irq_set_affinity(unsigned int irq,
>                 const struct cpumask *m)
>         {
>                 return -EINVAL;
>         }

You are right. We do need to set a sane default then.

> > partially initialized in set_type, I'd say just modify set_type to
> > initialize the source as well, and problem solved, no ?
> 
> The priority has to be initialized as well.  They could both be done
> in 
> 'mpic_set_irq_type', but that seems like a weird place since it has 
> nothing to do with actually setting the type.
> 
> Since we already have 'mpic_irq_set_priority' and 'mpic_set_vector', 
> perhaps a better option is to add 'mpic_set_destination' and put the 
> following in 'mpic_host_map' (using the cpuid helper function
> suggested 
> above):
> 
>         /* Lazy source init when MPIC_NO_RESET */
>         if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
>                 mpic_set_vector(virq, hw);
>                 mpic_set_destination(virq, mpic_cpuid(mpic));
>                 mpic_irq_set_priority(virq, 8);
>         }
> 
> It is more overhead, but it reads well.  Thoughts?

No objection.

Cheers,
Ben.