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

[PATCH v3 0/4] 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".  With
"pic-no-reset" in place the "protected-sources" property is no longer needed
and its full implementation was removed.  "protected-sources" is still checked
for, however, for legacy purposes.

For v3 of this patch 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: Hollis Blanchard <hollis_blanchard@mentor.com>

Meador Inge (4):
  powerpc: Removing support for 'protected-sources'
  powerpc: document the Open PIC device tree binding
  powerpc: make MPIC honor the "pic-no-reset" device tree property
  powerpc: Replacing "protected-sources" with "pic-no-reset" in DTS
    files

 Documentation/powerpc/dts-bindings/open-pic.txt |   98 ++++++++++++++++++++++
 arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts  |    6 +-
 arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts  |   11 +--
 arch/powerpc/boot/dts/p2020rdb_camp_core0.dts   |    7 +--
 arch/powerpc/boot/dts/p2020rdb_camp_core1.dts   |    7 +--
 arch/powerpc/include/asm/mpic.h                 |    7 +-
 arch/powerpc/sysdev/mpic.c                      |  102 ++++++++++++-----------
 7 files changed, 157 insertions(+), 81 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/open-pic.txt

[PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Meador Inge]

In a recent thread [1,2,3] concerning device trees for AMP systems, the
question of whether we really need 'protected-sources' arose.  The general
consensus was that a new boolean property 'pic-no-reset' (described in more
detail in a following patch) could be expanded to cover the use cases that
'protected-sources' was covering.

One concern that was raised was for legacy systems which already use the
'protected-sources' property [4].  For legacy use cases, 'protected-sources'
will be treated as an alias of 'pic-no-reset'.  The sources
encoded in the 'protected-sources' cells, however, will not be processed.  This
legacy check is added in a later patch in the series.

[1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
[2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html
[3] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004043.html
[4] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-February/004254.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

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Benjamin Herrenschmidt]

On Fri, 2011-02-04 at 17:25 -0600, Meador Inge wrote:
> In a recent thread [1,2,3] concerning device trees for AMP systems, the
> question of whether we really need 'protected-sources' arose.  The general
> consensus was that a new boolean property 'pic-no-reset' (described in more
> detail in a following patch) could be expanded to cover the use cases that
> 'protected-sources' was covering.
> 
> One concern that was raised was for legacy systems which already use the
> 'protected-sources' property [4].  For legacy use cases, 'protected-sources'
> will be treated as an alias of 'pic-no-reset'.  The sources
> encoded in the 'protected-sources' cells, however, will not be processed.  This
> legacy check is added in a later patch in the series.

I'm a bit annoyed here. Why do we need to do that ? Those Cell machines
are going to be real bastards to find and test with, and I don't really
see the point...

The reason for not resetting the MPIC wasn't -only- about the protected
sources, but also because, from memory, some MPIC implementations had
issues when toggling the reset lines (pseries I think is one).

That's actually why the MPIC_WANTS_RESET flag is working the other way
around, only platforms that actually want the reset set it.

This is orthogonal to the need to touch or not touch the interrupt
sources as set by firmware. Yes, having protected sources probably
implies no-reset but the reverse isn't necessarily true.

Ben.
 
> [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html
> [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html
> [3] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004043.html
> [4] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-February/004254.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

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Meador Inge]

On 02/06/2011 05:35 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2011-02-04 at 17:25 -0600, Meador Inge wrote:
>> In a recent thread [1,2,3] concerning device trees for AMP systems, the
>> question of whether we really need 'protected-sources' arose.  The general
>> consensus was that a new boolean property 'pic-no-reset' (described in more
>> detail in a following patch) could be expanded to cover the use cases that
>> 'protected-sources' was covering.
>>
>> One concern that was raised was for legacy systems which already use the
>> 'protected-sources' property [4].  For legacy use cases, 'protected-sources'
>> will be treated as an alias of 'pic-no-reset'.  The sources
>> encoded in the 'protected-sources' cells, however, will not be processed.  This
>> legacy check is added in a later patch in the series.
>
> I'm a bit annoyed here. Why do we need to do that ? Those Cell machines

Apologies, that certainly was not the intent.

> are going to be real bastards to find and test with, and I don't really
> see the point...

The idea came up when submitting a patch for documenting an Open PIC 
binding.  The following two properties were documented in that binding: 
(1) "protected-sources" and (2) "pic-no-reset".  "pic-no-reset" is a 
newly proposed property with the intent of specifying from a device tree 
that the PIC should not be reset.  The question of whether the one 
property, "pic-no-reset", would suffice for both purposes came up.

It seems reasonable that "pic-no-reset" could satisfy the use case that 
"protected-sources" is covering (since all of the sources that a 
particular machine is actually using are already explicitly mentioned in 
the device tree) and the use case of marking from a device tree that the 
PIC should not be reset.  So it is not so much as a need as it is a 
potential simplification.  It sounds like as a practical concern 
(several systems using "protected-sources" are already in the wild and 
testing considerations) that this may not be possible, which is fine.

> The reason for not resetting the MPIC wasn't -only- about the protected
> sources, but also because, from memory, some MPIC implementations had
> issues when toggling the reset lines (pseries I think is one).
>
> That's actually why the MPIC_WANTS_RESET flag is working the other way
> around, only platforms that actually want the reset set it.
>
> This is orthogonal to the need to touch or not touch the interrupt
> sources as set by firmware. Yes, having protected sources probably
> implies no-reset but the reverse isn't necessarily true.

So barring the removal of protected sources, does the inclusion of the 
"pic-no-reset" property seem reasonable?

> Ben.

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

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Benjamin Herrenschmidt]

On Sun, 2011-02-06 at 19:32 -0600, Meador Inge wrote:
> So barring the removal of protected sources, does the inclusion of the
> "pic-no-reset" property seem reasonable? 

Sure.

Ben.

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Meador Inge]

