[PATCH 02/25] pxa3xx_nand: introduce common timing to reduce read id times
Lei Wen
adrian.wenl at gmail.com
Fri Jun 18 04:08:13 EDT 2010
On Fri, Jun 18, 2010 at 2:33 PM, Eric Miao <eric.y.miao at gmail.com> wrote:
> On Fri, Jun 18, 2010 at 1:33 PM, Haojian Zhuang
> <haojian.zhuang at gmail.com> wrote:
>> From 832379d0da4d772e45872de365053f9a15733967 Mon Sep 17 00:00:00 2001
>> From: Lei Wen <leiwen at 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 at marvell.com>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang at 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
More information about the linux-mtd
mailing list