Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device

[Matthieu CASTET <matthieu.castet@parrot.com>]

Hi Florian,

Florian Fainelli a écrit :
> Hi Matthieu,
> 
> On Monday 02 August 2010 11:25:49 Matthieu CASTET wrote:
>> Florian Fainelli a écrit :
>>> Hi Matthieu,
>>>
>>> On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
>>>> Hi,
>>>>
>>>>
>>>> Also you don't handle endianness (integer are little endian) for value
>>>> in nand_onfi_params.
>>> Yes, so far the drivers using those values were doing the correct endian
>>> conversion when they need to use them.
>> In that case use le16, le32, ... type. Also prefer kernel type over
>> uintx_t type.
>>
>>>> This won't work this unknown nand, and not work with some LP nand that
>>>> doesn't provide additional id bytes.
>>> So how do you see things regarding the provisioning of the relevant ONFI
>>> parameters?
>> I will see something like in the patch attached in
>> http://article.gmane.org/gmane.linux.drivers.mtd/30935.
>>
>> ONFI parsing is done early in nand_get_flash_type (unknow chip or LP nand).
>> If the ONFI parsing is ok we bypass the old identification method
>> (additional id bytes).
> 
> Looks ok to me.
> 
>> As an example I attach a patch that mix your patch and mine.
>>
>>
>> Matthieu
>>
>> PS : the NAND_ONFI flags seems useless, we can use onfi_version (0 means
>> no onfi).
> 
> Right, thanks for noticing that.
> 
> I got a couple of comments on your patch that I inlined, the rest looks
> good.
> --

> +#if 1
> +       chip->onfi_version = 0;
> +       if (!type->name || !type->pagesize) {
> +               /* try ONFI for unknow chip or LP */
> +               chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> +               if (chip->read_byte(mtd) == 'O' &&
> +                       chip->read_byte(mtd) == 'N' &&
> +                       chip->read_byte(mtd) == 'F' &&
> +                       chip->read_byte(mtd) == 'I') {
> 
> Why not use what was in our original patch and do the memcmp? That looks
> cleaner to me and allows to invert the logic on the if statement to get the
> code cleaner. That's just cosmetic anyway.
I wanted to avoid to use read_buf, because some advanced controller 
(those who implement cmdfunc) need to overrides all io access.
But some driver assumed  that nand_scan_ident only used read_byte. That 
the case of the denali driver [1]. Using it will cause random read in 
memory and likely a kernel panic.

But we need read_buf for reading onfi page. Also these advanced 
controllers will break because they won't handle correctly in cmdfunc 
new NAND_CMD_READID and NAND_CMD_PARAM.

I don't know what the best way to handle them.


> +                       if (i < 3) {
> +                               /* check version */
> +                               int val = le16_to_cpu(p->revision);
> +                               if (!is_power_of_2(val) || val == 1 || val > (1 << 4)) {
> 
> the !is_power_of_2 check does not work for ONFI version 2.1 (3), so I would only
> keep the two other checks.
> 
Ok.

Will you take care to post a new patch ?


Matthieu


[1]
/* register the driver with the NAND core subsystem */
     denali->nand.select_chip = denali_select_chip;
     denali->nand.cmdfunc = denali_cmdfunc;
     denali->nand.read_byte = denali_read_byte;
     denali->nand.waitfunc = denali_waitfunc;

     /* scan for NAND devices attached to the controller
      * this is the first stage in a two step process to register
      * with the nand subsystem */
     if (nand_scan_ident(&denali->mtd, LLD_MAX_FLASH_BANKS, NULL))