Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1213284 > unrolled thread

Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC  support Brian Norris <computersforpeace@gmail.com> - 2015-08-25 22:00 +0200
    Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC  support Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-08-25 22:50 +0200
    Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC  support Stefan Agner <stefan@agner.ch> - 2015-08-26 20:00 +0200
      Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC  support Brian Norris <computersforpeace@gmail.com> - 2015-08-26 23:40 +0200
        Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC support Bill Pringlemeir <bpringle@sympatico.ca> - 2015-08-28 23:20 +0200

#1213284 — Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC support

On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> On 2015-08-03 11:27, Stefan Agner wrote:
> <snip>
> > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> > +					 uint8_t *oob, int oob_loaded)
> > +{
> > +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> > +	u8 ecc_status;
> > +	u8 ecc_count;
> > +	int flip;
> > +
> > +	ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);

Why __raw_readb()? That's not normally encourage, and it has issues with
endianness. It looks like maybe this is actulaly a 32-bit register, and
you're having trouble when trying to do bytewise access? I see this
earlier:

/*
 * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
 * and +7 for big-endian SoCs.
 */
#ifdef __LITTLE_ENDIAN
#define ECC_OFFSET      4
#else
#define ECC_OFFSET      7
#endif

So maybe you really just want:

#define ECC_OFFSET	4
...
	ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;

?

> > +	ecc_count = ecc_status & ECC_ERR_COUNT;
> > +
> > +	if (!(ecc_status & ECC_STATUS_MASK))
> > +		return ecc_count;
> > +
> > +	if (!oob_loaded)
> > +		vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> > +
> > +	/*
> > +	 * On an erased page, bit count (including OOB) should be zero or
> > +	 * at least less then half of the ECC strength.
> > +	 */
> > +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

Another side note: why are you using ecc_count as a max threshold? AIUI,
an ECC algorithm doesn't really report useful error count information if
it's above the correction limit. So wouldn't we be looking to count up
to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
comments below.

> > +	flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
> > +				   ecc_count);
> 
> With ECC the controller seems to clear the ECC bytes in SRAM buffer.
> This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
> 60 bytes of OOB for ECC:
> 
> [   22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Hmm, that's not really good. The point is that we need to make sure that
everything that could have been programmed (including the ECC area) was
not actually programmed. But your ECC controller is not, contrary to
MTD's expectations, dumping raw uncorrected data here.

> [   22.209698] vf610_nfc_correct_data, flips 1
> 
> Not sure if this is acceptable, but I now only count the bits in the
> non-ECC area of the OOB.

That's not the intention of my suggestion. You're still missing out on a
class of patterns that might look close to all 0xff but are not
actually.

If the HW ECC really doesn't give you valid data+OOB at this point, then
you might have to re-read with ECC disabled. Of course, that's got a
performance cost...

Or perhaps Boris has a better suggestion? He's been surveying other NAND
drivers that need to do similar things, and he's working on providing
some support code for common design patterns.

> Btw, if the ECC check fails, the controller seems kind of count the
> amount of bitflips. It works for most devices reliable, but we had
> devices for which that number was not accurate, see:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439

I'm a little confused there. Why would you be expecting to get a count
of bitflips, when the ECC engine can't correct all errors? How is it
supposed to know what the "right" data is if the bit errors are beyond
the correction strength?

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1213331

Brian, Stefan,

On Tue, 25 Aug 2015 12:54:11 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> > On 2015-08-03 11:27, Stefan Agner wrote:
> > <snip>
> > > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> > > +					 uint8_t *oob, int oob_loaded)
> > > +{
> > > +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> > > +	u8 ecc_status;
> > > +	u8 ecc_count;
> > > +	int flip;
> > > +
> > > +	ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
> 
> Why __raw_readb()? That's not normally encourage, and it has issues with
> endianness. It looks like maybe this is actulaly a 32-bit register, and
> you're having trouble when trying to do bytewise access? I see this
> earlier:
> 
> /*
>  * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
>  * and +7 for big-endian SoCs.
>  */
> #ifdef __LITTLE_ENDIAN
> #define ECC_OFFSET      4
> #else
> #define ECC_OFFSET      7
> #endif
> 
> So maybe you really just want:
> 
> #define ECC_OFFSET	4
> ...
> 	ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;
> 
> ?
> 
> > > +	ecc_count = ecc_status & ECC_ERR_COUNT;
> > > +
> > > +	if (!(ecc_status & ECC_STATUS_MASK))
> > > +		return ecc_count;
> > > +
> > > +	if (!oob_loaded)
> > > +		vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> > > +
> > > +	/*
> > > +	 * On an erased page, bit count (including OOB) should be zero or
> > > +	 * at least less then half of the ECC strength.
> > > +	 */
> > > +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
> Another side note: why are you using ecc_count as a max threshold? AIUI,
> an ECC algorithm doesn't really report useful error count information if
> it's above the correction limit. So wouldn't we be looking to count up
> to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
> comments below.

The exact threshold value is still something I'm not sure about, though
I'm sure it should be correlated to ecc.strength value (whether it's
directly set to ecc.strength or less than ecc.strength is something
we'll have to figure out).

> 
> > > +	flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
> > > +				   ecc_count);
> > 
> > With ECC the controller seems to clear the ECC bytes in SRAM buffer.
> > This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
> > 60 bytes of OOB for ECC:
> > 
> > [   22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> Hmm, that's not really good. The point is that we need to make sure that
> everything that could have been programmed (including the ECC area) was
> not actually programmed. But your ECC controller is not, contrary to
> MTD's expectations, dumping raw uncorrected data here.

Yep, for this test we really need the ECC bytes generated for the chunk
you're currently testing.
How to retrieve those bytes really depends on your NAND controller, but
such controllers usually provides a way to disable the ECC engine. The
only thing you'll have to do in this case is disable the ECC engine and
read the OOB data (using RNDOUT and read_buf for example).

> 
> > [   22.209698] vf610_nfc_correct_data, flips 1
> > 
> > Not sure if this is acceptable, but I now only count the bits in the
> > non-ECC area of the OOB.
> 
> That's not the intention of my suggestion. You're still missing out on a
> class of patterns that might look close to all 0xff but are not
> actually.

Exactly.

> 
> If the HW ECC really doesn't give you valid data+OOB at this point, then
> you might have to re-read with ECC disabled. Of course, that's got a
> performance cost...

As suggested above, if that's possible, reading the OOB area (or a
portion of the OOB area) with the ECC engine disabled should be enough.

