[PATCH] Add StorCenter DTS first draft.

[PATCH] Add StorCenter DTS first draft.

[Jon Loeliger]

Based on the Kurobox DTS files.

Signed-off-by: Oyvind Repvik <nail@nslu2-linux.org>
Signed-off-by: Jon Loeliger <jdl@jdl.com>
---

Comments welcome, of course.


 arch/powerpc/boot/dts/storcenter.dts |  142 ++++++++++++++++++++++++++++++++++
 1 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/boot/dts/storcenter.dts

diff --git a/arch/powerpc/boot/dts/storcenter.dts b/arch/powerpc/boot/dts/storcenter.dts
new file mode 100644
index 0000000..87193fa
--- /dev/null
+++ b/arch/powerpc/boot/dts/storcenter.dts
@@ -0,0 +1,142 @@
+/*
+ * Device Tree Source for IOMEGA StorCenter
+ *
+ * Copyright 2007 Oyvind Repvik, Jon Loeliger
+ *
+ * Based on the Kurobox DTS by G. Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * build using:
+ *    dtc -f -b 0 -I dts -O dtb -o storcenter.dtb storcenter.dts
+ */
+
+/ {
+	model = "StorCenter";
+	compatible = "storcenter";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		PowerPC,603e {			/* Really 8241 */
+			device_type = "cpu";
+			reg = <0>;
+			clock-frequency = <d# 200000000>;	/* Hz */
+			timebase-frequency = <d# 33333333>;	/* Hz */
+			bus-frequency = <0>;
+			/* Following required by dtc but not used */
+			i-cache-line-size = <0>;
+			d-cache-line-size = <0>;
+			i-cache-size = <4000>;
+			d-cache-size = <4000>;
+		};
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <00000000 04000000>;	/* 64MB @ 0x0 */
+	};
+
+	flash@ff800000 {
+		device_type = "rom";
+		compatible = "direct-mapped";
+		probe-type = "CFI";
+		reg = <ff800000 00800000>;
+		bank-width = <1>;
+		partitions = <
+				00000000 0000E000
+				0000E000 00002000
+				00010000 00040000
+				00050000 00200000
+				00250000 004B0000
+				00700000 00020000
+				00720000 00010000
+				00730000 00010000
+				00740000 000B0000
+		>;
+		partition-names = "uboot2env", "dtb", "uboot2",
+				  "emkernel", "emfs", "uboot1",
+				  "empty", "uboot1-env", "SysConf";
+	};
+
+
+	soc10x {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		#interrupt-cells = <2>;
+		device_type = "soc";
+		compatible = "mpc10x";
+		store-gathering = <0>; /* 0 == off, !0 == on */
+		reg = <80000000 00100000>;
+		ranges = <80000000 80000000 70000000	/* pci mem space */
+			  fdf00000 fdf00000 00100000	/* EUMB */
+			  fe000000 fe000000 00c00000	/* pci i/o space */
+			  fec00000 fec00000 00300000	/* pci cfg regs */
+			  fef00000 fef00000 00100000>;	/* pci iack */
+
+		i2c@fdf03000 {
+			device_type = "i2c";
+			compatible = "fsl-i2c";
+			reg = <fdf03000 1000>;
+			interrupts = <5 2>;
+			interrupt-parent = <&mpic>;
+		};
+
+		serial@fdf04500 {
+			device_type = "serial";
+			compatible = "ns16550";
+			reg = <fdf04500 8>;
+			clock-frequency = <d# 100000000>; /* Hz */
+			current-speed = <d# 115200>;
+			interrupts = <9 2>;
+			interrupt-parent = <&mpic>;
+		};
+
+		serial@fdf04600 {
+			device_type = "serial";
+			compatible = "ns16550";
+			reg = <fdf04600 8>;
+			clock-frequency = <d# 100000000>; /* Hz */
+			current-speed = <d# 19200>;
+			interrupts = <a 2>;
+			interrupt-parent = <&mpic>;
+		};
+
+		mpic: pic@fdf40000 {
+			#interrupt-cells = <2>;
+			#address-cells = <0>;
+			device_type = "open-pic";
+			compatible = "chrp,open-pic";
+			interrupt-controller;
+			reg = <fdf40000 40000>;
+			built-in;
+		};
+
+		pci@fe800000 {
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			device_type = "pci";
+			compatible = "mpc10x-pci";
+			reg = <fe800000 400000>;
+			ranges = <01000000 0        0 fe000000 0 00c00000
+				  02000000 0 80000000 80000000 0 70000000>;
+			bus-range = <0 ff>;
+			clock-frequency = <d# 100000000>; /* Hz */
+			interrupt-parent = <&mpic>;
+			interrupt-map-mask = <f800 0 0 7>;
+			interrupt-map = <
+				/* IDSEL 0x15 - ETH */
+				7800 0 0 1 &mpic 0 1
+				7800 0 0 2 &mpic 0 1
+				7800 0 0 3 &mpic 0 1
+				7800 0 0 4 &mpic 0 1 
+			>;
+		};
+	};
+};
-- 
1.5.2.2.249.g45fd

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

