Subject: Re: [PATCH 5/8] powerpc/fsl-booke: Add initial silicon device tree for

From: Scott Wood <[email-masked]>
Date: Tue, 5 Mar 2013 18:15:55 -0600

On 03/05/2013 05:15:57 PM, Kumar Gala wrote:
> Enable a baseline T4240 SoC to boot.  There are several things missing
> from the device trees for T4240:
>=20
> * Thread support on e6500

Why did threads get removed from the device tree?  It's supposed to =20
describe hardware, not what Linux currently supports.

> * Proper PAMU topology information
> * DPAA related nodes (Qman, Bman, Fman, Rman, DCE)
> * Prefetch Manager
> * Thermal monitor unit
> * Interlaken

The dts should be marked preliminary somehow -- we really should get =20
out of the habit of letting device nodes trickle in as drivers get =20
added.

> +/* controller at 0x240000 */
> +&pci0 {
> +	compatible =3D "fsl,t4240-pcie", "fsl,qoriq-pcie-v3.0";

We have a version register -- do we really need to keep sticking the =20
version number in the compatible?  Note that we've had device trees =20
that specified the version incorrectly in the past.

> +	device_type =3D "pci";
> +	#size-cells =3D <2>;
> +	#address-cells =3D <3>;
> +	bus-range =3D <0x0 0xff>;
> +	clock-frequency =3D <33333333>;

This clock-frequency is not correct (I doubt it's needed at all).

> +		PowerPC,e6500@1 {
> +			device_type =3D "cpu";
> +			reg =3D <2>;
> +			next-level-cache =3D <&L2_1>;
> +		};
> +		PowerPC,e6500@2 {
> +			device_type =3D "cpu";
> +			reg =3D <4>;
> +			next-level-cache =3D <&L2_1>;
> +		};
> +		PowerPC,e6500@3 {
> +			device_type =3D "cpu";
> +			reg =3D <6>;
> +			next-level-cache =3D <&L2_1>;
> +		};
> +
> +		PowerPC,e6500@4 {
> +			device_type =3D "cpu";
> +			reg =3D <8>;
> +			next-level-cache =3D <&L2_2>;
> +		};

Inconsistent whitespace.

As usual, the pre/post split is unnecessary.  Everything in it can go =20
in post.

-Scott=