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

login
register
mail settings
Submitter Albert Herranz
Date Nov. 22, 2009, 10:01 p.m.
Message ID <1258927311-4340-12-git-send-email-albert_herranz@yahoo.es>
Download mbox | patch
Permalink /patch/39010/
State Changes Requested
Headers show

Comments

Albert Herranz - Nov. 22, 2009, 10:01 p.m.
Add support for the interrupt controller included in the "Flipper"
chipset of the Nintendo GameCube video game console.
The same interrupt controller is also present in the "Hollywood" chipset
of the Nintendo Wii.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 arch/powerpc/platforms/embedded6xx/Kconfig       |    6 +
 arch/powerpc/platforms/embedded6xx/Makefile      |    1 +
 arch/powerpc/platforms/embedded6xx/flipper-pic.c |  247 ++++++++++++++++++++++
 arch/powerpc/platforms/embedded6xx/flipper-pic.h |   25 +++
 4 files changed, 279 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.c
 create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.h
Grant Likely - Nov. 22, 2009, 11:28 p.m.
On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herranz@yahoo.es> wrote:
> Add support for the interrupt controller included in the "Flipper"
> chipset of the Nintendo GameCube video game console.
> The same interrupt controller is also present in the "Hollywood" chipset
> of the Nintendo Wii.
>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> ---
>  arch/powerpc/platforms/embedded6xx/Kconfig       |    6 +
>  arch/powerpc/platforms/embedded6xx/Makefile      |    1 +
>  arch/powerpc/platforms/embedded6xx/flipper-pic.c |  247 ++++++++++++++++++++++
>  arch/powerpc/platforms/embedded6xx/flipper-pic.h |   25 +++
>  4 files changed, 279 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.c
>  create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.h
>
> diff --git a/arch/powerpc/platforms/embedded6xx/Kconfig b/arch/powerpc/platforms/embedded6xx/Kconfig
> index bfd88be..29a98c6 100644
> --- a/arch/powerpc/platforms/embedded6xx/Kconfig
> +++ b/arch/powerpc/platforms/embedded6xx/Kconfig
> @@ -94,6 +94,7 @@ config MPC10X_STORE_GATHERING
>  config GAMECUBE_COMMON
>        bool
>        select NOT_COHERENT_CACHE
> +       select FLIPPER_PIC
>
>  config USBGECKO_UDBG
>        bool "USB Gecko udbg console for the Nintendo GameCube/Wii"
> @@ -108,3 +109,8 @@ config USBGECKO_UDBG
>
>          If in doubt, say N here.
>
> +config FLIPPER_PIC
> +       bool
> +       depends on GAMECUBE_COMMON
> +       default y

You'll always want this driver when GAMECUBE common is set.  Don't add
another Kconfig entry.

> +
> diff --git a/arch/powerpc/platforms/embedded6xx/Makefile b/arch/powerpc/platforms/embedded6xx/Makefile
> index 0ab7492..35258fd 100644
> --- a/arch/powerpc/platforms/embedded6xx/Makefile
> +++ b/arch/powerpc/platforms/embedded6xx/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_PPC_HOLLY)         += holly.o
>  obj-$(CONFIG_PPC_PRPMC2800)    += prpmc2800.o
>  obj-$(CONFIG_PPC_C2K)          += c2k.o
>  obj-$(CONFIG_USBGECKO_UDBG)    += usbgecko_udbg.o
> +obj-$(CONFIG_FLIPPER_PIC)      += flipper-pic.o

