[RFC 3/3] powerpc: Add DTS file for the Motorola PrPMC2800 platform

Yoder Stuart-B08248 stuart.yoder at freescale.com
Thu Mar 29 02:43:16 EST 2007


Biggest concern is that I see a lot of undocumented device
types and properties...

Device types that I do not recognize include:
+			device_type = "brg";
+			device_type = "mv64x60-cunit";
+			device_type = "mpscrouting";
+			device_type = "mpscintr";
+			device_type = "mv64x60-pic";
+			device_type = "mv64x60-mpp";
+			device_type = "mv64x60-gpp";
+			device_type = "mv64x60-cpu-error";
+			device_type = "mv64x60-sram-error";
+			device_type = "mv64x60-pci-error";
+			device_type = "mv64x60-memctrl";

These should ideally be documented with required properties
and what they mean.

I think a document similar to Grant Likely's
Documentation/mpc52xx-device-tree-bindings.txt would be
very helpful here.  People using this DTS as a starting
point are going to have a lot of questions.

comments inline below-- 
[snip]

> +
> +	mv64x60 at f1000000 { /* Marvell Discovery */
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		#interrupt-cells = <1>;
> +		device_type = "mv64360";
> +		compatible = "mv64x60";
> +		clock-frequency = <7f28155>;		/* 
> 133.333333 Mhz */
> +		reg = <f1000000 00010000>;
> +		virtual-reg = <f1000000>;

What is virtual-reg for?  I gather it is related to the bootwrapper
needed virtual addresses for devices reg blocks.  But, I counted
12 instances of the property in this file.  Are all needed by the
bootwrapper??

Does anyone where this property came from historically?  If it's
specifically for a Linux bootwrapper the name should reflect 
that-- linux,bootwrapper-reg-vaddr -- or something...

I also wonder if there is a better place to put this information.
The device nodes strictly speaking should be describing the devices
_physical_ characteristics.

It also needs to be documented in booting-without-of.txt.

[snip]
> +			reg = <2000 2000>;
> +			virtual-reg = <f1002000>;
> +			eth0 {
> +				device_type = "network";
> +				compatible = "mv64x60-eth";
> +				block-index = <0>;
> +				interrupts = <20>;
> +				interrupt-parent = <&/mv64x60/pic>;
> +				phy = <&/mv64x60/mdio/ethernet-phy at 1>;
> +				local-mac-address = [ 00 00 00 
> 00 00 00 ];
> +			};
> +			eth1 {
> +				device_type = "network";
> +				compatible = "mv64x60-eth";
> +				block-index = <1>;
> +				interrupts = <21>;
> +				interrupt-parent = <&/mv64x60/pic>;
> +				phy = <&/mv64x60/mdio/ethernet-phy at 3>;
> +				local-mac-address = [ 00 00 00 
> 00 00 00 ];
> +			};
> +		};

Based on some offline dicussions I think I know what block-index
is, but I'd like to see this property documented in
booting-without-of.txt.

[snip]
> +		i2c at c000 {
> +			device_type = "i2c";
> +			compatible = "mv64x60-i2c";
> +			reg = <c000 20>;
> +			virtual-reg = <f100c000>;
> +			freq_m = <8>;
> +			freq_n = <3>;
> +			timeout = <3e8>;		/* 1000 

If freq_m and freq_n are Marvell specific should have
a "Marvel," prepended to them?

[snip]
> +		pci at 80000000 {
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			#interrupt-cells = <1>;
> +			device_type = "pci";
> +			compatible = "mv64x60-pci";
> +			reg = <0cf8 8>;
> +			ranges = <01000000 0        0 88000000 
> 0 01000000
> +				  02000000 0 80000000 80000000 
> 0 08000000>;
> +			bus-range = <0 ff>;
> +			clock-frequency = <3EF1480>;
> +			interrupt-pci-iack = <0c34>;

I don't think interrupt-pci-iack is standard.  If it's generally
applicable add it to booting-without-of.txt.  If its MV specific 
prepend with "Marvel,".

Regards,
Stuart Yoder



More information about the Linuxppc-dev mailing list