On 02/06/2011 05:35 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2011-02-04 at 17:25 -0600, Meador Inge wrote:
>> In a recent thread [1,2,3] concerning device trees for AMP systems, the
>> question of whether we really need 'protected-sources' arose.  The general
>> consensus was that a new boolean property 'pic-no-reset' (described in more
>> detail in a following patch) could be expanded to cover the use cases that
>> 'protected-sources' was covering.
>>
>> One concern that was raised was for legacy systems which already use the
>> 'protected-sources' property [4].  For legacy use cases, 'protected-sources'
>> will be treated as an alias of 'pic-no-reset'.  The sources
>> encoded in the 'protected-sources' cells, however, will not be processed.  This
>> legacy check is added in a later patch in the series.
>
> I'm a bit annoyed here. Why do we need to do that ? Those Cell machines
> are going to be real bastards to find and test with, and I don't really
> see the point...

In my previous reply I said that "it is not so much as a need as it is a 
potential simplification."  After further reflection, I don't think that 
is completely true.  As we get into AMP systems with higher core counts, 
then implementing this functionality using the existing 
"protected-sources" implementation versus the new "pic-no-reset" work is 
going to be harder to maintain.

The reason being that *every* OS instance has to know about *every* 
other OSes interrupt sources, which is a little gross.  You can see this 
happening already in "arch/powerpc/boot/dts/p2020rdb_camp_core0.dts" and 
"arch/powerpc/boot/dts/p2020rdb_camp_core1.dts":

	// p2020rdb_camp_core0.dts
	mpic: pic@40000 {
	...
		// Sources used by the OS on core 1
		protected-sources = <
		42 76 77 78 79 /* serial1 , dma2 */
		29 30 34 26 /* enet0, pci1 */
		0xe0 0xe1 0xe2 0xe3 /* msi */
		0xe4 0xe5 0xe6 0xe7
		>;
	};

	// p2020rdb_camp_core1.dts
	mpic: pic@40000 {
	...
		// Sources used by the OS on core 0
		protected-sources = <
		17 18 43 42 59 47 /*ecm, mem, i2c, serial0, spi,gpio */
		16 20 21 22 23 28 	/* L2, dma1, USB */
		03 35 36 40 31 32 33 	/* mdio, enet1, enet2 */
		72 45 58 25 		/* sdhci, crypto , pci */
		>;
	};

It is going to be a real pain to keep all of the lists up to date. 
Especially considering we already have sufficient information in the 
device tree to do this work.  I do understand the concern of 
finding/testing the older systems.  However, is the testing of those 
systems enough to keep out the proposed change and potentially lower 
maintenance in the future?  Is the legacy system argument the only 
reason to keep this change out or are there other technical deficiencies?

Also, in the proposed MPIC modifications there is a check for protected 
sources (it is treated as an alias for "pic-no-reset"; see PATCH 3 in 
the set) that should provide functionality equivalent to what systems 
using "protected-sources" already have.  That check only looks for the 
presence of "protected-sources" and does not process the cells.  Another 
option would be to leave in the protected sources implementation (but 
undocumented in the binding) and have the full "pic-no-reset" behavior 
there as well (and documented in the binding).

