Hi, scott
Patch V4 had send, Pls review.
Thanks.
Regards
Haijun.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, July 10, 2013 12:15 AM
> To: Zhang Haijun-B42677
> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Fleming
> Andy-AFLEMING; Zhang Haijun-B42677; Huang Changming-R66093; Xie Xiaobo-
> R63061
> Subject: Re: [PATCH 2/2 V3] powerpc/85xx: add the P1020RDB-PD DTS support
>
> 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.zh...@freescale.com>
> > Signed-off-by: Jerry Huang <chang-ming.hu...@freescale.com>
> > Signed-off-by: Xie Xiaobo-R63061 <x....@freescale.com>
> > CC: Scott Wood <scottw...@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@600000 {
> > + /* 4MB for Compressed Root file System Image */
> > + reg = <0x00600000 0x00400000>;
> > + label = "NAND Compressed RFS Image";
> > + };
> > +
> > + partition@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@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@2,0 {
> > + compatible = "fsl,p1020rdb-pd-cpld";
> > + reg = <0x2 0x0 0x20000>;
> > + read-only;
> > + };
>
> Remove read-only.
>
> > + spi@7000 {
> > + flash@0 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + compatible = "spansion,s25sl12801";
> > + reg = <0>;
> > + spi-max-frequency = <40000000>; /* input clock
> > */
> > +
> > + partition@u-boot {
> > + /* 512KB for u-boot Bootloader Image */
> > + reg = <0x0 0x00080000>;
> > + label = "u-boot";
> > + read-only;
> > + };
> > +
> > + partition@dtb {
> > + /* 512KB for DTB Image*/
> > + reg = <0x00080000 0x00080000>;
> > + label = "dtb";
> > + };
> > +
> > + partition@kernel {
> > + /* 4MB for Linux Kernel Image */
> > + reg = <0x00100000 0x00400000>;
> > + label = "kernel";
> > + };
>
> These unit addresses are not appropriate. They should match reg, not
> label.
>
> > + partition@fs {
> > + /* 4MB for Compressed RFS Image */
> > + reg = <0x00500000 0x00400000>;
> > + label = "file system";
> > + };
> > +
> > + partition@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@0 {
> > + compatible = "zarlink,le88266";
> > + reg = <1>;
> > + spi-max-frequency = <8000000>;
> > + };
> > +
> > + slic@1 {
> > + compatible = "zarlink,le88266";
> > + reg = <2>;
> > + spi-max-frequency = <8000000>;
> > + };
> > + };
> > +
> > + mdio@24000 {
> > + phy0: ethernet-phy@0 {
> > + interrupts = <3 1 0 0>;
> > + reg = <0x0>;
> > + };
> > + phy1: ethernet-phy@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@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
