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>
> ---
> 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.
> + #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.
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #interrupt-cells = <1>;
> + model = "flipper";
Drop the model property
> + 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.
> +
> + 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.
> + /* 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.
> + };
> +
> + 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>;
> + };
> + };
> + };
> +};
> +
> --
> 1.6.3.3
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
