diff mbox

[OneNAND] Write-while-program support

Message ID 20081114002830.GA25979@july
State New, archived
Headers show

Commit Message

Kyungmin Park Nov. 14, 2008, 12:28 a.m. UTC
This is from Adrian hunter and modified for write_ops operation.

It's similar with load-while-read method.
but it's write side performance improvement.

Here's brief performance results.
Note: Measured from OMAP3 with async OneNAND mode. (not sync mode)

Comments

Adrian Hunter Nov. 14, 2008, 10:32 a.m. UTC | #1
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;

> 	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?

> 				ONENAND_SET_BUFFERRAM1(this);
> 				onenand_update_bufferram(mtd, prev + this->writesize, !ret && !prev_subpage);
> 			}
> 
> 			if (ret) {

My original patch had "written -= prevlen" here.

> 				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?

> 			}
> 			ONENAND_SET_NEXT_BUFFERRAM(this);
> 
> 			if (written == len)
> 				break;
> 		}
> 
> 		this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
> 
> 		written += thislen;
> 
> 		column = 0;
> 		prev_subpage = subpage;
> 		prev = to;
> 		prevlen = thislen;
> 		to += thislen;
> 		buf += thislen;
> 		first = 0;
> 	}
> 
> 	ops->retlen = written;

Is ops->oobretlen needed here also?

> 
> 	return ret;
> }
Amit Kumar Sharma Nov. 14, 2008, 12:12 p.m. UTC | #2
Hi Adrian

FlexOneNand read dual data ram buffer and so we can not use
----- Original Message ----- 
From: "Adrian Hunter" <ext-adrian.hunter@nokia.com>
To: "Kyungmin Park" <kmpark@infradead.org>
Cc: <linux-mtd@lists.infradead.org>
Sent: Friday, November 14, 2008 4:02 PM
Subject: Re: [PATCH][OneNAND] Write-while-program support


> 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;
>
>> 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?
Amit : FlexOneNand use dual data ram buffer and so we can 
not use it and other point is for OneNand we have a plan to 
use dual  buufer
because OneNand page size will increase to 4k.
>
>> ONENAND_SET_BUFFERRAM1(this);
>> onenand_update_bufferram(mtd, prev + this->writesize, 
>> !ret && !prev_subpage);
>> }
>>
>> if (ret) {
>
> My original patch had "written -= prevlen" here.
>
>> 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?
>
Amit : same as kyungmin comment.
>> }
>> ONENAND_SET_NEXT_BUFFERRAM(this);
>>
>> if (written == len)
>> break;
>> }
>>
>> this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
>>
>> written += thislen;
>>
>> column = 0;
>> prev_subpage = subpage;
>> prev = to;
>> prevlen = thislen;
>> to += thislen;
>> buf += thislen;
>> first = 0;
>> }
>>
>> ops->retlen = written;
>
> Is ops->oobretlen needed here also?
>
>>
>> return ret;
>> }
>
>
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Kyungmin Park Nov. 17, 2008, 2:56 a.m. UTC | #3
HI,

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.

>
>>                                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.

>
>>                        }
>>                        ONENAND_SET_NEXT_BUFFERRAM(this);
>>
>>                        if (written == len)
>>                                break;
>>                }
>>
>>                this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
>>
>>                written += thislen;
>>
>>                column = 0;
>>                prev_subpage = subpage;
>>                prev = to;
>>                prevlen = thislen;
>>                to += thislen;
>>                buf += thislen;
>>                first = 0;
>>        }
>>
>>        ops->retlen = written;
>
> Is ops->oobretlen needed here also?

Okay I will added. I'm not sure YAFFS handle this one correctly. Maybe
it used the retlen in ops operation (data + spare).

>
>>
>>        return ret;
>> }

Thank you,
Kyungmin Park
Adrian Hunter Nov. 17, 2008, 7:57 a.m. UTC | #4
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);

>>>                                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.

>>>                        }
>>>                        ONENAND_SET_NEXT_BUFFERRAM(this);
>>>
>>>                        if (written == len)
>>>                                break;
>>>                }
>>>
>>>                this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
>>>
>>>                written += thislen;
>>>
>>>                column = 0;
>>>                prev_subpage = subpage;
>>>                prev = to;
>>>                prevlen = thislen;
>>>                to += thislen;
>>>                buf += thislen;
>>>                first = 0;
>>>        }
>>>
>>>        ops->retlen = written;
>> Is ops->oobretlen needed here also?
> 
> Okay I will added. I'm not sure YAFFS handle this one correctly. Maybe
> it used the retlen in ops operation (data + spare).
> 
>>>        return ret;
>>> }
> 
> Thank you,
> Kyungmin Park
>
Kyungmin Park Nov. 17, 2008, 8:23 a.m. UTC | #5
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.

