| Message ID | 1319824292-11085-13-git-send-email-robert.jarzmik@free.fr |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 28, 2011 at 06:51:31PM +0100, Robert Jarzmik wrote: > > +/** > + * struct docg3_bch - BCH engine > + */ > +static struct bch_control *docg3_bch; Why not putting this into your struct docg3, instead of adding a global var ? > +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc) > +{ > + u8 ecc[DOC_ECC_BCH_T + 1]; Should be 'u8 ecc[DOC_ECC_BCH_SIZE];' > + int errorpos[DOC_ECC_BCH_T + 1], i, numerrs; Using 'errorpos[DOC_ECC_BCH_T]' is fine, no need for '+ 1'. (...) > + > + for (i = 0; i < DOC_ECC_BCH_SIZE; i++) > + ecc[i] = bitrev8(calc_ecc[i]); > + numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, > + NULL, ecc, NULL, errorpos); > + if (numerrs < 0) > + return numerrs; Maybe do something like 'printk(KERN_ERR "ecc unrecoverable error\n");' when numerrs < 0 ? (...) > + for (i = 0; i < numerrs; i++) > + change_bit(errorpos[i], buf); 'buf' holds the buffer of read data (512 + 7 + 1 bytes); hence you should change the above code into something like: for (i = 0; i < numerrs; i++) if (errorpos[i] < DOC_ECC_BCH_COVERED_BYTES*8) /* error is located in data, correct it */ change_bit(errorpos[i], buf); /* else error in ecc bytes, no data change needed */ otherwise 'change_bit' will be out of bounds when a bitflip occurs in ecc bytes. Note that we still need to report bitflips in that case, to let upper layers scrub them. (...) > + docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T, > + DOC_ECC_BCH_PRIMPOLY); > + if (!docg3_bch) > + goto nomem2; Just a side note: if you need to get maximum performance from the BCH library, you can set fixed values for M and T in your Kconfig, with something like: config MTD_DOCG3 tristate "M-Systems Disk-On-Chip G3" select BCH ---help--- This provides an MTD device driver for the M-Systems DiskOnChip G3 devices. if MTD_DOCG3 config BCH_CONST_M default 14 config BCH_CONST_T default 4 endif The only drawback is that it won't work if you select your DOCG3 driver and, at the same time, other MTD drivers that also use fixed, but different parameters. (...) > @@ -1660,6 +1717,7 @@ static int __exit docg3_release(struct platform_device *pdev) > doc_release_device(docg3_floors[floor]); > > kfree(docg3_floors); > + kfree(docg3_bch); This should be 'free_bch(docg3_bch)'. Otherwise it looks OK to me; did you test BCH correction by simulating corruptions (of 1-4 bits, and also 5 bits to trigger failures) in nand data ? Best Regards, -- Ivan
On Sat, Oct 29, 2011 at 10:52:48AM +0200, Ivan Djelic wrote: > Just a side note: if you need to get maximum performance from the BCH library, > you can set fixed values for M and T in your Kconfig, with something like: > > config MTD_DOCG3 > tristate "M-Systems Disk-On-Chip G3" > select BCH Oops, just noticed that you would also need to add this line here: select BCH_CONST_PARAMS > ---help--- > This provides an MTD device driver for the M-Systems DiskOnChip > G3 devices. > > if MTD_DOCG3 > config BCH_CONST_M > default 14 > config BCH_CONST_T > default 4 > endif > > > The only drawback is that it won't work if you select your DOCG3 driver and, at > the same time, other MTD drivers that also use fixed, but different parameters.
Ivan Djelic <ivan.djelic@parrot.com> writes: > On Fri, Oct 28, 2011 at 06:51:31PM +0100, Robert Jarzmik wrote: >> >> +/** >> + * struct docg3_bch - BCH engine >> + */ >> +static struct bch_control *docg3_bch; > > Why not putting this into your struct docg3, instead of adding a global var ? Because I have multiple floors (ie. 4 floors for example), which are split into 4 different devices. If I put that in docg3 structures (ie. the 4 allocated structures, each for one floor), I'd either have to : - allocate 4 different bch "engines" - or count docg3 releases and release the bch at the last kfree(docg3), which makes me have another global variable. > >> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc) >> +{ >> + u8 ecc[DOC_ECC_BCH_T + 1]; > > Should be 'u8 ecc[DOC_ECC_BCH_SIZE];' OK, I'll amend it. > >> + int errorpos[DOC_ECC_BCH_T + 1], i, numerrs; > > Using 'errorpos[DOC_ECC_BCH_T]' is fine, no need for '+ 1'. OK, got it. >> + >> + for (i = 0; i < DOC_ECC_BCH_SIZE; i++) >> + ecc[i] = bitrev8(calc_ecc[i]); >> + numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, >> + NULL, ecc, NULL, errorpos); >> + if (numerrs < 0) >> + return numerrs; > > Maybe do something like 'printk(KERN_ERR "ecc unrecoverable error\n");' when > numerrs < 0 ? That will be too verbose. As there are special partitions where the ECC is not used that way (ie. SAFTL partitions I don't understand well yet), the ECC is always wrong there. Moreover, the error is reported as EBADMSG later (in doc_read), making the information available to userland. > > (...) > >> + for (i = 0; i < numerrs; i++) >> + change_bit(errorpos[i], buf); > > 'buf' holds the buffer of read data (512 + 7 + 1 bytes); hence you should > change the above code into something like: > > for (i = 0; i < numerrs; i++) > if (errorpos[i] < DOC_ECC_BCH_COVERED_BYTES*8) > /* error is located in data, correct it */ > change_bit(errorpos[i], buf); > /* else error in ecc bytes, no data change needed */ > > otherwise 'change_bit' will be out of bounds when a bitflip occurs in ecc > bytes. Note that we still need to report bitflips in that case, to let upper > layers scrub them. Ah OK, I wasn't aware of that. I'll amend the code, thanks. > > (...) >> + docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T, >> + DOC_ECC_BCH_PRIMPOLY); >> + if (!docg3_bch) >> + goto nomem2; > > Just a side note: if you need to get maximum performance from the BCH library, > you can set fixed values for M and T in your Kconfig, with something like: > > config MTD_DOCG3 > tristate "M-Systems Disk-On-Chip G3" > select BCH > ---help--- > This provides an MTD device driver for the M-Systems DiskOnChip > G3 devices. > > if MTD_DOCG3 > config BCH_CONST_M > default 14 > config BCH_CONST_T > default 4 > endif > > > The only drawback is that it won't work if you select your DOCG3 driver and, at > the same time, other MTD drivers that also use fixed, but different > parameters. Oh, that seems nice. As I'm working with a smartphone, there is only one mtd inside, so the speed-up will be nice. > > (...) >> @@ -1660,6 +1717,7 @@ static int __exit docg3_release(struct platform_device *pdev) >> doc_release_device(docg3_floors[floor]); >> >> kfree(docg3_floors); >> + kfree(docg3_bch); > > This should be 'free_bch(docg3_bch)'. Right. > > Otherwise it looks OK to me; did you test BCH correction by simulating > corruptions (of 1-4 bits, and also 5 bits to trigger failures) in nand data ? I did test the algorithm, yes. To be more precise, I tested your last BCH encoding algorithm (emulate_docg4_hw) which gives exactly the same ECC. I then flipped 2 bits (buf[0] ^= 0x01 and buf[1] ^= 0x02), and got correct errorpos (ie. 0 and 10 IIRC). The thing I didn't check is the decode_bch() call with the hardware calculated ECC. I tried on my PC: decode_bch(bch, data, 520, ecc, NULL, NULL, errorpos); => this *does* work while in the kernel I did: decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, NULL, ecc, NULL, errorpos); => this might work... What I'm a bit afraid of is my poor understanding of the hardware ECC engine. I know that the write part is correct (ie. ECC calculation), but I'm a bit confused by the read part. What wories me is that the hardware ECC got back while reading (ie. what I called calc_ecc) is always 00:00:00:00:00:00:00 when I read data (because I don't have bitflips on my flash). This looks to me more a "syndrom" than a "calc_ecc". To be sure, I could write a page of 512 bytes + 16 bytes, where the BCH would be forced (and incorrect), to check what the hardware generator gives me back. I'd like you to help me, ie: - tell me what to write to the first 512 bytes (only 0, all 0 but one byte to 1, other ...) - I think I'll write 8 bytes to 0x01 for the first 8 OOB bytes (Hamming false but I won't care) - tell me what to write to the 7 BCH ECC That way, I could provide you the result and you could tell me if doc_ecc_bch_fix_data() is correct or not. Would you agree to spend some time on that ? Cheers. -- Robert
On Sat, Oct 29, 2011 at 05:37:35PM +0100, Robert Jarzmik wrote: > >> +static struct bch_control *docg3_bch; > > > > Why not putting this into your struct docg3, instead of adding a global var ? > Because I have multiple floors (ie. 4 floors for example), which are split into > 4 different devices. If I put that in docg3 structures (ie. the 4 allocated > structures, each for one floor), I'd either have to : > - allocate 4 different bch "engines" > - or count docg3 releases and release the bch at the last kfree(docg3), which > makes me have another global variable. OK, got it; using a struct to hold all your common vars (docg3_floors, docg3_bch, ...) and hook that to your platform data instead of docg3_floors would still be a bit cleaner I think, but no big deal. > What I'm a bit afraid of is my poor understanding of the hardware ECC engine. I > know that the write part is correct (ie. ECC calculation), but I'm a bit > confused by the read part. > > What wories me is that the hardware ECC got back while reading (ie. what I > called calc_ecc) is always 00:00:00:00:00:00:00 when I read data (because I > don't have bitflips on my flash). This looks to me more a "syndrom" than a > "calc_ecc". OK, I'll try to clarify that. The hardware ECC engine divides a huge polynomial (520*8 = 4160 bits) by a generator polynomial and computes a 56-bit remainder. So this remainder (let's call it R) depends only on 520 input data bytes. - during a write operation: input data is what you write to the controller, you get R from the ecc engine and this is what you write to oob[8..14]. - during a read operation: the ecc engine computes R on 520 input bytes read from flash (this is calc_ecc), and also reads oob[8..14] (this is recv_ecc, previously programmed during the write operation). Then the ecc engine computes calc_ecc^recv_ecc, and this is what you get from the ecc registers. And as long as there is no bitflip, its all 00s (because calc_ecc=recv_ecc). > To be sure, I could write a page of 512 bytes + 16 bytes, where the BCH would be > forced (and incorrect), to check what the hardware generator gives me back. I'd > like you to help me, ie: > - tell me what to write to the first 512 bytes (only 0, all 0 but one byte to > 1, other ...) > - I think I'll write 8 bytes to 0x01 for the first 8 OOB bytes (Hamming false > but I won't care) > - tell me what to write to the 7 BCH ECC OK, this is really simple: 1. Prepare a buffer of 520 bytes of data, containing pseudo-random bytes or any pattern you like. Let's call this buffer 'ref_buf'. 2. Program 'ref_buf' to a nand page; you will write ecc bytes to oob during that operation; let's call those ecc bytes 'ref_ecc'. 3. Now, you are ready to perform corruption tests: 3.1 Make a copy of 'ref_buf' in which you flip 1, 2, 3 or 4 bits selected at random. 3.2 Program this corrupt buffer, _but_ write 'ref_ecc' to oob instead of hw generated ecc bytes. 3.3 Read page back: you should get exactly 'ref_buf', and the errorpos[] array of corrected bits should match your flip bits. After step 3.2, your flash is exactly in the same state as if it had produced the bitflips itself. Repeat steps 3.1 to 3.3 on a large enough set of random vectors to convince yourself that your code works (be careful not to wear out your device, though :-). You should also try a few 5-bit corruptions and see failures, just to verify that your corruptions have some effect. In theory, testing the BCH algorithm like you did should be enough, but real hardware tests are helpful to verify that the entire system behaves as expected. Hope that helps, BR, -- Ivan
Hi Robert, A few comments (limited to the bch decode for now)... On 10/28/2011 10:51 AM, Robert Jarzmik wrote: > > /** > + * doc_correct_data - Fix if need be read data from flash > + * @docg3: the device > + * @buf: the buffer of read data (512 + 7 + 1 bytes) > + * @calc_ecc: the hardware calculated ECC To avoid confusion with the terminology in Ivan's BCH algorithm, I wouldn't call it calc_ecc. It's actually recv_ecc ^ calc_ecc, where recv_ecc is what the hw read from the ecc field of the oob data, and calc_ecc is what the hw calculated from the page data. Maybe just hwecc or similiar. > + * > + * Checks if the received data matches the ECC, and if an error is detected, > + * tries to fix the bit flips (at most 4) in the buffer buf. As the docg3 > + * understands the (data, ecc, syndroms) in an inverted order in comparison to > + * the BCH library, the function reverses the order of bits (ie. bit7 and bit0, > + * bit6 and bit 1, ...) for all ECC data. > + * > + * Returns number of fixed bits (0, 1, 2, 3, 4) or -EBADMSG if too many bit > + * errors were detected and cannot be fixed. > + */ > +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc) > +{ > + u8 ecc[DOC_ECC_BCH_T + 1]; > + int errorpos[DOC_ECC_BCH_T + 1], i, numerrs; > + > + for (i = 0; i < DOC_ECC_BCH_SIZE; i++) > + ecc[i] = bitrev8(calc_ecc[i]); > + numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, > + NULL, ecc, NULL, errorpos); This looks right, with the redefined DOC_ECC_BCH_COVERED_BYTES (now 520). Some commentary would be helpful (though maybe I'm too verbose)... /* * The hardware ecc unit produces oob_ecc ^ calc_ecc. The kernel's bch * algorithm is used to decode this. However the hw operates on page * data in a bit order that is the reverse of that of the bch alg, * requiring that the bits be reversed on the result. Thanks to Ivan * Djelic for his analysis. */ I recommend you test this. One way would be to compile the bch algorithm in userspace and use it to generate the ecc for a 520 byte test vector. Reverse the bits of this ecc, then flip a few bits in the test vector and write it to a page in flash, with your driver modified to write the calculated ecc instead of that served up by the hardware. Then when you read the page, the above code should identify and correct the bits you flipped (assuming a genuine flash error did not occur while reading back the page). I have the bch alg modified for userspace, if that would help. Alternatively, you could just fill the flash with a fixed pattern, then read all the pages, waiting for an error to occur so that correct correction (ha) can be verified. > + if (numerrs < 0) > + return numerrs; I recommend you check for the -EINVAL return value and issue a big fat error. Maybe BUG_ON(numerrs == -EINVAL), at least for now. Another explanatory comment here... /* undo last step in BCH alg (modulo mirroring not needed) */ > + > + for (i = 0; i < numerrs; i++) > + errorpos[i] = (errorpos[i] & ~7) | (7 - (errorpos[i] & 7)); > + for (i = 0; i < numerrs; i++) > + change_bit(errorpos[i], buf); > + > + doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs); > + return numerrs; > +} > + > + Thanks, Mike
Mike Dunn <mikedunn@newsguy.com> writes: > Hi Robert, > > A few comments (limited to the bch decode for now)... > To avoid confusion with the terminology in Ivan's BCH algorithm, I wouldn't call > it calc_ecc. It's actually recv_ecc ^ calc_ecc, where recv_ecc is what the hw > read from the ecc field of the oob data, and calc_ecc is what the hw calculated > from the page data. Maybe just hwecc or similiar. Yep, point taken for V2, hwecc sounds good. ...zip... >> + numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, >> + NULL, ecc, NULL, errorpos); > This looks right, with the redefined DOC_ECC_BCH_COVERED_BYTES (now 520). > > Some commentary would be helpful (though maybe I'm too verbose)... > > /* > * The hardware ecc unit produces oob_ecc ^ calc_ecc. The kernel's bch > * algorithm is used to decode this. However the hw operates on page > * data in a bit order that is the reverse of that of the bch alg, > * requiring that the bits be reversed on the result. Thanks to Ivan > * Djelic for his analysis. > */ Right again. I will copy paste that explanation in the code. > I recommend you test this. One way would be to compile the bch algorithm in > userspace and use it to generate the ecc for a 520 byte test vector. Reverse > the bits of this ecc, then flip a few bits in the test vector and write it to a > page in flash, with your driver modified to write the calculated ecc instead of > that served up by the hardware. Then when you read the page, the above code > should identify and correct the bits you flipped (assuming a genuine flash error > did not occur while reading back the page). I have the bch alg modified for > userspace, if that would help. I did test it in userspace. And after Ivan's proposition, I tested it also with nandwrite/nanddump by introducing some random errors. The error correction code works, a few amendments (for V2) will be added for "bitflipped" data writes/reads. But the conclusion is that Ivan and your work is indeed directly applicable in the G3 case, and tested :) >> + if (numerrs < 0) >> + return numerrs; > > I recommend you check for the -EINVAL return value and issue a big fat error. > Maybe BUG_ON(numerrs == -EINVAL), at least for now. Okay. > > Another explanatory comment here... > /* undo last step in BCH alg (modulo mirroring not needed) */ Is that the same as the function comment about bit reversing (the modulo mirroring part) ? If so, the function comment might not be clear enough. If not, could you explain a bit further please ? Thanks for the review. Cheers. -- Robert
On 10/31/2011 09:39 AM, Robert Jarzmik wrote: > Mike Dunn <mikedunn@newsguy.com> writes: > >> Another explanatory comment here... >> /* undo last step in BCH alg (modulo mirroring not needed) */ > Is that the same as the function comment about bit reversing (the modulo > mirroring part) ? If so, the function comment might not be clear enough. If not, > could you explain a bit further please ? > > No, the "modulo mirroring" is not the same as the bit reversal. See Ivan's explanation here: http://lists.infradead.org/pipermail/linux-mtd/2011-October/038060.html Obviously the comment doesn't give much technical detail, but it points anyone puzzled by the step in the right direction. I don't like cryptic steps that give no explanation whatsoever. Just MHO. Thanks, Mike
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig index 6d91a1f..ae138c3 100644 --- a/drivers/mtd/devices/Kconfig +++ b/drivers/mtd/devices/Kconfig @@ -251,6 +251,7 @@ config MTD_DOC2001PLUS config MTD_DOCG3 tristate "M-Systems Disk-On-Chip G3" + select BCH ---help--- This provides an MTD device driver for the M-Systems DiskOnChip G3 devices. diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c index 9983594..3c4f7ed 100644 --- a/drivers/mtd/devices/docg3.c +++ b/drivers/mtd/devices/docg3.c @@ -29,6 +29,9 @@ #include <linux/delay.h> #include <linux/mtd/mtd.h> #include <linux/mtd/partitions.h> +#include <linux/bitmap.h> +#include <linux/bitrev.h> +#include <linux/bch.h> #include <linux/debugfs.h> #include <linux/seq_file.h> @@ -42,7 +45,6 @@ * As no specification is available from M-Systems/Sandisk, this drivers lacks * several functions available on the chip, as : * - IPL write - * - ECC fixing (lack of BCH algorith understanding) * - powerdown / powerup * * The bus data width (8bits versus 16bits) is not handled (if_cfg flag), and @@ -51,8 +53,7 @@ * DocG3 relies on 2 ECC algorithms, which are handled in hardware : * - a 1 byte Hamming code stored in the OOB for each page * - a 7 bytes BCH code stored in the OOB for each page - * The BCH part is only used for check purpose, no correction is available as - * some information is missing. What is known is that : + * The BCH ECC is : * - BCH is in GF(2^14) * - BCH is over data of 520 bytes (512 page + 7 page_info bytes * + 1 hamming byte) @@ -73,6 +74,11 @@ static struct nand_ecclayout docg3_oobinfo = { .oobfree = {{0, 7}, {15, 1} } }; +/** + * struct docg3_bch - BCH engine + */ +static struct bch_control *docg3_bch; + static inline u8 doc_readb(struct docg3 *docg3, u16 reg) { u8 val = readb(docg3->base + reg); @@ -576,6 +582,43 @@ static void doc_hamming_ecc_init(struct docg3 *docg3, int nb_bytes) } /** + * doc_correct_data - Fix if need be read data from flash + * @docg3: the device + * @buf: the buffer of read data (512 + 7 + 1 bytes) + * @calc_ecc: the hardware calculated ECC + * + * Checks if the received data matches the ECC, and if an error is detected, + * tries to fix the bit flips (at most 4) in the buffer buf. As the docg3 + * understands the (data, ecc, syndroms) in an inverted order in comparison to + * the BCH library, the function reverses the order of bits (ie. bit7 and bit0, + * bit6 and bit 1, ...) for all ECC data. + * + * Returns number of fixed bits (0, 1, 2, 3, 4) or -EBADMSG if too many bit + * errors were detected and cannot be fixed. + */ +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *calc_ecc) +{ + u8 ecc[DOC_ECC_BCH_T + 1]; + int errorpos[DOC_ECC_BCH_T + 1], i, numerrs; + + for (i = 0; i < DOC_ECC_BCH_SIZE; i++) + ecc[i] = bitrev8(calc_ecc[i]); + numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES, + NULL, ecc, NULL, errorpos); + if (numerrs < 0) + return numerrs; + + for (i = 0; i < numerrs; i++) + errorpos[i] = (errorpos[i] & ~7) | (7 - (errorpos[i] & 7)); + for (i = 0; i < numerrs; i++) + change_bit(errorpos[i], buf); + + doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs); + return numerrs; +} + + +/** * doc_read_page_prepare - Prepares reading data from a flash page * @docg3: the device * @block0: the first plane block index on flash memory @@ -756,7 +799,7 @@ static int doc_read(struct mtd_info *mtd, loff_t from, size_t len, { struct docg3 *docg3 = mtd->priv; int block0, block1, page, readlen, ret, ofs = 0; - u8 syn[DOC_ECC_BCH_SIZE], eccconf1; + u8 calc_ecc[DOC_ECC_BCH_SIZE], eccconf1; u8 oob[DOC_LAYOUT_OOB_SIZE]; ret = -EINVAL; @@ -777,7 +820,7 @@ static int doc_read(struct mtd_info *mtd, loff_t from, size_t len, ret = doc_read_page_prepare(docg3, block0, block1, page, ofs); if (ret < 0) goto err; - ret = doc_read_page_ecc_init(docg3, DOC_ECC_BCH_COVERED_BYTES); + ret = doc_read_page_ecc_init(docg3, DOC_ECC_BCH_TOTAL_BYTES); if (ret < 0) goto err_in_read; ret = doc_read_page_getbytes(docg3, readlen, buf, 1); @@ -788,24 +831,11 @@ static int doc_read(struct mtd_info *mtd, loff_t from, size_t len, if (ret < DOC_LAYOUT_OOB_SIZE) goto err_in_read; - *retlen += readlen; - buf += readlen; - len -= readlen; - - ofs ^= DOC_LAYOUT_PAGE_OOB_SIZE; - if (ofs == 0) - page += 2; - if (page > DOC_ADDR_PAGE_MASK) { - page = 0; - block0 += 2; - block1 += 2; - } - /* * There should be a BCH bitstream fixing algorithm here ... * By now, a page read failure is triggered by BCH error */ - doc_get_hw_bch_syndroms(docg3, syn); + doc_get_hw_bch_syndroms(docg3, calc_ecc); eccconf1 = doc_register_readb(docg3, DOC_ECCCONF1); doc_dbg("OOB - INFO: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n", @@ -817,20 +847,41 @@ static int doc_read(struct mtd_info *mtd, loff_t from, size_t len, oob[13], oob[14]); doc_dbg("OOB - UNUSED: %02x\n", oob[15]); doc_dbg("ECC checks: ECCConf1=%x\n", eccconf1); - doc_dbg("ECC BCH syndrom: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n", - syn[0], syn[1], syn[2], syn[3], syn[4], syn[5], syn[6]); + doc_dbg("ECC CALC_ECC: %02x:%02x:%02x:%02x:%02x:%02x:%02x\n", + calc_ecc[0], calc_ecc[1], calc_ecc[2], calc_ecc[3], + calc_ecc[4], calc_ecc[5], calc_ecc[6]); ret = -EBADMSG; - if (block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) { - if (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR) - goto err_in_read; - if (is_prot_seq_error(docg3)) - goto err_in_read; + if (is_prot_seq_error(docg3)) + goto err_in_read; + ret = 0; + if ((block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) && + (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR)) { + ret = doc_ecc_bch_fix_data(docg3, buf, calc_ecc); + if (ret < 0) + mtd->ecc_stats.failed++; + if (ret > 0) { + mtd->ecc_stats.corrected += ret; + ret = 0; + } } + doc_read_page_finish(docg3); + *retlen += readlen; + buf += readlen; + len -= readlen; + + ofs ^= DOC_LAYOUT_PAGE_OOB_SIZE; + if (ofs == 0) + page += 2; + if (page > DOC_ADDR_PAGE_MASK) { + page = 0; + block0 += 2; + block1 += 2; + } } - return 0; + return ret; err_in_read: doc_read_page_finish(docg3); err: @@ -1162,7 +1213,7 @@ static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf, if (ret) goto err; - doc_write_page_ecc_init(docg3, DOC_ECC_BCH_COVERED_BYTES); + doc_write_page_ecc_init(docg3, DOC_ECC_BCH_TOTAL_BYTES); doc_delay(docg3, 2); doc_write_page_putbytes(docg3, DOC_LAYOUT_PAGE_SIZE, buf); @@ -1602,7 +1653,11 @@ static int __init docg3_probe(struct platform_device *pdev) docg3_floors = kzalloc(sizeof(*docg3_floors) * DOC_MAX_NBFLOORS, GFP_KERNEL); if (!docg3_floors) - goto nomem; + goto nomem1; + docg3_bch = init_bch(DOC_ECC_BCH_M, DOC_ECC_BCH_T, + DOC_ECC_BCH_PRIMPOLY); + if (!docg3_bch) + goto nomem2; ret = 0; for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) { @@ -1635,7 +1690,9 @@ err_probe: for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++) if (docg3_floors[floor]) doc_release_device(docg3_floors[floor]); -nomem: +nomem2: + kfree(docg3_floors); +nomem1: iounmap(base); noress: return ret; @@ -1660,6 +1717,7 @@ static int __exit docg3_release(struct platform_device *pdev) doc_release_device(docg3_floors[floor]); kfree(docg3_floors); + kfree(docg3_bch); iounmap(base); return 0; } diff --git a/drivers/mtd/devices/docg3.h b/drivers/mtd/devices/docg3.h index 397e461..33db727 100644 --- a/drivers/mtd/devices/docg3.h +++ b/drivers/mtd/devices/docg3.h @@ -51,10 +51,19 @@ #define DOC_LAYOUT_WEAR_OFFSET (DOC_LAYOUT_PAGE_OOB_SIZE * 2) #define DOC_LAYOUT_BLOCK_SIZE \ (DOC_LAYOUT_PAGES_PER_BLOCK * DOC_LAYOUT_PAGE_SIZE) + +/* + * ECC related constants + */ +#define DOC_ECC_BCH_M 14 +#define DOC_ECC_BCH_T 4 +#define DOC_ECC_BCH_PRIMPOLY 0x4443 #define DOC_ECC_BCH_SIZE 7 #define DOC_ECC_BCH_COVERED_BYTES \ (DOC_LAYOUT_PAGE_SIZE + DOC_LAYOUT_OOB_PAGEINFO_SZ + \ - DOC_LAYOUT_OOB_HAMMING_SZ + DOC_LAYOUT_OOB_BCH_SZ) + DOC_LAYOUT_OOB_HAMMING_SZ) +#define DOC_ECC_BCH_TOTAL_BYTES \ + (DOC_ECC_BCH_COVERED_BYTES + DOC_LAYOUT_OOB_BCH_SZ) /* * Blocks distribution
Credit for discovering the BCH algorith parameters, and bit reversing algorithm is to be give to Mike Dunn and Ivan Djelic. The BCH correction code relied upon the BCH library, where all data and ECC is bit-reversed. The BCH library works correctly when each input byte is bit-reversed, and accordingly ECC output is also bit-reversed. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/mtd/devices/Kconfig | 1 + drivers/mtd/devices/docg3.c | 118 ++++++++++++++++++++++++++++++++----------- drivers/mtd/devices/docg3.h | 11 ++++- 3 files changed, 99 insertions(+), 31 deletions(-)