If this has no chance of acceptance (?), then I will just re-submit the 
binding and implementation with "protected-sources" and the limited form 
of "pic-no-reset".

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

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Benjamin Herrenschmidt]

> In my previous reply I said that "it is not so much as a need as it is a 
> potential simplification."  After further reflection, I don't think that 
> is completely true.  As we get into AMP systems with higher core counts, 
> then implementing this functionality using the existing 
> "protected-sources" implementation versus the new "pic-no-reset" work is 
> going to be harder to maintain.

I'm not arguing that your approach isn't more suitable for AMP systems,
I just want to leave the existing protected-sources mechanism alone. I'm
not opposing adding a new, better, mechanism for newer platforms.

However, I'd name it differently. "pic-no-reset" doesn't carry enough
meaning in that case. What we want to point out here is that the PIC
has been pre-initialized.

Another option, which may be cleaner, is to stick to "no-reset" (no need
for pic- prefix) and make it do just that (prevent the reset), and then
use a positive variant of "protected-sources", call it
"allowed-sources". Maybe even make it a series of ranges. Then have the
MPIC only access these.

I think this is more robust as it would also prevent "accidental" use of
the wrong sources (bad device-tree, drivers that let you muck around
with irq numbers, etc...).

Cheers,
Ben.

> The reason being that *every* OS instance has to know about *every* 
> other OSes interrupt sources, which is a little gross.  You can see this 
> happening already in "arch/powerpc/boot/dts/p2020rdb_camp_core0.dts" and 
> "arch/powerpc/boot/dts/p2020rdb_camp_core1.dts":
> 
> 	// p2020rdb_camp_core0.dts
> 	mpic: pic@40000 {
> 	...
> 		// Sources used by the OS on core 1
> 		protected-sources = <
> 		42 76 77 78 79 /* serial1 , dma2 */
> 		29 30 34 26 /* enet0, pci1 */
> 		0xe0 0xe1 0xe2 0xe3 /* msi */
> 		0xe4 0xe5 0xe6 0xe7
> 		>;
> 	};
> 
> 	// p2020rdb_camp_core1.dts
> 	mpic: pic@40000 {
> 	...
> 		// Sources used by the OS on core 0
> 		protected-sources = <
> 		17 18 43 42 59 47 /*ecm, mem, i2c, serial0, spi,gpio */
> 		16 20 21 22 23 28 	/* L2, dma1, USB */
> 		03 35 36 40 31 32 33 	/* mdio, enet1, enet2 */
> 		72 45 58 25 		/* sdhci, crypto , pci */
> 		>;
> 	};
> 
> It is going to be a real pain to keep all of the lists up to date. 
> Especially considering we already have sufficient information in the 
> device tree to do this work.  I do understand the concern of 
> finding/testing the older systems.  However, is the testing of those 
> systems enough to keep out the proposed change and potentially lower 
> maintenance in the future?  Is the legacy system argument the only 
> reason to keep this change out or are there other technical deficiencies?
> 
> Also, in the proposed MPIC modifications there is a check for protected 
> sources (it is treated as an alias for "pic-no-reset"; see PATCH 3 in 
> the set) that should provide functionality equivalent to what systems 
> using "protected-sources" already have.  That check only looks for the 
> presence of "protected-sources" and does not process the cells.  Another 
> option would be to leave in the protected sources implementation (but 
> undocumented in the binding) and have the full "pic-no-reset" behavior 
> there as well (and documented in the binding).
> 
> If this has no chance of acceptance (?), then I will just re-submit the 
> binding and implementation with "protected-sources" and the limited form 
> of "pic-no-reset".
> 

Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Meador Inge]

On 02/07/2011 03:45 PM, Benjamin Herrenschmidt wrote:
>
>> In my previous reply I said that "it is not so much as a need as it is a
>> potential simplification."  After further reflection, I don't think that
>> is completely true.  As we get into AMP systems with higher core counts,
>> then implementing this functionality using the existing
>> "protected-sources" implementation versus the new "pic-no-reset" work is
>> going to be harder to maintain.
>
> I'm not arguing that your approach isn't more suitable for AMP systems,
> I just want to leave the existing protected-sources mechanism alone. I'm
> not opposing adding a new, better, mechanism for newer platforms.

