Message ID | 201008021355.52744.ffainelli@freebox.fr |
---|---|
State | New, archived |
Headers | show |
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))
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 --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