Patchwork [RFC,02/19] powerpc: gamecube: device tree

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

Comments

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

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 arch/powerpc/boot/dts/gamecube.dts |  135 ++++++++++++++++++++++++++++++++++++
 1 files changed, 135 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/boot/dts/gamecube.dts
Grant Likely - Nov. 22, 2009, 11:02 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 GameCube video game console.
>
> Signed-off-by: Albert Herranz <albert_herranz@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.likely@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,gekko@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.

> +
> +               video@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: pic@0c003000 {
> +                       #interrupt-cells = <1>;
> +                       compatible = "nintendo,flipper-pic";
> +                       reg = <0x0c003000 0x8>;
> +                       interrupt-controller;
> +               };
> +
> +               resetswitch@0c003000 {
> +                       compatible = "nintendo,flipper-resetswitch";
> +                       reg = <0x0c003000 0x4>;
> +                       interrupts = <1>;
> +                       interrupt-parent = <&pic>;
> +               };
> +
> +               auxram@0c005000 {
> +                       compatible = "nintendo,flipper-auxram";
> +                       reg = <0x0c005000 0x200>;       /* DSP */
> +                       interrupts = <6>;
> +                       interrupt-parent = <&pic>;
> +               };
> +
> +               audio@0c005000 {
> +                       compatible = "nintendo,flipper-audio";
> +                       reg = <0x0c005000 0x200         /* DSP */
> +                              0x0c006c00 0x20>;        /* AI */
> +                       interrupts = <6>;
> +                       interrupt-parent = <&pic>;
> +               };
> +
> +               disk@0c006000 {
> +                       compatible = "nintendo,flipper-disk";
> +                       reg = <0x0c006000 0x40>;
> +                       interrupts = <2>;
> +                       interrupt-parent = <&pic>;
> +               };
> +
> +               serial@0c006400 {
> +                       compatible = "nintendo,flipper-serial";
> +                       reg = <0x0c006400 0x100>;
> +                       interrupts = <3>;
> +                       interrupt-parent = <&pic>;
> +               };
> +
> +               /* External Interface bus */
> +               exi@0c006800 {
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       compatible = "nintendo,flipper-exi";
> +                       reg = <0x0c006800 0x40>;
> +                       interrupts = <4>;
> +                       interrupt-parent = <&pic>;
> +
> +                       USBGECKO0: usbgecko@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
>
Albert Herranz - Nov. 23, 2009, 7:44 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 GameCube video game console.
>>
>> Signed-off-by: Albert Herranz <albert_herranz@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.likely@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,gekko@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.

>> +
>> +               video@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: pic@0c003000 {
>> +                       #interrupt-cells = <1>;
>> +                       compatible = "nintendo,flipper-pic";
>> +                       reg = <0x0c003000 0x8>;
>> +                       interrupt-controller;
>> +               };
>> +
>> +               resetswitch@0c003000 {
>> +                       compatible = "nintendo,flipper-resetswitch";
>> +                       reg = <0x0c003000 0x4>;
>> +                       interrupts = <1>;
>> +                       interrupt-parent = <&pic>;
>> +               };
>> +
>> +               auxram@0c005000 {
>> +                       compatible = "nintendo,flipper-auxram";
>> +                       reg = <0x0c005000 0x200>;       /* DSP */
>> +                       interrupts = <6>;
>> +                       interrupt-parent = <&pic>;
>> +               };
>> +
>> +               audio@0c005000 {
>> +                       compatible = "nintendo,flipper-audio";
>> +                       reg = <0x0c005000 0x200         /* DSP */
>> +                              0x0c006c00 0x20>;        /* AI */
>> +                       interrupts = <6>;
>> +                       interrupt-parent = <&pic>;
>> +               };
>> +
>> +               disk@0c006000 {
>> +                       compatible = "nintendo,flipper-disk";
>> +                       reg = <0x0c006000 0x40>;
>> +                       interrupts = <2>;
>> +                       interrupt-parent = <&pic>;
>> +               };
>> +
>> +               serial@0c006400 {
>> +                       compatible = "nintendo,flipper-serial";
>> +                       reg = <0x0c006400 0x100>;
>> +                       interrupts = <3>;
>> +                       interrupt-parent = <&pic>;
>> +               };
>> +
>> +               /* External Interface bus */
>> +               exi@0c006800 {
>> +                       #address-cells = <1>;
>> +                       #size-cells = <1>;
>> +                       compatible = "nintendo,flipper-exi";
>> +                       reg = <0x0c006800 0x40>;
>> +                       interrupts = <4>;
>> +                       interrupt-parent = <&pic>;
>> +
>> +                       USBGECKO0: usbgecko@0c006814 {
>> +                               compatible = "usbgecko,usbgecko";
>> +                               reg = <0x0c006814 0x14>;
>> +                               virtual-reg = <0xcc006814>;
>> +                       };
>> +               };
>> +        };
>> +};
>> +