> +unsigned int flipper_pic_get_irq(void)
> +{
> +       void __iomem *io_base = flipper_irq_host->host_data;
> +       int irq;
> +       u32 irq_status;
> +
> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
> +                    in_be32(io_base + FLIPPER_IMR);
> +       if (irq_status == 0)
> +               return -1;      /* no more IRQs pending */

NO_IRQ_IGNORE

> +
> +       __asm__ __volatile__("cntlzw %0,%1" : "=r"(irq) : "r"(irq_status));
> +       return irq_linear_revmap(flipper_irq_host, 31 - irq);
> +}
> +
Albert Herranz - Nov. 23, 2009, 7:59 p.m.
Grant Likely wrote:
> On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herranz@yahoo.es> wrote:
>> Add support for the interrupt controller included in the "Flipper"
>> chipset of the Nintendo GameCube video game console.
>> The same interrupt controller is also present in the "Hollywood" chipset
>> of the Nintendo Wii.
>>
>> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
>> ---
>>  arch/powerpc/platforms/embedded6xx/Kconfig       |    6 +
>>  arch/powerpc/platforms/embedded6xx/Makefile      |    1 +
>>  arch/powerpc/platforms/embedded6xx/flipper-pic.c |  247 ++++++++++++++++++++++
>>  arch/powerpc/platforms/embedded6xx/flipper-pic.h |   25 +++
>>  4 files changed, 279 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.c
>>  create mode 100644 arch/powerpc/platforms/embedded6xx/flipper-pic.h
>>
>> diff --git a/arch/powerpc/platforms/embedded6xx/Kconfig b/arch/powerpc/platforms/embedded6xx/Kconfig
>> index bfd88be..29a98c6 100644
>> --- a/arch/powerpc/platforms/embedded6xx/Kconfig
>> +++ b/arch/powerpc/platforms/embedded6xx/Kconfig
>> @@ -94,6 +94,7 @@ config MPC10X_STORE_GATHERING
>>  config GAMECUBE_COMMON
>>        bool
>>        select NOT_COHERENT_CACHE
>> +       select FLIPPER_PIC
>>
>>  config USBGECKO_UDBG
>>        bool "USB Gecko udbg console for the Nintendo GameCube/Wii"
>> @@ -108,3 +109,8 @@ config USBGECKO_UDBG
>>
>>          If in doubt, say N here.
>>
>> +config FLIPPER_PIC
>> +       bool
>> +       depends on GAMECUBE_COMMON
>> +       default y
> 
> You'll always want this driver when GAMECUBE common is set.  Don't add
> another Kconfig entry.
> 

Ok.

>> +
>> diff --git a/arch/powerpc/platforms/embedded6xx/Makefile b/arch/powerpc/platforms/embedded6xx/Makefile
>> index 0ab7492..35258fd 100644
>> --- a/arch/powerpc/platforms/embedded6xx/Makefile
>> +++ b/arch/powerpc/platforms/embedded6xx/Makefile
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_PPC_HOLLY)         += holly.o
>>  obj-$(CONFIG_PPC_PRPMC2800)    += prpmc2800.o
>>  obj-$(CONFIG_PPC_C2K)          += c2k.o
>>  obj-$(CONFIG_USBGECKO_UDBG)    += usbgecko_udbg.o
>> +obj-$(CONFIG_FLIPPER_PIC)      += flipper-pic.o
> 
>> +unsigned int flipper_pic_get_irq(void)
>> +{
>> +       void __iomem *io_base = flipper_irq_host->host_data;
>> +       int irq;
>> +       u32 irq_status;
>> +
>> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
>> +                    in_be32(io_base + FLIPPER_IMR);
>> +       if (irq_status == 0)
>> +               return -1;      /* no more IRQs pending */
> 
> NO_IRQ_IGNORE
> 

I'll fix that. Thanks.
I did it in the other interrupt controller but forgot about this.

>> +
>> +       __asm__ __volatile__("cntlzw %0,%1" : "=r"(irq) : "r"(irq_status));
>> +       return irq_linear_revmap(flipper_irq_host, 31 - irq);
>> +}
>> +
> 

Thanks.

Cheers,
Albert
Segher Boessenkool - Nov. 24, 2009, 9:30 p.m.
>  config GAMECUBE_COMMON
>  	bool
>  	select NOT_COHERENT_CACHE
> +	select FLIPPER_PIC

Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of GAMECUBE_COMMON
is a good name?

> +#define pr_fmt(fmt) DRV_MODULE_NAME ": " fmt

Unused

> +/*
> + * Each interrupt has a corresponding bit in both
> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
> + *
> + * Enabling/disabling an interrupt line involves asserting/clearing
> + * the corresponding bit in IMR. ACK'ing a request simply involves
> + * asserting the corresponding bit in ICR.
> + */

> +static void flipper_pic_ack(unsigned int virq)
> +{
> +	int irq = virq_to_hw(virq);
> +	void __iomem *io_base = get_irq_chip_data(virq);
> +
> +	set_bit(irq, io_base + FLIPPER_ICR);
> +}

So it should be a simple write instead of an RMW here, right?
As it is you are ACKing _all_ IRQs as far as I can see.

> +unsigned int flipper_pic_get_irq(void)
> +{
> +	void __iomem *io_base = flipper_irq_host->host_data;
> +	int irq;
> +	u32 irq_status;
> +
> +	irq_status = in_be32(io_base + FLIPPER_ICR) &
> +		     in_be32(io_base + FLIPPER_IMR);
> +	if (irq_status == 0)
> +		return -1;	/* no more IRQs pending */
> +
> +	__asm__ __volatile__("cntlzw %0,%1" : "=r"(irq) : "r"(irq_status));
> +	return irq_linear_revmap(flipper_irq_host, 31 - irq);
> +}

