Patchwork [RFC,04/19] powerpc: wii: device tree

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

Comments

Albert Herranz - Nov. 22, 2009, 10:01 p.m.
Add a device tree source file for the Nintendo Wii video game console.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 arch/powerpc/boot/dts/wii.dts |  244 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 244 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/boot/dts/wii.dts
Grant Likely - Nov. 22, 2009, 11:18 p.m.
On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herranz@yahoo.es> wrote:
> Add a device tree source file for the Nintendo Wii video game console.
>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>

Same comments apply here as for the gamecube.dts file, plus a few more below.

Cheers,
g.

> ---
>  arch/powerpc/boot/dts/wii.dts |  244 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 244 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/boot/dts/wii.dts
>
> diff --git a/arch/powerpc/boot/dts/wii.dts b/arch/powerpc/boot/dts/wii.dts
> new file mode 100644
> index 0000000..a30a804
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/wii.dts
> @@ -0,0 +1,244 @@
> +/*
> + * arch/powerpc/boot/dts/wii.dts
> + *
> + * Nintendo Wii platform device tree source
> + * Copyright (C) 2008-2009 The GameCube Linux Team
> + * Copyright (C) 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.
> + *
> + */
> +
> +/dts-v1/;
> +
> +/memreserve/ 0x01800000 0xe800000;     /* memory hole (includes I/O area) */
> +/memreserve/ 0x10000000 0x0004000;     /* DSP RAM */

This bothers me... see below.

> +
> +/ {
> +       model = "NintendoWii";
> +       compatible = "nintendo,wii";
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       chosen {
> +               /* root filesystem on 2nd partition of SD card */
> +               bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
> +               linux,stdout-path = &USBGECKO0;
> +       };
> +
> +       aliases {
> +               ugecon = &USBGECKO0;
> +               hw_gpio = &gpio1;
> +       };
> +
> +       /*
> +        * The Nintendo Wii has two discontiguous RAM memory areas called
> +        * MEM1 and MEM2.
> +        * MEM1 starts at 0x00000000 and contains 24MB of 1T-SRAM.
> +        * MEM2 starts at 0x10000000 and contains 64MB of DDR2 RAM.
> +        * Between both memory address ranges there is an address space
> +        * where memory-mapped I/O registers are found.
> +        *
> +        * Currently, Linux 32-bit PowerPC does not support RAM in
> +        * discontiguous memory address spaces. Thus, in order to use
> +        * both RAM areas, we declare as RAM the range from the start of
> +        * MEM1 to the end of useable MEM2 and exclude the needed parts
> +        * with /memreserve/ statements, at the expense of wasting a bit
> +        * of memory.

Hmmm.  It's not great practice to lie about hardware in the device
tree.  Better to describe the memory correctly here, and if you have
to work around Linux deficiencies, then do so in the platform support
code (arch/platforms/*).  I won't NAK the patch over it (feel free to
add my Acked-by line) because it doesn't impact other platforms, but
it should be fixed.

> +               i2c-video {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       compatible = "virtual,i2c-gpio";

There isn't a documented binding for this.  Is there a driver for it?

> +
> +                       gpios = <&gpio0  16 0 /* 31-15 */
> +                                &gpio0  17 0 /* 31-14 */
> +                               >;
> +                       sda-is-open-drain = <1>;
> +                       sda-enforce-dir = <1>;
> +                       scl-is-open-drain = <1>;
> +                       scl-is-output-only = <1>;
> +                       udelay = <2>;
> +
> +                       audio-video-encoder {
> +                               compatible = "nintendo,wii-ave-rvl";
> +                               reg = <0x70>;
> +                       };
> +               };
> +       };
> +};
> +
> --
> 1.6.3.3
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
Albert Herranz - Nov. 23, 2009, 7:54 p.m.
Grant Likely wrote:
> On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herranz@yahoo.es> wrote:
>> Add a device tree source file for the Nintendo Wii video game console.
>>
>> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> 
> Same comments apply here as for the gamecube.dts file, plus a few more below.
> 

Ok, I'll try to address them too here.

>> ---
>>  arch/powerpc/boot/dts/wii.dts |  244 +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 244 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/powerpc/boot/dts/wii.dts
>>
>> diff --git a/arch/powerpc/boot/dts/wii.dts b/arch/powerpc/boot/dts/wii.dts
>> new file mode 100644
>> index 0000000..a30a804
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/wii.dts
>> @@ -0,0 +1,244 @@
>> +/*
>> + * arch/powerpc/boot/dts/wii.dts
>> + *
>> + * Nintendo Wii platform device tree source
>> + * Copyright (C) 2008-2009 The GameCube Linux Team
>> + * Copyright (C) 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.
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +
>> +/memreserve/ 0x01800000 0xe800000;     /* memory hole (includes I/O area) */
>> +/memreserve/ 0x10000000 0x0004000;     /* DSP RAM */
> 
> This bothers me... see below.
> 
>> +
>> +/ {
>> +       model = "NintendoWii";
>> +       compatible = "nintendo,wii";
>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +
>> +       chosen {
>> +               /* root filesystem on 2nd partition of SD card */
>> +               bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
>> +               linux,stdout-path = &USBGECKO0;
>> +       };
>> +
>> +       aliases {
>> +               ugecon = &USBGECKO0;
>> +               hw_gpio = &gpio1;
>> +       };
>> +
>> +       /*
>> +        * The Nintendo Wii has two discontiguous RAM memory areas called
>> +        * MEM1 and MEM2.
>> +        * MEM1 starts at 0x00000000 and contains 24MB of 1T-SRAM.
>> +        * MEM2 starts at 0x10000000 and contains 64MB of DDR2 RAM.
>> +        * Between both memory address ranges there is an address space
>> +        * where memory-mapped I/O registers are found.
>> +        *
>> +        * Currently, Linux 32-bit PowerPC does not support RAM in
>> +        * discontiguous memory address spaces. Thus, in order to use
>> +        * both RAM areas, we declare as RAM the range from the start of
>> +        * MEM1 to the end of useable MEM2 and exclude the needed parts
>> +        * with /memreserve/ statements, at the expense of wasting a bit
>> +        * of memory.
> 
> Hmmm.  It's not great practice to lie about hardware in the device
> tree.  Better to describe the memory correctly here, and if you have
> to work around Linux deficiencies, then do so in the platform support
> code (arch/platforms/*).  I won't NAK the patch over it (feel free to
> add my Acked-by line) because it doesn't impact other platforms, but
> it should be fixed.
> 

I'll try to workaround the limitation via a fixups function in the bootwrapper instead.

>> +               i2c-video {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       compatible = "virtual,i2c-gpio";
> 
> There isn't a documented binding for this.  Is there a driver for it?
> 

I have a driver for it. But it isn't yet published.

This is the documentation I wrote so far for the bindings.
Is there a standard for this?

Documentation/powerpc/dts-bindings/gpio/i2c.txt

GPIO-based I2C

Required properties:
- compatible : should be "virtual,i2c-gpio".
- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
- sda-is-open-drain : should be non-zero if SDA gpio is open-drain.
- sda-enforce-dir : should be non-zero if SDA gpio must be configured for
                    input before reading and for output before writing.
- scl-is-open-drain : should be non-zero if SCL gpio is open-drain.
- scl-is-output-only : should be non-zero if SCL is an output gpio only.
- udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz
- timeout : clock stretching timeout in milliseconds.

Example:

gpio0: hollywood-gpio@0d8000c0 {
        compatible = "nintendo,hollywood-gpio";
        reg = <0x0d8000c0 0x20>;
        gpio-controller;
        #gpio-cells = <2>;
 };

i2c-video {
        #address-cells = <1>;
        #size-cells = <0>;
        compatible = "virtual,i2c-gpio";

        gpios = <&gpio0 16 0    /* SDA line */
                 &gpio0 17 0    /* SCL line */
                >;
        sda-is-open-drain = <1>;
        sda-enforce-dir = <1>;
        scl-is-open-drain = <1>;
        scl-is-output-only = <1>;
        udelay = <2>;

        audio-video-encoder {
                compatible = "nintendo,wii-ave-rvl";
                reg = <0x70>;
        };
};

>> +
>> +                       gpios = <&gpio0  16 0 /* 31-15 */
>> +                                &gpio0  17 0 /* 31-14 */
>> +                               >;
>> +                       sda-is-open-drain = <1>;
>> +                       sda-enforce-dir = <1>;
>> +                       scl-is-open-drain = <1>;
>> +                       scl-is-output-only = <1>;
>> +                       udelay = <2>;
>> +
>> +                       audio-video-encoder {
>> +                               compatible = "nintendo,wii-ave-rvl";
>> +                               reg = <0x70>;
>> +                       };
>> +               };
>> +       };
>> +};
>> +

Cheers,
Albert
Grant Likely - Nov. 23, 2009, 8:36 p.m.
On Mon, Nov 23, 2009 at 12:54 PM, Albert Herranz
<albert_herranz@yahoo.es> wrote:
> Grant Likely wrote:
>> On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herranz@yahoo.es> wrote:
>>> +               i2c-video {
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <0>;
>>> +                       compatible = "virtual,i2c-gpio";
>>
>> There isn't a documented binding for this.  Is there a driver for it?
>>
>
> I have a driver for it. But it isn't yet published.
>
> This is the documentation I wrote so far for the bindings.
> Is there a standard for this?

This looks pretty good to me.  The documentation format isn't strict,
just follow the pattern seen in other bindings.  Make sure you post
new bindings to devicetree-discuss@lists.ozlabs.org for review.  A
couple of comments below.

> Documentation/powerpc/dts-bindings/gpio/i2c.txt
>
> GPIO-based I2C
>
> Required properties:
> - compatible : should be "virtual,i2c-gpio".
> - gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> - sda-is-open-drain : should be non-zero if SDA gpio is open-drain.
> - sda-enforce-dir : should be non-zero if SDA gpio must be configured for
>                    input before reading and for output before writing.
> - scl-is-open-drain : should be non-zero if SCL gpio is open-drain.
> - scl-is-output-only : should be non-zero if SCL is an output gpio only.

Instead of looking for a value in these properties, just make them
empty properties and change behaviour based on whether or not the
property is present.

Why is the scl-is-output-only property needed?

> - udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz

You should follow the lead of
Documentation/powerpc/dts-bindings/fsl/i2c.txt here and specify a
clock-frequency property.

> - timeout : clock stretching timeout in milliseconds.

I don't understand what this property means.

g.
Albert Herranz - Nov. 23, 2009, 9:55 p.m.
Grant Likely wrote:
> This looks pretty good to me.  The documentation format isn't strict,
> just follow the pattern seen in other bindings.  Make sure you post
> new bindings to devicetree-discuss@lists.ozlabs.org for review.  A
> couple of comments below.
> 

Ok. I know it now for the next time :)

