Re: [PATCH] Add StorCenter DTS first draft.

[Segher Boessenkool <segher@kernel.crashing.org>]

> 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