There are generic macros for this kind of thing, no need for asm.  ffs()
or something.


Segher
Albert Herranz - Nov. 25, 2009, 5:13 p.m.
Segher Boessenkool wrote:
>>  config GAMECUBE_COMMON
>>      bool
>>      select NOT_COHERENT_CACHE
>> +    select FLIPPER_PIC
> 
> Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of GAMECUBE_COMMON
> is a good name?
> 

I'd prefer to not use a name that implies a specific hardware to describe two (mostly) similar but different hardwares.

>> +#define pr_fmt(fmt) DRV_MODULE_NAME ": " fmt
> 
> Unused
> 

Nope. It is used via the pr_{err,info,...} macros.

>> +/*
>> + * Each interrupt has a corresponding bit in both
>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>> + *
>> + * Enabling/disabling an interrupt line involves asserting/clearing
>> + * the corresponding bit in IMR. ACK'ing a request simply involves
>> + * asserting the corresponding bit in ICR.
>> + */
> 
>> +static void flipper_pic_ack(unsigned int virq)
>> +{
>> +    int irq = virq_to_hw(virq);
>> +    void __iomem *io_base = get_irq_chip_data(virq);
>> +
>> +    set_bit(irq, io_base + FLIPPER_ICR);
>> +}
> 
> So it should be a simple write instead of an RMW here, right?
> As it is you are ACKing _all_ IRQs as far as I can see.
> 

No, it acks just a single IRQ.
But the simple write idea may work. I need to check if writing 0s causes no harm (which IIRC is true).

>> +unsigned int flipper_pic_get_irq(void)
>> +{
>> +    void __iomem *io_base = flipper_irq_host->host_data;
>> +    int irq;
>> +    u32 irq_status;
>> +
>> +    irq_status = in_be32(io_base + FLIPPER_ICR) &
>> +             in_be32(io_base + FLIPPER_IMR);
>> +    if (irq_status == 0)
>> +        return -1;    /* no more IRQs pending */
>> +
>> +    __asm__ __volatile__("cntlzw %0,%1" : "=r"(irq) : "r"(irq_status));
>> +    return irq_linear_revmap(flipper_irq_host, 31 - irq);
>> +}
> 
> There are generic macros for this kind of thing, no need for asm.  ffs()
> or something.
> 

__ffs() matches exactly that. Thanks.

Cheers,
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 5:18 a.m.
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.

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 5:18 a.m.
On Sun, 2009-11-22 at 16:28 -0700, Grant Likely wrote:
> > +unsigned int flipper_pic_get_irq(void)
> > +{
> > +       void __iomem *io_base = flipper_irq_host->host_data;
> > +       int irq;
> > +       u32 irq_status;
> > +
> > +       irq_status = in_be32(io_base + FLIPPER_ICR) &
> > +                    in_be32(io_base + FLIPPER_IMR);
> > +       if (irq_status == 0)
> > +               return -1;      /* no more IRQs pending */
> 
> NO_IRQ_IGNORE 

Why no just 0 ? (aka NO_IRQ)

Or do you know you are getting lots of spurrious that you don't want to
account ?

Cheers,
Ben.
Albert Herranz - Nov. 26, 2009, 3:30 p.m.
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.

> Cheers,
> Ben.
> 

Thanks,
Albert
Albert Herranz - Nov. 26, 2009, 3:33 p.m.
Benjamin Herrenschmidt wrote:
> On Sun, 2009-11-22 at 16:28 -0700, Grant Likely wrote:
>>> +unsigned int flipper_pic_get_irq(void)
>>> +{
>>> +       void __iomem *io_base = flipper_irq_host->host_data;
>>> +       int irq;
>>> +       u32 irq_status;
>>> +
>>> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
>>> +                    in_be32(io_base + FLIPPER_IMR);
>>> +       if (irq_status == 0)
>>> +               return -1;      /* no more IRQs pending */
>> NO_IRQ_IGNORE 
> 
> Why no just 0 ? (aka NO_IRQ)
> 

I didn't know about 0 (I thought that was another valid interrupt).
I was used to -1 to tell that no IRQs were pending (at least from the ARCH=ppc days) :)

> Or do you know you are getting lots of spurrious that you don't want to
> account ?
> 

No, this is not the case here.

> Cheers,
> Ben.
> 

Thanks,
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 9:10 p.m.
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
Benjamin Herrenschmidt - Nov. 26, 2009, 9:12 p.m.
On Thu, 2009-11-26 at 16:33 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 16:28 -0700, Grant Likely wrote:
> >>> +unsigned int flipper_pic_get_irq(void)
> >>> +{
> >>> +       void __iomem *io_base = flipper_irq_host->host_data;
> >>> +       int irq;
> >>> +       u32 irq_status;
> >>> +
> >>> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
> >>> +                    in_be32(io_base + FLIPPER_IMR);
> >>> +       if (irq_status == 0)
> >>> +               return -1;      /* no more IRQs pending */
> >> NO_IRQ_IGNORE 
> > 
> > Why no just 0 ? (aka NO_IRQ)
> > 
> 
> I didn't know about 0 (I thought that was another valid interrupt).

0 is a valid hw number. It's not a valid linux "virq" number. In fact,
that's part of the reason why we generalized the whole virq thingy :-)
Because back then, Linus made the statement that 0 should never be a
valid IRQ number in linux. That also allowed us to make sure that 1..15
are never allocated to anything but a legacy 8259 to avoid all sorts of
problems with crackpot legacy x86 originated drivers who hard coded some
of these interrupts.

> I was used to -1 to tell that no IRQs were pending (at least from the
> ARCH=ppc days) :)

Yup, we changed that :-)

