Marvell MV64360 interrupt question

Marvell MV64360 interrupt question

[Walter L. Wimer III]

Hi All,

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)));
                    }
                }
                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?

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?


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?



Thanks!!!

Walt

Re: Marvell MV64360 interrupt question

[Walter L. Wimer III]

Thinking about this some more, I think the cascade mechanism I suggested
earlier is backwards.  Architecturally, I think it makes more sense to
define a platform-specific function like boardXYZ_get_irq() in
boardXYZ_setup.c that knows all of the idiosyncrasies of the board and
calls whatever standard PIC libraries it needs in order to do the right
thing.  Then we set ppc_md.get_irq = boardXYZ_get_irq;

I looked around, and lo and behold, the Radstone PPC7D already "stole"
my idea.  :-)  :-)

    static int ppc7d_get_irq(struct pt_regs *regs)
    {
        int irq;

        irq = mv64360_get_irq(regs);
        if (irq == (mv64360_irq_base + MV64x60_IRQ_GPP28))
            irq = i8259_irq(regs);
        return irq;
    }


Any comments on the "extra" mv64x60_read() call in mv64360_get_irq() are
still welcome.



Thanks!

Walt



On Fri, 2005-09-09 at 13:31 -0400, Walter L. Wimer III wrote:
> 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?
> 
> 
> 
> Thanks!!!
> 
> Walt

Re: Marvell MV64360 interrupt question

[Brian Waite]

On Friday 09 September 2005 3:09 pm, Walter L. Wimer III wrote:
> Thinking about this some more, I think the cascade mechanism I suggested
> earlier is backwards.  Architecturally, I think it makes more sense to
> define a platform-specific function like boardXYZ_get_irq() in
> boardXYZ_setup.c that knows all of the idiosyncrasies of the board and
> calls whatever standard PIC libraries it needs in order to do the right
> thing.  Then we set ppc_md.get_irq = boardXYZ_get_irq;
>
> I looked around, and lo and behold, the Radstone PPC7D already "stole"
> my idea.  :-)  :-)
>
>
Yes, the syslib case in the general case. You can easily override the 
get_irq() routine for your own platform to match a priority scheme you 
desire. 

Thanks
Brian

Re: 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

Re: Marvell MV64360 interrupt question

[Walter L. Wimer III]

Thanks for your response, Dale!


Walt



On Fri, 2005-09-09 at 12:27 -0700, Dale Farnsworth wrote:
> On Fri, Sep 09, 2005 at 05:31:32PM +0000, Walter L. Wimer III wrote:
> >                         mv64x60_write(&bh,
> >                                       MV64x60_GPP_INTR_CAUSE,
> >                                       (1 << (irq - 64)));
> 
> You dropped a ~ on the above line.  It confused me for a bit.
> 

Ooops!  Sorry about that!!!  Yikes!


> > 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.

I wondered about that, too....

> 
> -Dale
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: Marvell MV64360 interrupt question

[Walter L. Wimer III]

On Fri, 2005-09-09 at 12:27 -0700, Dale Farnsworth wrote:
> 
> 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.

Ah ha.  mv64x60.h also defines an mv64x60_modify() function that isn't
intrinsically atomic, so it needs the spinlock.  That in turn requires
mv64x60_read() and mv64x60_write() to play along too.

At least in the general case....



Walt

Re: Marvell MV64360 interrupt question

[Dale Farnsworth]

On Fri, Sep 09, 2005 at 08:20:20PM +0000, Walter L. Wimer III wrote:
> On Fri, 2005-09-09 at 12:27 -0700, Dale Farnsworth wrote:
> > 
> > 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.
> 
> Ah ha.  mv64x60.h also defines an mv64x60_modify() function that isn't
> intrinsically atomic, so it needs the spinlock.  That in turn requires
> mv64x60_read() and mv64x60_write() to play along too.

Yes, the lock is needed for mv64x60_modify(), mv64x60_write().  I still
don't think it's needed for mv64x60_read().

-Dale

Re: Marvell MV64360 interrupt question

[Mark A. Greer]

On Fri, Sep 09, 2005 at 01:49:55PM -0700, Dale Farnsworth wrote:
> On Fri, Sep 09, 2005 at 08:20:20PM +0000, Walter L. Wimer III wrote:
> > On Fri, 2005-09-09 at 12:27 -0700, Dale Farnsworth wrote:
> > > 
> > > 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.
> > 
> > Ah ha.  mv64x60.h also defines an mv64x60_modify() function that isn't
> > intrinsically atomic, so it needs the spinlock.  That in turn requires
> > mv64x60_read() and mv64x60_write() to play along too.
> 
> Yes, the lock is needed for mv64x60_modify(), mv64x60_write().  I still
> don't think it's needed for mv64x60_read().

IMHO, the mv64x60_read/write/modify routines should be deleted and the
locking + I/O done explicitly.  That makes it more obvious, more
efficient in places where there are multiple writes, etc. in a row (not
as much grabbing/releasing of the spinlock), and is the preferred way to do
things in linux.

A few times now, I almost started to do that...looked at it and went off
to do something else.  :)  Yes, I'm lazy.  Yes, I should do it.
Eventually, I will (however, if you want to, I won't complain ;).

Mark

Re: Marvell MV64360 interrupt question

[Brian Waite]

On Friday 09 September 2005 5:18 pm, Mark A. Greer wrote:
> On Fri, Sep 09, 2005 at 01:49:55PM -0700, Dale Farnsworth wrote:
> > On Fri, Sep 09, 2005 at 08:20:20PM +0000, Walter L. Wimer III wrote:
> > > On Fri, 2005-09-09 at 12:27 -0700, Dale Farnsworth wrote:
> > > > 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.
Unless somthing has changed, on the get_irq case, the interrupts are only 
handled by CPU0. At least that is the way all the platforms have handled it 
in the past. I'd be wary of distributing across other cpus until you really 
look at the enet and mpsc drivers. I don't know of any races off the top of 
my head, but this code was tested and developed with boards only handling 
IRQs on one processor.
> > >
> > > Ah ha.  mv64x60.h also defines an mv64x60_modify() function that isn't
> > > intrinsically atomic, so it needs the spinlock.  That in turn requires
> > > mv64x60_read() and mv64x60_write() to play along too.

> >
> > Yes, the lock is needed for mv64x60_modify(), mv64x60_write().  I still
> > don't think it's needed for mv64x60_read().
>
> IMHO, the mv64x60_read/write/modify routines should be deleted and the
> locking + I/O done explicitly.  That makes it more obvious, more
> efficient in places where there are multiple writes, etc. in a row (not
> as much grabbing/releasing of the spinlock), and is the preferred way to do
> things in linux.
mv64360_read() need not be locked. The data is intrinsicly stale by the time 
the read has completed. The action on the Marvell to write the register is 
atomic, so you will not get partial reads, thus the data is only accurate at 
the instant it is read. By the time code sees it, the data, is not accurate.

Mark is right, locking must be abstracted. The basic r/w functions should do 
locking whatsoever, it is a performance hit on SMP and has no benefit. 
mv64360_modify() is right out. It should never have been included (no insult 
intended Mark), because it appears to have the same behavior (ie atomicity) 
as the mv64360_{read,write}() but it does not. I say the the IO be locked and 
bit twiddled explicity by the user. 

Just my 2 cents. 

Thanks
Brian 
 

>
> A few times now, I almost started to do that...looked at it and went off
> to do something else.  :)  Yes, I'm lazy.  Yes, I should do it.
> Eventually, I will (however, if you want to, I won't complain ;).
>
> Mark
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded