RE: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
-Original Message- From: devicetree-discuss- bounces+stuart.yoder=freescale@lists.ozlabs.org [mailto:devicetree- discuss-bounces+stuart.yoder=freescale@lists.ozlabs.org] On Behalf Of Benjamin Herrenschmidt Sent: Monday, February 07, 2011 3:46 PM To: Meador Inge Cc: Hollis Blanchard; devicetree-disc...@lists.ozlabs.org; linuxppc- d...@lists.ozlabs.org Subject: Re: [PATCH v3 1/4] powerpc: Removing support for 'protected- sources' 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...). 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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@4 {
...
// 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@4 {
...
// 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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@4 {
...
// 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@4 {
...
// 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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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_i...@mentor.com
Cc: Hollis Blanchard hollis_blanch...@mentor.com
Cc: Benjamin Herrenschmidt b...@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_bankcpuregs[MPIC_MAX_CPUS];
struct mpic_reg_bankisus[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
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/4] powerpc: Removing support for 'protected-sources'
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
