Re: [PATCH][OneNAND] Write-while-program support

[Adrian Hunter <ext-adrian.hunter@nokia.com>]

Kyungmin Park wrote:
> On Mon, Nov 17, 2008 at 4:57 PM, Adrian Hunter
> <ext-adrian.hunter@nokia.com> wrote:
>> Kyungmin Park wrote:
>>> On Fri, Nov 14, 2008 at 7:32 PM, Adrian Hunter
>>> <ext-adrian.hunter@nokia.com> 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, ...)