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

[Florian Fainelli]

Hi Matthieu,

On Monday 09 August 2010 11:25:18 Matthieu CASTET wrote:
> 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.

Ok.

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

Ok, then I will update it as part as the patch adding ONFI reading so that it 
is future-proof anyway.

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

Such driver need to handle those anyway.

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

Sure, thanks for your follow-up.

> 
> 
> 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))
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/