[PATCH 01/25] pxa3xx_nand: condense the flash definition

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Lei Wen]

Adding a new flash definition would need less code.
Keep the platform passing flash defition method.
If one flash is both defined in platform data and builtin table,
driver would select the one from platform data first.

By this way, platform could select the timing most suit for itself,
not need to follow the common settings.

Signed-off-by: Lei Wen <leiwen@marvell.com>
Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 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;
-};
-
 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
@@ -113,6 +113,41 @@ enum {
 	STATE_PIO_WRITING,
 };

+struct pxa3xx_nand_timing {
+	uint32_t	tCH;  /* Enable signal hold time */
+	uint32_t	tCS;  /* Enable signal setup time */
+	uint32_t	tWH;  /* ND_nWE high duration */
+	uint32_t	tWP;  /* ND_nWE pulse time */
+	uint32_t	tRH;  /* ND_nRE high duration */
+	uint32_t	tRP;  /* ND_nRE pulse width */
+	uint32_t	tR;   /* ND_nWE high to ND_nRE low for read */
+	uint32_t	tWHR; /* ND_nWE high to ND_nRE low for status read */
+	uint32_t	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 {
+	uint32_t	chip_id;
+	uint16_t	page_per_block; /* Pages per block (PG_PER_BLK) */
+	uint16_t 	page_size;	/* Page size in bytes (PAGE_SZ) */
+	uint8_t		flash_width;	/* Width of Flash memory (DWIDTH_M) */
+	uint8_t 	dfc_width;	/* Width of flash controller(DWIDTH_C) */
+	uint32_t	num_blocks;	/* Number of physical blocks in Flash */
+	struct pxa3xx_nand_timing *timing;	/* NAND Flash timing */
+};
+
 struct pxa3xx_nand_info {
 	struct nand_chip	nand_chip;

@@ -177,20 +212,7 @@ MODULE_PARM_DESC(use_dma, "enable DMA for data
transfering to/from NAND HW");
 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 = {
 	.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,
-	.tRH	= 30,
-	.tRP	= 40,
-	.tR	= 11123,
-	.tWHR	= 110,
-	.tAR	= 10,
-};
-
-static struct pxa3xx_nand_flash samsung512MbX16 = {
-	.timing		= &samsung512MbX16_timing,
-	.cmdset		= &smallpage_cmdset,
-	.page_per_block	= 32,
-	.page_size	= 512,
-	.flash_width	= 16,
-	.dfc_width	= 16,
-	.num_blocks	= 4096,
-	.chip_id	= 0x46ec,
-};
-
-static struct pxa3xx_nand_flash samsung2GbX8 = {
-	.timing		= &samsung512MbX16_timing,
-	.cmdset		= &smallpage_cmdset,
-	.page_per_block	= 64,
-	.page_size	= 2048,
-	.flash_width	= 8,
-	.dfc_width	= 8,
-	.num_blocks	= 2048,
-	.chip_id	= 0xdaec,
+static struct pxa3xx_nand_timing __devinitdata timing[] = {
+	/* 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, },
 };

-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[] = {
+	{ 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], },
 };

-static struct pxa3xx_nand_timing micron_timing = {
-	.tCH	= 10,
-	.tCS	= 25,
-	.tWH	= 15,
-	.tWP	= 25,
-	.tRH	= 15,
-	.tRP	= 30,
-	.tR	= 25000,
-	.tWHR	= 60,
-	.tAR	= 10,
-};
-
-static struct pxa3xx_nand_flash micron1GbX8 = {
-	.timing		= &micron_timing,
-	.cmdset		= &largepage_cmdset,
-	.page_per_block	= 64,
-	.page_size	= 2048,
-	.flash_width	= 8,
-	.dfc_width	= 8,
-	.num_blocks	= 1024,
-	.chip_id	= 0xa12c,
-};
-
-static struct pxa3xx_nand_flash micron1GbX16 = {
-	.timing		= &micron_timing,
-	.cmdset		= &largepage_cmdset,
-	.page_per_block	= 64,
-	.page_size	= 2048,
-	.flash_width	= 16,
-	.dfc_width	= 16,
-	.num_blocks	= 1024,
-	.chip_id	= 0xb12c,
-};
-
-static struct pxa3xx_nand_flash micron4GbX8 = {
-	.timing		= &micron_timing,
-	.cmdset		= &largepage_cmdset,
-	.page_per_block	= 64,
-	.page_size	= 2048,
-	.flash_width	= 8,
-	.dfc_width	= 8,
-	.num_blocks	= 4096,
-	.chip_id	= 0xdc2c,
-};
-
-static struct pxa3xx_nand_flash micron4GbX16 = {
-	.timing		= &micron_timing,
-	.cmdset		= &largepage_cmdset,
-	.page_per_block	= 64,
-	.page_size	= 2048,
-	.flash_width	= 16,
-	.dfc_width	= 16,
-	.num_blocks	= 4096,
-	.chip_id	= 0xcc2c,
-};
-
-static struct pxa3xx_nand_timing stm2GbX16_timing = {
-	.tCH = 10,
-	.tCS = 35,
-	.tWH = 15,
-	.tWP = 25,
-	.tRH = 15,
-	.tRP = 25,
-	.tR = 25000,
-	.tWHR = 60,
-	.tAR = 10,
-};
-
-static struct pxa3xx_nand_flash stm2GbX16 = {
-	.timing = &stm2GbX16_timing,
-	.cmdset	= &largepage_cmdset,
-	.page_per_block = 64,
-	.page_size = 2048,
-	.flash_width = 16,
-	.dfc_width = 16,
-	.num_blocks = 2048,
-	.chip_id = 0xba20,
-};
-
-static struct pxa3xx_nand_flash *builtin_flash_types[] = {
-	&samsung512MbX16,
-	&samsung2GbX8,
-	&samsung32GbX8,
-	&micron1GbX8,
-	&micron1GbX16,
-	&micron4GbX8,
-	&micron4GbX16,
-	&stm2GbX16,
-};
-#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;

 	/* calculate data size */
 	switch (f->page_size) {
@@ -445,7 +349,7 @@ static int prepare_read_prog_cmd(struct
pxa3xx_nand_info *info,
 		 */
 		info->ndcb1 = page_addr << 8;

-	if (cmd == cmdset->program)
+	if (cmd == cmdset.program)
 		info->ndcb0 |= NDCB0_CMD_TYPE(1) | NDCB0_AUTO_RS;

 	return 0;
@@ -463,20 +367,18 @@ static int prepare_erase_cmd(struct
pxa3xx_nand_info *info,

 static int prepare_other_cmd(struct pxa3xx_nand_info *info, uint16_t cmd)
 {
-	const struct pxa3xx_nand_cmdset *cmdset = info->flash_info->cmdset;
-
 	info->ndcb0 = cmd | ((cmd & 0xff00) ? NDCB0_DBC : 0);
 	info->ndcb1 = 0;
 	info->ndcb2 = 0;

-	if (cmd == cmdset->read_id) {
+	if (cmd == cmdset.read_id) {
 		info->ndcb0 |= NDCB0_CMD_TYPE(3);
 		info->data_size = 8;
-	} else if (cmd == cmdset->read_status) {
+	} else if (cmd == cmdset.read_status) {
 		info->ndcb0 |= NDCB0_CMD_TYPE(4);
 		info->data_size = 8;
-	} else if (cmd == cmdset->reset || cmd == cmdset->lock ||
-		   cmd == cmdset->unlock) {
+	} else if (cmd == cmdset.reset || cmd == cmdset.lock ||
+		   cmd == cmdset.unlock) {
 		info->ndcb0 |= NDCB0_CMD_TYPE(5);
 	} else
 		return -EINVAL;
@@ -700,8 +602,6 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info
*mtd, unsigned command,
 				int column, int page_addr)
 {
 	struct pxa3xx_nand_info *info = mtd->priv;
-	const struct pxa3xx_nand_flash *flash_info = info->flash_info;
-	const struct pxa3xx_nand_cmdset *cmdset = flash_info->cmdset;
 	int ret;

 	info->use_dma = (use_dma) ? 1 : 0;
@@ -718,7 +618,7 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info
*mtd, unsigned command,
 		info->buf_start = mtd->writesize + column;
 		memset(info->data_buff, 0xFF, info->buf_count);

-		if (prepare_read_prog_cmd(info, cmdset->read1, column, page_addr))
+		if (prepare_read_prog_cmd(info, cmdset.read1, column, page_addr))
 			break;

 		pxa3xx_nand_do_cmd(info, NDSR_RDDREQ | NDSR_DBERR | NDSR_SBERR);
@@ -735,7 +635,7 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info
*mtd, unsigned command,
 		info->buf_count = mtd->writesize + mtd->oobsize;
 		memset(info->data_buff, 0xFF, info->buf_count);

-		if (prepare_read_prog_cmd(info, cmdset->read1, column, page_addr))
+		if (prepare_read_prog_cmd(info, cmdset.read1, column, page_addr))
 			break;

 		pxa3xx_nand_do_cmd(info, NDSR_RDDREQ | NDSR_DBERR | NDSR_SBERR);
@@ -761,14 +661,14 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info
*mtd, unsigned command,
 	case NAND_CMD_PAGEPROG:
 		info->use_ecc = (info->seqin_column >= mtd->writesize) ? 0 : 1;

-		if (prepare_read_prog_cmd(info, cmdset->program,
+		if (prepare_read_prog_cmd(info, cmdset.program,
 				info->seqin_column, info->seqin_page_addr))
 			break;

 		pxa3xx_nand_do_cmd(info, NDSR_WRDREQ);
 		break;
 	case NAND_CMD_ERASE1:
-		if (prepare_erase_cmd(info, cmdset->erase, page_addr))
+		if (prepare_erase_cmd(info, cmdset.erase, page_addr))
 			break;

 		pxa3xx_nand_do_cmd(info, NDSR_CS0_BBD | NDSR_CS0_CMDD);
@@ -783,13 +683,13 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info
*mtd, unsigned command,
 				info->read_id_bytes : 1;

 		if (prepare_other_cmd(info, (command == NAND_CMD_READID) ?
-				cmdset->read_id : cmdset->read_status))
+				cmdset.read_id : cmdset.read_status))
 			break;

 		pxa3xx_nand_do_cmd(info, NDSR_RDDREQ);
 		break;
 	case NAND_CMD_RESET:
-		if (prepare_other_cmd(info, cmdset->reset))
+		if (prepare_other_cmd(info, cmdset.reset))
 			break;

 		ret = pxa3xx_nand_do_cmd(info, NDSR_CS0_CMDD);
@@ -925,12 +825,10 @@ static int pxa3xx_nand_ecc_correct(struct mtd_info *mtd,

 static int __readid(struct pxa3xx_nand_info *info, uint32_t *id)
 {
-	const struct pxa3xx_nand_flash *f = info->flash_info;
-	const struct pxa3xx_nand_cmdset *cmdset = f->cmdset;
 	uint32_t ndcr;
 	uint8_t  id_buff[8];

-	if (prepare_other_cmd(info, cmdset->read_id)) {
+	if (prepare_other_cmd(info, cmdset.read_id)) {
 		printk(KERN_ERR "failed to prepare command\n");
 		return -EINVAL;
 	}
@@ -1027,11 +925,6 @@ static int pxa3xx_nand_detect_config(struct
pxa3xx_nand_info *info)
 	default_flash.flash_width = ndcr & NDCR_DWIDTH_M ? 16 : 8;
 	default_flash.dfc_width = ndcr & NDCR_DWIDTH_C ? 16 : 8;

-	if (default_flash.page_size == 2048)
-		default_flash.cmdset = &largepage_cmdset;
-	else
-		default_flash.cmdset = &smallpage_cmdset;
-
 	/* set info fields needed to __readid */
 	info->flash_info = &default_flash;
 	info->read_id_bytes = (default_flash.page_size == 2048) ? 4 : 2;
@@ -1067,7 +960,7 @@ static int pxa3xx_nand_detect_config(struct
pxa3xx_nand_info *info)
 		info->row_addr_cycles = 2;

 	pxa3xx_nand_detect_timing(info, &default_timing);
-	default_flash.timing = &default_timing;
+	memcpy(&default_flash.timing, &default_timing, sizeof(default_timing));

 	return 0;
 }
@@ -1096,10 +989,9 @@ static int pxa3xx_nand_detect_flash(struct
pxa3xx_nand_info *info,
 			return 0;
 	}

-#ifdef CONFIG_MTD_NAND_PXA3xx_BUILTIN
 	for (i = 0; i < ARRAY_SIZE(builtin_flash_types); i++) {

-		f = builtin_flash_types[i];
+		f = &builtin_flash_types[i];

 		if (pxa3xx_nand_config_flash(info, f))
 			continue;
@@ -1110,7 +1002,6 @@ static int pxa3xx_nand_detect_flash(struct
pxa3xx_nand_info *info,
 		if (id == f->chip_id)
 			return 0;
 	}
-#endif

 	dev_warn(&info->pdev->dev,
 		 "failed to detect configured nand flash; found %04x instead of\n",
-- 
1.7.0.4

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Haojian Zhuang]

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Eric Miao]

On Fri, Jun 18, 2010 at 1:32 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> From 76684334fbf214ab164ed07791b02535a3f04779 Mon Sep 17 00:00:00 2001
> From: Lei Wen <leiwen@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@marvell.com>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@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.

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Lei Wen]

On Fri, Jun 18, 2010 at 2:23 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Fri, Jun 18, 2010 at 1:32 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> From 76684334fbf214ab164ed07791b02535a3f04779 Mon Sep 17 00:00:00 2001
>> From: Lei Wen <leiwen@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?
Oops... Typo...
>> 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.

Em... I agree...
>
>>
>> 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@marvell.com>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@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?

No, platform could define its own timing. If there is definition for
the same chip,
driver would choose the one from platform definition first.

>
>> ?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?

The only difference of cmdsetbetween the small page and large page is
at the read command.
While large page is 0x3000, small page is 0x0.
This handle is not address in this patch, but put to latter patch of
0006 in this seris patch set.
For whole patch set chnage is so large, that it is not easy to make it
very clear at each patch...



>
>> ? ? ? ?.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, .... */
Agree.

>
>> + ? ? ? { 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.

My original thinking is for the cmdset is almost standardized, so make
such change for global structure.
Well, the suggestion also make sense. I would revert this change.

Best regards,
Lei

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Eric Miao]

>>> -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?
>
> No, platform could define its own timing. If there is definition for
> the same chip,
> driver would choose the one from platform definition first.
>

I'm seeing pxa3xx_nand_flash being referenced in pxa3xx_nand_platform_data
below, though?

struct pxa3xx_nand_platform_data {

      ......

	const struct pxa3xx_nand_flash * 	flash;
	size_t					num_flash;
};

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Lei Wen]

On Fri, Jun 18, 2010 at 4:00 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>>> -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?
>>
>> No, platform could define its own timing. If there is definition for
>> the same chip,
>> driver would choose the one from platform definition first.
>>
>
> I'm seeing pxa3xx_nand_flash being referenced in pxa3xx_nand_platform_data
> below, though?
>
> struct pxa3xx_nand_platform_data {
>
> ? ? ?......
>
> ? ? ? ?const struct pxa3xx_nand_flash * ? ? ? ?flash;
> ? ? ? ?size_t ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?num_flash;
> };
>

Yes, platform could define its own pxa3xx_nand_flash structure in its
own code, and pass the structure to let driver know.

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Eric Miao]

On Fri, Jun 18, 2010 at 4:10 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
> On Fri, Jun 18, 2010 at 4:00 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>>>> -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?
>>>
>>> No, platform could define its own timing. If there is definition for
>>> the same chip,
>>> driver would choose the one from platform definition first.
>>>
>>
>> I'm seeing pxa3xx_nand_flash being referenced in pxa3xx_nand_platform_data
>> below, though?
>>
>> struct pxa3xx_nand_platform_data {
>>
>> ? ? ?......
>>
>> ? ? ? ?const struct pxa3xx_nand_flash * ? ? ? ?flash;
>> ? ? ? ?size_t ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?num_flash;
>> };
>>
>
> Yes, platform could define its own pxa3xx_nand_flash structure in its
> own code, and pass the structure to let driver know.
>

I'm actually quite confused here. Did you ever try build a kernel with
platform defined flash info?

[PATCH 01/25] pxa3xx_nand: condense the flash definition

[Lei Wen]

On Fri, Jun 18, 2010 at 4:14 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Fri, Jun 18, 2010 at 4:10 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
>> On Fri, Jun 18, 2010 at 4:00 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>>>>>> -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?
>>>>
>>>> No, platform could define its own timing. If there is definition for
>>>> the same chip,
>>>> driver would choose the one from platform definition first.
>>>>
>>>
>>> I'm seeing pxa3xx_nand_flash being referenced in pxa3xx_nand_platform_data
>>> below, though?
>>>
>>> struct pxa3xx_nand_platform_data {
>>>
>>> ? ? ?......
>>>
>>> ? ? ? ?const struct pxa3xx_nand_flash * ? ? ? ?flash;
>>> ? ? ? ?size_t ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?num_flash;
>>> };
>>>
>>
>> Yes, platform could define its own pxa3xx_nand_flash structure in its
>> own code, and pass the structure to let driver know.
>>
>
> I'm actually quite confused here. Did you ever try build a kernel with
> platform defined flash info?
>

I haven't tried to build one platform code with flash definition
yet... My fault...
I would correct this error in .h file.