Re: [U-Boot] [PATCH v2 1/4] Flex-OneNAND driver

On Sun, Mar 08, 2009 at 11:45:37AM +0530, Rohit Hagargundgi wrote:

Sorries for the delay

On Tuesday 16 December 2008 03:14:22 Scott Wood wrote:

...

On Thu, Dec 11, 2008 at 07:57:18PM +0530, Rohit Hagargundgi wrote:

...

This patch adds support for MLC OneNAND and Flex-OneNAND devices.

Patch does not apply to u-boot-nand-flash/next (the master branch is for 2008.12 bugfixes only).

rebased to next branch.

Please send patches with a ready-for-git commit message.

Thanks, Rohit

Signed-off-by: Rohit Hagargundgi <h.rohit at samsung.com>

Comments that don't go into git, such as changes from a previous version, go below the above --- line.

drivers/mtd/onenand/onenand_base.c | 718 +++++++++++++++++++++++++++++++----- drivers/mtd/onenand/onenand_bbt.c | 14 drivers/mtd/onenand/onenand_uboot.c | 4 include/linux/mtd/onenand.h | 16 include/linux/mtd/onenand_regs.h | 18 include/onenand_uboot.h | 10 6 files changed, 680 insertions(+), 100 deletions(-)

Do you see this going into Linux soon? Unless there's some particular obstacle to that (other than the code's readiness) I think I'd rather wait for it to be accepted there, with (hopefully) more eyes looking over it, and then bring the result into u-boot.

• boundary = this->boundary[die];
• ofs += block << (this->erase_shift - 1);
• if (block > (boundary + 1))

• ofs += (block - boundary - 1) << (this->erase_shift - 1);
  

• return ofs;

+}

In the Linux patch here: http://groups.google.com/group/fa.linux.kernel/msg/8edda44803373f33?hl=en&am...

you deal with 64-bit offsets, but that's missing above.

+inline loff_t onenand_addr(struct onenand_chip *this, int block) +{

• if (!FLEXONENAND(this))

• return block << this->erase_shift;
  

• return flexonenand_addr(this, block);

+}

Let the compiler figure out what is inline. Can this function be static?

• if (FLEXONENAND(this)) {

• /* Find the eraseregion of this address */
  

• i = flexonenand_region(mtd, addr);
  

• region = &mtd->eraseregions[i];
  

• block_size = region->erasesize;
  

• region_end = region->offset + region->erasesize * region->numblocks;
  

• /* Start address within region must align on block boundary.
  

•  * Erase region's start offset is always block start address.
  

•  */
  

• if (unlikely((addr - region->offset) & (block_size - 1))) {
  

• 	MTDDEBUG (MTD_DEBUG_LEVEL0, "onenand_erase: Unaligned address\n");
  

• 	return -EINVAL;
  

• }
  

• } else {

• block_size = 1 << this->erase_shift;
  

• /* Start address must align on block boundary */
  

• if (unlikely(addr & (block_size - 1))) {
  

• 	MTDDEBUG (MTD_DEBUG_LEVEL0, "onenand_erase: Unaligned address\n");
  

• 	return -EINVAL;
  

• }
  

  }

It might be worthwhile to factor out the various if/else bits into some sort of "ops" struct, so that the differences in chip type are concentrated in one spot (and it's easier to add a third type if it comes along).

@@ -1512,9 +1755,6 @@ int onenand_erase(struct mtd_info *mtd, struct erase_info *instr) onenand_get_device(mtd, FL_ERASING);

/* Loop throught the pages */

• len = instr->len;

• addr = instr->addr;

• instr->state = MTD_ERASING;

  while (len) {

@@ -1541,14 +1781,7 @@ int onenand_erase(struct mtd_info *mtd, struct erase_info *instr) else MTDDEBUG (MTD_DEBUG_LEVEL0, "onenand_erase: " "Failed erase, block %d\n",

• 			  (unsigned)(addr >> this->erase_shift));
  

• 	if (ret == -EPERM)
  

• 		printk("onenand_erase: "
  

• 			  "Device is write protected!!!\n");
  

• 	else
  

• 		printk("onenand_erase: "
  

• 			  "Failed erase, block %d\n",
  

• 			  (unsigned)(addr >> this->erase_shift));
  

• 			onenand_block(this, addr));
  instr->state = MTD_ERASE_FAILED;
  instr->fail_addr = addr;
  
  

@@ -1557,6 +1790,21 @@ int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)

