* [PATCH] mtd: nand: gpmi: Fix ecc strength calculation @ 2016-06-21 14:35 Sascha Hauer 2016-06-21 14:46 ` Boris Brezillon 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2016-06-21 14:35 UTC (permalink / raw) To: linux-mtd; +Cc: Han Xu, Boris Brezillon, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation 2016-06-21 14:35 [PATCH] mtd: nand: gpmi: Fix ecc strength calculation Sascha Hauer @ 2016-06-21 14:46 ` Boris Brezillon 2016-06-21 15:52 ` Han Xu 0 siblings, 1 reply; 5+ messages in thread From: Boris Brezillon @ 2016-06-21 14:46 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-mtd, Han Xu 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation 2016-06-21 14:46 ` Boris Brezillon @ 2016-06-21 15:52 ` Han Xu 2016-06-22 6:33 ` Sascha Hauer 0 siblings, 1 reply; 5+ messages in thread From: Han Xu @ 2016-06-21 15:52 UTC (permalink / raw) To: Boris Brezillon; +Cc: Sascha Hauer, Han Xu, linux-mtd@lists.infradead.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation 2016-06-21 15:52 ` Han Xu @ 2016-06-22 6:33 ` Sascha Hauer 2016-06-22 8:34 ` Boris Brezillon 0 siblings, 1 reply; 5+ messages in thread From: Sascha Hauer @ 2016-06-22 6:33 UTC (permalink / raw) To: Han Xu; +Cc: Boris Brezillon, Han Xu, linux-mtd@lists.infradead.org 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 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mtd: nand: gpmi: Fix ecc strength calculation 2016-06-22 6:33 ` Sascha Hauer @ 2016-06-22 8:34 ` Boris Brezillon 0 siblings, 0 replies; 5+ messages in thread From: Boris Brezillon @ 2016-06-22 8:34 UTC (permalink / raw) To: Sascha Hauer; +Cc: Han Xu, Han Xu, linux-mtd@lists.infradead.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-22 8:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-21 14:35 [PATCH] mtd: nand: gpmi: Fix ecc strength calculation Sascha Hauer 2016-06-21 14:46 ` Boris Brezillon 2016-06-21 15:52 ` Han Xu 2016-06-22 6:33 ` Sascha Hauer 2016-06-22 8:34 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox