Re: [PATCH v2 6/7] Holly DTS

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

> +/ {
> +	model = "ppc750-tsi109";

"model" should be something really specific; typically
a (unique!) model number.  This means you can't use the
same device tree for Holly and Hickory (but there are
more reasons for that; see below).

> +	compatible = "ppc750-tsi";

The needs to be more specific as well; "ibm,holly"
or something.

> +		PowerPC,750@0 {

Needs to be PowerPC,750CL etc.

> +			name = "PowerPC,750";

You don't need to specify "name" in modern DTS.

> +  	tsi109@c0000000 {
> +		device_type = "tsi-bridge";

Don't put a "device_type" here, it is useless
(and undefined).  There are more like this, but
perhaps Linux (wrongly) probes on "device_type"
for those, so the kernel would need updating
first.

> +		bus-frequency = <0>;

0?

> +		ethernet@6200 {
> +			model = "TSI-ETH";

"model" should be an exact model name.  If you don't
have one, just leave out the "model" property.

> +			address = [ 00 0d 60 f4 3f 04 ];

"local-mac-address" instead.  And you don't want to
hardcode one specific address like this; just keep
it open so if the bootloader fails to fill it in you
notice.  If the bootloader cannot do that, just leave
out this property completely (not compliant, but neither
is filling in the wrong value).

> +		serial@7808 {
> +			reg-shift = <0>;

This is the default value so you can leave it out.

> +	  	MPIC: pic@7400 {
> +			built-in;

Why say this for MPIC only, not for most other nodes?
What binding defines "built-in", anyway?

> +				/*--------------------------------------------+
> +				| AD19.
> +				+--------------------------------------------*/
> +				1800 0 0 1 &RT0 26 0
> +				1800 0 0 2 &RT0 27 0
> +				1800 0 0 3 &RT0 24 0
> +				1800 0 0 4 &RT0 25 0
> +
> +				2000 0 0 1 &RT0 27 0
> +				2000 0 0 2 &RT0 24 0
> +				2000 0 0 3 &RT0 25 0
> +				2000 0 0 4 &RT0 26 0
> +				>;

You forgot to say "AD20" ;-)

> +	chosen {
> +		linux,platform = <0>;
> +		linux,initrd-start = <0>;
> +		linux,initrd-end = <0>;

Do you need to set those zero properties?


Segher