OneNAND: Update OOB free table

OneNAND: Update OOB free table

[Kyungmin Park]

Hi Timo Teras,

Yes, The OneNAND Spec. says these bytes are manged by internal ecc logic.
Acutually it means that when we write some data in this area. The ecc logic generates the ecc bytes automatically.
So even though we write 3 bytes. the OneNAND is written by 5 bytes (3 data bytes, 2 space ecc bytes).

I think we consider the whole spare area. if we don't use this area. we only have 8 bytes of 64 bytes in spare area.
If it makes misbehavior of JFFS2, we have alternative method turn off ecc logic when write oob area.

% original

 OOB Data: ff ff 85 19 03 ff ff ff ff ff ff 30 ff ff ff ff

% after patch

 OOB Data: ff ff 85 19 03 ff ff ff ff ff ff ff ff ff ff ff

Pleaes test this patch

Thank you,
Kyungmin Park

--

Index: drivers/mtd/onenand/onenand_base.c
===================================================================
RCS file: /cvsroot/linux-2.6.18-omap/drivers/mtd/onenand/onenand_base.c,v
retrieving revision 1.2
diff -u -p -r1.2 onenand_base.c
--- drivers/mtd/onenand/onenand_base.c	12 Oct 2006 06:59:27 -0000	1.2
+++ drivers/mtd/onenand/onenand_base.c	7 Dec 2006 23:51:01 -0000
@@ -924,6 +924,7 @@ static int onenand_do_write_oob(struct m
 	struct onenand_chip *this = mtd->priv;
 	int column, ret = 0;
 	int written = 0;
+	int syscfg;
 
 	DEBUG(MTD_DEBUG_LEVEL3, "onenand_write_oob: to = 0x%08x, len = %i\n", (unsigned int) to, (int) len);
 
@@ -939,6 +940,10 @@ static int onenand_do_write_oob(struct m
 	/* Grab the lock and see if the device is available */
 	onenand_get_device(mtd, FL_WRITING);
 
+	/* Turn off the ECC logic for spare area */
+	syscfg = this->read_word(this->base + ONENAND_REG_SYS_CFG1);
+	this->write_word(syscfg | ONENAND_SYS_CFG1_NO_ECC, this->base + ONENAND_REG_SYS_CFG1);
+
 	/* Loop until all data write */
 	while (written < len) {
 		int thislen = min_t(int, mtd->oobsize, len - written);
@@ -979,6 +984,8 @@ static int onenand_do_write_oob(struct m
 	}
 
 out:
+	/* Restore previous settings */
+	this->write_word(syscfg, this->base + ONENAND_REG_SYS_CFG1);
 	/* Deselect and wake up anyone waiting on the device */
 	onenand_release_device(mtd);
 



------- Original Message -------
Sender : Timo Teras<timo.teras@solidboot.com> 
Date   : Dec 07, 2006 19:13
Title  : [PATCH] OneNAND: Update OOB free table

Do not use bytes managed by OneNAND ECC logic for Logical Sector Number
data.

Signed-off-by: Timo Teras <timo.teras@solidboot.com>
---
The OneNAND specification says that the three bytes at offsets 2, 18, 34
and 50 are managed by internal ecc logic. The effect is obvious as
modifying these bytes, it also modifies ECC code for spare area.

Re: OneNAND: Update OOB free table

[Timo Teras]

Hi,

On Fri, Dec 08, 2006 at 12:11:06AM +0000, Kyungmin Park wrote:
> Yes, The OneNAND Spec. says these bytes are manged by internal ecc logic.
> Acutually it means that when we write some data in this area. The ecc logic generates the ecc bytes automatically.
> So even though we write 3 bytes. the OneNAND is written by 5 bytes (3 data bytes, 2 space ecc bytes).
> 
> I think we consider the whole spare area. if we don't use this area. we only have 8 bytes of 64 bytes in spare area.
> If it makes misbehavior of JFFS2, we have alternative method turn off ecc logic when write oob area.

Yes. The original problem was that when we wrote JFFS2 clean marker, the ECC
bytes changed which caused JFFS2 code to think the block unclean
effectively considering all clean and empty blocks as dirty at each boot.

> % original
> 
>  OOB Data: ff ff 85 19 03 ff ff ff ff ff ff 30 ff ff ff ff
> 
> % after patch
> 
>  OOB Data: ff ff 85 19 03 ff ff ff ff ff ff ff ff ff ff ff
> 
> Pleaes test this patch

I suppose this fixes the JFFS2 problem as well. 8 bytes of OOB data in a
page should be enough, but having more free OOB space is always a good thing.

Cheers,
  Timo

Re: OneNAND: Update OOB free table

[Timo Teras]

On Fri, Dec 08, 2006 at 12:11:06AM +0000, Kyungmin Park wrote:
> I think we consider the whole spare area. if we don't use this area. we
> only have 8 bytes of 64 bytes in spare area. If it makes misbehavior of
> JFFS2, we have alternative method turn off ecc logic when write oob area.
> 
> % original
> 
>  OOB Data: ff ff 85 19 03 ff ff ff ff ff ff 30 ff ff ff ff
> 
> % after patch
> 
>  OOB Data: ff ff 85 19 03 ff ff ff ff ff ff ff ff ff ff ff
> 
> Pleaes test this patch

Tested. This breaks reading OOB area and the clean marker from there. After
OOB has been written without ECC it must be read without ECC too or
we start to get ECC errors.

Cheers,
  Timo

OneNAND: Update OOB free table

[Kyungmin Park]

Hi, Timo

> Tested. This breaks reading OOB area and the clean marker from there. After
> OOB has been written without ECC it must be read without ECC too or
>we start to get ECC errors.

Yes, you're right. and the current code don't check the read error.
It's similay problem in onenand_wait which can't report the read/write/erase error.

I think I need to change the logic.

First, fix the onenand_wait error report
Second, check the read error.

Anyway come back to main topic

How about the the change the JFFS2 logic to skip the eccpos in cleanmark check.
In onenand_oob_64, it describes the eccpos. So the JFFS2 skip the eccpos

Is it possible?

Thank you,
Kyungmin Park

Re: OneNAND: Update OOB free table

[Timo Teras]

On Fri, Dec 15, 2006 at 02:38:17AM +0000, Kyungmin Park wrote:
> > Tested. This breaks reading OOB area and the clean marker from there. After
> > OOB has been written without ECC it must be read without ECC too or
> >we start to get ECC errors.
> 
> Yes, you're right. and the current code don't check the read error.
> It's similay problem in onenand_wait which can't report the read/write/erase error.
> 
> I think I need to change the logic.
> 
> First, fix the onenand_wait error report
> Second, check the read error.
> 
> Anyway come back to main topic
> 
> How about the the change the JFFS2 logic to skip the eccpos in cleanmark check.
> In onenand_oob_64, it describes the eccpos. So the JFFS2 skip the eccpos
> 
> Is it possible?

I suppose it's worth asking from JFFS2 people. The free OOB usage seems to be
a bit broken in JFFS2 (it only uses the first pos/len pair of the OOB free
table. 

Cheers,
  Timo

Re: OneNAND: Update OOB free table

[Artem Bityutskiy]

Kyungmin,

On Fri, 2006-12-15 at 02:38 +0000, Kyungmin Park wrote:
> How about the the change the JFFS2 logic to skip the eccpos in cleanmark check.
> In onenand_oob_64, it describes the eccpos. So the JFFS2 skip the eccpos

May you refine what is the problem please?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: OneNAND: Update OOB free table

[Timo Teras]

On Fri, Dec 15, 2006 at 12:41:53PM +0200, Artem Bityutskiy wrote:
> On Fri, 2006-12-15 at 02:38 +0000, Kyungmin Park wrote:
> > How about the the change the JFFS2 logic to skip the eccpos in cleanmark check.
> > In onenand_oob_64, it describes the eccpos. So the JFFS2 skip the eccpos
> 
> May you refine what is the problem please?

jffs2_check_oob_empty() check that the whole OOB area contains only 0xff
bytes except for the clean marker area. This is broken on OneNAND as writing
clean marker to OOB area will change ECC bytes.

Also the clean marker is not written properly as only the first oobfree
table element is used. But this is a different issue.

- Timo

OneNAND: Update OOB free table

[Kyungmin Park]

Hi all,

> On Fri, Dec 15, 2006 at 12:41:53PM +0200, Artem Bityutskiy wrote:
> > On Fri, 2006-12-15 at 02:38 +0000, Kyungmin Park wrote:
> > > How about the the change the JFFS2 logic to skip the eccpos in cleanmark check.
> > > In onenand_oob_64, it describes the eccpos. So the JFFS2 skip the eccpos
> > 
> > May you refine what is the problem please?

> jffs2_check_oob_empty() check that the whole OOB area contains only 0xff
> bytes except for the clean marker area. This is broken on OneNAND as writing
> clean marker to OOB area will change ECC bytes.
>
> Also the clean marker is not written properly as only the first oobfree
> table element is used. But this is a different issue.

How about the this patch?
It skips the ecc position which is defined in ecc layout in mtd drivers.

Thank you,
Kyungmin Park

--

diff -u -p -r1.1.1.1 wbuf.c
--- fs/jffs2/wbuf.c     12 Oct 2006 05:50:30 -0000      1.1.1.1
+++ fs/jffs2/wbuf.c     18 Dec 2006 04:21:29 -0000
@@ -967,6 +967,8 @@ int jffs2_check_oob_empty(struct jffs2_s
        int i, page, ret;
        int oobsize = c->mtd->oobsize;
        struct mtd_oob_ops ops;
+       struct nand_ecclayout *oinfo = c->mtd->ecclayout;
+       int eccpos = 0;

        ops.len = NR_OOB_SCAN_PAGES * oobsize;
        ops.ooblen = oobsize;
@@ -991,6 +993,12 @@ int jffs2_check_oob_empty(struct jffs2_s

        /* Special check for first page */
        for(i = 0; i < oobsize ; i++) {
+               /* Skip ECC position for OneNAND */
+               if (i == oinfo->eccpos[eccpos]) {
+                       eccpos++;
+                       continue;
+               }
+
                /* Yeah, we know about the cleanmarker. */
                if (mode && i >= c->fsdata_pos &&
                    i < c->fsdata_pos + c->fsdata_len)

Re: OneNAND: Update OOB free table

[Artem Bityutskiy]

Kyungmin,

On Mon, 2006-12-18 at 04:24 +0000, Kyungmin Park wrote:
> How about the this patch?
> It skips the ecc position which is defined in ecc layout in mtd drivers.
> 

I will look at this a bit later unless dwmw2 does. I'm really snowed
under at the moment. Apologies.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: OneNAND: Update OOB free table

[Artem Bityutskiy]

Hello,

On Mon, 2006-12-18 at 04:24 +0000, Kyungmin Park wrote:
> How about the this patch?
> It skips the ecc position which is defined in ecc layout in mtd drivers.

Here is my understanding of the problem.

Nowadays MTD supports nicer interface to work with OOB. This is the
MTD_OOB_AUTO command. Currently JFFS2 uses old MTD_OOB_PLACE command.

The idea behind the MTD_OOB_AUTO command is that user provides buffer
with his data, and MTD takes care about how to put it to the OOB area,
MTD knows which OOB bytes may be used. Similar for reading OOB - MTD
automatically reads data bytes, skips ECC, and returns the data.

So what we should do is to teach JFFS2 using MTD_OOB_AUTO instead of
MTD_OOB_PLACE.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: OneNAND: Update OOB free table

[Artem Bityutskiy]

On Thu, 2006-12-21 at 13:15 +0200, Artem Bityutskiy wrote:
> Hello,
> 
> On Mon, 2006-12-18 at 04:24 +0000, Kyungmin Park wrote:
> > How about the this patch?
> > It skips the ecc position which is defined in ecc layout in mtd drivers.
> 
> Here is my understanding of the problem.
> 
> Nowadays MTD supports nicer interface to work with OOB. This is the
> MTD_OOB_AUTO command. Currently JFFS2 uses old MTD_OOB_PLACE command.
> 
> The idea behind the MTD_OOB_AUTO command is that user provides buffer
> with his data, and MTD takes care about how to put it to the OOB area,
> MTD knows which OOB bytes may be used. Similar for reading OOB - MTD
> automatically reads data bytes, skips ECC, and returns the data.
> 
> So what we should do is to teach JFFS2 using MTD_OOB_AUTO instead of
> MTD_OOB_PLACE.

I may try to do this, but only in 2007. Vitaly said he may also try to
do this later.

Also, dwmw2 is concerned about compatibility, so we should explore this
too.

Kyungmin, may you make sure OneNAND is OK WRT MTD_OOB_AUTO?

Also Kyungmin, in you driver I see touch_softlockup_watchdog() calls. I
believe they should not be there. You must be hid a problem with this
instead of fixing it. Please, look at that stuff.

Kyungmin, do you have your own OneNAND git tree? If yes, what's the URL?
How about putting it on infradead.org, just like we have our UBI git
there?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

OneNAND: Update OOB free table

[Kyungmin Park]

Hi,

> > On Mon, 2006-12-18 at 04:24 +0000, Kyungmin Park wrote:
> > > How about the this patch?
> > > It skips the ecc position which is defined in ecc layout in mtd drivers.
> > 
> > Here is my understanding of the problem.
> > 
> > Nowadays MTD supports nicer interface to work with OOB. This is the
> > MTD_OOB_AUTO command. Currently JFFS2 uses old MTD_OOB_PLACE command.
> > 
> > The idea behind the MTD_OOB_AUTO command is that user provides buffer
> > with his data, and MTD takes care about how to put it to the OOB area,
> > MTD knows which OOB bytes may be used. Similar for reading OOB - MTD
> > automatically reads data bytes, skips ECC, and returns the data.
> > 
> > So what we should do is to teach JFFS2 using MTD_OOB_AUTO instead of
> > MTD_OOB_PLACE.
>
> I may try to do this, but only in 2007. Vitaly said he may also try to
> do this later.
> 
> Also, dwmw2 is concerned about compatibility, so we should explore this
> too.

Okay, I understand the MTD_OOB_AUTO concept. but now JFFS2 only use the MTD_OOB_PLACE. 
so JFFS2 skip the ecc position for backward compatibility.

>
> Kyungmin, may you make sure OneNAND is OK WRT MTD_OOB_AUTO?
>

Now OneNAND don't support MTD_OOB_AUTO, but if the jffs2 supports the this one. I will support it.
I think it's more reasonable to handle oob in driver level instead of filesystem.

> Also Kyungmin, in you driver I see touch_softlockup_watchdog() calls. I
> believe they should not be there. You must be hid a problem with this
> instead of fixing it. Please, look at that stuff.

Actually, I'm not sure where's the right place. But if we don't touch the watchdong.
it will soft lockup when mounting the JFFS2 with big partition. (I just borrowed it from nand.)

> 
> Kyungmin, do you have your own OneNAND git tree? If yes, what's the URL?
> How about putting it on infradead.org, just like we have our UBI git
> there?

No, I used it in infradead.org. but I just used for commit.

http://git.infradead.org/?p=users/kmpark/onenand-mtd-2.6.git;a=summary

Today, I will commit the onenand patches for test.

Thank you,
Kyungmin Park

Re: OneNAND: Update OOB free table

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On Fri, 2006-12-22 at 01:52 +0000, Kyungmin Park wrote:
> Okay, I understand the MTD_OOB_AUTO concept. but now JFFS2 only use the MTD_OOB_PLACE. 
> so JFFS2 skip the ecc position for backward compatibility.
...
> 
> Now OneNAND don't support MTD_OOB_AUTO, but if the jffs2 supports the this one. I will support it.
> I think it's more reasonable to handle oob in driver level instead of filesystem.

I take care about this in JFFS, you take care in OneNAND, OK?

> Actually, I'm not sure where's the right place. But if we don't touch the watchdong.
> it will soft lockup when mounting the JFFS2 with big partition. (I just borrowed it from nand.)

Anyway, you should not touch watchdog. You problem is fixed by adding
cond_resched() at the right places. What you did - you shut up watchdog
which screamed about a real error, instead of solving the error.

I have prepared a patch - it is in my dedekind-mtd-2.6.git GIT tree. It
is also attached.

> No, I used it in infradead.org. but I just used for commit.

Cool.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

[-- Attachment #2: onenand-sched.diff --]
[-- Type: application/mbox, Size: 3144 bytes --]

OneNAND: Update OOB free table

[Kyungmin Park]

> > Now OneNAND don't support MTD_OOB_AUTO, but if the jffs2 supports the this one. I will support it.
> > I think it's more reasonable to handle oob in driver level instead of filesystem.

> I take care about this in JFFS, you take care in OneNAND, OK?

No problem. I will support it.

> > Actually, I'm not sure where's the right place. But if we don't touch the watchdong.
> > it will soft lockup when mounting the JFFS2 with big partition. (I just borrowed it from nand.)

> Anyway, you should not touch watchdog. You problem is fixed by adding
> cond_resched() at the right places. What you did - you shut up watchdog
> which screamed about a real error, instead of solving the error.

It looks good. and also it works well. I will commit it.

Thank you,
Kyungmin Park