diff mbox

NAND: add support for reading ONFI parameters from NAND device

Message ID 201008021355.52744.ffainelli@freebox.fr
State New, archived
Headers show

Commit Message

Florian Fainelli Aug. 2, 2010, 11:55 a.m. UTC 
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.
--
+       chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
+
+       /* Read entire ID string */
+
+       for (i = 0; i < 8; i++)
+               id_data[i] = chip->read_byte(mtd);
 
        if (!type->name)
                return ERR_PTR(-ENODEV);
@@ -2886,6 +2990,21 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                mtd->oobsize = mtd->writesize / 32;
                busw = type->options & NAND_BUSWIDTH_16;
        }
+       /* Get chip options, preserve non chip based options */
+       chip->options &= ~NAND_CHIPOPTIONS_MSK;
+       chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+
+       /* Check if chip is a not a samsung device. Do not clear the
+        * options for chips which are not having an extended id.
+        */
+       if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
+               chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
+ident_done:
+
+       /*
+        * Set chip as a default. Board drivers can override it, if necessary
+        */
+       chip->options |= NAND_NO_AUTOINCR;
 
        /* Try to identify manufacturer */
        for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
@@ -2900,7 +3019,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
        if (busw != (chip->options & NAND_BUSWIDTH_16)) {
                printk(KERN_INFO "NAND device: Manufacturer ID:"
                       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
-                      dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
+                      *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
                printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
                       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
                       busw ? 16 : 8);
@@ -2924,21 +3043,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                NAND_LARGE_BADBLOCK_POS : NAND_SMALL_BADBLOCK_POS;
        chip->badblockbits = 8;
 
-       /* Get chip options, preserve non chip based options */
-       chip->options &= ~NAND_CHIPOPTIONS_MSK;
-       chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
-
-       /*
-        * Set chip as a default. Board drivers can override it, if necessary
-        */
-       chip->options |= NAND_NO_AUTOINCR;
-
-       /* Check if chip is a not a samsung device. Do not clear the
-        * options for chips which are not having an extended id.
-        */
-       if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-               chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
-
        /*
         * Bad block marker is stored in the last page of each block
         * on Samsung and Hynix MLC devices
@@ -2958,9 +3062,10 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
        if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
                chip->cmdfunc = nand_command_lp;
 
+       /* TODO onfi flash name */
        printk(KERN_INFO "NAND device: Manufacturer ID:"
-              " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
-              nand_manuf_ids[maf_idx].name, type->name);
+              " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
+              nand_manuf_ids[maf_idx].name, chip->onfi_version?type->name:chip->onfi_params.model);
 
        return type;
 }
@@ -2979,7 +3084,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 int nand_scan_ident(struct mtd_info *mtd, int maxchips,
                    struct nand_flash_dev *table)
 {
-       int i, busw, nand_maf_id;
+       int i, busw, nand_maf_id, nand_dev_id;
        struct nand_chip *chip = mtd->priv;
        struct nand_flash_dev *type;
 
@@ -2989,7 +3094,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
        nand_set_defaults(chip, busw);
 
        /* Read the flash type */
-       type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, table);
+       type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, &nand_dev_id, table);
 
        if (IS_ERR(type)) {
                if (!(chip->options & NAND_SCAN_SILENT_NODEV))
@@ -3007,7 +3112,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
                chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
                /* Read manufacturer and device IDs */
                if (nand_maf_id != chip->read_byte(mtd) ||
-                   type->id != chip->read_byte(mtd))
+                   nand_dev_id != chip->read_byte(mtd))
                        break;
        }
        if (i > 1)

--
Florian

Comments

Matthieu CASTET Aug. 9, 2010, 9:25 a.m. UTC | #1 
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))
Florian Fainelli Aug. 9, 2010, 9:43 a.m. UTC | #2 
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/
diff mbox

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a7b864..25a44fa 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2772,15 +2772,50 @@  static void nand_set_defaults(struct nand_chip *chip, int busw)
 
 }
 