> > Or do you know you are getting lots of spurrious that you don't want to
> > account ?
> > 
> 
> No, this is not the case here.

Cheers,
Ben.

> > Cheers,
> > Ben.
> > 
> 
> Thanks,
> Albert
Segher Boessenkool - Nov. 26, 2009, 9:52 p.m.
>> Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of GAMECUBE_COMMON
>> is a good name?
>
> I'd prefer to not use a name that implies a specific hardware to  
> describe two (mostly) similar but different hardwares.

Hollywood is 100% compatible to Flipper though.

>>> +/*
>>> + * Each interrupt has a corresponding bit in both
>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>> + *
>>> + * Enabling/disabling an interrupt line involves asserting/clearing
>>> + * the corresponding bit in IMR. ACK'ing a request simply involves
>>> + * asserting the corresponding bit in ICR.
>>> + */

I looked it up in YAGCD; it says that _reading_ the ICR reg already
acks all interrupts (and clears the bits), you never write this reg!

>>> +static void flipper_pic_ack(unsigned int virq)
>>> +{
>>> +    int irq = virq_to_hw(virq);
>>> +    void __iomem *io_base = get_irq_chip_data(virq);
>>> +
>>> +    set_bit(irq, io_base + FLIPPER_ICR);
>>> +}
>>
>> So it should be a simple write instead of an RMW here, right?
>> As it is you are ACKing _all_ IRQs as far as I can see.
>>
>
> No, it acks just a single IRQ.

No it doesn't.  Say IRQs 1 and 3 are asserted, so the reg contains 0x0a.
Now you want to ack IRQ1; set_bit() will write 0x0a | 0x02, not just  
0x02.


Segher
Albert Herranz - Nov. 26, 2009, 10:05 p.m.
Segher Boessenkool wrote:
>>> Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of GAMECUBE_COMMON
>>> is a good name?
>>
>> I'd prefer to not use a name that implies a specific hardware to
>> describe two (mostly) similar but different hardwares.
> 
> Hollywood is 100% compatible to Flipper though.
> 

No. There's no ARAM for example :)

>>>> +/*
>>>> + * Each interrupt has a corresponding bit in both
>>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>>> + *
>>>> + * Enabling/disabling an interrupt line involves asserting/clearing
>>>> + * the corresponding bit in IMR. ACK'ing a request simply involves
>>>> + * asserting the corresponding bit in ICR.
>>>> + */
> 
> I looked it up in YAGCD; it says that _reading_ the ICR reg already
> acks all interrupts (and clears the bits), you never write this reg!
> 

YAGCD is not always right. You should not take it as _the truth_.
I'm not telling this is the case. But I don't recall such behavior. It's been a long time since this is done this way.

>>>> +static void flipper_pic_ack(unsigned int virq)
>>>> +{
>>>> +    int irq = virq_to_hw(virq);
>>>> +    void __iomem *io_base = get_irq_chip_data(virq);
>>>> +
>>>> +    set_bit(irq, io_base + FLIPPER_ICR);
>>>> +}
>>>
>>> So it should be a simple write instead of an RMW here, right?
>>> As it is you are ACKing _all_ IRQs as far as I can see.
>>>
>>
>> No, it acks just a single IRQ.
> 
> No it doesn't.  Say IRQs 1 and 3 are asserted, so the reg contains 0x0a.
> Now you want to ack IRQ1; set_bit() will write 0x0a | 0x02, not just 0x02.
> 

