mtd: mxc_nand: fix 8 bit ECC and large oob
Subsystems (3)
cover mtd: mxc_nand: fix 8 bit ECC and large oob
This patch series fixes mxc_nand bugs that I encountered while trying to use 8 bit ECC mode on i.MX25 with the Micron MT29F8G08ABABA flash chip.
Many thanks to Uwe Kleine-K?nig for guiding me through the process of debugging and fixing, and for contributing a clean-up patch to this series.
This series applies and tested on current Linus tree as of d19d133e432248.
Baruch Siach (3):
mtd: mxc_nand: limit the size of used oob
mtd: mxc_nand: fix truncate of unaligned oob copying
mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC
Uwe Kleine-K?nig (1):
mtd: nand: mxc_nand: cleanup copy_spare function
drivers/mtd/nand/mxc_nand.c | 74 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 16 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/4 mtd: nand: mxc_nand: cleanup copy_spare function
From: Uwe Kleine-K?nig <[email protected]>
To give people without the reference manual at hand a chance to understand how spare area is handled in the i.MX nand controller, improve commenting, naming of variables and coding style.
No functional change intended.
--- drivers/mtd/nand/mxc_nand.c | 46 ++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)
drivers/mtd/nand/mxc_nand.c +31 -15
@@ -807,32 +807,48 @@ static void mxc_nand_select_chip_v2(struct mtd_info *mtd, int chip) }/* -* Function to transfer data to/from spare area. +* The controller splits a page into data chunks of 512 bytes + partial oob. +* There are writesize / 512 such chunks, the size of the partial oob parts is +* oobsize / #chunks rounded down to a multiple of 2. The last oob chunk then +* contains additionally the byte lost by rounding (if any). +* This function handles the needed shuffling between host->data_buf (which +* holds a page in natural order, i.e. writesize bytes data + oobsize bytes +* spare) and the NFC buffer. */ static void copy_spare (struct mtd_info *mtd,bool bfrom) {struct nand_chip *this = mtd->priv;struct mxc_nand_host *host = this->priv; -u16 i, j; -u16 n = mtd->writesize >>9 ; +u16 i, oob_chunk_size; +u16 num_chunks = mtd->writesize /512 ; +u8 *d = host->data_buf + mtd->writesize;u8 __iomem *s = host->spare0; -u16 t = host->devtype_data->spare_len; +u16 sparebuf_size = host->devtype_data->spare_len; - j = (mtd->oobsize / n >>1 ) <<1 ; +/* size of oob chunk for all but possibly the last one */ + oob_chunk_size = (mtd->oobsize / num_chunks) & ~1 ;if (bfrom) { -for (i =0 ; i < n -1 ; i++) -memcpy32_fromio (d + i * j, s + i * t, j); - -/* the last section */ -memcpy32_fromio (d + i * j, s + i * t, mtd->oobsize - i * j); +for (i =0 ; i < num_chunks -1 ; i++) +memcpy32_fromio (d + i * oob_chunk_size, + s + i * sparebuf_size, + oob_chunk_size); + +/* the last chunk */ +memcpy32_fromio (d + i * oob_chunk_size, + s + i * sparebuf_size, + mtd->oobsize - i * oob_chunk_size); }else { -for (i =0 ; i < n -1 ; i++) -memcpy32_toio (&s[i * t], &d[i * j], j); - -/* the last section */ -memcpy32_toio (&s[i * t], &d[i * j], mtd->oobsize - i * j); +for (i =0 ; i < num_chunks -1 ; i++) +memcpy32_toio (&s[i * sparebuf_size], + &d[i * oob_chunk_size], + oob_chunk_size); + +/* the last chunk */ +memcpy32_toio (&s[oob_chunk_size * sparebuf_size], + &d[i * oob_chunk_size], + mtd->oobsize - i * oob_chunk_size); } }
Signature · 1 line
2.1.4
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
In reply to Baruch SiachFor 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or 8bit ECC data, respectively. Larger oobsize confuses the logic of copy_spare(). Limit the size of used oob size to avoid that.
--- drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/mtd/nand/mxc_nand.c +11 -4
@@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) {struct nand_chip *this = mtd->priv;struct mxc_nand_host *host = this->priv; -u16 i, oob_chunk_size; +u16 i, oob_chunk_size, used_oobsize;u16 num_chunks = mtd->writesize /512 ;u8 *d = host->data_buf + mtd->writesize;u8 __iomem *s = host->spare0;u16 sparebuf_size = host->devtype_data->spare_len; +/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 ) + used_oobsize =218 ; +else if (mtd->oobsize >=128 ) + used_oobsize =128 ; +else + used_oobsize = mtd->oobsize;/* size of oob chunk for all but possibly the last one */ - oob_chunk_size = (mtd->oobsize / num_chunks) & ~1 ; + oob_chunk_size = (used_oobsize / num_chunks) & ~1 ;if (bfrom) {for (i =0 ; i < num_chunks -1 ; i++)
@@ -838,7 +845,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)/* the last chunk */ memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - mtd->oobsize - i * oob_chunk_size); + used_oobsize - i * oob_chunk_size); }else {for (i =0 ; i < num_chunks -1 ; i++) memcpy32_toio(&s[i * sparebuf_size],
@@ -848,7 +855,7 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)/* the last chunk */ memcpy32_toio (&s[oob_chunk_size * sparebuf_size], &d[i * oob_chunk_size], - mtd->oobsize - i * oob_chunk_size); + used_oobsize - i * oob_chunk_size); } }
Signature · 1 line
2.1.4
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Quoted 17 lines · Baruch Siach
For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or 8bit ECC data, respectively. Larger oobsize confuses the logic of
copy_spare(). Limit the size of used oob size to avoid that.Signed-off-by Baruch Siach <[email protected]>--- drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)drivers/mtd/nand/mxc_nand.c +0
-0@@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) {struct nand_chip *this = mtd->priv;struct mxc_nand_host *host = this->priv;
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 830
Quoted 10 lines
drivers/mtd/nand/mxc_nand.c +3
-1-u16 i, oob_chunk_size; +u16 i, oob_chunk_size, used_oobsize;u16 num_chunks = mtd->writesize /512 ;u8 *d = host->data_buf + mtd->writesize;u8 __iomem *s = host->spare0;u16 sparebuf_size = host->devtype_data->spare_len; +/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 )
IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?) Moreover the comment is misleading as it only applies to 4K flashes. At least the driver works well with (2ki +
128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?).
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 835
Quoted 5 lines
drivers/mtd/nand/mxc_nand.c +5
-0+ used_oobsize =218 ; + else if (mtd->oobsize >= 128) + used_oobsize =128 ; + else + used_oobsize = mtd->oobsize;
Is it worth to put this check into _probe and put the used size into driver data?
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Uwe,
Quoted 26 lines · Uwe Kleine-K?nig
Quoted 25 lines · Baruch Siach
For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or 8bit ECC data, respectively. Larger oobsize confuses the logic of
copy_spare(). Limit the size of used oob size to avoid that.Signed-off-by Baruch Siach <[email protected]>--- drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)drivers/mtd/nand/mxc_nand.c +1
-1@@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) {struct nand_chip *this = mtd->priv;struct mxc_nand_host *host = this->priv; -u16 i, oob_chunk_size; +u16 i, oob_chunk_size, used_oobsize;u16 num_chunks = mtd->writesize /512 ;u8 *d = host->data_buf + mtd->writesize;u8 __iomem *s = host->spare0;u16 sparebuf_size = host->devtype_data->spare_len;
Quoted 5 lines
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 830
Quoted 2 lines
drivers/mtd/nand/mxc_nand.c +2
-0+/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 )IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?)
Yes, but how would I know? I am following here the assumption of get_eccsize() that enables 8 bit ECC when there is enough oobsize.
Quoted 4 lines
Moreover the comment is misleading as it
only applies to 4K flashes. At least the driver works well with (2ki +
128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?).
I suspect there are more bugs. A simple way to trigger the bug I encountered is by doing a sub-page write, that is:
dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2
and then try reading this page. You may need to dirty the (useless; random) content of ecccalc to see the bug (you can conveniently use mxc_nand_calculate_ecc() for that). What copy_spare() currently do (spread ECC chunks evenly over oob) does not match nandv2_hw_eccoob_largepage.
Quoted 7 lines
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 835
Quoted 5 lines
drivers/mtd/nand/mxc_nand.c +5
-0+ used_oobsize =218 ; + else if (mtd->oobsize >= 128) + used_oobsize =128 ; + else + used_oobsize = mtd->oobsize;Is it worth to put this check into
_probeand put the used size into driver data?
I think you are right. I'll also add checks for smaller page/oob size.
Thanks for reviewing,
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hello,
Quoted 27 lines · Baruch Siach
Quoted 26 lines · Uwe Kleine-K?nig
Quoted 25 lines · Baruch Siach
For 4k pages the i.MX NFC hardware uses exactly 218 or 128 bytes for 4bit or 8bit ECC data, respectively. Larger oobsize confuses the logic of
copy_spare(). Limit the size of used oob size to avoid that.Signed-off-by Baruch Siach <[email protected]>--- drivers/mtd/nand/mxc_nand.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)drivers/mtd/nand/mxc_nand.c +1
-1@@ -819,15 +819,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom) {struct nand_chip *this = mtd->priv;struct mxc_nand_host *host = this->priv; -u16 i, oob_chunk_size; +u16 i, oob_chunk_size, used_oobsize;u16 num_chunks = mtd->writesize /512 ;u8 *d = host->data_buf + mtd->writesize;u8 __iomem *s = host->spare0;u16 sparebuf_size = host->devtype_data->spare_len;
Quoted 8 lines
Quoted 5 lines
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 830
Quoted 2 lines
drivers/mtd/nand/mxc_nand.c +2
-0+/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 )IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?)
Yes, but how would I know? I am following here the assumption of
get_eccsize()that enables 8 bit ECC when there is enough oobsize.
Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to 0x70 for 224 bytes oob? Optimally this would result in the last 6 bytes being written but not protected by hardware ecc.
Quoted 8 lines
Quoted 4 lines
Moreover the comment is misleading as it
only applies to 4K flashes. At least the driver works well with (2ki +
128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?).I suspect there are more bugs. A simple way to trigger the bug I encountered is by doing a sub-page write, that is:
Quoted 7 lines
dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2
and then try reading this page. You may need to dirty the (useless; random) content of ecccalc to see the bug (you can conveniently use
mxc_nand_calculate_ecc()for that). Whatcopy_spare()currently do (spread ECC chunks evenly over oob) does not matchnandv2_hw_eccoob_largepage.
I didn't follow, but would be willing to review a fix :-)
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Uwe,
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 8 lines · Baruch Siach
Quoted 6 lines · Uwe Kleine-K?nig
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 830
Quoted 2 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +2
-0+/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 )IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?)
Quoted 5 lines
Quoted 2 lines
Yes, but how would I know? I am following here the assumption of
get_eccsize()that enables 8 bit ECC when there is enough oobsize.Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?
Which register is that?
Quoted 2 lines
Optimally this would result in the last 6 bytes being written but not protected by hardware ecc.
Last 6 bytes of what? AFAIK, hardware ECC does not protect oob at all.
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Moreover the comment is misleading as it
only applies to 4K flashes. At least the driver works well with (2ki +
128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?).I suspect there are more bugs. A simple way to trigger the bug I encountered is by doing a sub-page write, that is:
Quoted 8 lines
Quoted 7 lines
dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2
and then try reading this page. You may need to dirty the (useless; random) content of ecccalc to see the bug (you can conveniently use
mxc_nand_calculate_ecc()for that). Whatcopy_spare()currently do (spread ECC chunks evenly over oob) does not matchnandv2_hw_eccoob_largepage.I didn't follow, but would be willing to review a fix :-)
OK. I'll try something.
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hello Baruch,
Quoted 10 lines · Baruch Siach
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 8 lines · Baruch Siach
Quoted 6 lines · Uwe Kleine-K?nig
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 830
Quoted 2 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +2
-0+/* hardware can only use 218 or 128 oob bytes for ecc */ +if (mtd->oobsize >=218 )IMHO this is the wrong check. What if your part (with 224 bytes spare) is used but the machine only uses the small layout e.g. for booting? (That would work, wouldn't it?)
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Yes, but how would I know? I am following here the assumption of
get_eccsize()that enables 8 bit ECC when there is enough oobsize.Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?Which register is that?
Spare Area Size Register (SPAS) at offset 0x1e10 for the i.MX25 (that's what you're using, don't you?).
Quoted 4 lines
Quoted 2 lines
Optimally this would result in the last 6 bytes being written but not protected by hardware ecc.
Last 6 bytes of what? AFAIK, hardware ECC does not protect oob at all.
I thought it did, might be wrong here.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Moreover the comment is misleading as it
only applies to 4K flashes. At least the driver works well with (2ki +
128) bytes pages (while there are only 64 bytes spare used?? Maybe there are still more bugs?).I suspect there are more bugs. A simple way to trigger the bug I encountered is by doing a sub-page write, that is:
dd if=/dev/zero bs=1024 count=1 of=/dev/mtd2 dd if=/dev/zero bs=1024 count=1 seek=1 of=/dev/mtd2
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
and then try reading this page. You may need to dirty the (useless; random) content of ecccalc to see the bug (you can conveniently use
mxc_nand_calculate_ecc()for that). Whatcopy_spare()currently do (spread ECC chunks evenly over oob) does not matchnandv2_hw_eccoob_largepage.I didn't follow, but would be willing to review a fix :-)
OK. I'll try something.
\o/
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Uwe,
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 6 lines · Baruch Siach
Quoted 3 lines · Uwe Kleine-K?nig
Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?Which register is that?
Spare Area Size Register (SPAS) at offset
0x1e10for the i.MX25 (that's what you're using, don't you?).
Yes, that's what I'm using.
I tried setting the SPAS register to oobsize/2 (0x70 in my case), but I see no change in behaviour. Moreover, it turns out the previously Barebox set this register (apparently wrongly) to 0x20 for spare size of 64. Current Barebox master still do. For v3 Barebox limits CONFIG2_SPAS to 218 bytes spare size since Eric B?nard's commit 632c45795065 (nand_imx: update to support onfi & 4k flashs, 2012-07-05). As you can see, the kernel doesn't touch this register for v2 NFC.
I have no idea what is the effect of the SPAS in v2 (or any other) NFC.
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hello,
Quoted 11 lines · Baruch Siach
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 6 lines · Baruch Siach
Quoted 3 lines · Uwe Kleine-K?nig
Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?Which register is that?
Spare Area Size Register (SPAS) at offset
0x1e10for the i.MX25 (that's what you're using, don't you?).
Quoted 6 lines
Yes, that's what I'm using.
I tried setting the SPAS register to oobsize/2 (
0x70in my case), but I see no change in behaviour. Moreover, it turns out the previously Barebox set this register (apparently wrongly) to0x20for spare size of 64. Current Barebox master still do. For v3 Barebox limitsCONFIG2_SPASto 218 bytes spare size
I guess for v21 writing to SPAS is just what the controller assumes after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand has 64.
Quoted 3 lines
since Eric B?nard's commit
632c45795065(nand_imx: update to support onfi & 4k flashs, 2012-07-05). As you can see, the kernel doesn't touch this register for v2 NFC.
right.
Quoted 1 line
I have no idea what is the effect of the SPAS in v2 (or any other) NFC.
I didn't try, but I'm surprised it doesn't make a difference. How did you test? I'd do in barebox:
memset -w 0xbb000000 0x55 0x1200 set SPAS to 218/2 trigger page read check how much of the 0x55 was overwritten set SPAS to 224/2 trigger page read compare with above
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Uwe,
Quoted 31 lines · Uwe Kleine-K?nig
Quoted 11 lines · Baruch Siach
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 6 lines · Baruch Siach
Quoted 3 lines · Uwe Kleine-K?nig
Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?Which register is that?
Spare Area Size Register (SPAS) at offset
0x1e10for the i.MX25 (that's what you're using, don't you?).Quoted 6 lines
Yes, that's what I'm using.
I tried setting the SPAS register to oobsize/2 (
0x70in my case), but I see no change in behaviour. Moreover, it turns out the previously Barebox set this register (apparently wrongly) to0x20for spare size of 64. Current Barebox master still do. For v3 Barebox limitsCONFIG2_SPASto 218 bytes spare sizeI guess for v21 writing to SPAS is just what the controller assumes after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand has 64.
Quoted 3 lines
since Eric B?nard's commit
632c45795065(nand_imx: update to support onfi & 4k flashs, 2012-07-05). As you can see, the kernel doesn't touch this register for v2 NFC.right.
Quoted 1 line
I have no idea what is the effect of the SPAS in v2 (or any other) NFC.
I didn't try, but I'm surprised it doesn't make a difference. How did you test? I'd do in barebox:
Quoted 7 lines
memset -w 0xbb000000 0x55 0x1200 set SPAS to 218/2 trigger page read check how much of the 0x55 was overwritten set SPAS to 224/2 trigger page read compare with above
Just did this test. No matter how I set SPAS, Only first 26 bytes of each 64 bytes spare data buffer chunk are overwritten. I tried setting SPAS to 224/2 (0x70), 218/2 (0x6d), and 128/2 (0x40).
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hello Baruch,
Cc += Fabio
Quoted 40 lines · Baruch Siach
Quoted 31 lines · Uwe Kleine-K?nig
Quoted 11 lines · Baruch Siach
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 6 lines · Baruch Siach
Quoted 3 lines · Uwe Kleine-K?nig
Hmm I rechecked the reference manual and found a register to specify the size of the spare area (I didn't notice that one before). Did you try what happens if you set this to
0x70for 224 bytes oob?Which register is that?
Spare Area Size Register (SPAS) at offset
0x1e10for the i.MX25 (that's what you're using, don't you?).Quoted 6 lines
Yes, that's what I'm using.
I tried setting the SPAS register to oobsize/2 (
0x70in my case), but I see no change in behaviour. Moreover, it turns out the previously Barebox set this register (apparently wrongly) to0x20for spare size of 64. Current Barebox master still do. For v3 Barebox limitsCONFIG2_SPASto 218 bytes spare sizeI guess for v21 writing to SPAS is just what the controller assumes after reset. I.e. that a 512-byte nand has 16 bytes spare and a 2k-nand has 64.
Quoted 3 lines
since Eric B?nard's commit
632c45795065(nand_imx: update to support onfi & 4k flashs, 2012-07-05). As you can see, the kernel doesn't touch this register for v2 NFC.right.
Quoted 1 line
I have no idea what is the effect of the SPAS in v2 (or any other) NFC.
I didn't try, but I'm surprised it doesn't make a difference. How did you test? I'd do in barebox:
Quoted 7 lines
memset -w 0xbb000000 0x55 0x1200 set SPAS to 218/2 trigger page read check how much of the 0x55 was overwritten set SPAS to 224/2 trigger page read compare with above
Quoted 3 lines
Just did this test. No matter how I set SPAS, Only first 26 bytes of each 64 bytes spare data buffer chunk are overwritten. I tried setting SPAS to 224/2 (
0x70), 218/2 (0x6d), and 128/2 (0x40).
That's surprising. It would be interesting to hear from Freescale what is wrong here (Fabio? Maybe contact support?). I quickly skimmed through the errata documents but didn't found anything. So it's not only that you need to handle a pristine flash differently on i.MX than on all other controllers but also that you cannot make full use of the flash's capacity. *sigh*
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Fabio Estevam
reviewerPATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Baruch,
Quoted 3 lines · Uwe Kleine-K?nig
That's surprising. It would be interesting to hear from Freescale what is wrong here (Fabio? Maybe contact support?). I quickly skimmed through the errata documents but didn't found anything. So it's not only that
Yes, that's something I am not aware of either.
Please contact the FSL support folks so that the hardware guys could assist on this topic.
Regards,
Fabio Estevam
PATCH 2/4 mtd: mxc_nand: limit the size of used oob
Hi Fabio,
Quoted 11 lines · Fabio Estevam
Quoted 3 lines · Uwe Kleine-K?nig
That's surprising. It would be interesting to hear from Freescale what is wrong here (Fabio? Maybe contact support?). I quickly skimmed through the errata documents but didn't found anything. So it's not only that
Yes, that's something I am not aware of either.
Please contact the FSL support folks so that the hardware guys could assist on this topic.
Our past experience with hardware support at the micro level was not all that great. Anyway, I dropped the limit to 128 for oobsize < 218, since this is only a theoretic bug, and I can't really test it.
Thanks,
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 3/4 mtd: mxc_nand: fix truncate of unaligned oob copying
In reply to Baruch SiachCopy to/from oob io area might not be aligned to 4 bytes. When 8 bit ECC is used, the buffer size is 26. Make sure we pass 4 bytes aligned buffer size to memcpy32_{to,from}io to avoid truncating the buffer.
--- drivers/mtd/nand/mxc_nand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/mtd/nand/mxc_nand.c +4 -4
@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)for (i =0 ; i < num_chunks -1 ; i++)memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - oob_chunk_size); +ALIGN (oob_chunk_size,4 ));/* the last chunk */ memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - used_oobsize - i * oob_chunk_size); +ALIGN (used_oobsize - i * oob_chunk_size,4 )); }else {for (i =0 ; i < num_chunks -1 ; i++)memcpy32_toio (&s[i * sparebuf_size], &d[i * oob_chunk_size], - oob_chunk_size); +ALIGN (oob_chunk_size,4 ));/* the last chunk */ memcpy32_toio (&s[oob_chunk_size * sparebuf_size], &d[i * oob_chunk_size], - used_oobsize - i * oob_chunk_size); +ALIGN (used_oobsize - i * oob_chunk_size,4 )); } }
Signature · 1 line
2.1.4
PATCH 3/4 mtd: mxc_nand: fix truncate of unaligned oob copying
Hello Baruch,
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 843
Quoted 10 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +1
-1@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)for (i =0 ; i < num_chunks -1 ; i++)memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - oob_chunk_size); +ALIGN (oob_chunk_size,4 ));
If oob_chunk_size isn't 32-bit-aligned, d + i * oob_chunk_size isn't either for uneven i. That's not nice. I suggest to use memcpy16_fromio.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 3/4 mtd: mxc_nand: fix truncate of unaligned oob copying
Hi Uwe,
Quoted 13 lines · Uwe Kleine-K?nig
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 843
Quoted 10 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +1
-1@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)for (i =0 ; i < num_chunks -1 ; i++)memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - oob_chunk_size); +ALIGN (oob_chunk_size,4 ));If
oob_chunk_sizeisn't 32-bit-aligned, d + i *oob_chunk_sizeisn't either for uneven i. That's not nice. I suggest to usememcpy16_fromio.
memcpy32_fromio() was introduced by Sascha in 096bcc231fd2 (mtd: mxc_nand: use 32bit copy functions, 2012-05-29), replacing memcpy_fromio() to force 32bit io access. Are you sure the hypothetical memcpy16_fromio() would work?
baruch
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 3/4 mtd: mxc_nand: fix truncate of unaligned oob copying
Hello Baruch,
Quoted 15 lines · Baruch Siach
Quoted 13 lines · Uwe Kleine-K?nig
drivers/mtd/nand/mxc_nand.c static void copy_spare(struct mtd_info *mtd, bool bfrom) · line 843
Quoted 10 lines · Baruch Siach
drivers/mtd/nand/mxc_nand.c +1
-1@@ -840,22 +840,22 @@ static void copy_spare(struct mtd_info *mtd, bool bfrom)for (i =0 ; i < num_chunks -1 ; i++)memcpy32_fromio (d + i * oob_chunk_size, s + i * sparebuf_size, - oob_chunk_size); +ALIGN (oob_chunk_size,4 ));If
oob_chunk_sizeisn't 32-bit-aligned, d + i *oob_chunk_sizeisn't either for uneven i. That's not nice. I suggest to usememcpy16_fromio.
Quoted 3 lines
memcpy32_fromio()was introduced by Sascha in096bcc231fd2(mtd:mxc_nand: use 32bit copy functions, 2012-05-29), replacingmemcpy_fromio()to force 32bit io access. Are you sure the hypotheticalmemcpy16_fromio()would work?
according to the reference manual you can use 16-bit or 32-bit accesses. And giving that in some cases the amount of valid data (in bytes) is ? 2 (mod 4) 16-bit access seem to be the obvious thing. Also Sascha only wrote about byte accesses being problematic in 096bcc231fd2. So I'm expecting that 16-bit operators should be fine.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 4/4 mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC
In reply to Baruch SiachHardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet another static struct nand_ecclayout, generate it in code.
--- drivers/mtd/nand/mxc_nand.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
drivers/mtd/nand/mxc_nand.c +20 -1
@@ -934,6 +934,22 @@ static int get_eccsize(struct mtd_info *mtd)return 8 ; } +static void ecc_8bit_layout (struct nand_ecclayout *layout) +{ +int i, j; + +for (i =0 ; i <8 ; i++) +for (j =0 ; j <18 ; j++) + layout->eccpos[i*18 + j] = i*26 + j +7 ; + + layout->oobfree[0 ].offset =2 ; + layout->oobfree[0 ].length =4 ; +for (i =1 ; i <8 ; i++) { + layout->oobfree[i].offset = i*26 ; + layout->oobfree[i].length =7 ; + } +} +static void preset_v1 (struct mtd_info *mtd) {struct nand_chip *nand_chip = mtd->priv;
@@ -1610,8 +1626,11 @@ static int mxcnd_probe(struct platform_device *pdev)if (mtd->writesize ==2048 ) this->ecc.layout = host->devtype_data->ecclayout_2k; -else if (mtd->writesize ==4096 ) +else if (mtd->writesize ==4096 ) { this->ecc.layout = host->devtype_data->ecclayout_4k; +if (get_eccsize (mtd) ==8 ) +ecc_8bit_layout (this->ecc.layout); + }if (this->ecc.mode ==NAND_ECC_HW ) {if (is_imx21_nfc (host) ||is_imx27_nfc (host))
Signature · 1 line
2.1.4