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

[Eric Miao]

>>> -       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
>