| Message ID | 1296861941-3370-2-git-send-email-meador_inge@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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.
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".
> 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". >
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
> -----Original Message----- > From: devicetree-discuss- > bounces+stuart.yoder=freescale.com@lists.ozlabs.org [mailto:devicetree- > discuss-bounces+stuart.yoder=freescale.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' > > > > 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
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
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(-)