| Message ID | 1258927311-4340-3-git-send-email-albert_herranz@yahoo.es (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 >
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
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.
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
>> + 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
>> 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
> + 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
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
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.
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.
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.
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.
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.
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.
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.
>> 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
>>> + 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
>>> + 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
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.
>>>> 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
>>> + 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
>> 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
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>; + }; + }; + }; +}; +
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