Cheers,
Albert
Grant Likely - Nov. 23, 2009, 8:19 p.m.
On Mon, Nov 23, 2009 at 12:44 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:
>>> +       /* 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).

Right.  Much like many other SoCs.  However, the SoC has all these
devices + the cpu core + the memory controller, and probably some
other stuff.  Whereas this particular node only encapsulates the
integrated peripherals on an internal bus, so the node really is
describing the internal bus, not the entire SoC.  On some chips it is
documented as the "internally memory mapped registers", or IMMR.  So,
it is better to name this node in a way that reflects what it is (an
internal bus) instead of as the whole chip.

Similarly, it is better to use a compatible value of something like:
compatible = "nintendo,flipper-immr"; (instead of "nintendo,flipper")
because your describing just the internal bus, not the entire chip.

g.
Albert Herranz - Nov. 23, 2009, 8:25 p.m.
Grant Likely wrote:
> On Mon, Nov 23, 2009 at 12:44 PM, Albert Herranz
>> 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).
> 
> Right.  Much like many other SoCs.  However, the SoC has all these
> devices + the cpu core + the memory controller, and probably some
> other stuff.  Whereas this particular node only encapsulates the
> integrated peripherals on an internal bus, so the node really is
> describing the internal bus, not the entire SoC.  On some chips it is
> documented as the "internally memory mapped registers", or IMMR.  So,
> it is better to name this node in a way that reflects what it is (an
> internal bus) instead of as the whole chip.
> 
> Similarly, it is better to use a compatible value of something like:
> compatible = "nintendo,flipper-immr"; (instead of "nintendo,flipper")
> because your describing just the internal bus, not the entire chip.
> 
> g.
> 

Thanks for the clarification.
I'll see what I can do :)

Cheers,
Albert
Segher Boessenkool - Nov. 24, 2009, 10:36 p.m.
>> +       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.

That, however, is a) useless; and b) not totally correct.
The "model" property should show an exact model number if available.
You should use the "vendor," thing indeed.

If you don't have any better model # to use, using the same thing
as "compatible" is okay I suppose.  But it's not the _best_ thing
to put here if you have the choice.  Maybe you should say  
"nintendo,RVL-001"
(well, that's Wii, you want the similar thing for NGC, but you get
the point I hope).

>> +               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.

You could, but being explicit about the supported ranges isn't
bad either.

> 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.

If you have only one interrupt controller, like here, you don't
need to refer to it _at all_ :-)


Segher
Segher Boessenkool - Nov. 24, 2009, 10:53 p.m.
>> 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).
>
> Right.  Much like many other SoCs.

It isn't a SoC, it's really just a memory bridge / I/O bridge
like e.g. MPC10x.

All the device addresses are fixed on the chip, there is no IMMR
or similar.

You can either group all devices under a "flipper" node, or just
put the devices directly in the root node, and have a separate
node for the generic control regs.  Both are a good description
of both the physical and logical structure, so it just comes down
to taste.

Good drivers can handle either structure btw, they should normally
only look at "compatible" to find their devices, so it doesn't
matter much anyway.


Segher
Segher Boessenkool - Nov. 25, 2009, 6 p.m.
> +	memory {
> +		device_type = "memory";
> +		/* 24M minus framebuffer memory area (640*576*2*2) */
> +		reg = <0x00000000 0x01698000>;

Put the whole 24MB here, probe the framebuffer address and size
in the platform code?

> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		#interrupt-cells = <1>;

