[PATCH 02/25] pxa3xx_nand: introduce common timing to reduce read id times
Lei Wen
adrian.wenl at gmail.com
Fri Jun 18 05:15:20 EDT 2010
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
>>
>
More information about the linux-arm-kernel
mailing list