[PATCH 2/2 V3] powerpc/85xx: add the P1020RDB-PD DTS support

[Scott Wood]

On 07/09/2013 03:35:43 AM, Haijun Zhang wrote:
> Overview of P1020RDB-PD device:
> - DDR3 2GB
> - NOR flash 64MB
> - NAND flash 128MB
> - SPI flash 16MB
> - I2C EEPROM 256Kb
> - eTSEC1 (RGMII PHY) connected to VSC7385 L2 switch
> - eTSEC2 (SGMII PHY)
> - eTSEC3 (RGMII PHY)
> - SDHC
> - 2 USB ports
> - 4 TDM ports
> - PCIe
> 
> Signed-off-by: Haijun Zhang <Haijun.Zhang at freescale.com>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang at freescale.com>
> Signed-off-by: Xie Xiaobo-R63061 <X.Xie at freescale.com>
> CC: Scott Wood <scottwood at freescale.com>
> ---
> changes for v3:
> 	- Remove some blank and changed the usb node
> 	- Renamed the dts file
> 	- change the cpld name of pd board
> 
>  arch/powerpc/boot/dts/p1020rdb-pd.dts  |  90 ++++++++++++
>  arch/powerpc/boot/dts/p1020rdb-pd.dtsi | 257  
> +++++++++++++++++++++++++++++++++
>  2 files changed, 347 insertions(+)
>  create mode 100644 arch/powerpc/boot/dts/p1020rdb-pd.dts
>  create mode 100644 arch/powerpc/boot/dts/p1020rdb-pd.dtsi

Again, why do you need a separate .dtsi?  If this isn't for a 32/36-bit  
split, what is the criteria for what goes in the .dts versus what goes  
in the .dtsi?

> +		partition at 600000 {
> +			/* 4MB for Compressed Root file System Image */
> +			reg = <0x00600000 0x00400000>;
> +			label = "NAND Compressed RFS Image";
> +		};
> +
> +		partition at a00000 {
> +			/* 22MB for JFFS2 based Root file System */
> +			reg = <0x00a00000 0x01600000>;
> +			label = "NAND JFFS2 Root File System";
> +		};

Don't refer to JFFS2.  It's bad enough that we specify partition layout  
here -- no need to specify the filesystem type, especially when it's a  
fs type that is no longer recommended.

> +		partition at 2000000 {
> +			/* 96MB for RAMDISK based Root file System */
> +			reg = <0x02000000 0x06000000>;
> +			label = "NAND Writable User area";
> +		};

Wouldn't it be better to combine these last three partitions?  Why do  
you need three root filesystems?

> +	cpld at 2,0 {
> +		compatible = "fsl,p1020rdb-pd-cpld";
> +		reg = <0x2 0x0 0x20000>;
> +		read-only;
> +	};

Remove read-only.

> +	spi at 7000 {
> +		flash at 0 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			compatible = "spansion,s25sl12801";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>; /* input clock  
> */
> +
> +			partition at u-boot {
> +				/* 512KB for u-boot Bootloader Image */
> +				reg = <0x0 0x00080000>;
> +				label = "u-boot";
> +				read-only;
> +			};
> +
> +			partition at dtb {
> +				/* 512KB for DTB Image*/
> +				reg = <0x00080000 0x00080000>;
> +				label = "dtb";
> +			};
> +
> +			partition at kernel {
> +				/* 4MB for Linux Kernel Image */
> +				reg = <0x00100000 0x00400000>;
> +				label = "kernel";
> +			};

These unit addresses are not appropriate.  They should match reg, not  
label.

> +			partition at fs {
> +				/* 4MB for Compressed RFS Image */
> +				reg = <0x00500000 0x00400000>;
> +				label = "file system";
> +			};
> +
> +			partition at jffs-fs {
> +				/* 7MB for JFFS2 based RFS */
> +				reg = <0x00900000 0x00700000>;
> +				label = "file system jffs2";
> +			};

As with NAND flash, please combine these and don't reference JFFS2.

> +		};
> +
> +		slic at 0 {
> +			compatible = "zarlink,le88266";
> +			reg = <1>;
> +			spi-max-frequency = <8000000>;
> +		};
> +
> +		slic at 1 {
> +			compatible = "zarlink,le88266";
> +			reg = <2>;
> +			spi-max-frequency = <8000000>;
> +		};
> +	};
> +
> +	mdio at 24000 {
> +		phy0: ethernet-phy at 0 {
> +			interrupts = <3 1 0 0>;
> +			reg = <0x0>;
> +		};
> +		phy1: ethernet-phy at 1 {
> +			interrupts = <2 1 0 0>;
> +			reg = <0x1>;
> +		};
> +	};

Again, leave a blank line between nodes.  If I comment about a style  
issue in one place, you should also fix the same issue in other places  
where it occurs.

> +	/*
> +	 * USB2 is shared with localbus, so it must be disabled
> +	 * by default. We can't put 'status = "disabled";' here
> +	 * since U-Boot doesn't clear the status property when
> +	 * it enables USB2. OTOH, U-Boot does create a new node
> +	 * when there isn't any. So, just comment it out.
> +	 * usb at 23000 {
> +	 * 	status = "disabled";
> +	 * 	phy_type = "ulpi";
> +	 * };
> +	 */

No.  Fix U-Boot to do whatever updating it needs to do based on runtime  
configuration.  Do not add commented out nodes to the tree.

-Scott