This isn't an interrupt controller, don't put #interrupt-cells
here.

> +		video@0c002000 {
> +			compatible = "nintendo,flipper-video";
> +			reg = <0x0c002000 0x100>;
> +			interrupts = <8>;
> +			interrupt-parent = <&pic>;
> +			/* XFB is the eXternal FrameBuffer */
> +			xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
> +			xfb-size = <0x168000>;

XFB address isn't fixed on the hardware, and the kernel might
want to move it, and you can easily probe for it anyway.  Remove
these last two properties please.


> +		auxram@0c005000 {
> +			compatible = "nintendo,flipper-auxram";
> +			reg = <0x0c005000 0x200>;	/* DSP */
> +			interrupts = <6>;
> +			interrupt-parent = <&pic>;
> +		};
> +
> +		audio@0c005000 {
> +			compatible = "nintendo,flipper-audio";
> +			reg = <0x0c005000 0x200		/* DSP */
> +			       0x0c006c00 0x20>;	/* AI */
> +			interrupts = <6>;
> +			interrupt-parent = <&pic>;
> +		};

These two have the same address, not good.  Just remove the
auxram node?

> +		disk@0c006000 {
> +			compatible = "nintendo,flipper-disk";

I always thought optical discs are spelled with a "c", but
people disagree a lot on this :-)

...and all the applicable things I mentioned in my Wii dev tree
reply, of course.

Wow, it wasn't as bad as I expected actually.  But you cheated,
you omitted most devices from the device trees :-)


Segher
Albert Herranz - Nov. 25, 2009, 6:43 p.m.
Segher Boessenkool wrote:
>> +	memory {
>> +		device_type = "memory";
>> +		/* 24M minus framebuffer memory area (640*576*2*2) */
>> +		reg = <0x00000000 0x01698000>;
> 
> Put the whole 24MB here, probe the framebuffer address and size
> in the platform code?
> 

Yes, I'll do that.

>> +	soc {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		#interrupt-cells = <1>;
> 
> This isn't an interrupt controller, don't put #interrupt-cells
> here.
> 

Isn't this needed to define what is to be expected in the "interrupts" properties of the child nodes?

>> +		video@0c002000 {
>> +			compatible = "nintendo,flipper-video";
>> +			reg = <0x0c002000 0x100>;
>> +			interrupts = <8>;
>> +			interrupt-parent = <&pic>;
>> +			/* XFB is the eXternal FrameBuffer */
>> +			xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
>> +			xfb-size = <0x168000>;
> 
> XFB address isn't fixed on the hardware, and the kernel might
> want to move it, and you can easily probe for it anyway.  Remove
> these last two properties please.
> 

I'll clean that too. The location of the framebuffer will be a platform code decission, instead of a device tree "setting".

> 
>> +		auxram@0c005000 {
>> +			compatible = "nintendo,flipper-auxram";
>> +			reg = <0x0c005000 0x200>;	/* DSP */
>> +			interrupts = <6>;
>> +			interrupt-parent = <&pic>;
>> +		};
>> +
>> +		audio@0c005000 {
>> +			compatible = "nintendo,flipper-audio";
>> +			reg = <0x0c005000 0x200		/* DSP */
>> +			       0x0c006c00 0x20>;	/* AI */
>> +			interrupts = <6>;
>> +			interrupt-parent = <&pic>;
>> +		};
> 
> These two have the same address, not good.  Just remove the
> auxram node?
> 

The DSP and the ARAM control/status bits share some registers in the same block.

How should I match the aram block driver if I remove the auxram node?

>> +		disk@0c006000 {
>> +			compatible = "nintendo,flipper-disk";
> 
> I always thought optical discs are spelled with a "c", but
> people disagree a lot on this :-)
> 

The "disk" here comes from "Disk Interface" (DI) as described by Nintendo patents about the GameCube.
So I chose "k" here :)

> ...and all the applicable things I mentioned in my Wii dev tree
> reply, of course.
> 
> Wow, it wasn't as bad as I expected actually.  But you cheated,
> you omitted most devices from the device trees :-)
> 

You're welcome to add them too if you have information about them :)

Thanks again,
Albert
Benjamin Herrenschmidt - Nov. 26, 2009, 4:21 a.m.
On Sun, 2009-11-22 at 16:02 -0700, Grant Likely wrote:

> > +       /* 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.

I would really just call it "flipper" :-)

> > +               #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.

On the other hand it is a useful "documentation" to specify the exact
range decoded when you know it :-) For non documented HW I prefer when
the DT contains as precise information as possible. It also allows, if
so wished, to create proper hierarchical struct resource in the kernel
that represent the bus hierarchy in a nicer way. So I vote for keeping
that in.

> > +
> > +               video@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.

Note that this is a linux-ism no ? (aka ePAPRism). If they aim toward
having a real OF which I think they do they may wish to pass on this
trick.

> > +                       /* 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 agree with using the same form as reg. I'm not sure about using "reg",
it depends. Albert, is that xfb location something that is configurable
or is it fixed ? If it's configurable, it could remain a separate
property I suppose, since it overlaps main memory, it's a bit fishy to
have it in "reg"... you do seem to strip off the fb from the memory
"reg" property though... Maybe the right thing to do is to leave the
memory "reg" property to be the whole RAM and just reserve the area
covered by the fb ?

> > +               /* External Interface bus */
> > +               exi@0c006800 {
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       compatible = "nintendo,flipper-exi";
> > +                       reg = <0x0c006800 0x40>;
> > +                       interrupts = <4>;
> > +                       interrupt-parent = <&pic>;
> > +
> > +                       USBGECKO0: usbgecko@0c006814 {
> > +                               compatible = "usbgecko,usbgecko";
> > +                               reg = <0x0c006814 0x14>;
> > +                               virtual-reg = <0xcc006814>;
> > +                       };
> > +               };
> > +        };
> > +};
> > +

