[PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[Sascha Hauer]

BCH ECC correction works in chunks of 512 bytes, so a 2k page size nand
is divided into 4 chunks. Hardware requires that each chunk has a full
number of bytes, so when we need 9 bits per chunk we must round up to
two bytes. The current code misses that and calculates a ECC strength
of 18 for a 2048+128 byte page size NAND. ECC strength of 18 requires
30 bytes per chunk, so a total of 4 * (512 + 30) + 10 = 2178 bytes when
the device only has a page size of 2176 bytes.

Fix this by first calculating the number of bytes per chunk we have
available for ECC which also makes it easier to follow the calculation.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 6e46156..95fb3dc 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -119,32 +119,20 @@ static irqreturn_t bch_irq(int irq, void *cookie)
 	return IRQ_HANDLED;
 }
 
-/*
- *  Calculate the ECC strength by hand:
- *	E : The ECC strength.
- *	G : the length of Galois Field.
- *	N : The chunk count of per page.
- *	O : the oobsize of the NAND chip.
- *	M : the metasize of per page.
- *
- *	The formula is :
- *		E * G * N
- *	      ------------ <= (O - M)
- *                  8
- *
- *      So, we get E by:
- *                    (O - M) * 8
- *              E <= -------------
- *                       G * N
- */
 static inline int get_ecc_strength(struct gpmi_nand_data *this)
 {
 	struct bch_geometry *geo = &this->bch_geometry;
 	struct mtd_info	*mtd = nand_to_mtd(&this->nand);
 	int ecc_strength;
+	int n;
+
+	/* number of ecc bytes we have per chunk */
+	n = (mtd->oobsize - geo->metadata_size) / geo->ecc_chunk_count;
+
+	/* in bits */
+	n <<= 3;
 
-	ecc_strength = ((mtd->oobsize - geo->metadata_size) * 8)
-			/ (geo->gf_len * geo->ecc_chunk_count);
+	ecc_strength = n / geo->gf_len;
 
 	/* We need the minor even number. */
 	return round_down(ecc_strength, 2);
-- 
2.8.1

Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[Boris Brezillon]

On Tue, 21 Jun 2016 16:35:49 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> BCH ECC correction works in chunks of 512 bytes, so a 2k page size nand
> is divided into 4 chunks. Hardware requires that each chunk has a full
> number of bytes, so when we need 9 bits per chunk we must round up to
> two bytes. The current code misses that and calculates a ECC strength
> of 18 for a 2048+128 byte page size NAND. ECC strength of 18 requires
> 30 bytes per chunk, so a total of 4 * (512 + 30) + 10 = 2178 bytes when
> the device only has a page size of 2176 bytes.

AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
to deal with BTW), and is only requiring a byte alignment on the total
number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').

Han, can you confirm that?

> 
> Fix this by first calculating the number of bytes per chunk we have
> available for ECC which also makes it easier to follow the calculation.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 6e46156..95fb3dc 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -119,32 +119,20 @@ static irqreturn_t bch_irq(int irq, void *cookie)
>  	return IRQ_HANDLED;
>  }
>  
> -/*
> - *  Calculate the ECC strength by hand:
> - *	E : The ECC strength.
> - *	G : the length of Galois Field.
> - *	N : The chunk count of per page.
> - *	O : the oobsize of the NAND chip.
> - *	M : the metasize of per page.
> - *
> - *	The formula is :
> - *		E * G * N
> - *	      ------------ <= (O - M)
> - *                  8
> - *
> - *      So, we get E by:
> - *                    (O - M) * 8
> - *              E <= -------------
> - *                       G * N
> - */
>  static inline int get_ecc_strength(struct gpmi_nand_data *this)
>  {
>  	struct bch_geometry *geo = &this->bch_geometry;
>  	struct mtd_info	*mtd = nand_to_mtd(&this->nand);
>  	int ecc_strength;
> +	int n;
> +
> +	/* number of ecc bytes we have per chunk */
> +	n = (mtd->oobsize - geo->metadata_size) / geo->ecc_chunk_count;
> +
> +	/* in bits */
> +	n <<= 3;
>  
> -	ecc_strength = ((mtd->oobsize - geo->metadata_size) * 8)
> -			/ (geo->gf_len * geo->ecc_chunk_count);
> +	ecc_strength = n / geo->gf_len;
>  
>  	/* We need the minor even number. */
>  	return round_down(ecc_strength, 2);

Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[Han Xu]

On Tue, Jun 21, 2016 at 9:46 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 21 Jun 2016 16:35:49 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
>> BCH ECC correction works in chunks of 512 bytes, so a 2k page size nand
>> is divided into 4 chunks. Hardware requires that each chunk has a full
>> number of bytes, so when we need 9 bits per chunk we must round up to
>> two bytes. The current code misses that and calculates a ECC strength
>> of 18 for a 2048+128 byte page size NAND. ECC strength of 18 requires
>> 30 bytes per chunk, so a total of 4 * (512 + 30) + 10 = 2178 bytes when
>> the device only has a page size of 2176 bytes.
>
> AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
> to deal with BTW), and is only requiring a byte alignment on the total
> number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
> fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').
>
> Han, can you confirm that?

