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
