Patchwork [01/20] mtd: pxa3xx_nand: refuse the flash definition get from platform

login
register
mail settings
Submitter Haojian Zhuang
Date May 14, 2010, 6:11 a.m.
Message ID <AANLkTinVBXhT77Zd-LNpufgJAl7WZdic0oxDFwvNkvJg@mail.gmail.com>
Download mbox | patch
Permalink /patch/52547/
State New
Headers show

Comments

Haojian Zhuang - May 14, 2010, 6:11 a.m.
From 16057e690aa4a2fd8a9c07c70ab48ffc2f76204f Mon Sep 17 00:00:00 2001
From: Lei Wen <leiwen@marvell.com>
Date: Sat, 20 Mar 2010 19:01:23 +0800
Subject: [PATCH] mtd: pxa3xx_nand: refuse the flash definition get from platform

For current usage, it is little reason to use a platform defined flash info
for the flash detection. Flash timing through platform should be the same.
And allow multiple platform to define the same flash chip would be a waste.

Also condense the flash definition way in the c file to simplify adding a
new chip.

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 |   40 ----
 drivers/mtd/nand/Kconfig                     |    7 -
 drivers/mtd/nand/pxa3xx_nand.c               |  262 ++++++--------------------
 3 files changed, 60 insertions(+), 249 deletions(-)

 	.program	= 0x1080,
