Small fixes for the Ebony device tree

Small fixes for the Ebony device tree

[David Gibson]

This patch corrects a number of minor errors in the Ebony device tree:
	- Missing (given as 0) cache sizes are added to the CPU node
	- device_type properties are removed from nodes which don't
have a reasonably well defined device_type binding.  This does require
a very small code change to locate the busses to be probed for
of_platform devices by 'compatible' instead of 'device_type'.
	- A node is added for the SRAM controller
	- The unit address of the small-flash node is adjusted to
correctly reflect the reg property.
	- device_type values for the MAL and ZMII are updated to
reflected more up-to-date versions of the binding.
	- An incorrect offset in the partition map for the large-flash
node is corrected.
	- Some redundant values, already commented out are removed
entirely.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---

Revised based on feedback from Segher et al.

Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-05-15 11:16:28.000000000 +1000
@@ -33,8 +33,8 @@
 			timebase-frequency = <0>; // Filled in by zImage
 			i-cache-line-size = <32>;
 			d-cache-line-size = <32>;
-			i-cache-size = <0>;
-			d-cache-size = <0>;
+			i-cache-size = <8000>; /* 32 kB */
+			d-cache-size = <8000>; /* 32 kB */
 			dcr-controller;
 			dcr-access-method = "native";
 		};
@@ -46,7 +46,6 @@
 	};
 
 	UIC0: interrupt-controller0 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <0>;
@@ -58,7 +57,6 @@
 	};
 
 	UIC1: interrupt-controller1 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <1>;