Shouldn't the above be dynamically detected ?

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 4:23 a.m.
On Mon, 2009-11-23 at 13:19 -0700, Grant Likely wrote:
> so the node really is
> describing the internal bus, not the entire SoC.  On some chips it is
> documented as the "internally memory mapped registers", or IMMR.  So,
> it is better to name this node in a way that reflects what it is (an
> internal bus) instead of as the whole chip.

It's not even the internal bus. Flipper and Hollywood are separate chips
from the PPC afaik. They are integrated northbridge + gfx + IOs
basically.

> Similarly, it is better to use a compatible value of something like:
> compatible = "nintendo,flipper-immr"; (instead of "nintendo,flipper")
> because your describing just the internal bus, not the entire chip. 

I would just call the nodes "flipper" and "hollywood".

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 4:23 a.m.
On Tue, 2009-11-24 at 23:36 +0100, Segher Boessenkool wrote:
> If you have only one interrupt controller, like here, you don't
> need to refer to it _at all_ :-)

I think Linux requires that you do though. It might be a mistake on our
part but heh ...

Cheers,
Ben.
Benjamin Herrenschmidt - Nov. 26, 2009, 4:27 a.m.
On Wed, 2009-11-25 at 19:00 +0100, Segher Boessenkool wrote:
> > +	memory {
> > +		device_type = "memory";
> > +		/* 24M minus framebuffer memory area (640*576*2*2) */
> > +		reg = <0x00000000 0x01698000>;
> 
> Put the whole 24MB here, probe the framebuffer address and size
> in the platform code?

Agreed. That's what I was proposing. Though you need to be careful about
early boot code that will try to allocate the hash table etc... or even
the DT itself. So you need to probe and reserve the fb really early,
for example in the platform probe() routine itself. Or you can stick it
in the reserve map too I suppose.

> > +		video@0c002000 {
> > +			compatible = "nintendo,flipper-video";
> > +			reg = <0x0c002000 0x100>;
> > +			interrupts = <8>;
> > +			interrupt-parent = <&pic>;
> > +			/* XFB is the eXternal FrameBuffer */
> > +			xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
> > +			xfb-size = <0x168000>;
> 
> XFB address isn't fixed on the hardware, and the kernel might
> want to move it, and you can easily probe for it anyway.  Remove
> these last two properties please.

Ok but you need to know what it was setup to by the FW no ? To avoid
having a temporary display "glitch" while booting... Also on a 24M
config,it might get tough for the driver to allocate 2M contiguous like
that if it's done late in the boot process.
> 
> > +		auxram@0c005000 {
> > +			compatible = "nintendo,flipper-auxram";
> > +			reg = <0x0c005000 0x200>;	/* DSP */
> > +			interrupts = <6>;
> > +			interrupt-parent = <&pic>;
> > +		};
> > +
> > +		audio@0c005000 {
> > +			compatible = "nintendo,flipper-audio";
> > +			reg = <0x0c005000 0x200		/* DSP */
> > +			       0x0c006c00 0x20>;	/* AI */
> > +			interrupts = <6>;
> > +			interrupt-parent = <&pic>;
> > +		};
> 
> These two have the same address, not good.  Just remove the
> auxram node?

Or make it a child of audio ? :-)

Cheers,
Ben.
Grant Likely - Nov. 26, 2009, 4:38 a.m.
On Wed, Nov 25, 2009 at 9:21 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Sun, 2009-11-22 at 16:02 -0700, Grant Likely wrote:
>> > +
>> > +               video@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.
>
> Note that this is a linux-ism no ? (aka ePAPRism). If they aim toward
> having a real OF which I think they do they may wish to pass on this
> trick.

But this isn't real OF.  Real OF can generate its own valid tree.
This is a flat tree, and it is valid according to all users of the
flat tree.  Besides, the last time we talked about this, you told me
that moving it to the root was a good idea, and I happily changed all
the 5200 .dts files.  :-)

