| Message ID | AANLkTinVBXhT77Zd-LNpufgJAl7WZdic0oxDFwvNkvJg@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
-----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 = µn_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 = µn_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 = µn_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 = µn_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, > - µn1GbX8, > - µn1GbX16, > - µn4GbX8, > - µn4GbX16, > - &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-----
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 = µn_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 = µn_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 = µn_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 = µn_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, > - µn1GbX8, > - µn1GbX16, > - µn4GbX8, > - µn4GbX16, > - &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:
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 = µn_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 = µn_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 = µn_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 = µn_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, >> - µn1GbX8, >> - µn1GbX16, >> - µn4GbX8, >> - µn4GbX16, >> - &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 >
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 = µn_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 = µn_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 = µn_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 = µn_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, >>> - µn1GbX8, >>> - µn1GbX16, >>> - µn4GbX8, >>> - µn4GbX16, >>> - &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 >>
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
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
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 >
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
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
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
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
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
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
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
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
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.
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
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
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
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,