>> Documentation/powerpc/dts-bindings/gpio/i2c.txt
>>
>> GPIO-based I2C
>>
>> Required properties:
>> - compatible : should be "virtual,i2c-gpio".
>> - gpios : should specify GPIOs used for SDA and SCL lines, in that order.
>> - sda-is-open-drain : should be non-zero if SDA gpio is open-drain.
>> - sda-enforce-dir : should be non-zero if SDA gpio must be configured for
>>                    input before reading and for output before writing.
>> - scl-is-open-drain : should be non-zero if SCL gpio is open-drain.
>> - scl-is-output-only : should be non-zero if SCL is an output gpio only.
> 
> Instead of looking for a value in these properties, just make them
> empty properties and change behaviour based on whether or not the
> property is present.
> 

It seems reasonable. Thanks.

> Why is the scl-is-output-only property needed?
> 

It is needed to specify that the I2C master can't honour clock stretching done by I2C slave devices, as it cannot read back SCL.

>> - udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz
> 
> You should follow the lead of
> Documentation/powerpc/dts-bindings/fsl/i2c.txt here and specify a
> clock-frequency property.
> 

Ok.

>> - timeout : clock stretching timeout in milliseconds.
> 
> I don't understand what this property means.
> 

It is the maximum time that the I2C master should wait for SCL to go high when a I2C slave is "clock-stretching".