Is the mechanism mentioned earlier of having "protected-sources" as a 
synonym for "pic-no-reset" not suitable?  Or would you like the current 
protected sources implementation left completely intact?

> However, I'd name it differently. "pic-no-reset" doesn't carry enough
> meaning in that case. What we want to point out here is that the PIC
> has been pre-initialized.
>
> Another option, which may be cleaner, is to stick to "no-reset" (no need
> for pic- prefix) and make it do just that (prevent the reset), and then

It originally was "no-reset", but that was considered too broad. [1] :)

> use a positive variant of "protected-sources", call it
> "allowed-sources". Maybe even make it a series of ranges. Then have the
> MPIC only access these.

That would work, but I still don't like having to mention this 
information twice in the device tree.  All the sources encoded in the 
various "interrupts" properties _are_ the allowed sources, right?

> I think this is more robust as it would also prevent "accidental" use of
> the wrong sources (bad device-tree, drivers that let you muck around
> with irq numbers, etc...).

That would be nice.  All though, it may not be as helpful as it sounds. 
  There is as much of a risk that someone will botch the 
"allowed-sources" property as there is they will botch the "interrupts" 
property.  We could perhaps still preform these checks without the extra 
property: if a source is not mentioned in an interrupts property, then 
it is not allowed.

> Cheers,
> Ben.
>

[1] http://lists.ozlabs.org/pipermail/linuxppc-dev/2011-February/088244.html

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

RE: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'

[Yoder Stuart-B08248]

> -----Original Message-----
> From: devicetree-discuss-
> bounces+stuart.yoder=3Dfreescale.com@lists.ozlabs.org [mailto:devicetree-
> discuss-bounces+stuart.yoder=3Dfreescale.com@lists.ozlabs.org] On Behalf =
Of
> Benjamin Herrenschmidt
> Sent: Monday, February 07, 2011 3:46 PM
> To: Meador Inge
> Cc: Hollis Blanchard; devicetree-discuss@lists.ozlabs.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-
> sources'
>=20
>=20
> > In my previous reply I said that "it is not so much as a need as it is
> > a potential simplification."  After further reflection, I don't think
> > that is completely true.  As we get into AMP systems with higher core
> > counts, then implementing this functionality using the existing
> > "protected-sources" implementation versus the new "pic-no-reset" work
> > is going to be harder to maintain.
>=20
> I'm not arguing that your approach isn't more suitable for AMP systems, I
> just want to leave the existing protected-sources mechanism alone. I'm no=
t
> opposing adding a new, better, mechanism for newer platforms.
>=20
> However, I'd name it differently. "pic-no-reset" doesn't carry enough
> meaning in that case. What we want to point out here is that the PIC has
> been pre-initialized.
>=20
> Another option, which may be cleaner, is to stick to "no-reset" (no need
> for pic- prefix) and make it do just that (prevent the reset), and then u=
se
> a positive variant of "protected-sources", call it "allowed-sources". May=
be
> even make it a series of ranges. Then have the MPIC only access these.
>=20
> I think this is more robust as it would also prevent "accidental" use of
> the wrong sources (bad device-tree, drivers that let you muck around with
> irq numbers, etc...).

What is the use case for "allowed-sources"?   For AMP the only device
nodes in your device tree should be your AMP partition's devices,
thus any interrupt specifiers in your dev tree are "allowed".

The MPIC is a shared device and thus the need for no-reset.

So, all newer platforms should need is "no-reset".

Stuart

[PATCH v3 2/4] 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

[PATCH v3 3/4] 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.

The presence of "protected-sources" is checked for the backwards
compatibility of systems which are already using "protected-sources"
in firmware and the firmware can not be updated.  Any sources encoded in the
"protected-sources" cells are not processed.  "protected-sources" is
effectively a synonym for "pic-no-reset".

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      |   64 +++++++++++++++++++++++++++++++-------
 2 files changed, 55 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..8de47ca 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 */
@@ -1029,6 +1055,16 @@ static struct irq_host_ops mpic_host_ops = {
 	.xlate = mpic_host_xlate,
 };
 
+static int mpic_reset_prohibited(struct device_node *node)
+{
+	/* The presence of "protected-sources" is checked for the backwards
+	 * compatibility of systems which are already using "protected-sources"
+	 * in firmware and the firmware can not be updated.
+	 */
+	return node && (of_get_property(node, "pic-no-reset", NULL)
+		|| of_get_property(node, "protected-sources", NULL));
+}
+
 /*
  * Exported functions
  */