>
>>>>                               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.

Thank you,
Kyungmin Park
Kyungmin Park Nov. 17, 2008, 8:37 a.m. UTC | #6
On Mon, Nov 17, 2008 at 5:41 PM, Adrian Hunter
<ext-adrian.hunter@nokia.com> wrote:
> 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, ...)
>
Ah sorry, you're right. I'm confused.

I will repost it.

Thank you,
Kyungmin Park
Adrian Hunter Nov. 17, 2008, 8:41 a.m. UTC | #7
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, ...)
diff mbox

Patch

=================================================================
speedtest: dev = 3
speedtest: Size=16777216  EB size=131072  Write size=2048  EB count=128
		Pages per EB=64  Page size=2048
speedtest: scanning for bad blocks
speedtest: scanned 0
speedtest: scanned 128, found 0 bad
speedtest: erasing
speedtest: erased 0
speedtest: erased 128
speedtest: Testing eraseblock write speed
eraseblock write speed is 2424 KiB/s
speedtest: Testing eraseblock read speed
eraseblock read speed is 11985 KiB/s
speedtest: Testing page write speed
page write speed is 2493 KiB/s
speedtest: Testing page read speed
page read speed is 11586 KiB/s
speedtest: Testing 2 page write speed
2 page write speed is 2681 KiB/s
speedtest: Testing 2 page read speed
2 page read speed is 10922 KiB/s
speedtest: Testing erase speed
erase speed is 192752 KiB/s
speedtest: speedtest finished
=================================================================

Also it's passed all nand-tests.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index aaf3504..bc9b420 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1455,7 +1455,8 @@  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, subpage;
+	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;
@@ -1492,76 +1493,90 @@  static int onenand_write_ops_nolock(struct mtd_info *mtd, loff_t to,
 	column = to & (mtd->writesize - 1);
 
 	/* Loop until all data write */
-	while (written < len) {
-		u_char *wbuf = (u_char *) buf;
+	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);
+			thislen = min_t(int, mtd->writesize - column, len - written);
+			thisooblen = min_t(int, oobsize - oobcolumn, ooblen - oobwritten);
 
-		cond_resched();
+			cond_resched();
 
-		this->command(mtd, ONENAND_CMD_BUFFERRAM, to, thislen);
+			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;
-		}
+			/* 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);
+			this->write_bufferram(mtd, ONENAND_DATARAM, wbuf, 0, mtd->writesize);
 
-		if (oob) {
-			oobbuf = this->oob_buf;
+			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);
+				/* 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;
+				oobwritten += thisooblen;
+				oob += thisooblen;
+				oobcolumn = 0;
+			} else
+				oobbuf = (u_char *) ffchars;
+
+			this->write_bufferram(mtd, ONENAND_SPARERAM, oobbuf, 0, mtd->oobsize);
 		} else
-			oobbuf = (u_char *) ffchars;
+			ONENAND_SET_NEXT_BUFFERRAM(this);
 
-		this->write_bufferram(mtd, ONENAND_SPARERAM, oobbuf, 0, mtd->oobsize);
+		if (!first) {
+			ONENAND_SET_PREV_BUFFERRAM(this);
 
-		this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
+			ret = this->wait(mtd, FL_WRITING);
 
-		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)) {
+				ONENAND_SET_BUFFERRAM1(this);
+				onenand_update_bufferram(mtd, prev + this->writesize, !ret && !prev_subpage);
+			}
 
-		/* In partial page write we don't update bufferram */
-		onenand_update_bufferram(mtd, to, !ret && !subpage);
-		if (ONENAND_IS_2PLANE(this)) {
-			ONENAND_SET_BUFFERRAM1(this);
-			onenand_update_bufferram(mtd, to + this->writesize, !ret && !subpage);
-		}
+			if (ret) {
+				printk(KERN_ERR "onenand_write_ops_nolock: write filaed %d\n", ret);
+				break;
+			}
 
-		if (ret) {
-			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;
+				}
+			}
+			ONENAND_SET_NEXT_BUFFERRAM(this);
 
-		/* Only check verify write turn on */
-		ret = onenand_verify(mtd, buf, to, thislen);
-		if (ret) {
-			printk(KERN_ERR "onenand_write_ops_nolock: verify failed %d\n", ret);
-			break;
+			if (written == len)
+				break;
 		}
 
-		written += thislen;
+		this->command(mtd, ONENAND_CMD_PROG, to, mtd->writesize);
 
-		if (written == len)
-			break;
+		written += thislen;
 
 		column = 0;
+		prev_subpage = subpage;
+		prev = to;
+		prevlen = thislen;
 		to += thislen;
 		buf += thislen;
+		first = 0;
 	}
 
 	ops->retlen = written;