http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-January/067046.html

g.
Benjamin Herrenschmidt - Nov. 26, 2009, 5:29 a.m.
On Wed, 2009-11-25 at 21:38 -0700, Grant Likely wrote:
> 
> But this isn't real OF.  Real OF can generate its own valid tree.
> This is a flat tree, and it is valid according to all users of the
> flat tree.  Besides, the last time we talked about this, you told me
> that moving it to the root was a good idea, and I happily changed all
> the 5200 .dts files.  :-)
> 
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-January/067046.html

Oh sure. I wasn't sure what the situation vs. real OF was. I just know
Segher is doing one and it would be nice to have the closest tree :-)

But no big deal, I'm happy with it at the root. I prefer not ommiting it
completely tho.

Cheers,
Ben.
Grant Likely - Nov. 26, 2009, 5:51 a.m.
On Wed, Nov 25, 2009 at 10:29 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2009-11-25 at 21:38 -0700, Grant Likely wrote:
>>
>> But this isn't real OF.  Real OF can generate its own valid tree.
>> This is a flat tree, and it is valid according to all users of the
>> flat tree.  Besides, the last time we talked about this, you told me
>> that moving it to the root was a good idea, and I happily changed all
>> the 5200 .dts files.  :-)
>>
>> http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-January/067046.html
>
> Oh sure. I wasn't sure what the situation vs. real OF was. I just know
> Segher is doing one and it would be nice to have the closest tree :-)
>
> But no big deal, I'm happy with it at the root. I prefer not ommiting it
> completely tho.

agreed.

g.
Segher Boessenkool - Nov. 26, 2009, 10:15 p.m.
>> If you have only one interrupt controller, like here, you don't
>> need to refer to it _at all_ :-)
>
> I think Linux requires that you do though. It might be a mistake on  
> our
> part but heh ...

Linux doesn't require it; (old) Macs are like this, for example,
and that works fine.  Oh and all Maple firmwares are like that as
well I think.  Etc.


Segher
Segher Boessenkool - Nov. 26, 2009, 10:19 p.m.
>>> +			xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
>>> +			xfb-size = <0x168000>;
>>
>> XFB address isn't fixed on the hardware, and the kernel might
>> want to move it, and you can easily probe for it anyway.  Remove
>> these last two properties please.
>
> Ok but you need to know what it was setup to by the FW no ? To avoid
> having a temporary display "glitch" while booting... Also on a 24M
> config,it might get tough for the driver to allocate 2M contiguous  
> like
> that if it's done late in the boot process.

Sure, the platform code will have to probe this very early.  Can always
do the /memreserve/ if really needed.

