[PATCH 02/25] pxa3xx_nand: introduce common timing to reduce read id times

[Lei Wen]

On Fri, Jun 18, 2010 at 4:12 PM, Eric Miao <eric.y.miao at 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
>>
>