Message ID | 1296697900-14004-2-git-send-email-meador_inge@mentor.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Thursday 03 February 2011, Meador Inge wrote: > In a recent discussion [1, 2] concerning device trees for AMP systems, the > question of whether we really need 'protected-sources' arose. The general > consensus was that if you don't want a source to be used, then it should *not* > be mentioned in an 'interrupts' property. If a source really needs to be > mentioned, then it should be put in a property other than 'interrupts' with > a specific binding for that use case. > > [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html > [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html That doesn't work in the case that this code was written for: http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg01394.html The problem is that you don't want the mpic to initialize the interrupt line to the default, but instead leave it at whatever the boot firmware has set up. Note that interrupt is not listed in any "interrupts" property of any of the devices on the CPU interpreting the device tree, but it may be mentioned in the device tree that another CPU uses to access the same MPIC. Arnd
On 02/03/2011 09:56 AM, Arnd Bergmann wrote: > On Thursday 03 February 2011, Meador Inge wrote: >> In a recent discussion [1, 2] concerning device trees for AMP systems, the >> question of whether we really need 'protected-sources' arose. The general >> consensus was that if you don't want a source to be used, then it should *not* >> be mentioned in an 'interrupts' property. If a source really needs to be >> mentioned, then it should be put in a property other than 'interrupts' with >> a specific binding for that use case. >> >> [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html >> [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.html > > That doesn't work in the case that this code was written for: > > http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg01394.html > > The problem is that you don't want the mpic to initialize the interrupt > line to the default, but instead leave it at whatever the boot firmware > has set up. Note that interrupt is not listed in any "interrupts" > property of any of the devices on the CPU interpreting the device > tree, but it may be mentioned in the device tree that another CPU > uses to access the same MPIC. > > Arnd We touched on that use case before on list. However, I did a really bad job of explaining things in the above patch description. I understand that the sources that are being protected are mentioned in a device tree other than the one that actually interprets the 'protected-sources' property. The idea is to try and expand the meaning of the 'no-reset' property to cover what 'protected-sources' was taking care of, but without explicitly naming the sources. In the protected sources version of the code, the relevant MPIC initialization went something like (in 'mpic_init'): for (i = 0; i < mpic->num_sources; i++) { /* start with vector = source number, and masked */ u32 vecpri = MPIC_VECPRI_MASK | i | (8 << MPIC_VECPRI_PRIORITY_SHIFT); /* check if protected */ if (mpic->protected && test_bit(i, mpic->protected)) continue; /* init hw */ mpic_irq_write(i, MPIC_INFO(IRQ_VECTOR_PRI), vecpri); mpic_irq_write(i, MPIC_INFO(IRQ_DESTINATION), 1 << cpu); } So unless a particular source was marked as protected, it would get the VECPRI and CPU binding initialization. This is the exact behavior that you describe above, Arnd. In the 'no-reset' model, the initialization looks more like (see PATCH 3 in the set for the full implementation): if (mpic->flags & MPIC_WANTS_RESET) { for (i = 0; i < mpic->num_sources; i++) { mpic_init_vector(mpic, hw); } } So in 'mpic_init' we don't initialize anything and then in 'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with: if (!(mpic->flags & MPIC_WANTS_RESET)) if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw))) mpic_init_vector(mpic, hw); Thus when 'no-reset' is thrown it ensures that only the sources which are mentioned in the device tree are actually initialized. The net effect should be the same as what 'protected-sources' was accomplishing, but without having to maintain the list of sources in the property cell.
On Friday 04 February 2011, Meador Inge wrote: > On 02/03/2011 09:56 AM, Arnd Bergmann wrote: > So in 'mpic_init' we don't initialize anything and then in > 'mpic_host_map' we lazily do the VECPRI and CPU binding initialization with: > > if (!(mpic->flags & MPIC_WANTS_RESET)) > if (!(mpic_is_ipi(mpic, hw) > || mpic_is_timer_interrupt(mpic, hw))) > mpic_init_vector(mpic, hw); > > Thus when 'no-reset' is thrown it ensures that only the sources which > are mentioned in the device tree are actually initialized. The net > effect should be the same as what 'protected-sources' was accomplishing, > but without having to maintain the list of sources in the property cell. That sounds like a good idea, but unfortunately, it's not what SLOF implements on QS21/QS22. It's a legacy product and there won't be any firmware updates. Moreover, it relies on the open firmware implementation and cannot boot with a flattened device tree image, so I don't see how your patch can work on the old systems. Maybe you can treat the presence of a 'protected-sources' property the same way that you treat the no-reset property? Arnd
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 discussion [1, 2] concerning device trees for AMP systems, the question of whether we really need 'protected-sources' arose. The general consensus was that if you don't want a source to be used, then it should *not* be mentioned in an 'interrupts' property. If a source really needs to be mentioned, then it should be put in a property other than 'interrupts' with a specific binding for that use case. [1] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/004038.html [2] http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-January/003991.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(-)