@@ -1129,7 +1165,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_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 +1291,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 +1334,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

[PATCH v3 4/4] powerpc: Replacing "protected-sources" with "pic-no-reset" in DTS files

[Meador Inge]

The "protected-sources" property was being used in the AMP configured
MPC8572DS and P2020RDB DTS files.  This changeset modifies those files
to use "pic-no-reset" instead.

Signed-off-by: Meador Inge <meador_inge@mentor.com>
Cc: Hollis Blanchard <hollis_blanchard@mentor.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts |    6 +-----
 arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts |   11 +----------
 arch/powerpc/boot/dts/p2020rdb_camp_core0.dts  |    7 +------
 arch/powerpc/boot/dts/p2020rdb_camp_core1.dts  |    7 +------
 4 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
index 3375c2a..4f7ee6d 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
@@ -252,11 +252,7 @@
 			reg = <0x40000 0x40000>;
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
-			protected-sources = <
-			31 32 33 37 38 39       /* enet2 enet3 */
-			76 77 78 79 26 42	/* dma2 pci2 serial*/
-			0xe4 0xe5 0xe6 0xe7	/* msi */
-			>;
+			pic-no-reset;
 		};
 	};
 
diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
index e7b477f..62fd64f 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
@@ -178,16 +178,7 @@
 			reg = <0x40000 0x40000>;
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
-			protected-sources = <
-			18 16 10 42 45 58	/* MEM L2 mdio serial crypto */
-			29 30 34 35 36 40	/* enet0 enet1 */
-			24 25 20 21 22 23	/* pci0 pci1 dma1 */
-			43			/* i2c */
-			0x1 0x2 0x3 0x4         /* pci slot */
-			0x9 0xa 0xb 0xc         /* usb */
-			0x6 0x7 0xe 0x5         /* Audio elgacy SATA */
-			0xe0 0xe1 0xe2 0xe3	/* msi */
-			>;
+			pic-no-reset;
 		};
 	};
 
diff --git a/arch/powerpc/boot/dts/p2020rdb_camp_core0.dts b/arch/powerpc/boot/dts/p2020rdb_camp_core0.dts
index 0fe93d0..f749cbb 100644
--- a/arch/powerpc/boot/dts/p2020rdb_camp_core0.dts
+++ b/arch/powerpc/boot/dts/p2020rdb_camp_core0.dts
@@ -318,12 +318,7 @@
 			reg = <0x40000 0x40000>;
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
-			protected-sources = <
-			42 76 77 78 79 /* serial1 , dma2 */
-			29 30 34 26 /* enet0, pci1 */
-			0xe0 0xe1 0xe2 0xe3 /* msi */
-			0xe4 0xe5 0xe6 0xe7
-			>;
+			pic-no-reset;
 		};
 
 		global-utilities@e0000 {
diff --git a/arch/powerpc/boot/dts/p2020rdb_camp_core1.dts b/arch/powerpc/boot/dts/p2020rdb_camp_core1.dts
index e95a512..6ef2e64 100644
--- a/arch/powerpc/boot/dts/p2020rdb_camp_core1.dts
+++ b/arch/powerpc/boot/dts/p2020rdb_camp_core1.dts
@@ -129,12 +129,7 @@
 			reg = <0x40000 0x40000>;
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
-			protected-sources = <
-			17 18 43 42 59 47 /*ecm, mem, i2c, serial0, spi,gpio */
-			16 20 21 22 23 28 	/* L2, dma1, USB */
-			03 35 36 40 31 32 33 	/* mdio, enet1, enet2 */
-			72 45 58 25 		/* sdhci, crypto , pci */
-			>;
+			pic-no-reset;
 		};
 
 		msi@41600 {
-- 
1.6.3.3

Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Meador Inge]

 From the feedback I have received so far, the fundamental ideas in this 
patch set are sane.  However, the following issues are still outstanding:

     1. What is the name of the no reset property?
        "pic-no-reset" or "no-reset"?
     2. Should we just keep the existing protected sources implementation
        in place?

For (1), I am fine with either.  For (2), I still think that we can make 
"pic-no-reset" a synonym for "protected-sources" and that things will 
work out.