len -= block_size;
addr += block_size;

• if (addr == region_end) {
  

• 	if (!len)
  

• 		break;
  

• 	region++;
  

• 	block_size = region->erasesize;
  

• 	region_end = region->offset + region->erasesize * region->numblocks;
  

• 	if (len & (block_size - 1)) {
  

• 		/* This has been checked at MTD partitioning level. */
  

• 		printk(KERN_ERR "onenand_erase: Unaligned address\n");
  

• 		goto erase_exit;
  

• 	}
  

• }
  

  }

  instr->state = MTD_ERASE_DONE;

@@ -1634,7 +1882,7 @@ static int onenand_default_block_markbad(struct mtd_info *mtd, loff_t ofs) int block;

/* Get block number */

• block = ((int) ofs) >> bbm->bbt_erase_shift;

• block = onenand_block(this, ofs); if (bbm->bbt) bbm->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);

@@ -1682,8 +1930,8 @@ static int onenand_do_lock_cmd(struct mtd_info *mtd, loff_t ofs, size_t len, int int start, end, block, value, status; int wp_status_mask;

• start = ofs >> this->erase_shift;
• end = len >> this->erase_shift;

• start = onenand_block(this, ofs);

• end = onenand_block(this, ofs + len);

  if (cmd == ONENAND_CMD_LOCK) wp_status_mask = ONENAND_WP_LS;

@@ -1718,7 +1966,7 @@ static int onenand_do_lock_cmd(struct mtd_info *mtd, loff_t ofs, size_t len, int }

/* Block lock scheme */

• for (block = start; block < start + end; block++) {

• for (block = start; block < end; block++) { /* Set block address */ value = onenand_block_address(this, block); this->write_word(value, this->base + ONENAND_REG_START_ADDRESS1);

@@ -1831,7 +2079,7 @@ static void onenand_unlock_all(struct mtd_info *mtd) { struct onenand_chip *this = mtd->priv; loff_t ofs = 0;

• size_t len = this->chipsize;

• size_t len = mtd->size;

  if (this->options & ONENAND_HAS_UNLOCK_ALL) { /* Set start block address */

@@ -1847,14 +2095,12 @@ static void onenand_unlock_all(struct mtd_info *mtd) & ONENAND_CTRL_ONGO) continue;

• return;
  

• /* Check lock status */ if (onenand_check_lock_status(this)) return;

  /* Workaround for all block unlock in DDP */

• if (ONENAND_IS_DDP(this)) {
  

• if (ONENAND_IS_DDP(this) && !FLEXONENAND(this)) {
  /* All blocks on another chip */
  ofs = this->chipsize >> 1;
  len = this->chipsize >> 1;
  

@@ -1906,6 +2152,14 @@ static void onenand_check_features(struct mtd_info *mtd) break; }

• if (ONENAND_IS_MLC(this))

• this->options &= ~ONENAND_HAS_2PLANE;
  

• if (FLEXONENAND(this)) {

• this->options &= ~ONENAND_HAS_CONT_LOCK;
  

• this->options |= ONENAND_HAS_UNLOCK_ALL;
  

• }
• if (this->options & ONENAND_HAS_CONT_LOCK) printk(KERN_DEBUG "Lock scheme is Continuous Lock\n"); if (this->options & ONENAND_HAS_UNLOCK_ALL)

@@ -1922,16 +2176,18 @@ static void onenand_check_features(struct mtd_info *mtd) */ char *onenand_print_device_info(int device, int version) {

• int vcc, demuxed, ddp, density;

• int vcc, demuxed, ddp, density, flexonenand; char *dev_info = malloc(80); char *p = dev_info;

  vcc = device & ONENAND_DEVICE_VCC_MASK; demuxed = device & ONENAND_DEVICE_IS_DEMUX; ddp = device & ONENAND_DEVICE_IS_DDP;

• density = device >> ONENAND_DEVICE_DENSITY_SHIFT;
• p += sprintf(dev_info, "%sOneNAND%s %dMB %sV 16-bit (0x%02x)",

• density = onenand_get_density(device);
• flexonenand = device & DEVICE_IS_FLEXONENAND;
• p += sprintf(dev_info, "%s%sOneNAND%s %dMB %sV 16-bit (0x%02x)", demuxed ? "" : "Muxed ",

•       flexonenand ? "Flex-" : "",
      ddp ? "(DDP)" : "",
      (16 << density), vcc ? "2.65/3.3" : "1.8", device);
  
  

@@ -1957,7 +2213,7 @@ static int onenand_check_maf(int manuf) char *name; int i;

• for (i = 0; size; i++)

• for (i = 0; i < size; i++) if (manuf == onenand_manuf_ids[i].id) break;

@@ -1974,6 +2230,258 @@ static int onenand_check_maf(int manuf) }

/** +* flexonenand_get_boundary - Reads the SLC boundary +* @param onenand_info - onenand info structure +* +* Fill up boundary[] field in onenand_chip +**/ +static int flexonenand_get_boundary(struct mtd_info *mtd) +{

• struct onenand_chip *this = mtd->priv;
• unsigned int die, bdry;
• int ret, syscfg, locked;
• /* Disable ECC */
• syscfg = this->read_word(this->base + ONENAND_REG_SYS_CFG1);
• this->write_word((syscfg | 0x0100), this->base + ONENAND_REG_SYS_CFG1);
• for (die = 0; die < this->dies; die++) {

• this->command(mtd, FLEXONENAND_CMD_PI_ACCESS, die, 0);
  

• this->wait(mtd, FL_SYNCING);
  

• this->command(mtd, FLEXONENAND_CMD_READ_PI, die, 0);
  

• ret = this->wait(mtd, FL_READING);
  

• bdry = this->read_word(this->base + ONENAND_DATARAM);
  

• if ((bdry >> FLEXONENAND_PI_UNLOCK_SHIFT) == 3)
  

• 	locked = 0;
  

• else
  

• 	locked = 1;
  

• this->boundary[die] = bdry & FLEXONENAND_PI_MASK;
  

• this->command(mtd, ONENAND_CMD_RESET, 0, 0);
  

• ret = this->wait(mtd, FL_RESETING);
  

• printk(KERN_INFO "Die %d boundary: %d%s\n", die,
  

•        this->boundary[die], locked ? "(Locked)" : "(Unlocked)");
  

• }
• /* Enable ECC */
• this->write_word(syscfg, this->base + ONENAND_REG_SYS_CFG1);
• return 0;

+}

+/**

• • flexonenand_get_size - Fill up fields in onenand_chip and mtd_info

• • 	  boundary[], diesize[], mtd->size, mtd->erasesize,
    

• • 	  mtd->eraseregions
    

• • @param mtd - MTD device structure
• */

+static void flexonenand_get_size(struct mtd_info *mtd) +{

• struct onenand_chip *this = mtd->priv;
• int die, ofs, i, eraseshift, density;
• int blksperdie, maxbdry;
• density = onenand_get_density(this->device_id);
• blksperdie = ((16 << density) << 20) >> (this->erase_shift);

Can this overflow? Might be better as:

blksperdie = 16 << (density + 20 - this->erase_shift);

• die = ofs = 0;
• i = -1;
• for (; die < this->dies; die++) {

• if (!die || this->boundary[die-1] != maxbdry) {
  

• 	i++;
  

• 	mtd->eraseregions[i].offset = ofs;
  

• 	mtd->eraseregions[i].erasesize = 1 << eraseshift;
  

• 	mtd->eraseregions[i].numblocks =
  

• 					this->boundary[die] + 1;
  

• 	ofs += mtd->eraseregions[i].numblocks << eraseshift;
  

• 	eraseshift++;
  

• } else {
  

• 	mtd->numeraseregions -= 1;
  

• 	mtd->eraseregions[i].numblocks +=
  

• 					this->boundary[die] + 1;
  

• 	ofs += (this->boundary[die] + 1) << (eraseshift - 1);
  

• }
  

• if (this->boundary[die] != maxbdry) {
  

• 	i++;
  

• 	mtd->eraseregions[i].offset = ofs;
  

• 	mtd->eraseregions[i].erasesize = 1 << eraseshift;
  

• 	mtd->eraseregions[i].numblocks = maxbdry ^
  

• 					 this->boundary[die];
  

• 	ofs += mtd->eraseregions[i].numblocks << eraseshift;
  

• 	eraseshift--;
  

• } else
  

• 	mtd->numeraseregions -= 1;
  

• }

Can the above be simplified, or at least documented? I'm having a hard time following it.

-Scott