Let me rephrase it. No it should just ack a single IRQ :)

So then this is working by accident.
If that's the case, I guess it works because the interrupt is not cleared at the source and the ICR is set again immediately for any pending interrupt?

> 
> Segher
> 
> 

Thanks,
Albert
Segher Boessenkool - Nov. 26, 2009, 11 p.m.
>>> +unsigned int flipper_pic_get_irq(void)
>>> +{
>>> +       void __iomem *io_base = flipper_irq_host->host_data;
>>> +       int irq;
>>> +       u32 irq_status;
>>> +
>>> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
>>> +                    in_be32(io_base + FLIPPER_IMR);
>>> +       if (irq_status == 0)
>>> +               return -1;      /* no more IRQs pending */
>>
>> NO_IRQ_IGNORE
>
> Why no just 0 ? (aka NO_IRQ)
>
> Or do you know you are getting lots of spurrious that you don't  
> want to
> account ?

IRQ #0 is a valid IRQ here (graphics error IIRC), it should be
remapped I suppose?


Segher
Benjamin Herrenschmidt - Nov. 26, 2009, 11:38 p.m.
On Fri, 2009-11-27 at 00:00 +0100, Segher Boessenkool wrote:
> >>> +unsigned int flipper_pic_get_irq(void)
> >>> +{
> >>> +       void __iomem *io_base = flipper_irq_host->host_data;
> >>> +       int irq;
> >>> +       u32 irq_status;
> >>> +
> >>> +       irq_status = in_be32(io_base + FLIPPER_ICR) &
> >>> +                    in_be32(io_base + FLIPPER_IMR);
> >>> +       if (irq_status == 0)
> >>> +               return -1;      /* no more IRQs pending */
> >>
> >> NO_IRQ_IGNORE
> >
> > Why no just 0 ? (aka NO_IRQ)
> >
> > Or do you know you are getting lots of spurrious that you don't  
> > want to
> > account ?
> 
> IRQ #0 is a valid IRQ here (graphics error IIRC), it should be
> remapped I suppose?

All interrupts are remapped. _get_irq() should call the appropriate
revmap function to remap the HW number into a linux number. 0 is never a
valid linux number and means "no interrupt".

In the above case, it would seem to me that what he gets is a bitfield
so 0 means no interrupt. Hence the code should be:

	if (irq_status == 0)
		return 0;
	
Then, find first bit and return the linear revmap... Just look at what
the old pmac_pic does, same stuff basically.

Cheers,
Ben.
Segher Boessenkool - Nov. 27, 2009, 12:06 a.m.
>>>> Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of  
>>>> GAMECUBE_COMMON
>>>> is a good name?
>>>
>>> I'd prefer to not use a name that implies a specific hardware to
>>> describe two (mostly) similar but different hardwares.
>>
>> Hollywood is 100% compatible to Flipper though.
>
> No. There's no ARAM for example :)

ARAM is an external device.

>>>>> +/*
>>>>> + * Each interrupt has a corresponding bit in both
>>>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>>>> + *
>>>>> + * Enabling/disabling an interrupt line involves asserting/ 
>>>>> clearing
>>>>> + * the corresponding bit in IMR. ACK'ing a request simply  
>>>>> involves
>>>>> + * asserting the corresponding bit in ICR.
>>>>> + */
>>
>> I looked it up in YAGCD; it says that _reading_ the ICR reg already
>> acks all interrupts (and clears the bits), you never write this reg!
>
> YAGCD is not always right. You should not take it as _the truth_.

Oh I know.  But I have no better source of information.  Well I could
actually test it, that's very easy using OF :-)  Let's do that.

>>> No, it acks just a single IRQ.
>>
>> No it doesn't.  Say IRQs 1 and 3 are asserted, so the reg contains  
>> 0x0a.
>> Now you want to ack IRQ1; set_bit() will write 0x0a | 0x02, not  
>> just 0x02.
>
> Let me rephrase it. No it should just ack a single IRQ :)
>
> So then this is working by accident.
> If that's the case, I guess it works because the interrupt is not  
> cleared at the source and the ICR is set again immediately for any  
> pending interrupt?

Probably, yes.  Also, you will not often have more than one interrupt
pending anyway (on the legacy PIC).


Segher
Segher Boessenkool - Nov. 27, 2009, 12:17 p.m.
>>>>>> +/*
>>>>>> + * Each interrupt has a corresponding bit in both
>>>>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>>>>> + *
>>>>>> + * Enabling/disabling an interrupt line involves asserting/ 
>>>>>> clearing
>>>>>> + * the corresponding bit in IMR. ACK'ing a request simply  
>>>>>> involves
>>>>>> + * asserting the corresponding bit in ICR.
>>>>>> + */
>>>
>>> I looked it up in YAGCD; it says that _reading_ the ICR reg already
>>> acks all interrupts (and clears the bits), you never write this reg!
>>
>> YAGCD is not always right. You should not take it as _the truth_.
>
> Oh I know.  But I have no better source of information.  Well I could
> actually test it, that's very easy using OF :-)  Let's do that.

I tested it.  It turns out that neither reading or writing this register
does anything; the bits are automatically cleared when the source  
deasserts.

I didn't test all interrupts, some are harder to generate "on demand"  
than
others.


Segher
Albert Herranz - Nov. 27, 2009, 5:27 p.m.
Segher Boessenkool wrote:
>>>>>>> +/*
>>>>>>> + * Each interrupt has a corresponding bit in both
>>>>>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>>>>>> + *
>>>>>>> + * Enabling/disabling an interrupt line involves asserting/clearing
>>>>>>> + * the corresponding bit in IMR. ACK'ing a request simply involves
>>>>>>> + * asserting the corresponding bit in ICR.
>>>>>>> + */
>>>>
>>>> I looked it up in YAGCD; it says that _reading_ the ICR reg already
>>>> acks all interrupts (and clears the bits), you never write this reg!
>>>
>>> YAGCD is not always right. You should not take it as _the truth_.
>>
>> Oh I know.  But I have no better source of information.  Well I could
>> actually test it, that's very easy using OF :-)  Let's do that.
> 
> I tested it.  It turns out that neither reading or writing this register
> does anything; the bits are automatically cleared when the source
> deasserts.
> 

I checked it too on the Nintendo GameCube making the ack a no-op.

It turns out that we _need_ to ack the RSW (Reset Switch) interrupt.
The other interrupt sources checked (EXI, VI, DI, AI, DSP/ARAM) need no explicit ack.

> I didn't test all interrupts, some are harder to generate "on demand" than
> others.
> 

Thanks,
Albert
Benjamin Herrenschmidt - Nov. 27, 2009, 9:34 p.m.
On Fri, 2009-11-27 at 18:27 +0100, Albert Herranz wrote:
> 
> I checked it too on the Nintendo GameCube making the ack a no-op.
> 
> It turns out that we _need_ to ack the RSW (Reset Switch) interrupt.
> The other interrupt sources checked (EXI, VI, DI, AI, DSP/ARAM) need
> no explicit ack.

That would probably mean that the reset switch interrupt is an edge
interrupt. Which means that you also use the wrong flow handler btw :-)
No biggie here tho, if it's acked before the handler is called, which
should be the case, and if masking it doesn't prevent the edge detector
from latching, then it should be allright.