@@ -203,143 +225,17 @@ 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_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, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
+{ 0xdaec, 64, 2048, 8, 8, 2048, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
+{ 0xd7ec, 128, 4096, 8, 8, 8192, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
+{ 0xa12c, 64, 2048, 8, 8, 1024, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
+{ 0xb12c, 64, 2048, 16, 16, 1024, { 10, 25, 15, 25, 15, 30, 25000,
60, 10, }, },
+{ 0xdc2c, 64, 2048, 8, 8, 4096, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
+{ 0xcc2c, 64, 2048, 16, 16, 4096, { 10, 25, 15, 25, 15, 30, 25000,
60, 10, }, },
+{ 0xba20, 64, 2048, 16, 16, 2048, { 10, 35, 15, 25, 15, 25, 25000,
60, 10, }, },
 };

-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 +308,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 +340,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 +358,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 +593,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 +609,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 +626,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 +652,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 +674,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 +816,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;
 	}
@@ -991,7 +880,7 @@ static int pxa3xx_nand_config_flash(struct
pxa3xx_nand_info *info,

 	info->reg_ndcr = ndcr;

-	pxa3xx_nand_set_timing(info, f->timing);
+	pxa3xx_nand_set_timing(info, &f->timing);
 	info->flash_info = f;
 	return 0;
 }
@@ -1027,11 +916,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 +951,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;
 }
@@ -1083,23 +967,9 @@ static int pxa3xx_nand_detect_flash(struct
pxa3xx_nand_info *info,
 		if (pxa3xx_nand_detect_config(info) == 0)
 			return 0;

-	for (i = 0; i<pdata->num_flash; ++i) {
-		f = pdata->flash + i;
-
-		if (pxa3xx_nand_config_flash(info, f))
-			continue;
-
-		if (__readid(info, &id))
-			continue;
-
-		if (id == f->chip_id)
-			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 +980,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",
@@ -1320,17 +1189,6 @@ static int pxa3xx_nand_probe(struct
platform_device *pdev)
 		goto fail_free_irq;
 	}

-	if (mtd_has_cmdlinepart()) {
-		static const char *probes[] = { "cmdlinepart", NULL };
-		struct mtd_partition *parts;
-		int nr_parts;
-
-		nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
-
-		if (nr_parts)
-			return add_mtd_partitions(mtd, parts, nr_parts);
-	}
-
 	return add_mtd_partitions(mtd, pdata->parts, pdata->nr_parts);

 fail_free_irq:
Marc Kleine-Budde - May 14, 2010, 11:09 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Haojian,

Haojian Zhuang wrote:
>>From 16057e690aa4a2fd8a9c07c70ab48ffc2f76204f Mon Sep 17 00:00:00 2001
> From: Lei Wen <leiwen@marvell.com>
> Date: Sat, 20 Mar 2010 19:01:23 +0800
> Subject: [PATCH] mtd: pxa3xx_nand: refuse the flash definition get from platform
> 
> For current usage, it is little reason to use a platform defined flash info
> for the flash detection. Flash timing through platform should be the same.
> And allow multiple platform to define the same flash chip would be a waste.
> 
> Also condense the flash definition way in the c file to simplify adding a
> new chip.
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>

NACK, see inline

> ---
>  arch/arm/plat-pxa/include/plat/pxa3xx_nand.h |   40 ----
>  drivers/mtd/nand/Kconfig                     |    7 -
>  drivers/mtd/nand/pxa3xx_nand.c               |  262 ++++++--------------------
>  3 files changed, 60 insertions(+), 249 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..c494f68 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
> @@ -54,9 +17,6 @@ struct pxa3xx_nand_platform_data {
> 
>  	const struct mtd_partition		*parts;
>  	unsigned int				nr_parts;
> -
> -	const struct pxa3xx_nand_flash * 	flash;
> -	size_t					num_flash;
>  };
> 
>  extern void pxa3xx_set_nand_info(struct pxa3xx_nand_platform_data *info);
> 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..da40b9a 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	tAR;  /* ND_ALE low to ND_nRE low delay */
> +	uint32_t	tWHR; /* ND_nWE high to ND_nRE low for status read */
> +	uint32_t	tR;   /* ND_nWE high to ND_nRE low for read */
> +};
> +
> +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,17 @@ 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_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, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xdaec, 64, 2048, 8, 8, 2048, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xd7ec, 128, 4096, 8, 8, 8192, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xa12c, 64, 2048, 8, 8, 1024, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
> +{ 0xb12c, 64, 2048, 16, 16, 1024, { 10, 25, 15, 25, 15, 30, 25000,
> 60, 10, }, },
> +{ 0xdc2c, 64, 2048, 8, 8, 4096, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
> +{ 0xcc2c, 64, 2048, 16, 16, 4096, { 10, 25, 15, 25, 15, 30, 25000,
> 60, 10, }, },
> +{ 0xba20, 64, 2048, 16, 16, 2048, { 10, 35, 15, 25, 15, 25, 25000,
> 60, 10, }, },
>  };
> 
> -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 +308,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 +340,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 +358,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 +593,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 +609,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 +626,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 +652,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 +674,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 +816,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;
>  	}
> @@ -991,7 +880,7 @@ static int pxa3xx_nand_config_flash(struct
> pxa3xx_nand_info *info,
> 
>  	info->reg_ndcr = ndcr;
> 
> -	pxa3xx_nand_set_timing(info, f->timing);
> +	pxa3xx_nand_set_timing(info, &f->timing);
>  	info->flash_info = f;
>  	return 0;
>  }
> @@ -1027,11 +916,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 +951,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;
>  }
> @@ -1083,23 +967,9 @@ static int pxa3xx_nand_detect_flash(struct
> pxa3xx_nand_info *info,
>  		if (pxa3xx_nand_detect_config(info) == 0)
>  			return 0;
> 
> -	for (i = 0; i<pdata->num_flash; ++i) {
> -		f = pdata->flash + i;
> -
> -		if (pxa3xx_nand_config_flash(info, f))
> -			continue;
> -
> -		if (__readid(info, &id))
> -			continue;
> -
> -		if (id == f->chip_id)
> -			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 +980,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",
> @@ -1320,17 +1189,6 @@ static int pxa3xx_nand_probe(struct
> platform_device *pdev)
>  		goto fail_free_irq;
>  	}
> 
> -	if (mtd_has_cmdlinepart()) {
> -		static const char *probes[] = { "cmdlinepart", NULL };
> -		struct mtd_partition *parts;
> -		int nr_parts;
> -
> -		nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
> -
> -		if (nr_parts)
> -			return add_mtd_partitions(mtd, parts, nr_parts);
> -	}
> -

Why are you removing this? It already works, has the same functionality
as your patch 17 and works without ugly defines.

Please don't remove this and drop your patch 17

>  	return add_mtd_partitions(mtd, pdata->parts, pdata->nr_parts);
> 
>  fail_free_irq:

cheers Marc

- --
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvtL0wACgkQjTAFq1RaXHOWgACdFRMRw1ywiDYcm1CqYKgJErUu
NX8AoIHPVxVPVLB+U9l6c0Umk4nGbdH4
=0Dv/
-----END PGP SIGNATURE-----
Mike Rapoport - May 24, 2010, 7:31 a.m.
Hi Haojian,
This is a comment to the entire series, and should have been Re: [PATCH 
0/20], but there's no cover letter for these patches.

The patches are all line-wrapped and do not apply. It's really hard to 
read them because of hunks added inside the existing functions. Please 
try to organize your changes in a more straight  forward way.

Haojian Zhuang wrote:
> From 16057e690aa4a2fd8a9c07c70ab48ffc2f76204f Mon Sep 17 00:00:00 2001
> From: Lei Wen <leiwen@marvell.com>
> Date: Sat, 20 Mar 2010 19:01:23 +0800
> Subject: [PATCH] mtd: pxa3xx_nand: refuse the flash definition get from platform
> 
> For current usage, it is little reason to use a platform defined flash info
> for the flash detection. Flash timing through platform should be the same.
> And allow multiple platform to define the same flash chip would be a waste.
> 
> Also condense the flash definition way in the c file to simplify adding a
> new chip.
> 
> 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 |   40 ----
>  drivers/mtd/nand/Kconfig                     |    7 -
>  drivers/mtd/nand/pxa3xx_nand.c               |  262 ++++++--------------------
>  3 files changed, 60 insertions(+), 249 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..c494f68 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
> @@ -54,9 +17,6 @@ struct pxa3xx_nand_platform_data {
> 
>  	const struct mtd_partition		*parts;
>  	unsigned int				nr_parts;
> -
> -	const struct pxa3xx_nand_flash * 	flash;
> -	size_t					num_flash;
>  };
> 
>  extern void pxa3xx_set_nand_info(struct pxa3xx_nand_platform_data *info);
> 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..da40b9a 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	tAR;  /* ND_ALE low to ND_nRE low delay */
> +	uint32_t	tWHR; /* ND_nWE high to ND_nRE low for status read */
> +	uint32_t	tR;   /* ND_nWE high to ND_nRE low for read */
> +};
> +
> +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,17 @@ 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_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, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xdaec, 64, 2048, 8, 8, 2048, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xd7ec, 128, 4096, 8, 8, 8192, { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, },
> +{ 0xa12c, 64, 2048, 8, 8, 1024, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
> +{ 0xb12c, 64, 2048, 16, 16, 1024, { 10, 25, 15, 25, 15, 30, 25000,
> 60, 10, }, },
> +{ 0xdc2c, 64, 2048, 8, 8, 4096, { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, },
> +{ 0xcc2c, 64, 2048, 16, 16, 4096, { 10, 25, 15, 25, 15, 30, 25000,
> 60, 10, }, },
> +{ 0xba20, 64, 2048, 16, 16, 2048, { 10, 35, 15, 25, 15, 25, 25000,
> 60, 10, }, },
>  };
> 
> -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 +308,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 +340,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 +358,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 +593,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 +609,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 +626,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 +652,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 +674,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 +816,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;
>  	}
> @@ -991,7 +880,7 @@ static int pxa3xx_nand_config_flash(struct
> pxa3xx_nand_info *info,
> 
>  	info->reg_ndcr = ndcr;
> 
> -	pxa3xx_nand_set_timing(info, f->timing);
> +	pxa3xx_nand_set_timing(info, &f->timing);
>  	info->flash_info = f;
>  	return 0;
>  }
> @@ -1027,11 +916,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 +951,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;
>  }
> @@ -1083,23 +967,9 @@ static int pxa3xx_nand_detect_flash(struct
> pxa3xx_nand_info *info,
>  		if (pxa3xx_nand_detect_config(info) == 0)
>  			return 0;
> 
> -	for (i = 0; i<pdata->num_flash; ++i) {
> -		f = pdata->flash + i;
> -
> -		if (pxa3xx_nand_config_flash(info, f))
> -			continue;
> -
> -		if (__readid(info, &id))
> -			continue;
> -
> -		if (id == f->chip_id)
> -			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 +980,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",
> @@ -1320,17 +1189,6 @@ static int pxa3xx_nand_probe(struct
> platform_device *pdev)
>  		goto fail_free_irq;
>  	}
> 
> -	if (mtd_has_cmdlinepart()) {
> -		static const char *probes[] = { "cmdlinepart", NULL };
> -		struct mtd_partition *parts;
> -		int nr_parts;
> -
> -		nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
> -
> -		if (nr_parts)
> -			return add_mtd_partitions(mtd, parts, nr_parts);
> -	}
> -
>  	return add_mtd_partitions(mtd, pdata->parts, pdata->nr_parts);
> 
>  fail_free_irq:
Lei Wen - May 24, 2010, 8:27 a.m.
Hi Mike,

This patch set is applied to mtd-2.6 git. We submit the patch with a
package in attachment already.
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818

Best regards,
Lei

On Mon, May 24, 2010 at 3:31 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Hi Haojian,
> This is a comment to the entire series, and should have been Re: [PATCH
> 0/20], but there's no cover letter for these patches.
>
> The patches are all line-wrapped and do not apply. It's really hard to read
> them because of hunks added inside the existing functions. Please try to
> organize your changes in a more straight  forward way.
>
> Haojian Zhuang wrote:
>>
>> From 16057e690aa4a2fd8a9c07c70ab48ffc2f76204f Mon Sep 17 00:00:00 2001
>> From: Lei Wen <leiwen@marvell.com>
>> Date: Sat, 20 Mar 2010 19:01:23 +0800
>> Subject: [PATCH] mtd: pxa3xx_nand: refuse the flash definition get from
>> platform
>>
>> For current usage, it is little reason to use a platform defined flash
>> info
>> for the flash detection. Flash timing through platform should be the same.
>> And allow multiple platform to define the same flash chip would be a
>> waste.
>>
>> Also condense the flash definition way in the c file to simplify adding a
>> new chip.
>>
>> 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 |   40 ----
>>  drivers/mtd/nand/Kconfig                     |    7 -
>>  drivers/mtd/nand/pxa3xx_nand.c               |  262
>> ++++++--------------------
>>  3 files changed, 60 insertions(+), 249 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..c494f68 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
>> @@ -54,9 +17,6 @@ struct pxa3xx_nand_platform_data {
>>
>>        const struct mtd_partition              *parts;
>>        unsigned int                            nr_parts;
>> -
>> -       const struct pxa3xx_nand_flash *        flash;
>> -       size_t                                  num_flash;
>>  };
>>
>>  extern void pxa3xx_set_nand_info(struct pxa3xx_nand_platform_data *info);
>> 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..da40b9a 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        tAR;  /* ND_ALE low to ND_nRE low delay */
>> +       uint32_t        tWHR; /* ND_nWE high to ND_nRE low for status read
>> */
>> +       uint32_t        tR;   /* ND_nWE high to ND_nRE low for read */
>> +};
>> +
>> +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,17 @@ 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_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, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>> }, },
>> +{ 0xdaec, 64, 2048, 8, 8, 2048, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>> }, },
>> +{ 0xd7ec, 128, 4096, 8, 8, 8192, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>> }, },
>> +{ 0xa12c, 64, 2048, 8, 8, 1024, { 10, 25, 15, 25, 15, 30, 25000, 60, 10,
>> }, },
>> +{ 0xb12c, 64, 2048, 16, 16, 1024, { 10, 25, 15, 25, 15, 30, 25000,
>> 60, 10, }, },
>> +{ 0xdc2c, 64, 2048, 8, 8, 4096, { 10, 25, 15, 25, 15, 30, 25000, 60, 10,
>> }, },
>> +{ 0xcc2c, 64, 2048, 16, 16, 4096, { 10, 25, 15, 25, 15, 30, 25000,
>> 60, 10, }, },
>> +{ 0xba20, 64, 2048, 16, 16, 2048, { 10, 35, 15, 25, 15, 25, 25000,
>> 60, 10, }, },
>>  };
>>
>> -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 +308,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 +340,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 +358,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 +593,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 +609,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 +626,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 +652,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 +674,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 +816,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;
>>        }
>> @@ -991,7 +880,7 @@ static int pxa3xx_nand_config_flash(struct
>> pxa3xx_nand_info *info,
>>
>>        info->reg_ndcr = ndcr;
>>
>> -       pxa3xx_nand_set_timing(info, f->timing);
>> +       pxa3xx_nand_set_timing(info, &f->timing);
>>        info->flash_info = f;
>>        return 0;
>>  }
>> @@ -1027,11 +916,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 +951,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;
>>  }
>> @@ -1083,23 +967,9 @@ static int pxa3xx_nand_detect_flash(struct
>> pxa3xx_nand_info *info,
>>                if (pxa3xx_nand_detect_config(info) == 0)
>>                        return 0;
>>
>> -       for (i = 0; i<pdata->num_flash; ++i) {
>> -               f = pdata->flash + i;
>> -
>> -               if (pxa3xx_nand_config_flash(info, f))
>> -                       continue;
>> -
>> -               if (__readid(info, &id))
>> -                       continue;
>> -
>> -               if (id == f->chip_id)
>> -                       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 +980,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",
>> @@ -1320,17 +1189,6 @@ static int pxa3xx_nand_probe(struct
>> platform_device *pdev)
>>                goto fail_free_irq;
>>        }
>>
>> -       if (mtd_has_cmdlinepart()) {
>> -               static const char *probes[] = { "cmdlinepart", NULL };
>> -               struct mtd_partition *parts;
>> -               int nr_parts;
>> -
>> -               nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
>> -
>> -               if (nr_parts)
>> -                       return add_mtd_partitions(mtd, parts, nr_parts);
>> -       }
>> -
>>        return add_mtd_partitions(mtd, pdata->parts, pdata->nr_parts);
>>
>>  fail_free_irq:
>
>
> --
> Sincerely yours,
> Mike.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Mike Rapoport - May 24, 2010, 11 a.m.
Hi Lei,

Lei Wen wrote:
> Hi Mike,
> 
> This patch set is applied to mtd-2.6 git. We submit the patch with a
> package in attachment already.
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818

After applying the patch set I've reviewed the entire pxa3xx-nand as a 
whole and there are several major points I don't like:
1) Two chip selects support is not robust enough. You allocate a lot of 
resources for both chip selects, although not necessarily both have NAND 
chip connected
2) I don't like hadrcoding of NAND parameters into the driver. You 
remove *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option 
and instead you enforce use of built-in definitions. The driver in its 
current state is robust enough to allow platforms to define optimized 
NAND timings either in the bootloader or in the kernel. If you don't 
like that multiple platforms define the same flash chip create an 
enumeration of built-in types and let platforms to use this enumeration 
to select the NAND chip. But, anyway, there should be a fallback mode 
that will support NAND chips that are not defined in the driver, 
probably with suboptimal timings.
3) The functions prepare_command_pool and alloc_nand_resource seem 
overgrown too me. Consolidation of prepare_*_cmd into one huge function 
does not seem right. And mixing between resource allocation and mtd 
struct initialization does not seem right either.


