Benjamin Herrenschmidt wrote:

On Wed, 2009-11-25 at 18:49 +0100, Segher Boessenkool wrote:

quoted

quoted

+/memreserve/ 0x01800000 0xe800000;	/* memory hole (includes I/O area) */

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

quoted

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

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

quoted

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

Question: why do you need/want nobats?

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

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

quoted

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

Just remove it?

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

quoted

quoted

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

All of 0cXXXXXX and 0dXXXXXX actually.

quoted

+			  0x133e0000 0x133e0000 0x00c20000>;

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

quoted

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

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

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

Let it be specific then :)

quoted

quoted

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

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

Add nodes for the other things ?

quoted

quoted

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

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

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

Yeah.

Agreed.

quoted

quoted

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

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

So what is at d0000000 ?

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

quoted

quoted

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

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

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

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

Cheers,
Ben.

Thanks,
Albert