Grant Likely wrote:
> On Sun, Nov 22, 2009 at 3:01 PM, Albert Herranz <albert_herr...@yahoo.es>
> wrote:
>> Add a device tree source file for the Nintendo GameCube video game console.
>>
>> Signed-off-by: Albert Herranz <albert_herr...@yahoo.es>
>
> Mostly looks good. A few comments below. Biggest comment is you need
> to add a brief blurb for every new "compatible" property value that
> you've added to Documentation/powerpc/dts-bindings. General agreement
> is that we don't merge drivers with new OF tree bindings unless the
> binding is also documented. It doesn't need to be long, it just needs
> to state what the device is, and what properties are expected. If you
> define new properties, then you need to state what they are used for.
>
> Once the comments below are addressed...
>
> Acked-by: Grant Likely <grant.lik...@secretlab.ca>
>
Thanks. I'll add the documentation for the new compatible properties.
>> ---
>> arch/powerpc/boot/dts/gamecube.dts | 135
>> ++++++++++++++++++++++++++++++++++++
>> 1 files changed, 135 insertions(+), 0 deletions(-)
>> create mode 100644 arch/powerpc/boot/dts/gamecube.dts
>>
>> diff --git a/arch/powerpc/boot/dts/gamecube.dts
>> b/arch/powerpc/boot/dts/gamecube.dts
>> new file mode 100644
>> index 0000000..941a2c4
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/gamecube.dts
>> @@ -0,0 +1,135 @@
>> +/*
>> + * arch/powerpc/boot/dts/gamecube.dts
>> + *
>> + * Nintendo GameCube platform device tree source
>> + * Copyright (C) 2007-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.
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +
>> +/ {
>> + model = "NintendoGameCube";
>> + compatible = "nintendo,gamecube";
>
> To date, we've been using the same form for both the model and
> compatible properties. Specifically the <vendor>,<model> form to
> maintain the namespace.
>
Ok, I'll change model to "nintendo,gamecube".
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + chosen {
>> + bootargs = "root=/dev/gcnsda2 rootwait udbg-immortal";
>> + linux,stdout-path = &USBGECKO0;
>> + };
>> +
>> + aliases {
>> + ugecon = &USBGECKO0;
>> + };
>> +
>> + memory {
>> + device_type = "memory";
>> + /* 24M minus framebuffer memory area (640*576*2*2) */
>> + reg = <0x00000000 0x01698000>;
>> + };
>> +
>> + cpus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + PowerPC,ge...@0 {
>> + device_type = "cpu";
>> + reg = <0>;
>> + clock-frequency = <486000000>; /* 486MHz */
>> + bus-frequency = <162000000>; /* 162MHz core-to-bus
>> 3x */
>> + timebase-frequency = <40500000>; /* 162MHz / 4 */
>> + i-cache-line-size = <32>;
>> + d-cache-line-size = <32>;
>> + i-cache-size = <32768>;
>> + d-cache-size = <32768>;
>> + };
>> + };
>> +
>> + /* devices contained int the flipper chipset */
>> + soc {
>
> It would be better to rename this as IMMR or the bus type. This node
> doesn't actually describe the entire chip, but describes the internal
> memory mapped registers.
>
Can you please elaborate more on this or point me to documentation?
The soc node here tries to represent the big multi-function chip that
integrates most of the devices of the video game consoles ("Flipper" on the
Nintendo GameCube and "Hollywood" on the Wii).
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + #interrupt-cells = <1>;
>> + model = "flipper";
>
> Drop the model property
>
Ok, I'll fix that.
>> + compatible = "nintendo,flipper";
>> + clock-frequency = <162000000>; /* 162MHz */
>> + ranges = <0x0c000000 0x0c000000 0x00010000>;
>
> Since you're only doing 1:1 mappings; you could replace this with an
> empty "ranges;" property instead.
>
Nice, didn't know about this. I'll do that.
>> +
>> + vi...@0c002000 {
>> + compatible = "nintendo,flipper-video";
>> + reg = <0x0c002000 0x100>;
>> + interrupts = <8>;
>> + interrupt-parent = <&pic>;
>
> Hint: If you move the interrupt-parent property up to the root node,
> then you don't need to specify it in every single device node; it will
> just inherit from the parent.
>
Got it. This makes a lot of sense here with a single interrupt controller.
>> + /* XFB is the eXternal FrameBuffer */
>> + xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
>> + xfb-size = <0x168000>;
>
> Can 'xfb' be made a second tuple to the 'reg' property so that all the
> address mapping code works on it? ie:
>
> reg = <0x0c002000 0x100
> 0x01698000 0x168000>;
>
> At the very least, it is wise to adopt the same form as the reg
> property when describing address ranges instead of splitting it into
> multiple properties.
>
I'll look into moving xfb-* into the reg property.
>> + };
>> +
>> + pic: p...@0c003000 {
>> + #interrupt-cells = <1>;
>> + compatible = "nintendo,flipper-pic";
>> + reg = <0x0c003000 0x8>;
>> + interrupt-controller;
>> + };
>> +
>> + resetswi...@0c003000 {
>> + compatible = "nintendo,flipper-resetswitch";
>> + reg = <0x0c003000 0x4>;
>> + interrupts = <1>;
>> + interrupt-parent = <&pic>;
>> + };
>> +
>> + aux...@0c005000 {
>> + compatible = "nintendo,flipper-auxram";
>> + reg = <0x0c005000 0x200>; /* DSP */
>> + interrupts = <6>;
>> + interrupt-parent = <&pic>;
>> + };
>> +
>> + au...@0c005000 {
>> + compatible = "nintendo,flipper-audio";
>> + reg = <0x0c005000 0x200 /* DSP */
>> + 0x0c006c00 0x20>; /* AI */
>> + interrupts = <6>;
>> + interrupt-parent = <&pic>;
>> + };
>> +
>> + d...@0c006000 {
>> + compatible = "nintendo,flipper-disk";
>> + reg = <0x0c006000 0x40>;
>> + interrupts = <2>;
>> + interrupt-parent = <&pic>;
>> + };
>> +
>> + ser...@0c006400 {
>> + compatible = "nintendo,flipper-serial";
>> + reg = <0x0c006400 0x100>;
>> + interrupts = <3>;
>> + interrupt-parent = <&pic>;
>> + };
>> +
>> + /* External Interface bus */
>> + e...@0c006800 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "nintendo,flipper-exi";
>> + reg = <0x0c006800 0x40>;
>> + interrupts = <4>;
>> + interrupt-parent = <&pic>;
>> +
>> + USBGECKO0: usbge...@0c006814 {
>> + compatible = "usbgecko,usbgecko";
>> + reg = <0x0c006814 0x14>;
>> + virtual-reg = <0xcc006814>;
>> + };
>> + };
>> + };
>> +};
>> +
Cheers,
Albert
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
