-----Original Message-----
 From: Wood Scott-B07421
 Sent: Tuesday, July 23, 2013 6:59 AM
 To: Liu Po-B43644
 Cc: linuxppc-dev@ozlabs.org; Hu Mingkai-B21284
 Subject: Re: [3/4] powerpc/85xx: Add C293PCIE board support
=20
 On Thu, Apr 25, 2013 at 09:54:16AM +0800, Po Liu wrote:
 > From: Mingkai Hu [off-list ref]
 >
 > C293PCIE board is a series of Freescale PCIe add-in cards to perform
 > as public key crypto accelerator or secure key management module.
 >
 >  - 512KB platform SRAM in addition to 512K L2 Cache/SRAM
 >  - 512MB soldered DDR3 32bit memory
 >  - CPLD System Logic
 >  - 64MB x16 NOR flash and 4GB x8 NAND flash
 >  - 16MB SPI flash
 >
 > Signed-off-by: Mingkai Hu [off-list ref]
 > Singed-off-by: Po Liu [off-list ref]
=20
 Signed
=20
 > +		partition@900000 {
 > +			/* 33MB for rootfs */
 > +			reg =3D <0x00900000 0x02100000>;
 > +			label =3D "NOR Rootfs Image";
 > +		};
 > +
 > +		partition@2a00000 {
 > +			/* 20MB for JFFS2 based Root file System */
 > +			reg =3D <0x02a00000 0x01400000>;
 > +			label =3D "NOR JFFS2 Root File System";
 > +		};
=20
 Don't specify JFFS2.  Combine these two partitions into one.

Ok, I'll merge up two partition.

=20
 > +		partition@600000 {
 > +			/* 4MB for Compressed Root file System Image */
 > +			reg =3D <0x00600000 0x00400000>;
 > +			label =3D "NAND Compressed RFS Image";
 > +		};
 > +
 > +		partition@a00000 {
 > +			/* 15MB for JFFS2 based Root file System */
 > +			reg =3D <0x00a00000 0x00f00000>;
 > +			label =3D "NAND JFFS2 Root File System";
 > +		};
=20
 Likewise.
=20
 > +		partition@1900000 {
 > +			/* 7MB for User Area */
 > +			reg =3D <0x01900000 0x00700000>;
 > +			label =3D "NAND User area";
 > +		};
=20
 Above you say there's 4 GiB of NAND, but here you define partitions that
 only cover 32 MiB.

Can I set one partion include all other space(4GB- 32MB) with label name "O=
thers"?

=20
 > +	};
 > +
 > +	cpld@2,0 {
 > +		#address-cells =3D <1>;
 > +		#size-cells =3D <1>;
 > +		compatible =3D "fsl,c293pcie-cpld";
 > +		reg =3D <0x2 0x0 0x0000020>;
 > +		bank-width =3D <1>;
 > +		device-width =3D <1>;
 > +	};
=20
 What do bank-width and device-width mean here?

I will remove these two lines? I thought I copy from other platform.

=20
 Why all the leading zeroes in 0x0000020?

I'll change to 0x20 from 0x0000020.

=20
 > +			partition@580000 {
 > +				/* 4MB for Compressed RFS Image */
 > +				reg =3D <0x00580000 0x00400000>;
 > +				label =3D "SPI Flash Compressed RFSImage";
 > +			};
 > +
 > +			partition@980000 {
 > +				/* 6.5MB for JFFS2 based RFS */
 > +				reg =3D <0x00980000 0x00680000>;
 > +				label =3D "SPI Flash JFFS2 RFS";
 > +			};
=20
 Again, merge these two and don't specify JFFS2.

Ok, thanks

=20
 > diff --git a/arch/powerpc/platforms/85xx/Kconfig
 > b/arch/powerpc/platforms/85xx/Kconfig
 > index a0dcd57..df26b21 100644
 > --- a/arch/powerpc/platforms/85xx/Kconfig
 > +++ b/arch/powerpc/platforms/85xx/Kconfig
 > @@ -32,6 +32,13 @@ config BSC9131_RDB
 >  	  StarCore SC3850 DSP
 >  	  Manufacturer : Freescale Semiconductor, Inc
 >
 > +config C293_PCIE
 > +	  bool "Freescale C293PCIE"
 > +	  select DEFAULT_UIMAGE
 > +	  select SWIOTLB
 > +	  help
 > +	  This option enables support for the C293PCIE board
=20
 Why do you need SWIOTLB if the board has 512 MiB soldered RAM?

I'll remove it.

=20
 > diff --git a/arch/powerpc/platforms/85xx/c293pcie.c
 > b/arch/powerpc/platforms/85xx/c293pcie.c
 > new file mode 100644
 > index 0000000..75dda12
 > --- /dev/null
 > +++ b/arch/powerpc/platforms/85xx/c293pcie.c
 > @@ -0,0 +1,82 @@
 > +/*
 > + * C293PCIE Board Setup
 > + *
 > + * Copyright 2013 Freescale Semiconductor Inc.
 > + *
 > + * This program is free software; you can redistribute  it and/or
 > +modify it
 > + * under  the terms of  the GNU General  Public License as published
 > +by the
 > + * Free Software Foundation;  either version 2 of the  License, or
 > +(at your
 > + * option) any later version.
 > + */
 > +
 > +#include <linux/stddef.h>
 > +#include <linux/kernel.h>
 > +#include <linux/pci.h>
 > +#include <linux/delay.h>
 > +#include <linux/interrupt.h>
 > +#include <linux/of_platform.h>
 > +
 > +#include <asm/time.h>
 > +#include <asm/machdep.h>
 > +#include <asm/pci-bridge.h>
 > +#include <mm/mmu_decl.h>
 > +#include <asm/prom.h>
 > +#include <asm/udbg.h>
 > +#include <asm/mpic.h>
 > +
 > +#include <sysdev/fsl_soc.h>
 > +#include <sysdev/fsl_pci.h>
 > +
 > +#include "mpc85xx.h"
=20
 Are you sure you need all of these?  I don't see any delays, for example=

.
Thanks, I'll test and remove redundant includes.

=20
 -Scott