>>> +		auxram@0c005000 {
>>> +			compatible = "nintendo,flipper-auxram";
>>> +			reg = <0x0c005000 0x200>;	/* DSP */
>>> +			interrupts = <6>;
>>> +			interrupt-parent = <&pic>;
>>> +		};
>>> +
>>> +		audio@0c005000 {
>>> +			compatible = "nintendo,flipper-audio";
>>> +			reg = <0x0c005000 0x200		/* DSP */
>>> +			       0x0c006c00 0x20>;	/* AI */
>>> +			interrupts = <6>;
>>> +			interrupt-parent = <&pic>;
>>> +		};
>>
>> These two have the same address, not good.  Just remove the
>> auxram node?
>
> Or make it a child of audio ? :-)

Hrm, actually, DSP and AI should be separate nodes, and ARAM a child
of DSP (on its own bus, so it can show that it is 16MB etc.)


Segher
Segher Boessenkool - Nov. 26, 2009, 10:30 p.m.
>>> +       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.
>
> I would really just call it "flipper" :-)

Yeah, I came to the same conclusion.

>> Since you're only doing 1:1 mappings; you could replace this with an
>> empty "ranges;" property instead.
>
> On the other hand it is a useful "documentation" to specify the exact
> range decoded when you know it :-)

Not the "decoded" range in this case, that is 0..4G :-)  The "canonical"
ranges is nice doc, yes.

>> 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.
>
> Note that this is a linux-ism no ? (aka ePAPRism).

Nope, it is from the standard interrupt mapping recommended practice.
It is fine.

> If they aim toward having a real OF which I think they do

We do.


Segher
Benjamin Herrenschmidt - Nov. 26, 2009, 10:38 p.m.
On Thu, 2009-11-26 at 23:15 +0100, Segher Boessenkool wrote:
> >> If you have only one interrupt controller, like here, you don't
> >> need to refer to it _at all_ :-)
> >
> > I think Linux requires that you do though. It might be a mistake on  
> > our
> > part but heh ...
> 
> Linux doesn't require it; (old) Macs are like this, for example,
> and that works fine.  Oh and all Maple firmwares are like that as
> well I think.  Etc.

It works fine on old macs thanks to a quirk we enable in the mac code
iirc and old maple works thanks to mere luck with the fallback code we
have which reads things of PCI config space :-)

Don't rely on that.

Cheers,
Ben.
Segher Boessenkool - Nov. 27, 2009, 12:18 a.m.
>>>> If you have only one interrupt controller, like here, you don't
>>>> need to refer to it _at all_ :-)
>>>
>>> I think Linux requires that you do though. It might be a mistake on
>>> our
>>> part but heh ...
>>
>> Linux doesn't require it; (old) Macs are like this, for example,
>> and that works fine.  Oh and all Maple firmwares are like that as
>> well I think.  Etc.
>
> It works fine on old macs thanks to a quirk we enable in the mac code
> iirc and old maple works thanks to mere luck with the fallback code we
> have which reads things of PCI config space :-)
>
> Don't rely on that.

It is standard OF.  Let's fix the Linux code if that is needed (and I  
don't
think it is, anyway).

OTOH, it's nice to use the interrupt mapping stuff in all new device  
trees.


Segher
Segher Boessenkool - Nov. 27, 2009, 12:07 p.m.
>>> +	soc {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		#interrupt-cells = <1>;
>>
>> This isn't an interrupt controller, don't put #interrupt-cells
>> here.
>
> Isn't this needed to define what is to be expected in the  
> "interrupts" properties of the child nodes?

Nope.  Only interrupt controllers have a #interrupt-cells.  When
interpreting the "interrupts" property of some node, you walk the
interrupt tree (which can be the same as the device tree, or fully
separate, or share some things; and doesn't have to be a tree either)
up to the next interrupt controller, and use the #interrupt-cells
from that.  (This is simplified a little bit).

>>> +		auxram@0c005000 {
>>> +			compatible = "nintendo,flipper-auxram";
>>> +			reg = <0x0c005000 0x200>;	/* DSP */
>>> +			interrupts = <6>;
>>> +			interrupt-parent = <&pic>;
>>> +		};
>>> +
>>> +		audio@0c005000 {
>>> +			compatible = "nintendo,flipper-audio";
>>> +			reg = <0x0c005000 0x200		/* DSP */
>>> +			       0x0c006c00 0x20>;	/* AI */
>>> +			interrupts = <6>;
>>> +			interrupt-parent = <&pic>;
>>> +		};
>>
>> These two have the same address, not good.  Just remove the
>> auxram node?
>
> The DSP and the ARAM control/status bits share some registers in  
> the same block.
>
> How should I match the aram block driver if I remove the auxram node?

You can make aram a child node of dsp, which allows you to show
its size as well.  I.e. you do #a-cells=1 #s-cells=1 in the dsp
node, and reg=<0 01000000> in the aram node.

Or you can do a simple property in the dsp node saying how big
the aram is, and assume the driver(s) that match know(s) how to
drive it.  You have to assume this anyway, you're not putting
e.g. bytecode drivers in the device tree ;-)

>> ...and all the applicable things I mentioned in my Wii dev tree
>> reply, of course.
>>
>> Wow, it wasn't as bad as I expected actually.  But you cheated,
>> you omitted most devices from the device trees :-)
>
> You're welcome to add them too if you have information about them :)

