| Message ID | AANLkTim1qcVoqSS7T8bE0Vsu6phC2iY_Ok671WYHcD0z@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 18, 2010 at 1:33 PM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > From 832379d0da4d772e45872de365053f9a15733967 Mon Sep 17 00:00:00 2001 > From: Lei Wen <leiwen@marvell.com> > Date: Wed, 2 Jun 2010 21:50:25 +0800 > Subject: [PATCH 02/25] pxa3xx_nand: introduce common timing to reduce > read id times > > We certainly don't need to send read id command times by times, since > we already know what the id is after the first read id... > > So create a common timing which could ensure it would successfully read id > out all supported chip. Then follow the build-in table to reconfigure > the timing. This is clever trick we'd like to have. And I'd prefer to call this a 'loose timing' or a 'default safe timing' instead of 'common timing'. > > Signed-off-by: Lei Wen <leiwen@marvell.com> > Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com> > --- > drivers/mtd/nand/pxa3xx_nand.c | 72 ++++++++++++++++----------------------- > 1 files changed, 30 insertions(+), 42 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 013e075..f939083 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -226,23 +226,26 @@ const static struct pxa3xx_nand_cmdset cmdset = { > }; > > static struct pxa3xx_nand_timing __devinitdata timing[] = { > + /* common timing used to detect flash id */ > + { 40, 80, 60, 100, 80, 100, 90000, 400, 40, }, Make sure this is the loose and safe enough timing. > /* Samsung NAND series */ > - { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, > + { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, > /* Micron NAND series */ > - { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, > + { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, > /* ST NAND series */ > - { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, > + { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, > }; > > static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { > - { 0x46ec, 32, 512, 16, 16, 4096, &timing[0], }, > - { 0xdaec, 64, 2048, 8, 8, 2048, &timing[0], }, > - { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[0], }, > - { 0xa12c, 64, 2048, 8, 8, 1024, &timing[1], }, > - { 0xb12c, 64, 2048, 16, 16, 1024, &timing[1], }, > - { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[1], }, > - { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[1], }, > - { 0xba20, 64, 2048, 16, 16, 2048, &timing[2], }, > + { 0, 0, 0, 0, 0, 0, &timing[0], }, > + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, > + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, > + { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[1], }, > + { 0xa12c, 64, 2048, 8, 8, 1024, &timing[2], }, > + { 0xb12c, 64, 2048, 16, 16, 1024, &timing[2], }, > + { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[2], }, > + { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[2], }, > + { 0xba20, 64, 2048, 16, 16, 2048, &timing[3], }, > }; > > #define NDTR0_tCH(c) (min((c), 7) << 19) > @@ -859,12 +862,6 @@ static int pxa3xx_nand_config_flash(struct > pxa3xx_nand_info *info, > struct pxa3xx_nand_platform_data *pdata = pdev->dev.platform_data; > uint32_t ndcr = 0x00000FFF; /* disable all interrupts */ > > - if (f->page_size != 2048 && f->page_size != 512) > - return -EINVAL; > - > - if (f->flash_width != 16 && f->flash_width != 8) > - return -EINVAL; > - I do think these are sanity check, that's useful to prevent incorrectly defined data (esp. coming from board code). Can we define the loose flash type as: > static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { > + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, > + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, > + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, with a comment of /* default flash type to detect ID */? My understanding is the minimum requirement to detect the NAND ID is a loose timing and 512 small page, 8-bit wide bus, so with a chip_id being 0x0000, that should be enough to tell it's a special flash type to detect ID. > /* calculate flash information */ > info->oob_size = (f->page_size == 2048) ? 64 : 16; > info->read_id_bytes = (f->page_size == 2048) ? 4 : 2; > @@ -976,36 +973,27 @@ 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; > - } > - > - for (i = 0; i < ARRAY_SIZE(builtin_flash_types); i++) { > - > - f = &builtin_flash_types[i]; > - > - if (pxa3xx_nand_config_flash(info, f)) > - continue; > - > - if (__readid(info, &id)) > - continue; > - > - if (id == f->chip_id) > + f = &builtin_flash_types[0]; > + pxa3xx_nand_config_flash(info, f); > + if (__readid(info, &id)) > + goto fail_detect; > + > + for (i=0; i<ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1; i++) { > + if (i < pdata->num_flash) > + f = pdata->flash + i; > + else > + f = &builtin_flash_types[i - pdata->num_flash + 1]; This looks a bit tricky and difficult to read, can you improve the readability here? > + if (f->chip_id == id) { > + dev_info(&info->pdev->dev, "detect chip id: 0x%x\n", id); > + pxa3xx_nand_config_flash(info, f); > return 0; > + } > } > > dev_warn(&info->pdev->dev, > - "failed to detect configured nand flash; found %04x instead of\n", > + "failed to detect configured nand flash; found %04x instead of\n", > id); > +fail_detect: > return -ENODEV; > } > I'm generally OK with the idea.
On Fri, Jun 18, 2010 at 2:33 PM, Eric Miao <eric.y.miao@gmail.com> wrote: > On Fri, Jun 18, 2010 at 1:33 PM, Haojian Zhuang > <haojian.zhuang@gmail.com> wrote: >> From 832379d0da4d772e45872de365053f9a15733967 Mon Sep 17 00:00:00 2001 >> From: Lei Wen <leiwen@marvell.com> >> Date: Wed, 2 Jun 2010 21:50:25 +0800 >> Subject: [PATCH 02/25] pxa3xx_nand: introduce common timing to reduce >> read id times >> >> We certainly don't need to send read id command times by times, since >> we already know what the id is after the first read id... >> >> So create a common timing which could ensure it would successfully read id >> out all supported chip. Then follow the build-in table to reconfigure >> the timing. > > This is clever trick we'd like to have. And I'd prefer to call this a > 'loose timing' > or a 'default safe timing' instead of 'common timing'. > >> >> Signed-off-by: Lei Wen <leiwen@marvell.com> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com> >> --- >> drivers/mtd/nand/pxa3xx_nand.c | 72 ++++++++++++++++----------------------- >> 1 files changed, 30 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c >> index 013e075..f939083 100644 >> --- a/drivers/mtd/nand/pxa3xx_nand.c >> +++ b/drivers/mtd/nand/pxa3xx_nand.c >> @@ -226,23 +226,26 @@ const static struct pxa3xx_nand_cmdset cmdset = { >> }; >> >> static struct pxa3xx_nand_timing __devinitdata timing[] = { >> + /* common timing used to detect flash id */ >> + { 40, 80, 60, 100, 80, 100, 90000, 400, 40, }, > > Make sure this is the loose and safe enough timing. > >> /* Samsung NAND series */ >> - { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, >> + { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, >> /* Micron NAND series */ >> - { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, >> + { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, >> /* ST NAND series */ >> - { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, >> + { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, >> }; >> >> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >> - { 0x46ec, 32, 512, 16, 16, 4096, &timing[0], }, >> - { 0xdaec, 64, 2048, 8, 8, 2048, &timing[0], }, >> - { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[0], }, >> - { 0xa12c, 64, 2048, 8, 8, 1024, &timing[1], }, >> - { 0xb12c, 64, 2048, 16, 16, 1024, &timing[1], }, >> - { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[1], }, >> - { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[1], }, >> - { 0xba20, 64, 2048, 16, 16, 2048, &timing[2], }, >> + { 0, 0, 0, 0, 0, 0, &timing[0], }, >> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, >> + { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[1], }, >> + { 0xa12c, 64, 2048, 8, 8, 1024, &timing[2], }, >> + { 0xb12c, 64, 2048, 16, 16, 1024, &timing[2], }, >> + { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[2], }, >> + { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[2], }, >> + { 0xba20, 64, 2048, 16, 16, 2048, &timing[3], }, >> }; >> >> #define NDTR0_tCH(c) (min((c), 7) << 19) >> @@ -859,12 +862,6 @@ static int pxa3xx_nand_config_flash(struct >> pxa3xx_nand_info *info, >> struct pxa3xx_nand_platform_data *pdata = pdev->dev.platform_data; >> uint32_t ndcr = 0x00000FFF; /* disable all interrupts */ >> >> - if (f->page_size != 2048 && f->page_size != 512) >> - return -EINVAL; >> - >> - if (f->flash_width != 16 && f->flash_width != 8) >> - return -EINVAL; >> - > > I do think these are sanity check, that's useful to prevent incorrectly defined > data (esp. coming from board code). Can we define the loose flash type as: > >> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >> + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, >> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, > > with a comment of /* default flash type to detect ID */? > > My understanding is the minimum requirement to detect the NAND ID is a > loose timing and 512 small page, 8-bit wide bus, so with a chip_id being > 0x0000, that should be enough to tell it's a special flash type to detect ID. There is no need to add the flash page size and bus width for the common timing, or loose timing... For the chip identification only need reset and read id command, and reset and read id command only care for the first command in NDCB0 and the timing setting in NDTR0CS0 and NDTR1CS0, the page size is not useful here and could make confussion if give such definition... > >> /* calculate flash information */ >> info->oob_size = (f->page_size == 2048) ? 64 : 16; >> info->read_id_bytes = (f->page_size == 2048) ? 4 : 2; >> @@ -976,36 +973,27 @@ 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; >> - } >> - >> - for (i = 0; i < ARRAY_SIZE(builtin_flash_types); i++) { >> - >> - f = &builtin_flash_types[i]; >> - >> - if (pxa3xx_nand_config_flash(info, f)) >> - continue; >> - >> - if (__readid(info, &id)) >> - continue; >> - >> - if (id == f->chip_id) >> + f = &builtin_flash_types[0]; >> + pxa3xx_nand_config_flash(info, f); >> + if (__readid(info, &id)) >> + goto fail_detect; >> + >> + for (i=0; i<ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1; i++) { >> + if (i < pdata->num_flash) >> + f = pdata->flash + i; >> + else >> + f = &builtin_flash_types[i - pdata->num_flash + 1]; > > This looks a bit tricky and difficult to read, can you improve the > readability here? The chip id identification logic here is changed in latter patch in this patch set... If from the last patch of view, does this part looks better? > >> + if (f->chip_id == id) { >> + dev_info(&info->pdev->dev, "detect chip id: 0x%x\n", id); >> + pxa3xx_nand_config_flash(info, f); >> return 0; >> + } >> } >> >> dev_warn(&info->pdev->dev, >> - "failed to detect configured nand flash; found %04x instead of\n", >> + "failed to detect configured nand flash; found %04x instead of\n", >> id); >> +fail_detect: >> return -ENODEV; >> } >> > > I'm generally OK with the idea. > Best regards, Lei
>>> - if (f->page_size != 2048 && f->page_size != 512) >>> - return -EINVAL; >>> - >>> - if (f->flash_width != 16 && f->flash_width != 8) >>> - return -EINVAL; >>> - >> >> I do think these are sanity check, that's useful to prevent incorrectly defined >> data (esp. coming from board code). Can we define the loose flash type as: >> >>> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >>> + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, >>> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >>> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, >> >> with a comment of /* default flash type to detect ID */? >> >> My understanding is the minimum requirement to detect the NAND ID is a >> loose timing and 512 small page, 8-bit wide bus, so with a chip_id being >> 0x0000, that should be enough to tell it's a special flash type to detect ID. > > There is no need to add the flash page size and bus width for the > common timing, or loose timing... > For the chip identification only need reset and read id command, and > reset and read id command only care > for the first command in NDCB0 and the timing setting in NDTR0CS0 and > NDTR1CS0, the page size is not useful > here and could make confussion if give such definition... I know, but I'd like to keep the checking of page_size, and flash_width, if you take a look into the rest of the code how much it is assumed that page_size being either 2048 or 512, flash_width being 8 or 16, you'll know why such an error checking here is mandatory actually. >> >>> /* calculate flash information */ >>> info->oob_size = (f->page_size == 2048) ? 64 : 16; >>> info->read_id_bytes = (f->page_size == 2048) ? 4 : 2; >>> @@ -976,36 +973,27 @@ 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; >>> - } >>> - >>> - for (i = 0; i < ARRAY_SIZE(builtin_flash_types); i++) { >>> - >>> - f = &builtin_flash_types[i]; >>> - >>> - if (pxa3xx_nand_config_flash(info, f)) >>> - continue; >>> - >>> - if (__readid(info, &id)) >>> - continue; >>> - >>> - if (id == f->chip_id) >>> + f = &builtin_flash_types[0]; >>> + pxa3xx_nand_config_flash(info, f); >>> + if (__readid(info, &id)) >>> + goto fail_detect; >>> + >>> + for (i=0; i<ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1; i++) { >>> + if (i < pdata->num_flash) >>> + f = pdata->flash + i; >>> + else >>> + f = &builtin_flash_types[i - pdata->num_flash + 1]; >> >> This looks a bit tricky and difficult to read, can you improve the >> readability here? > > The chip id identification logic here is changed in latter patch in > this patch set... > If from the last patch of view, does this part looks better? > Not sure, I'd rather this being separated into two loops if that helps readability. >> >>> + if (f->chip_id == id) { >>> + dev_info(&info->pdev->dev, "detect chip id: 0x%x\n", id); >>> + pxa3xx_nand_config_flash(info, f); >>> return 0; >>> + } >>> } >>> >>> dev_warn(&info->pdev->dev, >>> - "failed to detect configured nand flash; found %04x instead of\n", >>> + "failed to detect configured nand flash; found %04x instead of\n", >>> id); >>> +fail_detect: >>> return -ENODEV; >>> } >>> >> >> I'm generally OK with the idea. >> > Best regards, > Lei >
On Fri, Jun 18, 2010 at 4:12 PM, Eric Miao <eric.y.miao@gmail.com> wrote: >>>> - if (f->page_size != 2048 && f->page_size != 512) >>>> - return -EINVAL; >>>> - >>>> - if (f->flash_width != 16 && f->flash_width != 8) >>>> - return -EINVAL; >>>> - >>> >>> I do think these are sanity check, that's useful to prevent incorrectly defined >>> data (esp. coming from board code). Can we define the loose flash type as: >>> >>>> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >>>> + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, >>>> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >>>> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, >>> >>> with a comment of /* default flash type to detect ID */? >>> >>> My understanding is the minimum requirement to detect the NAND ID is a >>> loose timing and 512 small page, 8-bit wide bus, so with a chip_id being >>> 0x0000, that should be enough to tell it's a special flash type to detect ID. >> >> There is no need to add the flash page size and bus width for the >> common timing, or loose timing... >> For the chip identification only need reset and read id command, and >> reset and read id command only care >> for the first command in NDCB0 and the timing setting in NDTR0CS0 and >> NDTR1CS0, the page size is not useful >> here and could make confussion if give such definition... > > I know, but I'd like to keep the checking of page_size, and flash_width, > if you take a look into the rest of the code how much it is assumed that > page_size being either 2048 or 512, flash_width being 8 or 16, you'll > know why such an error checking here is mandatory actually. I know that the configure_flash function need to check page_size & flash_width to set the ndcr. But what the setting would not affect the reset/read id result anyway... For there is some chip has the same bytes in the first two bytes, I choose to always read out 4 bytes data when send read id command in latter patch of this patch set... > >>> >>>> /* calculate flash information */ >>>> info->oob_size = (f->page_size == 2048) ? 64 : 16; >>>> info->read_id_bytes = (f->page_size == 2048) ? 4 : 2; >>>> @@ -976,36 +973,27 @@ 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; >>>> - } >>>> - >>>> - for (i = 0; i < ARRAY_SIZE(builtin_flash_types); i++) { >>>> - >>>> - f = &builtin_flash_types[i]; >>>> - >>>> - if (pxa3xx_nand_config_flash(info, f)) >>>> - continue; >>>> - >>>> - if (__readid(info, &id)) >>>> - continue; >>>> - >>>> - if (id == f->chip_id) >>>> + f = &builtin_flash_types[0]; >>>> + pxa3xx_nand_config_flash(info, f); >>>> + if (__readid(info, &id)) >>>> + goto fail_detect; >>>> + >>>> + for (i=0; i<ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1; i++) { >>>> + if (i < pdata->num_flash) >>>> + f = pdata->flash + i; >>>> + else >>>> + f = &builtin_flash_types[i - pdata->num_flash + 1]; >>> >>> This looks a bit tricky and difficult to read, can you improve the >>> readability here? >> >> The chip id identification logic here is changed in latter patch in >> this patch set... >> If from the last patch of view, does this part looks better? >> > > Not sure, I'd rather this being separated into two loops if that helps > readability. > >>> >>>> + if (f->chip_id == id) { >>>> + dev_info(&info->pdev->dev, "detect chip id: 0x%x\n", id); >>>> + pxa3xx_nand_config_flash(info, f); >>>> return 0; >>>> + } >>>> } >>>> >>>> dev_warn(&info->pdev->dev, >>>> - "failed to detect configured nand flash; found %04x instead of\n", >>>> + "failed to detect configured nand flash; found %04x instead of\n", >>>> id); >>>> +fail_detect: >>>> return -ENODEV; >>>> } >>>> >>> >>> I'm generally OK with the idea. >>> >> Best regards, >> Lei >> >
On Fri, Jun 18, 2010 at 5:15 PM, Lei Wen <adrian.wenl@gmail.com> wrote: > On Fri, Jun 18, 2010 at 4:12 PM, Eric Miao <eric.y.miao@gmail.com> wrote: >>>>> - if (f->page_size != 2048 && f->page_size != 512) >>>>> - return -EINVAL; >>>>> - >>>>> - if (f->flash_width != 16 && f->flash_width != 8) >>>>> - return -EINVAL; >>>>> - >>>> >>>> I do think these are sanity check, that's useful to prevent incorrectly defined >>>> data (esp. coming from board code). Can we define the loose flash type as: >>>> >>>>> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >>>>> + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, >>>>> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >>>>> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, >>>> >>>> with a comment of /* default flash type to detect ID */? >>>> >>>> My understanding is the minimum requirement to detect the NAND ID is a >>>> loose timing and 512 small page, 8-bit wide bus, so with a chip_id being >>>> 0x0000, that should be enough to tell it's a special flash type to detect ID. >>> >>> There is no need to add the flash page size and bus width for the >>> common timing, or loose timing... >>> For the chip identification only need reset and read id command, and >>> reset and read id command only care >>> for the first command in NDCB0 and the timing setting in NDTR0CS0 and >>> NDTR1CS0, the page size is not useful >>> here and could make confussion if give such definition... >> >> I know, but I'd like to keep the checking of page_size, and flash_width, >> if you take a look into the rest of the code how much it is assumed that >> page_size being either 2048 or 512, flash_width being 8 or 16, you'll >> know why such an error checking here is mandatory actually. > > I know that the configure_flash function need to check page_size & > flash_width to set the > ndcr. But what the setting would not affect the reset/read id result anyway... > For there is some chip has the same bytes in the first two bytes, I > choose to always read out > 4 bytes data when send read id command in latter patch of this patch set... > My understanding is that NAND READID will at least work with the assumption of 512bytes/page and 8-bit wide bus, no matter how the NAND chips are connected. (unless some one comes up with a insane design of using multiple interlaced NAND chips of different types). Again, assume the NAND controller is not initialized (register values being garbage or all zero-ed), do you still think you can read out the ID without letting it know the page size, bus width and etc?
On Fri, Jun 18, 2010 at 5:19 PM, Eric Miao <eric.y.miao@gmail.com> wrote: > On Fri, Jun 18, 2010 at 5:15 PM, Lei Wen <adrian.wenl@gmail.com> wrote: >> On Fri, Jun 18, 2010 at 4:12 PM, Eric Miao <eric.y.miao@gmail.com> wrote: >>>>>> - if (f->page_size != 2048 && f->page_size != 512) >>>>>> - return -EINVAL; >>>>>> - >>>>>> - if (f->flash_width != 16 && f->flash_width != 8) >>>>>> - return -EINVAL; >>>>>> - >>>>> >>>>> I do think these are sanity check, that's useful to prevent incorrectly defined >>>>> data (esp. coming from board code). Can we define the loose flash type as: >>>>> >>>>>> static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { >>>>>> + { 0x0000, 0, 512, 8, 8, 0, &timing[0], }, >>>>>> + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, >>>>>> + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, >>>>> >>>>> with a comment of /* default flash type to detect ID */? >>>>> >>>>> My understanding is the minimum requirement to detect the NAND ID is a >>>>> loose timing and 512 small page, 8-bit wide bus, so with a chip_id being >>>>> 0x0000, that should be enough to tell it's a special flash type to detect ID. >>>> >>>> There is no need to add the flash page size and bus width for the >>>> common timing, or loose timing... >>>> For the chip identification only need reset and read id command, and >>>> reset and read id command only care >>>> for the first command in NDCB0 and the timing setting in NDTR0CS0 and >>>> NDTR1CS0, the page size is not useful >>>> here and could make confussion if give such definition... >>> >>> I know, but I'd like to keep the checking of page_size, and flash_width, >>> if you take a look into the rest of the code how much it is assumed that >>> page_size being either 2048 or 512, flash_width being 8 or 16, you'll >>> know why such an error checking here is mandatory actually. >> >> I know that the configure_flash function need to check page_size & >> flash_width to set the >> ndcr. But what the setting would not affect the reset/read id result anyway... >> For there is some chip has the same bytes in the first two bytes, I >> choose to always read out >> 4 bytes data when send read id command in latter patch of this patch set... >> > > My understanding is that NAND READID will at least work with the assumption > of 512bytes/page and 8-bit wide bus, no matter how the NAND chips are > connected. (unless some one comes up with a insane design of using multiple > interlaced NAND chips of different types). > > Again, assume the NAND controller is not initialized (register values being > garbage or all zero-ed), do you still think you can read out the ID without > letting it know the page size, bus width and etc? > Well... I understand it is good to reserve the page size and width judgement in configuration. I would make this part revert back to its original state... For read id, it would not care for the page size and width setting in NDCR...
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 013e075..f939083 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -226,23 +226,26 @@ const static struct pxa3xx_nand_cmdset cmdset = { }; static struct pxa3xx_nand_timing __devinitdata timing[] = { + /* common timing used to detect flash id */ + { 40, 80, 60, 100, 80, 100, 90000, 400, 40, }, /* Samsung NAND series */ - { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, + { 10, 0, 20, 40, 30, 40, 11123, 110, 10, }, /* Micron NAND series */ - { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, + { 10, 25, 15, 25, 15, 30, 25000, 60, 10, }, /* ST NAND series */ - { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, + { 10, 35, 15, 25, 15, 25, 25000, 60, 10, }, }; static struct pxa3xx_nand_flash __devinitdata builtin_flash_types[] = { - { 0x46ec, 32, 512, 16, 16, 4096, &timing[0], }, - { 0xdaec, 64, 2048, 8, 8, 2048, &timing[0], }, - { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[0], }, - { 0xa12c, 64, 2048, 8, 8, 1024, &timing[1], }, - { 0xb12c, 64, 2048, 16, 16, 1024, &timing[1], }, - { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[1], }, - { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[1], }, - { 0xba20, 64, 2048, 16, 16, 2048, &timing[2], }, + { 0, 0, 0, 0, 0, 0, &timing[0], }, + { 0x46ec, 32, 512, 16, 16, 4096, &timing[1], }, + { 0xdaec, 64, 2048, 8, 8, 2048, &timing[1], }, + { 0xd7ec, 128, 4096, 8, 8, 8192, &timing[1], }, + { 0xa12c, 64, 2048, 8, 8, 1024, &timing[2], }, + { 0xb12c, 64, 2048, 16, 16, 1024, &timing[2], }, + { 0xdc2c, 64, 2048, 8, 8, 4096, &timing[2], }, + { 0xcc2c, 64, 2048, 16, 16, 4096, &timing[2], }, + { 0xba20, 64, 2048, 16, 16, 2048, &timing[3], }, }; #define NDTR0_tCH(c) (min((c), 7) << 19) @@ -859,12 +862,6 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info, struct pxa3xx_nand_platform_data *pdata = pdev->dev.platform_data; uint32_t ndcr = 0x00000FFF; /* disable all interrupts */ - if (f->page_size != 2048 && f->page_size != 512) - return -EINVAL; - - if (f->flash_width != 16 && f->flash_width != 8) - return -EINVAL; - /* calculate flash information */ info->oob_size = (f->page_size == 2048) ? 64 : 16;