+static u16 onfi_crc(u16 crc, unsigned char const *p, size_t len)
+{
+       int i;
+       while (len--) {
+               crc ^= *p++ << 8;
+               for (i = 0; i < 8; i++)
+                       crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+       }
+       return crc;
+}
+
+/*
+ * sanitize ONFI strings so we can safely print them
+ */
+static void sanitize_string(uint8_t *s, size_t len)
+{
+       ssize_t i;
+
+       /* null terminate */
+       s[len - 1] = 0;
+
+       /* remove non printable chars */
+       for (i = 0; i < len - 1; i++) {
+               if (s[i] < ' ' || s[i] > 127)
+                       s[i] = '?';
+       }
+
+       /* remove trailing spaces */
+       for (i = len - 1; i >= 0; i--) {
+               if (s[i] && s[i] != ' ')
+                       break;
+               s[i] = 0;
+       }
+}
+
 /*
  * Get the flash and manufacturer id and lookup if the type is supported
  */
 static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                                                  struct nand_chip *chip,
-                                                 int busw, int *maf_id,
+                                                 int busw, int *maf_id, int *dev_id
                                                  struct nand_flash_dev *type)
 {
-       int i, dev_id, maf_idx;
+       int i, maf_idx;
        u8 id_data[8];
 
        /* Select the device */
@@ -2797,7 +2832,7 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
        /* Read manufacturer and device IDs */
        *maf_id = chip->read_byte(mtd);
-       dev_id = chip->read_byte(mtd);
+       *dev_id = chip->read_byte(mtd);
 
        /* Try again to make sure, as some systems the bus-hold or other
         * interface concerns can cause random data which looks like a
@@ -2807,15 +2842,13 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
        chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 
-       /* Read entire ID string */
-
-       for (i = 0; i < 8; i++)
+       for (i = 0; i < 2; i++)
                id_data[i] = chip->read_byte(mtd);
 
-       if (id_data[0] != *maf_id || id_data[1] != dev_id) {
+       if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
                printk(KERN_INFO "%s: second ID read did not match "
                       "%02x,%02x against %02x,%02x\n", __func__,
-                      *maf_id, dev_id, id_data[0], id_data[1]);
+                      *maf_id, *dev_id, id_data[0], id_data[1]);
                return ERR_PTR(-ENODEV);
        }
 
@@ -2823,8 +2856,79 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                type = nand_flash_ids;
 
        for (; type->name != NULL; type++)
-               if (dev_id == type->id)
+               if (*dev_id == type->id)
                         break;
+#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.

+
+                       struct nand_onfi_params *p = &chip->onfi_params;
+                       int i;
+
+                       printk(KERN_INFO "ONFI flash detected\n");
+                       chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+                       for (i = 0; i < 3; i++) {
+                               /* XXX this that ok to use read_buf at this stage ? */
+                               chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+                               if (onfi_crc(0x4F4E, (uint8_t *)p, 254) == le16_to_cpu(p->crc))
+                               {
+                                        printk(KERN_INFO "ONFI param page %d valid\n", i);
+                                        break;
+                               }
+                       }
+                       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.

+                                       printk(KERN_INFO "%s: unsupported ONFI version\n", __func__);
+                               }
+                               else {
+                                       if (val & (1 << 1))
+                                               chip->onfi_version = 10;
+                                       else if (val & (1 << 2))
+                                               chip->onfi_version = 20;
+                                       else if (val & (1 << 3))
+                                               chip->onfi_version = 21;
+                                       else
+                                               chip->onfi_version = 22;
+                               }
+                       }
+                       if (chip->onfi_version) {
+                               sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+                               sanitize_string(p->model, sizeof(p->model));
+                               if (!mtd->name)
+                                       mtd->name = p->model;
+                               mtd->writesize = le32_to_cpu(p->byte_per_page);
+                               mtd->erasesize = le32_to_cpu(p->pages_per_block)*mtd->writesize;
+                               mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+                               chip->chipsize = le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;
+                               busw = 0;
+                               if (le16_to_cpu(p->features) & 1)
+                                       busw = NAND_BUSWIDTH_16;
+
+                               chip->options &= ~NAND_CHIPOPTIONS_MSK;
+                               chip->options |= (NAND_NO_READRDY | NAND_NO_AUTOINCR) & NAND_CHIPOPTIONS_MSK;
+
+                               /* emulate cellinfo ??? */
+                               //chip->cellinfo = 0x8;
+                               goto ident_done;
+
+                       }
+               }
+       }
+#endif