From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <49212E46.6080801@nokia.com> Date: Mon, 17 Nov 2008 10:41:42 +0200 From: Adrian Hunter MIME-Version: 1.0 To: Kyungmin Park Subject: Re: [PATCH][OneNAND] Write-while-program support References: <20081114002830.GA25979@july> <491D53B8.1010401@nokia.com> <9c9fda240811161856u2c784946wabba1dfc5009ef46@mail.gmail.com> <492123ED.8020407@nokia.com> <9c9fda240811170023o264fc336g2ae0ef439908f8d2@mail.gmail.com> In-Reply-To: <9c9fda240811170023o264fc336g2ae0ef439908f8d2@mail.gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-mtd@lists.infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Kyungmin Park wrote: > On Mon, Nov 17, 2008 at 4:57 PM, Adrian Hunter > wrote: >> Kyungmin Park wrote: >>> On Fri, Nov 14, 2008 at 7:32 PM, Adrian Hunter >>> wrote: >>>> Kyungmin Park wrote: >>>>> This is from Adrian hunter and modified for write_ops operation. >>>> Thanks for looking at write-while-program. I have a few questions >>>> though. >>>> I have copied the whole function below because the diff is too confusing: >>>> >>>>> /** >>>>> * onenand_write_ops_nolock - [OneNAND Interface] write main and/or >>>>> out-of-band >>>>> * @param mtd MTD device structure >>>>> * @param to offset to write to >>>>> * @param ops oob operation description structure >>>>> * >>>>> * Write main and/or oob with ECC >>>>> */ >>>>> static int onenand_write_ops_nolock(struct mtd_info *mtd, loff_t to, >>>>> struct mtd_oob_ops *ops) >>>>> { >>>>> struct onenand_chip *this = mtd->priv; >>>>> int written = 0, column, thislen = 0, subpage = 0; >>>>> int prev = 0, prevlen = 0, prev_subpage = 0, first = 1; >>>>> int oobwritten = 0, oobcolumn, thisooblen, oobsize; >>>>> size_t len = ops->len; >>>>> size_t ooblen = ops->ooblen; >>>>> const u_char *buf = ops->datbuf; >>>>> const u_char *oob = ops->oobbuf; >>>>> u_char *oobbuf; >>>>> int ret = 0; >>>>> >>>>> DEBUG(MTD_DEBUG_LEVEL3, "onenand_write_ops_nolock: to = 0x%08x, >>>>> len >>>>> = %i\n", (unsigned int) to, (int) len); >>>>> >>>>> /* Initialize retlen, in case of early exit */ >>>>> ops->retlen = 0; >>>>> ops->oobretlen = 0; >>>>> >>>>> /* Do not allow writes past end of device */ >>>>> if (unlikely((to + len) > mtd->size)) { >>>>> printk(KERN_ERR "onenand_write_ops_nolock: Attempt write >>>>> to >>>>> past end of device\n"); >>>>> return -EINVAL; >>>>> } >>>>> >>>>> /* Reject writes, which are not page aligned */ >>>>> if (unlikely(NOTALIGNED(to) || NOTALIGNED(len))) { >>>>> printk(KERN_ERR "onenand_write_ops_nolock: Attempt to >>>>> write >>>>> not page aligned data\n"); >>>>> return -EINVAL; >>>>> } >>>>> >>>> The loop cannot handle the case when len is zero so I had: >>>> >>>> if (!len) >>>> return 0; >>> Yes there's length check. I wonder is it possible to write with zero? >>> Is it handle at driver level or upper level such as filesystem? >>> But no problem to just add length check here at this time. >>> >>>>> if (ops->mode == MTD_OOB_AUTO) >>>>> oobsize = this->ecclayout->oobavail; >>>>> else >>>>> oobsize = mtd->oobsize; >>>>> >>>>> oobcolumn = to & (mtd->oobsize - 1); >>>>> >>>>> column = to & (mtd->writesize - 1); >>>>> >>>>> /* Loop until all data write */ >>>>> while (1) { >>>>> if (written < len) { >>>>> u_char *wbuf = (u_char *) buf; >>>>> >>>>> thislen = min_t(int, mtd->writesize - column, len >>>>> - >>>>> written); >>>>> thisooblen = min_t(int, oobsize - oobcolumn, >>>>> ooblen >>>>> - oobwritten); >>>>> >>>>> cond_resched(); >>>>> >>>>> this->command(mtd, ONENAND_CMD_BUFFERRAM, to, >>>>> thislen); >>>>> >>>>> /* Partial page write */ >>>>> subpage = thislen < mtd->writesize; >>>>> if (subpage) { >>>>> memset(this->page_buf, 0xff, >>>>> mtd->writesize); >>>>> memcpy(this->page_buf + column, buf, >>>>> thislen); >>>>> wbuf = this->page_buf; >>>>> } >>>>> >>>>> this->write_bufferram(mtd, ONENAND_DATARAM, wbuf, >>>>> 0, mtd->writesize); >>>>> >>>>> if (oob) { >>>>> oobbuf = this->oob_buf; >>>>> >>>>> /* We send data to spare ram with oobsize >>>>> * to prevent byte access */ >>>>> memset(oobbuf, 0xff, mtd->oobsize); >>>>> if (ops->mode == MTD_OOB_AUTO) >>>>> onenand_fill_auto_oob(mtd, oobbuf, >>>>> oob, oobcolumn, thisooblen); >>>>> else >>>>> memcpy(oobbuf + oobcolumn, oob, >>>>> thisooblen); >>>>> >>>>> oobwritten += thisooblen; >>>>> oob += thisooblen; >>>>> oobcolumn = 0; >>>>> } else >>>>> oobbuf = (u_char *) ffchars; >>>>> >>>>> this->write_bufferram(mtd, ONENAND_SPARERAM, >>>>> oobbuf, 0, mtd->oobsize); >>>>> } else >>>>> ONENAND_SET_NEXT_BUFFERRAM(this); >>>>> >>>>> if (!first) { >>>>> ONENAND_SET_PREV_BUFFERRAM(this); >>>>> >>>>> ret = this->wait(mtd, FL_WRITING); >>>>> >>>>> /* In partial page write we don't update bufferram >>>>> */ >>>>> onenand_update_bufferram(mtd, prev, !ret && >>>>> !prev_subpage); >>>>> if (ONENAND_IS_2PLANE(this)) { >>>> I do not understand how 2PLANE is compatible with write-while-program >>>> because >>>> 2PLANE uses both buffers so we cannot start transferring to the second >>>> buffer >>>> while the program is ongoing. Does it work? >>>> >>>> Won't MLC and FlexOneNAND have that problem too? >>> Agree, I'm not yet tested the 2PLANE features and it doesn't support >>> write-while-program. >>> If the 2plane and flex-onenand, it used the previous one. >>> >>>>> ONENAND_SET_BUFFERRAM1(this); >>>>> onenand_update_bufferram(mtd, prev + >>>>> this->writesize, !ret && !prev_subpage); >>>>> } >>>>> >>>>> if (ret) { >>>> My original patch had "written -= prevlen" here. >>> Agreed. >>> >> Also we need to invalidate the other bufferram because we transferred the >> data but did not write it. i.e. >> >> if (written < len) >> onenand_update_bufferram(mtd, to, 0); > > For clarity, clear all bufferram at error case. OK >>>>> printk(KERN_ERR "onenand_write_ops_nolock: >>>>> write filaed %d\n", ret); >>>>> break; >>>>> } >>>>> >>>>> if (written == len) { >>>>> /* Only check verify write turn on */ >>>>> ret = onenand_verify(mtd, buf - len, to - >>>>> len, len); >>>>> if (ret) { >>>>> printk(KERN_ERR >>>>> "onenand_write_ops_nolock: verify failed %d\n", ret); >>>>> break; >>>>> } >>>> Why not just break here? >>> E.g., In original version, write bufferram 0 and it changes bufferam 1 >>> automatically. >>> Same as if break here, it points out the previous bufferram. >> The previous bufferram is the last one that was used, so it should >> point to the previous one. >> > > then next time, it overwrite the previous bufferram, but it expects it > write next bufferram. The current MTD write is finished because written == len. The next one starts by changing the bufferram with this->command(mtd, ONENAND_CMD_BUFFERRAM, ...)