> Best regards,
> Lei
> 
> On Mon, May 24, 2010 at 3:31 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> Hi Haojian,
>> This is a comment to the entire series, and should have been Re: [PATCH
>> 0/20], but there's no cover letter for these patches.
>>
>> The patches are all line-wrapped and do not apply. It's really hard to read
>> them because of hunks added inside the existing functions. Please try to
>> organize your changes in a more straight  forward way.
>>
>> Haojian Zhuang wrote:
>>> From 16057e690aa4a2fd8a9c07c70ab48ffc2f76204f Mon Sep 17 00:00:00 2001
>>> From: Lei Wen <leiwen@marvell.com>
>>> Date: Sat, 20 Mar 2010 19:01:23 +0800
>>> Subject: [PATCH] mtd: pxa3xx_nand: refuse the flash definition get from
>>> platform
>>>
>>> For current usage, it is little reason to use a platform defined flash
>>> info
>>> for the flash detection. Flash timing through platform should be the same.
>>> And allow multiple platform to define the same flash chip would be a
>>> waste.
>>>
>>> Also condense the flash definition way in the c file to simplify adding a
>>> new chip.
>>>
>>> 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 |   40 ----
>>>  drivers/mtd/nand/Kconfig                     |    7 -
>>>  drivers/mtd/nand/pxa3xx_nand.c               |  262
>>> ++++++--------------------
>>>  3 files changed, 60 insertions(+), 249 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..c494f68 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
>>> @@ -54,9 +17,6 @@ struct pxa3xx_nand_platform_data {
>>>
>>>        const struct mtd_partition              *parts;
>>>        unsigned int                            nr_parts;
>>> -
>>> -       const struct pxa3xx_nand_flash *        flash;
>>> -       size_t                                  num_flash;
>>>  };
>>>
>>>  extern void pxa3xx_set_nand_info(struct pxa3xx_nand_platform_data *info);
>>> 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..da40b9a 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        tAR;  /* ND_ALE low to ND_nRE low delay */
>>> +       uint32_t        tWHR; /* ND_nWE high to ND_nRE low for status read
>>> */
>>> +       uint32_t        tR;   /* ND_nWE high to ND_nRE low for read */
>>> +};
>>> +
>>> +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,17 @@ 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_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, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>>> }, },
>>> +{ 0xdaec, 64, 2048, 8, 8, 2048, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>>> }, },
>>> +{ 0xd7ec, 128, 4096, 8, 8, 8192, { 10, 0, 20, 40, 30, 40, 11123, 110, 10,
>>> }, },
>>> +{ 0xa12c, 64, 2048, 8, 8, 1024, { 10, 25, 15, 25, 15, 30, 25000, 60, 10,
>>> }, },
>>> +{ 0xb12c, 64, 2048, 16, 16, 1024, { 10, 25, 15, 25, 15, 30, 25000,
>>> 60, 10, }, },
>>> +{ 0xdc2c, 64, 2048, 8, 8, 4096, { 10, 25, 15, 25, 15, 30, 25000, 60, 10,
>>> }, },
>>> +{ 0xcc2c, 64, 2048, 16, 16, 4096, { 10, 25, 15, 25, 15, 30, 25000,
>>> 60, 10, }, },
>>> +{ 0xba20, 64, 2048, 16, 16, 2048, { 10, 35, 15, 25, 15, 25, 25000,
>>> 60, 10, }, },
>>>  };
>>>
>>> -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 +308,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 +340,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 +358,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 +593,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 +609,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 +626,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 +652,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 +674,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 +816,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;
>>>        }
>>> @@ -991,7 +880,7 @@ static int pxa3xx_nand_config_flash(struct
>>> pxa3xx_nand_info *info,
>>>
>>>        info->reg_ndcr = ndcr;
>>>
>>> -       pxa3xx_nand_set_timing(info, f->timing);
>>> +       pxa3xx_nand_set_timing(info, &f->timing);
>>>        info->flash_info = f;
>>>        return 0;
>>>  }
>>> @@ -1027,11 +916,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 +951,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;
>>>  }
>>> @@ -1083,23 +967,9 @@ static int pxa3xx_nand_detect_flash(struct
>>> pxa3xx_nand_info *info,
>>>                if (pxa3xx_nand_detect_config(info) == 0)
>>>                        return 0;
>>>
>>> -       for (i = 0; i<pdata->num_flash; ++i) {
>>> -               f = pdata->flash + i;
>>> -
>>> -               if (pxa3xx_nand_config_flash(info, f))
>>> -                       continue;
>>> -
>>> -               if (__readid(info, &id))
>>> -                       continue;
>>> -
>>> -               if (id == f->chip_id)
>>> -                       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 +980,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",
>>> @@ -1320,17 +1189,6 @@ static int pxa3xx_nand_probe(struct
>>> platform_device *pdev)
>>>                goto fail_free_irq;
>>>        }
>>>
>>> -       if (mtd_has_cmdlinepart()) {
>>> -               static const char *probes[] = { "cmdlinepart", NULL };
>>> -               struct mtd_partition *parts;
>>> -               int nr_parts;
>>> -
>>> -               nr_parts = parse_mtd_partitions(mtd, probes, &parts, 0);
>>> -
>>> -               if (nr_parts)
>>> -                       return add_mtd_partitions(mtd, parts, nr_parts);
>>> -       }
>>> -
>>>        return add_mtd_partitions(mtd, pdata->parts, pdata->nr_parts);
>>>
>>>  fail_free_irq:
>>
>> --
>> Sincerely yours,
>> Mike.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
Lei Wen - May 24, 2010, 11:53 a.m.
Hi Mike,

On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Hi Lei,
>
> Lei Wen wrote:
>>
>> Hi Mike,
>>
>> This patch set is applied to mtd-2.6 git. We submit the patch with a
>> package in attachment already.
>> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818
>
> After applying the patch set I've reviewed the entire pxa3xx-nand as a whole
> and there are several major points I don't like:
> 1) Two chip selects support is not robust enough. You allocate a lot of
> resources for both chip selects, although not necessarily both have NAND
> chip connected

I agree. I prepare to submit another patch set to fix it. Let more
resource go to pxa3xx_nand structure instead of pxa3xx_nand_info.

> 2) I don't like hadrcoding of NAND parameters into the driver. You remove
> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and instead
> you enforce use of built-in definitions. The driver in its current state is
> robust enough to allow platforms to define optimized NAND timings either in
> the bootloader or in the kernel. If you don't like that multiple platforms
> define the same flash chip create an enumeration of built-in types and let
> platforms to use this enumeration to select the NAND chip. But, anyway,
> there should be a fallback mode that will support NAND chips that are not
> defined in the driver, probably with suboptimal timings.

Original driver also use hardcoding. And in bootloader, this timing
parameter is also hard coding.
We cannot deduce a parameter set only from the nand id, that is why we
need a table to preset it.
If one nand chip is not listed in that table, the chip id would still
be printed out, so that we can do something for that.
If we encourage people to continue on this, we would not able to
really "driver" that nand.

As I said, different nand chip may have different requirement. And in
bootloader and kernel, may have different requirement
of timing parameter.


> 3) The functions prepare_command_pool and alloc_nand_resource seem overgrown
> too me. Consolidation of prepare_*_cmd into one huge function does not seem
> right. And mixing between resource allocation and mtd struct initialization
> does not seem right either.

The reason why I consolidate those prepare_*_cmd into one is for if
separate into several functions, it would create many code
duplication.
And with one function, the code execution path would be always one
way. This would greatly promote the code quality, for the same code
path is run by many commands in the same time. If not by this, some
errors may not be discovered in the first time...