Correct, BCH module works at bit level, 18bit ECC won't exceed the oob size.

>
>>
>> Fix this by first calculating the number of bytes per chunk we have
>> available for ECC which also makes it easier to follow the calculation.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 28 ++++++++--------------------
>>  1 file changed, 8 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> index 6e46156..95fb3dc 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> @@ -119,32 +119,20 @@ static irqreturn_t bch_irq(int irq, void *cookie)
>>       return IRQ_HANDLED;
>>  }
>>
>> -/*
>> - *  Calculate the ECC strength by hand:
>> - *   E : The ECC strength.
>> - *   G : the length of Galois Field.
>> - *   N : The chunk count of per page.
>> - *   O : the oobsize of the NAND chip.
>> - *   M : the metasize of per page.
>> - *
>> - *   The formula is :
>> - *           E * G * N
>> - *         ------------ <= (O - M)
>> - *                  8
>> - *
>> - *      So, we get E by:
>> - *                    (O - M) * 8
>> - *              E <= -------------
>> - *                       G * N
>> - */
>>  static inline int get_ecc_strength(struct gpmi_nand_data *this)
>>  {
>>       struct bch_geometry *geo = &this->bch_geometry;
>>       struct mtd_info *mtd = nand_to_mtd(&this->nand);
>>       int ecc_strength;
>> +     int n;
>> +
>> +     /* number of ecc bytes we have per chunk */
>> +     n = (mtd->oobsize - geo->metadata_size) / geo->ecc_chunk_count;
>> +
>> +     /* in bits */
>> +     n <<= 3;
>>
>> -     ecc_strength = ((mtd->oobsize - geo->metadata_size) * 8)
>> -                     / (geo->gf_len * geo->ecc_chunk_count);
>> +     ecc_strength = n / geo->gf_len;
>>
>>       /* We need the minor even number. */
>>       return round_down(ecc_strength, 2);
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Sincerely,

Han XU

Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[Sascha Hauer]

On Tue, Jun 21, 2016 at 10:52:09AM -0500, Han Xu wrote:
> On Tue, Jun 21, 2016 at 9:46 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Tue, 21 Jun 2016 16:35:49 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> >> BCH ECC correction works in chunks of 512 bytes, so a 2k page size nand
> >> is divided into 4 chunks. Hardware requires that each chunk has a full
> >> number of bytes, so when we need 9 bits per chunk we must round up to
> >> two bytes. The current code misses that and calculates a ECC strength
> >> of 18 for a 2048+128 byte page size NAND. ECC strength of 18 requires
> >> 30 bytes per chunk, so a total of 4 * (512 + 30) + 10 = 2178 bytes when
> >> the device only has a page size of 2176 bytes.
> >
> > AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
> > to deal with BTW), and is only requiring a byte alignment on the total
> > number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
> > fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').
> >
> > Han, can you confirm that?
> 
> Correct, BCH module works at bit level, 18bit ECC won't exceed the oob size.

I see, only subpage reads fail here in my case. The driver does only subpage
reads when the ECC size is byte aligned. This completely disables the subpage
read feature on certain Nand types. Is that really what we want?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation

[Boris Brezillon]

On Wed, 22 Jun 2016 08:33:41 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Tue, Jun 21, 2016 at 10:52:09AM -0500, Han Xu wrote:
> > On Tue, Jun 21, 2016 at 9:46 AM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:  
> > > On Tue, 21 Jun 2016 16:35:49 +0200
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >  
> > >> BCH ECC correction works in chunks of 512 bytes, so a 2k page size nand
> > >> is divided into 4 chunks. Hardware requires that each chunk has a full
> > >> number of bytes, so when we need 9 bits per chunk we must round up to
> > >> two bytes. The current code misses that and calculates a ECC strength
> > >> of 18 for a 2048+128 byte page size NAND. ECC strength of 18 requires
> > >> 30 bytes per chunk, so a total of 4 * (512 + 30) + 10 = 2178 bytes when
> > >> the device only has a page size of 2176 bytes.  
> > >
> > > AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
> > > to deal with BTW), and is only requiring a byte alignment on the total
> > > number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
> > > fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').
> > >
> > > Han, can you confirm that?  
> > 
> > Correct, BCH module works at bit level, 18bit ECC won't exceed the oob size.  
> 
> I see, only subpage reads fail here in my case. The driver does only subpage
> reads when the ECC size is byte aligned. This completely disables the subpage
> read feature on certain Nand types. Is that really what we want?

I'd definitely prefer to see only byte aligned settings, but the driver
already supports non-byte aligned configs, so this is not something we
can simply remove.

This being said, you have several solutions to address that:
1/ Patch the gpmi driver to take the information extracted from the
   nand-ecc-strength/step-size DT properties into account, and define
   these properties in your DT.
2/ Define 'fsl,use-minimum-ecc' in your DT and see what's happening. If
   you're lucky the ECC requirements will fall into a byte aligned case.
3/ Take advantage of the recent introduction of the generic
   'nand-ecc-maximize' property [1] and implement a different ECC
   maximization logic where you make sure your config generates
   byte-aligned ECC words

[1]https://lkml.org/lkml/2016/6/8/744