| Message ID | 1466519749-7729-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | Rejected |
| Headers | show |
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);
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/
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
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
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);
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(-)