> Comments welcome, of course.

Well you asked for it :-)

> +/ {
> +	model = "StorCenter";

If you can find a real model number, put it in here,
instead.

> +	compatible = "storcenter";

Needs a manufacturer name in there.

> +		PowerPC,603e {			/* Really 8241 */

So say "PowerPC,8241@0", or "PowerPC,e300@0" (or whatever
the CPU core in there is), or simply "cpu@0", following
the generic naming recommended practice.

> +			bus-frequency = <0>;

Is this filled in anywhere?  Please document that, if so.

> +			/* Following required by dtc but not used */
> +			i-cache-line-size = <0>;
> +			d-cache-line-size = <0>;
> +			i-cache-size = <4000>;
> +			d-cache-size = <4000>;

Not used _by the Linux kernel_, it's required by the
PowerPC binding.  Perhaps that should be modified
for flat device tree use, there are many more required
properties that no flat tree has anyway.

> +	flash@ff800000 {
> +		device_type = "rom";

I'm sure you know I find this "rom" binding to be crap.
However, I didn't yet write up the "cfi" binding, so I
can't complain ;-)

> +		partitions = <
> +				00000000 0000E000
> +				0000E000 00002000
> +				00010000 00040000
> +				00050000 00200000
> +				00250000 004B0000
> +				00700000 00020000
> +				00720000 00010000
> +				00730000 00010000
> +				00740000 000B0000
> +		>;

Nothing from 7f0000 to 7fffff?

> +	soc10x {

Bad name.  Where is the binding for this?  I don't think
I saw it before.

> +		compatible = "mpc10x";

"manufacturer,106-host" or similar.  But this isn't an 10x
at all, is it?

> +		store-gathering = <0>; /* 0 == off, !0 == on */

Don't define this as "!0", but as "1".

> +		i2c@fdf03000 {
> +			device_type = "i2c";

No device_type, there is no I2C binding.

> +			compatible = "fsl-i2c";

Needs to be more specific.

> +		mpic: pic@fdf40000 {

interrupt-controller@fdf40000

> +			#interrupt-cells = <2>;
> +			#address-cells = <0>;

No #address-cells here.

> +			device_type = "open-pic";

device_type = "interrupt-controller" I believe, unless
the mpic binding does something weird.

> +		pci@fe800000 {
> +			clock-frequency = <d# 100000000>; /* Hz */

100MHz PCI?  Interesting.

> +			interrupt-map = <
> +				/* IDSEL 0x15 - ETH */
> +				7800 0 0 1 &mpic 0 1

7800 isn't device 0x15.  I think you meant 15.


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Josh Boyer]

On Tue, 2007-07-17 at 16:57 +0200, Segher Boessenkool wrote:
> 
> > +	flash@ff800000 {
> > +		device_type = "rom";
> 
> I'm sure you know I find this "rom" binding to be crap.
> However, I didn't yet write up the "cfi" binding, so I
> can't complain ;-)

Not all chips are CFI compliant.  Be sure to write up the "jedec" and
"nand" bindings while you're at it ;)

josh

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>>> +	flash@ff800000 {
>>> +		device_type = "rom";
>>
>> I'm sure you know I find this "rom" binding to be crap.
>> However, I didn't yet write up the "cfi" binding, so I
>> can't complain ;-)
>
> Not all chips are CFI compliant.

I know :-)

> Be sure to write up the "jedec"

It's in the pipeline, it's quite a bit more complicated
than CFI though.

> and "nand" bindings while you're at it ;)

NAND is pure hell :-(


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Jon Loeliger]

So, like, the other day Segher Boessenkool mumbled:
> > +/ {
> > +	model = "StorCenter";
> 
> If you can find a real model number, put it in here, instead.

Yep, "StorCenter" is it.  No model numer/name beyond that.

> > +	compatible = "storcenter";
> 
> Needs a manufacturer name in there.

Right.  Will use:
	compatible = "iomega,storcenter"

> > +		PowerPC,603e {			/* Really 8241 */
> 
> So say "PowerPC,8241@0", or "PowerPC,e300@0" (or whatever
> the CPU core in there is), or simply "cpu@0", following
> the generic naming recommended practice.

Well, its the 8241 SoC with a 603e core...  (This is
the same phrase currently being used on the Kurobox.)
I'll use:

	PowerPC,8241@0 }


> > +			bus-frequency = <0>;
> 
> Is this filled in anywhere?  Please document that, if so.

Right.  boot{loader,wrapper}

> > +	soc10x {
> 
> Bad name.  Where is the binding for this?  I don't think
> I saw it before.

It's what is being used, again, by the Kurobox.  I understand
that doesn't make it "right", just precedented by now.

How about "soc8241@80000000" instead?

That would be similar to:
        soc8641@f8000000 {
and
       soc8272@f0000000 {

> > +		store-gathering = <0>; /* 0 == off, !0 == on */
> 
> Don't define this as "!0", but as "1".

OK.

> > +		i2c@fdf03000 {
> > +			device_type = "i2c";
> 
> No device_type, there is no I2C binding.

Right.

> > +			compatible = "fsl-i2c";
> 
> Needs to be more specific.

Hmmm...  Not sure what to use here then.  There are many
existing examples using "fsl-i2c" already.  Granted, we've
established that they could be wrong...  Should this be
more like this?:

    compatible = "fsl,mpc8241-i2c", "fsl-i2c";

> > +		mpic: pic@fdf40000 {
> 
> interrupt-controller@fdf40000

OK.

> > +		pci@fe800000 {
> > +			clock-frequency = <d# 100000000>; /* Hz */
> 
> 100MHz PCI?  Interesting.

Good point. 66666666 seems more likely...


Thanks for the review and help here!

jdl

Re: [PATCH] Add StorCenter DTS first draft.

[Scott Wood]

Jon Loeliger wrote:
> How about "soc8241@80000000" instead?
> 
> That would be similar to:
>         soc8641@f8000000 {
> and
>        soc8272@f0000000 {

How about just "soc@80000000"?  Those model numbers in the names are a 
PITA to find from limited functionality environments such as the 
bootwrapper, require things like stdout-path to be different on every 
soc, and don't comply with the generic names recommendation.

-Scott

Re: [PATCH] Add StorCenter DTS first draft.

[Jon Loeliger]

So, like, the other day Scott Wood mumbled:
> Jon Loeliger wrote:
> > How about "soc8241@80000000" instead?
> 
> How about just "soc@80000000"?  Those model numbers in the names are a 
> PITA to find from limited functionality environments such as the 
> bootwrapper, require things like stdout-path to be different on every 
> soc, and don't comply with the generic names recommendation.

Oh!  Right.  No problem.

jdl

Re: [PATCH] Add StorCenter DTS first draft.

[Benjamin Herrenschmidt]

> +		PowerPC,603e {			/* Really 8241 */
> +			device_type = "cpu";
> +			reg = <0>;
> +			clock-frequency = <d# 200000000>;	/* Hz */
> +			timebase-frequency = <d# 33333333>;	/* Hz */
> +			bus-frequency = <0>;
> +			/* Following required by dtc but not used */
> +			i-cache-line-size = <0>;
> +			d-cache-line-size = <0>;
> +			i-cache-size = <4000>;
> +			d-cache-size = <4000>;
> +		};

The 32 bits kernel may not be using those yet, but it will. 64 bits does
already and I plan to merge those bits at one point. Get your cache line
size right (0 is wrong, though I will provide a fallback to the cputable
when the device-tree looks obviously wrong).

Cheers,
Ben.

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>> +		PowerPC,603e {			/* Really 8241 */
>> +			device_type = "cpu";
>> +			reg = <0>;
>> +			clock-frequency = <d# 200000000>;	/* Hz */
>> +			timebase-frequency = <d# 33333333>;	/* Hz */
>> +			bus-frequency = <0>;
>> +			/* Following required by dtc but not used */
>> +			i-cache-line-size = <0>;
>> +			d-cache-line-size = <0>;
>> +			i-cache-size = <4000>;
>> +			d-cache-size = <4000>;
>> +		};
>
> The 32 bits kernel may not be using those yet, but it will. 64 bits  
> does
> already and I plan to merge those bits at one point.

Hrm, what does it use it for?  Are you going to require
all other defined CPU properties as well (like the PowerPC
binding does)?


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>>> +	compatible = "storcenter";
>>
>> Needs a manufacturer name in there.
>
> Right.  Will use:
> 	compatible = "iomega,storcenter"

Okido.

>>> +		PowerPC,603e {			/* Really 8241 */
>>
>> So say "PowerPC,8241@0", or "PowerPC,e300@0" (or whatever
>> the CPU core in there is), or simply "cpu@0", following
>> the generic naming recommended practice.
>
> Well, its the 8241 SoC with a 603e core...  (This is
> the same phrase currently being used on the Kurobox.)
> I'll use:
>
> 	PowerPC,8241@0 }

That might be best yes.

>>> +	soc10x {
>>
>> Bad name.  Where is the binding for this?  I don't think
>> I saw it before.
>
> It's what is being used, again, by the Kurobox.  I understand
> that doesn't make it "right", just precedented by now.

Sure, just trying to trick you into documenting it ;-)

> How about "soc8241@80000000" instead?

soc@ like suggested by Scott seems just fine.

>>> +			compatible = "fsl-i2c";
>>
>> Needs to be more specific.
>
> Hmmm...  Not sure what to use here then.  There are many
> existing examples using "fsl-i2c" already.  Granted, we've
> established that they could be wrong...  Should this be
> more like this?:
>
>     compatible = "fsl,mpc8241-i2c", "fsl-i2c";

That looks good yes.  Or if the kernel side code for
recognising fsl,mpc8241-i2c gets merged in time, you
can leave out fsl-i2c from your device tree completely.


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Kumar Gala]

On Jul 18, 2007, at 11:19 AM, Segher Boessenkool wrote:

>>>> +	compatible = "storcenter";
>>>
>>> Needs a manufacturer name in there.
>>
>> Right.  Will use:
>> 	compatible = "iomega,storcenter"
>
> Okido.
>
>>>> +		PowerPC,603e {			/* Really 8241 */
>>>
>>> So say "PowerPC,8241@0", or "PowerPC,e300@0" (or whatever
>>> the CPU core in there is), or simply "cpu@0", following
>>> the generic naming recommended practice.
>>
>> Well, its the 8241 SoC with a 603e core...  (This is
>> the same phrase currently being used on the Kurobox.)
>> I'll use:
>>
>> 	PowerPC,8241@0 }
>
> That might be best yes.
>
>>>> +	soc10x {
>>>
>>> Bad name.  Where is the binding for this?  I don't think
>>> I saw it before.
>>
>> It's what is being used, again, by the Kurobox.  I understand
>> that doesn't make it "right", just precedented by now.
>
> Sure, just trying to trick you into documenting it ;-)
>
>> How about "soc8241@80000000" instead?
>
> soc@ like suggested by Scott seems just fine.
>
>>>> +			compatible = "fsl-i2c";
>>>
>>> Needs to be more specific.
>>
>> Hmmm...  Not sure what to use here then.  There are many
>> existing examples using "fsl-i2c" already.  Granted, we've
>> established that they could be wrong...  Should this be
>> more like this?:
>>
>>     compatible = "fsl,mpc8241-i2c", "fsl-i2c";
>
> That looks good yes.  Or if the kernel side code for
> recognising fsl,mpc8241-i2c gets merged in time, you
> can leave out fsl-i2c from your device tree completely.

Hmm, there are really only two fsl,i2c controllers.  The one we call  
fsl-i2c, and the cpm-i2c controller.

So I'd prefer we don't use fsl,mpc8241-i2c.  I'd suggest fsl,ppc-i2c  
or something like that.

- k

Re: [PATCH] Add StorCenter DTS first draft.

[Benjamin Herrenschmidt]

On Wed, 2007-07-18 at 18:13 +0200, Segher Boessenkool wrote:
> >> +		PowerPC,603e {			/* Really 8241 */
> >> +			device_type = "cpu";
> >> +			reg = <0>;
> >> +			clock-frequency = <d# 200000000>;	/* Hz */
> >> +			timebase-frequency = <d# 33333333>;	/* Hz */
> >> +			bus-frequency = <0>;
> >> +			/* Following required by dtc but not used */
> >> +			i-cache-line-size = <0>;
> >> +			d-cache-line-size = <0>;
> >> +			i-cache-size = <4000>;
> >> +			d-cache-size = <4000>;
> >> +		};
> >
> > The 32 bits kernel may not be using those yet, but it will. 64 bits  
> > does
> > already and I plan to merge those bits at one point.
> 
> Hrm, what does it use it for?  Are you going to require
> all other defined CPU properties as well (like the PowerPC
> binding does)?

Cache line size is used by the kernel on ppc64 for things like clearing
memory (to get the stride between subsequent dcbz) or flushing the
cache :-) It's also passed on to userland.

If it's absent from the device-tree, we default to the values in the
cputable, but if you're going to put the properties in the tree, don't
put a 0 in there. As it is, the day I make the 64 bits code common, your
DT will break unless I special case "0".

Ben.

Re: [PATCH] Add StorCenter DTS first draft.

[Jon Loeliger]

On Wed, 2007-07-18 at 16:54, Benjamin Herrenschmidt wrote:

> Cache line size is used by the kernel on ppc64 for things like clearing
> memory (to get the stride between subsequent dcbz) or flushing the
> cache :-) It's also passed on to userland.
> 
> If it's absent from the device-tree, we default to the values in the
> cputable, but if you're going to put the properties in the tree, don't
> put a 0 in there. As it is, the day I make the 64 bits code common, your
> DT will break unless I special case "0".
> 
> Ben.

So, just to put this issue to rest some, I have
modified my DTS to have correct values here.
It was _easy_. ;-)

But, a followup patch should clean up _other_, existing
DTS files that have 0 there still.

jdl

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>>>     compatible = "fsl,mpc8241-i2c", "fsl-i2c";
>>
>> That looks good yes.  Or if the kernel side code for
>> recognising fsl,mpc8241-i2c gets merged in time, you
>> can leave out fsl-i2c from your device tree completely.
>
> Hmm, there are really only two fsl,i2c controllers.  The one we  
> call fsl-i2c, and the cpm-i2c controller.
>
> So I'd prefer we don't use fsl,mpc8241-i2c.  I'd suggest fsl,ppc- 
> i2c or something like that.

The actual name doesn't matter much, as long as it is "unique
enough"; a name that matches a name already in use in real life
is preferred; normally the name is just the name of the first
device that had this specific programming interface.

But choose whatever you want.  It is a good idea to always
put the exact name of the specific device in there, too, btw,
so you'd end up with "fsl,mpc8241-i2c", "fsl,ppc-i2c" if your
suggestion is accepted.


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>>>> +			/* Following required by dtc but not used */
>>>> +			i-cache-line-size = <0>;
>>>> +			d-cache-line-size = <0>;
>>>> +			i-cache-size = <4000>;
>>>> +			d-cache-size = <4000>;
>>>> +		};
>>>
>>> The 32 bits kernel may not be using those yet, but it will. 64 bits
>>> does
>>> already and I plan to merge those bits at one point.
>>
>> Hrm, what does it use it for?  Are you going to require
>> all other defined CPU properties as well (like the PowerPC
>> binding does)?
>
> Cache line size is used by the kernel on ppc64 for things like  
> clearing
> memory (to get the stride between subsequent dcbz) or flushing the
> cache :-) It's also passed on to userland.
>
> If it's absent from the device-tree, we default to the values in the
> cputable, but if you're going to put the properties in the tree, don't
> put a 0 in there. As it is, the day I make the 64 bits code common,  
> your
> DT will break unless I special case "0".

So your plan is to prefer the device tree over the cputable, and
maybe even deprecate the cputable?  I was under the impression
that we were going the exact opposite way (we don't have many of
required properties in CPU nodes in most DTS files).  Either way
is fine with me, just choose one; but picking a little from the
device tree and the rest from the cputable doesn't sound useful to
me, and it would be a lovely maintenance problem.


Segher

Re: [PATCH] Add StorCenter DTS first draft.

[Benjamin Herrenschmidt]

On Thu, 2007-07-19 at 19:07 +0200, Segher Boessenkool wrote:
> 
> So your plan is to prefer the device tree over the cputable, and
> maybe even deprecate the cputable?  

Nah, you are fine not putting anything, but if it's in the DT it should
be correct, not "0". Else, just don't put it in the DT.

We use the DT to override cputable but that's mostly useful for things
like pSeries where you can get some processors in "compatibility" mode
over another one, or some virtual PVRs, and other fancy things like that
where the cputable isn't very useful.

Ben.

Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool]

>> So your plan is to prefer the device tree over the cputable, and
>> maybe even deprecate the cputable?
>
> Nah, you are fine not putting anything, but if it's in the DT it  
> should
> be correct, not "0". Else, just don't put it in the DT.

Ah okay.  So we should recommend to not put any of those
properties into DTS files at all?  If so, I'll create a
patch to remove them.

> We use the DT to override cputable but that's mostly useful for things
> like pSeries where you can get some processors in "compatibility" mode
> over another one, or some virtual PVRs, and other fancy things like  
> that
> where the cputable isn't very useful.

Yeah, and it's a good idea to have the device tree override
the cputable always, with real OF, anyway.  (Well there are
buggy device trees out there, of course ;-) )


Segher