| Message ID | 1311211654-14326-7-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
Hello. I have been working on SMP support for the MPC8641D processor, so I have also worked on completing the SMP implementation of MPIC. I've been meaning to post a patch, but you beat me to it :) I compared your implementation to mine, and it boils down to the same, except that I had overlooked the problem of IPIs when multiple CPUs are targeted. I did some IPI tests with your code and it works fine generally, but some problems came up. It appears that the first time I trigger an IPI, CPU 0 always receives the IPI even if it wasn't targeted. The problem stops appearing after the first IPI. It turns out that all IDEs are initialized to 0x1, which is forcing CPU 0 to receive the first IPI. IPI related IDEs should therefore be initialized to 0 in mpic_reset. And I also have other comments. Some of them are below, and some others will come in a reply to another patch. Elie On 07/21/2011 03:27 AM, Alexander Graf wrote: > @@ -934,6 +936,17 @@ static uint32_t openpic_cpu_read_internal(void *opaque, target_phys_addr_t addr, > reset_bit(&src->ipvp, IPVP_ACTIVITY); > src->pending = 0; > } > + > + if ((n_IRQ>= opp->irq_ipi0)&& (n_IRQ< (opp->irq_ipi0 + 4))) { I think using (opp->irq_ipi0 + MAX_IPI) would be better? > + src->ide&= ~(1<< idx); > + if (src->ide&& !test_bit(&src->ipvp, IPVP_SENSE)) { > + /* trigger on CPUs that didn't know about it yet */ > + openpic_set_irq(opp, n_IRQ, 1); > + openpic_set_irq(opp, n_IRQ, 0); > + /* if all CPUs knew about it, set active bit again */ > + set_bit(&src->ipvp, IPVP_ACTIVITY); > + } > + } > } > break; > case 0xB0: /* PEOI */ Also, in openpic_cpu_read_internal() , there's is the following code : > #if MAX_IPI > 0 > case 0x40: /* IDE */ > case 0x50: > idx = (addr - 0x40) >> 4; > retval = read_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE); > break; > #endif These are the IPI dispatch registers which are write only, so I suppose this shouldn't be here right?
Hi :) On 22.07.2011, at 16:08, Elie Richa wrote: > Hello. > > I have been working on SMP support for the MPC8641D processor, so I have also worked on completing the SMP implementation of MPIC. I've been meaning to post a patch, but you beat me to it :) Ah, very nice! It's great to see more people interested in this. > I compared your implementation to mine, and it boils down to the same, except that I had overlooked the problem of IPIs when multiple CPUs are targeted. That was the really hard part. Took me 2 days to figure out how to route those properly internally :(. > I did some IPI tests with your code and it works fine generally, but some problems came up. It appears that the first time I trigger an IPI, CPU 0 always receives the IPI even if it wasn't targeted. The problem stops appearing after the first IPI. > > It turns out that all IDEs are initialized to 0x1, which is forcing CPU 0 to receive the first IPI. IPI related IDEs should therefore be initialized to 0 in mpic_reset. Nice catch! You're right - we should just initialize the IPI IDE registers to 0. > > And I also have other comments. Some of them are below, and some others will come in a reply to another patch. > > Elie > > On 07/21/2011 03:27 AM, Alexander Graf wrote: >> @@ -934,6 +936,17 @@ static uint32_t openpic_cpu_read_internal(void *opaque, target_phys_addr_t addr, >> reset_bit(&src->ipvp, IPVP_ACTIVITY); >> src->pending = 0; >> } >> + >> + if ((n_IRQ>= opp->irq_ipi0)&& (n_IRQ< (opp->irq_ipi0 + 4))) { > > I think using (opp->irq_ipi0 + MAX_IPI) would be better? Yes, totally. Must have overlooked this :). > >> + src->ide&= ~(1<< idx); >> + if (src->ide&& !test_bit(&src->ipvp, IPVP_SENSE)) { >> + /* trigger on CPUs that didn't know about it yet */ >> + openpic_set_irq(opp, n_IRQ, 1); >> + openpic_set_irq(opp, n_IRQ, 0); >> + /* if all CPUs knew about it, set active bit again */ >> + set_bit(&src->ipvp, IPVP_ACTIVITY); >> + } >> + } >> } >> break; >> case 0xB0: /* PEOI */ > > Also, in openpic_cpu_read_internal() , there's is the following code : > > > #if MAX_IPI > 0 > > case 0x40: /* IDE */ > > case 0x50: > > idx = (addr - 0x40) >> 4; > > retval = read_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE); > > break; > > #endif > > These are the IPI dispatch registers which are write only, so I suppose this shouldn't be here right? The code was there long before me :). No idea why it is there though - it tries to read out the IDE register for 2 IPIs. Maybe it was read-write in early versions of MPIC? Scott, any idea? Alex
On Fri, 22 Jul 2011 17:01:11 +0200 Alexander Graf <agraf@suse.de> wrote: > On 22.07.2011, at 16:08, Elie Richa wrote: > > > > #if MAX_IPI > 0 > > > case 0x40: /* IDE */ > > > case 0x50: > > > idx = (addr - 0x40) >> 4; > > > retval = read_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE); > > > break; > > > #endif > > > > These are the IPI dispatch registers which are write only, so I suppose this shouldn't be here right? > > The code was there long before me :). No idea why it is there though - it tries to read out the IDE register for 2 IPIs. Maybe it was read-write in early versions of MPIC? Scott, any idea? I don't know about early MPICs, but I don't know how you'd even define that register for read -- it's a command, not state. I suspect it's just a bug -- especially given the initialization to 1, which suggests the author saw it as just another destination register. -Scott
diff --git a/hw/openpic.c b/hw/openpic.c index ad45331..08a3a65 100644 --- a/hw/openpic.c +++ b/hw/openpic.c @@ -57,7 +57,7 @@ #define MAX_MBX 4 #define MAX_TMR 4 #define VECTOR_BITS 8 -#define MAX_IPI 0 +#define MAX_IPI 4 #define VID (0x00000000) @@ -840,7 +840,9 @@ static void openpic_cpu_write_internal(void *opaque, target_phys_addr_t addr, case 0x60: case 0x70: idx = (addr - 0x40) >> 4; - write_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE, val); + /* we use IDE as mask which CPUs to deliver the IPI to still. */ + write_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE, + opp->src[opp->irq_ipi0 + idx].ide | val); openpic_set_irq(opp, opp->irq_ipi0 + idx, 1); openpic_set_irq(opp, opp->irq_ipi0 + idx, 0); break; @@ -934,6 +936,17 @@ static uint32_t openpic_cpu_read_internal(void *opaque, target_phys_addr_t addr, reset_bit(&src->ipvp, IPVP_ACTIVITY); src->pending = 0; } + + if ((n_IRQ >= opp->irq_ipi0) && (n_IRQ < (opp->irq_ipi0 + 4))) { + src->ide &= ~(1 << idx); + if (src->ide && !test_bit(&src->ipvp, IPVP_SENSE)) { + /* trigger on CPUs that didn't know about it yet */ + openpic_set_irq(opp, n_IRQ, 1); + openpic_set_irq(opp, n_IRQ, 0); + /* if all CPUs knew about it, set active bit again */ + set_bit(&src->ipvp, IPVP_ACTIVITY); + } + } } break; case 0xB0: /* PEOI */
The current IPI support in the MPIC code is incomplete and doesn't work. This code adds proper support for IPIs in MPIC by using the IDE register to remember which CPUs IPIs are still outstanding to. New triggers through the IPI trigger register only add to the list of CPUs we want to IPI. Signed-off-by: Alexander Graf <agraf@suse.de> --- hw/openpic.c | 17 +++++++++++++++-- 1 files changed, 15 insertions(+), 2 deletions(-)