Thanks,
Lei
Marek Vasut - May 24, 2010, 12:11 p.m.
Dne Po 24. května 2010 13:53:46 Lei Wen napsal(a):
> Hi Mike,
> 
> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> > Hi Lei,
> > 
> > Lei Wen wrote:
> >> Hi Mike,
> >> 
> >> This patch set is applied to mtd-2.6 git. We submit the patch with a
> >> package in attachment already.
> >> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818
> > 
> > After applying the patch set I've reviewed the entire pxa3xx-nand as a
> > whole and there are several major points I don't like:
> > 1) Two chip selects support is not robust enough. You allocate a lot of
> > resources for both chip selects, although not necessarily both have NAND
> > chip connected
> 
> I agree. I prepare to submit another patch set to fix it. Let more
> resource go to pxa3xx_nand structure instead of pxa3xx_nand_info.
> 
> > 2) I don't like hadrcoding of NAND parameters into the driver. You remove
> > *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
> > instead you enforce use of built-in definitions. The driver in its
> > current state is robust enough to allow platforms to define optimized
> > NAND timings either in the bootloader or in the kernel. If you don't
> > like that multiple platforms define the same flash chip create an
> > enumeration of built-in types and let platforms to use this enumeration
> > to select the NAND chip. But, anyway, there should be a fallback mode
> > that will support NAND chips that are not defined in the driver,
> > probably with suboptimal timings.
> 
> Original driver also use hardcoding. And in bootloader, this timing
> parameter is also hard coding.

Not necessarily. If you use uboot on pxa3xx, it passes the bootrom-detected 
timing to the kernel.

> We cannot deduce a parameter set only from the nand id, that is why we
> need a table to preset it.
> If one nand chip is not listed in that table, the chip id would still
> be printed out, so that we can do something for that.
> If we encourage people to continue on this, we would not able to
> really "driver" that nand.
> 
> As I said, different nand chip may have different requirement. And in
> bootloader and kernel, may have different requirement
> of timing parameter.

In bootloader and kernel? Why would that be so?
> 
> > 3) The functions prepare_command_pool and alloc_nand_resource seem
> > overgrown too me. Consolidation of prepare_*_cmd into one huge function
> > does not seem right. And mixing between resource allocation and mtd
> > struct initialization does not seem right either.
> 
> The reason why I consolidate those prepare_*_cmd into one is for if
> separate into several functions, it would create many code
> duplication.
> And with one function, the code execution path would be always one
> way. This would greatly promote the code quality, for the same code
> path is run by many commands in the same time. If not by this, some
> errors may not be discovered in the first time...
> 
> Thanks,
> Lei
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Lei Wen - May 24, 2010, 12:31 p.m.
Hi Marek,

On Mon, May 24, 2010 at 8:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dne Po 24. května 2010 13:53:46 Lei Wen napsal(a):
>> Hi Mike,
>>
>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> > Hi Lei,
>> >
>> > Lei Wen wrote:
>> >> Hi Mike,
>> >>
>> >> This patch set is applied to mtd-2.6 git. We submit the patch with a
>> >> package in attachment already.
>> >> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818
>> >
>> > After applying the patch set I've reviewed the entire pxa3xx-nand as a
>> > whole and there are several major points I don't like:
>> > 1) Two chip selects support is not robust enough. You allocate a lot of
>> > resources for both chip selects, although not necessarily both have NAND
>> > chip connected
>>
>> I agree. I prepare to submit another patch set to fix it. Let more
>> resource go to pxa3xx_nand structure instead of pxa3xx_nand_info.
>>
>> > 2) I don't like hadrcoding of NAND parameters into the driver. You remove
>> > *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>> > instead you enforce use of built-in definitions. The driver in its
>> > current state is robust enough to allow platforms to define optimized
>> > NAND timings either in the bootloader or in the kernel. If you don't
>> > like that multiple platforms define the same flash chip create an
>> > enumeration of built-in types and let platforms to use this enumeration
>> > to select the NAND chip. But, anyway, there should be a fallback mode
>> > that will support NAND chips that are not defined in the driver,
>> > probably with suboptimal timings.
>>
>> Original driver also use hardcoding. And in bootloader, this timing
>> parameter is also hard coding.
>
> Not necessarily. If you use uboot on pxa3xx, it passes the bootrom-detected
> timing to the kernel.
>
>> We cannot deduce a parameter set only from the nand id, that is why we
>> need a table to preset it.
>> If one nand chip is not listed in that table, the chip id would still
>> be printed out, so that we can do something for that.
>> If we encourage people to continue on this, we would not able to
>> really "driver" that nand.
>>
>> As I said, different nand chip may have different requirement. And in
>> bootloader and kernel, may have different requirement
>> of timing parameter.
>
> In bootloader and kernel? Why would that be so?

The bootrom timing setting is not very satisfied. We have compared the
most optimized timing setting and what the timing setting is set in
bootrom. The read speed would be several times gap.(3x, or 4x). For
kernel is the place we want the most optimized performance, we may
adjust the timing according to our need.

Thanks,
Lei


>>
>> > 3) The functions prepare_command_pool and alloc_nand_resource seem
>> > overgrown too me. Consolidation of prepare_*_cmd into one huge function
>> > does not seem right. And mixing between resource allocation and mtd
>> > struct initialization does not seem right either.
>>
>> The reason why I consolidate those prepare_*_cmd into one is for if
>> separate into several functions, it would create many code
>> duplication.
>> And with one function, the code execution path would be always one
>> way. This would greatly promote the code quality, for the same code
>> path is run by many commands in the same time. If not by this, some
>> errors may not be discovered in the first time...
>>
>> Thanks,
>> Lei
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Marek Vasut - May 24, 2010, 1:05 p.m.
Dne Po 24. května 2010 14:31:39 Lei Wen napsal(a):
> Hi Marek,
> 
> On Mon, May 24, 2010 at 8:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Dne Po 24. května 2010 13:53:46 Lei Wen napsal(a):
> >> Hi Mike,
> >> 
> >> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> >> > Hi Lei,
> >> > 
> >> > Lei Wen wrote:
> >> >> Hi Mike,
> >> >> 
> >> >> This patch set is applied to mtd-2.6 git. We submit the patch with a
> >> >> package in attachment already.
> >> >> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818
> >> > 
> >> > After applying the patch set I've reviewed the entire pxa3xx-nand as a
> >> > whole and there are several major points I don't like:
> >> > 1) Two chip selects support is not robust enough. You allocate a lot
> >> > of resources for both chip selects, although not necessarily both
> >> > have NAND chip connected
> >> 
> >> I agree. I prepare to submit another patch set to fix it. Let more
> >> resource go to pxa3xx_nand structure instead of pxa3xx_nand_info.
> >> 
> >> > 2) I don't like hadrcoding of NAND parameters into the driver. You
> >> > remove *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration
> >> > option and instead you enforce use of built-in definitions. The
> >> > driver in its current state is robust enough to allow platforms to
> >> > define optimized NAND timings either in the bootloader or in the
> >> > kernel. If you don't like that multiple platforms define the same
> >> > flash chip create an enumeration of built-in types and let platforms
> >> > to use this enumeration to select the NAND chip. But, anyway, there
> >> > should be a fallback mode that will support NAND chips that are not
> >> > defined in the driver, probably with suboptimal timings.
> >> 
> >> Original driver also use hardcoding. And in bootloader, this timing
> >> parameter is also hard coding.
> > 
> > Not necessarily. If you use uboot on pxa3xx, it passes the
> > bootrom-detected timing to the kernel.
> > 
> >> We cannot deduce a parameter set only from the nand id, that is why we
> >> need a table to preset it.
> >> If one nand chip is not listed in that table, the chip id would still
> >> be printed out, so that we can do something for that.
> >> If we encourage people to continue on this, we would not able to
> >> really "driver" that nand.
> >> 
> >> As I said, different nand chip may have different requirement. And in
> >> bootloader and kernel, may have different requirement
> >> of timing parameter.
> > 
> > In bootloader and kernel? Why would that be so?
> 
> The bootrom timing setting is not very satisfied. We have compared the
> most optimized timing setting and what the timing setting is set in
> bootrom. The read speed would be several times gap.(3x, or 4x).

That's expectable.

> For kernel is the place we want the most optimized performance, we may
> adjust the timing according to our need.

Yeah, that I'm aware of. But if that's the case, why don't you move the timing 
setup into the bootloader altogether and be done with it ? Kernel relies on the 
BootROM if the NAND chip definition is missing ... therefore if the platform set 
it up correctly in the bootloader, all this goo could go away from the driver. 
(And you can possibly only leave that 'platform can adjust NAND timing' part for 
worst cases).

Maybe I'm missing something, it's hard to review. If that's the case, just 
ignore what I said.