But you may want to make sure you don't have another edge irq somewhere
in there...

Cheers,
Ben.
Segher Boessenkool - Nov. 28, 2009, 2:04 a.m.
>> It turns out that we _need_ to ack the RSW (Reset Switch) interrupt.
>> The other interrupt sources checked (EXI, VI, DI, AI, DSP/ARAM) need
>> no explicit ack.
>
> That would probably mean that the reset switch interrupt is an edge
> interrupt.

Nah, it means that the RSW is a flip-flop.


Segher
Benjamin Herrenschmidt - Nov. 28, 2009, 2:26 a.m.
On Sat, 2009-11-28 at 03:04 +0100, Segher Boessenkool wrote:
> >> It turns out that we _need_ to ack the RSW (Reset Switch)
> interrupt.
> >> The other interrupt sources checked (EXI, VI, DI, AI, DSP/ARAM)
> need
> >> no explicit ack.
> >
> > That would probably mean that the reset switch interrupt is an edge
> > interrupt.
> 
> Nah, it means that the RSW is a flip-flop.

Controlled by the PIC's ISR ? That would be weird. But heh, it's custom
(aka crap) HW so ... :-)

Cheers,
Ben.

Patch

diff --git a/arch/powerpc/platforms/embedded6xx/Kconfig b/arch/powerpc/platforms/embedded6xx/Kconfig
index bfd88be..29a98c6 100644
--- a/arch/powerpc/platforms/embedded6xx/Kconfig
+++ b/arch/powerpc/platforms/embedded6xx/Kconfig
@@ -94,6 +94,7 @@  config MPC10X_STORE_GATHERING
 config GAMECUBE_COMMON
 	bool
 	select NOT_COHERENT_CACHE