> 
> Or perhaps Boris has a better suggestion? He's been surveying other NAND
> drivers that need to do similar things, and he's working on providing
> some support code for common design patterns.

Yep, the patch series is here in case you want to have a look [1].

Best Regards,

Boris

[1]https://patchwork.ozlabs.org/patch/509970/


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214086

On 2015-08-25 12:54, Brian Norris wrote:
> On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
>> On 2015-08-03 11:27, Stefan Agner wrote:
>> <snip>
>> > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>> > +					 uint8_t *oob, int oob_loaded)
>> > +{
>> > +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> > +	u8 ecc_status;
>> > +	u8 ecc_count;
>> > +	int flip;
>> > +
>> > +	ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
> 
> Why __raw_readb()? That's not normally encourage, and it has issues with
> endianness. It looks like maybe this is actulaly a 32-bit register, and
> you're having trouble when trying to do bytewise access? I see this
> earlier:
> 
> /*
>  * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
>  * and +7 for big-endian SoCs.
>  */
> #ifdef __LITTLE_ENDIAN
> #define ECC_OFFSET      4
> #else
> #define ECC_OFFSET      7
> #endif
> 
> So maybe you really just want:
> 
> #define ECC_OFFSET	4
> ...
> 	ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;
> 
> ?
> 

Agreed, much cleaner.

>> > +	ecc_count = ecc_status & ECC_ERR_COUNT;
>> > +
>> > +	if (!(ecc_status & ECC_STATUS_MASK))
>> > +		return ecc_count;
>> > +
>> > +	if (!oob_loaded)
>> > +		vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
>> > +
>> > +	/*
>> > +	 * On an erased page, bit count (including OOB) should be zero or
>> > +	 * at least less then half of the ECC strength.
>> > +	 */
>> > +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
> Another side note: why are you using ecc_count as a max threshold? AIUI,
> an ECC algorithm doesn't really report useful error count information if
> it's above the correction limit. So wouldn't we be looking to count up
> to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
> comments below.
> 

Initially, that was the only threshold below, hence it made sense. But I
agree, we should count up to the threshold used below...

>> > +	flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
>> > +				   ecc_count);
>>
>> With ECC the controller seems to clear the ECC bytes in SRAM buffer.
>> This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
>> 60 bytes of OOB for ECC:
>>
>> [   22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> Hmm, that's not really good. The point is that we need to make sure that
> everything that could have been programmed (including the ECC area) was
> not actually programmed. But your ECC controller is not, contrary to
> MTD's expectations, dumping raw uncorrected data here.
> 
>> [   22.209698] vf610_nfc_correct_data, flips 1
>>
>> Not sure if this is acceptable, but I now only count the bits in the
>> non-ECC area of the OOB.
> 
> That's not the intention of my suggestion. You're still missing out on a
> class of patterns that might look close to all 0xff but are not
> actually.
> 
> If the HW ECC really doesn't give you valid data+OOB at this point, then
> you might have to re-read with ECC disabled. Of course, that's got a
> performance cost...

Yes I can do that. Not sure yet how it will look like exactly, maybe I
only need to reread the OOB area and (re-)use the main data part since
those arrive uncorrected in the error case.

> 
> Or perhaps Boris has a better suggestion? He's been surveying other NAND
> drivers that need to do similar things, and he's working on providing
> some support code for common design patterns.
> 
>> Btw, if the ECC check fails, the controller seems kind of count the
>> amount of bitflips. It works for most devices reliable, but we had
>> devices for which that number was not accurate, see:
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
> 
> I'm a little confused there. Why would you be expecting to get a count
> of bitflips, when the ECC engine can't correct all errors? How is it
> supposed to know what the "right" data is if the bit errors are beyond
> the correction strength?

When printing the ECC error count on ECC fail when reading an erased
NAND flash, the numbers of bit flips (stuck at zero) seem to widely
correlate with the number returned by the controller. While it seems to
correlate widely, there are exceptions, as discussed in the thread:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424

Maybe this is an artifact of the ECC algorithm we just can't/shouldn't
rely on? I am not sure where this originated, I did not found any
indication in the reference manual about what that value contains in the
error case.

Bill, do you have an idea why we used that value as threshold in early
implementations?

Otherwise I also think we should just drop the use of this value.

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214209

On Wed, Aug 26, 2015 at 10:57:38AM -0700, Stefan Agner wrote:
> On 2015-08-25 12:54, Brian Norris wrote:
> > On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> >> Btw, if the ECC check fails, the controller seems kind of count the
> >> amount of bitflips. It works for most devices reliable, but we had
> >> devices for which that number was not accurate, see:
> >> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
> > 
> > I'm a little confused there. Why would you be expecting to get a count
> > of bitflips, when the ECC engine can't correct all errors? How is it
> > supposed to know what the "right" data is if the bit errors are beyond
> > the correction strength?
> 
> When printing the ECC error count on ECC fail when reading an erased
> NAND flash, the numbers of bit flips (stuck at zero) seem to widely
> correlate with the number returned by the controller. While it seems to
> correlate widely, there are exceptions, as discussed in the thread:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424
> 
> Maybe this is an artifact of the ECC algorithm we just can't/shouldn't
> rely on? I am not sure where this originated, I did not found any
> indication in the reference manual about what that value contains in the
> error case.

Doesn't sound too reliable to me. And I'm not sure even if it was
reliable, that it would provide much value. We have to a lot of
re-counting anyway, so we might as well just be using our own threshold.
Or maybe I'm missing the point.

> Bill, do you have an idea why we used that value as threshold in early
> implementations?
> 
> Otherwise I also think we should just drop the use of this value.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215567 — Re: [PATCH v10 2/5] mtd: nand: vf610_nfc: add hardware BCH-ECC support

On 26 Aug 2015, computersforpeace@gmail.com wrote:

> On Wed, Aug 26, 2015 at 10:57:38AM -0700, Stefan Agner wrote:
>> When printing the ECC error count on ECC fail when reading an erased
>> NAND flash, the numbers of bit flips (stuck at zero) seem to widely
>> correlate with the number returned by the controller. While it seems
>> to correlate widely, there are exceptions, as discussed in the
>> thread: http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424

>> Maybe this is an artifact of the ECC algorithm we just
>> can't/shouldn't rely on? I am not sure where this originated, I did
>> not found any indication in the reference manual about what that
>> value contains in the error case.

> Doesn't sound too reliable to me. And I'm not sure even if it was
> reliable, that it would provide much value. We have to a lot of
> re-counting anyway, so we might as well just be using our own
> threshold.  Or maybe I'm missing the point.

>> Bill, do you have an idea why we used that value as threshold in
>> early implementations?

>> Otherwise I also think we should just drop the use of this value.

Yes, using this value is not especially useful if we re-read with ECC
disabled to count the bit flips for erased pages.   I think this is what
Stefan has done in the 11th patch set.

-- 
Married men live  longer than single men, but married  men are much more
willing to die - Dilworth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web