Cheers!
> 
> Thanks,
> Lei
> 
> >> > 3) The functions prepare_command_pool and alloc_nand_resource seem
> >> > overgrown too me. Consolidation of prepare_*_cmd into one huge
> >> > function does not seem right. And mixing between resource allocation
> >> > and mtd struct initialization does not seem right either.
> >> 
> >> The reason why I consolidate those prepare_*_cmd into one is for if
> >> separate into several functions, it would create many code
> >> duplication.
> >> And with one function, the code execution path would be always one
> >> way. This would greatly promote the code quality, for the same code
> >> path is run by many commands in the same time. If not by this, some
> >> errors may not be discovered in the first time...
> >> 
> >> Thanks,
> >> Lei
> >> 
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Lei Wen - May 24, 2010, 1:18 p.m.
On Mon, May 24, 2010 at 9:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dne Po 24. května 2010 14:31:39 Lei Wen napsal(a):
>> Hi Marek,
>>
>> On Mon, May 24, 2010 at 8:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > Dne Po 24. května 2010 13:53:46 Lei Wen napsal(a):
>> >> Hi Mike,
>> >>
>> >> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> >> > Hi Lei,
>> >> >
>> >> > Lei Wen wrote:
>> >> >> Hi Mike,
>> >> >>
>> >> >> This patch set is applied to mtd-2.6 git. We submit the patch with a
>> >> >> package in attachment already.
>> >> >> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/79818
>> >> >
>> >> > After applying the patch set I've reviewed the entire pxa3xx-nand as a
>> >> > whole and there are several major points I don't like:
>> >> > 1) Two chip selects support is not robust enough. You allocate a lot
>> >> > of resources for both chip selects, although not necessarily both
>> >> > have NAND chip connected
>> >>
>> >> I agree. I prepare to submit another patch set to fix it. Let more
>> >> resource go to pxa3xx_nand structure instead of pxa3xx_nand_info.
>> >>
>> >> > 2) I don't like hadrcoding of NAND parameters into the driver. You
>> >> > remove *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration
>> >> > option and instead you enforce use of built-in definitions. The
>> >> > driver in its current state is robust enough to allow platforms to
>> >> > define optimized NAND timings either in the bootloader or in the
>> >> > kernel. If you don't like that multiple platforms define the same
>> >> > flash chip create an enumeration of built-in types and let platforms
>> >> > to use this enumeration to select the NAND chip. But, anyway, there
>> >> > should be a fallback mode that will support NAND chips that are not
>> >> > defined in the driver, probably with suboptimal timings.
>> >>
>> >> Original driver also use hardcoding. And in bootloader, this timing
>> >> parameter is also hard coding.
>> >
>> > Not necessarily. If you use uboot on pxa3xx, it passes the
>> > bootrom-detected timing to the kernel.
>> >
>> >> We cannot deduce a parameter set only from the nand id, that is why we
>> >> need a table to preset it.
>> >> If one nand chip is not listed in that table, the chip id would still
>> >> be printed out, so that we can do something for that.
>> >> If we encourage people to continue on this, we would not able to
>> >> really "driver" that nand.
>> >>
>> >> As I said, different nand chip may have different requirement. And in
>> >> bootloader and kernel, may have different requirement
>> >> of timing parameter.
>> >
>> > In bootloader and kernel? Why would that be so?
>>
>> The bootrom timing setting is not very satisfied. We have compared the
>> most optimized timing setting and what the timing setting is set in
>> bootrom. The read speed would be several times gap.(3x, or 4x).
>
> That's expectable.
>
>> For kernel is the place we want the most optimized performance, we may
>> adjust the timing according to our need.
>
> Yeah, that I'm aware of. But if that's the case, why don't you move the timing
> setup into the bootloader altogether and be done with it ? Kernel relies on the
> BootROM if the NAND chip definition is missing ... therefore if the platform set
> it up correctly in the bootloader, all this goo could go away from the driver.
> (And you can possibly only leave that 'platform can adjust NAND timing' part for
> worst cases).
>
> Maybe I'm missing something, it's hard to review. If that's the case, just
> ignore what I said.

This back to what Mike ask me... The reason why we need the
built_in_table, is for we need tell what the special nand chip want
which cannot be gotten from bootloader setting. For the nand chip may
need higher ecc strength, like 8bit per 512 bytes. This require
kernel use different approach to deal with.

It is better that kernel could be standalone, so that we would be
troubled by the mistake of bootloader. For UBOOT or blob, it is not a
must thing, we could load kernel directly from OBM. (If you want to do
that, like kernel as bootloader...)

Thanks,
Lei
Mike Rapoport - May 24, 2010, 1:21 p.m.
Lei Wen wrote:
> Hi Mike,
> 
> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> Hi Lei,
>>
>> Lei Wen wrote:
>>> Hi Mike,
>>>
>> 2) I don't like hadrcoding of NAND parameters into the driver. You remove
>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and instead
>> you enforce use of built-in definitions. The driver in its current state is
>> robust enough to allow platforms to define optimized NAND timings either in
>> the bootloader or in the kernel. If you don't like that multiple platforms
>> define the same flash chip create an enumeration of built-in types and let
>> platforms to use this enumeration to select the NAND chip. But, anyway,
>> there should be a fallback mode that will support NAND chips that are not
>> defined in the driver, probably with suboptimal timings.
> 
> Original driver also use hardcoding. And in bootloader, this timing
> parameter is also hard coding.
> We cannot deduce a parameter set only from the nand id, that is why we
> need a table to preset it.
> If one nand chip is not listed in that table, the chip id would still
> be printed out, so that we can do something for that.
> If we encourage people to continue on this, we would not able to
> really "driver" that nand.

Currently pxa3xx-nand has three operational modes:
- use NAND parameters supplied by the platform
- use presets configured by the bootloader chain
- use built-in NAND parameters, marked as deprecated in favor of the 
first two
You remove the first two modes completely and require that each and 
every NAND chip used on pxa3xx based platform will be added to the 
driver. This way you make the driver less robust and harder to use for 
platform developers, not mentioning you're breaking the existing platforms.
In my opinion, the driver *may* support built-in definitions for certain 
NAND flashes and *must* support configuration of the NAND parameters by 
the platform code and bootloader.

> As I said, different nand chip may have different requirement. And in
> bootloader and kernel, may have different requirement
> of timing parameter.
> 
> 
>> 3) The functions prepare_command_pool and alloc_nand_resource seem overgrown
>> too me. Consolidation of prepare_*_cmd into one huge function does not seem
>> right. And mixing between resource allocation and mtd struct initialization
>> does not seem right either.
> 
> The reason why I consolidate those prepare_*_cmd into one is for if
> separate into several functions, it would create many code
> duplication.
> And with one function, the code execution path would be always one
> way. This would greatly promote the code quality, for the same code
> path is run by many commands in the same time. If not by this, some
> errors may not be discovered in the first time...
> 
> Thanks,
> Lei
Lei Wen - May 24, 2010, 1:40 p.m.
On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Lei Wen wrote:
>>
>> Hi Mike,
>>
>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
>> wrote:
>>>
>>> Hi Lei,
>>>
>>> Lei Wen wrote:
>>>>
>>>> Hi Mike,
>>>>
>>> 2) I don't like hadrcoding of NAND parameters into the driver. You remove
>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>> instead
>>> you enforce use of built-in definitions. The driver in its current state
>>> is
>>> robust enough to allow platforms to define optimized NAND timings either
>>> in
>>> the bootloader or in the kernel. If you don't like that multiple
>>> platforms
>>> define the same flash chip create an enumeration of built-in types and
>>> let
>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>> there should be a fallback mode that will support NAND chips that are not
>>> defined in the driver, probably with suboptimal timings.
>>
>> Original driver also use hardcoding. And in bootloader, this timing
>> parameter is also hard coding.
>> We cannot deduce a parameter set only from the nand id, that is why we
>> need a table to preset it.
>> If one nand chip is not listed in that table, the chip id would still
>> be printed out, so that we can do something for that.
>> If we encourage people to continue on this, we would not able to
>> really "driver" that nand.
>
> Currently pxa3xx-nand has three operational modes:
> - use NAND parameters supplied by the platform
> - use presets configured by the bootloader chain
> - use built-in NAND parameters, marked as deprecated in favor of the first
> two
> You remove the first two modes completely and require that each and every
> NAND chip used on pxa3xx based platform will be added to the driver. This
> way you make the driver less robust and harder to use for platform
> developers, not mentioning you're breaking the existing platforms.
> In my opinion, the driver *may* support built-in definitions for certain
> NAND flashes and *must* support configuration of the NAND parameters by the
> platform code and bootloader.
>

Hi Mike,

Well... I would submit another patch set which would reserve a way
that platform could pass its parameter setting.
Like specify the certain type of nand chip parameter for each chip
select. Is that ok for you?

For bootloader pass parameter method, I think this way should be
dropped... For there is attributed which we could not tell from
registers...
What do you think of this?