Cheers,
Albert
Segher Boessenkool - Nov. 25, 2009, 4:57 p.m.
>>> +               i2c-video {
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <0>;
>>> +                       compatible = "virtual,i2c-gpio";
>>
>> There isn't a documented binding for this.  Is there a driver for it?
>>
>
> I have a driver for it. But it isn't yet published.

I bet there already is a generic IIC-via-GPIOs driver that
you could use.


Segher
Segher Boessenkool - Nov. 25, 2009, 5:49 p.m.
> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */

Like others have said already, don't do this.  If you need
a workaround, put it in the platform code.

> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */

This address is fixed in the DSP hardware, right?  If not,
you shouldn't do a reserve here.

> +	chosen {
> +		/* root filesystem on 2nd partition of SD card */
> +		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";

Question: why do you need/want nobats?

> +	memory {
> +		device_type = "memory";
> +		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
> +		reg = <0x00000000 0x133e0000>;

This should be  <0 0x01800000  0x10000000 0x04000000>

I'm not sure what at the end of MEM2 you're trying to hide,
but if you need to hide anything, do it in the platform code
isntead.

> +	cpus {
> +		#cpus = <1>;

Don't use #cpus

> +	/* devices contained in the hollywood chipset */
> +	soc {

It's not a SoC, more like a bridge chip.  More to the point,
all the devices you put under this node actually sit in the
root domain logically/physically, so why not put them there
instead.  You'll want a node for the generic control registers,
if you do.

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		#interrupt-cells = <1>;
> +		model = "hollywood";

"model" isn't required here, if you don't have anything
good to put in it, just don't use the property.

> +		compatible = "nintendo,hollywood";
> +		clock-frequency = <243000000>; /* 243MHz */

What does this clock freq mean?  Hollywood has lots of
clocks, many of them configurable.  Bus frequency is in
the cpu node already.  The binding doesn't say what this
is either, since you didn't write a binding :-)

Just remove it?

> +		ranges = <0x0c000000 0x0c000000 0x00010000
> +			  0x0d000000 0x0d000000 0x00010000
> +			  0x0d040000 0x0d040000 0x00050000
> +			  0x0d800000 0x0d800000 0x00001000

All of 0cXXXXXX and 0dXXXXXX actually.

> +			  0x133e0000 0x133e0000 0x00c20000>;

This is just part of MEM2, don't treat it special here.

> +		video@0c002000 {
> +			compatible = "nintendo,hollywood-video";
> +			reg = <0x0c002000 0x100>;
> +			interrupts = <8>;
> +			interrupt-parent = <&PIC0>;

If you say interrupt-parent = <..> in the root node, you can
leave it out in most of the rest of the tree.  Using the Flipper
PIC for this sounds like a good plan.

> +		PIC0: pic0@0c003000 {
> +			#interrupt-cells = <1>;
> +			compatible = "nintendo,flipper-pic";
> +			reg = <0x0c003000 0x8>;

This register block is bigger actually.  It's not only PIC,
but some other bus stuff as well, dunno what to do here.

> +			interrupt-controller;
> +		};
> +
> +		resetswitch@0c003000 {
> +			compatible = "nintendo,hollywood-resetswitch";
> +			reg = <0x0c003000 0x4>;

That's the same address.  Don't do that.

Create a flipper-processor-interface node for the whole 3000
block (size 100)?  You can hang the PIC as a subnode under it,
without a "reg".

> +		/* Team Twiizers' 'mini' firmware IPC */
> +		mini@0d000000 {

Although mini is awesome, it isn't hardware, and doesn't
belong in the device tree.

> +			#address-cells = <1>;
> +			#size-cells = <1>;

There are no child nodes, these are meaningless.

> +			#interrupt-cells = <1>;

This isn't an interrupt controller.

> +			compatible = "twiizers,starlet-mini-ipc";
> +			reg = <0x0d000000 0x40	/* IPC */

You can get the IPC controller's address from the "main"
hollywood device node.

> +			       0x13fffffc 0x4>;	/* mini header pointer */

Put this in the platform code, it's not a hardware property.
It's not going to change either.

> +		serial@0d006400 {
> +			compatible = "nintendo,hollywood-serial";

You might want to pick a more descriptive name for this,
"serial" is usually understaood to mean "RS232".  Which
this isn't.

> +		/* External Interface bus */
> +		exi@0d006800 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			compatible = "nintendo,hollywood-exi";
> +			reg = <0x0d006800 0x40>;
> +			interrupts = <4>;
> +			interrupt-parent = <&PIC0>;
> +
> +			USBGECKO0: usbgecko@0d006814 {
> +				compatible = "usbgecko,usbgecko";
> +				reg = <0x0d006814 0x14>;
> +				virtual-reg = <0xcd006814>;
> +			};

I don't think you should put the usbgecko in the device tree,
it's a removable device, not everyone booting with this device
tree blob will have a usbgecko in this EXI port.  It's easy
to probe for as well.

> +		ehci@0d040000 {
> +			compatible = "nintendo,hollywood-ehci";

You might want to put the generic usb-ohci in here as well.
Although you need bug workarounds IIRC.

> +			reg = <0x0d040000 0x100
> +			       0x133e0000 0x80000>; /* 512 KB */

What's this MEM2 range?  A remnant from the old mini scheme?

> +		ohci0@0d050000 {

> +		ohci1@0d060000 {

> +		sdhc0@0d070000 {

> +		sdhc1@0d080000 {

Similar.

> +		hollywood-ahbprot@0d800064 {
> +			compatible = "nintendo,hollywood-ahbprot";
> +			reg = <0x0d800064 0x4>;
> +		};

There is no reason to single out this one register.  The kernel
shouldn't care for it anyway, the firmware sets it already.

> +		gpio0: hollywood-gpio@0d8000c0 {
> +			compatible = "nintendo,hollywood-gpio";
> +			reg = <0x0d8000c0 0x20>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +
> +		gpio1: hollywood-gpio@0d8000e0 {
> +			compatible = "nintendo,hollywood-gpio";
> +			reg = <0x0d8000e0 0x20>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};

Yuck, you have to make this two nodes for the GPIO binding?

> +		hollywood-resets@0d800194 {
> +			compatible = "nintendo,hollywood-resets";
> +			reg = <0x0d800194 0x4>;
> +		};

Don't do a separate node.

> +		i2c-video {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		        compatible = "virtual,i2c-gpio";

This is not a device from a company named "virtual".  Just
"i2c-gpio" will do.

As I said elsewhere, there should be a driver for this already,
and a device tree binding.  Add to that if at all possible, don't
make up something new.  Or if you do, give it a better name :-)

> +		        audio-video-encoder {
> +		                compatible = "nintendo,wii-ave-rvl";
> +		                reg = <0x70>;

audio-video-encoder@70 -- the unit address has to match the first
entry in "reg", always.

And "wii-ave-rvl", seriously?  Just call it "wii-audio-video-encoder",
a few extra chars don't hurt :-)


Segher
Albert Herranz - Nov. 25, 2009, 6:09 p.m.
Segher Boessenkool wrote:
>>>> +               i2c-video {
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>> +                       compatible = "virtual,i2c-gpio";
>>> There isn't a documented binding for this.  Is there a driver for it?
>>>
>> I have a driver for it. But it isn't yet published.
> 
> I bet there already is a generic IIC-via-GPIOs driver that
> you could use.
> 

Yes, there is a platform driver for that.
I picked that, extracted the common code into a common file, converted the existing driver to use the common code, then created a new one for platform bindings using the common code again.

(Then added gpio_direction_is_output() to gpiolib and sda_enforce_dir() to i2c-gpio).

Thanks,
Albert
Albert Herranz - Nov. 25, 2009, 6:34 p.m.
Segher Boessenkool wrote:
>> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
> 
> Like others have said already, don't do this.  If you need
> a workaround, put it in the platform code.
> 

I'll do. Thanks.

>> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
> 
> This address is fixed in the DSP hardware, right?  If not,
> you shouldn't do a reserve here.
> 

AFAIK, it is hardcoded in hardware.

>> +	chosen {
>> +		/* root filesystem on 2nd partition of SD card */
>> +		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
> 
> Question: why do you need/want nobats?
> 

We need nobats (or a hack in the mmu_mapin_ram() code) because of the discontiguous ram problem.

>> +	memory {
>> +		device_type = "memory";
>> +		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
>> +		reg = <0x00000000 0x133e0000>;
> 
> This should be  <0 0x01800000  0x10000000 0x04000000>
> 

Ok.

> I'm not sure what at the end of MEM2 you're trying to hide,
> but if you need to hide anything, do it in the platform code
> isntead.
> 
>> +	cpus {
>> +		#cpus = <1>;
> 
> Don't use #cpus
> 

I'll remove it if it's not needed. Thanks.

>> +	/* devices contained in the hollywood chipset */
>> +	soc {
> 
> It's not a SoC, more like a bridge chip.  More to the point,
> all the devices you put under this node actually sit in the
> root domain logically/physically, so why not put them there
> instead.  You'll want a node for the generic control registers,
> if you do.
> 

So, if i use a node here, what should be the proper name for it?

>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		#interrupt-cells = <1>;
>> +		model = "hollywood";
> 
> "model" isn't required here, if you don't have anything
> good to put in it, just don't use the property.
> 

I'll get rid of it.

>> +		compatible = "nintendo,hollywood";
>> +		clock-frequency = <243000000>; /* 243MHz */
> 
> What does this clock freq mean?  Hollywood has lots of
> clocks, many of them configurable.  Bus frequency is in
> the cpu node already.  The binding doesn't say what this
> is either, since you didn't write a binding :-)
> 
> Just remove it?

That's the bus frequency. If it's not needed there, I vote for dropping it.

> 
>> +		ranges = <0x0c000000 0x0c000000 0x00010000
>> +			  0x0d000000 0x0d000000 0x00010000
>> +			  0x0d040000 0x0d040000 0x00050000
>> +			  0x0d800000 0x0d800000 0x00001000
> 
> All of 0cXXXXXX and 0dXXXXXX actually.
> 
>> +			  0x133e0000 0x133e0000 0x00c20000>;
> 
> This is just part of MEM2, don't treat it special here.
> 

Ok.

>> +		video@0c002000 {
>> +			compatible = "nintendo,hollywood-video";
>> +			reg = <0x0c002000 0x100>;
>> +			interrupts = <8>;
>> +			interrupt-parent = <&PIC0>;
> 
> If you say interrupt-parent = <..> in the root node, you can
> leave it out in most of the rest of the tree.  Using the Flipper
> PIC for this sounds like a good plan.
> 

Yeah, agreed.

>> +		PIC0: pic0@0c003000 {
>> +			#interrupt-cells = <1>;
>> +			compatible = "nintendo,flipper-pic";
>> +			reg = <0x0c003000 0x8>;
> 
> This register block is bigger actually.  It's not only PIC,
> but some other bus stuff as well, dunno what to do here.
> 

Heh, you're the expert here :)

>> +			interrupt-controller;
>> +		};
>> +
>> +		resetswitch@0c003000 {
>> +			compatible = "nintendo,hollywood-resetswitch";
>> +			reg = <0x0c003000 0x4>;
> 
> That's the same address.  Don't do that.
> 
> Create a flipper-processor-interface node for the whole 3000
> block (size 100)?  You can hang the PIC as a subnode under it,
> without a "reg".
> 

Ok.

>> +		/* Team Twiizers' 'mini' firmware IPC */
>> +		mini@0d000000 {
> 
> Although mini is awesome, it isn't hardware, and doesn't
> belong in the device tree.
> 

True, now that I'm starting to understand what's supposed to be and what's supposed not to be in the device tree.

>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
> 
> There are no child nodes, these are meaningless.
> 

Yeah.
In previous versions of the tree, before AHBPROT was discovered, all hardware accessed via the firmware ipc interface hanged here, though.

>> +			#interrupt-cells = <1>;
> 
> This isn't an interrupt controller.
> 

Yup, not applicable anymore.

>> +			compatible = "twiizers,starlet-mini-ipc";
>> +			reg = <0x0d000000 0x40	/* IPC */
> 
> You can get the IPC controller's address from the "main"
> hollywood device node.
> 

What about the interrupt associated to the IPC hardware?

>> +			       0x13fffffc 0x4>;	/* mini header pointer */
> 
> Put this in the platform code, it's not a hardware property.
> It's not going to change either.
> 

Ok.

>> +		serial@0d006400 {
>> +			compatible = "nintendo,hollywood-serial";
> 
> You might want to pick a more descriptive name for this,
> "serial" is usually understaood to mean "RS232".  Which
> this isn't.
> 

Nintendo calls it "Serial Interface" (SI).
Would "nintendo,hollywood-serial-interface" work here?

>> +		/* External Interface bus */
>> +		exi@0d006800 {
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			compatible = "nintendo,hollywood-exi";
>> +			reg = <0x0d006800 0x40>;
>> +			interrupts = <4>;
>> +			interrupt-parent = <&PIC0>;
>> +
>> +			USBGECKO0: usbgecko@0d006814 {
>> +				compatible = "usbgecko,usbgecko";
>> +				reg = <0x0d006814 0x14>;
>> +				virtual-reg = <0xcd006814>;
>> +			};
> 
> I don't think you should put the usbgecko in the device tree,
> it's a removable device, not everyone booting with this device
> tree blob will have a usbgecko in this EXI port.  It's easy
> to probe for as well.
> 

Ok.

>> +		ehci@0d040000 {
>> +			compatible = "nintendo,hollywood-ehci";
> 
> You might want to put the generic usb-ohci in here as well.
> Although you need bug workarounds IIRC.
> 

Yes.

>> +			reg = <0x0d040000 0x100
>> +			       0x133e0000 0x80000>; /* 512 KB */
> 
> What's this MEM2 range?  A remnant from the old mini scheme?
> 
>> +		ohci0@0d050000 {
> 
>> +		ohci1@0d060000 {
> 
>> +		sdhc0@0d070000 {
> 
>> +		sdhc1@0d080000 {
> 
> Similar.
> 

Yes. This can be removed now.

>> +		hollywood-ahbprot@0d800064 {
>> +			compatible = "nintendo,hollywood-ahbprot";
>> +			reg = <0x0d800064 0x4>;
>> +		};
> 
> There is no reason to single out this one register.  The kernel
> shouldn't care for it anyway, the firmware sets it already.
> 

As long as a recent firmware version is used, yes.

How should all these register be declared in the device tree?
Using a block of registers and declaring the block as "nintendo,hollywood-control" or something?

>> +		gpio0: hollywood-gpio@0d8000c0 {
>> +			compatible = "nintendo,hollywood-gpio";
>> +			reg = <0x0d8000c0 0x20>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
>> +
>> +		gpio1: hollywood-gpio@0d8000e0 {
>> +			compatible = "nintendo,hollywood-gpio";
>> +			reg = <0x0d8000e0 0x20>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
> 
> Yuck, you have to make this two nodes for the GPIO binding?
> 

Yup. Two gpio nodes for two 32 gpio words.

>> +		hollywood-resets@0d800194 {
>> +			compatible = "nintendo,hollywood-resets";
>> +			reg = <0x0d800194 0x4>;
>> +		};
> 
> Don't do a separate node.
> 

So this should be part of the "control" block?

>> +		i2c-video {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		        compatible = "virtual,i2c-gpio";
> 
> This is not a device from a company named "virtual".  Just
> "i2c-gpio" will do.
> 
> As I said elsewhere, there should be a driver for this already,
> and a device tree binding.  Add to that if at all possible, don't
> make up something new.  Or if you do, give it a better name :-)
> 

Plese, see my previous comment on this.

>> +		        audio-video-encoder {
>> +		                compatible = "nintendo,wii-ave-rvl";
>> +		                reg = <0x70>;
> 
> audio-video-encoder@70 -- the unit address has to match the first
> entry in "reg", always.
> 
> And "wii-ave-rvl", seriously?  Just call it "wii-audio-video-encoder",
> a few extra chars don't hurt :-)
> 

Agreed too. :)

Thanks!
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 4:45 a.m.
On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:

> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */

Weird layout... nothing you can do about I suppose.

Out of curiosity, what is that DSP RAM ? Some actual DSP core somewhere
in the IO chip setup to use memory from up there ?

I'll skip on some things that I'm sure others will have commented
about :-)

> +	/*
> +	 * The Nintendo Wii has two discontiguous RAM memory areas called
> +	 * MEM1 and MEM2.
> +	 * MEM1 starts at 0x00000000 and contains 24MB of 1T-SRAM.
> +	 * MEM2 starts at 0x10000000 and contains 64MB of DDR2 RAM.
> +	 * Between both memory address ranges there is an address space
> +	 * where memory-mapped I/O registers are found.
> +	 *
> +	 * Currently, Linux 32-bit PowerPC does not support RAM in
> +	 * discontiguous memory address spaces. Thus, in order to use
> +	 * both RAM areas, we declare as RAM the range from the start of
> +	 * MEM1 to the end of useable MEM2 and exclude the needed parts
> +	 * with /memreserve/ statements, at the expense of wasting a bit
> +	 * of memory.
> +	 */
> +	memory {
> +		device_type = "memory";
> +		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
> +		reg = <0x00000000 0x133e0000>;
> +	};

So we do have a nasty hole here in the middle. How bad it is if you try
to just have two ranges in there (ie as if it was discontiguous) ? We
shouldn't be far from being able to do discontig mem actually, should be
easy enough to fix. Tho I don't have (yet) the HW :-) (I'm tempted...)

Same comment as other discussions about the framebuffer here.

> +	cpus {
> +		#cpus = <1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		PowerPC,broadway@0 {
> +			device_type = "cpu";
> +			reg = <0>;
> +			clock-frequency = <729000000>; /* 729MHz */
> +			bus-frequency = <243000000>; /* 243MHz core-to-bus 3x */
> +			timebase-frequency = <60750000>; /* 243MHz / 4 */
> +			i-cache-line-size = <32>;
> +			d-cache-line-size = <32>;
> +			i-cache-size = <32768>;
> +			d-cache-size = <32768>;
> +		};
> +	};
> +
> +	/* devices contained in the hollywood chipset */
> +	soc {

Call it "hollywood"

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		#interrupt-cells = <1>;
> +		model = "hollywood";
> +		compatible = "nintendo,hollywood";
> +		clock-frequency = <243000000>; /* 243MHz */
> +		ranges = <0x0c000000 0x0c000000 0x00010000
> +			  0x0d000000 0x0d000000 0x00010000
> +			  0x0d040000 0x0d040000 0x00050000
> +			  0x0d800000 0x0d800000 0x00001000
> +			  0x133e0000 0x133e0000 0x00c20000>;
> +
> +		video@0c002000 {
> +			compatible = "nintendo,hollywood-video";
> +			reg = <0x0c002000 0x100>;
> +			interrupts = <8>;
> +			interrupt-parent = <&PIC0>;
> +		};
> +
> +		PIC0: pic0@0c003000 {
> +			#interrupt-cells = <1>;
> +			compatible = "nintendo,flipper-pic";
> +			reg = <0x0c003000 0x8>;
> +			interrupt-controller;
> +		};
> +
> +		resetswitch@0c003000 {
> +			compatible = "nintendo,hollywood-resetswitch";
> +			reg = <0x0c003000 0x4>;
> +			interrupts = <1>;
> +			interrupt-parent = <&PIC0>;
> +		};
> +
> +		audio@0c005000 {
> +			compatible = "nintendo,hollywood-audio";
> +			reg = <0x0c005000 0x200		/* DSP */
> +			       0x0d006c00 0x20>;	/* AI */
> +			interrupts = <6>;
> +			interrupt-parent = <&PIC0>;
> +		};
> +
> +		/* Team Twiizers' 'mini' firmware IPC */

Out of curiosity, what are these ? :-)

> +		mini@0d000000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#interrupt-cells = <1>;
> +			compatible = "twiizers,starlet-mini-ipc";
> +			reg = <0x0d000000 0x40	/* IPC */
> +			       0x13fffffc 0x4>;	/* mini header pointer */
> +		};
> +
> +		serial@0d006400 {
> +			compatible = "nintendo,hollywood-serial";
> +			reg = <0x0d006400 0x100>;
> +			interrupts = <3>;
> +			interrupt-parent = <&PIC0>;
> +		};
> +
> +		/* External Interface bus */
> +		exi@0d006800 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			compatible = "nintendo,hollywood-exi";
> +			reg = <0x0d006800 0x40>;
> +			interrupts = <4>;
> +			interrupt-parent = <&PIC0>;
> +
> +			USBGECKO0: usbgecko@0d006814 {
> +				compatible = "usbgecko,usbgecko";
> +				reg = <0x0d006814 0x14>;
> +				virtual-reg = <0xcd006814>;
> +			};
> +		};

Similar comment as before, could the above be dynamically probed ? If
you are not a hacker you may not need that at all to use some linux
based piece of SW on the wii right ?

> +		ehci@0d040000 {
> +			compatible = "nintendo,hollywood-ehci";
> +			reg = <0x0d040000 0x100
> +			       0x133e0000 0x80000>; /* 512 KB */
> +			interrupts = <4>;
> +			interrupt-parent = <&PIC1>;
> +		};
> +
> +		ohci0@0d050000 {
> +			compatible = "nintendo,hollywood-ohci";
> +			reg = <0x0d050000 0x100
> +			       0x13460000 0x80000>; /* 512 KB */
> +			interrupts = <5>;
> +			interrupt-parent = <&PIC1>;
> +		};
> +
> +		ohci1@0d060000 {

Why the "1" ?

> +			compatible = "nintendo,hollywood-ohci";
> +			reg = <0x0d060000 0x100
> +			       0x134e0000 0x80000>; /* 512 KB */
> +			interrupts = <6>;
> +			interrupt-parent = <&PIC1>;
> +		};

Are the above OHCI and EHCI "special" ? If not, there's an existing
binding for that sort of thing, which btw requires properties to
indicate the endianness of the registers and in-memory data structures
etc...

> +		sdhc0@0d070000 {
> +			compatible = "nintendo,hollywood-sdhci";
> +			reg = <0x0d070000 0x200>;
> +			interrupts = <7>;
> +			interrupt-parent = <&PIC1>;
> +		};
> +
> +		sdhc1@0d080000 {
> +			compatible = "nintendo,hollywood-sdhci";
> +			reg = <0x0d080000 0x200>;
> +			interrupts = <8>;
> +			interrupt-parent = <&PIC1>;
> +		};

Again, no need for a "1" in there. Names don't have to be unique if the
unit address is different.

> +		PIC1: pic1@0d800030 {
> +			#interrupt-cells = <1>;
> +			compatible = "nintendo,hollywood-pic";
> +			reg = <0x0d800030 0x8>;
> +			interrupt-controller;
> +			interrupts = <14>;
> +			interrupt-parent = <&PIC0>;
> +		};

Ah, a cascaded PIC, fun fun fun

> +		hollywood-ahbprot@0d800064 {
> +			compatible = "nintendo,hollywood-ahbprot";
> +			reg = <0x0d800064 0x4>;
> +		};

What is this ?

> +		gpio0: hollywood-gpio@0d8000c0 {
> +			compatible = "nintendo,hollywood-gpio";
> +			reg = <0x0d8000c0 0x20>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +
> +		gpio1: hollywood-gpio@0d8000e0 {
> +			compatible = "nintendo,hollywood-gpio";
> +			reg = <0x0d8000e0 0x20>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};

Same comment about the "1"

> +		hollywood-resets@0d800194 {
> +			compatible = "nintendo,hollywood-resets";
> +			reg = <0x0d800194 0x4>;
> +		};
> +
> +		i2c-video {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		        compatible = "virtual,i2c-gpio";
> +
> +		        gpios = <&gpio0  16 0 /* 31-15 */
> +		                 &gpio0  17 0 /* 31-14 */
> +				>;
> +		        sda-is-open-drain = <1>;
> +		        sda-enforce-dir = <1>;
> +		        scl-is-open-drain = <1>;
> +		        scl-is-output-only = <1>;
> +		        udelay = <2>;
> +
> +		        audio-video-encoder {
> +		                compatible = "nintendo,wii-ave-rvl";
> +		                reg = <0x70>;
> +		        };
> +		};
> +	};
> +};
> +

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 4:51 a.m.
On Wed, 2009-11-25 at 18:49 +0100, Segher Boessenkool wrote:
> > +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
> 
> Like others have said already, don't do this.  If you need
> a workaround, put it in the platform code.
> 
> > +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
> 
> This address is fixed in the DSP hardware, right?  If not,
> you shouldn't do a reserve here.
> 
> > +	chosen {
> > +		/* root filesystem on 2nd partition of SD card */
> > +		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
> 
> Question: why do you need/want nobats?

Good point. I can't even guarantee that the kernel will work reliably
with nobats :-) At least you really want the kernel .text to be fully
covered by an IBAT.

> What does this clock freq mean?  Hollywood has lots of
> clocks, many of them configurable.  Bus frequency is in
> the cpu node already.  The binding doesn't say what this
> is either, since you didn't write a binding :-)
> 
> Just remove it?

BTW. If we want to play with clocks, maybe you should look at
my proposed binding for clocks and implementing the clk API :-)

> > +		ranges = <0x0c000000 0x0c000000 0x00010000
> > +			  0x0d000000 0x0d000000 0x00010000
> > +			  0x0d040000 0x0d040000 0x00050000
> > +			  0x0d800000 0x0d800000 0x00001000
> 
> All of 0cXXXXXX and 0dXXXXXX actually.
> 
> > +			  0x133e0000 0x133e0000 0x00c20000>;
> 
> This is just part of MEM2, don't treat it special here.
> 
> > +		video@0c002000 {
> > +			compatible = "nintendo,hollywood-video";
> > +			reg = <0x0c002000 0x100>;
> > +			interrupts = <8>;
> > +			interrupt-parent = <&PIC0>;
> 
> If you say interrupt-parent = <..> in the root node, you can
> leave it out in most of the rest of the tree.  Using the Flipper
> PIC for this sounds like a good plan.

Well, for the rest of the tree except stuff whose interrupt parent
is PIC1. With two PICs I prefer being explicit.

> > +		PIC0: pic0@0c003000 {
> > +			#interrupt-cells = <1>;
> > +			compatible = "nintendo,flipper-pic";
> > +			reg = <0x0c003000 0x8>;
> 
> This register block is bigger actually.  It's not only PIC,
> but some other bus stuff as well, dunno what to do here.

Add nodes for the other things ?

> > +			interrupt-controller;
> > +		};
> > +
> > +		resetswitch@0c003000 {
> > +			compatible = "nintendo,hollywood-resetswitch";
> > +			reg = <0x0c003000 0x4>;
> 
> That's the same address.  Don't do that.
> 
> Create a flipper-processor-interface node for the whole 3000
> block (size 100)?  You can hang the PIC as a subnode under it,
> without a "reg".

Yeah.

> > +		/* Team Twiizers' 'mini' firmware IPC */
> > +		mini@0d000000 {
> 
> Although mini is awesome, it isn't hardware, and doesn't
> belong in the device tree.

So what is at d0000000 ?

> > +		serial@0d006400 {
> > +			compatible = "nintendo,hollywood-serial";
> 
> You might want to pick a more descriptive name for this,
> "serial" is usually understaood to mean "RS232".  Which
> this isn't.

What is it then ? You definitely want some other name if it's not
classic rs232 style serial.

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 4:58 a.m.
On Wed, 2009-11-25 at 19:34 +0100, Albert Herranz wrote:
> 
> We need nobats (or a hack in the mmu_mapin_ram() code) because of the
> discontiguous ram problem.

I would vote for a hack in mmu_mapin_ram() for now.

Cheers,
Ben.
Albert Herranz - Nov. 26, 2009, 3:09 p.m.
Benjamin Herrenschmidt wrote:
> On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
> 
>> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
>> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
> 
> Weird layout... nothing you can do about I suppose.
> 
> Out of curiosity, what is that DSP RAM ? Some actual DSP core somewhere
> in the IO chip setup to use memory from up there ?
> 

Yes, there's a DSP used normally for audio purposes.

> I'll skip on some things that I'm sure others will have commented
> about :-)
> 

Ok :)

>> +	/*
>> +	 * The Nintendo Wii has two discontiguous RAM memory areas called
>> +	 * MEM1 and MEM2.
>> +	 * MEM1 starts at 0x00000000 and contains 24MB of 1T-SRAM.
>> +	 * MEM2 starts at 0x10000000 and contains 64MB of DDR2 RAM.
>> +	 * Between both memory address ranges there is an address space
>> +	 * where memory-mapped I/O registers are found.
>> +	 *
>> +	 * Currently, Linux 32-bit PowerPC does not support RAM in
>> +	 * discontiguous memory address spaces. Thus, in order to use
>> +	 * both RAM areas, we declare as RAM the range from the start of
>> +	 * MEM1 to the end of useable MEM2 and exclude the needed parts
>> +	 * with /memreserve/ statements, at the expense of wasting a bit
>> +	 * of memory.
>> +	 */
>> +	memory {
>> +		device_type = "memory";
>> +		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
>> +		reg = <0x00000000 0x133e0000>;
>> +	};
> 
> So we do have a nasty hole here in the middle. How bad it is if you try
> to just have two ranges in there (ie as if it was discontiguous) ? We
> shouldn't be far from being able to do discontig mem actually, should be
> easy enough to fix. Tho I don't have (yet) the HW :-) (I'm tempted...)
> 

My plan is to just put the two ranges on the device tree (to reflect the actual hardware) and coalesce them in the fixup function of the bootwrapper.
Then add the needed reservations and hacks to make that work (mmu_mapin_ram / ioremap).

> Same comment as other discussions about the framebuffer here.
> 

Ok.

>> +	cpus {
>> +		#cpus = <1>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		PowerPC,broadway@0 {
>> +			device_type = "cpu";
>> +			reg = <0>;
>> +			clock-frequency = <729000000>; /* 729MHz */
>> +			bus-frequency = <243000000>; /* 243MHz core-to-bus 3x */
>> +			timebase-frequency = <60750000>; /* 243MHz / 4 */
>> +			i-cache-line-size = <32>;
>> +			d-cache-line-size = <32>;
>> +			i-cache-size = <32768>;
>> +			d-cache-size = <32768>;
>> +		};
>> +	};
>> +
>> +	/* devices contained in the hollywood chipset */
>> +	soc {
> 
> Call it "hollywood"
> 

I like that too. Simple and effective.

>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		#interrupt-cells = <1>;
>> +		model = "hollywood";
>> +		compatible = "nintendo,hollywood";
>> +		clock-frequency = <243000000>; /* 243MHz */
>> +		ranges = <0x0c000000 0x0c000000 0x00010000
>> +			  0x0d000000 0x0d000000 0x00010000
>> +			  0x0d040000 0x0d040000 0x00050000
>> +			  0x0d800000 0x0d800000 0x00001000
>> +			  0x133e0000 0x133e0000 0x00c20000>;
>> +
>> +		video@0c002000 {
>> +			compatible = "nintendo,hollywood-video";
>> +			reg = <0x0c002000 0x100>;
>> +			interrupts = <8>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
>> +
>> +		PIC0: pic0@0c003000 {
>> +			#interrupt-cells = <1>;
>> +			compatible = "nintendo,flipper-pic";
>> +			reg = <0x0c003000 0x8>;
>> +			interrupt-controller;
>> +		};
>> +
>> +		resetswitch@0c003000 {
>> +			compatible = "nintendo,hollywood-resetswitch";
>> +			reg = <0x0c003000 0x4>;
>> +			interrupts = <1>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
>> +
>> +		audio@0c005000 {
>> +			compatible = "nintendo,hollywood-audio";
>> +			reg = <0x0c005000 0x200		/* DSP */
>> +			       0x0d006c00 0x20>;	/* AI */
>> +			interrupts = <6>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
>> +
>> +		/* Team Twiizers' 'mini' firmware IPC */
> 
> Out of curiosity, what are these ? :-)
> 

Those are the guys behind most Wii hacking efforts.

>> +		mini@0d000000 {
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			#interrupt-cells = <1>;
>> +			compatible = "twiizers,starlet-mini-ipc";
>> +			reg = <0x0d000000 0x40	/* IPC */
>> +			       0x13fffffc 0x4>;	/* mini header pointer */
>> +		};
>> +
>> +		serial@0d006400 {
>> +			compatible = "nintendo,hollywood-serial";
>> +			reg = <0x0d006400 0x100>;
>> +			interrupts = <3>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
>> +
>> +		/* External Interface bus */
>> +		exi@0d006800 {
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			compatible = "nintendo,hollywood-exi";
>> +			reg = <0x0d006800 0x40>;
>> +			interrupts = <4>;
>> +			interrupt-parent = <&PIC0>;
>> +
>> +			USBGECKO0: usbgecko@0d006814 {
>> +				compatible = "usbgecko,usbgecko";
>> +				reg = <0x0d006814 0x14>;
>> +				virtual-reg = <0xcd006814>;
>> +			};
>> +		};
> 
> Similar comment as before, could the above be dynamically probed ? If
> you are not a hacker you may not need that at all to use some linux
> based piece of SW on the wii right ?
> 

Yes, the usbgecko is more of a developer gadget.

>> +		ehci@0d040000 {
>> +			compatible = "nintendo,hollywood-ehci";
>> +			reg = <0x0d040000 0x100
>> +			       0x133e0000 0x80000>; /* 512 KB */
>> +			interrupts = <4>;
>> +			interrupt-parent = <&PIC1>;
>> +		};
>> +
>> +		ohci0@0d050000 {
>> +			compatible = "nintendo,hollywood-ohci";
>> +			reg = <0x0d050000 0x100
>> +			       0x13460000 0x80000>; /* 512 KB */
>> +			interrupts = <5>;
>> +			interrupt-parent = <&PIC1>;
>> +		};
>> +
>> +		ohci1@0d060000 {
> 
> Why the "1" ?
> 

I thought that names should be unique, but I see they don't need to.
I'll use just "ohci".

>> +			compatible = "nintendo,hollywood-ohci";
>> +			reg = <0x0d060000 0x100
>> +			       0x134e0000 0x80000>; /* 512 KB */
>> +			interrupts = <6>;
>> +			interrupt-parent = <&PIC1>;
>> +		};
> 
> Are the above OHCI and EHCI "special" ? If not, there's an existing
> binding for that sort of thing, which btw requires properties to
> indicate the endianness of the registers and in-memory data structures
> etc...
> 

They are a bit special because registers are in reverse little endian format, must be written in 32-bit chunks, and (all things point to) they have hardware bugs.

>> +		sdhc0@0d070000 {
>> +			compatible = "nintendo,hollywood-sdhci";
>> +			reg = <0x0d070000 0x200>;
>> +			interrupts = <7>;
>> +			interrupt-parent = <&PIC1>;
>> +		};
>> +
>> +		sdhc1@0d080000 {
>> +			compatible = "nintendo,hollywood-sdhci";
>> +			reg = <0x0d080000 0x200>;
>> +			interrupts = <8>;
>> +			interrupt-parent = <&PIC1>;
>> +		};
> 
> Again, no need for a "1" in there. Names don't have to be unique if the
> unit address is different.
> 

I'll fix this too.

>> +		PIC1: pic1@0d800030 {
>> +			#interrupt-cells = <1>;
>> +			compatible = "nintendo,hollywood-pic";
>> +			reg = <0x0d800030 0x8>;
>> +			interrupt-controller;
>> +			interrupts = <14>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
> 
> Ah, a cascaded PIC, fun fun fun
> 

It works fine :)

>> +		hollywood-ahbprot@0d800064 {
>> +			compatible = "nintendo,hollywood-ahbprot";
>> +			reg = <0x0d800064 0x4>;
>> +		};
> 
> What is this ?
> 

A register that controls if the PowerPC side can directly access part of the hardware or not.

>> +		gpio0: hollywood-gpio@0d8000c0 {
>> +			compatible = "nintendo,hollywood-gpio";
>> +			reg = <0x0d8000c0 0x20>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
>> +
>> +		gpio1: hollywood-gpio@0d8000e0 {
>> +			compatible = "nintendo,hollywood-gpio";
>> +			reg = <0x0d8000e0 0x20>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
> 
> Same comment about the "1"
> 

Yup.

>> +		hollywood-resets@0d800194 {
>> +			compatible = "nintendo,hollywood-resets";
>> +			reg = <0x0d800194 0x4>;
>> +		};
>> +
>> +		i2c-video {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		        compatible = "virtual,i2c-gpio";
>> +
>> +		        gpios = <&gpio0  16 0 /* 31-15 */
>> +		                 &gpio0  17 0 /* 31-14 */
>> +				>;
>> +		        sda-is-open-drain = <1>;
>> +		        sda-enforce-dir = <1>;
>> +		        scl-is-open-drain = <1>;
>> +		        scl-is-output-only = <1>;
>> +		        udelay = <2>;
>> +
>> +		        audio-video-encoder {
>> +		                compatible = "nintendo,wii-ave-rvl";
>> +		                reg = <0x70>;
>> +		        };
>> +		};
>> +	};
>> +};
>> +
> 
> Cheers,
> Ben.
> 

Thanks,
Albert
Albert Herranz - Nov. 26, 2009, 3:18 p.m.
Benjamin Herrenschmidt wrote:
> On Wed, 2009-11-25 at 18:49 +0100, Segher Boessenkool wrote:
>>> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
>> Like others have said already, don't do this.  If you need
>> a workaround, put it in the platform code.
>>
>>> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
>> This address is fixed in the DSP hardware, right?  If not,
>> you shouldn't do a reserve here.
>>
>>> +	chosen {
>>> +		/* root filesystem on 2nd partition of SD card */
>>> +		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
>> Question: why do you need/want nobats?
> 
> Good point. I can't even guarantee that the kernel will work reliably
> with nobats :-) At least you really want the kernel .text to be fully
> covered by an IBAT.
> 

It works with nobats.
I must say that all the patches posted (and the device drivers, which haven't been posted yet) are tested and working code.

>> What does this clock freq mean?  Hollywood has lots of
>> clocks, many of them configurable.  Bus frequency is in
>> the cpu node already.  The binding doesn't say what this
>> is either, since you didn't write a binding :-)
>>
>> Just remove it?
> 
> BTW. If we want to play with clocks, maybe you should look at
> my proposed binding for clocks and implementing the clk API :-)
> 
>>> +		ranges = <0x0c000000 0x0c000000 0x00010000
>>> +			  0x0d000000 0x0d000000 0x00010000
>>> +			  0x0d040000 0x0d040000 0x00050000
>>> +			  0x0d800000 0x0d800000 0x00001000
>> All of 0cXXXXXX and 0dXXXXXX actually.
>>
>>> +			  0x133e0000 0x133e0000 0x00c20000>;
>> This is just part of MEM2, don't treat it special here.
>>
>>> +		video@0c002000 {
>>> +			compatible = "nintendo,hollywood-video";
>>> +			reg = <0x0c002000 0x100>;
>>> +			interrupts = <8>;
>>> +			interrupt-parent = <&PIC0>;
>> If you say interrupt-parent = <..> in the root node, you can
>> leave it out in most of the rest of the tree.  Using the Flipper
>> PIC for this sounds like a good plan.
> 
> Well, for the rest of the tree except stuff whose interrupt parent
> is PIC1. With two PICs I prefer being explicit.
> 

Let it be specific then :)

>>> +		PIC0: pic0@0c003000 {
>>> +			#interrupt-cells = <1>;
>>> +			compatible = "nintendo,flipper-pic";
>>> +			reg = <0x0c003000 0x8>;
>> This register block is bigger actually.  It's not only PIC,
>> but some other bus stuff as well, dunno what to do here.
> 
> Add nodes for the other things ?
> 
>>> +			interrupt-controller;
>>> +		};
>>> +
>>> +		resetswitch@0c003000 {
>>> +			compatible = "nintendo,hollywood-resetswitch";
>>> +			reg = <0x0c003000 0x4>;
>> That's the same address.  Don't do that.
>>
>> Create a flipper-processor-interface node for the whole 3000
>> block (size 100)?  You can hang the PIC as a subnode under it,
>> without a "reg".
> 
> Yeah.
> 

Agreed.

>>> +		/* Team Twiizers' 'mini' firmware IPC */
>>> +		mini@0d000000 {
>> Although mini is awesome, it isn't hardware, and doesn't
>> belong in the device tree.
> 
> So what is at d0000000 ?
> 

There you can find the hardware interface that supports the IPC mechanism.
It is made up of a pair of registers to pass data between the processors and a pair of control/flags registers.
The hardware can interrupt the PowerPC side when there is data available for it.

>>> +		serial@0d006400 {
>>> +			compatible = "nintendo,hollywood-serial";
>> You might want to pick a more descriptive name for this,
>> "serial" is usually understaood to mean "RS232".  Which
>> this isn't.
> 
> What is it then ? You definitely want some other name if it's not
> classic rs232 style serial.
> 

It is what Nintendo calls the "Serial Interface" (SI) which is a proprietary serial hardware to drive the gamepads (and a custom keyboard too, once available for an RPG game).

> Cheers,
> Ben.
> 

Thanks,
Albert
Albert Herranz - Nov. 26, 2009, 3:19 p.m.
Benjamin Herrenschmidt wrote:
> On Wed, 2009-11-25 at 19:34 +0100, Albert Herranz wrote:
>> We need nobats (or a hack in the mmu_mapin_ram() code) because of the
>> discontiguous ram problem.
> 
> I would vote for a hack in mmu_mapin_ram() for now.
> 

I'll cook that.

> Cheers,
> Ben.
> 

Thanks,
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 8:48 p.m.
On Thu, 2009-11-26 at 16:09 +0100, Albert Herranz wrote:

> > Are the above OHCI and EHCI "special" ? If not, there's an existing
> > binding for that sort of thing, which btw requires properties to
> > indicate the endianness of the registers and in-memory data structures
> > etc...
> > 
> 
> They are a bit special because registers are in reverse little endian format,
> must be written in 32-bit chunks, and (all things point to) they have hardware bugs.

Well.. first what is "reverse little endian" ? :-) Big endian ?

The OHCI driver today has separate flags to control register endianness
and memory based data structures endianness.

I think we also only use 32-bit reads and writes no ? So that should be
covered :-)

As for HW bugs, well, as long as we know them we can define a quirk bit
and add the necessary workarounds to the driver :-)

Do you have a patch somewhere that adds the OCHI and EHCI support btw ?
 
Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 9:01 p.m.
> > Good point. I can't even guarantee that the kernel will work reliably
> > with nobats :-) At least you really want the kernel .text to be fully
> > covered by an IBAT.
> > 
> 
> It works with nobats.

But does it work -reliably- ? :-)

I haven't looked at that for years, but we used to have a subtle issue
if you happen to take a hash miss on the kernel text or data in the
wrong time, such as when SRR0/SRR1 are modified and before a subsequent
rfi. This is very very hard to trigger and maybe impossible without SMP
but to keep in mind.

Paulus added some code ages ago to close most of these by using the
MSR:RI bit so that the hash code could detect the situation and branch
to some "recovery" code, but from memory, while that dealt with missing
D-BATs, we still had a potential window of problem if the kernel text
itself wasn't entirely covered.

Any ways, not a big deal right now, as I said, we really want the BATs
for performances anyways, so we should probably just add some kind of
hack in mmu_mapin_ram() for the time being.

> I must say that all the patches posted (and the device drivers, which haven't
> been posted yet) are tested and working code.

That was my impression too, but in this case, I'm talking about a
potential very hard to hit problem that you may well have never managed
to actually trigger.

> There you can find the hardware interface that supports the IPC mechanism.
> It is made up of a pair of registers to pass data between the processors and a
> pair of control/flags registers.
> The hardware can interrupt the PowerPC side when there is data available for it.

Ok. So the right way to do that would be to have a node purely
representing the HW IPC, unrelated to whatever is running on the
secondary processor.

However, it's ok to have -below- that node, a set of device nodes or a
node with properties or whatever representing the FW in there and the
function it exposes.

That can be discussed later tho. I'm not that keen on having those info
be in the .dts coming with the kernel since those functions essentially
depend on what FW is loaded on the aux. processor.

What might do however is to have a way for that FW itself to provide you
with the nodes and properties for the functions it provides :-) Then you
can have the boot wrapper or the kernel platform init code use some well
defined (and as stable as possible) IPC API to identify the FW there and
expose all that stuff.

Of course that wouldn't work with FW we don't have control on. Can Linux
run on the wii with the original N. FW on the aux. processor ? Can we
detect what is running there ? Do we care ?

> It is what Nintendo calls the "Serial Interface" (SI) which is a proprietary
> serial hardware to drive the gamepads (and a custom keyboard too, once available
> for an RPG game).

So I would give it a different name than "serial" then. Make it gpsi
maybe ? (game pad serial interface ?) :-) Or invent something else...

Cheers,
Ben.

> > Cheers,
> > Ben.
> > 
> 
> Thanks,
> Albert
Albert Herranz - Nov. 26, 2009, 9:20 p.m.
Benjamin Herrenschmidt wrote:
> On Thu, 2009-11-26 at 16:09 +0100, Albert Herranz wrote:
> 
>>> Are the above OHCI and EHCI "special" ? If not, there's an existing
>>> binding for that sort of thing, which btw requires properties to
>>> indicate the endianness of the registers and in-memory data structures
>>> etc...
>>>
>> They are a bit special because registers are in reverse little endian format,
>> must be written in 32-bit chunks, and (all things point to) they have hardware bugs.
> 
> Well.. first what is "reverse little endian" ? :-) Big endian ?
> 

It's the same case as the register access for the Freescale eSDHC controller, which is already in the existing sdhci-of driver.

> The OHCI driver today has separate flags to control register endianness
> and memory based data structures endianness.
> 
> I think we also only use 32-bit reads and writes no ? So that should be
> covered :-)
> 

There are 8, 16 and 32 bit accesses. We need the same workarounds as present for the Freescale eSDHC controller.

> As for HW bugs, well, as long as we know them we can define a quirk bit
> and add the necessary workarounds to the driver :-)
> 

Yes, that's how it was done.
Although it it currently not integrated into sdhci-of. I think that the specific eSDHC code should be moved to another file, and leave the sdhci-of with the generic code and the sdhci_of_match table. Then adding a similar driver would simply consist in adding another file with the sdhci_data (containing ops and quirks) and adding an entry to the sdhci_of_match table in sdhci-of. 

> Do you have a patch somewhere that adds the OCHI and EHCI support btw ?
>  

Yes, too. Although it needs review and cleanup too.

I have a git tree with all the stuff, but it contains only the existing code previous to the effort of mainlining it:
 http://git.infradead.org/users/herraa1/gc-linux-2.6.git/shortlog/refs/heads/mini/gc-linux-2.6.31

> Cheers,
> Ben.
> 

Thanks,
Albert
Albert Herranz - Nov. 26, 2009, 9:38 p.m.
Benjamin Herrenschmidt wrote:
>>> Good point. I can't even guarantee that the kernel will work reliably
>>> with nobats :-) At least you really want the kernel .text to be fully
>>> covered by an IBAT.
>>>
>> It works with nobats.
> 
> But does it work -reliably- ? :-)
> 

It does AFAICT. My Wii is a 24x7 linux box although it is not stressed in any way usually.

> Any ways, not a big deal right now, as I said, we really want the BATs
> for performances anyways, so we should probably just add some kind of
> hack in mmu_mapin_ram() for the time being.
> 

Yup. The idea is to map the first 16MB of MEM1 with a BAT.
Mapping the whole 24MB with BATs may not be possible because we want the framebuffer in MEM1 for performance reasons.

>> I must say that all the patches posted (and the device drivers, which haven't
>> been posted yet) are tested and working code.
> 
> That was my impression too, but in this case, I'm talking about a
> potential very hard to hit problem that you may well have never managed
> to actually trigger.
> 

I haven't actually, if that applies :)

>> There you can find the hardware interface that supports the IPC mechanism.
>> It is made up of a pair of registers to pass data between the processors and a
>> pair of control/flags registers.
>> The hardware can interrupt the PowerPC side when there is data available for it.
> 
> Ok. So the right way to do that would be to have a node purely
> representing the HW IPC, unrelated to whatever is running on the
> secondary processor.
> 

Totally agreed.

> However, it's ok to have -below- that node, a set of device nodes or a
> node with properties or whatever representing the FW in there and the
> function it exposes.
> 
> That can be discussed later tho. I'm not that keen on having those info
> be in the .dts coming with the kernel since those functions essentially
> depend on what FW is loaded on the aux. processor.
> 

I think we can leave this for later as you said.

> What might do however is to have a way for that FW itself to provide you
> with the nodes and properties for the functions it provides :-) Then you
> can have the boot wrapper or the kernel platform init code use some well
> defined (and as stable as possible) IPC API to identify the FW there and
> expose all that stuff.
> 

Segher was playing with an OF implementation...

> Of course that wouldn't work with FW we don't have control on. Can Linux
> run on the wii with the original N. FW on the aux. processor ? Can we
> detect what is running there ? Do we care ?
> 

Yes, it can. And it is done. But I think we don't care/need that in mainline.

>> It is what Nintendo calls the "Serial Interface" (SI) which is a proprietary
>> serial hardware to drive the gamepads (and a custom keyboard too, once available
>> for an RPG game).
> 
> So I would give it a different name than "serial" then. Make it gpsi
> maybe ? (game pad serial interface ?) :-) Or invent something else...
> 

I'd choose "gcnsi" with a compatible like "nintendo,gamecube-si" :)

(gcn is the official short abbreviation for the Nintendo GameCube)

> Cheers,
> Ben.
> 

Thanks,
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 10:37 p.m.
On Thu, 2009-11-26 at 22:38 +0100, Albert Herranz wrote:

> Yup. The idea is to map the first 16MB of MEM1 with a BAT.
> Mapping the whole 24MB with BATs may not be possible because we want the framebuffer
> in MEM1 for performance reasons.

How big is the fb ? We have plenty of BATs on these things... so one 16M
for the low mem and one 64M for the "high" mem, that leaves something
like 6 to manage as much as possible up to the fb :-)

Let's keep 1 or 2 for debug and maybe even BAT map the fb itself, we
still are quite comfortable :-)

That reminds me we still need to re-introduce a clean version of
io_block_mapping (or ioremap flag that does the same) so the platform
code can establish a BAT mapping for a whole bunch of IOs and ioremap
transparenty picks addresses in there. We used to do it, dropped it
mostly because it was done badly (virtual address was hard coded which
caused all sorts of issues) but it shouldn't be hard to do it right and
it would improve perfs a little bit more.

Cheers,
Ben.
Segher Boessenkool - Nov. 26, 2009, 10:45 p.m.
>> +/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O  
>> area) */
>> +/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
>
> Weird layout... nothing you can do about I suppose.
>
> Out of curiosity, what is that DSP RAM ? Some actual DSP core  
> somewhere
> in the IO chip setup to use memory from up there ?

It's an actual DSP yes, and it seems it uses that fixed address range
(at the start of the 64MB GDDR3 memory) always.  So we have to stay
away from that memory range.

>> +	memory {
>> +		device_type = "memory";
>> +		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
>> +		reg = <0x00000000 0x133e0000>;
>> +	};
>
> So we do have a nasty hole here in the middle. How bad it is if you  
> try
> to just have two ranges in there (ie as if it was discontiguous) ?

Not bad at all.  There is no "as if" -- it _is_ discontiguous memory,
they are totally different memory technology even.

>> +		/* Team Twiizers' 'mini' firmware IPC */
>
> Out of curiosity, what are these ? :-)

There is an embedded ARM9 in the Hollywood.  It is "the boss" of the
system, not the PowerPC.  "mini" is the name of the code we run on it.

There is a hardware IPC interface between the ARM and the PowerPC (just
a bunch of doorbells and general purpose regs).

> Similar comment as before, could the above be dynamically probed ? If
> you are not a hacker you may not need that at all to use some linux
> based piece of SW on the wii right ?

Yeah.  Well, after any other drivers have been merged, anyway, heh :-)

>> +		ohci1@0d060000 {
>
> Why the "1" ?

Right, call them both just "ohci" please.  Or "usb" even.

>> +		PIC1: pic1@0d800030 {
>> +			#interrupt-cells = <1>;
>> +			compatible = "nintendo,hollywood-pic";
>> +			reg = <0x0d800030 0x8>;
>> +			interrupt-controller;
>> +			interrupts = <14>;
>> +			interrupt-parent = <&PIC0>;
>> +		};
>
> Ah, a cascaded PIC, fun fun fun

Well at least this cascaded PIC is sane, the root PIC is less so :-P

>> +		hollywood-ahbprot@0d800064 {
>> +			compatible = "nintendo,hollywood-ahbprot";
>> +			reg = <0x0d800064 0x4>;
>> +		};
>
> What is this ?

Most of the devices new in Hollywood (AES engine, NAND engine, USB,
SD, etc.) sit on an AHB bus.  The AHBPROT register is used to configure
which of these can be accessed from the PowerPC.

Modern "mini" always gives us full control.  Also, there is no reason
to single out the AHBPROT reg in the device tree like this, anyway.


Segher
Segher Boessenkool - Nov. 26, 2009, 10:50 p.m.
> BTW. If we want to play with clocks, maybe you should look at
> my proposed binding for clocks and implementing the clk API :-)

There are no clocks that are configurable from the PowerPC side
(well, maybe things like the USB clocks, but we do not know how).

>>> +		/* Team Twiizers' 'mini' firmware IPC */
>>> +		mini@0d000000 {
>>
>> Although mini is awesome, it isn't hardware, and doesn't
>> belong in the device tree.
>
> So what is at d0000000 ?

0c000000 is the flipper-compatible stuff
0d000000 is the hollywood new devices
0d800000 is the same as part of 0d000000, but with some extra bits
sometimes.

>>> +		serial@0d006400 {
>>> +			compatible = "nintendo,hollywood-serial";
>>
>> You might want to pick a more descriptive name for this,
>> "serial" is usually understaood to mean "RS232".  Which
>> this isn't.
>
> What is it then ? You definitely want some other name if it's not
> classic rs232 style serial.

It's SPI I think, with some extra signals.  It's the gamepads mostly.


Segher
Albert Herranz - Nov. 26, 2009, 11:02 p.m.
Segher Boessenkool wrote:
>> So what is at d0000000 ?
> 
> 0c000000 is the flipper-compatible stuff
> 0d000000 is the hollywood new devices
> 0d800000 is the same as part of 0d000000, but with some extra bits
> sometimes.
> 

Also, when in Wii native mode, stuff like EXI, SI and AI (which is flipper-compatible stuff too) is available at 0d000000 only.

>>>> +        serial@0d006400 {
>>>> +            compatible = "nintendo,hollywood-serial";
>>>
>>> You might want to pick a more descriptive name for this,
>>> "serial" is usually understaood to mean "RS232".  Which
>>> this isn't.
>>
>> What is it then ? You definitely want some other name if it's not
>> classic rs232 style serial.
> 
> It's SPI I think, with some extra signals.  It's the gamepads mostly.
> 

EXI is SPI.
But SI is a different thingy.

See http://www.int03.co.uk/crema/hardware/gamecube/gc-control.htm .

> 
> Segher
> 
> 

Cheers,
Albert
Segher Boessenkool - Nov. 26, 2009, 11:17 p.m.
>> They are a bit special because registers are in reverse little  
>> endian format,
>> must be written in 32-bit chunks, and (all things point to) they  
>> have hardware bugs.
>
> Well.. first what is "reverse little endian" ? :-) Big endian ?

Nah.  Little-endian, with a 32-bit bus swizzle.  This is not the
same thing as big-endian when not all your registers are 32 bits.

Also, you can only write 32 bits, they don't use byte enables.

> As for HW bugs, well, as long as we know them we can define a quirk  
> bit
> and add the necessary workarounds to the driver :-)

The question is if the device is close enough to OHCI to declare it
as that in the device tree.  I would say "yes".


Segher
Segher Boessenkool - Nov. 26, 2009, 11:25 p.m.
>> There you can find the hardware interface that supports the IPC  
>> mechanism.
>> It is made up of a pair of registers to pass data between the  
>> processors and a
>> pair of control/flags registers.
>> The hardware can interrupt the PowerPC side when there is data  
>> available for it.
>
> Ok. So the right way to do that would be to have a node purely
> representing the HW IPC, unrelated to whatever is running on the
> secondary processor.

Or you can keep it implicit in the Hollywood control registers.
It is one 512-byte block with lots of things thrown together (and
then mixed up real good).

> However, it's ok to have -below- that node, a set of device nodes or a
> node with properties or whatever representing the FW in there and the
> function it exposes.

That's not really useful though, it's easy to probe for.

> What might do however is to have a way for that FW itself to  
> provide you
> with the nodes and properties for the functions it provides :-)

You get a pointer at the very last word of memory.  It point to an
info header that has everything you need to know (most importantly,
a version identifier).

> Of course that wouldn't work with FW we don't have control on. Can  
> Linux
> run on the wii with the original N. FW on the aux. processor ?

It _can_, but there are no advantages to doing that, and lots and  
lots of
disadvantages, so the plan is to not add support for that in mainline.

> Can we
> detect what is running there ? Do we care ?

You can detect this for anything that uses a mini-compatible interface.
You shouldn't care for anything else ;-)


Segher
Benjamin Herrenschmidt - Nov. 27, 2009, 12:15 a.m.
On Fri, 2009-11-27 at 01:16 +0100, Segher Boessenkool wrote:
> 
> In all code I have done for the XFB, I map it like any other RAM and
> dcbst it after writing to it.  Maybe that is slower though, WIMG=0100
> might be better.  Dunno. 

Well, it depends also what you want to do with it. If you want to expose
it as a standard linux fbdev, it might break apps to expect them to use
dcbst... but it will make rmw cycles a lot faster and avoid wasting
memory with shadowfb.

It's going to mostly depend on what kind of HW accel we can get out of
it, if we get good enough accel we can probably got for I=1 like a
standard fb

Ben.
Segher Boessenkool - Nov. 27, 2009, 12:16 a.m.
>> Yup. The idea is to map the first 16MB of MEM1 with a BAT.
>> Mapping the whole 24MB with BATs may not be possible because we  
>> want the framebuffer
>> in MEM1 for performance reasons.
>
> How big is the fb ?

A bit more than a megabyte, something like that.

> We have plenty of BATs on these things... so one 16M
> for the low mem and one 64M for the "high" mem, that leaves something
> like 6 to manage as much as possible up to the fb :-)

The CXe in the gamecube has only four BATs though.

In all code I have done for the XFB, I map it like any other RAM and
dcbst it after writing to it.  Maybe that is slower though, WIMG=0100
might be better.  Dunno.


Segher

Patch

diff --git a/arch/powerpc/boot/dts/wii.dts b/arch/powerpc/boot/dts/wii.dts
new file mode 100644
index 0000000..a30a804
--- /dev/null
+++ b/arch/powerpc/boot/dts/wii.dts
@@ -0,0 +1,244 @@ 
+/*
+ * arch/powerpc/boot/dts/wii.dts
+ *
+ * Nintendo Wii platform device tree source
+ * Copyright (C) 2008-2009 The GameCube Linux Team
+ * Copyright (C) 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.
+ *
+ */
+
+/dts-v1/;
+
+/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */
+/memreserve/ 0x10000000 0x0004000;	/* DSP RAM */
+
+/ {
+	model = "NintendoWii";
+	compatible = "nintendo,wii";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	chosen {
+		/* root filesystem on 2nd partition of SD card */
+		bootargs = "nobats root=/dev/mmcblk0p2 rootwait udbg-immortal";
+		linux,stdout-path = &USBGECKO0;
+	};
+
+	aliases {
+		ugecon = &USBGECKO0;
+		hw_gpio = &gpio1;
+	};
+
+	/*
+	 * The Nintendo Wii has two discontiguous RAM memory areas called
+	 * MEM1 and MEM2.
+	 * MEM1 starts at 0x00000000 and contains 24MB of 1T-SRAM.
+	 * MEM2 starts at 0x10000000 and contains 64MB of DDR2 RAM.
+	 * Between both memory address ranges there is an address space
+	 * where memory-mapped I/O registers are found.
+	 *
+	 * Currently, Linux 32-bit PowerPC does not support RAM in
+	 * discontiguous memory address spaces. Thus, in order to use
+	 * both RAM areas, we declare as RAM the range from the start of
+	 * MEM1 to the end of useable MEM2 and exclude the needed parts
+	 * with /memreserve/ statements, at the expense of wasting a bit
+	 * of memory.
+	 */
+	memory {
+		device_type = "memory";
+		/* MEM1 + memory hole + MEM2 - firmware/buffers area */
+		reg = <0x00000000 0x133e0000>;
+	};
+
+	cpus {
+		#cpus = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		PowerPC,broadway@0 {
+			device_type = "cpu";
+			reg = <0>;
+			clock-frequency = <729000000>; /* 729MHz */
+			bus-frequency = <243000000>; /* 243MHz core-to-bus 3x */
+			timebase-frequency = <60750000>; /* 243MHz / 4 */
+			i-cache-line-size = <32>;
+			d-cache-line-size = <32>;
+			i-cache-size = <32768>;
+			d-cache-size = <32768>;
+		};
+	};
+
+	/* devices contained in the hollywood chipset */
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		#interrupt-cells = <1>;
+		model = "hollywood";
+		compatible = "nintendo,hollywood";
+		clock-frequency = <243000000>; /* 243MHz */
+		ranges = <0x0c000000 0x0c000000 0x00010000
+			  0x0d000000 0x0d000000 0x00010000
+			  0x0d040000 0x0d040000 0x00050000
+			  0x0d800000 0x0d800000 0x00001000
+			  0x133e0000 0x133e0000 0x00c20000>;
+
+		video@0c002000 {
+			compatible = "nintendo,hollywood-video";
+			reg = <0x0c002000 0x100>;
+			interrupts = <8>;
+			interrupt-parent = <&PIC0>;
+		};
+
+		PIC0: pic0@0c003000 {
+			#interrupt-cells = <1>;
+			compatible = "nintendo,flipper-pic";
+			reg = <0x0c003000 0x8>;
+			interrupt-controller;
+		};
+
+		resetswitch@0c003000 {
+			compatible = "nintendo,hollywood-resetswitch";
+			reg = <0x0c003000 0x4>;
+			interrupts = <1>;
+			interrupt-parent = <&PIC0>;
+		};
+
+		audio@0c005000 {
+			compatible = "nintendo,hollywood-audio";
+			reg = <0x0c005000 0x200		/* DSP */
+			       0x0d006c00 0x20>;	/* AI */
+			interrupts = <6>;
+			interrupt-parent = <&PIC0>;
+		};
+
+		/* Team Twiizers' 'mini' firmware IPC */
+		mini@0d000000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			#interrupt-cells = <1>;
+			compatible = "twiizers,starlet-mini-ipc";
+			reg = <0x0d000000 0x40	/* IPC */
+			       0x13fffffc 0x4>;	/* mini header pointer */
+		};
+
+		serial@0d006400 {
+			compatible = "nintendo,hollywood-serial";
+			reg = <0x0d006400 0x100>;
+			interrupts = <3>;
+			interrupt-parent = <&PIC0>;
+		};
+
+		/* External Interface bus */
+		exi@0d006800 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "nintendo,hollywood-exi";
+			reg = <0x0d006800 0x40>;
+			interrupts = <4>;
+			interrupt-parent = <&PIC0>;
+
+			USBGECKO0: usbgecko@0d006814 {
+				compatible = "usbgecko,usbgecko";
+				reg = <0x0d006814 0x14>;
+				virtual-reg = <0xcd006814>;
+			};
+		};
+
+		ehci@0d040000 {
+			compatible = "nintendo,hollywood-ehci";
+			reg = <0x0d040000 0x100
+			       0x133e0000 0x80000>; /* 512 KB */
+			interrupts = <4>;
+			interrupt-parent = <&PIC1>;
+		};
+
+		ohci0@0d050000 {
+			compatible = "nintendo,hollywood-ohci";
+			reg = <0x0d050000 0x100
+			       0x13460000 0x80000>; /* 512 KB */
+			interrupts = <5>;
+			interrupt-parent = <&PIC1>;
+		};
+
+		ohci1@0d060000 {
+			compatible = "nintendo,hollywood-ohci";
+			reg = <0x0d060000 0x100
+			       0x134e0000 0x80000>; /* 512 KB */
+			interrupts = <6>;
+			interrupt-parent = <&PIC1>;
+		};
+
+		sdhc0@0d070000 {
+			compatible = "nintendo,hollywood-sdhci";
+			reg = <0x0d070000 0x200>;
+			interrupts = <7>;
+			interrupt-parent = <&PIC1>;
+		};
+
+		sdhc1@0d080000 {
+			compatible = "nintendo,hollywood-sdhci";
+			reg = <0x0d080000 0x200>;
+			interrupts = <8>;
+			interrupt-parent = <&PIC1>;
+		};
+
+		PIC1: pic1@0d800030 {
+			#interrupt-cells = <1>;
+			compatible = "nintendo,hollywood-pic";
+			reg = <0x0d800030 0x8>;
+			interrupt-controller;
+			interrupts = <14>;
+			interrupt-parent = <&PIC0>;
+		};
+
+		hollywood-ahbprot@0d800064 {
+			compatible = "nintendo,hollywood-ahbprot";
+			reg = <0x0d800064 0x4>;
+		};
+
+		gpio0: hollywood-gpio@0d8000c0 {
+			compatible = "nintendo,hollywood-gpio";
+			reg = <0x0d8000c0 0x20>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio1: hollywood-gpio@0d8000e0 {
+			compatible = "nintendo,hollywood-gpio";
+			reg = <0x0d8000e0 0x20>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		hollywood-resets@0d800194 {
+			compatible = "nintendo,hollywood-resets";
+			reg = <0x0d800194 0x4>;
+		};
+
+		i2c-video {
+			#address-cells = <1>;
+			#size-cells = <0>;
+		        compatible = "virtual,i2c-gpio";
+
+		        gpios = <&gpio0  16 0 /* 31-15 */
+		                 &gpio0  17 0 /* 31-14 */
+				>;
+		        sda-is-open-drain = <1>;
+		        sda-enforce-dir = <1>;
+		        scl-is-open-drain = <1>;
+		        scl-is-output-only = <1>;
+		        udelay = <2>;
+
+		        audio-video-encoder {
+		                compatible = "nintendo,wii-ave-rvl";
+		                reg = <0x70>;
+		        };
+		};
+	};
+};
+