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

4 messages, 4 authors, 2011-02-07 · open the first message on its own page

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

From: Benjamin Herrenschmidt <hidden>
Date: 2011-02-06 23:35:36

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.
 
quoted hunk
[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 <redacted>
Cc: Hollis Blanchard <redacted>
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'

From: Meador Inge <hidden>
Date: 2011-02-07 01:32:29

On 02/06/2011 05:35 PM, Benjamin Herrenschmidt wrote:
On Fri, 2011-02-04 at 17:25 -0600, Meador Inge wrote:
quoted
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'

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2011-02-07 01:37:03

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'

From: Meador Inge <hidden>
Date: 2011-02-07 18:02:49

On 02/06/2011 05:35 PM, Benjamin Herrenschmidt wrote:
On Fri, 2011-02-04 at 17:25 -0600, Meador Inge wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help