Thanks,
Lei
Daniel Mack - May 24, 2010, 3:44 p.m.
On Mon, May 24, 2010 at 09:40:57PM +0800, Lei Wen wrote:
> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> > Currently pxa3xx-nand has three operational modes:
> > - use NAND parameters supplied by the platform
> > - use presets configured by the bootloader chain
> > - use built-in NAND parameters, marked as deprecated in favor of the first
> > two
> > You remove the first two modes completely and require that each and every
> > NAND chip used on pxa3xx based platform will be added to the driver. This
> > way you make the driver less robust and harder to use for platform
> > developers, not mentioning you're breaking the existing platforms.
> > In my opinion, the driver *may* support built-in definitions for certain
> > NAND flashes and *must* support configuration of the NAND parameters by the
> > platform code and bootloader.
> >
> 
> Hi Mike,
> 
> Well... I would submit another patch set which would reserve a way
> that platform could pass its parameter setting.
> Like specify the certain type of nand chip parameter for each chip
> select. Is that ok for you?
> 
> For bootloader pass parameter method, I think this way should be
> dropped... For there is attributed which we could not tell from
> registers...

No, please don't drop this, as critical procedures in production lines
depend on this. Think about replacing a certain type of NAND chip in the
middle of a production. You would probably need to update the bootloader
for that in order to make the PXA boot from NAND. That's bad enough.

In the current system, you're done by just providing a bootloader which
sets up the NAND correctly. If you deprecate that way, the kernel would
also need an update, for actually no good reason, as all necessary
settings have already been cared for earlier. While that's not the end
of the world, it's still extra work that can be avoided.

I still haven't got your point for removing this feature. Why not leave
it in?

Daniel
Mike Rapoport - May 25, 2010, 10:21 a.m.
Lei Wen wrote:
> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> Lei Wen wrote:
>>> Hi Mike,
>>>
>>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
>>> wrote:
>>>> Hi Lei,
>>>>
>>>> Lei Wen wrote:
>>>>> Hi Mike,
>>>>>
>>>> 2) I don't like hadrcoding of NAND parameters into the driver. You remove
>>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>>> instead
>>>> you enforce use of built-in definitions. The driver in its current state
>>>> is
>>>> robust enough to allow platforms to define optimized NAND timings either
>>>> in
>>>> the bootloader or in the kernel. If you don't like that multiple
>>>> platforms
>>>> define the same flash chip create an enumeration of built-in types and
>>>> let
>>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>>> there should be a fallback mode that will support NAND chips that are not
>>>> defined in the driver, probably with suboptimal timings.
>>> Original driver also use hardcoding. And in bootloader, this timing
>>> parameter is also hard coding.
>>> We cannot deduce a parameter set only from the nand id, that is why we
>>> need a table to preset it.
>>> If one nand chip is not listed in that table, the chip id would still
>>> be printed out, so that we can do something for that.
>>> If we encourage people to continue on this, we would not able to
>>> really "driver" that nand.
>> Currently pxa3xx-nand has three operational modes:
>> - use NAND parameters supplied by the platform
>> - use presets configured by the bootloader chain
>> - use built-in NAND parameters, marked as deprecated in favor of the first
>> two
>> You remove the first two modes completely and require that each and every
>> NAND chip used on pxa3xx based platform will be added to the driver. This
>> way you make the driver less robust and harder to use for platform
>> developers, not mentioning you're breaking the existing platforms.
>> In my opinion, the driver *may* support built-in definitions for certain
>> NAND flashes and *must* support configuration of the NAND parameters by the
>> platform code and bootloader.
>>
> 
> Hi Mike,
> 
> Well... I would submit another patch set which would reserve a way
> that platform could pass its parameter setting.
> Like specify the certain type of nand chip parameter for each chip
> select. Is that ok for you?
> 
> For bootloader pass parameter method, I think this way should be
> dropped... For there is attributed which we could not tell from
> registers...
> What do you think of this?

Can you please elaborate what are the attributes that cannot be detected?

> Thanks,
> Lei
Marek Vasut - May 25, 2010, 12:18 p.m.
Dne Út 25. května 2010 12:21:46 Mike Rapoport napsal(a):
> Lei Wen wrote:
> > On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> >> Lei Wen wrote:
> >>> Hi Mike,
> >>> 
> >>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
> >>> 
> >>> wrote:
> >>>> Hi Lei,
> >>>> 
> >>>> Lei Wen wrote:
> >>>>> Hi Mike,
> >>>> 
> >>>> 2) I don't like hadrcoding of NAND parameters into the driver. You
> >>>> remove *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration
> >>>> option and instead
> >>>> you enforce use of built-in definitions. The driver in its current
> >>>> state is
> >>>> robust enough to allow platforms to define optimized NAND timings
> >>>> either in
> >>>> the bootloader or in the kernel. If you don't like that multiple
> >>>> platforms
> >>>> define the same flash chip create an enumeration of built-in types and
> >>>> let
> >>>> platforms to use this enumeration to select the NAND chip. But,
> >>>> anyway, there should be a fallback mode that will support NAND chips
> >>>> that are not defined in the driver, probably with suboptimal timings.
> >>> 
> >>> Original driver also use hardcoding. And in bootloader, this timing
> >>> parameter is also hard coding.
> >>> We cannot deduce a parameter set only from the nand id, that is why we
> >>> need a table to preset it.
> >>> If one nand chip is not listed in that table, the chip id would still
> >>> be printed out, so that we can do something for that.
> >>> If we encourage people to continue on this, we would not able to
> >>> really "driver" that nand.
> >> 
> >> Currently pxa3xx-nand has three operational modes:
> >> - use NAND parameters supplied by the platform
> >> - use presets configured by the bootloader chain
> >> - use built-in NAND parameters, marked as deprecated in favor of the
> >> first two
> >> You remove the first two modes completely and require that each and
> >> every NAND chip used on pxa3xx based platform will be added to the
> >> driver. This way you make the driver less robust and harder to use for
> >> platform developers, not mentioning you're breaking the existing
> >> platforms. In my opinion, the driver *may* support built-in definitions
> >> for certain NAND flashes and *must* support configuration of the NAND
> >> parameters by the platform code and bootloader.
> > 
> > Hi Mike,
> > 
> > Well... I would submit another patch set which would reserve a way
> > that platform could pass its parameter setting.
> > Like specify the certain type of nand chip parameter for each chip
> > select. Is that ok for you?
> > 
> > For bootloader pass parameter method, I think this way should be
> > dropped... For there is attributed which we could not tell from
> > registers...
> > What do you think of this?
> 
> Can you please elaborate what are the attributes that cannot be detected?

MFPR drive strength maybe ? That's none of kernel's concern (as of now).
> 
> > Thanks,
> > Lei
Lei Wen - May 25, 2010, 1:01 p.m.
On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Lei Wen wrote:
>>
>> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il>
>> wrote:
>>>
>>> Lei Wen wrote:
>>>>
>>>> Hi Mike,
>>>>
>>>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
>>>> wrote:
>>>>>
>>>>> Hi Lei,
>>>>>
>>>>> Lei Wen wrote:
>>>>>>
>>>>>> Hi Mike,
>>>>>>
>>>>> 2) I don't like hadrcoding of NAND parameters into the driver. You
>>>>> remove
>>>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>>>> instead
>>>>> you enforce use of built-in definitions. The driver in its current
>>>>> state
>>>>> is
>>>>> robust enough to allow platforms to define optimized NAND timings
>>>>> either
>>>>> in
>>>>> the bootloader or in the kernel. If you don't like that multiple
>>>>> platforms
>>>>> define the same flash chip create an enumeration of built-in types and
>>>>> let
>>>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>>>> there should be a fallback mode that will support NAND chips that are
>>>>> not
>>>>> defined in the driver, probably with suboptimal timings.
>>>>
>>>> Original driver also use hardcoding. And in bootloader, this timing
>>>> parameter is also hard coding.
>>>> We cannot deduce a parameter set only from the nand id, that is why we
>>>> need a table to preset it.
>>>> If one nand chip is not listed in that table, the chip id would still
>>>> be printed out, so that we can do something for that.
>>>> If we encourage people to continue on this, we would not able to
>>>> really "driver" that nand.
>>>
>>> Currently pxa3xx-nand has three operational modes:
>>> - use NAND parameters supplied by the platform
>>> - use presets configured by the bootloader chain
>>> - use built-in NAND parameters, marked as deprecated in favor of the
>>> first
>>> two
>>> You remove the first two modes completely and require that each and every
>>> NAND chip used on pxa3xx based platform will be added to the driver. This
>>> way you make the driver less robust and harder to use for platform
>>> developers, not mentioning you're breaking the existing platforms.
>>> In my opinion, the driver *may* support built-in definitions for certain
>>> NAND flashes and *must* support configuration of the NAND parameters by
>>> the
>>> platform code and bootloader.
>>>
>>
>> Hi Mike,
>>
>> Well... I would submit another patch set which would reserve a way
>> that platform could pass its parameter setting.
>> Like specify the certain type of nand chip parameter for each chip
>> select. Is that ok for you?
>>
>> For bootloader pass parameter method, I think this way should be
>> dropped... For there is attributed which we could not tell from
>> registers...
>> What do you think of this?
>
> Can you please elaborate what are the attributes that cannot be detected?