On 02/10/2011 02:42 PM, Meador Inge wrote:
> ---------- Forwarded message ----------
> From: Meador Inge<meador_inge@mentor.com>
> Date: Fri, Feb 4, 2011 at 5:25 PM
> Subject: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
> To: linuxppc-dev@lists.ozlabs.org
> Cc: devicetree-discuss@lists.ozlabs.org, Hollis Blanchard<
> hollis_blanchard@mentor.com>
>
>
> This patch set provides a binding for Open PIC and implements support for
> a new property, specified by that binding, called "pic-no-reset".  With
> "pic-no-reset" in place the "protected-sources" property is no longer needed
> and its full implementation was removed.  "protected-sources" is still
> checked
> for, however, for legacy purposes.
>
> For v3 of this patch 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.
>

 From the feedback I have received so far, the fundamental ideas in this 
patch set are sane.  However, the following issues still need agreement:

     1. What should be the name of the no reset property?
        "pic-no-reset" or "no-reset"?
     2. Should we just keep the existing protected sources implementation
        in place?

For (1), I prefer "no-reset".  For (2), I still think that we can make 
"no-reset" a synonym for "protected-sources" and that things will work out.

Ben, you said that you would really like to leave the protected sources 
implementation alone.  Is the mechanism implemented in "PATCH v3 3/4" 
[1] of having "protected-sources" as a synonym for "pic-no-reset" not 
suitable?

[1] http://lists.ozlabs.org/pipermail/linuxppc-dev/2011-February/088262.html

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

Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Meador Inge]

Apologies for the bad post. =A0Bad day for email ... =A0Please ignore the
top reply in my previous reply. =A0The full reply is the below the
quote.

On Thu, Feb 10, 2011 at 8:01 PM, Meador Inge <meador_inge@mentor.com> wrote=
:
>
> On 02/10/2011 02:42 PM, Meador Inge wrote:
>>
>> ---------- Forwarded message ----------
>> From: Meador Inge<meador_inge@mentor.com>
>> Date: Fri, Feb 4, 2011 at 5:25 PM
>> Subject: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
>> To: linuxppc-dev@lists.ozlabs.org
>> Cc: devicetree-discuss@lists.ozlabs.org, Hollis Blanchard<
>> hollis_blanchard@mentor.com>
>>
>>
>> This patch set provides a binding for Open PIC and implements support fo=
r
>> a new property, specified by that binding, called "pic-no-reset". =A0Wit=
h
>> "pic-no-reset" in place the "protected-sources" property is no longer ne=
eded
>> and its full implementation was removed. =A0"protected-sources" is still
>> checked
>> for, however, for legacy purposes.
>>
>> For v3 of this patch the Open PIC binding was changed to be more consist=
ent
>> with existing bindings, several DTS files were cleaned up, "no-reset" wa=
s
>> changed to "pic-no-reset", and a check to treat "protected-sources" as a
>> synonym for "pic-no-reset" was added.
>>

>From the feedback I have received so far, the fundamental ideas in
this patch set are sane. =A0However, the following issues
still need agreement:

 =A0 =A01. What should be the name of the no reset property?
 =A0 =A0 =A0 "pic-no-reset" or "no-reset"?
 =A0 =A02. Should we just keep the existing protected sources implementatio=
n
 =A0 =A0 =A0 in place?

For (1), I prefer "no-reset". =A0For (2), I still think that we can make
"no-reset" a synonym for "protected-sources" and that things will work
out.

Ben, you said that you would really like to leave the protected
sources implementation alone. =A0Is the mechanism implemented in "PATCH
v3 3/4" [1] of having "protected-sources" as a synonym for
"pic-no-reset" not suitable?

[1] http://lists.ozlabs.org/pipermail/linuxppc-dev/2011-February/088262.htm=
l

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

RE: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Yoder Stuart-B08248]

> -----Original Message-----
> From: Meador Inge [mailto:meadori@gmail.com]
> Sent: Thursday, February 10, 2011 9:26 PM
> To: Benjamin Herrenschmidt
> Cc: Yoder Stuart-B08248; devicetree-discuss@lists.ozlabs.org; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
>=20
> Apologies for the bad post. =A0Bad day for email ... =A0Please ignore the=
 top
> reply in my previous reply. =A0The full reply is the below the quote.
>=20
> On Thu, Feb 10, 2011 at 8:01 PM, Meador Inge <meador_inge@mentor.com>
> wrote:
> >
> > On 02/10/2011 02:42 PM, Meador Inge wrote:
> >>
> >> ---------- Forwarded message ----------
> >> From: Meador Inge<meador_inge@mentor.com>
> >> Date: Fri, Feb 4, 2011 at 5:25 PM
> >> Subject: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
> >> To: linuxppc-dev@lists.ozlabs.org
> >> Cc: devicetree-discuss@lists.ozlabs.org, Hollis Blanchard<
> >> hollis_blanchard@mentor.com>
> >>
> >>
> >> This patch set provides a binding for Open PIC and implements support
> >> for a new property, specified by that binding, called "pic-no-reset".
> >> With "pic-no-reset" in place the "protected-sources" property is no
> >> longer needed and its full implementation was removed.
> >> "protected-sources" is still checked for, however, for legacy
> >> purposes.
> >>
> >> For v3 of this patch 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.
> >>
>=20
> From the feedback I have received so far, the fundamental ideas in this
> patch set are sane. =A0However, the following issues still need agreement=
:
>=20
>  =A0 =A01. What should be the name of the no reset property?
>  =A0 =A0 =A0 "pic-no-reset" or "no-reset"?
>  =A0 =A02. Should we just keep the existing protected sources implementat=
ion
>  =A0 =A0 =A0 in place?
>=20
> For (1), I prefer "no-reset".

I also prefer plain "no-reset".  The property is on a pic node so
"pic" on the property seems redundant.

> For (2), I still think that we can make "no-
> reset" a synonym for "protected-sources" and that things will work out.
>=20
> Ben, you said that you would really like to leave the protected sources
> implementation alone. =A0Is the mechanism implemented in "PATCH
> v3 3/4" [1] of having "protected-sources" as a synonym for "pic-no-reset"
> not suitable?

I thought what Ben was getting at was that there is existing
firmware that may provide a device tree with protected-sources,
and thus we should continue supporting it for backwards
compatibility.

So, I would say add "no-reset" as the preferred mechanism
going forward, but keep "protected-sources" for backwards
compatibility.

Stuart

Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Meador Inge]

On 02/11/2011 08:58 AM, Yoder Stuart-B08248 wrote:
>
>
>> -----Original Message-----
>> From: Meador Inge [mailto:meadori@gmail.com]
>> Sent: Thursday, February 10, 2011 9:26 PM
>> To: Benjamin Herrenschmidt
>> Cc: Yoder Stuart-B08248; devicetree-discuss@lists.ozlabs.org; linuxppc-
>> dev@lists.ozlabs.org
>> Subject: Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
>>
>> Apologies for the bad post.  Bad day for email ...  Please ignore the top
>> reply in my previous reply.  The full reply is the below the quote.
>>
>> On Thu, Feb 10, 2011 at 8:01 PM, Meador Inge<meador_inge@mentor.com>
>> wrote:
>>>
>>> On 02/10/2011 02:42 PM, Meador Inge wrote:
>>>>
>>>> ---------- Forwarded message ----------
>>>> From: Meador Inge<meador_inge@mentor.com>
>>>> Date: Fri, Feb 4, 2011 at 5:25 PM
>>>> Subject: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
>>>> To: linuxppc-dev@lists.ozlabs.org
>>>> Cc: devicetree-discuss@lists.ozlabs.org, Hollis Blanchard<
>>>> hollis_blanchard@mentor.com>
>>>>
>>>>
>>>> This patch set provides a binding for Open PIC and implements support
>>>> for a new property, specified by that binding, called "pic-no-reset".
>>>> With "pic-no-reset" in place the "protected-sources" property is no
>>>> longer needed and its full implementation was removed.
>>>> "protected-sources" is still checked for, however, for legacy
>>>> purposes.
>>>>
>>>> For v3 of this patch 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.
>>>>
>>
>>  From the feedback I have received so far, the fundamental ideas in this
>> patch set are sane.  However, the following issues still need agreement:
>>
>>      1. What should be the name of the no reset property?
>>         "pic-no-reset" or "no-reset"?
>>      2. Should we just keep the existing protected sources implementation
>>         in place?
>>
>> For (1), I prefer "no-reset".
>
> I also prefer plain "no-reset".  The property is on a pic node so
> "pic" on the property seems redundant.
>
>> For (2), I still think that we can make "no-
>> reset" a synonym for "protected-sources" and that things will work out.
>>
>> Ben, you said that you would really like to leave the protected sources
>> implementation alone.  Is the mechanism implemented in "PATCH
>> v3 3/4" [1] of having "protected-sources" as a synonym for "pic-no-reset"
>> not suitable?
>
> I thought what Ben was getting at was that there is existing
> firmware that may provide a device tree with protected-sources,
> and thus we should continue supporting it for backwards
> compatibility.

