[RFC PATCH 11/19] powerpc: gamecube/wii: flipper interrupt controller support

[Benjamin Herrenschmidt]

On Thu, 2009-11-26 at 16:30 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
> > 
> >> +static void flipper_pic_mask_and_ack(unsigned int virq)
> >> +{
> >> +	int irq = virq_to_hw(virq);
> >> +	void __iomem *io_base = get_irq_chip_data(virq);
> >> +
> >> +	clear_bit(irq, io_base + FLIPPER_IMR);
> >> +	set_bit(irq, io_base + FLIPPER_ICR);
> >> +}
> > 
> > Do not use clear_bit and set_bit on IOs. They will do lwarx/stwcx. which
> > is really not what you want. You can use __clear_bit and __set_bit but
> > it's still fishy. Those operate on unsigned long, so the size vary
> > between 32 and 64 bit etc... not something you care that much about, but
> > it's still the wrong tool for the job.
> > 
> > Do those guys have more than 32 interrupts ? If not, just hand
> > code the msak & shifts. If they do, then maybe stick with __clear_bit()
> > and __set_bit() which are the non atomic variants.
> > 
> 
> There can be only 32 interrupt sources per pic.
> I'll build a mask and check if just a simple write works too (it should IMHO), instead of a RWM.

For the ICR just writes should be ok. For the IMR, I wouldn't be
surprised if you need to RMW. In which case you have two options:

 - One is you just do RMW :-) As long as you don't do SMP, you don't
have a problem since these callbacks are generally called with local
IRQs off.

 - Or you could explicitely add a spinlock_irqsave just out of paranoia
or if you envision SMP type access to these things (from the aux
processor ? But then the spinlock would have to be shared with the aux
ptiocrocessor FW... fun)

 - In any case, clear_bit and set_bit are the wrong accessors since
lwarx and stwcx. are definitely the wrong instructions to use for non
cachable accesses. On other processor you probably would have blown up
here in fact. Also those accessors don't provide any memory barriers
which mean you may even have some ordering issues. Use readl_be() and
writel_be() (or the ppc specific in_be32/out_be32) or the _le variants
if you want to count bits backward :-)

- You could also keep in memory cache of what the mask is supposed to
be. You RMW the cache and do a simple store to the register. It won't
fix the concurrency problem (that you probably don't have anyways) but
it will make things faster by avoiding an MMIO load....

 - However you probably need that MMIO load anyways :-) Trick is, the
store you do to ack and mask an interrupt may take some time to hit the
PIC before the said PIC actually lowers the CPU EE line. Enough time for
your code to go back, re-enable MSR:EE and start calling into the
handlers. Enough time thus to catch a "spurrious" interrupt. Old pmacs
have a similar PIC and have the same problem. I would recommend that
after a mask or a mask & ack, you also read back the register. The MMIO
accessors (readl, etc...) will use a twi;isync construct to ensure that
the load has been fully completed before execution continues which will
help.

 - The construct and flow handler you use assume all interrupts are
level sensitive, this is the case right ?

Cheers,
Ben.

> > Cheers,
> > Ben.
> > 
> 
> Thanks,
> Albert