The attributes comes from two new features that would be included into
pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
just read the timing register, and just set use ecc bit in NDCR is
enough.
But for now there is two type of ECC type. For distinguish, we
introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
0 of that register control whether use the BCH ECC.
You may ask me why not just read from NDECCCTRL to know the ECC type.
It is right, but not always to be true. It the last command executed
for nand before kernel take control is not read or write operation,
the bootloader has no reason to set the NDECCCTRL register. So our
kernel also unable to detect whether to use BCH or HAMMING ECC.

This two type of ecc has different character, if use hamming ecc for
reading while that page is written by BCH, it would instant report of
DB error...
This is not the worst case., for we may ask the bootloader to always
set the NDECCCTRL. (There is no reason to apply ECC when do command
except the read and write although)

The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
BCH engine only support 16bit corroction per 2k data, aka 4bit per
512bytes.
If we need to support 8bit per 512bytes feature, which is needed by a
lot of newest flash which require such high ecc strength.
Then we have to apply the BCH to 1k data instead of 2k. The layout of
content on flash would change.
Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
bytes ECC |
Now after apply BCH to 1k range, the layout would be like: | 1k data |
+ | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
Even as I said let the bootloader to always set the NDECCCTRL. The
kernel would still cannot distinguish the two use case of 4bit per
512bytes and 8bit per 512 bytes ECC strength requirement.
For they both use BCH, the only difference could be told on the nand
layout, not by register reading.

Those two new feature would lead different nand content layout. So if
not distinguish properly, nand would not be able to work functional.
And obviously, the two features cannot get from register reading, and
that is the reason why I suggest drop the feature.

Thanks,
Lei
Eric Miao - May 25, 2010, 1:21 p.m.
On Tue, May 25, 2010 at 9:01 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
> On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>> Lei Wen wrote:
>>>
>>> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il>
>>> wrote:
>>>>
>>>> Lei Wen wrote:
>>>>>
>>>>> Hi Mike,
>>>>>
>>>>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
>>>>> wrote:
>>>>>>
>>>>>> Hi Lei,
>>>>>>
>>>>>> Lei Wen wrote:
>>>>>>>
>>>>>>> Hi Mike,
>>>>>>>
>>>>>> 2) I don't like hadrcoding of NAND parameters into the driver. You
>>>>>> remove
>>>>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>>>>> instead
>>>>>> you enforce use of built-in definitions. The driver in its current
>>>>>> state
>>>>>> is
>>>>>> robust enough to allow platforms to define optimized NAND timings
>>>>>> either
>>>>>> in
>>>>>> the bootloader or in the kernel. If you don't like that multiple
>>>>>> platforms
>>>>>> define the same flash chip create an enumeration of built-in types and
>>>>>> let
>>>>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>>>>> there should be a fallback mode that will support NAND chips that are
>>>>>> not
>>>>>> defined in the driver, probably with suboptimal timings.
>>>>>
>>>>> Original driver also use hardcoding. And in bootloader, this timing
>>>>> parameter is also hard coding.
>>>>> We cannot deduce a parameter set only from the nand id, that is why we
>>>>> need a table to preset it.
>>>>> If one nand chip is not listed in that table, the chip id would still
>>>>> be printed out, so that we can do something for that.
>>>>> If we encourage people to continue on this, we would not able to
>>>>> really "driver" that nand.
>>>>
>>>> Currently pxa3xx-nand has three operational modes:
>>>> - use NAND parameters supplied by the platform
>>>> - use presets configured by the bootloader chain
>>>> - use built-in NAND parameters, marked as deprecated in favor of the
>>>> first
>>>> two
>>>> You remove the first two modes completely and require that each and every
>>>> NAND chip used on pxa3xx based platform will be added to the driver. This
>>>> way you make the driver less robust and harder to use for platform
>>>> developers, not mentioning you're breaking the existing platforms.
>>>> In my opinion, the driver *may* support built-in definitions for certain
>>>> NAND flashes and *must* support configuration of the NAND parameters by
>>>> the
>>>> platform code and bootloader.
>>>>
>>>
>>> Hi Mike,
>>>
>>> Well... I would submit another patch set which would reserve a way
>>> that platform could pass its parameter setting.
>>> Like specify the certain type of nand chip parameter for each chip
>>> select. Is that ok for you?
>>>
>>> For bootloader pass parameter method, I think this way should be
>>> dropped... For there is attributed which we could not tell from
>>> registers...
>>> What do you think of this?
>>
>> Can you please elaborate what are the attributes that cannot be detected?
>
> The attributes comes from two new features that would be included into
> pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
> Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
> just read the timing register, and just set use ecc bit in NDCR is
> enough.
> But for now there is two type of ECC type. For distinguish, we
> introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
> 0 of that register control whether use the BCH ECC.
> You may ask me why not just read from NDECCCTRL to know the ECC type.
> It is right, but not always to be true. It the last command executed
> for nand before kernel take control is not read or write operation,
> the bootloader has no reason to set the NDECCCTRL register. So our
> kernel also unable to detect whether to use BCH or HAMMING ECC.
>
> This two type of ecc has different character, if use hamming ecc for
> reading while that page is written by BCH, it would instant report of
> DB error...
> This is not the worst case., for we may ask the bootloader to always
> set the NDECCCTRL. (There is no reason to apply ECC when do command
> except the read and write although)
>
> The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
> BCH engine only support 16bit corroction per 2k data, aka 4bit per
> 512bytes.
> If we need to support 8bit per 512bytes feature, which is needed by a
> lot of newest flash which require such high ecc strength.
> Then we have to apply the BCH to 1k data instead of 2k. The layout of
> content on flash would change.
> Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
> bytes ECC |
> Now after apply BCH to 1k range, the layout would be like: | 1k data |
> + | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
> Even as I said let the bootloader to always set the NDECCCTRL. The
> kernel would still cannot distinguish the two use case of 4bit per
> 512bytes and 8bit per 512 bytes ECC strength requirement.
> For they both use BCH, the only difference could be told on the nand
> layout, not by register reading.
>
> Those two new feature would lead different nand content layout. So if
> not distinguish properly, nand would not be able to work functional.
> And obviously, the two features cannot get from register reading, and
> that is the reason why I suggest drop the feature.
>

Then would it be possible to just drop the feature for pxa168 and onward?
I guess there might be some existing usages out there for pxa3xx already,
so not to affect existing systems.

Actually, it's now quite easy for a single driver to distinguish this, through
platform_device_id table please, see drivers/i2c/busses/i2c-pxa.c for an
example. I'd like to call the device pxa168-nand or something.
Mike Rapoport - May 26, 2010, 9:57 a.m.
Lei Wen wrote:
> On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:

>> Can you please elaborate what are the attributes that cannot be detected?
> 
> The attributes comes from two new features that would be included into
> pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
> Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
> just read the timing register, and just set use ecc bit in NDCR is
> enough.
> But for now there is two type of ECC type. For distinguish, we
> introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
> 0 of that register control whether use the BCH ECC.
> You may ask me why not just read from NDECCCTRL to know the ECC type.
> It is right, but not always to be true. It the last command executed
> for nand before kernel take control is not read or write operation,
> the bootloader has no reason to set the NDECCCTRL register. So our
> kernel also unable to detect whether to use BCH or HAMMING ECC.
> 
> This two type of ecc has different character, if use hamming ecc for
> reading while that page is written by BCH, it would instant report of
> DB error...
> This is not the worst case., for we may ask the bootloader to always
> set the NDECCCTRL. (There is no reason to apply ECC when do command
> except the read and write although)
> 
> The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
> BCH engine only support 16bit corroction per 2k data, aka 4bit per
> 512bytes.
> If we need to support 8bit per 512bytes feature, which is needed by a
> lot of newest flash which require such high ecc strength.
> Then we have to apply the BCH to 1k data instead of 2k. The layout of
> content on flash would change.
> Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
> bytes ECC |
> Now after apply BCH to 1k range, the layout would be like: | 1k data |
> + | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
> Even as I said let the bootloader to always set the NDECCCTRL. The
> kernel would still cannot distinguish the two use case of 4bit per
> 512bytes and 8bit per 512 bytes ECC strength requirement.
> For they both use BCH, the only difference could be told on the nand
> layout, not by register reading.
> 
> Those two new feature would lead different nand content layout. So if
> not distinguish properly, nand would not be able to work functional.
> And obviously, the two features cannot get from register reading, and
> that is the reason why I suggest drop the feature.

Why not make the ECC type and strength part of the 
pxa3xx_nand_platform_data? This way you can ensure NAND layout 
consistency for particular platform without reduction in the driver 
robustness.