Yup, Arnd pointed that out as well.  That is why in "PATCH v3 3/4" I 
added a check for "protected-sources".  If it is found, then it is 
treated exactly the same way as "no-reset", which should give equivalent 
behavior.

For example, say we have 100 sources and the sources [1, 50] are the 
only ones actually mentioned in the device tree.  Also assume we set 
"protected-sources = <51 52 53>".

Then, with the protected sources model sources [1, 50] and [54, 100] 
would have there VECPRI/cpu binding initialization.  Where as in the 
enhanced "no-reset" model, only sources [1, 50] would have the 
initialization done.

So unless there is some problem with not initializing the remaining 
sources, e.g. sources [51, 100] in the previous example, then the 
expanded "no-reset" should offer equivalent behavior to "protected-sources".

> So, I would say add "no-reset" as the preferred mechanism
> going forward, but keep "protected-sources" for backwards
> compatibility.
>
> Stuart
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss


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

Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Scott Wood]

On Fri, 11 Feb 2011 14:58:13 +0000
Yoder Stuart-B08248 <B08248@freescale.com> wrote:

>=20
>=20
> > -----Original Message-----
> > From: Meador Inge [mailto:meadori@gmail.com]
> > Sent: Thursday, February 10, 2011 9:26 PM
> > To: Benjamin Herrenschmidt
> > Cc: Yoder Stuart-B08248; devicetree-discuss@lists.ozlabs.org; linuxppc-
> > dev@lists.ozlabs.org
> > Subject: Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"
> >=20
> > From the feedback I have received so far, the fundamental ideas in this
> > patch set are sane. =C2=A0However, the following issues still need agre=
ement:
> >=20
> >  =C2=A0 =C2=A01. What should be the name of the no reset property?
> >  =C2=A0 =C2=A0 =C2=A0 "pic-no-reset" or "no-reset"?
> >  =C2=A0 =C2=A02. Should we just keep the existing protected sources imp=
lementation
> >  =C2=A0 =C2=A0 =C2=A0 in place?
> >=20
> > For (1), I prefer "no-reset".
>=20
> I also prefer plain "no-reset".  The property is on a pic node so
> "pic" on the property seems redundant.

It's not redundant, it's namespacing.  Before there was a generic "status"
property, someone who wanted a device-specific "status" could have made
the same argument.  Usually we use a vendor prefix to avoid that problem,
but that won't work here.

-Scott

Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-reset"

[Grant Likely]

On Fri, Feb 11, 2011 at 11:41 AM, Scott Wood <scottwood@freescale.com> wrot=
e:
> On Fri, 11 Feb 2011 14:58:13 +0000
> Yoder Stuart-B08248 <B08248@freescale.com> wrote:
>
>>
>>
>> > -----Original Message-----
>> > From: Meador Inge [mailto:meadori@gmail.com]
>> > Sent: Thursday, February 10, 2011 9:26 PM
>> > To: Benjamin Herrenschmidt
>> > Cc: Yoder Stuart-B08248; devicetree-discuss@lists.ozlabs.org; linuxppc=
-
>> > dev@lists.ozlabs.org
>> > Subject: Re: [PATCH v3 0/4] powerpc: Open PIC binding and "pic-no-rese=
t"
>> >
>> > From the feedback I have received so far, the fundamental ideas in thi=
s
>> > patch set are sane. =A0However, the following issues still need agreem=
ent:
>> >
>> > =A0=A0 =A01. What should be the name of the no reset property?
>> > =A0=A0 =A0 =A0 "pic-no-reset" or "no-reset"?
>> > =A0=A0 =A02. Should we just keep the existing protected sources implem=
entation
>> > =A0=A0 =A0 =A0 in place?
>> >
>> > For (1), I prefer "no-reset".
>>
>> I also prefer plain "no-reset". =A0The property is on a pic node so
>> "pic" on the property seems redundant.
>
> It's not redundant, it's namespacing. =A0Before there was a generic "stat=
us"
> property, someone who wanted a device-specific "status" could have made
> the same argument. =A0Usually we use a vendor prefix to avoid that proble=
m,
> but that won't work here.

Yes, it is a namespace issue.  Please keep the 'pic-' or some other
prefix to reduce the likelyhood of a global namespace clash.
'no-reset' is vanilla enough that it is conceivable it could be
defined as part of a common binding sometime in the future.

g.