I'll do that later, yes.  It's not so big a problem if the device
tree doesn't describe devices you do not support at all.


Segher
Segher Boessenkool - Nov. 27, 2009, 12:09 p.m.
>> Similarly, it is better to use a compatible value of something like:
>> compatible = "nintendo,flipper-immr"; (instead of "nintendo,flipper")
>> because your describing just the internal bus, not the entire chip.
>
> I would just call the nodes "flipper" and "hollywood".

Either that, or not have these omnibus nodes at all, they are useless
and do not describe the hardware very well either.


Segher

Patch

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";
+	#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,gekko@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 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		#interrupt-cells = <1>;
+		model = "flipper";
+		compatible = "nintendo,flipper";
+		clock-frequency = <162000000>; /* 162MHz */
+		ranges = <0x0c000000 0x0c000000 0x00010000>;
+
+		video@0c002000 {
+			compatible = "nintendo,flipper-video";
+			reg = <0x0c002000 0x100>;
+			interrupts = <8>;
+			interrupt-parent = <&pic>;
+			/* XFB is the eXternal FrameBuffer */
+			xfb-start = <0x01698000>; /* end-of-ram - xfb-size */
+			xfb-size = <0x168000>;
+		};
+
+		pic: pic@0c003000 {
+			#interrupt-cells = <1>;
+			compatible = "nintendo,flipper-pic";
+			reg = <0x0c003000 0x8>;
+			interrupt-controller;
+		};
+
+		resetswitch@0c003000 {
+			compatible = "nintendo,flipper-resetswitch";
+			reg = <0x0c003000 0x4>;
+			interrupts = <1>;
+			interrupt-parent = <&pic>;
+		};
+
+		auxram@0c005000 {
+			compatible = "nintendo,flipper-auxram";
+			reg = <0x0c005000 0x200>;	/* DSP */
+			interrupts = <6>;
+			interrupt-parent = <&pic>;
+		};
+
+		audio@0c005000 {
+			compatible = "nintendo,flipper-audio";
+			reg = <0x0c005000 0x200		/* DSP */
+			       0x0c006c00 0x20>;	/* AI */
+			interrupts = <6>;
+			interrupt-parent = <&pic>;
+		};
+
+		disk@0c006000 {
+			compatible = "nintendo,flipper-disk";
+			reg = <0x0c006000 0x40>;
+			interrupts = <2>;
+			interrupt-parent = <&pic>;
+		};
+
+		serial@0c006400 {
+			compatible = "nintendo,flipper-serial";
+			reg = <0x0c006400 0x100>;
+			interrupts = <3>;
+			interrupt-parent = <&pic>;
+		};
+
+		/* External Interface bus */
+		exi@0c006800 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "nintendo,flipper-exi";
+			reg = <0x0c006800 0x40>;
+			interrupts = <4>;
+			interrupt-parent = <&pic>;
+
+			USBGECKO0: usbgecko@0c006814 {
+				compatible = "usbgecko,usbgecko";
+				reg = <0x0c006814 0x14>;
+				virtual-reg = <0xcc006814>;
+			};
+		};
+        };
+};
+