@@ -71,36 +69,37 @@
 	};
 
 	CPC0: cpc {
-		device_type = "ibm,cpc";
 		compatible = "ibm,cpc-440gp";
 		dcr-reg = <0b0 003 0e0 010>;
 		// FIXME: anything else?
 	};
 
 	plb {
-		device_type = "ibm,plb";
 		compatible = "ibm,plb-440gp", "ibm,plb4";
 		#address-cells = <2>;
 		#size-cells = <1>;
 		ranges;
 		clock-frequency = <0>; // Filled in by zImage
 
-		SDRAM0: sdram {
-			device_type = "memory-controller";
+		SDRAM0: memory-controller {
 			compatible = "ibm,sdram-440gp", "ibm,sdram";
 			dcr-reg = <010 2>;
 			// FIXME: anything else?
 		};
 
+		SRAM0: sram {
+			compatible = "ibm,sram440gp";
+			dcr-reg = <020 8 00a 1>;
+		};
+
 		DMA0: dma {
 			// FIXME: ???
-			device_type = "ibm,dma-4xx";
 			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
 			dcr-reg = <100 027>;
 		};
 
 		MAL0: mcmal {
-			device_type = "mcmal-dma";
+			device_type = "dma-controller";
 			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
 			dcr-reg = <180 62>;
 			num-tx-chans = <4>;
@@ -119,7 +118,6 @@
 		};
 
 		POB0: opb {
-			device_type = "ibm,opb";
 			compatible = "ibm,opb-440gp", "ibm,opb";
 			#address-cells = <1>;
 			#size-cells = <1>;
@@ -133,8 +131,7 @@
 			clock-frequency = <0>; // Filled in by zImage
 
 			EBC0: ebc {
-				device_type = "ibm,ebc";
-				compatible = "ibm,ebc-440gp";
+				compatible = "ibm,ebc-440gp", "ibm,ebc";
 				dcr-reg = <012 2>;
 				#address-cells = <2>;
 				#size-cells = <1>;
@@ -147,7 +144,7 @@
 				interrupts = <5 4>;
 				interrupt-parent = <&UIC1>;
 
-				small-flash@0,0 {
+				small-flash@0,80000 {
 					device_type = "rom";
 					compatible = "direct-mapped";
 					probe-type = "JEDEC";
@@ -159,7 +156,6 @@
 
 				ds1743@1,0 {
 					/* NVRAM & RTC */
-					device_type = "nvram";
 					compatible = "ds1743";
 					reg = <1 0 2000>;
 				};
@@ -170,7 +166,7 @@
 					probe-type = "JEDEC";
 					bank-width = <1>;
 					partitions = <0 380000
-						      280000 80000>;
+						      380000 80000>;
 					partition-names = "fs", "firmware";
 					reg = <2 0 400000>;
 				};
@@ -226,13 +222,12 @@
 
 			GPIO0: gpio@40000700 {
 				/* FIXME */
-				device_type = "gpio";
 				compatible = "ibm,gpio-440gp";
 				reg = <40000700 20>;
 			};
 
 			ZMII0: emac-zmii@40000780 {
-				device_type = "emac-zmii";
+				device_type = "zmii-interface";
 				compatible = "ibm,zmii-440gp", "ibm,zmii";
 				reg = <40000780 c>;
 			};
@@ -299,9 +294,5 @@
 
 	chosen {
 		linux,stdout-path = "/plb/opb/serial@40000200";
-//		linux,initrd-start = <0>; /* FIXME */
-//		linux,initrd-end = <0>;
-//		bootargs = "";
 	};
 };
-
Index: working-2.6/arch/powerpc/platforms/44x/ebony.c
===================================================================
--- working-2.6.orig/arch/powerpc/platforms/44x/ebony.c	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/platforms/44x/ebony.c	2007-05-15 11:12:15.000000000 +1000
@@ -27,9 +27,9 @@
 #include "44x.h"
 
 static struct of_device_id ebony_of_bus[] = {
-	{ .type = "ibm,plb", },
-	{ .type = "ibm,opb", },
-	{ .type = "ibm,ebc", },
+	{ .compatible = "ibm,plb", },
+	{ .compatible = "ibm,opb", },
+	{ .compatible = "ibm,ebc", },
 	{},
 };
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

Just a few more things...

>  	plb {
> -		device_type = "ibm,plb";
>  		compatible = "ibm,plb-440gp", "ibm,plb4";

You match on "ibm,plb" though.  Well you got one
in three right ;-)

> +		SDRAM0: memory-controller {
>  			compatible = "ibm,sdram-440gp", "ibm,sdram";

"ibm,sdram" is way too generic to ever be usefully
used for device matching.

> +		SRAM0: sram {
> +			compatible = "ibm,sram440gp";

You spell this one without a dash, all others with one.


Segher

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 07:11:07AM +0200, Segher Boessenkool wrote:
> Just a few more things...
> 
> >  	plb {
> > -		device_type = "ibm,plb";
> >  		compatible = "ibm,plb-440gp", "ibm,plb4";
> 
> You match on "ibm,plb" though.  Well you got one
> in three right ;-)

Indeed.  And I'm still not sure how it's recognizing the plb devices
right now...

> > +		SDRAM0: memory-controller {
> >  			compatible = "ibm,sdram-440gp", "ibm,sdram";
> 
> "ibm,sdram" is way too generic to ever be usefully
> used for device matching.

Yeah, I guess this should be ibm,sdram-44x or ibm,sdram-4xx.  Better
go look up if any 40x chips use a similar sdram controller.

> > +		SRAM0: sram {
> > +			compatible = "ibm,sram440gp";
> 
> You spell this one without a dash, all others with one.

Ah, yes, oops...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Stefan Roese]

On Tuesday 15 May 2007, David Gibson wrote:
> > "ibm,sdram" is way too generic to ever be usefully
> > used for device matching.
>
> Yeah, I guess this should be ibm,sdram-44x or ibm,sdram-4xx.  Better
> go look up if any 40x chips use a similar sdram controller.

Here a list of the current 4xx SDRAM controllers:

SDRAM:		all 40x (except for 405EZ)
DDR:		440GP, 440GX, 440EP, 440GR
DDR2:		440SP, 440SPe
DDR2 (Denali):	440EPx, 440GRx

Best regards,
Stefan

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 08:11:09AM +0200, Stefan Roese wrote:
> On Tuesday 15 May 2007, David Gibson wrote:
> > > "ibm,sdram" is way too generic to ever be usefully
> > > used for device matching.
> >
> > Yeah, I guess this should be ibm,sdram-44x or ibm,sdram-4xx.  Better
> > go look up if any 40x chips use a similar sdram controller.
> 
> Here a list of the current 4xx SDRAM controllers:

Ah, thanks :).

Anyone have any objection to the following:  each of these controllers
should have compatible properties listing 3 string, first, a specific
version for the CPU in question (e.g "ibm,sdram-440gp"), then a string
based on the type as listed below, finally "ibm,sdram-4xx".

> SDRAM:		all 40x (except for 405EZ)
	ibm,sdram-40x

> DDR:		440GP, 440GX, 440EP, 440GR
	ibm,sdram-44x-ddr

> DDR2:		440SP, 440SPe
	ibm,sdram-44x-ddr2

> DDR2 (Denali):	440EPx, 440GRx
	ibm,sdram-44x-ddr2denali

Seem reasonable?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

> Anyone have any objection to the following:  each of these controllers
> should have compatible properties listing 3 string, first, a specific
> version for the CPU in question (e.g "ibm,sdram-440gp"), then a string
> based on the type as listed below, finally "ibm,sdram-4xx".

Is that last entry ever useful?  Do all those controllers
have something in common?

>> SDRAM:		all 40x (except for 405EZ)
> 	ibm,sdram-40x
>
>> DDR:		440GP, 440GX, 440EP, 440GR
> 	ibm,sdram-44x-ddr
>
>> DDR2:		440SP, 440SPe
> 	ibm,sdram-44x-ddr2
>
>> DDR2 (Denali):	440EPx, 440GRx
> 	ibm,sdram-44x-ddr2denali

Is this one a superset of the previous one?  If so,
put both names in.

> Seem reasonable?

It's better to say 440 than 44x, maybe all "441" parts
will be totally different.  I don't know what to do
with 40x; maybe say 405 as that seems to be the most
common chip.  OTOH, you don't have to deal with that
right now :-)


Segher

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 08:28:09AM +0200, Segher Boessenkool wrote:
> > Anyone have any objection to the following:  each of these controllers
> > should have compatible properties listing 3 string, first, a specific
> > version for the CPU in question (e.g "ibm,sdram-440gp"), then a string
> > based on the type as listed below, finally "ibm,sdram-4xx".
> 
> Is that last entry ever useful?  Do all those controllers
> have something in common?

A quick glance over the descriptions in the 440GP and 405GP user
manuals suggests that they have most of the registers in common,
though I haven't checked to see if the semantics of those registers
are actually all the same.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

>>> finally "ibm,sdram-4xx".
>>
>> Is that last entry ever useful?  Do all those controllers
>> have something in common?
>
> A quick glance over the descriptions in the 440GP and 405GP user
> manuals suggests that they have most of the registers in common,
> though I haven't checked to see if the semantics of those registers
> are actually all the same.

Let me rephrase: can a kernel driver ever do anything
useful with the device if it _only_ knows it is some
4xx memory controller?

If that isn't the case, but for example, the 440GP's
controller can be driven by the 405GP driver, you
can express exactly that in the "compatible" property.
It is a bad plan in general to say "4xx", it suggests
compatibility both ways, which almost never is true;
so what _does_ it mean?  Lowest common denominator?
Say exactly that, then.  "compatible" is meant for
drivers to figure if they can drive the device (or,
equivalently, for the kernel to figure out which driver
can drive a given device).


Segher

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 08:47:00AM +0200, Segher Boessenkool wrote:
> >>> finally "ibm,sdram-4xx".
> >>
> >> Is that last entry ever useful?  Do all those controllers
> >> have something in common?
> >
> > A quick glance over the descriptions in the 440GP and 405GP user
> > manuals suggests that they have most of the registers in common,
> > though I haven't checked to see if the semantics of those registers
> > are actually all the same.
> 
> Let me rephrase: can a kernel driver ever do anything
> useful with the device if it _only_ knows it is some
> 4xx memory controller?
> 
> If that isn't the case, but for example, the 440GP's
> controller can be driven by the 405GP driver, you
> can express exactly that in the "compatible" property.
> It is a bad plan in general to say "4xx", it suggests
> compatibility both ways, which almost never is true;
> so what _does_ it mean?  Lowest common denominator?
> Say exactly that, then.  "compatible" is meant for
> drivers to figure if they can drive the device (or,
> equivalently, for the kernel to figure out which driver
> can drive a given device).

Ok, I see what you mean.

Sod it, I think I'll just drop everything except "ibm,sdram-440gp" for
now.  The one place we access the SDRAM controller, it's under
platform control anyway we do it via direct DCR access anyway, so the
compatible property is pretty much irrelevant.

If a clearer pattern emerges later, we can add more compatible
strings.  If some later driver has to have an extra backwards
compatibility id_table entry for the Ebony/440GP it's no great
disaster.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

> Sod it, I think I'll just drop everything except "ibm,sdram-440gp" for
> now.

Perfect plan :-)  And then 440GX, GR, etc. can claim
compatibility to 440GP, which is the nestor of the
family.

> If some later driver has to have an extra backwards
> compatibility id_table entry for the Ebony/440GP it's no great
> disaster.

Sure, mistakes happen.  They can be fixed in the
bootwrapper, too -- or just in the DTS file if your
device tree is shipped with the kernel.


Segher

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 09:07:52AM +0200, Segher Boessenkool wrote:
> > Sod it, I think I'll just drop everything except "ibm,sdram-440gp" for
> > now.
> 
> Perfect plan :-)  And then 440GX, GR, etc. can claim
> compatibility to 440GP, which is the nestor of the
> family.

Right, done.  Same for the SRAM and DMA controller nodes.

New patch coming shortly.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Stefan Roese]

On Tuesday 15 May 2007, David Gibson wrote:
> Anyone have any objection to the following:  each of these controllers
> should have compatible properties listing 3 string, first, a specific
> version for the CPU in question (e.g "ibm,sdram-440gp"), then a string
> based on the type as listed below, finally "ibm,sdram-4xx".
>
> > SDRAM:		all 40x (except for 405EZ)
>
> 	ibm,sdram-40x
>
> > DDR:		440GP, 440GX, 440EP, 440GR
>
> 	ibm,sdram-44x-ddr
>
> > DDR2:		440SP, 440SPe
>
> 	ibm,sdram-44x-ddr2
>
> > DDR2 (Denali):	440EPx, 440GRx
>
> 	ibm,sdram-44x-ddr2denali

Why "ibm,xxx"? Is this for the CPU core? Or for the SDRAM controller core? 
Both are not IBM in this case.

> Seem reasonable?

I forgot the new 405EX which will be equipped with a DDR2 controller (not sure 
right now if it's the Denali or the other core right now). So we should 
probably drop this 40x/44x completely. Does this make sense?

Best regards,
Stefan

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

> Why "ibm,xxx"? Is this for the CPU core? Or for the SDRAM controller 
> core?

For whatever is behind the comma.  The prefixes are
meant to give each manufacturer his own namespace,
to reduce the number of name collisions.

> Both are not IBM in this case.

Yeah.

> I forgot the new 405EX which will be equipped with a DDR2 controller 
> (not sure
> right now if it's the Denali or the other core right now). So we should
> probably drop this 40x/44x completely. Does this make sense?

Yes.  To claim compatibility to a family of devices,
specify the oldest part, or the most popular part,
or a few parts if different generations added more
features (and this part is compatible with those).


Segher

Small fixes for the Ebony device tree

[David Gibson]

This patch corrects a number of minor errors in the Ebony device tree:
	- Missing (given as 0) cache sizes are added to the CPU node
	- device_type properties are removed from nodes which don't
have a reasonably well defined device_type binding.  This does require
a very small code change to locate the busses to be probed for
of_platform devices by 'compatible' instead of 'device_type'.
	- A node is added for the SRAM controller
	- The unit address of the small-flash node is adjusted to
correctly reflect the reg property.
	- device_type values for the MAL and ZMII are updated to
reflected more up-to-date versions of the binding.
	- An incorrect offset in the partition map for the large-flash
node is corrected.
	- Some redundant values, already commented out are removed
entirely.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
	Further revisions based on feedback.
	Note that for EMAC to work with this device tree, you need the
EMAC driver patch which I posted today (20070516).

Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-05-16 13:44:10.000000000 +1000
@@ -33,8 +33,8 @@
 			timebase-frequency = <0>; // Filled in by zImage
 			i-cache-line-size = <32>;
 			d-cache-line-size = <32>;
-			i-cache-size = <0>;
-			d-cache-size = <0>;
+			i-cache-size = <8000>; /* 32 kB */
+			d-cache-size = <8000>; /* 32 kB */
 			dcr-controller;
 			dcr-access-method = "native";
 		};
@@ -46,7 +46,6 @@
 	};
 
 	UIC0: interrupt-controller0 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <0>;
@@ -58,7 +57,6 @@
 	};
 
 	UIC1: interrupt-controller1 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <1>;
@@ -71,36 +69,36 @@
 	};
 
 	CPC0: cpc {
-		device_type = "ibm,cpc";
 		compatible = "ibm,cpc-440gp";
 		dcr-reg = <0b0 003 0e0 010>;
 		// FIXME: anything else?
 	};
 
 	plb {
-		device_type = "ibm,plb";
 		compatible = "ibm,plb-440gp", "ibm,plb4";
 		#address-cells = <2>;
 		#size-cells = <1>;
 		ranges;
 		clock-frequency = <0>; // Filled in by zImage
 
-		SDRAM0: sdram {
-			device_type = "memory-controller";
-			compatible = "ibm,sdram-440gp", "ibm,sdram";
+		SDRAM0: memory-controller {
+			compatible = "ibm,sdram-440gp";
 			dcr-reg = <010 2>;
 			// FIXME: anything else?
 		};
 
+		SRAM0: sram {
+			compatible = "ibm,sram-440gp";
+			dcr-reg = <020 8 00a 1>;
+		};
+
 		DMA0: dma {
 			// FIXME: ???
-			device_type = "ibm,dma-4xx";
-			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
+			compatible = "ibm,dma-440gp";
 			dcr-reg = <100 027>;
 		};
 
 		MAL0: mcmal {
-			device_type = "mcmal-dma";
 			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
 			dcr-reg = <180 62>;
 			num-tx-chans = <4>;
@@ -119,7 +117,6 @@
 		};
 
 		POB0: opb {
-			device_type = "ibm,opb";
 			compatible = "ibm,opb-440gp", "ibm,opb";
 			#address-cells = <1>;
 			#size-cells = <1>;
@@ -133,8 +130,7 @@
 			clock-frequency = <0>; // Filled in by zImage
 
 			EBC0: ebc {
-				device_type = "ibm,ebc";
-				compatible = "ibm,ebc-440gp";
+				compatible = "ibm,ebc-440gp", "ibm,ebc";
 				dcr-reg = <012 2>;
 				#address-cells = <2>;
 				#size-cells = <1>;
@@ -147,7 +143,7 @@
 				interrupts = <5 4>;
 				interrupt-parent = <&UIC1>;
 
-				small-flash@0,0 {
+				small-flash@0,80000 {
 					device_type = "rom";
 					compatible = "direct-mapped";
 					probe-type = "JEDEC";
@@ -159,7 +155,6 @@
 
 				ds1743@1,0 {
 					/* NVRAM & RTC */
-					device_type = "nvram";
 					compatible = "ds1743";
 					reg = <1 0 2000>;
 				};
@@ -170,7 +165,7 @@
 					probe-type = "JEDEC";
 					bank-width = <1>;
 					partitions = <0 380000
-						      280000 80000>;
+						      380000 80000>;
 					partition-names = "fs", "firmware";
 					reg = <2 0 400000>;
 				};
@@ -226,13 +221,11 @@
 
 			GPIO0: gpio@40000700 {
 				/* FIXME */
-				device_type = "gpio";
 				compatible = "ibm,gpio-440gp";
 				reg = <40000700 20>;
 			};
 
 			ZMII0: emac-zmii@40000780 {
-				device_type = "emac-zmii";
 				compatible = "ibm,zmii-440gp", "ibm,zmii";
 				reg = <40000780 c>;
 			};
@@ -299,9 +292,5 @@
 
 	chosen {
 		linux,stdout-path = "/plb/opb/serial@40000200";
-//		linux,initrd-start = <0>; /* FIXME */
-//		linux,initrd-end = <0>;
-//		bootargs = "";
 	};
 };
-
Index: working-2.6/arch/powerpc/platforms/44x/ebony.c
===================================================================
--- working-2.6.orig/arch/powerpc/platforms/44x/ebony.c	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/platforms/44x/ebony.c	2007-05-16 13:27:11.000000000 +1000
@@ -27,9 +27,9 @@
 #include "44x.h"
 
 static struct of_device_id ebony_of_bus[] = {
-	{ .type = "ibm,plb", },
-	{ .type = "ibm,opb", },
-	{ .type = "ibm,ebc", },
+	{ .compatible = "ibm,plb4", },
+	{ .compatible = "ibm,opb", },
+	{ .compatible = "ibm,ebc", },
 	{},
 };
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

> This patch corrects a number of minor errors in the Ebony device tree:
> 	- Missing (given as 0) cache sizes are added to the CPU node
> 	- device_type properties are removed from nodes which don't
> have a reasonably well defined device_type binding.  This does require
> a very small code change to locate the busses to be probed for
> of_platform devices by 'compatible' instead of 'device_type'.
> 	- A node is added for the SRAM controller
> 	- The unit address of the small-flash node is adjusted to
> correctly reflect the reg property.
> 	- device_type values for the MAL and ZMII are updated to
> reflected more up-to-date versions of the binding.
> 	- An incorrect offset in the partition map for the large-flash
> node is corrected.
> 	- Some redundant values, already commented out are removed
> entirely.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Segher Boessenkool <segher@kernel.crashing.org>


Segher

Small fixes for the Ebony device tree

[David Gibson]

This patch corrects a number of minor errors in the Ebony device tree:
	- Missing (given as 0) cache sizes are added to the CPU node
	- device_type properties are removed from nodes which don't
have a reasonably well defined device_type binding.  This does require
a very small code change to locate the busses to be probed for
of_platform devices by 'compatible' instead of 'device_type'.
	- A node is added for the SRAM controller
	- The unit address of the small-flash node is adjusted to
correctly reflect the reg property.
	- device_type values for the MAL and ZMII are updated to
reflected more up-to-date versions of the binding.
	- An incorrect offset in the partition map for the large-flash
node is corrected.
	- Some redundant values, already commented out are removed
entirely.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---

The flash partition offset correction, at least, should go into
2.6.22.  I think the rest while only borderline "bugfixes" is also
reasonable for inclusion in 2.6.22.

Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-05-14 14:38:39.000000000 +1000
@@ -33,8 +33,8 @@
 			timebase-frequency = <0>; // Filled in by zImage
 			i-cache-line-size = <32>;
 			d-cache-line-size = <32>;
-			i-cache-size = <0>;
-			d-cache-size = <0>;
+			i-cache-size = <2000000>; /* 32 kB */
+			d-cache-size = <2000000>; /* 32 kB */
 			dcr-controller;
 			dcr-access-method = "native";
 		};
@@ -46,7 +46,6 @@
 	};
 
 	UIC0: interrupt-controller0 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <0>;
@@ -58,7 +57,6 @@
 	};
 
 	UIC1: interrupt-controller1 {
-		device_type = "ibm,uic";
 		compatible = "ibm,uic-440gp", "ibm,uic";
 		interrupt-controller;
 		cell-index = <1>;
@@ -71,14 +69,12 @@
 	};
 
 	CPC0: cpc {
-		device_type = "ibm,cpc";
 		compatible = "ibm,cpc-440gp";
 		dcr-reg = <0b0 003 0e0 010>;
 		// FIXME: anything else?
 	};
 
 	plb {
-		device_type = "ibm,plb";
 		compatible = "ibm,plb-440gp", "ibm,plb4";
 		#address-cells = <2>;
 		#size-cells = <1>;
@@ -86,21 +82,24 @@
 		clock-frequency = <0>; // Filled in by zImage
 
 		SDRAM0: sdram {
-			device_type = "memory-controller";
 			compatible = "ibm,sdram-440gp", "ibm,sdram";
 			dcr-reg = <010 2>;
 			// FIXME: anything else?
 		};
 
+		SRAM0: sram {
+			compatible = "ibm,sram440gp";
+			dcr-reg = <020 8 00a 1>;
+		};
+
 		DMA0: dma {
 			// FIXME: ???
-			device_type = "ibm,dma-4xx";
 			compatible = "ibm,dma-440gp", "ibm,dma-4xx";
 			dcr-reg = <100 027>;
 		};
 
 		MAL0: mcmal {
-			device_type = "mcmal-dma";
+			device_type = "dma-controller";
 			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
 			dcr-reg = <180 62>;
 			num-tx-chans = <4>;
@@ -119,7 +118,6 @@
 		};
 
 		POB0: opb {
-			device_type = "ibm,opb";
 			compatible = "ibm,opb-440gp", "ibm,opb";
 			#address-cells = <1>;
 			#size-cells = <1>;
@@ -133,7 +131,6 @@
 			clock-frequency = <0>; // Filled in by zImage
 
 			EBC0: ebc {
-				device_type = "ibm,ebc";
 				compatible = "ibm,ebc-440gp";
 				dcr-reg = <012 2>;
 				#address-cells = <2>;
@@ -147,7 +144,7 @@
 				interrupts = <5 4>;
 				interrupt-parent = <&UIC1>;
 
-				small-flash@0,0 {
+				small-flash@0,80000 {
 					device_type = "rom";
 					compatible = "direct-mapped";
 					probe-type = "JEDEC";
@@ -159,7 +156,6 @@
 
 				ds1743@1,0 {
 					/* NVRAM & RTC */
-					device_type = "nvram";
 					compatible = "ds1743";
 					reg = <1 0 2000>;
 				};
@@ -170,7 +166,7 @@
 					probe-type = "JEDEC";
 					bank-width = <1>;
 					partitions = <0 380000
-						      280000 80000>;
+						      380000 80000>;
 					partition-names = "fs", "firmware";
 					reg = <2 0 400000>;
 				};
@@ -226,13 +222,12 @@
 
 			GPIO0: gpio@40000700 {
 				/* FIXME */
-				device_type = "gpio";
 				compatible = "ibm,gpio-440gp";
 				reg = <40000700 20>;
 			};
 
 			ZMII0: emac-zmii@40000780 {
-				device_type = "emac-zmii";
+				device_type = "zmii-interface";
 				compatible = "ibm,zmii-440gp", "ibm,zmii";
 				reg = <40000780 c>;
 			};
@@ -299,9 +294,5 @@
 
 	chosen {
 		linux,stdout-path = "/plb/opb/serial@40000200";
-//		linux,initrd-start = <0>; /* FIXME */
-//		linux,initrd-end = <0>;
-//		bootargs = "";
 	};
 };
-
Index: working-2.6/arch/powerpc/platforms/44x/ebony.c
===================================================================
--- working-2.6.orig/arch/powerpc/platforms/44x/ebony.c	2007-05-08 15:07:45.000000000 +1000
+++ working-2.6/arch/powerpc/platforms/44x/ebony.c	2007-05-14 14:37:51.000000000 +1000
@@ -27,9 +27,9 @@
 #include "44x.h"
 
 static struct of_device_id ebony_of_bus[] = {
-	{ .type = "ibm,plb", },
-	{ .type = "ibm,opb", },
-	{ .type = "ibm,ebc", },
+	{ .compatible = "ibm,plb", },
+	{ .compatible = "ibm,opb", },
+	{ .compatible = "ibm,ebc", },
 	{},
 };
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Josh Boyer]

On Mon, 2007-05-14 at 14:54 +1000, David Gibson wrote:
> This patch corrects a number of minor errors in the Ebony device tree:
> 	- Missing (given as 0) cache sizes are added to the CPU node
> 	- device_type properties are removed from nodes which don't
> have a reasonably well defined device_type binding.  This does require
> a very small code change to locate the busses to be probed for
> of_platform devices by 'compatible' instead of 'device_type'.
> 	- A node is added for the SRAM controller
> 	- The unit address of the small-flash node is adjusted to
> correctly reflect the reg property.
> 	- device_type values for the MAL and ZMII are updated to
> reflected more up-to-date versions of the binding.
> 	- An incorrect offset in the partition map for the large-flash
> node is corrected.
> 	- Some redundant values, already commented out are removed
> entirely.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

> -			i-cache-size = <0>;
> -			d-cache-size = <0>;
> +			i-cache-size = <2000000>; /* 32 kB */
> +			d-cache-size = <2000000>; /* 32 kB */

That's 32MB, not 32kB.  Better fix this :-)

>  	UIC0: interrupt-controller0 {

>  	UIC1: interrupt-controller1 {

It's a shame you can't use unit addresses for these since
you use "dcr-reg" instead of "reg".  Oh well.

>  		SDRAM0: sdram {
> -			device_type = "memory-controller";
>  			compatible = "ibm,sdram-440gp", "ibm,sdram";

Maybe rename the node to "memory-controller"?

> +		SRAM0: sram {
> +			compatible = "ibm,sram440gp";
> +			dcr-reg = <020 8 00a 1>;
> +		};

Is this thing _only_ addressable over DCRs?  Weird.

>  		MAL0: mcmal {
> -			device_type = "mcmal-dma";
> +			device_type = "dma-controller";
>  			compatible = "ibm,mcmal-440gp", "ibm,mcmal";

Remove "device_type", change name to "dma-controller"?

>  			EBC0: ebc {
> -				device_type = "ibm,ebc";
>  				compatible = "ibm,ebc-440gp";

You forgot "ibm,ebc" here.


Segher

Re: Small fixes for the Ebony device tree

[Josh Boyer]

On Mon, 2007-05-14 at 14:59 +0200, Segher Boessenkool wrote:
> > -			i-cache-size = <0>;
> > -			d-cache-size = <0>;
> > +			i-cache-size = <2000000>; /* 32 kB */
> > +			d-cache-size = <2000000>; /* 32 kB */
> 
> That's 32MB, not 32kB.  Better fix this :-)

Argh.  What's even sadder is I just did the exact same thing for Bamboo.

> > +		SRAM0: sram {
> > +			compatible = "ibm,sram440gp";
> > +			dcr-reg = <020 8 00a 1>;
> > +		};
> 
> Is this thing _only_ addressable over DCRs?  Weird.

Yes.

> 
> >  		MAL0: mcmal {
> > -			device_type = "mcmal-dma";
> > +			device_type = "dma-controller";
> >  			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
> 
> Remove "device_type", change name to "dma-controller"?
> 
> >  			EBC0: ebc {
> > -				device_type = "ibm,ebc";
> >  				compatible = "ibm,ebc-440gp";
> 
> You forgot "ibm,ebc" here.

/me notes that for Bamboo

josh

Re: Small fixes for the Ebony device tree

[David Gibson]

On Mon, May 14, 2007 at 02:59:31PM +0200, Segher Boessenkool wrote:
> > -			i-cache-size = <0>;
> > -			d-cache-size = <0>;
> > +			i-cache-size = <2000000>; /* 32 kB */
> > +			d-cache-size = <2000000>; /* 32 kB */
> 
> That's 32MB, not 32kB.  Better fix this :-)

Duh.  Fixed.

> >  	UIC0: interrupt-controller0 {
> 
> >  	UIC1: interrupt-controller1 {
> 
> It's a shame you can't use unit addresses for these since
> you use "dcr-reg" instead of "reg".  Oh well.
> 
> >  		SDRAM0: sdram {
> > -			device_type = "memory-controller";
> >  			compatible = "ibm,sdram-440gp", "ibm,sdram";
> 
> Maybe rename the node to "memory-controller"?

Hmm, yeah, I guess so.

> > +		SRAM0: sram {
> > +			compatible = "ibm,sram440gp";
> > +			dcr-reg = <020 8 00a 1>;
> > +		};
> 
> Is this thing _only_ addressable over DCRs?  Weird.

Well... the control registers are certainly DCR only.  I guess there's
the actual SRAM itself, though whether this belongs in this node, or
elsewhere isn't immediately clear.  I haven't yet investigated how the
SRAM is mapped (it depends on DIP switch settings) so I'm certainly
not considering this node complete yet.

> >  		MAL0: mcmal {
> > -			device_type = "mcmal-dma";
> > +			device_type = "dma-controller";
> >  			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
> 
> Remove "device_type", change name to "dma-controller"?

Don't really want to remove the device_type, because the MAL driver
looks for it at present.  Don't really want to change the name, since
that might encourage confusion with the other (more conventional) DMA
controller.

> >  			EBC0: ebc {
> > -				device_type = "ibm,ebc";
> >  				compatible = "ibm,ebc-440gp";
> 
> You forgot "ibm,ebc" here.

Hmm.. yeah, I guess.

Revised patch coming shortly.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Segher Boessenkool]

>>> +		SRAM0: sram {
>>> +			compatible = "ibm,sram440gp";
>>> +			dcr-reg = <020 8 00a 1>;
>>> +		};
>>
>> Is this thing _only_ addressable over DCRs?  Weird.
>
> Well... the control registers are certainly DCR only.  I guess there's
> the actual SRAM itself, though whether this belongs in this node, or
> elsewhere isn't immediately clear.  I haven't yet investigated how the
> SRAM is mapped (it depends on DIP switch settings) so I'm certainly
> not considering this node complete yet.

If it is supposed to have a "reg" property, and it doesn't
yet, it might be a good idea to comment it out in the DTS
for now, so later kernels can work with the older device
tree correctly.

>>>  		MAL0: mcmal {
>>> -			device_type = "mcmal-dma";
>>> +			device_type = "dma-controller";
>>>  			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
>>
>> Remove "device_type", change name to "dma-controller"?
>
> Don't really want to remove the device_type, because the MAL driver
> looks for it at present.

Fair enough.  But you change the "device_type" in
this patch, so presumably you change it in the kernel
driver as well -- can't you just *fix* the kernel driver,
instead?

> Don't really want to change the name, since
> that might encourage confusion with the other (more conventional) DMA
> controller.

Nah, just look at the other properties in the node and
you know what is what.  It is quite common to have nodes
with the same name representing different devices (for
example, "ethernet" devices -- "dma-controller" would be
a bit more unusual, sure).

I have no strong feelings about the name, "mcmal" is
generic enough a name as far as I'm concerned.

>>>  			EBC0: ebc {
>>> -				device_type = "ibm,ebc";
>>>  				compatible = "ibm,ebc-440gp";
>>
>> You forgot "ibm,ebc" here.
>
> Hmm.. yeah, I guess.

Well that's what the kernel code matches on ;-)

> Revised patch coming shortly.

Looking forward to it!


Segher

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 06:59:49AM +0200, Segher Boessenkool wrote:
> >>> +		SRAM0: sram {
> >>> +			compatible = "ibm,sram440gp";
> >>> +			dcr-reg = <020 8 00a 1>;
> >>> +		};
> >>
> >> Is this thing _only_ addressable over DCRs?  Weird.
> >
> > Well... the control registers are certainly DCR only.  I guess there's
> > the actual SRAM itself, though whether this belongs in this node, or
> > elsewhere isn't immediately clear.  I haven't yet investigated how the
> > SRAM is mapped (it depends on DIP switch settings) so I'm certainly
> > not considering this node complete yet.
> 
> If it is supposed to have a "reg" property, and it doesn't
> yet, it might be a good idea to comment it out in the DTS
> for now, so later kernels can work with the older device
> tree correctly.

Given that I'm not aware of any Ebony firmwares that actually supply a
device tree, so in practice the kernel's tree will always come from an
attached zImage, I don't think this is really a big consideration.

> >>>  		MAL0: mcmal {
> >>> -			device_type = "mcmal-dma";
> >>> +			device_type = "dma-controller";
> >>>  			compatible = "ibm,mcmal-440gp", "ibm,mcmal";
> >>
> >> Remove "device_type", change name to "dma-controller"?
> >
> > Don't really want to remove the device_type, because the MAL driver
> > looks for it at present.
> 
> Fair enough.  But you change the "device_type" in
> this patch, so presumably you change it in the kernel

The kernel driver recognizes both variants, but the one I had
previously is marked deprecated.

> driver as well -- can't you just *fix* the kernel driver,
> instead?

Well.. I guess, but I'd prefer to leave that to BenH, who wrote the
driver.

> > Don't really want to change the name, since
> > that might encourage confusion with the other (more conventional) DMA
> > controller.
> 
> Nah, just look at the other properties in the node and
> you know what is what.  It is quite common to have nodes
> with the same name representing different devices (for
> example, "ethernet" devices -- "dma-controller" would be
> a bit more unusual, sure).
> 
> I have no strong feelings about the name, "mcmal" is
> generic enough a name as far as I'm concerned.
> 
> >>>  			EBC0: ebc {
> >>> -				device_type = "ibm,ebc";
> >>>  				compatible = "ibm,ebc-440gp";
> >>
> >> You forgot "ibm,ebc" here.
> >
> > Hmm.. yeah, I guess.
> 
> Well that's what the kernel code matches on ;-)

Um.. yes.  I wonder how it was working before...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[David Gibson]

On Tue, May 15, 2007 at 03:46:26PM +1000, David Gibson wrote:
> On Tue, May 15, 2007 at 06:59:49AM +0200, Segher Boessenkool wrote:
[snip]
> > driver as well -- can't you just *fix* the kernel driver,
> > instead?
> 
> Well.. I guess, but I'd prefer to leave that to BenH, who wrote the
> driver.

On second thoughts I will alter the driver, the new version I sent out
today has the fix to only look at 'compatible'.

> > > Don't really want to change the name, since
> > > that might encourage confusion with the other (more conventional) DMA
> > > controller.
> > 
> > Nah, just look at the other properties in the node and
> > you know what is what.  It is quite common to have nodes
> > with the same name representing different devices (for
> > example, "ethernet" devices -- "dma-controller" would be
> > a bit more unusual, sure).
> > 
> > I have no strong feelings about the name, "mcmal" is
> > generic enough a name as far as I'm concerned.

I'll leave it then.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Small fixes for the Ebony device tree

[Mark A. Greer]

On Mon, May 14, 2007 at 02:54:04PM +1000, David Gibson wrote:

> Index: working-2.6/arch/powerpc/boot/dts/ebony.dts
> ===================================================================
> --- working-2.6.orig/arch/powerpc/boot/dts/ebony.dts	2007-05-08 15:07:45.000000000 +1000
> +++ working-2.6/arch/powerpc/boot/dts/ebony.dts	2007-05-14 14:38:39.000000000 +1000
> @@ -33,8 +33,8 @@
>  			timebase-frequency = <0>; // Filled in by zImage
>  			i-cache-line-size = <32>;
>  			d-cache-line-size = <32>;
> -			i-cache-size = <0>;
> -			d-cache-size = <0>;
> +			i-cache-size = <2000000>; /* 32 kB */
> +			d-cache-size = <2000000>; /* 32 kB */
>  			dcr-controller;
>  			dcr-access-method = "native";
>  		};

Ahh HA!  You did it too! :)