[v2,13/16] mtd/docg3: add ECC correction code

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>

---
Since V1: integrate reviews from Ivan Djelic and Mike Dunn.
---
 drivers/mtd/devices/Kconfig |    9 +++
 drivers/mtd/devices/docg3.c |  116 ++++++++++++++++++++++++++++++++++--------
 drivers/mtd/devices/docg3.h |   11 ++++-
 3 files changed, 113 insertions(+), 23 deletions(-)

On 11/10/2011 12:05 AM, Robert Jarzmik wrote:
>  
> +if MTD_DOCG3
> +config BCH_CONST_M
> +	default 14
> +config BCH_CONST_T
> +	default 4
> +endif


It might be better to let the user set this in the kernel config.  Doing it here
precludes the use of the algorithm by any other module that needs to use it with
different parameters.


>  
>  /**
> + * doc_correct_data - Fix if need be read data from flash
> + * @docg3: the device
> + * @buf: the buffer of read data (512 + 7 + 1 bytes)
> + * @hwecc: the hardware calculated ECC.
> + *         It's in fact recv_ecc ^ calc_ecc, where recv_ecc was read from OOB
> + *         area data, and calc_ecc the ECC calculated by the hardware generator.
> + *
> + * 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.
> + *
> + * 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.
> + *
> + * 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 *hwecc)


Nit: function name in comment is inconsistent with its actual name.


> +{
> +	u8 ecc[DOC_ECC_BCH_SIZE];
> +	int errorpos[DOC_ECC_BCH_T], i, numerrs;
> +
> +	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
> +		ecc[i] = bitrev8(hwecc[i]);
> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
> +			     NULL, ecc, NULL, errorpos);
> +	BUG_ON(numerrs == -EINVAL);
> +	if (numerrs < 0)
> +		goto out;
> +
> +	for (i = 0; i < numerrs; i++)
> +		errorpos[i] = (errorpos[i] & ~7) | (7 - (errorpos[i] & 7));


There's that unexplained cryptic step again :-)


> +	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);
> +out:
> +	doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs);
> +	return numerrs;
> +}


Where do you check for reads of a blank page?  When an erased page is read,
uncorrectible ecc errors will occur (at least).  You can compare the bytes read
from the hw ecc to those generated when a bank page is read to determine if the
page is indeed blank.  At Ivan's suggestion, I went a step further and used oob
byte 15 as a "programmed flag", which is used to determine if a page has been
written or not.  This is then used as a secondary check for a blank page read,
which will avoid the situation where a blank page is read but not detected
because a genuine bit flip occurred when reading a blank page.  In that case the
hw ecc will not generate the usual blank page values.  (You can have a look at
correct_data() in the latest G4 driver patch to see what I'm talking about.)


> +
> +
> +/**
>   * doc_read_page_prepare - Prepares reading data from a flash page
>   * @docg3: the device
>   * @block0: the first plane block index on flash memory
> @@ -762,7 +816,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
>  	u8 *oobbuf = ops->oobbuf;
>  	u8 *buf = ops->datbuf;
>  	size_t len, ooblen, nbdata, nboob;
> -	u8 calc_ecc[DOC_ECC_BCH_SIZE], eccconf1;
> +	u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1;
>  
>  	if (buf)
>  		len = ops->len;
> @@ -797,7 +851,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
>  		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);


Not specifically related to this patch, but... are you sure you want to
initialize the ecc on every read?  I'm sure it's not necessary; you can just
leave it on; maybe turn it off if doing raw reads.  I know this is the case for
both the P3 and G4 when running under PalmOS / TrueFFS library.  I notice that
this function has delays and polls the status register in between calls to
cpu_relax(), so the performance hit is probably not insignificant, especiallu
when done for every 512 byte page.


>  		if (ret < 0)
>  			goto err_in_read;
>  		ret = doc_read_page_getbytes(docg3, nbdata, buf, 1);
> @@ -811,7 +865,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
>  		doc_read_page_getbytes(docg3, DOC_LAYOUT_OOB_SIZE - nboob,
>  				       NULL, 0);
>  
> -		doc_get_hw_bch_syndroms(docg3, calc_ecc);
> +		doc_get_hw_bch_syndroms(docg3, hwecc);


Another nit (also not specifically related to this patch): bad name for this
function.  The ecc being read is not the BCH syndrome, as we now know.  This is
a pet peeve of mine; M-sys abused that word by misapplying it to the byts read
from the ecc hw, which confused the hell out of me as I tried to understand what
the hw was generating.


Otherwise, looks correct.  And if it's passing nandtest we know it works!

Mike

Mike Dunn <mikedunn@newsguy.com> writes:

> On 11/10/2011 12:05 AM, Robert Jarzmik wrote:
>>  
>> +if MTD_DOCG3
>> +config BCH_CONST_M
>> +	default 14
>> +config BCH_CONST_T
>> +	default 4
>> +endif
>
>
> It might be better to let the user set this in the kernel config.  Doing it here
> precludes the use of the algorithm by any other module that needs to use it with
> different parameters.
You're right.
I'll shift that to mioa701 board code, where I'm sure no other BCH is necessary.

>> +static int doc_ecc_bch_fix_data(struct docg3 *docg3, void *buf, u8 *hwecc)
> Nit: function name in comment is inconsistent with its actual name.
Good catch. Will fix.
>> +	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);
>> +out:
>> +	doc_dbg("doc_ecc_bch_fix_data: flipped %d bits\n", numerrs);
>> +	return numerrs;
>> +}
>
>
> Where do you check for reads of a blank page?
On stack frame above. Look at doc_read_oob():
		if ((block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) &&
		    (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR) &&
		    (eccconf1 & DOC_ECCCONF1_PAGE_IS_WRITTEN) &&
                                \---> this is the key
		    (ops->mode != MTD_OPS_RAW) &&
		    (nbdata == DOC_LAYOUT_PAGE_SIZE)) {
			ret = doc_ecc_bch_fix_data(docg3, buf, hwecc);

Here you see that I'll make the error correction only if the page was written
before. If it's blank, I continue reading without attempting ECC correction.

> Not specifically related to this patch, but... are you sure you want to
> initialize the ecc on every read?  I'm sure it's not necessary; you can just
> leave it on; maybe turn it off if doing raw reads.  I know this is the case for
> both the P3 and G4 when running under PalmOS / TrueFFS library.  I notice that
> this function has delays and polls the status register in between calls to
> cpu_relax(), so the performance hit is probably not insignificant, especiallu
> when done for every 512 byte page.
Well, that's some info.
And yes, it adds some latency.
Now for the necessity, I'm not fully convinced. I know that the ECC register is
set up differently for reads and writes (that's the
DOC_ECCCONF0_READ_MODE). When doc_read_oob() is called, I don't know if the
previous action was a read or a write ...

What I could do to improve performance would be to store in the docg3 private
data if last action was a read or a write, and perform the doc_*_page_ecc_init()
only if action changes. What do you think ?

> Another nit (also not specifically related to this patch): bad name for this
> function.  The ecc being read is not the BCH syndrome, as we now know.  This is
> a pet peeve of mine; M-sys abused that word by misapplying it to the byts read
> from the ecc hw, which confused the hell out of me as I tried to understand what
> the hw was generating.
Yes, I'll change the name to hw_ecc or something like that.

Thanks for the review.

On 11/13/2011 02:35 AM, Robert Jarzmik wrote:
> Mike Dunn <mikedunn@newsguy.com> writes:
>
>>
>> Where do you check for reads of a blank page?
> On stack frame above. Look at doc_read_oob():
> 		if ((block0 >= DOC_LAYOUT_BLOCK_FIRST_DATA) &&
> 		    (eccconf1 & DOC_ECCCONF1_BCH_SYNDROM_ERR) &&
> 		    (eccconf1 & DOC_ECCCONF1_PAGE_IS_WRITTEN) &&
>                                 \---> this is the key
> 		    (ops->mode != MTD_OPS_RAW) &&
> 		    (nbdata == DOC_LAYOUT_PAGE_SIZE)) {
> 			ret = doc_ecc_bch_fix_data(docg3, buf, hwecc);
>
> Here you see that I'll make the error correction only if the page was written
> before. If it's blank, I continue reading without attempting ECC correction.


G4 probably has this capability too.  If so, I'll be improving my blank page
detection.  I *really* have to go through your driver in its entirety to see
what other insights you have.  Currently getting pulled in multiple directions.


>> Not specifically related to this patch, but... are you sure you want to
>> initialize the ecc on every read?  I'm sure it's not necessary; you can just
>> leave it on; maybe turn it off if doing raw reads.  I know this is the case for
>> both the P3 and G4 when running under PalmOS / TrueFFS library.  I notice that
>> this function has delays and polls the status register in between calls to
>> cpu_relax(), so the performance hit is probably not insignificant, especiallu
>> when done for every 512 byte page.
> Well, that's some info.
> And yes, it adds some latency.
> Now for the necessity, I'm not fully convinced. I know that the ECC register is
> set up differently for reads and writes (that's the
> DOC_ECCCONF0_READ_MODE). When doc_read_oob() is called, I don't know if the
> previous action was a read or a write ...
>
> What I could do to improve performance would be to store in the docg3 private
> data if last action was a read or a write, and perform the doc_*_page_ecc_init()
> only if action changes. What do you think ?
>


Never mind.  Sorry.  I was thinking DOC_ECCCONF1, which I write to during
initialization to (I believe) enable ecc.  Anyway, you have a better handle on
the internals of the chip than I do.  I'm still looking forward to comparing
your code to my old P3 test code (which mostly just parrots what I observed
during operation).  Until then I'll keep my opinions on register sequences, etc,
to myself!'

Good job!

Mike