lkml.org 
[lkml]   [2011]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 13/16] mtd/docg3: add ECC correction code
Date
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.

--
Robert


\
 
 \ /
  Last update: 2011-11-13 11:37    [W:0.066 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site