> Thanks,
> Lei
Lei Wen - May 26, 2010, 1:35 p.m.
On Tue, May 25, 2010 at 9:21 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Tue, May 25, 2010 at 9:01 PM, Lei Wen <adrian.wenl@gmail.com> wrote:
>> On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike@compulab.co.il> wrote:
>>> Lei Wen wrote:
>>>>
>>>> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike@compulab.co.il>
>>>> wrote:
>>>>>
>>>>> Lei Wen wrote:
>>>>>>
>>>>>> Hi Mike,
>>>>>>
>>>>>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike@compulab.co.il>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Lei,
>>>>>>>
>>>>>>> Lei Wen wrote:
>>>>>>>>
>>>>>>>> Hi Mike,
>>>>>>>>
>>>>>>> 2) I don't like hadrcoding of NAND parameters into the driver. You
>>>>>>> remove
>>>>>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>>>>>> instead
>>>>>>> you enforce use of built-in definitions. The driver in its current
>>>>>>> state
>>>>>>> is
>>>>>>> robust enough to allow platforms to define optimized NAND timings
>>>>>>> either
>>>>>>> in
>>>>>>> the bootloader or in the kernel. If you don't like that multiple
>>>>>>> platforms
>>>>>>> define the same flash chip create an enumeration of built-in types and
>>>>>>> let
>>>>>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>>>>>> there should be a fallback mode that will support NAND chips that are
>>>>>>> not
>>>>>>> defined in the driver, probably with suboptimal timings.
>>>>>>
>>>>>> Original driver also use hardcoding. And in bootloader, this timing
>>>>>> parameter is also hard coding.
>>>>>> We cannot deduce a parameter set only from the nand id, that is why we
>>>>>> need a table to preset it.
>>>>>> If one nand chip is not listed in that table, the chip id would still
>>>>>> be printed out, so that we can do something for that.
>>>>>> If we encourage people to continue on this, we would not able to
>>>>>> really "driver" that nand.
>>>>>
>>>>> Currently pxa3xx-nand has three operational modes:
>>>>> - use NAND parameters supplied by the platform
>>>>> - use presets configured by the bootloader chain
>>>>> - use built-in NAND parameters, marked as deprecated in favor of the
>>>>> first
>>>>> two
>>>>> You remove the first two modes completely and require that each and every
>>>>> NAND chip used on pxa3xx based platform will be added to the driver. This
>>>>> way you make the driver less robust and harder to use for platform
>>>>> developers, not mentioning you're breaking the existing platforms.
>>>>> In my opinion, the driver *may* support built-in definitions for certain
>>>>> NAND flashes and *must* support configuration of the NAND parameters by
>>>>> the
>>>>> platform code and bootloader.
>>>>>
>>>>
>>>> Hi Mike,
>>>>
>>>> Well... I would submit another patch set which would reserve a way
>>>> that platform could pass its parameter setting.
>>>> Like specify the certain type of nand chip parameter for each chip
>>>> select. Is that ok for you?
>>>>
>>>> For bootloader pass parameter method, I think this way should be
>>>> dropped... For there is attributed which we could not tell from
>>>> registers...
>>>> What do you think of this?
>>>
>>> Can you please elaborate what are the attributes that cannot be detected?
>>
>> The attributes comes from two new features that would be included into
>> pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
>> Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
>> just read the timing register, and just set use ecc bit in NDCR is
>> enough.
>> But for now there is two type of ECC type. For distinguish, we
>> introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
>> 0 of that register control whether use the BCH ECC.
>> You may ask me why not just read from NDECCCTRL to know the ECC type.
>> It is right, but not always to be true. It the last command executed
>> for nand before kernel take control is not read or write operation,
>> the bootloader has no reason to set the NDECCCTRL register. So our
>> kernel also unable to detect whether to use BCH or HAMMING ECC.
>>
>> This two type of ecc has different character, if use hamming ecc for
>> reading while that page is written by BCH, it would instant report of
>> DB error...
>> This is not the worst case., for we may ask the bootloader to always
>> set the NDECCCTRL. (There is no reason to apply ECC when do command
>> except the read and write although)
>>
>> The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
>> BCH engine only support 16bit corroction per 2k data, aka 4bit per
>> 512bytes.
>> If we need to support 8bit per 512bytes feature, which is needed by a
>> lot of newest flash which require such high ecc strength.
>> Then we have to apply the BCH to 1k data instead of 2k. The layout of
>> content on flash would change.
>> Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
>> bytes ECC |
>> Now after apply BCH to 1k range, the layout would be like: | 1k data |
>> + | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
>> Even as I said let the bootloader to always set the NDECCCTRL. The
>> kernel would still cannot distinguish the two use case of 4bit per
>> 512bytes and 8bit per 512 bytes ECC strength requirement.
>> For they both use BCH, the only difference could be told on the nand
>> layout, not by register reading.
>>
>> Those two new feature would lead different nand content layout. So if
>> not distinguish properly, nand would not be able to work functional.
>> And obviously, the two features cannot get from register reading, and
>> that is the reason why I suggest drop the feature.
>>
>
> Then would it be possible to just drop the feature for pxa168 and onward?
> I guess there might be some existing usages out there for pxa3xx already,
> so not to affect existing systems.
>
> Actually, it's now quite easy for a single driver to distinguish this, through
> platform_device_id table please, see drivers/i2c/busses/i2c-pxa.c for an
> example. I'd like to call the device pxa168-nand or something.

Yes, that is also what I want. Just put another attributes like ecc
strength into flash_info structure.
This info could be revealed in the built_in_table or flash info passed
from platform data.

Also this ecc strength could take place the ecc type used in the flash
info structure.

Thanks,
Lei
Lei Wen - May 26, 2010, 1:42 p.m.
On Wed, May 26, 2010 at 5:57 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Lei Wen wrote:
>>
>> On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike@compulab.co.il>
>> wrote:
>
>>> Can you please elaborate what are the attributes that cannot be detected?
>>
>> The attributes comes from two new features that would be included into
>> pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
>> Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
>> just read the timing register, and just set use ecc bit in NDCR is
>> enough.
>> But for now there is two type of ECC type. For distinguish, we
>> introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
>> 0 of that register control whether use the BCH ECC.
>> You may ask me why not just read from NDECCCTRL to know the ECC type.
>> It is right, but not always to be true. It the last command executed
>> for nand before kernel take control is not read or write operation,
>> the bootloader has no reason to set the NDECCCTRL register. So our
>> kernel also unable to detect whether to use BCH or HAMMING ECC.
>>
>> This two type of ecc has different character, if use hamming ecc for
>> reading while that page is written by BCH, it would instant report of
>> DB error...
>> This is not the worst case., for we may ask the bootloader to always
>> set the NDECCCTRL. (There is no reason to apply ECC when do command
>> except the read and write although)
>>
>> The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
>> BCH engine only support 16bit corroction per 2k data, aka 4bit per
>> 512bytes.
>> If we need to support 8bit per 512bytes feature, which is needed by a
>> lot of newest flash which require such high ecc strength.
>> Then we have to apply the BCH to 1k data instead of 2k. The layout of
>> content on flash would change.
>> Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
>> bytes ECC |
>> Now after apply BCH to 1k range, the layout would be like: | 1k data |
>> + | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
>> Even as I said let the bootloader to always set the NDECCCTRL. The
>> kernel would still cannot distinguish the two use case of 4bit per
>> 512bytes and 8bit per 512 bytes ECC strength requirement.
>> For they both use BCH, the only difference could be told on the nand
>> layout, not by register reading.
>>
>> Those two new feature would lead different nand content layout. So if
>> not distinguish properly, nand would not be able to work functional.
>> And obviously, the two features cannot get from register reading, and
>> that is the reason why I suggest drop the feature.
>
> Why not make the ECC type and strength part of the
> pxa3xx_nand_platform_data? This way you can ensure NAND layout consistency
> for particular platform without reduction in the driver robustness.
>

The reason why we need define different ecc type and ecc strength is
for those different kind of ecc strength could give different
performance cost. For data safe consideration, the higher of ecc
strength would be safer. But you must notice that the higher ecc
strength would bring to us more performance downgrade. For flash that
only need 1bit correction per 512 bytes, we certainly don't
need the 8bit ECC, or we would suffer from the performance.

One table with the ECC strength description would solve all the
problem we have, and also no risk in the driver robustness.

Thanks,
Lei

Patch

diff --git a/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
b/arch/arm/plat-pxa/include/plat/pxa3xx_nand.h
index 3478eae..c494f68 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
@@ -54,9 +17,6 @@  struct pxa3xx_nand_platform_data {

 	const struct mtd_partition		*parts;
 	unsigned int				nr_parts;
-
-	const struct pxa3xx_nand_flash * 	flash;
-	size_t					num_flash;
 };

 extern void pxa3xx_set_nand_info(struct pxa3xx_nand_platform_data *info);
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..da40b9a 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	tAR;  /* ND_ALE low to ND_nRE low delay */
+	uint32_t	tWHR; /* ND_nWE high to ND_nRE low for status read */
+	uint32_t	tR;   /* ND_nWE high to ND_nRE low for read */
+};
+
+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,