+	select FLIPPER_PIC
 
 config USBGECKO_UDBG
 	bool "USB Gecko udbg console for the Nintendo GameCube/Wii"
@@ -108,3 +109,8 @@  config USBGECKO_UDBG
 
 	  If in doubt, say N here.
 
+config FLIPPER_PIC
+	bool
+	depends on GAMECUBE_COMMON
+	default y
+
diff --git a/arch/powerpc/platforms/embedded6xx/Makefile b/arch/powerpc/platforms/embedded6xx/Makefile
index 0ab7492..35258fd 100644
--- a/arch/powerpc/platforms/embedded6xx/Makefile
+++ b/arch/powerpc/platforms/embedded6xx/Makefile
@@ -8,3 +8,4 @@  obj-$(CONFIG_PPC_HOLLY)		+= holly.o
 obj-$(CONFIG_PPC_PRPMC2800)	+= prpmc2800.o
 obj-$(CONFIG_PPC_C2K)		+= c2k.o
 obj-$(CONFIG_USBGECKO_UDBG)	+= usbgecko_udbg.o
+obj-$(CONFIG_FLIPPER_PIC)	+= flipper-pic.o
diff --git a/arch/powerpc/platforms/embedded6xx/flipper-pic.c b/arch/powerpc/platforms/embedded6xx/flipper-pic.c
new file mode 100644
index 0000000..98c2e9f
--- /dev/null
+++ b/arch/powerpc/platforms/embedded6xx/flipper-pic.c
@@ -0,0 +1,247 @@ 
+/*
+ * arch/powerpc/platforms/embedded6xx/flipper-pic.c
+ *
+ * Nintendo GameCube/Wii "Flipper" interrupt controller support.
+ * Copyright (C) 2004-2009 The GameCube Linux Team
+ * Copyright (C) 2007,2008,2009 Albert Herranz
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ */
+#define DRV_MODULE_NAME "flipper-pic"
+#define pr_fmt(fmt) DRV_MODULE_NAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/of.h>
+#include <asm/io.h>
+
+#include "flipper-pic.h"
+
+#define FLIPPER_NR_IRQS		32
+
+/*
+ * Each interrupt has a corresponding bit in both
+ * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
+ *
+ * Enabling/disabling an interrupt line involves asserting/clearing
+ * the corresponding bit in IMR. ACK'ing a request simply involves
+ * asserting the corresponding bit in ICR.
+ */
+#define FLIPPER_ICR		0x00
+#define FLIPPER_ICR_RSS		(1<<16) /* reset switch state */
+
+#define FLIPPER_IMR		0x04
+
+#define FLIPPER_RESET		0x24
+
+
+/*
+ * IRQ chip hooks.
+ *
+ */
+
+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);
+}
+
+static void flipper_pic_ack(unsigned int virq)
+{
+	int irq = virq_to_hw(virq);
+	void __iomem *io_base = get_irq_chip_data(virq);
+
+	set_bit(irq, io_base + FLIPPER_ICR);
+}
+
+static void flipper_pic_mask(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);
+}
+
+static void flipper_pic_unmask(unsigned int virq)
+{
+	int irq = virq_to_hw(virq);
+	void __iomem *io_base = get_irq_chip_data(virq);
+
+	set_bit(irq, io_base + FLIPPER_IMR);
+}
+
+
+static struct irq_chip flipper_pic = {
+	.typename	= "flipper-pic",
+	.ack		= flipper_pic_ack,
+	.mask_ack	= flipper_pic_mask_and_ack,
+	.mask		= flipper_pic_mask,
+	.unmask		= flipper_pic_unmask,
+};
+
+/*
+ * IRQ host hooks.
+ *
+ */
+
+static struct irq_host *flipper_irq_host;
+
+static int flipper_pic_map(struct irq_host *h, unsigned int virq,
+			   irq_hw_number_t hwirq)
+{
+	set_irq_chip_data(virq, h->host_data);
+	get_irq_desc(virq)->status |= IRQ_LEVEL;
+	set_irq_chip_and_handler(virq, &flipper_pic, handle_level_irq);
+	return 0;
+}
+
+static void flipper_pic_unmap(struct irq_host *h, unsigned int irq)
+{
+	set_irq_chip_data(irq, NULL);
+	set_irq_chip(irq, NULL);
+}
+
+static int flipper_pic_match(struct irq_host *h, struct device_node *np)
+{
+	return 1;
+}
+
+
+static struct irq_host_ops flipper_irq_host_ops = {
+	.map = flipper_pic_map,
+	.unmap = flipper_pic_unmap,
+	.match = flipper_pic_match,
+};
+
+/*
+ * Platform hooks.
+ *
+ */
+
+static void __flipper_quiesce(void __iomem *io_base)
+{
+	/* mask and ack all IRQs */
+	out_be32(io_base + FLIPPER_IMR, 0x00000000);
+	out_be32(io_base + FLIPPER_ICR, 0xffffffff);
+}
+
+struct irq_host * __init flipper_pic_init(struct device_node *np)
+{
+	struct irq_host *irq_host;
+	struct resource res;
+	void __iomem *io_base;
+	int retval;
+
+	retval = of_address_to_resource(np, 0, &res);
+	if (retval) {
+		pr_err("no io memory range found\n");
+		return NULL;
+	}
+	io_base = ioremap(res.start, res.end - res.start + 1);
+
+	pr_info("controller at 0x%08x mapped to 0x%p\n", res.start, io_base);
+
+	__flipper_quiesce(io_base);
+
+	irq_host = irq_alloc_host(np, IRQ_HOST_MAP_LINEAR, FLIPPER_NR_IRQS,
+				  &flipper_irq_host_ops, -1);
+	if (!irq_host) {
+		pr_err("failed to allocate irq_host\n");
+		return NULL;
+	}
+
+	irq_host->host_data = io_base;
+
+	return irq_host;
+}
+
+unsigned int flipper_pic_get_irq(void)
+{
+	void __iomem *io_base = flipper_irq_host->host_data;
+	int irq;
+	u32 irq_status;
+
+	irq_status = in_be32(io_base + FLIPPER_ICR) &
+		     in_be32(io_base + FLIPPER_IMR);
+	if (irq_status == 0)
+		return -1;	/* no more IRQs pending */
+
+	__asm__ __volatile__("cntlzw %0,%1" : "=r"(irq) : "r"(irq_status));
+	return irq_linear_revmap(flipper_irq_host, 31 - irq);
+}
+
+/*
+ * Probe function.
+ *
+ */
+
+void __init flipper_pic_probe(void)
+{
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "nintendo,flipper-pic");
+	BUG_ON(!np);
+
+	flipper_irq_host = flipper_pic_init(np);
+	BUG_ON(!flipper_irq_host);
+
+	irq_set_default_host(flipper_irq_host);
+
+	of_node_put(np);
+}
+
+/*
+ * Misc functions related to the flipper chipset.
+ *
+ */
+
+/**
+ * flipper_quiesce() - quiesce flipper irq controller
+ *
+ * Mask and ack all interrupt sources.
+ *
+ */
+void flipper_quiesce(void)
+{
+	void __iomem *io_base = flipper_irq_host->host_data;
+
+	__flipper_quiesce(io_base);
+}
+
+/*
+ * Resets the platform.
+ */
+void flipper_platform_reset(void)
+{
+	void __iomem *io_base;
+
+	if (flipper_irq_host && flipper_irq_host->host_data) {
+		io_base = flipper_irq_host->host_data;
+		out_8(io_base + FLIPPER_RESET, 0x00);
+	}
+}
+
+/*
+ * Returns non-zero if the reset button is pressed.
+ */
+int flipper_is_reset_button_pressed(void)
+{
+	void __iomem *io_base;
+	u32 icr;
+
+	if (flipper_irq_host && flipper_irq_host->host_data) {
+		io_base = flipper_irq_host->host_data;
+		icr = in_be32(io_base + FLIPPER_ICR);
+		return !(icr & FLIPPER_ICR_RSS);
+	}
+	return 0;
+}
+
diff --git a/arch/powerpc/platforms/embedded6xx/flipper-pic.h b/arch/powerpc/platforms/embedded6xx/flipper-pic.h
new file mode 100644
index 0000000..e339186
--- /dev/null
+++ b/arch/powerpc/platforms/embedded6xx/flipper-pic.h
@@ -0,0 +1,25 @@ 
+/*
+ * arch/powerpc/platforms/embedded6xx/flipper-pic.h
+ *
+ * Nintendo GameCube/Wii "Flipper" interrupt controller support.
+ * Copyright (C) 2004-2009 The GameCube Linux Team
+ * Copyright (C) 2007,2008,2009 Albert Herranz
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ */
+
+#ifndef __FLIPPER_PIC_H
+#define __FLIPPER_PIC_H
+
+unsigned int flipper_pic_get_irq(void);
+void __init flipper_pic_probe(void);
+
+void flipper_quiesce(void);
+void flipper_platform_reset(void);
+int flipper_is_reset_button_pressed(void);
+
+#endif