Marvell MV64360 interrupt question

[Dale Farnsworth]

On Fri, Sep 09, 2005 at 05:31:32PM +0000, Walter L. Wimer III wrote:
> I'm looking at arch/ppc/syslib/mv64360_pic.c in the 2.6.13 kernel and I
> see the following code:
> 
>     int
>     mv64360_get_irq(struct pt_regs *regs)
>     {
>         .
>         .
>         .
>         if (irq == -1) {
>             irq = mv64x60_read(&bh, MV64360_IC_MAIN_CAUSE_HI);
>             irq = __ilog2((irq & 0x1f0003f7) & ppc_cached_irq_mask[1]);
> 
>             if (irq == -1)
>                 irq = -2; /* bogus interrupt, should never happen */
>             else {
>                 if ((irq >= 24) && (irq < MV64x60_IRQ_DOORBELL)) {
>                     irq_gpp = mv64x60_read(&bh, MV64x60_GPP_INTR_CAUSE);
>                     irq_gpp = __ilog2(irq_gpp & ppc_cached_irq_mask[2]);
> 
>                     if (irq_gpp == -1)
>                         irq = -2;
>                     else {
>                         irq = irq_gpp + 64;
>                         mv64x60_write(&bh,
>                                       MV64x60_GPP_INTR_CAUSE,
>                                       (1 << (irq - 64)));

You dropped a ~ on the above line.  It confused me for a bit.

>                     }
>                 }
>                 else
> 		    irq += 32;
>             }
>         }
> 
>         (void)mv64x60_read(&bh, MV64x60_GPP_INTR_CAUSE);
>         .
>         .
>         .
>     }
> 
> Does anybody know what the last mv64x60_read() is intended to do?  It
> *looks* like it's clearing/acknowledging the interrupt, but I don't see
> anything in the Marvell documentation that suggests that *reading* the
> interrupt cause register has such an effect.  From my reading, the
> documentation says that you should write a '0' into the appropriate bit
> position to clear the interrupt.
> 
> Hmmm...  There's an mv64x60_write() a little earlier in the code...  Is
> the "extra" mv64x60_read() here to enforce ordering?  If so, should it
> be moved inside the "if" statement so that it's only executed when
> actually necessary?

Yes.  It's there to make sure that the clearing of the GPP interrupt
is seen by the hardware and takes effect before we return from the function.

> And what about locking?  The mv64x60_xxxx() functions are protected by a
> spinlock, but if we're trying to enforce ordering, shouldn't we hold the
> lock during the entire write+read operation, rather than dropping and
> reacquiring the lock in between?

No additional locking is necessary.  In fact, it seems to me that the 32-bit
register reads and writes are already atomic and all of the locking using
mv64x60_lock is superfluous.

> At the moment, I'm not running into any actual problems here; this just
> caught my eye and I started wondering about it.
> 
> BTW, the reason I'm looking at this code is that the board I'm working
> on has a cascaded interrupt controller (implemented in an FPGA) feeding
> into the MV64360 interrupt controller.  I'm thinking about adding a
> cascade mechanism to mv64360_pic.c similar to the one in the OpenPIC
> code (i.e. like the openpic_hookup_cascade() function).  Any opinions on
> whether this is a good idea or a bad one?

Sounds reasonable to me.

-Dale