[PATCH 01/25] pxa3xx_nand: condense the flash definition
Eric Miao
eric.y.miao at gmail.com
Fri Jun 18 02:23:02 EDT 2010
On Fri, Jun 18, 2010 at 1:32 PM, Haojian Zhuang
<haojian.zhuang at gmail.com> wrote:
> From 76684334fbf214ab164ed07791b02535a3f04779 Mon Sep 17 00:00:00 2001
> From: Lei Wen <leiwen at marvell.com>
> Date: Sat, 20 Mar 2010 19:01:23 +0800
> Subject: [PATCH 01/25] pxa3xx_nand: condense the flash definition
>
> Adding a new flash definition would need less code.
> Keep the platform passing flash defition method.
^^^^^^^^ definition?
> If one flash is both defined in platform data and builtin table,
> driver would select the one from platform data first.
This makes sense. I thin the CONFIG_MTD_NAND_PXA3xx_BUILTIN is introduced
as an optimization so to reduce the code size if platform flash definition
is provided, instead of an option to remove built-in definitions.
>
> By this way, platform could select the timing most suit for itself,
^^^^ suitable
> not need to follow the common settings.
>
> Signed-off-by: Lei Wen <leiwen at marvell.com>
> Signed-off-by: Haojian Zhuang <haojian.zhuang at marvell.com>
// there are some line wrapped though, maybe it's my gmail fault.
> ---
> arch/arm/plat-pxa/include/plat/pxa3xx_nand.h | 37 ----
> drivers/mtd/nand/Kconfig | 7 -
> drivers/mtd/nand/pxa3xx_nand.c | 241 +++++++-------------------
> 3 files changed, 66 insertions(+), 219 deletions(-)
>
> diff --git a/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
> b/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
> index 3478eae..7e5a28f 100644
> --- a/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
> +++ b/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
> @@ -4,43 +4,6 @@
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
>
> -struct pxa3xx_nand_timing {
> - unsigned int tCH; /* Enable signal hold time */
> - unsigned int tCS; /* Enable signal setup time */
> - unsigned int tWH; /* ND_nWE high duration */
> - unsigned int tWP; /* ND_nWE pulse time */
> - unsigned int tRH; /* ND_nRE high duration */
> - unsigned int tRP; /* ND_nRE pulse width */
> - unsigned int tR; /* ND_nWE high to ND_nRE low for read */
> - unsigned int tWHR; /* ND_nWE high to ND_nRE low for status read */
> - unsigned int tAR; /* ND_ALE low to ND_nRE low delay */
> -};
> -
> -struct pxa3xx_nand_cmdset {
> - uint16_t read1;
> - uint16_t read2;
> - uint16_t program;
> - uint16_t read_status;
> - uint16_t read_id;
> - uint16_t erase;
> - uint16_t reset;
> - uint16_t lock;
> - uint16_t unlock;
> - uint16_t lock_status;
> -};
> -
> -struct pxa3xx_nand_flash {
> - const struct pxa3xx_nand_timing *timing; /* NAND Flash timing */
> - const struct pxa3xx_nand_cmdset *cmdset;
> -
> - uint32_t page_per_block;/* Pages per block (PG_PER_BLK) */
> - uint32_t page_size; /* Page size in bytes (PAGE_SZ) */
> - uint32_t flash_width; /* Width of Flash memory (DWIDTH_M) */
> - uint32_t dfc_width; /* Width of flash controller(DWIDTH_C) */
> - uint32_t num_blocks; /* Number of physical blocks in Flash */
> - uint32_t chip_id;
> -};
> -
So does this prevent board code from defining their own timing and etc?
> struct pxa3xx_nand_platform_data {
>
> /* the data flash bus is shared between the Static Memory
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 98a04b3..9a35d92 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -399,13 +399,6 @@ config MTD_NAND_PXA3xx
> This enables the driver for the NAND flash device found on
> PXA3xx processors
>
> -config MTD_NAND_PXA3xx_BUILTIN
> - bool "Use builtin definitions for some NAND chips (deprecated)"
> - depends on MTD_NAND_PXA3xx
> - help
> - This enables builtin definitions for some NAND chips. This
> - is deprecated in favor of platform specific data.
> -
> config MTD_NAND_CM_X270
> tristate "Support for NAND Flash on CM-X270 modules"
> depends on MTD_NAND && MACH_ARMCORE
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index e02fa4f..013e075 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
... snipped ...
> static struct pxa3xx_nand_timing default_timing;
> static struct pxa3xx_nand_flash default_flash;
>
> -static struct pxa3xx_nand_cmdset smallpage_cmdset = {
> - .read1 = 0x0000,
> - .read2 = 0x0050,
> - .program = 0x1080,
> - .read_status = 0x0070,
> - .read_id = 0x0090,
> - .erase = 0xD060,
> - .reset = 0x00FF,
> - .lock = 0x002A,
> - .unlock = 0x2423,
> - .lock_status = 0x007A,
> -};
> -
> -static struct pxa3xx_nand_cmdset largepage_cmdset = {
> +const static struct pxa3xx_nand_cmdset cmdset = {
So how to handle the difference of command set between smallpage and
largepage?
> .read1 = 0x3000,
> .read2 = 0x0050,
> .program = 0x1080,
> @@ -203,143 +225,26 @@ static struct pxa3xx_nand_cmdset largepage_cmdset = {
> .lock_status = 0x007A,
> };
>
> -#ifdef CONFIG_MTD_NAND_PXA3xx_BUILTIN
> -static struct pxa3xx_nand_timing samsung512MbX16_timing = {
> - .tCH = 10,
> - .tCS = 0,
> - .tWH = 20,
> - .tWP = 40,
...
> +static struct pxa3xx_nand_timing __devinitdata timing[] = {
Better with some comment columns like below:
/* tCH tCS tWH tWP ... */
> + /* Samsung NAND series */
> + { 10, 0, 20, 40, 30, 40, 11123, 110, 10, },
> + /* Micron NAND series */
> + { 10, 25, 15, 25, 15, 30, 25000, 60, 10, },
> + /* ST NAND series */
> + { 10, 35, 15, 25, 15, 25, 25000, 60, 10, },
Now define something like
#define NAND_TIMING_SAMSUNG (&timing[0])
#define NAND_TIMING_MICRON (&timing[1])
#define NAND_TIMING_ST (&timing[2])
> };
>
> -static struct pxa3xx_nand_flash samsung32GbX8 = {
> - .timing = &samsung512MbX16_timing,
> - .cmdset = &smallpage_cmdset,
> - .page_per_block = 128,
> - .page_size = 4096,
> - .flash_width = 8,
> - .dfc_width = 8,
> - .num_blocks = 8192,
> - .chip_id = 0xd7ec,
> +static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = {
/* Chip ID, page per block, page size, flash width, .... */
> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[0], },
> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[0], },
> + { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[0], },
> + { 0xa12c, 64, 2048, 8, 8, 1024, &timing[1], },
> + { 0xb12c, 64, 2048, 16, 16, 1024, &timing[1], },
> + { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[1], },
> + { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[1], },
> + { 0xba20, 64, 2048, 16, 16, 2048, &timing[2], },
> };
BTW, I like the array above very much.
>
> -static struct pxa3xx_nand_timing micron_timing = {
> - .tCH = 10,
> - .tCS = 25,
> - .tWH = 15,
> - .tWP = 25,
> - .tRH = 15,
> - .tRP = 30,
> - .tR = 25000,
... snipped ...
> -};
> -#endif /* CONFIG_MTD_NAND_PXA3xx_BUILTIN */
> -
> #define NDTR0_tCH(c) (min((c), 7) << 19)
> #define NDTR0_tCS(c) (min((c), 7) << 16)
> #define NDTR0_tWH(c) (min((c), 7) << 11)
> @@ -412,7 +317,6 @@ static int prepare_read_prog_cmd(struct
> pxa3xx_nand_info *info,
> uint16_t cmd, int column, int page_addr)
> {
> const struct pxa3xx_nand_flash *f = info->flash_info;
> - const struct pxa3xx_nand_cmdset *cmdset = f->cmdset;
I think it's better to keep a local pointer. Reference within a function
to a global structure is always a bad idea, one day you will find you've
to modify all these references again when you add a new cmdset.
And most of the modifications below can thus be removed and patch simplified.
More information about the linux-arm-kernel
mailing list